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

[Martin Storsjö via Phabricator via libcxx-commits]

mstorsjo added a comment.

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.



================
Comment at: libcxx/src/new_handler.cpp:33
 
 new_handler get_new_handler() noexcept { return __libcpp_atomic_load(&__new_handler); }
 
----------------
This uses our own implementation of this, while in the linked Discourse post, you mentioned that it probably should use https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/set-new-handler?view=msvc-170 instead. Do you know of any practical pros/cons between the two approaches?

I guess the MS `_set_new_handler` resides in the CRT, so the setting might be shared across a larger portion of the app (e.g. if we've got multiple statically linked libc++ instances, but we're linking against a shared UCRT).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158450