[clangd] Use different FS in PreambleThread

[kadircet]

Summary

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.

[sammccall]

TL;DR: I think there are three viable paths that we should consider:
A) minimal change: put the FSProvider in ParseInputs
B) this patch, but with more documentation and safety
C) fight to clean up VFS multithreading semantics

Assuming we don't want to block on C, I'm not sure whether A or B is conceptually better. But A is a smaller change and brings side benefits, so wondering if you see advantages in B.

---

You're right about the race condition of course and I'm sure this patch is correct.
But this rubs me the wrong way a bit - the API change doesn't seem clearly motivated either from a high level (we conceptually *want* a single FS to build a single version of code) or from a low level (what are we actually doing in parallel that isn't threadsafe?).

Looking at the low level perspective first. VFS has three classes threadsafety issues:

• it contains the "working directory" as mutable state, which isn't multi-threading friendly. We do set working directory concurrently with other work, but always to the same thing. We could factor our way around this by setting the working directory prior to any concurrency (to CDB.Directory) and ensuring we never set it again.
• the RealFilesystem in real-working-directory mode is a singleton so its use is thread-hostile. This isn't really relevant to us because we use emulated-working-directory mode and have distinct instances. But it probably informed VFS's lack of useful threading semantics.
• read operations aren't const or documented as threadsafe even though the canonical RealFilesystem is. In-tree implementations seem to be threadsafe, some out-of-tree ones are not :-( This definitely bites us here.

I think it would be fundamentally possible to use a single VFS here. We need to modify the VFS interface to make concurrent reads safe (and const, while here) and fix any implementations. This isn't trivial. I think it's worthwhile, but experience tells me different projects want different things from the VFS interface...

The high-level view: IMO FSProvider is basically a hack around VFS's thread-safety issues, rather than a fundamental part of the application - it's a filesystem that doesn't have an attached working directory and whose read operations (e.g. getFileSystem()->status()) are threadsafe. We could rename FSProvider -> ThreadsafeFS and getFileSystem() -> newCursor(WD) or something, and maybe we should.
From this perspective, this patch is changing two things that have little to do with each other: it's replacing FS -> ThreadsafeFS and it's moving the FS ownership from one revision of the file to the TUScheduler itself.

I'm not really sure whether the latter is a good or a bad idea. If we want to do it, the use of ParseInputs as the interface to TUScheduler is a bit confusing - we should at least document that FS isn't used (as we do for CompileCommand) and probably assert that it's null, too.

But maybe simpler would be to "just" change the VFS member in ParseInputs to a const FSProvider*. This keeps the design approximately what it is now, but simplifies the ParseInputs concept: it's now truly readonly data, and functions that consume it won't have effects that escape.

clang-tools-extra/clangd/TUScheduler.cpp

629

Yikes, ISTM we're getting the FS from Inputs here, and from this->FSProvider below. We should use one or the other (i.e. FSProvider only, following this patch) unless we're trying to do something really subtle...

[kadircet]

In D80900#2066327, @sammccall wrote:

TL;DR: I think there are three viable paths that we should consider:
A) minimal change: put the FSProvider in ParseInputs
B) this patch, but with more documentation and safety
C) fight to clean up VFS multithreading semantics

Assuming we don't want to block on C, I'm not sure whether A or B is conceptually better. But A is a smaller change and brings side benefits, so wondering if you see advantages in B.

I didn't want to go for A) as I was afraid of satisfying the contract on FileSystemProvider::getFileSystem

/// Context::current() will be the context passed to the clang entrypoint,
/// such as addDocument(), and will also be propagated to result callbacks.

As this patch keep the FileSystemProvider inside TUScheduler.cpp and we need to ensure context is always the one we received in the entrypoint. Whereas ParseInputs is widely
passed around, and ensuring that assumption holds might be hard.

You're right about the race condition of course and I'm sure this patch is correct.
But this rubs me the wrong way a bit - the API change doesn't seem clearly motivated either from a high level (we conceptually *want* a single FS to build a single version of code) or from a low level (what are we actually doing in parallel that isn't threadsafe?).

Looking at the low level perspective first. VFS has three classes threadsafety issues:

• it contains the "working directory" as mutable state, which isn't multi-threading friendly. We do set working directory concurrently with other work, but always to the same thing. We could factor our way around this by setting the working directory prior to any concurrency (to CDB.Directory) and ensuring we never set it again.
• the RealFilesystem in real-working-directory mode is a singleton so its use is thread-hostile. This isn't really relevant to us because we use emulated-working-directory mode and have distinct instances. But it probably informed VFS's lack of useful threading semantics.
• read operations aren't const or documented as threadsafe even though the canonical RealFilesystem is. In-tree implementations seem to be threadsafe, some out-of-tree ones are not :-( This definitely bites us here.

I think it would be fundamentally possible to use a single VFS here. We need to modify the VFS interface to make concurrent reads safe (and const, while here) and fix any implementations. This isn't trivial. I think it's worthwhile, but experience tells me different projects want different things from the VFS interface...

I totally agree, this was actually my initial motivation. But I wasn't sure if it is worth the hassle, as this might receive some push-back from downstream users (VFS interface is not just in clang, but in llvm) and coming to a consensus is likely to take time.
It would be nice to clear that technical debt, but I don't think it provides any other practical benefits.

The high-level view: IMO FSProvider is basically a hack around VFS's thread-safety issues, rather than a fundamental part of the application - it's a filesystem that doesn't have an attached working directory and whose read operations (e.g. getFileSystem()->status()) are threadsafe. We could rename FSProvider -> ThreadsafeFS and getFileSystem() -> newCursor(WD) or something, and maybe we should.
From this perspective, this patch is changing two things that have little to do with each other: it's replacing FS -> ThreadsafeFS and it's moving the FS ownership from one revision of the file to the TUScheduler itself.

I'm not really sure whether the latter is a good or a bad idea. If we want to do it, the use of ParseInputs as the interface to TUScheduler is a bit confusing - we should at least document that FS isn't used (as we do for CompileCommand) and probably assert that it's null, too.

Right, the latter was a consequence of not storing FSProvider in ParseInputs, rather than a deliberate one. We can also push it around as a parameter to update, rather than a member if that looks more clear.
I didn't do that as it required more plumbing and change was actually local to TUScheduler.

But maybe simpler would be to "just" change the VFS member in ParseInputs to a const FSProvider*. This keeps the design approximately what it is now, but simplifies the ParseInputs concept: it's now truly readonly data, and functions that consume it won't have effects that escape.

Isn't this just what you proposed in A) ? My concern above applies to this one too. It is not a practical concern as of today, as most uses of ParseInputs is through callbacks in ASTWorker, hence the context should be implicitly set. Unless, someone attaches another async callback.

[sammccall]

In D80900#2069566, @kadircet wrote:

In D80900#2066327, @sammccall wrote:

TL;DR: I think there are three viable paths that we should consider:
A) minimal change: put the FSProvider in ParseInputs
B) this patch, but with more documentation and safety
C) fight to clean up VFS multithreading semantics

Assuming we don't want to block on C, I'm not sure whether A or B is conceptually better. But A is a smaller change and brings side benefits, so wondering if you see advantages in B.

I didn't want to go for A) as I was afraid of satisfying the contract on FileSystemProvider::getFileSystem

/// Context::current() will be the context passed to the clang entrypoint,
/// such as addDocument(), and will also be propagated to result callbacks.

As this patch keep the FileSystemProvider inside TUScheduler.cpp and we need to ensure context is always the one we received in the entrypoint. Whereas ParseInputs is widely
passed around, and ensuring that assumption holds might be hard.

That makes sense. However I don't think that contract is conceptually that great, having the ideas of "threadsafe FS" and "maybe-context-aware FS" coupled together is... weird. The benefit is that it's an easier contract to satisfy.

It's already the case that the context could/should be plumbed through to the actual FS operations and other extension points. Things like tracing rely on it. I think saying "we always plumb context through to everything" is a better contract even if it's a bit stronger than we need and harder to prove satisfied. This frees up FileSystemProvider to be the conceptually-simple ThreadsafeFS that could be lifted from clangd/support to llvm/support if we want.

WDYT?

[kadircet]

In D80900#2071207, @sammccall wrote:

That makes sense. However I don't think that contract is conceptually that great, having the ideas of "threadsafe FS" and "maybe-context-aware FS" coupled together is... weird. The benefit is that it's an easier contract to satisfy.

It's already the case that the context could/should be plumbed through to the actual FS operations and other extension points. Things like tracing rely on it. I think saying "we always plumb context through to everything" is a better contract even if it's a bit stronger than we need and harder to prove satisfied. This frees up FileSystemProvider to be the conceptually-simple ThreadsafeFS that could be lifted from clangd/support to llvm/support if we want.

WDYT?

Did that in D81173, there were two issues:

• Sometimes we create ParseInputs without an access to an FSProvider, overcome this one with a helper that wraps a VFS and returns an FSProvider for it.
• TestTU::inputs() needs to return a ParseInputs, but it is unclear which FSProvider it should point to :/ make it a parameter to the function. Since it needs to out-live the returned ParseInputs.

[kadircet]

we've went with D81173 instead.

clang-tools-extra/clangd/ClangdServer.cpp

: FSProvider(FSProvider),
       PreserveRecoveryASTType(Opts.PreserveRecoveryASTType),
       TweakFilter(Opts.TweakFilter), WorkspaceRoot(Opts.WorkspaceRoot),
       // Pass a callback into `WorkScheduler` to extract symbols from a newly
       // parsed file and rebuild the file index synchronously each time an AST
       // is parsed.
       // 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.
   auto AddIndex = [&](SymbolIndex *Idx) {
     if (this->Index != nullptr) {
       MergedIdx.push_back(std::make_unique<MergedIndex>(Idx, this->Index));
       this->Index = MergedIdx.back().get();
     } else {
void ClangdServer::addDocument(PathRef File, llvm::StringRef Contents,
   Opts.ClangTidyOpts = tidy::ClangTidyOptions::getDefaults();
   // FIXME: call tidy options builder on the worker thread, it can do IO.
   if (GetClangTidyOptions)
     Opts.ClangTidyOpts = GetClangTidyOptions(*FS, File);
   Opts.SuggestMissingIncludes = SuggestMissingIncludes;
 
   // 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;
   Inputs.Opts = std::move(Opts);
   Inputs.Index = Index;
   Inputs.Opts.BuildRecoveryAST = BuildRecoveryAST;
   Inputs.Opts.PreserveRecoveryASTType = PreserveRecoveryASTType;
   bool NewFile = WorkScheduler.update(File, Inputs, WantDiags);

clang-tools-extra/clangd/TUScheduler.h

 //===--- TUScheduler.h -------------------------------------------*-C++-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_TUSCHEDULER_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_TUSCHEDULER_H
 
 #include "Compiler.h"
 #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"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include <chrono>
struct Options {
     /// Determines when to keep idle ASTs in memory for future use.
     ASTRetentionPolicy RetentionPolicy;
 
     /// Whether to run PreamblePeer asynchronously.
     /// No-op if AsyncThreadsCount is 0.
     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();
 
   struct FileStats {
     std::size_t UsedBytes = 0;
     unsigned PreambleBuilds = 0;
     unsigned ASTBuilds = 0;
   };
public:
   // The file being built/processed in the current thread. This is a hack in
   // order to get the file name into the index implementations. Do not depend on
   // this inside clangd.
   // FIXME: remove this when there is proper index support via build system
   // integration.
   static llvm::Optional<llvm::StringRef> getFileBeingProcessedInContext();
 
 private:
+  const FileSystemProvider &FSProvider;
   const GlobalCompilationDatabase &CDB;
   const Options Opts;
   std::unique_ptr<ParsingCallbacks> Callbacks; // not nullptr
   Semaphore Barrier;
   llvm::StringMap<std::unique_ptr<FileData>> Files;
   std::unique_ptr<ASTCache> IdleASTs;
   // None when running tasks synchronously and non-None when running tasks
   // asynchronously.
   llvm::Optional<AsyncTaskRunner> PreambleTasks;
   llvm::Optional<AsyncTaskRunner> WorkerThreads;
 };
 
 } // namespace clangd
 } // namespace clang
 
 #endif

clang-tools-extra/clangd/TUScheduler.cpp

 #include "Compiler.h"
 #include "Diagnostics.h"
 #include "GlobalCompilationDatabase.h"
 #include "ParsedAST.h"
 #include "Preamble.h"
 #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"
 #include "support/Trace.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "llvm/ADT/FunctionExtras.h"
 #include "llvm/ADT/None.h"

 /// The ASTWorker that manages the AST is shared by both the processing thread
 /// and the TUScheduler. The TUScheduler should discard an ASTWorker when
 /// remove() is called, but its thread may be busy and we don't want to block.
 /// So the workers are accessed via an ASTWorkerHandle. Destroying the handle
 /// signals the worker to exit its run loop and gives up shared ownership of the
 /// 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);
 
 public:
   /// Create a new ASTWorker and return a handle to it.
   /// The processing thread is spawned using \p Tasks. However, when \p Tasks
   /// 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,
+         const TUScheduler::Options &Opts, ParsingCallbacks &Callbacks);
   ~ASTWorker();
 
   void update(ParseInputs Inputs, WantDiagnostics);
   void
   runWithAST(llvm::StringRef Name,
              llvm::unique_function<void(llvm::Expected<InputsAndAST>)> Action,
              TUScheduler::ASTActionInvalidation);
   bool blockUntilIdle(Deadline Timeout) const;
private:
 
   /// Handles retention of ASTs.
   TUScheduler::ASTCache &IdleASTs;
   const bool RunSync;
   /// Time to wait after an update to see whether another update obsoletes it.
   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;
   /// Latest build preamble for current TU.
   std::shared_ptr<const PreambleData> LatestPreamble;
   Notification BuiltFirstPreamble;
 
   Semaphore &Barrier;
public:
   /// are possible.
   std::shared_ptr<const ASTWorker> lock() { return Worker; }
 
 private:
   std::shared_ptr<ASTWorker> Worker;
 };
 
 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(); });
     Tasks->runAsync("PreambleWorker:" + llvm::sys::path::filename(FileName),
                     [Worker]() { Worker->PreamblePeer.run(); });
   }
 
   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
   // `Inputs` is initialized. Other fields are only used after initialization
   // from client inputs.
   FileInputs.CompileCommand = CDB.getFallbackCommand(FileName);
 }
 
auto Task = [=]() mutable {
 
     log("ASTWorker building file {0} version {1} with command {2}\n[{3}]\n{4}",
         FileName, Inputs.Version, Inputs.CompileCommand.Heuristic,
         Inputs.CompileCommand.Directory,
         llvm::join(Inputs.CompileCommand.CommandLine, " "));
 
     StoreDiags CompilerInvocationDiagConsumer;
     std::vector<std::string> CC1Args;
     std::unique_ptr<CompilerInvocation> Invocation = buildCompilerInvocation(
         Inputs, CompilerInvocationDiagConsumer, &CC1Args);
     // Log cc1 args even (especially!) if creating invocation failed.
     if (!CC1Args.empty())
       vlog("Driver produced command: cc1 {0}", llvm::join(CC1Args, " "));
     std::vector<Diag> CompilerInvocationDiags =
         CompilerInvocationDiagConsumer.take();
     if (!Invocation) {
       elog("Could not build CompilerInvocation for file {0}", FileName);
if (!Invocation) {
                                 Publish();
                             });
       // Make sure anyone waiting for the preamble gets notified it could not be
       // built.
       BuiltFirstPreamble.notify();
       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
     // imply rebuilding it from scratch.
     // This isn't the natural place to block, rather where the preamble would be
     // consumed. But that's too late, we'd be running on the worker thread with
     // the PreambleTask scheduled and so we'd deadlock.
     ReceivedPreamble.wait();

 }
 
 struct TUScheduler::FileData {
   /// Latest inputs, passed to TUScheduler::update().
   std::string Contents;
   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),
       IdleASTs(
           std::make_unique<ASTCache>(Opts.RetentionPolicy.MaxRetainedASTs)) {
   if (0 < Opts.AsyncThreadsCount) {
     PreambleTasks.emplace();
     WorkerThreads.emplace();

 
 bool TUScheduler::update(PathRef File, ParseInputs Inputs,
                          WantDiagnostics WantDiags) {
   std::unique_ptr<FileData> &FD = Files[File];
   bool NewFile = FD == nullptr;
   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>(
         new FileData{Inputs.Contents, std::move(Worker)});
   } else {
     FD->Contents = Inputs.Contents;
   }
   FD->Worker->update(std::move(Inputs), WantDiags);

clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp

TUScheduler::Options optsForTest() {
   return TUScheduler::Options(ClangdServer::optsForTest());
 }
 
 class TUSchedulerTests : public ::testing::Test {
 protected:
   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;
   }
 
   void updateWithCallback(TUScheduler &S, PathRef File,
                           llvm::StringRef Contents, WantDiagnostics WD,
                           llvm::unique_function<void()> CB) {
protected:
 
   void updateWithDiags(TUScheduler &S, PathRef File, llvm::StringRef Contents,
                        WantDiagnostics WD,
                        llvm::unique_function<void(std::vector<Diag>)> CB) {
     return updateWithDiags(S, File, getInputs(File, std::string(Contents)), WD,
                            std::move(CB));
   }
 
-  llvm::StringMap<std::string> Files;
-  llvm::StringMap<time_t> Timestamps;
+  MockFSProvider FSProvider;
   MockCompilationDatabase CDB;
 };
 
 Key<llvm::unique_function<void(PathRef File, std::vector<Diag>)>>
     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);
 
   // 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(

 }
 
 TEST_F(TUSchedulerTests, WantDiagnostics) {
   std::atomic<int> CallbackCount(0);
   {
     // 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(); });
     updateWithDiags(S, Path, "request diags", WantDiagnostics::Yes,
                     [&](std::vector<Diag>) { ++CallbackCount; });
     updateWithDiags(S, Path, "auto (clobbered)", WantDiagnostics::Auto,
                     [&](std::vector<Diag>) {
                       ADD_FAILURE()
TEST_F(TUSchedulerTests, WantDiagnostics) {
   EXPECT_EQ(2, CallbackCount);
 }
 
 TEST_F(TUSchedulerTests, Debounce) {
   std::atomic<int> CallbackCount(0);
   {
     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,
                     [&](std::vector<Diag>) {
                       ADD_FAILURE()
                           << "auto should have been debounced and canceled";
                     });
     std::this_thread::sleep_for(std::chrono::milliseconds(200));
TEST_F(TUSchedulerTests, Cancellation) {
   //   U2(WantDiags=Yes) <-- cancelled
   //    R2A              <-- cancelled
   //    R2B
   //   U3(WantDiags=Yes)
   //    R3               <-- cancelled
   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 {
       auto T = cancelableTask();
       WithContext C(std::move(T.first));
       updateWithDiags(
           S, Path, "//" + ID, WantDiagnostics::Yes,
           [&, ID](std::vector<Diag> Diags) { DiagsSeen.push_back(ID); });
TEST_F(TUSchedulerTests, Cancellation) {
   EXPECT_THAT(ReadsSeen, ElementsAre("R2B"))
       << "All reads other than R2B were cancelled";
   EXPECT_THAT(ReadsCanceled, ElementsAre("R1", "R2A", "R3"))
       << "All reads other than R2B were cancelled";
 }
 
 TEST_F(TUSchedulerTests, InvalidationNoCrash) {
   auto Path = testPath("foo.cpp");
-  TUScheduler S(CDB, optsForTest(), captureDiags());
+  TUScheduler S(FSProvider, CDB, optsForTest(), captureDiags());
 
   Notification StartedRunning;
   Notification ScheduledChange;
   // We expect invalidation logic to not crash by trying to invalidate a running
   // request.
   S.update(Path, getInputs(Path, ""), WantDiagnostics::Auto);
   ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
   S.runWithAST(
       "invalidatable-but-running", Path,
       [&](llvm::Expected<InputsAndAST> AST) {
         StartedRunning.notify();
         ScheduledChange.wait();
         ASSERT_TRUE(bool(AST));
       },
       TUScheduler::InvalidateOnUpdate);
   StartedRunning.wait();
   S.update(Path, getInputs(Path, ""), WantDiagnostics::Auto);
   ScheduledChange.notify();
   ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
 }
 
 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;
   updateWithDiags(S, Path, "a", WantDiagnostics::Yes, [&](std::vector<Diag>) {
     ++Builds;
     Start.wait();
   });
   S.runWithAST(
TEST_F(TUSchedulerTests, ManyUpdates) {
   int TotalPreambleReads = 0;
   int TotalUpdates = 0;
   llvm::StringMap<int> LatestDiagVersion;
 
   // Run TUScheduler and collect some stats.
   {
     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";
     StringRef Contents2 = R"cpp(int main() { return 1; })cpp";
     StringRef Contents3 = R"cpp(int a; int b; int sum() { return a + b; })cpp";
 
     StringRef AllContents[] = {Contents1, Contents2, Contents3};
     const int AllContentsSize = 3;

 }
 
 TEST_F(TUSchedulerTests, EvictedAST) {
   std::atomic<int> BuiltASTCounter(0);
   auto Opts = optsForTest();
   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;
     double* b = a;
   )cpp";
   llvm::StringLiteral OtherSourceContents = R"cpp(
     int* a;
     double* b = a + 0;

 }
 
 // We send "empty" changes to TUScheduler when we think some external event
 // *might* have invalidated current state (e.g. a header was edited).
 // Verify that this doesn't evict our cache entries.
 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;");
   auto Bar = testPath("bar.cpp");
   auto BarInputs = getInputs(Bar, "int x=2;");
 
   // After opening Foo then Bar, AST cache contains Bar.
   S.update(Foo, FooInputs, WantDiagnostics::Auto);
TEST_F(TUSchedulerTests, NoopChangesDontThrashCache) {
   ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
   ASSERT_THAT(S.getFilesWithCachedAST(), ElementsAre(Bar));
   // In fact each file has been built only once.
   ASSERT_EQ(S.fileStats().lookup(Foo).ASTBuilds, 1u);
   ASSERT_EQ(S.fileStats().lookup(Bar).ASTBuilds, 1u);
 }
 
 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() {}
   )cpp";
   auto WithEmptyPreamble = R"cpp(int main() {})cpp";
   S.update(Foo, getInputs(Foo, WithPreamble), WantDiagnostics::Auto);
   S.runWithPreamble(
       "getNonEmptyPreamble", Foo, TUScheduler::Stale,
S.runWithPreamble(
             cantFail(std::move(Preamble)).Preamble->Preamble.getBounds().Size,
             0u);
       });
 }
 
 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
     #define BAR 2
 
     int main() {}
   )cpp";
   constexpr int ReadsToSchedule = 10;
TEST_F(TUSchedulerTests, RunWaitsForPreamble) {
   ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
   // Check all actions got the same non-null preamble.
   std::lock_guard<std::mutex> Lock(PreamblesMut);
   ASSERT_NE(Preambles[0], nullptr);
   ASSERT_THAT(Preambles, Each(Preambles[0]));
 }
 
 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"
       int b = a;
     )cpp";
 
   // Return value indicates if the updated callback was received.
   auto DoUpdate = [&](std::string Contents) -> bool {
TEST_F(TUSchedulerTests, NoopOnEmptyChanges) {
   ASSERT_TRUE(DoUpdate(SourceContents));
   ASSERT_EQ(S.fileStats().lookup(Source).ASTBuilds, 1u);
   ASSERT_EQ(S.fileStats().lookup(Source).PreambleBuilds, 1u);
   ASSERT_FALSE(DoUpdate(SourceContents));
   ASSERT_EQ(S.fileStats().lookup(Source).ASTBuilds, 1u);
   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);
   ASSERT_EQ(S.fileStats().lookup(Source).PreambleBuilds, 2u);
 
   // Update to the contents should cause a rebuild.
   SourceContents += "\nint c = b;";
   ASSERT_TRUE(DoUpdate(SourceContents));
   ASSERT_FALSE(DoUpdate(SourceContents));
   ASSERT_EQ(S.fileStats().lookup(Source).ASTBuilds, 3u);
   ASSERT_EQ(S.fileStats().lookup(Source).PreambleBuilds, 2u);
 
   // Update to the compile commands should also cause a rebuild.
   CDB.ExtraClangFlags.push_back("-DSOMETHING");
   ASSERT_TRUE(DoUpdate(SourceContents));
   ASSERT_FALSE(DoUpdate(SourceContents));
   ASSERT_EQ(S.fileStats().lookup(Source).ASTBuilds, 4u);
   ASSERT_EQ(S.fileStats().lookup(Source).PreambleBuilds, 3u);
 }
 
 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");
 
   auto SourceContents = R"cpp(
       #include "foo.h"
       int b = a;
     )cpp";
 
   ParseInputs Inputs = getInputs(Source, SourceContents);
   std::atomic<size_t> DiagCount(0);
 
   // Update the source contents, which should trigger an initial build with
   // the header file missing.
   updateWithDiags(
       S, Source, Inputs, WantDiagnostics::Yes,
       [&DiagCount](std::vector<Diag> Diags) {
         ++DiagCount;
         EXPECT_THAT(Diags,
                     ElementsAre(Field(&Diag::Message, "'foo.h' file not found"),
                                 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
   // we don't track missing files.
   updateWithDiags(
       S, Source, Inputs, WantDiagnostics::Yes,
       [&DiagCount](std::vector<Diag> Diags) {
         ++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.
   Inputs.ForceRebuild = true;
   updateWithDiags(S, Source, Inputs, WantDiagnostics::Yes,
                   [&DiagCount](std::vector<Diag> Diags) {
                     ++DiagCount;
                     EXPECT_THAT(Diags, IsEmpty());
                   });
 
   ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
   EXPECT_EQ(DiagCount, 2U);
 }
 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;";
 
   EXPECT_THAT(Tracer.takeMetric("ast_access_read", "hit"), SizeIs(0));
   EXPECT_THAT(Tracer.takeMetric("ast_access_read", "miss"), SizeIs(0));
   EXPECT_THAT(Tracer.takeMetric("ast_access_diag", "hit"), SizeIs(0));
   EXPECT_THAT(Tracer.takeMetric("ast_access_diag", "miss"), SizeIs(0));
TEST_F(TUSchedulerTests, NoChangeDiags) {
   // diags have previously been reported and the inputs didn't change.
   updateWithDiags(
       S, FooCpp, Contents, WantDiagnostics::Auto,
       [&](std::vector<Diag>) { ADD_FAILURE() << "Should not be called."; });
   ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
 }
 
 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; });
   ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
   EXPECT_EQ(Counter.load(), 3);
 
   Notification TaskRun;
   Key<int> TestKey;

 
 TEST_F(TUSchedulerTests, CommandLineErrors) {
   // We should see errors from command-line parsing inside the main file.
   CDB.ExtraClangFlags = {"-fsome-unknown-flag"};
 
   // (!) '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) {
                     Diagnostics = std::move(D);
                     Ready.notify();
                   });
   Ready.wait();
 
   EXPECT_THAT(
       Diagnostics,
       ElementsAre(AllOf(
           Field(&Diag::ID, Eq(diag::err_drv_unknown_argument)),
           Field(&Diag::Name, Eq("drv_unknown_argument")),
           Field(&Diag::Message, "unknown argument: '-fsome-unknown-flag'"))));
 }
 
 TEST_F(TUSchedulerTests, CommandLineWarnings) {
   // We should not see warnings from command-line parsing.
   CDB.ExtraClangFlags = {"-Wsome-unknown-warning"};
 
   // (!) '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) {
                     Diagnostics = std::move(D);
                     Ready.notify();
                   });
   Ready.wait();
 
TEST_F(TUSchedulerTests, AsyncPreambleThread) {
   private:
     llvm::StringRef BlockVersion;
     Notification &N;
   };
 
   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");
   auto PI = getInputs(File, "");
   PI.Version = InputsV0.str();
   S.update(File, PI, WantDiagnostics::Auto);
   S.blockUntilIdle(timeoutSeconds(10));
 

clang-tools-extra/clangd/unittests/TestFS.h

 llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>
 buildTestFS(llvm::StringMap<std::string> const &Files,
             llvm::StringMap<time_t> const &Timestamps = {});
 
 // A VFS provider that returns TestFSes containing a provided set of files.
 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.
 class MockCompilationDatabase : public GlobalCompilationDatabase {
 public:
   /// If \p Directory is not empty, use that as the Directory field of the
   /// CompileCommand, and as project SourceRoot.
   ///