[clangd] Filter out plugin related flags and move all commandline manipulations into OverlayCDB.

[kadircet]

Summary

Some projects make use of clang plugins when building, but clangd is
not aware of those plugins therefore can't work with the same compile command
arguments.

There were multiple places clangd performed commandline manipulations,
this one also moves them all into OverlayCDB.

[ioeric]

clangd/ClangdUnit.cpp

429 ↗

(On Diff #182244)

This doesn't seem to be clangd specific; clang-tidy seems to have the same issue. Could we share the filtering logic (e.g. in lib/Tooling/ArgumentsAdjusters.cpp)?

[ilya-biryukov]

LG, just a few comments. Supporting compiler plugins in clangd is non-trivial and we don't support them so far, so filtering out the options make sense.

It feels these helper belong in the Tooling library, that would allow to reuse code between clang-tidy, clangd, etc. Could even be the default behavior, since most of the tools won't handle compiler plugins well, unless compiled with the same exact version of the compiler.
I'd be interested in what other people think, @sammccall, @klimek?

clangd/ClangdUnit.cpp

429 ↗

(On Diff #182244)

We need a comment mentioning that we are filtering out the plugin options and why we're doing that here

433 ↗

(On Diff #182244)

Maybe move this code into ClangdServer::getCompileCommand to keep all argument adjustments that we do in clangd in one place.
We add -resource-dir there.

[ilya-biryukov]

clangd/ClangdUnit.cpp

429 ↗

(On Diff #182244)

+1 to the suggestion.
WDYT about landing it into clangd as a quick workaround and moving into tooling later?
The reason I think it might be better is because that would unblock usage of clangd in Chromium.

[ioeric]

clangd/ClangdUnit.cpp

429 ↗

(On Diff #182244)

WDYT about landing it into clangd as a quick workaround and moving into tooling later?
The reason I think it might be better is because that would unblock usage of clangd in Chromium.

I don't see why it would take much more effort to move the shared logic into tooling now?

[kadircet]

It seems like the most relevant place in tooling is ArgumentsAdjuster as @ioeric pointed out. Happy to move it to there if @sammccall and @klimek agrees as well.

As a side note ArgumentsAdjusters unfortunately causes a copy of the original command line arguments. Not sure how important this factor is compared to spinning up a compiler instance, just wanted to point it out.

clangd/ClangdUnit.cpp

433 ↗

(On Diff #182244)

As mentioned in the change description, we also have a different compiler invocation in BackgroundIndex which is not aware of ClangdServer::getCompileCommand

[kadircet]

• Address comments

[ilya-biryukov]

clangd/ClangdUnit.cpp

429 ↗

(On Diff #182244)

My approximation would be half an hour to finish this and land in clangd and 2-3 hours to land this in tooling, update clangd, clang-tidy, etc.
Not a big difference, though, up to Kadir to actually quantify how hard he thinks this would be.

[klimek]

In D56841#1361395, @kadircet wrote:

It seems like the most relevant place in tooling is ArgumentsAdjuster as @ioeric pointed out. Happy to move it to there if @sammccall and @klimek agrees as well.

As a side note ArgumentsAdjusters unfortunately causes a copy of the original command line arguments. Not sure how important this factor is compared to spinning up a compiler instance, just wanted to point it out.

From my point of view, it looks like this isn't enough to be a huge thing in tooling. That said, if we can find a nice abstraction to pull out, I'm generally in favor :) (I know, I know, I'm not a big help)

[sammccall]

In D56841#1361395, @kadircet wrote:

It seems like the most relevant place in tooling is ArgumentsAdjuster as @ioeric pointed out. Happy to move it to there if @sammccall and @klimek agrees as well.

As a side note ArgumentsAdjusters unfortunately causes a copy of the original command line arguments. Not sure how important this factor is compared to spinning up a compiler instance, just wanted to point it out.

I do think this should be a standard arguments adjuster available in Tooling.
This is a pattern that's likely to recur (clang-tidy will need the same fix to work with chromium). The fix is small but fiddly enough that it might warrant some improvement over time - I think we want to avoid people cloning their own copies of the fix.
The ArgumentsAdjuster interface is a little unfortunate regarding the copy, but I don't think this is significant in practice.

Once verifying this fixes the issue, we should consider requesting the tooling and clangd patches be cherrypicked to the LLVM-8 branch.
(And possibly do the same for clang-tidy)

[kadircet]

clang-tidy also has the same fix in place, https://reviews.llvm.org/D18806

[ilya-biryukov]

In D56841#1361492, @klimek wrote:

(I know, I know, I'm not a big help)

That's fine, mostly wanted to hear your opinion :-)

That said, if we can find a nice abstraction to pull out, I'm generally in favor :)

Eric suggested putting this into lib/Tooling/ArgumentsAdjusters.cpp, looks like a proper place for this kind of thing.

As a side note ArgumentsAdjusters unfortunately causes a copy of the original command line arguments. Not sure how important this factor is compared to spinning up a compiler instance, just wanted to point it out.

Yeah, running the frontend later would definitely outweigh this inefficiency, so we don't care.
I do agree that's an unfortunate design, though.

clangd/ClangdUnit.cpp

433 ↗

(On Diff #182244)

Ah, right :-( That's annoying. Does it also manually override -resource-dir? I'd move both into a single place, maybe this function is a better place to do so.

[kadircet]

Moving stripper to tooling.

clangd/ClangdUnit.cpp

433 ↗

(On Diff #182244)

It turned out to be more complicated actually, for example CodeCompletion also invokes a frontend action and it doesn't go through this function.....

[kadircet]

• Use getStripPluginsAdjuster and move manipulations into OverlayCDB

[ilya-biryukov]

just a few nits

clangd/GlobalCompilationDatabase.cpp

23

naming NIT: use adjustArguments

24

NIT: use StringRef

136

NIT: avoid an unnecessary copy: ResourceDir ? std::move(*ResourceDir) : …

[kadircet]

• Address comments

[kadircet]

clangd/GlobalCompilationDatabase.cpp

24

Need a string for the concatenation below, when pushing to vector. Otherwise we get a Twine

[ilya-biryukov]

clangd/GlobalCompilationDatabase.cpp

24

Why not call .str() on a Twine?

[kadircet]

Address comments

clangd/GlobalCompilationDatabase.cpp

24

Sure it is also an option but didn't think it would look any better, but there you go.

[ilya-biryukov]

LGTM

clangd/GlobalCompilationDatabase.cpp

24

Looks kinda ugly, but so efficient... (Not that we care much here)

clangd/ClangdLSPServer.cpp

void ClangdLSPServer::onInitialize(const InitializeParams &Params,
   if (Server)
     return Reply(llvm::make_error<LSPError>("server already initialized",
                                             ErrorCode::InvalidRequest));
   if (const auto &Dir = Params.initializationOptions.compilationDatabasePath)
     CompileCommandsDir = Dir;
   if (UseDirBasedCDB)
     BaseCDB = llvm::make_unique<DirectoryBasedGlobalCompilationDatabase>(
         CompileCommandsDir);
-  CDB.emplace(BaseCDB.get(), Params.initializationOptions.fallbackFlags);
+  CDB.emplace(BaseCDB.get(), Params.initializationOptions.fallbackFlags,
+              ClangdServerOpts.ResourceDir);
   Server.emplace(*CDB, FSProvider, static_cast<DiagnosticsConsumer &>(*this),
                  ClangdServerOpts);
   applyConfiguration(Params.initializationOptions.ConfigSettings);
 
   CCOpts.EnableSnippets = Params.capabilities.CompletionSnippets;
   DiagOpts.EmbedFixesInDiagnostics = Params.capabilities.DiagnosticFixes;
   DiagOpts.SendDiagnosticCategory = Params.capabilities.DiagnosticCategory;
   if (Params.capabilities.WorkspaceSymbolKinds)

clangd/ClangdServer.cpp

 #include "llvm/Support/raw_ostream.h"
 #include <future>
 #include <mutex>
 
 namespace clang {
 namespace clangd {
 namespace {
 
-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:
   void handleError(llvm::Error Err) override {
     assert(!Result.hasValue());
     // FIXME: figure out a way to return better message for DiagnosticError.
     // clangd uses llvm::toString to convert the Err to string, however, for
     // DiagnosticError, only "clang diagnostic" will be generated.
ClangdServer::Options ClangdServer::optsForTest() {
   return Opts;
 }
 
 ClangdServer::ClangdServer(const GlobalCompilationDatabase &CDB,
                            const FileSystemProvider &FSProvider,
                            DiagnosticsConsumer &DiagConsumer,
                            const Options &Opts)
     : CDB(CDB), FSProvider(FSProvider),
-      ResourceDir(Opts.ResourceDir ? *Opts.ResourceDir
-                                   : getStandardResourceDir()),
       DynamicIdx(Opts.BuildDynamicSymbolIndex
                      ? new FileIndex(Opts.HeavyweightDynamicSymbolIndex)
                      : nullptr),
       WorkspaceRoot(Opts.WorkspaceRoot),
       PCHs(std::make_shared<PCHContainerOperations>()),
       // 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.
auto AddIndex = [&](SymbolIndex *Idx) {
     } else {
       this->Index = Idx;
     }
   };
   if (Opts.StaticIndex)
     AddIndex(Opts.StaticIndex);
   if (Opts.BackgroundIndex) {
     BackgroundIdx = llvm::make_unique<BackgroundIndex>(
-        Context::current().clone(), ResourceDir, FSProvider, CDB,
+        Context::current().clone(), FSProvider, CDB,
         BackgroundIndexStorage::createDiskBackedStorageFactory(),
         Opts.BackgroundIndexRebuildPeriodMs);
     AddIndex(BackgroundIdx.get());
   }
   if (DynamicIdx)
     AddIndex(DynamicIdx.get());
 }
 
void ClangdServer::findHover(PathRef File, Position Pos,
   WorkScheduler.runWithAST("Hover", File, Bind(Action, std::move(CB)));
 }
 
 tooling::CompileCommand ClangdServer::getCompileCommand(PathRef File) {
   trace::Span Span("GetCompileCommand");
   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);
   return std::move(*C);
 }
 
 void ClangdServer::onFileEvent(const DidChangeWatchedFilesParams &Params) {
   // FIXME: Do nothing for now. This will be used for indexing and potentially
   // invalidating other caches.
 }
 

clangd/GlobalCompilationDatabase.h

 //===--- GlobalCompilationDatabase.h -----------------------------*- 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_GLOBALCOMPILATIONDATABASE_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_GLOBALCOMPILATIONDATABASE_H
 
 #include "Function.h"
 #include "Path.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringMap.h"
 #include <memory>
 #include <mutex>
 #include <vector>
 
 namespace clang {
 
 namespace tooling {

 
 /// Wraps another compilation database, and supports overriding the commands
 /// using an in-memory mapping.
 class OverlayCDB : public GlobalCompilationDatabase {
 public:
   // Base may be null, in which case no entries are inherited.
   // FallbackFlags are added to the fallback compile command.
   OverlayCDB(const GlobalCompilationDatabase *Base,
-             std::vector<std::string> FallbackFlags = {});
+             std::vector<std::string> FallbackFlags = {},
+             llvm::Optional<std::string> ResourceDir = llvm::None);
 
   llvm::Optional<tooling::CompileCommand>
   getCompileCommand(PathRef File, ProjectInfo * = nullptr) const override;
   tooling::CompileCommand getFallbackCommand(PathRef File) const override;
 
   /// Sets or clears the compilation command for a particular file.
   void
   setCompileCommand(PathRef File,
                     llvm::Optional<tooling::CompileCommand> CompilationCommand);
 
 private:
   mutable std::mutex Mutex;
   llvm::StringMap<tooling::CompileCommand> Commands; /* GUARDED_BY(Mut) */
   const GlobalCompilationDatabase *Base;
+  std::string ResourceDir;
   std::vector<std::string> FallbackFlags;
   CommandChanged::Subscription BaseChanged;
 };
 
 } // namespace clangd
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_GLOBALCOMPILATIONDATABASE_H

clangd/GlobalCompilationDatabase.cpp

 //===--- GlobalCompilationDatabase.cpp ---------------------------*- C++-*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "GlobalCompilationDatabase.h"
 #include "Logger.h"
+#include "clang/Frontend/CompilerInvocation.h"
+#include "clang/Tooling/ArgumentsAdjusters.h"
 #include "clang/Tooling/CompilationDatabase.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 
 namespace clang {
 namespace clangd {
+namespace {
+
+void adjustArguments(tooling::CompileCommand &Cmd,
+                     llvm::StringRef ResourceDir) {
+  // Strip plugin related command line arguments. Clangd does
+  // not support plugins currently. Therefore it breaks if
+  // compiler tries to load plugins.
+  Cmd.CommandLine =
+      tooling::getStripPluginsAdjuster()(Cmd.CommandLine, Cmd.Filename);
+  // Inject the resource dir.
+  // FIXME: Don't overwrite it if it's already there.
+  if (!ResourceDir.empty())
+    Cmd.CommandLine.push_back(("-resource-dir=" + ResourceDir).str());
+}
+
+std::string getStandardResourceDir() {
+  static int Dummy; // Just an address in this process.
+  return CompilerInvocation::GetResourcesPath("clangd", (void *)&Dummy);
+}
+
+} // namespace
 
 static std::string getFallbackClangPath() {
   static int Dummy;
   std::string ClangdExecutable =
       llvm::sys::fs::getMainExecutable("clangd", (void *)&Dummy);
   SmallString<128> ClangPath;
   ClangPath = llvm::sys::path::parent_path(ClangdExecutable);
   llvm::sys::path::append(ClangPath, "clang");
DirectoryBasedGlobalCompilationDatabase::getCDBForFile(
   // FIXME: getAllFiles() may return relative paths, we need absolute paths.
   // Hopefully the fix is to change JSONCompilationDatabase and the interface.
   if (CDB && !Cached)
     OnCommandChanged.broadcast(CDB->getAllFiles());
   return CDB;
 }
 
 OverlayCDB::OverlayCDB(const GlobalCompilationDatabase *Base,
-                       std::vector<std::string> FallbackFlags)
-    : Base(Base), FallbackFlags(std::move(FallbackFlags)) {
+                       std::vector<std::string> FallbackFlags,
+                       llvm::Optional<std::string> ResourceDir)
+    : Base(Base), ResourceDir(ResourceDir ? std::move(*ResourceDir)
+                                          : getStandardResourceDir()),
+      FallbackFlags(std::move(FallbackFlags)) {
   if (Base)
     BaseChanged = Base->watch([this](const std::vector<std::string> Changes) {
       OnCommandChanged.broadcast(Changes);
     });
 }
 
 llvm::Optional<tooling::CompileCommand>
 OverlayCDB::getCompileCommand(PathRef File, ProjectInfo *Project) const {
+  llvm::Optional<tooling::CompileCommand> Cmd;
   {
     std::lock_guard<std::mutex> Lock(Mutex);
     auto It = Commands.find(File);
     if (It != Commands.end()) {
       if (Project)
         Project->SourceRoot = "";
-      return It->second;
+      Cmd = It->second;
     }
   }
-  return Base ? Base->getCompileCommand(File, Project) : None;
+  if (!Cmd && Base)
+    Cmd = Base->getCompileCommand(File, Project);
+  if (!Cmd)
+    return llvm::None;
+  adjustArguments(*Cmd, ResourceDir);
+  return Cmd;
 }
 
 tooling::CompileCommand OverlayCDB::getFallbackCommand(PathRef File) const {
   auto Cmd = Base ? Base->getFallbackCommand(File)
                   : GlobalCompilationDatabase::getFallbackCommand(File);
   std::lock_guard<std::mutex> Lock(Mutex);
   Cmd.CommandLine.insert(Cmd.CommandLine.end(), FallbackFlags.begin(),
                          FallbackFlags.end());

clangd/index/Background.h

 // all commands in a compilation database. Indexing happens in the background.
 // FIXME: it should also persist its state on disk for fast start.
 // FIXME: it should watch for changes to files on disk.
 class BackgroundIndex : public SwapIndex {
 public:
   /// If BuildIndexPeriodMs is greater than 0, the symbol index will only be
   /// rebuilt periodically (one per \p BuildIndexPeriodMs); otherwise, index is
   /// rebuilt for each indexed file.
-  // FIXME: resource-dir injection should be hoisted somewhere common.
-  BackgroundIndex(Context BackgroundContext, llvm::StringRef ResourceDir,
-                  const FileSystemProvider &,
+  BackgroundIndex(Context BackgroundContext, const FileSystemProvider &,
                   const GlobalCompilationDatabase &CDB,
                   BackgroundIndexStorage::Factory IndexStorageFactory,
                   size_t BuildIndexPeriodMs = 0,
                   size_t ThreadPoolSize = llvm::hardware_concurrency());
   ~BackgroundIndex(); // Blocks while the current task finishes.
 
   // Enqueue translation units for indexing.
   // The indexing happens in a background thread, so the symbols will be
private:
   /// Given index results from a TU, only update symbols coming from files with
   /// different digests than \p DigestsSnapshot. Also stores new index
   /// information on IndexStorage.
   void update(llvm::StringRef MainFile, IndexFileIn Index,
               const llvm::StringMap<FileDigest> &DigestsSnapshot,
               BackgroundIndexStorage *IndexStorage);
 
   // configuration
-  std::string ResourceDir;
   const FileSystemProvider &FSProvider;
   const GlobalCompilationDatabase &CDB;
   Context BackgroundContext;
 
   // index state
   llvm::Error index(tooling::CompileCommand,
                     BackgroundIndexStorage *IndexStorage);
   void buildIndex(); // Rebuild index periodically every BuildIndexPeriodMs.

clangd/index/Background.cpp

if (llvm::sys::path::is_absolute(Cmd.Filename)) {
     AbsolutePath = Cmd.Directory;
     llvm::sys::path::append(AbsolutePath, Cmd.Filename);
   }
   return AbsolutePath;
 }
 } // namespace
 
 BackgroundIndex::BackgroundIndex(
-    Context BackgroundContext, llvm::StringRef ResourceDir,
-    const FileSystemProvider &FSProvider, const GlobalCompilationDatabase &CDB,
+    Context BackgroundContext, const FileSystemProvider &FSProvider,
+    const GlobalCompilationDatabase &CDB,
     BackgroundIndexStorage::Factory IndexStorageFactory,
     size_t BuildIndexPeriodMs, size_t ThreadPoolSize)
-    : SwapIndex(llvm::make_unique<MemIndex>()), ResourceDir(ResourceDir),
-      FSProvider(FSProvider), CDB(CDB),
-      BackgroundContext(std::move(BackgroundContext)),
+    : SwapIndex(llvm::make_unique<MemIndex>()), FSProvider(FSProvider),
+      CDB(CDB), BackgroundContext(std::move(BackgroundContext)),
       BuildIndexPeriodMs(BuildIndexPeriodMs),
       SymbolsUpdatedSinceLastIndex(false),
       IndexStorageFactory(std::move(IndexStorageFactory)),
       CommandsChanged(
           CDB.watch([&](const std::vector<std::string> &ChangedFiles) {
             enqueue(ChangedFiles);
           })) {
   assert(ThreadPoolSize > 0 && "Thread pool size can't be zero.");
enqueueTask(
       },
       ThreadPriority::Normal);
 }
 
 void BackgroundIndex::enqueue(tooling::CompileCommand Cmd,
                               BackgroundIndexStorage *Storage) {
   enqueueTask(Bind(
                   [this, Storage](tooling::CompileCommand Cmd) {
-                    Cmd.CommandLine.push_back("-resource-dir=" + ResourceDir);
                     // We can't use llvm::StringRef here since we are going to
                     // move from Cmd during the call below.
                     const std::string FileName = Cmd.Filename;
                     if (auto Error = index(std::move(Cmd), Storage))
                       elog("Indexing {0} failed: {1}", FileName,
                            std::move(Error));
                   },
                   std::move(Cmd)),

unittests/clangd/BackgroundIndexTests.cpp

 
 TEST_F(BackgroundIndexTest, NoCrashOnErrorFile) {
   MockFSProvider FS;
   FS.Files[testPath("root/A.cc")] = "error file";
   llvm::StringMap<std::string> Storage;
   size_t CacheHits = 0;
   MemoryShardStorage MSS(Storage, CacheHits);
   OverlayCDB CDB(/*Base=*/nullptr);
-  BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+  BackgroundIndex Idx(Context::empty(), FS, CDB,
                       [&](llvm::StringRef) { return &MSS; });
 
   tooling::CompileCommand Cmd;
   Cmd.Filename = testPath("root/A.cc");
   Cmd.Directory = testPath("root");
   Cmd.CommandLine = {"clang++", "-DA=1", testPath("root/A.cc")};
   CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
 
FS.Files[testPath("root/B.cc")] =
       #include "A.h"
       void f_b() {
         (void)common;
       })cpp";
   llvm::StringMap<std::string> Storage;
   size_t CacheHits = 0;
   MemoryShardStorage MSS(Storage, CacheHits);
   OverlayCDB CDB(/*Base=*/nullptr);
-  BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+  BackgroundIndex Idx(Context::empty(), FS, CDB,
                       [&](llvm::StringRef) { return &MSS; });
 
   tooling::CompileCommand Cmd;
   Cmd.Filename = testPath("root/A.cc");
   Cmd.Directory = testPath("root");
   Cmd.CommandLine = {"clang++", "-DA=1", testPath("root/A.cc")};
   CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
 
TEST_F(BackgroundIndexTest, ShardStorageTest) {
 
   tooling::CompileCommand Cmd;
   Cmd.Filename = testPath("root/A.cc");
   Cmd.Directory = testPath("root");
   Cmd.CommandLine = {"clang++", testPath("root/A.cc")};
   // Check nothing is loaded from Storage, but A.cc and A.h has been stored.
   {
     OverlayCDB CDB(/*Base=*/nullptr);
-    BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+    BackgroundIndex Idx(Context::empty(), FS, CDB,
                         [&](llvm::StringRef) { return &MSS; });
     CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
     ASSERT_TRUE(Idx.blockUntilIdleForTest());
   }
   EXPECT_EQ(CacheHits, 0U);
   EXPECT_EQ(Storage.size(), 2U);
 
   {
     OverlayCDB CDB(/*Base=*/nullptr);
-    BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+    BackgroundIndex Idx(Context::empty(), FS, CDB,
                         [&](llvm::StringRef) { return &MSS; });
     CDB.setCompileCommand(testPath("root"), Cmd);
     ASSERT_TRUE(Idx.blockUntilIdleForTest());
   }
   EXPECT_EQ(CacheHits, 2U); // Check both A.cc and A.h loaded from cache.
   EXPECT_EQ(Storage.size(), 2U);
 
   auto ShardHeader = MSS.loadShard(testPath("root/A.h"));
TEST_F(BackgroundIndexTest, DirectIncludesTest) {
   MemoryShardStorage MSS(Storage, CacheHits);
 
   tooling::CompileCommand Cmd;
   Cmd.Filename = testPath("root/A.cc");
   Cmd.Directory = testPath("root");
   Cmd.CommandLine = {"clang++", testPath("root/A.cc")};
   {
     OverlayCDB CDB(/*Base=*/nullptr);
-    BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+    BackgroundIndex Idx(Context::empty(), FS, CDB,
                         [&](llvm::StringRef) { return &MSS; });
     CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
     ASSERT_TRUE(Idx.blockUntilIdleForTest());
   }
 
   auto ShardSource = MSS.loadShard(testPath("root/A.cc"));
   EXPECT_TRUE(ShardSource->Sources);
   EXPECT_EQ(ShardSource->Sources->size(), 2U); // A.cc, A.h

 // re-enable.
 TEST_F(BackgroundIndexTest, DISABLED_PeriodicalIndex) {
   MockFSProvider FS;
   llvm::StringMap<std::string> Storage;
   size_t CacheHits = 0;
   MemoryShardStorage MSS(Storage, CacheHits);
   OverlayCDB CDB(/*Base=*/nullptr);
   BackgroundIndex Idx(
-      Context::empty(), "", FS, CDB, [&](llvm::StringRef) { return &MSS; },
+      Context::empty(), FS, CDB, [&](llvm::StringRef) { return &MSS; },
       /*BuildIndexPeriodMs=*/500);
 
   FS.Files[testPath("root/A.cc")] = "#include \"A.h\"";
 
   tooling::CompileCommand Cmd;
   FS.Files[testPath("root/A.h")] = "class X {};";
   Cmd.Filename = testPath("root/A.cc");
   Cmd.CommandLine = {"clang++", Cmd.Filename};
TEST_F(BackgroundIndexTest, ShardStorageLoad) {
 
   tooling::CompileCommand Cmd;
   Cmd.Filename = testPath("root/A.cc");
   Cmd.Directory = testPath("root");
   Cmd.CommandLine = {"clang++", testPath("root/A.cc")};
   // Check nothing is loaded from Storage, but A.cc and A.h has been stored.
   {
     OverlayCDB CDB(/*Base=*/nullptr);
-    BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+    BackgroundIndex Idx(Context::empty(), FS, CDB,
                         [&](llvm::StringRef) { return &MSS; });
     CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
     ASSERT_TRUE(Idx.blockUntilIdleForTest());
   }
 
   // Change header.
   FS.Files[testPath("root/A.h")] = R"cpp(
       void common();
       void f_b();
       class A_CC {};
       class A_CCnew {};
       )cpp";
   {
     OverlayCDB CDB(/*Base=*/nullptr);
-    BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+    BackgroundIndex Idx(Context::empty(), FS, CDB,
                         [&](llvm::StringRef) { return &MSS; });
     CDB.setCompileCommand(testPath("root"), Cmd);
     ASSERT_TRUE(Idx.blockUntilIdleForTest());
   }
   EXPECT_EQ(CacheHits, 2U); // Check both A.cc and A.h loaded from cache.
 
   // Check if the new symbol has arrived.
   auto ShardHeader = MSS.loadShard(testPath("root/A.h"));
   EXPECT_NE(ShardHeader, nullptr);
   EXPECT_THAT(*ShardHeader->Symbols, Contains(Named("A_CCnew")));
 
   // Change source.
   FS.Files[testPath("root/A.cc")] =
       "#include \"A.h\"\nvoid g() { (void)common; }\nvoid f_b() {}";
   {
     CacheHits = 0;
     OverlayCDB CDB(/*Base=*/nullptr);
-    BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+    BackgroundIndex Idx(Context::empty(), FS, CDB,
                         [&](llvm::StringRef) { return &MSS; });
     CDB.setCompileCommand(testPath("root"), Cmd);
     ASSERT_TRUE(Idx.blockUntilIdleForTest());
   }
   EXPECT_EQ(CacheHits, 2U); // Check both A.cc and A.h loaded from cache.
 
   // Check if the new symbol has arrived.
   ShardHeader = MSS.loadShard(testPath("root/A.h"));
TEST_F(BackgroundIndexTest, ShardStorageEmptyFile) {
 
   tooling::CompileCommand Cmd;
   Cmd.Filename = testPath("root/A.cc");
   Cmd.Directory = testPath("root");
   Cmd.CommandLine = {"clang++", testPath("root/A.cc")};
   // Check that A.cc, A.h and B.h has been stored.
   {
     OverlayCDB CDB(/*Base=*/nullptr);
-    BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+    BackgroundIndex Idx(Context::empty(), FS, CDB,
                         [&](llvm::StringRef) { return &MSS; });
     CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
     ASSERT_TRUE(Idx.blockUntilIdleForTest());
   }
   EXPECT_THAT(Storage.keys(),
               UnorderedElementsAre(testPath("root/A.cc"), testPath("root/A.h"),
                                    testPath("root/B.h")));
   auto ShardHeader = MSS.loadShard(testPath("root/B.h"));
   EXPECT_NE(ShardHeader, nullptr);
   EXPECT_TRUE(ShardHeader->Symbols->empty());
 
   // Check that A.cc, A.h and B.h has been loaded.
   {
     CacheHits = 0;
     OverlayCDB CDB(/*Base=*/nullptr);
-    BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+    BackgroundIndex Idx(Context::empty(), FS, CDB,
                         [&](llvm::StringRef) { return &MSS; });
     CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
     ASSERT_TRUE(Idx.blockUntilIdleForTest());
   }
   EXPECT_EQ(CacheHits, 3U);
 
   // Update B.h to contain some symbols.
   FS.Files[testPath("root/B.h")] = R"cpp(
       #include "A.h"
       void new_func();
       )cpp";
   // Check that B.h has been stored with new contents.
   {
     CacheHits = 0;
     OverlayCDB CDB(/*Base=*/nullptr);
-    BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+    BackgroundIndex Idx(Context::empty(), FS, CDB,
                         [&](llvm::StringRef) { return &MSS; });
     CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
     ASSERT_TRUE(Idx.blockUntilIdleForTest());
   }
   EXPECT_EQ(CacheHits, 3U);
   ShardHeader = MSS.loadShard(testPath("root/B.h"));
   EXPECT_NE(ShardHeader, nullptr);
   EXPECT_THAT(*ShardHeader->Symbols,
               Contains(AllOf(Named("new_func"), Declared(), Not(Defined()))));
 }
 
 } // namespace clangd
 } // namespace clang

unittests/clangd/ClangdTests.cpp

 //===-- ClangdTests.cpp - Clangd unit tests ---------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "Annotations.h"
 #include "ClangdLSPServer.h"
 #include "ClangdServer.h"
+#include "GlobalCompilationDatabase.h"
 #include "Matchers.h"
 #include "SyncAPI.h"
 #include "TestFS.h"
 #include "URI.h"
 #include "clang/Config/config.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/Support/Errc.h"
auto Completions = cantFail(runCodeComplete(Server, SourcePath, Code.point(),
                                               clangd::CodeCompleteOptions()))
                          .Completions;
   EXPECT_EQ(CountStats["foo.h"], Before);
   EXPECT_THAT(Completions,
               ElementsAre(Field(&CodeCompletion::Name, "TestSym")));
 }
 #endif
 
+TEST_F(ClangdVFSTest, FlagsWithPlugins) {
+  MockFSProvider FS;
+  ErrorCheckingDiagConsumer DiagConsumer;
+  MockCompilationDatabase CDB;
+  CDB.ExtraClangFlags = {
+      "-Xclang",
+      "-add-plugin",
+      "-Xclang",
+      "random-plugin",
+  };
+  OverlayCDB OCDB(&CDB);
+  ClangdServer Server(OCDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+  auto FooCpp = testPath("foo.cpp");
+  const auto SourceContents = "int main() { return 0; }";
+  FS.Files[FooCpp] = FooCpp;
+  Server.addDocument(FooCpp, SourceContents);
+  auto Result = dumpASTWithoutMemoryLocs(Server, FooCpp);
+  EXPECT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for diagnostics";
+  EXPECT_NE(Result, "<no-ast>");
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang

unittests/clangd/GlobalCompilationDatabaseTests.cpp

class OverlayCDBTest : public ::testing::Test {
   };
 
 protected:
   OverlayCDBTest() : Base(llvm::make_unique<BaseCDB>()) {}
   std::unique_ptr<GlobalCompilationDatabase> Base;
 };
 
 TEST_F(OverlayCDBTest, GetCompileCommand) {
-  OverlayCDB CDB(Base.get());
+  OverlayCDB CDB(Base.get(), {}, std::string(""));
   EXPECT_EQ(CDB.getCompileCommand(testPath("foo.cc")),
             Base->getCompileCommand(testPath("foo.cc")));
   EXPECT_EQ(CDB.getCompileCommand(testPath("missing.cc")), llvm::None);
 
   auto Override = cmd(testPath("foo.cc"), "-DA=3");
   CDB.setCompileCommand(testPath("foo.cc"), Override);
   EXPECT_EQ(CDB.getCompileCommand(testPath("foo.cc")), Override);
   EXPECT_EQ(CDB.getCompileCommand(testPath("missing.cc")), llvm::None);
   CDB.setCompileCommand(testPath("missing.cc"), Override);
   EXPECT_EQ(CDB.getCompileCommand(testPath("missing.cc")), Override);
 }
 
 TEST_F(OverlayCDBTest, GetFallbackCommand) {
   OverlayCDB CDB(Base.get(), {"-DA=4"});
   EXPECT_THAT(CDB.getFallbackCommand(testPath("bar.cc")).CommandLine,
               ElementsAre("clang", "-DA=2", testPath("bar.cc"), "-DA=4"));
 }
 
 TEST_F(OverlayCDBTest, NoBase) {
-  OverlayCDB CDB(nullptr, {"-DA=6"});
+  OverlayCDB CDB(nullptr, {"-DA=6"}, std::string(""));
   EXPECT_EQ(CDB.getCompileCommand(testPath("bar.cc")), None);
   auto Override = cmd(testPath("bar.cc"), "-DA=5");
   CDB.setCompileCommand(testPath("bar.cc"), Override);
   EXPECT_EQ(CDB.getCompileCommand(testPath("bar.cc")), Override);
 
   EXPECT_THAT(CDB.getFallbackCommand(testPath("foo.cc")).CommandLine,
               ElementsAre(EndsWith("clang"), testPath("foo.cc"), "-DA=6"));
 }