diff --git a/clang-tools-extra/clangd/TUScheduler.cpp b/clang-tools-extra/clangd/TUScheduler.cpp --- 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,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(*Invocation), + Inputs, CompilerInvocationDiags); + PreamblePeer.update(std::move(Invocation), std::move(Inputs), std::move(CompilerInvocationDiags), WantDiags); std::unique_lock 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. diff --git a/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp b/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp --- 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,22 @@ #include "llvm/ADT/StringRef.h" #include "gmock/gmock.h" #include "gtest/gtest.h" -#include #include #include +#include #include #include +#include #include #include #include +#include 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 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 Diags, PublishFn Publish) override { + ADD_FAILURE() << "Received failed ast for: " << File << " with version " + << Version << '\n'; + } + + std::optional> + WaitForNewDiags(TUScheduler &S, PathRef File, ParseInputs PI) { + std::unique_lock 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> diagVersions() { + std::scoped_lock Lock(DiagMu); + return DiagVersions; + } + + private: + bool BuildBefore = false; + Notification &N; + std::mutex DiagMu; + std::condition_variable DiagsReceived; + std::vector> + DiagVersions; + }; + + Config Cfg; + Cfg.Diagnostics.AllowStalePreamble = true; + WithContextValue WithCfg(Config::Key, std::move(Cfg)); + + Notification BlockPreamble; + auto DiagCallbacks = std::make_unique(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) {