Driver: Remove custom MinGW linker detection

[martell]

Summary

In rL289668 the ability to specify the default linker at compile time
was added but because the MinGW driver used custom detection we could
not take advantage of this new CMAKE flag CLANG_DEFAULT_LINKER.

[mstorsjo]

I'm not sure I like this direction. In my setups, a build-time default isn't right since I'm using the same clang builds for both native-on-linux building (with the default ld as linker) and cross-building targeting windows (using ldd), and keeping the code that reads -fuse-ld= is essential.

I haven't followed the changes closely on how this works with the build-time CLANG_DEFAULT_LINKER, but can't we just make the current code check this define if -fuse-ld isn't set?

lib/Driver/ToolChains/MinGW.cpp

107

This diff isn't based on the current master version, where this defaults to ld

236

So if you do a clang build that defaults to lld, this should suddenly be included?

[mstorsjo]

Actually, after testing this a bit more and looking at it, I withdraw my earlier complaints - this does indeed seem to work fine with my test setup. The generic TC.GetLinkerPath() picks up -fuse-ld=lld correctly and converts it into ld.lld which should be equivalent to lld -flavor gnu, at least in my linux cross compilation setup. And the extra AddLibGCC() call doesn't seem harmful for the lld setup.

[martell]

In D37727#867627, @mstorsjo wrote:

I'm not sure I like this direction. In my setups, a build-time default isn't right since I'm using the same clang builds for both native-on-linux building (with the default ld as linker) and cross-building targeting windows (using ldd), and keeping the code that reads -fuse-ld= is essential.

The GetLinkerPath function in lib/Driver/ToolChain.cpp that I replaced this with already checks -fuse-ld and does everything we need correctly.

I haven't followed the changes closely on how this works with the build-time CLANG_DEFAULT_LINKER, but can't we just make the current code check this define if -fuse-ld isn't set?

If you specifically want a toolchain that uses lld by default for MINGW but ld for the platform and or host you should override the getDefaultLinker function in the Mingw driver with an out of tree patch.
This is now what I do in my own builds since applying this patch. Hopefully when the MinGW driver becomes more stable such a patch can land in tree :)

If you are happy with all being lld or all ld you should set CLANG_DEFAULT_LINKER.
Note that GetLinkerPath also gives us the extra option to pass -fuse-ld=platform to use the platform default instead of the compile time default.

lib/Driver/ToolChains/MinGW.cpp

107

Yeah the line was supposed to read
StringRef LinkerName = Args.getLastArgValue(options::OPT_fuse_ld_EQ, "ld");
It was applied on top a commit for clang that changed ld -> lld here.
The reason I want to merge this is so that I can stop doing that :)
For the review because I am removing the line it does not matter much, I will update the diff to remove the extra l in the next revision.

236

The previous check was if not lld so it would include it if it was ld or anything else.
The MINGW lld driver does not complain about this, the behaviour should be the same for both.

[mstorsjo]

In D37727#867753, @martell wrote:

In D37727#867627, @mstorsjo wrote:

I'm not sure I like this direction. In my setups, a build-time default isn't right since I'm using the same clang builds for both native-on-linux building (with the default ld as linker) and cross-building targeting windows (using ldd), and keeping the code that reads -fuse-ld= is essential.

The GetLinkerPath function in lib/Driver/ToolChain.cpp that I replaced this with already checks -fuse-ld and does everything we need correctly.

Yup, figured it out later after reading it more thorhoughly later.

I haven't followed the changes closely on how this works with the build-time CLANG_DEFAULT_LINKER, but can't we just make the current code check this define if -fuse-ld isn't set?

If you specifically want a toolchain that uses lld by default for MINGW but ld for the platform and or host you should override the getDefaultLinker function in the Mingw driver with an out of tree patch.
This is now what I do in my own builds since applying this patch.

I do the same by adding -rtlib=compiler-rt -stdlib=libc++ -fuse-ld=lld -Qunused-arguments in the frontend script wrapper (a wrapper named <arch>-w64-mingw32-clang that just calls clang -target <arch>-w64-mingw32 -rtlib=compiler-rt -stdlib=libc++ -fuse-ld=lld -Qunused-arguments.

[martell]

update tests

[mstorsjo]

Looks good to me (but maybe someone else should approve it as well)

[martell]

In D37727#867751, @mstorsjo wrote:

I do the same by adding -rtlib=compiler-rt -stdlib=libc++ -fuse-ld=lld -Qunused-arguments in the frontend script wrapper (a wrapper named <arch>-w64-mingw32-clang that just calls clang -target <arch>-w64-mingw32 -rtlib=compiler-rt -stdlib=libc++ -fuse-ld=lld -Qunused-arguments.

I used to do this also but I didn't want to just disable warnings for unused arguments which left me with problems.

The following should work for that if you are happy with it being the default for your linux target also

-DCLANG_DEFAULT_RTLIB=compiler-rt
-DCLANG_DEFAULT_CXX_STDLIB=libc++
-CLANG_DEFAULT_LINKER=lld

Though adding something like this to Driver/MinGW.h

RuntimeLibType GetDefaultRuntimeLibType() const override {
  return RuntimeLibType::RLT_CompilerRT;
}

CXXStdlibType
GetCXXStdlibType(const llvm::opt::ArgList &Args) const override {
  return ToolChain::CST_Libcxx;
}

const char *getDefaultLinker() const override {
  return "lld";
}

should work for what you want more. (assuming you want your other targets to stay the same)

Yup, figured it out later after reading it more thorhoughly later.

We posted at roughly the same time so I missed it :)

Actually, after testing this a bit more and looking at it, I withdraw my earlier complaints - this does indeed seem to work fine with my test setup. The generic TC.GetLinkerPath() picks up -fuse-ld=lld correctly and converts it into ld.lld which should be equivalent to lld -flavor gnu, at least in my linux cross compilation setup. And the extra AddLibGCC() call doesn't seem harmful for the lld setup.

Great, I'll just wait for reid or peter to approve and merge then.

[rnk]

Nice! IIUC, now that the gnu ld driver works on Windows we can remove this logic. Thanks!

[martell]

In D37727#868284, @rnk wrote:

Nice! IIUC, now that the gnu ld driver works on Windows we can remove this logic. Thanks!

We could have removed it before the addition of the gnu ld driver, I think the author just missed this target and possibly others when the original patch was created.

Regardless, this now enables us to make standalone mingw-w64 clang toolchains with only llvm+clang+lld+compiler-rt without having any out of tree patches :)

[martell]

Landed in rL313082

lib/Driver/ToolChains/MinGW.cpp

void tools::MinGW::Linker::ConstructJob(Compilation &C, const JobAction &JA,
   // Silence warning for "clang -g foo.o -o foo"
   Args.ClaimAllArgs(options::OPT_g_Group);
   // and "clang -emit-llvm foo.o -o foo"
   Args.ClaimAllArgs(options::OPT_emit_llvm);
   // and for "clang -w foo.o -o foo". Other warning options are already
   // handled somewhere else.
   Args.ClaimAllArgs(options::OPT_w);
 
-  StringRef LinkerName = Args.getLastArgValue(options::OPT_fuse_ld_EQ, "ld");
-  if (LinkerName.equals_lower("lld")) {
-    CmdArgs.push_back("-flavor");
-    CmdArgs.push_back("gnu");
-  } else if (!LinkerName.equals_lower("ld")) {
-    D.Diag(diag::err_drv_unsupported_linker) << LinkerName;
-  }
-
   if (!D.SysRoot.empty())
     CmdArgs.push_back(Args.MakeArgString("--sysroot=" + D.SysRoot));
 
   if (Args.hasArg(options::OPT_s))
     CmdArgs.push_back("-s");
 
   CmdArgs.push_back("-m");
   switch (TC.getArch()) {
if (!Args.hasArg(options::OPT_nodefaultlibs)) {
       }
       CmdArgs.push_back("-ladvapi32");
       CmdArgs.push_back("-lshell32");
       CmdArgs.push_back("-luser32");
       CmdArgs.push_back("-lkernel32");
 
       if (Args.hasArg(options::OPT_static))
         CmdArgs.push_back("--end-group");
-      else if (!LinkerName.equals_lower("lld"))
+      else
         AddLibGCC(Args, CmdArgs);
     }
 
     if (!Args.hasArg(options::OPT_nostartfiles)) {
       // Add crtfastmath.o if available and fast math is enabled.
       TC.AddFastMathRuntimeIfAvailable(Args, CmdArgs);
 
       CmdArgs.push_back(Args.MakeArgString(TC.GetFilePath("crtend.o")));
     }
   }
-  const char *Exec = Args.MakeArgString(TC.GetProgramPath(LinkerName.data()));
+  const char *Exec = Args.MakeArgString(TC.GetLinkerPath());
   C.addCommand(llvm::make_unique<Command>(JA, *this, Exec, CmdArgs, Inputs));
 }
 
 // Simplified from Generic_GCC::GCCInstallationDetector::ScanLibDirForGCCTriple.
 static bool findGccVersion(StringRef LibDir, std::string &GccLibDir,
                            std::string &Ver) {
   auto Version = toolchains::Generic_GCC::GCCVersion::Parse("0.0.0");
   std::error_code EC;

test/Driver/mingw-useld.c

-// RUN: %clang -### -target i686-pc-windows-gnu --sysroot=%S/Inputs/mingw_clang_tree/mingw32 %s 2>&1 | FileCheck -check-prefix=CHECK_LD_32 %s
-// CHECK_LD_32: ld{{(.exe)?}}"
+// RUN: %clang -### -target i686-pc-windows-gnu --sysroot=%S/Inputs/mingw_clang_tree/mingw32 %s -fuse-ld=platform 2>&1 | FileCheck -check-prefix=CHECK_LD_32 %s
+// CHECK_LD_32: "{{[^"]*}}ld{{(.exe)?}}"
 // CHECK_LD_32: "i386pe"
-// CHECK_LD_32-NOT: "-flavor" "gnu"
+// CHECK_LD_32-NOT: "{{[^"]*}}ld.lld{{(.exe)?}}"
 
 // RUN: %clang -### -target i686-pc-windows-gnu --sysroot=%S/Inputs/mingw_clang_tree/mingw32 %s -fuse-ld=lld 2>&1 | FileCheck -check-prefix=CHECK_LLD_32 %s
 // CHECK_LLD_32-NOT: invalid linker name in argument
-// CHECK_LLD_32: lld{{(.exe)?}}" "-flavor" "gnu"
+// CHECK_LLD_32: "{{[^"]*}}ld.lld{{(.exe)?}}"
 // CHECK_LLD_32: "i386pe"
 
 // RUN: %clang -### -target x86_64-pc-windows-gnu --sysroot=%S/Inputs/mingw_clang_tree/mingw32 %s -fuse-ld=lld 2>&1 | FileCheck -check-prefix=CHECK_LLD_64 %s
 // CHECK_LLD_64-NOT: invalid linker name in argument
-// CHECK_LLD_64: lld{{(.exe)?}}" "-flavor" "gnu"
+// CHECK_LLD_64: "{{[^"]*}}ld.lld{{(.exe)?}}"
 // CHECK_LLD_64: "i386pep"
 
 // RUN: %clang -### -target arm-pc-windows-gnu --sysroot=%S/Inputs/mingw_clang_tree/mingw32 %s -fuse-ld=lld 2>&1 | FileCheck -check-prefix=CHECK_LLD_ARM %s
 // CHECK_LLD_ARM-NOT: invalid linker name in argument
-// CHECK_LLD_ARM: lld{{(.exe)?}}" "-flavor" "gnu"
+// CHECK_LLD_ARM: "{{[^"]*}}ld.lld{{(.exe)?}}"
 // CHECK_LLD_ARM: "thumb2pe"