[libc++] Fix bug in allocate_shared_for_overwrite

Instead of destroying the object with allocator::destroy, we must
call its destructor directly. As a fly-by also mark LWG3008 as
fixed since it is handled by our implementation.

This was pointed out by Tim Song in https://reviews.llvm.org/D140913.

Differential Revision: https://reviews.llvm.org/D143791

(cherry picked from commit 5801090258011cfe636cda1493ac9bc07fb2a889)

5 files changed, 123 insertions, 57 deletions

diff --git a/libcxx/docs/Status/Cxx20Issues.csv b/libcxx/docs/Status/Cxx20Issues.csv
index db0546147f1a..2d2318af228d 100644
--- a/libcxx/docs/Status/Cxx20Issues.csv
+++ b/libcxx/docs/Status/Cxx20Issues.csv
@@ -103,7 +103,7 @@
 "`2960 <https://wg21.link/LWG2960>`__","[fund.ts.v3] ``nonesuch``\  is insufficiently useless","San Diego","|Complete|",""
 "`2995 <https://wg21.link/LWG2995>`__","``basic_stringbuf``\  default constructor forbids it from using SSO capacity","San Diego","",""
 "`2996 <https://wg21.link/LWG2996>`__","Missing rvalue overloads for ``shared_ptr``\  operations","San Diego","",""
-"`3008 <https://wg21.link/LWG3008>`__","``make_shared``\  (sub)object destruction semantics are not specified","San Diego","",""
+"`3008 <https://wg21.link/LWG3008>`__","``make_shared``\  (sub)object destruction semantics are not specified","San Diego","|Complete|","16.0"
 "`3022 <https://wg21.link/LWG3022>`__","``is_convertible<derived*, base*>``\  may lead to ODR","San Diego","Resolved by 1285R0",""
 "`3025 <https://wg21.link/LWG3025>`__","Map-like container deduction guides should use ``pair<Key, T>``\ , not ``pair<const Key, T>``\ ","San Diego","|Complete|",""
 "`3031 <https://wg21.link/LWG3031>`__","Algorithms and predicates with non-const reference arguments","San Diego","",""
diff --git a/libcxx/include/__memory/construct_at.h b/libcxx/include/__memory/construct_at.h
index ffee0022c243..14484dd6aa0d 100644
--- a/libcxx/include/__memory/construct_at.h
+++ b/libcxx/include/__memory/construct_at.h
@@ -83,6 +83,16 @@ _ForwardIterator __destroy(_ForwardIterator __first, _ForwardIterator __last) {
     return __first;
 }
 
+template <class _BidirectionalIterator>
+_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20
+_BidirectionalIterator __reverse_destroy(_BidirectionalIterator __first, _BidirectionalIterator __last) {
+    while (__last != __first) {
+        --__last;
+        std::__destroy_at(std::addressof(*__last));
+    }
+    return __last;
+}
+
 #if _LIBCPP_STD_VER > 14
 
 template <class _Tp, enable_if_t<!is_array_v<_Tp>, int> = 0>
diff --git a/libcxx/include/__memory/shared_ptr.h b/libcxx/include/__memory/shared_ptr.h
index 88c26fdb3809..f0ffa26abfeb 100644
--- a/libcxx/include/__memory/shared_ptr.h
+++ b/libcxx/include/__memory/shared_ptr.h
@@ -260,7 +260,10 @@ __shared_ptr_pointer<_Tp, _Dp, _Alloc>::__on_zero_shared_weak() _NOEXCEPT
     __a.deallocate(_PTraits::pointer_to(*this), 1);
 }
 
-struct __default_initialize_tag {};
+// This tag is used to instantiate an allocator type. The various shared_ptr control blocks
+// detect that the allocator has been instantiated for this type and perform alternative
+// initialization/destruction based on that.
+struct __for_overwrite_tag {};
 
 template <class _Tp, class _Alloc>
 struct __shared_ptr_emplace
@@ -271,25 +274,20 @@ struct __shared_ptr_emplace
     explicit __shared_ptr_emplace(_Alloc __a, _Args&& ...__args)
         : __storage_(_VSTD::move(__a))
     {
-#if _LIBCPP_STD_VER > 17
-        using _TpAlloc = typename __allocator_traits_rebind<_Alloc, _Tp>::type;
-        _TpAlloc __tmp(*__get_alloc());
-        allocator_traits<_TpAlloc>::construct(__tmp, __get_elem(), _VSTD::forward<_Args>(__args)...);
+#if _LIBCPP_STD_VER >= 20
+        if constexpr (is_same_v<typename _Alloc::value_type, __for_overwrite_tag>) {
+            static_assert(sizeof...(_Args) == 0, "No argument should be provided to the control block when using _for_overwrite");
+            ::new ((void*)__get_elem()) _Tp;
+        } else {
+            using _TpAlloc = typename __allocator_traits_rebind<_Alloc, _Tp>::type;
+            _TpAlloc __tmp(*__get_alloc());
+            allocator_traits<_TpAlloc>::construct(__tmp, __get_elem(), _VSTD::forward<_Args>(__args)...);
+        }
 #else
         ::new ((void*)__get_elem()) _Tp(_VSTD::forward<_Args>(__args)...);
 #endif
     }
 
-
-#if _LIBCPP_STD_VER >= 20
-    _LIBCPP_HIDE_FROM_ABI
-    explicit __shared_ptr_emplace(__default_initialize_tag, _Alloc __a)
-        : __storage_(std::move(__a))
-    {
-        ::new ((void*)__get_elem()) _Tp;
-    }
-#endif
-
     _LIBCPP_HIDE_FROM_ABI
     _Alloc* __get_alloc() _NOEXCEPT { return __storage_.__get_alloc(); }
 
@@ -299,9 +297,13 @@ struct __shared_ptr_emplace
 private:
     void __on_zero_shared() _NOEXCEPT override {
 #if _LIBCPP_STD_VER > 17
-        using _TpAlloc = typename __allocator_traits_rebind<_Alloc, _Tp>::type;
-        _TpAlloc __tmp(*__get_alloc());
-        allocator_traits<_TpAlloc>::destroy(__tmp, __get_elem());
+        if constexpr (is_same_v<typename _Alloc::value_type, __for_overwrite_tag>) {
+            __get_elem()->~_Tp();
+        } else {
+            using _TpAlloc = typename __allocator_traits_rebind<_Alloc, _Tp>::type;
+            _TpAlloc __tmp(*__get_alloc());
+            allocator_traits<_TpAlloc>::destroy(__tmp, __get_elem());
+        }
 #else
         __get_elem()->~_Tp();
 #endif
@@ -1008,12 +1010,9 @@ template<class _Tp, class _Alloc, __enable_if_t<!is_array<_Tp>::value, int> = 0>
 _LIBCPP_HIDE_FROM_ABI
 shared_ptr<_Tp> allocate_shared_for_overwrite(const _Alloc& __a)
 {
-    using _ControlBlock = __shared_ptr_emplace<_Tp, _Alloc>;
-    using _ControlBlockAllocator = typename __allocator_traits_rebind<_Alloc, _ControlBlock>::type;
-    __allocation_guard<_ControlBlockAllocator> __guard(__a, 1);
-    ::new ((void*)_VSTD::addressof(*__guard.__get())) _ControlBlock(__default_initialize_tag{}, __a);
-    auto __control_block = __guard.__release_ptr();
-    return shared_ptr<_Tp>::__create_with_control_block((*__control_block).__get_elem(), _VSTD::addressof(*__control_block));
+    using _ForOverwriteAllocator = __allocator_traits_rebind_t<_Alloc, __for_overwrite_tag>;
+    _ForOverwriteAllocator __alloc(__a);
+    return std::allocate_shared<_Tp>(__alloc);
 }
 
 template<class _Tp, __enable_if_t<!is_array<_Tp>::value, int> = 0>
@@ -1052,19 +1051,18 @@ struct __unbounded_array_control_block<_Tp[], _Alloc> : __shared_weak_count
     explicit __unbounded_array_control_block(_Alloc const& __alloc, size_t __count)
         : __alloc_(__alloc), __count_(__count)
     {
-        std::__uninitialized_allocator_value_construct_n_multidimensional(__alloc_, std::begin(__data_), __count_);
-    }
-
 #if _LIBCPP_STD_VER >= 20
-    _LIBCPP_HIDE_FROM_ABI
-    explicit __unbounded_array_control_block(_Alloc const& __alloc, size_t __count, __default_initialize_tag)
-        : __alloc_(__alloc), __count_(__count)
-    {
-        // We are purposefully not using an allocator-aware default construction because the spec says so.
-        // There's currently no way of expressing default initialization in an allocator-aware manner anyway.
-        std::uninitialized_default_construct_n(std::begin(__data_), __count_);
-    }
+        if constexpr (is_same_v<typename _Alloc::value_type, __for_overwrite_tag>) {
+            // We are purposefully not using an allocator-aware default construction because the spec says so.
+            // There's currently no way of expressing default initialization in an allocator-aware manner anyway.
+            std::uninitialized_default_construct_n(std::begin(__data_), __count_);
+        } else {
+            std::__uninitialized_allocator_value_construct_n_multidimensional(__alloc_, std::begin(__data_), __count_);
+        }
+#else
+        std::__uninitialized_allocator_value_construct_n_multidimensional(__alloc_, std::begin(__data_), __count_);
 #endif
+    }
 
     // Returns the number of bytes required to store a control block followed by the given number
     // of elements of _Tp, with the whole storage being aligned to a multiple of _Tp's alignment.
@@ -1087,8 +1085,17 @@ struct __unbounded_array_control_block<_Tp[], _Alloc> : __shared_weak_count
 
 private:
     void __on_zero_shared() _NOEXCEPT override {
+#if _LIBCPP_STD_VER >= 20
+        if constexpr (is_same_v<typename _Alloc::value_type, __for_overwrite_tag>) {
+            std::__reverse_destroy(__data_, __data_ + __count_);
+        } else {
+            __allocator_traits_rebind_t<_Alloc, _Tp> __value_alloc(__alloc_);
+            std::__allocator_destroy_multidimensional(__value_alloc, __data_, __data_ + __count_);
+        }
+#else
         __allocator_traits_rebind_t<_Alloc, _Tp> __value_alloc(__alloc_);
         std::__allocator_destroy_multidimensional(__value_alloc, __data_, __data_ + __count_);
+#endif
     }
 
     void __on_zero_shared_weak() _NOEXCEPT override {
@@ -1146,25 +1153,35 @@ struct __bounded_array_control_block<_Tp[_Count], _Alloc>
 
     _LIBCPP_HIDE_FROM_ABI
     explicit __bounded_array_control_block(_Alloc const& __alloc) : __alloc_(__alloc) {
-        std::__uninitialized_allocator_value_construct_n_multidimensional(__alloc_, std::addressof(__data_[0]), _Count);
-    }
-
 #if _LIBCPP_STD_VER >= 20
-    _LIBCPP_HIDE_FROM_ABI
-    explicit __bounded_array_control_block(_Alloc const& __alloc, __default_initialize_tag) : __alloc_(__alloc) {
-        // We are purposefully not using an allocator-aware default construction because the spec says so.
-        // There's currently no way of expressing default initialization in an allocator-aware manner anyway.
-        std::uninitialized_default_construct_n(std::addressof(__data_[0]), _Count);
-    }
+        if constexpr (is_same_v<typename _Alloc::value_type, __for_overwrite_tag>) {
+            // We are purposefully not using an allocator-aware default construction because the spec says so.
+            // There's currently no way of expressing default initialization in an allocator-aware manner anyway.
+            std::uninitialized_default_construct_n(std::addressof(__data_[0]), _Count);
+        } else {
+            std::__uninitialized_allocator_value_construct_n_multidimensional(__alloc_, std::addressof(__data_[0]), _Count);
+        }
+#else
+        std::__uninitialized_allocator_value_construct_n_multidimensional(__alloc_, std::addressof(__data_[0]), _Count);
 #endif
+    }
 
     _LIBCPP_HIDE_FROM_ABI_VIRTUAL
     ~__bounded_array_control_block() override { } // can't be `= default` because of the sometimes-non-trivial union member __data_
 
 private:
     void __on_zero_shared() _NOEXCEPT override {
+#if _LIBCPP_STD_VER >= 20
+        if constexpr (is_same_v<typename _Alloc::value_type, __for_overwrite_tag>) {
+            std::__reverse_destroy(__data_, __data_ + _Count);
+        } else {
+            __allocator_traits_rebind_t<_Alloc, _Tp> __value_alloc(__alloc_);
+            std::__allocator_destroy_multidimensional(__value_alloc, __data_, __data_ + _Count);
+        }
+#else
         __allocator_traits_rebind_t<_Alloc, _Tp> __value_alloc(__alloc_);
         std::__allocator_destroy_multidimensional(__value_alloc, __data_, __data_ + _Count);
+#endif
     }
 
     void __on_zero_shared_weak() _NOEXCEPT override {
@@ -1220,7 +1237,9 @@ template<class _Tp, class _Alloc, __enable_if_t<is_bounded_array<_Tp>::value, in
 _LIBCPP_HIDE_FROM_ABI
 shared_ptr<_Tp> allocate_shared_for_overwrite(const _Alloc& __a)
 {
-    return std::__allocate_shared_bounded_array<_Tp>(__a, __default_initialize_tag{});
+    using _ForOverwriteAllocator = __allocator_traits_rebind_t<_Alloc, __for_overwrite_tag>;
+    _ForOverwriteAllocator __alloc(__a);
+    return std::__allocate_shared_bounded_array<_Tp>(__alloc);
 }
 
 template<class _Tp, class = __enable_if_t<is_bounded_array<_Tp>::value>>
@@ -1241,7 +1260,7 @@ template<class _Tp, __enable_if_t<is_bounded_array<_Tp>::value, int> = 0>
 _LIBCPP_HIDE_FROM_ABI
 shared_ptr<_Tp> make_shared_for_overwrite()
 {
-    return std::__allocate_shared_bounded_array<_Tp>(allocator<_Tp>(), __default_initialize_tag{});
+    return std::__allocate_shared_bounded_array<_Tp>(allocator<__for_overwrite_tag>());
 }
 
 // unbounded array variants
@@ -1263,7 +1282,9 @@ template<class _Tp, class _Alloc, __enable_if_t<is_unbounded_array<_Tp>::value,
 _LIBCPP_HIDE_FROM_ABI
 shared_ptr<_Tp> allocate_shared_for_overwrite(const _Alloc& __a, size_t __n)
 {
-    return std::__allocate_shared_unbounded_array<_Tp>(__a, __n, __default_initialize_tag{});
+    using _ForOverwriteAllocator = __allocator_traits_rebind_t<_Alloc, __for_overwrite_tag>;
+    _ForOverwriteAllocator __alloc(__a);
+    return std::__allocate_shared_unbounded_array<_Tp>(__alloc, __n);
 }
 
 template<class _Tp, class = __enable_if_t<is_unbounded_array<_Tp>::value>>
@@ -1284,7 +1305,7 @@ template<class _Tp, __enable_if_t<is_unbounded_array<_Tp>::value, int> = 0>
 _LIBCPP_HIDE_FROM_ABI
 shared_ptr<_Tp> make_shared_for_overwrite(size_t __n)
 {
-    return std::__allocate_shared_unbounded_array<_Tp>(allocator<_Tp>(), __n, __default_initialize_tag{});
+    return std::__allocate_shared_unbounded_array<_Tp>(allocator<__for_overwrite_tag>(), __n);
 }
 
 #endif // _LIBCPP_STD_VER > 17
diff --git a/libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/allocate_shared_for_overwrite.pass.cpp b/libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/allocate_shared_for_overwrite.pass.cpp
index e69adeeb3ea6..27ff3cd56374 100644
--- a/libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/allocate_shared_for_overwrite.pass.cpp
+++ b/libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/allocate_shared_for_overwrite.pass.cpp
@@ -94,37 +94,62 @@ void testTypeWithDefaultCtor() {
   testDefaultConstructor<bare_allocator<WithDefaultCtor>>();
 }
 
+struct CountDestructions {
+  int* destructions_;
+  constexpr CountDestructions() = default;
+  constexpr CountDestructions(int* d) : destructions_(d) { }
+  constexpr ~CountDestructions() { ++*destructions_; }
+};
+
 void testAllocatorOperationsCalled() {
   // single
   {
     test_allocator_statistics alloc_stats;
+    int destructions = 0;
     {
-      [[maybe_unused]] std::same_as<std::shared_ptr<int>> auto ptr =
-          std::allocate_shared_for_overwrite<int>(test_allocator<void>{&alloc_stats});
+      [[maybe_unused]] std::same_as<std::shared_ptr<CountDestructions>> auto ptr =
+          std::allocate_shared_for_overwrite<CountDestructions>(test_allocator<void>{&alloc_stats});
+      std::construct_at<CountDestructions>(ptr.get(), &destructions);
       assert(alloc_stats.alloc_count == 1);
+      assert(alloc_stats.construct_count == 0);
     }
+    assert(destructions == 1);
+    assert(alloc_stats.destroy_count == 0);
     assert(alloc_stats.alloc_count == 0);
   }
 
   // bounded array
   {
     test_allocator_statistics alloc_stats;
+    int destructions = 0;
     {
-      [[maybe_unused]] std::same_as<std::shared_ptr<int[2]>> auto ptr =
-          std::allocate_shared_for_overwrite<int[2]>(test_allocator<void>{&alloc_stats});
+      [[maybe_unused]] std::same_as<std::shared_ptr<CountDestructions[2]>> auto ptr =
+          std::allocate_shared_for_overwrite<CountDestructions[2]>(test_allocator<void>{&alloc_stats});
+      std::construct_at<CountDestructions>(ptr.get() + 0, &destructions);
+      std::construct_at<CountDestructions>(ptr.get() + 1, &destructions);
       assert(alloc_stats.alloc_count == 1);
+      assert(alloc_stats.construct_count == 0);
     }
+    assert(destructions == 2);
+    assert(alloc_stats.destroy_count == 0);
     assert(alloc_stats.alloc_count == 0);
   }
 
   // unbounded array
   {
     test_allocator_statistics alloc_stats;
+    int destructions = 0;
     {
-      [[maybe_unused]] std::same_as<std::shared_ptr<int[]>> auto ptr =
-          std::allocate_shared_for_overwrite<int[]>(test_allocator<void>{&alloc_stats}, 3);
+      [[maybe_unused]] std::same_as<std::shared_ptr<CountDestructions[]>> auto ptr =
+          std::allocate_shared_for_overwrite<CountDestructions[]>(test_allocator<void>{&alloc_stats}, 3);
+      std::construct_at<CountDestructions>(ptr.get() + 0, &destructions);
+      std::construct_at<CountDestructions>(ptr.get() + 1, &destructions);
+      std::construct_at<CountDestructions>(ptr.get() + 2, &destructions);
       assert(alloc_stats.alloc_count == 1);
+      assert(alloc_stats.construct_count == 0);
     }
+    assert(destructions == 3);
+    assert(alloc_stats.destroy_count == 0);
     assert(alloc_stats.alloc_count == 0);
   }
 }
diff --git a/libcxx/test/support/test_allocator.h b/libcxx/test/support/test_allocator.h
index 3b9676ee1752..419e661d511b 100644
--- a/libcxx/test/support/test_allocator.h
+++ b/libcxx/test/support/test_allocator.h
@@ -31,6 +31,8 @@ struct test_allocator_statistics {
   int throw_after = INT_MAX;
   int count = 0;
   int alloc_count = 0;
+  int construct_count = 0; // the number of times that ::construct was called
+  int destroy_count = 0; // the number of times that ::destroy was called
   int copied = 0;
   int moved = 0;
   int converted = 0;
@@ -40,6 +42,8 @@ struct test_allocator_statistics {
     count = 0;
     time_to_throw = 0;
     alloc_count = 0;
+    construct_count = 0;
+    destroy_count = 0;
     throw_after = INT_MAX;
     clear_ctor_counters();
   }
@@ -144,7 +148,7 @@ public:
   TEST_CONSTEXPR pointer address(reference x) const { return &x; }
   TEST_CONSTEXPR const_pointer address(const_reference x) const { return &x; }
 
-  TEST_CONSTEXPR_CXX14 pointer allocate(size_type n, const void* = 0) {
+  TEST_CONSTEXPR_CXX14 pointer allocate(size_type n, const void* = nullptr) {
     assert(data_ != test_alloc_base::destructed_value);
     if (stats_ != nullptr) {
       if (stats_->time_to_throw >= stats_->throw_after)
@@ -166,6 +170,8 @@ public:
 
   template <class U>
   TEST_CONSTEXPR_CXX20 void construct(pointer p, U&& val) {
+    if (stats_ != nullptr)
+      ++stats_->construct_count;
 #if TEST_STD_VER > 17
     std::construct_at(std::to_address(p), std::forward<U>(val));
 #else
@@ -173,7 +179,11 @@ public:
 #endif
   }
 
-  TEST_CONSTEXPR_CXX14 void destroy(pointer p) { p->~T(); }
+  TEST_CONSTEXPR_CXX14 void destroy(pointer p) {
+    if (stats_ != nullptr)
+      ++stats_->destroy_count;
+    p->~T();
+  }
   TEST_CONSTEXPR friend bool operator==(const test_allocator& x, const test_allocator& y) { return x.data_ == y.data_; }
   TEST_CONSTEXPR friend bool operator!=(const test_allocator& x, const test_allocator& y) { return !(x == y); }