[PATCH] D151855: [clang] Use `{File,Directory}EntryRef` in modular header search (part 2/2)

Wed Jun 7 10:56:14 PDT 2023

jansvoboda11 requested review of this revision.
jansvoboda11 added a comment.

Regarding the failing `Modules/crash-vfs-umbrella-frameworks.m` test, this is my understanding after debugging it.

Whenever we find that `A.framework/Frameworks/B.framework` is a symlink, we treat `B` as a separate top-level framework, not as a subframework of `A`. We detect this in  `::getTopFrameworkDir()` (in `HeaderSearch.cpp`) by calling `FileManager::getCanonicalPath()`, which resolves the symlink. We use the returned directory when calling the module map parser. Before this change, the `DirectoryEntry::getName()` ended up being the path Clang first accessed: `A.framework/Frameworks/B.framework`. After this change, it's the as-requested canonical path instead: `B.framework`. Since this canonicalization takes place and doesn't have any side-effect (I believe & hope), I think it should be fine to allow dropping the `A.framework/Frameworks/B.framework` directory from the reproducer VFS.
Interestingly, before this change, the reproducer generated PCMs only for `A` and `I`, which makes me believe `B` ended up being treated as a subframework somehow (maybe the VFS interferes with the symlink canonicalization?). With this patch, the reproducer generates PCMs for all `A`, `B` and `I`, which matches the original (crashing) invocation on line 13. For the record, the test was introduced in D20194 <https://reviews.llvm.org/D20194>.

@benlangmuir, does that sound like a reasonable thing to do?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151855