This has been merged to the 8.0 branch in r351860. Please let me know if there are any follow-ups so they can be merged too.
Thanks, Hans On Tue, Jan 22, 2019 at 1:10 AM Kadir Cetinkaya via cfe-commits <cfe-commits@lists.llvm.org> wrote: > > 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); > > > _______________________________________________ > cfe-commits mailing list > cfe-commits@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
