[libc++] LWG2070: Use Allocator construction for objects created with allocate_shared

[ldionne]

Summary

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, as explained in the code:

Prior to C++20 (or if the compiler doesn't support [[no_unique_address]]),
we use an implementation that performs EBO for both the allocator and the
object type stored in the `shared_ptr`. This is the original implementation
that has been used in libc++, and we must be ABI compatible with it.

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 [[no_unique_address]].

To achieve ABI compatibility, this patch implements an ABI-compatible type equivalent to __compressed_pair, but where the data member is stored in a union, and hence available to be initialized via Allocator construction. The tricky part with this emulation is that it needs to be 100% ABI compatible with __compressed_pair, which means also emulating its drawbacks, such as the inability to elide storage for final classes.

Fixes https://llvm.org/PR41900
Supersedes https://llvm.org/D62760

[zoecarver]

libcxx/include/memory

3369

I don't think this works. We need to call allocator_traits<_Tp>::construct which requires us to have a pointer of type _Tp* which is why D62760 has to use __shared_ptr_pointer instead of __shared_ptr_emplace.

allocator_traits<_ControlBlockAllocator>::construct isn't sufficient, see the relevant part of the standard: https://eel.is/c++draft/util.smartptr.shared#create-7.5

[zoecarver]

This is my bad; I should have made this more clear or changed the order of the patches. One of the primary reasons I'm pushing on D62760 is because it adds __shared_ptr_aligned_block and the preliminary logic to use it. This is necessary to support array types in make_shared and allocate_shared, and I thought it would be easier if there was a separate patch to implement __shared_ptr_aligned_block.

That being said, make_shared and allocate_shared will have different overloads for array types, so if we can get away with using __shared_ptr_emplace in the non-array overloads, we should 100% try to do that to simplify the implementation (as is done in this patch).

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

29

In either patch, it would probably be a good idea to move these into MyAllocator so we can make sure that the correct specialization is setting them.

[ldionne]

Update with a new approach entirely.

[ldionne]

Reupload as patch application failed

[ldionne]

Another approach, which Zoe thought about just now and told me on Slack, would be to implement the storage like this instead (untested, I just wrote this up):

struct _Storage {
    _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 { /* compute offset */ }
    _Tp* __get_elem() _NOEXCEPT { /* compute offset */ }
    char alignas(__compressed_pair<_Alloc, _Tp>) __blob_[sizeof(__compressed_pair<_Alloc, _Tp>)];
};

I think we'd be able to compute the offsets with a bit of help from the __compressed_pair, and then I'm pretty sure it would be valid to placement-new into the blob as we like. I'll explore this approach too to see what it gives - the benefit would be not needing [[no_unique_address]] and potentially simpler code.

[zoecarver]

Let me start by saying that I haven't spent a lot of time looking at it but I really like this implementation.

---

In D91201#2453425, @ldionne wrote:

Another approach, which Zoe thought about just now and told me on Slack, would be to implement the storage like this instead (untested, I just wrote this up):

struct _Storage {
    _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 { /* compute offset */ }
    _Tp* __get_elem() _NOEXCEPT { /* compute offset */ }
    char alignas(__compressed_pair<_Alloc, _Tp>) __blob_[sizeof(__compressed_pair<_Alloc, _Tp>)];
};

I think we'd be able to compute the offsets with a bit of help from the __compressed_pair, and then I'm pretty sure it would be valid to placement-new into the blob as we like. I'll explore this approach too to see what it gives - the benefit would be not needing [[no_unique_address]] and potentially simpler code.

If the approach I'm talking about is simpler in this patch, it might not be simpler in absolute terms. If we're going to implement _Storage anyway as a more general thing, I think this implementation makes a lot of sense.

I think the code snippet I quoted is kind of missing my point. Using aligned storage, or a union, or anything similar is a bit irrelevant to what I was trying to say. I was highlighting a good way to do the "compute offset" part. The fundamental problem that this patch is trying to solve is that we need a pointer to the second element of __data_ in __shared_ptr_emplace, and we need that pointer before __shared_ptr_emplace is initialized. One way to do that (which you implement in this patch) is to make the second member a union and take its address. Another solution (what I was proposing) is we could simply static cast a pointer to __data_ to the correct __compressed_pair_elem type. Because __compressed_pair inherits from both of its elements, this should Just Work™. The latter solution would require no changes to the layout of __shared_ptr_emplace (and depending on how you look at it, that might be positive).

libcxx/include/memory

2611

This reminds me that we should support/test move-only allocator types.

2634

Do we really need __tmp allocators everywhere?

2639

IMHO we should make _Storage a more general utility.

Also can you please static assert that the alignment and size is the same as an equivilant __compressed_pair.

2643

This class is a work of art :)

[ldionne]

Add static_assert

libcxx/include/memory

2634

We need this one because we destroy the original one when we call ~Storage(), and then we need to deallocate this using the allocator. So I really don't see a way to get around this.

2639

IMHO we should make _Storage a more general utility.

I think I agree, but I'd like to do that in a separate patch. Making it generic will add complexity, like the ability to skip initialization of either member, etc. I'd like to keep this change small because of the trickiness.

Also can you please static assert that the alignment and size is the same as an equivilant __compressed_pair.

Done.

2643

I'm not sure whether I want to say thank you or run away and hide :-). I guess the problem we're trying to solve is pretty terrible, but the solution isn't too bad.

[ldionne]

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

29

I don't think we can, because the allocator is going to be rebound to allocate an unknown object (ie the shared pointer control block).

[zoecarver]

libcxx/include/memory

2648

The above comment got me thinking... why can't this be a default destructor?

[ldionne]

libcxx/include/memory

2648

Frankly, I was getting an error about the compiler being unable to generate a destructor because the type in the union is non trivial, and this stopped the error.

I imagine the compiler is saying: you have a union with a non-trivial destructor so I want you to write your cleanup explicitly if you need some. But I could be wrong.

[zoecarver]

This LGTM. Not only to fix the LWG issue, but this is a great cleanup to this part of the codebase. So, thanks Louis.

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

39

Maybe this should be a libcxx test only? I see "libcxx" in the name, but maybe it should be moved to the "libcxx" test directory.

138

This test makes me feel really confident that there won't be any ABI issues. Thanks!

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

29

Fair enough. Makes sense.

[ldionne]

Use alternative implementation that's much simpler than [[no_unique_address]].

[ldionne]

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

39

It is a libc++ test only, since it has // REQUIRES: libc++. I've actually been thinking about not having separate directories for libcxx specific tests and other tests, since we have lit markup that can handle it.

I believe one of the reasons was to ease the life of MSVC and libstdc++ using our test suite, however they now run using lit and so they do have the features available.

[zoecarver]

libcxx/include/memory

2635

Alternatively, you could make __blob_ a std::aligned_storage.

2639

(This is a new comment, I'm not replying to an old comment, even though that's what it looks like.)

Isn't this UB? __blob_ is uninitialized when you call .first() on it in __get_alloc().

[ldionne]

libcxx/include/memory

2635

I used to love aligned_storage, and then I read http://open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1413r2.pdf. I'm not sure I agree entirely with the paper, but I've been moving away from the type since it seemed like there were efforts to deprecate it. I'm not sure where those efforts ended up, though.

2639

Yes, it is indeed UB (hopefully benign UB). I ran it through ubsan before updating the patch and it didn't complain, so I thought to myself "it's good enough". But you're entirely right, it *is* plain and simple UB.

The issue is that I haven't been able to find a way to implement it without that. Simply, what we want is this:

_Alloc* __get_alloc() _NOEXCEPT {
    std::ptrdiff_t __offset = _CompressedPair::__first_offset(); // how to compute this?
    _Alloc* __alloc = static_cast<_Alloc*>(&__blob_ + __offset);
    return __alloc;
}

I wasn't able to find an implementation that didn't actually require an object to be alive:

_LIBCPP_INLINE_VISIBILITY
static std::ptrdiff_t __first_offset() _NOEXCEPT {
    __compressed_pair __p; // also requires default-constructing, which doesn't work
    _T1 const* __first = _VSTD::addressof(__p.first());
    return static_cast<char const*>(__first) - static_cast<char const*>(&__p);
}

I tried something with offsetof inside __compressed_pair_elem, but I couldn't compute the offset of the second element of the compressed pair. I don't know whether it's impossible or whether I'm just not imaginative enough. If we can't think of something, I can go back to the no_unique_address implementation (it's just a bit more complicated, but it's correct AFAICT).

[zoecarver]

libcxx/include/memory

2639

I actually really liked the no_unique_address implementation. And I think at this point we only support (or should only support) compiler that have implemented no_unique_address all the way back to C++03.

That being said, there is a way to do what you're thinking. We discussed this earlier, but I don't think I did a good job explaining. Basically, here's the idea: __compressed_pair *always* inherits from two __compressed_pair_elems. Always. We can simply static_cast a __compressed_pair pointer to both of it's base classes. That will give you two pointers which you can use to do placement new on both members.

(I think that might still technically be UB, because you never actually construct the object, but it seems like better UB then calling a member on an uninitialized object, which seems fairly dangerous IMHO.)

[zoecarver]

libcxx/include/memory

2639

Here's a pseudo-code example:

_Alloc* __get_alloc() _NOEXCEPT {
    _CompressedPair *__as_pair = reinterpret_cast<_CompressedPair*>(__blob_);
    _CompressedPair::_Base1* __first = static_cast<_CompressedPair::_Base1*>(__as_pair);
    _Alloc *__alloc = reinterpret_cast<_Alloc*>(__first);
    return __alloc;
}

[ldionne]

Remove UB!

libcxx/include/memory

2639

Hmm, that's clever, and actually very simple. Do you agree to go forward with that implementation, then?

Working with you is very humbling :-).

[zoecarver]

This looks great. Thanks for spending so much time iterating on this patch!

libcxx/include/memory

2635

The reason I liked using it in libc++ is that it works in C++03 mode (and alignas / alignof didn't use to). But now that we don't need to use it, I'm on board with moving away.

Thanks for the paper link :)

2639

Yes, I'm happy to go forward with that.

Thanks, you're too kind :)

[ldionne]

CI is passing, I'm going to ship this.

[curdeius]

@ldionne, shouldn't the related bug be updated?
https://llvm.org/PR41900

[ldionne]

In D91201#2487683, @curdeius wrote:

@ldionne, shouldn't the related bug be updated?
https://llvm.org/PR41900

Yes, thanks for the reminder. Done now.

libcxx/include/memory

#endif
   _LIBCPP_INLINE_VISIBILITY reference __get() _NOEXCEPT { return *this; }
   _LIBCPP_INLINE_VISIBILITY
   const_reference __get() const _NOEXCEPT { return *this; }
 };
 
 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
   //  object and the allocator have the same type).
   static_assert((!is_same<_T1, _T2>::value),
     "__compressed_pair cannot be instantiated when T1 and T2 are the same type; "
     "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 &&
           __dependent_type<is_default_constructible<_T2>, _Dummy>::value
       >::type
   >
   _LIBCPP_INLINE_VISIBILITY
   _LIBCPP_CONSTEXPR __compressed_pair() : _Base1(__value_init_tag()), _Base2(__value_init_tag()) {}
typename _Base2::reference second() _NOEXCEPT {
     return static_cast<_Base2&>(*this).__get();
   }
 
   _LIBCPP_INLINE_VISIBILITY
   typename _Base2::const_reference second() const _NOEXCEPT {
     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 &&
                __is_nothrow_swappable<_T2>::value)
   {
     using _VSTD::swap;
     swap(first(), __x.first());
     swap(second(), __x.second());
__shared_ptr_pointer<_Tp, _Dp, _Alloc>::__on_zero_shared_weak() _NOEXCEPT
     __data_.second().~_Alloc();
     __a.deallocate(_PTraits::pointer_to(*this), 1);
 }
 
 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;
 template <>
 class _LIBCPP_TEMPLATE_VIS allocator<__shared_ptr_dummy_rebind_allocator_type>
 {
 public:
     template <class _Other>
shared_ptr<_Tp> allocate_shared(const _Alloc& __a, _Args&& ...__args)
     auto __control_block = __guard.__release_ptr();
     return shared_ptr<_Tp>::__create_with_control_block((*__control_block).__get_elem(), _VSTD::addressof(*__control_block));
 }
 
 template<class _Tp, class ..._Args, class = _EnableIf<!is_array<_Tp>::value> >
 _LIBCPP_HIDE_FROM_ABI
 shared_ptr<_Tp> make_shared(_Args&& ...__args)
 {
     return _VSTD::allocate_shared<_Tp>(allocator<_Tp>(), _VSTD::forward<_Args>(__args)...);
 }
 
 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/std/utilities/memory/util.smartptr/util.smartptr.shared/libcxx.control_block_layout.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
+//
+//===----------------------------------------------------------------------===//
+
+// 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 <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::ptrdiff_t old_offset = old_elem - reinterpret_cast<char const*>(&old);
+    std::ptrdiff_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::ptrdiff_t old_offset = old_alloc - reinterpret_cast<char const*>(&old);
+    std::ptrdiff_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;
+}

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

struct Three
     int value;
     Three(int v, int, int) : value(v) {++count;}
     Three(Three const & o) : value(o.value) {++count;}
     ~Three() {--count;}
 };
 
 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()
 {
     int const bad = -1;
     {
     std::shared_ptr<Zero> p = std::allocate_shared<Zero>(Alloc());
     assert(Zero::count == 1);
     }
int main(int, char**)
     char c = 'f';
     std::shared_ptr<A> p = std::allocate_shared<A>(bare_allocator<void>(), i, c);
     assert(A::count == 1);
     assert(p->get_int() == 68);
     assert(p->get_char() == 'f');
     }
     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;
 }

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
+//
+//===----------------------------------------------------------------------===//
+
+// 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::ptrdiff_t n) {
+    return pointer(static_cast<T*>(::operator new(n * sizeof(T))));
+  }
+
+  void deallocate(pointer p, std::ptrdiff_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;
+}