diff --git a/clang-tools-extra/clangd/Preamble.h b/clang-tools-extra/clangd/Preamble.h --- a/clang-tools-extra/clangd/Preamble.h +++ b/clang-tools-extra/clangd/Preamble.h @@ -27,7 +27,9 @@ #include "Diagnostics.h" #include "FS.h" #include "Headers.h" +#include "Path.h" #include "index/CanonicalIncludes.h" +#include "clang/Frontend/CompilerInvocation.h" #include "clang/Frontend/PrecompiledPreamble.h" #include "clang/Tooling/CompilationDatabase.h" @@ -72,17 +74,19 @@ const CanonicalIncludes &)>; /// Build a preamble for the new inputs unless an old one can be reused. -/// If \p OldPreamble can be reused, it is returned unchanged. -/// If \p OldPreamble is null, always builds the preamble. /// If \p PreambleCallback is set, it will be run on top of the AST while -/// building the preamble. Note that if the old preamble was reused, no AST is -/// built and, therefore, the callback will not be executed. +/// building the preamble. std::shared_ptr buildPreamble(PathRef FileName, CompilerInvocation CI, - std::shared_ptr OldPreamble, const ParseInputs &Inputs, bool StoreInMemory, PreambleParsedCallback PreambleCallback); +/// Returns true if \p Preamble is reusable for \p Inputs. Note that it will +/// return true when some missing headers are now available. +/// FIXME: Should return more information about the delta between \p Preamble +/// and \p Inputs, e.g. new headers. +bool isPreambleUsable(const PreambleData &Preamble, const ParseInputs &Inputs, + PathRef FileName, const CompilerInvocation &CI); } // namespace clangd } // namespace clang diff --git a/clang-tools-extra/clangd/Preamble.cpp b/clang-tools-extra/clangd/Preamble.cpp --- a/clang-tools-extra/clangd/Preamble.cpp +++ b/clang-tools-extra/clangd/Preamble.cpp @@ -90,7 +90,6 @@ std::shared_ptr buildPreamble(PathRef FileName, CompilerInvocation CI, - std::shared_ptr OldPreamble, const ParseInputs &Inputs, bool StoreInMemory, PreambleParsedCallback PreambleCallback) { // Note that we don't need to copy the input contents, preamble can live @@ -100,23 +99,6 @@ auto Bounds = ComputePreambleBounds(*CI.getLangOpts(), ContentsBuffer.get(), 0); - if (OldPreamble && - compileCommandsAreEqual(Inputs.CompileCommand, - OldPreamble->CompileCommand) && - OldPreamble->Preamble.CanReuse(CI, ContentsBuffer.get(), Bounds, - Inputs.FS.get())) { - vlog("Reusing preamble version {0} for version {1} of {2}", - OldPreamble->Version, Inputs.Version, FileName); - return OldPreamble; - } - if (OldPreamble) - vlog("Rebuilding invalidated preamble for {0} version {1} " - "(previous was version {2})", - FileName, Inputs.Version, OldPreamble->Version); - else - vlog("Building first preamble for {0} verson {1}", FileName, - Inputs.Version); - trace::Span Tracer("BuildPreamble"); SPAN_ATTACH(Tracer, "File", FileName); StoreDiags PreambleDiagnostics; @@ -168,5 +150,16 @@ } } +bool isPreambleUsable(const PreambleData &Preamble, const ParseInputs &Inputs, + PathRef FileName, const CompilerInvocation &CI) { + auto ContentsBuffer = + llvm::MemoryBuffer::getMemBuffer(Inputs.Contents, FileName); + auto Bounds = + ComputePreambleBounds(*CI.getLangOpts(), ContentsBuffer.get(), 0); + return compileCommandsAreEqual(Inputs.CompileCommand, + Preamble.CompileCommand) && + Preamble.Preamble.CanReuse(CI, ContentsBuffer.get(), Bounds, + Inputs.FS.get()); +} } // namespace clangd } // namespace clang diff --git a/clang-tools-extra/clangd/TUScheduler.cpp b/clang-tools-extra/clangd/TUScheduler.cpp --- a/clang-tools-extra/clangd/TUScheduler.cpp +++ b/clang-tools-extra/clangd/TUScheduler.cpp @@ -67,11 +67,13 @@ #include "llvm/Support/Path.h" #include "llvm/Support/Threading.h" #include +#include #include #include #include #include #include +#include namespace clang { namespace clangd { @@ -196,6 +198,9 @@ /// built synchronously in update() instead. class PreambleThread { public: + using PreambleCallback = + std::function, ParseInputs, + std::shared_ptr)>; PreambleThread(llvm::StringRef FileName, ParsingCallbacks &Callbacks, bool StorePreambleInMemory, bool RunSync, SynchronizedTUStatus &Status) @@ -211,7 +216,7 @@ /// It isn't guaranteed that each requested version will be built. If there /// are multiple update requests while building a preamble, only the last one /// will be built. - void update(CompilerInvocation *CI, ParseInputs PI) { + void update(CompilerInvocation *CI, ParseInputs PI, PreambleCallback PC) { // If compiler invocation was broken, just fail out early. if (!CI) { // Make sure anyone waiting for the preamble gets notified it could not be @@ -220,7 +225,8 @@ return; } // Make possibly expensive copy while not holding the lock. - Request Req = {std::make_unique(*CI), std::move(PI)}; + Request Req = {std::make_unique(*CI), std::move(PI), + std::move(PC), Context::current().clone()}; if (RunSync) { build(std::move(Req)); return; @@ -259,6 +265,7 @@ CurrentReq = std::move(*NextReq); NextReq.reset(); } + WithContext Guard(std::move(CurrentReq->Ctx)); // Build the preamble and let the waiters know about it. build(std::move(*CurrentReq)); bool IsEmpty = false; @@ -305,6 +312,8 @@ struct Request { std::unique_ptr CI; ParseInputs Inputs; + PreambleCallback PC; + Context Ctx; }; bool isDone() { @@ -319,21 +328,32 @@ void build(Request Req) { assert(Req.CI && "Got preamble request with null compiler invocation"); const ParseInputs &Inputs = Req.Inputs; - std::shared_ptr OldPreamble = - Inputs.ForceRebuild ? nullptr : latest(); + + if (LatestBuild) { + vlog("Rebuilding invalidated preamble for {0} version {1} " + "(previous was version {2})", + FileName, Inputs.Version, LatestBuild->Version); + } else { + vlog("Building first preamble for {0} version {1}", FileName, + Inputs.Version); + } Status.update([&](TUStatus &Status) { Status.PreambleActivity = PreambleAction::Building; }); auto Preamble = clang::clangd::buildPreamble( - FileName, std::move(*Req.CI), OldPreamble, Inputs, StoreInMemory, + FileName, *Req.CI, Inputs, StoreInMemory, [this, Version(Inputs.Version)]( ASTContext &Ctx, std::shared_ptr PP, const CanonicalIncludes &CanonIncludes) { Callbacks.onPreambleAST(FileName, Version, Ctx, std::move(PP), CanonIncludes); }); + // We run preamble callbacks before updating latest build preamble to make + // sure ASTWorker doesn't see the fresh preamble until preambleworker + // finishes. + Req.PC(std::move(Req.CI), std::move(Req.Inputs), Preamble); { std::lock_guard Lock(Mutex); // LatestBuild might be the last reference to old preamble, do not trigger @@ -623,19 +643,54 @@ std::vector CC1Args; std::unique_ptr Invocation = buildCompilerInvocation( Inputs, CompilerInvocationDiagConsumer, &CC1Args); - // This is true for now, as we always block until new preamble is build. - // Once we start to block preambles out-of-order we need to make sure - // OldPreamble refers to the preamble that was used to build last AST. - auto OldPreamble = PW.latest(); - PW.update(Invocation.get(), Inputs); - // FIXME make use of the stale preambles instead. - PW.blockUntilIdle(Deadline::infinity()); - auto NewPreamble = PW.latest(); // Log cc1 args even (especially!) if creating invocation failed. if (!CC1Args.empty()) vlog("Driver produced command: cc1 {0}", llvm::join(CC1Args, " ")); std::vector CompilerInvocationDiags = CompilerInvocationDiagConsumer.take(); + auto Preamble = Inputs.ForceRebuild ? nullptr : PW.latest(); + bool PreambleFresh = + Invocation && Preamble && + isPreambleUsable(*Preamble, Inputs, FileName, *Invocation); + if (!PreambleFresh) { + PW.update( + Invocation.get(), Inputs, + [this, WantDiags, CompilerInvocationDiags, RunPublish, + TaskName](std::unique_ptr CI, ParseInputs PI, + std::shared_ptr Preamble) { + if (WantDiags == WantDiagnostics::No) + return; + Status.update([TaskName = std::move(TaskName)](TUStatus &Status) { + Status.ASTActivity.K = ASTAction::Building; + Status.ASTActivity.Name = TaskName; + }); + if (auto AST = + buildAST(FileName, std::move(CI), CompilerInvocationDiags, + std::move(PI), std::move(Preamble))) { + Status.update([](TUStatus &Status) { + Status.Details.BuildFailed = false; + Status.Details.ReuseAST = false; + }); + trace::Span Span("Running main AST callback"); + Callbacks.onMainAST(FileName, *AST, RunPublish); + // Note that we are not caching the AST we've just built, or + // updating ASTWorker state saying that we've ran AST callbacks. + // This results in building and running callbacks on some updates + // possibly twice, but makes reasoning about ASTWorker internals + // easier, as they are manipulated by only a single thread. + } else { + Status.update([](TUStatus &Status) { + Status.Details.BuildFailed = true; + Status.Details.ReuseAST = false; + }); + } + }); + } else { + vlog("Reusing preamble version {0} for version {1} of {2}", + Preamble->Version, Inputs.Version, FileName); + } + // FIXME: make use of the stale preambles instead. + PW.blockUntilIdle(Deadline::infinity()); if (!Invocation) { elog("Could not build CompilerInvocation for file {0}", FileName); // Remove the old AST if it's still in cache. @@ -646,16 +701,20 @@ return; } - bool CanReuseAST = InputsAreTheSame && (OldPreamble == NewPreamble); - // Before doing the expensive AST reparse, we want to release our reference - // to the old preamble, so it can be freed if there are no other references - // to it. - OldPreamble.reset(); + // Do not build AST with stale preambles, as diagnostics might contain false + // positives and symbol information might be misleading. + if (!PreambleFresh) { + // FIXME: We should build patched-up ASTs in case of stale preambles and + // keep using them. + IdleASTs.take(this); + return; + } + Status.update([&](TUStatus &Status) { Status.ASTActivity.K = ASTAction::Building; Status.ASTActivity.Name = std::move(TaskName); }); - if (!CanReuseAST) { + if (!InputsAreTheSame) { IdleASTs.take(this); // Remove the old AST if it's still in cache. } else { // We don't need to rebuild the AST, check if we need to run the callback. @@ -697,7 +756,7 @@ if (!AST) { llvm::Optional NewAST = buildAST(FileName, std::move(Invocation), CompilerInvocationDiags, - Inputs, NewPreamble); + Inputs, Preamble); // buildAST fails. Status.update([&](TUStatus &Status) { Status.Details.ReuseAST = false; diff --git a/clang-tools-extra/clangd/unittests/FileIndexTests.cpp b/clang-tools-extra/clangd/unittests/FileIndexTests.cpp --- a/clang-tools-extra/clangd/unittests/FileIndexTests.cpp +++ b/clang-tools-extra/clangd/unittests/FileIndexTests.cpp @@ -286,7 +286,7 @@ FileIndex Index; bool IndexUpdated = false; - buildPreamble(FooCpp, *CI, /*OldPreamble=*/nullptr, PI, + buildPreamble(FooCpp, *CI, PI, /*StoreInMemory=*/true, [&](ASTContext &Ctx, std::shared_ptr PP, const CanonicalIncludes &CanonIncludes) { @@ -424,7 +424,7 @@ } TEST(FileIndexTest, MergeMainFileSymbols) { - const char* CommonHeader = "void foo();"; + const char *CommonHeader = "void foo();"; TestTU Header = TestTU::withCode(CommonHeader); TestTU Cpp = TestTU::withCode("void foo() {}"); Cpp.Filename = "foo.cpp"; diff --git a/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp b/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp --- a/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp +++ b/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp @@ -570,6 +570,17 @@ auto Bar = testPath("bar.cpp"); auto Baz = testPath("baz.cpp"); + // Build all of the files once, so that we've got their preambles ready. + updateWithCallback(S, Foo, SourceContents, WantDiagnostics::Yes, + [&BuiltASTCounter]() { ++BuiltASTCounter; }); + updateWithCallback(S, Bar, SourceContents, WantDiagnostics::Yes, + [&BuiltASTCounter]() { ++BuiltASTCounter; }); + updateWithCallback(S, Baz, SourceContents, WantDiagnostics::Yes, + [&BuiltASTCounter]() { ++BuiltASTCounter; }); + ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10))); + ASSERT_EQ(BuiltASTCounter.load(), 3); + BuiltASTCounter = 0; + // Build one file in advance. We will not access it later, so it will be the // one that the cache will evict. updateWithCallback(S, Foo, SourceContents, WantDiagnostics::Yes, @@ -697,26 +708,29 @@ }; // Test that subsequent updates with the same inputs do not cause rebuilds. - ASSERT_TRUE(DoUpdate(SourceContents)); - ASSERT_FALSE(DoUpdate(SourceContents)); + ASSERT_TRUE(DoUpdate(SourceContents)); // Builds preamble + ASSERT_TRUE(DoUpdate(SourceContents)); // Builds AST + ASSERT_FALSE(DoUpdate(SourceContents)); // Reuses it // Update to a header should cause a rebuild, though. Timestamps[Header] = time_t(1); - ASSERT_TRUE(DoUpdate(SourceContents)); - ASSERT_FALSE(DoUpdate(SourceContents)); + ASSERT_TRUE(DoUpdate(SourceContents)); // Builds preamble + ASSERT_TRUE(DoUpdate(SourceContents)); // Builds AST + ASSERT_FALSE(DoUpdate(SourceContents)); // Reuses AST // Update to the contents should cause a rebuild. auto OtherSourceContents = R"cpp( #include "foo.h" int c = d; )cpp"; - ASSERT_TRUE(DoUpdate(OtherSourceContents)); - ASSERT_FALSE(DoUpdate(OtherSourceContents)); + ASSERT_TRUE(DoUpdate(OtherSourceContents)); // Builds AST, reusing preamble + ASSERT_FALSE(DoUpdate(OtherSourceContents)); // Reuses AST // Update to the compile commands should also cause a rebuild. CDB.ExtraClangFlags.push_back("-DSOMETHING"); - ASSERT_TRUE(DoUpdate(OtherSourceContents)); - ASSERT_FALSE(DoUpdate(OtherSourceContents)); + ASSERT_TRUE(DoUpdate(SourceContents)); // Builds preamble + ASSERT_TRUE(DoUpdate(OtherSourceContents)); // Builds AST + ASSERT_FALSE(DoUpdate(OtherSourceContents)); // Reuses it } TEST_F(TUSchedulerTests, ForceRebuild) { @@ -732,6 +746,10 @@ ParseInputs Inputs = getInputs(Source, SourceContents); + // Send an initial update to make sure we've got preamble ready. + S.update(Source, Inputs, WantDiagnostics::Yes); + ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10))); + // Update the source contents, which should trigger an initial build with // the header file missing. updateWithDiags( @@ -853,13 +871,14 @@ TUState(PreambleAction::Idle, ASTAction::RunningAction), // We build the preamble TUState(PreambleAction::Building, ASTAction::RunningAction), + // We start building the ast, as a result of building + // preamble. + TUState(PreambleAction::Building, ASTAction::Building), + // AST built finished successfully + TUState(PreambleAction::Building, ASTAction::Building), // Preamble worker goes idle - TUState(PreambleAction::Idle, ASTAction::RunningAction), - // We start building the ast - TUState(PreambleAction::Idle, ASTAction::Building), - // Built finished succesffully TUState(PreambleAction::Idle, ASTAction::Building), - // Rnning go to def + // Running go to def TUState(PreambleAction::Idle, ASTAction::RunningAction), // both workers go idle TUState(PreambleAction::Idle, ASTAction::Idle))); diff --git a/clang-tools-extra/clangd/unittests/TestTU.cpp b/clang-tools-extra/clangd/unittests/TestTU.cpp --- a/clang-tools-extra/clangd/unittests/TestTU.cpp +++ b/clang-tools-extra/clangd/unittests/TestTU.cpp @@ -66,8 +66,7 @@ auto CI = buildCompilerInvocation(Inputs, Diags); assert(CI && "Failed to build compilation invocation."); auto Preamble = - buildPreamble(FullFilename, *CI, - /*OldPreamble=*/nullptr, Inputs, + buildPreamble(FullFilename, *CI, Inputs, /*StoreInMemory=*/true, /*PreambleCallback=*/nullptr); auto AST = buildAST(FullFilename, std::move(CI), Diags.take(), Inputs, Preamble);