[libc++] Simplify when the sized global deallocations overloads are available

[philnik777]

There doesn't seem to be much benefit in always providing declarations for the sized deallocations from C++14 onwards if the user explicitly passed -fno-sized-deallocation to disable them. This patch simplifies the declarations to be available exactly when the compiler expects sized deallocation functions to be available.

[llvmbot]

@llvm/pr-subscribers-libcxxabi

@llvm/pr-subscribers-libcxx

Author: Nikolas Klauser (philnik777)

Changes

There doesn't seem to be much benefit in always providing declarations for the sized deallocations from C++14 onwards if the user explicitly passed -fno-sized-deallocation to disable them. This patch simplifies the declarations to be available exactly when the compiler expects sized deallocation functions to be available.

---

Full diff: https://github.com/llvm/llvm-project/pull/114667.diff

1 Files Affected:

• (modified) libcxx/include/new (+4-16)

diff --git a/libcxx/include/new b/libcxx/include/new
index 290ad9e97f8ded..c7678b64c7d494 100644
--- a/libcxx/include/new
+++ b/libcxx/include/new
@@ -104,18 +104,6 @@ void  operator delete[](void* ptr, void*) noexcept;
 #endif
 
 #if defined(__cpp_sized_deallocation) && __cpp_sized_deallocation >= 201309L
-#  define _LIBCPP_HAS_LANGUAGE_SIZED_DEALLOCATION 1
-#else
-#  define _LIBCPP_HAS_LANGUAGE_SIZED_DEALLOCATION 0
-#endif
-
-#if _LIBCPP_STD_VER >= 14 || _LIBCPP_HAS_LANGUAGE_SIZED_DEALLOCATION
-#  define _LIBCPP_HAS_LIBRARY_SIZED_DEALLOCATION 1
-#else
-#  define _LIBCPP_HAS_LIBRARY_SIZED_DEALLOCATION 0
-#endif
-
-#if _LIBCPP_HAS_LIBRARY_SIZED_DEALLOCATION && _LIBCPP_HAS_LANGUAGE_SIZED_DEALLOCATION
 #  define _LIBCPP_HAS_SIZED_DEALLOCATION 1
 #else
 #  define _LIBCPP_HAS_SIZED_DEALLOCATION 0
@@ -214,7 +202,7 @@ inline constexpr destroying_delete_t destroying_delete{};
     _LIBCPP_NOALIAS;
 _LIBCPP_OVERRIDABLE_FUNC_VIS void operator delete(void* __p) _NOEXCEPT;
 _LIBCPP_OVERRIDABLE_FUNC_VIS void operator delete(void* __p, const std::nothrow_t&) _NOEXCEPT;
-#  if _LIBCPP_HAS_LIBRARY_SIZED_DEALLOCATION
+#  if _LIBCPP_HAS_SIZED_DEALLOCATION
 _LIBCPP_OVERRIDABLE_FUNC_VIS void operator delete(void* __p, std::size_t __sz) _NOEXCEPT;
 #  endif
 
@@ -223,7 +211,7 @@ _LIBCPP_OVERRIDABLE_FUNC_VIS void operator delete(void* __p, std::size_t __sz) _
     _LIBCPP_NOALIAS;
 _LIBCPP_OVERRIDABLE_FUNC_VIS void operator delete[](void* __p) _NOEXCEPT;
 _LIBCPP_OVERRIDABLE_FUNC_VIS void operator delete[](void* __p, const std::nothrow_t&) _NOEXCEPT;
-#  if _LIBCPP_HAS_LIBRARY_SIZED_DEALLOCATION
+#  if _LIBCPP_HAS_SIZED_DEALLOCATION
 _LIBCPP_OVERRIDABLE_FUNC_VIS void operator delete[](void* __p, std::size_t __sz) _NOEXCEPT;
 #  endif
 
@@ -233,7 +221,7 @@ _LIBCPP_OVERRIDABLE_FUNC_VIS void operator delete[](void* __p, std::size_t __sz)
 operator new(std::size_t __sz, std::align_val_t, const std::nothrow_t&) _NOEXCEPT _LIBCPP_NOALIAS;
 _LIBCPP_OVERRIDABLE_FUNC_VIS void operator delete(void* __p, std::align_val_t) _NOEXCEPT;
 _LIBCPP_OVERRIDABLE_FUNC_VIS void operator delete(void* __p, std::align_val_t, const std::nothrow_t&) _NOEXCEPT;
-#    if _LIBCPP_HAS_LIBRARY_SIZED_DEALLOCATION
+#    if _LIBCPP_HAS_SIZED_DEALLOCATION
 _LIBCPP_OVERRIDABLE_FUNC_VIS void operator delete(void* __p, std::size_t __sz, std::align_val_t) _NOEXCEPT;
 #    endif
 
@@ -243,7 +231,7 @@ operator new[](std::size_t __sz, std::align_val_t) _THROW_BAD_ALLOC;
 operator new[](std::size_t __sz, std::align_val_t, const std::nothrow_t&) _NOEXCEPT _LIBCPP_NOALIAS;
 _LIBCPP_OVERRIDABLE_FUNC_VIS void operator delete[](void* __p, std::align_val_t) _NOEXCEPT;
 _LIBCPP_OVERRIDABLE_FUNC_VIS void operator delete[](void* __p, std::align_val_t, const std::nothrow_t&) _NOEXCEPT;
-#    if _LIBCPP_HAS_LIBRARY_SIZED_DEALLOCATION
+#    if _LIBCPP_HAS_SIZED_DEALLOCATION
 _LIBCPP_OVERRIDABLE_FUNC_VIS void operator delete[](void* __p, std::size_t __sz, std::align_val_t) _NOEXCEPT;
 #    endif
 #  endif

[ldionne]

This makes sense to me, and I think the same should apply to aligned allocation as well (we should do that separately).

However, I'd like to get @EricWF 's thoughts on this since he was the one to introduce this 2-level dance for aligned allocation if I remember correctly.

Also, some CI failures are not flukes and should be addressed.

[ldionne]

LGTM w/ the CI fixed. I think the CI issue is precisely what I wanted Eric's input on.

[ahatanak]

This change breaks the following code if -fno-sized-deallocation is passed, which was previously working.

#include <new>

void foo(void *p) {
  // user wants sized deallocation here.
  operator delete(p, 100);
}

void bar(double *d) {
  // but doesn't want sized deallocation anywhere else.
  delete d;
}

Just wondering whether -fno-sized-deallocation is supposed to disable sized deallocation altogether or just instruct the compiler not to emit calls to sized deallocation functions.

[philnik777]

@ahatanak Why would you ever want to not let the compiler call sized deallocation functions when you know they are available?

[ahatanak]

It could affect code size because the size argument has to be passed, although I'm not sure how much users care about that in practice.

But that was a convoluted example. In the actual case I encountered, the user was explicitly calling the sized deallocation function on a platform where sized deallocation is disabled by default.

[philnik777]

I guess that would be possible. OTOH if you use sized deallocation somewhere you'll probably have a custom allocator anyways to make use of the improved performance, in which case I'd be very surprised if the tiny size improvement would be worth the runtime cost. Anyways, we are aware that this scenario is broken by the patch, but didn't think anybody would care that much. The way to fix it is to simply pass -fsized-deallocation to let the compiler know the function is available, which doesn't seem like it should be that hard to do. If this was causing widespread breakage I think we would be open to a temporary escape hatch.

Unless there is a compelling example why this is a configuration you'd want and not simply an oversight by the programmer I don't think we want to support the configuration. Given that nobody complained so far I don't think there is one.

To answer your original question, we've treated it as "the compiler shouldn't emit calls" before the patch and changed it to "it's either not available so nobody can rely on it or the compiler should be free to emit calls to it" since it complicated the library and we couldn't find a compelling use-case. In the end I think we're more likely to find misconfigured setups than anything else.

Sorry for the rambling.

[ldionne]

Just to be clear, there is definitely breakage caused by enabling sized deallocation, and I believe that is why it took so long for Clang to make it the default. That's why -fsized-deallocation and -fno-sized-deallocation were introduced in the first place, as opposed to simply implementing the standard feature unconditionally in Clang.

We're hitting this because AppleClang does not enable sized deallocation by default (yet) due to deployment challenges, which means that on our platform the library went from providing operator delete(void*, size_t) in C++14/C++17/C++20/etc to not providing it ever, since __cpp_sized_deallocation is not defined.

That being said, I do think that supporting the case where the compiler doesn't implement sized deallocation but we still provide the declarations doesn't make sense for upstream, so we'll tackle that differently. I just wanted to provide additional context.

[EricWF]

So the reason why there were two switches (one for the compiler, one for the library) is because they're turning on different things.

The library flag enables a library feature.

The compiler flag tells the compiler "I'm targeting platforms where sized-delete is available, you're free to implicitly generate calls to it".

As others point out, the library feature can be used independently of the compiler configuration.

Telling the compiler to not generate calls to sized delete implicitly should not affect the feature-completeness of libc++.