[libc++] Fix noexcept behaviour of operator new helper functions #74337
Conversation
llvmbot
added
libc++
|
@llvm/pr-subscribers-libcxx @llvm/pr-subscribers-libcxxabi Author: None (azhan92) ChangesThis PR removes the > throw an exception of type bad_alloc or a class derived from bad_alloc as a behaviour of a Full diff: https://github.com/llvm/llvm-project/pull/74337.diff 3 Files Affected:
diff --git a/libcxx/src/new.cpp b/libcxx/src/new.cpp
index 033bba5c1fc95..cb8b4aae8d5f9 100644
--- a/libcxx/src/new.cpp
+++ b/libcxx/src/new.cpp
@@ -20,7 +20,7 @@
// in this shared library, so that they can be overridden by programs
// that define non-weak copies of the functions.
-static void* operator_new_impl(std::size_t size) noexcept {
+static void* operator_new_impl(std::size_t size) {
if (size == 0)
size = 1;
void* p;
@@ -87,7 +87,7 @@ _LIBCPP_WEAK void operator delete[](void* ptr, size_t) noexcept { ::operator del
# if !defined(_LIBCPP_HAS_NO_LIBRARY_ALIGNED_ALLOCATION)
-static void* operator_new_aligned_impl(std::size_t size, std::align_val_t alignment) noexcept {
+static void* operator_new_aligned_impl(std::size_t size, std::align_val_t alignment) {
if (size == 0)
size = 1;
if (static_cast<size_t>(alignment) < sizeof(void*))
diff --git a/libcxxabi/src/stdlib_new_delete.cpp b/libcxxabi/src/stdlib_new_delete.cpp
index 6c9990f063dde..f8a00ec584256 100644
--- a/libcxxabi/src/stdlib_new_delete.cpp
+++ b/libcxxabi/src/stdlib_new_delete.cpp
@@ -30,7 +30,7 @@
// in this shared library, so that they can be overridden by programs
// that define non-weak copies of the functions.
-static void* operator_new_impl(std::size_t size) noexcept {
+static void* operator_new_impl(std::size_t size) {
if (size == 0)
size = 1;
void* p;
@@ -107,7 +107,7 @@ void operator delete[](void* ptr, size_t) noexcept { ::operator delete[](ptr); }
#if !defined(_LIBCPP_HAS_NO_LIBRARY_ALIGNED_ALLOCATION)
-static void* operator_new_aligned_impl(std::size_t size, std::align_val_t alignment) noexcept {
+static void* operator_new_aligned_impl(std::size_t size, std::align_val_t alignment) {
if (size == 0)
size = 1;
if (static_cast<size_t>(alignment) < sizeof(void*))
diff --git a/libcxxabi/test/test_memory_alloc.pass.cpp b/libcxxabi/test/test_memory_alloc.pass.cpp
new file mode 100644
index 0000000000000..219b85b9afcaf
--- /dev/null
+++ b/libcxxabi/test/test_memory_alloc.pass.cpp
@@ -0,0 +1,34 @@
+//===----------------------------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: no-exceptions
+
+#include <new>
+#include <cassert>
+#include <limits>
+
+int new_handler_called = 0;
+
+void my_new_handler() {
+ ++new_handler_called;
+ throw std::bad_alloc();
+}
+
+int main(int, char**) {
+ std::set_new_handler(my_new_handler);
+ try {
+ void* x = operator new[](std::numeric_limits<std::size_t>::max());
+ (void)x;
+ assert(false);
+ } catch (std::bad_alloc const&) {
+ assert(new_handler_called == 1);
+ } catch (...) {
+ assert(false);
+ }
+ return 0;
+}
|
| @@ -0,0 +1,34 @@ | |||
| //===----------------------------------------------------------------------===// | |||
There was a problem hiding this comment.
We have a bunch of tests located in e.g. libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new.size.pass.cpp. Those are testing the various version of operator new we have:
void* operator new(std::size_t);
void* operator new[](std::size_t);
void* operator new(std::size_t, std::align_val_t);
void* operator new[](std::size_t, std::align_val_t);You'll see each overload has its own test file. Can you add one test for each such overload? Note that we also have std::nothrow_t overloads of operator new but I don't think it makes sense to test that the new_handler throws bad_alloc for those, since the only thing it would do is terminate.
There was a problem hiding this comment.
You'll see each overload has its own test file. Can you add one test for each such overload? Note that we also have
std::nothrow_toverloads ofoperator newbut I don't think it makes sense to test that thenew_handlerthrowsbad_allocfor those, since the only thing it would do is terminate.
@ldionne @azhan92: If I read the following correctly, testing the std::nothrow_t overloads is meaningful (to ensure that the exception is caught by the default implementation):
Default behavior: Calls operator new(size), or operator new(size, alignment), respectively. If
the call returns normally, returns the result of that call. Otherwise, returns a null pointer.
| void* x = operator new[](std::numeric_limits<std::size_t>::max()); | ||
| (void)x; | ||
| assert(false); | ||
| } catch (std::bad_alloc const&) { |
There was a problem hiding this comment.
I believe it would be prudent to throw (and catch), within the test, an exception of a class type (defined as part of the test itself) derived from std::bad_alloc. If extra paranoid, the class type should also record the value of the this pointer on construction using a "tag" class type and have a copy constructor that abort()s:
struct my_bad_alloc : std::bad_alloc {
my_bad_alloc(const my_bad_alloc &) { abort(); }
my_bad_alloc(construction_key) self(this) {}
const my_bad_alloc *const self;
};The recorded pointer value can then be checked against the address of the caught exception object in the catch block.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
| //===----------------------------------------------------------------------===// | ||
|
|
||
| // UNSUPPORTED: no-exceptions | ||
| #include <new> |
There was a problem hiding this comment.
Nitpick, please add a newline between the UNSUPPORTED and the #include <new>. Here and in the other tests too.
| @@ -0,0 +1,42 @@ | |||
| //===----------------------------------------------------------------------===// | |||
There was a problem hiding this comment.
Let's move these tests under libcxx/test/std/language.support/support.dynamic/new.delete. They test the general conformance of operator new, so they should be in the libc++ conformance suite instead. Please also look at e.g. libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single & friends and see if it would make sense to add these tests to existing files instead, guarded by #ifndef TEST_HAS_NO_EXCEPTIONS.
|
Also, the latest CI runs shows that your tests seem to be failing under the sanitizers. IDK if that shows a problem with your test or if it's unavoidable, so it would have to be investigated a bit. |
I noticed that |
|
Almost certainly! Moving the tests like I requested would hence also have the side effect of fixing this. |
Thank you! I looked at the ones found in |
|
@azhan92 Can you please uncheck the "keep my email private" checkbox from the Github preferences? Otherwise, this will get committed with an anonymous email address and we're trying to steer away from that. |
Yes, done! |
This patch removes the noexcept specifier introduced in #69407 since the
Standard allows a new handler to throw an exception of type bad_alloc
(or derived from it). With the noexcept specifier on the helper functions,
we would immediately terminate the program.
The patch also adds tests for the case that had regressed.