This is an archive of the discontinued LLVM Phabricator instance.

[MinGW] Don't link -lmsvcrt if a different msvcrt version is to be linked
ClosedPublic

Authored by mstorsjo on Sep 6 2017, 1:19 PM.

Diff Detail

Event Timeline

mstorsjo created this revision.Sep 6 2017, 1:19 PM
rnk edited edge metadata.Sep 6 2017, 2:18 PM

What do you think of letting people spell this as -lmsvcrt120? We could forward those options and suppress our implicit addition of -lmsvcrt if we see -lmsvcr* anywhere.

lib/Driver/ToolChains/MinGW.cpp
161

I guess ld.bfd for COFF does some wacky name mangling. =/

In D37530#862644, @rnk wrote:

What do you think of letting people spell this as -lmsvcrt120? We could forward those options and suppress our implicit addition of -lmsvcrt if we see -lmsvcr* anywhere.

That might work and would probably be sensible. Does the argument order matter, if the -lmsvcr120 is listed at the end or at the same spot as -lmsvcrt were to be included?

Another complicating matter is that the pattern shouldn't just be -lmsvcr*, it should also match -lucrtbase since that's the name of the new Win10 CRT DLL (that I've just posted patches for mingw to support). And not sure if there later would be another import library for the case where one would link to api-ms-win-crt*-dll instead (which is mostly similar ucrtbase.dll but split up over a number of smaller files).

lib/Driver/ToolChains/MinGW.cpp
161

Oh, sorry - this was unrelated to this patch and slip through accidentally. (It turned out to be an issue with the lld mingw frontend, not with clang itself.)

mstorsjo updated this revision to Diff 114429.Sep 8 2017, 2:13 PM
mstorsjo retitled this revision from [MinGW] Allow overriding which version of msvcrt to link to to [MinGW] Don't link -lmsvcrt if a different msvcrt version is to be linked.
mstorsjo edited the summary of this revision. (Show Details)

Attempted implementing Reid's suggestion.

On the mingw-w64 mailing list (https://sourceforge.net/p/mingw-w64/mailman/message/36030072/), there were points made that it would be better with some mechanism that controls both linking the right msvcrt version and setting __MSVCRT_VERSION__ in sync.

And someone pointed out some widely used (but not upstreamed) patches for GCC that adds an option -mcrtdll which does pretty much exactly what my -mmsvcrt option did in the previous iteration (https://raw.githubusercontent.com/Alexpux/MINGW-packages/master/mingw-w64-gcc-git/0006-gcc-7-branch-Windows-New-feature-to-allow-overriding.patch), which in another non-upstreamed patch also is used to set __MSVCRT_VERSION__ (https://raw.githubusercontent.com/nak5124/build_env/master/gcc_build/patches/gcc/0020-MinGW-w64-Define-__MSVCRT_VERSION__.patch).

rnk accepted this revision.Sep 11 2017, 12:29 PM

lgtm

This revision is now accepted and ready to land.Sep 11 2017, 12:29 PM
rnk added a comment.Sep 11 2017, 12:30 PM

Attempted implementing Reid's suggestion.

On the mingw-w64 mailing list (https://sourceforge.net/p/mingw-w64/mailman/message/36030072/), there were points made that it would be better with some mechanism that controls both linking the right msvcrt version and setting __MSVCRT_VERSION__ in sync.

And someone pointed out some widely used (but not upstreamed) patches for GCC that adds an option -mcrtdll which does pretty much exactly what my -mmsvcrt option did in the previous iteration (https://raw.githubusercontent.com/Alexpux/MINGW-packages/master/mingw-w64-gcc-git/0006-gcc-7-branch-Windows-New-feature-to-allow-overriding.patch), which in another non-upstreamed patch also is used to set __MSVCRT_VERSION__ (https://raw.githubusercontent.com/nak5124/build_env/master/gcc_build/patches/gcc/0020-MinGW-w64-Define-__MSVCRT_VERSION__.patch).

Hm, these are interesting. Getting a separate concept of CRT version would be nice. We've talked about getting something like that for glibc as well, since there were some new math builtins we'd like to use there.

martell accepted this revision.EditedSep 11 2017, 4:56 PM

LGTM

This revision was automatically updated to reflect the committed changes.