Page MenuHomePhabricator

[Modules] Use looked-up filename when looking for module maps
AbandonedPublic

Authored by bnbarham on Apr 4 2022, 5:20 PM.

Details

Summary

Prevent possible modulemap collisions by making sure to always use the
looked-up filename, regardless of any possible overlays.

Upstreamed from apple#llvm-project
72cf785051fb1b3ef22eee4dd33366e41a275981. As well as preventing the
collisions as above, it also highlighted an issue with the recent change
to narrow the FileManager hack of setting the DirectoryEntry in
FileEntry to the most recently looked up directory
(3fda0edc51fd68192a30e302d45db081bb02d7f9). Specifically, it would cause
the incorrect path to be used in DoFrameworkLookup in a crash
reproducer:

  • Crash reproducers have use-external-names set to false
  • File->getFileEntry().getDir()->getName() would then be the *cached* path, not the just looked up one
  • crash-vfs-umbrella-frameworks.m fails with this change since the correct path is now looked up and causes B to be loaded twice

Diff Detail

Event Timeline

bnbarham created this revision.Apr 4 2022, 5:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 4 2022, 5:20 PM
bnbarham requested review of this revision.Apr 4 2022, 5:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 4 2022, 5:20 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

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.
clang/lib/Lex/HeaderSearch.cpp
1570–1579

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.

1571–1572

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.

dexonsmith accepted this revision.Apr 4 2022, 7:19 PM

(Forgot to click "accept".)

This revision is now accepted and ready to land.Apr 4 2022, 7:19 PM
benlangmuir accepted this revision.Apr 5 2022, 10:56 AM
benlangmuir added inline comments.
clang/include/clang/Lex/HeaderSearch.h
763

This parameter could use a comment, even if it just points you to read the FIXME in the implementation.

dexonsmith added inline comments.Apr 5 2022, 5:07 PM
clang/include/clang/Lex/HeaderSearch.h
763

Good point; maybe it could have a better name/type, too, something like:

Optional<StringRef> OverrideModuleMapFilename = None
bnbarham added inline comments.Apr 5 2022, 6:01 PM
clang/include/clang/Lex/HeaderSearch.h
763

I believe this is actually the header name, but Optional + OverrideFilename seems worthwhile to me.

bnbarham updated this revision to Diff 420678.Apr 5 2022, 6:16 PM

Added a potential plan to remove the FileManager hacks.

bnbarham abandoned this revision.Apr 8 2022, 9:06 AM

Looks like there's more changes required for modulemap-collision.m to actually pass. I'll try figure those out when I have the time.

Looks like there's more changes required for modulemap-collision.m to actually pass. I'll try figure those out when I have the time.

Capturing clang -check error since the build log won't be available forever:

error: 'warning' diagnostics seen but not expected: 

  File /var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/test/Modules/Output/modulemap-collision.m.tmp/build/module.private.modulemap Line 1: private submodule 'A.Private' in private module map, expected top-level module

error: 'note' diagnostics seen but not expected: 

  File /var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/test/Modules/Output/modulemap-collision.m.tmp/build/module.private.modulemap Line 1: rename 'A.Private' to ensure it can be found by name

I wonder if https://github.com/apple/llvm-project/commit/67c70038bcc0d771e2e39c875ad7d69e329c7fc4 could be related. A long shot (I can't see how that could be it!), but the other not-upstreamed patches look even less relevant.

clang/test/Modules/modulemap-collision.m
12

Just noticed -fmodules-cache-path=tmp, which seems like it should be =%t/cache instead... but I doubt that's the cause of the failure.

I wonder if https://github.com/apple/llvm-project/commit/67c70038bcc0d771e2e39c875ad7d69e329c7fc4 could be related. A long shot (I can't see how that could be it!), but the other not-upstreamed patches look even less relevant.

In case it is related, note the thread ending at https://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20170807/200915.html, which explains why it was reverted from upstream in 4164dd9167f2f049e2a5e1bd3970a6bb45889462 and describes an alternative fix. @Bigcheese @jansvoboda11 @vsapsai, perhaps something to be aware of.