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@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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits