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 @@ -221,6 +221,7 @@ /// will be built. void update(std::unique_ptr CI, ParseInputs PI, std::vector CIDiags, WantDiagnostics WantDiags) { + dlog("Preamble worker queueing {0}", PI.Version); // Make possibly expensive copy while not holding the lock. Request Req = {std::move(CI), std::move(PI), std::move(CIDiags), WantDiags, Context::current().clone()}; @@ -232,10 +233,14 @@ return; } { - std::lock_guard Lock(Mutex); + std::unique_lock Lock(Mutex); // If shutdown is issued, don't bother building. if (Done) return; + ReqCV.wait(Lock, [this] { + return !NextReq || NextReq->WantDiags != WantDiagnostics::Yes; + }); + dlog("preamble worker queued {0}", Req.Inputs.Version); NextReq = std::move(Req); } // Let the worker thread know there's a request, notify_one is safe as there @@ -291,6 +296,7 @@ } bool blockUntilIdle(Deadline Timeout) const { + dlog("blocking for preamble worker"); std::unique_lock Lock(Mutex); return wait(Lock, ReqCV, Timeout, [&] { return !NextReq && !CurrentReq; }); } @@ -874,6 +880,8 @@ void ASTWorker::getCurrentPreamble( llvm::unique_function)> Callback) { + if (RunSync) + return Callback(getPossiblyStalePreamble()); // We could just call startTask() to throw the read on the queue, knowing // it will run after any updates. But we know this task is cheap, so to // improve latency we cheat: insert it on the queue after the last update. @@ -881,23 +889,23 @@ auto LastUpdate = std::find_if(Requests.rbegin(), Requests.rend(), [](const Request &R) { return R.UpdateType.hasValue(); }); - // If there were no writes in the queue, and CurrentRequest is not a write, - // the preamble is ready now. - if (LastUpdate == Requests.rend() && - (!CurrentRequest || CurrentRequest->UpdateType.hasValue())) { - Lock.unlock(); - return Callback(getPossiblyStalePreamble()); - } - assert(!RunSync && "Running synchronously, but queue is non-empty!"); - Requests.insert(LastUpdate.base(), - Request{[Callback = std::move(Callback), this]() mutable { - Callback(getPossiblyStalePreamble()); - }, - "GetPreamble", steady_clock::now(), - Context::current().clone(), - /*UpdateType=*/None, - /*InvalidationPolicy=*/TUScheduler::NoInvalidation, - /*Invalidate=*/nullptr}); + // FIXME: We should only serve stale preambles or build it here instead. + Requests.insert( + LastUpdate.base(), + Request{ + [Callback = std::move(Callback), this]() mutable { + PreamblePeer.blockUntilIdle(Deadline::infinity()); + PreambleRequests.push( + {[Callback = std::move(Callback), this]() mutable { + Callback(getPossiblyStalePreamble()); + }, + "GetPreamble", steady_clock::now(), Context::current().clone(), + None, TUScheduler::NoInvalidation, nullptr}); + }, + "WaitForPreamble", steady_clock::now(), Context::current().clone(), + /*UpdateType=*/None, + /*InvalidationPolicy=*/TUScheduler::NoInvalidation, + /*Invalidate=*/nullptr}); Lock.unlock(); RequestsCV.notify_all(); } @@ -947,6 +955,7 @@ llvm::unique_function Task, llvm::Optional UpdateType, TUScheduler::ASTActionInvalidation Invalidation) { + dlog("ASTWorker queueing {0}", Name); if (RunSync) { assert(!Done && "running a task after stop()"); trace::Span Tracer(Name + ":" + llvm::sys::path::filename(FileName)); @@ -1139,9 +1148,18 @@ bool ASTWorker::blockUntilIdle(Deadline Timeout) const { std::unique_lock Lock(Mutex); - return wait(Lock, RequestsCV, Timeout, [&] { - return PreambleRequests.empty() && Requests.empty() && !CurrentRequest; - }); + // Make sure ASTWorker has processed all requests. + if (!wait(Lock, RequestsCV, Timeout, + [&] { return Requests.empty() && !CurrentRequest; })) + return false; + Lock.unlock(); + if (!PreamblePeer.blockUntilIdle(Timeout)) + return false; + Lock.lock(); + assert(Requests.empty() && "Received new requests during blockUntilIdle"); + // Make sure ASTWorker has processed all preambles. + return wait(Lock, RequestsCV, Timeout, + [&] { return PreambleRequests.empty() && !CurrentRequest; }); } // Render a TUAction to a user-facing string representation. diff --git a/clang-tools-extra/clangd/unittests/ClangdTests.cpp b/clang-tools-extra/clangd/unittests/ClangdTests.cpp --- a/clang-tools-extra/clangd/unittests/ClangdTests.cpp +++ b/clang-tools-extra/clangd/unittests/ClangdTests.cpp @@ -210,18 +210,18 @@ FS.Files[FooCpp] = SourceContents; Server.addDocument(FooCpp, SourceContents); - auto DumpParse1 = dumpASTWithoutMemoryLocs(Server, FooCpp); ASSERT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for diagnostics"; + auto DumpParse1 = dumpASTWithoutMemoryLocs(Server, FooCpp); EXPECT_FALSE(DiagConsumer.hadErrorInLastDiags()); Server.addDocument(FooCpp, ""); - auto DumpParseEmpty = dumpASTWithoutMemoryLocs(Server, FooCpp); ASSERT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for diagnostics"; + auto DumpParseEmpty = dumpASTWithoutMemoryLocs(Server, FooCpp); EXPECT_FALSE(DiagConsumer.hadErrorInLastDiags()); Server.addDocument(FooCpp, SourceContents); - auto DumpParse2 = dumpASTWithoutMemoryLocs(Server, FooCpp); ASSERT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for diagnostics"; + auto DumpParse2 = dumpASTWithoutMemoryLocs(Server, FooCpp); EXPECT_FALSE(DiagConsumer.hadErrorInLastDiags()); EXPECT_EQ(DumpParse1, DumpParse2); @@ -246,20 +246,20 @@ FS.Files[FooCpp] = SourceContents; Server.addDocument(FooCpp, SourceContents); - auto DumpParse1 = dumpASTWithoutMemoryLocs(Server, FooCpp); ASSERT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for diagnostics"; + auto DumpParse1 = dumpASTWithoutMemoryLocs(Server, FooCpp); EXPECT_FALSE(DiagConsumer.hadErrorInLastDiags()); FS.Files[FooH] = ""; Server.addDocument(FooCpp, SourceContents); - auto DumpParseDifferent = dumpASTWithoutMemoryLocs(Server, FooCpp); ASSERT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for diagnostics"; + auto DumpParseDifferent = dumpASTWithoutMemoryLocs(Server, FooCpp); EXPECT_TRUE(DiagConsumer.hadErrorInLastDiags()); FS.Files[FooH] = "int a;"; Server.addDocument(FooCpp, SourceContents); - auto DumpParse2 = dumpASTWithoutMemoryLocs(Server, FooCpp); ASSERT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for diagnostics"; + auto DumpParse2 = dumpASTWithoutMemoryLocs(Server, FooCpp); EXPECT_FALSE(DiagConsumer.hadErrorInLastDiags()); EXPECT_EQ(DumpParse1, DumpParse2); 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 @@ -323,9 +323,11 @@ // Helper to schedule a named update and return a function to cancel it. auto Update = [&](std::string ID) -> Canceler { auto T = cancelableTask(); + auto Inp = getInputs(Path, "//" + ID); + Inp.Version = ID; WithContext C(std::move(T.first)); updateWithDiags( - S, Path, "//" + ID, WantDiagnostics::Yes, + S, Path, std::move(Inp), WantDiagnostics::Yes, [&, ID](std::vector Diags) { DiagsSeen.push_back(ID); }); return std::move(T.second); }; @@ -498,7 +500,7 @@ WithContextValue WithNonce(NonceKey, ++Nonce); Inputs.Version = std::to_string(Nonce); updateWithDiags( - S, File, Inputs, WantDiagnostics::Auto, + S, File, Inputs, WantDiagnostics::Yes, [File, Nonce, &Mut, &TotalUpdates](std::vector) { EXPECT_THAT(Context::current().get(NonceKey), Pointee(Nonce)); @@ -735,7 +737,8 @@ )cpp"; ParseInputs Inputs = getInputs(Source, SourceContents); - std::atomic DiagCount(0); + Inputs.Version = "MissingHeader"; + std::atomic DiagCount(0); // Update the source contents, which should trigger an initial build with // the header file missing. @@ -754,6 +757,7 @@ Files[Header] = "int a;"; Timestamps[Header] = time_t(1); Inputs = getInputs(Source, SourceContents); + Inputs.Version = "WithHeader"; // The addition of the missing header file shouldn't trigger a rebuild since // we don't track missing files. @@ -767,6 +771,7 @@ // Forcing the reload should should cause a rebuild which no longer has any // errors. Inputs.ForceRebuild = true; + Inputs.Version = "ForceRebuild"; updateWithDiags(S, Source, Inputs, WantDiagnostics::Yes, [&DiagCount](std::vector Diags) { ++DiagCount; @@ -858,27 +863,20 @@ ASSERT_TRUE(Server.blockUntilIdleForTest()); - EXPECT_THAT(CaptureTUStatus.allStatus(), - ElementsAre( - // Everything starts with ASTWorker starting to execute an - // update - TUState(PreambleAction::Idle, ASTAction::RunningAction), - // We build the preamble - TUState(PreambleAction::Building, ASTAction::RunningAction), - // We built the preamble, and issued ast built on ASTWorker - // thread. Preambleworker goes idle afterwards. - TUState(PreambleAction::Idle, ASTAction::RunningAction), - // Start task for building the ast, as a result of building - // preamble, on astworker thread. - TUState(PreambleAction::Idle, ASTAction::RunningAction), - // AST build starts. - TUState(PreambleAction::Idle, ASTAction::Building), - // AST built finished successfully - TUState(PreambleAction::Idle, ASTAction::Building), - // Running go to def - TUState(PreambleAction::Idle, ASTAction::RunningAction), - // ASTWorker goes idle. - TUState(PreambleAction::Idle, ASTAction::Idle))); + auto Statuses = CaptureTUStatus.allStatus(); + const std::vector PreambleStatuses = { + PreambleAction::Idle, PreambleAction::Building, PreambleAction::Idle}; + + llvm::ArrayRef RemainingStatuses = + llvm::makeArrayRef(PreambleStatuses); + ASSERT_FALSE(Statuses.empty()); + for (size_t I = 0, E = Statuses.size(); I != E; ++I) { + if (RemainingStatuses.front() != Statuses[I].PreambleActivity) + RemainingStatuses = RemainingStatuses.drop_front(); + ASSERT_FALSE(RemainingStatuses.empty()); + EXPECT_EQ(Statuses[I].PreambleActivity, RemainingStatuses.front()); + } + EXPECT_EQ(RemainingStatuses.size(), 1U); } TEST_F(TUSchedulerTests, CommandLineErrors) { diff --git a/clang-tools-extra/clangd/unittests/XRefsTests.cpp b/clang-tools-extra/clangd/unittests/XRefsTests.cpp --- a/clang-tools-extra/clangd/unittests/XRefsTests.cpp +++ b/clang-tools-extra/clangd/unittests/XRefsTests.cpp @@ -968,6 +968,7 @@ // Only preamble is built, and no AST is built in this request. Server.addDocument(FooCpp, FooWithoutHeader.code(), "null", WantDiagnostics::No); + ASSERT_TRUE(Server.blockUntilIdleForTest()); // We build AST here, and it should use the latest preamble rather than the // stale one. EXPECT_THAT( @@ -979,6 +980,7 @@ // Both preamble and AST are built in this request. Server.addDocument(FooCpp, FooWithoutHeader.code(), "null", WantDiagnostics::Yes); + ASSERT_TRUE(Server.blockUntilIdleForTest()); // Use the AST being built in above request. EXPECT_THAT( cantFail(runLocateSymbolAt(Server, FooCpp, FooWithoutHeader.point())),