From: Matthew Voss Date: Wed, 10 Jul 2019 18:16:35 +0000 (+0000) Subject: Revert "[clangd] Filter out non-governed files from broadcast" X-Git-Tag: llvmorg-10-init~724 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=6d1a64e489ecd71d042609d4f79901dba3cb060b;p=platform%2Fupstream%2Fllvm.git Revert "[clangd] Filter out non-governed files from broadcast" This reverts commit d5214dfa7b5650745eaeb102857c9e90adb16137. It's causing failures, both in our local CI and the PS4 Windows bot. http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/26872/steps/test/logs/stdio llvm-svn: 365678 --- diff --git a/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp b/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp index 51f0e7d..9a09482 100644 --- a/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp +++ b/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp @@ -8,18 +8,12 @@ #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 { @@ -49,16 +43,6 @@ 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() { @@ -97,138 +81,60 @@ DirectoryBasedGlobalCompilationDatabase:: ~DirectoryBasedGlobalCompilationDatabase() = default; llvm::Optional -DirectoryBasedGlobalCompilationDatabase::getCompileCommand(PathRef File) const { - CDBLookupRequest Req; - Req.FileName = File; - Req.ShouldBroadcast = true; - - auto Res = lookupCDB(Req); - if (!Res) { +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 { 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.CDB.get(), CachedIt->second.SentBroadcast}; + return {CachedIt->second.get(), true}; std::string Error = ""; - - CachedCDB Entry; - Entry.CDB = tooling::CompilationDatabase::loadFromDirectory(Dir, Error); - auto Result = Entry.CDB.get(); - CompilationDatabases[Dir] = std::move(Entry); - + auto CDB = tooling::CompilationDatabase::loadFromDirectory(Dir, Error); + auto Result = CDB.get(); + CompilationDatabases.insert(std::make_pair(Dir, std::move(CDB))); return {Result, false}; } -llvm::Optional -DirectoryBasedGlobalCompilationDatabase::lookupCDB( - CDBLookupRequest Request) const { - assert(llvm::sys::path::is_absolute(Request.FileName) && +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)) && "path must be absolute"); - 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. + tooling::CompilationDatabase *CDB = nullptr; + bool Cached = false; + std::lock_guard Lock(Mutex); if (CompileCommandsDir) { - 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; - }); + 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; } } - - 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; + // 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, @@ -244,16 +150,19 @@ OverlayCDB::OverlayCDB(const GlobalCompilationDatabase *Base, } llvm::Optional -OverlayCDB::getCompileCommand(PathRef File) const { +OverlayCDB::getCompileCommand(PathRef File, ProjectInfo *Project) const { llvm::Optional Cmd; { std::lock_guard Lock(Mutex); auto It = Commands.find(File); - if (It != Commands.end()) + if (It != Commands.end()) { + if (Project) + Project->SourceRoot = ""; Cmd = It->second; + } } if (!Cmd && Base) - Cmd = Base->getCompileCommand(File); + Cmd = Base->getCompileCommand(File, Project); if (!Cmd) return llvm::None; adjustArguments(*Cmd, ResourceDir); @@ -282,17 +191,5 @@ 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 95aaa33..d16ea67 100644 --- a/clang-tools-extra/clangd/GlobalCompilationDatabase.h +++ b/clang-tools-extra/clangd/GlobalCompilationDatabase.h @@ -40,13 +40,9 @@ 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) const = 0; - - /// Finds the closest project to \p File. - virtual llvm::Optional getProjectInfo(PathRef File) const { - return llvm::None; - } + getCompileCommand(PathRef File, ProjectInfo * = nullptr) const = 0; /// Makes a guess at how to build a file. /// The default implementation just runs clang on the file. @@ -75,40 +71,20 @@ 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) const override; - - /// Returns the path to first directory containing a compilation database in - /// \p File's parents. - llvm::Optional getProjectInfo(PathRef File) const override; + getCompileCommand(PathRef File, ProjectInfo * = nullptr) const override; private: - std::pair + tooling::CompilationDatabase *getCDBForFile(PathRef File, + ProjectInfo *) const; + 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). - struct CachedCDB { - std::unique_ptr CDB = nullptr; - bool SentBroadcast = false; - }; - mutable llvm::StringMap CompilationDatabases; + mutable llvm::StringMap> + CompilationDatabases; /// Used for command argument pointing to folder where compile_commands.json /// is located. @@ -133,9 +109,8 @@ public: llvm::Optional ResourceDir = llvm::None); llvm::Optional - getCompileCommand(PathRef File) const override; + getCompileCommand(PathRef File, ProjectInfo * = nullptr) 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 de512bc..5561841 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) const override { - auto Cmd = Base->getCompileCommand(File); + getCompileCommand(PathRef File, ProjectInfo *PI = nullptr) const override { + auto Cmd = Base->getCompileCommand(File, PI); if (!Cmd || Cmd->CommandLine.empty()) return Cmd; @@ -240,10 +240,6 @@ 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 13c4271..7c734d3 100644 --- a/clang-tools-extra/clangd/index/Background.cpp +++ b/clang-tools-extra/clangd/index/Background.cpp @@ -619,15 +619,11 @@ BackgroundIndex::loadShards(std::vector ChangedFiles) { llvm::StringSet<> LoadedShards; Rebuilder.startLoading(); for (const auto &File : ChangedFiles) { - auto Cmd = CDB.getCompileCommand(File); + ProjectInfo PI; + auto Cmd = CDB.getCompileCommand(File, &PI); if (!Cmd) continue; - - std::string ProjectRoot; - if (auto PI = CDB.getProjectInfo(File)) - ProjectRoot = std::move(PI->SourceRoot); - - BackgroundIndexStorage *IndexStorage = IndexStorageFactory(ProjectRoot); + BackgroundIndexStorage *IndexStorage = IndexStorageFactory(PI.SourceRoot); 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 25c82e4..ac4ff6e 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) const override { + getCompileCommand(PathRef File, ProjectInfo * = nullptr) 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 5a09cfa..6823fc5 100644 --- a/clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp +++ b/clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp @@ -8,21 +8,10 @@ #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 { @@ -31,10 +20,8 @@ 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); @@ -63,9 +50,13 @@ class OverlayCDBTest : public ::testing::Test { class BaseCDB : public GlobalCompilationDatabase { public: llvm::Optional - getCompileCommand(llvm::StringRef File) const override { - if (File == testPath("foo.cc")) + getCompileCommand(llvm::StringRef File, + ProjectInfo *Project) const override { + if (File == testPath("foo.cc")) { + if (Project) + Project->SourceRoot = testRoot(); return cmd(File, "-DA=1"); + } return None; } @@ -73,10 +64,6 @@ 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: @@ -166,109 +153,6 @@ 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(), 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 5f5c5c2..c5b2613 100644 --- a/clang-tools-extra/clangd/unittests/TestFS.cpp +++ b/clang-tools-extra/clangd/unittests/TestFS.cpp @@ -6,11 +6,7 @@ // //===----------------------------------------------------------------------===// #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" @@ -40,13 +36,9 @@ 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) const { +MockCompilationDatabase::getCompileCommand(PathRef File, + ProjectInfo *Project) const { if (ExtraClangFlags.empty()) return None; @@ -65,6 +57,8 @@ MockCompilationDatabase::getCompileCommand(PathRef File) const { 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 a635d7c..eabdddf 100644 --- a/clang-tools-extra/clangd/unittests/TestFS.h +++ b/clang-tools-extra/clangd/unittests/TestFS.h @@ -12,8 +12,6 @@ #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" @@ -50,9 +48,7 @@ public: StringRef RelPathPrefix = StringRef()); llvm::Optional - getCompileCommand(PathRef File) const override; - - llvm::Optional getProjectInfo(PathRef File) const override; + getCompileCommand(PathRef File, ProjectInfo * = nullptr) const override; std::vector ExtraClangFlags;