[PATCH] D85981: [clang][Modules] Increase the Entropy of ModuleManager Map Keys

[Robert Widmann via Phabricator via cfe-commits]

CodaFi updated this revision to Diff 286679.
CodaFi retitled this revision from "[clang][Modules] Use File Names Instead of 
inodes As Loaded Module Keys" to "[clang][Modules] Increase the Entropy of 
ModuleManager Map Keys".

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85981/new/

https://reviews.llvm.org/D85981

Files:
  clang/include/clang/Serialization/ModuleManager.h
  clang/lib/Serialization/ModuleManager.cpp


Index: clang/lib/Serialization/ModuleManager.cpp
===================================================================
--- clang/lib/Serialization/ModuleManager.cpp
+++ clang/lib/Serialization/ModuleManager.cpp
@@ -59,7 +59,7 @@
 }
 
 ModuleFile *ModuleManager::lookup(const FileEntry *File) const {
-  auto Known = Modules.find(File);
+  auto Known = Modules.find(EntryKey{File});
   if (Known == Modules.end())
     return nullptr;
 
@@ -72,7 +72,7 @@
                                /*CacheFailure=*/false);
   if (!Entry)
     return nullptr;
-  return std::move(InMemoryBuffers[*Entry]);
+  return std::move(InMemoryBuffers[EntryKey{*Entry}]);
 }
 
 static bool checkSignature(ASTFileSignature Signature,
@@ -133,7 +133,7 @@
   }
 
   // Check whether we already loaded this module, before
-  if (ModuleFile *ModuleEntry = Modules.lookup(Entry)) {
+  if (ModuleFile *ModuleEntry = Modules.lookup(EntryKey{Entry})) {
     // Check the stored signature.
     if (checkSignature(ModuleEntry->Signature, ExpectedSignature, ErrorStr))
       return OutOfDate;
@@ -208,7 +208,7 @@
     return OutOfDate;
 
   // We're keeping this module.  Store it everywhere.
-  Module = Modules[Entry] = NewModule.get();
+  Module = Modules[EntryKey{Entry}] = NewModule.get();
 
   updateModuleImports(*NewModule, ImportedBy, ImportLoc);
 
@@ -255,7 +255,7 @@
 
   // Delete the modules and erase them from the various structures.
   for (ModuleIterator victim = First; victim != Last; ++victim) {
-    Modules.erase(victim->File);
+    Modules.erase(EntryKey{victim->File});
 
     if (modMap) {
       StringRef ModuleName = victim->ModuleName;
@@ -274,7 +274,7 @@
                                  std::unique_ptr<llvm::MemoryBuffer> Buffer) {
   const FileEntry *Entry =
       FileMgr.getVirtualFile(FileName, Buffer->getBufferSize(), 0);
-  InMemoryBuffers[Entry] = std::move(Buffer);
+  InMemoryBuffers[EntryKey{Entry}] = std::move(Buffer);
 }
 
 ModuleManager::VisitState *ModuleManager::allocateVisitState() {
Index: clang/include/clang/Serialization/ModuleManager.h
===================================================================
--- clang/include/clang/Serialization/ModuleManager.h
+++ clang/include/clang/Serialization/ModuleManager.h
@@ -59,8 +59,67 @@
   // to implement short-circuiting logic when running DFS over the dependencies.
   SmallVector<ModuleFile *, 2> 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;
+    off_t Size;
+    time_t ModTime;
+
+    EntryKey(const FileEntry *Entry) : Entry(Entry), Size(0), ModTime(0) {
+      if (Entry) {
+        Size = Entry->getSize();
+        ModTime = Entry->getModificationTime();
+      }
+    }
+
+    EntryKey(const FileEntry *Entry, off_t Size, time_t ModTime)
+        : Entry(Entry), Size(Size), ModTime(ModTime) {}
+
+    struct Info {
+      static inline EntryKey getEmptyKey() {
+        return EntryKey{
+          llvm::DenseMapInfo<const FileEntry *>::getEmptyKey(),
+          llvm::DenseMapInfo<off_t>::getEmptyKey(),
+          llvm::DenseMapInfo<time_t>::getEmptyKey(),
+        };
+      }
+      static inline EntryKey getTombstoneKey() {
+        return EntryKey{
+          llvm::DenseMapInfo<const FileEntry *>::getTombstoneKey(),
+          llvm::DenseMapInfo<off_t>::getTombstoneKey(),
+          llvm::DenseMapInfo<time_t>::getTombstoneKey(),
+        };
+      }
+      static unsigned getHashValue(const EntryKey &Val) {
+        return llvm::DenseMapInfo<const FileEntry *>::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.Size == RHS.Size &&
+               LHS.ModTime == RHS.ModTime;
+      }
+    };
+  };
+
   /// All loaded modules, indexed by name.
-  llvm::DenseMap<const FileEntry *, ModuleFile *> Modules;
+  llvm::DenseMap<EntryKey, ModuleFile *, EntryKey::Info> Modules;
 
   /// FileManager that handles translating between filenames and
   /// FileEntry *.
@@ -76,7 +135,7 @@
   const HeaderSearch &HeaderSearchInfo;
 
   /// A lookup of in-memory (virtual file) buffers
-  llvm::DenseMap<const FileEntry *, std::unique_ptr<llvm::MemoryBuffer>>
+  llvm::DenseMap<EntryKey, std::unique_ptr<llvm::MemoryBuffer>, EntryKey::Info>
       InMemoryBuffers;
 
   /// The visitation order.

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits