[libc++] Avoid type-punning between __value_type and pair (#134819)

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.

Author: philnik777

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;

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;

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;
+    }
   }
 };
 

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;
 
@@ -599,7 +590,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;
 
@@ -650,11 +641,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;
@@ -685,7 +676,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);
   }
@@ -715,7 +706,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;
@@ -789,7 +780,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;
@@ -802,7 +793,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_) {}
@@ -1107,6 +1098,18 @@ public:
     return __emplace_hint_unique(__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_unique_from_orphaned_node(const_iterator __p, __get_node_value_type_t<_Tp>&& __value) {
+    using __key_type = typename _NodeTypes::key_type;
+    __emplace_hint_unique(__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_unique_from_orphaned_node(const_iterator __p, _Tp&& __value) {
+    __emplace_hint_unique(__p, std::move(__value));
+  }
+
   _LIBCPP_HIDE_FROM_ABI iterator __insert_multi(__container_value_type&& __v) {
     return __emplace_multi(std::move(__v));
   }
@@ -1125,6 +1128,18 @@ 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_multi_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_multi_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);
 
@@ -1266,6 +1281,21 @@ 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;
+
+    // This is technically UB, since the object was constructed as `const`.
+    // Clang doesn't optimize on this currently though.
+    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),
@@ -1406,14 +1436,14 @@ 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();
     }
   }
   const_iterator __e = end();
   for (; __first != __last; ++__first)
-    __insert_multi(__e, _NodeTypes::__get_value(*__first));
+    __insert_multi(__e, *__first);
 }
 
 template <class _Tp, class _Compare, class _Allocator>
@@ -1492,13 +1522,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_multi_from_orphaned_node(__e, std::move(__t.remove(__t.begin())->__value_));
+    }
   }
 }
 
@@ -1524,7 +1555,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);
   }
 }
@@ -1794,10 +1825,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;
 }
@@ -1865,7 +1895,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;
@@ -2027,7 +2057,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;
 }