[libc++] Provide sized deallocation declarations even when the compiler doesn't support sized deallocation

[ldionne]

After ef804d8, we stopped providing the declaration of sized deallocation functions unless the compiler provides support for the language feature. In reality, we can still provide the declarations of global operator delete for users who want to call these operators directly without going through the compiler rewrite.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Louis Dionne (ldionne)

Changes

After ef804d8, we stopped providing the declaration of sized deallocation functions unless the compiler provides support for the language feature. In reality, we can still provide the declarations of global operator delete for users who want to call these operators directly without going through the compiler rewrite.

---

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

2 Files Affected:

• (modified) libcxx/include/__new/global_new_delete.h (+1-1)
• (added) libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/sized_delete.fno-sized-deallocation.pass.cpp (+27)

diff --git a/libcxx/include/__new/global_new_delete.h b/libcxx/include/__new/global_new_delete.h
index 96510ab56b00b5a..3d1f7b6f3d24090 100644
--- a/libcxx/include/__new/global_new_delete.h
+++ b/libcxx/include/__new/global_new_delete.h
@@ -25,7 +25,7 @@
 #  define _THROW_BAD_ALLOC
 #endif
 
-#if defined(__cpp_sized_deallocation) && __cpp_sized_deallocation >= 201309L
+#if _LIBCPP_STD_VER >= 14
 #  define _LIBCPP_HAS_SIZED_DEALLOCATION 1
 #else
 #  define _LIBCPP_HAS_SIZED_DEALLOCATION 0
diff --git a/libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/sized_delete.fno-sized-deallocation.pass.cpp b/libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/sized_delete.fno-sized-deallocation.pass.cpp
new file mode 100644
index 000000000000000..daa0374fd28125d
--- /dev/null
+++ b/libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/sized_delete.fno-sized-deallocation.pass.cpp
@@ -0,0 +1,27 @@
+//===----------------------------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+// Ensure that libc++ still provides the declaration of sized operator delete even
+// when sized deallocation support is disabled at the language level, since it should
+// still be valid to call these operators explicitly (as opposed to via a compiler
+// rewrite of a delete expression).
+
+// UNSUPPORTED: c++03, c++11
+
+// ADDITIONAL_COMPILE_FLAGS: -fno-sized-deallocation
+
+// Sized deallocation support was introduced in LLVM 11
+// XFAIL: using-built-library-before-llvm-11
+
+#include <new>
+
+int main(int, char**) {
+    void* p = ::operator new(10);
+    ::operator delete(p, 10);
+    return 0;
+}

[ldionne]

Another view point on this is that the library should not provide the declaration when the language feature is turned off, but I'm tempted to decouple the two.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[philnik777]

I don't think this is a good idea. If sized deallocation is disabled there tends to be a reason for that - most likely that they're not available. This just pushes the diagnostic to be a linker error instead most likely, which isn't great. So unless there is a good use-case for sized deallocation being disabled at the language level but should be provided by the library I don't think we should go forward with this.

[var-const]

@philnik777

This just pushes the diagnostic to be a linker error instead most likely, which isn't great.

What would the linker error look like?

[philnik777]

@philnik777

This just pushes the diagnostic to be a linker error instead most likely, which isn't great.

What would the linker error look like?

Probably something along the lines of "undefined symbol x referenced in y". Or, if it's linked dynamically, at program startup something similar.

[var-const]

@philnik777 I was unsuccessful trying to get one on Godbolt. The compiler simply ignores the user-provided sized overload when resolving a delete expression if sized allocations are not available -- but it's very possible I'm not trying the right combination of the compiler / flags / etc.

[philnik777]

@philnik777 I was unsuccessful trying to get one on Godbolt. The compiler simply ignores the user-provided sized overload when resolving a delete expression if sized allocations are not available -- but it's very possible I'm not trying the right combination of the compiler / flags / etc.

Yes, that's the point of the flag. You have to explicitly use the sized deallocation overload with this patch, but I don't see when you'd want to be able to manually call the function but not tell the compiler that it exists.

[var-const]

Yes, that's the point of the flag. You have to explicitly use the sized deallocation overload with this patch, but I don't see when you'd want to be able to manually call the function but not tell the compiler that it exists

Right, but that's a separate issue -- I'd like to first make sure this patch doesn't result in linker errors (in some contexts) before discussing whether we should do anything in the library to prevent silently ignoring user's overload.

[philnik777]

Thinking about it, we could add deleted versions of sized deallocation and tell the user that they should enable -fsized-deallocation if they provide their own overloads.

[philnik777]

Yes, that's the point of the flag. You have to explicitly use the sized deallocation overload with this patch, but I don't see when you'd want to be able to manually call the function but not tell the compiler that it exists

Right, but that's a separate issue -- I'd like to first make sure this patch doesn't result in linker errors (in some contexts) before discussing whether we should do anything in the library to prevent silently ignoring user's overload.

I don't think that's a separate issue. AFAICT this is the only scenario this patch allows. If you thought I meant this could lead to linker issues for code that doesn't manually call the sized overloads, I'm sorry for the confusion. I didn't mean that.

[ldionne]

The compiler's -fno-sized-deallocation flag allows controlling whether delete expressions should use sized delete or not, which is a breaking change for some code, since it changes which operator delete gets called without making any changes to the user code. AFAIU, that's the purpose of that flag and that's the reason why it wasn't made the default until recently (upstream).

Some users might be in a situation where they don't want their delete expressions to resolve differently, but where it is 100% acceptable to call the sized operator delete explicitly. In that case, they could pass -fno-sized-deallocation yet still call the library function explicitly (after this patch, but not before).

For availability issues, we have attributes on these declarations: if someone attempts to use a sized operator delete while compiling for a target that does not support the functionality, that should be caught at compile-time by these attributes.

[philnik777]

The compiler's -fno-sized-deallocation flag allows controlling whether delete expressions should use sized delete or not, which is a breaking change for some code, since it changes which operator delete gets called without making any changes to the user code. AFAIU, that's the purpose of that flag and that's the reason why it wasn't made the default until recently (upstream).

I don't think that's the problem. The sized overload just calls the unsized one by default, so I don't see how that's breaking. My understanding is that the fundamental is that the compiler silently changes to generating calls to functions which may not exist in the runtime yet. That could probably have been solved by using availability information, but most platforms don't have that.

Some users might be in a situation where they don't want their delete expressions to resolve differently, but where it is 100% acceptable to call the sized operator delete explicitly. In that case, they could pass -fno-sized-deallocation yet still call the library function explicitly (after this patch, but not before).

In what scenario is that the case? Even if I grant that it's a breaking change because allocation replacements may not be aware of the new overload, this can only result in an allocator/deallocator mismatch at that point, which is always a bad idea.

For availability issues, we have attributes on these declarations: if someone attempts to use a sized operator delete while compiling for a target that does not support the functionality, that should be caught at compile-time by these attributes.

Do you mean we'd have them if there are back-deployment problems? Because I can't find any.

[ldionne]

I don't think that's the problem. The sized overload just calls the unsized one by default, so I don't see how that's breaking. My understanding is that the fundamental is that the compiler silently changes to generating calls to functions which may not exist in the runtime yet. That could probably have been solved by using availability information, but most platforms don't have that.

I think you're correct about this. I did some more archeology and that (missing symbols in most existing libraries) seems to be the reason why -fsized-deallocation wasn't made default for so long. And since the sized operator delete does call the normal operator delete, you're also correct that there shouldn't be a backwards compatibility issue here. The situation we're running into downstream probably calls for a different solution.

Do you mean we'd have them if there are back-deployment problems? Because I can't find any.

Ah, you're right here too, I wrongly assumed there were availability attributes. That looks like an oversight, since availability attributes are definitely the way we'd normally catch these kinds of issues.

Either way, I think I'm convinced that the current code is the way it should be, so I'll close this. Thanks for the discussion. We will pursue another direction for the downstream issues we saw related to this.