[PATCH] D80880: [clang] [MinGW] Link kernel32 once after the last instance of msvcrt

[Martin Storsjö via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGbeeed368b602: [clang] [MinGW] Link kernel32 once after the 
last instance of msvcrt (authored by mstorsjo).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80880/new/

https://reviews.llvm.org/D80880

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


Index: clang/test/Driver/mingw-msvcrt.c
===
--- clang/test/Driver/mingw-msvcrt.c
+++ clang/test/Driver/mingw-msvcrt.c
@@ -4,6 +4,7 @@
 // RUN: %clang -v -target i686-pc-windows-gnu -lucrt -### %s 2>&1 | FileCheck 
-check-prefix=CHECK_UCRT %s
 
 // CHECK_DEFAULT: "-lmingwex" "-lmsvcrt" "-ladvapi32"
+// CHECK_DEFAULT-SAME: "-lmsvcrt" "-lkernel32" "{{.*}}crtend.o"
 // CHECK_MSVCR120: "-lmsvcr120"
 // CHECK_MSVCR120-SAME: "-lmingwex" "-ladvapi32"
 // CHECK_UCRTBASE: "-lucrtbase"
Index: clang/lib/Driver/ToolChains/MinGW.cpp
===
--- clang/lib/Driver/ToolChains/MinGW.cpp
+++ clang/lib/Driver/ToolChains/MinGW.cpp
@@ -304,10 +304,13 @@
 CmdArgs.push_back("-lkernel32");
   }
 
-  if (Args.hasArg(options::OPT_static))
+  if (Args.hasArg(options::OPT_static)) {
 CmdArgs.push_back("--end-group");
-  else
+  } else {
 AddLibGCC(Args, CmdArgs);
+if (!HasWindowsApp)
+  CmdArgs.push_back("-lkernel32");
+  }
 }
 
 if (!Args.hasArg(options::OPT_nostartfiles)) {


Index: clang/test/Driver/mingw-msvcrt.c
===
--- clang/test/Driver/mingw-msvcrt.c
+++ clang/test/Driver/mingw-msvcrt.c
@@ -4,6 +4,7 @@
 // RUN: %clang -v -target i686-pc-windows-gnu -lucrt -### %s 2>&1 | FileCheck -check-prefix=CHECK_UCRT %s
 
 // CHECK_DEFAULT: "-lmingwex" "-lmsvcrt" "-ladvapi32"
+// CHECK_DEFAULT-SAME: "-lmsvcrt" "-lkernel32" "{{.*}}crtend.o"
 // CHECK_MSVCR120: "-lmsvcr120"
 // CHECK_MSVCR120-SAME: "-lmingwex" "-ladvapi32"
 // CHECK_UCRTBASE: "-lucrtbase"
Index: clang/lib/Driver/ToolChains/MinGW.cpp
===
--- clang/lib/Driver/ToolChains/MinGW.cpp
+++ clang/lib/Driver/ToolChains/MinGW.cpp
@@ -304,10 +304,13 @@
 CmdArgs.push_back("-lkernel32");
   }
 
-  if (Args.hasArg(options::OPT_static))
+  if (Args.hasArg(options::OPT_static)) {
 CmdArgs.push_back("--end-group");
-  else
+  } else {
 AddLibGCC(Args, CmdArgs);
+if (!HasWindowsApp)
+  CmdArgs.push_back("-lkernel32");
+  }
 }
 
 if (!Args.hasArg(options::OPT_nostartfiles)) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80880: [clang] [MinGW] Link kernel32 once after the last instance of msvcrt

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In D80880#2066938 , @amccarth wrote:

> Yowza.  Mingw still links against MSVCRT?!


Sorry, I missed this comment earlier - that I see that I want to reply to.

Traditionally, mingw still links against the OS private msvcrt.dll yes. But 
nowadays it's also possible make it target UCRT instead. As the default 
`-lmsvcrt` is pretty deeply ingrained in both GCC and Clang, and there's no 
very convenient method of changing it that works on both compilers(*), the 
approach taken for using other CRTs, is to install the chosen default CRT under 
the name of `libmsvcrt.a` so that all tools use it by default. (The import 
library specifically for the system's msvcrt.dll is also available under the 
name `libmsvcrt-os.a` for disambiguation.) So in that context, `-lmsvcrt` no 
longer specifically means msvcrt.dll, in practice it just means "the default 
CRT".

As changing from one CRT to another affects a lot of the built code (e.g. CRT 
structures have different layout, inline functions in headers redirect calls to 
different physical functions in the CRT, etc), the most safe way of changing it 
is to pick a different default when building a mingw toolchain from scratch, 
which can be done by passing `--with-default-msvcrt=ucrt` when 
installing/building mingw-w64-headers and mingw-w64-crt.

*) With GCC, to change the default linked CRT, one can dump the built-in spec 
files, edit it to change `-lmsvcrt` into something else, and then build by 
passing that custom spec file to GCC with `-spec myspecfile`. MSYS2 carries (or 
at least used to carry) a non-upstream GCC patch adding a custom option for 
overriding the name of the default CRT. With Clang, we have logic that looks 
for linked libraries named `-lmsvcr*` or `-lucrt*`. If any such one is present 
on the link command line, the default implicit `-lmsvcrt` is omitted. (This 
doesn't place the custom CRT libs in the exact same spot in the link command 
line, but that doesn't matter very much for ld.lld, only for ld.bfd.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80880/new/

https://reviews.llvm.org/D80880



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

[PATCH] D80880: [clang] [MinGW] Link kernel32 once after the last instance of msvcrt

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth accepted this revision.
amccarth added a comment.

Yowza.  Mingw still links against MSVCRT?!

Anyway, I think the patch is fine.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80880/new/

https://reviews.llvm.org/D80880



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

[PATCH] D80880: [clang] [MinGW] Link kernel32 once after the last instance of msvcrt

[Mateusz Mikuła via Phabricator via cfe-commits]

mati865 added a comment.

Thanks, sounds good.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80880/new/

https://reviews.llvm.org/D80880



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

[PATCH] D80880: [clang] [MinGW] Link kernel32 once after the last instance of msvcrt

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In D80880#2066130 , @mati865 wrote:

> I don't know why `AddLibGCC` has to be called twice but that doesn't really 
> matter for this diff.


From the clang perspective, I guess it's to match GCC. Originally, I guess the 
reason is that there's some nontrivial dependencies among all the default 
linked libraries - for G++ with `-pthread`, this is what gcc links: `-lstdc++ 
-lmingw32 -lgcc_s -lgcc -lmoldname -lmingwex -lmsvcrt -lpthread -ladvapi32 
-lshell32 -luser32 -lkernel32 -lmingw32 -lgcc_s -lgcc -lmoldname -lmingwex 
-lmsvcrt`.

As there's functions in libmingwex which maybe used by libpthread - and we also 
occasionally move functions from libmingwex to libmsvcrt (for functions that 
aren't needed when on ucrt), and such functions may require both libgcc and 
libkernel32, it's all pretty fragile - a more robust solution would be to just 
add `--start-group` and `--end-group` around it (which would end up matching 
what lld always does anyway), but I'm not sure how receptive GCC would be to 
that. (Clang does add `--start-group` though, but only when linking with 
`-static`.)

As the issue in https://github.com/msys2/MINGW-packages/pull/6539 seems to end 
up resolved by using the "simpler" (linking wise) stdio functions in 
libwinpthread, lh_mouse also concluded that this added `-lkernel32` isn't 
really necessary in the end in that case, so if it ends up backed out from GCC 
I might skip pushing this one here as well.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80880/new/

https://reviews.llvm.org/D80880



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

[PATCH] D80880: [clang] [MinGW] Link kernel32 once after the last instance of msvcrt

[Mateusz Mikuła via Phabricator via cfe-commits]

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

I don't know why `AddLibGCC` has to be called twice but that doesn't really 
matter for this diff.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80880/new/

https://reviews.llvm.org/D80880



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

[PATCH] D80880: [clang] [MinGW] Link kernel32 once after the last instance of msvcrt

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In D80880#2066078 , @mati865 wrote:

> Wouldn't it better fit in `AddLibGCC`?


`AddLibGCC` is called twice, and we already add `-lkernel32` (plus a bunch of 
other libraries) after the first invocation, so that would either add another 
extra `-lkernel32` or require touching that code.

Also, with the other issue pointer out in 
https://github.com/msys2/MINGW-packages/pull/6539, I'm not sure if we also 
should try to another instance of `-lgcc` after `-lmingwex`, or move 
`-lmingwex` before `-lgcc` (as mingwex easily can contain references to libgcc 
builtins like division helper routines).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80880/new/

https://reviews.llvm.org/D80880



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

[PATCH] D80880: [clang] [MinGW] Link kernel32 once after the last instance of msvcrt

[Mateusz Mikuła via Phabricator via cfe-commits]

mati865 added a comment.

Wouldn't it better fit in `AddLibGCC`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80880/new/

https://reviews.llvm.org/D80880



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

[PATCH] D80880: [clang] [MinGW] Link kernel32 once after the last instance of msvcrt

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo created this revision.
mstorsjo added reviewers: jacek, mati865, amccarth, rnk.
Herald added a project: clang.

The msvcrt library isn't a pure import library; it does contain regular object 
files with wrappers/fallbacks, and these can require linking against kernel32.

This only makes a difference when linking with ld.bfd, as lld always searches 
all static libraries.

This matches a similar change made recently in gcc in 
https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=850533ab160ef40eccfd039e1e3b138cf26e76b8,
 although clang adds --start-group --end-group around these libraries if 
-static is specified, which gcc doesn't. But try to match gcc's linking order 
in any case, for consistency.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D80880

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


Index: clang/test/Driver/mingw-msvcrt.c
===
--- clang/test/Driver/mingw-msvcrt.c
+++ clang/test/Driver/mingw-msvcrt.c
@@ -4,6 +4,7 @@
 // RUN: %clang -v -target i686-pc-windows-gnu -lucrt -### %s 2>&1 | FileCheck 
-check-prefix=CHECK_UCRT %s
 
 // CHECK_DEFAULT: "-lmingwex" "-lmsvcrt" "-ladvapi32"
+// CHECK_DEFAULT-SAME: "-lmsvcrt" "-lkernel32" "{{.*}}crtend.o"
 // CHECK_MSVCR120: "-lmsvcr120"
 // CHECK_MSVCR120-SAME: "-lmingwex" "-ladvapi32"
 // CHECK_UCRTBASE: "-lucrtbase"
Index: clang/lib/Driver/ToolChains/MinGW.cpp
===
--- clang/lib/Driver/ToolChains/MinGW.cpp
+++ clang/lib/Driver/ToolChains/MinGW.cpp
@@ -310,10 +310,13 @@
 CmdArgs.push_back("-lkernel32");
   }
 
-  if (Args.hasArg(options::OPT_static))
+  if (Args.hasArg(options::OPT_static)) {
 CmdArgs.push_back("--end-group");
-  else
+  } else {
 AddLibGCC(Args, CmdArgs);
+if (!HasWindowsApp)
+  CmdArgs.push_back("-lkernel32");
+  }
 }
 
 if (!Args.hasArg(options::OPT_nostartfiles)) {


Index: clang/test/Driver/mingw-msvcrt.c
===
--- clang/test/Driver/mingw-msvcrt.c
+++ clang/test/Driver/mingw-msvcrt.c
@@ -4,6 +4,7 @@
 // RUN: %clang -v -target i686-pc-windows-gnu -lucrt -### %s 2>&1 | FileCheck -check-prefix=CHECK_UCRT %s
 
 // CHECK_DEFAULT: "-lmingwex" "-lmsvcrt" "-ladvapi32"
+// CHECK_DEFAULT-SAME: "-lmsvcrt" "-lkernel32" "{{.*}}crtend.o"
 // CHECK_MSVCR120: "-lmsvcr120"
 // CHECK_MSVCR120-SAME: "-lmingwex" "-ladvapi32"
 // CHECK_UCRTBASE: "-lucrtbase"
Index: clang/lib/Driver/ToolChains/MinGW.cpp
===
--- clang/lib/Driver/ToolChains/MinGW.cpp
+++ clang/lib/Driver/ToolChains/MinGW.cpp
@@ -310,10 +310,13 @@
 CmdArgs.push_back("-lkernel32");
   }
 
-  if (Args.hasArg(options::OPT_static))
+  if (Args.hasArg(options::OPT_static)) {
 CmdArgs.push_back("--end-group");
-  else
+  } else {
 AddLibGCC(Args, CmdArgs);
+if (!HasWindowsApp)
+  CmdArgs.push_back("-lkernel32");
+  }
 }
 
 if (!Args.hasArg(options::OPT_nostartfiles)) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits