[clang][deps] Add module map describing compiled module to file dependencies. #160226

vsapsai · 2025-09-23T04:03:17Z

When we add the module map describing the compiled module to the command line, add it to the file dependencies as well.

Discovered while working on reproducers where a command line input was missing in the captured files as it wasn't considered a dependency.

…dencies. When we add the module map describing the compiled module to the command line, add it to the file dependencies as well. Discovered while working on reproducers where a command line input was missing in the captured files as it wasn't considered a dependency.

llvmbot · 2025-09-23T04:03:47Z

@llvm/pr-subscribers-clang

Author: Volodymyr Sapsai (vsapsai)

Changes

When we add the module map describing the compiled module to the command line, add it to the file dependencies as well.

Discovered while working on reproducers where a command line input was missing in the captured files as it wasn't considered a dependency.

Full diff: https://github.com/llvm/llvm-project/pull/160226.diff

5 Files Affected:

(modified) clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp (+3-1)
(modified) clang/test/ClangScanDeps/modules-fmodule-name-no-module-built.m (+2-1)
(modified) clang/test/ClangScanDeps/modules-header-sharing.m (+2-1)
(modified) clang/test/ClangScanDeps/modules-implementation-module-map.c (+2-1)
(modified) clang/test/ClangScanDeps/modules-implementation-private.m (+2-1)

diff --git a/clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp b/clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
index d67178c153e88..9109ce789ee4c 100644
--- a/clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
+++ b/clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
@@ -444,9 +444,11 @@ void ModuleDepCollector::applyDiscoveredDependencies(CompilerInvocation &CI) {
       if (OptionalFileEntryRef CurrentModuleMap =
               PP.getHeaderSearchInfo()
                   .getModuleMap()
-                  .getModuleMapFileForUniquing(CurrentModule))
+                  .getModuleMapFileForUniquing(CurrentModule)) {
         CI.getFrontendOpts().ModuleMapFiles.emplace_back(
             CurrentModuleMap->getNameAsRequested());
+        Consumer.handleFileDependency(CurrentModuleMap->getNameAsRequested());
+      }
 
     SmallVector<ModuleID> DirectDeps;
     for (const auto &KV : ModularDeps)
diff --git a/clang/test/ClangScanDeps/modules-fmodule-name-no-module-built.m b/clang/test/ClangScanDeps/modules-fmodule-name-no-module-built.m
index cfe29c2bf7cdb..1fdcbc51c8e1f 100644
--- a/clang/test/ClangScanDeps/modules-fmodule-name-no-module-built.m
+++ b/clang/test/ClangScanDeps/modules-fmodule-name-no-module-built.m
@@ -46,7 +46,8 @@
 // CHECK:            "file-deps": [
 // CHECK-NEXT:         "[[PREFIX]]/modules-fmodule-name-no-module-built.m",
 // CHECK-NEXT:         "[[PREFIX]]/Inputs/header3.h",
-// CHECK-NEXT:         "[[PREFIX]]/Inputs/header.h"
+// CHECK-NEXT:         "[[PREFIX]]/Inputs/header.h",
+// CHECK-NEXT:         "Inputs/module.modulemap"
 // CHECK-NEXT:       ],
 // CHECK-NEXT:       "input-file": "[[PREFIX]]/modules-fmodule-name-no-module-built.m"
 // CHECK-NEXT:     }
diff --git a/clang/test/ClangScanDeps/modules-header-sharing.m b/clang/test/ClangScanDeps/modules-header-sharing.m
index 31ef351ec38b7..005b22dc6902a 100644
--- a/clang/test/ClangScanDeps/modules-header-sharing.m
+++ b/clang/test/ClangScanDeps/modules-header-sharing.m
@@ -79,7 +79,8 @@
 // CHECK:                ],
 // CHECK:                "file-deps": [
 // CHECK-NEXT:             "[[PREFIX]]/tu.m",
-// CHECK-NEXT:             "[[PREFIX]]/shared/H.h"
+// CHECK-NEXT:             "[[PREFIX]]/shared/H.h",
+// CHECK-NEXT:             "[[PREFIX]]/frameworks/A.framework/Modules/module.modulemap"
 // CHECK-NEXT:           ],
 // CHECK-NEXT:           "input-file": "[[PREFIX]]/tu.m"
 // CHECK-NEXT:         }
diff --git a/clang/test/ClangScanDeps/modules-implementation-module-map.c b/clang/test/ClangScanDeps/modules-implementation-module-map.c
index b7637d0c9143a..a7170aab2448c 100644
--- a/clang/test/ClangScanDeps/modules-implementation-module-map.c
+++ b/clang/test/ClangScanDeps/modules-implementation-module-map.c
@@ -28,7 +28,8 @@ framework module FWPrivate { header "private.h" }
 // CHECK:                "-fmodule-name=FWPrivate",
 // CHECK:              ],
 // CHECK:              "file-deps": [
-// CHECK-NEXT:           "[[PREFIX]]/tu.m"
+// CHECK-NEXT:           "[[PREFIX]]/tu.m",
+// CHECK-NEXT:           "[[PREFIX]]/frameworks/FW.framework/Modules/module.private.modulemap"
 // CHECK-NEXT:         ],
 // CHECK-NEXT:         "input-file": "[[PREFIX]]/tu.m"
 // CHECK-NEXT:       }
diff --git a/clang/test/ClangScanDeps/modules-implementation-private.m b/clang/test/ClangScanDeps/modules-implementation-private.m
index b376073f4b9ee..210fbfb424aca 100644
--- a/clang/test/ClangScanDeps/modules-implementation-private.m
+++ b/clang/test/ClangScanDeps/modules-implementation-private.m
@@ -65,7 +65,8 @@
 // CHECK:                "file-deps": [
 // CHECK-NEXT:             "[[PREFIX]]/tu.m",
 // CHECK-NEXT:             "[[PREFIX]]/frameworks/FW.framework/PrivateHeaders/Missed.h",
-// CHECK-NEXT:             "[[PREFIX]]/frameworks/FW.framework/Headers/FW.h"
+// CHECK-NEXT:             "[[PREFIX]]/frameworks/FW.framework/Headers/FW.h",
+// CHECK-NEXT:             "[[PREFIX]]/frameworks/FW.framework/Modules/module.modulemap"
 // CHECK-NEXT:           ],
 // CHECK-NEXT:           "input-file": "[[PREFIX]]/tu.m"
 // CHECK-NEXT:         }

vsapsai · 2025-09-23T04:04:21Z

I don't think it is worth creating a separate test case as it seems to be covered by existing ones.

vsapsai · 2025-09-23T04:08:23Z

clang/test/ClangScanDeps/modules-fmodule-name-no-module-built.m

 // CHECK-NEXT:         "[[PREFIX]]/Inputs/header3.h",
-// CHECK-NEXT:         "[[PREFIX]]/Inputs/header.h"
+// CHECK-NEXT:         "[[PREFIX]]/Inputs/header.h",
+// CHECK-NEXT:         "Inputs/module.modulemap"


I'm a little bit suspicious that here we use a relative path to the modulemap. But it is the same value we provide on the command line. cc @benlangmuir as the author of 73dba2f.

vsapsai · 2025-10-02T01:35:38Z

Ping. I'd like to know if it aligns with the architecture and direction of ModuleDepCollector.cpp. This is a pretty simple change but I don't know how it fits into a bigger picture.

benlangmuir · 2025-10-02T17:26:00Z

clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp

+                  .getModuleMapFileForUniquing(CurrentModule)) {
        CI.getFrontendOpts().ModuleMapFiles.emplace_back(
            CurrentModuleMap->getNameAsRequested());
+        Consumer.handleFileDependency(CurrentModuleMap->getNameAsRequested());


All other file dependencies go through addFileDep which handles making the file path match the expected style (absolute, with native path separators). That's why you're seeing a relative modulemap path.

I see, thanks. Switched to addFileDep as this looks like a common approach and Consumer.handleFileDependency is invoked in 1 place only.

jansvoboda11 · 2025-10-02T18:00:36Z

clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp

+                  .getModuleMapFileForUniquing(CurrentModule)) {
        CI.getFrontendOpts().ModuleMapFiles.emplace_back(
            CurrentModuleMap->getNameAsRequested());
+        Consumer.handleFileDependency(CurrentModuleMap->getNameAsRequested());


I think this should be just ->getName() - the build system won't know how to map between the as-requested path and the on-disk path.

I'd also prefer if we did this in ModuleDepCollectorPP::EndOfMainFile(). This function should just modify the CI with the information collected so far, not perform more logic and reporting. It may also get called multiple times for a driver command, and re-invoking the Consumer doesn't make sense IMO.

Moved the module map dependency reporting to ModuleDepCollectorPP::EndOfMainFile. Thanks for pointing out the better structuring.

I don't know, I'm still sticking with ->getNameAsRequested() because of 73dba2f. But don't have a strong opinion.

I think @jansvoboda11 is right. The difference here is that this is an output path for consumption by external tools, which don't know how to map the requested path to the real on-disk location, while the commit you referenced is changing an input to the compiler, which needs to have the virtual path and knows how to map it as needed.

Tested ->getName() and it fails with my use case. I'm going to try to add a test capturing this difference as the current tests pass with both getName and getNameAsRequested.

I suspect the problem might be with symlinks where getName follows a symlink and getNameAsRequested doesn't. So when we have getNameAsRequested on the command line we cannot find a module map at that location as reproducers don't capture symlinks. Maybe reproducers should capture the symlink structure but so far it wasn't required.

…epCollectorPP::EndOfMainFile`.

…etimes.

vsapsai · 2025-10-08T05:21:35Z

clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp

+    if (OptionalFileEntryRef CurrentModuleMap =
+            PP.getHeaderSearchInfo().getModuleMap().getModuleMapFileForUniquing(
+                CurrentModule))
+      MDC.addFileDep(CurrentModuleMap->getName());


With ->getName() I have

Failed Tests (1): Clang :: ClangScanDeps/modules-current-modulemap-file-dep.c

The failure is expected. The "file-deps" field is intended for consumption by tools that might not understand Clang's virtual filesystem overlays, so the only thing they can reason about is the on-disk path for files ("external-contents" in the VFS overlay file). That's of course different from what we put on the command-line for Clang itself, as Ben explained above.

Let's take a step back. What use cases do you have for "file-deps"? Because I've realized that for reproducers I'm not interested in the output paths to process them somehow. Actually, I want to take the paths and provide them to the compiler as the input.

I'm considering if we should have 2 flavours of file dependencies - one before all the file system remappings and one after clang applied the remappings.

The build system uses file-deps to check their modification time during incremental builds and figure out what needs to be recompiled.

What do you mean by input and output paths? All file dependencies are inputs currently.

How do you plan to feed the list of files to the compiler?

vsapsai · 2025-10-08T21:04:29Z

There was some CI breakage, details are in #162384 Got to merge "main" after the fix was made.

vsapsai requested a review from jansvoboda11 September 23, 2025 04:03

llvmbot added the clang Clang issues not falling into any other category label Sep 23, 2025

vsapsai commented Sep 23, 2025

View reviewed changes

benlangmuir reviewed Oct 2, 2025

View reviewed changes

jansvoboda11 reviewed Oct 2, 2025

View reviewed changes

vsapsai added 3 commits October 6, 2025 15:22

Merge branch 'main' into add-extra-modulemap-to-file-dependencies

377a157

Address PR comments: move module map dependency reporting to `ModuleD…

3902244

…epCollectorPP::EndOfMainFile`.

Add a test case showing that FileEntryRef::getName doesn't work som…

f2e0215

…etimes.

vsapsai commented Oct 8, 2025

View reviewed changes

vsapsai added 4 commits October 7, 2025 22:22

Switch getName to getNameAsRequested.

e0643f7

Attempt to handle back slashes on Windows.

b9928d7

Merge branch 'main' into add-extra-modulemap-to-file-dependencies

8af54e0

Merge branch 'main' into add-extra-modulemap-to-file-dependencies

7cd357d

Try a different approach to handle backslash.

bbfebe5

[clang][deps] Add module map describing compiled module to file dependencies. #160226

Are you sure you want to change the base?

[clang][deps] Add module map describing compiled module to file dependencies. #160226

Uh oh!

Conversation

vsapsai commented Sep 23, 2025

Uh oh!

llvmbot commented Sep 23, 2025

Uh oh!

vsapsai commented Sep 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vsapsai commented Oct 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vsapsai commented Oct 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants