[clangd] Don't expose vfs in TUScheduler::runWithPreamble.

Summary:
It was previously an easy way to concurrently access a mutable vfs,
which is a recipe for disaster.

Reviewers: sammccall

Reviewed By: sammccall

Subscribers: klimek, jkorous-apple, cfe-commits, ioeric

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

llvm-svn: 327537

clang-tools-extra/clangd/ClangdServer.cpp

@@ -159,12 +159,11 @@ void ClangdServer::codeComplete(PathRef File, Position Pos,
                   llvm::Expected<InputsAndPreamble> IP) {
     assert(IP && "error when trying to read preamble for codeComplete");
     auto PreambleData = IP->Preamble;
-    auto &Command = IP->Inputs.CompileCommand;
 
     // FIXME(ibiryukov): even if Preamble is non-null, we may want to check
     // both the old and the new version in case only one of them matches.
     CompletionList Result = clangd::codeComplete(
-        File, Command, PreambleData ? &PreambleData->Preamble : nullptr,
+        File, IP->Command, PreambleData ? &PreambleData->Preamble : nullptr,
         Contents, Pos, FS, PCHs, CodeCompleteOpts);
     CB(std::move(Result));
   };
@@ -191,8 +190,7 @@ void ClangdServer::signatureHelp(PathRef File, Position Pos,
       return CB(IP.takeError());
 
     auto PreambleData = IP->Preamble;
-    auto &Command = IP->Inputs.CompileCommand;
-    CB(clangd::signatureHelp(File, Command,
+    CB(clangd::signatureHelp(File, IP->Command,
                              PreambleData ? &PreambleData->Preamble : nullptr,
                              Contents, Pos, FS, PCHs));
   };

clang-tools-extra/clangd/TUScheduler.cpp

@@ -407,7 +407,8 @@ unsigned getDefaultAsyncThreadsCount() {
 
 struct TUScheduler::FileData {
   /// Latest inputs, passed to TUScheduler::update().
-  ParseInputs Inputs;
+  std::string Contents;
+  tooling::CompileCommand Command;
   ASTWorkerHandle Worker;
 };
 
@@ -456,9 +457,11 @@ void TUScheduler::update(PathRef File, ParseInputs Inputs,
         File, WorkerThreads ? WorkerThreads.getPointer() : nullptr, Barrier,
         CppFile(File, StorePreamblesInMemory, PCHOps, ASTCallback),
         UpdateDebounce);
-    FD = std::unique_ptr<FileData>(new FileData{Inputs, std::move(Worker)});
+    FD = std::unique_ptr<FileData>(new FileData{
+        Inputs.Contents, Inputs.CompileCommand, std::move(Worker)});
   } else {
-    FD->Inputs = Inputs;
+    FD->Contents = Inputs.Contents;
+    FD->Command = Inputs.CompileCommand;
   }
   FD->Worker->update(std::move(Inputs), WantDiags, std::move(OnUpdated));
 }
@@ -500,26 +503,28 @@ void TUScheduler::runWithPreamble(
     SPAN_ATTACH(Tracer, "file", File);
     std::shared_ptr<const PreambleData> Preamble =
         It->second->Worker->getPossiblyStalePreamble();
-    Action(InputsAndPreamble{It->second->Inputs, Preamble.get()});
+    Action(InputsAndPreamble{It->second->Contents, It->second->Command,
+                             Preamble.get()});
     return;
   }
 
-  ParseInputs InputsCopy = It->second->Inputs;
   std::shared_ptr<const ASTWorker> Worker = It->second->Worker.lock();
-  auto Task = [InputsCopy, Worker, this](std::string Name, std::string File,
-                                         Context Ctx,
-                                         decltype(Action) Action) mutable {
+  auto Task = [Worker, this](std::string Name, std::string File,
+                             std::string Contents,
+                             tooling::CompileCommand Command, Context Ctx,
+                             decltype(Action) Action) mutable {
     std::lock_guard<Semaphore> BarrierLock(Barrier);
     WithContext Guard(std::move(Ctx));
     trace::Span Tracer(Name);
     SPAN_ATTACH(Tracer, "file", File);
     std::shared_ptr<const PreambleData> Preamble =
         Worker->getPossiblyStalePreamble();
-    Action(InputsAndPreamble{InputsCopy, Preamble.get()});
+    Action(InputsAndPreamble{Contents, Command, Preamble.get()});
   };
 
   PreambleTasks->runAsync("task:" + llvm::sys::path::filename(File),
                           Bind(Task, std::string(Name), std::string(File),
+                               It->second->Contents, It->second->Command,
                                Context::current().clone(), std::move(Action)));
 }
 

clang-tools-extra/clangd/TUScheduler.h

@@ -29,7 +29,8 @@ struct InputsAndAST {
 };
 
 struct InputsAndPreamble {
-  const ParseInputs &Inputs;
+  llvm::StringRef Contents;
+  const tooling::CompileCommand &Command;
   const PreambleData *Preamble;
 };
 
@@ -78,11 +79,14 @@ public:
   void runWithAST(llvm::StringRef Name, PathRef File,
                   Callback<InputsAndAST> Action);
 
-  /// Schedule an async read of the Preamble. Preamble passed to \p Action may
-  /// be built for any version of the file, callers must not rely on it being
-  /// consistent with the current version of the file.
-  /// If an error occurs during processing, it is forwarded to the \p Action
-  /// callback.
+  /// Schedule an async read of the Preamble.
+  /// The preamble may be stale, generated from an older version of the file.
+  /// Reading from locations in the preamble may cause the files to be re-read.
+  /// This gives callers two options:
+  /// - validate that the preamble is still valid, and only use it in this case
+  /// - accept that preamble contents may be outdated, and try to avoid reading
+  /// source code from headers. If an error occurs during processing, it is
+  /// forwarded to the \p Action callback.
   void runWithPreamble(llvm::StringRef Name, PathRef File,
                        Callback<InputsAndPreamble> Action);
 

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

@@ -226,8 +226,7 @@ TEST_F(TUSchedulerTests, ManyUpdates) {
                 EXPECT_THAT(Context::current().get(NonceKey), Pointee(Nonce));
 
                 ASSERT_TRUE((bool)Preamble);
-                EXPECT_EQ(Preamble->Inputs.FS, Inputs.FS);
-                EXPECT_EQ(Preamble->Inputs.Contents, Inputs.Contents);
+                EXPECT_EQ(Preamble->Contents, Inputs.Contents);
 
                 std::lock_guard<std::mutex> Lock(Mut);
                 ++TotalPreambleReads;