Author: ibiryukov Date: Thu May 24 08:50:15 2018 New Revision: 333196 URL: http://llvm.org/viewvc/llvm-project?rev=333196&view=rev Log: [clangd] Build index on preamble changes instead of the AST changes
Summary: This is more efficient and avoids data races when reading files that come from the preamble. The staleness can occur when reading a file from disk that changed after the preamble was built. This can lead to crashes, e.g. when parsing comments. We do not to rely on symbols from the main file anyway, since any info that those provide can always be taken from the AST. Reviewers: ioeric, sammccall Reviewed By: ioeric Subscribers: malaperle, klimek, javed.absar, MaskRay, jkorous, cfe-commits Differential Revision: https://reviews.llvm.org/D47272 Modified: clang-tools-extra/trunk/clangd/ClangdServer.cpp clang-tools-extra/trunk/clangd/ClangdUnit.cpp clang-tools-extra/trunk/clangd/ClangdUnit.h clang-tools-extra/trunk/clangd/TUScheduler.cpp clang-tools-extra/trunk/clangd/TUScheduler.h clang-tools-extra/trunk/clangd/index/FileIndex.cpp clang-tools-extra/trunk/clangd/index/FileIndex.h clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp clang-tools-extra/trunk/unittests/clangd/TestTU.cpp Modified: clang-tools-extra/trunk/clangd/ClangdServer.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdServer.cpp?rev=333196&r1=333195&r2=333196&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/ClangdServer.cpp (original) +++ clang-tools-extra/trunk/clangd/ClangdServer.cpp Thu May 24 08:50:15 2018 @@ -17,6 +17,7 @@ #include "clang/Format/Format.h" #include "clang/Frontend/CompilerInstance.h" #include "clang/Frontend/CompilerInvocation.h" +#include "clang/Lex/Preprocessor.h" #include "clang/Tooling/CompilationDatabase.h" #include "clang/Tooling/Refactoring/RefactoringResultConsumer.h" #include "clang/Tooling/Refactoring/Rename/RenamingAction.h" @@ -92,12 +93,14 @@ ClangdServer::ClangdServer(GlobalCompila // is parsed. // FIXME(ioeric): this can be slow and we may be able to index on less // critical paths. - WorkScheduler(Opts.AsyncThreadsCount, Opts.StorePreamblesInMemory, - FileIdx - ? [this](PathRef Path, - ParsedAST *AST) { FileIdx->update(Path, AST); } - : ASTParsedCallback(), - Opts.UpdateDebounce) { + WorkScheduler( + Opts.AsyncThreadsCount, Opts.StorePreamblesInMemory, + FileIdx + ? [this](PathRef Path, ASTContext &AST, + std::shared_ptr<Preprocessor> + PP) { FileIdx->update(Path, &AST, std::move(PP)); } + : PreambleParsedCallback(), + Opts.UpdateDebounce) { if (FileIdx && Opts.StaticIndex) { MergedIndex = mergeIndex(FileIdx.get(), Opts.StaticIndex); Index = MergedIndex.get(); Modified: clang-tools-extra/trunk/clangd/ClangdUnit.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdUnit.cpp?rev=333196&r1=333195&r2=333196&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/ClangdUnit.cpp (original) +++ clang-tools-extra/trunk/clangd/ClangdUnit.cpp Thu May 24 08:50:15 2018 @@ -13,6 +13,8 @@ #include "Logger.h" #include "SourceCode.h" #include "Trace.h" +#include "clang/AST/ASTContext.h" +#include "clang/Basic/LangOptions.h" #include "clang/Frontend/CompilerInstance.h" #include "clang/Frontend/CompilerInvocation.h" #include "clang/Frontend/FrontendActions.h" @@ -25,7 +27,6 @@ #include "clang/Sema/Sema.h" #include "clang/Serialization/ASTWriter.h" #include "clang/Tooling/CompilationDatabase.h" -#include "clang/Basic/LangOptions.h" #include "llvm/ADT/ArrayRef.h" #include "llvm/ADT/SmallVector.h" #include "llvm/Support/CrashRecoveryContext.h" @@ -86,6 +87,9 @@ private: class CppFilePreambleCallbacks : public PreambleCallbacks { public: + CppFilePreambleCallbacks(PathRef File, PreambleParsedCallback ParsedCallback) + : File(File), ParsedCallback(ParsedCallback) {} + std::vector<serialization::DeclID> takeTopLevelDeclIDs() { return std::move(TopLevelDeclIDs); } @@ -102,6 +106,13 @@ public: } } + void AfterExecute(CompilerInstance &CI) override { + if (!ParsedCallback) + return; + trace::Span Tracer("Running PreambleCallback"); + ParsedCallback(File, CI.getASTContext(), CI.getPreprocessorPtr()); + } + void HandleTopLevelDecl(DeclGroupRef DG) override { for (Decl *D : DG) { if (isa<ObjCMethodDecl>(D)) @@ -122,6 +133,8 @@ public: } private: + PathRef File; + PreambleParsedCallback ParsedCallback; std::vector<Decl *> TopLevelDecls; std::vector<serialization::DeclID> TopLevelDeclIDs; std::vector<Inclusion> Inclusions; @@ -277,9 +290,9 @@ ParsedAST::ParsedAST(std::shared_ptr<con CppFile::CppFile(PathRef FileName, bool StorePreamblesInMemory, std::shared_ptr<PCHContainerOperations> PCHs, - ASTParsedCallback ASTCallback) + PreambleParsedCallback PreambleCallback) : FileName(FileName), StorePreamblesInMemory(StorePreamblesInMemory), - PCHs(std::move(PCHs)), ASTCallback(std::move(ASTCallback)) { + PCHs(std::move(PCHs)), PreambleCallback(std::move(PreambleCallback)) { log("Created CppFile for " + FileName); } @@ -346,10 +359,6 @@ llvm::Optional<std::vector<Diag>> CppFil Diagnostics.insert(Diagnostics.end(), NewAST->getDiagnostics().begin(), NewAST->getDiagnostics().end()); } - if (ASTCallback && NewAST) { - trace::Span Tracer("Running ASTCallback"); - ASTCallback(FileName, NewAST.getPointer()); - } // Write the results of rebuild into class fields. Command = std::move(Inputs.CompileCommand); @@ -404,7 +413,7 @@ CppFile::rebuildPreamble(CompilerInvocat assert(!CI.getFrontendOpts().SkipFunctionBodies); CI.getFrontendOpts().SkipFunctionBodies = true; - CppFilePreambleCallbacks SerializedDeclsCollector; + CppFilePreambleCallbacks SerializedDeclsCollector(FileName, PreambleCallback); auto BuiltPreamble = PrecompiledPreamble::Build( CI, &ContentsBuffer, Bounds, *PreambleDiagsEngine, FS, PCHs, /*StoreInMemory=*/StorePreamblesInMemory, SerializedDeclsCollector); Modified: clang-tools-extra/trunk/clangd/ClangdUnit.h URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdUnit.h?rev=333196&r1=333195&r2=333196&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/ClangdUnit.h (original) +++ clang-tools-extra/trunk/clangd/ClangdUnit.h Thu May 24 08:50:15 2018 @@ -17,6 +17,7 @@ #include "Protocol.h" #include "clang/Frontend/FrontendAction.h" #include "clang/Frontend/PrecompiledPreamble.h" +#include "clang/Lex/Preprocessor.h" #include "clang/Serialization/ASTBitCodes.h" #include "clang/Tooling/CompilationDatabase.h" #include "clang/Tooling/Core/Replacement.h" @@ -126,7 +127,8 @@ private: std::vector<Inclusion> Inclusions; }; -using ASTParsedCallback = std::function<void(PathRef Path, ParsedAST *)>; +using PreambleParsedCallback = std::function<void( + PathRef Path, ASTContext &, std::shared_ptr<clang::Preprocessor>)>; /// Manages resources, required by clangd. Allows to rebuild file with new /// contents, and provides AST and Preamble for it. @@ -134,7 +136,7 @@ class CppFile { public: CppFile(PathRef FileName, bool StorePreamblesInMemory, std::shared_ptr<PCHContainerOperations> PCHs, - ASTParsedCallback ASTCallback); + PreambleParsedCallback PreambleCallback); /// Rebuild the AST and the preamble. /// Returns a list of diagnostics or llvm::None, if an error occured. @@ -170,7 +172,7 @@ private: std::shared_ptr<PCHContainerOperations> PCHs; /// This is called after the file is parsed. This can be nullptr if there is /// no callback. - ASTParsedCallback ASTCallback; + PreambleParsedCallback PreambleCallback; }; /// Get the beginning SourceLocation at a specified \p Pos. Modified: clang-tools-extra/trunk/clangd/TUScheduler.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/TUScheduler.cpp?rev=333196&r1=333195&r2=333196&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/TUScheduler.cpp (original) +++ clang-tools-extra/trunk/clangd/TUScheduler.cpp Thu May 24 08:50:15 2018 @@ -414,11 +414,11 @@ struct TUScheduler::FileData { TUScheduler::TUScheduler(unsigned AsyncThreadsCount, bool StorePreamblesInMemory, - ASTParsedCallback ASTCallback, + PreambleParsedCallback PreambleCallback, steady_clock::duration UpdateDebounce) : StorePreamblesInMemory(StorePreamblesInMemory), PCHOps(std::make_shared<PCHContainerOperations>()), - ASTCallback(std::move(ASTCallback)), Barrier(AsyncThreadsCount), + PreambleCallback(std::move(PreambleCallback)), Barrier(AsyncThreadsCount), UpdateDebounce(UpdateDebounce) { if (0 < AsyncThreadsCount) { PreambleTasks.emplace(); @@ -455,7 +455,7 @@ void TUScheduler::update(PathRef File, P // Create a new worker to process the AST-related tasks. ASTWorkerHandle Worker = ASTWorker::Create( File, WorkerThreads ? WorkerThreads.getPointer() : nullptr, Barrier, - CppFile(File, StorePreamblesInMemory, PCHOps, ASTCallback), + CppFile(File, StorePreamblesInMemory, PCHOps, PreambleCallback), UpdateDebounce); FD = std::unique_ptr<FileData>(new FileData{ Inputs.Contents, Inputs.CompileCommand, std::move(Worker)}); Modified: clang-tools-extra/trunk/clangd/TUScheduler.h URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/TUScheduler.h?rev=333196&r1=333195&r2=333196&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/TUScheduler.h (original) +++ clang-tools-extra/trunk/clangd/TUScheduler.h Thu May 24 08:50:15 2018 @@ -52,7 +52,7 @@ enum class WantDiagnostics { class TUScheduler { public: TUScheduler(unsigned AsyncThreadsCount, bool StorePreamblesInMemory, - ASTParsedCallback ASTCallback, + PreambleParsedCallback PreambleCallback, std::chrono::steady_clock::duration UpdateDebounce); ~TUScheduler(); @@ -101,7 +101,7 @@ private: const bool StorePreamblesInMemory; const std::shared_ptr<PCHContainerOperations> PCHOps; - const ASTParsedCallback ASTCallback; + const PreambleParsedCallback PreambleCallback; Semaphore Barrier; llvm::StringMap<std::unique_ptr<FileData>> Files; // None when running tasks synchronously and non-None when running tasks Modified: clang-tools-extra/trunk/clangd/index/FileIndex.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/FileIndex.cpp?rev=333196&r1=333195&r2=333196&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/index/FileIndex.cpp (original) +++ clang-tools-extra/trunk/clangd/index/FileIndex.cpp Thu May 24 08:50:15 2018 @@ -10,12 +10,12 @@ #include "FileIndex.h" #include "SymbolCollector.h" #include "clang/Index/IndexingAction.h" +#include "clang/Lex/Preprocessor.h" namespace clang { namespace clangd { -SymbolSlab indexAST(ParsedAST *AST) { - assert(AST && "AST must not be nullptr!"); +SymbolSlab indexAST(ASTContext &AST, std::shared_ptr<Preprocessor> PP) { SymbolCollector::Options CollectorOpts; // FIXME(ioeric): we might also want to collect include headers. We would need // to make sure all includes are canonicalized (with CanonicalIncludes), which @@ -26,15 +26,18 @@ SymbolSlab indexAST(ParsedAST *AST) { CollectorOpts.CountReferences = false; SymbolCollector Collector(std::move(CollectorOpts)); - Collector.setPreprocessor(AST->getPreprocessorPtr()); + Collector.setPreprocessor(PP); index::IndexingOptions IndexOpts; // We only need declarations, because we don't count references. IndexOpts.SystemSymbolFilter = index::IndexingOptions::SystemSymbolFilterKind::DeclarationsOnly; IndexOpts.IndexFunctionLocals = false; - index::indexTopLevelDecls(AST->getASTContext(), AST->getTopLevelDecls(), - Collector, IndexOpts); + std::vector<const Decl *> TopLevelDecls( + AST.getTranslationUnitDecl()->decls().begin(), + AST.getTranslationUnitDecl()->decls().end()); + index::indexTopLevelDecls(AST, TopLevelDecls, Collector, IndexOpts); + return Collector.takeSymbols(); } @@ -69,12 +72,14 @@ std::shared_ptr<std::vector<const Symbol return {std::move(Snap), Pointers}; } -void FileIndex::update(PathRef Path, ParsedAST *AST) { +void FileIndex::update(PathRef Path, ASTContext *AST, + std::shared_ptr<Preprocessor> PP) { if (!AST) { FSymbols.update(Path, nullptr); } else { + assert(PP); auto Slab = llvm::make_unique<SymbolSlab>(); - *Slab = indexAST(AST); + *Slab = indexAST(*AST, PP); FSymbols.update(Path, std::move(Slab)); } auto Symbols = FSymbols.allSymbols(); Modified: clang-tools-extra/trunk/clangd/index/FileIndex.h URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/FileIndex.h?rev=333196&r1=333195&r2=333196&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/index/FileIndex.h (original) +++ clang-tools-extra/trunk/clangd/index/FileIndex.h Thu May 24 08:50:15 2018 @@ -19,6 +19,7 @@ #include "../ClangdUnit.h" #include "Index.h" #include "MemIndex.h" +#include "clang/Lex/Preprocessor.h" namespace clang { namespace clangd { @@ -56,8 +57,10 @@ private: class FileIndex : public SymbolIndex { public: /// \brief Update symbols in \p Path with symbols in \p AST. If \p AST is - /// nullptr, this removes all symbols in the file - void update(PathRef Path, ParsedAST *AST); + /// nullptr, this removes all symbols in the file. + /// If \p AST is not null, \p PP cannot be null and it should be the + /// preprocessor that was used to build \p AST. + void update(PathRef Path, ASTContext *AST, std::shared_ptr<Preprocessor> PP); bool fuzzyFind(const FuzzyFindRequest &Req, @@ -73,7 +76,7 @@ private: /// Retrieves namespace and class level symbols in \p AST. /// Exposed to assist in unit tests. -SymbolSlab indexAST(ParsedAST *AST); +SymbolSlab indexAST(ASTContext &AST, std::shared_ptr<Preprocessor> PP); } // namespace clangd } // namespace clang Modified: clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp?rev=333196&r1=333195&r2=333196&view=diff ============================================================================== --- clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp (original) +++ clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp Thu May 24 08:50:15 2018 @@ -7,8 +7,13 @@ // //===----------------------------------------------------------------------===// +#include "ClangdUnit.h" +#include "TestFS.h" #include "TestTU.h" #include "index/FileIndex.h" +#include "clang/Frontend/PCHContainerOperations.h" +#include "clang/Lex/Preprocessor.h" +#include "clang/Tooling/CompilationDatabase.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -87,7 +92,7 @@ void update(FileIndex &M, llvm::StringRe File.HeaderFilename = (Basename + ".h").str(); File.HeaderCode = Code; auto AST = File.build(); - M.update(File.Filename, &AST); + M.update(File.Filename, &AST.getASTContext(), AST.getPreprocessorPtr()); } TEST(FileIndexTest, IndexAST) { @@ -129,13 +134,13 @@ TEST(FileIndexTest, RemoveAST) { Req.Scopes = {"ns::"}; EXPECT_THAT(match(M, Req), UnorderedElementsAre("ns::f", "ns::X")); - M.update("f1.cpp", nullptr); + M.update("f1.cpp", nullptr, nullptr); EXPECT_THAT(match(M, Req), UnorderedElementsAre()); } TEST(FileIndexTest, RemoveNonExisting) { FileIndex M; - M.update("no.cpp", nullptr); + M.update("no.cpp", nullptr, nullptr); EXPECT_THAT(match(M, FuzzyFindRequest()), UnorderedElementsAre()); } @@ -200,6 +205,58 @@ vector<Ty> make_vector(Arg A) {} EXPECT_TRUE(SeenMakeVector); } +TEST(FileIndexTest, RebuildWithPreamble) { + auto FooCpp = testPath("foo.cpp"); + auto FooH = testPath("foo.h"); + FileIndex Index; + bool IndexUpdated = false; + CppFile File("foo.cpp", /*StorePreambleInMemory=*/true, + std::make_shared<PCHContainerOperations>(), + [&Index, &IndexUpdated](PathRef FilePath, ASTContext &Ctx, + std::shared_ptr<Preprocessor> PP) { + EXPECT_FALSE(IndexUpdated) + << "Expected only a single index update"; + IndexUpdated = true; + Index.update(FilePath, &Ctx, std::move(PP)); + }); + + // Preparse ParseInputs. + ParseInputs PI; + PI.CompileCommand.Directory = testRoot(); + PI.CompileCommand.Filename = FooCpp; + PI.CompileCommand.CommandLine = {"clang", "-xc++", FooCpp}; + + llvm::StringMap<std::string> Files; + Files[FooCpp] = ""; + Files[FooH] = R"cpp( + namespace ns_in_header { + int func_in_header(); + } + )cpp"; + PI.FS = buildTestFS(std::move(Files)); + + PI.Contents = R"cpp( + #include "foo.h" + namespace ns_in_source { + int func_in_source(); + } + )cpp"; + + // Rebuild the file. + File.rebuild(std::move(PI)); + ASSERT_TRUE(IndexUpdated); + + // Check the index contains symbols from the preamble, but not from the main + // file. + FuzzyFindRequest Req; + Req.Query = ""; + Req.Scopes = {"", "ns_in_header::"}; + + EXPECT_THAT( + match(Index, Req), + UnorderedElementsAre("ns_in_header", "ns_in_header::func_in_header")); +} + } // namespace } // namespace clangd } // namespace clang Modified: clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp?rev=333196&r1=333195&r2=333196&view=diff ============================================================================== --- clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp (original) +++ clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp Thu May 24 08:50:15 2018 @@ -42,7 +42,7 @@ private: TEST_F(TUSchedulerTests, MissingFiles) { TUScheduler S(getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true, - /*ASTParsedCallback=*/nullptr, + /*PreambleParsedCallback=*/nullptr, /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero()); auto Added = testPath("added.cpp"); @@ -98,7 +98,7 @@ TEST_F(TUSchedulerTests, WantDiagnostics TUScheduler S( getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true, - /*ASTParsedCallback=*/nullptr, + /*PreambleParsedCallback=*/nullptr, /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero()); auto Path = testPath("foo.cpp"); S.update(Path, getInputs(Path, ""), WantDiagnostics::Yes, @@ -126,7 +126,7 @@ TEST_F(TUSchedulerTests, Debounce) { { TUScheduler S(getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true, - /*ASTParsedCallback=*/nullptr, + /*PreambleParsedCallback=*/nullptr, /*UpdateDebounce=*/std::chrono::seconds(1)); // FIXME: we could probably use timeouts lower than 1 second here. auto Path = testPath("foo.cpp"); @@ -157,7 +157,7 @@ TEST_F(TUSchedulerTests, ManyUpdates) { { TUScheduler S(getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true, - /*ASTParsedCallback=*/nullptr, + /*PreambleParsedCallback=*/nullptr, /*UpdateDebounce=*/std::chrono::milliseconds(50)); std::vector<std::string> Files; Modified: clang-tools-extra/trunk/unittests/clangd/TestTU.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/TestTU.cpp?rev=333196&r1=333195&r2=333196&view=diff ============================================================================== --- clang-tools-extra/trunk/unittests/clangd/TestTU.cpp (original) +++ clang-tools-extra/trunk/unittests/clangd/TestTU.cpp Thu May 24 08:50:15 2018 @@ -43,7 +43,7 @@ ParsedAST TestTU::build() const { SymbolSlab TestTU::headerSymbols() const { auto AST = build(); - return indexAST(&AST); + return indexAST(AST.getASTContext(), AST.getPreprocessorPtr()); } std::unique_ptr<SymbolIndex> TestTU::index() const { _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
