[Clang][ScanDeps] Use the virtual path for module maps

[Bigcheese]

Summary

This patch makes clang-scan-deps use the virtual path for module maps instead of the on disk path. This is needed so that modulemap relative lookups are done correctly in the actual module builds. The file dependencies still use the on disk path as that's what matters for build invalidation.

[jansvoboda11]

LGTM provided the Windows crash gets resolved.

[Bigcheese]

There were two issues for Windows.

The first is an actual bug in the Clang VFS that should be fixed separately. You cannot pass just main.m as the input to Clang. Clang asks for its parent directory textually, which is .. It then tries to stat ., which the VFS rejects. Not sure why it's OK on non-Windows.

The other issue is not handling path separators in the FileCheck test. Fixed by adding sed 's:\\\\\?:/:g'.

I'm uploading a new patch and will commit on clean precommit tests.

[rsmith]

clang/test/ClangScanDeps/modulemap-via-vfs.m

6–7

There seems to be a problem with this test: it's writing a module cache to the current working directory instead of writing it into somewhere under %t. Presumably this just requires adding a module cache path parameter?

[lenary]

clang/test/ClangScanDeps/modulemap-via-vfs.m

6–7

I think the easiest fix is to change the command string in the json below, to update the passed -fmodules-cache-path. I'm working on a patch right now.

[lenary]

@Bigcheese have you got a fix for the vfs issue with . and ..?

Looking closely at your comment, I'm surprised you landed this without an llvm-lit xfail for windows, given your comment above.

[lenary]

Update: I'm going to revert this on main. This patch was accepted conditionally on you fixing the windows crash, which you have not done, so this was landed without being accepted.

Revert is here: https://reviews.llvm.org/rGea835171389a

When you re-land this, don't forget to include the changes from D116611, please.

[Bigcheese]

In D114206#3221893, @lenary wrote:

Update: I'm going to revert this on main. This patch was accepted conditionally on you fixing the windows crash, which you have not done, so this was landed without being accepted.

Revert is here: https://reviews.llvm.org/rGea835171389a

When you re-land this, don't forget to include the changes from D116611, please.

There is no crash on Windows with this patch. The issue is if you pass main.m instead of DIR/main.m. The commit that landed had both fixes I mentioned. Did you try this on Windows or look at the bots? In the future please be more careful.

[lenary]

In D114206#3221950, @Bigcheese wrote:

In D114206#3221893, @lenary wrote:

Update: I'm going to revert this on main. This patch was accepted conditionally on you fixing the windows crash, which you have not done, so this was landed without being accepted.

Revert is here: https://reviews.llvm.org/rGea835171389a

When you re-land this, don't forget to include the changes from D116611, please.

There is no crash on Windows with this patch. The issue is if you pass main.m instead of DIR/main.m. The commit that landed had both fixes I mentioned. Did you try this on Windows or look at the bots? In the future please be more careful.

I did try this on windows, and I'm still seeing a crash about traversal segments, I'll continue investigating that crash.

[lenary]

@Bigcheese I am sorry - after a lot of further investigation, it was due to an odd interaction with some downstream changes. I will re-land your changes, with correct attribution to you.

clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp

ModuleID ModuleDepCollectorPP::handleTopLevelModule(const Module *M) {
   MD.ImportedByMainFile = DirectModularDeps.contains(M);
   MD.ImplicitModulePCMPath = std::string(M->getASTFile()->getName());
   MD.IsSystem = M->IsSystem;
 
   const FileEntry *ModuleMap = MDC.ScanInstance.getPreprocessor()
                                    .getHeaderSearchInfo()
                                    .getModuleMap()
                                    .getModuleMapFileForUniquing(M);
-  MD.ClangModuleMapFile = std::string(ModuleMap ? ModuleMap->getName() : "");
+
+  if (ModuleMap) {
+    StringRef Path = ModuleMap->tryGetRealPathName();
+    if (Path.empty())
+      Path = ModuleMap->getName();
+    MD.ClangModuleMapFile = std::string(Path);
+  }
 
   serialization::ModuleFile *MF =
       MDC.ScanInstance.getASTReader()->getModuleManager().lookup(
           M->getASTFile());
   MDC.ScanInstance.getASTReader()->visitInputFiles(
       *MF, true, true, [&](const serialization::InputFile &IF, bool isSystem) {
         // __inferred_module.map is the result of the way in which an implicit
         // module build handles inferred modules. It adds an overlay VFS with

clang/test/ClangScanDeps/modulemap-via-vfs.m

+// RUN: rm -rf %t.dir
+// RUN: split-file %s %t.dir
+// RUN: sed -e "s|DIR|%/t.dir|g" %t.dir/build/compile-commands.json.in > %t.dir/build/compile-commands.json
+// RUN: sed -e "s|DIR|%/t.dir|g" %t.dir/build/vfs.yaml.in > %t.dir/build/vfs.yaml
+// RUN: clang-scan-deps -compilation-database %t.dir/build/compile-commands.json -j 1 -format experimental-full \
+// RUN:   -mode preprocess-minimized-sources -generate-modules-path-args > %t.db
+// RUN: %python %S/../../utils/module-deps-to-rsp.py %t.db --module-name=A > %t.A.cc1.rsp
+// RUN: cat %t.A.cc1.rsp | sed 's:\\\\\?:/:g' | FileCheck %s
+
+// CHECK-NOT: build/module.modulemap
+// CHECK: A/module.modulemap
+
+//--- build/compile-commands.json.in
+
+[
+{
+  "directory": "DIR",
+  "command": "clang DIR/main.m -Imodules/A -fmodules -fmodules-cache-path=DIR/module-cache -fimplicit-modules -fimplicit-module-maps -ivfsoverlay build/vfs.yaml",
+  "file": "DIR/main.m"
+}
+]
+
+//--- build/module.modulemap
+
+module A {
+  umbrella header "A.h"
+}
+
+//--- modules/A/A.h
+
+typedef int A_t;
+
+//--- build/vfs.yaml.in
+
+{
+  "version": 0,
+  "case-sensitive": "false",
+  "roots": [
+  {
+     "contents": [
+     {
+        "external-contents": "DIR/build/module.modulemap",
+        "name": "module.modulemap",
+        "type": "file"
+     }],
+     "name": "DIR/modules/A",
+     "type": "directory"
+  }
+  ]
+}
+
+//--- main.m
+
+@import A;
+
+A_t a = 0;