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>

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

diff --git a/include/clang/Basic/FileManager.h b/include/clang/Basic/FileManager.h
index 023433b25befe0be5225767096b0f65b889d5a8d..f0f0d2f2e192fb44a24d969838a1519fd8e594ed 100644 (file)
--- a/include/clang/Basic/FileManager.h
+++ b/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/include/clang/Serialization/ModuleManager.h b/include/clang/Serialization/ModuleManager.h
index 08c1735ab01ea571cb4f36935bf56721d9b826a6..3259902222d85556688f77c43b007d75ed11e52e 100644 (file)
--- a/include/clang/Serialization/ModuleManager.h
+++ b/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/lib/Basic/FileManager.cpp b/lib/Basic/FileManager.cpp
index aa06020be616d1f1a14aef9611848e5efbebebfc..17622c7e13f0040c98b54eba40457e2ccb5ac50a 100644 (file)
--- a/lib/Basic/FileManager.cpp
+++ b/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/lib/Serialization/ASTReader.cpp b/lib/Serialization/ASTReader.cpp
index 0c9f263faf1269725bcf6d5f0e2eb5520db8db4f..777ea83198192946e9712ca469ed52af25183160 100644 (file)
--- a/lib/Serialization/ASTReader.cpp
+++ b/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/lib/Serialization/ModuleManager.cpp b/lib/Serialization/ModuleManager.cpp
index c5b4dd120cbd326339b25991ac56c49d0eee8f8e..0a9ea69d695c3a339df82b58804b6c3887223c41 100644 (file)
--- a/lib/Serialization/ModuleManager.cpp
+++ b/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;