This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG465ee9bfb26d: [clangd] Publish diagnostics with stale
preambles (authored by kadircet).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D144456/new/
https://reviews.llvm.org/D144456
Files:
clang-tools-extra/clangd/TUScheduler.cpp
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
@@ -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 @@
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) {
Index: clang-tools-extra/clangd/TUScheduler.cpp
===================================================================
--- clang-tools-extra/clangd/TUScheduler.cpp
+++ 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 @@
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 @@
// 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.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
