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

[EricWF]

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.

[EricWF]

Adding cfe-commits

[smeenai]

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]

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]

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]

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]

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]

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]

lib/CMakeLists.txt

112 ↗

(On Diff #83508)

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

[EricWF]

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]

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]

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]

• Add comment explaining need for iso_stdio_wide_specifiers.lib.

[halyavin]

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]

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]

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.

[EricWF]

In D28441#639023, @smeenai wrote:

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]

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?

[smeenai]

In D28441#646145, @EricWF wrote:

In D28441#639023, @smeenai wrote:

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]

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

[smeenai]

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]

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]

• Address inline comments.

libcxx/trunk/CMakeLists.txt

 
 # Require out of source build.
 include(MacroEnsureOutOfSourceBuild)
 MACRO_ENSURE_OUT_OF_SOURCE_BUILD(
  "${PROJECT_NAME} requires an out of source build. Please create a separate
  build directory and run 'cmake /path/to/${PROJECT_NAME} [options]' there."
  )
 
+if (MSVC)
+  set(LIBCXX_TARGETING_MSVC ON)
+else()
+  set(LIBCXX_TARGETING_MSVC OFF)
+endif()
+
 #===============================================================================
 # Setup CMake Options
 #===============================================================================
 include(CMakeDependentOption)
 
 # Basic options ---------------------------------------------------------------
 option(LIBCXX_ENABLE_ASSERTIONS "Enable assertions independent of build mode." ON)
 option(LIBCXX_ENABLE_SHARED "Build libc++ as a shared library." ON)

 
 if (NOT LIBCXX_STANDALONE_BUILD)
   # Remove flags that may have snuck in.
   remove_flags(-DNDEBUG -UNDEBUG -D_DEBUG
                -lc++abi -m32)
 endif()
 remove_flags(-stdlib=libc++ -stdlib=libstdc++)
 
+# FIXME: Remove all debug flags and flags that change which Windows
+# default libraries are linked. Currently we only support linking the
+# non-debug DLLs
+remove_flags("/D_DEBUG" "/MTd" "/MDd" "/MT" "/Md" "/RTC1")
+
 # FIXME(EricWF): See the FIXME on LIBCXX_ENABLE_PEDANTIC.
 # Remove the -pedantic flag and -Wno-pedantic and -pedantic-errors
 # so they don't get transformed into -Wno and -errors respectivly.
 remove_flags(-Wno-pedantic -pedantic-errors -pedantic)
 
 # Required flags ==============================================================
 set(LIBCXX_STANDARD_VER c++11 CACHE INTERNAL "internal option to change build dialect")
 if (LIBCXX_HAS_MUSL_LIBC)
if (LIBCXX_BUILD_EXTERNAL_THREAD_LIBRARY AND LIBCXX_ENABLE_SHARED)
   endif()
 endif()
 
 # Assertion flags =============================================================
 define_if(LIBCXX_ENABLE_ASSERTIONS -UNDEBUG)
 define_if_not(LIBCXX_ENABLE_ASSERTIONS -DNDEBUG)
 if (LIBCXX_ENABLE_ASSERTIONS)
   # MSVC doesn't like _DEBUG on release builds. See PR 4379.
-  define_if_not(MSVC -D_DEBUG)
+  define_if_not(LIBCXX_TARGETING_MSVC -D_DEBUG)
 endif()
 
 # Modules flags ===============================================================
 # FIXME The libc++ sources are fundamentally non-modular. They need special
 # versions of the headers in order to provide C++03 and legacy ABI definitions.
 # NOTE: The public headers can be used with modules in all other contexts.
 if (LLVM_ENABLE_MODULES)
   # Ignore that the rest of the modules flags are now unused.

libcxx/trunk/cmake/Modules/HandleLibcxxFlags.cmake

macro(mangle_name str output)
   string(TOUPPER "${strippedStr}" ${output})
 endmacro()
 
 # Remove a list of flags from all CMake variables that affect compile flags.
 # This can be used to remove unwanted flags specified on the command line
 # or added in other parts of LLVM's cmake configuration.
 macro(remove_flags)
   foreach(var ${ARGN})
+    string(REPLACE "${var}" "" CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG}")
+    string(REPLACE "${var}" "" CMAKE_CXX_FLAGS_MINSIZEREL "${CMAKE_CXX_FLAGS_DEBUG}")
+    string(REPLACE "${var}" "" CMAKE_CXX_FLAGS_RELEASE "${CMAKE_CXX_FLAGS_DEBUG}")
+    string(REPLACE "${var}" "" CMAKE_CXX_FLAGS_RELWITHDEBINFO "${CMAKE_CXX_FLAGS_DEBUG}")
     string(REPLACE "${var}" "" CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS}")
     string(REPLACE "${var}" "" CMAKE_C_FLAGS "${CMAKE_C_FLAGS}")
     string(REPLACE "${var}" "" CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS}")
     string(REPLACE "${var}" "" CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS}")
     string(REPLACE "${var}" "" CMAKE_SHARED_MODULE_FLAGS "${CMAKE_SHARED_MODULE_FLAGS}")
     remove_definitions(${var})
   endforeach()
 endmacro(remove_flags)

libcxx/trunk/lib/CMakeLists.txt

 endif()
 
 # Setup flags.
 if (NOT WIN32)
   add_flags_if_supported(-fPIC)
 endif()
 add_link_flags_if_supported(-nodefaultlibs)
 
+if (LIBCXX_TARGETING_MSVC)
+  add_compile_flags(/Zl)
+  add_link_flags(/nodefaultlib)
+  add_library_flags(ucrt) # Universal C runtime
+  add_library_flags(vcruntime) # C++ runtime
+  add_library_flags(msvcrt) # C runtime startup files
+  # Required for standards-complaint wide character formatting functions
+  # (e.g. `printfw`/`scanfw`)
+  add_library_flags(iso_stdio_wide_specifiers)
+endif()
+
 if (LIBCXX_OSX_REEXPORT_SYSTEM_ABI_LIBRARY)
   if (NOT DEFINED LIBCXX_LIBCPPABI_VERSION)
     set(LIBCXX_LIBCPPABI_VERSION "2") # Default value
     execute_process(
       COMMAND xcrun --show-sdk-version
       OUTPUT_VARIABLE sdk_ver
       RESULT_VARIABLE res
       OUTPUT_STRIP_TRAILING_WHITESPACE)

libcxx/trunk/test/libcxx/test/config.py

def configure_link_flags_abi_library(self):
                         abs_path = os.path.join(cxxabi_library_root, libname)
                         self.cxx.link_flags += [abs_path]
                     else:
                         self.cxx.link_flags += ['-lc++abi']
         elif cxx_abi == 'libcxxrt':
             self.cxx.link_flags += ['-lcxxrt']
         elif cxx_abi == 'none' or cxx_abi == 'default':
             if self.is_windows:
-                self.cxx.link_flags += ['-lmsvcrtd']
+                self.cxx.link_flags += ['-lmsvcrt']
         else:
             self.lit_config.fatal(
                 'C++ ABI setting %s unsupported for tests' % cxx_abi)
 
     def configure_extra_library_flags(self):
         if self.get_lit_bool('cxx_ext_threads', default=False):
             self.cxx.link_flags += ['-lc++external_threads']
         self.target_info.add_cxx_link_flags(self.cxx.link_flags)