[PATCH] D28441: [libc++] [CMake] Link with /nodefaultlibs on Windows

Sat Jan 7 09:53:24 PST 2017

smeenai added a comment.

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

================
Comment at: CMakeLists.txt:43
+if (WIN32 AND NOT MINGW)
+  set(LIBCXX_TARGETING_WINDOWS ON)
+else()
----------------
EricWF wrote:
> EricWF wrote:
> > smeenai wrote:
> > > 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.
> > 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. 
> 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.
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](http://www.sourceware.org/pthreads-win32/) or other libraries.

================
Comment at: CMakeLists.txt:388
+# non-debug DLLs
+remove_flags("/D_DEBUG" "/MTd" "/MDd" "/MT" "/Md" "/RTC1")
+
----------------
EricWF wrote:
> smeenai wrote:
> > 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?
> My goal here is only to remove flags which CMake adds by default as part of `CMAKE_CXX_FLAGS_<BUILD_TYPE>_INIT`.
Fair enough, works for me.

================
Comment at: lib/CMakeLists.txt:108-109
+if (LIBCXX_TARGETING_WINDOWS)
+  add_compile_flags(/Zl)
+  add_link_flags(/nodefaultlib)
+  add_library_flags(ucrt) # Universal C runtime
----------------
EricWF wrote:
> smeenai wrote:
> > halyavin wrote:
> > > smeenai wrote:
> > > > 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).
> > > 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.
> > 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.)
> 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?
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?

================
Comment at: lib/CMakeLists.txt:111
+  add_library_flags(ucrt) # Universal C runtime
+  add_library_flags(vcruntime) # C++ runtime
+  add_library_flags(msvcrt) # C runtime startup files
----------------
EricWF wrote:
> smeenai wrote:
> > halyavin wrote:
> > > smeenai wrote:
> > > > Idk if there's anything specific to C++ in vcruntime; it's more compiler runtime functions as far as I know.
> > > It contains exception handling stuff.
> > 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.
> FYI here is the documentation I was reading when deciding what libraries to link: https://msdn.microsoft.com/en-us/library/abx4dbyh.aspx
Yeah. I guess it's kinda sorta serving as the ABI library here, almost.

================
Comment at: lib/CMakeLists.txt:112
+  add_library_flags(vcruntime) # C++ runtime
+  add_library_flags(msvcrt) # C runtime startup files
+  add_library_flags(iso_stdio_wide_specifiers)
----------------
EricWF wrote:
> halyavin wrote:
> > halyavin wrote:
> > > As far as I know, applications shouldn't use msvcrt.dll ([[ https://blogs.msdn.microsoft.com/oldnewthing/20140411-00/?p=1273 | Windows is not a Microsoft Visual C/C++ Run-Time delivery channel ]]) Can we survive on ucrt only?
> > Oh, it is static library that doesn't correspond to a dll.
> 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`.
There's a distinction between `msvcrt.lib` and `msvcrt.dll`. `msvcrt.lib` is a static library which contains the entry point (`_DllMainCRTStartup at 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`.

================
Comment at: lib/CMakeLists.txt:113
+  add_library_flags(msvcrt) # C runtime startup files
+  add_library_flags(iso_stdio_wide_specifiers)
+endif()
----------------
halyavin wrote:
> EricWF wrote:
> > halyavin wrote:
> > > EricWF wrote:
> > > > smeenai wrote:
> > > > > Maybe add a comment explaining the purpose of this one as well?
> > > > 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"?
> > > From very few hits Google gave me, it looks like this library implements proper behavior for %s and %c in printfw/scanfw-like functions. 
> > 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`)"
> 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).
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.

https://reviews.llvm.org/D28441