commit 02254cb068de462e07facd5c12bc06c59e22ec86 Author: Ben Langmuir Date: Thu Jun 19 14:52:55 2014 -0700 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. diff --git a/include/clang/Basic/FileManager.h b/include/clang/Basic/FileManager.h index 023433b..f0f0d2f 100644 --- 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 08c1735..3259902 100644 --- 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 &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 aa06020..17622c7 100644 --- a/lib/Basic/FileManager.cpp +++ b/lib/Basic/FileManager.cpp @@ -388,7 +388,7 @@ void FileManager::FixupRelativePath(SmallVectorImpl &path) const { llvm::MemoryBuffer *FileManager:: getBufferForFile(const FileEntry *Entry, std::string *ErrorStr, - bool isVolatile) { + bool isVolatile, bool ShouldCloseOpenFile) { std::unique_ptr 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 0c9f263..777ea83 100644 --- 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 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 c5b4dd1..0a9ea69 100644 --- 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 &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 &LoadedSuccessfully, + ModuleMap *modMap) { if (first == last) return; // Collect the set of module file pointers that we'll be removing. llvm::SmallPtrSet 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 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;