ilya-biryukov updated this revision to Diff 210808.
ilya-biryukov added a comment.
- Remove a leftover comment from the previous version
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64985/new/
https://reviews.llvm.org/D64985
Files:
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/TUScheduler.cpp
clang-tools-extra/clangd/TUScheduler.h
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
@@ -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<ParsingCallbacks> captureDiags() {
class CaptureDiags : public ParsingCallbacks {
- void onDiagnostics(PathRef File, std::vector<Diag> 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<llvm::unique_function<void(PathRef, std::vector<Diag>)> &> (
- *D)(File, Diags);
+
+ Publish([&]() {
+ const_cast<
+ llvm::unique_function<void(PathRef, std::vector<Diag>)> &> (*D)(
+ File, std::move(Diags));
+ });
}
};
return llvm::make_unique<CaptureDiags>();
@@ -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<InputsAndAST> 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<int> 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,
Index: clang-tools-extra/clangd/TUScheduler.h
===================================================================
--- clang-tools-extra/clangd/TUScheduler.h
+++ 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 <future>
@@ -98,6 +99,10 @@
virtual void onPreambleAST(PathRef Path, ASTContext &Ctx,
std::shared_ptr<clang::Preprocessor> PP,
const CanonicalIncludes &) {}
+ /// The argument function is run under the critical section guarding against
+ /// races when closing the files.
+ using PublishFn = llvm::function_ref<void(llvm::function_ref<void()>)>;
+
/// 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<Diag> 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) {}
Index: clang-tools-extra/clangd/TUScheduler.cpp
===================================================================
--- clang-tools-extra/clangd/TUScheduler.cpp
+++ 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<Request> 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<std::mutex> Lock(Mutex);
FileInputs = std::make_shared<ParseInputs>(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<std::mutex> Lock(DiagsMu);
+ std::lock_guard<std::mutex> 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<std::mutex> 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<void()> 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<std::mutex> 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<std::mutex> Lock(DiagsMu);
- ReportDiagnostics = false;
+ std::lock_guard<std::mutex> Lock(PublishMu);
+ CanPublishResults = false;
}
{
std::lock_guard<std::mutex> Lock(Mutex);
@@ -630,9 +632,9 @@
Status.Action = std::move(Action);
if (Details)
Status.Details = *Details;
- std::lock_guard<std::mutex> Lock(DiagsMu);
+ std::lock_guard<std::mutex> Lock(PublishMu);
// Do not emit TU statuses when the ASTWorker is shutting down.
- if (ReportDiagnostics) {
+ if (CanPublishResults) {
Callbacks.onFileUpdated(FileName, Status);
}
}
Index: clang-tools-extra/clangd/ClangdServer.cpp
===================================================================
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ 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<Diag> Diagnostics = AST.getDiagnostics();
+ std::vector<HighlightingToken> Highlightings;
if (SemanticHighlighting)
- DiagConsumer.onHighlightingsReady(Path, getSemanticHighlightings(AST));
- }
+ Highlightings = getSemanticHighlightings(AST);
- void onDiagnostics(PathRef File, std::vector<Diag> 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 {
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
