[libcxx-commits] [libcxx] 5801090 - [libc++] Fix bug in allocate_shared_for_overwrite
Louis Dionne via libcxx-commits
libcxx-commits at lists.llvm.org
Mon Feb 13 05:50:59 PST 2023
Author: Louis Dionne
Date: 2023-02-13T08:50:54-05:00
New Revision: 5801090258011cfe636cda1493ac9bc07fb2a889
URL: https://github.com/llvm/llvm-project/commit/5801090258011cfe636cda1493ac9bc07fb2a889
DIFF: https://github.com/llvm/llvm-project/commit/5801090258011cfe636cda1493ac9bc07fb2a889.diff
LOG: [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
Added:
Modified:
libcxx/docs/Status/Cxx20Issues.csv
libcxx/include/__memory/construct_at.h
libcxx/include/__memory/shared_ptr.h
libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/allocate_shared_for_overwrite.pass.cpp
libcxx/test/support/test_allocator.h
Removed:
################################################################################
diff --git a/libcxx/docs/Status/Cxx20Issues.csv b/libcxx/docs/Status/Cxx20Issues.csv
index 588d29b73caf8..df03ca2b41d4a 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 3953e9869d7e0..5e9abff85ef2f 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 b4798c6b31cff..8c861392eea51 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 e69adeeb3ea69..27ff3cd563740 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 b2c1ed6323eee..c78224946236b 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 @@ class test_allocator {
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 @@ class test_allocator {
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 @@ class test_allocator {
#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); }
More information about the libcxx-commits
mailing list