kadircet created this revision.
kadircet added a reviewer: sammccall.
Herald added subscribers: cfe-commits, usaxena95, arphaman, jkorous, MaskRay,
javed.absar, ilya-biryukov.
Herald added a project: clang.
As vfs::FileSystem is not threadsafe, we've introduced a data race when
PreambleThread started to run in async mode. As both ASTWorker and
PreambleWorker can concurrently work on the same FS.
This patch fixes that data race by propagating FSProvider into TUScheduler and
ASTWorker and making use of a new FS when issuing update to the PreambleThread.
This doesn't break our contract on FSProvider::getFileSystem, as it says the
active context will be client's context at the time of request and
ASTWorker::update is run with the context we've captured on
ClangdServer::AddDocument.
As for the operations, this implies preamble and ast threads can see different
versions of the filesystems. For example in presence of a snapshotted filesystem
- client makes an AddDocument request with FS at snapshot v1
- client then updates the FS to v2
- ASTWorker starts handling the update, caches the FS v1 in FileInputs, which
is used for serving reads.
- Schedules a preamble update, this fetches a new FS from provider, which might
yield v2. This says `might` as clients can handle this case by storing snapshot
version in context.
- Preamble build finishes and clangd publishes diagnostics for FS v2.
- Client issues a read (e.g. go-to-def), clangd uses FS v1 for serving the
request.
This can be overcome by 2 filesystems into TUScheduler in
ClangdServer::AddDocument, but I don't think it is an issue in practice
currently. As our beloved snapshotted filesystem provider stores the snapshot
version in context, and should always return the same FS for requests coming
from the same context.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D80900
Files:
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/TUScheduler.cpp
clang-tools-extra/clangd/TUScheduler.h
clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
clang-tools-extra/clangd/unittests/TestFS.h
Index: clang-tools-extra/clangd/unittests/TestFS.h
===================================================================
--- clang-tools-extra/clangd/unittests/TestFS.h
+++ clang-tools-extra/clangd/unittests/TestFS.h
@@ -31,11 +31,12 @@
class MockFSProvider : public FileSystemProvider {
public:
IntrusiveRefCntPtr<llvm::vfs::FileSystem> getFileSystem() const override {
- return buildTestFS(Files);
+ return buildTestFS(Files, Timestamps);
}
// If relative paths are used, they are resolved with testPath().
llvm::StringMap<std::string> Files;
+ llvm::StringMap<time_t> Timestamps;
};
// A Compilation database that returns a fixed set of compile flags.
Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -73,7 +73,7 @@
ParseInputs getInputs(PathRef File, std::string Contents) {
ParseInputs Inputs;
Inputs.CompileCommand = *CDB.getCompileCommand(File);
- Inputs.FS = buildTestFS(Files, Timestamps);
+ Inputs.FS = FSProvider.getFileSystem();
Inputs.Contents = std::move(Contents);
Inputs.Opts = ParseOptions();
return Inputs;
@@ -149,8 +149,7 @@
std::move(CB));
}
- llvm::StringMap<std::string> Files;
- llvm::StringMap<time_t> Timestamps;
+ MockFSProvider FSProvider;
MockCompilationDatabase CDB;
};
@@ -158,13 +157,13 @@
TUSchedulerTests::DiagsCallbackKey;
TEST_F(TUSchedulerTests, MissingFiles) {
- TUScheduler S(CDB, optsForTest());
+ TUScheduler S(FSProvider, CDB, optsForTest());
auto Added = testPath("added.cpp");
- Files[Added] = "x";
+ FSProvider.Files[Added] = "x";
auto Missing = testPath("missing.cpp");
- Files[Missing] = "";
+ FSProvider.Files[Missing] = "";
S.update(Added, getInputs(Added, "x"), WantDiagnostics::No);
@@ -205,7 +204,7 @@
// To avoid a racy test, don't allow tasks to actually run on the worker
// thread until we've scheduled them all.
Notification Ready;
- TUScheduler S(CDB, optsForTest(), captureDiags());
+ TUScheduler S(FSProvider, CDB, optsForTest(), captureDiags());
auto Path = testPath("foo.cpp");
updateWithDiags(S, Path, "", WantDiagnostics::Yes,
[&](std::vector<Diag>) { Ready.wait(); });
@@ -234,7 +233,7 @@
{
auto Opts = optsForTest();
Opts.UpdateDebounce = DebouncePolicy::fixed(std::chrono::seconds(1));
- TUScheduler S(CDB, Opts, captureDiags());
+ TUScheduler S(FSProvider, CDB, Opts, captureDiags());
// FIXME: we could probably use timeouts lower than 1 second here.
auto Path = testPath("foo.cpp");
updateWithDiags(S, Path, "auto (debounced)", WantDiagnostics::Auto,
@@ -267,7 +266,7 @@
std::vector<std::string> DiagsSeen, ReadsSeen, ReadsCanceled;
{
Notification Proceed; // Ensure we schedule everything.
- TUScheduler S(CDB, optsForTest(), captureDiags());
+ TUScheduler S(FSProvider, CDB, optsForTest(), captureDiags());
auto Path = testPath("foo.cpp");
// Helper to schedule a named update and return a function to cancel it.
auto Update = [&](std::string ID) -> Canceler {
@@ -324,7 +323,7 @@
TEST_F(TUSchedulerTests, InvalidationNoCrash) {
auto Path = testPath("foo.cpp");
- TUScheduler S(CDB, optsForTest(), captureDiags());
+ TUScheduler S(FSProvider, CDB, optsForTest(), captureDiags());
Notification StartedRunning;
Notification ScheduledChange;
@@ -348,7 +347,7 @@
TEST_F(TUSchedulerTests, Invalidation) {
auto Path = testPath("foo.cpp");
- TUScheduler S(CDB, optsForTest(), captureDiags());
+ TUScheduler S(FSProvider, CDB, optsForTest(), captureDiags());
std::atomic<int> Builds(0), Actions(0);
Notification Start;
@@ -419,13 +418,13 @@
{
auto Opts = optsForTest();
Opts.UpdateDebounce = DebouncePolicy::fixed(std::chrono::milliseconds(50));
- TUScheduler S(CDB, Opts, captureDiags());
+ TUScheduler S(FSProvider, CDB, Opts, captureDiags());
std::vector<std::string> Files;
for (int I = 0; I < FilesCount; ++I) {
std::string Name = "foo" + std::to_string(I) + ".cpp";
Files.push_back(testPath(Name));
- this->Files[Files.back()] = "";
+ FSProvider.Files[Files.back()] = "";
}
StringRef Contents1 = R"cpp(int a;)cpp";
@@ -526,7 +525,7 @@
Opts.AsyncThreadsCount = 1;
Opts.RetentionPolicy.MaxRetainedASTs = 2;
trace::TestTracer Tracer;
- TUScheduler S(CDB, Opts);
+ TUScheduler S(FSProvider, CDB, Opts);
llvm::StringLiteral SourceContents = R"cpp(
int* a;
@@ -586,7 +585,7 @@
TEST_F(TUSchedulerTests, NoopChangesDontThrashCache) {
auto Opts = optsForTest();
Opts.RetentionPolicy.MaxRetainedASTs = 1;
- TUScheduler S(CDB, Opts);
+ TUScheduler S(FSProvider, CDB, Opts);
auto Foo = testPath("foo.cpp");
auto FooInputs = getInputs(Foo, "int x=1;");
@@ -612,13 +611,13 @@
}
TEST_F(TUSchedulerTests, EmptyPreamble) {
- TUScheduler S(CDB, optsForTest());
+ TUScheduler S(FSProvider, CDB, optsForTest());
auto Foo = testPath("foo.cpp");
auto Header = testPath("foo.h");
- Files[Header] = "void foo()";
- Timestamps[Header] = time_t(0);
+ FSProvider.Files[Header] = "void foo()";
+ FSProvider.Timestamps[Header] = time_t(0);
auto WithPreamble = R"cpp(
#include "foo.h"
int main() {}
@@ -653,7 +652,7 @@
TEST_F(TUSchedulerTests, RunWaitsForPreamble) {
// Testing strategy: we update the file and schedule a few preamble reads at
// the same time. All reads should get the same non-null preamble.
- TUScheduler S(CDB, optsForTest());
+ TUScheduler S(FSProvider, CDB, optsForTest());
auto Foo = testPath("foo.cpp");
auto NonEmptyPreamble = R"cpp(
#define FOO 1
@@ -681,13 +680,13 @@
}
TEST_F(TUSchedulerTests, NoopOnEmptyChanges) {
- TUScheduler S(CDB, optsForTest(), captureDiags());
+ TUScheduler S(FSProvider, CDB, optsForTest(), captureDiags());
auto Source = testPath("foo.cpp");
auto Header = testPath("foo.h");
- Files[Header] = "int a;";
- Timestamps[Header] = time_t(0);
+ FSProvider.Files[Header] = "int a;";
+ FSProvider.Timestamps[Header] = time_t(0);
std::string SourceContents = R"cpp(
#include "foo.h"
@@ -715,7 +714,7 @@
ASSERT_EQ(S.fileStats().lookup(Source).PreambleBuilds, 1u);
// Update to a header should cause a rebuild, though.
- Timestamps[Header] = time_t(1);
+ FSProvider.Timestamps[Header] = time_t(1);
ASSERT_TRUE(DoUpdate(SourceContents));
ASSERT_FALSE(DoUpdate(SourceContents));
ASSERT_EQ(S.fileStats().lookup(Source).ASTBuilds, 2u);
@@ -737,7 +736,7 @@
}
TEST_F(TUSchedulerTests, ForceRebuild) {
- TUScheduler S(CDB, optsForTest(), captureDiags());
+ TUScheduler S(FSProvider, CDB, optsForTest(), captureDiags());
auto Source = testPath("foo.cpp");
auto Header = testPath("foo.h");
@@ -761,11 +760,12 @@
Field(&Diag::Message,
"use of undeclared identifier 'a'")));
});
+ ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
// Add the header file. We need to recreate the inputs since we changed a
// file from underneath the test FS.
- Files[Header] = "int a;";
- Timestamps[Header] = time_t(1);
+ FSProvider.Files[Header] = "int a;";
+ FSProvider.Timestamps[Header] = time_t(1);
Inputs = getInputs(Source, SourceContents);
// The addition of the missing header file shouldn't trigger a rebuild since
@@ -776,6 +776,7 @@
++DiagCount;
ADD_FAILURE() << "Did not expect diagnostics for missing header update";
});
+ ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
// Forcing the reload should should cause a rebuild which no longer has any
// errors.
@@ -791,7 +792,7 @@
}
TEST_F(TUSchedulerTests, NoChangeDiags) {
trace::TestTracer Tracer;
- TUScheduler S(CDB, optsForTest(), captureDiags());
+ TUScheduler S(FSProvider, CDB, optsForTest(), captureDiags());
auto FooCpp = testPath("foo.cpp");
const auto *Contents = "int a; int b;";
@@ -830,7 +831,7 @@
}
TEST_F(TUSchedulerTests, Run) {
- TUScheduler S(CDB, optsForTest());
+ TUScheduler S(FSProvider, CDB, optsForTest());
std::atomic<int> Counter(0);
S.run("add 1", [&] { ++Counter; });
S.run("add 2", [&] { Counter += 2; });
@@ -931,7 +932,7 @@
// (!) 'Ready' must live longer than TUScheduler.
Notification Ready;
- TUScheduler S(CDB, optsForTest(), captureDiags());
+ TUScheduler S(FSProvider, CDB, optsForTest(), captureDiags());
std::vector<Diag> Diagnostics;
updateWithDiags(S, testPath("foo.cpp"), "void test() {}",
WantDiagnostics::Yes, [&](std::vector<Diag> D) {
@@ -955,7 +956,7 @@
// (!) 'Ready' must live longer than TUScheduler.
Notification Ready;
- TUScheduler S(CDB, optsForTest(), captureDiags());
+ TUScheduler S(FSProvider, CDB, optsForTest(), captureDiags());
std::vector<Diag> Diagnostics;
updateWithDiags(S, testPath("foo.cpp"), "void test() {}",
WantDiagnostics::Yes, [&](std::vector<Diag> D) {
@@ -1015,7 +1016,7 @@
static constexpr llvm::StringLiteral InputsV0 = "v0";
static constexpr llvm::StringLiteral InputsV1 = "v1";
Notification Ready;
- TUScheduler S(CDB, optsForTest(),
+ TUScheduler S(FSProvider, CDB, optsForTest(),
std::make_unique<BlockPreambleThread>(InputsV1, Ready));
Path File = testPath("foo.cpp");
Index: clang-tools-extra/clangd/TUScheduler.h
===================================================================
--- clang-tools-extra/clangd/TUScheduler.h
+++ clang-tools-extra/clangd/TUScheduler.h
@@ -13,6 +13,7 @@
#include "Diagnostics.h"
#include "GlobalCompilationDatabase.h"
#include "index/CanonicalIncludes.h"
+#include "support/FSProvider.h"
#include "support/Function.h"
#include "support/Path.h"
#include "support/Threading.h"
@@ -197,7 +198,8 @@
bool AsyncPreambleBuilds = false;
};
- TUScheduler(const GlobalCompilationDatabase &CDB, const Options &Opts,
+ TUScheduler(const FileSystemProvider &FSProvider,
+ const GlobalCompilationDatabase &CDB, const Options &Opts,
std::unique_ptr<ParsingCallbacks> ASTCallbacks = nullptr);
~TUScheduler();
@@ -304,6 +306,7 @@
static llvm::Optional<llvm::StringRef> getFileBeingProcessedInContext();
private:
+ const FileSystemProvider &FSProvider;
const GlobalCompilationDatabase &CDB;
const Options Opts;
std::unique_ptr<ParsingCallbacks> Callbacks; // not nullptr
Index: clang-tools-extra/clangd/TUScheduler.cpp
===================================================================
--- clang-tools-extra/clangd/TUScheduler.cpp
+++ clang-tools-extra/clangd/TUScheduler.cpp
@@ -55,6 +55,7 @@
#include "index/CanonicalIncludes.h"
#include "support/Cancellation.h"
#include "support/Context.h"
+#include "support/FSProvider.h"
#include "support/Logger.h"
#include "support/Path.h"
#include "support/Threading.h"
@@ -361,7 +362,8 @@
/// worker.
class ASTWorker {
friend class ASTWorkerHandle;
- ASTWorker(PathRef FileName, const GlobalCompilationDatabase &CDB,
+ ASTWorker(PathRef FileName, const FileSystemProvider &FSProvider,
+ const GlobalCompilationDatabase &CDB,
TUScheduler::ASTCache &LRUCache, Semaphore &Barrier, bool RunSync,
const TUScheduler::Options &Opts, ParsingCallbacks &Callbacks);
@@ -371,12 +373,11 @@
/// is null, all requests will be processed on the calling thread
/// synchronously instead. \p Barrier is acquired when processing each
/// request, it is used to limit the number of actively running threads.
- static ASTWorkerHandle create(PathRef FileName,
- const GlobalCompilationDatabase &CDB,
- TUScheduler::ASTCache &IdleASTs,
- AsyncTaskRunner *Tasks, Semaphore &Barrier,
- const TUScheduler::Options &Opts,
- ParsingCallbacks &Callbacks);
+ static ASTWorkerHandle
+ create(PathRef FileName, const FileSystemProvider &FSProvider,
+ const GlobalCompilationDatabase &CDB, TUScheduler::ASTCache &IdleASTs,
+ AsyncTaskRunner *Tasks, Semaphore &Barrier,
+ const TUScheduler::Options &Opts, ParsingCallbacks &Callbacks);
~ASTWorker();
void update(ParseInputs Inputs, WantDiagnostics);
@@ -455,6 +456,7 @@
const DebouncePolicy UpdateDebounce;
/// File that ASTWorker is responsible for.
const Path FileName;
+ const FileSystemProvider &FSProvider;
const GlobalCompilationDatabase &CDB;
/// Callback invoked when preamble or main file AST is built.
ParsingCallbacks &Callbacks;
@@ -541,13 +543,15 @@
};
ASTWorkerHandle ASTWorker::create(PathRef FileName,
+ const FileSystemProvider &FSProvider,
const GlobalCompilationDatabase &CDB,
TUScheduler::ASTCache &IdleASTs,
AsyncTaskRunner *Tasks, Semaphore &Barrier,
const TUScheduler::Options &Opts,
ParsingCallbacks &Callbacks) {
- std::shared_ptr<ASTWorker> Worker(new ASTWorker(
- FileName, CDB, IdleASTs, Barrier, /*RunSync=*/!Tasks, Opts, Callbacks));
+ std::shared_ptr<ASTWorker> Worker(
+ new ASTWorker(FileName, FSProvider, CDB, IdleASTs, Barrier,
+ /*RunSync=*/!Tasks, Opts, Callbacks));
if (Tasks) {
Tasks->runAsync("ASTWorker:" + llvm::sys::path::filename(FileName),
[Worker]() { Worker->run(); });
@@ -558,13 +562,15 @@
return ASTWorkerHandle(std::move(Worker));
}
-ASTWorker::ASTWorker(PathRef FileName, const GlobalCompilationDatabase &CDB,
+ASTWorker::ASTWorker(PathRef FileName, const FileSystemProvider &FSProvider,
+ const GlobalCompilationDatabase &CDB,
TUScheduler::ASTCache &LRUCache, Semaphore &Barrier,
bool RunSync, const TUScheduler::Options &Opts,
ParsingCallbacks &Callbacks)
: IdleASTs(LRUCache), RunSync(RunSync), UpdateDebounce(Opts.UpdateDebounce),
- FileName(FileName), CDB(CDB), Callbacks(Callbacks), Barrier(Barrier),
- Done(false), Status(FileName, Callbacks),
+ FileName(FileName), FSProvider(FSProvider), CDB(CDB),
+ Callbacks(Callbacks), Barrier(Barrier), Done(false),
+ Status(FileName, Callbacks),
PreamblePeer(FileName, Callbacks, Opts.StorePreamblesInMemory,
RunSync || !Opts.AsyncPreambleBuilds, Status, *this) {
// Set a fallback command because compile command can be accessed before
@@ -649,6 +655,9 @@
return;
}
+ // We need a new FS to be used by preamble thread, as the old one can be
+ // used by astworker thread and FS is not thread-safe.
+ Inputs.FS = FSProvider.getFileSystem();
PreamblePeer.update(std::move(Invocation), std::move(Inputs),
std::move(CompilerInvocationDiags), WantDiags);
// Block until first preamble is ready, as patching an empty preamble would
@@ -1201,10 +1210,11 @@
ASTWorkerHandle Worker;
};
-TUScheduler::TUScheduler(const GlobalCompilationDatabase &CDB,
+TUScheduler::TUScheduler(const FileSystemProvider &FSProvider,
+ const GlobalCompilationDatabase &CDB,
const Options &Opts,
std::unique_ptr<ParsingCallbacks> Callbacks)
- : CDB(CDB), Opts(Opts),
+ : FSProvider(FSProvider), CDB(CDB), Opts(Opts),
Callbacks(Callbacks ? move(Callbacks)
: std::make_unique<ParsingCallbacks>()),
Barrier(Opts.AsyncThreadsCount),
@@ -1244,7 +1254,7 @@
if (!FD) {
// Create a new worker to process the AST-related tasks.
ASTWorkerHandle Worker =
- ASTWorker::create(File, CDB, *IdleASTs,
+ ASTWorker::create(File, FSProvider, CDB, *IdleASTs,
WorkerThreads ? WorkerThreads.getPointer() : nullptr,
Barrier, Opts, *Callbacks);
FD = std::unique_ptr<FileData>(
Index: clang-tools-extra/clangd/ClangdServer.cpp
===================================================================
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -146,7 +146,7 @@
// FIXME(ioeric): this can be slow and we may be able to index on less
// critical paths.
WorkScheduler(
- CDB, TUScheduler::Options(Opts),
+ FSProvider, CDB, TUScheduler::Options(Opts),
std::make_unique<UpdateIndexCallbacks>(
DynamicIdx.get(), Callbacks, Opts.TheiaSemanticHighlighting)) {
// Adds an index to the stack, at higher priority than existing indexes.
@@ -190,7 +190,7 @@
// Compile command is set asynchronously during update, as it can be slow.
ParseInputs Inputs;
- Inputs.FS = FS;
+ Inputs.FS = std::move(FS);
Inputs.Contents = std::string(Contents);
Inputs.Version = Version.str();
Inputs.ForceRebuild = ForceRebuild;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
