Skip to content

Commit 1cf9f76

Browse files
authored
[clang][frontend] Make CompilerInstance::FailedModules thread-safe (#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.
1 parent 984ec70 commit 1cf9f76

File tree

2 files changed

+63
-96
lines changed

2 files changed

+63
-96
lines changed

clang/include/clang/Frontend/CompilerInstance.h

Lines changed: 17 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -134,23 +134,13 @@ class CompilerInstance : public ModuleLoader {
134134

135135
std::vector<std::shared_ptr<DependencyCollector>> DependencyCollectors;
136136

137-
/// Records the set of modules
138-
class FailedModulesSet {
139-
llvm::StringSet<> Failed;
140-
141-
public:
142-
bool hasAlreadyFailed(StringRef module) { return Failed.count(module) > 0; }
143-
144-
void addFailed(StringRef module) { Failed.insert(module); }
145-
};
146-
147137
/// The set of modules that failed to build.
148138
///
149-
/// This pointer will be shared among all of the compiler instances created
139+
/// This value will be passed among all of the compiler instances created
150140
/// to (re)build modules, so that once a module fails to build anywhere,
151141
/// other instances will see that the module has failed and won't try to
152142
/// build it again.
153-
std::shared_ptr<FailedModulesSet> FailedModules;
143+
llvm::StringSet<> FailedModules;
154144

155145
/// The set of top-level modules that has already been built on the
156146
/// fly as part of this overall compilation action.
@@ -637,24 +627,6 @@ class CompilerInstance : public ModuleLoader {
637627
return *FrontendTimer;
638628
}
639629

640-
/// @}
641-
/// @name Failed modules set
642-
/// @{
643-
644-
bool hasFailedModulesSet() const { return (bool)FailedModules; }
645-
646-
void createFailedModulesSet() {
647-
FailedModules = std::make_shared<FailedModulesSet>();
648-
}
649-
650-
std::shared_ptr<FailedModulesSet> getFailedModulesSetPtr() const {
651-
return FailedModules;
652-
}
653-
654-
void setFailedModulesSet(std::shared_ptr<FailedModulesSet> FMS) {
655-
FailedModules = FMS;
656-
}
657-
658630
/// }
659631
/// @name Output Files
660632
/// @{
@@ -870,6 +842,13 @@ class CompilerInstance : public ModuleLoader {
870842
SourceLocation ModuleNameLoc,
871843
bool IsInclusionDirective);
872844

845+
/// Creates a \c CompilerInstance for compiling a module.
846+
///
847+
/// This expects a properly initialized \c FrontendInputFile.
848+
std::unique_ptr<CompilerInstance> cloneForModuleCompileImpl(
849+
SourceLocation ImportLoc, StringRef ModuleName, FrontendInputFile Input,
850+
StringRef OriginalModuleMapFile, StringRef ModuleFileName);
851+
873852
public:
874853
/// Creates a new \c CompilerInstance for compiling a module.
875854
///
@@ -879,6 +858,14 @@ class CompilerInstance : public ModuleLoader {
879858
cloneForModuleCompile(SourceLocation ImportLoc, Module *Module,
880859
StringRef ModuleFileName);
881860

861+
/// Compile a module file for the given module, using the options
862+
/// provided by the importing compiler instance. Returns true if the module
863+
/// was built without errors.
864+
// FIXME: This should be private, but it's called from static non-member
865+
// functions in the implementation file.
866+
bool compileModule(SourceLocation ImportLoc, StringRef ModuleName,
867+
StringRef ModuleFileName, CompilerInstance &Instance);
868+
882869
ModuleLoadResult loadModule(SourceLocation ImportLoc, ModuleIdPath Path,
883870
Module::NameVisibilityKind Visibility,
884871
bool IsInclusionDirective) override;

clang/lib/Frontend/CompilerInstance.cpp

Lines changed: 46 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -1150,19 +1150,11 @@ static Language getLanguageFromOptions(const LangOptions &LangOpts) {
11501150
return LangOpts.CPlusPlus ? Language::CXX : Language::C;
11511151
}
11521152

1153-
/// Creates a \c CompilerInstance for compiling a module.
1154-
///
1155-
/// This expects a properly initialized \c FrontendInputFile.
1156-
static std::unique_ptr<CompilerInstance>
1157-
createCompilerInstanceForModuleCompileImpl(CompilerInstance &ImportingInstance,
1158-
SourceLocation ImportLoc,
1159-
StringRef ModuleName,
1160-
FrontendInputFile Input,
1161-
StringRef OriginalModuleMapFile,
1162-
StringRef ModuleFileName) {
1153+
std::unique_ptr<CompilerInstance> CompilerInstance::cloneForModuleCompileImpl(
1154+
SourceLocation ImportLoc, StringRef ModuleName, FrontendInputFile Input,
1155+
StringRef OriginalModuleMapFile, StringRef ModuleFileName) {
11631156
// Construct a compiler invocation for creating this module.
1164-
auto Invocation =
1165-
std::make_shared<CompilerInvocation>(ImportingInstance.getInvocation());
1157+
auto Invocation = std::make_shared<CompilerInvocation>(getInvocation());
11661158

11671159
PreprocessorOptions &PPOpts = Invocation->getPreprocessorOpts();
11681160

@@ -1182,7 +1174,7 @@ createCompilerInstanceForModuleCompileImpl(CompilerInstance &ImportingInstance,
11821174

11831175
// If the original compiler invocation had -fmodule-name, pass it through.
11841176
Invocation->getLangOpts().ModuleName =
1185-
ImportingInstance.getInvocation().getLangOpts().ModuleName;
1177+
getInvocation().getLangOpts().ModuleName;
11861178

11871179
// Note the name of the module we're building.
11881180
Invocation->getLangOpts().CurrentModule = std::string(ModuleName);
@@ -1206,82 +1198,70 @@ createCompilerInstanceForModuleCompileImpl(CompilerInstance &ImportingInstance,
12061198
DiagnosticOptions &DiagOpts = Invocation->getDiagnosticOpts();
12071199

12081200
DiagOpts.VerifyDiagnostics = 0;
1209-
assert(ImportingInstance.getInvocation().getModuleHash() ==
1210-
Invocation->getModuleHash() && "Module hash mismatch!");
1201+
assert(getInvocation().getModuleHash() == Invocation->getModuleHash() &&
1202+
"Module hash mismatch!");
12111203

12121204
// Construct a compiler instance that will be used to actually create the
12131205
// module. Since we're sharing an in-memory module cache,
12141206
// CompilerInstance::CompilerInstance is responsible for finalizing the
12151207
// buffers to prevent use-after-frees.
12161208
auto InstancePtr = std::make_unique<CompilerInstance>(
1217-
ImportingInstance.getPCHContainerOperations(),
1218-
&ImportingInstance.getModuleCache());
1209+
getPCHContainerOperations(), &getModuleCache());
12191210
auto &Instance = *InstancePtr;
12201211

12211212
auto &Inv = *Invocation;
12221213
Instance.setInvocation(std::move(Invocation));
12231214

12241215
Instance.createDiagnostics(
1225-
ImportingInstance.getVirtualFileSystem(),
1226-
new ForwardingDiagnosticConsumer(ImportingInstance.getDiagnosticClient()),
1216+
getVirtualFileSystem(),
1217+
new ForwardingDiagnosticConsumer(getDiagnosticClient()),
12271218
/*ShouldOwnClient=*/true);
12281219

12291220
if (llvm::is_contained(DiagOpts.SystemHeaderWarningsModules, ModuleName))
12301221
Instance.getDiagnostics().setSuppressSystemWarnings(false);
12311222

12321223
if (FrontendOpts.ModulesShareFileManager) {
1233-
Instance.setFileManager(&ImportingInstance.getFileManager());
1224+
Instance.setFileManager(&getFileManager());
12341225
} else {
1235-
Instance.createFileManager(&ImportingInstance.getVirtualFileSystem());
1226+
Instance.createFileManager(&getVirtualFileSystem());
12361227
}
12371228
Instance.createSourceManager(Instance.getFileManager());
12381229
SourceManager &SourceMgr = Instance.getSourceManager();
12391230

12401231
// Note that this module is part of the module build stack, so that we
12411232
// can detect cycles in the module graph.
1242-
SourceMgr.setModuleBuildStack(
1243-
ImportingInstance.getSourceManager().getModuleBuildStack());
1233+
SourceMgr.setModuleBuildStack(getSourceManager().getModuleBuildStack());
12441234
SourceMgr.pushModuleBuildStack(ModuleName,
1245-
FullSourceLoc(ImportLoc, ImportingInstance.getSourceManager()));
1235+
FullSourceLoc(ImportLoc, getSourceManager()));
12461236

1247-
// Make sure that the failed-module structure has been allocated in
1248-
// the importing instance, and propagate the pointer to the newly-created
1249-
// instance.
1250-
if (!ImportingInstance.hasFailedModulesSet())
1251-
ImportingInstance.createFailedModulesSet();
1252-
Instance.setFailedModulesSet(ImportingInstance.getFailedModulesSetPtr());
1237+
// Make a copy for the new instance.
1238+
Instance.FailedModules = FailedModules;
12531239

12541240
// If we're collecting module dependencies, we need to share a collector
12551241
// between all of the module CompilerInstances. Other than that, we don't
12561242
// want to produce any dependency output from the module build.
1257-
Instance.setModuleDepCollector(ImportingInstance.getModuleDepCollector());
1243+
Instance.setModuleDepCollector(getModuleDepCollector());
12581244
Inv.getDependencyOutputOpts() = DependencyOutputOptions();
12591245

12601246
return InstancePtr;
12611247
}
12621248

1263-
/// Compile a module file for the given module, using the options
1264-
/// provided by the importing compiler instance. Returns true if the module
1265-
/// was built without errors.
1266-
static bool compileModule(CompilerInstance &ImportingInstance,
1267-
SourceLocation ImportLoc, StringRef ModuleName,
1268-
StringRef ModuleFileName,
1269-
CompilerInstance &Instance) {
1249+
bool CompilerInstance::compileModule(SourceLocation ImportLoc,
1250+
StringRef ModuleName,
1251+
StringRef ModuleFileName,
1252+
CompilerInstance &Instance) {
12701253
llvm::TimeTraceScope TimeScope("Module Compile", ModuleName);
12711254

12721255
// Never compile a module that's already finalized - this would cause the
12731256
// existing module to be freed, causing crashes if it is later referenced
1274-
if (ImportingInstance.getModuleCache().getInMemoryModuleCache().isPCMFinal(
1275-
ModuleFileName)) {
1276-
ImportingInstance.getDiagnostics().Report(
1277-
ImportLoc, diag::err_module_rebuild_finalized)
1257+
if (getModuleCache().getInMemoryModuleCache().isPCMFinal(ModuleFileName)) {
1258+
getDiagnostics().Report(ImportLoc, diag::err_module_rebuild_finalized)
12781259
<< ModuleName;
12791260
return false;
12801261
}
12811262

1282-
ImportingInstance.getDiagnostics().Report(ImportLoc,
1283-
diag::remark_module_build)
1284-
<< ModuleName << ModuleFileName;
1263+
getDiagnostics().Report(ImportLoc, diag::remark_module_build)
1264+
<< ModuleName << ModuleFileName;
12851265

12861266
// Execute the action to actually build the module in-place. Use a separate
12871267
// thread so that we get a stack large enough.
@@ -1292,13 +1272,15 @@ static bool compileModule(CompilerInstance &ImportingInstance,
12921272
},
12931273
DesiredStackSize);
12941274

1295-
ImportingInstance.getDiagnostics().Report(ImportLoc,
1296-
diag::remark_module_build_done)
1297-
<< ModuleName;
1275+
getDiagnostics().Report(ImportLoc, diag::remark_module_build_done)
1276+
<< ModuleName;
12981277

12991278
// Propagate the statistics to the parent FileManager.
1300-
if (!ImportingInstance.getFrontendOpts().ModulesShareFileManager)
1301-
ImportingInstance.getFileManager().AddStats(Instance.getFileManager());
1279+
if (!getFrontendOpts().ModulesShareFileManager)
1280+
getFileManager().AddStats(Instance.getFileManager());
1281+
1282+
// Propagate the failed modules to the parent instance.
1283+
FailedModules = std::move(Instance.FailedModules);
13021284

13031285
if (Crashed) {
13041286
// Clear the ASTConsumer if it hasn't been already, in case it owns streams
@@ -1312,8 +1294,8 @@ static bool compileModule(CompilerInstance &ImportingInstance,
13121294

13131295
// We've rebuilt a module. If we're allowed to generate or update the global
13141296
// module index, record that fact in the importing compiler instance.
1315-
if (ImportingInstance.getFrontendOpts().GenerateGlobalModuleIndex) {
1316-
ImportingInstance.setBuildGlobalModuleIndex(true);
1297+
if (getFrontendOpts().GenerateGlobalModuleIndex) {
1298+
setBuildGlobalModuleIndex(true);
13171299
}
13181300

13191301
// If \p AllowPCMWithCompilerErrors is set return 'success' even if errors
@@ -1378,8 +1360,8 @@ std::unique_ptr<CompilerInstance> CompilerInstance::cloneForModuleCompile(
13781360
bool IsSystem = isSystem(SLoc.getFile().getFileCharacteristic());
13791361

13801362
// Use the module map where this module resides.
1381-
return createCompilerInstanceForModuleCompileImpl(
1382-
*this, ImportLoc, ModuleName,
1363+
return cloneForModuleCompileImpl(
1364+
ImportLoc, ModuleName,
13831365
FrontendInputFile(ModuleMapFilePath, IK, IsSystem),
13841366
ModMap.getModuleMapFileForUniquing(Module)->getName(), ModuleFileName);
13851367
}
@@ -1395,8 +1377,8 @@ std::unique_ptr<CompilerInstance> CompilerInstance::cloneForModuleCompile(
13951377
llvm::raw_string_ostream OS(InferredModuleMapContent);
13961378
Module->print(OS);
13971379

1398-
auto Instance = createCompilerInstanceForModuleCompileImpl(
1399-
*this, ImportLoc, ModuleName,
1380+
auto Instance = cloneForModuleCompileImpl(
1381+
ImportLoc, ModuleName,
14001382
FrontendInputFile(FakeModuleMapFile, IK, +Module->IsSystem),
14011383
ModMap.getModuleMapFileForUniquing(Module)->getName(), ModuleFileName);
14021384

@@ -1460,9 +1442,9 @@ static bool compileModuleAndReadASTImpl(CompilerInstance &ImportingInstance,
14601442
auto Instance = ImportingInstance.cloneForModuleCompile(ModuleNameLoc, Module,
14611443
ModuleFileName);
14621444

1463-
if (!compileModule(ImportingInstance, ModuleNameLoc,
1464-
Module->getTopLevelModuleName(), ModuleFileName,
1465-
*Instance)) {
1445+
if (!ImportingInstance.compileModule(ModuleNameLoc,
1446+
Module->getTopLevelModuleName(),
1447+
ModuleFileName, *Instance)) {
14661448
ImportingInstance.getDiagnostics().Report(ModuleNameLoc,
14671449
diag::err_module_not_built)
14681450
<< Module->Name << SourceRange(ImportLoc, ModuleNameLoc);
@@ -2002,7 +1984,7 @@ ModuleLoadResult CompilerInstance::findOrCompileModuleAndReadAST(
20021984
}
20031985

20041986
// Check whether we have already attempted to build this module (but failed).
2005-
if (FailedModules && FailedModules->hasAlreadyFailed(ModuleName)) {
1987+
if (FailedModules.contains(ModuleName)) {
20061988
getDiagnostics().Report(ModuleNameLoc, diag::err_module_not_built)
20071989
<< ModuleName << SourceRange(ImportLoc, ModuleNameLoc);
20081990
return nullptr;
@@ -2013,8 +1995,7 @@ ModuleLoadResult CompilerInstance::findOrCompileModuleAndReadAST(
20131995
ModuleFilename)) {
20141996
assert(getDiagnostics().hasErrorOccurred() &&
20151997
"undiagnosed error in compileModuleAndReadAST");
2016-
if (FailedModules)
2017-
FailedModules->addFailed(ModuleName);
1998+
FailedModules.insert(ModuleName);
20181999
return nullptr;
20192000
}
20202001

@@ -2238,8 +2219,8 @@ void CompilerInstance::createModuleFromSource(SourceLocation ImportLoc,
22382219

22392220
std::string NullTerminatedSource(Source.str());
22402221

2241-
auto Other = createCompilerInstanceForModuleCompileImpl(
2242-
*this, ImportLoc, ModuleName, Input, StringRef(), ModuleFileName);
2222+
auto Other = cloneForModuleCompileImpl(ImportLoc, ModuleName, Input,
2223+
StringRef(), ModuleFileName);
22432224

22442225
// Create a virtual file containing our desired source.
22452226
// FIXME: We shouldn't need to do this.
@@ -2252,8 +2233,7 @@ void CompilerInstance::createModuleFromSource(SourceLocation ImportLoc,
22522233
Other->DeleteBuiltModules = false;
22532234

22542235
// Build the module, inheriting any modules that we've built locally.
2255-
bool Success =
2256-
compileModule(*this, ImportLoc, ModuleName, ModuleFileName, *Other);
2236+
bool Success = compileModule(ImportLoc, ModuleName, ModuleFileName, *Other);
22572237

22582238
BuiltModules = std::move(Other->BuiltModules);
22592239

0 commit comments

Comments
 (0)