From ad54935c7781008d3ad780767aec309f28360ff9 Mon Sep 17 00:00:00 2001 From: Kadir Cetinkaya Date: Thu, 11 Jul 2019 09:54:31 +0000 Subject: [PATCH] [clangd] Reland rL365634 This was reverted in rL365678, the failure was due to YAML parsing of compile_commands.json. Converting backslashes to forward slashes to fix the issue in unittest. llvm-svn: 365748 --- .../clangd/GlobalCompilationDatabase.cpp | 187 ++++++++++++++---- .../clangd/GlobalCompilationDatabase.h | 43 +++- .../clangd/QueryDriverDatabase.cpp | 8 +- clang-tools-extra/clangd/index/Background.cpp | 10 +- .../clangd/unittests/ClangdTests.cpp | 4 +- .../GlobalCompilationDatabaseTests.cpp | 129 +++++++++++- clang-tools-extra/clangd/unittests/TestFS.cpp | 14 +- clang-tools-extra/clangd/unittests/TestFS.h | 6 +- 8 files changed, 332 insertions(+), 69 deletions(-) diff --git a/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp b/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp index 9a09482ce390..51f0e7d015ed 100644 --- a/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp +++ b/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp @@ -8,12 +8,18 @@ #include "GlobalCompilationDatabase.h" #include "Logger.h" +#include "Path.h" #include "clang/Frontend/CompilerInvocation.h" #include "clang/Tooling/ArgumentsAdjusters.h" #include "clang/Tooling/CompilationDatabase.h" +#include "llvm/ADT/None.h" #include "llvm/ADT/Optional.h" +#include "llvm/ADT/STLExtras.h" #include "llvm/Support/FileSystem.h" #include "llvm/Support/Path.h" +#include +#include +#include namespace clang { namespace clangd { @@ -43,6 +49,16 @@ std::string getStandardResourceDir() { return CompilerInvocation::GetResourcesPath("clangd", (void *)&Dummy); } +// Runs the given action on all parent directories of filename, starting from +// deepest directory and going up to root. Stops whenever action succeeds. +void actOnAllParentDirectories(PathRef FileName, + llvm::function_ref Action) { + for (auto Path = llvm::sys::path::parent_path(FileName); + !Path.empty() && !Action(Path); + Path = llvm::sys::path::parent_path(Path)) + ; +} + } // namespace static std::string getFallbackClangPath() { @@ -81,60 +97,138 @@ DirectoryBasedGlobalCompilationDatabase:: ~DirectoryBasedGlobalCompilationDatabase() = default; llvm::Optional -DirectoryBasedGlobalCompilationDatabase::getCompileCommand( - PathRef File, ProjectInfo *Project) const { - if (auto CDB = getCDBForFile(File, Project)) { - auto Candidates = CDB->getCompileCommands(File); - if (!Candidates.empty()) { - return std::move(Candidates.front()); - } - } else { +DirectoryBasedGlobalCompilationDatabase::getCompileCommand(PathRef File) const { + CDBLookupRequest Req; + Req.FileName = File; + Req.ShouldBroadcast = true; + + auto Res = lookupCDB(Req); + if (!Res) { log("Failed to find compilation database for {0}", File); + return llvm::None; } + + auto Candidates = Res->CDB->getCompileCommands(File); + if (!Candidates.empty()) + return std::move(Candidates.front()); + return None; } -std::pair +std::pair DirectoryBasedGlobalCompilationDatabase::getCDBInDirLocked(PathRef Dir) const { // FIXME(ibiryukov): Invalidate cached compilation databases on changes auto CachedIt = CompilationDatabases.find(Dir); if (CachedIt != CompilationDatabases.end()) - return {CachedIt->second.get(), true}; + return {CachedIt->second.CDB.get(), CachedIt->second.SentBroadcast}; std::string Error = ""; - auto CDB = tooling::CompilationDatabase::loadFromDirectory(Dir, Error); - auto Result = CDB.get(); - CompilationDatabases.insert(std::make_pair(Dir, std::move(CDB))); + + CachedCDB Entry; + Entry.CDB = tooling::CompilationDatabase::loadFromDirectory(Dir, Error); + auto Result = Entry.CDB.get(); + CompilationDatabases[Dir] = std::move(Entry); + return {Result, false}; } -tooling::CompilationDatabase * -DirectoryBasedGlobalCompilationDatabase::getCDBForFile( - PathRef File, ProjectInfo *Project) const { - namespace path = llvm::sys::path; - assert((path::is_absolute(File, path::Style::posix) || - path::is_absolute(File, path::Style::windows)) && +llvm::Optional +DirectoryBasedGlobalCompilationDatabase::lookupCDB( + CDBLookupRequest Request) const { + assert(llvm::sys::path::is_absolute(Request.FileName) && "path must be absolute"); - tooling::CompilationDatabase *CDB = nullptr; - bool Cached = false; - std::lock_guard Lock(Mutex); + CDBLookupResult Result; + bool SentBroadcast = false; + + { + std::lock_guard Lock(Mutex); + if (CompileCommandsDir) { + std::tie(Result.CDB, SentBroadcast) = + getCDBInDirLocked(*CompileCommandsDir); + Result.PI.SourceRoot = *CompileCommandsDir; + } else { + actOnAllParentDirectories( + Request.FileName, [this, &SentBroadcast, &Result](PathRef Path) { + std::tie(Result.CDB, SentBroadcast) = getCDBInDirLocked(Path); + Result.PI.SourceRoot = Path; + return Result.CDB != nullptr; + }); + } + + if (!Result.CDB) + return llvm::None; + + // Mark CDB as broadcasted to make sure discovery is performed once. + if (Request.ShouldBroadcast && !SentBroadcast) + CompilationDatabases[Result.PI.SourceRoot].SentBroadcast = true; + } + + // FIXME: Maybe make the following part async, since this can block retrieval + // of compile commands. + if (Request.ShouldBroadcast && !SentBroadcast) + broadcastCDB(Result); + return Result; +} + +void DirectoryBasedGlobalCompilationDatabase::broadcastCDB( + CDBLookupResult Result) const { + assert(Result.CDB && "Trying to broadcast an invalid CDB!"); + + std::vector AllFiles = Result.CDB->getAllFiles(); + // We assume CDB in CompileCommandsDir owns all of its entries, since we don't + // perform any search in parent paths whenever it is set. if (CompileCommandsDir) { - std::tie(CDB, Cached) = getCDBInDirLocked(*CompileCommandsDir); - if (Project && CDB) - Project->SourceRoot = *CompileCommandsDir; - } else { - for (auto Path = path::parent_path(File); !CDB && !Path.empty(); - Path = path::parent_path(Path)) { - std::tie(CDB, Cached) = getCDBInDirLocked(Path); - if (Project && CDB) - Project->SourceRoot = Path; + assert(*CompileCommandsDir == Result.PI.SourceRoot && + "Trying to broadcast a CDB outside of CompileCommandsDir!"); + OnCommandChanged.broadcast(std::move(AllFiles)); + return; + } + + llvm::StringMap DirectoryHasCDB; + { + std::lock_guard Lock(Mutex); + // Uniquify all parent directories of all files. + for (llvm::StringRef File : AllFiles) { + actOnAllParentDirectories(File, [&](PathRef Path) { + auto It = DirectoryHasCDB.try_emplace(Path); + // Already seen this path, and all of its parents. + if (!It.second) + return true; + + auto Res = getCDBInDirLocked(Path); + It.first->second = Res.first != nullptr; + return Path == Result.PI.SourceRoot; + }); } } - // 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; + + std::vector GovernedFiles; + for (llvm::StringRef File : AllFiles) { + // A file is governed by this CDB if lookup for the file would find it. + // Independent of whether it has an entry for that file or not. + actOnAllParentDirectories(File, [&](PathRef Path) { + if (DirectoryHasCDB.lookup(Path)) { + if (Path == Result.PI.SourceRoot) + GovernedFiles.push_back(File); + // Stop as soon as we hit a CDB. + return true; + } + return false; + }); + } + + OnCommandChanged.broadcast(std::move(GovernedFiles)); +} + +llvm::Optional +DirectoryBasedGlobalCompilationDatabase::getProjectInfo(PathRef File) const { + CDBLookupRequest Req; + Req.FileName = File; + Req.ShouldBroadcast = false; + auto Res = lookupCDB(Req); + if (!Res) + return llvm::None; + return Res->PI; } OverlayCDB::OverlayCDB(const GlobalCompilationDatabase *Base, @@ -150,19 +244,16 @@ OverlayCDB::OverlayCDB(const GlobalCompilationDatabase *Base, } llvm::Optional -OverlayCDB::getCompileCommand(PathRef File, ProjectInfo *Project) const { +OverlayCDB::getCompileCommand(PathRef File) const { llvm::Optional Cmd; { std::lock_guard Lock(Mutex); auto It = Commands.find(File); - if (It != Commands.end()) { - if (Project) - Project->SourceRoot = ""; + if (It != Commands.end()) Cmd = It->second; - } } if (!Cmd && Base) - Cmd = Base->getCompileCommand(File, Project); + Cmd = Base->getCompileCommand(File); if (!Cmd) return llvm::None; adjustArguments(*Cmd, ResourceDir); @@ -191,5 +282,17 @@ void OverlayCDB::setCompileCommand( OnCommandChanged.broadcast({File}); } +llvm::Optional OverlayCDB::getProjectInfo(PathRef File) const { + { + std::lock_guard Lock(Mutex); + auto It = Commands.find(File); + if (It != Commands.end()) + return ProjectInfo{}; + } + if (Base) + return Base->getProjectInfo(File); + + return llvm::None; +} } // namespace clangd } // namespace clang diff --git a/clang-tools-extra/clangd/GlobalCompilationDatabase.h b/clang-tools-extra/clangd/GlobalCompilationDatabase.h index 145d0038c53c..35708c8e3024 100644 --- a/clang-tools-extra/clangd/GlobalCompilationDatabase.h +++ b/clang-tools-extra/clangd/GlobalCompilationDatabase.h @@ -36,9 +36,13 @@ public: virtual ~GlobalCompilationDatabase() = default; /// If there are any known-good commands for building this file, returns one. - /// If the ProjectInfo pointer is set, it will also be populated. virtual llvm::Optional - getCompileCommand(PathRef File, ProjectInfo * = nullptr) const = 0; + getCompileCommand(PathRef File) const = 0; + + /// Finds the closest project to \p File. + virtual llvm::Optional getProjectInfo(PathRef File) const { + return llvm::None; + } /// Makes a guess at how to build a file. /// The default implementation just runs clang on the file. @@ -67,20 +71,40 @@ public: /// Scans File's parents looking for compilation databases. /// Any extra flags will be added. + /// Might trigger OnCommandChanged, if CDB wasn't broadcasted yet. llvm::Optional - getCompileCommand(PathRef File, ProjectInfo * = nullptr) const override; + getCompileCommand(PathRef File) const override; + + /// Returns the path to first directory containing a compilation database in + /// \p File's parents. + llvm::Optional getProjectInfo(PathRef File) const override; private: - tooling::CompilationDatabase *getCDBForFile(PathRef File, - ProjectInfo *) const; - std::pair + std::pair getCDBInDirLocked(PathRef File) const; + struct CDBLookupRequest { + PathRef FileName; + // Whether this lookup should trigger discovery of the CDB found. + bool ShouldBroadcast = false; + }; + struct CDBLookupResult { + tooling::CompilationDatabase *CDB = nullptr; + ProjectInfo PI; + }; + llvm::Optional lookupCDB(CDBLookupRequest Request) const; + + // Performs broadcast on governed files. + void broadcastCDB(CDBLookupResult Res) const; + mutable std::mutex Mutex; /// Caches compilation databases loaded from directories(keys are /// directories). - mutable llvm::StringMap> - CompilationDatabases; + struct CachedCDB { + std::unique_ptr CDB = nullptr; + bool SentBroadcast = false; + }; + mutable llvm::StringMap CompilationDatabases; /// Used for command argument pointing to folder where compile_commands.json /// is located. @@ -105,8 +129,9 @@ public: llvm::Optional ResourceDir = llvm::None); llvm::Optional - getCompileCommand(PathRef File, ProjectInfo * = nullptr) const override; + getCompileCommand(PathRef File) const override; tooling::CompileCommand getFallbackCommand(PathRef File) const override; + llvm::Optional getProjectInfo(PathRef File) const override; /// Sets or clears the compilation command for a particular file. void diff --git a/clang-tools-extra/clangd/QueryDriverDatabase.cpp b/clang-tools-extra/clangd/QueryDriverDatabase.cpp index 55618415c68f..de512bcd0342 100644 --- a/clang-tools-extra/clangd/QueryDriverDatabase.cpp +++ b/clang-tools-extra/clangd/QueryDriverDatabase.cpp @@ -215,8 +215,8 @@ public: } llvm::Optional - getCompileCommand(PathRef File, ProjectInfo *PI = nullptr) const override { - auto Cmd = Base->getCompileCommand(File, PI); + getCompileCommand(PathRef File) const override { + auto Cmd = Base->getCompileCommand(File); if (!Cmd || Cmd->CommandLine.empty()) return Cmd; @@ -240,6 +240,10 @@ public: return addSystemIncludes(*Cmd, SystemIncludes); } + llvm::Optional getProjectInfo(PathRef File) const override { + return Base->getProjectInfo(File); + } + private: mutable std::mutex Mu; // Caches includes extracted from a driver. diff --git a/clang-tools-extra/clangd/index/Background.cpp b/clang-tools-extra/clangd/index/Background.cpp index 7c734d39cb3c..13c427110f0b 100644 --- a/clang-tools-extra/clangd/index/Background.cpp +++ b/clang-tools-extra/clangd/index/Background.cpp @@ -619,11 +619,15 @@ BackgroundIndex::loadShards(std::vector ChangedFiles) { llvm::StringSet<> LoadedShards; Rebuilder.startLoading(); for (const auto &File : ChangedFiles) { - ProjectInfo PI; - auto Cmd = CDB.getCompileCommand(File, &PI); + auto Cmd = CDB.getCompileCommand(File); if (!Cmd) continue; - BackgroundIndexStorage *IndexStorage = IndexStorageFactory(PI.SourceRoot); + + std::string ProjectRoot; + if (auto PI = CDB.getProjectInfo(File)) + ProjectRoot = std::move(PI->SourceRoot); + + BackgroundIndexStorage *IndexStorage = IndexStorageFactory(ProjectRoot); auto Dependencies = loadShard(*Cmd, IndexStorage, LoadedShards); for (const auto &Dependency : Dependencies) { if (!Dependency.NeedsReIndexing || FilesToIndex.count(Dependency.Path)) diff --git a/clang-tools-extra/clangd/unittests/ClangdTests.cpp b/clang-tools-extra/clangd/unittests/ClangdTests.cpp index ac4ff6ed077a..25c82e4895fc 100644 --- a/clang-tools-extra/clangd/unittests/ClangdTests.cpp +++ b/clang-tools-extra/clangd/unittests/ClangdTests.cpp @@ -1064,7 +1064,7 @@ TEST_F(ClangdVFSTest, FallbackWhenPreambleIsNotReady) { ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest()); auto FooCpp = testPath("foo.cpp"); - Annotations Code(R"cpp( + Annotations Code(R"cpp( namespace ns { int xyz; } using namespace ns; int main() { @@ -1113,7 +1113,7 @@ TEST_F(ClangdVFSTest, FallbackWhenWaitingForCompileCommand) { : CanReturnCommand(CanReturnCommand) {} llvm::Optional - getCompileCommand(PathRef File, ProjectInfo * = nullptr) const override { + getCompileCommand(PathRef File) const override { // FIXME: make this timeout and fail instead of waiting forever in case // something goes wrong. CanReturnCommand.wait(); diff --git a/clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp b/clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp index 6823fc557eeb..e6cc01b8a768 100644 --- a/clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp +++ b/clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp @@ -8,10 +8,21 @@ #include "GlobalCompilationDatabase.h" +#include "Path.h" #include "TestFS.h" +#include "llvm/ADT/Optional.h" +#include "llvm/ADT/SmallString.h" #include "llvm/ADT/StringExtras.h" +#include "llvm/ADT/StringRef.h" +#include "llvm/Support/FileSystem.h" +#include "llvm/Support/FormatVariadic.h" +#include "llvm/Support/MemoryBuffer.h" +#include "llvm/Support/Path.h" +#include "llvm/Support/raw_ostream.h" #include "gmock/gmock.h" #include "gtest/gtest.h" +#include +#include namespace clang { namespace clangd { @@ -20,8 +31,10 @@ using ::testing::AllOf; using ::testing::Contains; using ::testing::ElementsAre; using ::testing::EndsWith; +using ::testing::IsEmpty; using ::testing::Not; using ::testing::StartsWith; +using ::testing::UnorderedElementsAre; TEST(GlobalCompilationDatabaseTest, FallbackCommand) { DirectoryBasedGlobalCompilationDatabase DB(None); @@ -50,13 +63,9 @@ class OverlayCDBTest : public ::testing::Test { class BaseCDB : public GlobalCompilationDatabase { public: llvm::Optional - getCompileCommand(llvm::StringRef File, - ProjectInfo *Project) const override { - if (File == testPath("foo.cc")) { - if (Project) - Project->SourceRoot = testRoot(); + getCompileCommand(llvm::StringRef File) const override { + if (File == testPath("foo.cc")) return cmd(File, "-DA=1"); - } return None; } @@ -64,6 +73,10 @@ class OverlayCDBTest : public ::testing::Test { getFallbackCommand(llvm::StringRef File) const override { return cmd(File, "-DA=2"); } + + llvm::Optional getProjectInfo(PathRef File) const override { + return ProjectInfo{testRoot()}; + } }; protected: @@ -153,6 +166,110 @@ TEST_F(OverlayCDBTest, Adjustments) { Not(Contains("random-plugin")))); } +TEST(GlobalCompilationDatabaseTest, DiscoveryWithNestedCDBs) { + const char *const CDBOuter = + R"cdb( + [ + { + "file": "a.cc", + "command": "", + "directory": "{0}", + }, + { + "file": "build/gen.cc", + "command": "", + "directory": "{0}", + }, + { + "file": "build/gen2.cc", + "command": "", + "directory": "{0}", + } + ] + )cdb"; + const char *const CDBInner = + R"cdb( + [ + { + "file": "gen.cc", + "command": "", + "directory": "{0}/build", + } + ] + )cdb"; + class CleaningFS { + public: + llvm::SmallString<128> Root; + + CleaningFS() { + EXPECT_FALSE( + llvm::sys::fs::createUniqueDirectory("clangd-cdb-test", Root)) + << "Failed to create unique directory"; + } + + ~CleaningFS() { + EXPECT_FALSE(llvm::sys::fs::remove_directories(Root)) + << "Failed to cleanup " << Root; + } + + void registerFile(PathRef RelativePath, llvm::StringRef Contents) { + llvm::SmallString<128> AbsPath(Root); + llvm::sys::path::append(AbsPath, RelativePath); + + EXPECT_FALSE(llvm::sys::fs::create_directories( + llvm::sys::path::parent_path(AbsPath))) + << "Failed to create directories for: " << AbsPath; + + std::error_code EC; + llvm::raw_fd_ostream OS(AbsPath, EC); + EXPECT_FALSE(EC) << "Failed to open " << AbsPath << " for writing"; + OS << llvm::formatv(Contents.data(), + llvm::sys::path::convert_to_slash(Root)); + OS.close(); + + EXPECT_FALSE(OS.has_error()); + } + }; + + CleaningFS FS; + FS.registerFile("compile_commands.json", CDBOuter); + FS.registerFile("build/compile_commands.json", CDBInner); + + // Note that gen2.cc goes missing with our following model, not sure this + // happens in practice though. + { + DirectoryBasedGlobalCompilationDatabase DB(llvm::None); + std::vector DiscoveredFiles; + auto Sub = + DB.watch([&DiscoveredFiles](const std::vector Changes) { + DiscoveredFiles = Changes; + }); + DB.getCompileCommand((FS.Root + "/a.cc").str()); + EXPECT_THAT(DiscoveredFiles, UnorderedElementsAre(EndsWith("a.cc"))); + + DB.getCompileCommand((FS.Root + "/build/gen.cc").str()); + EXPECT_THAT(DiscoveredFiles, UnorderedElementsAre(EndsWith("gen.cc"))); + } + + // With a custom compile commands dir. + { + DirectoryBasedGlobalCompilationDatabase DB(FS.Root.str().str()); + std::vector DiscoveredFiles; + auto Sub = + DB.watch([&DiscoveredFiles](const std::vector Changes) { + DiscoveredFiles = Changes; + }); + DB.getCompileCommand((FS.Root + "/a.cc").str()); + EXPECT_THAT(DiscoveredFiles, + UnorderedElementsAre(EndsWith("a.cc"), EndsWith("gen.cc"), + EndsWith("gen2.cc"))); + + DiscoveredFiles.clear(); + DB.getCompileCommand((FS.Root + "/build/gen.cc").str()); + EXPECT_THAT(DiscoveredFiles, IsEmpty()); + } +} + } // namespace } // namespace clangd } // namespace clang diff --git a/clang-tools-extra/clangd/unittests/TestFS.cpp b/clang-tools-extra/clangd/unittests/TestFS.cpp index c5b2613f7592..5f5c5c21794f 100644 --- a/clang-tools-extra/clangd/unittests/TestFS.cpp +++ b/clang-tools-extra/clangd/unittests/TestFS.cpp @@ -6,7 +6,11 @@ // //===----------------------------------------------------------------------===// #include "TestFS.h" +#include "GlobalCompilationDatabase.h" +#include "Path.h" #include "URI.h" +#include "llvm/ADT/None.h" +#include "llvm/ADT/Optional.h" #include "llvm/ADT/StringRef.h" #include "llvm/Support/Errc.h" #include "llvm/Support/Path.h" @@ -36,9 +40,13 @@ MockCompilationDatabase::MockCompilationDatabase(llvm::StringRef Directory, // -ffreestanding avoids implicit stdc-predef.h. } +llvm::Optional +MockCompilationDatabase::getProjectInfo(PathRef File) const { + return ProjectInfo{Directory}; +}; + llvm::Optional -MockCompilationDatabase::getCompileCommand(PathRef File, - ProjectInfo *Project) const { +MockCompilationDatabase::getCompileCommand(PathRef File) const { if (ExtraClangFlags.empty()) return None; @@ -57,8 +65,6 @@ MockCompilationDatabase::getCompileCommand(PathRef File, CommandLine.push_back(RelativeFilePath.str()); } - if (Project) - Project->SourceRoot = Directory; return {tooling::CompileCommand(Directory != llvm::StringRef() ? Directory : llvm::sys::path::parent_path(File), diff --git a/clang-tools-extra/clangd/unittests/TestFS.h b/clang-tools-extra/clangd/unittests/TestFS.h index eabdddf70acc..a635d7c53a07 100644 --- a/clang-tools-extra/clangd/unittests/TestFS.h +++ b/clang-tools-extra/clangd/unittests/TestFS.h @@ -12,6 +12,8 @@ #ifndef LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_TESTFS_H #define LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_TESTFS_H #include "ClangdServer.h" +#include "GlobalCompilationDatabase.h" +#include "Path.h" #include "llvm/ADT/IntrusiveRefCntPtr.h" #include "llvm/Support/Path.h" #include "llvm/Support/VirtualFileSystem.h" @@ -48,7 +50,9 @@ public: StringRef RelPathPrefix = StringRef()); llvm::Optional - getCompileCommand(PathRef File, ProjectInfo * = nullptr) const override; + getCompileCommand(PathRef File) const override; + + llvm::Optional getProjectInfo(PathRef File) const override; std::vector ExtraClangFlags;