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

Wed Jun 14 13:01:02 PDT 2023

mstorsjo added subscribers: thieta, phosek, hans, rnk.
mstorsjo added a comment.

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

================
Comment at: libcxx/src/CMakeLists.txt:270
   endif()
+  if(LIBCXX_TARGETING_MSVC AND LIBCXX_ENABLE_EXPERIMENTAL_DLL)
+    target_link_libraries(cxx_shared PRIVATE cxx_experimental_stub)
----------------
andrewng wrote:
> Mordante wrote:
> > 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.
> 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!
> 
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).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144734