[libcxx] Provide set_new_handler/get_new_handler on Windows

[phosek]

Summary

These are not provided by VCRuntime so we need to provide our own.

[phosek]

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.

[mstorsjo]

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

libcxx/src/new_handler.cpp

66

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

[phosek]

libcxx/src/new_handler.cpp

66

The issue is that _set_new_handler has a slightly different interface from std::set_new_handler; specifically the _set_new_handler handler is expected to return an integer value (1 in the case of success), whereas std::new_handler doesn't have a return value.

This means that in order to use _set_new_handler, we need to use an adapter that invokes the provided std::new_handler and then return 1. There's also no _get_new_handler so we need a static global variable to store the provided std::new_handler. That's what STL implementation uses as well.

If we had a multiple instances of libc++ inside the process, e.g. multiple shared libraries each statically linking a copy of libc++, each of those libraries would have a different view of what the new_handler is which wouldn't match that of _set_new_handler and might potentially break logic like https://github.com/llvm/llvm-project/blob/7c4e8c6a273f25b3ef33e9c123b3969632ab59bb/libcxx/src/new.cpp#L34.

With our own implementation of this, every library would still have its own view of what new_handler is, but at least that view is internally consistent.

[mstorsjo]

libcxx/src/new_handler.cpp

66

Ok, that sounds very reasonable! Yes I don't disagree with the direction per se, on the contrary, I just wanted the consideration done more publicly, as the earlier Discourse post pointed towards that implementation.

[Mordante]

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

[phosek]

In D158450#4604722, @mstorsjo wrote:

In 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, @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.

[mstorsjo]

I see that the CI build pointed out a bunch of practical issues:

• The DLL build fails with errors like this:

include\c++\v1\new(170,39): error: redeclaration of 'std::set_new_handler' should not add 'dllexport' attribute [-Werror,-Wdll-attribute-on-redeclaration]
_LIBCPP_EXPORTED_FROM_ABI new_handler set_new_handler(new_handler) _NOEXCEPT;
                                      ^
C:\Program Files (x86)\Windows Kits\10\Include\10.0.19041.0\ucrt\new.h(32,42): note: previous declaration is here
            _CRTIMP2 new_handler __cdecl set_new_handler(_In_opt_ new_handler _NewHandler) throw();
                                         ^

• The test llvm-libc++-static-clangcl.cfg.in :: std/language.support/support.dynamic/alloc.errors/set.new.handler/get_new_handler.pass.cpp which previously was XFAILed no longer fails. This is good at least :-)
• A bunch of tests in std/language.support/support.dynamic/new.delete/new.delete.{single,array} fail. I would guess that the issue is that we get the various instances of operator new from vcruntime, and on failures, vcruntime calls the preexisting new handler from there, instead of the new one we provide in libc++. I guess that means that unless we move to provide all operator new ourselves, we'd need to build on top of msvcprt's _set_new_handler like STL does.

[phosek]

libcxx/src/new_handler.cpp

66

I just realized that https://github.com/llvm/llvm-project/blob/7c4e8c6a273f25b3ef33e9c123b3969632ab59bb/libcxx/src/new.cpp is compiled out when using VCRuntime so I guess my concern is main moot and we could use the same approach as STL.

[mstorsjo]

libcxx/include/new

172

Hmm, does this mean that these functions won't be exported in the case of the DLL build? Wouldn't that lead to linking against the msvcprt implementation for set_new_handler and failing to link get_new_handler?

I see that this did pass in the CI build though - how does that work?

libcxx/include/new

class _LIBCPP_EXPORTED_FROM_ABI bad_array_new_length
     : public bad_alloc
 {
 public:
     bad_array_new_length() _NOEXCEPT;
     ~bad_array_new_length() _NOEXCEPT override;
     const char* what() const _NOEXCEPT override;
 };
 
-typedef void (*new_handler)();
-_LIBCPP_EXPORTED_FROM_ABI new_handler set_new_handler(new_handler) _NOEXCEPT;
-_LIBCPP_EXPORTED_FROM_ABI new_handler get_new_handler() _NOEXCEPT;
-
 #elif defined(_HAS_EXCEPTIONS) && _HAS_EXCEPTIONS == 0 // !_LIBCPP_ABI_VCRUNTIME
 
 // When _HAS_EXCEPTIONS == 0, these complete definitions are needed,
 // since they would normally be provided in vcruntime_exception.h
 class bad_alloc : public exception {
 public:
   bad_alloc() noexcept : exception("bad allocation") {}
 
 private:
   friend class bad_array_new_length;
 
   bad_alloc(char const* const __message) noexcept : exception(__message) {}
 };
 
 class bad_array_new_length : public bad_alloc {
 public:
   bad_array_new_length() noexcept : bad_alloc("bad array new length") {}
 };
 #endif // defined(_LIBCPP_ABI_VCRUNTIME) && defined(_HAS_EXCEPTIONS) && _HAS_EXCEPTIONS == 0
 
+#if defined(_LIBCPP_ABI_VCRUNTIME)
+#define _NEW_HANDLER_EXPORTED
+#else
+#define _NEW_HANDLER_EXPORTED _LIBCPP_EXPORTED_FROM_ABI
+#endif
+
+typedef void (*new_handler)();
+_NEW_HANDLER_EXPORTED new_handler set_new_handler(new_handler) _NOEXCEPT;
+_NEW_HANDLER_EXPORTED new_handler get_new_handler() _NOEXCEPT;
+
 _LIBCPP_NORETURN _LIBCPP_EXPORTED_FROM_ABI void __throw_bad_alloc();  // not in C++ spec
 
 _LIBCPP_NORETURN inline _LIBCPP_INLINE_VISIBILITY
 void __throw_bad_array_new_length()
 {
 #ifndef _LIBCPP_HAS_NO_EXCEPTIONS
     throw bad_array_new_length();
 #else

libcxx/src/new_handler.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
+#include <mutex>
 #include <new>
 
 #include "include/atomic_support.h"
 
 #if defined(_LIBCPP_ABI_MICROSOFT)
-#  if !defined(_LIBCPP_ABI_VCRUNTIME)
-#    define _LIBPCPP_DEFINE_NEW_HANDLER
-#  endif
+#  define _LIBPCPP_DEFINE_NEW_HANDLER
 #elif defined(LIBCXX_BUILDING_LIBCXXABI)
 // nothing to do, we use the one from libc++abi
 #elif defined(LIBCXXRT)
 #  define _LIBPCPP_DEFINE_NEW_HANDLER
 #elif defined(__GLIBCXX__)
 // nothing to do, we use the one from libstdc++/libsupc++
 #else
 #  define _LIBPCPP_DEFINE_NEW_HANDLER
 #endif
 
 #if defined(_LIBPCPP_DEFINE_NEW_HANDLER)
 
+#  if defined(_LIBCPP_ABI_MICROSOFT)
+
+using _new_handler = int(__cdecl*)(size_t);
+extern "C" _new_handler __cdecl _set_new_handler(_new_handler);
+
+#  endif // _LIBCPP_ABI_MICROSOFT
+
 namespace std { // purposefully not versioned
 
+#  if defined(_LIBCPP_ABI_MICROSOFT)
+
+static constinit new_handler __new_handler = nullptr;
+static mutex __new_handler_lock;
+
+int __cdecl _new_handler_interface(size_t) {
+  __new_handler();
+  return 1;
+}
+
+new_handler set_new_handler(new_handler handler) noexcept {
+  lock_guard<mutex> guard(__new_handler_lock);
+  new_handler old = __new_handler;
+  __new_handler   = handler;
+  _set_new_handler(handler ? _new_handler_interface : nullptr);
+  return old;
+}
+
+new_handler get_new_handler() noexcept {
+  lock_guard<mutex> guard(__new_handler_lock);
+  return __new_handler;
+}
+
+#  else // _LIBCPP_ABI_MICROSOFT
+
 static constinit std::new_handler __new_handler = nullptr;
 
 new_handler set_new_handler(new_handler handler) noexcept { return __libcpp_atomic_exchange(&__new_handler, handler); }
 
 new_handler get_new_handler() noexcept { return __libcpp_atomic_load(&__new_handler); }
 
+#  endif // _LIBCPP_ABI_MICROSOFT
+
 } // namespace std
 
 #endif // _LIBPCPP_DEFINE_NEW_HANDLER

libcxx/test/std/language.support/support.dynamic/alloc.errors/set.new.handler/get_new_handler.pass.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 // test get_new_handler
 
 // FIXME: When libc++ is linked against vcruntime (i.e. the default config in
 // MSVC mode), the declarations of std::set_new_handler and std::get_new_handler
 // are provided by vcruntime/UCRT's new.h. However, that header actually only
 // declares set_new_handler - it's missing a declaration of get_new_handler.
 
-// XFAIL: msvc && stdlib=libc++
-
 #include <new>
 #include <cassert>
 
 #include "test_macros.h"
 
 void f1() {}
 void f2() {}