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

Tue Aug 1 20:20:14 PDT 2023

vsapsai added a comment.

In D156749#4551596 <https://reviews.llvm.org/D156749#4551596>, @jansvoboda11 wrote:

> What are the performance implications of making VFS overlays part of the context hash?

I don't have specific measurements but for Xcode projects different targets have different VFS overlays. So adding VFS overlays to the context hash would prevent module sharing between targets. To use llvm+clang as an example it means different libraries will have different versions of the same module. For example, clangLex will have its version of llvmSupport and clangSerialization will have its version too. Actual impact depends on a project but for clang with 29 subdirectories in "clang/lib" an approximate estimate would be 10 times more modules to build. Note that clang build doesn't use VFS, so it won't be an actual impact on clang build but on projects with the size and structure similar to clang.

In D156749#4551596 <https://reviews.llvm.org/D156749#4551596>, @jansvoboda11 wrote:

>> And it is build system's responsibility to provide `-ivfsoverlay` options that don't cause observable differences.
>
> I wasn't aware of that. Do we document this anywhere? It surprises me that we'd impose such restriction on the build system. This seems fairly easy to accidentally violate and end up in the same situation - Clang instances with different VFS overlays, identical context hashes and different canonical module map paths for the same module.

There are no documents describing responsibilities  of different components, as far as I know. I haven't researched the issue extensively but I think only Xcode build system uses VFS overlays. And if you have only one client, it is a questionable use of time to formalize the interface. And the absence of formal contract allows to evolve different sides faster.

Yes, if a build system provides bad data to a compiler, it can cause errors. And there are no ways to force build systems not to do that. I don't think it is the compiler's responsibility to verify build system behavior. For example, if for incremental build not all impacted files are recompiled (given the dependencies are correct) - the compiler shouldn't track it themselves and recompile the required files.

In D156749#4551596 <https://reviews.llvm.org/D156749#4551596>, @jansvoboda11 wrote:

> Alternatively, we could keep VFS overlays out of the context hash but create `<hash2>` from the on-disk real path of the defining module map and make the whole PCM VFS-agnostic. Then it'd be correct to import that PCM regardless of the specific VFS overlay setup, as long as all VFS queries of the importer resolve the same way they resolved within the instance that built the PCM. Maybe we can force the importer to recompile the PCM if that's not the case, similar to what we do for diagnostic options.

I'm not sure I understand your proposal. Before this change we were calculating hash from the on-disk real path of the defining module map. And due to different VFS/no-VFS options defining module map is at different locations on-disk.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156749