From 422c828dfce022a87fe28473743c0111f582a0ee Mon Sep 17 00:00:00 2001 From: Sam McCall Date: Mon, 26 Nov 2018 16:00:11 +0000 Subject: [PATCH] [clangd] Enable auto-index behind a flag. Summary: Ownership and configuration: The auto-index (background index) is maintained by ClangdServer, like Dynamic. (This means ClangdServer will be able to enqueue preamble indexing in future). For now it's enabled by a simple boolean flag in ClangdServer::Options, but we probably want to eventually allow injecting the storage strategy. New 'sync' command: In order to meaningfully test the integration (not just unit-test components) we need a way for tests to ensure the asynchronous index reads/writes occur before a certain point. Because these tests and assertions are few, I think exposing an explicit "sync" command for use in tests is simpler than allowing threading to be completely disabled in the background index (as we do for TUScheduler). Bugs: I fixed a couple of trivial bugs I found while testing, but there's one I can't. JSONCompilationDatabase::getAllFiles() may return relative paths, and currently we trigger an assertion that assumes they are absolute. There's no efficient way to resolve them (you have to retrieve the corresponding command and then resolve against its directory property). In general I think this behavior is broken and we should fix it in JSONCompilationDatabase and require CompilationDatabase::getAllFiles() to be absolute. Reviewers: kadircet Subscribers: ilya-biryukov, ioeric, MaskRay, jkorous, arphaman, cfe-commits Differential Revision: https://reviews.llvm.org/D54894 llvm-svn: 347567 --- clang-tools-extra/clangd/ClangdLSPServer.cpp | 12 +++++ clang-tools-extra/clangd/ClangdLSPServer.h | 1 + clang-tools-extra/clangd/ClangdServer.cpp | 33 +++++++++----- clang-tools-extra/clangd/ClangdServer.h | 12 +++-- .../clangd/GlobalCompilationDatabase.cpp | 4 +- clang-tools-extra/clangd/index/Background.cpp | 6 ++- clang-tools-extra/clangd/index/Background.h | 3 +- .../clangd/index/BackgroundIndexStorage.cpp | 5 ++- clang-tools-extra/clangd/tool/ClangdMain.cpp | 7 +++ .../Inputs/background-index/compile_commands.json | 5 +++ .../Inputs/background-index/definition.jsonrpc | 51 ++++++++++++++++++++++ .../test/clangd/Inputs/background-index/foo.cpp | 2 + .../test/clangd/Inputs/background-index/foo.h | 4 ++ .../test/clangd/background-index.test | 21 +++++++++ .../unittests/clangd/BackgroundIndexTests.cpp | 6 +-- 15 files changed, 150 insertions(+), 22 deletions(-) create mode 100644 clang-tools-extra/test/clangd/Inputs/background-index/compile_commands.json create mode 100644 clang-tools-extra/test/clangd/Inputs/background-index/definition.jsonrpc create mode 100644 clang-tools-extra/test/clangd/Inputs/background-index/foo.cpp create mode 100644 clang-tools-extra/test/clangd/Inputs/background-index/foo.h create mode 100644 clang-tools-extra/test/clangd/background-index.test diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp b/clang-tools-extra/clangd/ClangdLSPServer.cpp index b2ac431..9ed22fd 100644 --- a/clang-tools-extra/clangd/ClangdLSPServer.cpp +++ b/clang-tools-extra/clangd/ClangdLSPServer.cpp @@ -337,6 +337,17 @@ void ClangdLSPServer::onShutdown(const ShutdownParams &Params, Reply(nullptr); } +// sync is a clangd extension: it blocks until all background work completes. +// It blocks the calling thread, so no messages are processed until it returns! +void ClangdLSPServer::onSync(const NoParams &Params, + Callback Reply) { + if (Server->blockUntilIdleForTest(/*TimeoutSeconds=*/60)) + Reply(nullptr); + else + Reply(createStringError(llvm::inconvertibleErrorCode(), + "Not idle after a minute")); +} + void ClangdLSPServer::onDocumentDidOpen( const DidOpenTextDocumentParams &Params) { PathRef File = Params.textDocument.uri.file(); @@ -701,6 +712,7 @@ ClangdLSPServer::ClangdLSPServer(class Transport &Transp, // clang-format off MsgHandler->bind("initialize", &ClangdLSPServer::onInitialize); MsgHandler->bind("shutdown", &ClangdLSPServer::onShutdown); + MsgHandler->bind("sync", &ClangdLSPServer::onSync); MsgHandler->bind("textDocument/rangeFormatting", &ClangdLSPServer::onDocumentRangeFormatting); MsgHandler->bind("textDocument/onTypeFormatting", &ClangdLSPServer::onDocumentOnTypeFormatting); MsgHandler->bind("textDocument/formatting", &ClangdLSPServer::onDocumentFormatting); diff --git a/clang-tools-extra/clangd/ClangdLSPServer.h b/clang-tools-extra/clangd/ClangdLSPServer.h index e4a369b..25d701a 100644 --- a/clang-tools-extra/clangd/ClangdLSPServer.h +++ b/clang-tools-extra/clangd/ClangdLSPServer.h @@ -57,6 +57,7 @@ private: // Calls have signature void(const Params&, Callback). void onInitialize(const InitializeParams &, Callback); void onShutdown(const ShutdownParams &, Callback); + void onSync(const NoParams &, Callback); void onDocumentDidOpen(const DidOpenTextDocumentParams &); void onDocumentDidChange(const DidChangeTextDocumentParams &); void onDocumentDidClose(const DidCloseTextDocumentParams &); diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp index 8a86a5b..2b48281 100644 --- a/clang-tools-extra/clangd/ClangdServer.cpp +++ b/clang-tools-extra/clangd/ClangdServer.cpp @@ -121,16 +121,25 @@ ClangdServer::ClangdServer(const GlobalCompilationDatabase &CDB, llvm::make_unique(DynamicIdx.get(), DiagConsumer), Opts.UpdateDebounce, Opts.RetentionPolicy) { - if (DynamicIdx && Opts.StaticIndex) { - MergedIdx = - llvm::make_unique(DynamicIdx.get(), Opts.StaticIndex); - Index = MergedIdx.get(); - } else if (DynamicIdx) - Index = DynamicIdx.get(); - else if (Opts.StaticIndex) - Index = Opts.StaticIndex; - else - Index = nullptr; + // Adds an index to the stack, at higher priority than existing indexes. + auto AddIndex = [&](SymbolIndex *Idx) { + if (this->Index != nullptr) { + MergedIdx.push_back(llvm::make_unique(Idx, this->Index)); + this->Index = MergedIdx.back().get(); + } else { + this->Index = Idx; + } + }; + if (Opts.StaticIndex) + AddIndex(Opts.StaticIndex); + if (Opts.BackgroundIndex) { + BackgroundIdx = llvm::make_unique( + Context::current().clone(), ResourceDir, FSProvider, CDB, + BackgroundIndexStorage::createDiskBackedStorageFactory()); + AddIndex(BackgroundIdx.get()); + } + if (DynamicIdx) + AddIndex(DynamicIdx.get()); } void ClangdServer::addDocument(PathRef File, StringRef Contents, @@ -501,7 +510,9 @@ ClangdServer::getUsedBytesPerFile() const { LLVM_NODISCARD bool ClangdServer::blockUntilIdleForTest(Optional TimeoutSeconds) { - return WorkScheduler.blockUntilIdle(timeoutSeconds(TimeoutSeconds)); + return WorkScheduler.blockUntilIdle(timeoutSeconds(TimeoutSeconds)) && + (!BackgroundIdx || + BackgroundIdx->blockUntilIdleForTest(TimeoutSeconds)); } } // namespace clangd diff --git a/clang-tools-extra/clangd/ClangdServer.h b/clang-tools-extra/clangd/ClangdServer.h index 8edbdae..18527cb 100644 --- a/clang-tools-extra/clangd/ClangdServer.h +++ b/clang-tools-extra/clangd/ClangdServer.h @@ -18,6 +18,7 @@ #include "GlobalCompilationDatabase.h" #include "Protocol.h" #include "TUScheduler.h" +#include "index/Background.h" #include "index/FileIndex.h" #include "index/Index.h" #include "clang/Tooling/CompilationDatabase.h" @@ -78,6 +79,9 @@ public: /// Use a heavier and faster in-memory index implementation. /// FIXME: we should make this true if it isn't too slow to build!. bool HeavyweightDynamicSymbolIndex = false; + /// If true, ClangdServer automatically indexes files in the current project + /// on background threads. The index is stored in the project root. + bool BackgroundIndex = false; /// If set, use this index to augment code completion results. SymbolIndex *StaticIndex = nullptr; @@ -234,11 +238,13 @@ private: // - the dynamic index owned by ClangdServer (DynamicIdx) // - the static index passed to the constructor // - a merged view of a static and dynamic index (MergedIndex) - const SymbolIndex *Index; + const SymbolIndex *Index = nullptr; // If present, an index of symbols in open files. Read via *Index. std::unique_ptr DynamicIdx; - // If present, storage for the merged static/dynamic index. Read via *Index. - std::unique_ptr MergedIdx; + // If present, the new "auto-index" maintained in background threads. + std::unique_ptr BackgroundIdx; + // Storage for merged views of the various indexes. + std::vector> MergedIdx; // GUARDED_BY(CachedCompletionFuzzyFindRequestMutex) llvm::StringMap> diff --git a/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp b/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp index d2a10ba..055f92c 100644 --- a/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp +++ b/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp @@ -87,6 +87,8 @@ DirectoryBasedGlobalCompilationDatabase::getCDBForFile( Project->SourceRoot = Path; } } + // 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; @@ -112,7 +114,7 @@ OverlayCDB::getCompileCommand(PathRef File, ProjectInfo *Project) const { return It->second; } } - return Base ? Base->getCompileCommand(File) : None; + return Base ? Base->getCompileCommand(File, Project) : None; } tooling::CompileCommand OverlayCDB::getFallbackCommand(PathRef File) const { diff --git a/clang-tools-extra/clangd/index/Background.cpp b/clang-tools-extra/clangd/index/Background.cpp index 9d82e65..ecd5865 100644 --- a/clang-tools-extra/clangd/index/Background.cpp +++ b/clang-tools-extra/clangd/index/Background.cpp @@ -99,9 +99,11 @@ void BackgroundIndex::run() { } } -void BackgroundIndex::blockUntilIdleForTest() { +bool BackgroundIndex::blockUntilIdleForTest( + llvm::Optional TimeoutSeconds) { std::unique_lock Lock(QueueMu); - QueueCV.wait(Lock, [&] { return Queue.empty() && NumActiveTasks == 0; }); + return wait(Lock, QueueCV, timeoutSeconds(TimeoutSeconds), + [&] { return Queue.empty() && NumActiveTasks == 0; }); } void BackgroundIndex::enqueue(const std::vector &ChangedFiles) { diff --git a/clang-tools-extra/clangd/index/Background.h b/clang-tools-extra/clangd/index/Background.h index eb9ce3e..c71e48f 100644 --- a/clang-tools-extra/clangd/index/Background.h +++ b/clang-tools-extra/clangd/index/Background.h @@ -81,7 +81,8 @@ public: void stop(); // Wait until the queue is empty, to allow deterministic testing. - void blockUntilIdleForTest(); + LLVM_NODISCARD bool + blockUntilIdleForTest(llvm::Optional TimeoutSeconds = 10); using FileDigest = decltype(llvm::SHA1::hash({})); diff --git a/clang-tools-extra/clangd/index/BackgroundIndexStorage.cpp b/clang-tools-extra/clangd/index/BackgroundIndexStorage.cpp index 64512e8..4325ded 100644 --- a/clang-tools-extra/clangd/index/BackgroundIndexStorage.cpp +++ b/clang-tools-extra/clangd/index/BackgroundIndexStorage.cpp @@ -30,7 +30,7 @@ std::string getShardPathFromFilePath(llvm::StringRef ShardRoot, llvm::sys::path::append(ShardRootSS, llvm::sys::path::filename(FilePath) + "." + llvm::toHex(digest(FilePath)) + ".idx"); - return ShardRoot.str(); + return ShardRootSS.str(); } // Uses disk as a storage for index shards. Creates a directory called @@ -101,6 +101,9 @@ public: // Creates and owns IndexStorages for multiple CDBs. class DiskBackedIndexStorageManager { public: + DiskBackedIndexStorageManager() + : IndexStorageMapMu(llvm::make_unique()) {} + // Creates or fetches to storage from cache for the specified CDB. BackgroundIndexStorage *operator()(llvm::StringRef CDBDirectory) { std::lock_guard Lock(*IndexStorageMapMu); diff --git a/clang-tools-extra/clangd/tool/ClangdMain.cpp b/clang-tools-extra/clangd/tool/ClangdMain.cpp index ff87985..e349ee0 100644 --- a/clang-tools-extra/clangd/tool/ClangdMain.cpp +++ b/clang-tools-extra/clangd/tool/ClangdMain.cpp @@ -164,6 +164,12 @@ static cl::opt IndexFile( "eventually. Don't rely on it."), cl::init(""), cl::Hidden); +static cl::opt EnableBackgroundIndex( + "background-index", + cl::desc("Index project code in the background and persist index on disk. " + "Experimental"), + cl::init(false), cl::Hidden); + enum CompileArgsFrom { LSPCompileArgs, FilesystemCompileArgs }; static cl::opt CompileArgsFrom( "compile_args_from", cl::desc("The source of compile commands"), @@ -344,6 +350,7 @@ int main(int argc, char *argv[]) { Opts.ResourceDir = ResourceDir; Opts.BuildDynamicSymbolIndex = EnableIndex; Opts.HeavyweightDynamicSymbolIndex = UseDex; + Opts.BackgroundIndex = EnableBackgroundIndex; std::unique_ptr StaticIdx; std::future AsyncIndexLoad; // Block exit while loading the index. if (EnableIndex && !IndexFile.empty()) { diff --git a/clang-tools-extra/test/clangd/Inputs/background-index/compile_commands.json b/clang-tools-extra/test/clangd/Inputs/background-index/compile_commands.json new file mode 100644 index 0000000..1bb835f --- /dev/null +++ b/clang-tools-extra/test/clangd/Inputs/background-index/compile_commands.json @@ -0,0 +1,5 @@ +[{ + "directory": "DIRECTORY", + "command": "clang foo.cpp", + "file": "DIRECTORY/foo.cpp" +}] diff --git a/clang-tools-extra/test/clangd/Inputs/background-index/definition.jsonrpc b/clang-tools-extra/test/clangd/Inputs/background-index/definition.jsonrpc new file mode 100644 index 0000000..89d5048 --- /dev/null +++ b/clang-tools-extra/test/clangd/Inputs/background-index/definition.jsonrpc @@ -0,0 +1,51 @@ +{ + "jsonrpc": "2.0", + "id": 0, + "method": "initialize", + "params": { + "processId": 123, + "rootPath": "clangd", + "capabilities": {}, + "trace": "off" + } +} +--- +{ + "jsonrpc": "2.0", + "method": "textDocument/didOpen", + "params": { + "textDocument": { + "uri": "file://DIRECTORY/bar.cpp", + "languageId": "cpp", + "version": 1, + "text": "#include \"foo.h\"\nint main(){\nreturn foo();\n}" + } + } +} +--- +{ + "jsonrpc": "2.0", + "id": 1, + "method": "sync", + "params": null +} +--- +{ + "jsonrpc": "2.0", + "id": 2, + "method": "textDocument/definition", + "params": { + "textDocument": { + "uri": "file://DIRECTORY/bar.cpp" + }, + "position": { + "line": 2, + "character": 8 + } + } +} +# CHECK: "uri": "file://DIRECTORY/foo.cpp" +--- +{"jsonrpc":"2.0","id":3,"method":"shutdown"} +--- +{"jsonrpc":"2.0","method":"exit"} diff --git a/clang-tools-extra/test/clangd/Inputs/background-index/foo.cpp b/clang-tools-extra/test/clangd/Inputs/background-index/foo.cpp new file mode 100644 index 0000000..c42ca4d --- /dev/null +++ b/clang-tools-extra/test/clangd/Inputs/background-index/foo.cpp @@ -0,0 +1,2 @@ +#include "foo.h" +int foo() { return 42; } diff --git a/clang-tools-extra/test/clangd/Inputs/background-index/foo.h b/clang-tools-extra/test/clangd/Inputs/background-index/foo.h new file mode 100644 index 0000000..9539f1d --- /dev/null +++ b/clang-tools-extra/test/clangd/Inputs/background-index/foo.h @@ -0,0 +1,4 @@ +#ifndef FOO_H +#define FOO_H +int foo(); +#endif diff --git a/clang-tools-extra/test/clangd/background-index.test b/clang-tools-extra/test/clangd/background-index.test new file mode 100644 index 0000000..fbd3da8 --- /dev/null +++ b/clang-tools-extra/test/clangd/background-index.test @@ -0,0 +1,21 @@ +# We need to splice paths into file:// URIs for this test. +# UNSUPPORTED: win32 + +# Use a copy of inputs, as we'll mutate it (as will the background index). +# RUN: rm -rf %t +# RUN: cp -r %S/Inputs/background-index %t +# Need to embed the correct temp path in the actual JSON-RPC requests. +# RUN: sed -i -e "s|DIRECTORY|%t|" %t/* + +# We're editing bar.cpp, which includes foo.h. +# foo() is declared in foo.h and defined in foo.cpp. +# The background index should allow us to go-to-definition on foo(). +# RUN: clangd -background-index -lit-test < %t/definition.jsonrpc | FileCheck %t/definition.jsonrpc + +# Test that the index is writing files in the expected location. +# RUN: ls %t/.clangd-index/foo.cpp.*.idx + +# Test the index is read from disk: delete code and restart clangd. +# FIXME: This test currently fails as we don't read the index yet. +# RUN: rm %t/foo.cpp +# RUN: clangd -background-index -lit-test < %t/definition.jsonrpc | not FileCheck %t/definition.jsonrpc diff --git a/clang-tools-extra/unittests/clangd/BackgroundIndexTests.cpp b/clang-tools-extra/unittests/clangd/BackgroundIndexTests.cpp index 493d7c2..2294add3 100644 --- a/clang-tools-extra/unittests/clangd/BackgroundIndexTests.cpp +++ b/clang-tools-extra/unittests/clangd/BackgroundIndexTests.cpp @@ -90,7 +90,7 @@ TEST(BackgroundIndexTest, IndexTwoFiles) { Cmd.CommandLine = {"clang++", "-DA=1", testPath("root/A.cc")}; CDB.setCompileCommand(testPath("root"), Cmd); - Idx.blockUntilIdleForTest(); + ASSERT_TRUE(Idx.blockUntilIdleForTest()); EXPECT_THAT( runFuzzyFind(Idx, ""), UnorderedElementsAre(Named("common"), Named("A_CC"), @@ -100,7 +100,7 @@ TEST(BackgroundIndexTest, IndexTwoFiles) { Cmd.CommandLine = {"clang++", Cmd.Filename}; CDB.setCompileCommand(testPath("root"), Cmd); - Idx.blockUntilIdleForTest(); + ASSERT_TRUE(Idx.blockUntilIdleForTest()); // B_CC is dropped as we don't collect symbols from A.h in this compilation. EXPECT_THAT(runFuzzyFind(Idx, ""), UnorderedElementsAre(Named("common"), Named("A_CC"), @@ -141,7 +141,7 @@ TEST(BackgroundIndexTest, ShardStorageWriteTest) { BackgroundIndex Idx(Context::empty(), "", FS, CDB, [&](llvm::StringRef) { return &MSS; }); CDB.setCompileCommand(testPath("root"), Cmd); - Idx.blockUntilIdleForTest(); + ASSERT_TRUE(Idx.blockUntilIdleForTest()); } EXPECT_EQ(CacheHits, 0U); EXPECT_EQ(Storage.size(), 2U); -- 2.7.4