[PATCH] D37727: Driver: Remove custom MinGW linker detection

[Martell Malone via Phabricator via cfe-commits]

martell closed this revision.
martell added a comment.

Landed in https://reviews.llvm.org/rL313082


Repository:
  rL LLVM

https://reviews.llvm.org/D37727



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37727: Driver: Remove custom MinGW linker detection

[Martell Malone via Phabricator via cfe-commits]

martell added a comment.

In https://reviews.llvm.org/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 :)


Repository:
  rL LLVM

https://reviews.llvm.org/D37727



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37727: Driver: Remove custom MinGW linker detection

[Reid Kleckner via Phabricator via cfe-commits]

rnk accepted this revision.
rnk added a comment.
This revision is now accepted and ready to land.

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


Repository:
  rL LLVM

https://reviews.llvm.org/D37727



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37727: Driver: Remove custom MinGW linker detection

[Martell Malone via Phabricator via cfe-commits]

martell added a comment.

In https://reviews.llvm.org/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 
> -w64-mingw32-clang that just calls clang -target -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 ) const override {
return ToolChain::CST_Libcxx;
  }
  
  const char *getDefaultLinker() const override {
return "lld";
  }

should work for what you want more.

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


Repository:
  rL LLVM

https://reviews.llvm.org/D37727



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37727: Driver: Remove custom MinGW linker detection

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

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


Repository:
  rL LLVM

https://reviews.llvm.org/D37727



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37727: Driver: Remove custom MinGW linker detection

[Martell Malone via Phabricator via cfe-commits]

martell updated this revision to Diff 114801.
martell added a comment.

update tests


Repository:
  rL LLVM

https://reviews.llvm.org/D37727

Files:
  lib/Driver/ToolChains/MinGW.cpp
  test/Driver/mingw-useld.c


Index: test/Driver/mingw-useld.c
===
--- test/Driver/mingw-useld.c
+++ test/Driver/mingw-useld.c
@@ -1,19 +1,19 @@
-// 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"
Index: lib/Driver/ToolChains/MinGW.cpp
===
--- lib/Driver/ToolChains/MinGW.cpp
+++ lib/Driver/ToolChains/MinGW.cpp
@@ -104,14 +104,6 @@
   // 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));
 
@@ -241,7 +233,7 @@
 
   if (Args.hasArg(options::OPT_static))
 CmdArgs.push_back("--end-group");
-  else if (!LinkerName.equals_lower("lld"))
+  else
 AddLibGCC(Args, CmdArgs);
 }
 
@@ -252,7 +244,7 @@
   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(JA, *this, Exec, CmdArgs, Inputs));
 }
 


Index: test/Driver/mingw-useld.c
===
--- test/Driver/mingw-useld.c
+++ test/Driver/mingw-useld.c
@@ -1,19 +1,19 @@
-// 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"
Index: lib/Driver/ToolChains/MinGW.cpp
===
--- lib/Driver/ToolChains/MinGW.cpp
+++ lib/Driver/ToolChains/MinGW.cpp
@@ 

[PATCH] D37727: Driver: Remove custom MinGW linker detection

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In https://reviews.llvm.org/D37727#867753, @martell wrote:

> In https://reviews.llvm.org/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 
`-w64-mingw32-clang` that just calls `clang -target -w64-mingw32 
-rtlib=compiler-rt -stdlib=libc++ -fuse-ld=lld -Qunused-arguments`.


Repository:
  rL LLVM

https://reviews.llvm.org/D37727



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37727: Driver: Remove custom MinGW linker detection

[Martell Malone via Phabricator via cfe-commits]

martell added a comment.

In https://reviews.llvm.org/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.




Comment at: lib/Driver/ToolChains/MinGW.cpp:107
 
-  StringRef LinkerName = Args.getLastArgValue(options::OPT_fuse_ld_EQ, "lld");
-  if (LinkerName.equals_lower("lld")) {

mstorsjo wrote:
> This diff isn't based on the current master version, where this defaults to 
> `ld`
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.




Comment at: lib/Driver/ToolChains/MinGW.cpp:236
 CmdArgs.push_back("--end-group");
-  else if (!LinkerName.equals_lower("lld"))
+  else
 AddLibGCC(Args, CmdArgs);

mstorsjo wrote:
> So if you do a clang build that defaults to `lld`, this should suddenly be 
> included?
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D37727



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37727: Driver: Remove custom MinGW linker detection

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

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.


Repository:
  rL LLVM

https://reviews.llvm.org/D37727



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37727: Driver: Remove custom MinGW linker detection

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

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?




Comment at: lib/Driver/ToolChains/MinGW.cpp:107
 
-  StringRef LinkerName = Args.getLastArgValue(options::OPT_fuse_ld_EQ, "lld");
-  if (LinkerName.equals_lower("lld")) {

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



Comment at: lib/Driver/ToolChains/MinGW.cpp:236
 CmdArgs.push_back("--end-group");
-  else if (!LinkerName.equals_lower("lld"))
+  else
 AddLibGCC(Args, CmdArgs);

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


Repository:
  rL LLVM

https://reviews.llvm.org/D37727



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits