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

Tue Jun 13 06:21:08 PDT 2023

andrewng 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?

When I initially set out on building a LLVM toolchain on Windows with `libcxx` and `RPMalloc`, I basically set out with the intention of enabling and testing everything that was possible. The reasoning was that we also wanted to get a feel for the stability and test coverage of `libcxx` on Windows. That's why this patch has ended up becoming quite big and "sprawling".

It should be easy to split out some of the miscellaneous changes that are related but not directly dependent. Some of the other changes might not be so straightforward. My main concern is time, splitting it up will need more time and testing. The testing can be particularly time and resource consuming. I kind of added `LIBCXX_ENABLE_EXPERIMENTAL_DLL`, which is off by default, because of its "controversial" nature.

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

Yes, this is a bit of a mess right now. I basically take the approach of setting both `MSVC_RUNTIME_LIBRARY` and `LLVM_USE_CRT_*` to my preferred runtime and that appears to work for now.

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

OK, I can drop these parts if it's been decided this is the best approach.

I will try to split up this patch but as it's now CI "friendly", it would be great if it doesn't need to be split up too much. There's quite a lot going on but it's also not too large a patch either. If `LIBCXX_ENABLE_EXPERIMENTAL_DLL` is too controversial, it can always be removed later?

================
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},
----------------
mstorsjo wrote:
> This change seems quite unrelated to the rest.
Yes, it isn't directly related but it is something that I stumbled upon whilst testing CI configuration builds on my own Windows 10 PC. For one/some of the tests my system was seeing this error rather than any of the other possible Windows errors. No idea why, but could be related to a recent change in Windows security software on my PC.

This change can be split out but I wanted to get everything together to check the automated CI.

================
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);
----------------
mstorsjo wrote:
> 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?
The comment is correct but the code AFAIK is not correct and it definitely didn't work. The new code is the correct way to redirect to `stderr` and it works!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144734