[PATCH] D86823: [clang][Modules] Perform an Extra Consistency Check When Searching The ModuleManager's Cache For Implicit Modules
This revision was automatically updated to reflect the committed changes.
Closed by commit rG272742a92d24: Perform an extra consistency check when
searching ModuleManagers (authored by aprantl).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D86823/new/
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,38 @@
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.
Index: clang/lib/Serialization/ModuleManager.cpp
===
--- clang/lib/Serialization/ModuleManager.cpp
+++ clang/lib/Serialization/ModuleManager.cpp
@@ -132,15 +132,38 @@
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.
[PATCH] D86823: [clang][Modules] Perform an Extra Consistency Check When Searching The ModuleManager's Cache For Implicit Modules
aprantl accepted this revision. aprantl added a comment. This revision is now accepted and ready to land. Assuming that it works, this seems fine. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86823/new/ https://reviews.llvm.org/D86823 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D86823: [clang][Modules] Perform an Extra Consistency Check When Searching The ModuleManager's Cache For Implicit Modules
CodaFi updated this revision to Diff 288734.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D86823/new/
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,38 @@
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.
Index: clang/lib/Serialization/ModuleManager.cpp
===
--- clang/lib/Serialization/ModuleManager.cpp
+++ clang/lib/Serialization/ModuleManager.cpp
@@ -132,15 +132,38 @@
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.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
[PATCH] D86823: [clang][Modules] Perform an Extra Consistency Check When Searching The ModuleManager's Cache For Implicit Modules
aprantl added inline comments.
Comment at: clang/lib/Serialization/ModuleManager.cpp:152
+ const FileEntry *Entry) -> bool {
+if (Kind != MK_ImplicitModule) {
+ return true;
nit: LLVM style omits curly braces on single statements.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D86823/new/
https://reviews.llvm.org/D86823
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D86823: [clang][Modules] Perform an Extra Consistency Check When Searching The ModuleManager's Cache For Implicit Modules
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.
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
