[libc++] Use [[no_unique_address]] in __compressed_pair in ABIv2

[philnik]

• Fix CI

[Mordante]

I haven't looked at the patch closely. But I wonder whether we really want this.

For example a user uses the unstable ABI and uses std::unique_ptr in an interface of their own library.
When this library is used with other libraries that use different language versions, for example C++17 and C++20, the layout of std::unique_ptr may differ between these two libraries, causing ODR violations.
(Obviously this affects more types since __compressed_pair is used at a lot of places.

Unless somebody convinces me I'm mistaken regarding the ODR violations, I would suggest to abandon this patch.
(In that case let's discuss it further on Discord.)

[Mordante]

We've discussed it on Discord and it looks like it might be save. Once it's certain the reasoning why it's safe should be part of the summary so it becomes part of the commit message.

[ldionne]

libcxx/include/__memory/compressed_pair.h

164

I would like to better understand the benefit of making this change. What do we gain? At a glance, we seem to gain EBO for types marked final -- I think that's it, right?

I wonder whether it's worth adding complexity for that, but I'm not closed to the idea.

libcxx/include/__memory/shared_ptr.h

333

I don't think this part of the patch should be tied to the change in __compressed_pair. It's really not related -- the idea is only that if we're using the unstable ABI, we can suddenly simplify the implementation of the control block, but doing so has nothing to do with using [[no_unqiue_address]] inside __compressed_pair.

[philnik]

libcxx/include/__memory/compressed_pair.h

164

There are a few things:
(1) final types are compressed
(2) slightly faster compile times, because less templates have to be instantiated (I haven't measured)
(3) this change has to be made to allow using [[no_unique_address]] directly in the classes if the support for ABIv1 will ever be dropped

libcxx/include/__memory/shared_ptr.h

333

It kind-of has. The control-block is tied to the layout of the ABIv1 __compressed_pair. So the ABIv2 control block has to be used when enabling the ABIv2 __compressed_pair. Do you just want to have another macro or should I put in an error if _LIBCPP_ABI_COMPRESSED_PAIR_USE_NO_UNIQUE_ADDRESS is set but something like _LIBCPP_ABI_SHARED_PTR_SIMPLE_CONTROL_BLOCK is not set? I don't think there are any other _LIBCPP_ABI* macros that depend on another yet.

[ldionne]

I would like us to check whether https://reviews.llvm.org/D63744#2369890 can be implemented (especially since we now support recent compilers only, which wasn't the case back then). If that works, I think this would remove the need for this patch and provide a bunch of additional benefits even in the stable ABI. If not, then I think this patch is fine. Requesting changes for now.

libcxx/include/__memory/shared_ptr.h

333

Ahh, I see it now -- there is no __get_second_base function after your patch, so the control block implementation does need to change.

libcxx/include/__config

 // std::random_device, one needs to retain the same object layout to remain ABI
 // compatible. This switch removes these workarounds for platforms that don't care
 // about ABI compatibility.
 #  define _LIBCPP_ABI_NO_RANDOM_DEVICE_COMPATIBILITY_LAYOUT
 // Remove basic_string common base
 #  define _LIBCPP_ABI_DO_NOT_EXPORT_BASIC_STRING_COMMON
 // Remove vector base class
 #  define _LIBCPP_ABI_DO_NOT_EXPORT_VECTOR_BASE_COMMON
+// Use [[no_unique_address]] in compressed_pair
+#  define _LIBCPP_COMPRESSED_PAIR_USE_NO_UNIQUE_ADDRESS
 #elif _LIBCPP_ABI_VERSION == 1
 #  if !defined(_LIBCPP_OBJECT_FORMAT_COFF)
 // Enable compiling copies of now inline methods into the dylib to support
 // applications compiled against older libraries. This is unnecessary with
 // COFF dllexport semantics, since dllexport forces a non-inline definition
 // of inline functions to be emitted anyway. Our own non-inline copy would
 // conflict with the dllexport-emitted copy, so we disable it.
 #    define _LIBCPP_DEPRECATED_ABI_LEGACY_LIBRARY_DEFINITIONS_FOR_INLINE_FUNCTIONS

libcxx/include/__memory/compressed_pair.h

 #endif
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 // Tag used to default initialize one or both of the pair's elements.
 struct __default_init_tag {};
 struct __value_init_tag {};
 
+#ifndef _LIBCPP_COMPRESSED_PAIR_USE_NO_UNIQUE_ADDRESS
+
 template <class _Tp, int _Idx, bool _CanBeEmptyBase = is_empty<_Tp>::value && !__libcpp_is_final<_Tp>::value>
 struct __compressed_pair_elem {
   using _ParamT = _Tp;
   using reference = _Tp&;
   using const_reference = const _Tp&;
 
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR explicit __compressed_pair_elem(__default_init_tag) {}
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR explicit __compressed_pair_elem(__value_init_tag) : __value_() {}
template <bool _Dummy = true,
     class = __enable_if_t<
         __dependent_type<is_default_constructible<_T1>, _Dummy>::value &&
         __dependent_type<is_default_constructible<_T2>, _Dummy>::value
     >
   >
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR
   explicit __compressed_pair() : _Base1(__value_init_tag()), _Base2(__value_init_tag()) {}
 
+  // Use the default constructor instead
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR __compressed_pair(__value_init_tag, __value_init_tag) = delete;
+
   template <class _U1, class _U2>
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR
   explicit __compressed_pair(_U1&& __t1, _U2&& __t2) : _Base1(std::forward<_U1>(__t1)), _Base2(std::forward<_U2>(__t2)) {}
 
 #ifndef _LIBCPP_CXX03_LANG
   template <class... _Args1, class... _Args2>
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX14
   explicit __compressed_pair(piecewise_construct_t __pc, tuple<_Args1...> __first_args,
#endif
   void swap(__compressed_pair& __x)
       _NOEXCEPT_(__is_nothrow_swappable<_T1>::value && __is_nothrow_swappable<_T2>::value) {
     using std::swap;
     swap(first(), __x.first());
     swap(second(), __x.second());
   }
 };
 
+#else
+
+template <class _T1, class _T2>
+class __compressed_pair {
+  _LIBCPP_NO_UNIQUE_ADDRESS _T1 __first;
+  _LIBCPP_NO_UNIQUE_ADDRESS _T2 __second;
+public:
+
+  _LIBCPP_HIDE_FROM_ABI constexpr __compressed_pair() : __first(), __second() {}
+  _LIBCPP_HIDE_FROM_ABI constexpr __compressed_pair(__default_init_tag, __default_init_tag) {}
+  // Use the default constructor instead
+  _LIBCPP_HIDE_FROM_ABI constexpr __compressed_pair(__value_init_tag, __value_init_tag) = delete;
+
+  template <class _U1>
+  _LIBCPP_HIDE_FROM_ABI constexpr __compressed_pair(_U1&& __t1, __default_init_tag)
+      : __first(std::forward<_U1>(__t1)) {}
+
+  template <class _U2>
+  _LIBCPP_HIDE_FROM_ABI constexpr __compressed_pair(__default_init_tag, _U2&& __t2)
+      : __second(std::forward<_U2>(__t2)) {}
+
+  template <class _U1>
+  _LIBCPP_HIDE_FROM_ABI constexpr __compressed_pair(_U1&& __t1, __value_init_tag)
+      : __first(std::forward<_U1>(__t1)), __second() {}
+
+  template <class _U2>
+  _LIBCPP_HIDE_FROM_ABI constexpr __compressed_pair(__value_init_tag, _U2&& __t2)
+      : __first(), __second(std::forward<_U2>(__t2)) {}
+
+  template <class _U1, class _U2>
+  _LIBCPP_HIDE_FROM_ABI constexpr
+  __compressed_pair(_U1&& __t1, _U2&& __t2) : __first(std::forward<_U1>(__t1)), __second(std::forward<_U2>(__t2)) {}
+
+  template <class... _Args1, class... _Args2>
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX14
+  __compressed_pair(piecewise_construct_t, tuple<_Args1...> __first_args, tuple<_Args2...> __second_args)
+      : __first(
+          piecewise_construct_t{},
+          std::forward<_Args1>(std::get<typename __make_tuple_indices<sizeof...(_Args1)>::type>(__first_args))...),
+        __second(
+          piecewise_construct_t{},
+          std::forward<_Args2>(std::get<typename __make_tuple_indices<sizeof...(_Args2)>::type>(__second_args))...) {}
+
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX11 _T1& first()        noexcept { return __first; }
+  _LIBCPP_HIDE_FROM_ABI constexpr               const _T1& first()  const noexcept { return __first; }
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX11 _T2& second()       noexcept { return __second; }
+  _LIBCPP_HIDE_FROM_ABI constexpr               const _T2& second() const noexcept { return __second; }
+
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX11
+  void swap(__compressed_pair& __other)
+      noexcept(__is_nothrow_swappable<_T1>::value && __is_nothrow_swappable<_T2>::value) {
+    using std::swap;
+    swap(first(), __other.first());
+    swap(second(), __other.second());
+  }
+
+};
+
+#endif // _LIBCPP_COMPRESSED_PAIR_USE_NO_UNIQUE_ADDRESS
+
 template <class _T1, class _T2>
 inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX11
 void swap(__compressed_pair<_T1, _T2>& __x, __compressed_pair<_T1, _T2>& __y)
     _NOEXCEPT_(__is_nothrow_swappable<_T1>::value && __is_nothrow_swappable<_T2>::value) {
   __x.swap(__y);
 }
 
 _LIBCPP_END_NAMESPACE_STD
 
 #endif // _LIBCPP___MEMORY_COMPRESSED_PAIR_H

libcxx/include/__memory/shared_ptr.h

#endif
     // 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.
+#ifndef _LIBCPP_COMPRESSED_PAIR_USE_NO_UNIQUE_ADDRESS
     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() {
struct _ALIGNAS_TYPE(_CompressedPair) _Storage {
             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), "");
+#else
+    struct _Storage {
+        _LIBCPP_NO_UNIQUE_ADDRESS _Alloc __alloc;
+        _LIBCPP_NO_UNIQUE_ADDRESS union { _Tp __elem; };
+
+        _LIBCPP_HIDE_FROM_ABI explicit _Storage(_Alloc&& __a) : __alloc(std::move(__a)) {}
+        _LIBCPP_HIDE_FROM_ABI ~_Storage() {}
+
+        _Alloc* __get_alloc() noexcept { return std::addressof(__alloc); }
+        _Tp* __get_elem() noexcept { return std::addressof(__elem); }
+    };
+#endif
+
     _Storage __storage_;
 };
 
 struct __shared_ptr_dummy_rebind_allocator_type;
 template <>
 class _LIBCPP_TEMPLATE_VIS allocator<__shared_ptr_dummy_rebind_allocator_type>
 {
 public:

libcxx/include/__memory/unique_ptr.h

private:
   using _EnableIfDeleterAssignable = typename enable_if<
       is_assignable<_Dp&, _UDel&&>::value
     >::type;
 
 public:
   template <bool _Dummy = true,
             class = _EnableIfDeleterDefaultConstructible<_Dummy> >
   _LIBCPP_INLINE_VISIBILITY
-  _LIBCPP_CONSTEXPR unique_ptr() _NOEXCEPT : __ptr_(__value_init_tag(), __value_init_tag()) {}
+  _LIBCPP_CONSTEXPR unique_ptr() _NOEXCEPT : __ptr_() {}
 
   template <bool _Dummy = true,
             class = _EnableIfDeleterDefaultConstructible<_Dummy> >
   _LIBCPP_INLINE_VISIBILITY
-  _LIBCPP_CONSTEXPR unique_ptr(nullptr_t) _NOEXCEPT : __ptr_(__value_init_tag(), __value_init_tag()) {}
+  _LIBCPP_CONSTEXPR unique_ptr(nullptr_t) _NOEXCEPT : __ptr_() {}
 
   template <bool _Dummy = true,
             class = _EnableIfDeleterDefaultConstructible<_Dummy> >
   _LIBCPP_INLINE_VISIBILITY
   explicit unique_ptr(pointer __p) _NOEXCEPT : __ptr_(__p, __value_init_tag()) {}
 
   template <bool _Dummy = true,
             class = _EnableIfDeleterConstructible<_LValRefType<_Dummy> > >
private:
   using _EnableIfDeleterAssignable _LIBCPP_NODEBUG = typename enable_if<
       is_assignable<_Dp&, _UDel&&>::value
     >::type;
 
 public:
   template <bool _Dummy = true,
             class = _EnableIfDeleterDefaultConstructible<_Dummy> >
   _LIBCPP_INLINE_VISIBILITY
-  _LIBCPP_CONSTEXPR unique_ptr() _NOEXCEPT : __ptr_(__value_init_tag(), __value_init_tag()) {}
+  _LIBCPP_CONSTEXPR unique_ptr() _NOEXCEPT : __ptr_() {}
 
   template <bool _Dummy = true,
             class = _EnableIfDeleterDefaultConstructible<_Dummy> >
   _LIBCPP_INLINE_VISIBILITY
-  _LIBCPP_CONSTEXPR unique_ptr(nullptr_t) _NOEXCEPT : __ptr_(__value_init_tag(), __value_init_tag()) {}
+  _LIBCPP_CONSTEXPR unique_ptr(nullptr_t) _NOEXCEPT : __ptr_() {}
 
   template <class _Pp, bool _Dummy = true,
             class = _EnableIfDeleterDefaultConstructible<_Dummy>,
             class = _EnableIfPointerConvertible<_Pp> >
   _LIBCPP_INLINE_VISIBILITY
   explicit unique_ptr(_Pp __p) _NOEXCEPT
       : __ptr_(__p, __value_init_tag()) {}
 

libcxx/test/libcxx/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
+// UNSUPPORTED: libcpp-abi-version=2
 
 // 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.