kuganv updated this revision to Diff 522471.
kuganv added a comment.
As per review, moved Preamble indexing into ClangdServer's IndexTasks.
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/ClangdServer.h
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,10 +21,12 @@
continue;
TU.Code = Input.second.Code;
TU.Filename = Input.first().str();
- TU.preamble([&](ASTContext &Ctx, Preprocessor &PP,
- const CanonicalIncludes &CanonIncludes) {
+ TU.preamble([&](CapturedASTCtx ASTCtx,
+ const std::shared_ptr<CanonicalIncludes> CanonIncludes) {
+ auto &Ctx = ASTCtx.getASTContext();
+ auto &PP = ASTCtx.getPreprocessor();
Index->updatePreamble(testPath(Input.first()), "null", Ctx, PP,
- CanonIncludes);
+ *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,8 @@
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<CanonicalIncludes>) override {
if (Version == BlockVersion)
N.wait();
}
@@ -1208,9 +1207,8 @@
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<CanonicalIncludes>) override {
if (BuildBefore)
ASSERT_TRUE(UnblockPreamble.wait(timeoutSeconds(5)))
<< "Expected notification";
@@ -1562,9 +1560,8 @@
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<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
@@ -307,13 +307,15 @@
bool IndexUpdated = false;
buildPreamble(FooCpp, *CI, PI,
/*StoreInMemory=*/true,
- [&](ASTContext &Ctx, Preprocessor &PP,
- const CanonicalIncludes &CanonIncludes) {
+ [&](CapturedASTCtx ASTCtx,
+ const std::shared_ptr<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);
+ *CanonIncludes);
});
ASSERT_TRUE(IndexUpdated);
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<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<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<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,38 @@
namespace clang {
namespace clangd {
+/// The captured AST conext.
+///
+/// As we index the preamble in a different thread, we extend the life of
+/// associated data by capturing and storing references here.
+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; }
+ 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 +101,15 @@
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<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<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,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,20 @@
}
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);
+ 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 +212,6 @@
private:
PathRef File;
- PreambleParsedCallback ParsedCallback;
IncludeStructure Includes;
CanonicalIncludes CanonIncludes;
include_cleaner::PragmaIncludes Pragmas;
@@ -216,6 +225,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 +645,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 +653,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 +688,18 @@
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<CanonicalIncludes>(
+ CapturedInfo.takeCanonicalIncludes());
+ Result->StatCache = StatCache;
Result->MainIsIncludeGuarded = CapturedInfo.isMainFileIncludeGuarded();
+ if (PreambleCallback) {
+ trace::Span Tracer("Running PreambleCallback");
+ auto Ctx = CapturedInfo.takeLife();
+ // 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.h
===================================================================
--- clang-tools-extra/clangd/ClangdServer.h
+++ clang-tools-extra/clangd/ClangdServer.h
@@ -447,6 +447,7 @@
std::optional<std::string> WorkspaceRoot;
std::optional<AsyncTaskRunner> IndexTasks; // for stdlib indexing.
+ std::optional<Semaphore> Barrier;
std::optional<TUScheduler> WorkScheduler;
// Invalidation policy used for actions that we assume are "transient".
TUScheduler::ASTActionInvalidation Transient;
Index: clang-tools-extra/clangd/ClangdServer.cpp
===================================================================
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -95,10 +95,10 @@
if (Stdlib->isBest(LO))
FIndex->updatePreamble(std::move(IF));
};
- if (Tasks)
- // This doesn't have a semaphore to enforce -j, but it's rare.
+ if (Tasks) {
+ std::unique_lock<Semaphore> Lock(*Barrier, std::try_to_lock);
Tasks->runAsync("IndexStdlib", std::move(Task));
- else
+ } else
Task();
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
