From 955dd7b7f3f6df79f573508ffb567f3923e892f7 Mon Sep 17 00:00:00 2001 From: Louis Dionne Date: Fri, 11 Dec 2020 12:22:16 -0500 Subject: [PATCH] [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 --- libcxx/include/memory | 85 +++++++--- .../libcxx.control_block_layout.pass.cpp | 164 +++++++++++++++++++ .../allocate_shared.pass.cpp | 23 +++ .../allocate_shared_construct.pass.cpp | 176 +++++++++++++++++++++ 4 files changed, 429 insertions(+), 19 deletions(-) create mode 100644 libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/libcxx.control_block_layout.pass.cpp create mode 100644 libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/allocate_shared_construct.pass.cpp diff --git a/libcxx/include/memory b/libcxx/include/memory index 4167f01..ef1a62b 100644 --- a/libcxx/include/memory +++ b/libcxx/include/memory @@ -1286,9 +1286,7 @@ struct __compressed_pair_elem<_Tp, _Idx, true> : private _Tp { template 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 , _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 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 + template _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 0000000..9fc5273 --- /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 +// different starting in C++20, a change that was required to implement P0674. + +#include +#include +#include +#include +#include + +#include +#include + +#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 +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 data_; +}; + +template class Alloc> +void test() { + using Old = OldEmplaceControlBlock>; + using New = std::__shared_ptr_emplace>; + + static_assert(sizeof(New) == sizeof(Old), ""); + static_assert(alignof(New) == alignof(Old), ""); + + // Also make sure each member is at the same offset + Alloc a; + Old old(a); + New new_(a); + + // 1. Check the stored object + { + char const* old_elem = reinterpret_cast(old.get_elem()); + char const* new_elem = reinterpret_cast(new_.__get_elem()); + std::ptrdiff_t old_offset = old_elem - reinterpret_cast(&old); + std::ptrdiff_t new_offset = new_elem - reinterpret_cast(&new_); + assert(new_offset == old_offset && "offset of stored element changed"); + } + + // 2. Check the allocator + { + char const* old_alloc = reinterpret_cast(old.get_alloc()); + char const* new_alloc = reinterpret_cast(new_.__get_alloc()); + std::ptrdiff_t old_offset = old_alloc - reinterpret_cast(&old); + std::ptrdiff_t new_offset = new_alloc - reinterpret_cast(&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::value == std::is_trivial::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 +struct TrivialEmptyAlloc { + using value_type = T; + TrivialEmptyAlloc() = default; + template TrivialEmptyAlloc(TrivialEmptyAlloc) { } + T* allocate(std::size_t) { return nullptr; } + void deallocate(T*, std::size_t) { } +}; +template +struct TrivialNonEmptyAlloc { + char storage[77]; + using value_type = T; + TrivialNonEmptyAlloc() = default; + template TrivialNonEmptyAlloc(TrivialNonEmptyAlloc) { } + T* allocate(std::size_t) { return nullptr; } + void deallocate(T*, std::size_t) { } +}; +template +struct FinalEmptyAlloc final { + using value_type = T; + FinalEmptyAlloc() = default; + template FinalEmptyAlloc(FinalEmptyAlloc) { } + T* allocate(std::size_t) { return nullptr; } + void deallocate(T*, std::size_t) { } +}; +template +struct NonTrivialAlloc { + char storage[88]; + using value_type = T; + NonTrivialAlloc() { } + template NonTrivialAlloc(NonTrivialAlloc) { } + T* allocate(std::size_t) { return nullptr; } + void deallocate(T*, std::size_t) { } +}; + +int main(int, char**) { + test(); + test(); + test(); + test(); + + test(); + test(); + test(); + test(); + + test(); + test(); + test(); + test(); + + test(); + test(); + test(); + test(); + + // Test a few real world types just to make sure we didn't mess up badly somehow + test(); + test(); + test, 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 7206945..f7a96d8 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 +struct AllocNoConstruct : std::allocator +{ + AllocNoConstruct() = default; + + template + AllocNoConstruct(AllocNoConstruct) {} + + template + struct rebind { + typedef AllocNoConstruct other; + }; + + void construct(void*) { assert(false); } +}; + template 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(AllocNoConstruct()); + } +#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 0000000..d45d6e1 --- /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 + +// + +// shared_ptr + +// template +// shared_ptr 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 +#include + +static bool construct_called = false; +static bool destroy_called = false; +static unsigned allocator_id = 0; + +template +struct MyAllocator { +public: + typedef T value_type; + typedef T* pointer; + + unsigned id = 0; + + MyAllocator() = default; + MyAllocator(int i) : id(i) {} + + template + MyAllocator(MyAllocator const& other) : id(other.id){}; + + pointer allocate(std::ptrdiff_t n) { + return pointer(static_cast(::operator new(n * sizeof(T)))); + } + + void deallocate(pointer p, std::ptrdiff_t) { return ::operator delete(p); } + + template + void construct(T* p, Args&& ...args) { + construct_called = true; + destroy_called = false; + allocator_id = id; + ::new (p) T(std::forward(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 allocate(); +}; + +template +struct FactoryAllocator; + +struct Private { + int id; + +private: + friend FactoryAllocator; + Private(int i) : id(i) {} + ~Private() {} +}; + +template +struct FactoryAllocator : std::allocator { + FactoryAllocator() = default; + + template + FactoryAllocator(FactoryAllocator) {} + + template + struct rebind { + typedef FactoryAllocator other; + }; + + void construct(void* p, int id) { ::new (p) Private(id); } + void destroy(Private* p) { p->~Private(); } +}; + +std::shared_ptr Factory::allocate() { + FactoryAllocator factory_alloc; + return std::allocate_shared(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(p) % align == 0); +} + +int main(int, char**) { + { + std::shared_ptr p = std::allocate_shared(MyAllocator()); + assert(construct_called); + } + assert(destroy_called); + { + std::shared_ptr p = + std::allocate_shared(MyAllocator(), Foo(42), Foo(100)); + assert(construct_called); + assert(p->val == 142); + } + assert(destroy_called); + { // Make sure allocator is copied. + std::shared_ptr p = std::allocate_shared(MyAllocator(3)); + assert(allocator_id == 3); + + allocator_id = 0; + } + assert(allocator_id == 3); + + { + std::shared_ptr p = std::allocate_shared(MyAllocator(), 42); + assert(construct_called); + assert(*p == 42); + } + assert(destroy_called); + + { // Make sure allocator is properly re-bound. + std::shared_ptr p = + std::allocate_shared(MyAllocator(), 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 p = Factory().allocate(); + assert(p->id == 42); + } + + { + std::shared_ptr p; + test_aligned(p.get(), alignof(Bar)); + } + + return 0; +} -- 2.7.4