Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
46288ff
[clang][deps] Add module map describing compiled module to file depen…
vsapsai Sep 23, 2025
377a157
Merge branch 'main' into add-extra-modulemap-to-file-dependencies
vsapsai Oct 6, 2025
3902244
Address PR comments: move module map dependency reporting to `ModuleD…
vsapsai Oct 6, 2025
f2e0215
Add a test case showing that `FileEntryRef::getName` doesn't work som…
vsapsai Oct 8, 2025
e0643f7
Switch `getName` to `getNameAsRequested`.
vsapsai Oct 8, 2025
b9928d7
Attempt to handle back slashes on Windows.
vsapsai Oct 8, 2025
8af54e0
Merge branch 'main' into add-extra-modulemap-to-file-dependencies
vsapsai Oct 8, 2025
7cd357d
Merge branch 'main' into add-extra-modulemap-to-file-dependencies
vsapsai Oct 8, 2025
bbfebe5
Try a different approach to handle backslash.
vsapsai Oct 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

SmallVector<ModuleID> DirectDeps;
for (const auto &KV : ModularDeps)
Expand Down
3 changes: 2 additions & 1 deletion clang/test/ClangScanDeps/modules-fmodule-name-no-module-built.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// CHECK-NEXT: ],
// CHECK-NEXT: "input-file": "[[PREFIX]]/modules-fmodule-name-no-module-built.m"
// CHECK-NEXT: }
3 changes: 2 additions & 1 deletion clang/test/ClangScanDeps/modules-header-sharing.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: }
Expand Down
3 changes: 2 additions & 1 deletion clang/test/ClangScanDeps/modules-implementation-module-map.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: }
Expand Down
3 changes: 2 additions & 1 deletion clang/test/ClangScanDeps/modules-implementation-private.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: }
Expand Down
Loading