[libcxx-commits] [PATCH] D158450: [libcxx] Provide set_new_handler/get_new_handler on Windows

[Petr Hosek via Phabricator via libcxx-commits]

phosek added a comment.

In D158450#4604722 <https://reviews.llvm.org/D158450#4604722>, @mstorsjo wrote:

> In D158450#4604636 <https://reviews.llvm.org/D158450#4604636>, @phosek wrote:
>
>> I ran into this while trying to use libc++ on Windows, see https://discourse.llvm.org/t/re-land-stdlib-libc-support-in-clang-cl/61406/7?u=petrhosek for more details.
>
> Thanks for the context!
>
> However we do have tests that exercise `std::set_new_handler`, and they are executed successfully in the clang-cl configuration - see e.g. https://github.com/llvm/llvm-project/blob/llvmorg-18-init/libcxx/test/std/language.support/support.dynamic/alloc.errors/set.new.handler/set_new_handler.pass.cpp. The test that uses `std::get_new_handler`, https://github.com/llvm/llvm-project/blob/llvmorg-18-init/libcxx/test/std/language.support/support.dynamic/alloc.errors/set.new.handler/get_new_handler.pass.cpp, does have an XFAIL for the MSVC configuration though.
>
> The tests link explicitly against `msvcprt`, see https://github.com/llvm/llvm-project/blob/llvmorg-18-init/libcxx/test/configs/llvm-libc%2B%2B-shared-clangcl.cfg.in#L11. I'm pretty sure that these tests plus a bunch of other tests would start failing if we'd remove the explicit linking against `msvcprt`. (I think it might be useful to do such a CI test run to see exactly how much currently relies on explicitly linking against that library, even if linking libc++ as a DLL.)
>
> So currently, I would say that to use libc++ in a clang-cl setting, you need to link against both libc++ and msvcprt (and we have fairly complete test coverage for this configuration). I would expect that this is how e.g. Chromium are using it too.
>
> Reducing the reliance on msvcprt certainly is a good thing to do in any case, but as we do have a current known working test setup in CI, I think it would be good to frame these changes based on that; e.g. "provide these functions that previously were used implicitly from msvcprt" - or even better, change the test config to no longer link directly against msvcprt and keep that config green.

I agree, I think this is the right approach, especially given the number of different configurations there are on Windows which makes it difficult to exhaustively test everything locally. I noticed that the presubmit tests are currently failing so the next step for me is to debug those issues and make sure that all presubmit tests are green.

In D158450#4605626 <https://reviews.llvm.org/D158450#4605626>, @Mordante wrote:

> I think this resolves https://github.com/llvm/llvm-project/issues/64813. Is that correct?

Yes, I believe this change should resolved that issue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158450