Avoid invalidating successfully loaded module files

Successfully loaded module files may be referenced in other
ModuleManagers, so don't invalidate them. Two related things are fixed:

1) I thought the last module in the manager was always the one that
failed, but it isn't.  So check explicitly against the list of
vetted modules from ReadASTCore.

2) We now keep the file descriptor of pcm file open, which avoids the
possibility of having two different pcms for the same module loaded when
building in parallel with headers being modified during a build.

<rdar://problem/16835846>

llvm-svn: 211330

5 files changed, 33 insertions, 27 deletions

diff --git a/clang/include/clang/Basic/FileManager.h b/clang/include/clang/Basic/FileManager.h
index 023433b25be..f0f0d2f2e19 100644
--- a/clang/include/clang/Basic/FileManager.h
+++ b/clang/include/clang/Basic/FileManager.h
@@ -242,7 +242,8 @@ public:
   /// MemoryBuffer if successful, otherwise returning null.
   llvm::MemoryBuffer *getBufferForFile(const FileEntry *Entry,
                                        std::string *ErrorStr = nullptr,
-                                       bool isVolatile = false);
+                                       bool isVolatile = false,
+                                       bool ShouldCloseOpenFile = true);
   llvm::MemoryBuffer *getBufferForFile(StringRef Filename,
                                        std::string *ErrorStr = nullptr);
 
diff --git a/clang/include/clang/Serialization/ModuleManager.h b/clang/include/clang/Serialization/ModuleManager.h
index 08c1735ab01..3259902222d 100644
--- a/clang/include/clang/Serialization/ModuleManager.h
+++ b/clang/include/clang/Serialization/ModuleManager.h
@@ -18,6 +18,7 @@
 #include "clang/Basic/FileManager.h"
 #include "clang/Serialization/Module.h"
 #include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/SmallPtrSet.h"
 
 namespace clang { 
 
@@ -194,6 +195,7 @@ public:
 
   /// \brief Remove the given set of modules.
   void removeModules(ModuleIterator first, ModuleIterator last,
+                     llvm::SmallPtrSetImpl<ModuleFile *> &LoadedSuccessfully,
                      ModuleMap *modMap);
 
   /// \brief Add an in-memory buffer the list of known buffers
diff --git a/clang/lib/Basic/FileManager.cpp b/clang/lib/Basic/FileManager.cpp
index aa06020be61..17622c7e13f 100644
--- a/clang/lib/Basic/FileManager.cpp
+++ b/clang/lib/Basic/FileManager.cpp
@@ -388,7 +388,7 @@ void FileManager::FixupRelativePath(SmallVectorImpl<char> &path) const {
 
 llvm::MemoryBuffer *FileManager::
 getBufferForFile(const FileEntry *Entry, std::string *ErrorStr,
-                 bool isVolatile) {
+                 bool isVolatile, bool ShouldCloseOpenFile) {
   std::unique_ptr<llvm::MemoryBuffer> Result;
   std::error_code ec;
 
@@ -405,7 +405,10 @@ getBufferForFile(const FileEntry *Entry, std::string *ErrorStr,
                                 /*RequiresNullTerminator=*/true, isVolatile);
     if (ErrorStr)
       *ErrorStr = ec.message();
-    Entry->closeFile();
+    // FIXME: we need a set of APIs that can make guarantees about whether a
+    // FileEntry is open or not.
+    if (ShouldCloseOpenFile)
+      Entry->closeFile();
     return Result.release();
   }
 
diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index 0c9f263faf1..777ea831981 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -3464,8 +3464,13 @@ ASTReader::ASTReadResult ASTReader::ReadAST(const std::string &FileName,
   case OutOfDate:
   case VersionMismatch:
   case ConfigurationMismatch:
-  case HadErrors:
+  case HadErrors: {
+    llvm::SmallPtrSet<ModuleFile *, 4> LoadedSet;
+    for (const ImportedModule &IM : Loaded)
+      LoadedSet.insert(IM.Mod);
+
     ModuleMgr.removeModules(ModuleMgr.begin() + NumModules, ModuleMgr.end(),
+                            LoadedSet,
                             Context.getLangOpts().Modules
                               ? &PP.getHeaderSearchInfo().getModuleMap()
                               : nullptr);
@@ -3475,7 +3480,7 @@ ASTReader::ASTReadResult ASTReader::ReadAST(const std::string &FileName,
     GlobalIndex.reset();
     ModuleMgr.setGlobalIndex(nullptr);
     return ReadResult;
-
+  }
   case Success:
     break;
   }
diff --git a/clang/lib/Serialization/ModuleManager.cpp b/clang/lib/Serialization/ModuleManager.cpp
index c5b4dd120cb..0a9ea69d695 100644
--- a/clang/lib/Serialization/ModuleManager.cpp
+++ b/clang/lib/Serialization/ModuleManager.cpp
@@ -109,8 +109,15 @@ ModuleManager::addModule(StringRef FileName, ModuleKind Type,
         ec = llvm::MemoryBuffer::getSTDIN(New->Buffer);
         if (ec)
           ErrorStr = ec.message();
-      } else
-        New->Buffer.reset(FileMgr.getBufferForFile(FileName, &ErrorStr));
+      } else {
+        // Leave the FileEntry open so if it gets read again by another
+        // ModuleManager it must be the same underlying file.
+        // FIXME: Because FileManager::getFile() doesn't guarantee that it will
+        // give us an open file, this may not be 100% reliable.
+        New->Buffer.reset(FileMgr.getBufferForFile(New->File, &ErrorStr,
+                                                   /*IsVolatile*/false,
+                                                   /*ShouldClose*/false));
+      }
       
       if (!New->Buffer)
         return Missing;
@@ -135,31 +142,16 @@ ModuleManager::addModule(StringRef FileName, ModuleKind Type,
   return NewModule? NewlyLoaded : AlreadyLoaded;
 }
 
-static void getModuleFileAncestors(
-    ModuleFile *F,
-    llvm::SmallPtrSetImpl<ModuleFile *> &Ancestors) {
-  Ancestors.insert(F);
-  for (ModuleFile *Importer : F->ImportedBy)
-    getModuleFileAncestors(Importer, Ancestors);
-}
-
-void ModuleManager::removeModules(ModuleIterator first, ModuleIterator last,
-                                  ModuleMap *modMap) {
+void ModuleManager::removeModules(
+    ModuleIterator first, ModuleIterator last,
+    llvm::SmallPtrSetImpl<ModuleFile *> &LoadedSuccessfully,
+    ModuleMap *modMap) {
   if (first == last)
     return;
 
   // Collect the set of module file pointers that we'll be removing.
   llvm::SmallPtrSet<ModuleFile *, 4> victimSet(first, last);
 
-  // The last module file caused the load failure, so it and its ancestors in
-  // the module dependency tree will be rebuilt (or there was an error), so
-  // there should be no references to them. Collect the files to remove from
-  // the cache below, since rebuilding them will create new files at the old
-  // locations.
-  llvm::SmallPtrSet<ModuleFile *, 4> Ancestors;
-  getModuleFileAncestors(*(last-1), Ancestors);
-  assert(Ancestors.count(*first) && "non-dependent module loaded");
-
   // Remove any references to the now-destroyed modules.
   for (unsigned i = 0, n = Chain.size(); i != n; ++i) {
     Chain[i]->ImportedBy.remove_if([&](ModuleFile *MF) {
@@ -178,7 +170,10 @@ void ModuleManager::removeModules(ModuleIterator first, ModuleIterator last,
       }
     }
 
-    if (Ancestors.count(*victim))
+    // Files that didn't make it through ReadASTCore successfully will be
+    // rebuilt (or there was an error). Invalidate them so that we can load the
+    // new files that will be renamed over the old ones.
+    if (LoadedSuccessfully.count(*victim) == 0)
       FileMgr.invalidateCache((*victim)->File);
 
     delete *victim;