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

Thu Mar 2 02:06:29 PST 2023

andrewng added a comment.

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

================
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:
> 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.
It's part of `std::experimental` which is excluded from testing in CI, so that's why it isn't causing any issues.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144734