[PATCH] D85981: [clang][Modules] Use File Names Instead of inodes As Loaded Module Keys

[Robert Widmann via Phabricator via cfe-commits]

CodaFi created this revision.
CodaFi added reviewers: vsapsai, Bigcheese, doug.gregor.
CodaFi added a project: clang.
Herald added subscribers: cfe-commits, dexonsmith.
CodaFi requested review of this revision.

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.

It's fine to use the file management utilities to guarantee the presence
of module data, but we need a better source of key material that is
invariant with respect to these OS-level details. Switch instead to use
the given name of the FileEntry node, which should provide separate entries
in the map in the event of inode reuse.

rdar://48443680


Repository:
  rG LLVM Github Monorepo

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(File->getName());
   if (Known == Modules.end())
     return nullptr;
 
@@ -133,7 +133,8 @@
   }
 
   // Check whether we already loaded this module, before
-  if (ModuleFile *ModuleEntry = Modules.lookup(Entry)) {
+  assert(!Entry || Entry->getName() == FileName);
+  if (ModuleFile *ModuleEntry = Modules.lookup(FileName)) {
     // Check the stored signature.
     if (checkSignature(ModuleEntry->Signature, ExpectedSignature, ErrorStr))
       return OutOfDate;
@@ -208,7 +209,7 @@
     return OutOfDate;
 
   // We're keeping this module.  Store it everywhere.
-  Module = Modules[Entry] = NewModule.get();
+  Module = Modules[FileName] = NewModule.get();
 
   updateModuleImports(*NewModule, ImportedBy, ImportLoc);
 
@@ -255,7 +256,7 @@
 
   // Delete the modules and erase them from the various structures.
   for (ModuleIterator victim = First; victim != Last; ++victim) {
-    Modules.erase(victim->File);
+    Modules.erase(victim->File->getName());
 
     if (modMap) {
       StringRef ModuleName = victim->ModuleName;
Index: clang/include/clang/Serialization/ModuleManager.h
===================================================================
--- clang/include/clang/Serialization/ModuleManager.h
+++ clang/include/clang/Serialization/ModuleManager.h
@@ -59,8 +59,8 @@
   // to implement short-circuiting logic when running DFS over the 
dependencies.
   SmallVector<ModuleFile *, 2> Roots;
 
-  /// All loaded modules, indexed by name.
-  llvm::DenseMap<const FileEntry *, ModuleFile *> Modules;
+  /// All loaded modules, indexed by file name.
+  llvm::StringMap<ModuleFile *> Modules;
 
   /// FileManager that handles translating between filenames and
   /// FileEntry *.



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(File->getName());
   if (Known == Modules.end())
     return nullptr;
 
@@ -133,7 +133,8 @@
   }
 
   // Check whether we already loaded this module, before
-  if (ModuleFile *ModuleEntry = Modules.lookup(Entry)) {
+  assert(!Entry || Entry->getName() == FileName);
+  if (ModuleFile *ModuleEntry = Modules.lookup(FileName)) {
     // Check the stored signature.
     if (checkSignature(ModuleEntry->Signature, ExpectedSignature, ErrorStr))
       return OutOfDate;
@@ -208,7 +209,7 @@
     return OutOfDate;
 
   // We're keeping this module.  Store it everywhere.
-  Module = Modules[Entry] = NewModule.get();
+  Module = Modules[FileName] = NewModule.get();
 
   updateModuleImports(*NewModule, ImportedBy, ImportLoc);
 
@@ -255,7 +256,7 @@
 
   // Delete the modules and erase them from the various structures.
   for (ModuleIterator victim = First; victim != Last; ++victim) {
-    Modules.erase(victim->File);
+    Modules.erase(victim->File->getName());
 
     if (modMap) {
       StringRef ModuleName = victim->ModuleName;
Index: clang/include/clang/Serialization/ModuleManager.h
===================================================================
--- clang/include/clang/Serialization/ModuleManager.h
+++ clang/include/clang/Serialization/ModuleManager.h
@@ -59,8 +59,8 @@
   // to implement short-circuiting logic when running DFS over the dependencies.
   SmallVector<ModuleFile *, 2> Roots;
 
-  /// All loaded modules, indexed by name.
-  llvm::DenseMap<const FileEntry *, ModuleFile *> Modules;
+  /// All loaded modules, indexed by file name.
+  llvm::StringMap<ModuleFile *> Modules;
 
   /// FileManager that handles translating between filenames and
   /// FileEntry *.

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