Page MenuHomePhabricator

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

Authored by CodaFi on Aug 28 2020, 4:25 PM.

Details

Summary

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

Diff Detail

Event Timeline

CodaFi created this revision.Aug 28 2020, 4:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 28 2020, 4:25 PM
CodaFi requested review of this revision.Aug 28 2020, 4:25 PM
aprantl added inline comments.Aug 28 2020, 4:44 PM
clang/lib/Serialization/ModuleManager.cpp
152

nit: LLVM style omits curly braces on single statements.

CodaFi updated this revision to Diff 288734.Aug 28 2020, 5:18 PM
CodaFi marked an inline comment as done.Aug 28 2020, 5:45 PM
aprantl accepted this revision.Aug 28 2020, 8:12 PM

Assuming that it works, this seems fine.

This revision is now accepted and ready to land.Aug 28 2020, 8:12 PM