ilya-biryukov created this revision.
ilya-biryukov added a reviewer: sammccall.
Herald added subscribers: kadircet, arphaman, jkorous, MaskRay, javed.absar.
Herald added a project: clang.
By exposing a callback that can guard code publishing results of
'onMainAST' callback in the same manner we guard diagnostics.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D64985
Files:
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/TUScheduler.cpp
clang-tools-extra/clangd/TUScheduler.h
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>
@@ -88,6 +89,7 @@
BuildDetails Details;
};
+
class ParsingCallbacks {
public:
virtual ~ParsingCallbacks() = default;
@@ -98,6 +100,10 @@
virtual void onPreambleAST(PathRef Path, ASTContext &Ctx,
std::shared_ptr<clang::Preprocessor> PP,
const CanonicalIncludes &) {}
+ /// A function that is run under the critical section guarding against races
+ /// when closing the files.
+ using PublishResults = 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,7 +114,14 @@
/// 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) {}
+ ///
+ /// This callback is run on a worker thread and if we need to send results of
+ /// processing AST to the users, we might run into a race condition with file
+ /// removal. To avoid the race condition, send the results by running \p
+ /// Publish. That would ensure publishing the results is either sequenced
+ /// before removing the file or does not happen at all.
+ virtual void onMainAST(PathRef Path, ParsedAST &AST,
+ llvm::function_ref<void(PublishResults)> Publish) {}
/// Called whenever the diagnostics for \p File are produced.
virtual void onDiagnostics(PathRef File, std::vector<Diag> Diags) {}
Index: clang-tools-extra/clangd/TUScheduler.cpp
===================================================================
--- clang-tools-extra/clangd/TUScheduler.cpp
+++ clang-tools-extra/clangd/TUScheduler.cpp
@@ -486,13 +486,22 @@
// spam us with updates.
// Note *AST can still be null if buildAST fails.
if (*AST) {
- {
+ // Results from the worker thread should only be published if the file was
+ // not removed. This callback ensures the passed operation is not racing
+ // with file removal.
+ auto Publish = [&](ParsingCallbacks::PublishResults Action) {
std::lock_guard<std::mutex> Lock(DiagsMu);
if (ReportDiagnostics)
- Callbacks.onDiagnostics(FileName, (*AST)->getDiagnostics());
- }
+ Action();
+ };
+
+ // Publish diagnostics.
+ Publish([&]() {
+ Callbacks.onDiagnostics(FileName, (*AST)->getDiagnostics());
+ });
+
trace::Span Span("Running main AST callback");
- Callbacks.onMainAST(FileName, **AST);
+ Callbacks.onMainAST(FileName, **AST, Publish);
DiagsWereReported = true;
}
// Stash the AST in the cache for further use.
Index: clang-tools-extra/clangd/ClangdServer.cpp
===================================================================
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -31,6 +31,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,11 +61,17 @@
FIndex->updatePreamble(Path, Ctx, std::move(PP), CanonIncludes);
}
- void onMainAST(PathRef Path, ParsedAST &AST) override {
+ void onMainAST(PathRef Path, ParsedAST &AST,
+ llvm::function_ref<void(PublishResults)> Publish) override {
if (FIndex)
FIndex->updateMain(Path, AST);
- if (SemanticHighlighting)
- DiagConsumer.onHighlightingsReady(Path, getSemanticHighlightings(AST));
+
+ if (!SemanticHighlighting)
+ return;
+ auto Highlightings = getSemanticHighlightings(AST);
+ Publish([&]() {
+ DiagConsumer.onHighlightingsReady(Path, std::move(Highlightings));
+ });
}
void onDiagnostics(PathRef File, std::vector<Diag> Diags) override {
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
