Skip to content

[clang][frontend] Make CompilerInstance::FailedModules thread-safe #135473

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Apr 15, 2025
Merged

[clang][frontend] Make CompilerInstance::FailedModules thread-safe #135473

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b58c683
[clang][frontend] Promote `cloneForModuleCompileImpl` to `CompilerIns…
jansvoboda11 Apr 11, 2025
d29fe56
[clang][frontend] Promote `compileModule` to `CompilerInstance` member
jansvoboda11 Apr 11, 2025
f98b989
[clang][frontend] Make `CompilerInstance::FailedModules` thread-safe
jansvoboda11 Apr 11, 2025
2a1d274
clang-format
jansvoboda11 Apr 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 17 additions & 30 deletions clang/include/clang/Frontend/CompilerInstance.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
/// @{
Expand Down Expand Up @@ -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.
///
Expand All @@ -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;
Expand Down
112 changes: 46 additions & 66 deletions clang/lib/Frontend/CompilerInstance.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand All @@ -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);
Expand All @@ -1206,82 +1198,70 @@ createCompilerInstanceForModuleCompileImpl(CompilerInstance &ImportingInstance,
DiagnosticOptions &DiagOpts = Invocation->getDiagnosticOpts();

DiagOpts.VerifyDiagnostics = 0;
assert(ImportingInstance.getInvocation().getModuleHash() ==
Invocation->getModuleHash() && "Module hash mismatch!");
assert(getInvocation().getModuleHash() == Invocation->getModuleHash() &&
"Module hash mismatch!");

// Construct a compiler instance that will be used to actually create the
// module. Since we're sharing an in-memory module cache,
// 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();

// 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());
SourceMgr.setModuleBuildStack(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.
Expand All @@ -1292,13 +1272,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
Expand All @@ -1312,8 +1294,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
Expand Down Expand Up @@ -1378,8 +1360,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);
}
Expand All @@ -1395,8 +1377,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);

Expand Down Expand Up @@ -1460,9 +1442,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);
Expand Down Expand Up @@ -2002,7 +1984,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;
Expand All @@ -2013,8 +1995,7 @@ ModuleLoadResult CompilerInstance::findOrCompileModuleAndReadAST(
ModuleFilename)) {
assert(getDiagnostics().hasErrorOccurred() &&
"undiagnosed error in compileModuleAndReadAST");
if (FailedModules)
FailedModules->addFailed(ModuleName);
FailedModules.insert(ModuleName);
return nullptr;
}

Expand Down Expand Up @@ -2238,8 +2219,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.
Expand All @@ -2252,8 +2233,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);

Expand Down