[clang-tools-extra] 465ee9b - [clangd] Publish diagnostics with stale preambles
Kadir Cetinkaya via cfe-commits
cfe-commits at lists.llvm.org
Wed Feb 22 06:54:46 PST 2023
Author: Kadir Cetinkaya
Date: 2023-02-22T15:54:16+01:00
New Revision: 465ee9bfb26d46f2732d8b238dcbadc38373dbb3
URL: https://github.com/llvm/llvm-project/commit/465ee9bfb26d46f2732d8b238dcbadc38373dbb3
DIFF: https://github.com/llvm/llvm-project/commit/465ee9bfb26d46f2732d8b238dcbadc38373dbb3.diff
LOG: [clangd] Publish diagnostics with stale preambles
This patch achieves this by building an AST and invoking main file
callbacks on each update, in addition to preamble updates.
It means we might have some extra AST builds now (e.g. if an update was
with a stale preamble and there were no reads on it, we would only build
an AST once we had the fresh preamble. Now we'll build 2, once with the
stale preamble and another with the fresh one, but we'll have one more
diagnostics cycle in between.).
This patch preserves forward progress of diagnostics by always using the
latest main file contents when emitting diagnostics after preamble
builds. It also guarantees eventual consistency:
- if an update doesn't invalidate preamble, we'll emit diagnostics with
fresh preamble already.
- if an update invalidates preamble, we'll first emit diagnostics with
stale contents, and then once the preamble build finishes it'll emit
diagnostics (as preamble has changed) with newest version.
This has implications on parsing callbacks, as previously onMainAST
callback was called at most once, now it can be called up to 2 times.
All of the existing clients can already deal with callback firing
multiple times.
Differential Revision: https://reviews.llvm.org/D144456
Added:
Modified:
clang-tools-extra/clangd/TUScheduler.cpp
clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/TUScheduler.cpp b/clang-tools-extra/clangd/TUScheduler.cpp
index 354638cd7b1e..a4f6a93b616a 100644
--- a/clang-tools-extra/clangd/TUScheduler.cpp
+++ b/clang-tools-extra/clangd/TUScheduler.cpp
@@ -49,6 +49,7 @@
#include "TUScheduler.h"
#include "CompileCommands.h"
#include "Compiler.h"
+#include "Config.h"
#include "Diagnostics.h"
#include "GlobalCompilationDatabase.h"
#include "ParsedAST.h"
@@ -938,8 +939,19 @@ void ASTWorker::update(ParseInputs Inputs, WantDiagnostics WantDiags,
return;
}
- PreamblePeer.update(std::move(Invocation), std::move(Inputs),
- std::move(CompilerInvocationDiags), WantDiags);
+ // Inform preamble peer, before attempting to build diagnostics so that they
+ // can be built concurrently.
+ PreamblePeer.update(std::make_unique<CompilerInvocation>(*Invocation),
+ Inputs, CompilerInvocationDiags, WantDiags);
+
+ // Emit diagnostics from (possibly) stale preamble while waiting for a
+ // rebuild. Newly built preamble cannot emit diagnostics before this call
+ // finishes (ast callbacks are called from astpeer thread), hence we
+ // gurantee eventual consistency.
+ if (LatestPreamble && Config::current().Diagnostics.AllowStalePreamble)
+ generateDiagnostics(std::move(Invocation), std::move(Inputs),
+ std::move(CompilerInvocationDiags));
+
std::unique_lock<std::mutex> Lock(Mutex);
PreambleCV.wait(Lock, [this] {
// Block until we reiceve a preamble request, unless a preamble already
@@ -1118,6 +1130,18 @@ void ASTWorker::updatePreamble(std::unique_ptr<CompilerInvocation> CI,
// We only need to build the AST if diagnostics were requested.
if (WantDiags == WantDiagnostics::No)
return;
+ // The file may have been edited since we started building this preamble.
+ // If diagnostics need a fresh preamble, we must use the old version that
+ // matches the preamble. We make forward progress as updatePreamble()
+ // receives increasing versions, and this is the only place we emit
+ // diagnostics.
+ // If diagnostics can use a stale preamble, we use the current contents of
+ // the file instead. This provides more up-to-date diagnostics, and avoids
+ // diagnostics going backwards (we may have already emitted staler-preamble
+ // diagnostics for the new version). We still have eventual consistency: at
+ // some point updatePreamble() will catch up to the current file.
+ if (Config::current().Diagnostics.AllowStalePreamble)
+ PI = FileInputs;
// Report diagnostics with the new preamble to ensure progress. Otherwise
// diagnostics might get stale indefinitely if user keeps invalidating the
// preamble.
diff --git a/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp b/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
index 5b3cab16ddb6..ead85a4ce5f2 100644
--- a/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ b/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -8,6 +8,8 @@
#include "Annotations.h"
#include "ClangdServer.h"
+#include "Compiler.h"
+#include "Config.h"
#include "Diagnostics.h"
#include "GlobalCompilationDatabase.h"
#include "Matchers.h"
@@ -21,7 +23,6 @@
#include "support/Path.h"
#include "support/TestTracer.h"
#include "support/Threading.h"
-#include "support/ThreadsafeFS.h"
#include "clang/Basic/DiagnosticDriver.h"
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/FunctionExtras.h"
@@ -31,19 +32,23 @@
#include "llvm/ADT/StringRef.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
-#include <algorithm>
#include <atomic>
#include <chrono>
+#include <condition_variable>
#include <cstdint>
+#include <functional>
#include <memory>
+#include <mutex>
#include <optional>
#include <string>
#include <utility>
+#include <vector>
namespace clang {
namespace clangd {
namespace {
+using ::testing::_;
using ::testing::AllOf;
using ::testing::AnyOf;
using ::testing::Contains;
@@ -1194,6 +1199,118 @@ TEST_F(TUSchedulerTests, OnlyPublishWhenPreambleIsBuilt) {
EXPECT_EQ(PreamblePublishCount, 2);
}
+TEST_F(TUSchedulerTests, PublishWithStalePreamble) {
+ // Callbacks that blocks the preamble thread after the first preamble is
+ // built and stores preamble/main-file versions for diagnostics released.
+ class BlockPreambleThread : public ParsingCallbacks {
+ public:
+ using DiagsCB = std::function<void(ParsedAST &)>;
+ BlockPreambleThread(Notification &UnblockPreamble, DiagsCB CB)
+ : UnblockPreamble(UnblockPreamble), CB(std::move(CB)) {}
+
+ void onPreambleAST(PathRef Path, llvm::StringRef Version,
+ const CompilerInvocation &, ASTContext &Ctx,
+ Preprocessor &, const CanonicalIncludes &) override {
+ if (BuildBefore)
+ ASSERT_TRUE(UnblockPreamble.wait(timeoutSeconds(5)))
+ << "Expected notification";
+ BuildBefore = true;
+ }
+
+ void onMainAST(PathRef File, ParsedAST &AST, PublishFn Publish) override {
+ CB(AST);
+ }
+
+ void onFailedAST(PathRef File, llvm::StringRef Version,
+ std::vector<Diag> Diags, PublishFn Publish) override {
+ ADD_FAILURE() << "Received failed ast for: " << File << " with version "
+ << Version << '\n';
+ }
+
+ private:
+ bool BuildBefore = false;
+ Notification &UnblockPreamble;
+ std::function<void(ParsedAST &)> CB;
+ };
+
+ // Helpers for issuing blocking update requests on a TUScheduler, whose
+ // onMainAST callback would call onDiagnostics.
+ class DiagCollector {
+ public:
+ void onDiagnostics(ParsedAST &AST) {
+ std::scoped_lock<std::mutex> Lock(DiagMu);
+ DiagVersions.emplace_back(
+ std::make_pair(AST.preambleVersion()->str(), AST.version().str()));
+ DiagsReceived.notify_all();
+ }
+
+ std::pair<std::string, std::string>
+ waitForNewDiags(TUScheduler &S, PathRef File, ParseInputs PI) {
+ std::unique_lock<std::mutex> Lock(DiagMu);
+ // Perform the update under the lock to make sure it isn't handled until
+ // we're waiting for it.
+ S.update(File, std::move(PI), WantDiagnostics::Auto);
+ size_t OldSize = DiagVersions.size();
+ bool ReceivedDiags = DiagsReceived.wait_for(
+ Lock, std::chrono::seconds(5),
+ [this, OldSize] { return OldSize + 1 == DiagVersions.size(); });
+ if (!ReceivedDiags) {
+ ADD_FAILURE() << "Timed out waiting for diags";
+ return {"invalid", "version"};
+ }
+ return DiagVersions.back();
+ }
+
+ std::vector<std::pair<std::string, std::string>> diagVersions() {
+ std::scoped_lock<std::mutex> Lock(DiagMu);
+ return DiagVersions;
+ }
+
+ private:
+ std::condition_variable DiagsReceived;
+ std::mutex DiagMu;
+ std::vector<std::pair</*PreambleVersion*/ std::string,
+ /*MainFileVersion*/ std::string>>
+ DiagVersions;
+ };
+
+ Config Cfg;
+ Cfg.Diagnostics.AllowStalePreamble = true;
+ WithContextValue WithCfg(Config::Key, std::move(Cfg));
+
+ DiagCollector Collector;
+ Notification UnblockPreamble;
+ auto DiagCallbacks = std::make_unique<BlockPreambleThread>(
+ UnblockPreamble,
+ [&Collector](ParsedAST &AST) { Collector.onDiagnostics(AST); });
+ TUScheduler S(CDB, optsForTest(), std::move(DiagCallbacks));
+ Path File = testPath("foo.cpp");
+ auto BlockForDiags = [&](ParseInputs PI) {
+ return Collector.waitForNewDiags(S, File, std::move(PI));
+ };
+
+ // Build first preamble.
+ auto PI = getInputs(File, "");
+ PI.Version = PI.Contents = "1";
+ ASSERT_THAT(BlockForDiags(PI), testing::Pair("1", "1"));
+
+ // Now preamble thread is blocked, so rest of the requests sees only the
+ // stale preamble.
+ PI.Version = "2";
+ PI.Contents = "#define BAR\n" + PI.Version;
+ ASSERT_THAT(BlockForDiags(PI), testing::Pair("1", "2"));
+
+ PI.Version = "3";
+ PI.Contents = "#define FOO\n" + PI.Version;
+ ASSERT_THAT(BlockForDiags(PI), testing::Pair("1", "3"));
+
+ UnblockPreamble.notify();
+ S.blockUntilIdle(timeoutSeconds(5));
+
+ // Make sure that we have eventual consistency.
+ EXPECT_THAT(Collector.diagVersions().back(), Pair(PI.Version, PI.Version));
+}
+
// If a header file is missing from the CDB (or inferred using heuristics), and
// it's included by another open file, then we parse it using that files flags.
TEST_F(TUSchedulerTests, IncluderCache) {
More information about the cfe-commits
mailing list