Keep track of modules that have failed to build. If we encounter an

import of that module elsewhere, don't try to build the module again:
it won't work, and the experience is quite dreadful. We track this
information somewhat globally, shared among all of the related
CompilerInvocations used to build modules on-the-fly, so that a
particular Clang instance will only try to build a given module once.

Fixes <rdar://problem/12552849>.




git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@168961 91177308-0d34-0410-b5e6-96231b3b80d8

7 files changed, 135 insertions, 33 deletions

diff --git a/include/clang/Frontend/ASTUnit.h b/include/clang/Frontend/ASTUnit.h
index 5e409bd7ed..ac9a0461b8 100644
--- a/include/clang/Frontend/ASTUnit.h
+++ b/include/clang/Frontend/ASTUnit.h
@@ -830,11 +830,12 @@ public:
   /// \returns True if an error occurred, false otherwise.
   bool serialize(raw_ostream &OS);
   
-  virtual Module *loadModule(SourceLocation ImportLoc, ModuleIdPath Path,
-                             Module::NameVisibilityKind Visibility,
-                             bool IsInclusionDirective) {
+  virtual ModuleLoadResult loadModule(SourceLocation ImportLoc,
+                                      ModuleIdPath Path,
+                                      Module::NameVisibilityKind Visibility,
+                                      bool IsInclusionDirective) {
     // ASTUnit doesn't know how to load modules (not that this matters).
-    return 0;
+    return ModuleLoadResult();
   }
 };
 
diff --git a/include/clang/Frontend/CompilerInstance.h b/include/clang/Frontend/CompilerInstance.h
index 2f3dc3f808..2861511dd1 100644
--- a/include/clang/Frontend/CompilerInstance.h
+++ b/include/clang/Frontend/CompilerInstance.h
@@ -94,7 +94,7 @@ class CompilerInstance : public ModuleLoader {
 
   /// \brief The semantic analysis object.
   OwningPtr<Sema> TheSema;
-  
+
   /// \brief The frontend timer
   OwningPtr<llvm::Timer> FrontendTimer;
 
@@ -111,7 +111,7 @@ class CompilerInstance : public ModuleLoader {
   
   /// \brief The result of the last module import.
   ///
-  Module *LastModuleImportResult;
+  ModuleLoadResult LastModuleImportResult;
   
   /// \brief Holds information about the output file.
   ///
@@ -645,9 +645,10 @@ public:
 
   /// }
   
-  virtual Module *loadModule(SourceLocation ImportLoc, ModuleIdPath Path,
-                             Module::NameVisibilityKind Visibility,
-                             bool IsInclusionDirective);
+  virtual ModuleLoadResult loadModule(SourceLocation ImportLoc,
+                                      ModuleIdPath Path,
+                                      Module::NameVisibilityKind Visibility,
+                                      bool IsInclusionDirective);
 };
 
 } // end namespace clang
diff --git a/include/clang/Lex/ModuleLoader.h b/include/clang/Lex/ModuleLoader.h
index 36d03c0aa2..3a5f41d510 100644
--- a/include/clang/Lex/ModuleLoader.h
+++ b/include/clang/Lex/ModuleLoader.h
@@ -17,16 +17,37 @@
 #include "clang/Basic/Module.h"
 #include "clang/Basic/SourceLocation.h"
 #include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/PointerIntPair.h"
 
 namespace clang {
 
 class IdentifierInfo;
-  
+class Module;
+
 /// \brief A sequence of identifier/location pairs used to describe a particular
 /// module or submodule, e.g., std.vector.
 typedef llvm::ArrayRef<std::pair<IdentifierInfo*, SourceLocation> > 
   ModuleIdPath;
-  
+
+/// \brief Describes the result of attempting to load a module.
+class ModuleLoadResult {
+  llvm::PointerIntPair<Module *, 1, bool> Storage;
+
+public:
+  ModuleLoadResult() : Storage() { }
+
+  ModuleLoadResult(Module *module, bool missingExpected)
+    : Storage(module, missingExpected) { }
+
+  operator Module *() const { return Storage.getPointer(); }
+
+  /// \brief 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(); }
+};
+
 /// \brief Abstract interface for a module loader.
 ///
 /// This abstract interface describes a module loader, which is responsible
@@ -55,9 +76,10 @@ public:
   ///
   /// \returns If successful, returns the loaded module. Otherwise, returns 
   /// NULL to indicate that the module could not be loaded.
-  virtual Module *loadModule(SourceLocation ImportLoc, ModuleIdPath Path,
-                             Module::NameVisibilityKind Visibility,
-                             bool IsInclusionDirective) = 0;
+  virtual ModuleLoadResult loadModule(SourceLocation ImportLoc,
+                                      ModuleIdPath Path,
+                                      Module::NameVisibilityKind Visibility,
+                                      bool IsInclusionDirective) = 0;
 };
   
 }
diff --git a/include/clang/Lex/PreprocessorOptions.h b/include/clang/Lex/PreprocessorOptions.h
index e5fe373793..857161ddc0 100644
--- a/include/clang/Lex/PreprocessorOptions.h
+++ b/include/clang/Lex/PreprocessorOptions.h
@@ -13,6 +13,7 @@
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringSet.h"
 #include <cassert>
 #include <string>
 #include <utility>
@@ -126,7 +127,29 @@ public:
   /// to do so (e.g., if on-demand module construction moves out-of-process),
   /// we can add a cc1-level option to do so.
   SmallVector<std::string, 2> ModuleBuildPath;
+
+  /// \brief Records the set of modules
+  class FailedModulesSet : public llvm::RefCountedBase<FailedModulesSet> {
+    llvm::StringSet<> Failed;
+
+  public:
+    bool hasAlreadyFailed(StringRef module) {
+      return Failed.count(module) > 0;
+    }
+
+    void addFailed(StringRef module) {
+      Failed.insert(module);
+    }
+  };
   
+  /// \brief The set of modules that failed to build.
+  ///
+  /// This pointer will be shared 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.
+  llvm::IntrusiveRefCntPtr<FailedModulesSet> FailedModules;
+
   typedef std::vector<std::pair<std::string, std::string> >::iterator
     remapped_file_iterator;
   typedef std::vector<std::pair<std::string, std::string> >::const_iterator
diff --git a/lib/Frontend/CompilerInstance.cpp b/lib/Frontend/CompilerInstance.cpp
index b01a3f6e1a..f87a635420 100644
--- a/lib/Frontend/CompilerInstance.cpp
+++ b/lib/Frontend/CompilerInstance.cpp
@@ -801,6 +801,15 @@ static void compileModule(CompilerInstance &ImportingInstance,
   // can detect cycles in the module graph.
   PPOpts.ModuleBuildPath.push_back(Module->getTopLevelModuleName());
 
+  // Make sure that the failed-module structure has been allocated in
+  // the importing instance, and propagate the pointer to the newly-created
+  // instance.
+  PreprocessorOptions &ImportingPPOpts
+    = ImportingInstance.getInvocation().getPreprocessorOpts();
+  if (!ImportingPPOpts.FailedModules)
+    ImportingPPOpts.FailedModules = new PreprocessorOptions::FailedModulesSet;
+  PPOpts.FailedModules = ImportingPPOpts.FailedModules;
+
   // If there is a module map file, build the module using the module map.
   // Set up the inputs/outputs so that we build the module from its umbrella
   // header.
@@ -872,10 +881,11 @@ static void compileModule(CompilerInstance &ImportingInstance,
     llvm::sys::Path(TempModuleMapFileName).eraseFromDisk();
 }
 
-Module *CompilerInstance::loadModule(SourceLocation ImportLoc, 
-                                     ModuleIdPath Path,
-                                     Module::NameVisibilityKind Visibility,
-                                     bool IsInclusionDirective) {
+ModuleLoadResult
+CompilerInstance::loadModule(SourceLocation ImportLoc,
+                             ModuleIdPath Path,
+                             Module::NameVisibilityKind Visibility,
+                             bool IsInclusionDirective) {
   // 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.
@@ -910,14 +920,14 @@ Module *CompilerInstance::loadModule(SourceLocation ImportLoc,
       ModuleFileName = PP->getHeaderSearchInfo().getModuleFileName(Module);
     else
       ModuleFileName = PP->getHeaderSearchInfo().getModuleFileName(ModuleName);
-    
+
     if (ModuleFileName.empty()) {
       getDiagnostics().Report(ModuleNameLoc, diag::err_module_not_found)
         << ModuleName
         << SourceRange(ImportLoc, ModuleNameLoc);
       LastModuleImportLoc = ImportLoc;
-      LastModuleImportResult = 0;
-      return 0;
+      LastModuleImportResult = ModuleLoadResult();
+      return LastModuleImportResult;
     }
     
     const FileEntry *ModuleFile
@@ -943,7 +953,18 @@ Module *CompilerInstance::loadModule(SourceLocation ImportLoc,
 
         getDiagnostics().Report(ModuleNameLoc, diag::err_module_cycle)
           << ModuleName << CyclePath;
-        return 0;
+        return ModuleLoadResult();
+      }
+
+      // 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);
+
+        return ModuleLoadResult();
       }
 
       getDiagnostics().Report(ModuleNameLoc, diag::warn_module_build)
@@ -951,6 +972,9 @@ Module *CompilerInstance::loadModule(SourceLocation ImportLoc,
       BuildingModule = true;
       compileModule(*this, Module, ModuleFileName);
       ModuleFile = FileMgr->getFile(ModuleFileName);
+
+      if (!ModuleFile)
+        getPreprocessorOpts().FailedModules->addFailed(ModuleName);
     }
 
     if (!ModuleFile) {
@@ -959,7 +983,7 @@ Module *CompilerInstance::loadModule(SourceLocation ImportLoc,
                                             : diag::err_module_not_found)
         << ModuleName
         << SourceRange(ImportLoc, ModuleNameLoc);
-      return 0;
+      return ModuleLoadResult();
     }
 
     // If we don't already have an ASTReader, create one now.
@@ -1004,6 +1028,18 @@ Module *CompilerInstance::loadModule(SourceLocation ImportLoc,
       getFileManager().invalidateCache(ModuleFile);
       bool Existed;
       llvm::sys::fs::remove(ModuleFileName, Existed);
+
+      // 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);
+
+        return ModuleLoadResult();
+      }
+
       compileModule(*this, Module, ModuleFileName);
 
       // Try loading the module again.
@@ -1012,8 +1048,9 @@ Module *CompilerInstance::loadModule(SourceLocation ImportLoc,
           ModuleManager->ReadAST(ModuleFileName,
                                  serialization::MK_Module, ImportLoc,
                                  ASTReader::ARR_None) != ASTReader::Success) {
+        getPreprocessorOpts().FailedModules->addFailed(ModuleName);
         KnownModules[Path[0].first] = 0;
-        return 0;
+        return ModuleLoadResult();
       }
 
       // Okay, we've rebuilt and now loaded the module.
@@ -1026,12 +1063,12 @@ Module *CompilerInstance::loadModule(SourceLocation ImportLoc,
       // FIXME: The ASTReader will already have complained, but can we showhorn
       // that diagnostic information into a more useful form?
       KnownModules[Path[0].first] = 0;
-      return 0;
+      return ModuleLoadResult();
 
     case ASTReader::Failure:
       // Already complained, but note now that we failed.
       KnownModules[Path[0].first] = 0;
-      return 0;
+      return ModuleLoadResult();
     }
     
     if (!Module) {
@@ -1050,7 +1087,7 @@ Module *CompilerInstance::loadModule(SourceLocation ImportLoc,
   
   // If we never found the module, fail.
   if (!Module)
-    return 0;
+    return ModuleLoadResult();
   
   // Verify that the rest of the module path actually corresponds to
   // a submodule.
@@ -1120,7 +1157,7 @@ Module *CompilerInstance::loadModule(SourceLocation ImportLoc,
         << Module->getFullModuleName()
         << SourceRange(Path.front().second, Path.back().second);
       
-      return 0;
+      return ModuleLoadResult(0, true);
     }
 
     // Check whether this module is available.
@@ -1131,8 +1168,8 @@ Module *CompilerInstance::loadModule(SourceLocation ImportLoc,
         << Feature
         << SourceRange(Path.front().second, Path.back().second);
       LastModuleImportLoc = ImportLoc;
-      LastModuleImportResult = 0;
-      return 0;
+      LastModuleImportResult = ModuleLoadResult();
+      return ModuleLoadResult();
     }
 
     ModuleManager->makeModuleVisible(Module, Visibility);
@@ -1151,6 +1188,6 @@ Module *CompilerInstance::loadModule(SourceLocation ImportLoc,
   }
   
   LastModuleImportLoc = ImportLoc;
-  LastModuleImportResult = Module;
-  return Module;
+  LastModuleImportResult = ModuleLoadResult(Module, false);
+  return LastModuleImportResult;
 }
diff --git a/lib/Lex/PPDirectives.cpp b/lib/Lex/PPDirectives.cpp
index 50b161a172..89490a3f23 100644
--- a/lib/Lex/PPDirectives.cpp
+++ b/lib/Lex/PPDirectives.cpp
@@ -1483,7 +1483,7 @@ void Preprocessor::HandleIncludeDirective(SourceLocation HashLoc,
     // If this was an #__include_macros directive, only make macros visible.
     Module::NameVisibilityKind Visibility 
       = (IncludeKind == 3)? Module::MacrosVisible : Module::AllVisible;
-    Module *Imported
+    ModuleLoadResult Imported
       = TheModuleLoader.loadModule(IncludeTok.getLocation(), Path, Visibility,
                                    /*IsIncludeDirective=*/true);
     assert((Imported == 0 || Imported == SuggestedModule) &&
@@ -1498,6 +1498,13 @@ void Preprocessor::HandleIncludeDirective(SourceLocation HashLoc,
       }
       return;
     }
+
+    // If we failed to find a submodule that we expected to find, we can
+    // continue. Otherwise, there's an error in the included file, so we
+    // don't want to include it.
+    if (!BuildingImportedModule && !Imported.isMissingExpected()) {
+      return;
+    }
   }
 
   if (Callbacks && SuggestedModule) {
diff --git a/test/Modules/epic-fail.m b/test/Modules/epic-fail.m
new file mode 100644
index 0000000000..7d2cdcf602
--- /dev/null
+++ b/test/Modules/epic-fail.m
@@ -0,0 +1,11 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodule-cache-path %t -fmodules -F %S/Inputs -DgetModuleVersion="epic fail" %s 2>&1 | FileCheck %s
+
+@__experimental_modules_import Module;
+@__experimental_modules_import DependsOnModule;
+
+// CHECK: error: expected ';' after top level declarator
+// CHECK: note: expanded from macro 'getModuleVersion'
+// CHECK: fatal error: could not build module 'Module'
+// CHECK: fatal error: could not build module 'Module'
+// CHECK-NOT: error: