clang/Modules: Refactor CompilerInstance::loadModule, NFC

[dexonsmith]

Summary

Refactor the logic on CompilerInstance::loadModule and a couple of
surrounding methods in order to clarify what's going on.

• Rename ModuleLoader::loadModuleFromSource to compileModuleFromSource and fix its documentation, since it never loads a module. It just creates/compiles one.
• Rename one of the overloads of compileModuleImpl to compileModule, making it more obvious which one calls the other.
• Rename compileAndLoadModule to compileModuleAndReadAST. This clarifies the relationship between this helper and its caller, CompilerInstance::loadModule (the old name implied the opposite relationship). It also (correctly) indicates that more needs to be done to load the module than this function is responsible for.
• Split findOrCompileModuleAndReadAST out of loadModule. Besides reducing nesting for this code thanks to early returns and the like, this refactor clarifies the logic in loadModule, particularly around calls to ModuleMap::cacheModuleLoad and ModuleMap::getCachedModuleLoad. findOrCompileModuleAndReadAST also breaks early if the initial ReadAST call returns Missing or OutOfDate, allowing the last ditch call to compileModuleAndReadAST to come at the end of the function body.
  • Additionally split out selectModuleSource, clarifying the logic due to early returns.
  • Add ModuleLoadResult::isNormal and OtherUncachedFailure, so that loadModule knows whether to cache the result. OtherUncachedFailure was added to keep this patch NFC, but there's a chance that these cases were uncached by accident, through copy/paste/modify failures. These should be audited as a follow-up (maybe we can eliminate this case).
  • Do *not* lift the setting of ModuleLoadFailed = true to loadModule because there isn't a clear pattern for when it's set. This should be reconsidered in a follow-up, in case it would be correct to set ModuleLoadFailed whenever no module is returned and the result is either Normal or OtherUncachedFailure.
• Add some header documentation where it was missing, and fix it where it was wrong.

This should have no functionality change.

[dexonsmith]

clang/include/clang/Frontend/CompilerInstance.h

801–804

I put this in the header to reduce how many parameters to pass in by-reference (it felt unnecessarily verbose). But it's possible to make it a static in the source file if that's better.

clang/lib/Frontend/CompilerInstance.cpp

1867–1874

Note that this FIXME predates this patch (it has just moved).

[jkorous]

clang/include/clang/Lex/ModuleLoader.h

50

Just a typo - the comma at the end.

[dexonsmith]

clang/include/clang/Lex/ModuleLoader.h

50

That comma was intentional, actually! Then the next time a value gets added/removed it doesn't unnecessarily cause differences to other liens :).

[Bigcheese]

lgtm. Nice cleanup.

[bruno]

Nice! LGTM

[dexonsmith]

Pushed as 20d51b2f14ac4488f684f8fc57cb0ba718a6b91d.

clang/include/clang/Frontend/CompilerInstance.h

std::unique_ptr<llvm::raw_pwrite_stream> takeOutputStream() {
     return std::move(OutputStream);
   }
 
   // Create module manager.
   void createModuleManager();
 
   bool loadModuleFile(StringRef FileName);
 
+private:
+  /// Find a module, potentially compiling it, before reading its AST.  This is
+  /// the guts of loadModule.
+  ///
+  /// For prebuilt modules, the Module is not expected to exist in
+  /// HeaderSearch's ModuleMap.  If a ModuleFile by that name is in the
+  /// ModuleManager, then it will be loaded and looked up.
+  ///
+  /// For implicit modules, the Module is expected to already be in the
+  /// ModuleMap.  First attempt to load it from the given path on disk.  If that
+  /// fails, defer to compileModuleAndReadAST, which will first build and then
+  /// load it.
+  ModuleLoadResult findOrCompileModuleAndReadAST(StringRef ModuleName,
+                                                 SourceLocation ImportLoc,
+                                                 SourceLocation ModuleNameLoc,
+                                                 bool IsInclusionDirective);
+
+public:
   ModuleLoadResult loadModule(SourceLocation ImportLoc, ModuleIdPath Path,
                               Module::NameVisibilityKind Visibility,
                               bool IsInclusionDirective) override;
 
-  void loadModuleFromSource(SourceLocation ImportLoc, StringRef ModuleName,
+  void createModuleFromSource(SourceLocation ImportLoc, StringRef ModuleName,
                               StringRef Source) override;
 
   void makeModuleVisible(Module *Mod, Module::NameVisibilityKind Visibility,
                          SourceLocation ImportLoc) override;
 
   bool hadModuleLoaderFatalFailure() const {
     return ModuleLoader::HadFatalFailure;
   }
 

clang/include/clang/Lex/ModuleLoader.h

enum LoadResultKind {
     Normal,
 
     // The module exists, but does not actually contain the named submodule.
     // This should only happen if the named submodule was inferred from an
     // umbrella directory, but not actually part of the umbrella header.
     MissingExpected,
 
     // The module exists but cannot be imported due to a configuration mismatch.
-    ConfigMismatch
+    ConfigMismatch,
+
+    // We failed to load the module, but we shouldn't cache the failure.
+    OtherUncachedFailure,
   };
   llvm::PointerIntPair<Module *, 2, LoadResultKind> Storage;
 
   ModuleLoadResult() = default;
   ModuleLoadResult(Module *M) : Storage(M, Normal) {}
   ModuleLoadResult(LoadResultKind Kind) : Storage(nullptr, Kind) {}
 
   operator Module *() const { return Storage.getPointer(); }
 
+  /// Determines whether this is a normal return, whether or not loading the
+  /// module was successful.
+  bool isNormal() const { return Storage.getInt() == Normal; }
+
   /// Determines whether the module, which failed to load, was
   /// actually a submodule that we expected to see (based on implying the
   /// submodule from header structure), but didn't materialize in the actual
   /// module.
   bool isMissingExpected() const { return Storage.getInt() == MissingExpected; }
 
   /// Determines whether the module failed to load due to a configuration
   /// mismatch with an explicitly-named .pcm file from the command line.
public:
   /// Flag indicating whether this instance is building a module.
   void setBuildingModule(bool BuildingModuleFlag) {
     BuildingModule = BuildingModuleFlag;
   }
 
   /// Attempt to load the given module.
   ///
   /// This routine attempts to load the module described by the given
-  /// parameters.
+  /// parameters.  If there is a module cache, this may implicitly compile the
+  /// module before loading it.
   ///
   /// \param ImportLoc The location of the 'import' keyword.
   ///
   /// \param Path The identifiers (and their locations) of the module
   /// "path", e.g., "std.vector" would be split into "std" and "vector".
   ///
   /// \param Visibility The visibility provided for the names in the loaded
   /// module.
   ///
   /// \param IsInclusionDirective Indicates that this module is being loaded
   /// implicitly, due to the presence of an inclusion directive. Otherwise,
   /// it is being loaded due to an import declaration.
   ///
   /// \returns If successful, returns the loaded module. Otherwise, returns
   /// NULL to indicate that the module could not be loaded.
   virtual ModuleLoadResult loadModule(SourceLocation ImportLoc,
                                       ModuleIdPath Path,
                                       Module::NameVisibilityKind Visibility,
                                       bool IsInclusionDirective) = 0;
 
-  /// Attempt to load the given module from the specified source buffer. Does
-  /// not make any submodule visible; for that, use loadModule or
-  /// makeModuleVisible.
+  /// Attempt to create the given module from the specified source buffer.
+  /// Does not load the module or make any submodule visible; for that, use
+  /// loadModule and makeModuleVisible.
   ///
-  /// \param Loc The location at which the module was loaded.
-  /// \param ModuleName The name of the module to build.
+  /// \param Loc The location at which to create the module.
+  /// \param ModuleName The name of the module to create.
   /// \param Source The source of the module: a (preprocessed) module map.
-  virtual void loadModuleFromSource(SourceLocation Loc, StringRef ModuleName,
+  virtual void createModuleFromSource(SourceLocation Loc, StringRef ModuleName,
                                       StringRef Source) = 0;
 
   /// Make the given module visible.
   virtual void makeModuleVisible(Module *Mod,
                                  Module::NameVisibilityKind Visibility,
                                  SourceLocation ImportLoc) = 0;
 
   /// Load, create, or return global module.
   /// This function returns an existing global module index, if one
public:
   /// \param TriggerLoc The location for what triggered the load.
   /// \returns Returns true if any modules with that symbol found.
   virtual bool lookupMissingImports(StringRef Name,
                                     SourceLocation TriggerLoc) = 0;
 
   bool HadFatalFailure = false;
 };
 
-/// A module loader that doesn't know how to load modules.
+/// A module loader that doesn't know how to create or load modules.
 class TrivialModuleLoader : public ModuleLoader {
 public:
   ModuleLoadResult loadModule(SourceLocation ImportLoc, ModuleIdPath Path,
                               Module::NameVisibilityKind Visibility,
                               bool IsInclusionDirective) override {
     return {};
   }
 
-  void loadModuleFromSource(SourceLocation ImportLoc, StringRef ModuleName,
+  void createModuleFromSource(SourceLocation ImportLoc, StringRef ModuleName,
                               StringRef Source) override {}
 
   void makeModuleVisible(Module *Mod, Module::NameVisibilityKind Visibility,
                          SourceLocation ImportLoc) override {}
 
   GlobalModuleIndex *loadGlobalModuleIndex(SourceLocation TriggerLoc) override {
     return nullptr;
   }
 

clang/lib/Frontend/CompilerInstance.cpp

else if (Filename == "module.private.modulemap")
     llvm::sys::path::append(PublicFilename, "module.modulemap");
   else
     return nullptr;
   if (auto FE = FileMgr.getFile(PublicFilename))
     return *FE;
   return nullptr;
 }
 
-/// 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,
-                              Module *Module,
-                              StringRef ModuleFileName) {
+/// 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,
   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();
   bool Result;
   if (const FileEntry *ModuleMapFile =
static bool compileModule(CompilerInstance &ImportingInstance,
   // module index, record that fact in the importing compiler instance.
   if (ImportingInstance.getFrontendOpts().GenerateGlobalModuleIndex) {
     ImportingInstance.setBuildGlobalModuleIndex(true);
   }
 
   return Result;
 }
 
-static bool compileAndLoadModule(CompilerInstance &ImportingInstance,
+/// Compile a module in a separate compiler instance and read the AST,
+/// returning true if the module compiles without errors.
+///
+/// Uses a lock file manager and exponential backoff to reduce the chances that
+/// multiple instances will compete to create the same module.  On timeout,
+/// deletes the lock file in order to avoid deadlock from crashing processes or
+/// bugs in the lock file manager.
+static bool compileModuleAndReadAST(CompilerInstance &ImportingInstance,
                                     SourceLocation ImportLoc,
-                                 SourceLocation ModuleNameLoc, Module *Module,
-                                 StringRef ModuleFileName) {
+                                    SourceLocation ModuleNameLoc,
+                                    Module *Module, StringRef ModuleFileName) {
   DiagnosticsEngine &Diags = ImportingInstance.getDiagnostics();
 
   auto diagnoseBuildFailure = [&] {
     Diags.Report(ModuleNameLoc, diag::err_module_not_built)
         << Module->Name << SourceRange(ImportLoc, ModuleNameLoc);
   };
 
   // FIXME: have LockFileManager return an error_code so that we can
case llvm::LockFileManager::LFS_Error:
       // related errors.
       Diags.Report(ModuleNameLoc, diag::remark_module_lock_failure)
           << Module->Name << Locked.getErrorMessage();
       // Clear out any potential leftover.
       Locked.unsafeRemoveLockFile();
       LLVM_FALLTHROUGH;
     case llvm::LockFileManager::LFS_Owned:
       // We're responsible for building the module ourselves.
-      if (!compileModuleImpl(ImportingInstance, ModuleNameLoc, Module,
-                             ModuleFileName)) {
+      if (!compileModule(ImportingInstance, ModuleNameLoc, Module,
         diagnoseBuildFailure();
         return false;
       }
       break;
 
     case llvm::LockFileManager::LFS_Shared:
       // Someone else is responsible for building the module. Wait for them to
       // finish.
case ASTReader::ConfigurationMismatch:
     ListenerRef.markAllUnavailable();
     return true;
 
   default:
     return false;
   }
 }
 
-ModuleLoadResult
-CompilerInstance::loadModule(SourceLocation ImportLoc,
-                             ModuleIdPath Path,
-                             Module::NameVisibilityKind Visibility,
-                             bool IsInclusionDirective) {
-  // Determine what file we're searching from.
-  StringRef ModuleName = Path[0].first->getName();
-  SourceLocation ModuleNameLoc = Path[0].second;
-
-  // If we've already handled this import, just return the cached result.
-  // This one-element cache is important to eliminate redundant diagnostics
-  // when both the preprocessor and parser see the same import declaration.
-  if (ImportLoc.isValid() && LastModuleImportLoc == ImportLoc) {
-    // Make the named module visible.
-    if (LastModuleImportResult && ModuleName != getLangOpts().CurrentModule)
-      ModuleManager->makeModuleVisible(LastModuleImportResult, Visibility,
-                                       ImportLoc);
-    return LastModuleImportResult;
-  }
-
-  // If we don't already have information on this module, load the module now.
-  Module *Module = nullptr;
-  ModuleMap &MM = getPreprocessor().getHeaderSearchInfo().getModuleMap();
-  if (auto MaybeModule = MM.getCachedModuleLoad(*Path[0].first)) {
-    // Use the cached result, which may be nullptr.
-    Module = *MaybeModule;
-  } else if (ModuleName == getLangOpts().CurrentModule) {
-    // This is the module we're building.
-    Module = PP->getHeaderSearchInfo().lookupModule(
-        ModuleName, /*AllowSearch*/ true,
-        /*AllowExtraModuleMapSearch*/ !IsInclusionDirective);
-    /// FIXME: perhaps we should (a) look for a module using the module name
-    //  to file map (PrebuiltModuleFiles) and (b) diagnose if still not found?
-    //if (Module == nullptr) {
-    //  getDiagnostics().Report(ModuleNameLoc, diag::err_module_not_found)
-    //    << ModuleName;
-    //  ModuleBuildFailed = true;
-    //  return ModuleLoadResult();
-    //}
-    MM.cacheModuleLoad(*Path[0].first, Module);
-  } else {
-    // Search for a module with the given name.
-    Module = PP->getHeaderSearchInfo().lookupModule(ModuleName, true,
-                                                    !IsInclusionDirective);
-    HeaderSearchOptions &HSOpts =
-        PP->getHeaderSearchInfo().getHeaderSearchOpts();
-
-    std::string ModuleFileName;
-    enum ModuleSource {
-      ModuleNotFound, ModuleCache, PrebuiltModulePath, ModuleBuildPragma
-    } Source = ModuleNotFound;
+namespace {
+  MS_ModuleNotFound,
+  MS_ModuleCache,
+  MS_PrebuiltModulePath,
+  MS_ModuleBuildPragma
+};
+} // end namespace
+
+/// Select a source for loading the named module and compute the filename to
+/// load it from.
+static ModuleSource
+selectModuleSource(Module *M, StringRef ModuleName, std::string &ModuleFilename,
+                   const std::map<std::string, std::string> &BuiltModules,
+                   HeaderSearch &HS) {
+  assert(ModuleFilename.empty() && "Already has a module source?");
 
-    // Check to see if the module has been built as part of this compilation
-    // via a module build pragma.
-    auto BuiltModuleIt = BuiltModules.find(ModuleName);
-    if (BuiltModuleIt != BuiltModules.end()) {
-      ModuleFileName = BuiltModuleIt->second;
-      Source = ModuleBuildPragma;
-    }
+    ModuleFilename = BuiltModuleIt->second;
+    return MS_ModuleBuildPragma;
 
-    // Try to load the module from the prebuilt module path.
-    if (Source == ModuleNotFound && (!HSOpts.PrebuiltModuleFiles.empty() ||
-                                     !HSOpts.PrebuiltModulePaths.empty())) {
-      ModuleFileName =
-        PP->getHeaderSearchInfo().getPrebuiltModuleFileName(ModuleName);
-      if (!ModuleFileName.empty())
-        Source = PrebuiltModulePath;
-    }
+  const HeaderSearchOptions &HSOpts = HS.getHeaderSearchOpts();
+  if (!HSOpts.PrebuiltModuleFiles.empty() ||
+      !HSOpts.PrebuiltModulePaths.empty()) {
+    ModuleFilename = HS.getPrebuiltModuleFileName(ModuleName);
+    if (!ModuleFilename.empty())
+      return MS_PrebuiltModulePath;
 
-    // Try to load the module from the module cache.
-    if (Source == ModuleNotFound && Module) {
-      ModuleFileName = PP->getHeaderSearchInfo().getCachedModuleFileName(Module);
-      Source = ModuleCache;
-    }
+  if (M) {
+    ModuleFilename = HS.getCachedModuleFileName(M);
+    return MS_ModuleCache;
 
-    if (Source == ModuleNotFound) {
-      // We can't find a module, error out here.
-      getDiagnostics().Report(ModuleNameLoc, diag::err_module_not_found)
-          << ModuleName << SourceRange(ImportLoc, ModuleNameLoc);
-      ModuleBuildFailed = true;
-      return ModuleLoadResult();
-    }
-
-    if (ModuleFileName.empty()) {
-      if (Module && Module->HasIncompatibleModuleFile) {
-        // We tried and failed to load a module file for this module. Fall
-        // back to textual inclusion for its headers.
-        return ModuleLoadResult::ConfigMismatch;
-      }
+  return MS_ModuleNotFound;
+}
+
+ModuleLoadResult CompilerInstance::findOrCompileModuleAndReadAST(
+    StringRef ModuleName, SourceLocation ImportLoc,
+    SourceLocation ModuleNameLoc, bool IsInclusionDirective) {
+  // Search for a module with the given name.
+  HeaderSearch &HS = PP->getHeaderSearchInfo();
+  Module *M = HS.lookupModule(ModuleName, true, !IsInclusionDirective);
+
+  // Select the source and filename for loading the named module.
+  std::string ModuleFilename;
+  ModuleSource Source =
+      selectModuleSource(M, ModuleName, ModuleFilename, BuiltModules, HS);
+  if (Source == MS_ModuleNotFound) {
+    // FIXME: Why is this not cached?
+    return ModuleLoadResult::OtherUncachedFailure;
+  if (ModuleFilename.empty()) {
+    if (M && M->HasIncompatibleModuleFile) {
 
-      getDiagnostics().Report(ModuleNameLoc, diag::err_module_build_disabled)
-          << ModuleName;
-      ModuleBuildFailed = true;
-      return ModuleLoadResult();
-    }
+    // FIXME: Why is this not cached?
+    return ModuleLoadResult::OtherUncachedFailure;
 
-    // If we don't already have an ASTReader, create one now.
-    if (!ModuleManager)
-      createModuleManager();
+  // Create an ASTReader on demand.
+  if (!getModuleManager())
 
-    llvm::Timer Timer;
-    if (FrontendTimerGroup)
-      Timer.init("loading." + ModuleFileName, "Loading " + ModuleFileName,
-                 *FrontendTimerGroup);
-    llvm::TimeRegion TimeLoading(FrontendTimerGroup ? &Timer : nullptr);
-    llvm::TimeTraceScope TimeScope("Module Load", ModuleName);
+  // Time how long it takes to load the module.
+    Timer.init("loading." + ModuleFilename, "Loading " + ModuleFilename,
 
-    // Try to load the module file. If we are not trying to load from the
-    // module cache, we don't know how to rebuild modules.
-    unsigned ARRFlags = Source == ModuleCache ?
-                        ASTReader::ARR_OutOfDate | ASTReader::ARR_Missing :
-                        Source == PrebuiltModulePath ?
-                            0 :
-                            ASTReader::ARR_ConfigurationMismatch;
-    switch (ModuleManager->ReadAST(ModuleFileName,
-                                   Source == PrebuiltModulePath
-                                       ? serialization::MK_PrebuiltModule
-                                       : Source == ModuleBuildPragma
-                                             ? serialization::MK_ExplicitModule
-                                             : serialization::MK_ImplicitModule,
-                                   ImportLoc, ARRFlags)) {
-    case ASTReader::Success: {
-      if (Source != ModuleCache && !Module) {
-        Module = PP->getHeaderSearchInfo().lookupModule(ModuleName, true,
-                                                        !IsInclusionDirective);
-        auto ModuleFile = FileMgr->getFile(ModuleFileName);
-        if (!Module || !Module->getASTFile() ||
-            !ModuleFile || (*ModuleFile != Module->getASTFile())) {
-          // Error out if Module does not refer to the file in the prebuilt
-          // module path.
-          getDiagnostics().Report(ModuleNameLoc, diag::err_module_prebuilt)
-              << ModuleName;
-          ModuleBuildFailed = true;
-          MM.cacheModuleLoad(*Path[0].first, nullptr);
-          return ModuleLoadResult();
-        }
-      }
-      break;
-    }
+  unsigned ARRFlags = Source == MS_ModuleCache
+                          ? ASTReader::ARR_OutOfDate | ASTReader::ARR_Missing
+                          : Source == MS_PrebuiltModulePath
+                                ? 0
+                                : ASTReader::ARR_ConfigurationMismatch;
+  switch (getModuleManager()->ReadAST(
+      ModuleFilename,
+      Source == MS_PrebuiltModulePath
+          : Source == MS_ModuleBuildPragma ? serialization::MK_ExplicitModule
+    if (M)
+      return M;
+    assert(Source != MS_ModuleCache &&
+           "missing module, but file loaded from cache");
+
+    // A prebuilt module is indexed as a ModuleFile; the Module does not exist
+    // until the first call to ReadAST.  Look it up now.
+    M = HS.lookupModule(ModuleName, true, !IsInclusionDirective);
+
+    // Check whether M refers to the file in the prebuilt module path.
+    if (M && M->getASTFile())
+      if (auto ModuleFile = FileMgr->getFile(ModuleFilename))
+        if (*ModuleFile == M->getASTFile())
+          return M;
+
+    ModuleBuildFailed = true;
+
+  case ASTReader::OutOfDate:
+  case ASTReader::Missing:
+    // The most interesting case.
+
+  case ASTReader::ConfigurationMismatch:
+    if (Source == MS_PrebuiltModulePath)
+      // FIXME: We shouldn't be setting HadFatalFailure below if we only
+      // produce a warning here!
+      getDiagnostics().Report(SourceLocation(),
+                              diag::warn_module_config_mismatch)
+          << ModuleFilename;
+    // Fall through to error out.
+    LLVM_FALLTHROUGH;
+  case ASTReader::VersionMismatch:
+  case ASTReader::HadErrors:
+    // FIXME: Should this set ModuleBuildFailed = true?
+    ModuleLoader::HadFatalFailure = true;
+    // FIXME: The ASTReader will already have complained, but can we shoehorn
+    // that diagnostic information into a more useful form?
+    return ModuleLoadResult();
+
+  case ASTReader::Failure:
+    // FIXME: Should this set ModuleBuildFailed = true?
+    ModuleLoader::HadFatalFailure = true;
+    return ModuleLoadResult();
 
-    case ASTReader::OutOfDate:
-    case ASTReader::Missing: {
-      if (Source != ModuleCache) {
-        // We don't know the desired configuration for this module and don't
-        // necessarily even have a module map. Since ReadAST already produces
-        // diagnostics for these two cases, we simply error out here.
-        ModuleBuildFailed = true;
-        MM.cacheModuleLoad(*Path[0].first, nullptr);
-        return ModuleLoadResult();
-      }
+  // ReadAST returned Missing or OutOfDate.
+  if (Source != MS_ModuleCache) {
 
-      // The module file is missing or out-of-date. Build it.
-      assert(Module && "missing module file");
-      // Check whether there is a cycle in the module graph.
-      ModuleBuildStack ModPath = getSourceManager().getModuleBuildStack();
-      ModuleBuildStack::iterator Pos = ModPath.begin(), PosEnd = ModPath.end();
-      for (; Pos != PosEnd; ++Pos) {
-        if (Pos->first == ModuleName)
-          break;
-      }
+  assert(M && "missing module, but trying to compile for cache");
+
 
-      if (Pos != PosEnd) {
-        SmallString<256> CyclePath;
-        for (; Pos != PosEnd; ++Pos) {
-          CyclePath += Pos->first;
-          CyclePath += " -> ";
-        }
-        CyclePath += ModuleName;
 
-        getDiagnostics().Report(ModuleNameLoc, diag::err_module_cycle)
-          << ModuleName << CyclePath;
-        return ModuleLoadResult();
-      }
+    // FIXME: Should this set ModuleBuildFailed = true?
+    // FIXME: Why is this not cached?
+    return ModuleLoadResult::OtherUncachedFailure;
 
-      // Check whether we have already attempted to build this module (but
-      // failed).
-      if (getPreprocessorOpts().FailedModules &&
-          getPreprocessorOpts().FailedModules->hasAlreadyFailed(ModuleName)) {
-        getDiagnostics().Report(ModuleNameLoc, diag::err_module_not_built)
-          << ModuleName
-          << SourceRange(ImportLoc, ModuleNameLoc);
-        ModuleBuildFailed = true;
-        return ModuleLoadResult();
-      }
+        << ModuleName << SourceRange(ImportLoc, ModuleNameLoc);
+    // FIXME: Why is this not cached?
+    return ModuleLoadResult::OtherUncachedFailure;
 
-      // Try to compile and then load the module.
-      if (!compileAndLoadModule(*this, ImportLoc, ModuleNameLoc, Module,
-                                ModuleFileName)) {
-        assert(getDiagnostics().hasErrorOccurred() &&
-               "undiagnosed error in compileAndLoadModule");
-        if (getPreprocessorOpts().FailedModules)
-          getPreprocessorOpts().FailedModules->addFailed(ModuleName);
-        MM.cacheModuleLoad(*Path[0].first, nullptr);
-        ModuleBuildFailed = true;
-        return ModuleLoadResult();
-      }
+  // Try to compile and then read the AST.
+  if (!compileModuleAndReadAST(*this, ImportLoc, ModuleNameLoc, M,
+                               ModuleFilename)) {
+           "undiagnosed error in compileModuleAndReadAST");
+    // FIXME: Why is this not cached?
+    return ModuleLoadResult::OtherUncachedFailure;
 
-      // Okay, we've rebuilt and now loaded the module.
-      break;
-    }
+  return M;
 
-    case ASTReader::ConfigurationMismatch:
-      if (Source == PrebuiltModulePath)
-        // FIXME: We shouldn't be setting HadFatalFailure below if we only
-        // produce a warning here!
-        getDiagnostics().Report(SourceLocation(),
-                                diag::warn_module_config_mismatch)
-            << ModuleFileName;
-      // Fall through to error out.
-      LLVM_FALLTHROUGH;
-    case ASTReader::VersionMismatch:
-    case ASTReader::HadErrors:
-      ModuleLoader::HadFatalFailure = true;
-      // FIXME: The ASTReader will already have complained, but can we shoehorn
-      // that diagnostic information into a more useful form?
-      MM.cacheModuleLoad(*Path[0].first, nullptr);
-      return ModuleLoadResult();
+ModuleLoadResult
+CompilerInstance::loadModule(SourceLocation ImportLoc,
+                             ModuleIdPath Path,
+                             Module::NameVisibilityKind Visibility,
+                             bool IsInclusionDirective) {
+  // Determine what file we're searching from.
+  StringRef ModuleName = Path[0].first->getName();
+  SourceLocation ModuleNameLoc = Path[0].second;
 
-    case ASTReader::Failure:
-      ModuleLoader::HadFatalFailure = true;
-      // Already complained, but note now that we failed.
-      MM.cacheModuleLoad(*Path[0].first, nullptr);
-      ModuleBuildFailed = true;
-      return ModuleLoadResult();
-    }
+  // If we've already handled this import, just return the cached result.
+  // This one-element cache is important to eliminate redundant diagnostics
+  // when both the preprocessor and parser see the same import declaration.
+  if (ImportLoc.isValid() && LastModuleImportLoc == ImportLoc) {
+    // Make the named module visible.
+    if (LastModuleImportResult && ModuleName != getLangOpts().CurrentModule)
+      ModuleManager->makeModuleVisible(LastModuleImportResult, Visibility,
+                                       ImportLoc);
+    return LastModuleImportResult;
 
-    // Cache the result of this top-level module lookup for later.
+  // If we don't already have information on this module, load the module now.
+  Module *Module = nullptr;
+  ModuleMap &MM = getPreprocessor().getHeaderSearchInfo().getModuleMap();
+  if (auto MaybeModule = MM.getCachedModuleLoad(*Path[0].first)) {
+    // Use the cached result, which may be nullptr.
+    Module = *MaybeModule;
+  } else if (ModuleName == getLangOpts().CurrentModule) {
+    // This is the module we're building.
+    Module = PP->getHeaderSearchInfo().lookupModule(
+        ModuleName, /*AllowSearch*/ true,
+        /*AllowExtraModuleMapSearch*/ !IsInclusionDirective);
+    /// FIXME: perhaps we should (a) look for a module using the module name
+    //  to file map (PrebuiltModuleFiles) and (b) diagnose if still not found?
+    //if (Module == nullptr) {
+    //  getDiagnostics().Report(ModuleNameLoc, diag::err_module_not_found)
+    //    << ModuleName;
+    //  ModuleBuildFailed = true;
+    //  return ModuleLoadResult();
+    //}
     MM.cacheModuleLoad(*Path[0].first, Module);
+  } else {
+    ModuleLoadResult Result = findOrCompileModuleAndReadAST(
+        ModuleName, ImportLoc, ModuleNameLoc, IsInclusionDirective);
+    // FIXME: Can we pull 'ModuleBuildFailed = true' out of the return
+    // sequences for findOrCompileModuleAndReadAST and do it here (as long as
+    // the result is not a config mismatch)?  See FIXMEs there.
+    if (!Result.isNormal())
+      return Result;
+    Module = Result;
+    MM.cacheModuleLoad(*Path[0].first, Module);
+    if (!Module)
+      return Module;
   }
 
-  // If we never found the module, fail.
+  // If we never found the module, fail.  Otherwise, verify the module and link
+  // it up.
   if (!Module)
     return ModuleLoadResult();
 
   // Verify that the rest of the module path actually corresponds to
   // a submodule.
   bool MapPrivateSubModToTopLevel = false;
   if (Path.size() > 1) {
     for (unsigned I = 1, N = Path.size(); I != N; ++I) {
getPreprocessor()
       .getModuleMap()
       .resolveLinkAsDependencies(TopModule);
 
   LastModuleImportLoc = ImportLoc;
   LastModuleImportResult = ModuleLoadResult(Module);
   return LastModuleImportResult;
 }
 
-void CompilerInstance::loadModuleFromSource(SourceLocation ImportLoc,
+void CompilerInstance::createModuleFromSource(SourceLocation ImportLoc,
                                               StringRef ModuleName,
                                               StringRef Source) {
   // Avoid creating filenames with special characters.
   SmallString<128> CleanModuleName(ModuleName);
   for (auto &C : CleanModuleName)
     if (!isAlphanumeric(C))
       C = '_';
 
   // FIXME: Using a randomized filename here means that our intermediate .pcm
   // output is nondeterministic (as .pcm files refer to each other by name).

clang/lib/Lex/Pragma.cpp

void Preprocessor::HandlePragmaModuleBuild(Token &Tok) {
   CurLexer->LexingRawMode = false;
 
   // Load the extracted text as a preprocessed module.
   assert(CurLexer->getBuffer().begin() <= Start &&
          Start <= CurLexer->getBuffer().end() &&
          CurLexer->getBuffer().begin() <= End &&
          End <= CurLexer->getBuffer().end() &&
          "module source range not contained within same file buffer");
-  TheModuleLoader.loadModuleFromSource(Loc, ModuleName->getName(),
+  TheModuleLoader.createModuleFromSource(Loc, ModuleName->getName(),
                                          StringRef(Start, End - Start));
 }
 
 void Preprocessor::HandlePragmaHdrstop(Token &Tok) {
   Lex(Tok);
   if (Tok.is(tok::l_paren)) {
     Diag(Tok.getLocation(), diag::warn_pp_hdrstop_filename_ignored);
 
     std::string FileName;