Skip to content

Commit

Permalink
[clangd] Provide a way to publish highlightings in non-racy manner
Browse files Browse the repository at this point in the history 
Summary:
By exposing a callback that can guard code publishing results of
'onMainAST' callback in the same manner we guard diagnostics.

Reviewers: sammccall

Reviewed By: sammccall

Subscribers: javed.absar, MaskRay, jkorous, arphaman, kadircet, hokein, jvikstrom, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D64985

llvm-svn: 366577
  • Loading branch information
@ilya-biryukov
ilya-biryukov committed Jul 19, 2019
1 parent c35dd05 commit 8bb8915
Show file tree
Hide file tree
Showing 4 changed files with 77 additions and 47 deletions.
17 changes: 12 additions & 5 deletions clang-tools-extra/clangd/ClangdServer.cpp
View file
Expand Up @@ -14,6 +14,7 @@
#include "FormattedString.h"
#include "Headers.h"
#include "Protocol.h"
#include "SemanticHighlighting.h"
#include "SourceCode.h"
#include "TUScheduler.h"
#include "Trace.h"
Expand All @@ -31,6 +32,7 @@
#include "clang/Tooling/Core/Replacement.h"
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/Optional.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/ScopeExit.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/Errc.h"
Expand Down Expand Up @@ -60,15 +62,20 @@ struct UpdateIndexCallbacks : public ParsingCallbacks {
FIndex->updatePreamble(Path, Ctx, std::move(PP), CanonIncludes);
}

void onMainAST(PathRef Path, ParsedAST &AST) override {
void onMainAST(PathRef Path, ParsedAST &AST, PublishFn Publish) override {
if (FIndex)
FIndex->updateMain(Path, AST);

std::vector<Diag> Diagnostics = AST.getDiagnostics();
std::vector<HighlightingToken> Highlightings;
if (SemanticHighlighting)
DiagConsumer.onHighlightingsReady(Path, getSemanticHighlightings(AST));
}
Highlightings = getSemanticHighlightings(AST);

void onDiagnostics(PathRef File, std::vector<Diag> Diags) override {
DiagConsumer.onDiagnosticsReady(File, std::move(Diags));
Publish([&]() {
DiagConsumer.onDiagnosticsReady(Path, std::move(Diagnostics));
if (SemanticHighlighting)
DiagConsumer.onHighlightingsReady(Path, std::move(Highlightings));
});
}

void onFileUpdated(PathRef File, const TUStatus &Status) override {
Expand Down
60 changes: 31 additions & 29 deletions clang-tools-extra/clangd/TUScheduler.cpp
View file
Expand Up @@ -249,9 +249,8 @@ class ASTWorker {
TUStatus Status;

Semaphore &Barrier;
/// Whether the diagnostics for the current FileInputs were reported to the
/// users before.
bool DiagsWereReported = false;
/// Whether the 'onMainAST' callback ran for the current FileInputs.
bool RanASTCallback = false;
/// Guards members used by both TUScheduler and the worker thread.
mutable std::mutex Mutex;
/// File inputs, currently being used by the worker.
Expand All @@ -265,15 +264,16 @@ class ASTWorker {
bool Done; /* GUARDED_BY(Mutex) */
std::deque<Request> Requests; /* GUARDED_BY(Mutex) */
mutable std::condition_variable RequestsCV;
// FIXME: rename it to better fix the current usage, we also use it to guard
// emitting TUStatus.
/// Guards a critical section for running the diagnostics callbacks.
std::mutex DiagsMu;
// Used to prevent remove document + leading to out-of-order diagnostics:
/// Guards the callback that publishes results of AST-related computations
/// (diagnostics, highlightings) and file statuses.
std::mutex PublishMu;
// Used to prevent remove document + add document races that lead to
// out-of-order callbacks for publishing results of onMainAST callback.
//
// The lifetime of the old/new ASTWorkers will overlap, but their handles
// don't. When the old handle is destroyed, the old worker will stop reporting
// diagnostics.
bool ReportDiagnostics = true; /* GUARDED_BY(DiagsMu) */
// any results to the user.
bool CanPublishResults = true; /* GUARDED_BY(PublishMu) */
};

/// A smart-pointer-like class that points to an active ASTWorker.
Expand Down Expand Up @@ -381,12 +381,12 @@ void ASTWorker::update(ParseInputs Inputs, WantDiagnostics WantDiags) {
std::tie(Inputs.CompileCommand, Inputs.Contents);

tooling::CompileCommand OldCommand = PrevInputs->CompileCommand;
bool PrevDiagsWereReported = DiagsWereReported;
bool RanCallbackForPrevInputs = RanASTCallback;
{
std::lock_guard<std::mutex> Lock(Mutex);
FileInputs = std::make_shared<ParseInputs>(Inputs);
}
DiagsWereReported = false;
RanASTCallback = false;
emitTUStatus({TUAction::BuildingPreamble, TaskName});
log("Updating file {0} with command {1}\n[{2}]\n{3}", FileName,
Inputs.CompileCommand.Heuristic,
Expand Down Expand Up @@ -432,10 +432,9 @@ void ASTWorker::update(ParseInputs Inputs, WantDiagnostics WantDiags) {
if (!CanReuseAST) {
IdleASTs.take(this); // Remove the old AST if it's still in cache.
} else {
// Since we don't need to rebuild the AST, we might've already reported
// the diagnostics for it.
if (PrevDiagsWereReported) {
DiagsWereReported = true;
// We don't need to rebuild the AST, check if we need to run the callback.
if (RanCallbackForPrevInputs) {
RanASTCallback = true;
// Take a shortcut and don't report the diagnostics, since they should
// not changed. All the clients should handle the lack of OnUpdated()
// call anyway to handle empty result from buildAST.
Expand All @@ -457,10 +456,10 @@ void ASTWorker::update(ParseInputs Inputs, WantDiagnostics WantDiags) {
return;

{
std::lock_guard<std::mutex> Lock(DiagsMu);
std::lock_guard<std::mutex> Lock(PublishMu);
// No need to rebuild the AST if we won't send the diagnotics. However,
// note that we don't prevent preamble rebuilds.
if (!ReportDiagnostics)
if (!CanPublishResults)
return;
}

Expand All @@ -486,14 +485,17 @@ void ASTWorker::update(ParseInputs Inputs, WantDiagnostics WantDiags) {
// spam us with updates.
// Note *AST can still be null if buildAST fails.
if (*AST) {
{
std::lock_guard<std::mutex> Lock(DiagsMu);
if (ReportDiagnostics)
Callbacks.onDiagnostics(FileName, (*AST)->getDiagnostics());
}
trace::Span Span("Running main AST callback");
Callbacks.onMainAST(FileName, **AST);
DiagsWereReported = true;
auto RunPublish = [&](llvm::function_ref<void()> Publish) {
// Ensure we only publish results from the worker if the file was not
// removed, making sure there are not race conditions.
std::lock_guard<std::mutex> Lock(PublishMu);
if (CanPublishResults)
Publish();
};

Callbacks.onMainAST(FileName, **AST, RunPublish);
RanASTCallback = true;
}
// Stash the AST in the cache for further use.
IdleASTs.put(this, std::move(*AST));
Expand Down Expand Up @@ -594,8 +596,8 @@ bool ASTWorker::isASTCached() const { return IdleASTs.getUsedBytes(this) != 0; }

void ASTWorker::stop() {
{
std::lock_guard<std::mutex> Lock(DiagsMu);
ReportDiagnostics = false;
std::lock_guard<std::mutex> Lock(PublishMu);
CanPublishResults = false;
}
{
std::lock_guard<std::mutex> Lock(Mutex);
Expand Down Expand Up @@ -630,9 +632,9 @@ void ASTWorker::emitTUStatus(TUAction Action,
Status.Action = std::move(Action);
if (Details)
Status.Details = *Details;
std::lock_guard<std::mutex> Lock(DiagsMu);
std::lock_guard<std::mutex> Lock(PublishMu);
// Do not emit TU statuses when the ASTWorker is shutting down.
if (ReportDiagnostics) {
if (CanPublishResults) {
Callbacks.onFileUpdated(FileName, Status);
}
}
Expand Down
20 changes: 16 additions & 4 deletions clang-tools-extra/clangd/TUScheduler.h
View file
Expand Up @@ -15,6 +15,7 @@
#include "Threading.h"
#include "index/CanonicalIncludes.h"
#include "llvm/ADT/Optional.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/StringMap.h"
#include "llvm/ADT/StringRef.h"
#include <future>
Expand Down Expand Up @@ -98,6 +99,10 @@ class ParsingCallbacks {
virtual void onPreambleAST(PathRef Path, ASTContext &Ctx,
std::shared_ptr<clang::Preprocessor> PP,
const CanonicalIncludes &) {}

/// The argument function is run under the critical section guarding against
/// races when closing the files.
using PublishFn = llvm::function_ref<void(llvm::function_ref<void()>)>;
/// Called on the AST built for the file itself. Note that preamble AST nodes
/// are not deserialized and should be processed in the onPreambleAST call
/// instead.
Expand All @@ -108,10 +113,17 @@ class ParsingCallbacks {
/// etc. Clients are expected to process only the AST nodes from the main file
/// in this callback (obtained via ParsedAST::getLocalTopLevelDecls) to obtain
/// optimal performance.
virtual void onMainAST(PathRef Path, ParsedAST &AST) {}

/// Called whenever the diagnostics for \p File are produced.
virtual void onDiagnostics(PathRef File, std::vector<Diag> Diags) {}
///
/// When information about the file (diagnostics, syntax highlighting) is
/// published to clients, this should be wrapped in Publish, e.g.
/// void onMainAST(...) {
/// Highlights = computeHighlights();
/// Publish([&] { notifyHighlights(Path, Highlights); });
/// }
/// This guarantees that clients will see results in the correct sequence if
/// the file is concurrently closed and/or reopened. (The lambda passed to
/// Publish() may never run in this case).
virtual void onMainAST(PathRef Path, ParsedAST &AST, PublishFn Publish) {}

/// Called whenever the TU status is updated.
virtual void onFileUpdated(PathRef File, const TUStatus &Status) {}
Expand Down
27 changes: 18 additions & 9 deletions clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
View file
Expand Up @@ -7,10 +7,14 @@
//===----------------------------------------------------------------------===//

#include "Annotations.h"
#include "ClangdUnit.h"
#include "Context.h"
#include "Diagnostics.h"
#include "Matchers.h"
#include "Path.h"
#include "TUScheduler.h"
#include "TestFS.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/ScopeExit.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
Expand Down Expand Up @@ -56,12 +60,17 @@ class TUSchedulerTests : public ::testing::Test {
/// in updateWithDiags.
static std::unique_ptr<ParsingCallbacks> captureDiags() {
class CaptureDiags : public ParsingCallbacks {
void onDiagnostics(PathRef File, std::vector<Diag> Diags) override {
void onMainAST(PathRef File, ParsedAST &AST, PublishFn Publish) override {
auto Diags = AST.getDiagnostics();
auto D = Context::current().get(DiagsCallbackKey);
if (!D)
return;
const_cast<llvm::unique_function<void(PathRef, std::vector<Diag>)> &> (
*D)(File, Diags);

Publish([&]() {
const_cast<
llvm::unique_function<void(PathRef, std::vector<Diag>)> &> (*D)(
File, std::move(Diags));
});
}
};
return llvm::make_unique<CaptureDiags>();
Expand Down Expand Up @@ -116,8 +125,8 @@ TEST_F(TUSchedulerTests, MissingFiles) {
S.update(Added, getInputs(Added, "x"), WantDiagnostics::No);
EXPECT_EQ(S.getContents(Added), "x");

// Assert each operation for missing file is an error (even if it's available
// in VFS).
// Assert each operation for missing file is an error (even if it's
// available in VFS).
S.runWithAST("", Missing,
[&](Expected<InputsAndAST> AST) { EXPECT_ERROR(AST); });
S.runWithPreamble(
Expand Down Expand Up @@ -367,8 +376,8 @@ TEST_F(TUSchedulerTests, ManyUpdates) {
StringRef AllContents[] = {Contents1, Contents2, Contents3};
const int AllContentsSize = 3;

// Scheduler may run tasks asynchronously, but should propagate the context.
// We stash a nonce in the context, and verify it in the task.
// Scheduler may run tasks asynchronously, but should propagate the
// context. We stash a nonce in the context, and verify it in the task.
static Key<int> NonceKey;
int Nonce = 0;

Expand Down Expand Up @@ -465,8 +474,8 @@ TEST_F(TUSchedulerTests, EvictedAST) {
ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
ASSERT_EQ(BuiltASTCounter.load(), 1);

// Build two more files. Since we can retain only 2 ASTs, these should be the
// ones we see in the cache later.
// Build two more files. Since we can retain only 2 ASTs, these should be
// the ones we see in the cache later.
updateWithCallback(S, Bar, SourceContents, WantDiagnostics::Yes,
[&BuiltASTCounter]() { ++BuiltASTCounter; });
updateWithCallback(S, Baz, SourceContents, WantDiagnostics::Yes,
Expand Down

0 comments on commit 8bb8915

Please sign in to comment.