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

Mon Feb 27 14:07:41 PST 2023

mstorsjo added a comment.

In D144734#4155353 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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.

================
Comment at: libcxx/CMakeLists.txt:489
+if (LIBCXX_TARGETING_MSVC)
+  string(REGEX MATCH "(^| )/MTd?( |$)" result ${CMAKE_CXX_FLAGS_${uppercase_CMAKE_BUILD_TYPE}})
+  if ("${result}" STREQUAL "")
----------------
andrewng wrote:
> mstorsjo wrote:
> > You'd want to match for `-` in addition to `/` for the character starting the option; `-MD` works just as well as `/MD`.
> > 
> 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.
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.

================
Comment at: libcxx/test/std/experimental/memory/memory.resource.global/new_delete_resource.pass.cpp:94
     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));
----------------
andrewng wrote:
> mstorsjo wrote:
> > This looks like a change that probably could be split out and landed separately, before the rest?
> 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.
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144734