[libc++] Pass type information down to __libcpp_allocate

[ldionne]

Currently, places where we call __libcpp_allocate must drop type information on the ground even when they actually have such information available. That is unfortunate since some toolchains and system allocators are able to provide improved security when they know what type is being allocated.

This is the purpose of http://wg21.link/p2719, where we introduce a new variant of operator new which takes a type in its interface. A different but related issue is that std::allocator does not honor any in-class T::operator new since it is specified to call the global ::operator new instead.

This patch closes the gap to make it trivial for implementations that provide typed memory allocators to actually benefit from that information in more contexts, and also makes libc++ forward-compatible with future proposals that would fix the existing defects in std::allocator. It also makes the internal allocation API higher level by operating on objects instead of operating on bytes of memory.

Since this is a widely-used function and making this a template could have an impact on debug info sizes, I tried minimizing the number of templated layers by removing __do_deallocate_handle_size, which was easy to replace with a macro (and IMO this leads to cleaner code).

We could also explore using _LIBCPP_NODEBUG on __libcpp_allocate and friends if that proves to be a problem.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Louis Dionne (ldionne)

Changes

Currently, places where we call __libcpp_allocate must drop type information on the ground even when they actually have such information available. That is unfortunate since some toolchains and system allocators are able to provide improved security when they know what type is being allocated.

This is the purpose of http://wg21.link/p2719, where we introduce a new variant of operator new which takes a type in its interface. A different but related issue is that std::allocator does not honor any in-class T::operator new since it is specified to call the global ::operator new instead.

This patch closes the gap to make it trivial for implementations that provide typed memory allocators to actually benefit from that information in more contexts, and also makes libc++ forward-compatible with future proposals that would fix the existing defects in std::allocator.

Since this is a widely-used function and making this a template could have an impact on debug info sizes, I tried minimizing the number of templated layers by removing __do_deallocate_handle_size, which was easy to replace with a macro (and IMO this leads to cleaner code).

We could also explore using _LIBCPP_NODEBUG on __libcpp_allocate and friends if that proves to be a problem.

---

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

7 Files Affected:

• (modified) libcxx/include/__memory/allocator.h (+2-2)
• (modified) libcxx/include/__memory/builtin_new_allocator.h (+10-5)
• (modified) libcxx/include/__memory/unique_temporary_buffer.h (+1-1)
• (modified) libcxx/include/__utility/small_buffer.h (+2-2)
• (modified) libcxx/include/new (+17-17)
• (modified) libcxx/src/memory_resource.cpp (+6-4)
• (modified) libcxx/test/libcxx/language.support/support.dynamic/libcpp_deallocate.sh.cpp (+18-19)

diff --git a/libcxx/include/__memory/allocator.h b/libcxx/include/__memory/allocator.h
index ddb4179940b8b1..3c8f6dd616b1d4 100644
--- a/libcxx/include/__memory/allocator.h
+++ b/libcxx/include/__memory/allocator.h
@@ -100,7 +100,7 @@ class _LIBCPP_TEMPLATE_VIS allocator : private __non_trivial_if<!is_void<_Tp>::v
     if (__libcpp_is_constant_evaluated()) {
       return static_cast<_Tp*>(::operator new(__n * sizeof(_Tp)));
     } else {
-      return static_cast<_Tp*>(std::__libcpp_allocate(__n * sizeof(_Tp), _LIBCPP_ALIGNOF(_Tp)));
+      return static_cast<_Tp*>(std::__libcpp_allocate<_Tp>(__n * sizeof(_Tp), _LIBCPP_ALIGNOF(_Tp)));
     }
   }
 
@@ -115,7 +115,7 @@ class _LIBCPP_TEMPLATE_VIS allocator : private __non_trivial_if<!is_void<_Tp>::v
     if (__libcpp_is_constant_evaluated()) {
       ::operator delete(__p);
     } else {
-      std::__libcpp_deallocate((void*)__p, __n * sizeof(_Tp), _LIBCPP_ALIGNOF(_Tp));
+      std::__libcpp_deallocate<_Tp>((void*)__p, __n * sizeof(_Tp), _LIBCPP_ALIGNOF(_Tp));
     }
   }
 
diff --git a/libcxx/include/__memory/builtin_new_allocator.h b/libcxx/include/__memory/builtin_new_allocator.h
index 128288efb05bc1..0b4a024fc39184 100644
--- a/libcxx/include/__memory/builtin_new_allocator.h
+++ b/libcxx/include/__memory/builtin_new_allocator.h
@@ -23,6 +23,9 @@ _LIBCPP_BEGIN_NAMESPACE_STD
 // deallocating memory using __builtin_operator_new and
 // __builtin_operator_delete. It should be used in preference to
 // `std::allocator<T>` to avoid additional instantiations.
+//
+// TODO: Get rid of this class since it's only used in std::function and
+//       we use __allocate_type there anyway.
 struct __builtin_new_allocator {
   struct __builtin_new_deleter {
     typedef void* pointer_type;
@@ -31,7 +34,7 @@ struct __builtin_new_allocator {
         : __size_(__size), __align_(__align) {}
 
     _LIBCPP_HIDE_FROM_ABI void operator()(void* __p) const _NOEXCEPT {
-      std::__libcpp_deallocate(__p, __size_, __align_);
+      std::__libcpp_deallocate<char>(__p, __size_, __align_);
     }
 
   private:
@@ -42,22 +45,24 @@ struct __builtin_new_allocator {
   typedef unique_ptr<void, __builtin_new_deleter> __holder_t;
 
   _LIBCPP_HIDE_FROM_ABI static __holder_t __allocate_bytes(size_t __s, size_t __align) {
-    return __holder_t(std::__libcpp_allocate(__s, __align), __builtin_new_deleter(__s, __align));
+    return __holder_t(std::__libcpp_allocate<char>(__s, __align), __builtin_new_deleter(__s, __align));
   }
 
   _LIBCPP_HIDE_FROM_ABI static void __deallocate_bytes(void* __p, size_t __s, size_t __align) _NOEXCEPT {
-    std::__libcpp_deallocate(__p, __s, __align);
+    std::__libcpp_deallocate<char>(__p, __s, __align);
   }
 
   template <class _Tp>
   _LIBCPP_NODEBUG _LIBCPP_ALWAYS_INLINE _LIBCPP_HIDE_FROM_ABI static __holder_t __allocate_type(size_t __n) {
-    return __allocate_bytes(__n * sizeof(_Tp), _LIBCPP_ALIGNOF(_Tp));
+    auto const __size  = __n * sizeof(_Tp);
+    auto const __align = _LIBCPP_ALIGNOF(_Tp);
+    return __holder_t(std::__libcpp_allocate<_Tp>(__size, __align), __builtin_new_deleter(__size, __align));
   }
 
   template <class _Tp>
   _LIBCPP_NODEBUG _LIBCPP_ALWAYS_INLINE _LIBCPP_HIDE_FROM_ABI static void
   __deallocate_type(void* __p, size_t __n) _NOEXCEPT {
-    __deallocate_bytes(__p, __n * sizeof(_Tp), _LIBCPP_ALIGNOF(_Tp));
+    std::__libcpp_deallocate<_Tp>(__p, __n * sizeof(_Tp), _LIBCPP_ALIGNOF(_Tp));
   }
 };
 
diff --git a/libcxx/include/__memory/unique_temporary_buffer.h b/libcxx/include/__memory/unique_temporary_buffer.h
index 4f47c84e2f8dbe..a9e5083da8016d 100644
--- a/libcxx/include/__memory/unique_temporary_buffer.h
+++ b/libcxx/include/__memory/unique_temporary_buffer.h
@@ -39,7 +39,7 @@ struct __temporary_buffer_deleter {
       return;
     }
 
-    std::__libcpp_deallocate_unsized((void*)__ptr, _LIBCPP_ALIGNOF(_Tp));
+    std::__libcpp_deallocate_unsized<_Tp>((void*)__ptr, _LIBCPP_ALIGNOF(_Tp));
   }
 };
 
diff --git a/libcxx/include/__utility/small_buffer.h b/libcxx/include/__utility/small_buffer.h
index b44b37e90e7653..cf5fd67605f742 100644
--- a/libcxx/include/__utility/small_buffer.h
+++ b/libcxx/include/__utility/small_buffer.h
@@ -66,7 +66,7 @@ class __small_buffer {
     if constexpr (__fits_in_buffer<_Stored>) {
       return std::launder(reinterpret_cast<_Stored*>(__buffer_));
     } else {
-      byte* __allocation = static_cast<byte*>(std::__libcpp_allocate(sizeof(_Stored), alignof(_Stored)));
+      byte* __allocation = static_cast<byte*>(std::__libcpp_allocate<_Stored>(sizeof(_Stored), alignof(_Stored)));
       std::construct_at(reinterpret_cast<byte**>(__buffer_), __allocation);
       return std::launder(reinterpret_cast<_Stored*>(__allocation));
     }
@@ -75,7 +75,7 @@ class __small_buffer {
   template <class _Stored>
   _LIBCPP_HIDE_FROM_ABI void __dealloc() noexcept {
     if constexpr (!__fits_in_buffer<_Stored>)
-      std::__libcpp_deallocate(*reinterpret_cast<void**>(__buffer_), sizeof(_Stored), alignof(_Stored));
+      std::__libcpp_deallocate<_Stored>(*reinterpret_cast<void**>(__buffer_), sizeof(_Stored), alignof(_Stored));
   }
 
   template <class _Stored, class... _Args>
diff --git a/libcxx/include/new b/libcxx/include/new
index 49e500809e158f..ff215dcd9f80b7 100644
--- a/libcxx/include/new
+++ b/libcxx/include/new
@@ -271,7 +271,7 @@ _LIBCPP_CONSTEXPR inline _LIBCPP_HIDE_FROM_ABI bool __is_overaligned_for_new(siz
 #endif
 }
 
-template <class... _Args>
+template <class _Tp, class... _Args>
 _LIBCPP_HIDE_FROM_ABI void* __libcpp_operator_new(_Args... __args) {
 #if __has_builtin(__builtin_operator_new) && __has_builtin(__builtin_operator_delete)
   return __builtin_operator_new(__args...);
@@ -280,7 +280,7 @@ _LIBCPP_HIDE_FROM_ABI void* __libcpp_operator_new(_Args... __args) {
 #endif
 }
 
-template <class... _Args>
+template <class _Tp, class... _Args>
 _LIBCPP_HIDE_FROM_ABI void __libcpp_operator_delete(_Args... __args) _NOEXCEPT {
 #if __has_builtin(__builtin_operator_new) && __has_builtin(__builtin_operator_delete)
   __builtin_operator_delete(__args...);
@@ -289,52 +289,52 @@ _LIBCPP_HIDE_FROM_ABI void __libcpp_operator_delete(_Args... __args) _NOEXCEPT {
 #endif
 }
 
+template <class _Tp>
 inline _LIBCPP_HIDE_FROM_ABI void* __libcpp_allocate(size_t __size, size_t __align) {
 #if _LIBCPP_HAS_ALIGNED_ALLOCATION
   if (__is_overaligned_for_new(__align)) {
     const align_val_t __align_val = static_cast<align_val_t>(__align);
-    return __libcpp_operator_new(__size, __align_val);
+    return std::__libcpp_operator_new<_Tp>(__size, __align_val);
   }
 #endif
 
   (void)__align;
-  return __libcpp_operator_new(__size);
+  return std::__libcpp_operator_new<_Tp>(__size);
 }
 
-template <class... _Args>
-_LIBCPP_HIDE_FROM_ABI void __do_deallocate_handle_size(void* __ptr, size_t __size, _Args... __args) _NOEXCEPT {
-#if !_LIBCPP_HAS_SIZED_DEALLOCATION
-  (void)__size;
-  return std::__libcpp_operator_delete(__ptr, __args...);
+#if _LIBCPP_HAS_SIZED_DEALLOCATION
+#  define _LIBCPP_ONLY_IF_SIZED_DEALLOCATION(...) __VA_ARGS__
 #else
-  return std::__libcpp_operator_delete(__ptr, __size, __args...);
+#  define _LIBCPP_ONLY_IF_SIZED_DEALLOCATION(...) /* nothing */
 #endif
-}
 
+template <class _Tp>
 inline _LIBCPP_HIDE_FROM_ABI void __libcpp_deallocate(void* __ptr, size_t __size, size_t __align) _NOEXCEPT {
+  (void)__size;
 #if !_LIBCPP_HAS_ALIGNED_ALLOCATION
   (void)__align;
-  return __do_deallocate_handle_size(__ptr, __size);
+  return std::__libcpp_operator_delete<_Tp>(__ptr _LIBCPP_ONLY_IF_SIZED_DEALLOCATION(, __size));
 #else
   if (__is_overaligned_for_new(__align)) {
     const align_val_t __align_val = static_cast<align_val_t>(__align);
-    return __do_deallocate_handle_size(__ptr, __size, __align_val);
+    return std::__libcpp_operator_delete<_Tp>(__ptr _LIBCPP_ONLY_IF_SIZED_DEALLOCATION(, __size), __align_val);
   } else {
-    return __do_deallocate_handle_size(__ptr, __size);
+    return std::__libcpp_operator_delete<_Tp>(__ptr _LIBCPP_ONLY_IF_SIZED_DEALLOCATION(, __size));
   }
 #endif
 }
 
+template <class _Tp>
 inline _LIBCPP_HIDE_FROM_ABI void __libcpp_deallocate_unsized(void* __ptr, size_t __align) _NOEXCEPT {
 #if !_LIBCPP_HAS_ALIGNED_ALLOCATION
   (void)__align;
-  return __libcpp_operator_delete(__ptr);
+  return std::__libcpp_operator_delete<_Tp>(__ptr);
 #else
   if (__is_overaligned_for_new(__align)) {
     const align_val_t __align_val = static_cast<align_val_t>(__align);
-    return __libcpp_operator_delete(__ptr, __align_val);
+    return std::__libcpp_operator_delete<_Tp>(__ptr, __align_val);
   } else {
-    return __libcpp_operator_delete(__ptr);
+    return std::__libcpp_operator_delete<_Tp>(__ptr);
   }
 #endif
 }
diff --git a/libcxx/src/memory_resource.cpp b/libcxx/src/memory_resource.cpp
index 0cd575e995c0ff..d01f7b611c7ca4 100644
--- a/libcxx/src/memory_resource.cpp
+++ b/libcxx/src/memory_resource.cpp
@@ -41,20 +41,22 @@ static bool is_aligned_to(void* ptr, size_t align) {
 class _LIBCPP_EXPORTED_FROM_ABI __new_delete_memory_resource_imp : public memory_resource {
   void* do_allocate(size_t bytes, size_t align) override {
 #if _LIBCPP_HAS_ALIGNED_ALLOCATION
-    return std::__libcpp_allocate(bytes, align);
+    return std::__libcpp_allocate<std::byte>(bytes, align);
 #else
     if (bytes == 0)
       bytes = 1;
-    void* result = std::__libcpp_allocate(bytes, align);
+    void* result = std::__libcpp_allocate<std::byte>(bytes, align);
     if (!is_aligned_to(result, align)) {
-      std::__libcpp_deallocate(result, bytes, align);
+      std::__libcpp_deallocate<std::byte>(result, bytes, align);
       __throw_bad_alloc();
     }
     return result;
 #endif
   }
 
-  void do_deallocate(void* p, size_t bytes, size_t align) override { std::__libcpp_deallocate(p, bytes, align); }
+  void do_deallocate(void* p, size_t bytes, size_t align) override {
+    std::__libcpp_deallocate<std::byte>(p, bytes, align);
+  }
 
   bool do_is_equal(const memory_resource& other) const noexcept override { return &other == this; }
 };
diff --git a/libcxx/test/libcxx/language.support/support.dynamic/libcpp_deallocate.sh.cpp b/libcxx/test/libcxx/language.support/support.dynamic/libcpp_deallocate.sh.cpp
index 37e3f8167051aa..2ce153c6f5883a 100644
--- a/libcxx/test/libcxx/language.support/support.dynamic/libcpp_deallocate.sh.cpp
+++ b/libcxx/test/libcxx/language.support/support.dynamic/libcpp_deallocate.sh.cpp
@@ -94,34 +94,34 @@ struct alloc_stats {
 };
 alloc_stats stats;
 
-void operator delete(void* p)TEST_NOEXCEPT {
+void operator delete(void* p) TEST_NOEXCEPT {
   ::free(p);
   stats.plain_called++;
   stats.last_size = stats.last_align = -1;
 }
 
 #ifndef NO_SIZE
-void operator delete(void* p, std::size_t n)TEST_NOEXCEPT {
+void operator delete(void* p, std::size_t n) TEST_NOEXCEPT {
   ::free(p);
   stats.sized_called++;
-  stats.last_size = n;
+  stats.last_size  = n;
   stats.last_align = -1;
 }
 #endif
 
 #ifndef NO_ALIGN
-void operator delete(void* p, std::align_val_t a)TEST_NOEXCEPT {
+void operator delete(void* p, std::align_val_t a) TEST_NOEXCEPT {
   std::__libcpp_aligned_free(p);
   stats.aligned_called++;
   stats.last_align = static_cast<int>(a);
-  stats.last_size = -1;
+  stats.last_size  = -1;
 }
 
-void operator delete(void* p, std::size_t n, std::align_val_t a)TEST_NOEXCEPT {
+void operator delete(void* p, std::size_t n, std::align_val_t a) TEST_NOEXCEPT {
   std::__libcpp_aligned_free(p);
   stats.aligned_sized_called++;
   stats.last_align = static_cast<int>(a);
-  stats.last_size = n;
+  stats.last_size  = n;
 }
 #endif
 
@@ -133,45 +133,45 @@ void test_libcpp_dealloc() {
   std::size_t over_align_val = TEST_ALIGNOF(std::max_align_t) * 2;
 #endif
   std::size_t under_align_val = TEST_ALIGNOF(int);
-  std::size_t with_size_val = 2;
+  std::size_t with_size_val   = 2;
 
   {
-    std::__libcpp_deallocate_unsized(p, under_align_val);
+    std::__libcpp_deallocate_unsized<char>(p, under_align_val);
     assert(stats.expect_plain());
   }
   stats.reset();
 
 #if defined(NO_SIZE) && defined(NO_ALIGN)
   {
-    std::__libcpp_deallocate(p, with_size_val, over_align_val);
+    std::__libcpp_deallocate<char>(p, with_size_val, over_align_val);
     assert(stats.expect_plain());
   }
   stats.reset();
 #elif defined(NO_SIZE)
   {
-    std::__libcpp_deallocate(p, with_size_val, over_align_val);
+    std::__libcpp_deallocate<char>(p, with_size_val, over_align_val);
     assert(stats.expect_align(over_align_val));
   }
   stats.reset();
 #elif defined(NO_ALIGN)
   {
-    std::__libcpp_deallocate(p, with_size_val, over_align_val);
+    std::__libcpp_deallocate<char>(p, with_size_val, over_align_val);
     assert(stats.expect_size(with_size_val));
   }
   stats.reset();
 #else
   {
-    std::__libcpp_deallocate(p, with_size_val, over_align_val);
+    std::__libcpp_deallocate<char>(p, with_size_val, over_align_val);
     assert(stats.expect_size_align(with_size_val, over_align_val));
   }
   stats.reset();
   {
-    std::__libcpp_deallocate_unsized(p, over_align_val);
+    std::__libcpp_deallocate_unsized<char>(p, over_align_val);
     assert(stats.expect_align(over_align_val));
   }
   stats.reset();
   {
-    std::__libcpp_deallocate(p, with_size_val, under_align_val);
+    std::__libcpp_deallocate<char>(p, with_size_val, under_align_val);
     assert(stats.expect_size(with_size_val));
   }
   stats.reset();
@@ -200,13 +200,13 @@ void test_allocator_and_new_match() {
   stats.reset();
 #elif defined(NO_SIZE)
   stats.reset();
-#if TEST_STD_VER >= 11
+#  if TEST_STD_VER >= 11
   {
     int* x = DoNotOptimize(new int(42));
     delete x;
     assert(stats.expect_plain());
   }
-#endif
+#  endif
   stats.reset();
   {
     AlignedType* a = DoNotOptimize(new AlignedType());
@@ -239,8 +239,7 @@ void test_allocator_and_new_match() {
   {
     AlignedType* a = DoNotOptimize(new AlignedType());
     delete a;
-    assert(stats.expect_size_align(sizeof(AlignedType),
-                                   TEST_ALIGNOF(AlignedType)));
+    assert(stats.expect_size_align(sizeof(AlignedType), TEST_ALIGNOF(AlignedType)));
   }
   stats.reset();
 #endif

[zmodem]

We're hitting CFI errors for this cast (see https://crbug.com/392652315), which I suppose is expected since it's casting a pointer to uninitialized memory(?). Should __libcpp_allocate have a _LIBCPP_NO_CFI attribute?

[ldionne]

Probably. Thanks for the heads up. See #124805