diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp b/clang-tools-extra/clangd/ClangdLSPServer.cpp --- a/clang-tools-extra/clangd/ClangdLSPServer.cpp +++ b/clang-tools-extra/clangd/ClangdLSPServer.cpp @@ -1296,7 +1296,8 @@ // clang-format on } -ClangdLSPServer::~ClangdLSPServer() { IsBeingDestroyed = true; +ClangdLSPServer::~ClangdLSPServer() { + IsBeingDestroyed = true; // Explicitly destroy ClangdServer first, blocking on threads it owns. // This ensures they don't access any other members. Server.reset(); @@ -1484,8 +1485,9 @@ // two statuses are running faster in practice, which leads the UI constantly // changing, and doesn't provide much value. We may want to emit status at a // reasonable time interval (e.g. 0.5s). - if (Status.Action.S == TUAction::BuildingFile || - Status.Action.S == TUAction::RunningAction) + if (Status.PreambleAction.S == TUAction::Idle && + (Status.ASTAction.S == TUAction::Building || + Status.ASTAction.S == TUAction::RunningAction)) return; notify("textDocument/clangd.fileStatus", Status.render(File)); } diff --git a/clang-tools-extra/clangd/TUScheduler.h b/clang-tools-extra/clangd/TUScheduler.h --- a/clang-tools-extra/clangd/TUScheduler.h +++ b/clang-tools-extra/clangd/TUScheduler.h @@ -85,16 +85,15 @@ struct TUAction { enum State { - Queued, // The TU is pending in the thread task queue to be built. - RunningAction, // Starting running actions on the TU. - BuildingPreamble, // The preamble of the TU is being built. - BuildingFile, // The TU is being built. It is only emitted when building - // the AST for diagnostics in write action (update). + Queued, // The TU is pending in the thread task queue to be built. + RunningAction, // Starting running actions on the TU. + Building, // The TU is being built. Idle, // Indicates the worker thread is idle, and ready to run any upcoming // actions. }; + TUAction() = default; TUAction(State S, llvm::StringRef Name) : S(S), Name(Name) {} - State S; + State S = TUAction::Idle; /// The name of the action currently running, e.g. Update, GoToDef, Hover. /// Empty if we are in the idle state. std::string Name; @@ -102,17 +101,23 @@ // Internal status of the TU in TUScheduler. struct TUStatus { - struct BuildDetails { - /// Indicates whether clang failed to build the TU. - bool BuildFailed = false; - /// Indicates whether we reused the prebuilt AST. - bool ReuseAST = false; + enum BuildStatus { + /// Haven't run a built yet + None, + /// Indicates clang failed to build the TU. + BuildFailed, + /// Indicates we reused the prebuilt AST. + ReuseAST, + /// Indicated TU was successfuly built. + Built, }; /// Serialize this to an LSP file status item. FileStatus render(PathRef File) const; - TUAction Action; - BuildDetails Details; + TUAction PreambleAction; + TUAction ASTAction; + /// Stores status of the last build for the translation unit. + BuildStatus BS = None; }; class ParsingCallbacks { 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 @@ -58,14 +58,18 @@ #include "clang/Tooling/CompilationDatabase.h" #include "llvm/ADT/Optional.h" #include "llvm/ADT/ScopeExit.h" +#include "llvm/ADT/SmallVector.h" +#include "llvm/ADT/StringExtras.h" #include "llvm/ADT/StringRef.h" #include "llvm/Support/Errc.h" +#include "llvm/Support/ErrorHandling.h" #include "llvm/Support/Path.h" #include "llvm/Support/Threading.h" #include #include #include #include +#include #include namespace clang { @@ -152,6 +156,51 @@ }; namespace { +/// Threadsafe manager for updating a TUStatus and emitting it after each +/// update. +class TUStatusManager { +public: + TUStatusManager(PathRef FileName, ParsingCallbacks &Callbacks) + : FileName(FileName), Callbacks(Callbacks) {} + + void setPreambleAction(TUAction A) { + std::lock_guard Lock(StatusMu); + Status.PreambleAction = std::move(A); + emitStatusLocked(); + } + + void setASTAction(TUAction A) { + std::lock_guard Lock(StatusMu); + Status.ASTAction = std::move(A); + emitStatusLocked(); + } + + void setBuildStatus(TUStatus::BuildStatus BS) { + std::lock_guard Lock(StatusMu); + Status.BS = std::move(BS); + emitStatusLocked(); + } + + /// Prevents emitting of further updates. + void stop() { + std::lock_guard Lock(StatusMu); + CanPublish = false; + } + +private: + void emitStatusLocked() { + if (CanPublish) + Callbacks.onFileUpdated(FileName, Status); + } + + const Path FileName; + + mutable std::mutex StatusMu; + TUStatus Status; + bool CanPublish = true; + ParsingCallbacks &Callbacks; +}; + /// Responsible for building and providing access to the preamble of a TU. /// Processes async build requests on a dedicated thread executing run method. /// Unless RunSync is provided, which will build preambles on the requesting @@ -159,9 +208,11 @@ class PreambleThread { public: PreambleThread(llvm::StringRef FileName, ParsingCallbacks &Callbacks, - bool StorePreambleInMemory, bool RunSync) + bool StorePreambleInMemory, bool RunSync, + TUStatusManager &StatusManager) : FileName(FileName), Callbacks(Callbacks), - StoreInMemory(StorePreambleInMemory), RunSync(RunSync) {} + StoreInMemory(StorePreambleInMemory), RunSync(RunSync), + StatusManager(StatusManager) {} size_t getUsedBytes() const { auto Preamble = latest(); @@ -174,10 +225,6 @@ void update(CompilerInvocation *CI, ParseInputs PI) { // If compiler invocation was broken, just fail out early. if (!CI) { - TUStatus::BuildDetails Details; - Details.BuildFailed = true; - std::string TaskName = llvm::formatv("Update ({0})", PI.Version); - emitTUStatus({TUAction::BuildingPreamble, std::move(TaskName)}, &Details); // Make sure anyone waiting for the preamble gets notified it could not be // built. BuiltFirst.notify(); @@ -227,6 +274,16 @@ } // Build the preamble and let the waiters know about it. build(std::move(*CurrentReq)); + bool IsEmpty = false; + { + std::lock_guard Lock(Mutex); + CurrentReq.reset(); + IsEmpty = !NextReq.hasValue(); + } + if (IsEmpty) + StatusManager.setPreambleAction({TUAction::Idle, /*Name=*/""}); + BuiltFirst.notify(); + ReqCV.notify_all(); } // We are no longer going to build any preambles, let the waiters know that. BuiltFirst.notify(); @@ -262,18 +319,6 @@ return Done; } - /// Updates the TUStatus and emits it. Only called in the worker thread. - void emitTUStatus(TUAction Action, - const TUStatus::BuildDetails *Details = nullptr) { - // Do not emit TU statuses when the worker is shutting down. - if (isDone()) - return; - TUStatus Status({std::move(Action), {}}); - if (Details) - Status.Details = *Details; - Callbacks.onFileUpdated(FileName, Status); - } - /// Builds a preamble for Req and caches it. Might re-use the latest built /// preamble if it is valid for Req. Also signals waiters about the build. /// FIXME: We shouldn't cache failed preambles, if we've got a successful @@ -285,7 +330,7 @@ Inputs.ForceRebuild ? nullptr : latest(); std::string TaskName = llvm::formatv("Update ({0})", Inputs.Version); - emitTUStatus({TUAction::BuildingPreamble, std::move(TaskName)}); + StatusManager.setPreambleAction({TUAction::Building, TaskName}); auto Preamble = clang::clangd::buildPreamble( FileName, std::move(*Req.CI), OldPreamble, Inputs, StoreInMemory, @@ -298,10 +343,7 @@ { std::lock_guard Lock(Mutex); LatestBuild = std::move(Preamble); - CurrentReq.reset(); } - BuiltFirst.notify(); - ReqCV.notify_all(); } mutable std::mutex Mutex; @@ -320,6 +362,8 @@ ParsingCallbacks &Callbacks; const bool StoreInMemory; const bool RunSync; + + TUStatusManager &StatusManager; }; class ASTWorkerHandle; @@ -390,9 +434,6 @@ void startTask(llvm::StringRef Name, llvm::unique_function Task, llvm::Optional UpdateType, TUScheduler::ASTActionInvalidation); - /// Updates the TUStatus and emits it. Only called in the worker thread. - void emitTUStatus(TUAction FAction, - const TUStatus::BuildDetails *Detail = nullptr); /// Determines the next action to perform. /// All actions that should never run are discarded. @@ -426,8 +467,6 @@ const GlobalCompilationDatabase &CDB; /// Callback invoked when preamble or main file AST is built. ParsingCallbacks &Callbacks; - /// Only accessed by the worker thread. - TUStatus Status; Semaphore &Barrier; /// Whether the 'onMainAST' callback ran for the current FileInputs. @@ -457,6 +496,7 @@ // any results to the user. bool CanPublishResults = true; /* GUARDED_BY(PublishMu) */ + TUStatusManager StatusManager; PreambleThread PW; }; @@ -525,11 +565,9 @@ bool RunSync, DebouncePolicy UpdateDebounce, bool StorePreamblesInMemory, ParsingCallbacks &Callbacks) : IdleASTs(LRUCache), RunSync(RunSync), UpdateDebounce(UpdateDebounce), - FileName(FileName), CDB(CDB), - Callbacks(Callbacks), Status{TUAction(TUAction::Idle, ""), - TUStatus::BuildDetails()}, - Barrier(Barrier), Done(false), - PW(FileName, Callbacks, StorePreamblesInMemory, RunSync) { + FileName(FileName), CDB(CDB), Callbacks(Callbacks), Barrier(Barrier), + Done(false), StatusManager(FileName, Callbacks), + PW(FileName, Callbacks, StorePreamblesInMemory, RunSync, StatusManager) { auto Inputs = std::make_shared(); // Set a fallback command because compile command can be accessed before // `Inputs` is initialized. Other fields are only used after initialization @@ -616,7 +654,7 @@ // to the old preamble, so it can be freed if there are no other references // to it. OldPreamble.reset(); - emitTUStatus({TUAction::BuildingFile, TaskName}); + StatusManager.setASTAction({TUAction::Building, TaskName}); if (!CanReuseAST) { IdleASTs.take(this); // Remove the old AST if it's still in cache. } else { @@ -632,9 +670,8 @@ // current file at this point? log("Skipping rebuild of the AST for {0}, inputs are the same.", FileName); - TUStatus::BuildDetails Details; - Details.ReuseAST = true; - emitTUStatus({TUAction::BuildingFile, TaskName}, &Details); + + StatusManager.setBuildStatus(TUStatus::ReuseAST); return; } } @@ -659,16 +696,14 @@ buildAST(FileName, std::move(Invocation), CompilerInvocationDiags, Inputs, NewPreamble); AST = NewAST ? std::make_unique(std::move(*NewAST)) : nullptr; - if (!(*AST)) { // buildAST fails. - TUStatus::BuildDetails Details; - Details.BuildFailed = true; - emitTUStatus({TUAction::BuildingFile, TaskName}, &Details); - } + // buildAST fails. + if (!(*AST)) + StatusManager.setBuildStatus(TUStatus::BuildFailed); + else + StatusManager.setBuildStatus(TUStatus::Built); } else { // We are reusing the AST. - TUStatus::BuildDetails Details; - Details.ReuseAST = true; - emitTUStatus({TUAction::BuildingFile, TaskName}, &Details); + StatusManager.setBuildStatus(TUStatus::ReuseAST); } // We want to report the diagnostics even if this update was cancelled. @@ -814,6 +849,7 @@ assert(!Done && "stop() called twice"); Done = true; } + StatusManager.stop(); RequestsCV.notify_all(); } @@ -855,18 +891,6 @@ RequestsCV.notify_all(); } -void ASTWorker::emitTUStatus(TUAction Action, - const TUStatus::BuildDetails *Details) { - Status.Action = std::move(Action); - if (Details) - Status.Details = *Details; - std::lock_guard Lock(PublishMu); - // Do not emit TU statuses when the ASTWorker is shutting down. - if (CanPublishResults) { - Callbacks.onFileUpdated(FileName, Status); - } -} - void ASTWorker::run() { auto _ = llvm::make_scope_exit([this] { PW.stop(); }); while (true) { @@ -890,7 +914,8 @@ Tracer.emplace("Debounce"); SPAN_ATTACH(*Tracer, "next_request", Requests.front().Name); if (!(Wait == Deadline::infinity())) { - emitTUStatus({TUAction::Queued, Requests.front().Name}); + StatusManager.setASTAction( + {TUAction::Queued, Requests.front().Name}); SPAN_ATTACH(*Tracer, "sleep_ms", std::chrono::duration_cast( Wait.time() - steady_clock::now()) @@ -909,12 +934,13 @@ { std::unique_lock Lock(Barrier, std::try_to_lock); if (!Lock.owns_lock()) { - emitTUStatus({TUAction::Queued, CurrentRequest->Name}); + StatusManager.setASTAction({TUAction::Queued, CurrentRequest->Name}); Lock.lock(); } WithContext Guard(std::move(CurrentRequest->Ctx)); trace::Span Tracer(CurrentRequest->Name); - emitTUStatus({TUAction::RunningAction, CurrentRequest->Name}); + StatusManager.setASTAction( + {TUAction::RunningAction, CurrentRequest->Name}); CurrentRequest->Action(); } @@ -925,7 +951,7 @@ IsEmpty = Requests.empty(); } if (IsEmpty) - emitTUStatus({TUAction::Idle, /*Name*/ ""}); + StatusManager.setASTAction({TUAction::Idle, /*Name*/ ""}); RequestsCV.notify_all(); } } @@ -1009,27 +1035,27 @@ // TUAction represents clangd-internal states, we don't intend to expose them // to users (say C++ programmers) directly to avoid confusion, we use terms that // are familiar by C++ programmers. -std::string renderTUAction(const TUAction &Action) { - std::string Result; - llvm::raw_string_ostream OS(Result); +std::string renderTUAction(bool BuildingPreamble, const TUAction &Action) { + llvm::SmallVector Result; + if (BuildingPreamble) + Result.push_back("parsing includes"); switch (Action.S) { case TUAction::Queued: - OS << "file is queued"; + Result.push_back("file is queued"); break; case TUAction::RunningAction: - OS << "running " << Action.Name; - break; - case TUAction::BuildingPreamble: - OS << "parsing includes"; + Result.push_back("running " + Action.Name); break; - case TUAction::BuildingFile: - OS << "parsing main file"; + case TUAction::Building: + Result.push_back("parsing main file"); break; case TUAction::Idle: - OS << "idle"; + // We handle idle specially below. break; } - return OS.str(); + if (Result.empty()) + return "idle"; + return llvm::join(Result, ","); } } // namespace @@ -1041,7 +1067,8 @@ FileStatus TUStatus::render(PathRef File) const { FileStatus FStatus; FStatus.uri = URIForFile::canonicalize(File, /*TUPath=*/File); - FStatus.state = renderTUAction(Action); + FStatus.state = + renderTUAction(PreambleAction.S == TUAction::Building, ASTAction); return FStatus; } 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 @@ -40,13 +40,17 @@ using ::testing::Pointee; using ::testing::UnorderedElementsAre; -MATCHER_P2(TUState, State, ActionName, "") { - if (arg.Action.S != State) { - *result_listener << "state is " << arg.Action.S; +MATCHER_P3(TUState, PreambleState, ASTState, BS, "") { + if (arg.PreambleAction.S != PreambleState) { + *result_listener << "preamblestate is " << arg.PreambleAction.S; return false; } - if (arg.Action.Name != ActionName) { - *result_listener << "name is " << arg.Action.Name; + if (arg.ASTAction.S != ASTState) { + *result_listener << "aststate is " << arg.ASTAction.S; + return false; + } + if (arg.BS != BS) { + *result_listener << "builddetail is " << arg.BS; return false; } return true; @@ -732,14 +736,13 @@ // Update the source contents, which should trigger an initial build with // the header file missing. - updateWithDiags(S, Source, Inputs, WantDiagnostics::Yes, - [](std::vector Diags) { - EXPECT_THAT( - Diags, - ElementsAre( - Field(&Diag::Message, "'foo.h' file not found"), - Field(&Diag::Message, "use of undeclared identifier 'a'"))); - }); + updateWithDiags( + S, Source, Inputs, WantDiagnostics::Yes, [](std::vector Diags) { + EXPECT_THAT(Diags, + ElementsAre(Field(&Diag::Message, "'foo.h' file not found"), + Field(&Diag::Message, + "use of undeclared identifier 'a'"))); + }); // Add the header file. We need to recreate the inputs since we changed a // file from underneath the test FS. @@ -749,18 +752,17 @@ // The addition of the missing header file shouldn't trigger a rebuild since // we don't track missing files. - updateWithDiags(S, Source, Inputs, WantDiagnostics::Yes, - [](std::vector Diags) { - ADD_FAILURE() << "Did not expect diagnostics for missing header update"; - }); + updateWithDiags( + S, Source, Inputs, WantDiagnostics::Yes, [](std::vector Diags) { + ADD_FAILURE() << "Did not expect diagnostics for missing header update"; + }); // Forcing the reload should should cause a rebuild which no longer has any // errors. Inputs.ForceRebuild = true; - updateWithDiags(S, Source, Inputs, WantDiagnostics::Yes, - [](std::vector Diags) { - EXPECT_THAT(Diags, IsEmpty()); - }); + updateWithDiags( + S, Source, Inputs, WantDiagnostics::Yes, + [](std::vector Diags) { EXPECT_THAT(Diags, IsEmpty()); }); ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10))); } @@ -846,16 +848,24 @@ ASSERT_TRUE(Server.blockUntilIdleForTest()); - EXPECT_THAT(CaptureTUStatus.allStatus(), - ElementsAre( - // Statuses of "Update" action. - TUState(TUAction::RunningAction, "Update (1)"), - TUState(TUAction::BuildingPreamble, "Update (1)"), - TUState(TUAction::BuildingFile, "Update (1)"), - - // Statuses of "Definitions" action - TUState(TUAction::RunningAction, "Definitions"), - TUState(TUAction::Idle, /*No action*/ ""))); + EXPECT_THAT( + CaptureTUStatus.allStatus(), + ElementsAre( + // Everything starts with ASTWorker starting to execute an + // update + TUState(TUAction::Idle, TUAction::RunningAction, TUStatus::None), + // We build the preamble + TUState(TUAction::Building, TUAction::RunningAction, TUStatus::None), + // Preamble worker goes idle + TUState(TUAction::Idle, TUAction::RunningAction, TUStatus::None), + // We start building the ast + TUState(TUAction::Idle, TUAction::Building, TUStatus::None), + // Built finished succesffully + TUState(TUAction::Idle, TUAction::Building, TUStatus::Built), + // Rnning go to def + TUState(TUAction::Idle, TUAction::RunningAction, TUStatus::Built), + // both workers go idle + TUState(TUAction::Idle, TUAction::Idle, TUStatus::Built))); } TEST_F(TUSchedulerTests, CommandLineErrors) { @@ -868,8 +878,7 @@ TUScheduler S(CDB, optsForTest(), captureDiags()); std::vector Diagnostics; updateWithDiags(S, testPath("foo.cpp"), "void test() {}", - WantDiagnostics::Yes, - [&](std::vector D) { + WantDiagnostics::Yes, [&](std::vector D) { Diagnostics = std::move(D); Ready.notify(); }); @@ -893,8 +902,7 @@ TUScheduler S(CDB, optsForTest(), captureDiags()); std::vector Diagnostics; updateWithDiags(S, testPath("foo.cpp"), "void test() {}", - WantDiagnostics::Yes, - [&](std::vector D) { + WantDiagnostics::Yes, [&](std::vector D) { Diagnostics = std::move(D); Ready.notify(); });