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
