[PATCH] D28441: [libc++] [CMake] Link with /nodefaultlibs on Windows
This revision was automatically updated to reflect the committed changes.
Closed by commit rL292001: [libc++] [CMake] Link with /nodefaultlibs on Windows
(authored by EricWF).
Changed prior to commit:
https://reviews.llvm.org/D28441?vs=84428=84430#toc
Repository:
rL LLVM
https://reviews.llvm.org/D28441
Files:
libcxx/trunk/CMakeLists.txt
libcxx/trunk/cmake/Modules/HandleLibcxxFlags.cmake
libcxx/trunk/lib/CMakeLists.txt
libcxx/trunk/test/libcxx/test/config.py
Index: libcxx/trunk/cmake/Modules/HandleLibcxxFlags.cmake
===
--- libcxx/trunk/cmake/Modules/HandleLibcxxFlags.cmake
+++ libcxx/trunk/cmake/Modules/HandleLibcxxFlags.cmake
@@ -26,6 +26,10 @@
# 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}")
Index: libcxx/trunk/test/libcxx/test/config.py
===
--- libcxx/trunk/test/libcxx/test/config.py
+++ libcxx/trunk/test/libcxx/test/config.py
@@ -667,7 +667,7 @@
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)
Index: libcxx/trunk/CMakeLists.txt
===
--- libcxx/trunk/CMakeLists.txt
+++ libcxx/trunk/CMakeLists.txt
@@ -39,6 +39,12 @@
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
#===
@@ -377,6 +383,11 @@
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.
@@ -476,7 +487,7 @@
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 ===
Index: libcxx/trunk/lib/CMakeLists.txt
===
--- libcxx/trunk/lib/CMakeLists.txt
+++ libcxx/trunk/lib/CMakeLists.txt
@@ -104,6 +104,17 @@
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
Index: libcxx/trunk/cmake/Modules/HandleLibcxxFlags.cmake
===
--- libcxx/trunk/cmake/Modules/HandleLibcxxFlags.cmake
+++ libcxx/trunk/cmake/Modules/HandleLibcxxFlags.cmake
@@ -26,6 +26,10 @@
# 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}" ""
[PATCH] D28441: [libc++] [CMake] Link with /nodefaultlibs on Windows
EricWF updated this revision to Diff 84428.
EricWF added a comment.
- Address inline comments.
https://reviews.llvm.org/D28441
Files:
CMakeLists.txt
cmake/Modules/HandleLibcxxFlags.cmake
lib/CMakeLists.txt
test/libcxx/test/config.py
Index: test/libcxx/test/config.py
===
--- test/libcxx/test/config.py
+++ test/libcxx/test/config.py
@@ -667,7 +667,7 @@
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)
Index: lib/CMakeLists.txt
===
--- lib/CMakeLists.txt
+++ lib/CMakeLists.txt
@@ -104,6 +104,17 @@
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
Index: cmake/Modules/HandleLibcxxFlags.cmake
===
--- cmake/Modules/HandleLibcxxFlags.cmake
+++ cmake/Modules/HandleLibcxxFlags.cmake
@@ -26,6 +26,10 @@
# 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}")
Index: CMakeLists.txt
===
--- CMakeLists.txt
+++ CMakeLists.txt
@@ -39,6 +39,12 @@
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
#===
@@ -377,6 +383,11 @@
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.
@@ -476,7 +487,7 @@
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 ===
Index: test/libcxx/test/config.py
===
--- test/libcxx/test/config.py
+++ test/libcxx/test/config.py
@@ -667,7 +667,7 @@
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)
Index: lib/CMakeLists.txt
===
--- lib/CMakeLists.txt
+++ lib/CMakeLists.txt
@@ -104,6 +104,17 @@
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
[PATCH] D28441: [libc++] [CMake] Link with /nodefaultlibs on Windows
EricWF marked an inline comment as done. EricWF added inline comments. Comment at: lib/CMakeLists.txt:113 + add_library_flags(msvcrt) # C runtime startup files + # Required for wide character formatting functions (e.g. `printfw`/`scanfw`) + add_library_flags(iso_stdio_wide_specifiers) smeenai wrote: > Would be slightly more specific: "required for **standards-compliant** > wide-character formatting functions" Sure, I'll make the change. Although if we can't link libc++ we can't really provide any standards conformance :-P https://reviews.llvm.org/D28441 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28441: [libc++] [CMake] Link with /nodefaultlibs on Windows
smeenai added inline comments. 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: > smeenai wrote: > > 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`. > `msvcrt.dll` doesn't exist anymore according to > [this](https://msdn.microsoft.com/en-us/library/abx4dbyh.aspx) 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. https://reviews.llvm.org/D28441 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28441: [libc++] [CMake] Link with /nodefaultlibs on Windows
smeenai accepted this revision. smeenai added a comment. This revision is now accepted and ready to land. There's a bunch of follow-up work here, but I'm fine with this for now. https://reviews.llvm.org/D28441 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28441: [libc++] [CMake] Link with /nodefaultlibs on Windows
smeenai added a comment. In https://reviews.llvm.org/D28441#646145, @EricWF wrote: > In https://reviews.llvm.org/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. Comment at: CMakeLists.txt:488 define_if_not(LIBCXX_ENABLE_ASSERTIONS -DNDEBUG) -if (LIBCXX_ENABLE_ASSERTIONS) +if (LIBCXX_ENABLE_ASSERTIONS AND NOT LIBCXX_TARGETING_MSVC) # MSVC doesn't like _DEBUG on release builds. See PR 4379. Isn't this redundant, considering the `define_if_not(MSVC)` below? I suppose that can be changed to an unconditional define. Comment at: lib/CMakeLists.txt:113 + add_library_flags(msvcrt) # C runtime startup files + # Required for wide character formatting functions (e.g. `printfw`/`scanfw`) + add_library_flags(iso_stdio_wide_specifiers) Would be slightly more specific: "required for **standards-compliant** wide-character formatting functions" https://reviews.llvm.org/D28441 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28441: [libc++] [CMake] Link with /nodefaultlibs on Windows
EricWF updated this revision to Diff 84414.
EricWF added a comment.
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?
https://reviews.llvm.org/D28441
Files:
CMakeLists.txt
cmake/Modules/HandleLibcxxFlags.cmake
lib/CMakeLists.txt
test/libcxx/test/config.py
Index: test/libcxx/test/config.py
===
--- test/libcxx/test/config.py
+++ test/libcxx/test/config.py
@@ -667,7 +667,7 @@
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)
Index: lib/CMakeLists.txt
===
--- lib/CMakeLists.txt
+++ lib/CMakeLists.txt
@@ -104,6 +104,16 @@
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 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
Index: cmake/Modules/HandleLibcxxFlags.cmake
===
--- cmake/Modules/HandleLibcxxFlags.cmake
+++ cmake/Modules/HandleLibcxxFlags.cmake
@@ -26,6 +26,10 @@
# 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}")
Index: CMakeLists.txt
===
--- CMakeLists.txt
+++ CMakeLists.txt
@@ -39,6 +39,12 @@
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
#===
@@ -377,6 +383,11 @@
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.
@@ -474,7 +485,7 @@
# Assertion flags =
define_if(LIBCXX_ENABLE_ASSERTIONS -UNDEBUG)
define_if_not(LIBCXX_ENABLE_ASSERTIONS -DNDEBUG)
-if (LIBCXX_ENABLE_ASSERTIONS)
+if (LIBCXX_ENABLE_ASSERTIONS AND NOT LIBCXX_TARGETING_MSVC)
# MSVC doesn't like _DEBUG on release builds. See PR 4379.
define_if_not(MSVC -D_DEBUG)
endif()
Index: test/libcxx/test/config.py
===
--- test/libcxx/test/config.py
+++ test/libcxx/test/config.py
@@ -667,7 +667,7 @@
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)
Index: lib/CMakeLists.txt
===
--- lib/CMakeLists.txt
+++ lib/CMakeLists.txt
@@ -104,6 +104,16 @@
endif()
add_link_flags_if_supported(-nodefaultlibs)
+if (LIBCXX_TARGETING_MSVC)
+ add_compile_flags(/Zl)
+
[PATCH] D28441: [libc++] [CMake] Link with /nodefaultlibs on Windows
EricWF added a comment. In https://reviews.llvm.org/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? Comment at: CMakeLists.txt:43 +if (WIN32 AND NOT MINGW) + set(LIBCXX_TARGETING_WINDOWS ON) +else() rnk wrote: > smeenai wrote: > > 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. > 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. > 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](https://msdn.microsoft.com/en-us/library/abx4dbyh.aspx) `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.
[PATCH] D28441: [libc++] [CMake] Link with /nodefaultlibs on Windows
rnk added inline comments. Comment at: CMakeLists.txt:43 +if (WIN32 AND NOT MINGW) + set(LIBCXX_TARGETING_WINDOWS ON) +else() smeenai wrote: > 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. 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. 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 smeenai wrote: > 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
[PATCH] D28441: [libc++] [CMake] Link with /nodefaultlibs on Windows
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__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
[PATCH] D28441: [libc++] [CMake] Link with /nodefaultlibs on Windows
halyavin added inline comments. Comment at: lib/CMakeLists.txt:113 + add_library_flags(msvcrt) # C runtime startup files + add_library_flags(iso_stdio_wide_specifiers) +endif() 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). https://reviews.llvm.org/D28441 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28441: [libc++] [CMake] Link with /nodefaultlibs on Windows
EricWF updated this revision to Diff 83526.
EricWF added a comment.
- Add comment explaining need for `iso_stdio_wide_specifiers.lib`.
https://reviews.llvm.org/D28441
Files:
CMakeLists.txt
cmake/Modules/HandleLibcxxFlags.cmake
lib/CMakeLists.txt
test/libcxx/test/config.py
Index: test/libcxx/test/config.py
===
--- test/libcxx/test/config.py
+++ test/libcxx/test/config.py
@@ -667,7 +667,7 @@
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)
Index: lib/CMakeLists.txt
===
--- lib/CMakeLists.txt
+++ lib/CMakeLists.txt
@@ -104,6 +104,16 @@
endif()
add_link_flags_if_supported(-nodefaultlibs)
+if (LIBCXX_TARGETING_WINDOWS)
+ 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 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
Index: cmake/Modules/HandleLibcxxFlags.cmake
===
--- cmake/Modules/HandleLibcxxFlags.cmake
+++ cmake/Modules/HandleLibcxxFlags.cmake
@@ -26,6 +26,10 @@
# 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}")
Index: CMakeLists.txt
===
--- CMakeLists.txt
+++ CMakeLists.txt
@@ -39,6 +39,12 @@
build directory and run 'cmake /path/to/${PROJECT_NAME} [options]' there."
)
+if (WIN32 AND NOT MINGW)
+ set(LIBCXX_TARGETING_WINDOWS ON)
+else()
+ set(LIBCXX_TARGETING_WINDOWS OFF)
+endif()
+
#===
# Setup CMake Options
#===
@@ -376,6 +382,11 @@
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.
@@ -473,7 +484,7 @@
# Assertion flags =
define_if(LIBCXX_ENABLE_ASSERTIONS -UNDEBUG)
define_if_not(LIBCXX_ENABLE_ASSERTIONS -DNDEBUG)
-if (LIBCXX_ENABLE_ASSERTIONS)
+if (LIBCXX_ENABLE_ASSERTIONS AND NOT LIBCXX_TARGETING_WINDOWS)
# MSVC doesn't like _DEBUG on release builds. See PR 4379.
define_if_not(MSVC -D_DEBUG)
endif()
Index: test/libcxx/test/config.py
===
--- test/libcxx/test/config.py
+++ test/libcxx/test/config.py
@@ -667,7 +667,7 @@
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)
Index: lib/CMakeLists.txt
===
--- lib/CMakeLists.txt
+++ lib/CMakeLists.txt
@@ -104,6 +104,16 @@
endif()
add_link_flags_if_supported(-nodefaultlibs)
+if (LIBCXX_TARGETING_WINDOWS)
+ 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 wide character formatting functions (e.g. `printfw`/`scanfw`)
+
[PATCH] D28441: [libc++] [CMake] Link with /nodefaultlibs on Windows
EricWF added inline comments. 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: > > 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`)" https://reviews.llvm.org/D28441 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28441: [libc++] [CMake] Link with /nodefaultlibs on Windows
halyavin added inline comments. Comment at: lib/CMakeLists.txt:113 + add_library_flags(msvcrt) # C runtime startup files + add_library_flags(iso_stdio_wide_specifiers) +endif() 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. https://reviews.llvm.org/D28441 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28441: [libc++] [CMake] Link with /nodefaultlibs on Windows
EricWF added inline comments. Comment at: CMakeLists.txt:43 +if (WIN32 AND NOT MINGW) + set(LIBCXX_TARGETING_WINDOWS ON) +else() 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. 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) 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`. https://reviews.llvm.org/D28441 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28441: [libc++] [CMake] Link with /nodefaultlibs on Windows
halyavin added inline comments. 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) 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. https://reviews.llvm.org/D28441 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28441: [libc++] [CMake] Link with /nodefaultlibs on Windows
halyavin added inline comments. 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) 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? https://reviews.llvm.org/D28441 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28441: [libc++] [CMake] Link with /nodefaultlibs on Windows
EricWF added inline comments.
Comment at: CMakeLists.txt:388
+# non-debug DLLs
+remove_flags("/D_DEBUG" "/MTd" "/MDd" "/MT" "/Md" "/RTC1")
+
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__INIT`.
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
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?
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
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
Comment at: lib/CMakeLists.txt:113
+ add_library_flags(msvcrt) # C runtime startup files
+ add_library_flags(iso_stdio_wide_specifiers)
+endif()
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"?
https://reviews.llvm.org/D28441
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28441: [libc++] [CMake] Link with /nodefaultlibs on Windows
smeenai added inline comments. 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 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.) 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 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. https://reviews.llvm.org/D28441 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28441: [libc++] [CMake] Link with /nodefaultlibs on Windows
halyavin added inline comments. 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 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. 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 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. https://reviews.llvm.org/D28441 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28441: [libc++] [CMake] Link with /nodefaultlibs on Windows
smeenai added inline comments.
Comment at: CMakeLists.txt:43
+if (WIN32 AND NOT MINGW)
+ set(LIBCXX_TARGETING_WINDOWS ON)
+else()
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.
Comment at: CMakeLists.txt:388
+# non-debug DLLs
+remove_flags("/D_DEBUG" "/MTd" "/MDd" "/MT" "/Md" "/RTC1")
+
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?
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
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).
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
Idk if there's anything specific to C++ in vcruntime; it's more compiler
runtime functions as far as I know.
Comment at: lib/CMakeLists.txt:113
+ add_library_flags(msvcrt) # C runtime startup files
+ add_library_flags(iso_stdio_wide_specifiers)
+endif()
Maybe add a comment explaining the purpose of this one as well?
https://reviews.llvm.org/D28441
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28441: [libc++] [CMake] Link with /nodefaultlibs on Windows
EricWF added a comment. Adding cfe-commits https://reviews.llvm.org/D28441 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
