kadircet created this revision. kadircet added a reviewer: sammccall. Herald added subscribers: cfe-commits, usaxena95, jfb, arphaman, mgrang, jkorous, MaskRay, javed.absar, ilya-biryukov. Herald added a project: clang.
This is achieved by calculating newly added includes and implicitly parsing them as if they were part of the main file. This also gets rid of the need for consistent preamble reads. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D77392 Files: clang-tools-extra/clangd/ClangdServer.cpp clang-tools-extra/clangd/CodeComplete.cpp clang-tools-extra/clangd/Headers.cpp clang-tools-extra/clangd/Headers.h clang-tools-extra/clangd/Preamble.cpp clang-tools-extra/clangd/Preamble.h clang-tools-extra/clangd/TUScheduler.cpp clang-tools-extra/clangd/TUScheduler.h clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp clang-tools-extra/clangd/unittests/HeadersTests.cpp clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp =================================================================== --- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp +++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp @@ -242,63 +242,6 @@ EXPECT_EQ(2, CallbackCount); } -static std::vector<std::string> includes(const PreambleData *Preamble) { - std::vector<std::string> Result; - if (Preamble) - for (const auto &Inclusion : Preamble->Includes.MainFileIncludes) - Result.push_back(Inclusion.Written); - return Result; -} - -TEST_F(TUSchedulerTests, PreambleConsistency) { - std::atomic<int> CallbackCount(0); - { - Notification InconsistentReadDone; // Must live longest. - TUScheduler S(CDB, optsForTest()); - auto Path = testPath("foo.cpp"); - // Schedule two updates (A, B) and two preamble reads (stale, consistent). - // The stale read should see A, and the consistent read should see B. - // (We recognize the preambles by their included files). - auto Inputs = getInputs(Path, "#include <A>"); - Inputs.Version = "A"; - updateWithCallback(S, Path, Inputs, WantDiagnostics::Yes, [&]() { - // This callback runs in between the two preamble updates. - - // This blocks update B, preventing it from winning the race - // against the stale read. - // If the first read was instead consistent, this would deadlock. - InconsistentReadDone.wait(); - // This delays update B, preventing it from winning a race - // against the consistent read. The consistent read sees B - // only because it waits for it. - // If the second read was stale, it would usually see A. - std::this_thread::sleep_for(std::chrono::milliseconds(100)); - }); - Inputs.Contents = "#include <B>"; - Inputs.Version = "B"; - S.update(Path, Inputs, WantDiagnostics::Yes); - - S.runWithPreamble("StaleRead", Path, TUScheduler::Stale, - [&](Expected<InputsAndPreamble> Pre) { - ASSERT_TRUE(bool(Pre)); - EXPECT_EQ(Pre->Preamble->Version, "A"); - EXPECT_THAT(includes(Pre->Preamble), - ElementsAre("<A>")); - InconsistentReadDone.notify(); - ++CallbackCount; - }); - S.runWithPreamble("ConsistentRead", Path, TUScheduler::Consistent, - [&](Expected<InputsAndPreamble> Pre) { - ASSERT_TRUE(bool(Pre)); - EXPECT_EQ(Pre->Preamble->Version, "B"); - EXPECT_THAT(includes(Pre->Preamble), - ElementsAre("<B>")); - ++CallbackCount; - }); - } - EXPECT_EQ(2, CallbackCount); -} - TEST_F(TUSchedulerTests, Cancellation) { // We have the following update/read sequence // U0 Index: clang-tools-extra/clangd/unittests/HeadersTests.cpp =================================================================== --- clang-tools-extra/clangd/unittests/HeadersTests.cpp +++ clang-tools-extra/clangd/unittests/HeadersTests.cpp @@ -6,6 +6,7 @@ // //===----------------------------------------------------------------------===// +#include "Annotations.h" #include "Headers.h" #include "Compiler.h" @@ -15,9 +16,13 @@ #include "clang/Frontend/CompilerInvocation.h" #include "clang/Frontend/FrontendActions.h" #include "clang/Lex/PreprocessorOptions.h" +#include "llvm/ADT/StringRef.h" #include "llvm/Support/Path.h" #include "gmock/gmock.h" #include "gtest/gtest.h" +#include "gtest/internal/gtest-port.h" +#include <tuple> +#include <vector> namespace clang { namespace clangd { @@ -136,6 +141,13 @@ return arg.getKey() == File && arg.getValue() == D; } +MATCHER(EqInc, "") { + Inclusion Actual = testing::get<0>(arg); + Inclusion Expected = testing::get<1>(arg); + return std::tie(Actual.HashOffset, Actual.R, Actual.Written) == + std::tie(Expected.HashOffset, Expected.R, Expected.Written); +} + TEST_F(HeadersTest, CollectRewrittenAndResolved) { FS.Files[MainFile] = R"cpp( #include "sub/bar.h" // not shortest @@ -285,6 +297,74 @@ llvm::None); } +TEST(GetPreambleIncludes, All) { + llvm::StringRef Cases[] = { + // Only preamble + R"cpp(^#include [["a.h"]])cpp", + // Both preamble and mainfile + R"cpp( + ^#include [["a.h"]] + garbage, finishes preamble + #include "a.h")cpp", + // Mixed directives + R"cpp( + ^#include [["a.h"]] + #pragma directive + // some comments + ^#include_next [[<a.h>]] + #ifdef skipped + ^#import [["a.h"]] + #endif)cpp", + // Broken directives + R"cpp( + #include "a + ^#include [["a.h"]] + #include <b + ^#include [[<b.h>]])cpp", + }; + LangOptions LangOpts; + LangOpts.CPlusPlus = true; + for (const auto Case : Cases) { + Annotations Test(Case); + const auto Code = Test.code(); + const auto Points = Test.points(); + const auto Ranges = Test.ranges(); + + std::vector<Inclusion> ExpectedIncs; + ExpectedIncs.resize(Points.size()); + for (size_t I = 0, E = ExpectedIncs.size(); I != E; ++I) { + Inclusion &Inc = ExpectedIncs[I]; + Inc.HashOffset = llvm::cantFail(positionToOffset(Code, Points[I])); + Inc.R = Ranges[I]; + auto StartOffset = llvm::cantFail(positionToOffset(Code, Inc.R.start)); + auto EndOffset = llvm::cantFail(positionToOffset(Code, Inc.R.end)); + Inc.Written = Code.substr(StartOffset, EndOffset - StartOffset).str(); + } + + EXPECT_THAT(getPreambleIncludes(Code, LangOpts), + testing::UnorderedPointwise(EqInc(), ExpectedIncs)); + } +} + +TEST(NewIncludes, All) { + std::vector<Inclusion> LHS(5); + LHS[0].Written = "\"a.h\""; // Deleted, ignored + LHS[1].Written = "\"b.h\""; + LHS[2].Written = "\"c.h\""; + LHS[3].Written = "<a.h>"; + LHS[4].Written = "<b.h>"; + std::vector<Inclusion> RHS(6); + RHS[0].Written = "\"b.h\""; + RHS[1].Written = "\"b1.h\""; // Added, in middle. + RHS[2].Written = "\"c.h\""; + RHS[3].Written = "<a.h>"; + RHS[4].Written = "<b.h>"; + RHS[5].Written = "<c.h>"; // Added, at the end. + + EXPECT_THAT(newIncludes(LHS, RHS), + ElementsAre(Written("\"b1.h\""), Written("<c.h>"))); +} + } // namespace } // namespace clangd } // namespace clang Index: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp =================================================================== --- clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp +++ clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp @@ -1188,6 +1188,32 @@ } } +TEST(SignatureHelpTest, StalePreamble) { + TestTU TU; + TU.Code = ""; + IgnoreDiagnostics Diags; + auto Inputs = TU.inputs(); + auto CI = buildCompilerInvocation(Inputs, Diags); + ASSERT_TRUE(CI); + auto EmptyPreamble = buildPreamble(testPath(TU.Filename), *CI, + /*OldPreamble=*/nullptr, Inputs, + /*InMemory=*/true, /*Callback=*/nullptr); + ASSERT_TRUE(EmptyPreamble); + + TU.AdditionalFiles["a.h"] = "int foo(int x);"; + const Annotations Test(R"cpp( + #include "a.h" + void bar() { foo(^2); })cpp"); + TU.Code = Test.code().str(); + Inputs = TU.inputs(); + auto Results = + signatureHelp(testPath(TU.Filename), Inputs.CompileCommand, + *EmptyPreamble, TU.Code, Test.point(), Inputs.FS, nullptr); + EXPECT_THAT(Results.signatures, ElementsAre(Sig("foo([[int x]]) -> int"))); + EXPECT_EQ(0, Results.activeSignature); + EXPECT_EQ(0, Results.activeParameter); +} + class IndexRequestCollector : public SymbolIndex { public: bool Index: clang-tools-extra/clangd/TUScheduler.h =================================================================== --- clang-tools-extra/clangd/TUScheduler.h +++ clang-tools-extra/clangd/TUScheduler.h @@ -245,11 +245,6 @@ /// Controls whether preamble reads wait for the preamble to be up-to-date. enum PreambleConsistency { - /// The preamble is generated from the current version of the file. - /// If the content was recently updated, we will wait until we have a - /// preamble that reflects that update. - /// This is the slowest option, and may be delayed by other tasks. - Consistent, /// The preamble may be generated from an older version of the file. /// Reading from locations in the preamble may cause files to be re-read. /// This gives callers two options: Index: clang-tools-extra/clangd/TUScheduler.cpp =================================================================== --- clang-tools-extra/clangd/TUScheduler.cpp +++ clang-tools-extra/clangd/TUScheduler.cpp @@ -595,36 +595,6 @@ return LastBuiltPreamble; } -void ASTWorker::getCurrentPreamble( - llvm::unique_function<void(std::shared_ptr<const PreambleData>)> Callback) { - // We could just call startTask() to throw the read on the queue, knowing - // it will run after any updates. But we know this task is cheap, so to - // improve latency we cheat: insert it on the queue after the last update. - std::unique_lock<std::mutex> Lock(Mutex); - auto LastUpdate = - std::find_if(Requests.rbegin(), Requests.rend(), - [](const Request &R) { return R.UpdateType.hasValue(); }); - // If there were no writes in the queue, and CurrentRequest is not a write, - // the preamble is ready now. - if (LastUpdate == Requests.rend() && - (!CurrentRequest || CurrentRequest->UpdateType.hasValue())) { - Lock.unlock(); - return Callback(getPossiblyStalePreamble()); - } - assert(!RunSync && "Running synchronously, but queue is non-empty!"); - Requests.insert(LastUpdate.base(), - Request{[Callback = std::move(Callback), this]() mutable { - Callback(getPossiblyStalePreamble()); - }, - "GetPreamble", steady_clock::now(), - Context::current().clone(), - /*UpdateType=*/None, - /*InvalidationPolicy=*/TUScheduler::NoInvalidation, - /*Invalidate=*/nullptr}); - Lock.unlock(); - RequestsCV.notify_all(); -} - void ASTWorker::waitForFirstPreamble() const { PreambleWasBuilt.wait(); } std::shared_ptr<const ParseInputs> ASTWorker::getCurrentFileInputs() const { @@ -1013,38 +983,21 @@ return; } - // Future is populated if the task needs a specific preamble. - std::future<std::shared_ptr<const PreambleData>> ConsistentPreamble; - if (Consistency == Consistent) { - std::promise<std::shared_ptr<const PreambleData>> Promise; - ConsistentPreamble = Promise.get_future(); - It->second->Worker->getCurrentPreamble( - [Promise = std::move(Promise)]( - std::shared_ptr<const PreambleData> Preamble) mutable { - Promise.set_value(std::move(Preamble)); - }); - } - std::shared_ptr<const ASTWorker> Worker = It->second->Worker.lock(); auto Task = [Worker, Consistency, Name = Name.str(), File = File.str(), Contents = It->second->Contents, Command = Worker->getCurrentCompileCommand(), Ctx = Context::current().derive(kFileBeingProcessed, std::string(File)), - ConsistentPreamble = std::move(ConsistentPreamble), Action = std::move(Action), this]() mutable { std::shared_ptr<const PreambleData> Preamble; - if (ConsistentPreamble.valid()) { - Preamble = ConsistentPreamble.get(); - } else { - if (Consistency != PreambleConsistency::StaleOrAbsent) { - // Wait until the preamble is built for the first time, if preamble - // is required. This avoids extra work of processing the preamble - // headers in parallel multiple times. - Worker->waitForFirstPreamble(); - } - Preamble = Worker->getPossiblyStalePreamble(); + if (Consistency == PreambleConsistency::Stale) { + // Wait until the preamble is built for the first time, if preamble + // is required. This avoids extra work of processing the preamble + // headers in parallel multiple times. + Worker->waitForFirstPreamble(); } + Preamble = Worker->getPossiblyStalePreamble(); std::lock_guard<Semaphore> BarrierLock(Barrier); WithContext Guard(std::move(Ctx)); Index: clang-tools-extra/clangd/Preamble.h =================================================================== --- clang-tools-extra/clangd/Preamble.h +++ clang-tools-extra/clangd/Preamble.h @@ -47,7 +47,8 @@ std::vector<Diag> Diags, IncludeStructure Includes, MainFileMacros Macros, std::unique_ptr<PreambleFileStatusCache> StatCache, - CanonicalIncludes CanonIncludes); + CanonicalIncludes CanonIncludes, + std::vector<Inclusion> LexedIncludes); // Version of the ParseInputs this preamble was built from. std::string Version; @@ -65,6 +66,9 @@ // When reusing a preamble, this cache can be consumed to save IO. std::unique_ptr<PreambleFileStatusCache> StatCache; CanonicalIncludes CanonIncludes; + /// Contains all of the includes spelled in the preamble section, even the + /// ones in disabled regions. + std::vector<Inclusion> LexedIncludes; }; using PreambleParsedCallback = Index: clang-tools-extra/clangd/Preamble.cpp =================================================================== --- clang-tools-extra/clangd/Preamble.cpp +++ clang-tools-extra/clangd/Preamble.cpp @@ -8,11 +8,13 @@ #include "Preamble.h" #include "Compiler.h" +#include "Headers.h" #include "Logger.h" #include "Trace.h" #include "clang/Basic/SourceLocation.h" #include "clang/Lex/PPCallbacks.h" #include "clang/Lex/PreprocessorOptions.h" +#include <vector> namespace clang { namespace clangd { @@ -81,12 +83,13 @@ std::vector<Diag> Diags, IncludeStructure Includes, MainFileMacros Macros, std::unique_ptr<PreambleFileStatusCache> StatCache, - CanonicalIncludes CanonIncludes) + CanonicalIncludes CanonIncludes, + std::vector<Inclusion> LexedIncludes) : Version(Inputs.Version), CompileCommand(Inputs.CompileCommand), Preamble(std::move(Preamble)), Diags(std::move(Diags)), Includes(std::move(Includes)), Macros(std::move(Macros)), - StatCache(std::move(StatCache)), CanonIncludes(std::move(CanonIncludes)) { -} + StatCache(std::move(StatCache)), CanonIncludes(std::move(CanonIncludes)), + LexedIncludes(std::move(LexedIncludes)) {} std::shared_ptr<const PreambleData> buildPreamble(PathRef FileName, CompilerInvocation &CI, @@ -164,7 +167,8 @@ Inputs, std::move(*BuiltPreamble), std::move(Diags), SerializedDeclsCollector.takeIncludes(), SerializedDeclsCollector.takeMacros(), std::move(StatCache), - SerializedDeclsCollector.takeCanonicalIncludes()); + SerializedDeclsCollector.takeCanonicalIncludes(), + getPreambleIncludes(Inputs.Contents, *CI.getLangOpts())); } else { elog("Could not build a preamble for file {0} version {1}", FileName, Inputs.Version); Index: clang-tools-extra/clangd/Headers.h =================================================================== --- clang-tools-extra/clangd/Headers.h +++ clang-tools-extra/clangd/Headers.h @@ -195,6 +195,16 @@ tooling::HeaderIncludes Inserter; // Computers insertion replacement. }; +/// Gets the includes in the preamble section of the file by running lexer over +/// \p Contents. Returned inclusions are sorted according to written filename. +std::vector<Inclusion> getPreambleIncludes(llvm::StringRef Contents, + const LangOptions &LangOpts); + +/// Returns set of includes existing in \p New but missing in \p Old. Assumes +/// inputs are sorted w.r.t Written field. +std::vector<Inclusion> newIncludes(llvm::ArrayRef<Inclusion> Old, + llvm::ArrayRef<Inclusion> New); + } // namespace clangd } // namespace clang Index: clang-tools-extra/clangd/Headers.cpp =================================================================== --- clang-tools-extra/clangd/Headers.cpp +++ clang-tools-extra/clangd/Headers.cpp @@ -10,10 +10,13 @@ #include "Compiler.h" #include "Logger.h" #include "SourceCode.h" +#include "clang/Basic/LangOptions.h" #include "clang/Frontend/CompilerInstance.h" #include "clang/Frontend/CompilerInvocation.h" #include "clang/Frontend/FrontendActions.h" #include "clang/Lex/HeaderSearch.h" +#include "llvm/ADT/Optional.h" +#include "llvm/ADT/STLExtras.h" #include "llvm/ADT/StringRef.h" #include "llvm/Support/Path.h" @@ -231,5 +234,72 @@ << Inc.R; } +std::vector<Inclusion> getPreambleIncludes(llvm::StringRef Contents, + const LangOptions &LangOpts) { + auto ContentsBuffer = llvm::MemoryBuffer::getMemBuffer(Contents, "dummy"); + auto Bounds = ComputePreambleBounds(LangOpts, ContentsBuffer.get(), 0); + + SourceManagerForFile SMFF("dummy", ContentsBuffer->getBuffer()); + auto &SM = SMFF.get(); + auto FID = SM.getMainFileID(); + + Lexer TheLexer(FID, SM.getBuffer(FID), SM, LangOpts); + Token TheTok; + std::vector<Inclusion> Incs; + Inclusion Inc; + + // Lex until end of preamble bounds. + while (!TheLexer.LexFromRawLexer(TheTok) && + TheLexer.getCurrentBufferOffset() < Bounds.Size) { + // Skip unless at the start of a PP directive. + if (!TheTok.isAtStartOfLine() || !TheTok.is(tok::hash)) + continue; + Inc.HashOffset = TheLexer.getCurrentBufferOffset() - TheTok.getLength(); + + // Skip if not an include directive. + TheLexer.LexFromRawLexer(TheTok); + if (!TheTok.is(tok::raw_identifier)) + continue; + llvm::StringRef Directive = TheTok.getRawIdentifier(); + if (Directive != "include" && Directive != "import" && + Directive != "include_next") + continue; + + // Skip if on a broken include directive + TheLexer.LexIncludeFilename(TheTok); + if (!TheTok.isLiteral()) + continue; + Inc.Written = + llvm::StringRef(TheTok.getLiteralData(), TheTok.getLength()).str(); + Inc.R = halfOpenToRange(SM, CharSourceRange::getCharRange( + TheTok.getLocation(), TheTok.getEndLoc())); + Incs.push_back(std::move(Inc)); + } + llvm::sort(Incs, [](const Inclusion &LHS, const Inclusion &RHS) { + return LHS.Written < RHS.Written; + }); + + return Incs; +} + +std::vector<Inclusion> newIncludes(llvm::ArrayRef<Inclusion> Old, + llvm::ArrayRef<Inclusion> New) { + std::vector<Inclusion> Added; + while (!Old.empty() && !New.empty()) { + // Old.front() doesn't exists in New. Ignored as we are only looking for + // additions. + if (Old.front().Written < New.front().Written) + Old = Old.drop_front(); + else if (Old.front().Written > New.front().Written) { + Added.push_back(New.front()); + New = New.drop_front(); + } else { + Old = Old.drop_front(); + New = New.drop_front(); + } + } + Added.insert(Added.end(), New.begin(), New.end()); + return Added; +} } // namespace clangd } // namespace clang Index: clang-tools-extra/clangd/CodeComplete.cpp =================================================================== --- clang-tools-extra/clangd/CodeComplete.cpp +++ clang-tools-extra/clangd/CodeComplete.cpp @@ -1048,9 +1048,13 @@ // Invokes Sema code completion on a file. // If \p Includes is set, it will be updated based on the compiler invocation. +// If \p PatchAdditionalIncludes is set, calculates newly introduced includes to +// current file contents since \p Input.Preamble was built and parses those +// includes as part of the mainfile. bool semaCodeComplete(std::unique_ptr<CodeCompleteConsumer> Consumer, const clang::CodeCompleteOptions &Options, const SemaCompleteInput &Input, + bool PatchAdditionalIncludes, IncludeStructure *Includes = nullptr) { trace::Span Tracer("Sema completion"); llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS = Input.VFS; @@ -1068,6 +1072,16 @@ elog("Couldn't create CompilerInvocation"); return false; } + if (PatchAdditionalIncludes) { + for (const auto &Inc : newIncludes( + Input.Preamble.LexedIncludes, + getPreambleIncludes(ParseInput.Contents, *CI->getLangOpts()))) { + // Inc.Written contains quotes or angles, preprocessor requires them to be + // stripped. + CI->getPreprocessorOpts().Includes.emplace_back( + llvm::StringRef(Inc.Written).drop_back().drop_front()); + } + } auto &FrontendOpts = CI->getFrontendOpts(); FrontendOpts.SkipFunctionBodies = true; // Disable typo correction in Sema. @@ -1312,8 +1326,10 @@ Recorder = RecorderOwner.get(); + // Note that we don't patch in case of a stale preamble to improve latency, + // while giving up on correctness. semaCodeComplete(std::move(RecorderOwner), Opts.getClangCompleteOpts(), - SemaCCInput, &Includes); + SemaCCInput, /*PatchAdditionalIncludes=*/false, &Includes); logResults(Output, Tracer); return Output; } @@ -1775,10 +1791,12 @@ Options.IncludeMacros = false; Options.IncludeCodePatterns = false; Options.IncludeBriefComments = false; - IncludeStructure PreambleInclusions; // Unused for signatureHelp + // Patch additional includes since signaturehelp can be trigerred after a code + // completion which insterted a new header. semaCodeComplete( std::make_unique<SignatureHelpCollector>(Options, Index, Result), Options, - {FileName, Command, Preamble, Contents, *Offset, std::move(VFS)}); + {FileName, Command, Preamble, Contents, *Offset, std::move(VFS)}, + /*PatchAdditionalIncludes=*/true); return Result; } Index: clang-tools-extra/clangd/ClangdServer.cpp =================================================================== --- clang-tools-extra/clangd/ClangdServer.cpp +++ clang-tools-extra/clangd/ClangdServer.cpp @@ -280,11 +280,8 @@ Pos, FS, Index)); }; - // Unlike code completion, we wait for an up-to-date preamble here. - // Signature help is often triggered after code completion. If the code - // completion inserted a header to make the symbol available, then using - // the old preamble would yield useless results. - WorkScheduler.runWithPreamble("SignatureHelp", File, TUScheduler::Consistent, + // Unlike code completion, we wait for a preamble here. + WorkScheduler.runWithPreamble("SignatureHelp", File, TUScheduler::Stale, std::move(Action)); }
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits