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

[maxim-kuvyrkov]

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.

[maxim-kuvyrkov]

Added missing header to pull in definition of CLANG_DEFAULT_LINKER.

[asl]

clang/lib/Driver/ToolChains/MSVC.cpp

582

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

[maxim-kuvyrkov]

clang/lib/Driver/ToolChains/MSVC.cpp

582

Yes, by default it's empty.
The definition is at https://github.com/llvm/llvm-project/blob/d9ef6bc42643ae4feab3f9eca97864d72034f2ce/clang/CMakeLists.txt#L241 .

[asl]

LGTM, thanks!

[maxim-kuvyrkov]

@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.

[SureYeaah]

What if CLANG_DEFAULT_LINKER=ld?

[maxim-kuvyrkov]

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.

[maxim-kuvyrkov]

In D98493#2634089, @SureYeaah wrote:

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?

[SureYeaah]

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?

[maxim-kuvyrkov]

In D98493#2634107, @SureYeaah wrote:

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]

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

[maxim-kuvyrkov]

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.

[phosek]

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.

[maxim-kuvyrkov]

In D98493#2636616, @phosek wrote:

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?

[phosek]

In D98493#2636648, @maxim-kuvyrkov wrote:

In D98493#2636616, @phosek wrote:

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.

clang/lib/Driver/ToolChains/MSVC.cpp

 //===-- MSVC.cpp - MSVC ToolChain Implementations -------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "MSVC.h"
 #include "CommonArgs.h"
 #include "Darwin.h"
 #include "clang/Basic/CharInfo.h"
 #include "clang/Basic/Version.h"
+#include "clang/Config/config.h"
 #include "clang/Driver/Compilation.h"
 #include "clang/Driver/Driver.h"
 #include "clang/Driver/DriverDiagnostic.h"
 #include "clang/Driver/Options.h"
 #include "clang/Driver/SanitizerArgs.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/Option/Arg.h"
void visualstudio::Linker::ConstructJob(Compilation &C, const JobAction &JA,
   TC.addProfileRTLibs(Args, CmdArgs);
 
   std::vector<const char *> Environment;
 
   // We need to special case some linker paths.  In the case of lld, we need to
   // translate 'lld' into 'lld-link', and in the case of the regular msvc
   // linker, we need to use a special search algorithm.
   llvm::SmallString<128> linkPath;
-  StringRef Linker = Args.getLastArgValue(options::OPT_fuse_ld_EQ, "link");
+  StringRef Linker = Args.getLastArgValue(options::OPT_fuse_ld_EQ,
+            CLANG_DEFAULT_LINKER);
+  if (Linker.empty())
+    Linker = "link";
   if (Linker.equals_lower("lld"))
     Linker = "lld-link";
 
   if (Linker.equals_lower("link")) {
     // If we're using the MSVC linker, it's not sufficient to just use link
     // from the program PATH, because other environments like GnuWin32 install
     // their own link.exe which may come first.
     linkPath = FindVisualStudioExecutable(TC, "link.exe");