Verify all the module map files for a pcm are the same on load

We already verified the primary module map file (either the one that
defines the top-level module, or the one that allows inferring it if it
is an inferred framework module). Now we also verify any other module
map files that define submodules, such as when there is a
module.private.modulemap file.

llvm-svn: 215455

10 files changed, 183 insertions, 57 deletions

diff --git a/clang/include/clang/Basic/DiagnosticSerializationKinds.td b/clang/include/clang/Basic/DiagnosticSerializationKinds.td
index be9d2bdbd2d..1c81fc60906 100644
--- a/clang/include/clang/Basic/DiagnosticSerializationKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSerializationKinds.td
@@ -56,6 +56,9 @@ def err_imported_module_not_found : Error<
 def err_imported_module_modmap_changed : Error<
     "module '%0' imported by AST file '%1' found in a different module map file"
     " (%2) than when the importing AST file was built (%3)">, DefaultFatal;
+def err_module_different_modmap : Error<
+    "module '%0' %select{uses|does not use}1 additional module map '%2'"
+    "%select{| not}1 used when the module was built">;
 def warn_module_conflict : Warning<
     "module '%0' conflicts with already-imported module '%1': %2">, 
     InGroup<ModuleConflict>;
diff --git a/clang/include/clang/Lex/ModuleMap.h b/clang/include/clang/Lex/ModuleMap.h
index d37f8e833aa..d92272bbe5c 100644
--- a/clang/include/clang/Lex/ModuleMap.h
+++ b/clang/include/clang/Lex/ModuleMap.h
@@ -107,6 +107,8 @@ public:
     }
   };
 
+  typedef llvm::SmallPtrSet<const FileEntry *, 1> AdditionalModMapsSet;
+
 private:
   typedef llvm::DenseMap<const FileEntry *, SmallVector<KnownHeader, 1> >
   HeadersMap;
@@ -150,6 +152,8 @@ private:
   /// inference.
   llvm::DenseMap<const Module *, const FileEntry *> InferredModuleAllowedBy;
 
+  llvm::DenseMap<const Module *, AdditionalModMapsSet> AdditionalModMaps;
+
   /// \brief Describes whether we haved parsed a particular file as a module
   /// map.
   llvm::DenseMap<const FileEntry *, bool> ParsedModuleMap;
@@ -349,7 +353,7 @@ public:
   ///
   /// \returns The file entry for the module map file containing the given
   /// module, or NULL if the module definition was inferred.
-  const FileEntry *getContainingModuleMapFile(Module *Module) const;
+  const FileEntry *getContainingModuleMapFile(const Module *Module) const;
 
   /// \brief Get the module map file that (along with the module name) uniquely
   /// identifies this module.
@@ -360,10 +364,25 @@ public:
   /// of inferred modules, returns the module map that allowed the inference
   /// (e.g. contained 'module *'). Otherwise, returns
   /// getContainingModuleMapFile().
-  const FileEntry *getModuleMapFileForUniquing(Module *M) const;
+  const FileEntry *getModuleMapFileForUniquing(const Module *M) const;
 
   void setInferredModuleAllowedBy(Module *M, const FileEntry *ModuleMap);
 
+  /// \brief Get any module map files other than getModuleMapFileForUniquing(M)
+  /// that define submodules of a top-level module \p M. This is cheaper than
+  /// getting the module map file for each submodule individually, since the
+  /// expected number of results is very small.
+  AdditionalModMapsSet *getAdditionalModuleMapFiles(const Module *M) {
+    auto I = AdditionalModMaps.find(M);
+    if (I == AdditionalModMaps.end())
+      return nullptr;
+    return &I->second;
+  }
+
+  void addAdditionalModuleMapFile(const Module *M, const FileEntry *ModuleMap) {
+    AdditionalModMaps[M].insert(ModuleMap);
+  }
+
   /// \brief Resolve all of the unresolved exports in the given module.
   ///
   /// \param Mod The module whose exports should be resolved.
diff --git a/clang/include/clang/Serialization/ASTReader.h b/clang/include/clang/Serialization/ASTReader.h
index c8701c4fbc7..213eacdfbc5 100644
--- a/clang/include/clang/Serialization/ASTReader.h
+++ b/clang/include/clang/Serialization/ASTReader.h
@@ -1117,6 +1117,9 @@ private:
   bool ReadSourceManagerBlock(ModuleFile &F);
   llvm::BitstreamCursor &SLocCursorForID(int ID);
   SourceLocation getImportLocation(ModuleFile *F);
+  ASTReadResult ReadModuleMapFileBlock(RecordData &Record, ModuleFile &F,
+                                       const ModuleFile *ImportedBy,
+                                       unsigned ClientLoadCapabilities);
   ASTReadResult ReadSubmoduleBlock(ModuleFile &F,
                                    unsigned ClientLoadCapabilities);
   static bool ParseLanguageOptions(const RecordData &Record, bool Complain,
diff --git a/clang/lib/Lex/ModuleMap.cpp b/clang/lib/Lex/ModuleMap.cpp
index 92de71d8f37..0af63e1bf44 100644
--- a/clang/lib/Lex/ModuleMap.cpp
+++ b/clang/lib/Lex/ModuleMap.cpp
@@ -797,7 +797,7 @@ void ModuleMap::addHeader(Module *Mod, const FileEntry *Header,
 }
 
 const FileEntry *
-ModuleMap::getContainingModuleMapFile(Module *Module) const {
+ModuleMap::getContainingModuleMapFile(const Module *Module) const {
   if (Module->DefinitionLoc.isInvalid())
     return nullptr;
 
@@ -805,7 +805,7 @@ ModuleMap::getContainingModuleMapFile(Module *Module) const {
            SourceMgr.getFileID(Module->DefinitionLoc));
 }
 
-const FileEntry *ModuleMap::getModuleMapFileForUniquing(Module *M) const {
+const FileEntry *ModuleMap::getModuleMapFileForUniquing(const Module *M) const {
   if (M->IsInferred) {
     assert(InferredModuleAllowedBy.count(M) && "missing inferred module map");
     return InferredModuleAllowedBy.find(M)->second;
@@ -1347,8 +1347,11 @@ void ModuleMapParser::parseModuleDecl() {
     // This module map defines a submodule. Go find the module of which it
     // is a submodule.
     ActiveModule = nullptr;
+    const Module *TopLevelModule = nullptr;
     for (unsigned I = 0, N = Id.size() - 1; I != N; ++I) {
       if (Module *Next = Map.lookupModuleQualified(Id[I].first, ActiveModule)) {
+        if (I == 0)
+          TopLevelModule = Next;
         ActiveModule = Next;
         continue;
       }
@@ -1363,7 +1366,14 @@ void ModuleMapParser::parseModuleDecl() {
       HadError = true;
       return;
     }
-  } 
+
+    if (ModuleMapFile != Map.getContainingModuleMapFile(TopLevelModule)) {
+      assert(ModuleMapFile != Map.getModuleMapFileForUniquing(TopLevelModule) &&
+             "submodule defined in same file as 'module *' that allowed its "
+             "top-level module");
+      Map.addAdditionalModuleMapFile(TopLevelModule, ModuleMapFile);
+    }
+  }
   
   StringRef ModuleName = Id.back().first;
   SourceLocation ModuleNameLoc = Id.back().second;
diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index bc7778adc6c..1cce383f2d0 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -2468,45 +2468,9 @@ ASTReader::ReadControlBlock(ModuleFile &F,
       break;
 
     case MODULE_MAP_FILE:
-      F.ModuleMapPath = Blob;
-
-      // Try to resolve ModuleName in the current header search context and
-      // verify that it is found in the same module map file as we saved. If the
-      // top-level AST file is a main file, skip this check because there is no
-      // usable header search context.
-      assert(!F.ModuleName.empty() &&
-             "MODULE_NAME should come before MOUDLE_MAP_FILE");
-      if (F.Kind == MK_Module &&
-          (*ModuleMgr.begin())->Kind != MK_MainFile) {
-        Module *M = PP.getHeaderSearchInfo().lookupModule(F.ModuleName);
-        if (!M) {
-          assert(ImportedBy && "top-level import should be verified");
-          if ((ClientLoadCapabilities & ARR_Missing) == 0)
-            Diag(diag::err_imported_module_not_found)
-              << F.ModuleName << ImportedBy->FileName;
-          return Missing;
-        }
-
-        HeaderSearch &HS = PP.getHeaderSearchInfo();
-        const FileEntry *StoredModMap = FileMgr.getFile(F.ModuleMapPath);
-        const FileEntry *ModMap =
-            HS.getModuleMap().getModuleMapFileForUniquing(M);
-        if (StoredModMap == nullptr || StoredModMap != ModMap) {
-          assert(ModMap && "found module is missing module map file");
-          assert(M->Name == F.ModuleName && "found module with different name");
-          assert(ImportedBy && "top-level import should be verified");
-          if ((ClientLoadCapabilities & ARR_OutOfDate) == 0)
-            Diag(diag::err_imported_module_modmap_changed)
-              << F.ModuleName << ImportedBy->FileName
-              << ModMap->getName() << F.ModuleMapPath;
-          return OutOfDate;
-        }
-      }
-
-      if (Listener)
-        Listener->ReadModuleMapFile(F.ModuleMapPath);
-      break;
-
+      if (ASTReadResult Result =
+              ReadModuleMapFileBlock(Record, F, ImportedBy, ClientLoadCapabilities))
+        return Result;
     case INPUT_FILE_OFFSETS:
       F.InputFileOffsets = (const uint32_t *)Blob.data();
       F.InputFilesLoaded.resize(Record[0]);
@@ -3315,6 +3279,89 @@ ASTReader::ReadASTBlock(ModuleFile &F, unsigned ClientLoadCapabilities) {
   }
 }
 
+ASTReader::ASTReadResult
+ASTReader::ReadModuleMapFileBlock(RecordData &Record, ModuleFile &F,
+                                  const ModuleFile *ImportedBy,
+                                  unsigned ClientLoadCapabilities) {
+  unsigned Idx = 0;
+  F.ModuleMapPath = ReadString(Record, Idx);
+
+  // Try to resolve ModuleName in the current header search context and
+  // verify that it is found in the same module map file as we saved. If the
+  // top-level AST file is a main file, skip this check because there is no
+  // usable header search context.
+  assert(!F.ModuleName.empty() &&
+         "MODULE_NAME should come before MOUDLE_MAP_FILE");
+  if (F.Kind == MK_Module && (*ModuleMgr.begin())->Kind != MK_MainFile) {
+    Module *M = PP.getHeaderSearchInfo().lookupModule(F.ModuleName);
+    if (!M) {
+      assert(ImportedBy && "top-level import should be verified");
+      if ((ClientLoadCapabilities & ARR_Missing) == 0)
+        Diag(diag::err_imported_module_not_found)
+          << F.ModuleName << ImportedBy->FileName;
+      return Missing;
+    }
+
+    // Check the primary module map file.
+    auto &Map = PP.getHeaderSearchInfo().getModuleMap();
+    const FileEntry *StoredModMap = FileMgr.getFile(F.ModuleMapPath);
+    const FileEntry *ModMap = Map.getModuleMapFileForUniquing(M);
+    if (StoredModMap == nullptr || StoredModMap != ModMap) {
+      assert(ModMap && "found module is missing module map file");
+      assert(M->Name == F.ModuleName && "found module with different name");
+      assert(ImportedBy && "top-level import should be verified");
+      if ((ClientLoadCapabilities & ARR_OutOfDate) == 0)
+        Diag(diag::err_imported_module_modmap_changed)
+          << F.ModuleName << ImportedBy->FileName
+          << ModMap->getName() << F.ModuleMapPath;
+      return OutOfDate;
+    }
+
+    llvm::SmallPtrSet<const FileEntry *, 1> AdditionalStoredMaps;
+    for (unsigned I = 0, N = Record[Idx++]; I < N; ++I) {
+      // FIXME: we should use input files rather than storing names.
+      std::string Filename = ReadString(Record, Idx);
+      const FileEntry *F =
+          FileMgr.getFile(Filename, false, false);
+      if (F == nullptr) {
+        if ((ClientLoadCapabilities & ARR_OutOfDate) == 0)
+          Error("could not find file '" + Filename +"' referenced by AST file");
+        return OutOfDate;
+      }
+      AdditionalStoredMaps.insert(F);
+    }
+
+    // Check any additional module map files (e.g. module.private.modulemap)
+    // that are not in the pcm.
+    if (auto *AdditionalModuleMaps = Map.getAdditionalModuleMapFiles(M)) {
+      for (const FileEntry *ModMap : *AdditionalModuleMaps) {
+        // Remove files that match
+        // Note: SmallPtrSet::erase is really remove
+        if (!AdditionalStoredMaps.erase(ModMap)) {
+          if ((ClientLoadCapabilities & ARR_OutOfDate) == 0)
+            Diag(diag::err_module_different_modmap)
+              << F.ModuleName << /*new*/0 << ModMap->getName();
+          return OutOfDate;
+        }
+      }
+    }
+
+    // Check any additional module map files that are in the pcm, but not
+    // found in header search. Cases that match are already removed.
+    for (const FileEntry *ModMap : AdditionalStoredMaps) {
+      if ((ClientLoadCapabilities & ARR_OutOfDate) == 0)
+        Diag(diag::err_module_different_modmap)
+          << F.ModuleName << /*not new*/1 << ModMap->getName();
+      return OutOfDate;
+    }
+  }
+
+  if (Listener)
+    Listener->ReadModuleMapFile(F.ModuleMapPath);
+  return Success;
+}
+
+
 /// \brief Move the given method to the back of the global list of methods.
 static void moveMethodToBackOfGlobalList(Sema &S, ObjCMethodDecl *Method) {
   // Find the entry for this selector in the method pool.
@@ -4136,9 +4183,11 @@ bool ASTReader::readASTFileControlBlock(StringRef Filename,
     case MODULE_NAME:
       Listener.ReadModuleName(Blob);
       break;
-    case MODULE_MAP_FILE:
-      Listener.ReadModuleMapFile(Blob);
+    case MODULE_MAP_FILE: {
+      unsigned Idx = 0;
+      Listener.ReadModuleMapFile(ReadString(Record, Idx));
       break;
+    }
     case LANGUAGE_OPTIONS:
       if (ParseLanguageOptions(Record, false, Listener))
         return true;
diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp
index dadbd12b23d..ccd57086c38 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/lib/Serialization/ASTWriter.cpp
@@ -1134,18 +1134,28 @@ void ASTWriter::WriteControlBlock(Preprocessor &PP, ASTContext &Context,
 
   // Module map file
   if (WritingModule) {
-    BitCodeAbbrev *Abbrev = new BitCodeAbbrev();
-    Abbrev->Add(BitCodeAbbrevOp(MODULE_MAP_FILE));
-    Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Blob)); // Filename
-    unsigned AbbrevCode = Stream.EmitAbbrev(Abbrev);
+    Record.clear();
+    auto addModMap = [&](const FileEntry *F) {
+      SmallString<128> ModuleMap(F->getName());
+      llvm::sys::fs::make_absolute(ModuleMap);
+      AddString(ModuleMap.str(), Record);
+    };
 
-    const FileEntry *ModMapFile = PP.getHeaderSearchInfo().getModuleMap().
-        getModuleMapFileForUniquing(WritingModule);
-    SmallString<128> ModuleMap(ModMapFile->getName());
-    llvm::sys::fs::make_absolute(ModuleMap);
-    RecordData Record;
-    Record.push_back(MODULE_MAP_FILE);
-    Stream.EmitRecordWithBlob(AbbrevCode, Record, ModuleMap.str());
+    auto &Map = PP.getHeaderSearchInfo().getModuleMap();
+
+    // Primary module map file.
+    addModMap(Map.getModuleMapFileForUniquing(WritingModule));
+
+    // Additional module map files.
+    if (auto *AdditionalModMaps = Map.getAdditionalModuleMapFiles(WritingModule)) {
+      Record.push_back(AdditionalModMaps->size());
+      for (const FileEntry *F : *AdditionalModMaps)
+        addModMap(F);
+    } else {
+      Record.push_back(0);
+    }
+
+    Stream.EmitRecord(MODULE_MAP_FILE, Record);
   }
 
   // Imports
diff --git a/clang/test/Modules/Inputs/AddRemovePrivate.framework/Headers/AddRemovePrivate.h b/clang/test/Modules/Inputs/AddRemovePrivate.framework/Headers/AddRemovePrivate.h
new file mode 100644
index 00000000000..3ab77431aa2
--- /dev/null
+++ b/clang/test/Modules/Inputs/AddRemovePrivate.framework/Headers/AddRemovePrivate.h
@@ -0,0 +1 @@
+// AddRemovePrivate.h
diff --git a/clang/test/Modules/Inputs/AddRemovePrivate.framework/Modules/module.modulemap b/clang/test/Modules/Inputs/AddRemovePrivate.framework/Modules/module.modulemap
new file mode 100644
index 00000000000..7d84297a970
--- /dev/null
+++ b/clang/test/Modules/Inputs/AddRemovePrivate.framework/Modules/module.modulemap
@@ -0,0 +1 @@
+framework module AddRemovePrivate { umbrella header "AddRemovePrivate.h" }
diff --git a/clang/test/Modules/Inputs/AddRemovePrivate.framework/Modules/module.private.modulemap b/clang/test/Modules/Inputs/AddRemovePrivate.framework/Modules/module.private.modulemap
new file mode 100644
index 00000000000..69b67c2cee7
--- /dev/null
+++ b/clang/test/Modules/Inputs/AddRemovePrivate.framework/Modules/module.private.modulemap
@@ -0,0 +1 @@
+explicit module AddRemovePrivate.Private { }
diff --git a/clang/test/Modules/add-remove-private.m b/clang/test/Modules/add-remove-private.m
new file mode 100644
index 00000000000..7aa1b39fc22
--- /dev/null
+++ b/clang/test/Modules/add-remove-private.m
@@ -0,0 +1,29 @@
+// REQUIRES: shell
+// RUN: rm -rf %t
+// RUN: rm -rf %t.mcp
+// RUN: mkdir -p %t
+// RUN: cp -r %S/Inputs/AddRemovePrivate.framework %t/AddRemovePrivate.framework
+
+// Build with module.private.modulemap
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t.mcp -fdisable-module-hash -F %t %s -verify -DP
+// RUN: cp %t.mcp/AddRemovePrivate.pcm %t/with.pcm
+
+// Build without module.private.modulemap
+// RUN: rm %t/AddRemovePrivate.framework/Modules/module.private.modulemap
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t.mcp -fdisable-module-hash -F %t %s -verify
+// RUN: not diff %t.mcp/AddRemovePrivate.pcm %t/with.pcm
+// RUN: cp %t.mcp/AddRemovePrivate.pcm %t/without.pcm
+// RUN: not %clang_cc1 -fmodules -fmodules-cache-path=%t.mcp -fdisable-module-hash -F %t %s -DP 2>&1 | FileCheck %s
+// CHECK: no submodule named 'Private'
+
+// Build with module.private.modulemap (again)
+// RUN: cp %S/Inputs/AddRemovePrivate.framework/Modules/module.private.modulemap %t/AddRemovePrivate.framework/Modules/module.private.modulemap
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t.mcp -fdisable-module-hash -F %t %s -verify -DP
+// RUN: not diff %t.mcp/AddRemovePrivate.pcm %t/without.pcm
+
+// expected-no-diagnostics
+
+@import AddRemovePrivate;
+#ifdef P
+@import AddRemovePrivate.Private;
+#endif