[clang][frontend] Make CompilerInstance::FailedModules thread-safe
#135473
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-clang Author: Jan Svoboda (jansvoboda11) ChangesThis PR makes some progress towards making it possible to create clones of Besides that change, this PR also turns two previously free functions with internal linkage into member functions of Full diff: https://github.com/llvm/llvm-project/pull/135473.diff 2 Files Affected:
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);
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Bigcheese
reviewed
Apr 14, 2025
Bigcheese
approved these changes
Apr 15, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/166/builds/994 Here is the relevant piece of the build log for the reference |
var-const
pushed a commit
to ldionne/llvm-project
that referenced
this pull request
Apr 17, 2025
…lvm#135473) 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.
jansvoboda11
added a commit
to jansvoboda11/llvm-project
that referenced
this pull request
Apr 24, 2025
…tor thread-safe The same principle as llvm#135473, llvm#135737, llvm#136178, llvm#136601 & llvm#137059.
jansvoboda11
added a commit
that referenced
this pull request
Apr 29, 2025
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
…tor thread-safe (llvm#137227) The same principle as llvm#135473, llvm#135737, llvm#136178, llvm#136601 & llvm#137059.
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
…tor thread-safe (llvm#137227) The same principle as llvm#135473, llvm#135737, llvm#136178, llvm#136601 & llvm#137059.
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
…tor thread-safe (llvm#137227) The same principle as llvm#135473, llvm#135737, llvm#136178, llvm#136601 & llvm#137059.
GeorgeARM
pushed a commit
to GeorgeARM/llvm-project
that referenced
this pull request
May 7, 2025
…tor thread-safe (llvm#137227) The same principle as llvm#135473, llvm#135737, llvm#136178, llvm#136601 & llvm#137059.
Ankur-0429
pushed a commit
to Ankur-0429/llvm-project
that referenced
this pull request
May 9, 2025
…tor thread-safe (llvm#137227) The same principle as llvm#135473, llvm#135737, llvm#136178, llvm#136601 & llvm#137059.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR makes some progress towards making it possible to create clones of
CompilerInstancethat are independent of each other and can be used in a multi-threaded setting. This PR tacklesCompilerInstance::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 toFailedModules. This reduces some API churn that was necessary for each new member ofCompilerInstancethat needs to be cloned.