diff --git a/clang/include/clang/Serialization/ModuleManager.h b/clang/include/clang/Serialization/ModuleManager.h index 3c5ccb1d88fb0..15ddb9875ff17 100644 --- a/clang/include/clang/Serialization/ModuleManager.h +++ b/clang/include/clang/Serialization/ModuleManager.h @@ -59,50 +59,8 @@ class ModuleManager { // to implement short-circuiting logic when running DFS over the dependencies. SmallVector Roots; - /// An \c EntryKey is a thin wrapper around a \c FileEntry that implements - /// a richer notion of identity. - /// - /// A plain \c FileEntry has its identity tied to inode numbers. When the - /// module cache regenerates a PCM, some filesystem allocators may reuse - /// inode numbers for distinct modules, which can cause the cache to return - /// mismatched entries. An \c EntryKey ensures that the size and modification - /// time are taken into account when determining the identity of a key, which - /// significantly decreases - but does not eliminate - the chance of - /// a collision. - struct EntryKey { - const FileEntry *Entry; - - struct Info { - static inline EntryKey getEmptyKey() { - return EntryKey{llvm::DenseMapInfo::getEmptyKey()}; - } - static inline EntryKey getTombstoneKey() { - return EntryKey{ - llvm::DenseMapInfo::getTombstoneKey()}; - } - static unsigned getHashValue(const EntryKey &Val) { - return llvm::DenseMapInfo::getHashValue(Val.Entry); - } - static bool isEqual(const EntryKey &LHS, const EntryKey &RHS) { - if (LHS.Entry == getEmptyKey().Entry || - LHS.Entry == getTombstoneKey().Entry || - RHS.Entry == getEmptyKey().Entry || - RHS.Entry == getTombstoneKey().Entry) { - return LHS.Entry == RHS.Entry; - } - if (LHS.Entry == nullptr || RHS.Entry == nullptr) { - return LHS.Entry == RHS.Entry; - } - return LHS.Entry == RHS.Entry && - LHS.Entry->getSize() == RHS.Entry->getSize() && - LHS.Entry->getModificationTime() == - RHS.Entry->getModificationTime(); - } - }; - }; - /// All loaded modules, indexed by name. - llvm::DenseMap Modules; + llvm::DenseMap Modules; /// FileManager that handles translating between filenames and /// FileEntry *. @@ -118,7 +76,7 @@ class ModuleManager { const HeaderSearch &HeaderSearchInfo; /// A lookup of in-memory (virtual file) buffers - llvm::DenseMap, EntryKey::Info> + llvm::DenseMap> InMemoryBuffers; /// The visitation order. diff --git a/clang/lib/Serialization/ModuleManager.cpp b/clang/lib/Serialization/ModuleManager.cpp index f5d83964b90ff..43d49e1ad45b7 100644 --- a/clang/lib/Serialization/ModuleManager.cpp +++ b/clang/lib/Serialization/ModuleManager.cpp @@ -59,7 +59,7 @@ ModuleFile *ModuleManager::lookupByModuleName(StringRef Name) const { } ModuleFile *ModuleManager::lookup(const FileEntry *File) const { - auto Known = Modules.find(EntryKey{File}); + auto Known = Modules.find(File); if (Known == Modules.end()) return nullptr; @@ -72,7 +72,7 @@ ModuleManager::lookupBuffer(StringRef Name) { /*CacheFailure=*/false); if (!Entry) return nullptr; - return std::move(InMemoryBuffers[EntryKey{*Entry}]); + return std::move(InMemoryBuffers[*Entry]); } static bool checkSignature(ASTFileSignature Signature, @@ -132,15 +132,38 @@ ModuleManager::addModule(StringRef FileName, ModuleKind Type, return Missing; } + // The ModuleManager's use of FileEntry nodes as the keys for its map of + // loaded modules is less than ideal. Uniqueness for FileEntry nodes is + // maintained by FileManager, which in turn uses inode numbers on hosts + // that support that. When coupled with the module cache's proclivity for + // turning over and deleting stale PCMs, this means entries for different + // module files can wind up reusing the same underlying inode. When this + // happens, subsequent accesses to the Modules map will disagree on the + // ModuleFile associated with a given file. In general, it is not sufficient + // to resolve this conundrum with a type like FileEntryRef that stores the + // name of the FileEntry node on first access because of path canonicalization + // issues. However, the paths constructed for implicit module builds are + // fully under Clang's control. We *can*, therefore, rely on their structure + // being consistent across operating systems and across subsequent accesses + // to the Modules map. + auto implicitModuleNamesMatch = [](ModuleKind Kind, const ModuleFile *MF, + const FileEntry *Entry) -> bool { + if (Kind != MK_ImplicitModule) + return true; + return Entry->getName() == MF->FileName; + }; + // Check whether we already loaded this module, before - if (ModuleFile *ModuleEntry = Modules.lookup(EntryKey{Entry})) { - // Check the stored signature. - if (checkSignature(ModuleEntry->Signature, ExpectedSignature, ErrorStr)) - return OutOfDate; - - Module = ModuleEntry; - updateModuleImports(*ModuleEntry, ImportedBy, ImportLoc); - return AlreadyLoaded; + if (ModuleFile *ModuleEntry = Modules.lookup(Entry)) { + if (implicitModuleNamesMatch(Type, ModuleEntry, Entry)) { + // Check the stored signature. + if (checkSignature(ModuleEntry->Signature, ExpectedSignature, ErrorStr)) + return OutOfDate; + + Module = ModuleEntry; + updateModuleImports(*ModuleEntry, ImportedBy, ImportLoc); + return AlreadyLoaded; + } } // Allocate a new module. @@ -208,7 +231,7 @@ ModuleManager::addModule(StringRef FileName, ModuleKind Type, return OutOfDate; // We're keeping this module. Store it everywhere. - Module = Modules[EntryKey{Entry}] = NewModule.get(); + Module = Modules[Entry] = NewModule.get(); updateModuleImports(*NewModule, ImportedBy, ImportLoc); @@ -255,7 +278,7 @@ void ModuleManager::removeModules(ModuleIterator First, ModuleMap *modMap) { // Delete the modules and erase them from the various structures. for (ModuleIterator victim = First; victim != Last; ++victim) { - Modules.erase(EntryKey{victim->File}); + Modules.erase(victim->File); if (modMap) { StringRef ModuleName = victim->ModuleName; @@ -274,7 +297,7 @@ ModuleManager::addInMemoryBuffer(StringRef FileName, std::unique_ptr Buffer) { const FileEntry *Entry = FileMgr.getVirtualFile(FileName, Buffer->getBufferSize(), 0); - InMemoryBuffers[EntryKey{Entry}] = std::move(Buffer); + InMemoryBuffers[Entry] = std::move(Buffer); } ModuleManager::VisitState *ModuleManager::allocateVisitState() {