allocate_shared should call allocator_traits::construct

[zoecarver]

Summary

This patch updates allocate_shared to call allocator_traits::construct. Fixes issue 41900. Based on D62233.

Note: __shared_ptr_aligned_block is very similar to __shared_ptr_array_block from D62641 (but with support for custom pointers).

Currently, I have not transferred the changes so the C++03 overloads. Once this has been reviewed a bit, then I will update those.

[zoecarver]

• update fail tests

[zoecarver]

• Fix C++03 tests
• Rebase off master

[zoecarver]

@ldionne ping. This patch has most of the boilerplate needed for D62641/P0674R1.

[ldionne]

libcxx/include/memory

3506 ↗

(On Diff #263861)

Wait, so you have an array of sizeof(ControlBlock) elements of aligned storage with sizeof(ControlBlock)? That's sizeof(ControlBlock) * sizeof(ControlBlock) bytes of storage. Why do you need that?

3511 ↗

(On Diff #263861)

Similar comment but with sizeof(_Tp) * sizeof(_Tp). I also don't understand why this needs to be aligned to alignment_of<ControlBlock>. Can you explain?

3611 ↗

(On Diff #263861)

Isn't this an ABI break? It's not clear to me why we need to store the size -- we should always be calling allocator.deallocate with one instance of sizeof(__shared_ptr_pointer), no?

4546 ↗

(On Diff #263861)

These variable names (like _D1) are not really helpful. I know there weren't better before, but it would be easier to read with a better name.

4548 ↗

(On Diff #263861)

Shouldn't this be <_Tp*, _D1, _TAlloc> instead of <_Tp*, _D1, _Alloc>? If not, why?

4564 ↗

(On Diff #263861)

__create_with_control_block needs to be marked as noexcept if you want to do this and make it clear that it's safe. Otherwise we could leak __hold2 in case an exception was thrown from within __create_with_control_block.

libcxx/test/libcxx/utilities/memory/util.smartptr/util.smartptr.shared/function_type_default_deleter.fail.cpp

41 ↗

(On Diff #263861)

This expected-error is brittle, it won't work if we e.g. change the ABI namespace.

libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/allocate_shared_construct.pass.cpp

1 ↗

(On Diff #263861)

Could you add a test (in another test file) with the reproducer for https://bugs.llvm.org/show_bug.cgi?id=41900, with both private and protected constructors? IIUC, the intent of this patch was primarily to fix that.

16 ↗

(On Diff #263861)

Please explain why this needs to be tested in the comment. It could be as simple as "[...] as requested in C++20", which is the case I think.

109 ↗

(On Diff #263861)

Please fix this todo. It's fine to assume that C++03 has variadics, cause we only support C++03 with Clang, which supports variadics.

[zoecarver]

Thanks for the review!

libcxx/include/memory

3506 ↗

(On Diff #263861)

🤦‍♂️ Oops. Good catch. I originally had alignas(_CntrlBlk) char __cntrl_buff[sizeof(_CntrlBlk)] which didn't work in C++03 mode so, I used aligned_storage and seem to have forgotten to remove the array part.

3611 ↗

(On Diff #263861)

I was worried this might be an ABI break.

We need size, though. Because we need to know how big __shared_ptr_aligned_block is (and later we'll need to know how big __shared_ptr_aligned_block + elements is).

sizeof(__shared_ptr_pointer) would just be the control block. If we knew that __shared_ptr_pointer would only be used by allocate_shared we could do something like sizeof(__shared_ptr_aligned_block< __shared_ptr_pointer , remove_pointer_t<_Tp>>). To do that we'd probably have to duplicate __shared_ptr_pointer which would be an ABI break too, I think.

4546 ↗

(On Diff #263861)

You're right. I'll make these more clear.

4548 ↗

(On Diff #263861)

It doesn't really matter because it gets rebound to char before being called. I'll put a comment here.

4564 ↗

(On Diff #263861)

OK, I'll make that a separate patch, though. __create_with_control_block was already used here.

libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/allocate_shared_construct.pass.cpp

1 ↗

(On Diff #263861)

Yes. I'll add a test for that. Good call.

16 ↗

(On Diff #263861)

Huh, you're right. I didn't realize this was since C++20. I'll add the comment.

109 ↗

(On Diff #263861)

Yes, I'll fix this. I want to make it a separate patch because it will be a fairly large change.

[zoecarver]

• Fix based on review

[zoecarver]

libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/allocate_shared_construct.pass.cpp

130 ↗

(On Diff #264036)

Because of construct is currently implemented, this only works when passed exactly one argument. I'll fix that when I remove the rebind variadics.

[zoecarver]

• Format

[ldionne]

libcxx/include/memory

3658 ↗

(On Diff #264037)

Can you explain why, in this patch, using __a.deallocate(_PTraits::pointer_to(*this), 1); is not sufficient?

4542 ↗

(On Diff #264037)

This should either be there, or not be there. We don't leave commented-out stuff in the code :-)

libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/allocate_shared_construct.pass.cpp

16 ↗

(On Diff #263861)

Is this a DR that was back-ported to older standards, or should this "feature" only work in C++20? If so, I would expect something like UNSUPPORTED: c++03, c++11, c++14, c++17.

109 ↗

(On Diff #263861)

I guess my point is that you can always assume variadics, even in C++03 mode. We can remove any attempt to emulate variadics in C++03 prior to applying this patch.

[ldionne]

Requesting changes so it shows up properly in my review queue.

[zoecarver]

libcxx/include/memory

3658 ↗

(On Diff #264037)

Because _Al is rebound to char type and used in _ATraits which is used to get the pointer type for _PTraits so, _PTraits:: pointer_to will be expecting a char*.

4542 ↗

(On Diff #264037)

Oops. I thought I removed this.

libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/allocate_shared_construct.pass.cpp

16 ↗

(On Diff #263861)

According to cppreference, it should use placement new before C++20 (FWIW that seems a bit odd to me). I'll update this to use placement new before C++20.

109 ↗

(On Diff #263861)

OK good to know. I'll submit a patch for that today.

[zoecarver]

• Don't call construct before C++20
• Remove static_assert

[ldionne]

libcxx/include/memory

3658 ↗

(On Diff #264037)

But why do you need to rebind it to char?

[zoecarver]

libcxx/include/memory

3658 ↗

(On Diff #264037)

What other type would you bind it to? Sometimes we're going to need to deallocate sizeof(__shared_ptr_pointer) but sometimes it will need to be sizeof(__shared_ptr_aligned_block< __shared_ptr_pointer , remove_pointer_t<_Tp>>) and later it will need to be that plus the size and number of elements we allocated. We don't really know.

A temporary alternative could be adding another template argument which the allocator could rebind to but, we'd need to add back size in D62641 (to support unbounded array types).

[zoecarver]

Friendly ping @ldionne.

libcxx/include/memory

3611 ↗

(On Diff #263861)

Is there a way to verify that this is in fact an ABI break? If this is an ABI break, I have an idea about how to implement this by storing the size at the beginning of __shared_ptr_aligned_block but, I think that will add a lot of complexity, so this is (IMHO) a much better implementation if we can get away with it.

[ldionne]

Zoe and I discussed this offline, but saying here for watchers. I had to dive deep into this change because it is pretty complicated and I did not fully understand it. After playing around, I now understand why some things were done that way (e.g. why aligned_storage is being used) -- that is to avoid using __compressed_pair, which breaks the use case of private destructors.

However, I've come up with what I believe is a simpler approach to making this change in D91201. Zoe and I will be discussing that tomorrow, and we'll see what approach we end up taking (or perhaps a hybrid).

include/memory

public:
     _LIBCPP_INLINE_VISIBILITY __allocator_destructor(_Alloc& __a, size_type __s)
              _NOEXCEPT
         : __alloc_(__a), __s_(__s) {}
     _LIBCPP_INLINE_VISIBILITY
     void operator()(pointer __p) _NOEXCEPT
         {__alloc_traits::deallocate(__alloc_, __p, __s_);}
 };
 
+template <class _Alloc>
+class __allocator_destroyer
+{
+    typedef allocator_traits<_Alloc> __alloc_traits;
+    _Alloc __alloc_;
+public:
+    _LIBCPP_INLINE_VISIBILITY __allocator_destroyer(_Alloc const& __a)
+             _NOEXCEPT
+        : __alloc_(__a) {}
+
+    template<class _Tp>
+    _LIBCPP_INLINE_VISIBILITY
+    void operator()(_Tp* __p) _NOEXCEPT
+        {__alloc_traits::destroy(__alloc_, __p);}
+};
+
 template <class _InputIterator, class _ForwardIterator>
 _ForwardIterator
 uninitialized_copy(_InputIterator __f, _InputIterator __l, _ForwardIterator __r)
 {
     typedef typename iterator_traits<_ForwardIterator>::value_type value_type;
 #ifndef _LIBCPP_NO_EXCEPTIONS
     _ForwardIterator __s = __r;
     try

 {
 #ifndef _LIBCPP_NO_EXCEPTIONS
     throw bad_weak_ptr();
 #else
     _VSTD::abort();
 #endif
 }
 
+template<class _CntrlBlk, class _Tp>
+struct __shared_ptr_aligned_block
+{
+    typename std::aligned_storage<
+        sizeof(_CntrlBlk),
+        alignment_of<_CntrlBlk>::value
+    >::type __cntrl_buff[sizeof(_CntrlBlk)];
+
+    typename std::aligned_storage<
+        sizeof(_Tp),
+        alignment_of<_CntrlBlk>::value
+    >::type __value_buff[sizeof(_Tp)];
+};
+
 template<class _Tp> class _LIBCPP_TEMPLATE_VIS weak_ptr;
 
 class _LIBCPP_TYPE_VIS __shared_count
 {
     __shared_count(const __shared_count&);
     __shared_count& operator=(const __shared_count&);
 
 protected:
private:
     virtual void __on_zero_shared_weak() _NOEXCEPT = 0;
 };
 
 template <class _Tp, class _Dp, class _Alloc>
 class __shared_ptr_pointer
     : public __shared_weak_count
 {
     __compressed_pair<__compressed_pair<_Tp, _Dp>, _Alloc> __data_;
+    size_t __size;
+
 public:
     _LIBCPP_INLINE_VISIBILITY
-    __shared_ptr_pointer(_Tp __p, _Dp __d, _Alloc __a)
-        :  __data_(__compressed_pair<_Tp, _Dp>(__p, _VSTD::move(__d)), _VSTD::move(__a)) {}
+    __shared_ptr_pointer(_Tp __p, _Dp __d, _Alloc __a,
+                         size_t __size = sizeof(__shared_ptr_pointer))
+        :  __data_(__compressed_pair<_Tp, _Dp>(__p, _VSTD::move(__d)), _VSTD::move(__a)),
+           __size(__size) {}
 
 #ifndef _LIBCPP_NO_RTTI
     virtual const void* __get_deleter(const type_info&) const _NOEXCEPT;
 #endif
 
 private:
     virtual void __on_zero_shared() _NOEXCEPT;
     virtual void __on_zero_shared_weak() _NOEXCEPT;
__shared_ptr_pointer<_Tp, _Dp, _Alloc>::__on_zero_shared() _NOEXCEPT
     __data_.first().second()(__data_.first().first());
     __data_.first().second().~_Dp();
 }
 
 template <class _Tp, class _Dp, class _Alloc>
 void
 __shared_ptr_pointer<_Tp, _Dp, _Alloc>::__on_zero_shared_weak() _NOEXCEPT
 {
-    typedef typename __allocator_traits_rebind<_Alloc, __shared_ptr_pointer>::type _Al;
+    typedef typename __allocator_traits_rebind<_Alloc, char>::type _Al;
     typedef allocator_traits<_Al> _ATraits;
     typedef pointer_traits<typename _ATraits::pointer> _PTraits;
 
     _Al __a(__data_.second());
     __data_.second().~_Alloc();
-    __a.deallocate(_PTraits::pointer_to(*this), 1);
+    __a.deallocate(_PTraits::pointer_to(*reinterpret_cast<char*>(this)), __size);
 }
 
 template <class _Tp, class _Alloc>
 class __shared_ptr_emplace
     : public __shared_weak_count
 {
 private:
     __compressed_pair<_Alloc, _Tp> __data_;

 <
     !is_array<_Tp>::value,
     shared_ptr<_Tp>
 >::type
 allocate_shared(const _Alloc& __a, _Args&& ...__args)
 {
     static_assert(is_constructible<_Tp, _Args...>::value, "Can't construct object in allocate_shared");
 
-    typedef __shared_ptr_emplace<_Tp, _Alloc> _CntrlBlk;
-    typedef typename __allocator_traits_rebind<_Alloc, _CntrlBlk>::type _A2;
+    typedef typename __allocator_traits_rebind<_Alloc, _Tp>::type _TAlloc;
+    typedef __allocator_destroyer<_TAlloc> _D1;
+    typedef __shared_ptr_pointer<_Tp*, _D1, _Alloc> _CntrlBlk;
+    typedef __shared_ptr_aligned_block<_CntrlBlk, _Tp> _AlignedBlk;
+    typedef typename __allocator_traits_rebind<_Alloc, _AlignedBlk>::type _A2;
     typedef __allocator_destructor<_A2> _D2;
+    typedef allocator_traits<_TAlloc> _ATraits;
 
     _A2 __a2(__a);
-    unique_ptr<_CntrlBlk, _D2> __hold2(__a2.allocate(1), _D2(__a2, 1));
-    ::new(static_cast<void*>(_VSTD::addressof(*__hold2.get())))
-        _CntrlBlk(__a, _VSTD::forward<_Args>(__args)...);
-
-    return shared_ptr<_Tp>::__create_with_cntrl_block(__hold2.get()->get(),
-                                                      _VSTD::addressof(*__hold2.release()));
+    _TAlloc __a3(__a);
+    unique_ptr<_AlignedBlk, _D2> __hold2(__a2.allocate(1), _D2(__a2, 1));
+    _CntrlBlk*   __cntrl_buff = reinterpret_cast<_CntrlBlk*>(__hold2.get()->__cntrl_buff);
+    _Tp*         __value_buff = reinterpret_cast<_Tp*>      (__hold2.get()->__value_buff);
+
+    ::new(static_cast<void*>(__cntrl_buff))
+        _CntrlBlk(__value_buff, _D1(__a), __a2, sizeof(_AlignedBlk));
+    _ATraits::construct(__a3, __value_buff, _VSTD::forward<_Args>(__args)...);
+
+    __hold2.release();
+    return shared_ptr<_Tp>::__create_with_cntrl_block(__value_buff,
+                                                      __cntrl_buff);
 }
 
 #else  // _LIBCPP_HAS_NO_VARIADICS
 
 template<class _Tp>
 shared_ptr<_Tp>
 make_shared()
 {

test/libcxx/utilities/memory/util.smartptr/util.smartptr.shared/function_type_default_deleter.fail.cpp

int main(int, char**) {
   {
     SPtr<0> s; // OK
     SPtr<1> s1(nullptr); // OK
     SPtr<2> s2(getFn<2>(), Deleter{}); // OK
     SPtr<3> s3(nullptr, Deleter{}); // OK
   }
   // expected-error-re@memory:* 2 {{static_assert failed{{.*}} "default_delete cannot be instantiated for function types"}}
   {
+    // expected-error@memory:* {{no member named 'value' in 'std::__1::is_empty<std::__1::__compressed_pair<void (*)(Tag<4>), std::__1::default_delete<void (Tag<4>)> > >'}}
+    // expected-error@memory:* {{cannot initialize return object of type 'std::__1::__shared_weak_count *' with an rvalue of type 'std::__1::__shared_ptr_pointer<void (*)(Tag<4>), std::__1::default_delete<void (Tag<4>)>, std::__1::allocator<std::__1::__shared_ptr_dummy_rebind_allocator_type> > *'}}
     SPtr<4> s4(getFn<4>()); // expected-note {{requested here}}
     SPtr<5> s5(getFn<5>(), std::default_delete<FnType<5>>{}); // expected-note {{requested here}}
   }
 
   return 0;
 }

test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/allocate_shared_construct.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+// <memory>
+
+// shared_ptr
+
+// template<class T, class A, class... Args>
+//    shared_ptr<T> allocate_shared(const A& a, Args&&... args);
+
+// This patch tests that allocator_traits::construct is used in allocate_shared.
+
+// XFAIL UNTIL THE PATCH IS FINISHED
+
+#include <memory>
+
+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 id) : id(id) {}
+
+    template<class U>
+    MyAllocator(MyAllocator<U> const& other)
+        : id(other.id) {};
+
+    pointer allocate(std::ptrdiff_t n)
+    {
+        return pointer(static_cast<T*>(::operator new(n*sizeof(T))));
+    }
+
+    void deallocate(pointer p, std::ptrdiff_t)
+    {
+        return ::operator delete(p);
+    }
+
+    void construct(T *p)
+    {
+        construct_called = true;
+        destroy_called = false;
+        allocator_id = id;
+        ::new(p) T;
+    }
+
+    void construct(T *p, T x)
+    {
+        construct_called = true;
+        destroy_called = false;
+        allocator_id = id;
+        ::new(p) T(x);
+    }
+
+    void construct(T *p, T x, T y)
+    {
+        construct_called = true;
+        destroy_called = false;
+        allocator_id = id;
+        ::new(p) T(x, y);
+    }
+
+    void destroy(T *p)
+    {
+        destroy_called = true;
+        construct_called = false;
+        allocator_id = id;
+        p->~T();
+    }
+};
+
+struct mchar { char c; };
+
+struct Foo
+{
+    int val;
+
+    Foo(int val) : val(val) {}
+
+    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<int> p = std::allocate_shared<int>(MyAllocator<int>(), 42);
+        assert(construct_called);
+        assert(*p == 42);
+    }
+    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 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 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<Bar> p;
+        test_aligned(p.get(), alignof(Bar));
+    }
+
+    return 0;
+}