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

[mstorsjo]

Summary

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.

[mati865]

Wouldn't it better fit in AddLibGCC?

[mstorsjo]

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

[mati865]

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

[mstorsjo]

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.

[mati865]

Thanks, sounds good.

[amccarth]

Yowza. Mingw still links against MSVCRT?!

Anyway, I think the patch is fine.

[mstorsjo]

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

clang/lib/Driver/ToolChains/MinGW.cpp

if (!Args.hasArg(options::OPT_nodefaultlibs)) {
           CmdArgs.push_back("-lcomdlg32");
         }
         CmdArgs.push_back("-ladvapi32");
         CmdArgs.push_back("-lshell32");
         CmdArgs.push_back("-luser32");
         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)) {
       // Add crtfastmath.o if available and fast math is enabled.
       TC.addFastMathRuntimeIfAvailable(Args, CmdArgs);
 
       CmdArgs.push_back(Args.MakeArgString(TC.GetFilePath("crtend.o")));
     }

clang/test/Driver/mingw-msvcrt.c

 // RUN: %clang -v -target i686-pc-windows-gnu -### %s 2>&1 | FileCheck -check-prefix=CHECK_DEFAULT %s
 // RUN: %clang -v -target i686-pc-windows-gnu -lmsvcr120 -### %s 2>&1 | FileCheck -check-prefix=CHECK_MSVCR120 %s
 // RUN: %clang -v -target i686-pc-windows-gnu -lucrtbase -### %s 2>&1 | FileCheck -check-prefix=CHECK_UCRTBASE %s
 // 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"
 // CHECK_UCRTBASE-SAME: "-lmingwex" "-ladvapi32"
 // CHECK_UCRT: "-lucrt"
 // CHECK_UCRT-SAME: "-lmingwex" "-ladvapi32"