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

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added a comment.

In D151855#4403934 <https://reviews.llvm.org/D151855#4403934>, @jansvoboda11 wrote:

> In D151855#4403879 <https://reviews.llvm.org/D151855#4403879>, @benlangmuir wrote:
>
>>> I think it should be fine to allow dropping the A.framework/Frameworks/B.framework directory from the reproducer VFS
>>
>> I think technically this is wrong, since if you're missing the symlink, then A might not build -- e.g. it could be doing a relative include that needs the symlink.  But am I understanding correctly that the reproducer was already broken in this case? If so I'm fine with this.
>
> You're right, this would break relative includes, which I believe the current test case would handle correctly. (It only fails to canonicalize the paths to `B.framework` and make it into a top-level framework.) I'll try to verify this, just to be sure.

I tried changing the `#include <B/B.h>` to relative `#include "../Frameworks/B.framework/Headers/B.h"` in `A.h`. The original Clang invocation compiles both of these fine with and without this patch, promoting `B` to a top-level framework.

The relative include fails to compile with the reproducer VFS, both with and without this patch. This boils down to Clang calling `FileSystem::getRealPath("<snip>/Frameworks/A.framework/Frameworks/B.framework")` when deciding whether to promote `B` to top-level framework. Here the `RedirectingFileSystem` ends up just canonicalizing the path (making it absolute, removing `.`, etc.) and forwarding <https://github.com/llvm/llvm-project/blob/8404b23acd70b8db1411b98a04b4ea62eaeb48dd/llvm/lib/Support/VirtualFileSystem.cpp#L2546-L2549> to the underlying (real) FS. This doesn't have the symlink, so it returns non-zero error-code, and `FileManager::getCanonicalName()` ends up returning the virtual path <https://github.com/llvm/llvm-project/blob/8404b23acd70b8db1411b98a04b4ea62eaeb48dd/clang/lib/Basic/FileManager.cpp#L640-L647> under `A.framework`, preventing promotion of `B` to a top-level framework. This ends up registering `B` as a sub-framework of `A` in one place, but then Clang sees `<snip>/Frameworks/B.framework` and attempts to create `B` again with the same `DirectoryEntry` umbrella directory, resulting in the error below:

  <build>/tools/clang/test/Modules/Output/crash-vfs-umbrella-frameworks.m.tmp/i/Frameworks/A.framework/Frameworks/B.framework/Modules/module.modulemap:2:19: error: umbrella for module 'A.B' already covers this directory
      2 |   umbrella header "B.h"
        |                   ^
  <build>/tools/clang/test/Modules/Output/crash-vfs-umbrella-frameworks.m.tmp/i/Frameworks/A.framework/Frameworks/B.framework/Modules/module.modulemap:4:10: error: inferred submodules require a module with an umbrella
      4 |   module * { export * }
        |          ^
  While building module 'I' imported from <source>/clang/test/Modules/crash-vfs-umbrella-frameworks.m:42:
  In file included from <module-includes>:1:
  <build>/tools/clang/test/Modules/Output/crash-vfs-umbrella-frameworks.m.tmp/i/Frameworks/I.framework/Headers/I.h:2:9: fatal error: could not build module 'A'
      2 | #import <A/A.h>
        |  ~~~~~~~^
  <source>/clang/test/Modules/crash-vfs-umbrella-frameworks.m:42:9: fatal error: could not build module 'I'
     42 | @import I;
        |  ~~~~~~~^
  4 errors generated.

(Note that the `A.framework/Frameworks/B.framework/Headers/B.h` and `A.framework/Frameworks/B.framework/Modules/module.modulemap` files do make it into the reproducer VFS with this patch, since they both get touched by the original Clang invocation. For some reason, we end up with duplicate entries for these in the overlay file without this patch.)

That said, this patch doesn't regress the relative include reproducer, since it's already broken. I'm not exactly sure what the fix for that particular issue is, since the interactions are pretty subtle. I don't think it's worth blocking this patch on it, though. What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151855