[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 sensible choice to avoid
runtime issues.

libc++ now depends on c++experimental and therefore the MSVC 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. Building of a
c++experimental DLL must be enabled with LIBCXX_ENABLE_EXPERIMENTAL_DLL
and is disabled by default.

Testing on Windows with int128 support 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 from
stalling the testing.

[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

490

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

26

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

91–92

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

490

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

26

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

91–92

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

490

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

91–92

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

91–92

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.

[mstorsjo]

FYI, the project minimum CMake version has more been bumped to 3.20, so this can be updated to only use CMAKE_MSVC_RUNTIME_LIBRARY now.

[mstorsjo]

In D144734#4347261, @mstorsjo wrote:

FYI, the project minimum CMake version has more been bumped to 3.20, so this can be updated to only use CMAKE_MSVC_RUNTIME_LIBRARY now.

Actually, libcxx seems to include ChooseMSVCCRT.cmake still, which expects the old style of setting the option, so this thing hasn't really settled yet. See https://reviews.llvm.org/D150688 for updates on that topic. WRT this patch, it's probably best to hold off for another couple weeks to see how these things settle.

[andrewng]

In D144734#4347261, @mstorsjo wrote:

FYI, the project minimum CMake version has more been bumped to 3.20, so this can be updated to only use CMAKE_MSVC_RUNTIME_LIBRARY now.

I already did this sometime ago, just waiting and waiting for things to settle. However, I may try updating the patch to check the situation with regards to libc++ CI.

[mstorsjo]

In D144734#4349737, @andrewng wrote:

In D144734#4347261, @mstorsjo wrote:

FYI, the project minimum CMake version has more been bumped to 3.20, so this can be updated to only use CMAKE_MSVC_RUNTIME_LIBRARY now.

I already did this sometime ago, just waiting and waiting for things to settle. However, I may try updating the patch to check the situation with regards to libc++ CI.

Thanks! Things seem to take some time to settle though, as the CMake bump patch got reverted now.

Additionally, with https://reviews.llvm.org/D150688 it looks like we might end up sticking with the old way of picking CRT for some time still; primarily for other parts of llvm, but it might end up having to apply to libcxx too...

[andrewng]

Additionally, with https://reviews.llvm.org/D150688 it looks like we might end up sticking with the old way of picking CRT for some time still; primarily for other parts of llvm, but it might end up having to apply to libcxx too...

Hopefully, the "new" way of picking the flavour of MS run-time will be adopted across all projects, although that might not be so straightforward to do. Ideally it would need to support the current settings too during a transition period, assuming those settings can be effectively mapped.

[andrewng]

Update for review comments and to fix CI.

[mstorsjo]

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

libcxx/src/filesystem/operations.cpp

417

This change seems quite unrelated to the rest.

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

11

@jyknight This seems relevant for fixing running tests with debug builds, as discussed on Discord a couple of weeks ago.

31

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.

libcxx/test/support/set_windows_crt_report_mode.h

30

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?

[andrewng]

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

libcxx/src/filesystem/operations.cpp

417

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.

libcxx/test/support/set_windows_crt_report_mode.h

30

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!

[mstorsjo]

In D144734#4417378, @andrewng wrote:

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.

Right - but even if it's off by default, committing it requires general approval - the libcxx maintainers (not including me) have gone a fair bit in the other direction of removing such configurability to reduce the number of potential configurations, so adding new options, even if disabled, can also be controversial.

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

Ok, I see.

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.

While it's not super large, it has way too many orthogonal nontrivial changes, so it definitely does need to be split up.

If LIBCXX_ENABLE_EXPERIMENTAL_DLL is too controversial, it can always be removed later?

While I'm not a libcxx maintainer, I would say that that's not really the way it works - I don't think we merge things "just in case", but what's merged are things that are fully agreed on first.

[Mordante]

In D144734#4417496, @mstorsjo wrote:

In D144734#4417378, @andrewng wrote:

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.

While it's not super large, it has way too many orthogonal nontrivial changes, so it definitely does need to be split up.

I like that people try to be CI "friendly". But I consider a clean git history more important. When a patch breaks something on a platform we revert the entire patch, so when 3/4 of the changes are an improvement 4/4 will be reverted.

If LIBCXX_ENABLE_EXPERIMENTAL_DLL is too controversial, it can always be removed later?

While I'm not a libcxx maintainer, I would say that that's not really the way it works - I don't think we merge things "just in case", but what's merged are things that are fully agreed on first.

As mentioned in the comments this should really be a different patch and only be added if it's useful. We indeed don't add features that might be useful, we want features that are useful to (a part of) our user base.

As mentioned by @mstorsjo we have enabled 128-bit support on Windows now, please update your patch for these changes.

libcxx/CMakeLists.txt

755–758

Why not one target_link_libraries instead? I would like to keep one library + comment per line.

libcxx/src/CMakeLists.txt

270

I'm too quite wary of this flag, especially since there are no CI changes to test this. Most (all?) libc++ developers are not on Windows, so without a CI to test this it will break. I really want to see this in a separate patch.

libcxx/test/support/set_windows_crt_report_mode.h

12–22

Can you undo these unrelated formatting changes?

[andrewng]

While it's not super large, it has way too many orthogonal nontrivial changes, so it definitely does need to be split up.

I like that people try to be CI "friendly". But I consider a clean git history more important. When a patch breaks something on a platform we revert the entire patch, so when 3/4 of the changes are an improvement 4/4 will be reverted.

Yes, a clean git history is important but so is getting better platform test coverage. It's always somewhat of a balancing act in the end.

If LIBCXX_ENABLE_EXPERIMENTAL_DLL is too controversial, it can always be removed later?

While I'm not a libcxx maintainer, I would say that that's not really the way it works - I don't think we merge things "just in case", but what's merged are things that are fully agreed on first.

As mentioned in the comments this should really be a different patch and only be added if it's useful. We indeed don't add features that might be useful, we want features that are useful to (a part of) our user base.

The reason for the c++experimental DLL is that this is the simplest way to enable c++experimental testing when using the MSVC DLL run-time. There are alternative solutions which would involve using different "DLL" annotations for components of c++experimental but I believe this would be more of a maintenance burden/annoyance, particularly for non-Windows users!

As mentioned by @mstorsjo we have enabled 128-bit support on Windows now, please update your patch for these changes.

Yes, IIUC, int128 support has just been disabled for clang-cl. My intention with this patch was to enable int128 testing for clang-cl/MSVC despite all the issues regarding proper deployment of this feature. However, it seems that the preferred approach is to remove all int128 related support.

libcxx/CMakeLists.txt

755–758

Yes, definitely could be merged into one. I was just following the existing style on the assumption that it was the "preferred" style.

libcxx/src/CMakeLists.txt

270

IIRC, @mstorsjo suggested adding the "new" CI configurations as a separate patch. However, if I'm going to be splitting things up then I may be able to add CI configurations as appropriate. Are there any guidelines regarding how many CI configurations can/should be added? Also debug based configurations can be extremely slow!

libcxx/test/support/set_windows_crt_report_mode.h

12–22

I'm pretty sure this was clang-format's doing, but yes when I split this patch out, I can remove the clang-format changes if that's preferred.

[mstorsjo]

In D144734#4421798, @andrewng wrote:

While it's not super large, it has way too many orthogonal nontrivial changes, so it definitely does need to be split up.

I like that people try to be CI "friendly". But I consider a clean git history more important. When a patch breaks something on a platform we revert the entire patch, so when 3/4 of the changes are an improvement 4/4 will be reverted.

Yes, a clean git history is important but so is getting better platform test coverage.

To me, this isn't even so much about clean git history, it's primarily for reasoning about and deciding about each detail separately.

Right now, looking at the patch as a whole, my verdict for each of the 3-5 changes is "maybe". But it's very hard to move from "maybe" to "yes" unless the detail is crystal clear and all implications are discussed/brought up/agreed on. Split up, it's much easier to move a few of them from "maybe" to "yes", and discuss/settle/improve the details of the remaining bits.

Also FWIW, I might be able to help out with this (splitting/testing/adjusting of these patches) in a few weeks (sometime in July maybe).

The reason for the c++experimental DLL is that this is the simplest way to enable c++experimental testing when using the MSVC DLL run-time. There are alternative solutions which would involve using different "DLL" annotations for components of c++experimental but I believe this would be more of a maintenance burden/annoyance, particularly for non-Windows users!

Yes, making separate export/import annotations for c++experimental only for the sake of Windows probably won't fly. But as we have a working CI setup already, without c++experimental on Windows, we can do stepwise improvements on top of that, without testing c++experimental, even if your final target is to test even c++experimental too.

Yes, IIUC, int128 support has just been disabled for clang-cl. My intention with this patch was to enable int128 testing for clang-cl/MSVC despite all the issues regarding proper deployment of this feature. However, it seems that the preferred approach is to remove all int128 related support.

Yes, ideally we'd enable int128 support. But enabling it in the tests isn't enough, it would need to be enabled in every case of user code invoking the libc++ APIs that use int128 too. So for now that whole thing is deferred, pending discussions with @thieta @rnk @phosek @hans about how to best automatically link in compiler-rt builtins.

libcxx/src/CMakeLists.txt

270

Testing the debug configuration probably is out of scope for the CI if it's significantly slower than the usual release runs - we'll just have to test that manually occasionally.

But if we do add a way of building c++experimental as a DLL, in a separate patch solely for that, we should probably enable that in the current clang-cl dll CI test configuration as well (it probably doesn't require a separate CI run just for that).

[rnk]

We should prioritize fixing the int128 issues. I've lost track of the next step. Let me know where to focus if I can help move it along. I can see that it's blocking a lot of things. Is Tobias's linker library autodiscovery the next step? What prevents us from emitting the appropriate autolinking directives today in LLVM?

[mstorsjo]

In D144734#4422521, @rnk wrote:

We should prioritize fixing the int128 issues. I've lost track of the next step. Let me know where to focus if I can help move it along. I can see that it's blocking a lot of things. Is Tobias's linker library autodiscovery the next step? What prevents us from emitting the appropriate autolinking directives today in LLVM?

I think the private mail discsussion between us from around May 26th had a lot of good conclusions from yourself regarding how to move forward with this; we should probably mirror those conclusions and/or maybe the whole discussion somewhere public.

Getting the autolinking directives emitted properly (only when needed, in order to not break setups where clang_rt.builtins isn't available, e.g. cross compiling with just a bundled MSVC/WinSDK, without building clang runtimes for all MSVC cross targets) probaby is the biggest hurdle. As you noted there, due to LLVM_ENABLE_PER_TARGET_RUNTIME_DIR, the clang_rt.builtins files can exist with a couple different names and in a few different directories.

Once we do have autolinking directives emitted, we're almost there; for x86 targets, MSVC already does ship clang_rt.builtins-<arch>.lib, available in the link path. So there we don't really even need the linker library directory support either, unless we want the latest compiler-rt (and for arm) that we'd build ourselves, instead of whatever was shipped with MSVC.

[thieta]

Thanks for reminding me - I have posted a summary in discourse: https://discourse.llvm.org/t/improve-autolinking-of-compiler-rt-and-libc-on-windows-with-lld-link/71392

[andrewng]

I like that people try to be CI "friendly". But I consider a clean git history more important. When a patch breaks something on a platform we revert the entire patch, so when 3/4 of the changes are an improvement 4/4 will be reverted.

Yes, a clean git history is important but so is getting better platform test coverage.

To me, this isn't even so much about clean git history, it's primarily for reasoning about and deciding about each detail separately.

Right now, looking at the patch as a whole, my verdict for each of the 3-5 changes is "maybe". But it's very hard to move from "maybe" to "yes" unless the detail is crystal clear and all implications are discussed/brought up/agreed on. Split up, it's much easier to move a few of them from "maybe" to "yes", and discuss/settle/improve the details of the remaining bits.

Yes, I agree it will make review/evaluation much easier. It's been quite tricky finding all the issues and authoring this patch and this is the "cleaned up" version of my original patch which was even more complex!

Also FWIW, I might be able to help out with this (splitting/testing/adjusting of these patches) in a few weeks (sometime in July maybe).

This is much appreciated and I may well have to take you up on the offer as I don't know how much time I'll have to spend on it.

[mstorsjo]

In D144734#4424412, @andrewng wrote:

Also FWIW, I might be able to help out with this (splitting/testing/adjusting of these patches) in a few weeks (sometime in July maybe).

This is much appreciated and I may well have to take you up on the offer as I don't know how much time I'll have to spend on it.

I dug through this patch now, split things out and reimplement some bits. I omitted the shared libc++experimental and int128 builtins bits for now. (The int128 builtins aren't needed in a default configuration any longer, and there's work done on getting that fixed properly in clang/lld recently.) See D155553, D155554, D155555, D155560, D155561 and D155562 for the individual patches that were split out from here.

libcxx/CMakeLists.txt

 
 # Basic options ---------------------------------------------------------------
 option(LIBCXX_ENABLE_ASSERTIONS
   "Enable assertions inside the compiled library, and at the same time make it the
    default when compiling user code. Note that assertions can be enabled or disabled
    by users in their own code regardless of this option." OFF)
 option(LIBCXX_ENABLE_SHARED "Build libc++ as a shared library." ON)
 option(LIBCXX_ENABLE_STATIC "Build libc++ as a static library." ON)
+cmake_dependent_option(LIBCXX_ENABLE_EXPERIMENTAL_DLL
+  "Enable building of Windows c++experimental.dll." OFF
+  "LIBCXX_ENABLE_SHARED" OFF)
 option(LIBCXX_ENABLE_FILESYSTEM
   "Whether to include support for parts of the library that rely on a filesystem being
    available on the platform. This includes things like most parts of <filesystem> and
    others like <fstream>" ON)
 option(LIBCXX_INCLUDE_TESTS "Build the libc++ tests." ${LLVM_INCLUDE_TESTS})
 option(LIBCXX_ENABLE_DEBUG_MODE
   "Whether to build libc++ with the debug mode enabled.
    By default, this is turned off. Turning it on results in a different ABI (additional

 
 string(TOUPPER "${CMAKE_BUILD_TYPE}" uppercase_CMAKE_BUILD_TYPE)
 if (uppercase_CMAKE_BUILD_TYPE STREQUAL "DEBUG")
   set(LIBCXX_DEBUG_BUILD ON)
 else()
   set(LIBCXX_DEBUG_BUILD OFF)
 endif()
 
+if (LIBCXX_TARGETING_MSVC)
+  set(LIBCXX_MSVC_DYNAMIC_RUNTIME ON)
+  if (CMAKE_MSVC_RUNTIME_LIBRARY)
+    string(FIND ${CMAKE_MSVC_RUNTIME_LIBRARY} "DLL" idx)
+    if (${idx} EQUAL -1)
+      set(LIBCXX_MSVC_DYNAMIC_RUNTIME OFF)
+    endif()
+  endif()
+endif()
+
 #===============================================================================
 # Setup Compiler Flags
 #===============================================================================
 
 include(HandleLibCXXABI) # Setup the ABI library flags
 
-# 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")
-
 # 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 respectively.
 remove_flags(-Wno-pedantic -pedantic-errors -pedantic)
 
 # Required flags ==============================================================
 function(cxx_add_basic_build_flags target)
 
   # Require C++20 for all targets. C++17 is needed to use aligned allocation
   # in the dylib. C++20 is needed to use char8_t.
   set_target_properties(${target} PROPERTIES
     CXX_STANDARD 20
     CXX_STANDARD_REQUIRED YES
     CXX_EXTENSIONS NO)
 
+  if (LIBCXX_TARGETING_MSVC)
+    cmake_parse_arguments(ARG "DYNAMIC_RUNTIME" "" "" ${ARGN})
+    set_property(TARGET ${target} PROPERTY
+      MSVC_RUNTIME_LIBRARY "MultiThreaded$<${LIBCXX_DEBUG_BUILD}:Debug>$<$<OR:${ARG_DYNAMIC_RUNTIME},${LIBCXX_MSVC_DYNAMIC_RUNTIME}>:DLL>")
+  endif()
+
   # When building the dylib, don't warn for unavailable aligned allocation
   # functions based on the deployment target -- they are always available
   # because they are provided by the dylib itself with the exception of z/OS.
   if (ZOS)
     target_add_compile_flags_if_supported(${target} PRIVATE -fno-aligned-allocation)
   else()
     target_add_compile_flags_if_supported(${target} PRIVATE -faligned-allocation)
   endif()
if (LIBCXX_HAS_ATOMIC_LIB)
     target_link_libraries(${target} PRIVATE atomic)
   endif()
 
   if (MINGW)
     target_link_libraries(${target} PRIVATE "${MINGW_LIBRARIES}")
   endif()
 
   if (LIBCXX_TARGETING_MSVC)
+    cmake_parse_arguments(ARG "DYNAMIC_RUNTIME" "" "" ${ARGN})
+
     if (LIBCXX_DEBUG_BUILD)
       set(LIB_SUFFIX "d")
     else()
       set(LIB_SUFFIX "")
     endif()
 
-    target_link_libraries(${target} PRIVATE ucrt${LIB_SUFFIX}) # Universal C runtime
-    target_link_libraries(${target} PRIVATE vcruntime${LIB_SUFFIX}) # C++ runtime
-    target_link_libraries(${target} PRIVATE msvcrt${LIB_SUFFIX}) # C runtime startup files
-    target_link_libraries(${target} PRIVATE msvcprt${LIB_SUFFIX}) # C++ standard library. Required for exception_ptr internals.
+    if (ARG_DYNAMIC_RUNTIME OR LIBCXX_MSVC_DYNAMIC_RUNTIME)
+      set(UCRT_LIB "ucrt")
+      set(CXXRT_LIB "vcruntime")
+      set(CRT_LIB "msvcrt")
+      set(CXX_LIB "msvcprt")
+    else()
+      set(UCRT_LIB "libucrt")
+      set(CXXRT_LIB "libvcruntime")
+      set(CRT_LIB "libcmt")
+      set(CXX_LIB "libcpmt")
+    endif()
+
+    target_link_libraries(${target} PRIVATE ${UCRT_LIB}${LIB_SUFFIX}) # Universal C runtime
+    target_link_libraries(${target} PRIVATE ${CXXRT_LIB}${LIB_SUFFIX}) # C++ runtime
+    target_link_libraries(${target} PRIVATE ${CRT_LIB}${LIB_SUFFIX}) # C runtime startup files
+    target_link_libraries(${target} PRIVATE ${CXX_LIB}${LIB_SUFFIX}) # C++ standard library. Required for exception_ptr internals.
     # Required for standards-complaint wide character formatting functions
     # (e.g. `printfw`/`scanfw`)
     target_link_libraries(${target} PRIVATE iso_stdio_wide_specifiers)
   endif()
 
   if (ANDROID AND ANDROID_PLATFORM_LEVEL LESS 21)
     target_link_libraries(${target} PUBLIC android_support)
   endif()

 if (WIN32 AND LIBCXX_ENABLE_STATIC_ABI_LIBRARY)
   # If linking libcxxabi statically into libcxx, skip the dllimport attributes
   # on symbols we refer to from libcxxabi.
   add_definitions(-D_LIBCXXABI_DISABLE_VISIBILITY_ANNOTATIONS)
 endif()
 
 # Setup all common build flags =================================================
 function(cxx_add_common_build_flags target)
-  cxx_add_basic_build_flags(${target})
+  cxx_add_basic_build_flags(${target} ${ARGN})
   cxx_add_warning_flags(${target} ${LIBCXX_ENABLE_WERROR} ${LIBCXX_ENABLE_PEDANTIC})
   cxx_add_windows_flags(${target})
   cxx_add_exception_flags(${target})
   cxx_add_rtti_flags(${target})
   cxx_add_module_flags(${target})
-  cxx_link_system_libraries(${target})
+  cxx_link_system_libraries(${target} ${ARGN})
 endfunction()
 
 #===============================================================================
 # Setup Source Code And Tests
 #===============================================================================
 add_subdirectory(include)
 add_subdirectory(src)
 add_subdirectory(utils)

libcxx/src/CMakeLists.txt

set_target_properties(cxx_shared
     PROPERTIES
       COMPILE_FLAGS "${LIBCXX_COMPILE_FLAGS}"
       LINK_FLAGS    "${LIBCXX_LINK_FLAGS}"
       OUTPUT_NAME   "${LIBCXX_SHARED_OUTPUT_NAME}"
       VERSION       "${LIBCXX_LIBRARY_VERSION}"
       SOVERSION     "${LIBCXX_ABI_VERSION}"
       DEFINE_SYMBOL ""
   )
-  cxx_add_common_build_flags(cxx_shared)
+  cxx_add_common_build_flags(cxx_shared DYNAMIC_RUNTIME)
 
   if(ZOS)
     add_custom_command(TARGET cxx_shared POST_BUILD
       COMMAND
         ${LIBCXX_SOURCE_DIR}/utils/zos_rename_dll_side_deck.sh
         $<TARGET_LINKER_FILE_NAME:cxx_shared> $<TARGET_FILE_NAME:cxx_shared> "${LIBCXX_DLL_NAME}"
       COMMENT "Rename dll name inside the side deck file"
       WORKING_DIRECTORY $<TARGET_FILE_DIR:cxx_shared>
if (LIBCXX_ENABLE_SHARED)
   list(APPEND LIBCXX_BUILD_TARGETS "cxx_shared")
   if(WIN32 AND NOT MINGW AND NOT "${CMAKE_HOST_SYSTEM_NAME}" STREQUAL "Windows")
     # Since we most likely do not have a mt.exe replacement, disable the
     # manifest bundling.  This allows a normal cmake invocation to pass which
     # will attempt to use the manifest tool to generate the bundled manifest
     set_target_properties(cxx_shared PROPERTIES
                           APPEND_STRING PROPERTY LINK_FLAGS " /MANIFEST:NO")
   endif()
+  if(LIBCXX_TARGETING_MSVC AND LIBCXX_ENABLE_EXPERIMENTAL_DLL)
+    target_link_libraries(cxx_shared PRIVATE cxx_experimental_stub)
+    set(LIBCXX_MSVC_EXPERIMENTAL_SHARED ON)
+  endif()
 endif()
 
 set(CMAKE_STATIC_LIBRARY_PREFIX "lib")
 
 # Build the static library.
 if (LIBCXX_ENABLE_STATIC)
   add_library(cxx_static STATIC ${exclude_from_all} ${LIBCXX_SOURCES} ${LIBCXX_HEADERS})
   target_include_directories(cxx_static PRIVATE ${CMAKE_CURRENT_SOURCE_DIR})

 set_target_properties(cxx_experimental
   PROPERTIES
     COMPILE_FLAGS "${LIBCXX_COMPILE_FLAGS}"
     OUTPUT_NAME   "c++experimental"
 )
 cxx_add_common_build_flags(cxx_experimental)
 target_compile_options(cxx_experimental PUBLIC -D_LIBCPP_ENABLE_EXPERIMENTAL)
 
+if (LIBCXX_MSVC_EXPERIMENTAL_SHARED)
+  add_library(cxx_experimental_shared SHARED ${LIBCXX_EXPERIMENTAL_SOURCES})
+  target_link_libraries(cxx_experimental_shared PUBLIC cxx-headers)
+  target_link_libraries(cxx_experimental_shared PRIVATE cxx_shared)
+
+  set_target_properties(cxx_experimental_shared
+    PROPERTIES
+      COMPILE_FLAGS "${LIBCXX_COMPILE_FLAGS}"
+      OUTPUT_NAME   "c++experimental"
+  )
+  cxx_add_common_build_flags(cxx_experimental_shared DYNAMIC_RUNTIME)
+  target_compile_options(cxx_experimental_shared PUBLIC -D_LIBCPP_ENABLE_EXPERIMENTAL)
+
+  add_library(cxx_experimental_stub SHARED ${LIBCXX_EXPERIMENTAL_SOURCES})
+  target_link_libraries(cxx_experimental_stub PUBLIC cxx-headers)
+
+  set_target_properties(cxx_experimental_stub
+    PROPERTIES
+      COMPILE_FLAGS "${LIBCXX_COMPILE_FLAGS}"
+      LINK_FLAGS    "/FORCE:UNRESOLVED"
+      RUNTIME_OUTPUT_NAME      "c++experimental"
+      ARCHIVE_OUTPUT_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}"
+      LIBRARY_OUTPUT_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}"
+      RUNTIME_OUTPUT_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}"
+  )
+  cxx_add_common_build_flags(cxx_experimental_stub DYNAMIC_RUNTIME)
+  target_compile_options(cxx_experimental_stub PUBLIC -D_LIBCPP_ENABLE_EXPERIMENTAL)
+endif()
 
 if (LIBCXX_BUILD_EXTERNAL_THREAD_LIBRARY)
   set(LIBCXX_EXTERNAL_THREADING_SUPPORT_SOURCES
       "${CMAKE_CURRENT_SOURCE_DIR}/../test/support/external_threads.cpp")
 
   if (LIBCXX_ENABLE_SHARED)
     add_library(cxx_external_threads SHARED ${LIBCXX_EXTERNAL_THREADING_SUPPORT_SOURCES})
   else()
     add_library(cxx_external_threads STATIC ${LIBCXX_EXTERNAL_THREADING_SUPPORT_SOURCES})
   endif()
 
   set_target_properties(cxx_external_threads
     PROPERTIES
       LINK_FLAGS    "${LIBCXX_LINK_FLAGS}"
       COMPILE_FLAGS "${LIBCXX_COMPILE_FLAGS}"
       OUTPUT_NAME   "c++external_threads"
   )
 
   target_link_libraries(cxx_external_threads PRIVATE cxx-headers)
 endif()
 
 if (LIBCXX_INSTALL_SHARED_LIBRARY)
-  install(TARGETS cxx_shared
-    ARCHIVE DESTINATION ${LIBCXX_INSTALL_LIBRARY_DIR} COMPONENT cxx
-    LIBRARY DESTINATION ${LIBCXX_INSTALL_LIBRARY_DIR} COMPONENT cxx
-    RUNTIME DESTINATION ${LIBCXX_INSTALL_RUNTIME_DIR} COMPONENT cxx)
+  list(APPEND install_targets cxx_shared)
 endif()
 
 if (LIBCXX_INSTALL_STATIC_LIBRARY)
-  install(TARGETS cxx_static
-    ARCHIVE DESTINATION ${LIBCXX_INSTALL_LIBRARY_DIR} COMPONENT cxx
-    LIBRARY DESTINATION ${LIBCXX_INSTALL_LIBRARY_DIR} COMPONENT cxx
-    RUNTIME DESTINATION ${LIBCXX_INSTALL_RUNTIME_DIR} COMPONENT cxx)
+  list(APPEND install_targets cxx_static)
 endif()
 
 if (LIBCXX_INSTALL_LIBRARY)
-  install(TARGETS cxx_experimental
+  list(APPEND install_targets cxx_experimental)
+  if (LIBCXX_MSVC_EXPERIMENTAL_SHARED)
+    list(APPEND install_targets cxx_experimental_shared)
+  endif()
+endif()
+
+if (install_targets)
+  install(TARGETS ${install_targets}
     LIBRARY DESTINATION ${LIBCXX_INSTALL_LIBRARY_DIR} COMPONENT cxx
     ARCHIVE DESTINATION ${LIBCXX_INSTALL_LIBRARY_DIR} COMPONENT cxx
     RUNTIME DESTINATION ${LIBCXX_INSTALL_RUNTIME_DIR} COMPONENT cxx)
 endif()
 
 # NOTE: This install command must go after the cxx install command otherwise
 # it will not be executed after the library symlinks are installed.
 if (LIBCXX_ENABLE_SHARED AND LIBCXX_ENABLE_ABI_LINKER_SCRIPT)
   install(FILES "$<TARGET_LINKER_FILE:cxx_shared>"
     DESTINATION ${LIBCXX_INSTALL_LIBRARY_DIR}
     COMPONENT libcxx)
 endif()
 
 if (NOT CMAKE_CONFIGURATION_TYPES)
     if(LIBCXX_INSTALL_LIBRARY)
       set(lib_install_target "cxx;cxx_experimental")
+      if (LIBCXX_MSVC_EXPERIMENTAL_SHARED)
+        string(APPEND lib_install_target ";experimental_shared")
+      endif()
     endif()
     if(LIBCXX_INSTALL_HEADERS)
       set(header_install_target install-cxx-headers)
     endif()
     add_custom_target(install-cxx
                       DEPENDS ${lib_install_target}
                               cxx_experimental
                               ${header_install_target}

libcxx/src/filesystem/operations.cpp

 
 errc __win_err_to_errc(int err) {
   constexpr struct {
     DWORD win;
     errc errc;
   } win_error_mapping[] = {
       {ERROR_ACCESS_DENIED, errc::permission_denied},
       {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},
       {ERROR_BAD_PATHNAME, errc::no_such_file_or_directory},
       {ERROR_BAD_UNIT, errc::no_such_device},
       {ERROR_BROKEN_PIPE, errc::broken_pipe},
       {ERROR_BUFFER_OVERFLOW, errc::filename_too_long},
       {ERROR_BUSY, errc::device_or_resource_busy},
       {ERROR_BUSY_DRIVE, errc::device_or_resource_busy},
       {ERROR_CANNOT_MAKE, errc::permission_denied},

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

 # This testing configuration handles running the test suite against LLVM's libc++
 # using a DLL, with Clang-cl on Windows.
 
+runtime_lib = 'msvcprt'
+fms_runtime_lib = 'dll'
+dbg_include = ''
+
+if '@LIBCXX_DEBUG_BUILD@' == 'ON':
+    fms_runtime_lib += '_dbg'
+    runtime_lib += 'd'
+    dbg_include = ' -include set_windows_crt_report_mode.h'
+
 lit_config.load_config(config, '@CMAKE_CURRENT_BINARY_DIR@/cmake-bridge.cfg')
 
 config.substitutions.append(('%{flags}', '--driver-mode=g++'))
 config.substitutions.append(('%{compile_flags}',
-    '-nostdinc++ -I %{include} -I %{target-include} -I %{libcxx}/test/support -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_STDIO_ISO_WIDE_SPECIFIERS -DNOMINMAX'
+    '-fms-runtime-lib=' + fms_runtime_lib + ' -nostdinc++ -I %{include} -I %{target-include} -I %{libcxx}/test/support -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_STDIO_ISO_WIDE_SPECIFIERS -DNOMINMAX' + dbg_include
 ))
+link_flags_idx = len(config.substitutions)
 config.substitutions.append(('%{link_flags}',
-    '-nostdlib -L %{lib} -lc++ -lmsvcrt -lmsvcprt -loldnames'
+    '-nostartfiles -nostdlib++ -L %{lib} -lc++ -l' + runtime_lib
 ))
 config.substitutions.append(('%{exec}',
     '%{executor} --execdir %T --prepend_env PATH=%{lib} -- '
 ))
 
 import os, site
 site.addsitedir(os.path.join('@LIBCXX_SOURCE_DIR@', 'utils'))
 import libcxx.test.params, libcxx.test.config
+
+if not '_LIBCPP_HAS_NO_INT128' in libcxx.test.dsl.compilerMacros(config):
+    subst = config.substitutions[link_flags_idx]
+    config.substitutions[link_flags_idx] = (subst[0], '-rtlib=compiler-rt ' + subst[1])
+
 libcxx.test.config.configure(
     libcxx.test.params.DEFAULT_PARAMETERS,
     libcxx.test.features.DEFAULT_FEATURES,
     config,
     lit_config
 )

libcxx/test/configs/llvm-libc++-shared-no-vcruntime-clangcl.cfg.in

 # This testing configuration handles running the test suite against LLVM's libc++
 # using a DLL, with Clang-cl on Windows. This variant sets _HAS_EXCEPTIONS = 0
-# which removes exception class definitions  from the vcruntime.
+# which removes exception class definitions from the vcruntime.
 
+runtime_lib = 'msvcprt'
+fms_runtime_lib = 'dll'
+dbg_include = ''
+
+if '@LIBCXX_DEBUG_BUILD@' == 'ON':
+    fms_runtime_lib += '_dbg'
+    runtime_lib += 'd'
+    dbg_include = ' -include set_windows_crt_report_mode.h'
+
 lit_config.load_config(config, '@CMAKE_CURRENT_BINARY_DIR@/cmake-bridge.cfg')
 
 config.substitutions.append(('%{flags}', '--driver-mode=g++'))
 config.substitutions.append(('%{compile_flags}',
-    '-nostdinc++ -I %{include} -I %{target-include} -I %{libcxx}/test/support -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_STDIO_ISO_WIDE_SPECIFIERS -DNOMINMAX -D_HAS_EXCEPTIONS=0'
+    '-fms-runtime-lib=' + fms_runtime_lib + ' -nostdinc++ -I %{include} -I %{target-include} -I %{libcxx}/test/support -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_STDIO_ISO_WIDE_SPECIFIERS -DNOMINMAX -D_HAS_EXCEPTIONS=0' + dbg_include
 ))
+link_flags_idx = len(config.substitutions)
 config.substitutions.append(('%{link_flags}',
-    '-nostdlib -L %{lib} -lc++ -lmsvcrt -lmsvcprt -loldnames'
+    '-nostartfiles -nostdlib++ -L %{lib} -lc++ -l' + runtime_lib
 ))
 config.substitutions.append(('%{exec}',
     '%{executor} --execdir %T --prepend_env PATH=%{lib} -- '
 ))
 
 import os, site
 site.addsitedir(os.path.join('@LIBCXX_SOURCE_DIR@', 'utils'))
 import libcxx.test.params, libcxx.test.config
+
+if not '_LIBCPP_HAS_NO_INT128' in libcxx.test.dsl.compilerMacros(config):
+    subst = config.substitutions[link_flags_idx]
+    config.substitutions[link_flags_idx] = (subst[0], '-rtlib=compiler-rt ' + subst[1])
+
 libcxx.test.config.configure(
     libcxx.test.params.DEFAULT_PARAMETERS,
     libcxx.test.features.DEFAULT_FEATURES,
     config,
     lit_config
 )

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

 # This testing configuration handles running the test suite against LLVM's libc++
 # using a static library, with Clang-cl on Windows.
 
+if '@LIBCXX_MSVC_DYNAMIC_RUNTIME@' == 'ON':
+    fms_runtime_lib = 'dll'
+    runtime_lib = 'msvcprt'
+else:
+    fms_runtime_lib = 'static'
+    runtime_lib = 'libcpmt'
+
+dbg_include = ''
+
+if '@LIBCXX_DEBUG_BUILD@' == 'ON':
+    fms_runtime_lib += '_dbg'
+    runtime_lib += 'd'
+    dbg_include = ' -include set_windows_crt_report_mode.h'
+
 lit_config.load_config(config, '@CMAKE_CURRENT_BINARY_DIR@/cmake-bridge.cfg')
 
 config.substitutions.append(('%{flags}', '--driver-mode=g++'))
 config.substitutions.append(('%{compile_flags}',
-    '-nostdinc++ -I %{include} -I %{target-include} -I %{libcxx}/test/support -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_STDIO_ISO_WIDE_SPECIFIERS -DNOMINMAX'
+    '-fms-runtime-lib=' + fms_runtime_lib + ' -nostdinc++ -I %{include} -I %{target-include} -I %{libcxx}/test/support -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_STDIO_ISO_WIDE_SPECIFIERS -DNOMINMAX -D_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS=' + dbg_include
 ))
+link_flags_idx = len(config.substitutions)
 config.substitutions.append(('%{link_flags}',
-    '-nostdlib -L %{lib} -llibc++ -lmsvcrt -lmsvcprt -loldnames'
+    '-nostartfiles -nostdlib++ -L %{lib} -llibc++ -l' + runtime_lib
 ))
 config.substitutions.append(('%{exec}',
     '%{executor} --execdir %T -- '
 ))
 
 import os, site
 site.addsitedir(os.path.join('@LIBCXX_SOURCE_DIR@', 'utils'))
 import libcxx.test.params, libcxx.test.config
+
+if not '_LIBCPP_HAS_NO_INT128' in libcxx.test.dsl.compilerMacros(config):
+    subst = config.substitutions[link_flags_idx]
+    config.substitutions[link_flags_idx] = (subst[0], '-rtlib=compiler-rt ' + subst[1])
+
 libcxx.test.config.configure(
     libcxx.test.params.DEFAULT_PARAMETERS,
     libcxx.test.features.DEFAULT_FEATURES,
     config,
     lit_config
 )

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

 void test_allocate_deallocate()
 {
     ex::memory_resource & r1 = *ex::new_delete_resource();
 
     globalMemCounter.reset();
 
     void *ret = r1.allocate(50);
     assert(ret);
-    assert(globalMemCounter.checkOutstandingNewEq(1));
-    assert(globalMemCounter.checkLastNewSizeEq(50));
+    ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS(globalMemCounter.checkOutstandingNewEq(1));
+    ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS(globalMemCounter.checkLastNewSizeEq(50));
 
     r1.deallocate(ret, 1);
-    assert(globalMemCounter.checkOutstandingNewEq(0));
-    assert(globalMemCounter.checkDeleteCalledEq(1));
-
+    ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS(globalMemCounter.checkOutstandingNewEq(0));
+    ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS(globalMemCounter.checkDeleteCalledEq(1));
 }
 
 int main(int, char**)
 {
     static_assert(noexcept(ex::new_delete_resource()), "Must be noexcept");
     test_return();
     test_equality();
     test_allocate_deallocate();
 
   return 0;
 }

libcxx/test/support/set_windows_crt_report_mode.h

 // -*- C++ -*-
 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef SUPPORT_SET_WINDOWS_CRT_REPORT_MODE_H
 #define SUPPORT_SET_WINDOWS_CRT_REPORT_MODE_H
 
-#ifndef _DEBUG
-#error _DEBUG must be defined when using this header
-#endif
+#if defined(__cplusplus) && __cplusplus > 199711L
+#  ifndef _DEBUG
+#    error _DEBUG must be defined when using this header
+#  endif
 
-#ifndef _WIN32
-#error This header can only be used when targeting Windows
-#endif
+#  ifndef _WIN32
+#    error This header can only be used when targeting Windows
+#  endif
 
-#include <crtdbg.h>
+#  include <crtdbg.h>
 
 // On Windows in debug builds the default assertion handler opens a new dialog
 // window which must be dismissed manually by the user. This function overrides
 // that setting and instead changes the assertion handler to log to stderr
 // instead.
-inline int init_crt_report_mode() {
-  _CrtSetReportMode(_CRT_WARN, _CRTDBG_MODE_DEBUG);
-  _CrtSetReportMode(_CRT_ERROR, _CRTDBG_MODE_DEBUG);
-  _CrtSetReportMode(_CRT_ASSERT, _CRTDBG_MODE_DEBUG);
+inline int init_crt_report_mode(void) {
+  _CrtSetReportMode(_CRT_WARN, _CRTDBG_MODE_FILE);
+  _CrtSetReportFile(_CRT_WARN, _CRTDBG_FILE_STDERR);
+  _CrtSetReportMode(_CRT_ERROR, _CRTDBG_MODE_FILE);
+  _CrtSetReportFile(_CRT_ERROR, _CRTDBG_FILE_STDERR);
+  _CrtSetReportMode(_CRT_ASSERT, _CRTDBG_MODE_FILE);
+  _CrtSetReportFile(_CRT_ASSERT, _CRTDBG_FILE_STDERR);
   return 0;
 }
 
 static int init_crt_anchor = init_crt_report_mode();
+#endif
 
 #endif // SUPPORT_SET_WINDOWS_CRT_REPORT_MODE_H

libcxx/utils/libcxx/test/features.py

 _isClang = lambda cfg: "__clang__" in compilerMacros(
     cfg
 ) and "__apple_build_version__" not in compilerMacros(cfg)
 _isAppleClang = lambda cfg: "__apple_build_version__" in compilerMacros(cfg)
 _isGCC = lambda cfg: "__GNUC__" in compilerMacros(
     cfg
 ) and "__clang__" not in compilerMacros(cfg)
 _isMSVC = lambda cfg: "_MSC_VER" in compilerMacros(cfg)
+_isMSVCStatic = lambda cfg: _isMSVC(
+    cfg
+) and "_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS" in compilerMacros(cfg)
 _msvcVersion = lambda cfg: (
     int(compilerMacros(cfg)["_MSC_VER"]) // 100,
     int(compilerMacros(cfg)["_MSC_VER"]) % 100,
 )
 
 
 def _getSuitableClangTidy(cfg):
     try:

libcxx/utils/libcxx/test/params.py

 # ===----------------------------------------------------------------------===##
 #
 # Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 # See https://llvm.org/LICENSE.txt for license information.
 # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 #
 # ===----------------------------------------------------------------------===##
 
 from libcxx.test.dsl import *
-from libcxx.test.features import _isMSVC
+from libcxx.test.features import _isMSVCStatic
 import re
 
 _warningFlags = [
     "-Werror",
     "-Wall",
     "-Wctad-maybe-unsupported",
     "-Wextra",
     "-Wshadow",
Parameter(
             # When linking in MSVC mode via the Clang driver, a -l<foo>
             # maps to <foo>.lib, so we need to use -llibc++experimental here
             # to make it link against the static libc++experimental.lib.
             # We can't check for the feature 'msvc' in available_features
             # as those features are added after processing parameters.
             AddFeature("c++experimental"),
             PrependLinkFlag(
                 lambda cfg: "-llibc++experimental"
-                if _isMSVC(cfg)
+                if _isMSVCStatic(cfg)
                 else "-lc++experimental"
             ),
             AddCompileFlag("-D_LIBCPP_ENABLE_EXPERIMENTAL"),
         ]
         if experimental
         else [
             AddFeature("libcpp-has-no-incomplete-pstl"),
         ],