[clang-tools-extra] r351788 - [clangd] Filter out plugin related flags and move all commandline manipulations into OverlayCDB.

Kadir Cetinkaya via cfe-commits cfe-commits at lists.llvm.org
Tue Jan 22 01:10:21 PST 2019


Author: kadircet
Date: Tue Jan 22 01:10:20 2019
New Revision: 351788

URL: http://llvm.org/viewvc/llvm-project?rev=351788&view=rev
Log:
[clangd] Filter out plugin related flags and move all commandline manipulations into OverlayCDB.

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.

Reviewers: ilya-biryukov

Subscribers: klimek, sammccall, ioeric, MaskRay, jkorous, arphaman, cfe-commits

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

Modified:
    clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
    clang-tools-extra/trunk/clangd/ClangdServer.cpp
    clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.cpp
    clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.h
    clang-tools-extra/trunk/clangd/index/Background.cpp
    clang-tools-extra/trunk/clangd/index/Background.h
    clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp
    clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp
    clang-tools-extra/trunk/unittests/clangd/GlobalCompilationDatabaseTests.cpp

Modified: clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp?rev=351788&r1=351787&r2=351788&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp (original)
+++ clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp Tue Jan 22 01:10:20 2019
@@ -289,7 +289,8 @@ void ClangdLSPServer::onInitialize(const
   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);

Modified: clang-tools-extra/trunk/clangd/ClangdServer.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdServer.cpp?rev=351788&r1=351787&r2=351788&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/ClangdServer.cpp (original)
+++ clang-tools-extra/trunk/clangd/ClangdServer.cpp Tue Jan 22 01:10:20 2019
@@ -37,11 +37,6 @@ 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:
@@ -107,8 +102,6 @@ ClangdServer::ClangdServer(const GlobalC
                            DiagnosticsConsumer &DiagConsumer,
                            const Options &Opts)
     : CDB(CDB), FSProvider(FSProvider),
-      ResourceDir(Opts.ResourceDir ? *Opts.ResourceDir
-                                   : getStandardResourceDir()),
       DynamicIdx(Opts.BuildDynamicSymbolIndex
                      ? new FileIndex(Opts.HeavyweightDynamicSymbolIndex)
                      : nullptr),
@@ -136,7 +129,7 @@ ClangdServer::ClangdServer(const GlobalC
     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());
@@ -461,10 +454,6 @@ tooling::CompileCommand ClangdServer::ge
   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);
 }
 

Modified: clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.cpp?rev=351788&r1=351787&r2=351788&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.cpp (original)
+++ clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.cpp Tue Jan 22 01:10:20 2019
@@ -8,12 +8,36 @@
 
 #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;
@@ -105,8 +129,11 @@ DirectoryBasedGlobalCompilationDatabase:
 }
 
 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);
@@ -115,16 +142,22 @@ OverlayCDB::OverlayCDB(const GlobalCompi
 
 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 {

Modified: clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.h?rev=351788&r1=351787&r2=351788&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.h (original)
+++ clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.h Tue Jan 22 01:10:20 2019
@@ -11,6 +11,7 @@
 
 #include "Function.h"
 #include "Path.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringMap.h"
 #include <memory>
 #include <mutex>
@@ -97,7 +98,8 @@ 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;
@@ -112,6 +114,7 @@ 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;
 };

Modified: clang-tools-extra/trunk/clangd/index/Background.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/Background.cpp?rev=351788&r1=351787&r2=351788&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/index/Background.cpp (original)
+++ clang-tools-extra/trunk/clangd/index/Background.cpp Tue Jan 22 01:10:20 2019
@@ -126,13 +126,12 @@ llvm::SmallString<128> getAbsolutePath(c
 } // 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)),
@@ -229,7 +228,6 @@ void BackgroundIndex::enqueue(tooling::C
                               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;

Modified: clang-tools-extra/trunk/clangd/index/Background.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/Background.h?rev=351788&r1=351787&r2=351788&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/index/Background.h (original)
+++ clang-tools-extra/trunk/clangd/index/Background.h Tue Jan 22 01:10:20 2019
@@ -67,9 +67,7 @@ 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,
@@ -98,7 +96,6 @@ private:
               BackgroundIndexStorage *IndexStorage);
 
   // configuration
-  std::string ResourceDir;
   const FileSystemProvider &FSProvider;
   const GlobalCompilationDatabase &CDB;
   Context BackgroundContext;

Modified: clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp?rev=351788&r1=351787&r2=351788&view=diff
==============================================================================
--- clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp Tue Jan 22 01:10:20 2019
@@ -76,7 +76,7 @@ TEST_F(BackgroundIndexTest, NoCrashOnErr
   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;
@@ -113,7 +113,7 @@ TEST_F(BackgroundIndexTest, IndexTwoFile
   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;
@@ -168,7 +168,7 @@ TEST_F(BackgroundIndexTest, ShardStorage
   // 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());
@@ -178,7 +178,7 @@ TEST_F(BackgroundIndexTest, ShardStorage
 
   {
     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());
@@ -224,7 +224,7 @@ TEST_F(BackgroundIndexTest, DirectInclud
   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());
@@ -262,7 +262,7 @@ TEST_F(BackgroundIndexTest, DISABLED_Per
   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\"";
@@ -310,7 +310,7 @@ TEST_F(BackgroundIndexTest, ShardStorage
   // 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());
@@ -325,7 +325,7 @@ TEST_F(BackgroundIndexTest, ShardStorage
       )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());
@@ -343,7 +343,7 @@ TEST_F(BackgroundIndexTest, ShardStorage
   {
     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());
@@ -384,7 +384,7 @@ TEST_F(BackgroundIndexTest, ShardStorage
   // 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());
@@ -400,7 +400,7 @@ TEST_F(BackgroundIndexTest, ShardStorage
   {
     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());
@@ -416,7 +416,7 @@ TEST_F(BackgroundIndexTest, ShardStorage
   {
     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());

Modified: clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp?rev=351788&r1=351787&r2=351788&view=diff
==============================================================================
--- clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp Tue Jan 22 01:10:20 2019
@@ -9,6 +9,7 @@
 #include "Annotations.h"
 #include "ClangdLSPServer.h"
 #include "ClangdServer.h"
+#include "GlobalCompilationDatabase.h"
 #include "Matchers.h"
 #include "SyncAPI.h"
 #include "TestFS.h"
@@ -1036,6 +1037,28 @@ TEST(ClangdTests, PreambleVFSStatCache)
 }
 #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

Modified: clang-tools-extra/trunk/unittests/clangd/GlobalCompilationDatabaseTests.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/GlobalCompilationDatabaseTests.cpp?rev=351788&r1=351787&r2=351788&view=diff
==============================================================================
--- clang-tools-extra/trunk/unittests/clangd/GlobalCompilationDatabaseTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/GlobalCompilationDatabaseTests.cpp Tue Jan 22 01:10:20 2019
@@ -64,7 +64,7 @@ protected:
 };
 
 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);
@@ -84,7 +84,7 @@ TEST_F(OverlayCDBTest, GetFallbackComman
 }
 
 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);




More information about the cfe-commits mailing list