Skip to content

New option format for prefix mapping #10723

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

Jump to bottom
Merged
merged 7 commits into from
May 22, 2025
Merged

New option format for prefix mapping #10723

merged 7 commits into from
May 22, 2025

Conversation

sina-mahdavi
Copy link

The current option for specifying prefix mappings -fdepscan-prefix-map=<old>=<new> fails to handle paths containing = characters properly. This PR adds a 2nd form of that option -fdepscan-prefix-map old new while keeping the old format as well to fix this problem.

@akyrtzi akyrtzi requested a review from cachemeifyoucan May 21, 2025 20:09
@akyrtzi
Copy link

akyrtzi commented May 21, 2025

@swift-ci Please test LLVM

@akyrtzi akyrtzi requested a review from benlangmuir May 21, 2025 20:49
benlangmuir
benlangmuir reviewed May 21, 2025
View reviewed changes
Copy link

@benlangmuir benlangmuir left a comment

Choose a reason for hiding this comment

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

We should have a new Driver test that checks the new spelling and confirms the ordering when you mix it with the old spelling is preserved in the cc1 command line.

clang/lib/Driver/ToolChains/Clang.cpp Outdated
@@ -5093,6 +5093,18 @@ void Clang::AddPrefixMappingOptions(const ArgList &Args, ArgStringList &CmdArgs,
A->render(Args, CmdArgs);
}
}

for (const Arg *A : Args.filtered(options::OPT_fdepscan_prefix_map)) {
Copy link

Choose a reason for hiding this comment

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

We should make a single loop that handles both OPT_fdepscan_prefix_map and OPT_fdepscan_prefix_map_EQ so that the order is preserved with either spelling.

Copy link
Author

Choose a reason for hiding this comment

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

I implemented this in my recent commit, could you please check that? 😄

cachemeifyoucan
cachemeifyoucan reviewed May 21, 2025
View reviewed changes
Copy link

@cachemeifyoucan cachemeifyoucan left a comment

Choose a reason for hiding this comment

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

Current version looks good.

Can you add some lit tests to use the new flag you added and also mixing it together with the old flag?

benlangmuir
benlangmuir reviewed May 21, 2025
View reviewed changes
@sina-mahdavi sina-mahdavi force-pushed the sina-mahdavi/prefix-mapping-fix branch from e5bb87c to 40f8661 Compare May 22, 2025 00:53
@sina-mahdavi
Copy link
Author

sina-mahdavi commented May 22, 2025 •
edited
Loading

There are some check-clang-clangscandeps tests failing, and most of that comes from clang-scan-deps and the error message is just unknown argument: -fdepscan-prefix-map=.... I think there's some other place where the old format is being emitted that I also need to change. Edit: Yes, -fdepscan-prefix-map-sdk and -fdepscan-prefix-map-toolchain are still being translated to the old format.

@sina-mahdavi sina-mahdavi force-pushed the sina-mahdavi/prefix-mapping-fix branch from 40f8661 to 83b5c98 Compare May 22, 2025 01:24
benlangmuir
benlangmuir reviewed May 22, 2025
View reviewed changes
Copy link

@benlangmuir benlangmuir left a comment

Choose a reason for hiding this comment

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

The implementation LGTM, so I think we just need a new driver test that checks the behaviour of mixing this with the old flag.

@benlangmuir
Copy link

@swift-ci please test llvm

benlangmuir
benlangmuir approved these changes May 22, 2025
View reviewed changes
Copy link

@benlangmuir benlangmuir left a comment

Choose a reason for hiding this comment

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

LGTM assuming PR tests don't show any related failures. Thanks!

@sina-mahdavi
Copy link
Author

sina-mahdavi commented May 22, 2025 •
edited
Loading

LGTM assuming PR tests don't show any related failures. Thanks!

I think there's some tests failing in check-clang-cas most of which seems like false positives because the inputs are directly running frontend commands with the old option format. I'm editing those now.

Edit: should be fixed now, check-clang-cas is passing locally for me.

@sina-mahdavi
Copy link
Author

@swift-ci please test llvm

@sina-mahdavi
Copy link
Author

Alright, I think the tests are okay now. The only failing tests are in Clang :: CodeGen and LLVM :: Transforms which I'm assuming are not related to this PR (+ I've Cmd-F'd their test files and they don't include any reference to fdepscan-prefix-map).

@benlangmuir benlangmuir merged commit d13d6bd into swiftlang:next May 22, 2025
0 of 2 checks passed
@sina-mahdavi sina-mahdavi mentioned this pull request May 23, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cachemeifyoucan cachemeifyoucan cachemeifyoucan left review comments

@benlangmuir benlangmuir benlangmuir approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sina-mahdavi @akyrtzi @benlangmuir @cachemeifyoucan