kadircet created this revision.
kadircet added a reviewer: sammccall.
Herald added subscribers: usaxena95, arphaman, javed.absar.
kadircet requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.
Previously notification of the Server about semantic happened strictly
before notification of the AST thread.
Hence a racy Server could make a request (like semantic tokens) after
the notification, with the assumption that it'll be served fresh
content. But it wasn't true if AST thread wasn't notified about the
change yet.
This change reverses the order of those notifications to prevent racy
interactions.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D102761
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
@@ -169,6 +169,10 @@
/// Called whenever the TU status is updated.
virtual void onFileUpdated(PathRef File, const TUStatus &Status) {}
+
+ /// Semantics for the TU might've changed (e.g. a new preamble), any actions
+ /// on the file are guranteed to see new semantics after the callback.
+ virtual void onSemanticsMaybeChanged(PathRef File) {}
};
/// Handles running tasks for ClangdServer and managing the resources (e.g.,
Index: clang-tools-extra/clangd/TUScheduler.cpp
===================================================================
--- clang-tools-extra/clangd/TUScheduler.cpp
+++ clang-tools-extra/clangd/TUScheduler.cpp
@@ -909,6 +909,7 @@
ASTPeer.updatePreamble(std::move(Req.CI), std::move(Req.Inputs),
LatestBuild, std::move(Req.CIDiags),
std::move(Req.WantDiags));
+ Callbacks.onSemanticsMaybeChanged(FileName);
});
if (!LatestBuild || Inputs.ForceRebuild) {
Index: clang-tools-extra/clangd/ClangdServer.cpp
===================================================================
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -75,8 +75,6 @@
const CanonicalIncludes &CanonIncludes) override {
if (FIndex)
FIndex->updatePreamble(Path, Version, Ctx, std::move(PP), CanonIncludes);
- if (ServerCallbacks)
- ServerCallbacks->onSemanticsMaybeChanged(Path);
}
void onMainAST(PathRef Path, ParsedAST &AST, PublishFn Publish) override {
@@ -105,6 +103,11 @@
ServerCallbacks->onFileUpdated(File, Status);
}
+ void onSemanticsMaybeChanged(PathRef File) override {
+ if (ServerCallbacks)
+ ServerCallbacks->onSemanticsMaybeChanged(File);
+ }
+
private:
FileIndex *FIndex;
ClangdServer::Callbacks *ServerCallbacks;
Index: clang-tools-extra/clangd/TUScheduler.h
===================================================================
--- clang-tools-extra/clangd/TUScheduler.h
+++ clang-tools-extra/clangd/TUScheduler.h
@@ -169,6 +169,10 @@
/// Called whenever the TU status is updated.
virtual void onFileUpdated(PathRef File, const TUStatus &Status) {}
+
+ /// Semantics for the TU might've changed (e.g. a new preamble), any actions
+ /// on the file are guranteed to see new semantics after the callback.
+ virtual void onSemanticsMaybeChanged(PathRef File) {}
};
/// Handles running tasks for ClangdServer and managing the resources (e.g.,
Index: clang-tools-extra/clangd/TUScheduler.cpp
===================================================================
--- clang-tools-extra/clangd/TUScheduler.cpp
+++ clang-tools-extra/clangd/TUScheduler.cpp
@@ -909,6 +909,7 @@
ASTPeer.updatePreamble(std::move(Req.CI), std::move(Req.Inputs),
LatestBuild, std::move(Req.CIDiags),
std::move(Req.WantDiags));
+ Callbacks.onSemanticsMaybeChanged(FileName);
});
if (!LatestBuild || Inputs.ForceRebuild) {
Index: clang-tools-extra/clangd/ClangdServer.cpp
===================================================================
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -75,8 +75,6 @@
const CanonicalIncludes &CanonIncludes) override {
if (FIndex)
FIndex->updatePreamble(Path, Version, Ctx, std::move(PP), CanonIncludes);
- if (ServerCallbacks)
- ServerCallbacks->onSemanticsMaybeChanged(Path);
}
void onMainAST(PathRef Path, ParsedAST &AST, PublishFn Publish) override {
@@ -105,6 +103,11 @@
ServerCallbacks->onFileUpdated(File, Status);
}
+ void onSemanticsMaybeChanged(PathRef File) override {
+ if (ServerCallbacks)
+ ServerCallbacks->onSemanticsMaybeChanged(File);
+ }
+
private:
FileIndex *FIndex;
ClangdServer::Callbacks *ServerCallbacks;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
