diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp --- a/clang-tools-extra/clangd/ClangdServer.cpp +++ b/clang-tools-extra/clangd/ClangdServer.cpp @@ -14,6 +14,7 @@ #include "FormattedString.h" #include "Headers.h" #include "Protocol.h" +#include "SemanticHighlighting.h" #include "SourceCode.h" #include "TUScheduler.h" #include "Trace.h" @@ -31,6 +32,7 @@ #include "clang/Tooling/Core/Replacement.h" #include "llvm/ADT/ArrayRef.h" #include "llvm/ADT/Optional.h" +#include "llvm/ADT/STLExtras.h" #include "llvm/ADT/ScopeExit.h" #include "llvm/ADT/StringRef.h" #include "llvm/Support/Errc.h" @@ -60,15 +62,20 @@ FIndex->updatePreamble(Path, Ctx, std::move(PP), CanonIncludes); } - void onMainAST(PathRef Path, ParsedAST &AST) override { + void onMainAST(PathRef Path, ParsedAST &AST, PublishFn Publish) override { if (FIndex) FIndex->updateMain(Path, AST); + + std::vector Diagnostics = AST.getDiagnostics(); + std::vector Highlightings; if (SemanticHighlighting) - DiagConsumer.onHighlightingsReady(Path, getSemanticHighlightings(AST)); - } + Highlightings = getSemanticHighlightings(AST); - void onDiagnostics(PathRef File, std::vector Diags) override { - DiagConsumer.onDiagnosticsReady(File, std::move(Diags)); + Publish([&]() { + DiagConsumer.onDiagnosticsReady(Path, std::move(Diagnostics)); + if (SemanticHighlighting) + DiagConsumer.onHighlightingsReady(Path, std::move(Highlightings)); + }); } void onFileUpdated(PathRef File, const TUStatus &Status) override { 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 @@ -15,6 +15,7 @@ #include "Threading.h" #include "index/CanonicalIncludes.h" #include "llvm/ADT/Optional.h" +#include "llvm/ADT/STLExtras.h" #include "llvm/ADT/StringMap.h" #include "llvm/ADT/StringRef.h" #include @@ -98,6 +99,10 @@ virtual void onPreambleAST(PathRef Path, ASTContext &Ctx, std::shared_ptr PP, const CanonicalIncludes &) {} + + /// The argument function is run under the critical section guarding against + /// races when closing the files. + using PublishFn = llvm::function_ref)>; /// Called on the AST built for the file itself. Note that preamble AST nodes /// are not deserialized and should be processed in the onPreambleAST call /// instead. @@ -108,10 +113,17 @@ /// etc. Clients are expected to process only the AST nodes from the main file /// in this callback (obtained via ParsedAST::getLocalTopLevelDecls) to obtain /// optimal performance. - virtual void onMainAST(PathRef Path, ParsedAST &AST) {} - - /// Called whenever the diagnostics for \p File are produced. - virtual void onDiagnostics(PathRef File, std::vector Diags) {} + /// + /// When information about the file (diagnostics, syntax highlighting) is + /// published to clients, this should be wrapped in Publish, e.g. + /// void onMainAST(...) { + /// Highlights = computeHighlights(); + /// Publish([&] { notifyHighlights(Path, Highlights); }); + /// } + /// This guarantees that clients will see results in the correct sequence if + /// the file is concurrently closed and/or reopened. (The lambda passed to + /// Publish() may never run in this case). + virtual void onMainAST(PathRef Path, ParsedAST &AST, PublishFn Publish) {} /// Called whenever the TU status is updated. virtual void onFileUpdated(PathRef File, const TUStatus &Status) {} 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 @@ -249,9 +249,8 @@ TUStatus Status; Semaphore &Barrier; - /// Whether the diagnostics for the current FileInputs were reported to the - /// users before. - bool DiagsWereReported = false; + /// Whether the 'onMainAST' callback ran for the current FileInputs. + bool RanASTCallback = false; /// Guards members used by both TUScheduler and the worker thread. mutable std::mutex Mutex; /// File inputs, currently being used by the worker. @@ -265,15 +264,16 @@ bool Done; /* GUARDED_BY(Mutex) */ std::deque Requests; /* GUARDED_BY(Mutex) */ mutable std::condition_variable RequestsCV; - // FIXME: rename it to better fix the current usage, we also use it to guard - // emitting TUStatus. - /// Guards a critical section for running the diagnostics callbacks. - std::mutex DiagsMu; - // Used to prevent remove document + leading to out-of-order diagnostics: + /// Guards the callback that publishes results of AST-related computations + /// (diagnostics, highlightings) and file statuses. + std::mutex PublishMu; + // Used to prevent remove document + add document races that lead to + // out-of-order callbacks for publishing results of onMainAST callback. + // // The lifetime of the old/new ASTWorkers will overlap, but their handles // don't. When the old handle is destroyed, the old worker will stop reporting - // diagnostics. - bool ReportDiagnostics = true; /* GUARDED_BY(DiagsMu) */ + // any results to the user. + bool CanPublishResults = true; /* GUARDED_BY(PublishMu) */ }; /// A smart-pointer-like class that points to an active ASTWorker. @@ -381,12 +381,12 @@ std::tie(Inputs.CompileCommand, Inputs.Contents); tooling::CompileCommand OldCommand = PrevInputs->CompileCommand; - bool PrevDiagsWereReported = DiagsWereReported; + bool RanCallbackForPrevInputs = RanASTCallback; { std::lock_guard Lock(Mutex); FileInputs = std::make_shared(Inputs); } - DiagsWereReported = false; + RanASTCallback = false; emitTUStatus({TUAction::BuildingPreamble, TaskName}); log("Updating file {0} with command {1}\n[{2}]\n{3}", FileName, Inputs.CompileCommand.Heuristic, @@ -432,10 +432,9 @@ if (!CanReuseAST) { IdleASTs.take(this); // Remove the old AST if it's still in cache. } else { - // Since we don't need to rebuild the AST, we might've already reported - // the diagnostics for it. - if (PrevDiagsWereReported) { - DiagsWereReported = true; + // We don't need to rebuild the AST, check if we need to run the callback. + 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. @@ -457,10 +456,10 @@ return; { - std::lock_guard Lock(DiagsMu); + std::lock_guard Lock(PublishMu); // No need to rebuild the AST if we won't send the diagnotics. However, // note that we don't prevent preamble rebuilds. - if (!ReportDiagnostics) + if (!CanPublishResults) return; } @@ -486,14 +485,17 @@ // spam us with updates. // Note *AST can still be null if buildAST fails. if (*AST) { - { - std::lock_guard Lock(DiagsMu); - if (ReportDiagnostics) - Callbacks.onDiagnostics(FileName, (*AST)->getDiagnostics()); - } trace::Span Span("Running main AST callback"); - Callbacks.onMainAST(FileName, **AST); - DiagsWereReported = true; + auto RunPublish = [&](llvm::function_ref Publish) { + // Ensure we only publish results from the worker if the file was not + // removed, making sure there are not race conditions. + std::lock_guard Lock(PublishMu); + if (CanPublishResults) + Publish(); + }; + + Callbacks.onMainAST(FileName, **AST, RunPublish); + RanASTCallback = true; } // Stash the AST in the cache for further use. IdleASTs.put(this, std::move(*AST)); @@ -594,8 +596,8 @@ void ASTWorker::stop() { { - std::lock_guard Lock(DiagsMu); - ReportDiagnostics = false; + std::lock_guard Lock(PublishMu); + CanPublishResults = false; } { std::lock_guard Lock(Mutex); @@ -630,9 +632,9 @@ Status.Action = std::move(Action); if (Details) Status.Details = *Details; - std::lock_guard Lock(DiagsMu); + std::lock_guard Lock(PublishMu); // Do not emit TU statuses when the ASTWorker is shutting down. - if (ReportDiagnostics) { + if (CanPublishResults) { Callbacks.onFileUpdated(FileName, Status); } } 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 @@ -7,10 +7,14 @@ //===----------------------------------------------------------------------===// #include "Annotations.h" +#include "ClangdUnit.h" #include "Context.h" +#include "Diagnostics.h" #include "Matchers.h" +#include "Path.h" #include "TUScheduler.h" #include "TestFS.h" +#include "llvm/ADT/STLExtras.h" #include "llvm/ADT/ScopeExit.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -56,12 +60,17 @@ /// in updateWithDiags. static std::unique_ptr captureDiags() { class CaptureDiags : public ParsingCallbacks { - void onDiagnostics(PathRef File, std::vector Diags) override { + void onMainAST(PathRef File, ParsedAST &AST, PublishFn Publish) override { + auto Diags = AST.getDiagnostics(); auto D = Context::current().get(DiagsCallbackKey); if (!D) return; - const_cast)> &> ( - *D)(File, Diags); + + Publish([&]() { + const_cast< + llvm::unique_function)> &> (*D)( + File, std::move(Diags)); + }); } }; return llvm::make_unique(); @@ -116,8 +125,8 @@ S.update(Added, getInputs(Added, "x"), WantDiagnostics::No); EXPECT_EQ(S.getContents(Added), "x"); - // Assert each operation for missing file is an error (even if it's available - // in VFS). + // Assert each operation for missing file is an error (even if it's + // available in VFS). S.runWithAST("", Missing, [&](Expected AST) { EXPECT_ERROR(AST); }); S.runWithPreamble( @@ -367,8 +376,8 @@ StringRef AllContents[] = {Contents1, Contents2, Contents3}; const int AllContentsSize = 3; - // Scheduler may run tasks asynchronously, but should propagate the context. - // We stash a nonce in the context, and verify it in the task. + // Scheduler may run tasks asynchronously, but should propagate the + // context. We stash a nonce in the context, and verify it in the task. static Key NonceKey; int Nonce = 0; @@ -465,8 +474,8 @@ ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10))); ASSERT_EQ(BuiltASTCounter.load(), 1); - // Build two more files. Since we can retain only 2 ASTs, these should be the - // ones we see in the cache later. + // Build two more files. Since we can retain only 2 ASTs, these should be + // the ones we see in the cache later. updateWithCallback(S, Bar, SourceContents, WantDiagnostics::Yes, [&BuiltASTCounter]() { ++BuiltASTCounter; }); updateWithCallback(S, Baz, SourceContents, WantDiagnostics::Yes,