[clang] Extract CompilerInstance creation out of compileModuleImpl()
#134887
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
|
The only non-NFC change I'm aware of is that the |
|
@llvm/pr-subscribers-clang Author: Jan Svoboda (jansvoboda11) ChangesThis PR extracts the creation of Full diff: https://github.com/llvm/llvm-project/pull/134887.diff 1 Files Affected:
diff --git a/clang/lib/Frontend/CompilerInstance.cpp b/clang/lib/Frontend/CompilerInstance.cpp
index 9cab17ae70eeb..62fff9e2397a1 100644
--- a/clang/lib/Frontend/CompilerInstance.cpp
+++ b/clang/lib/Frontend/CompilerInstance.cpp
@@ -1138,41 +1138,16 @@ void CompilerInstance::LoadRequestedPlugins() {
}
}
-/// Determine the appropriate source input kind based on language
-/// options.
-static Language getLanguageFromOptions(const LangOptions &LangOpts) {
- if (LangOpts.OpenCL)
- return Language::OpenCL;
- if (LangOpts.CUDA)
- return Language::CUDA;
- if (LangOpts.ObjC)
- return LangOpts.CPlusPlus ? Language::ObjCXX : Language::ObjC;
- return LangOpts.CPlusPlus ? Language::CXX : Language::C;
-}
-
-/// 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
-compileModuleImpl(CompilerInstance &ImportingInstance, SourceLocation ImportLoc,
- StringRef ModuleName, FrontendInputFile Input,
- StringRef OriginalModuleMapFile, StringRef ModuleFileName,
- llvm::function_ref<void(CompilerInstance &)> PreBuildStep =
- [](CompilerInstance &) {},
- llvm::function_ref<void(CompilerInstance &)> PostBuildStep =
- [](CompilerInstance &) {}) {
- 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)
- << ModuleName;
- return false;
- }
-
+/// 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) {
// Construct a compiler invocation for creating this module.
auto Invocation =
std::make_shared<CompilerInvocation>(ImportingInstance.getInvocation());
@@ -1226,8 +1201,11 @@ compileModuleImpl(CompilerInstance &ImportingInstance, SourceLocation ImportLoc,
// module. Since we're sharing an in-memory module cache,
// CompilerInstance::CompilerInstance is responsible for finalizing the
// buffers to prevent use-after-frees.
- CompilerInstance Instance(ImportingInstance.getPCHContainerOperations(),
- &ImportingInstance.getModuleCache());
+ auto InstancePtr = std::make_unique<CompilerInstance>(
+ ImportingInstance.getPCHContainerOperations(),
+ &ImportingInstance.getModuleCache());
+ auto &Instance = *InstancePtr;
+
auto &Inv = *Invocation;
Instance.setInvocation(std::move(Invocation));
@@ -1267,45 +1245,19 @@ compileModuleImpl(CompilerInstance &ImportingInstance, SourceLocation ImportLoc,
Instance.setModuleDepCollector(ImportingInstance.getModuleDepCollector());
Inv.getDependencyOutputOpts() = DependencyOutputOptions();
- ImportingInstance.getDiagnostics().Report(ImportLoc,
- diag::remark_module_build)
- << ModuleName << ModuleFileName;
-
- PreBuildStep(Instance);
-
- // Execute the action to actually build the module in-place. Use a separate
- // thread so that we get a stack large enough.
- bool Crashed = !llvm::CrashRecoveryContext().RunSafelyOnThread(
- [&]() {
- GenerateModuleFromModuleMapAction Action;
- Instance.ExecuteAction(Action);
- },
- DesiredStackSize);
-
- PostBuildStep(Instance);
-
- ImportingInstance.getDiagnostics().Report(ImportLoc,
- diag::remark_module_build_done)
- << ModuleName;
-
- // Propagate the statistics to the parent FileManager.
- if (!FrontendOpts.ModulesShareFileManager)
- ImportingInstance.getFileManager().AddStats(Instance.getFileManager());
-
- if (Crashed) {
- // Clear the ASTConsumer if it hasn't been already, in case it owns streams
- // that must be closed before clearing output files.
- Instance.setSema(nullptr);
- Instance.setASTConsumer(nullptr);
-
- // Delete any remaining temporary files related to Instance.
- Instance.clearOutputFiles(/*EraseFiles=*/true);
- }
+ return InstancePtr;
+}
- // If \p AllowPCMWithCompilerErrors is set return 'success' even if errors
- // occurred.
- return !Instance.getDiagnostics().hasErrorOccurred() ||
- Instance.getFrontendOpts().AllowPCMWithCompilerErrors;
+/// Determine the appropriate source input kind based on language
+/// options.
+static Language getLanguageFromOptions(const LangOptions &LangOpts) {
+ if (LangOpts.OpenCL)
+ return Language::OpenCL;
+ if (LangOpts.CUDA)
+ return Language::CUDA;
+ if (LangOpts.ObjC)
+ return LangOpts.CPlusPlus ? Language::ObjCXX : Language::ObjC;
+ return LangOpts.CPlusPlus ? Language::CXX : Language::C;
}
static OptionalFileEntryRef getPublicModuleMap(FileEntryRef File,
@@ -1321,20 +1273,24 @@ static OptionalFileEntryRef getPublicModuleMap(FileEntryRef File,
return FileMgr.getOptionalFileRef(PublicFilename);
}
-/// Compile a module file for the given module in a separate compiler instance,
-/// 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, Module *Module,
- StringRef ModuleFileName) {
+/// Creates a \c CompilerInstance for compiling a module.
+///
+/// This takes care of creating appropriate \c FrontendInputFile for
+/// public/private frameworks, inferred modules and such.
+static std::unique_ptr<CompilerInstance>
+createCompilerInstanceForModuleCompile(CompilerInstance &ImportingInstance,
+ SourceLocation ImportLoc, Module *Module,
+ StringRef ModuleFileName) {
+ StringRef ModuleName = Module->getTopLevelModuleName();
+
InputKind IK(getLanguageFromOptions(ImportingInstance.getLangOpts()),
InputKind::ModuleMap);
// Get or create the module map that we'll use to build this module.
- ModuleMap &ModMap
- = ImportingInstance.getPreprocessor().getHeaderSearchInfo().getModuleMap();
+ ModuleMap &ModMap =
+ ImportingInstance.getPreprocessor().getHeaderSearchInfo().getModuleMap();
SourceManager &SourceMgr = ImportingInstance.getSourceManager();
- bool Result;
+
if (FileID ModuleMapFID = ModMap.getContainingModuleMapFileID(Module);
ModuleMapFID.isValid()) {
// We want to use the top-level module map. If we don't, the compiling
@@ -1368,44 +1324,115 @@ static bool compileModule(CompilerInstance &ImportingInstance,
bool IsSystem = isSystem(SLoc.getFile().getFileCharacteristic());
// Use the module map where this module resides.
- Result = compileModuleImpl(
- ImportingInstance, ImportLoc, Module->getTopLevelModuleName(),
+ return createCompilerInstanceForModuleCompileImpl(
+ ImportingInstance, ImportLoc, ModuleName,
FrontendInputFile(ModuleMapFilePath, IK, IsSystem),
ModMap.getModuleMapFileForUniquing(Module)->getName(), ModuleFileName);
- } else {
- // FIXME: We only need to fake up an input file here as a way of
- // transporting the module's directory to the module map parser. We should
- // be able to do that more directly, and parse from a memory buffer without
- // inventing this file.
- SmallString<128> FakeModuleMapFile(Module->Directory->getName());
- llvm::sys::path::append(FakeModuleMapFile, "__inferred_module.map");
-
- std::string InferredModuleMapContent;
- llvm::raw_string_ostream OS(InferredModuleMapContent);
- Module->print(OS);
-
- Result = compileModuleImpl(
- ImportingInstance, ImportLoc, Module->getTopLevelModuleName(),
- FrontendInputFile(FakeModuleMapFile, IK, +Module->IsSystem),
- ModMap.getModuleMapFileForUniquing(Module)->getName(),
- ModuleFileName,
- [&](CompilerInstance &Instance) {
- std::unique_ptr<llvm::MemoryBuffer> ModuleMapBuffer =
- llvm::MemoryBuffer::getMemBuffer(InferredModuleMapContent);
- FileEntryRef ModuleMapFile = Instance.getFileManager().getVirtualFileRef(
- FakeModuleMapFile, InferredModuleMapContent.size(), 0);
- Instance.getSourceManager().overrideFileContents(
- ModuleMapFile, std::move(ModuleMapBuffer));
- });
}
+ // FIXME: We only need to fake up an input file here as a way of
+ // transporting the module's directory to the module map parser. We should
+ // be able to do that more directly, and parse from a memory buffer without
+ // inventing this file.
+ SmallString<128> FakeModuleMapFile(Module->Directory->getName());
+ llvm::sys::path::append(FakeModuleMapFile, "__inferred_module.map");
+
+ std::string InferredModuleMapContent;
+ llvm::raw_string_ostream OS(InferredModuleMapContent);
+ Module->print(OS);
+
+ auto Instance = createCompilerInstanceForModuleCompileImpl(
+ ImportingInstance, ImportLoc, ModuleName,
+ FrontendInputFile(FakeModuleMapFile, IK, +Module->IsSystem),
+ ModMap.getModuleMapFileForUniquing(Module)->getName(), ModuleFileName);
+
+ std::unique_ptr<llvm::MemoryBuffer> ModuleMapBuffer =
+ llvm::MemoryBuffer::getMemBufferCopy(InferredModuleMapContent);
+ FileEntryRef ModuleMapFile = Instance->getFileManager().getVirtualFileRef(
+ FakeModuleMapFile, InferredModuleMapContent.size(), 0);
+ Instance->getSourceManager().overrideFileContents(ModuleMapFile,
+ std::move(ModuleMapBuffer));
+
+ return Instance;
+}
+
+/// 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 compileModuleImpl(CompilerInstance &ImportingInstance,
+ SourceLocation ImportLoc, StringRef ModuleName,
+ StringRef ModuleFileName,
+ CompilerInstance &Instance) {
+ llvm::TimeTraceScope TimeScope("Module Compile", ModuleName);
+
+ ImportingInstance.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.
+ bool Crashed = !llvm::CrashRecoveryContext().RunSafelyOnThread(
+ [&]() {
+ GenerateModuleFromModuleMapAction Action;
+ Instance.ExecuteAction(Action);
+ },
+ DesiredStackSize);
+
+ ImportingInstance.getDiagnostics().Report(ImportLoc,
+ diag::remark_module_build_done)
+ << ModuleName;
+
+ // Propagate the statistics to the parent FileManager.
+ if (ImportingInstance.getFileManagerPtr() != Instance.getFileManagerPtr())
+ ImportingInstance.getFileManager().AddStats(Instance.getFileManager());
+
+ if (Crashed) {
+ // Clear the ASTConsumer if it hasn't been already, in case it owns streams
+ // that must be closed before clearing output files.
+ Instance.setSema(nullptr);
+ Instance.setASTConsumer(nullptr);
+
+ // Delete any remaining temporary files related to Instance.
+ Instance.clearOutputFiles(/*EraseFiles=*/true);
+ }
+
+ // If \p AllowPCMWithCompilerErrors is set return 'success' even if errors
+ // occurred.
+ return !Instance.getDiagnostics().hasErrorOccurred() ||
+ Instance.getFrontendOpts().AllowPCMWithCompilerErrors;
+}
+
+/// Compile a module file for the given module in a separate compiler instance,
+/// 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, Module *Module,
+ StringRef ModuleFileName) {
+ StringRef ModuleName = Module->getTopLevelModuleName();
+
+ // 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)
+ << ModuleName;
+ return false;
+ }
+
+ auto Instance = createCompilerInstanceForModuleCompile(
+ ImportingInstance, ImportLoc, Module, ModuleFileName);
+
+ bool Success = compileModuleImpl(ImportingInstance, ImportLoc, ModuleName,
+ ModuleFileName, *Instance);
+
// 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);
}
- return Result;
+ return Success;
}
/// Read the AST right after compiling the module.
@@ -2232,25 +2259,26 @@ void CompilerInstance::createModuleFromSource(SourceLocation ImportLoc,
std::string NullTerminatedSource(Source.str());
- auto PreBuildStep = [&](CompilerInstance &Other) {
- // Create a virtual file containing our desired source.
- // FIXME: We shouldn't need to do this.
- FileEntryRef ModuleMapFile = Other.getFileManager().getVirtualFileRef(
- ModuleMapFileName, NullTerminatedSource.size(), 0);
- Other.getSourceManager().overrideFileContents(
- ModuleMapFile, llvm::MemoryBuffer::getMemBuffer(NullTerminatedSource));
+ auto Other = createCompilerInstanceForModuleCompileImpl(
+ *this, ImportLoc, ModuleName, Input, StringRef(), ModuleFileName);
- Other.BuiltModules = std::move(BuiltModules);
- Other.DeleteBuiltModules = false;
- };
+ // Create a virtual file containing our desired source.
+ // FIXME: We shouldn't need to do this.
+ FileEntryRef ModuleMapFile = Other->getFileManager().getVirtualFileRef(
+ ModuleMapFileName, NullTerminatedSource.size(), 0);
+ Other->getSourceManager().overrideFileContents(
+ ModuleMapFile, llvm::MemoryBuffer::getMemBuffer(NullTerminatedSource));
- auto PostBuildStep = [this](CompilerInstance &Other) {
- BuiltModules = std::move(Other.BuiltModules);
- };
+ Other->BuiltModules = std::move(BuiltModules);
+ Other->DeleteBuiltModules = false;
// Build the module, inheriting any modules that we've built locally.
- if (compileModuleImpl(*this, ImportLoc, ModuleName, Input, StringRef(),
- ModuleFileName, PreBuildStep, PostBuildStep)) {
+ bool Success =
+ compileModuleImpl(*this, ImportLoc, ModuleName, ModuleFileName, *Other);
+
+ BuiltModules = std::move(Other->BuiltModules);
+
+ if (Success) {
BuiltModules[std::string(ModuleName)] = std::string(ModuleFileName);
llvm::sys::RemoveFileOnSignal(ModuleFileName);
}
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
Can you not just move the |
I didn't want to do it there to avoid unnecessarily creating the // 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);
} |
Bigcheese
approved these changes
Apr 10, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/18/builds/14360 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/3/builds/14412 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
…()` (llvm#134887) This PR extracts the creation of `CompilerInstance` for compiling an implicitly-discovered module out of `compileModuleImpl()` into its own separate function and passes it into `compileModuleImpl()` from the outside. This makes the instance creation logic reusable (useful for my experiments) and also simplifies the API, removing the `PreBuildStep` and `PostBuildStep` hooks from `compileModuleImpl()`.
bnbarham
added a commit
to swiftlang/llvm-project
that referenced
this pull request
Jul 3, 2025
llvm#134887 added a clone for the compiler instance in `compileModuleAndReadASTImpl`, which would then be destroyed *after* the corresponding read in the importing instance. Swift has a `SwiftNameLookupExtension` module extension which updates (effectively) global state - populating the lookup table for a module on read and removing it when the module is destroyed. With newly cloned instance, we would then see: - Module compiled with cloned instance - Module read with importing instance - Lookup table for that module added - Cloned instance destroyed - Module from that cloned instance destroyed - Lookup table for that module name removed Depending on the original semantics is incredibly fragile, but for now it's good enough to ensure that the read in the importing instance is after the cloned instanced is destroyed. Ideally we'd only ever add to the lookup tables in the original importing instance, never its clones.
AnthonyLatsis
pushed a commit
to swiftlang/llvm-project
that referenced
this pull request
Jul 18, 2025
llvm#134887 added a clone for the compiler instance in `compileModuleAndReadASTImpl`, which would then be destroyed *after* the corresponding read in the importing instance. Swift has a `SwiftNameLookupExtension` module extension which updates (effectively) global state - populating the lookup table for a module on read and removing it when the module is destroyed. With newly cloned instance, we would then see: - Module compiled with cloned instance - Module read with importing instance - Lookup table for that module added - Cloned instance destroyed - Module from that cloned instance destroyed - Lookup table for that module name removed Depending on the original semantics is incredibly fragile, but for now it's good enough to ensure that the read in the importing instance is after the cloned instanced is destroyed. Ideally we'd only ever add to the lookup tables in the original importing instance, never its clones.
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 extracts the creation of
CompilerInstancefor compiling an implicitly-discovered module out ofcompileModuleImpl()into its own separate function and passes it intocompileModuleImpl()from the outside. This makes the instance creation logic reusable (useful for my experiments) and also simplifies the API, removing thePreBuildStepandPostBuildStephooks fromcompileModuleImpl().