[PATCH] D156749: [modules] Fix error about the same module being defined in different .pcm files when using VFS overlays.

Mon Aug 7 08:20:07 PDT 2023

jansvoboda11 accepted this revision.
jansvoboda11 added a comment.

In D156749#4562469 <https://reviews.llvm.org/D156749#4562469>, @vsapsai wrote:

> In D156749#4561803 <https://reviews.llvm.org/D156749#4561803>, @jansvoboda11 wrote:
>
>> My suggestion is to use the actual real on-disk path. Not `FileEntryRef::getName()`, but something that always behaves as if `use-external-name` was set to `true`. I believe this would handle your VFS/VFS-use-external-name-true/VFS-use-external-name-false problem. It would also handle another pitfall: two compilations with distinct VFS overlays that redirect two different as-requested module map paths into the same on-disk path.
>
> Do you suggest doing it for the hashing or for ASTWriter or both? We are already doing some module map path canonicalization (added a comment in corresponding place) but it's not pure on-disk path, it takes into account VFS.

I mainly meant in `<hash2>`. And then make the PCM files VFS-agnostic, which would probably require us to do the same thing for all the paths we serialize there. But I don't know how feasible that is. Besides, the scanner relies on the virtual paths in PCM files.

I think your solution is the most pragmatic. If you're confident this doesn't break anything internally, I say go for it. But I think it's good to be aware of the pitfall I mentioned, and make sure the build system doesn't trigger that.

================
Comment at: clang/lib/Serialization/ASTWriter.cpp:1330
     AddPath(WritingModule->PresumedModuleMapFile.empty()
-                ? Map.getModuleMapFileForUniquing(WritingModule)->getName()
+                ? Map.getModuleMapFileForUniquing(WritingModule)
+                      ->getNameAsRequested()
----------------
Can we canonicalize this also? It'd be useful in the scanner.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156749