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

Thu Mar 2 03:23:43 PST 2023

mstorsjo added a comment.

In D144734#4164141 <https://reviews.llvm.org/D144734#4164141>, @andrewng wrote:

>>> So should I revise this patch with these configuration modifications? I suspect that this should result in significant simplification. Or should I wait until after the move to CMake 3.20?
>>
>> Yes, I would suggest to simplify things with this in mind first - and split out as much as possible from the patch.
>
> I will try to simplify and split up the patch but with the aim of getting this "full" patch upstream because regardless of the various deployment issues, I think it would be valuable to have a working "fully" tested Windows configuration moving forwards. Does this sound reasonable?

Of course it would be great to have a fully tested setup. libc++experimental is already tested both in mingw configurations and in clang-cl mode when linked statically. int128 support is tested in mingw mode.

How to handle int128 support in clang-cl, and how to handle a dynamically linked libc++experimental, are two entirely separate topics. Work on them certainly is welcome if split out in separate patches, but remember that there are specific reasons for why things are the way they are, and attempts to change it should take that whole situation into account - and clearly acknowledgeing how the various concerns are handled.

For the int128 issue, the nearest solution at hand is to just disable it within libc++, see D134912 <https://reviews.llvm.org/D134912>. Then all tests will pass without the user needing to manually disable it.

>> After you've got a good narrowed down version of the patch, I would suggest adapting it to work with CMake 3.20 (by either setting the policy to new, or increasing the minimum version). To get it working in CI right now, I would kinda suggest you include that change in the patch, but remember to hold off of actually pushing the patch until that bit has been updated in git.
>
> Are you saying that the new approach for selecting MSVC runtime should be used in `libc++` and thus be different from the rest of LLVM? Does that also imply a separate `libc++` CMake variable to control it?

Currently, there's no way to select the CRT used in libc++. Within LLVM itself, it's selectable with LLVM's custom options , like `LLVM_USE_CRT_RELEASE`.

As CMake 3.20 introduces control over this with a new option like `CMAKE_MSVC_RUNTIME_LIBRARY`. I would expect that the main LLVM build would gradually transition to use/prefer this option, deprecating the old LLVM specific settings too (at some point in the future). Therefore, as there's currently no other selection mechanism in play at all within libc++, I would suggest to start out this new effort based on the new option. I don't think there's a strict need for a separate libc++ specific option for it from the start, but I guess someone may have a need for that too (to set it differently than for the other runtimes built in the same build), by passing that option on to the `MSVC_RUNTIME_LIBRARY` target property on the library (and/or interpreting it for our custom cmake code).

I guess we could add this to our cmake file already now:

  if(POLICY CMP0091)
    cmake_policy(SET CMP0091 NEW)
  endif()

If the user doesn't set anything, it works as before, but now the user should be able to affect this setting via `CMAKE_MSVC_RUNTIME_LIBRARY` as the future default will be.

If your patch can handle the setting both from `CMAKE_MSVC_RUNTIME_LIBRARY` and from e.g. `CMAKE_CXX_FLAGS_RELEASE`, then that's probably fine too, but I would focus on the former, since that's soon going to be the default anyway.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144734