Author: Jan Svoboda Date: 2025-04-14T20:01:06-07:00 New Revision: 1cf9f764ac41fb3492e10c78640dd50e616388db
URL: https://github.com/llvm/llvm-project/commit/1cf9f764ac41fb3492e10c78640dd50e616388db DIFF: https://github.com/llvm/llvm-project/commit/1cf9f764ac41fb3492e10c78640dd50e616388db.diff LOG: [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. Added: Modified: clang/include/clang/Frontend/CompilerInstance.h clang/lib/Frontend/CompilerInstance.cpp Removed: ################################################################################ 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..243e0a3c15b05 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,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. @@ -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 @@ -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 @@ -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); } @@ -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); @@ -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); @@ -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; @@ -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; } @@ -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. @@ -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); _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
