Page MenuHomePhabricator

[libc++] [CMake] Link with /nodefaultlibs on Windows
ClosedPublic

Authored by EricWF on Jan 6 2017, 10:00 PM.

Details

Summary

This patch attempts to fix the libc++ build/link so that it doesn't use an default C++ libraries on Windows. This is needed to prevent linking to MSVC's STL library.

Additionally this patch changes libc++ so that it is always linked with the non-debug DLL's (e.g. /MD). This is needed so that the test suite can correctly link the same libraries without needing to know which configuration c++.dll was linked with.

Diff Detail

Repository
rL LLVM

Event Timeline

EricWF updated this revision to Diff 83508.Jan 6 2017, 10:00 PM
EricWF retitled this revision from to [libc++] [CMake] Link with /nodefaultlibs on Windows.
EricWF updated this object.

Adding cfe-commits

smeenai added inline comments.Jan 6 2017, 10:22 PM
CMakeLists.txt
43 ↗(On Diff #83508)

Not the biggest fan of this name, since it's not obvious why MinGW shouldn't count as targeting Windows. I thought of LIBCXX_TARGETING_NATIVE_WINDOWS or LIBCXX_TARGETING_MSVCRT instead, but MinGW is also native Windows and targets MSVCRT, so those names aren't any better from that perspective either. I can't think of anything else at the moment, but I'm sure there's a better name.

388 ↗(On Diff #83508)

cl (and presumably clang-cl) also accepts all these flags with a leading dash instead of a leading slash, and cmake at least has a tendency to do -D instead of /D, so you might need to take those into account as well. Also, what about the other potential /RTC options?

lib/CMakeLists.txt
108–109 ↗(On Diff #83508)

These should be guarded under a check for a cl or cl-like frontend rather than LIBCXX_TARGETING_WINDOWS (since in theory we could be using the regular clang frontend to compile for Windows as well).

111 ↗(On Diff #83508)

Idk if there's anything specific to C++ in vcruntime; it's more compiler runtime functions as far as I know.

113 ↗(On Diff #83508)

Maybe add a comment explaining the purpose of this one as well?

halyavin added inline comments.Jan 6 2017, 11:41 PM
lib/CMakeLists.txt
108–109 ↗(On Diff #83508)

Regular clang supports both gcc-like and cl-like options (there are 2 compilers: clang.exe and clang-cl.exe). I think it is not worth it to support both considering they differ only in command line options handling.

111 ↗(On Diff #83508)

It contains exception handling stuff.

smeenai added inline comments.Jan 7 2017, 12:18 AM
lib/CMakeLists.txt
108–109 ↗(On Diff #83508)

I'm aware of the separate drivers, but I still think it's worthwhile specifying appropriate conditionals when it's easy enough to do. (In this case, the inverse check of https://reviews.llvm.org/diffusion/L/browse/libcxx/trunk/CMakeLists.txt;291339$394 should do the trick.)

111 ↗(On Diff #83508)

You're right, but it also contains longjmp, memcpy, memmove, memset, etc, which is why I found the comment slightly weird initially. I guess it's fairly accurate as far as the usage of vcruntime in libc++ goes though.

EricWF added inline comments.Jan 7 2017, 2:31 AM
CMakeLists.txt
388 ↗(On Diff #83508)

My goal here is only to remove flags which CMake adds by default as part of CMAKE_CXX_FLAGS_<BUILD_TYPE>_INIT.

lib/CMakeLists.txt
108–109 ↗(On Diff #83508)

Is it alright if libc++'s CMakeLists.txt only supports targeting clang-cl for the time being? I would love to also support clang++ but that seems like a ways off.

For now my only concern is getting clang-cl working. Expanding to include supporting clang++ seems like it's a ways away. @smeenai Would this be a regression for you?

111 ↗(On Diff #83508)

FYI here is the documentation I was reading when deciding what libraries to link: https://msdn.microsoft.com/en-us/library/abx4dbyh.aspx

113 ↗(On Diff #83508)

Not sure what the comment should read. I originally implemented this patch without this library, but during testing it generated a undefined reference to ___PLEASE_LINK_WITH_iso_stdio_wide_specifiers.lib. So should the comment read "MSVC told me to link this"?

EricWF added inline comments.Jan 7 2017, 2:47 AM
CMakeLists.txt
43 ↗(On Diff #83508)

Thanks for the feedback. I'm not exactly sure how this macro should be defined either.

I thought that MinGW provided its own C library runtime wrappers that forward to MSVCRT.

The difference I was imagining between Native Windows builds and MinGW is that it's possible to use
pthread with MinGW but not with native Windows targets.

halyavin added inline comments.Jan 7 2017, 3:53 AM
lib/CMakeLists.txt
112 ↗(On Diff #83508)

As far as I know, applications shouldn't use msvcrt.dll (Windows is not a Microsoft Visual C/C++ Run-Time delivery channel) Can we survive on ucrt only?

halyavin added inline comments.Jan 7 2017, 4:05 AM
lib/CMakeLists.txt
112 ↗(On Diff #83508)

Oh, it is static library that doesn't correspond to a dll.

EricWF added inline comments.Jan 7 2017, 4:07 AM
CMakeLists.txt
43 ↗(On Diff #83508)

Another distinction I would like this macro to embody is whether on not the compiler defines _MSC_VER. clang-cl, clang++ on Windows, and MSVC cl all define this macro.

lib/CMakeLists.txt
112 ↗(On Diff #83508)

I don't think that link suggests that applications shouldn't link msvcrt.dll.

Instead all of the doc I see suggests that msvcrt.dll is linked implicitly by /MD. However since libc++ removes /MD and adds /nodefaultlibs we need to manually re-create the /MD link without the MSVC STL. That involves manually linking msvcrt.dll.

halyavin added inline comments.Jan 7 2017, 4:31 AM
lib/CMakeLists.txt
113 ↗(On Diff #83508)

From very few hits Google gave me, it looks like this library implements proper behavior for %s and %c in printfw/scanfw-like functions.

EricWF added inline comments.Jan 7 2017, 4:47 AM
lib/CMakeLists.txt
113 ↗(On Diff #83508)

That would make sense since the undefined symbols were in locale.cpp. I guess I'll update the comment to say "# required for wide character formatting (e.g. printfw/scanfw)"

EricWF updated this revision to Diff 83526.Jan 7 2017, 4:49 AM
  • Add comment explaining need for iso_stdio_wide_specifiers.lib.
halyavin added inline comments.Jan 7 2017, 4:55 AM
lib/CMakeLists.txt
113 ↗(On Diff #83508)

It is required for correct wide character formatting functions. Hence the ISO in the name. They were previously implemented incorrectly and this library fixes this (probably just flips a switch between legacy and correct behavior).

smeenai edited edge metadata.Jan 7 2017, 9:53 AM

One potential issue with always forcing the non-debug msvcrt is that any apps that link against libc++ would also then need to use the non-debug CRT libraries, otherwise you'd get all sorts of fun runtime errors (e.g. cross-domain frees). I think the default Visual Studio settings are to link against msvcrtd, ucrtd, etc. for debug builds and msvcrt, ucrt, etc. for release builds, so having libc++ unconditionally use the non-debug versions is probably bad for interoperability.

I think the right thing to do would be to either always compile two versions of libc++, one linked against the debug libraries and the other against the non-debug libraries, or make the Debug configuration use the debug libraries and the Release configuration use the release libraries (and have config.py deal with this as well).

CMakeLists.txt
43 ↗(On Diff #83508)

If I recall correctly, MinGW uses mscvrt.dll but not msvcrt.lib (see my other comment for the distinction). I'm fine with this name for now then; we can always bikeshed later.

Btw it's also possible to use pthread with native Windows targets, via pthreads-win32 or other libraries.

388 ↗(On Diff #83508)

Fair enough, works for me.

lib/CMakeLists.txt
108–109 ↗(On Diff #83508)

It would. It's easy enough to work around locally, so I don't care too much, but it's also easy enough to not regress in the first place :p Would using add_flags_if_supported and add_link_flags_if_supported instead of the unconditional versions here work?

111 ↗(On Diff #83508)

Yeah. I guess it's kinda sorta serving as the ABI library here, almost.

112 ↗(On Diff #83508)

There's a distinction between msvcrt.lib and msvcrt.dll. msvcrt.lib is a static library which contains the entry point (_DllMainCRTStartup@12, etc.). It's basically the equivalent of crtbegin for Windows. msvcrt.dll is the unversioned legacy version of the C runtime, which is what you're not supposed to use. It's kinda confusing since the normal convention is for X.lib to be the import library corresponding to X.dll, but that's not the case for msvcrt.

113 ↗(On Diff #83508)

Yup. Basically, without it, in a wide printf format string, %s corresponds to a wide string and %S corresponds to a narrow string. With it, %s corresponds to a narrow string and %ls corresponds to a wide string. As @halyavin said, I would update the comment to mention correct/standards compliant handling of wide format strings.

rnk added inline comments.Jan 9 2017, 2:03 PM
CMakeLists.txt
43 ↗(On Diff #83508)

I'd call this LIBCXX_TARGETING_MSVCRT. "msvcrt.dll" usually refers to an ancient version (6?) of the Visual C runtime that is distributed as part of Windows. Typically it is found at C:/Windows/system32/msvcrt.dll. Mingw uses this, because it is available on all Windows systems they care to support. This DLL basically never receives updates, so I wouldn't want to build libc++ functionality on top of it. Over time, it seems that mingw has had to implement more and more C runtime functionality itself, and I wouldn't want libc++ to have to do that.

Until recently, MSVC users were required to redistribute the version of the CRT they chose to link against, and it was expected that all DLLs sharing CRT resources had to link against the same CRT version. Of course, this caused problems, so they went back to the single, OS-provided CRT in VS 2015 (https://blogs.msdn.microsoft.com/vcblog/2015/03/03/introducing-the-universal-crt/).

My conclusion is that it's no longer worth thinking of that old DLL as the Visual C runtime. It's just an artifact of history at this point. If you say libc++ targets the MSVCRT, you're probably talking about the modern, universal CRT.


If you want this option to imply both the C++ ABI and the C runtime, then LIBCXX_TARGETING_MSVC would probably be a more appropriate name. Clang and LLVM have some support for using the Itanium C++ ABI with the MSVCRT, but it is more experimental, and not ABI compatible with any existing software. Saleem could say more about where he thinks this target will go in the future and whether it's worth adding to the libc++ support configuration matrix.

lib/CMakeLists.txt
108–109 ↗(On Diff #83508)

Any reason not to use if (MSVC)? That generally implies that the compiler supports MSVC flags. It's true for clang-cl, at least.

If you don't like that, we should probably make some attempt to identify the linker command line syntax and use the appropriate flag.

One potential issue with always forcing the non-debug msvcrt is that any apps that link against libc++ would also then need to use the non-debug CRT libraries, otherwise you'd get all sorts of fun runtime errors (e.g. cross-domain frees). I think the default Visual Studio settings are to link against msvcrtd, ucrtd, etc. for debug builds and msvcrt, ucrt, etc. for release builds, so having libc++ unconditionally use the non-debug versions is probably bad for interoperability.

I agree. Forcing the use of release libraries is just a starting point.

I think the right thing to do would be to either always compile two versions of libc++, one linked against the debug libraries and the other against the non-debug libraries, or make the Debug configuration use the debug libraries and the Release configuration use the release libraries (and have config.py deal with this as well).

Is it OK if this is implemented *after* this patch?

CMakeLists.txt
43 ↗(On Diff #83508)

My conclusion is that it's no longer worth thinking of that old DLL as the Visual C runtime. It's just an artifact of history at this point. If you say libc++ targets the MSVCRT, you're probably talking about the modern, universal CRT.

According to the docs msvcrt now contains the "startup files" for the universal CRT.

If you want this option to imply both the C++ ABI and the C runtime, then LIBCXX_TARGETING_MSVC would probably be a more appropriate name.

Ack. Changing it to LIBCXX_TARGETING_MSVC and enabling it using the MSVC CMake variable.

lib/CMakeLists.txt
112 ↗(On Diff #83508)

msvcrt.dll doesn't exist anymore according to this

EricWF updated this revision to Diff 84414.Jan 13 2017, 6:35 PM
EricWF edited edge metadata.

Attempt to address review comments

  • Rename LIBCXX_TARGETING_WINDOWS to LIBCXX_TARGETING_MSVC. It is set to on when CMake defines MSVC to ON. This macro is intented to represent cases where we are targeting native Windows and the native CRT.

There has been a lot of noise on this patch. What else needs to be done before committing?

I think the right thing to do would be to either always compile two versions of libc++, one linked against the debug libraries and the other against the non-debug libraries, or make the Debug configuration use the debug libraries and the Release configuration use the release libraries (and have config.py deal with this as well).

Is it OK if this is implemented *after* this patch?

I guess so. It feels slightly ugly because there's a bit of fighting the build system going on in this patch (w.r.t. getting rid of flags), but this is also strictly better than what we have right now, so sure.

CMakeLists.txt
488 ↗(On Diff #84414)

Isn't this redundant, considering the define_if_not(MSVC) below? I suppose that can be changed to an unconditional define.

lib/CMakeLists.txt
113 ↗(On Diff #84414)

Would be slightly more specific: "required for standards-compliant wide-character formatting functions"

smeenai accepted this revision.Jan 13 2017, 10:04 PM
smeenai edited edge metadata.

There's a bunch of follow-up work here, but I'm fine with this for now.

This revision is now accepted and ready to land.Jan 13 2017, 10:04 PM
smeenai added inline comments.Jan 13 2017, 10:12 PM
lib/CMakeLists.txt
112 ↗(On Diff #83508)

It's not documented anymore (because they don't want you using it), but you can find it in C:\Windows\system32, and MinGW still linked against it, the last time I checked.

EricWF marked an inline comment as done.Jan 13 2017, 10:15 PM
EricWF added inline comments.
lib/CMakeLists.txt
113 ↗(On Diff #84414)

Sure, I'll make the change.

Although if we can't link libc++ we can't really provide any standards conformance :-P

EricWF updated this revision to Diff 84428.Jan 13 2017, 10:16 PM
EricWF edited edge metadata.
  • Address inline comments.
EricWF closed this revision.Jan 13 2017, 10:17 PM
This revision was automatically updated to reflect the committed changes.