Page MenuHomePhabricator

[WoA][MSVC] Use default linker setting in MSVC-compatible driver
ClosedPublic

Authored by maxim-kuvyrkov on Mar 12 2021, 5:10 AM.

Details

Summary

At the moment "link.exe" is hard-coded as default linker in MSVC.cpp,
so there's no way to use LLD as default linker for MSVC driver.

This patch adds checking of CLANG_DEFAULT_LINKER to MSVC.cpp.

Diff Detail

Event Timeline

maxim-kuvyrkov requested review of this revision.Mar 12 2021, 5:10 AM
maxim-kuvyrkov created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMar 12 2021, 5:11 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Added missing header to pull in definition of CLANG_DEFAULT_LINKER.

asl added inline comments.Mar 17 2021, 12:15 PM
clang/lib/Driver/ToolChains/MSVC.cpp
581

How is CLANG_DEFAULT_LINKER initialized by default? Is it just empty?

clang/lib/Driver/ToolChains/MSVC.cpp
581
asl accepted this revision.Mar 17 2021, 12:28 PM

LGTM, thanks!

This revision is now accepted and ready to land.Mar 17 2021, 12:28 PM
This revision was landed with ongoing or failed builds.Mar 18 2021, 12:44 AM
This revision was automatically updated to reflect the committed changes.

@tstellar , OK to backport to release/12.x if buildbots are happy with this?
I've filed https://bugs.llvm.org/show_bug.cgi?id=49624 with background info.

What if CLANG_DEFAULT_LINKER=ld?

This patch broke clang-ppc64le-rhel builder. The patch makes MSVC-compatible driver honour CLANG_DEFAULT_LINKER setting, which this builder sets to “lld”. Therefore there’s an expected change in the output now that CLANG_DEFAULT_LINKER can override link.exe.

This is the only builder, which sets -DCLANG_DEFAULT_LINKER to something other than the default. I’m looking how to fix this.

What if CLANG_DEFAULT_LINKER=ld?

@SureYeaah , then *-msvc targets will attempt to use "ld" linker as instructed. Or are you asking about something else?

We are seeing some internal breakage in google because CLANG_DEFAULT_LINKER is set to ld. It would be nice to make the tests more tolerant.

Can we revert this patch until then?

We are seeing some internal breakage in google because CLANG_DEFAULT_LINKER is set to ld. It would be nice to make the tests more tolerant.

Can we revert this patch until then?

I'm testing a patch to the testsuite to fix the tests. I'll revert the patch if can't get it fixes in a few hours.

phosek added a subscriber: phosek.Mar 18 2021, 6:18 PM

This is still broken, we're seeing multiple failing tests, can we please revert this change for now?

Hi @phosek ,

The only upstream bot that was broken by this change was clang-ppc64le-rhel, and I have notified the bot owner of the problem and said that I'm working on this within 15 minutes of the breakage. I posted a tentative fix for the testsuite several hours later.

I assume you have reverted this patch because you are seeing breakage in one of your internal bots. If that's the case then some details on the breakage would be useful.

They're not part of buildbot but they're not internal, you can see the log here: https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket.appspot.com/8852352747956966400/+/u/clang/test/stdout.

I noticed D98862, but it still hasn't landed and given that it's been 24 hours since your change landed and anyone who is using CLANG_DEFAULT_LINKER is currently broken (I for example use this option in my local build and tests have been failing for me locally as well), I think it's it better to revert this change and then reland it once your fix is ready.

They're not part of buildbot but they're not internal, you can see the log here: https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket.appspot.com/8852352747956966400/+/u/clang/test/stdout.

I noticed D98862, but it still hasn't landed and given that it's been 24 hours since your change landed and anyone who is using CLANG_DEFAULT_LINKER is currently broken (I for example use this option in my local build and tests have been failing for me locally as well), I think it's it better to revert this change and then reland it once your fix is ready.

Would you please test D98862 (which I've just updated) on top of this patch and see that completely fixes your bots?

They're not part of buildbot but they're not internal, you can see the log here: https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket.appspot.com/8852352747956966400/+/u/clang/test/stdout.

I noticed D98862, but it still hasn't landed and given that it's been 24 hours since your change landed and anyone who is using CLANG_DEFAULT_LINKER is currently broken (I for example use this option in my local build and tests have been failing for me locally as well), I think it's it better to revert this change and then reland it once your fix is ready.

Would you please test D98862 (which I've just updated) on top of this patch and see that completely fixes your bots?

It's passing for me locally.