kuganv updated this revision to Diff 524836.
kuganv marked an inline comment as done.
kuganv added a comment.
Set just PrecompilePreambleConsumer/PCHGenerator and ASTMutationListener to
null.
Set the FileManager VFS to consuming FS so that it is thread safe.
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/ClangdServer.cpp
clang-tools-extra/clangd/ParsedAST.cpp
clang-tools-extra/clangd/Preamble.cpp
clang-tools-extra/clangd/Preamble.h
clang-tools-extra/clangd/TUScheduler.cpp
clang-tools-extra/clangd/TUScheduler.h
clang-tools-extra/clangd/tool/Check.cpp
clang-tools-extra/clangd/unittests/FileIndexTests.cpp
clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
clang-tools-extra/clangd/unittests/TestWorkspace.cpp
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/TestWorkspace.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/TestWorkspace.cpp
+++ clang-tools-extra/clangd/unittests/TestWorkspace.cpp
@@ -21,11 +21,14 @@
continue;
TU.Code = Input.second.Code;
TU.Filename = Input.first().str();
- TU.preamble([&](ASTContext &Ctx, Preprocessor &PP,
- const CanonicalIncludes &CanonIncludes) {
- Index->updatePreamble(testPath(Input.first()), "null", Ctx, PP,
- CanonIncludes);
- });
+ TU.preamble(
+ [&](CapturedASTCtx ASTCtx,
+ const std::shared_ptr<const CanonicalIncludes> CanonIncludes) {
+ auto &Ctx = ASTCtx.getASTContext();
+ auto &PP = ASTCtx.getPreprocessor();
+ Index->updatePreamble(testPath(Input.first()), "null", Ctx, PP,
+ *CanonIncludes);
+ });
ParsedAST MainAST = TU.build();
Index->updateMain(testPath(Input.first()), MainAST);
}
Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -1129,9 +1129,9 @@
public:
BlockPreambleThread(llvm::StringRef BlockVersion, Notification &N)
: BlockVersion(BlockVersion), N(N) {}
- void onPreambleAST(PathRef Path, llvm::StringRef Version,
- const CompilerInvocation &, ASTContext &Ctx,
- Preprocessor &, const CanonicalIncludes &) override {
+ void
+ onPreambleAST(PathRef Path, llvm::StringRef Version, CapturedASTCtx,
+ const std::shared_ptr<const CanonicalIncludes>) override {
if (Version == BlockVersion)
N.wait();
}
@@ -1208,9 +1208,9 @@
BlockPreambleThread(Notification &UnblockPreamble, DiagsCB CB)
: UnblockPreamble(UnblockPreamble), CB(std::move(CB)) {}
- void onPreambleAST(PathRef Path, llvm::StringRef Version,
- const CompilerInvocation &, ASTContext &Ctx,
- Preprocessor &, const CanonicalIncludes &) override {
+ void
+ onPreambleAST(PathRef Path, llvm::StringRef Version, CapturedASTCtx,
+ const std::shared_ptr<const CanonicalIncludes>) override {
if (BuildBefore)
ASSERT_TRUE(UnblockPreamble.wait(timeoutSeconds(5)))
<< "Expected notification";
@@ -1562,9 +1562,9 @@
std::vector<std::string> &Filenames;
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 {
+ void
+ onPreambleAST(PathRef Path, llvm::StringRef Version, CapturedASTCtx,
+ const std::shared_ptr<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/FileIndexTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/FileIndexTests.cpp
+++ clang-tools-extra/clangd/unittests/FileIndexTests.cpp
@@ -305,16 +305,18 @@
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);
- });
+ buildPreamble(
+ FooCpp, *CI, PI,
+ /*StoreInMemory=*/true,
+ [&](CapturedASTCtx ASTCtx,
+ const std::shared_ptr<const CanonicalIncludes> CanonIncludes) {
+ auto &Ctx = ASTCtx.getASTContext();
+ auto &PP = ASTCtx.getPreprocessor();
+ EXPECT_FALSE(IndexUpdated) << "Expected only a single index update";
+ IndexUpdated = true;
+ Index.updatePreamble(FooCpp, /*Version=*/"null", Ctx, PP,
+ *CanonIncludes);
+ });
ASSERT_TRUE(IndexUpdated);
// Check the index contains symbols from the preamble, but not from the main
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,16 @@
// 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);
- });
+ Preamble = buildPreamble(
+ File, *Invocation, Inputs, /*StoreInMemory=*/true,
+ [&](CapturedASTCtx Ctx,
+ const std::shared_ptr<const CanonicalIncludes> Includes) {
+ if (!Opts.BuildDynamicSymbolIndex)
+ return;
+ log("Indexing headers...");
+ Index.updatePreamble(File, /*Version=*/"null", Ctx.getASTContext(),
+ Ctx.getPreprocessor(), *Includes);
+ });
if (!Preamble) {
elog("Failed to build preamble");
return false;
Index: clang-tools-extra/clangd/TUScheduler.h
===================================================================
--- clang-tools-extra/clangd/TUScheduler.h
+++ clang-tools-extra/clangd/TUScheduler.h
@@ -162,8 +162,8 @@
/// contains only AST nodes from the #include directives at the start of the
/// file. AST node in the current file should be observed on onMainAST call.
virtual void onPreambleAST(PathRef Path, llvm::StringRef Version,
- const CompilerInvocation &CI, ASTContext &Ctx,
- Preprocessor &PP, const CanonicalIncludes &) {}
+ CapturedASTCtx Ctx,
+ const std::shared_ptr<const CanonicalIncludes>) {}
/// The argument function is run under the critical section guarding against
/// races when closing the files.
Index: clang-tools-extra/clangd/TUScheduler.cpp
===================================================================
--- clang-tools-extra/clangd/TUScheduler.cpp
+++ clang-tools-extra/clangd/TUScheduler.cpp
@@ -1084,9 +1084,9 @@
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,
+ [&](CapturedASTCtx ASTCtx,
+ std::shared_ptr<const CanonicalIncludes> CanonIncludes) {
+ Callbacks.onPreambleAST(FileName, Inputs.Version, std::move(ASTCtx),
CanonIncludes);
},
&Stats);
Index: clang-tools-extra/clangd/Preamble.h
===================================================================
--- clang-tools-extra/clangd/Preamble.h
+++ clang-tools-extra/clangd/Preamble.h
@@ -44,6 +44,46 @@
namespace clang {
namespace clangd {
+/// The captured AST conext.
+/// Keeps necessary structs for an ASTContext and Preprocessor alive.
+/// This enables consuming them after context that produced the AST is gone.
+/// (e.g. indexing a preamble ast on a separate thread). ASTContext stored
+/// inside is still not thread-safe.
+
+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()) {}
+
+ CapturedASTCtx(const CapturedASTCtx &) = delete;
+ CapturedASTCtx &operator=(const CapturedASTCtx &) = delete;
+ CapturedASTCtx(CapturedASTCtx &&) = default;
+ CapturedASTCtx &operator=(CapturedASTCtx &&) = default;
+
+ ASTContext &getASTContext() { return *Context; }
+ Preprocessor &getPreprocessor() { return *PP; }
+ CompilerInvocation &getCompilerInvocation() { return *Invocation; }
+ FileManager &getFileManager() { return *FileMgr; }
+ void setStatCache(std::shared_ptr<PreambleFileStatusCache> StatCache) {
+ this->StatCache = StatCache;
+ }
+
+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;
+ std::shared_ptr<PreambleFileStatusCache> StatCache;
+};
+
/// The parsed preamble and associated data.
///
/// As we must avoid re-parsing the preamble, any information that can only
@@ -69,15 +109,16 @@
std::vector<PragmaMark> Marks;
// Cache of FS operations performed when building the preamble.
// When reusing a preamble, this cache can be consumed to save IO.
- std::unique_ptr<PreambleFileStatusCache> StatCache;
- CanonicalIncludes CanonIncludes;
+ std::shared_ptr<PreambleFileStatusCache> StatCache;
+ std::shared_ptr<const CanonicalIncludes> CanonIncludes;
// Whether there was a (possibly-incomplete) include-guard on the main file.
// We need to propagate this information "by hand" to subsequent parses.
bool MainIsIncludeGuarded = false;
};
-using PreambleParsedCallback = std::function<void(ASTContext &, Preprocessor &,
- const CanonicalIncludes &)>;
+using PreambleParsedCallback =
+ std::function<void(CapturedASTCtx ASTCtx,
+ std::shared_ptr<const CanonicalIncludes> CanonIncludes)>;
/// 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,
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,7 @@
#include "clang/Lex/PPCallbacks.h"
#include "clang/Lex/Preprocessor.h"
#include "clang/Lex/PreprocessorOptions.h"
+#include "clang/Serialization/ASTReader.h"
#include "clang/Tooling/CompilationDatabase.h"
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/DenseMap.h"
@@ -77,10 +80,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 +97,19 @@
}
CanonicalIncludes takeCanonicalIncludes() { return std::move(CanonIncludes); }
+ std::optional<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);
+ // Set PrecompilePreambleConsumer/PCHGenerator to null.
+ if (CI.getASTReader()) {
+ 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
@@ -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);
@@ -644,7 +651,7 @@
auto VFS = Inputs.TFS->view(Inputs.CompileCommand.Directory);
llvm::SmallString<32> AbsFileName(FileName);
VFS->makeAbsolute(AbsFileName);
- auto StatCache = std::make_unique<PreambleFileStatusCache>(AbsFileName);
+ auto StatCache = std::make_shared<PreambleFileStatusCache>(AbsFileName);
auto StatCacheFS = StatCache->getProducingFS(VFS);
llvm::IntrusiveRefCntPtr<TimerFS> TimedFS(new TimerFS(StatCacheFS));
@@ -679,9 +686,21 @@
Result->Pragmas = CapturedInfo.takePragmaIncludes();
Result->Macros = CapturedInfo.takeMacros();
Result->Marks = CapturedInfo.takeMarks();
- Result->CanonIncludes = CapturedInfo.takeCanonicalIncludes();
- Result->StatCache = std::move(StatCache);
+ Result->CanonIncludes = std::make_shared<const CanonicalIncludes>(
+ (CapturedInfo.takeCanonicalIncludes()));
+ Result->StatCache = StatCache;
Result->MainIsIncludeGuarded = CapturedInfo.isMainFileIncludeGuarded();
+ if (PreambleCallback) {
+ trace::Span Tracer("Running PreambleCallback");
+ auto Ctx = CapturedInfo.takeLife();
+ // Set the FileManager VFS to consuming FS.
+ auto StatCacheFS = Result->StatCache->getConsumingFS(VFS);
+ Ctx->getFileManager().setVirtualFileSystem(StatCacheFS);
+ // While extending the life of FileMgr and VFS, StatCache should also be
+ // extended.
+ Ctx->setStatCache(Result->StatCache);
+ PreambleCallback(std::move(*Ctx), Result->CanonIncludes);
+ }
return Result;
}
Index: clang-tools-extra/clangd/ParsedAST.cpp
===================================================================
--- clang-tools-extra/clangd/ParsedAST.cpp
+++ clang-tools-extra/clangd/ParsedAST.cpp
@@ -622,7 +622,7 @@
// non-preamble includes below.
CanonicalIncludes CanonIncludes;
if (Preamble)
- CanonIncludes = Preamble->CanonIncludes;
+ CanonIncludes = *Preamble->CanonIncludes;
else
CanonIncludes.addSystemHeadersMapping(Clang->getLangOpts());
std::unique_ptr<CommentHandler> IWYUHandler =
Index: clang-tools-extra/clangd/ClangdServer.cpp
===================================================================
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -65,17 +65,30 @@
: FIndex(FIndex), ServerCallbacks(ServerCallbacks), TFS(TFS),
Stdlib{std::make_shared<StdLibSet>()}, Tasks(Tasks) {}
- void onPreambleAST(PathRef Path, llvm::StringRef Version,
- const CompilerInvocation &CI, ASTContext &Ctx,
- Preprocessor &PP,
- const CanonicalIncludes &CanonIncludes) override {
- // If this preamble uses a standard library we haven't seen yet, index it.
- if (FIndex)
- if (auto Loc = Stdlib->add(*CI.getLangOpts(), PP.getHeaderSearchInfo()))
- indexStdlib(CI, std::move(*Loc));
+ void onPreambleAST(
+ PathRef Path, llvm::StringRef Version, CapturedASTCtx ASTCtx,
+ const std::shared_ptr<const CanonicalIncludes> CanonIncludes) override {
+
+ if (!FIndex)
+ return;
+
+ auto &PP = ASTCtx.getPreprocessor();
+ auto &CI = ASTCtx.getCompilerInvocation();
+ if (auto Loc = Stdlib->add(*CI.getLangOpts(), PP.getHeaderSearchInfo()))
+ indexStdlib(CI, std::move(*Loc));
+
+ auto Task = [FIndex(FIndex), Path(Path.str()), Version(Version.str()),
+ ASTCtx(std::move(ASTCtx)),
+ CanonIncludes(CanonIncludes)]() mutable {
+ FIndex->updatePreamble(Path, Version, ASTCtx.getASTContext(),
+ ASTCtx.getPreprocessor(), *CanonIncludes);
+ };
- if (FIndex)
- FIndex->updatePreamble(Path, Version, Ctx, PP, CanonIncludes);
+ if (Tasks) {
+ Tasks->runAsync("Preamble indexing for:" + Path + Version,
+ std::move(Task));
+ } else
+ Task();
}
void indexStdlib(const CompilerInvocation &CI, StdLibLocation Loc) {
@@ -195,6 +208,7 @@
DirtyFS(std::make_unique<DraftStoreFS>(TFS, DraftMgr)) {
if (Opts.AsyncThreadsCount != 0)
IndexTasks.emplace();
+
// Pass a callback into `WorkScheduler` to extract symbols from a newly
// parsed file and rebuild the file index synchronously each time an AST
// is parsed.
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
