[PATCH] D123229: [clang][deps] Ensure deterministic file names on case-insensitive filesystems

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith accepted this revision.
dexonsmith added a reviewer: benlangmuir.
dexonsmith added a subscriber: benlangmuir.
dexonsmith added a comment.
This revision is now accepted and ready to land.

LGTM.

Once we've removed the last use of `DirectoryEntry::getName()`, we should drop `DirectoryEntry::getName()` to avoid picking up other users. Perhaps `DirectoryEntry` itself could be made private to the FileManager.

> The root cause is the fact that Clang is using DirectoryEntry::getName in the header search algorithm. This function returns the path that was first used to construct the (shared) entry in FileManager. Using DirectoryEntryRef::getName instead preserves the case as it was spelled out for the current "get directory entry" request.

For clarity, this is just one of various root causes of FileManager being unsound to reuse. Might be good to reword the commit message to avoid implying that it's the only one.

The obvious thing is that FileEntry is still used in a few places, especially in modules.

The non-obvious thing is that the "redirection hack" in `FileManager::getFileRef()`, which deals with a VFS remapping an external name, can cause future lookups to succeed that would previously have failed. @benlangmuir hit this recently when he was experimenting with NOT sharing the FileManager during implicit modules scanning; he can share the sequence of steps (I tried just now to remember them but I couldn't work it out).

IMO, we should turn off sharing the FileManager across multiple TUs (except as an experimental option) until we actually believe that it's sound. I'm not convinced there's a lot of benefit anyway:

- Stat failures need to be cleared between TUs
- Stat successes are cached by the dependency scanning filesystem
- The contents are (mostly? entirely?) bump-ptr allocated since https://reviews.llvm.org/D123144


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123229