[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.

Author: jansvoboda11

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;

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);