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 @@ -6,16 +6,46 @@ // //===----------------------------------------------------------------------===// // For each file, managed by TUScheduler, we create a single ASTWorker that -// manages an AST for that file. All operations that modify or read the AST are -// run on a separate dedicated thread asynchronously in FIFO order. +// manages an AST and a preamble for that file. All operations that modify or +// read the AST are run on a separate dedicated thread asynchronously in FIFO +// order. There is a second thread for preamble builds, ASTWorker thread issues +// updates on that preamble thread as preamble becomes stale. +// +// As a consequence of that, read requests to the AST always get processed in +// order and get the exact version of the file when they were issued. Those ASTs +// are always built with a compatible preamble, built with latest update that +// modified preamble section of the file. +// FIXME: Get rid of the synchronization between ASTWorker::update and +// PreambleThread builds by using patched ASTs instead of compatible preambles +// for reads. Only keep the blocking behaviour for picky read requests, e.g. +// Tweaks. +// +// Updates are processed in two phases, by issuing a preamble and an ast build. +// The preamble build gets issued into a different worker and might be +// overwritten by subsequent updates. An AST will be built for an update if +// latest built preamble is compatible for it or a new preamble gets built for +// that update. +// AST builds are always executed on ASTWorker thread, either immediately +// following an update request with a compatible preamble, or through a new +// request, inserted at front of the queue, from PreambleThread after building a +// new preamble, we'll call the ASTs built in latter case "golden AST"s. +// +// Diagnostics and index is updated as a result of building AST for write +// requests. Progress on those updates are ensured with golden ASTs, as they'll +// be built even when ASTWorker queue is full. Since ASTs aren't built with +// incompatible preambles, we can partition diagnostic updates into epochs of +// preambles. Each epoch starts with a golden AST(unless that update had +// WantDiagnostics::No), and then moves forward with updates to the AST. Since +// preambles are always built with newer versions of the file and updates in the +// ASTWorker are in order, we ensure diagnostics are always built with newer +// versions of the file. // // We start processing each update immediately after we receive it. If two or // more updates come subsequently without reads in-between, we attempt to drop // an older one to not waste time building the ASTs we don't need. // -// The processing thread of the ASTWorker is also responsible for building the -// preamble. However, unlike AST, the same preamble can be read concurrently, so -// we run each of async preamble reads on its own thread. +// ASTWorker is also responsible for serving preambles. Since preambles can be +// read concurrently, we run each of async preamble reads on its own thread. // // To limit the concurrent load that clangd produces we maintain a semaphore // that keeps more than a fixed number of threads from running concurrently. @@ -56,6 +86,7 @@ #include "index/CanonicalIncludes.h" #include "clang/Frontend/CompilerInvocation.h" #include "clang/Tooling/CompilationDatabase.h" +#include "llvm/ADT/None.h" #include "llvm/ADT/Optional.h" #include "llvm/ADT/STLExtras.h" #include "llvm/ADT/ScopeExit.h" @@ -64,14 +95,19 @@ #include "llvm/ADT/StringRef.h" #include "llvm/Support/Errc.h" #include "llvm/Support/ErrorHandling.h" +#include "llvm/Support/FormatVariadic.h" #include "llvm/Support/Path.h" #include "llvm/Support/Threading.h" #include +#include +#include #include #include #include #include #include +#include +#include namespace clang { namespace clangd { @@ -190,12 +226,15 @@ ParsingCallbacks &Callbacks; }; -/// Responsible for building and providing access to the preamble of a TU. -/// Whenever the thread is idle and the preamble is outdated, it starts to build -/// a fresh preamble from the latest inputs. If RunSync is true, preambles are -/// built synchronously in update() instead. +/// Responsible for building preambles. Whenever the thread is idle and the +/// preamble is outdated, it starts to build a fresh preamble from the latest +/// inputs. If RunSync is true, preambles are 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) @@ -203,31 +242,25 @@ StoreInMemory(StorePreambleInMemory), RunSync(RunSync), Status(Status) { } - size_t getUsedBytes() const { - auto Preamble = latest(); - return Preamble ? Preamble->Preamble.getSize() : 0; - } - /// 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) { - // If compiler invocation was broken, just fail out early. - if (!CI) { - // Make sure anyone waiting for the preamble gets notified it could not be - // built. - BuiltFirst.notify(); - return; - } + void update(const CompilerInvocation &CI, ParseInputs PI, + PreambleCallback PC) { // 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; } { std::lock_guard Lock(Mutex); - assert(!Done && "Build request to PreambleWorker after stop"); + // ASTWorker might've updates left in it even after PreambleWorker is + // stopped. Ignore those as it should be able to serve remaining requests + // with stale preambles. + if (Done) + return; NextReq = std::move(Req); } // Let the worker thread know there's a request, notify_one is safe as there @@ -235,17 +268,6 @@ ReqCV.notify_all(); } - /// Blocks until at least a single request has been processed. Note that it - /// will unblock even after an unsuccessful build. - void waitForFirst() const { BuiltFirst.wait(); } - - /// Returns the latest built preamble, might be null if no preamble has been - /// built or latest attempt resulted in a failure. - std::shared_ptr latest() const { - std::lock_guard Lock(Mutex); - return LatestBuild; - } - void run() { dlog("Starting preamble worker for {0}", FileName); while (true) { @@ -259,6 +281,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; @@ -275,11 +298,8 @@ Status.PreambleActivity = PreambleAction::Idle; }); } - BuiltFirst.notify(); ReqCV.notify_all(); } - // We are no longer going to build any preambles, let the waiters know that. - BuiltFirst.notify(); dlog("Preamble worker for {0} finished", FileName); } @@ -289,6 +309,7 @@ { std::lock_guard Lock(Mutex); Done = true; + NextReq.reset(); } // Let the worker thread know that it should stop. ReqCV.notify_all(); @@ -305,6 +326,8 @@ struct Request { std::unique_ptr CI; ParseInputs Inputs; + PreambleCallback PC; + Context Ctx; }; bool isDone() { @@ -319,27 +342,31 @@ 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 (LastInputVersion) { + vlog("Rebuilding invalidated preamble for {0} version {1} (previous was " + "version {2})", + FileName, Inputs.Version, *LastInputVersion); + } else { + vlog("Building first preamble for {0} version {1}", FileName, + Inputs.Version); + } + LastInputVersion = 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); }); - { - std::lock_guard Lock(Mutex); - // LatestBuild might be the last reference to old preamble, do not trigger - // destructor while holding the lock. - std::swap(LatestBuild, Preamble); - } + // FIXME: Invalidate NextReq, if this Preamble is compatible with that. + Req.PC(std::move(Req.CI), std::move(Req.Inputs), std::move(Preamble)); } mutable std::mutex Mutex; @@ -348,14 +375,14 @@ llvm::Optional CurrentReq; /* GUARDED_BY(Mutex) */ // Signaled whenever a thread populates NextReq or worker thread builds a // Preamble. - mutable std::condition_variable ReqCV; /* GUARDED_BY(Mutex) */ - std::shared_ptr LatestBuild; /* GUARDED_BY(Mutex) */ + mutable std::condition_variable ReqCV; /* GUARDED_BY(Mutex) */ - Notification BuiltFirst; const Path FileName; ParsingCallbacks &Callbacks; const bool StoreInMemory; const bool RunSync; + // Used only for logging. + llvm::Optional LastInputVersion; SynchronizedTUStatus &Status; }; @@ -418,6 +445,13 @@ bool isASTCached() const; private: + /// Returns the latest built preamble, might be null if no preamble has been + /// built or latest attempt resulted in a failure. + std::shared_ptr latestPreamble() const { + std::lock_guard Lock(Mutex); + return LatestPreamble; + } + // Must be called exactly once on processing thread. Will return after // stop() is called on a separate thread and all pending requests are // processed. @@ -461,6 +495,9 @@ const GlobalCompilationDatabase &CDB; /// Callback invoked when preamble or main file AST is built. ParsingCallbacks &Callbacks; + /// Latest build preamble for current TU. + std::shared_ptr LatestPreamble; + Notification BuiltFirstPreamble; Semaphore &Barrier; /// Whether the 'onMainAST' callback ran for the current FileInputs. @@ -602,10 +639,9 @@ else // FIXME: consider using old command if it's not a fallback one. Inputs.CompileCommand = CDB.getFallbackCommand(FileName); - auto PrevInputs = getCurrentFileInputs(); // Will be used to check if we can avoid rebuilding the AST. bool InputsAreTheSame = - std::tie(PrevInputs->CompileCommand, PrevInputs->Contents) == + std::tie(FileInputs->CompileCommand, FileInputs->Contents) == std::tie(Inputs.CompileCommand, Inputs.Contents); bool RanCallbackForPrevInputs = RanASTCallback; @@ -623,14 +659,6 @@ 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, " ")); @@ -643,31 +671,167 @@ // Report the diagnostics we collected when parsing the command line. Callbacks.onFailedAST(FileName, Inputs.Version, std::move(CompilerInvocationDiags), RunPublish); + // Make sure anyone waiting for the preamble gets notified it could not be + // built. + BuiltFirstPreamble.notify(); return; } + // Task that builds an AST and runs callbacks. Must be run with a preamble + // that's compatible with Inputs. Might be run immediately or in future, + // after a compatible preamble becomes ready. The built AST will only be + // cached if it is compatible with latest inputs of the ASTWorker, which + // might not be the case in case of deferred builds. + auto ASTTask = [this, WantDiags, + CIDiags = std::move(CompilerInvocationDiags), + TaskName = std::move(TaskName), + RunPublish = std::move(RunPublish)]( + std::shared_ptr Preamble, + std::unique_ptr Invocation, + ParseInputs Inputs) mutable { + if (Preamble != LatestPreamble) { + std::lock_guard Lock(Mutex); + // LatestPreamble might be the last reference to old preamble, do not + // trigger destructor while holding the lock. + std::swap(LatestPreamble, Preamble); + BuiltFirstPreamble.notify(); + } + // We only need to build the AST if diagnostics were requested. + if (WantDiags == WantDiagnostics::No) + return; + { + std::lock_guard Lock(PublishMu); + // No need to rebuild the AST if we won't send the diagnostics. + if (!CanPublishResults) + return; + } + // Give up our ownership to old preamble before starting expensive AST + // build. + Preamble.reset(); + // Used to check whether we can update AST cache and callback status. + bool InputsAreTheSame = + std::tie(FileInputs->CompileCommand, FileInputs->Contents) == + std::tie(Inputs.CompileCommand, Inputs.Contents); - 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(); - Status.update([&](TUStatus &Status) { - Status.ASTActivity.K = ASTAction::Building; - Status.ASTActivity.Name = std::move(TaskName); - }); - if (!CanReuseAST) { - IdleASTs.take(this); // Remove the old AST if it's still in cache. + Status.update([&](TUStatus &Status) { + Status.ASTActivity.K = ASTAction::Building; + Status.ASTActivity.Name = std::move(TaskName); + }); + // Get the AST for diagnostics. + llvm::Optional> AST = IdleASTs.take(this); + llvm::Optional RebuildDuration; + if (!AST) { + auto RebuildStartTime = DebouncePolicy::clock::now(); + llvm::Optional NewAST = buildAST( + FileName, std::move(Invocation), CIDiags, Inputs, LatestPreamble); + RebuildDuration = DebouncePolicy::clock::now() - RebuildStartTime; + // buildAST fails. + Status.update([&](TUStatus &Status) { + Status.Details.ReuseAST = false; + Status.Details.BuildFailed = !NewAST.hasValue(); + }); + AST = + NewAST ? std::make_unique(std::move(*NewAST)) : nullptr; + } else { + // We are reusing the AST. + Status.update([](TUStatus &Status) { + Status.Details.ReuseAST = true; + Status.Details.BuildFailed = false; + }); + } + // We want to report the diagnostics even if this update was cancelled. It + // seems more useful than making the clients wait indefinitely if they + // spam us with updates. Note *AST can still be null if buildAST fails. + if (*AST) { + { + // Try to record the AST-build time, to inform future update + // debouncing. This is best-effort only: if the lock is held, don't + // bother. + std::unique_lock Lock(Mutex, std::try_to_lock); + if (RebuildDuration && Lock.owns_lock()) { + // Do not let RebuildTimes grow beyond its small-size (i.e. + // capacity). + if (RebuildTimes.size() == RebuildTimes.capacity()) + RebuildTimes.erase(RebuildTimes.begin()); + RebuildTimes.push_back(*RebuildDuration); + } + } + trace::Span Span("Running main AST callback"); + Callbacks.onMainAST(FileName, **AST, RunPublish); + // RanASTCallback might be referring to a different FileInputs in case + // of deferred builds, only update if that's not the case. + if (InputsAreTheSame) + RanASTCallback = true; + } else { + // Failed to build the AST, at least report diagnostics from the + // command line if there were any. + // FIXME: we might have got more errors while trying to build the + // AST, surface them too. + Callbacks.onFailedAST(FileName, Inputs.Version, CIDiags, RunPublish); + } + // Stash the AST in the cache for further use iff it is compatible with + // current ASTWorker inputs. + if (InputsAreTheSame) + IdleASTs.put(this, std::move(*AST)); + }; + + bool PreambleFresh = + !Inputs.ForceRebuild && LatestPreamble && + isPreambleUsable(*LatestPreamble, Inputs, FileName, *Invocation); + // Do not build AST with stale preambles, as diagnostics might contain false + // positives and symbol information might be misleading. Instead defer the + // build until a compatible preamble is ready. It might be overwritten by a + // subsequent update. + if (!PreambleFresh) { + // Preamble is not compatible with current inputs. + IdleASTs.take(this); + PW.update( + *Invocation, Inputs, + [this, ASTTask = std::move(ASTTask)]( + std::unique_ptr CI, ParseInputs PI, + std::shared_ptr Preamble) { + std::string TaskName = llvm::formatv("Build AST ({0})", PI.Version); + // Build golden AST with corresponding inputs. + auto DeferredTask = [Preamble = std::move(Preamble), + CI = std::move(CI), PI = std::move(PI), + ASTTask = std::move(ASTTask)]() mutable { + ASTTask(std::move(Preamble), std::move(CI), std::move(PI)); + }; + if (RunSync) { + DeferredTask(); + return; + } + { + std::lock_guard Lock(Mutex); + // We issue the update with a WantDiagnostics::Yes to make sure it + // doesn't get debounced and put it in front, as this update was + // issued before any requests in the queue. + Requests.push_front( + {std::move(DeferredTask), std::move(TaskName), + steady_clock::now(), Context::current().clone(), + WantDiagnostics::Yes, TUScheduler::NoInvalidation, nullptr}); + } + RequestsCV.notify_all(); + }); + // FIXME: We should build patched-up ASTs in case of stale preambles + // instead of blocking. + PW.blockUntilIdle(Deadline::infinity()); + return; + } + vlog("Reusing preamble version {0} for version {1} of {2}", + LatestPreamble->Version, Inputs.Version, FileName); + if (!InputsAreTheSame) { + // Cannot use the cached AST if inputs have changed. + IdleASTs.take(this); } else { - // We don't need to rebuild the AST, check if we need to run the callback. + // Take a shortcut and don't report the diagnostics, since they should be + // the same. All the clients should handle the lack of OnUpdated() call + // anyway to handle empty result from buildAST. + // FIXME: the AST could actually change if non-preamble includes changed, + // but we choose to ignore it. + // FIXME: should we refresh the cache in IdleASTs for the current file at + // this point? if (RanCallbackForPrevInputs) { RanASTCallback = true; - // Take a shortcut and don't report the diagnostics, since they should - // not changed. All the clients should handle the lack of OnUpdated() - // call anyway to handle empty result from buildAST. - // FIXME(ibiryukov): the AST could actually change if non-preamble - // includes changed, but we choose to ignore it. - // FIXME(ibiryukov): should we refresh the cache in IdleASTs for the - // current file at this point? log("Skipping rebuild of the AST for {0}, inputs are the same.", FileName); @@ -678,71 +842,7 @@ return; } } - - // We only need to build the AST if diagnostics were requested. - if (WantDiags == WantDiagnostics::No) - return; - - { - std::lock_guard Lock(PublishMu); - // No need to rebuild the AST if we won't send the diagnostics. However, - // note that we don't prevent preamble rebuilds. - if (!CanPublishResults) - return; - } - - // Get the AST for diagnostics. - llvm::Optional> AST = IdleASTs.take(this); - auto RebuildStartTime = DebouncePolicy::clock::now(); - if (!AST) { - llvm::Optional NewAST = - buildAST(FileName, std::move(Invocation), CompilerInvocationDiags, - Inputs, NewPreamble); - // buildAST fails. - Status.update([&](TUStatus &Status) { - Status.Details.ReuseAST = false; - Status.Details.BuildFailed = !NewAST.hasValue(); - }); - AST = NewAST ? std::make_unique(std::move(*NewAST)) : nullptr; - } else { - // We are reusing the AST. - Status.update([](TUStatus &Status) { - Status.Details.ReuseAST = true; - Status.Details.BuildFailed = false; - }); - } - - // We want to report the diagnostics even if this update was cancelled. - // It seems more useful than making the clients wait indefinitely if they - // spam us with updates. - // Note *AST can still be null if buildAST fails. - if (*AST) { - { - // Try to record the AST-build time, to inform future update debouncing. - // This is best-effort only: if the lock is held, don't bother. - auto RebuildDuration = DebouncePolicy::clock::now() - RebuildStartTime; - std::unique_lock Lock(Mutex, std::try_to_lock); - if (Lock.owns_lock()) { - // Do not let RebuildTimes grow beyond its small-size (i.e. capacity). - if (RebuildTimes.size() == RebuildTimes.capacity()) - RebuildTimes.erase(RebuildTimes.begin()); - RebuildTimes.push_back(RebuildDuration); - } - } - trace::Span Span("Running main AST callback"); - - Callbacks.onMainAST(FileName, **AST, RunPublish); - RanASTCallback = true; - } else { - // Failed to build the AST, at least report diagnostics from the command - // line if there were any. - // FIXME: we might have got more errors while trying to build the AST, - // surface them too. - Callbacks.onFailedAST(FileName, Inputs.Version, CompilerInvocationDiags, - RunPublish); - } - // Stash the AST in the cache for further use. - IdleASTs.put(this, std::move(*AST)); + ASTTask(LatestPreamble, std::move(Invocation), std::move(Inputs)); }; startTask(TaskName, std::move(Task), WantDiags, TUScheduler::NoInvalidation); } @@ -788,7 +888,8 @@ std::shared_ptr ASTWorker::getPossiblyStalePreamble() const { - return PW.latest(); + std::lock_guard Lock(Mutex); + return LatestPreamble; } void ASTWorker::getCurrentPreamble( @@ -821,7 +922,7 @@ RequestsCV.notify_all(); } -void ASTWorker::waitForFirstPreamble() const { PW.waitForFirst(); } +void ASTWorker::waitForFirstPreamble() const { BuiltFirstPreamble.wait(); } std::shared_ptr ASTWorker::getCurrentFileInputs() const { std::unique_lock Lock(Mutex); @@ -855,6 +956,9 @@ assert(!Done && "stop() called twice"); Done = true; } + // We are no longer going to build any preambles, let the waiters know that. + BuiltFirstPreamble.notify(); + PW.stop(); Status.stop(); RequestsCV.notify_all(); } @@ -898,7 +1002,6 @@ } void ASTWorker::run() { - auto _ = llvm::make_scope_exit([this] { PW.stop(); }); while (true) { { std::unique_lock Lock(Mutex); 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 @@ -283,6 +283,7 @@ S.runWithPreamble("StaleRead", Path, TUScheduler::Stale, [&](Expected Pre) { ASSERT_TRUE(bool(Pre)); + ASSERT_TRUE(Pre->Preamble); EXPECT_EQ(Pre->Preamble->Version, "A"); EXPECT_THAT(includes(Pre->Preamble), ElementsAre("")); @@ -292,11 +293,13 @@ S.runWithPreamble("ConsistentRead", Path, TUScheduler::Consistent, [&](Expected Pre) { ASSERT_TRUE(bool(Pre)); + ASSERT_TRUE(Pre->Preamble); EXPECT_EQ(Pre->Preamble->Version, "B"); EXPECT_THAT(includes(Pre->Preamble), ElementsAre("")); ++CallbackCount; }); + S.blockUntilIdle(timeoutSeconds(10)); } EXPECT_EQ(2, CallbackCount); } @@ -853,15 +856,19 @@ TUState(PreambleAction::Idle, ASTAction::RunningAction), // We build the preamble TUState(PreambleAction::Building, ASTAction::RunningAction), - // Preamble worker goes idle + // We built the preamble, and issued ast built on ASTWorker + // thread. Preambleworker goes idle afterwards. TUState(PreambleAction::Idle, ASTAction::RunningAction), - // We start building the ast + // Start task for building the ast, as a result of building + // preamble, on astworker thread. + TUState(PreambleAction::Idle, ASTAction::RunningAction), + // AST build starts. TUState(PreambleAction::Idle, ASTAction::Building), - // Built finished succesffully + // AST built finished successfully TUState(PreambleAction::Idle, ASTAction::Building), - // Rnning go to def + // Running go to def TUState(PreambleAction::Idle, ASTAction::RunningAction), - // both workers go idle + // ASTWorker goes 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);