[clang] [clang][frontend] Make `CompilerInstance::FailedModules` thread-safe (PR #135473)
via cfe-commits
cfe-commits at lists.llvm.org
Fri Apr 11 20:56:56 PDT 2025
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang
Author: Jan Svoboda (jansvoboda11)
<details>
<summary>Changes</summary>
This PR makes some progress towards making it possible to create clones of `CompilerInstance` that are independent of each other and can be used in a multi-threaded setting. This PR tackles `CompilerInstance::FailedModules`, makes it a value-type instead of a mutable shared pointer, and adds explicit copies & moves where appropriate.
Besides that change, this PR also turns two previously free functions with internal linkage into member functions of `CompilerInstance`, which makes it possible to reduce the public API of that class that relates to `FailedModules`. This reduces some API churn that was necessary for each new member of `CompilerInstance` that needs to be cloned.
---
Full diff: https://github.com/llvm/llvm-project/pull/135473.diff
2 Files Affected:
- (modified) clang/include/clang/Frontend/CompilerInstance.h (+17-30)
- (modified) clang/lib/Frontend/CompilerInstance.cpp (+46-64)
``````````diff
diff --git a/clang/include/clang/Frontend/CompilerInstance.h b/clang/include/clang/Frontend/CompilerInstance.h
index 66f56932c51a3..41dc7f1fef461 100644
--- a/clang/include/clang/Frontend/CompilerInstance.h
+++ b/clang/include/clang/Frontend/CompilerInstance.h
@@ -134,23 +134,13 @@ class CompilerInstance : public ModuleLoader {
std::vector<std::shared_ptr<DependencyCollector>> DependencyCollectors;
- /// Records the set of modules
- class FailedModulesSet {
- llvm::StringSet<> Failed;
-
- public:
- bool hasAlreadyFailed(StringRef module) { return Failed.count(module) > 0; }
-
- void addFailed(StringRef module) { Failed.insert(module); }
- };
-
/// The set of modules that failed to build.
///
- /// This pointer will be shared among all of the compiler instances created
+ /// This value will be passed among all of the compiler instances created
/// to (re)build modules, so that once a module fails to build anywhere,
/// other instances will see that the module has failed and won't try to
/// build it again.
- std::shared_ptr<FailedModulesSet> FailedModules;
+ llvm::StringSet<> FailedModules;
/// The set of top-level modules that has already been built on the
/// fly as part of this overall compilation action.
@@ -637,24 +627,6 @@ class CompilerInstance : public ModuleLoader {
return *FrontendTimer;
}
- /// @}
- /// @name Failed modules set
- /// @{
-
- bool hasFailedModulesSet() const { return (bool)FailedModules; }
-
- void createFailedModulesSet() {
- FailedModules = std::make_shared<FailedModulesSet>();
- }
-
- std::shared_ptr<FailedModulesSet> getFailedModulesSetPtr() const {
- return FailedModules;
- }
-
- void setFailedModulesSet(std::shared_ptr<FailedModulesSet> FMS) {
- FailedModules = FMS;
- }
-
/// }
/// @name Output Files
/// @{
@@ -870,6 +842,13 @@ class CompilerInstance : public ModuleLoader {
SourceLocation ModuleNameLoc,
bool IsInclusionDirective);
+ /// Creates a \c CompilerInstance for compiling a module.
+ ///
+ /// This expects a properly initialized \c FrontendInputFile.
+ std::unique_ptr<CompilerInstance> cloneForModuleCompileImpl(
+ SourceLocation ImportLoc, StringRef ModuleName, FrontendInputFile Input,
+ StringRef OriginalModuleMapFile, StringRef ModuleFileName);
+
public:
/// Creates a new \c CompilerInstance for compiling a module.
///
@@ -879,6 +858,14 @@ class CompilerInstance : public ModuleLoader {
cloneForModuleCompile(SourceLocation ImportLoc, Module *Module,
StringRef ModuleFileName);
+ /// Compile a module file for the given module, using the options
+ /// provided by the importing compiler instance. Returns true if the module
+ /// was built without errors.
+ // FIXME: This should be private, but it's called from static non-member
+ // functions in the implementation file.
+ bool compileModule(SourceLocation ImportLoc, StringRef ModuleName,
+ StringRef ModuleFileName, CompilerInstance &Instance);
+
ModuleLoadResult loadModule(SourceLocation ImportLoc, ModuleIdPath Path,
Module::NameVisibilityKind Visibility,
bool IsInclusionDirective) override;
diff --git a/clang/lib/Frontend/CompilerInstance.cpp b/clang/lib/Frontend/CompilerInstance.cpp
index eb138de9d20cc..7ff711df171fa 100644
--- a/clang/lib/Frontend/CompilerInstance.cpp
+++ b/clang/lib/Frontend/CompilerInstance.cpp
@@ -1150,19 +1150,11 @@ static Language getLanguageFromOptions(const LangOptions &LangOpts) {
return LangOpts.CPlusPlus ? Language::CXX : Language::C;
}
-/// Creates a \c CompilerInstance for compiling a module.
-///
-/// This expects a properly initialized \c FrontendInputFile.
-static std::unique_ptr<CompilerInstance>
-createCompilerInstanceForModuleCompileImpl(CompilerInstance &ImportingInstance,
- SourceLocation ImportLoc,
- StringRef ModuleName,
- FrontendInputFile Input,
- StringRef OriginalModuleMapFile,
- StringRef ModuleFileName) {
+std::unique_ptr<CompilerInstance> CompilerInstance::cloneForModuleCompileImpl(
+ SourceLocation ImportLoc, StringRef ModuleName, FrontendInputFile Input,
+ StringRef OriginalModuleMapFile, StringRef ModuleFileName) {
// Construct a compiler invocation for creating this module.
- auto Invocation =
- std::make_shared<CompilerInvocation>(ImportingInstance.getInvocation());
+ auto Invocation = std::make_shared<CompilerInvocation>(getInvocation());
PreprocessorOptions &PPOpts = Invocation->getPreprocessorOpts();
@@ -1182,7 +1174,7 @@ createCompilerInstanceForModuleCompileImpl(CompilerInstance &ImportingInstance,
// If the original compiler invocation had -fmodule-name, pass it through.
Invocation->getLangOpts().ModuleName =
- ImportingInstance.getInvocation().getLangOpts().ModuleName;
+ getInvocation().getLangOpts().ModuleName;
// Note the name of the module we're building.
Invocation->getLangOpts().CurrentModule = std::string(ModuleName);
@@ -1206,7 +1198,7 @@ createCompilerInstanceForModuleCompileImpl(CompilerInstance &ImportingInstance,
DiagnosticOptions &DiagOpts = Invocation->getDiagnosticOpts();
DiagOpts.VerifyDiagnostics = 0;
- assert(ImportingInstance.getInvocation().getModuleHash() ==
+ assert(getInvocation().getModuleHash() ==
Invocation->getModuleHash() && "Module hash mismatch!");
// Construct a compiler instance that will be used to actually create the
@@ -1214,25 +1206,25 @@ createCompilerInstanceForModuleCompileImpl(CompilerInstance &ImportingInstance,
// CompilerInstance::CompilerInstance is responsible for finalizing the
// buffers to prevent use-after-frees.
auto InstancePtr = std::make_unique<CompilerInstance>(
- ImportingInstance.getPCHContainerOperations(),
- &ImportingInstance.getModuleCache());
+ getPCHContainerOperations(),
+ &getModuleCache());
auto &Instance = *InstancePtr;
auto &Inv = *Invocation;
Instance.setInvocation(std::move(Invocation));
Instance.createDiagnostics(
- ImportingInstance.getVirtualFileSystem(),
- new ForwardingDiagnosticConsumer(ImportingInstance.getDiagnosticClient()),
+ getVirtualFileSystem(),
+ new ForwardingDiagnosticConsumer(getDiagnosticClient()),
/*ShouldOwnClient=*/true);
if (llvm::is_contained(DiagOpts.SystemHeaderWarningsModules, ModuleName))
Instance.getDiagnostics().setSuppressSystemWarnings(false);
if (FrontendOpts.ModulesShareFileManager) {
- Instance.setFileManager(&ImportingInstance.getFileManager());
+ Instance.setFileManager(&getFileManager());
} else {
- Instance.createFileManager(&ImportingInstance.getVirtualFileSystem());
+ Instance.createFileManager(&getVirtualFileSystem());
}
Instance.createSourceManager(Instance.getFileManager());
SourceManager &SourceMgr = Instance.getSourceManager();
@@ -1240,48 +1232,38 @@ createCompilerInstanceForModuleCompileImpl(CompilerInstance &ImportingInstance,
// Note that this module is part of the module build stack, so that we
// can detect cycles in the module graph.
SourceMgr.setModuleBuildStack(
- ImportingInstance.getSourceManager().getModuleBuildStack());
+ getSourceManager().getModuleBuildStack());
SourceMgr.pushModuleBuildStack(ModuleName,
- FullSourceLoc(ImportLoc, ImportingInstance.getSourceManager()));
+ FullSourceLoc(ImportLoc, getSourceManager()));
- // Make sure that the failed-module structure has been allocated in
- // the importing instance, and propagate the pointer to the newly-created
- // instance.
- if (!ImportingInstance.hasFailedModulesSet())
- ImportingInstance.createFailedModulesSet();
- Instance.setFailedModulesSet(ImportingInstance.getFailedModulesSetPtr());
+ // Make a copy for the new instance.
+ Instance.FailedModules = FailedModules;
// If we're collecting module dependencies, we need to share a collector
// between all of the module CompilerInstances. Other than that, we don't
// want to produce any dependency output from the module build.
- Instance.setModuleDepCollector(ImportingInstance.getModuleDepCollector());
+ Instance.setModuleDepCollector(getModuleDepCollector());
Inv.getDependencyOutputOpts() = DependencyOutputOptions();
return InstancePtr;
}
-/// Compile a module file for the given module, using the options
-/// provided by the importing compiler instance. Returns true if the module
-/// was built without errors.
-static bool compileModule(CompilerInstance &ImportingInstance,
- SourceLocation ImportLoc, StringRef ModuleName,
- StringRef ModuleFileName,
- CompilerInstance &Instance) {
+bool CompilerInstance::compileModule(SourceLocation ImportLoc,
+ StringRef ModuleName,
+ StringRef ModuleFileName,
+ CompilerInstance &Instance) {
llvm::TimeTraceScope TimeScope("Module Compile", ModuleName);
// Never compile a module that's already finalized - this would cause the
// existing module to be freed, causing crashes if it is later referenced
- if (ImportingInstance.getModuleCache().getInMemoryModuleCache().isPCMFinal(
- ModuleFileName)) {
- ImportingInstance.getDiagnostics().Report(
- ImportLoc, diag::err_module_rebuild_finalized)
+ if (getModuleCache().getInMemoryModuleCache().isPCMFinal(ModuleFileName)) {
+ getDiagnostics().Report(ImportLoc, diag::err_module_rebuild_finalized)
<< ModuleName;
return false;
}
- ImportingInstance.getDiagnostics().Report(ImportLoc,
- diag::remark_module_build)
- << ModuleName << ModuleFileName;
+ getDiagnostics().Report(ImportLoc, diag::remark_module_build)
+ << ModuleName << ModuleFileName;
// Execute the action to actually build the module in-place. Use a separate
// thread so that we get a stack large enough.
@@ -1292,13 +1274,15 @@ static bool compileModule(CompilerInstance &ImportingInstance,
},
DesiredStackSize);
- ImportingInstance.getDiagnostics().Report(ImportLoc,
- diag::remark_module_build_done)
- << ModuleName;
+ getDiagnostics().Report(ImportLoc, diag::remark_module_build_done)
+ << ModuleName;
// Propagate the statistics to the parent FileManager.
- if (!ImportingInstance.getFrontendOpts().ModulesShareFileManager)
- ImportingInstance.getFileManager().AddStats(Instance.getFileManager());
+ if (!getFrontendOpts().ModulesShareFileManager)
+ getFileManager().AddStats(Instance.getFileManager());
+
+ // Propagate the failed modules to the parent instance.
+ FailedModules = std::move(Instance.FailedModules);
if (Crashed) {
// Clear the ASTConsumer if it hasn't been already, in case it owns streams
@@ -1312,8 +1296,8 @@ static bool compileModule(CompilerInstance &ImportingInstance,
// We've rebuilt a module. If we're allowed to generate or update the global
// module index, record that fact in the importing compiler instance.
- if (ImportingInstance.getFrontendOpts().GenerateGlobalModuleIndex) {
- ImportingInstance.setBuildGlobalModuleIndex(true);
+ if (getFrontendOpts().GenerateGlobalModuleIndex) {
+ setBuildGlobalModuleIndex(true);
}
// If \p AllowPCMWithCompilerErrors is set return 'success' even if errors
@@ -1378,8 +1362,8 @@ std::unique_ptr<CompilerInstance> CompilerInstance::cloneForModuleCompile(
bool IsSystem = isSystem(SLoc.getFile().getFileCharacteristic());
// Use the module map where this module resides.
- return createCompilerInstanceForModuleCompileImpl(
- *this, ImportLoc, ModuleName,
+ return cloneForModuleCompileImpl(
+ ImportLoc, ModuleName,
FrontendInputFile(ModuleMapFilePath, IK, IsSystem),
ModMap.getModuleMapFileForUniquing(Module)->getName(), ModuleFileName);
}
@@ -1395,8 +1379,8 @@ std::unique_ptr<CompilerInstance> CompilerInstance::cloneForModuleCompile(
llvm::raw_string_ostream OS(InferredModuleMapContent);
Module->print(OS);
- auto Instance = createCompilerInstanceForModuleCompileImpl(
- *this, ImportLoc, ModuleName,
+ auto Instance = cloneForModuleCompileImpl(
+ ImportLoc, ModuleName,
FrontendInputFile(FakeModuleMapFile, IK, +Module->IsSystem),
ModMap.getModuleMapFileForUniquing(Module)->getName(), ModuleFileName);
@@ -1460,9 +1444,9 @@ static bool compileModuleAndReadASTImpl(CompilerInstance &ImportingInstance,
auto Instance = ImportingInstance.cloneForModuleCompile(ModuleNameLoc, Module,
ModuleFileName);
- if (!compileModule(ImportingInstance, ModuleNameLoc,
- Module->getTopLevelModuleName(), ModuleFileName,
- *Instance)) {
+ if (!ImportingInstance.compileModule(ModuleNameLoc,
+ Module->getTopLevelModuleName(),
+ ModuleFileName, *Instance)) {
ImportingInstance.getDiagnostics().Report(ModuleNameLoc,
diag::err_module_not_built)
<< Module->Name << SourceRange(ImportLoc, ModuleNameLoc);
@@ -2002,7 +1986,7 @@ ModuleLoadResult CompilerInstance::findOrCompileModuleAndReadAST(
}
// Check whether we have already attempted to build this module (but failed).
- if (FailedModules && FailedModules->hasAlreadyFailed(ModuleName)) {
+ if (FailedModules.contains(ModuleName)) {
getDiagnostics().Report(ModuleNameLoc, diag::err_module_not_built)
<< ModuleName << SourceRange(ImportLoc, ModuleNameLoc);
return nullptr;
@@ -2013,8 +1997,7 @@ ModuleLoadResult CompilerInstance::findOrCompileModuleAndReadAST(
ModuleFilename)) {
assert(getDiagnostics().hasErrorOccurred() &&
"undiagnosed error in compileModuleAndReadAST");
- if (FailedModules)
- FailedModules->addFailed(ModuleName);
+ FailedModules.insert(ModuleName);
return nullptr;
}
@@ -2238,8 +2221,8 @@ void CompilerInstance::createModuleFromSource(SourceLocation ImportLoc,
std::string NullTerminatedSource(Source.str());
- auto Other = createCompilerInstanceForModuleCompileImpl(
- *this, ImportLoc, ModuleName, Input, StringRef(), ModuleFileName);
+ auto Other = cloneForModuleCompileImpl(
+ ImportLoc, ModuleName, Input, StringRef(), ModuleFileName);
// Create a virtual file containing our desired source.
// FIXME: We shouldn't need to do this.
@@ -2252,8 +2235,7 @@ void CompilerInstance::createModuleFromSource(SourceLocation ImportLoc,
Other->DeleteBuiltModules = false;
// Build the module, inheriting any modules that we've built locally.
- bool Success =
- compileModule(*this, ImportLoc, ModuleName, ModuleFileName, *Other);
+ bool Success = compileModule(ImportLoc, ModuleName, ModuleFileName, *Other);
BuiltModules = std::move(Other->BuiltModules);
``````````
</details>
https://github.com/llvm/llvm-project/pull/135473
More information about the cfe-commits
mailing list