[libc++] Avoid type-punning between __value_type and pair

[philnik777]

Before this patch, we were dereferencing pointers to objects which were never constructed. Now we always assume that nodes store pair<const KeyT, ValueT> for maps instead, as they actually do. This patch also allows for significant follow-up simplifications, since __node_value_type and __container_value_type are the same type now.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[frederick-vs-ja]

Hmm, it seems that core language UB can't be avoided in the current strategy (for which I submitted CWG2514). We need to do different operations in constant evaluation. (No change for this is requested for this PR.)

Now this PR assumes that pair<K, V> and pair<const K, V> have the same layout, which is not guaranteed - especially when there's a user-defined pair specialization. Is it possible to use less scary const_cast?

[philnik777]

We already assume that. No, it's not. We're type-punning and not just playing with constness.

[philnik777]

Sorry, I might have misunderstood you. Are you suggesting that I should assign the values individually instead of doing the reinterpret_cast?

[frederick-vs-ja]

Sorry, I might have misunderstood you. Are you suggesting that I should assign the values individually instead of doing the reinterpret_cast?

Yes. I'm afraid this might be necessary somewhere. It seems that the assumption about layout sameness can only be made when node handles are involved (per [container.node.overview]/4).

Given that MSVC STL is doing similar reinterpret_cast in assignment operators of unordered associative containers, do we want an LWG issue to allow the assumption to be made elsewhere?

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Nikolas Klauser (philnik777)

Changes

---

Patch is 29.43 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/134819.diff

7 Files Affected:

• (modified) libcxx/include/__fwd/pair.h (+6)
• (modified) libcxx/include/__memory/uses_allocator_construction.h (+1-8)
• (modified) libcxx/include/__node_handle (+28-4)
• (modified) libcxx/include/__tree (+56-41)
• (modified) libcxx/include/map (+31-94)
• (modified) libcxx/test/libcxx/containers/associative/tree_key_value_traits.pass.cpp (-4)
• (modified) libcxx/utils/gdb/libcxx/printers.py (+2-2)

diff --git a/libcxx/include/__fwd/pair.h b/libcxx/include/__fwd/pair.h
index ea81a81ef8e11..cf07eabab6903 100644
--- a/libcxx/include/__fwd/pair.h
+++ b/libcxx/include/__fwd/pair.h
@@ -22,6 +22,12 @@ _LIBCPP_BEGIN_NAMESPACE_STD
 template <class, class>
 struct pair;
 
+template <class _Type>
+inline const bool __is_pair_v = false;
+
+template <class _Type1, class _Type2>
+inline const bool __is_pair_v<pair<_Type1, _Type2> > = true;
+
 template <size_t _Ip, class _T1, class _T2>
 _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 typename tuple_element<_Ip, pair<_T1, _T2> >::type&
 get(pair<_T1, _T2>&) _NOEXCEPT;
diff --git a/libcxx/include/__memory/uses_allocator_construction.h b/libcxx/include/__memory/uses_allocator_construction.h
index 955879ffc5845..49ddf99d9cc95 100644
--- a/libcxx/include/__memory/uses_allocator_construction.h
+++ b/libcxx/include/__memory/uses_allocator_construction.h
@@ -14,7 +14,6 @@
 #include <__memory/uses_allocator.h>
 #include <__tuple/tuple_like_no_subrange.h>
 #include <__type_traits/enable_if.h>
-#include <__type_traits/is_same.h>
 #include <__type_traits/remove_cv.h>
 #include <__utility/declval.h>
 #include <__utility/pair.h>
@@ -31,14 +30,8 @@ _LIBCPP_BEGIN_NAMESPACE_STD
 
 #if _LIBCPP_STD_VER >= 17
 
-template <class _Type>
-inline constexpr bool __is_std_pair = false;
-
-template <class _Type1, class _Type2>
-inline constexpr bool __is_std_pair<pair<_Type1, _Type2>> = true;
-
 template <class _Tp>
-inline constexpr bool __is_cv_std_pair = __is_std_pair<remove_cv_t<_Tp>>;
+inline constexpr bool __is_cv_std_pair = __is_pair_v<remove_cv_t<_Tp>>;
 
 template <class _Tp, class = void>
 struct __uses_allocator_construction_args;
diff --git a/libcxx/include/__node_handle b/libcxx/include/__node_handle
index 08c4ffa5ff17b..5c559c657ef50 100644
--- a/libcxx/include/__node_handle
+++ b/libcxx/include/__node_handle
@@ -62,6 +62,7 @@ public:
 #include <__config>
 #include <__memory/allocator_traits.h>
 #include <__memory/pointer_traits.h>
+#include <__type_traits/is_specialization.h>
 #include <optional>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
@@ -173,17 +174,40 @@ struct __set_node_handle_specifics {
   _LIBCPP_HIDE_FROM_ABI value_type& value() const { return static_cast<_Derived const*>(this)->__ptr_->__get_value(); }
 };
 
+template <class, class>
+struct __hash_value_type;
+
 template <class _NodeType, class _Derived>
 struct __map_node_handle_specifics {
-  typedef typename _NodeType::__node_value_type::key_type key_type;
-  typedef typename _NodeType::__node_value_type::mapped_type mapped_type;
+  template <class _Tp>
+  struct __get_type {
+    using key_type    = __remove_const_t<typename _Tp::first_type>;
+    using mapped_type = typename _Tp::second_type;
+  };
+
+  template <class _Key, class _Mapped>
+  struct __get_type<__hash_value_type<_Key, _Mapped> > {
+    using key_type    = _Key;
+    using mapped_type = _Mapped;
+  };
+
+  using key_type    = typename __get_type<typename _NodeType::__node_value_type>::key_type;
+  using mapped_type = typename __get_type<typename _NodeType::__node_value_type>::mapped_type;
 
   _LIBCPP_HIDE_FROM_ABI key_type& key() const {
-    return static_cast<_Derived const*>(this)->__ptr_->__get_value().__ref().first;
+    if constexpr (__is_specialization_v<typename _NodeType::__node_value_type, __hash_value_type>) {
+      return static_cast<_Derived const*>(this)->__ptr_->__get_value().__ref().first;
+    } else {
+      return const_cast<key_type&>(static_cast<_Derived const*>(this)->__ptr_->__get_value().first);
+    }
   }
 
   _LIBCPP_HIDE_FROM_ABI mapped_type& mapped() const {
-    return static_cast<_Derived const*>(this)->__ptr_->__get_value().__ref().second;
+    if constexpr (__is_specialization_v<typename _NodeType::__node_value_type, __hash_value_type>) {
+      return static_cast<_Derived const*>(this)->__ptr_->__get_value().__ref().second;
+    } else {
+      return static_cast<_Derived const*>(this)->__ptr_->__get_value().second;
+    }
   }
 };
 
diff --git a/libcxx/include/__tree b/libcxx/include/__tree
index e84bc4ffda0bd..4275133f8a351 100644
--- a/libcxx/include/__tree
+++ b/libcxx/include/__tree
@@ -14,6 +14,7 @@
 #include <__assert>
 #include <__config>
 #include <__fwd/map.h>
+#include <__fwd/pair.h>
 #include <__fwd/set.h>
 #include <__iterator/distance.h>
 #include <__iterator/iterator_traits.h>
@@ -25,6 +26,7 @@
 #include <__memory/swap_allocator.h>
 #include <__memory/unique_ptr.h>
 #include <__type_traits/can_extract_key.h>
+#include <__type_traits/copy_cvref.h>
 #include <__type_traits/enable_if.h>
 #include <__type_traits/invoke.h>
 #include <__type_traits/is_const.h>
@@ -505,48 +507,24 @@ struct __is_tree_value_type<_One> : __is_tree_value_type_imp<__remove_cvref_t<_O
 template <class _Tp>
 struct __tree_key_value_types {
   typedef _Tp key_type;
-  typedef _Tp __node_value_type;
   typedef _Tp __container_value_type;
   static const bool __is_map = false;
 
   _LIBCPP_HIDE_FROM_ABI static key_type const& __get_key(_Tp const& __v) { return __v; }
-  _LIBCPP_HIDE_FROM_ABI static __container_value_type const& __get_value(__node_value_type const& __v) { return __v; }
-  _LIBCPP_HIDE_FROM_ABI static __container_value_type* __get_ptr(__node_value_type& __n) { return std::addressof(__n); }
-  _LIBCPP_HIDE_FROM_ABI static __container_value_type&& __move(__node_value_type& __v) { return std::move(__v); }
 };
 
 template <class _Key, class _Tp>
 struct __tree_key_value_types<__value_type<_Key, _Tp> > {
   typedef _Key key_type;
   typedef _Tp mapped_type;
-  typedef __value_type<_Key, _Tp> __node_value_type;
   typedef pair<const _Key, _Tp> __container_value_type;
   typedef __container_value_type __map_value_type;
   static const bool __is_map = true;
 
-  _LIBCPP_HIDE_FROM_ABI static key_type const& __get_key(__node_value_type const& __t) {
-    return __t.__get_value().first;
-  }
-
   template <class _Up, __enable_if_t<__is_same_uncvref<_Up, __container_value_type>::value, int> = 0>
   _LIBCPP_HIDE_FROM_ABI static key_type const& __get_key(_Up& __t) {
     return __t.first;
   }
-
-  _LIBCPP_HIDE_FROM_ABI static __container_value_type const& __get_value(__node_value_type const& __t) {
-    return __t.__get_value();
-  }
-
-  template <class _Up, __enable_if_t<__is_same_uncvref<_Up, __container_value_type>::value, int> = 0>
-  _LIBCPP_HIDE_FROM_ABI static __container_value_type const& __get_value(_Up& __t) {
-    return __t;
-  }
-
-  _LIBCPP_HIDE_FROM_ABI static __container_value_type* __get_ptr(__node_value_type& __n) {
-    return std::addressof(__n.__get_value());
-  }
-
-  _LIBCPP_HIDE_FROM_ABI static pair<key_type&&, mapped_type&&> __move(__node_value_type& __v) { return __v.__move(); }
 };
 
 template <class _VoidPtr>
@@ -587,6 +565,19 @@ struct __tree_map_pointer_types<_Tp, _AllocPtr, _KVTypes, true> {
   typedef __rebind_pointer_t<_AllocPtr, const _Mv> __const_map_value_type_pointer;
 };
 
+template <class _Tp>
+struct __get_node_value_type {
+  using type _LIBCPP_NODEBUG = _Tp;
+};
+
+template <class _Key, class _ValueT>
+struct __get_node_value_type<__value_type<_Key, _ValueT> > {
+  using type _LIBCPP_NODEBUG = pair<const _Key, _ValueT>;
+};
+
+template <class _Tp>
+using __get_node_value_type_t _LIBCPP_NODEBUG = typename __get_node_value_type<_Tp>::type;
+
 template <class _NodePtr, class _NodeT = typename pointer_traits<_NodePtr>::element_type>
 struct __tree_node_types;
 
@@ -601,7 +592,7 @@ public:
   typedef typename pointer_traits<_NodePtr>::element_type __node_type;
   typedef _NodePtr __node_pointer;
 
-  typedef _Tp __node_value_type;
+  using __node_value_type _LIBCPP_NODEBUG = __get_node_value_type_t<_Tp>;
   typedef __rebind_pointer_t<_VoidPtr, __node_value_type> __node_value_type_pointer;
   typedef __rebind_pointer_t<_VoidPtr, const __node_value_type> __const_node_value_type_pointer;
   typedef typename __base::__end_node_pointer __iter_pointer;
@@ -653,11 +644,11 @@ public:
 template <class _Tp, class _VoidPtr>
 class _LIBCPP_STANDALONE_DEBUG __tree_node : public __tree_node_base<_VoidPtr> {
 public:
-  typedef _Tp __node_value_type;
+  using __node_value_type _LIBCPP_NODEBUG = __get_node_value_type_t<_Tp>;
 
   __node_value_type __value_;
 
-  _LIBCPP_HIDE_FROM_ABI _Tp& __get_value() { return __value_; }
+  _LIBCPP_HIDE_FROM_ABI __node_value_type& __get_value() { return __value_; }
 
   ~__tree_node()                             = delete;
   __tree_node(__tree_node const&)            = delete;
@@ -688,7 +679,7 @@ public:
 
   _LIBCPP_HIDE_FROM_ABI void operator()(pointer __p) _NOEXCEPT {
     if (__value_constructed)
-      __alloc_traits::destroy(__na_, _NodeTypes::__get_ptr(__p->__value_));
+      __alloc_traits::destroy(__na_, std::addressof(__p->__value_));
     if (__p)
       __alloc_traits::deallocate(__na_, __p, 1);
   }
@@ -719,7 +710,7 @@ class __tree_iterator {
 
 public:
   typedef bidirectional_iterator_tag iterator_category;
-  typedef _Tp value_type;
+  using value_type = __get_node_value_type_t<_Tp>;
   typedef _DiffType difference_type;
   typedef value_type& reference;
   typedef typename _NodeTypes::__node_value_type_pointer pointer;
@@ -796,7 +787,7 @@ class __tree_const_iterator {
 
 public:
   typedef bidirectional_iterator_tag iterator_category;
-  typedef _Tp value_type;
+  using value_type = __get_node_value_type_t<_Tp>;
   typedef _DiffType difference_type;
   typedef const value_type& reference;
   typedef typename _NodeTypes::__const_node_value_type_pointer pointer;
@@ -809,7 +800,7 @@ public:
   }
 
 private:
-  typedef __tree_iterator<value_type, __node_pointer, difference_type> __non_const_iterator;
+  typedef __tree_iterator<_Tp, __node_pointer, difference_type> __non_const_iterator;
 
 public:
   _LIBCPP_HIDE_FROM_ABI __tree_const_iterator(__non_const_iterator __p) _NOEXCEPT : __ptr_(__p.__ptr_) {}
@@ -1135,6 +1126,17 @@ public:
     return __emplace_hint_multi(__p, std::forward<_Vp>(__v));
   }
 
+  template <class _ValueT = _Tp, __enable_if_t<__is_tree_value_type<_ValueT>::value, int> = 0>
+  _LIBCPP_HIDE_FROM_ABI void __insert_from_orphaned_node(const_iterator __p, __get_node_value_type_t<_Tp>&& __value) {
+    using __key_type = typename _NodeTypes::key_type;
+    __emplace_hint_multi(__p, const_cast<__key_type&&>(__value.first), std::move(__value.second));
+  }
+
+  template <class _ValueT = _Tp, __enable_if_t<!__is_tree_value_type<_ValueT>::value, int> = 0>
+  _LIBCPP_HIDE_FROM_ABI void __insert_from_orphaned_node(const_iterator __p, _Tp&& __value) {
+    __emplace_hint_multi(__p, std::move(__value));
+  }
+
   _LIBCPP_HIDE_FROM_ABI pair<iterator, bool>
   __node_assign_unique(const __container_value_type& __v, __node_pointer __dest);
 
@@ -1276,6 +1278,19 @@ private:
   }
   _LIBCPP_HIDE_FROM_ABI void __move_assign_alloc(__tree&, false_type) _NOEXCEPT {}
 
+  template <class _From, __enable_if_t<__is_pair_v<__remove_cvref_t<_From> >, int> = 0>
+  _LIBCPP_HIDE_FROM_ABI static void __assign_value(__get_node_value_type_t<value_type>& __lhs, _From&& __rhs) {
+    using __key_type = typename _NodeTypes::key_type;
+
+    const_cast<__key_type&>(__lhs.first) = const_cast<__copy_cvref_t<_From, __key_type>&&>(__rhs.first);
+    __lhs.second                         = std::forward<_From>(__rhs).second;
+  }
+
+  template <class _To, class _From, class _ValueT = _Tp, __enable_if_t<!__is_pair_v<__remove_cvref_t<_From> >, int> = 0>
+  _LIBCPP_HIDE_FROM_ABI static void __assign_value(_To& __lhs, _From&& __rhs) {
+    __lhs = std::forward<_From>(__rhs);
+  }
+
   struct _DetachedTreeCache {
     _LIBCPP_HIDE_FROM_ABI explicit _DetachedTreeCache(__tree* __t) _NOEXCEPT
         : __t_(__t),
@@ -1416,13 +1431,13 @@ void __tree<_Tp, _Compare, _Allocator>::__assign_multi(_InputIterator __first, _
   if (size() != 0) {
     _DetachedTreeCache __cache(this);
     for (; __cache.__get() && __first != __last; ++__first) {
-      __cache.__get()->__value_ = *__first;
+      __assign_value(__cache.__get()->__value_, *__first);
       __node_insert_multi(__cache.__get());
       __cache.__advance();
     }
   }
   for (; __first != __last; ++__first)
-    __insert_multi(_NodeTypes::__get_value(*__first));
+    __insert_multi(*__first);
 }
 
 template <class _Tp, class _Compare, class _Allocator>
@@ -1501,13 +1516,14 @@ void __tree<_Tp, _Compare, _Allocator>::__move_assign(__tree& __t, false_type) {
     if (size() != 0) {
       _DetachedTreeCache __cache(this);
       while (__cache.__get() != nullptr && __t.size() != 0) {
-        __cache.__get()->__value_ = std::move(__t.remove(__t.begin())->__value_);
+        __assign_value(__cache.__get()->__value_, std::move(__t.remove(__t.begin())->__value_));
         __node_insert_multi(__cache.__get());
         __cache.__advance();
       }
     }
-    while (__t.size() != 0)
-      __insert_multi(__e, _NodeTypes::__move(__t.remove(__t.begin())->__value_));
+    while (__t.size() != 0) {
+      __insert_from_orphaned_node(__e, std::move(__t.remove(__t.begin())->__value_));
+    }
   }
 }
 
@@ -1533,7 +1549,7 @@ void __tree<_Tp, _Compare, _Allocator>::destroy(__node_pointer __nd) _NOEXCEPT {
     destroy(static_cast<__node_pointer>(__nd->__left_));
     destroy(static_cast<__node_pointer>(__nd->__right_));
     __node_allocator& __na = __node_alloc();
-    __node_traits::destroy(__na, _NodeTypes::__get_ptr(__nd->__value_));
+    __node_traits::destroy(__na, std::addressof(__nd->__value_));
     __node_traits::deallocate(__na, __nd, 1);
   }
 }
@@ -1803,10 +1819,9 @@ template <class _Tp, class _Compare, class _Allocator>
 template <class... _Args>
 typename __tree<_Tp, _Compare, _Allocator>::__node_holder
 __tree<_Tp, _Compare, _Allocator>::__construct_node(_Args&&... __args) {
-  static_assert(!__is_tree_value_type<_Args...>::value, "Cannot construct from __value_type");
   __node_allocator& __na = __node_alloc();
   __node_holder __h(__node_traits::allocate(__na, 1), _Dp(__na));
-  __node_traits::construct(__na, _NodeTypes::__get_ptr(__h->__value_), std::forward<_Args>(__args)...);
+  __node_traits::construct(__na, std::addressof(__h->__value_), std::forward<_Args>(__args)...);
   __h.get_deleter().__value_constructed = true;
   return __h;
 }
@@ -1874,7 +1889,7 @@ __tree<_Tp, _Compare, _Allocator>::__node_assign_unique(const __container_value_
   __node_pointer __r           = static_cast<__node_pointer>(__child);
   bool __inserted              = false;
   if (__child == nullptr) {
-    __nd->__value_ = __v;
+    __assign_value(__nd->__value_, __v);
     __insert_node_at(__parent, __child, static_cast<__node_base_pointer>(__nd));
     __r        = __nd;
     __inserted = true;
@@ -2036,7 +2051,7 @@ typename __tree<_Tp, _Compare, _Allocator>::iterator __tree<_Tp, _Compare, _Allo
   __node_pointer __np    = __p.__get_np();
   iterator __r           = __remove_node_pointer(__np);
   __node_allocator& __na = __node_alloc();
-  __node_traits::destroy(__na, _NodeTypes::__get_ptr(const_cast<__node_value_type&>(*__p)));
+  __node_traits::destroy(__na, std::addressof(const_cast<__node_value_type&>(*__p)));
   __node_traits::deallocate(__na, __np, 1);
   return __r;
 }
diff --git a/libcxx/include/map b/libcxx/include/map
index a244696295fb8..1821aa0155a29 100644
--- a/libcxx/include/map
+++ b/libcxx/include/map
@@ -593,7 +593,6 @@ erase_if(multimap<Key, T, Compare, Allocator>& c, Predicate pred);  // C++20
 #  include <__memory/pointer_traits.h>
 #  include <__memory/unique_ptr.h>
 #  include <__memory_resource/polymorphic_allocator.h>
-#  include <__new/launder.h>
 #  include <__node_handle>
 #  include <__ranges/concepts.h>
 #  include <__ranges/container_compatible_range.h>
@@ -645,13 +644,13 @@ public:
       : _Compare(__c) {}
   _LIBCPP_HIDE_FROM_ABI const _Compare& key_comp() const _NOEXCEPT { return *this; }
   _LIBCPP_HIDE_FROM_ABI bool operator()(const _CP& __x, const _CP& __y) const {
-    return static_cast<const _Compare&>(*this)(__x.__get_value().first, __y.__get_value().first);
+    return static_cast<const _Compare&>(*this)(__x.first, __y.first);
   }
   _LIBCPP_HIDE_FROM_ABI bool operator()(const _CP& __x, const _Key& __y) const {
-    return static_cast<const _Compare&>(*this)(__x.__get_value().first, __y);
+    return static_cast<const _Compare&>(*this)(__x.first, __y);
   }
   _LIBCPP_HIDE_FROM_ABI bool operator()(const _Key& __x, const _CP& __y) const {
-    return static_cast<const _Compare&>(*this)(__x, __y.__get_value().first);
+    return static_cast<const _Compare&>(*this)(__x, __y.first);
   }
   _LIBCPP_HIDE_FROM_ABI void swap(__map_value_compare& __y) _NOEXCEPT_(__is_nothrow_swappable_v<_Compare>) {
     using std::swap;
@@ -661,12 +660,12 @@ public:
 #  if _LIBCPP_STD_VER >= 14
   template <typename _K2>
   _LIBCPP_HIDE_FROM_ABI bool operator()(const _K2& __x, const _CP& __y) const {
-    return static_cast<const _Compare&>(*this)(__x, __y.__get_value().first);
+    return static_cast<const _Compare&>(*this)(__x, __y.first);
   }
 
   template <typename _K2>
   _LIBCPP_HIDE_FROM_ABI bool operator()(const _CP& __x, const _K2& __y) const {
-    return static_cast<const _Compare&>(*this)(__x.__get_value().first, __y);
+    return static_cast<const _Compare&>(*this)(__x.first, __y);
   }
 #  endif
 };
@@ -682,15 +681,9 @@ public:
       : __comp_(__c) {}
   _LIBCPP_HIDE_FROM_ABI const _Compare& key_comp() const _NOEXCEPT { return __comp_; }
 
-  _LIBCPP_HIDE_FROM_ABI bool operator()(const _CP& __x, const _CP& __y) const {
-    return __comp_(__x.__get_value().first, __y.__get_value().first);
-  }
-  _LIBCPP_HIDE_FROM_ABI bool operator()(const _CP& __x, const _Key& __y) const {
-    return __comp_(__x.__get_value().first, __y);
-  }
-  _LIBCPP_HIDE_FROM_ABI bool operator()(const _Key& __x, const _CP& __y) const {
-    return __comp_(__x, __y.__get_value().first);
-  }
+  _LIBCPP_HIDE_FROM_ABI bool operator()(const _CP& __x, const _CP& __y) const { return __comp_(__x.first, __y.first); }
+  _LIBCPP_HIDE_FROM_ABI bool operator()(const _CP& __x, const _Key& __y) const { return __comp_(__x.first, __y); }
+  _LIBCPP_HIDE_FROM_ABI bool operator()(const _Key& __x, const _CP& __y) const { return __comp_(__x, __y.first); }
   void swap(__map_value_compare& __y) _NOEXCEPT_(__is_nothrow_swappable_v<_Compare>) {
     using std::swap;
     swap(__comp_, __y.__comp_);
@@ -749,9 +742,9 @@ public:
 
   _LIBCPP_HIDE_FROM_ABI void operator()(pointer __p) _NOEXCEPT {
     if (__second_constructed)
-      __alloc_traits::destroy(__na_, std::addressof(__p->__value_.__get_value().second));
+      __alloc_traits::destroy(__na_, std::addressof(__p->__value_.second));
     if (__first_constructed)
-      __alloc_traits::destroy(__na_, std::addressof(__p->__value_.__get_value().first));
+      __alloc_traits::destroy(__na_, std::addressof(__p->__value_.first));
     if (__p)
       __alloc_traits::deallocate(__na_, __p, 1);
   }
@@ -763,64 +756,7 @@ class __map_const_iterator;
 #  ifndef _LIBCPP_CXX03_LANG
 
 template <class _Key, class _Tp>
-struct _LIBCPP_STANDALONE_DEBUG __value_type {
-  typedef _Key key_type;
-  typedef _Tp mapped_type;
-  typedef pair<const key_type, mapped_type> value_type;
-  typedef pair<key_type&, mapped_type&> __nc_ref_pair_type;
-  typedef pair<key_type&&, mapped_type&&> __nc_rref_pair_type;
-
-private:
-  value_type __cc_;
-
-public:
-  _LIBCPP_HIDE_FROM_ABI value_type& __get_value() {
-#    if _LIBCPP_STD_VER >= 17
-    return *std::launder(std::addressof(__cc_));
-#    else
-    return __cc_;
-#    endif
-  }
-
-  _LIBCPP_HIDE_FROM_ABI const value_type& __get_value() const {
-#    if _LIBCPP_STD_VER >= 17
-    return *std::launder(std::addressof(__cc_));
-#    else
-    return __cc_;
-#    endif
-  }
-
-  _LIBCPP_HIDE_FROM_ABI __nc_ref_pair_type __ref() {
-    value_type& __v = __get_value();
-    return __nc_ref_pair_type(const_cast<key_type&>(__v.first), __v.second);
-  }
-
-  _LIBCPP_HIDE_FROM_ABI __nc_rref_pair_type __move() {
-    value_type& __v = __get_value();
-    return __nc_rref_pair_type(std::move(const_cast<key_type&>(__v.first)), std::move(__v.second));
-  }
-
-  _LIBCPP_HIDE_FROM_ABI __value_type& operator=(const __value_type& __v) {
-    __ref() = __v.__get_value();
-    return *this;
-  }
-
-  _LIBCPP_HIDE_FROM_ABI __value_type& operator=(__value_type&& __v) {
-    __ref() = __v.__move();
-    return *this;
-  }
-
-  template <class _ValueTp, __enable_if_t<__is_same_uncvref<_ValueTp, value_type>::value, int> = 0>
-  _LIBCPP_HIDE_FROM_ABI __value_type& ...
[truncated]

[EricWF]

Is there code that's being miscompiled because of our implementation?
Or is this just a cleanup?

[EricWF]

Do you have any plans for testing the if this change reduces or increases the amount of template instantiations / code bloat produced by the container?

[ldionne]

I'd like to inline those functions or at least move them closer to their only point of use.

[philnik777]

This is now used in map as well to avoid a reinterpret_cast there.

[mstorsjo]

This broke building a number of real world projects. The smallest reduced example I have is this:

#include <map>
void insert(std::map<int, int>& m, int key, int val) { m.insert_or_assign(m.end(), key, val); }

$ clang -target x86_64-w64-mingw32 -c stdmap.cpp 
In file included from stdmap.cpp:1:
/home/martin/clang-nightly/x86_64-w64-mingw32/include/c++/v1/map:1208:12: error: no member named '__get_value' in 'std::pair<const int, int>'
 1208 |       __r->__get_value().second = std::forward<_Vp>(__v);
      |       ~~~~~^
stdmap.cpp:2:58: note: in instantiation of function template specialization 'std::map<int, int>::insert_or_assign<int &>' requested here
    2 | void insert(std::map<int, int>& m, int key, int val) { m.insert_or_assign(m.end(), key, val); }
      |                                                          ^
1 error generated.

I ran into one case of this in Qt and one in libaribcaption.

I also ran into errors in building google protobuf, where I haven't reduced the case quite as well. I get the following errors:

$ clang -target i686-w64-mingw32 -c -x c++ ../src/google/protobuf/map_field.h -I../src
In file included from ../src/google/protobuf/map_field.h:43:
In file included from ../src/google/protobuf/map_entry.h:36:
In file included from ../src/google/protobuf/map_entry_lite.h:46:
../src/google/protobuf/map.h:124:17: error: implicit instantiation of undefined template 'std::__value_type<std::reference_wrapper<const google::protobuf::MapKey>, void *>'
  124 |   static_assert(alignof(value_type) <= 8, "");
      |                 ^
/home/martin/clang-nightly/i686-w64-mingw32/include/c++/v1/__memory/allocator_traits.h:229:59: note: in instantiation of template class 'google::protobuf::internal::MapAllocator<std::__value_type<std::reference_wrapper<const google::protobuf::MapKey>, void *>>' requested here
  229 |   using value_type                             = typename allocator_type::value_type;
      |                                                           ^
/home/martin/clang-nightly/i686-w64-mingw32/include/c++/v1/__tree:868:64: note: in instantiation of template class 'std::allocator_traits<google::protobuf::internal::MapAllocator<std::__value_type<std::reference_wrapper<const google::protobuf::MapKey>, void *>>>' requested here
  868 |   typedef typename __make_tree_node_types<value_type, typename __alloc_traits::void_pointer>::type _NodeTypes;
      |                                                                ^
/home/martin/clang-nightly/i686-w64-mingw32/include/c++/v1/map:916:20: note: in instantiation of template class 'std::__tree<std::__value_type<std::reference_wrapper<const google::protobuf::MapKey>, void *>, std::__map_value_compare<std::reference_wrapper<const google::protobuf::MapKey>, std::pair<const std::reference_wrapper<const google::protobuf::MapKey>, void *>, std::less<google::protobuf::MapKey>>, google::protobuf::internal::MapAllocator<std::__value_type<std::reference_wrapper<const google::protobuf::MapKey>, void *>>>' requested here
  916 |   typedef typename __base::__node_traits __node_traits;
      |                    ^
../src/google/protobuf/map.h:492:35: note: in instantiation of template class 'std::map<std::reference_wrapper<const google::protobuf::MapKey>, void *, std::less<google::protobuf::MapKey>, google::protobuf::internal::MapAllocator<std::pair<const std::reference_wrapper<const google::protobuf::MapKey>, void *>>>' requested here
  492 |     using TreeIterator = typename Tree::iterator;
      |                                   ^
../src/google/protobuf/map.h:1432:12: note: in instantiation of member class 'google::protobuf::Map<google::protobuf::MapKey, google::protobuf::MapValueRef>::InnerMap' requested here
 1432 |   InnerMap elements_;
      |            ^
../src/google/protobuf/map_field.h:521:12: note: in instantiation of template class 'google::protobuf::Map<google::protobuf::MapKey, google::protobuf::MapValueRef>' requested here
  521 |   typename Map<Key, T>::const_iterator& InternalGetIterator(
      |            ^
../src/google/protobuf/map_field.h:663:14: note: in instantiation of template class 'google::protobuf::internal::TypeDefinedMapFieldBase<google::protobuf::MapKey, google::protobuf::MapValueRef>' requested here
  663 |     : public TypeDefinedMapFieldBase<MapKey, MapValueRef> {
      |              ^
/home/martin/clang-nightly/i686-w64-mingw32/include/c++/v1/map:757:8: note: template is declared here
  757 | struct __value_type;
      |        ^
1 error generated.

[mstorsjo]

I see a commit being pushed somewhere, trying to fix this issue; that fixes the above smaller repro issue, but does not fix the bigger (unreduced) case with google protobuf.

[philnik777]

@mstorsjo #140225 should fix the protobuf case.

[mstorsjo]

@mstorsjo #140225 should fix the protobuf case.

Thanks, that does indeed seem to fix it! (However that one seems to stumble upon something else in CI.)

[cmtice]

I'm also seeing errors like:

libcxx/include/__tree:1292:5: error: const_cast from 'prototemplate::AttributeType_Type' to '__key_type &' (aka 'std::pair<prototemplate::AttributeType_Type, std::string> &') is not allowed
1292 | const_cast<__key_type&>(__lhs.first) = const_cast<__copy_cvref_t<_From, __key_type>&&>(__rhs.first);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
libcxx/include/__tree:1441:7: note: in instantiation of function template specialization 'std::__tree<std::pair<prototemplate::AttributeType_Type, std::string>, std::less<std::pair<prototemplate::AttributeType_Type, std::string>>, std::allocator<std::pair<prototemplate::AttributeType_Type, std::string>>>::__assign_value<const std::pair<prototemplate::AttributeType_Type, std::string> &, 0>' requested here
1441 | __assign_value(__cache.__get()->_value, *__first);
| ^
libcxx/include/__tree:1404:5: note: in instantiation of function template specialization 'std::__tree<std::pair<prototemplate::AttributeType_Type, std::string>, std::less<std::pair<prototemplate::AttributeType_Type, std::string>>, std::allocator<std::pair<prototemplate::AttributeType_Type, std::string>>>::__assign_multi<std::__tree_const_iterator<std::pair<prototemplate::AttributeType_Type, std::string>, std::__tree_node<std::pair<prototemplate::AttributeType_Type, std::string>, void *> *, long>>' requested here
1404 | __assign_multi(__t.begin(), __t.end());
| ^
libcxx/include/set:666:13: note: in instantiation of member function 'std::__tree<std::pair<prototemplate::AttributeType_Type, std::string>, std::less<std::pair<prototemplate::AttributeType_Type, std::string>>, std::allocator<std::pair<prototemplate::AttributeType_Type, std::string>>>::operator=' requested here
666 | _tree = __s._tree;
| ^

None of the fixes reference above fixes this issue.

[slackito]

The error referenced by @cmtice above should be reproducible with something like:

#include <set>
#include <utility>

void f() {
  std::set<std::pair<int, int>> s1;
  std::set<std::pair<int, int>> s2;

  s2 = s1;
}

The error message looks like this:

In file included from experimental/users/jgorbe/libcxx-repro/a.cc:1:
In file included from third_party/stl/cxx17/set:4:
In file included from third_party/crosstool/v18/llvm_unstable/src/libcxx/include/set:537:
third_party/crosstool/v18/llvm_unstable/src/libcxx/include/__tree:1292:5: error: const_cast from 'int' to '__key_type &' (aka 'std::pair<int, int> &') is not allowed
 1292 |     const_cast<__key_type&>(__lhs.first) = const_cast<__copy_cvref_t<_From, __key_type>&&>(__rhs.first);
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
third_party/crosstool/v18/llvm_unstable/src/libcxx/include/__tree:1441:7: note: in instantiation of function template specialization 'std::__tree<std::pair<int, int>, std::less<std::pair<int, int>>, std::allocator<std::pair<int, int>>>::__assign_value<const std::pair<int, int> &, 0>' requested here
 1441 |       __assign_value(__cache.__get()->__value_, *__first);
      |       ^
third_party/crosstool/v18/llvm_unstable/src/libcxx/include/__tree:1404:5: note: in instantiation of function template specialization 'std::__tree<std::pair<int, int>, std::less<std::pair<int, int>>, std::allocator<std::pair<int, int>>>::__assign_multi<std::__tree_const_iterator<std::pair<int, int>, std::__tree_node<std::pair<int, int>, void *> *, long>>' requested here
 1404 |     __assign_multi(__t.begin(), __t.end());
      |     ^
third_party/crosstool/v18/llvm_unstable/src/libcxx/include/set:666:13: note: in instantiation of member function 'std::__tree<std::pair<int, int>, std::less<std::pair<int, int>>, std::allocator<std::pair<int, int>>>::operator=' requested here
  666 |     __tree_ = __s.__tree_;
      |             ^
experimental/users/jgorbe/libcxx-repro/a.cc:8:6: note: in instantiation of member function 'std::set<std::pair<int, int>>::operator=' requested here
    8 |   s2 = s1;
      |      ^
1 error generated.

[philnik777]

@cmtice @slackito #140385 should fix that bug. I hope that's all the bugs I've introduced here...