[libcxx-commits] [libcxx] 955dd7b - [libc++] LWG2070: Use Allocator construction for objects created with allocate_shared
Louis Dionne via libcxx-commits
libcxx-commits at lists.llvm.org
Fri Jan 8 10:04:38 PST 2021
Author: Louis Dionne
Date: 2021-01-08T13:04:03-05:00
New Revision: 955dd7b7f3f6df79f573508ffb567f3923e892f7
URL: https://github.com/llvm/llvm-project/commit/955dd7b7f3f6df79f573508ffb567f3923e892f7
DIFF: https://github.com/llvm/llvm-project/commit/955dd7b7f3f6df79f573508ffb567f3923e892f7.diff
LOG: [libc++] LWG2070: Use Allocator construction for objects created with allocate_shared
This patch updates `allocate_shared` to call `allocator_traits::construct`
when creating the object held inside the shared_pointer, and
`allocator_traits::destroy` when destroying it. This resolves
the part of P0674R1 that was originally filed as LWG2070.
This change is landed separately from the rest of P0674R1 because it is
incredibly tricky from an ABI perspective.
This is the reason why this change is so tricky is that we previously
used EBO in a compressed pair to store both the allocator and the object
type stored in the `shared_ptr`. However, starting in C++20, P0674
requires us to use Allocator construction for initializing the object type.
That requirement rules out the use of the EBO for the object type, since
using the EBO implies that the base will be initialized when the control
block is initialized (and hence we can't do it through Allocator construction).
Hence, supporting P0674 requires changing how we store the object type
inside the control block, which we do while being ABI compatible by using
some trickery with a properly aligned char buffer.
Fixes https://llvm.org/PR41900
Supersedes https://llvm.org/D62760
Differential Revision: https://reviews.llvm.org/D91201
Added:
libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/libcxx.control_block_layout.pass.cpp
libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/allocate_shared_construct.pass.cpp
Modified:
libcxx/include/memory
libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/allocate_shared.pass.cpp
Removed:
################################################################################
diff --git a/libcxx/include/memory b/libcxx/include/memory
index 4167f0140d51..ef1a62b15eac 100644
--- a/libcxx/include/memory
+++ b/libcxx/include/memory
@@ -1286,9 +1286,7 @@ struct __compressed_pair_elem<_Tp, _Idx, true> : private _Tp {
template <class _T1, class _T2>
class __compressed_pair : private __compressed_pair_elem<_T1, 0>,
private __compressed_pair_elem<_T2, 1> {
- typedef _LIBCPP_NODEBUG_TYPE __compressed_pair_elem<_T1, 0> _Base1;
- typedef _LIBCPP_NODEBUG_TYPE __compressed_pair_elem<_T2, 1> _Base2;
-
+public:
// NOTE: This static assert should never fire because __compressed_pair
// is *almost never* used in a scenario where it's possible for T1 == T2.
// (The exception is std::function where it is possible that the function
@@ -1298,7 +1296,9 @@ class __compressed_pair : private __compressed_pair_elem<_T1, 0>,
"The current implementation is NOT ABI-compatible with the previous "
"implementation for this configuration");
-public:
+ typedef _LIBCPP_NODEBUG_TYPE __compressed_pair_elem<_T1, 0> _Base1;
+ typedef _LIBCPP_NODEBUG_TYPE __compressed_pair_elem<_T2, 1> _Base2;
+
template <bool _Dummy = true,
class = typename enable_if<
__dependent_type<is_default_constructible<_T1>, _Dummy>::value &&
@@ -1344,6 +1344,15 @@ public:
return static_cast<_Base2 const&>(*this).__get();
}
+ _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR
+ static _Base1* __get_first_base(__compressed_pair* __pair) _NOEXCEPT {
+ return static_cast<_Base1*>(__pair);
+ }
+ _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR
+ static _Base2* __get_second_base(__compressed_pair* __pair) _NOEXCEPT {
+ return static_cast<_Base2*>(__pair);
+ }
+
_LIBCPP_INLINE_VISIBILITY
void swap(__compressed_pair& __x)
_NOEXCEPT_(__is_nothrow_swappable<_T1>::value &&
@@ -2574,43 +2583,81 @@ template <class _Tp, class _Alloc>
struct __shared_ptr_emplace
: __shared_weak_count
{
- _LIBCPP_HIDE_FROM_ABI
- explicit __shared_ptr_emplace(_Alloc __a)
- : __data_(_VSTD::move(__a), __value_init_tag())
- { }
-
- template <class ..._Args>
+ template<class ..._Args>
_LIBCPP_HIDE_FROM_ABI
explicit __shared_ptr_emplace(_Alloc __a, _Args&& ...__args)
-#ifndef _LIBCPP_CXX03_LANG
- : __data_(piecewise_construct, _VSTD::forward_as_tuple(__a),
- _VSTD::forward_as_tuple(_VSTD::forward<_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)...);
#else
- : __data_(__a, _Tp(_VSTD::forward<_Args>(__args)...))
+ ::new ((void*)__get_elem()) _Tp(_VSTD::forward<_Args>(__args)...);
#endif
- { }
+ }
_LIBCPP_HIDE_FROM_ABI
- _Tp* __get_elem() _NOEXCEPT { return _VSTD::addressof(__data_.second()); }
+ _Alloc* __get_alloc() _NOEXCEPT { return __storage_.__get_alloc(); }
_LIBCPP_HIDE_FROM_ABI
- _Alloc* __get_alloc() _NOEXCEPT { return _VSTD::addressof(__data_.first()); }
+ _Tp* __get_elem() _NOEXCEPT { return __storage_.__get_elem(); }
private:
virtual void __on_zero_shared() _NOEXCEPT {
+#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());
+#else
__get_elem()->~_Tp();
+#endif
}
virtual void __on_zero_shared_weak() _NOEXCEPT {
using _ControlBlockAlloc = typename __allocator_traits_rebind<_Alloc, __shared_ptr_emplace>::type;
using _ControlBlockPointer = typename allocator_traits<_ControlBlockAlloc>::pointer;
_ControlBlockAlloc __tmp(*__get_alloc());
- __get_alloc()->~_Alloc();
+ __storage_.~_Storage();
allocator_traits<_ControlBlockAlloc>::deallocate(__tmp,
pointer_traits<_ControlBlockPointer>::pointer_to(*this), 1);
}
- __compressed_pair<_Alloc, _Tp> __data_;
+ // This class implements the control block for non-array shared pointers created
+ // through `std::allocate_shared` and `std::make_shared`.
+ //
+ // In previous versions of the library, we used a compressed pair to store
+ // both the _Alloc and the _Tp. This implies using EBO, which is incompatible
+ // with Allocator construction for _Tp. To allow implementing P0674 in C++20,
+ // we now use a properly aligned char buffer while making sure that we maintain
+ // the same layout that we had when we used a compressed pair.
+ using _CompressedPair = __compressed_pair<_Alloc, _Tp>;
+ struct _ALIGNAS_TYPE(_CompressedPair) _Storage {
+ char __blob_[sizeof(_CompressedPair)];
+
+ _LIBCPP_HIDE_FROM_ABI explicit _Storage(_Alloc&& __a) {
+ ::new ((void*)__get_alloc()) _Alloc(_VSTD::move(__a));
+ }
+ _LIBCPP_HIDE_FROM_ABI ~_Storage() {
+ __get_alloc()->~_Alloc();
+ }
+ _Alloc* __get_alloc() _NOEXCEPT {
+ _CompressedPair *__as_pair = reinterpret_cast<_CompressedPair*>(__blob_);
+ typename _CompressedPair::_Base1* __first = _CompressedPair::__get_first_base(__as_pair);
+ _Alloc *__alloc = reinterpret_cast<_Alloc*>(__first);
+ return __alloc;
+ }
+ _Tp* __get_elem() _NOEXCEPT {
+ _CompressedPair *__as_pair = reinterpret_cast<_CompressedPair*>(__blob_);
+ typename _CompressedPair::_Base2* __second = _CompressedPair::__get_second_base(__as_pair);
+ _Tp *__elem = reinterpret_cast<_Tp*>(__second);
+ return __elem;
+ }
+ };
+
+ static_assert(_LIBCPP_ALIGNOF(_Storage) == _LIBCPP_ALIGNOF(_CompressedPair), "");
+ static_assert(sizeof(_Storage) == sizeof(_CompressedPair), "");
+ _Storage __storage_;
};
struct __shared_ptr_dummy_rebind_allocator_type;
diff --git a/libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/libcxx.control_block_layout.pass.cpp b/libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/libcxx.control_block_layout.pass.cpp
new file mode 100644
index 000000000000..9fc5273e89e9
--- /dev/null
+++ b/libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/libcxx.control_block_layout.pass.cpp
@@ -0,0 +1,164 @@
+//===----------------------------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03
+// REQUIRES: libc++
+
+// This test makes sure that the control block implementation used for non-array
+// types in std::make_shared and std::allocate_shared is ABI compatbile with the
+// original implementation.
+//
+// This test is relevant because the implementation of that control block is
+//
diff erent starting in C++20, a change that was required to implement P0674.
+
+#include <cassert>
+#include <cstddef>
+#include <memory>
+#include <type_traits>
+#include <utility>
+
+#include <string>
+#include <vector>
+
+#include "test_macros.h"
+
+// This is the pre-C++20 implementation of the control block used by non-array
+// std::allocate_shared and std::make_shared. We keep it here so that we can
+// make sure our implementation is backwards compatible with it forever.
+//
+// Of course, the class and its methods were renamed, but the size and layout
+// of the class should remain the same as the original implementation.
+template <class T, class Alloc>
+struct OldEmplaceControlBlock
+ : std::__shared_weak_count
+{
+ explicit OldEmplaceControlBlock(Alloc a) : data_(std::move(a), std::__value_init_tag()) { }
+ T* get_elem() noexcept { return std::addressof(data_.second()); }
+ Alloc* get_alloc() noexcept { return std::addressof(data_.first()); }
+
+private:
+ virtual void __on_zero_shared() noexcept {
+ // Not implemented
+ }
+
+ virtual void __on_zero_shared_weak() noexcept {
+ // Not implemented
+ }
+
+ std::__compressed_pair<Alloc, T> data_;
+};
+
+template <class T, template <class> class Alloc>
+void test() {
+ using Old = OldEmplaceControlBlock<T, Alloc<T>>;
+ using New = std::__shared_ptr_emplace<T, Alloc<T>>;
+
+ static_assert(sizeof(New) == sizeof(Old), "");
+ static_assert(alignof(New) == alignof(Old), "");
+
+ // Also make sure each member is at the same offset
+ Alloc<T> a;
+ Old old(a);
+ New new_(a);
+
+ // 1. Check the stored object
+ {
+ char const* old_elem = reinterpret_cast<char const*>(old.get_elem());
+ char const* new_elem = reinterpret_cast<char const*>(new_.__get_elem());
+ std::ptr
diff _t old_offset = old_elem - reinterpret_cast<char const*>(&old);
+ std::ptr
diff _t new_offset = new_elem - reinterpret_cast<char const*>(&new_);
+ assert(new_offset == old_offset && "offset of stored element changed");
+ }
+
+ // 2. Check the allocator
+ {
+ char const* old_alloc = reinterpret_cast<char const*>(old.get_alloc());
+ char const* new_alloc = reinterpret_cast<char const*>(new_.__get_alloc());
+ std::ptr
diff _t old_offset = old_alloc - reinterpret_cast<char const*>(&old);
+ std::ptr
diff _t new_offset = new_alloc - reinterpret_cast<char const*>(&new_);
+ assert(new_offset == old_offset && "offset of allocator changed");
+ }
+
+ // Make sure both types have the same triviality (that has ABI impact since
+ // it determined how objects are passed). Both should be non-trivial.
+ static_assert(std::is_trivial<New>::value == std::is_trivial<Old>::value, "");
+}
+
+// Object types to store in the control block
+struct TrivialEmptyType { };
+struct TrivialNonEmptyType { char c[11]; };
+struct FinalEmptyType final { };
+struct NonTrivialType {
+ char c[22];
+ NonTrivialType() : c{'x'} { }
+};
+
+// Allocator types
+template <class T>
+struct TrivialEmptyAlloc {
+ using value_type = T;
+ TrivialEmptyAlloc() = default;
+ template <class U> TrivialEmptyAlloc(TrivialEmptyAlloc<U>) { }
+ T* allocate(std::size_t) { return nullptr; }
+ void deallocate(T*, std::size_t) { }
+};
+template <class T>
+struct TrivialNonEmptyAlloc {
+ char storage[77];
+ using value_type = T;
+ TrivialNonEmptyAlloc() = default;
+ template <class U> TrivialNonEmptyAlloc(TrivialNonEmptyAlloc<U>) { }
+ T* allocate(std::size_t) { return nullptr; }
+ void deallocate(T*, std::size_t) { }
+};
+template <class T>
+struct FinalEmptyAlloc final {
+ using value_type = T;
+ FinalEmptyAlloc() = default;
+ template <class U> FinalEmptyAlloc(FinalEmptyAlloc<U>) { }
+ T* allocate(std::size_t) { return nullptr; }
+ void deallocate(T*, std::size_t) { }
+};
+template <class T>
+struct NonTrivialAlloc {
+ char storage[88];
+ using value_type = T;
+ NonTrivialAlloc() { }
+ template <class U> NonTrivialAlloc(NonTrivialAlloc<U>) { }
+ T* allocate(std::size_t) { return nullptr; }
+ void deallocate(T*, std::size_t) { }
+};
+
+int main(int, char**) {
+ test<TrivialEmptyType, TrivialEmptyAlloc>();
+ test<TrivialEmptyType, TrivialNonEmptyAlloc>();
+ test<TrivialEmptyType, FinalEmptyAlloc>();
+ test<TrivialEmptyType, NonTrivialAlloc>();
+
+ test<TrivialNonEmptyType, TrivialEmptyAlloc>();
+ test<TrivialNonEmptyType, TrivialNonEmptyAlloc>();
+ test<TrivialNonEmptyType, FinalEmptyAlloc>();
+ test<TrivialNonEmptyType, NonTrivialAlloc>();
+
+ test<FinalEmptyType, TrivialEmptyAlloc>();
+ test<FinalEmptyType, TrivialNonEmptyAlloc>();
+ test<FinalEmptyType, FinalEmptyAlloc>();
+ test<FinalEmptyType, NonTrivialAlloc>();
+
+ test<NonTrivialType, TrivialEmptyAlloc>();
+ test<NonTrivialType, TrivialNonEmptyAlloc>();
+ test<NonTrivialType, FinalEmptyAlloc>();
+ test<NonTrivialType, NonTrivialAlloc>();
+
+ // Test a few real world types just to make sure we didn't mess up badly somehow
+ test<std::string, std::allocator>();
+ test<int, std::allocator>();
+ test<std::vector<int>, std::allocator>();
+
+ return 0;
+}
diff --git a/libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/allocate_shared.pass.cpp b/libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/allocate_shared.pass.cpp
index 72069456fa71..f7a96d8d24d0 100644
--- a/libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/allocate_shared.pass.cpp
+++ b/libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/allocate_shared.pass.cpp
@@ -94,6 +94,22 @@ struct Three
int Three::count = 0;
+template<class T>
+struct AllocNoConstruct : std::allocator<T>
+{
+ AllocNoConstruct() = default;
+
+ template <class T1>
+ AllocNoConstruct(AllocNoConstruct<T1>) {}
+
+ template <class T1>
+ struct rebind {
+ typedef AllocNoConstruct<T1> other;
+ };
+
+ void construct(void*) { assert(false); }
+};
+
template <class Alloc>
void test()
{
@@ -161,5 +177,12 @@ int main(int, char**)
}
assert(A::count == 0);
+ // Test that we don't call construct before C++20.
+#if TEST_STD_VER < 20
+ {
+ (void)std::allocate_shared<int>(AllocNoConstruct<int>());
+ }
+#endif // TEST_STD_VER < 20
+
return 0;
}
diff --git a/libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/allocate_shared_construct.pass.cpp b/libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/allocate_shared_construct.pass.cpp
new file mode 100644
index 000000000000..d45d6e124b87
--- /dev/null
+++ b/libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/allocate_shared_construct.pass.cpp
@@ -0,0 +1,176 @@
+//===----------------------------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+
+// <memory>
+
+// shared_ptr
+
+// template<class T, class A, class... Args>
+// shared_ptr<T> allocate_shared(const A& a, Args&&... args);
+
+// This test checks that allocator_traits::construct is used in allocate_shared
+// as requested in C++20 (via P0674R1).
+
+#include "test_macros.h"
+
+#include <memory>
+#include <cassert>
+
+static bool construct_called = false;
+static bool destroy_called = false;
+static unsigned allocator_id = 0;
+
+template <class T>
+struct MyAllocator {
+public:
+ typedef T value_type;
+ typedef T* pointer;
+
+ unsigned id = 0;
+
+ MyAllocator() = default;
+ MyAllocator(int i) : id(i) {}
+
+ template <class U>
+ MyAllocator(MyAllocator<U> const& other) : id(other.id){};
+
+ pointer allocate(std::ptr
diff _t n) {
+ return pointer(static_cast<T*>(::operator new(n * sizeof(T))));
+ }
+
+ void deallocate(pointer p, std::ptr
diff _t) { return ::operator delete(p); }
+
+ template <typename ...Args>
+ void construct(T* p, Args&& ...args) {
+ construct_called = true;
+ destroy_called = false;
+ allocator_id = id;
+ ::new (p) T(std::forward<Args>(args)...);
+ }
+
+ void destroy(T* p) {
+ construct_called = false;
+ destroy_called = true;
+ allocator_id = id;
+ p->~T();
+ }
+};
+
+struct Private;
+
+class Factory {
+public:
+ static std::shared_ptr<Private> allocate();
+};
+
+template <class T>
+struct FactoryAllocator;
+
+struct Private {
+ int id;
+
+private:
+ friend FactoryAllocator<Private>;
+ Private(int i) : id(i) {}
+ ~Private() {}
+};
+
+template <class T>
+struct FactoryAllocator : std::allocator<T> {
+ FactoryAllocator() = default;
+
+ template <class T1>
+ FactoryAllocator(FactoryAllocator<T1>) {}
+
+ template <class T1>
+ struct rebind {
+ typedef FactoryAllocator<T1> other;
+ };
+
+ void construct(void* p, int id) { ::new (p) Private(id); }
+ void destroy(Private* p) { p->~Private(); }
+};
+
+std::shared_ptr<Private> Factory::allocate() {
+ FactoryAllocator<Private> factory_alloc;
+ return std::allocate_shared<Private>(factory_alloc, 42);
+}
+
+struct mchar {
+ char c;
+};
+
+struct Foo {
+ int val;
+
+ Foo(int v) : val(v) {}
+
+ Foo(Foo a, Foo b) : val(a.val + b.val) {}
+};
+
+struct Bar {
+ std::max_align_t y;
+};
+
+void test_aligned(void* p, size_t align) {
+ assert(reinterpret_cast<uintptr_t>(p) % align == 0);
+}
+
+int main(int, char**) {
+ {
+ std::shared_ptr<int> p = std::allocate_shared<int>(MyAllocator<int>());
+ assert(construct_called);
+ }
+ assert(destroy_called);
+ {
+ std::shared_ptr<Foo> p =
+ std::allocate_shared<Foo>(MyAllocator<Foo>(), Foo(42), Foo(100));
+ assert(construct_called);
+ assert(p->val == 142);
+ }
+ assert(destroy_called);
+ { // Make sure allocator is copied.
+ std::shared_ptr<int> p = std::allocate_shared<int>(MyAllocator<int>(3));
+ assert(allocator_id == 3);
+
+ allocator_id = 0;
+ }
+ assert(allocator_id == 3);
+
+ {
+ std::shared_ptr<int> p = std::allocate_shared<int>(MyAllocator<int>(), 42);
+ assert(construct_called);
+ assert(*p == 42);
+ }
+ assert(destroy_called);
+
+ { // Make sure allocator is properly re-bound.
+ std::shared_ptr<int> p =
+ std::allocate_shared<int>(MyAllocator<mchar>(), 42);
+ assert(construct_called);
+ assert(*p == 42);
+ }
+ assert(destroy_called);
+
+ {
+ // Make sure that we call the correct allocator::construct. Private has a private constructor
+ // so the construct method must be called on its friend Factory's allocator
+ // (Factory::Allocator).
+ std::shared_ptr<Private> p = Factory().allocate();
+ assert(p->id == 42);
+ }
+
+ {
+ std::shared_ptr<Bar> p;
+ test_aligned(p.get(), alignof(Bar));
+ }
+
+ return 0;
+}
More information about the libcxx-commits
mailing list