[libcxx-commits] [PATCH] D144734: [libcxx] Enable support for static and debug Windows runtimes

Tue Jun 13 05:01:01 PDT 2023

mstorsjo added a subscriber: jyknight.
mstorsjo added a comment.

The changes about `LIBCXX_ENABLE_EXPERIMENTAL_DLL` are probably more controversial than the rest (or, controversial on their own) - is it possible to split out all the changes relating to the experimental library to a separate review?

Regarding ways of selecting the CRT, unfortunately things aren't quite straightforward yet - even if the minimum of CMake 3.20 was committed, the behaviour of the new policy CMP0091, https://cmake.org/cmake/help/latest/policy/CMP0091.html, was reverted to the old one in D150688 <https://reviews.llvm.org/D150688> - see https://github.com/llvm/llvm-project/issues/63286. So for now, the old mechanism is defacto in place still. I don't quite know how setting the `MSVC_RUNTIME_LIBRARY` property on a target behaves though. Instead of intercepting the individual `/MT` flags, I guess we could just look at the `LLVM_USE_CRT_*` flags from ChooseMSVCCRT.cmake (https://github.com/llvm/llvm-project/blob/llvmorg-16.0.3/llvm/cmake/modules/ChooseMSVCCRT.cmake) though.

There still are a lot of different things going in this patch; some which I mostly agree with, some which are more subtle. As far as I'm concerned, I see that this patch does the following list of things. If it woudl be possible to split it up into a series of patches in that way, it would be much easier to take incremental action on it:

- Fixes running tests with libcxx built in Debug mode. This bit is probably unambiguously good and a step forward. (Currently, we hardcode a debug build of libcxx to use the debug version of the CRT. Ideally I would see that this could be switched separately from the rest; if `CMAKE_MSVC_RUNTIME_LIBRARY` is set, it should probably override that default. But I guess others don't care quite as much about that bit? I guess I could tackle that bit myself later if I get time.)

- Allows switching the static build of libcxx to use the statically linked CRT. That's probably also good. I kinda would prefer to have the CRT switching mechanism a bit more free, to let `CMAKE_MSVC_RUNTIME_LIBRARY` take precedence for both dynamic and static builds of libc++, allowing any configuration. Due to the unfinished CMake policy change mess, wrt https://github.com/llvm/llvm-project/issues/63286, this step is less clear to make though. But if things work for you and works in CI, I guess we could move forward with that, and I or someone else could improve configurability further later.

- Attempts to fix tests without int128 disabled. This has been entirely settled for now in D134912 <https://reviews.llvm.org/D134912> by just flat out disabling int128 in all clang-cl configurations, so those bits can be dropped for now, and proper generic fixes in clang/lld are worked on elsewhere.

- Allowing building `c++experimental` as a DLL. I wouldn't oppose that, but it's a somewhat controversial change and needs to be judged on its own separate from the rest of this change.

Can you split up this change into such a stack of patches? The first one should be fairly straightforward - just the checks for `LIBCXX_DEBUG_BUILD` in the test configs and the changes to `set_windows_crt_report_mode.h` (with appropriate reasoning).

================
Comment at: libcxx/src/filesystem/operations.cpp:417
       {ERROR_ALREADY_EXISTS, errc::file_exists},
+      {ERROR_BAD_NET_NAME, errc::no_such_file_or_directory},
       {ERROR_BAD_NETPATH, errc::no_such_file_or_directory},
----------------
This change seems quite unrelated to the rest.

================
Comment at: libcxx/test/configs/llvm-libc++-shared-clangcl.cfg.in:11
+    runtime_lib += 'd'
+    dbg_include = ' -include set_windows_crt_report_mode.h'
+
----------------
@jyknight This seems relevant for fixing running tests with debug builds, as discussed on Discord a couple of weeks ago.

================
Comment at: libcxx/test/configs/llvm-libc++-shared-clangcl.cfg.in:31
+
+if not '_LIBCPP_HAS_NO_INT128' in libcxx.test.dsl.compilerMacros(config):
+    subst = config.substitutions[link_flags_idx]
----------------
The handling of compiler-rt and int128 support is a separate issue which we should keep out from the rest of these changes here. Note that D134912 was merged recently, which made `_LIBCPP_HAS_NO_INT128` the default on clang-cl configurations, so these workarounds shouldn't be needed for now.

================
Comment at: libcxx/test/support/set_windows_crt_report_mode.h:30
+  _CrtSetReportMode(_CRT_WARN, _CRTDBG_MODE_FILE);
+  _CrtSetReportFile(_CRT_WARN, _CRTDBG_FILE_STDERR);
+  _CrtSetReportMode(_CRT_ERROR, _CRTDBG_MODE_FILE);
----------------
What's the functional difference of this change here? The previous code just set `_CRTDBG_MODE_DEBUG`, which supposedly according to the comment changes to log to stderr, while this sets the mode to `_CRTDBG_MODE_FILE` with the file set to `_CRTDBG_FILE_STDERR`. Is the old comment incorrect, or has something changed that causes the previous form of the mode setting to no longer work?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144734/new/

https://reviews.llvm.org/D144734