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

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

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

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?

4545

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.

4547

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

4566

__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

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

2

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.

17

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.

110

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

🤦‍♂️ 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

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.

4545

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

4547

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

4566

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

2

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

17

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

110

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

131

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

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

4542

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

17

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.

110

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

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

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

17

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.

110

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

But why do you need to rebind it to char?

[zoecarver]

libcxx/include/memory

3658

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

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).

libcxx/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 _VSTD::aligned_storage<
+        sizeof(_CntrlBlk),
+        _VSTD::alignment_of<_CntrlBlk>::value
+    >::type __cntrl_buff;
+
+    typename _VSTD::aligned_storage<
+        sizeof(_Tp),
+        _VSTD::alignment_of<_Tp>::value
+    >::type __value_buff;
+};
+
 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
 {
     __compressed_pair<_Alloc, _Tp> __data_;
 public:

 
 template<class _Tp, class _Alloc, class ..._Args>
 inline _LIBCPP_INLINE_VISIBILITY
 typename enable_if
 <
     !is_array<_Tp>::value,
     shared_ptr<_Tp>
 >::type
-allocate_shared(const _Alloc& __a, _Args&& ...__args)
+allocate_shared(const _Alloc& __alloc, _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 __allocator_destructor<_A2> _D2;
-
-    _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)...);
+//    static_assert( is_constructible<_Tp, _Args...>::value, "Can't construct object in allocate_shared");
 
-    typename shared_ptr<_Tp>::element_type *__p = __hold2.get()->get();
-    return shared_ptr<_Tp>::__create_with_control_block(__p, _VSTD::addressof(*__hold2.release()));
+    typedef typename __allocator_traits_rebind<_Alloc, _Tp>::type _AllocForTp;
+    typedef allocator_traits<_AllocForTp> _AllocTraitsForTp;
+    typedef __allocator_destroyer<_AllocForTp> _DestroyerForTp;
+
+    // The allocator here doesn't matter because it's going to be rebound before it's used.
+    typedef __shared_ptr_pointer<_Tp*, _DestroyerForTp, _Alloc> _CntrlBlk;
+    typedef __shared_ptr_aligned_block<_CntrlBlk, _Tp> _AlignedBlk;
+    typedef typename __allocator_traits_rebind<_Alloc, _AlignedBlk>::type _AllocForCntrlBlk;
+    typedef __allocator_destructor<_AllocForCntrlBlk> _DestroyerForCntrlBlk;
+
+    _AllocForCntrlBlk __alloc_for_cntrl(__alloc);
+    _AllocForTp __alloc_for_tp(__alloc);
+    unique_ptr<_AlignedBlk, _DestroyerForCntrlBlk> __holder(__alloc_for_cntrl.allocate(1),
+                                                            _DestroyerForCntrlBlk(__alloc_for_cntrl, 1));
+    _CntrlBlk* __cntrl_buff = reinterpret_cast<_CntrlBlk*>(&__holder.get()->__cntrl_buff);
+    _Tp* __value_buff = reinterpret_cast<_Tp*>(&__holder.get()->__value_buff);
+
+    ::new(static_cast<void*>(__cntrl_buff))
+        _CntrlBlk(__value_buff, _DestroyerForTp(__alloc), __alloc_for_cntrl, sizeof(_AlignedBlk));
+
+    _AllocTraitsForTp::construct(__alloc_for_tp, __value_buff, _VSTD::forward<_Args>(__args)...);
+    __holder.release();
+    return shared_ptr<_Tp>::__create_with_control_block(__value_buff, __cntrl_buff);
 }
 
 template<class _Tp, class _Up>
 inline _LIBCPP_INLINE_VISIBILITY
 bool
 operator==(const shared_ptr<_Tp>& __x, const shared_ptr<_Up>& __y) _NOEXCEPT
 {
     return __x.get() == __y.get();

libcxx/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 '{{.*}}is_empty<{{.*}}__compressed_pair<void (*)(Tag<4>), {{.*}}default_delete<void (Tag<4>)> > >'}}
+    // expected-error@memory:* {{cannot initialize return object of type '{{.*}}__shared_weak_count *' with an rvalue of type '{{.*}}__shared_ptr_pointer<void (*)(Tag<4>), {{.*}}default_delete<void (Tag<4>)>, {{.*}}allocator<{{.*}}__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;
 }

libcxx/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
+// as requested in C++20.
+
+#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 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 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 id) : id(id) {}
+  ~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 val) : val(val) {}
+
+  Foo(Foo a, Foo b) : val(a.val + b.val) {}
+};
+
+#if TEST_STD_VER >= 11
+struct Bar {
+  std::max_align_t y;
+};
+#endif // TEST_STD_VER >= 11
+
+void test_aligned(void* p, size_t align) {
+  assert(reinterpret_cast<uintptr_t>(p) % align == 0);
+}
+
+int main(int, char**) {
+  // TODO: in C++03 mode construct only works for some variadics.
+#if TEST_STD_VER >= 11
+  {
+    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);
+#endif // TEST_STD_VER >= 11
+
+  {
+    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);
+  }
+
+#if TEST_STD_VER >= 11
+  {
+    std::shared_ptr<Bar> p;
+    test_aligned(p.get(), alignof(Bar));
+  }
+#endif // TEST_STD_VER >= 11
+
+  return 0;
+}