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

[vsapsai]

Summary

Fix errors like

module 'MultiPath' is defined in both 'path/to/modules.cache/3JR48BPRU7BCG/MultiPath-1352QHUF8RNMU.pcm' and 'path/to/modules.cache/3JR48BPRU7BCG/MultiPath-20HNSLLIUDDV1.pcm'

To avoid building extra identical modules -ivfsoverlay option is not a
part of the hash like "/3JR48BPRU7BCG/". And it is build system's
responsibility to provide -ivfsoverlay options that don't cause
observable differences. We also need to make sure the hash like
"-1352QHUF8RNMU" is not affected by -ivfsoverlay. As this hash is
defined by the module map path, use the path prior to any VFS
remappings.

rdar://111921464

[ChuanqiXu]

Although there is a FIXME in the definition of getNameAsRequested(), it looks not sense to require you to fix that. It might not be an over burden for someone who will be intended to fix this later. So LGTM.

[vsapsai]

Thanks for the quick review!

In D156749#4549226, @ChuanqiXu wrote:

Although there is a FIXME in the definition of getNameAsRequested(), it looks not sense to require you to fix that. It might not be an over burden for someone who will be intended to fix this later. So LGTM.

I've discussed with @jansvoboda11 the future of FileEntryRef::getNameAsRequested and it is supposed to replace FileEntryRef::getName (eventually, not immediately), if I understand correctly. Given that, it is the move in the right direction regardless of FIXMEs. If I've misunderstood something, Jan can chime in and correct me.

[jansvoboda11]

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.

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

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.

[jansvoboda11]

CC @benlangmuir, since we've talked about this.

[vsapsai]

In 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, @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, @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.

[vsapsai]

Ping @jansvoboda11 , @benlangmuir in case you still have thoughts on this.

[jansvoboda11]

In D156749#4552590, @vsapsai wrote:

In 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.

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.

[vsapsai]

In 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.

clang/lib/Lex/HeaderSearch.cpp

259–260

Sort of canonicalization and obtaining real on-disk path we are doing.

[jansvoboda11]

In D156749#4562469, @vsapsai wrote:

In 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.

clang/lib/Serialization/ASTWriter.cpp

1330–1331

Can we canonicalize this also? It'd be useful in the scanner.

[vsapsai]

In D156749#4565994, @jansvoboda11 wrote:

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.

As far as I have tested, it isn't breaking anything.

clang/lib/Serialization/ASTWriter.cpp

1330–1331

I'm not sure about that. ASTReader has some complicated logic around reading this value https://github.com/llvm/llvm-project/blob/c1803d5366c794ecade4e4ccd0013690a1976d49/clang/lib/Serialization/ASTReader.cpp#L4005 So if we don't have a proven need for it with a test case, I wouldn't change it.

[jansvoboda11]

clang/lib/Serialization/ASTWriter.cpp

1330–1331

Good point. I do have a use for it, but I think it'd be safer to do separately from this patch a qualify it in isolation.

clang/lib/Lex/HeaderSearch.cpp

 
 std::string HeaderSearch::getCachedModuleFileName(Module *Module) {
   OptionalFileEntryRef ModuleMap =
       getModuleMap().getModuleMapFileForUniquing(Module);
   // The ModuleMap maybe a nullptr, when we load a cached C++ module without
   // *.modulemap file. In this case, just return an empty string.
   if (!ModuleMap)
     return {};
-  return getCachedModuleFileName(Module->Name, ModuleMap->getName());
+  return getCachedModuleFileName(Module->Name, ModuleMap->getNameAsRequested());
 }
 
 std::string HeaderSearch::getPrebuiltModuleFileName(StringRef ModuleName,
                                                     bool FileMapOnly) {
   // First check the module name to pcm file map.
   auto i(HSOpts->PrebuiltModuleFiles.find(ModuleName));
   if (i != HSOpts->PrebuiltModuleFiles.end())
     return i->second;
std::string HeaderSearch::getCachedModuleFileNameImpl(StringRef ModuleName,
   } else {
     // Construct the name <ModuleName>-<hash of ModuleMapPath>.pcm which should
     // ideally be globally unique to this particular module. Name collisions
     // in the hash are safe (because any translation unit can only import one
     // module with each name), but result in a loss of caching.
     //
     // To avoid false-negatives, we form as canonical a path as we can, and map
     // to lower-case in case we're on a case-insensitive file system.
     SmallString<128> CanonicalPath(ModuleMapPath);
     if (getModuleMap().canonicalizeModuleMapPath(CanonicalPath))
       return {};
 
     llvm::hash_code Hash = llvm::hash_combine(CanonicalPath.str().lower());
 
     SmallString<128> HashStr;
     llvm::APInt(64, size_t(Hash)).toStringUnsigned(HashStr, /*Radix*/36);
     llvm::sys::path::append(Result, ModuleName + "-" + HashStr + ".pcm");
   }

clang/lib/Serialization/ASTWriter.cpp

void ASTWriter::WriteControlBlock(Preprocessor &PP, ASTContext &Context,
   }
 
   // Module map file
   if (WritingModule && WritingModule->Kind == Module::ModuleMapModule) {
     Record.clear();
 
     auto &Map = PP.getHeaderSearchInfo().getModuleMap();
     AddPath(WritingModule->PresumedModuleMapFile.empty()
-                ? Map.getModuleMapFileForUniquing(WritingModule)->getName()
+                ? Map.getModuleMapFileForUniquing(WritingModule)
+                      ->getNameAsRequested()
                 : StringRef(WritingModule->PresumedModuleMapFile),
             Record);
 
     // Additional module map files.
     if (auto *AdditionalModMaps =
             Map.getAdditionalModuleMapFiles(WritingModule)) {
       Record.push_back(AdditionalModMaps->size());
       SmallVector<const FileEntry *, 1> ModMaps(AdditionalModMaps->begin(),

clang/test/VFS/module-map-path.m

+// Test the module map path is consistent between clang invocations when using VFS overlays.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// Pre-populate the module cache with the modules that don't use VFS overlays.
+// RUN: %clang_cc1 -fsyntax-only -F%t/Frameworks -I%t/include %t/prepopulate_module_cache.m \
+// RUN:     -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+
+// Execute a compilation with VFS overlay. .pcm file path looks like <hash1>/ModuleName-<hash2>.pcm.
+// <hash1> corresponds to the compilation settings like language options.
+// <hash2> corresponds to the module map path. So if any of those change, we should use a different module.
+// But for VFS overlay we make an exception that it's not a part of <hash1> to reduce the number of built .pcm files.
+// Test that paths in overlays don't leak into <hash2> and don't cause using 2 .pcm files for the same module.
+// DEFINE: %{command} = %clang_cc1 -fsyntax-only -verify -F%t/Frameworks -I%t/include %t/test.m \
+// DEFINE:    -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+// RUN: sed -e "s@TMP_DIR@%{/t:regex_replacement}@g" -e "s@USE_EXTERNAL_NAMES_OPTION@@g" %t/overlay.yaml.template > %t/external-names-default.yaml
+// RUN: %{command} -ivfsoverlay %t/external-names-default.yaml
+
+// RUN: sed -e "s@TMP_DIR@%{/t:regex_replacement}@g" -e "s@USE_EXTERNAL_NAMES_OPTION@'use-external-names': true,@g" %t/overlay.yaml.template > %t/external-names-true.yaml
+// RUN: %{command} -ivfsoverlay %t/external-names-true.yaml
+
+// RUN: sed -e "s@TMP_DIR@%{/t:regex_replacement}@g" -e "s@USE_EXTERNAL_NAMES_OPTION@'use-external-names': false,@g" %t/overlay.yaml.template > %t/external-names-false.yaml
+// RUN: %{command} -ivfsoverlay %t/external-names-false.yaml
+
+//--- prepopulate_module_cache.m
+#import <Redirecting/Redirecting.h>
+
+//--- test.m
+// At first import multi-path modules directly, so clang decides which .pcm file they should belong to.
+#import <MultiPath/MultiPath.h>
+#import <MultiPathHeader.h>
+
+// Then import a module from the module cache and all its transitive dependencies.
+// Make sure the .pcm files loaded directly are the same as 'Redirecting' is referencing.
+#import <Redirecting/Redirecting.h>
+// expected-no-diagnostics
+
+
+//--- Frameworks/MultiPath.framework/Headers/MultiPath.h
+void multiPathFramework(void);
+
+//--- Frameworks/MultiPath.framework/Modules/module.modulemap
+framework module MultiPath {
+    header "MultiPath.h"
+    export *
+}
+
+
+//--- include/MultiPathHeader.h
+void multiPathHeader(void);
+
+//--- include/module.modulemap
+module MultiPathHeader {
+    header "MultiPathHeader.h"
+    export *
+}
+
+
+//--- Frameworks/Redirecting.framework/Headers/Redirecting.h
+#import <MultiPath/MultiPath.h>
+#import <MultiPathHeader.h>
+
+//--- Frameworks/Redirecting.framework/Modules/module.modulemap
+framework module Redirecting {
+    header "Redirecting.h"
+    export *
+}
+
+
+//--- BuildTemporaries/MultiPath.h
+void multiPathFramework(void);
+//--- BuildTemporaries/module.modulemap
+framework module MultiPath {
+    header "MultiPath.h"
+    export *
+}
+//--- BuildTemporaries/header.h
+void multiPathHeader(void);
+//--- BuildTemporaries/include.modulemap
+module MultiPathHeader {
+    header "MultiPathHeader.h"
+    export *
+}
+
+//--- overlay.yaml.template
+{
+  'version': 0,
+  USE_EXTERNAL_NAMES_OPTION
+  'roots': [
+    { 'name': 'TMP_DIR/Frameworks/MultiPath.framework/Headers', 'type': 'directory',
+      'contents': [
+        { 'name': 'MultiPath.h', 'type': 'file',
+          'external-contents': 'TMP_DIR/BuildTemporaries/MultiPath.h'}
+    ]},
+    { 'name': 'TMP_DIR/Frameworks/MultiPath.framework/Modules', 'type': 'directory',
+      'contents': [
+        { 'name': 'module.modulemap', 'type': 'file',
+          'external-contents': 'TMP_DIR/BuildTemporaries/module.modulemap'}
+    ]},
+    { 'name': 'TMP_DIR/include', 'type': 'directory',
+      'contents': [
+        { 'name': 'MultiPathHeader.h', 'type': 'file',
+          'external-contents': 'TMP_DIR/BuildTemporaries/header.h'},
+        { 'name': 'module.modulemap', 'type': 'file',
+          'external-contents': 'TMP_DIR/BuildTemporaries/include.modulemap'}
+    ]}
+  ]
+}
+