[PATCH] D86823: [clang][Modules] Perform an Extra Consistency Check When Searching The ModuleManager's Cache For Implicit Modules

[Robert Widmann via Phabricator via cfe-commits]

CodaFi created this revision.
CodaFi added reviewers: vsapsai, aprantl, doug.gregor.
Herald added subscribers: cfe-commits, danielkiss.
Herald added a project: clang.
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.

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.

To mitigate the effects of inode reuse, perform an extra name check when
implicit modules are returned from the cache. This has the effect of
forcing reused FileEntry nodes to stomp over existing-but-stale entries
in the cache, which simulates a miss - exactly the desired behavior.

rdar://48443680


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D86823

Files:
  clang/lib/Serialization/ModuleManager.cpp


Index: clang/lib/Serialization/ModuleManager.cpp
===================================================================
--- clang/lib/Serialization/ModuleManager.cpp
+++ clang/lib/Serialization/ModuleManager.cpp
@@ -132,15 +132,40 @@
     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(Entry)) {
-    // Check the stored signature.
-    if (checkSignature(ModuleEntry->Signature, ExpectedSignature, ErrorStr))
-      return OutOfDate;
-
-    Module = ModuleEntry;
-    updateModuleImports(*ModuleEntry, ImportedBy, ImportLoc);
-    return AlreadyLoaded;
+    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.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D86823.288728.patch
Type: text/x-patch
Size: 2347 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200828/a4f86145/attachment.bin>