kuganv updated this revision to Diff 521936. kuganv edited the summary of this revision. kuganv added a comment. Herald added a project: clang.
Re-implemented based on review Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148088/new/ https://reviews.llvm.org/D148088 Files: clang-tools-extra/clangd/Preamble.cpp clang-tools-extra/clangd/Preamble.h clang-tools-extra/clangd/TUScheduler.cpp clang-tools-extra/clangd/tool/Check.cpp clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp clang-tools-extra/clangd/unittests/FileIndexTests.cpp clang-tools-extra/clangd/unittests/ParsedASTTests.cpp clang-tools-extra/clangd/unittests/PreambleTests.cpp clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp clang-tools-extra/clangd/unittests/TestTU.cpp clang-tools-extra/clangd/unittests/TestTU.h clang/include/clang/Frontend/CompilerInstance.h
Index: clang/include/clang/Frontend/CompilerInstance.h =================================================================== --- clang/include/clang/Frontend/CompilerInstance.h +++ clang/include/clang/Frontend/CompilerInstance.h @@ -12,6 +12,7 @@ #include "clang/AST/ASTConsumer.h" #include "clang/Basic/Diagnostic.h" #include "clang/Basic/SourceManager.h" +#include "clang/Basic/TargetInfo.h" #include "clang/Frontend/CompilerInvocation.h" #include "clang/Frontend/PCHContainerOperations.h" #include "clang/Frontend/Utils.h" @@ -233,6 +234,8 @@ return *Invocation; } + std::shared_ptr<CompilerInvocation> getInvocationPtr() { return Invocation; } + /// setInvocation - Replace the current invocation. void setInvocation(std::shared_ptr<CompilerInvocation> Value); @@ -338,6 +341,11 @@ return *Diagnostics; } + IntrusiveRefCntPtr<DiagnosticsEngine> getDiagnosticsPtr() const { + assert(Diagnostics && "Compiler instance has no diagnostics!"); + return Diagnostics; + } + /// setDiagnostics - Replace the current diagnostics engine. void setDiagnostics(DiagnosticsEngine *Value); @@ -373,6 +381,11 @@ return *Target; } + IntrusiveRefCntPtr<TargetInfo> getTargetPtr() const { + assert(Target && "Compiler instance has no target!"); + return Target; + } + /// Replace the current Target. void setTarget(TargetInfo *Value); @@ -406,6 +419,11 @@ return *FileMgr; } + IntrusiveRefCntPtr<FileManager> getFileManagerPtr() const { + assert(FileMgr && "Compiler instance has no file manager!"); + return FileMgr; + } + void resetAndLeakFileManager() { llvm::BuryPointer(FileMgr.get()); FileMgr.resetWithoutRelease(); @@ -426,6 +444,11 @@ return *SourceMgr; } + IntrusiveRefCntPtr<SourceManager> getSourceManagerPtr() const { + assert(SourceMgr && "Compiler instance has no source manager!"); + return SourceMgr; + } + void resetAndLeakSourceManager() { llvm::BuryPointer(SourceMgr.get()); SourceMgr.resetWithoutRelease(); @@ -466,6 +489,11 @@ return *Context; } + IntrusiveRefCntPtr<ASTContext> getASTContextPtr() const { + assert(Context && "Compiler instance has no AST context!"); + return Context; + } + void resetAndLeakASTContext() { llvm::BuryPointer(Context.get()); Context.resetWithoutRelease(); Index: clang-tools-extra/clangd/unittests/TestTU.h =================================================================== --- clang-tools-extra/clangd/unittests/TestTU.h +++ clang-tools-extra/clangd/unittests/TestTU.h @@ -32,6 +32,8 @@ namespace clang { namespace clangd { +using PreambleParsedCallback = std::function<void(ASTContext &, Preprocessor &, + const CanonicalIncludes &)>; struct TestTU { static TestTU withCode(llvm::StringRef Code) { TestTU TU; @@ -89,7 +91,7 @@ // The result will always have getDiagnostics() populated. ParsedAST build() const; std::shared_ptr<const PreambleData> - preamble(PreambleParsedCallback PreambleCallback = nullptr) const; + preamble(PreambleParsedCallback = nullptr) const; ParseInputs inputs(MockFS &FS) const; SymbolSlab headerSymbols() const; RefSlab headerRefs() const; Index: clang-tools-extra/clangd/unittests/TestTU.cpp =================================================================== --- clang-tools-extra/clangd/unittests/TestTU.cpp +++ clang-tools-extra/clangd/unittests/TestTU.cpp @@ -107,8 +107,16 @@ initializeModuleCache(*CI); auto ModuleCacheDeleter = llvm::make_scope_exit( std::bind(deleteModuleCache, CI->getHeaderSearchOpts().ModuleCachePath)); - return clang::clangd::buildPreamble(testPath(Filename), *CI, Inputs, - /*StoreInMemory=*/true, PreambleCallback); + auto res = clang::clangd::buildPreamble(testPath(Filename), *CI, Inputs, + /*StoreInMemory=*/true); + if (PreambleCallback) { + auto &ASTCtx = *res.second; + ASTContext &Ctx = ASTCtx.getASTContext(); + Preprocessor &PP = ASTCtx.getPreprocessor(); + auto &CanonIncludes = res.first->CanonIncludes; + PreambleCallback(Ctx, PP, CanonIncludes); + } + return res.first; } ParsedAST TestTU::build() const { @@ -124,8 +132,8 @@ std::bind(deleteModuleCache, CI->getHeaderSearchOpts().ModuleCachePath)); auto Preamble = clang::clangd::buildPreamble(testPath(Filename), *CI, Inputs, - /*StoreInMemory=*/true, - /*PreambleCallback=*/nullptr); + /*StoreInMemory=*/true) + .first; auto AST = ParsedAST::build(testPath(Filename), Inputs, std::move(CI), Diags.take(), Preamble); if (!AST) { Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp =================================================================== --- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp +++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp @@ -1563,8 +1563,8 @@ CaptureBuiltFilenames(std::vector<std::string> &Filenames) : Filenames(Filenames) {} void onPreambleAST(PathRef Path, llvm::StringRef Version, - const CompilerInvocation &CI, ASTContext &Ctx, - Preprocessor &PP, const CanonicalIncludes &) override { + const CompilerInvocation &, ASTContext &Ctx, + Preprocessor &, const CanonicalIncludes &) override { // Deliberately no synchronization. // The PreambleThrottler should serialize these calls, if not then tsan // will find a bug here. Index: clang-tools-extra/clangd/unittests/PreambleTests.cpp =================================================================== --- clang-tools-extra/clangd/unittests/PreambleTests.cpp +++ clang-tools-extra/clangd/unittests/PreambleTests.cpp @@ -195,8 +195,10 @@ TU.AdditionalFiles["b.h"] = ""; TU.AdditionalFiles["c.h"] = ""; auto PI = TU.inputs(FS); - auto BaselinePreamble = buildPreamble( - TU.Filename, *buildCompilerInvocation(PI, Diags), PI, true, nullptr); + auto BaselinePreamble = + buildPreamble(TU.Filename, *buildCompilerInvocation(PI, Diags), PI, true, + nullptr) + .first; // We drop c.h from modified and add a new header. Since the latter is patched // we should only get a.h in preamble includes. d.h shouldn't be part of the // preamble, as it's coming from a disabled region. Index: clang-tools-extra/clangd/unittests/ParsedASTTests.cpp =================================================================== --- clang-tools-extra/clangd/unittests/ParsedASTTests.cpp +++ clang-tools-extra/clangd/unittests/ParsedASTTests.cpp @@ -496,7 +496,7 @@ auto Inputs = TU.inputs(FS); auto CI = buildCompilerInvocation(Inputs, Diags); auto EmptyPreamble = - buildPreamble(testPath("foo.cpp"), *CI, Inputs, true, nullptr); + buildPreamble(testPath("foo.cpp"), *CI, Inputs, true).first; ASSERT_TRUE(EmptyPreamble); EXPECT_THAT(EmptyPreamble->Includes.MainFileIncludes, IsEmpty()); @@ -539,7 +539,7 @@ auto Inputs = TU.inputs(FS); auto CI = buildCompilerInvocation(Inputs, Diags); auto BaselinePreamble = - buildPreamble(testPath("foo.cpp"), *CI, Inputs, true, nullptr); + buildPreamble(testPath("foo.cpp"), *CI, Inputs, true).first; ASSERT_TRUE(BaselinePreamble); EXPECT_THAT(BaselinePreamble->Includes.MainFileIncludes, ElementsAre(testing::Field(&Inclusion::Written, "<foo.h>"))); Index: clang-tools-extra/clangd/unittests/FileIndexTests.cpp =================================================================== --- clang-tools-extra/clangd/unittests/FileIndexTests.cpp +++ clang-tools-extra/clangd/unittests/FileIndexTests.cpp @@ -305,16 +305,19 @@ FileIndex Index; bool IndexUpdated = false; - buildPreamble(FooCpp, *CI, PI, - /*StoreInMemory=*/true, - [&](ASTContext &Ctx, Preprocessor &PP, - const CanonicalIncludes &CanonIncludes) { - EXPECT_FALSE(IndexUpdated) - << "Expected only a single index update"; - IndexUpdated = true; - Index.updatePreamble(FooCpp, /*Version=*/"null", Ctx, PP, - CanonIncludes); - }); + auto res = buildPreamble(FooCpp, *CI, PI, + /*StoreInMemory=*/true); + auto CanonIncludes = res.first->CanonIncludes; + auto AST = *res.second; + auto index_fn = [&](CapturedASTCtx &CapturedCtx, + const CanonicalIncludes &CanonIncludes) { + EXPECT_FALSE(IndexUpdated) << "Expected only a single index update"; + IndexUpdated = true; + ASTContext &Ctx = CapturedCtx.getASTContext(); + Preprocessor &PP = CapturedCtx.getPreprocessor(); + Index.updatePreamble(FooCpp, /*Version=*/"null", Ctx, PP, CanonIncludes); + }; + index_fn(AST, CanonIncludes); ASSERT_TRUE(IndexUpdated); // Check the index contains symbols from the preamble, but not from the main Index: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp =================================================================== --- clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp +++ clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp @@ -133,7 +133,8 @@ return {}; } auto Preamble = buildPreamble(testPath(TU.Filename), *CI, Inputs, - /*InMemory=*/true, /*Callback=*/nullptr); + /*InMemory=*/true) + .first; return codeComplete(testPath(TU.Filename), Point, Preamble.get(), Inputs, Opts); } @@ -1323,7 +1324,8 @@ return {}; } auto Preamble = buildPreamble(testPath(TU.Filename), *CI, Inputs, - /*InMemory=*/true, /*Callback=*/nullptr); + /*InMemory=*/true) + .first; if (!Preamble) { ADD_FAILURE() << "Couldn't build Preamble"; return {}; @@ -1588,7 +1590,8 @@ auto CI = buildCompilerInvocation(Inputs, Diags); ASSERT_TRUE(CI); auto EmptyPreamble = buildPreamble(testPath(TU.Filename), *CI, Inputs, - /*InMemory=*/true, /*Callback=*/nullptr); + /*InMemory=*/true) + .first; ASSERT_TRUE(EmptyPreamble); TU.AdditionalFiles["a.h"] = "int foo(int x);"; Index: clang-tools-extra/clangd/tool/Check.cpp =================================================================== --- clang-tools-extra/clangd/tool/Check.cpp +++ clang-tools-extra/clangd/tool/Check.cpp @@ -206,15 +206,9 @@ // Build preamble and AST, and index them. bool buildAST() { log("Building preamble..."); - Preamble = buildPreamble(File, *Invocation, Inputs, /*StoreInMemory=*/true, - [&](ASTContext &Ctx, Preprocessor &PP, - const CanonicalIncludes &Includes) { - if (!Opts.BuildDynamicSymbolIndex) - return; - log("Indexing headers..."); - Index.updatePreamble(File, /*Version=*/"null", - Ctx, PP, Includes); - }); + auto BuildResult = + buildPreamble(File, *Invocation, Inputs, /*StoreInMemory=*/true); + Preamble = BuildResult.first; if (!Preamble) { elog("Failed to build preamble"); return false; Index: clang-tools-extra/clangd/TUScheduler.cpp =================================================================== --- clang-tools-extra/clangd/TUScheduler.cpp +++ clang-tools-extra/clangd/TUScheduler.cpp @@ -1049,16 +1049,27 @@ assert(Req.CI && "Got preamble request with null compiler invocation"); const ParseInputs &Inputs = Req.Inputs; bool ReusedPreamble = false; + std::optional<CapturedASTCtx> CapturedCtx; Status.update([&](TUStatus &Status) { Status.PreambleActivity = PreambleAction::Building; }); - auto _ = llvm::make_scope_exit([this, &Req, &ReusedPreamble] { + auto _ = llvm::make_scope_exit([&] { ASTPeer.updatePreamble(std::move(Req.CI), std::move(Req.Inputs), LatestBuild, std::move(Req.CIDiags), std::move(Req.WantDiags)); - if (!ReusedPreamble) + if (!ReusedPreamble) { Callbacks.onPreamblePublished(FileName); + if (LatestBuild) { + assert(CapturedCtx); + CanonicalIncludes CanonIncludes = LatestBuild->CanonIncludes; + CompilerInvocation &CI = CapturedCtx->getCompilerInvocation(); + ASTContext &Ctx = CapturedCtx->getASTContext(); + Preprocessor &PP = CapturedCtx->getPreprocessor(); + Callbacks.onPreambleAST(FileName, Inputs.Version, CI, Ctx, PP, + CanonIncludes); + } + } }); if (!LatestBuild || Inputs.ForceRebuild) { @@ -1082,17 +1093,14 @@ PreambleBuildStats Stats; bool IsFirstPreamble = !LatestBuild; - LatestBuild = clang::clangd::buildPreamble( - FileName, *Req.CI, Inputs, StoreInMemory, - [&](ASTContext &Ctx, Preprocessor &PP, - const CanonicalIncludes &CanonIncludes) { - Callbacks.onPreambleAST(FileName, Inputs.Version, *Req.CI, Ctx, PP, - CanonIncludes); - }, - &Stats); - if (!LatestBuild) + auto BuildResult = clang::clangd::buildPreamble(FileName, *Req.CI, Inputs, + StoreInMemory, &Stats); + LatestBuild = BuildResult.first; + if (!LatestBuild) { return; + } reportPreambleBuild(Stats, IsFirstPreamble); + CapturedCtx = BuildResult.second; if (isReliable(LatestBuild->CompileCommand)) HeaderIncluders.update(FileName, LatestBuild->Includes.allHeaders()); } Index: clang-tools-extra/clangd/Preamble.h =================================================================== --- clang-tools-extra/clangd/Preamble.h +++ clang-tools-extra/clangd/Preamble.h @@ -44,6 +44,30 @@ namespace clang { namespace clangd { +struct CapturedASTCtx { +public: + CapturedASTCtx(CompilerInstance &Clang) + : Invocation(Clang.getInvocationPtr()), + Diagnostics(Clang.getDiagnosticsPtr()), Target(Clang.getTargetPtr()), + AuxTarget(Clang.getAuxTarget()), FileMgr(Clang.getFileManagerPtr()), + SourceMgr(Clang.getSourceManagerPtr()), PP(Clang.getPreprocessorPtr()), + Context(Clang.getASTContextPtr()) {} + + ASTContext &getASTContext() { return *Context; } + Preprocessor &getPreprocessor() { return *PP; } + CompilerInvocation &getCompilerInvocation() { return *Invocation; } + +private: + std::shared_ptr<CompilerInvocation> Invocation; + IntrusiveRefCntPtr<DiagnosticsEngine> Diagnostics; + IntrusiveRefCntPtr<TargetInfo> Target; + IntrusiveRefCntPtr<TargetInfo> AuxTarget; + IntrusiveRefCntPtr<FileManager> FileMgr; + IntrusiveRefCntPtr<SourceManager> SourceMgr; + std::shared_ptr<Preprocessor> PP; + IntrusiveRefCntPtr<ASTContext> Context; +}; + /// The parsed preamble and associated data. /// /// As we must avoid re-parsing the preamble, any information that can only @@ -76,9 +100,6 @@ bool MainIsIncludeGuarded = false; }; -using PreambleParsedCallback = std::function<void(ASTContext &, Preprocessor &, - const CanonicalIncludes &)>; - /// Timings and statistics from the premble build. Unlike PreambleData, these /// do not need to be stored for later, but can be useful for logging, metrics, /// etc. @@ -101,10 +122,9 @@ /// If \p PreambleCallback is set, it will be run on top of the AST while /// building the preamble. /// If Stats is not non-null, build statistics will be exported there. -std::shared_ptr<const PreambleData> +std::pair<std::shared_ptr<const PreambleData>, std::optional<CapturedASTCtx>> buildPreamble(PathRef FileName, CompilerInvocation CI, const ParseInputs &Inputs, bool StoreInMemory, - PreambleParsedCallback PreambleCallback, PreambleBuildStats *Stats = nullptr); /// Returns true if \p Preamble is reusable for \p Inputs. Note that it will Index: clang-tools-extra/clangd/Preamble.cpp =================================================================== --- clang-tools-extra/clangd/Preamble.cpp +++ clang-tools-extra/clangd/Preamble.cpp @@ -26,7 +26,9 @@ #include "clang/Basic/LangOptions.h" #include "clang/Basic/SourceLocation.h" #include "clang/Basic/SourceManager.h" +#include "clang/Basic/TargetInfo.h" #include "clang/Basic/TokenKinds.h" +#include "clang/Frontend/CompilerInstance.h" #include "clang/Frontend/CompilerInvocation.h" #include "clang/Frontend/FrontendActions.h" #include "clang/Frontend/PrecompiledPreamble.h" @@ -35,6 +37,8 @@ #include "clang/Lex/PPCallbacks.h" #include "clang/Lex/Preprocessor.h" #include "clang/Lex/PreprocessorOptions.h" +#include "clang/Sema/Sema.h" +#include "clang/Serialization/ASTReader.h" #include "clang/Tooling/CompilationDatabase.h" #include "llvm/ADT/ArrayRef.h" #include "llvm/ADT/DenseMap.h" @@ -77,10 +81,9 @@ class CppFilePreambleCallbacks : public PreambleCallbacks { public: CppFilePreambleCallbacks( - PathRef File, PreambleParsedCallback ParsedCallback, - PreambleBuildStats *Stats, bool ParseForwardingFunctions, + PathRef File, PreambleBuildStats *Stats, bool ParseForwardingFunctions, std::function<void(CompilerInstance &)> BeforeExecuteCallback) - : File(File), ParsedCallback(ParsedCallback), Stats(Stats), + : File(File), Stats(Stats), ParseForwardingFunctions(ParseForwardingFunctions), BeforeExecuteCallback(std::move(BeforeExecuteCallback)) {} @@ -95,13 +98,18 @@ } CanonicalIncludes takeCanonicalIncludes() { return std::move(CanonIncludes); } + CapturedASTCtx takeLife() { return std::move(*CapturedCtx); } + bool isMainFileIncludeGuarded() const { return IsMainFileIncludeGuarded; } void AfterExecute(CompilerInstance &CI) override { - if (ParsedCallback) { - trace::Span Tracer("Running PreambleCallback"); - ParsedCallback(CI.getASTContext(), CI.getPreprocessor(), CanonIncludes); - } + CI.setSema(nullptr); + CI.setASTConsumer(nullptr); + CI.getASTReader()->setDeserializationListener(nullptr); + /* This just sets consumer to null when DeserializationListener is null */ + CI.getASTReader()->StartTranslationUnit(nullptr); + CI.getASTContext().setASTMutationListener(nullptr); + CapturedCtx.emplace(CI); const SourceManager &SM = CI.getSourceManager(); const FileEntry *MainFE = SM.getFileEntryForID(SM.getMainFileID()); @@ -202,7 +210,6 @@ private: PathRef File; - PreambleParsedCallback ParsedCallback; IncludeStructure Includes; CanonicalIncludes CanonIncludes; include_cleaner::PragmaIncludes Pragmas; @@ -216,6 +223,7 @@ PreambleBuildStats *Stats; bool ParseForwardingFunctions; std::function<void(CompilerInstance &)> BeforeExecuteCallback; + std::optional<CapturedASTCtx> CapturedCtx; }; // Represents directives other than includes, where basic textual information is @@ -577,16 +585,16 @@ }; } // namespace -std::shared_ptr<const PreambleData> +std::pair<std::shared_ptr<const PreambleData>, std::optional<CapturedASTCtx>> buildPreamble(PathRef FileName, CompilerInvocation CI, const ParseInputs &Inputs, bool StoreInMemory, - PreambleParsedCallback PreambleCallback, PreambleBuildStats *Stats) { // Note that we don't need to copy the input contents, preamble can live // without those. auto ContentsBuffer = llvm::MemoryBuffer::getMemBuffer(Inputs.Contents, FileName); auto Bounds = ComputePreambleBounds(*CI.getLangOpts(), *ContentsBuffer, 0); + std::optional<CapturedASTCtx> CapturedCtx; trace::Span Tracer("BuildPreamble"); SPAN_ATTACH(Tracer, "File", FileName); @@ -635,8 +643,7 @@ CI.getPreprocessorOpts().WriteCommentListToPCH = false; CppFilePreambleCallbacks CapturedInfo( - FileName, PreambleCallback, Stats, - Inputs.Opts.PreambleParseForwardingFunctions, + FileName, Stats, Inputs.Opts.PreambleParseForwardingFunctions, [&ASTListeners](CompilerInstance &CI) { for (const auto &L : ASTListeners) L->beforeExecute(CI); @@ -682,7 +689,8 @@ Result->CanonIncludes = CapturedInfo.takeCanonicalIncludes(); Result->StatCache = std::move(StatCache); Result->MainIsIncludeGuarded = CapturedInfo.isMainFileIncludeGuarded(); - return Result; + CapturedCtx.emplace(CapturedInfo.takeLife()); + return std::make_pair(Result, CapturedCtx); } elog("Could not build a preamble for file {0} version {1}: {2}", FileName, @@ -693,7 +701,8 @@ // Not an ideal way to show errors, but better than nothing! elog(" error: {0}", D.Message); } - return nullptr; + return std::make_pair<std::shared_ptr<const PreambleData>, + std::optional<CapturedASTCtx>>(nullptr, {}); } bool isPreambleCompatible(const PreambleData &Preamble,
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits