This is an archive of the discontinued LLVM Phabricator instance.

[clang][Driver] Pass correct reproduce flag to lld-link
ClosedPublic

Authored by abrachet on Aug 4 2022, 3:48 PM.

Details

Summary

Additionally, the explicit linux target has been removed from the test.

Diff Detail

Event Timeline

abrachet created this revision.Aug 4 2022, 3:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 4 2022, 3:48 PM
Herald added a subscriber: StephenFan. · View Herald Transcript
abrachet requested review of this revision.Aug 4 2022, 3:48 PM
thakis added a subscriber: mstorsjo.Aug 4 2022, 6:48 PM
thakis added inline comments.
clang/lib/Driver/Driver.cpp
1635

@mstorsjo, will this do the right thing for mingw?

mstorsjo added inline comments.Aug 4 2022, 11:47 PM
clang/lib/Driver/Driver.cpp
1635

The correct thing for the mingw linker interface would be to do the same thing as for GNU style linkers, i.e. using the unix parameter form.

(With the lld mingw interface, the canonical way to pass options through to the underlying lld-link interface is e.g. -Xlink=/reproduce:. But as an unintended loophole, as long as you pass lld-link options with a leading slash instead of a dash, e.g. if you pass e.g. /reproduce: directly to the mingw interface, the mingw interface considers it a filename and passes it through as-is to lld-link, and lld-link the interprets it as an option. But that's not the kosher way to do it IMO.)

Thus, here, I think the correct thing would be to check C.getDefaultToolChain().getTriple().isWindowsMSVCEnvironment().

abrachet updated this revision to Diff 450836.Aug 8 2022, 9:19 AM
abrachet marked 2 inline comments as done.
abrachet added inline comments.
clang/lib/Driver/Driver.cpp
1635

Thanks

thakis accepted this revision.Aug 9 2022, 5:29 AM

Thanks! LG with comment.

clang/lib/Driver/Driver.cpp
1634–1638

Twines shouldn't be on the stack. There's no reason for this to be a Twine either -- make it a StringRef (and then cast below if needed)

This revision is now accepted and ready to land.Aug 9 2022, 5:29 AM
This revision was landed with ongoing or failed builds.Aug 18 2022, 1:12 PM
This revision was automatically updated to reflect the committed changes.
abrachet marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptAug 18 2022, 1:12 PM

The removal of the explicit target broke the test on Mac: http://45.33.8.238/macm1/42754/step_7.txt

(Just need to be more permissive about leading underscores)

Please take a look and revert for now if it takes a while to fix.

The removal of the explicit target broke the test on Mac: http://45.33.8.238/macm1/42754/step_7.txt

(Just need to be more permissive about leading underscores)

Please take a look and revert for now if it takes a while to fix.

Thanks should be fixed by https://github.com/llvm/llvm-project/commit/c175d80be2ed496debb98d47f315aa5e60116768