[PATCH] D123104: [Modules] Use looked-up filename when looking for module maps

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

dexonsmith added reviewers: benlangmuir, bruno.
dexonsmith added a comment.

This LGTM, with a couple of comments (on the comments!) inline.

> Upstreamed from apple#llvm-project 72cf785051fb1b3ef22eee4dd33366e41a275981.

Note, for some extra background:

- This was out-of-tree because it seemed "hacky". The thinking at the time (2017!) was that we should try to get a "clean" fix for upstream, which is what `FileEntryRef` is on the path toward...
- ... but `FileEntryRef` is blocked because of hard-to-reason-about failures related to modules (see, for example, my reverted patch at https://reviews.llvm.org/D92975).
- In the meantime, the in-tree behaviour is wrong, and it doesn't make sense to just leave it broken upstream indefinitely... and keeping it downstream is making it harder to wrap our heads around the full problem.
- In discussing the intersection of this with the fix @bnbarham is working on (reverted in https://reviews.llvm.org/D123103 after this test failed in downstream integration), we've come up with a more incremental way of pushing `FileEntryRef` forward... hopefully incremental enough that we'll be unblocked and can finish it off. That's what I want documented in the FileManager FIXME as part of this patch.



================
Comment at: clang/lib/Lex/HeaderSearch.cpp:1570
     // If there is a module that corresponds to this header, suggest it.
-    hasModuleMap(File->getName(), Root, IsSystemHeaderDir);
+
+    // FIXME: File->getName() *should* be the same as FileName, but because
----------------
Nit: I'd very slightly prefer the two paragraphs be part of the same comment (put a `//` here for the blank line), but if you'd prefer it like this it's fine.


================
Comment at: clang/lib/Lex/HeaderSearch.cpp:1571-1572
+
+    // FIXME: File->getName() *should* be the same as FileName, but because
+    // of the VFS and various hacks in FileManager, that's not necessarily the
+    // case. We should update this to use FileEntryRef instead and either
----------------
Can you also expand the "redirection" FIXME in FileManager.cpp (or add an appropriate one)?

I think the steps are something like:
- Add API to determine if the name has been rewritten by the VFS (that's the patch you're working on).
- Expose the requested filename for adoption. I think the way to implement this is to allow "redirection FileEntryRef"s to be returned, rather than returning the pointed-at-FileEntryRef, and customizing `getName()` to see through the indirection.
- Update callers such as `HeaderSearch::findUsableModuleForHeader()` (comment should mention this function specifically as an example I think!) to explicitly get the requested filename rather than using `getName()`.
- Add a FileManager::getExternalPath API for explicitly getting the remapped external filename when there is one available, and adopt it in callers like diagnostics/deps reporting instead of calling `getName()` directly.
- Switch the meaning of `FileEntryRef::getName()` to get the requested name, not the external name. Once that sticks, revert callers that want the requested name back to calling `getName()`.
- Add an API to VFS to get the external filename lazily, stop doing it up front, and update FileManager::getExternalPath to do the right thing. (This will stop creating redirection entries for those cases.)

The need to expose the requested-name explicitly as a first step is a new insight we had when discussing this offline. Overall I think the plan for unwinding these hacks is under-documented; since we've spent a good deal of time on this, I'd like to record what we know so anyone new looking at `FileManager::getFileRef()` can guess how to help out (and better understand any odd behaviour they're seeing). Feel free to trim down and/or expand and/or completely rewrite, as long as the comment in FileManager encapsulates your understanding of how we can incrementally unwind all these hacks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123104