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

Mon Feb 27 07:48:29 PST 2023

andrewng added a comment.

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.

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

================
Comment at: libcxx/test/configs/llvm-libc++-static-clangcl.cfg.in:25
 config.substitutions.append(('%{link_flags}',
-    '-nostdlib -L %{lib} -llibc++ -lmsvcrt -lmsvcprt -loldnames'
+    '-nostartfiles -nostdlib++ -rtlib=compiler-rt -L %{lib} -llibc++ -l' + runtime_lib
 ))
----------------
mstorsjo wrote:
> What does the newly added `-rtlib=compiler-rt` have to do with the rest of the changes here?
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?

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144734