kadircet created this revision.
kadircet added a reviewer: sammccall.
Herald added subscribers: arphaman, javed.absar.
Herald added a project: All.
kadircet requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.
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.
Repository:
rG LLVM Github Monorepo
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,22 @@
#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 <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 +1198,96 @@
EXPECT_EQ(PreamblePublishCount, 2);
}
+TEST_F(TUSchedulerTests, PublishWithStalePreamble) {
+ class BlockPreambleThread : public ParsingCallbacks {
+ public:
+ BlockPreambleThread(Notification &N) : N(N) {}
+ void onPreambleAST(PathRef Path, llvm::StringRef Version,
+ const CompilerInvocation &, ASTContext &Ctx,
+ Preprocessor &, const CanonicalIncludes &) override {
+ if (BuildBefore)
+ ASSERT_TRUE(N.wait(timeoutSeconds(5))) << "Expected notification";
+ BuildBefore = true;
+ }
+ void onMainAST(PathRef File, ParsedAST &AST, PublishFn Publish) override {
+ std::scoped_lock<std::mutex> Lock(DiagMu);
+ DiagVersions.emplace_back(
+ std::make_pair(AST.preambleVersion()->str(), AST.version().str()));
+ DiagsReceived.notify_all();
+ }
+
+ 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';
+ }
+
+ std::optional<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), [&] {
+ return OldSize + 1 == DiagVersions.size();
+ });
+ if (!ReceivedDiags)
+ return std::nullopt;
+ return DiagVersions.back();
+ }
+
+ std::vector<std::pair<std::string, std::string>> diagVersions() {
+ std::scoped_lock<std::mutex> Lock(DiagMu);
+ return DiagVersions;
+ }
+
+ private:
+ bool BuildBefore = false;
+ Notification &N;
+ std::mutex DiagMu;
+ std::condition_variable DiagsReceived;
+ std::vector<std::pair</*PreambleVersion*/ std::string,
+ /*MainFileVersion*/ std::string>>
+ DiagVersions;
+ };
+
+ Config Cfg;
+ Cfg.Diagnostics.AllowStalePreamble = true;
+ WithContextValue WithCfg(Config::Key, std::move(Cfg));
+
+ Notification BlockPreamble;
+ auto DiagCallbacks = std::make_unique<BlockPreambleThread>(BlockPreamble);
+ auto DCPtr = DiagCallbacks.get();
+ TUScheduler S(CDB, optsForTest(), std::move(DiagCallbacks));
+ Path File = testPath("foo.cpp");
+ auto BlockForDiags = [&](ParseInputs PI) mutable {
+ return DCPtr->WaitForNewDiags(S, File, std::move(PI)).value();
+ };
+
+ // Build first preamble.
+ auto PI = getInputs(File, "");
+ PI.Version = PI.Contents = "1";
+ ASSERT_THAT(BlockForDiags(PI), testing::Pair("1", PI.Version));
+
+ // 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", PI.Version));
+
+ PI.Version = "3";
+ PI.Contents = "#define FOO\n" + PI.Version;
+ ASSERT_THAT(BlockForDiags(PI), testing::Pair("1", PI.Version));
+
+ BlockPreamble.notify();
+ S.blockUntilIdle(timeoutSeconds(5));
+
+ // Make sure that we have eventual consistency.
+ EXPECT_THAT(DCPtr->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,6 +939,14 @@
return;
}
+ // 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::make_unique<CompilerInvocation>(*Invocation),
+ Inputs, CompilerInvocationDiags);
+
PreamblePeer.update(std::move(Invocation), std::move(Inputs),
std::move(CompilerInvocationDiags), WantDiags);
std::unique_lock<std::mutex> Lock(Mutex);
@@ -1118,6 +1127,14 @@
// We only need to build the AST if diagnostics were requested.
if (WantDiags == WantDiagnostics::No)
return;
+ // Use latest file contents instead of the preamble inputs whenever we can
+ // emit stale diagnostics. This ensures diagnostics are newer backwards,
+ // e.g. after emitting stale diagnostics for version N + 1, if a preamble
+ // request from the past, version N, finishes, we chose to emit, possibly
+ // wrong diagnostics with version N+1, rather than backwards diags with
+ // version N.
+ 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
