[libcxx] Enable support for static and debug Windows runtimes

[andrewng]

Summary

Add initial support to enable use of /MT, /MTd and /MDd when building
and testing libc++ on Windows. For the shared DLL version of libc++
always use the DLL runtime because this is the only sensible choice.

libc++ now depends on c++experimental and therefore the libc++ DLL
requires a c++experimental DLL. Because of the cyclic dependencies
between these, a "stub" c++experimental DLL is built to create an import
lib which enables the libc++ DLL to be built first.

Testing on Windows requires compiler-rt builtins.

For testing with the debug runtimes, the CRT debug report mode must be
set to output to stderr to avoid the interactive pop-up dialog.

[Mordante]

Thanks for fixing this. I'm not too familiar with Windows so I leave the final approval to others. The changes to <format> are fine.

[mstorsjo]

This looks like a rather large change, it's hard to get a proper overview of everything that is going on here, so I can't really give an overall comment on it as such. I would very much prefer if this can be split up into a series of commits (I see at least 3-5 different individual atomic changes here). E.g. first, allow linking against different C runtime configurations only, then adjusting how libc++experimental is handled.

Note that we normally only build a static c++experimental, since it's not ABI frozen; starting to provide a shared linked libc++experimental for this configuration, since it doesn't work without that, is a rather significant policy change, which definitely needs to be split out into a separate commit, clearly labelled as such, and with pros/cons of the change enumerated.

Also, note that https://cmake.org/cmake/help/latest/policy/CMP0091.html changes how the MSVC runtimes are selected - previously, as this patch shows, they were visible via e.g. CMAKE_CXX_FLAGS_RELEASE, but after that policy change, AFAIK they wouldn't be visible there. As the current CMake baseline version is < 3.15, we're still on the old behaviour, but we'll soon switch to requiring 3.20, which I believe would trigger the new behaviour. So for this change, maybe it's simplest to just hold off of doing it until we've switched to requiring 3.20, and then do it according to the new policy only - instead of requiring handling two different ways of signaling the same?

libcxx/CMakeLists.txt

489

You'd want to match for - in addition to / for the character starting the option; -MD works just as well as /MD.

libcxx/test/configs/llvm-libc++-static-clangcl.cfg.in

25

What does the newly added -rtlib=compiler-rt have to do with the rest of the changes here?

libcxx/test/std/experimental/memory/memory.resource.global/new_delete_resource.pass.cpp

94

This looks like a change that probably could be split out and landed separately, before the rest?

[mstorsjo]

Testing on Windows requires compiler-rt builtins.

Currently it doesn't (the tests run without it), and regular use of libc++ in clang-cl mode wouldn't normally add that option. I know this would be useful for getting int128 handling fixed, but to get it fixed in practice and not only in tests, we'd also need to make sure that all users link with -rtlib=compiler-rt - and for users linking by invoking link.exe or lld-link directly instead of via the compiler wrapper, that's not happening. So this aspect is also a larger policy decision that needs to be split out and agreed upon by relevant stakeholders on its own, not just slipped into a giant refactoring patch.

Currently tests pass without this option, and I presume some tests that currently are expected to fail would start to work if we'd do this. Which tests are that? Or conversely, as tests demonstrably do work right now, what fails after your refactoring if you don't add it?

For the shared DLL version of libc++ always use the DLL runtime because this is the only sensible choice.

I woulnd't entirely agree with that - all four combinations of {static,shared} libc++ vs {static,shared} CRT have their uses IMO.

In the end, it would be good to add CI coverage for at least some of the new CRT configurations. Or does this change imply that static libc++ now automatically uses static CRT instead of the dynamic one which is default so far? (This will change the outcome of some CI tests.)

Also finally, do note that out of 3 current clang-cl configurations in CI, this broke 2 entirely (one fails with lld-link: error: could not open 'cxx_shared.lib': no such file or directory, one fails with fatal: unable to parse config file) and the third one has a new failing test (libcxx/vendor/clang-cl/static-lib-exports.sh.cpp).

[mstorsjo]

In D144734#4154997, @mstorsjo wrote:

Testing on Windows requires compiler-rt builtins.

Currently it doesn't (the tests run without it), and regular use of libc++ in clang-cl mode wouldn't normally add that option. I know this would be useful for getting int128 handling fixed, but to get it fixed in practice and not only in tests, we'd also need to make sure that all users link with -rtlib=compiler-rt - and for users linking by invoking link.exe or lld-link directly instead of via the compiler wrapper, that's not happening. So this aspect is also a larger policy decision that needs to be split out and agreed upon by relevant stakeholders on its own, not just slipped into a giant refactoring patch.

Also, we can't generally assume that compiler-rt builtins are available for these targets; not all clang-cl distributions ship with the builtins available, in particular not for all 4 relevant architecture targets. MSVC itself does ship with compiler-rt builtins for x86 and x64, but not for arm and arm64. So if we'd suddenly start requiring it, we'd break a bunch of currently working setups.

I.e., this is definitely a larger policy change that needs to be signaled and handled across stakeholders.

[andrewng]

Hi @mstorsjo, thanks for your comments.

This looks like a rather large change, it's hard to get a proper overview of everything that is going on here, so I can't really give an overall comment on it as such. I would very much prefer if this can be split up into a series of commits (I see at least 3-5 different individual atomic changes here). E.g. first, allow linking against different C runtime configurations only, then adjusting how libc++experimental is handled.

This is my first look into libc++, so I'm sorry that I'm not familiar with the current status of libc++ on Windows and in general. I only ended up creating this patch because of a downstream need to build and test a Windows based LLVM toolchain built with libc++, RPMalloc and the static MSVC runtime (requirement of RPMalloc support). Because I had already done quite a bit of the work downstream, I thought I would try to finish and clean it for upstream.

It is quite a large change and that's probably partly due to my unfamiliarity with libc++. But also I wanted to "validate" my build changes by also addressing the related testing issues which is partly why this patch is bigger than desired. I assumed that any patch would also need to pass all the testing but perhaps my Windows libc++ configuration is not quite "right".

Note that we normally only build a static c++experimental, since it's not ABI frozen; starting to provide a shared linked libc++experimental for this configuration, since it doesn't work without that, is a rather significant policy change, which definitely needs to be split out into a separate commit, clearly labelled as such, and with pros/cons of the change enumerated.

I was not aware of the c++experimental situation and again this change seemed to be necessary to enable the libc++ DLL with dynamic runtime. Because of the circular dependencies between libc++ and c++experimental and the current shared import/export annotations, linking a static c++experimental with the libc++ DLL does not work.

Also, note that https://cmake.org/cmake/help/latest/policy/CMP0091.html changes how the MSVC runtimes are selected - previously, as this patch shows, they were visible via e.g. CMAKE_CXX_FLAGS_RELEASE, but after that policy change, AFAIK they wouldn't be visible there. As the current CMake baseline version is < 3.15, we're still on the old behaviour, but we'll soon switch to requiring 3.20, which I believe would trigger the new behaviour. So for this change, maybe it's simplest to just hold off of doing it until we've switched to requiring 3.20, and then do it according to the new policy only - instead of requiring handling two different ways of signaling the same?

I have been using CMake 3.24 during the development of this patch. However, I guess LLVM might move to the new policy once 3.20 becomes the base requirement.

Currently it doesn't (the tests run without it), and regular use of libc++ in clang-cl mode wouldn't normally add that option. I know this would be useful for getting int128 handling fixed, but to get it fixed in practice and not only in tests, we'd also need to make sure that all users link with -rtlib=compiler-rt - and for users linking by invoking link.exe or lld-link directly instead of via the compiler wrapper, that's not happening. So this aspect is also a larger policy decision that needs to be split out and agreed upon by relevant stakeholders on its own, not just slipped into a giant refactoring patch.

Currently tests pass without this option, and I presume some tests that currently are expected to fail would start to work if we'd do this. Which tests are that? Or conversely, as tests demonstrably do work right now, what fails after your refactoring if you don't add it?

In my configuration the int128 tests were running and failing which is why I added compiler-rt. If the int128 tests are not meant to be running then I don't think this would be required. AFAIK, the libc++ support in clang-cl still isn't really fully implemented, e.g. no option to enable it (have to add header paths manually), so it's kind of tricky to know what should or should not be working.

I woulnd't entirely agree with that - all four combinations of {static,shared} libc++ vs {static,shared} CRT have their uses IMO.

Yes, there are definitely use cases for all combinations but using the static runtime in DLLs means there's a separate runtime instantiation in each DLL which isn't always the desired behaviour but also mostly "works" too.

In the end, it would be good to add CI coverage for at least some of the new CRT configurations. Or does this change imply that static libc++ now automatically uses static CRT instead of the dynamic one which is default so far? (This will change the outcome of some CI tests.)

Basically, this patch allows for three main combinations (also supports both release and debug runtime variants). DLL with dynamic runtime, static libraries with dynamic runtime and static libraries with static runtime. Whether the static library uses the dynamic or static runtime is determined by the default runtime or that set by the LLVM_USE_CRT_<BUILD_TYPE> CMake variables (the current approach to setting the MSVC runtime).

Also finally, do note that out of 3 current clang-cl configurations in CI, this broke 2 entirely (one fails with lld-link: error: could not open 'cxx_shared.lib': no such file or directory, one fails with fatal: unable to parse config file) and the third one has a new failing test (libcxx/vendor/clang-cl/static-lib-exports.sh.cpp).

I tested my patch locally in all 6 combinations, i.e. including release and debug runtimes. However, it's already clear that the configuration that I've been using does not match CI. Now that there are CI failures, I should hopefully be able to find out the differences in configuration.

libcxx/CMakeLists.txt

489

I was just following what appears to be the existing convention of only checking for the / variation of Window arguments. But yes, - works just as well.

libcxx/test/configs/llvm-libc++-static-clangcl.cfg.in

25

This was added to fix test failures that I was seeing whilst developing this patch related to unresolved symbols such as __umodti3, __udivti3 and __floattidf. These all appear to be related to int128 support. I just kind of assumed that since these tests were running they should pass. Should I be somehow disabling int128 support and testing?

libcxx/test/std/experimental/memory/memory.resource.global/new_delete_resource.pass.cpp

94

This was to fix this test from failing in my builds using the MS dynamic runtime. TBH, I'm surprised this does not already fail CI, but I guess that depends on what runtime is being used. Yes, you're right this can be separated, although I guess it won't be "tested" because it's not causing any issues right now.

[andrewng]

I tested my patch locally in all 6 combinations, i.e. including release and debug runtimes. However, it's already clear that the configuration that I've been using does not match CI. Now that there are CI failures, I should hopefully be able to find out the differences in configuration.

OK, having looked at the CI failures, I think I've identified the key configuration differences. CI disables int128 support with -DLIBCXX_EXTRA_SITE_DEFINES=_LIBCPP_HAS_NO_INT128 and c++experimental is disabled in the testing with -DLIBCXX_TEST_PARAMS=enable_experimental=False. I think this would explain the differences that I saw with test failures when testing my patch. Unfortunately, I was not aware that these features are unsupported on Windows.

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?

[mstorsjo]

In D144734#4155353, @andrewng wrote:

This is my first look into libc++, so I'm sorry that I'm not familiar with the current status of libc++ on Windows and in general. I only ended up creating this patch because of a downstream need to build and test a Windows based LLVM toolchain built with libc++, RPMalloc and the static MSVC runtime (requirement of RPMalloc support). Because I had already done quite a bit of the work downstream, I thought I would try to finish and clean it for upstream.

Thanks - much appreciated that you want to upstream your efforts instead of keeping them to yourself!

I assumed that any patch would also need to pass all the testing but perhaps my Windows libc++ configuration is not quite "right".

Any patch does indeed need to pass all the testing, but that's primarily defined by the CI configurations; those must keep passing. Ideally, the tests should pass in all possible build configurations, but reality isn't quite there.

Note that we normally only build a static c++experimental, since it's not ABI frozen; starting to provide a shared linked libc++experimental for this configuration, since it doesn't work without that, is a rather significant policy change, which definitely needs to be split out into a separate commit, clearly labelled as such, and with pros/cons of the change enumerated.

I was not aware of the c++experimental situation and again this change seemed to be necessary to enable the libc++ DLL with dynamic runtime. Because of the circular dependencies between libc++ and c++experimental and the current shared import/export annotations, linking a static c++experimental with the libc++ DLL does not work.

I would kinda formulate this issue slightly differently. If we'd split the import/export annotations for what belongs to libc++experimental and what belongs to libc++, I believe it should be doable to use a statically linked libc++experimental with a dynamically linked libc++. (Separate context; for mingw builds, we can actually do without explicit import annotations, and that also makes that build configuration work fine there.) So with that in mind, the shared libc++experimental is a configuration which really isn't necessarily something we need to worry about.

(But even then, I don't quite see why libc++ needs to depend on it in the first place? Isn't it enough that libc++experimental would depend on libc++ but not the other way around? I haven't looked closely at that aspect though. I guess it has to do with the format_error destructor somehow?)

Also, note that https://cmake.org/cmake/help/latest/policy/CMP0091.html changes how the MSVC runtimes are selected - previously, as this patch shows, they were visible via e.g. CMAKE_CXX_FLAGS_RELEASE, but after that policy change, AFAIK they wouldn't be visible there. As the current CMake baseline version is < 3.15, we're still on the old behaviour, but we'll soon switch to requiring 3.20, which I believe would trigger the new behaviour. So for this change, maybe it's simplest to just hold off of doing it until we've switched to requiring 3.20, and then do it according to the new policy only - instead of requiring handling two different ways of signaling the same?

I have been using CMake 3.24 during the development of this patch. However, I guess LLVM might move to the new policy once 3.20 becomes the base requirement.

Yes - as long as the project itself declares a lower minimum version, cmake consistently gives the old behaviour, regardless of cmake version. Once 3.20 is the new base requirement, I doubt we'd want to keep explicitly requesting the old policy here, so I'd primarily focus my efforts on the new policy mode.

In my configuration the int128 tests were running and failing which is why I added compiler-rt. If the int128 tests are not meant to be running then I don't think this would be required.

Yes - int128 vs clang-cl vs libc++ is limbo, somewhat stuck/moving slowly. As you've discovered, the CI tests run by defining _LIBCPP_HAS_NO_INT128. You can read up on https://reviews.llvm.org/D91139, followed by https://reviews.llvm.org/D134912. So for now, the tests only pass fully with that define set.

AFAIK, the libc++ support in clang-cl still isn't really fully implemented, e.g. no option to enable it (have to add header paths manually), so it's kind of tricky to know what should or should not be working.

This aspect is a bit separate though. There have been attempts at implementing that too but they've had to be backed out for various reasons (I don't have a reference right now).

I woulnd't entirely agree with that - all four combinations of {static,shared} libc++ vs {static,shared} CRT have their uses IMO.

Yes, there are definitely use cases for all combinations but using the static runtime in DLLs means there's a separate runtime instantiation in each DLL which isn't always the desired behaviour but also mostly "works" too.

Yep, separate instantiations of the CRT isn't always ideal/desired, but I can also see cases where it could be useful.

Basically, this patch allows for three main combinations (also supports both release and debug runtime variants). DLL with dynamic runtime, static libraries with dynamic runtime and static libraries with static runtime. Whether the static library uses the dynamic or static runtime is determined by the default runtime or that set by the LLVM_USE_CRT_<BUILD_TYPE> CMake variables (the current approach to setting the MSVC runtime).

Right - but I presume that the default case if the user doesn't change/set anything, is to keep behaving as it used to?

Also finally, do note that out of 3 current clang-cl configurations in CI, this broke 2 entirely (one fails with lld-link: error: could not open 'cxx_shared.lib': no such file or directory, one fails with fatal: unable to parse config file) and the third one has a new failing test (libcxx/vendor/clang-cl/static-lib-exports.sh.cpp).

I tested my patch locally in all 6 combinations, i.e. including release and debug runtimes. However, it's already clear that the configuration that I've been using does not match CI. Now that there are CI failures, I should hopefully be able to find out the differences in configuration.

In D144734#4155741, @andrewng wrote:

I tested my patch locally in all 6 combinations, i.e. including release and debug runtimes. However, it's already clear that the configuration that I've been using does not match CI. Now that there are CI failures, I should hopefully be able to find out the differences in configuration.

OK, having looked at the CI failures, I think I've identified the key configuration differences. CI disables int128 support with -DLIBCXX_EXTRA_SITE_DEFINES=_LIBCPP_HAS_NO_INT128 and c++experimental is disabled in the testing with -DLIBCXX_TEST_PARAMS=enable_experimental=False. I think this would explain the differences that I saw with test failures when testing my patch. Unfortunately, I was not aware that these features are unsupported on Windows.

Yeah, enable_experimental=False is required if libc++ is built shared. I tried doing something about this in https://reviews.llvm.org/D99178, but that wasn't accepted as such. And lately, things have changed so that libc++experimental can't be disabled anymore - but it's not really expected to work with a dynamically linked libc++ at the moment. int128 is mostly in limbo, but practically not really supported.

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.

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.

Then it would also be good to add CI coverage for at least some new configuration here - I guess static libc++ with static CRT is the prime candidate here. But this can probably follow in a separate patch later. I think this can cause some changes in test results. Some test cases currently fail, due to UCRT bugs that are fixed in newer versions. In the CI environment, the statically linked UCRT version is probably newer than the dynamically linked version provided by the OS. Although for some cases, we do have detection of the issue when starting the testsuite, see e.g. https://github.com/llvm/llvm-project/blob/llvmorg-17-init/libcxx/utils/libcxx/test/features.py#L115-L126. As you've found the run-buildbot script, you probably also have found buildkite-pipeline.yml - there it should be quite obvious how to add another test configuration. (For iterating the patch, for quicker turnaround, you may want to modify the patch so that it removes all other irrelevant test configurations from the pipeline file, other than the windows ones, and once it seems to behave as you want to, you can remove those parts of the patch again.)

But I think adding a new CI configuration can come as a separate patch (especially if the initial patch is nontrivial); then it's clear that the first patch just adds more possibilities of doing things (while keeping the default behaviour unchanged), and the second one tries to use the new behaviours.

libcxx/CMakeLists.txt

489

If there are other cases where we inspect option strings looking for clang-cl/MSVC style options, then I would prefer to amend them to check for both - and / styled options, and stick with checking both forms (which is trivial when we can use regexes here) in new code.

libcxx/test/std/experimental/memory/memory.resource.global/new_delete_resource.pass.cpp

94

Hmm, this test should probably be passing already in CI indeed, with dynamic CRT, with both shared and statically linked libc++. I wonder why it would be failing for you with the dynamic CRT.

With a more split up and/or simplified patchset, I'd hope that it'd be easier to pinpoint what is causing this.

[andrewng]

I assumed that any patch would also need to pass all the testing but perhaps my Windows libc++ configuration is not quite "right".

Any patch does indeed need to pass all the testing, but that's primarily defined by the CI configurations; those must keep passing. Ideally, the tests should pass in all possible build configurations, but reality isn't quite there.

This patch does successfully run all the tests in the 6 different combinations that I mentioned with the "default" CMake configuration, i.e. with int128 support and c++experimental testing enabled. However, it does break CI because of the introduction of the compiler-rt dependency in testing and not quite coping with the combinations of CMake configuration in use by CI on Windows. But I am now aware of those configurations and it should be possible to make those work as before.

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?

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?

But I think adding a new CI configuration can come as a separate patch (especially if the initial patch is nontrivial); then it's clear that the first patch just adds more possibilities of doing things (while keeping the default behaviour unchanged), and the second one tries to use the new behaviours.

Yes, definitely worthwhile to add more CI configurations for Windows.

I don't know how much time I'll be able to find to do this, so updates may take a while.

libcxx/test/std/experimental/memory/memory.resource.global/new_delete_resource.pass.cpp

94

It's part of std::experimental which is excluded from testing in CI, so that's why it isn't causing any issues.

[mstorsjo]

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