[clangd] Moved caching of compile commands to ClangdServer

[ilya-biryukov]

Summary

It allows to get rid of CppFile::getLastCommand and simplify the
code in the upcoming threading patch.

[sammccall]

Thanks for this cleanup!

The way we deal with ResourceDir still doesn't feel ideal, but that's no worse here and everything else is better!

clangd/ClangdServer.cpp

199

nit: the comment and whitespace seems unneccesary, this is exactly what invalidate() is documented to do.
(A comment saying *why* might be useful, but we don't really know why here)

272–273

nit: remove blank line?

clangd/ClangdServer.h

340

nit: can we hide this in the cpp file or move to GCD.h to reduce clutter?

340

Can the comment give some motivation here? It's clear enough from the name what it does, but why?

341

sorry for naive question - this doesn't need to be threadsafe, because clangdserver itself isn't, right?
I never really looked at the threading model of clangdserver - it looks like most stuff is called externally on one thread, and any async logic avoids accessing most of the members?

[ilya-biryukov]

• Addressed review comments

clangd/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   Support
   )
 
 add_clang_library(clangDaemon
   ClangdLSPServer.cpp
   ClangdServer.cpp
   ClangdUnit.cpp
   ClangdUnitStore.cpp
   CodeComplete.cpp
   CodeCompletionStrings.cpp
-  Context.cpp
+  CompileArgsCache.cpp
   Compiler.cpp
+  Context.cpp
   DraftStore.cpp
   FuzzyMatch.cpp
   GlobalCompilationDatabase.cpp
   JSONExpr.cpp
   JSONRPCDispatcher.cpp
   Logger.cpp
   Protocol.cpp
   ProtocolHandlers.cpp

clangd/ClangdServer.h

 //===--- ClangdServer.h - Main clangd server code ----------------*- C++-*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_CLANGDSERVER_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_CLANGDSERVER_H
 
 #include "ClangdUnit.h"
 #include "ClangdUnitStore.h"
 #include "CodeComplete.h"
+#include "CompileArgsCache.h"
 #include "DraftStore.h"
 #include "Function.h"
 #include "GlobalCompilationDatabase.h"
 #include "Protocol.h"
 #include "index/FileIndex.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
private:
   /// slow. Think of a way to cache this.
   llvm::Expected<tooling::Replacements>
   formatCode(llvm::StringRef Code, PathRef File,
              ArrayRef<tooling::Range> Ranges);
 
   std::future<Context>
   scheduleReparseAndDiags(Context Ctx, PathRef File, VersionedDraft Contents,
                           std::shared_ptr<CppFile> Resources,
-                          Tagged<IntrusiveRefCntPtr<vfs::FileSystem>> TaggedFS,
-                          bool AllowCachedCompileFlags);
+                          Tagged<IntrusiveRefCntPtr<vfs::FileSystem>> TaggedFS);
 
   std::future<Context>
   scheduleCancelRebuild(Context Ctx, std::shared_ptr<CppFile> Resources);
 
-  GlobalCompilationDatabase &CDB;
+  CompileArgsCache CompileArgs;
   DiagnosticsConsumer &DiagConsumer;
   FileSystemProvider &FSProvider;
   DraftStore DraftMgr;
   // The index used to look up symbols. This could be:
   //   - null (all index functionality is optional)
   //   - the dynamic index owned by ClangdServer (FileIdx)
   //   - the static index passed to the constructor
   //   - a merged view of a static and dynamic index (MergedIndex)
   SymbolIndex *Index;
   // If present, an up-to-date of symbols in open files. Read via Index.
   std::unique_ptr<FileIndex> FileIdx;
   // If present, a merged view of FileIdx and an external index. Read via Index.
   std::unique_ptr<SymbolIndex> MergedIndex;
   CppFileCollection Units;
-  std::string ResourceDir;
   // If set, this represents the workspace path.
   llvm::Optional<std::string> RootPath;
   std::shared_ptr<PCHContainerOperations> PCHs;
   bool StorePreamblesInMemory;
   /// Used to serialize diagnostic callbacks.
   /// FIXME(ibiryukov): get rid of an extra map and put all version counters
   /// into CppFile.
   std::mutex DiagnosticsMutex;

clangd/ClangdServer.cpp

 #include "llvm/Support/raw_ostream.h"
 #include <future>
 
 using namespace clang;
 using namespace clang::clangd;
 
 namespace {
 
-tooling::CompileCommand getCompileCommand(GlobalCompilationDatabase &CDB,
-                                          PathRef File, PathRef ResourceDir) {
-  llvm::Optional<tooling::CompileCommand> C = CDB.getCompileCommand(File);
-  if (!C) // FIXME: Suppress diagnostics? Let the user know?
-    C = CDB.getFallbackCommand(File);
-
-  // Inject the resource dir.
-  // FIXME: Don't overwrite it if it's already there.
-  C->CommandLine.push_back("-resource-dir=" + ResourceDir.str());
-  return std::move(*C);
-}
-
 std::string getStandardResourceDir() {
   static int Dummy; // Just an address in this process.
   return CompilerInvocation::GetResourcesPath("clangd", (void *)&Dummy);
 }
 
 class RefactoringResultCollector final
     : public tooling::RefactoringResultConsumer {
 public:

 
 ClangdServer::ClangdServer(GlobalCompilationDatabase &CDB,
                            DiagnosticsConsumer &DiagConsumer,
                            FileSystemProvider &FSProvider,
                            unsigned AsyncThreadsCount,
                            bool StorePreamblesInMemory,
                            bool BuildDynamicSymbolIndex, SymbolIndex *StaticIdx,
                            llvm::Optional<StringRef> ResourceDir)
-    : CDB(CDB), DiagConsumer(DiagConsumer), FSProvider(FSProvider),
+    : CompileArgs(CDB,
+                  ResourceDir ? ResourceDir->str() : getStandardResourceDir()),
+      DiagConsumer(DiagConsumer), FSProvider(FSProvider),
       FileIdx(BuildDynamicSymbolIndex ? new FileIndex() : nullptr),
       // Pass a callback into `Units` 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.
       Units(FileIdx
                 ? [this](const Context &Ctx, PathRef Path,
                          ParsedAST *AST) { FileIdx->update(Ctx, Path, AST); }
                 : ASTParsedCallback()),
-      ResourceDir(ResourceDir ? ResourceDir->str() : getStandardResourceDir()),
       PCHs(std::make_shared<PCHContainerOperations>()),
       StorePreamblesInMemory(StorePreamblesInMemory),
       WorkScheduler(AsyncThreadsCount) {
   if (FileIdx && StaticIdx) {
     MergedIndex = mergeIndex(FileIdx.get(), StaticIdx);
     Index = MergedIndex.get();
   } else if (FileIdx)
     Index = FileIdx.get();

 }
 
 std::future<Context> ClangdServer::addDocument(Context Ctx, PathRef File,
                                                StringRef Contents) {
   DocVersion Version = DraftMgr.updateDraft(File, Contents);
 
   auto TaggedFS = FSProvider.getTaggedFileSystem(File);
   std::shared_ptr<CppFile> Resources =
-      Units.getOrCreateFile(File, ResourceDir, StorePreamblesInMemory, PCHs);
+      Units.getOrCreateFile(File, StorePreamblesInMemory, PCHs);
   return scheduleReparseAndDiags(std::move(Ctx), File,
                                  VersionedDraft{Version, Contents.str()},
-                                 std::move(Resources), std::move(TaggedFS),
-                                 /*AllowCachedCompileFlags=*/true);
+                                 std::move(Resources), std::move(TaggedFS));
 }
 
 std::future<Context> ClangdServer::removeDocument(Context Ctx, PathRef File) {
   DraftMgr.removeDraft(File);
+  CompileArgs.invalidate(File);
+
   std::shared_ptr<CppFile> Resources = Units.removeIfPresent(File);
   return scheduleCancelRebuild(std::move(Ctx), std::move(Resources));
 }
 
 std::future<Context> ClangdServer::forceReparse(Context Ctx, PathRef File) {
   auto FileContents = DraftMgr.getDraft(File);
   assert(FileContents.Draft &&
          "forceReparse() was called for non-added document");
 
+  // forceReparse promises to request new compilation flags from CDB, so we
+  // remove any cahced flags.
+  CompileArgs.invalidate(File);
+
   auto TaggedFS = FSProvider.getTaggedFileSystem(File);
   std::shared_ptr<CppFile> Resources =
-      Units.getOrCreateFile(File, ResourceDir, StorePreamblesInMemory, PCHs);
+      Units.getOrCreateFile(File, StorePreamblesInMemory, PCHs);
   return scheduleReparseAndDiags(std::move(Ctx), File, FileContents,
-                                 std::move(Resources), std::move(TaggedFS),
-                                 /*AllowCachedCompileFlags=*/false);
+                                 std::move(Resources), std::move(TaggedFS));
 }
 
 std::future<std::pair<Context, Tagged<CompletionList>>>
 ClangdServer::codeComplete(Context Ctx, PathRef File, Position Pos,
                            const clangd::CodeCompleteOptions &Opts,
                            llvm::Optional<StringRef> OverridenContents,
                            IntrusiveRefCntPtr<vfs::FileSystem> *UsedFS) {
   using ResultType = std::pair<Context, Tagged<CompletionList>>;
void ClangdServer::codeComplete(
   auto CodeCompleteOpts = Opts;
   if (!CodeCompleteOpts.Index) // Respect overridden index.
     CodeCompleteOpts.Index = Index;
 
   // Copy File, as it is a PathRef that will go out of scope before Task is
   // executed.
   Path FileStr = File;
   // Copy PCHs to avoid accessing this->PCHs concurrently
   std::shared_ptr<PCHContainerOperations> PCHs = this->PCHs;
-
-  assert(Resources->getLastCommand() &&
-         "CppFile is in inconsistent state, missing CompileCommand");
-  tooling::CompileCommand CompileCommand = *Resources->getLastCommand();
-
+  tooling::CompileCommand CompileCommand = CompileArgs.getCompileCommand(File);
   // A task that will be run asynchronously.
   auto Task =
       // 'mutable' to reassign Preamble variable.
       [FileStr, Preamble, Resources, Contents, Pos, CodeCompleteOpts, TaggedFS,
        PCHs, CompileCommand](Context Ctx, CallbackType Callback) mutable {
         if (!Preamble) {
           // Maybe we built some preamble before processing this request.
           Preamble = Resources->getPossiblyStalePreamble();
if (UsedFS)
     *UsedFS = TaggedFS.Value;
 
   std::shared_ptr<CppFile> Resources = Units.getFile(File);
   if (!Resources)
     return llvm::make_error<llvm::StringError>(
         "signatureHelp is called for non-added document",
         llvm::errc::invalid_argument);
 
-  assert(Resources->getLastCommand() &&
-         "CppFile is in inconsistent state, missing CompileCommand");
-
   auto Preamble = Resources->getPossiblyStalePreamble();
   auto Result =
-      clangd::signatureHelp(Ctx, File, *Resources->getLastCommand(),
+      clangd::signatureHelp(Ctx, File, CompileArgs.getCompileCommand(File),
                             Preamble ? &Preamble->Preamble : nullptr,
                             *OverridenContents, Pos, TaggedFS.Value, PCHs);
   return make_tagged(std::move(Result), TaggedFS.Tag);
 }
 
 llvm::Expected<tooling::Replacements>
 ClangdServer::formatRange(StringRef Code, PathRef File, Range Rng) {
   size_t Begin = positionToOffset(Code, Rng.start);
ClangdServer::findDocumentHighlights(const Context &Ctx, PathRef File,
   if (Err)
     return std::move(*Err);
   return make_tagged(Result, TaggedFS.Tag);
 }
 
 std::future<Context> ClangdServer::scheduleReparseAndDiags(
     Context Ctx, PathRef File, VersionedDraft Contents,
     std::shared_ptr<CppFile> Resources,
-    Tagged<IntrusiveRefCntPtr<vfs::FileSystem>> TaggedFS,
-    bool AllowCachedCompileFlags) {
-  llvm::Optional<tooling::CompileCommand> ReusedCommand;
-  if (AllowCachedCompileFlags)
-    ReusedCommand = Resources->getLastCommand();
-  tooling::CompileCommand Command =
-      ReusedCommand ? std::move(*ReusedCommand)
-                    : getCompileCommand(CDB, File, ResourceDir);
-
-  ParseInputs Inputs = {std::move(Command), std::move(TaggedFS.Value),
-                        *std::move(Contents.Draft)};
+    Tagged<IntrusiveRefCntPtr<vfs::FileSystem>> TaggedFS) {
   assert(Contents.Draft && "Draft must have contents");
+  ParseInputs Inputs = {CompileArgs.getCompileCommand(File),
+                        std::move(TaggedFS.Value), *std::move(Contents.Draft)};
+
   UniqueFunction<llvm::Optional<std::vector<DiagWithFixIts>>(const Context &)>
       DeferredRebuild = Resources->deferRebuild(std::move(Inputs));
   std::promise<Context> DonePromise;
   std::future<Context> DoneFuture = DonePromise.get_future();
 
   DocVersion Version = Contents.Version;
   Path FileStr = File;
   VFSTag Tag = TaggedFS.Tag;

clangd/ClangdUnit.h

public:
 
   /// Returns a future to get the most fresh AST for a file. Returned AST is
   /// wrapped to prevent concurrent accesses.
   /// We use std::shared_ptr here because MVSC fails to compile non-copyable
   /// classes as template arguments of promise/future. It is guaranteed to
   /// always be non-null.
   std::shared_future<std::shared_ptr<ParsedASTWrapper>> getAST() const;
 
-  /// Get the latest CompileCommand used to build this CppFile. Returns
-  /// llvm::None before first call to rebuild() or after calls to
-  /// cancelRebuild().
-  // In practice we always call rebuild() when adding a CppFile to the
-  // CppFileCollection, and only `cancelRebuild()` after removing it. This means
-  // files in the CppFileCollection always have a compile command available.
-  llvm::Optional<tooling::CompileCommand> getLastCommand() const;
-
 private:
   /// A helper guard that manages the state of CppFile during rebuild.
   class RebuildGuard {
   public:
     RebuildGuard(CppFile &File, unsigned RequestRebuildCounter);
     ~RebuildGuard();
 
     bool wasCancelledBeforeConstruction() const;
private:
   /// mutated.
   mutable std::mutex Mutex;
   /// A counter to cancel old rebuilds.
   unsigned RebuildCounter;
   /// Used to wait when rebuild is finished before starting another one.
   bool RebuildInProgress;
   /// Condition variable to indicate changes to RebuildInProgress.
   std::condition_variable RebuildCond;
-  llvm::Optional<tooling::CompileCommand> LastCommand;
 
   /// Promise and future for the latests AST. Fulfilled during rebuild.
   /// We use std::shared_ptr here because MVSC fails to compile non-copyable
   /// classes as template arguments of promise/future.
   std::promise<std::shared_ptr<ParsedASTWrapper>> ASTPromise;
   std::shared_future<std::shared_ptr<ParsedASTWrapper>> ASTFuture;
 
   /// Promise and future for the latests Preamble. Fulfilled during rebuild.

clangd/ClangdUnit.cpp

if (futureIsReady(this->PreambleFuture)) {
       this->PreamblePromise =
           std::promise<std::shared_ptr<const PreambleData>>();
       this->PreambleFuture = this->PreamblePromise.get_future();
     }
     if (futureIsReady(this->ASTFuture)) {
       this->ASTPromise = std::promise<std::shared_ptr<ParsedASTWrapper>>();
       this->ASTFuture = this->ASTPromise.get_future();
     }
-    this->LastCommand = Inputs.CompileCommand;
   } // unlock Mutex.
   // Notify about changes to RebuildCounter.
   RebuildCond.notify_all();
 
   // A helper to function to finish the rebuild. May be run on a different
   // thread.
 
   // Don't let this CppFile die before rebuild is finished.
std::shared_ptr<const PreambleData> CppFile::getPossiblyStalePreamble() const {
   return LatestAvailablePreamble;
 }
 
 std::shared_future<std::shared_ptr<ParsedASTWrapper>> CppFile::getAST() const {
   std::lock_guard<std::mutex> Lock(Mutex);
   return ASTFuture;
 }
 
-llvm::Optional<tooling::CompileCommand> CppFile::getLastCommand() const {
-  std::lock_guard<std::mutex> Lock(Mutex);
-  return LastCommand;
-}
-
 CppFile::RebuildGuard::RebuildGuard(CppFile &File,
                                     unsigned RequestRebuildCounter)
     : File(File), RequestRebuildCounter(RequestRebuildCounter) {
   std::unique_lock<std::mutex> Lock(File.Mutex);
   WasCancelledBeforeConstruction = File.RebuildCounter != RequestRebuildCounter;
   if (WasCancelledBeforeConstruction)
     return;
 

clangd/ClangdUnitStore.h

 class CppFileCollection {
 public:
   /// \p ASTCallback is called when a file is parsed synchronously. This should
   /// not be expensive since it blocks diagnostics.
   explicit CppFileCollection(ASTParsedCallback ASTCallback)
       : ASTCallback(std::move(ASTCallback)) {}
 
   std::shared_ptr<CppFile>
-  getOrCreateFile(PathRef File, PathRef ResourceDir,
-                  bool StorePreamblesInMemory,
+  getOrCreateFile(PathRef File, bool StorePreamblesInMemory,
                   std::shared_ptr<PCHContainerOperations> PCHs) {
     std::lock_guard<std::mutex> Lock(Mutex);
     auto It = OpenedFiles.find(File);
     if (It == OpenedFiles.end()) {
       It = OpenedFiles
                .try_emplace(File, CppFile::Create(File, StorePreamblesInMemory,
                                                   std::move(PCHs), ASTCallback))
                .first;