[libc++] Avoid type-punning between __hash_value_type and pair #143501

philnik777 · 2025-06-10T09:59:34Z

This patch is very similar to #134819 in nature. 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 unordered_maps instead, as they actually do.

github-actions · 2025-06-10T10:02:00Z

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

philnik777 · 2025-06-12T07:40:34Z

@Michael137 Looks like I'm breaking the formatters again. Could you take a look? I'm basically removing an extra indirection though __hash_value_type. A very similar change to __tree (i.e. {multi,}map) worked just fine, so maybe they should use the same approach.

FWIW I think the problem is that GetElementType in the unordered_map formatter looks at a template argument.

llvmbot · 2025-06-15T13:55:01Z

@llvm/pr-subscribers-libcxx

Author: Nikolas Klauser (philnik777)

Changes

This patch is very similar to #134819 in nature. 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 unordered_maps instead, as they actually do.

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

3 Files Affected:

(modified) libcxx/include/__hash_table (+64-20)
(modified) libcxx/include/unordered_map (+29-124)
(modified) libcxx/utils/gdb/libcxx/printers.py (+2-12)

diff --git a/libcxx/include/__hash_table b/libcxx/include/__hash_table
index aefa8e19c1864..0f88ade2c9854 100644
--- a/libcxx/include/__hash_table
+++ b/libcxx/include/__hash_table
@@ -29,6 +29,7 @@
 #include <__memory/unique_ptr.h>
 #include <__new/launder.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>
@@ -108,9 +109,22 @@ struct __hash_node_base {
   _LIBCPP_HIDE_FROM_ABI explicit __hash_node_base(__next_pointer __next) _NOEXCEPT : __next_(__next) {}
 };
 
+template <class _Tp>
+struct __get_hash_node_value_type {
+  using type _LIBCPP_NODEBUG = _Tp;
+};
+
+template <class _Key, class _Tp>
+struct __get_hash_node_value_type<__hash_value_type<_Key, _Tp> > {
+  using type _LIBCPP_NODEBUG = pair<const _Key, _Tp>;
+};
+
+template <class _Tp>
+using __get_hash_node_value_type_t _LIBCPP_NODEBUG = typename __get_hash_node_value_type<_Tp>::type;
+
 template <class _Tp, class _VoidPtr>
 struct __hash_node : public __hash_node_base< __rebind_pointer_t<_VoidPtr, __hash_node<_Tp, _VoidPtr> > > {
-  typedef _Tp __node_value_type;
+  using __node_value_type _LIBCPP_NODEBUG = __get_hash_node_value_type_t<_Tp>;
   using _Base _LIBCPP_NODEBUG          = __hash_node_base<__rebind_pointer_t<_VoidPtr, __hash_node<_Tp, _VoidPtr> > >;
   using __next_pointer _LIBCPP_NODEBUG = typename _Base::__next_pointer;
 
@@ -122,18 +136,20 @@ struct __hash_node : public __hash_node_base< __rebind_pointer_t<_VoidPtr, __has
 
 private:
   union {
-    _Tp __value_;
+    __node_value_type __value_;
   };
 
 public:
-  _LIBCPP_HIDE_FROM_ABI _Tp& __get_value() { return __value_; }
+  _LIBCPP_HIDE_FROM_ABI __node_value_type& __get_value() { return __value_; }
 #else
 
 private:
-  _ALIGNAS_TYPE(_Tp) char __buffer_[sizeof(_Tp)];
+  _ALIGNAS_TYPE(__node_value_type) char __buffer_[sizeof(__node_value_type)];
 
 public:
-  _LIBCPP_HIDE_FROM_ABI _Tp& __get_value() { return *std::__launder(reinterpret_cast<_Tp*>(&__buffer_)); }
+  _LIBCPP_HIDE_FROM_ABI __node_value_type& __get_value() {
+    return *std::__launder(reinterpret_cast<__node_value_type*>(&__buffer_));
+  }
 #endif
 
   _LIBCPP_HIDE_FROM_ABI explicit __hash_node(__next_pointer __next, size_t __hash) : _Base(__next), __hash_(__hash) {}
@@ -201,8 +217,8 @@ struct __hash_key_value_types<__hash_value_type<_Key, _Tp> > {
     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 __container_value_type* __get_ptr(__container_value_type& __n) {
+    return std::addressof(__n);
   }
   _LIBCPP_HIDE_FROM_ABI static pair<key_type&&, mapped_type&&> __move(__node_value_type& __v) { return __v.__move(); }
 };
@@ -242,7 +258,7 @@ public:
 
   typedef typename __node_base_type::__next_pointer __next_pointer;
 
-  typedef _Tp __node_value_type;
+  using __node_value_type _LIBCPP_NODEBUG = __get_hash_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;
 
@@ -667,14 +683,14 @@ int __diagnose_unordered_container_requirements(void*);
 template <class _Tp, class _Hash, class _Equal, class _Alloc>
 class __hash_table {
 public:
-  typedef _Tp value_type;
+  using value_type = __get_hash_node_value_type_t<_Tp>;
   typedef _Hash hasher;
   typedef _Equal key_equal;
   typedef _Alloc allocator_type;
 
 private:
   typedef allocator_traits<allocator_type> __alloc_traits;
-  typedef typename __make_hash_node_types<value_type, typename __alloc_traits::void_pointer>::type _NodeTypes;
+  typedef typename __make_hash_node_types<_Tp, typename __alloc_traits::void_pointer>::type _NodeTypes;
 
 public:
   typedef typename _NodeTypes::__node_value_type __node_value_type;
@@ -855,6 +871,22 @@ public:
     return __emplace_hint_multi(__p, std::forward<_Pp>(__x));
   }
 
+  template <class _ValueT = _Tp, __enable_if_t<__is_hash_value_type<_ValueT>::value, int> = 0>
+  _LIBCPP_HIDE_FROM_ABI void __insert_multi_from_orphaned_node(value_type&& __value) {
+    using __key_type = typename _NodeTypes::key_type;
+
+    __node_holder __h = __construct_node(const_cast<__key_type&&>(__value.first), std::move(__value.second));
+    __node_insert_multi(__h.get());
+    __h.release();
+  }
+
+  template <class _ValueT = _Tp, __enable_if_t<!__is_hash_value_type<_ValueT>::value, int> = 0>
+  _LIBCPP_HIDE_FROM_ABI void __insert_multi_from_orphaned_node(value_type&& __value) {
+    __node_holder __h = __construct_node(std::move(__value));
+    __node_insert_multi(__h.get());
+    __h.release();
+  }
+
   _LIBCPP_HIDE_FROM_ABI pair<iterator, bool> __insert_unique(const __container_value_type& __x) {
     return __emplace_unique_key_args(_NodeTypes::__get_key(__x), __x);
   }
@@ -1020,6 +1052,21 @@ private:
   _LIBCPP_HIDE_FROM_ABI void __deallocate_node(__next_pointer __np) _NOEXCEPT;
   _LIBCPP_HIDE_FROM_ABI __next_pointer __detach() _NOEXCEPT;
 
+  template <class _From, class _ValueT = _Tp, __enable_if_t<__is_hash_value_type<_ValueT>::value, int> = 0>
+  _LIBCPP_HIDE_FROM_ABI void __assign_value(__get_hash_node_value_type_t<_Tp>& __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 _From, class _ValueT = _Tp, __enable_if_t<!__is_hash_value_type<_ValueT>::value, int> = 0>
+  _LIBCPP_HIDE_FROM_ABI void __assign_value(_Tp& __lhs, _From&& __rhs) {
+    __lhs = std::forward<_From>(__rhs);
+  }
+
   template <class, class, class, class, class>
   friend class unordered_map;
   template <class, class, class, class, class>
@@ -1216,8 +1263,8 @@ void __hash_table<_Tp, _Hash, _Equal, _Alloc>::__move_assign(__hash_table& __u,
 #endif // _LIBCPP_HAS_EXCEPTIONS
         const_iterator __i = __u.begin();
         while (__cache != nullptr && __u.size() != 0) {
-          __cache->__upcast()->__get_value() = std::move(__u.remove(__i++)->__get_value());
-          __next_pointer __next              = __cache->__next_;
+          __assign_value(__cache->__upcast()->__get_value(), std::move(__u.remove(__i++)->__get_value()));
+          __next_pointer __next = __cache->__next_;
           __node_insert_multi(__cache->__upcast());
           __cache = __next;
         }
@@ -1230,11 +1277,8 @@ void __hash_table<_Tp, _Hash, _Equal, _Alloc>::__move_assign(__hash_table& __u,
       __deallocate_node(__cache);
     }
     const_iterator __i = __u.begin();
-    while (__u.size() != 0) {
-      __node_holder __h = __construct_node(_NodeTypes::__move(__u.remove(__i++)->__get_value()));
-      __node_insert_multi(__h.get());
-      __h.release();
-    }
+    while (__u.size() != 0)
+      __insert_multi_from_orphaned_node(std::move(__u.remove(__i++)->__get_value()));
   }
 }
 
@@ -1262,8 +1306,8 @@ void __hash_table<_Tp, _Hash, _Equal, _Alloc>::__assign_unique(_InputIterator __
     try {
 #endif // _LIBCPP_HAS_EXCEPTIONS
       for (; __cache != nullptr && __first != __last; ++__first) {
-        __cache->__upcast()->__get_value() = *__first;
-        __next_pointer __next              = __cache->__next_;
+        __assign_value(__cache->__upcast()->__get_value(), *__first);
+        __next_pointer __next = __cache->__next_;
         __node_insert_unique(__cache->__upcast());
         __cache = __next;
       }
@@ -1294,7 +1338,7 @@ void __hash_table<_Tp, _Hash, _Equal, _Alloc>::__assign_multi(_InputIterator __f
     try {
 #endif // _LIBCPP_HAS_EXCEPTIONS
       for (; __cache != nullptr && __first != __last; ++__first) {
-        __cache->__upcast()->__get_value() = *__first;
+        __assign_value(__cache->__upcast()->__get_value(), *__first);
         __next_pointer __next              = __cache->__next_;
         __node_insert_multi(__cache->__upcast());
         __cache = __next;
diff --git a/libcxx/include/unordered_map b/libcxx/include/unordered_map
index b7f333e5f1178..925b1431667e1 100644
--- a/libcxx/include/unordered_map
+++ b/libcxx/include/unordered_map
@@ -654,9 +654,7 @@ public:
   _LIBCPP_HIDE_FROM_ABI __unordered_map_hasher(const _Hash& __h) _NOEXCEPT_(is_nothrow_copy_constructible<_Hash>::value)
       : _Hash(__h) {}
   _LIBCPP_HIDE_FROM_ABI const _Hash& hash_function() const _NOEXCEPT { return *this; }
-  _LIBCPP_HIDE_FROM_ABI size_t operator()(const _Cp& __x) const {
-    return static_cast<const _Hash&>(*this)(__x.__get_value().first);
-  }
+  _LIBCPP_HIDE_FROM_ABI size_t operator()(const _Cp& __x) const { return static_cast<const _Hash&>(*this)(__x.first); }
   _LIBCPP_HIDE_FROM_ABI size_t operator()(const _Key& __x) const { return static_cast<const _Hash&>(*this)(__x); }
 #  if _LIBCPP_STD_VER >= 20
   template <typename _K2>
@@ -680,7 +678,7 @@ public:
   _LIBCPP_HIDE_FROM_ABI __unordered_map_hasher(const _Hash& __h) _NOEXCEPT_(is_nothrow_copy_constructible<_Hash>::value)
       : __hash_(__h) {}
   _LIBCPP_HIDE_FROM_ABI const _Hash& hash_function() const _NOEXCEPT { return __hash_; }
-  _LIBCPP_HIDE_FROM_ABI size_t operator()(const _Cp& __x) const { return __hash_(__x.__get_value().first); }
+  _LIBCPP_HIDE_FROM_ABI size_t operator()(const _Cp& __x) const { return __hash_(__x.first); }
   _LIBCPP_HIDE_FROM_ABI size_t operator()(const _Key& __x) const { return __hash_(__x); }
 #  if _LIBCPP_STD_VER >= 20
   template <typename _K2>
@@ -713,10 +711,10 @@ public:
       : _Pred(__p) {}
   _LIBCPP_HIDE_FROM_ABI const _Pred& key_eq() const _NOEXCEPT { return *this; }
   _LIBCPP_HIDE_FROM_ABI bool operator()(const _Cp& __x, const _Cp& __y) const {
-    return static_cast<const _Pred&>(*this)(__x.__get_value().first, __y.__get_value().first);
+    return static_cast<const _Pred&>(*this)(__x.first, __y.first);
   }
   _LIBCPP_HIDE_FROM_ABI bool operator()(const _Cp& __x, const _Key& __y) const {
-    return static_cast<const _Pred&>(*this)(__x.__get_value().first, __y);
+    return static_cast<const _Pred&>(*this)(__x.first, __y);
   }
   _LIBCPP_HIDE_FROM_ABI bool operator()(const _Key& __x, const _Cp& __y) const {
     return static_cast<const _Pred&>(*this)(__x, __y.__get_value().first);
@@ -724,7 +722,7 @@ public:
 #  if _LIBCPP_STD_VER >= 20
   template <typename _K2>
   _LIBCPP_HIDE_FROM_ABI bool operator()(const _Cp& __x, const _K2& __y) const {
-    return static_cast<const _Pred&>(*this)(__x.__get_value().first, __y);
+    return static_cast<const _Pred&>(*this)(__x.first, __y);
   }
   template <typename _K2>
   _LIBCPP_HIDE_FROM_ABI bool operator()(const _K2& __x, const _Cp& __y) const {
@@ -755,23 +753,17 @@ public:
   _LIBCPP_HIDE_FROM_ABI __unordered_map_equal(const _Pred& __p) _NOEXCEPT_(is_nothrow_copy_constructible<_Pred>::value)
       : __pred_(__p) {}
   _LIBCPP_HIDE_FROM_ABI const _Pred& key_eq() const _NOEXCEPT { return __pred_; }
-  _LIBCPP_HIDE_FROM_ABI bool operator()(const _Cp& __x, const _Cp& __y) const {
-    return __pred_(__x.__get_value().first, __y.__get_value().first);
-  }
-  _LIBCPP_HIDE_FROM_ABI bool operator()(const _Cp& __x, const _Key& __y) const {
-    return __pred_(__x.__get_value().first, __y);
-  }
-  _LIBCPP_HIDE_FROM_ABI bool operator()(const _Key& __x, const _Cp& __y) const {
-    return __pred_(__x, __y.__get_value().first);
-  }
+  _LIBCPP_HIDE_FROM_ABI bool operator()(const _Cp& __x, const _Cp& __y) const { return __pred_(__x.first, __y.first); }
+  _LIBCPP_HIDE_FROM_ABI bool operator()(const _Cp& __x, const _Key& __y) const { return __pred_(__x.first, __y); }
+  _LIBCPP_HIDE_FROM_ABI bool operator()(const _Key& __x, const _Cp& __y) const { return __pred_(__x, __y.first); }
 #  if _LIBCPP_STD_VER >= 20
   template <typename _K2>
   _LIBCPP_HIDE_FROM_ABI bool operator()(const _Cp& __x, const _K2& __y) const {
-    return __pred_(__x.__get_value().first, __y);
+    return __pred_(__x.first, __y);
   }
   template <typename _K2>
   _LIBCPP_HIDE_FROM_ABI bool operator()(const _K2& __x, const _Cp& __y) const {
-    return __pred_(__x, __y.__get_value().first);
+    return __pred_(__x, __y.first);
   }
   template <typename _K2>
   _LIBCPP_HIDE_FROM_ABI bool operator()(const _Key& __x, const _K2& __y) const {
@@ -833,96 +825,16 @@ public:
 
   _LIBCPP_HIDE_FROM_ABI void operator()(pointer __p) _NOEXCEPT {
     if (__second_constructed)
-      __alloc_traits::destroy(__na_, std::addressof(__p->__get_value().__get_value().second));
+      __alloc_traits::destroy(__na_, std::addressof(__p->__get_value().second));
     if (__first_constructed)
-      __alloc_traits::destroy(__na_, std::addressof(__p->__get_value().__get_value().first));
+      __alloc_traits::destroy(__na_, std::addressof(__p->__get_value().first));
     if (__p)
       __alloc_traits::deallocate(__na_, __p, 1);
   }
 };
 
-#  ifndef _LIBCPP_CXX03_LANG
 template <class _Key, class _Tp>
-struct _LIBCPP_STANDALONE_DEBUG __hash_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 __hash_value_type& operator=(const __hash_value_type& __v) {
-    __ref() = __v.__get_value();
-    return *this;
-  }
-
-  _LIBCPP_HIDE_FROM_ABI __hash_value_type& operator=(__hash_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 __hash_value_type& operator=(_ValueTp&& __v) {
-    __ref() = std::forward<_ValueTp>(__v);
-    return *this;
-  }
-
-  __hash_value_type(const __hash_value_type& __v) = delete;
-  __hash_value_type(__hash_value_type&& __v)      = delete;
-  template <class... _Args>
-  explicit __hash_value_type(_Args&&... __args) = delete;
-
-  ~__hash_value_type() = delete;
-};
-
-#  else
-
-template <class _Key, class _Tp>
-struct __hash_value_type {
-  typedef _Key key_type;
-  typedef _Tp mapped_type;
-  typedef pair<const key_type, mapped_type> value_type;
-
-private:
-  value_type __cc_;
-
-public:
-  _LIBCPP_HIDE_FROM_ABI value_type& __get_value() { return __cc_; }
-  _LIBCPP_HIDE_FROM_ABI const value_type& __get_value() const { return __cc_; }
-
-  ~__hash_value_type() = delete;
-};
-
-#  endif
+struct __hash_value_type;
 
 template <class _HashIterator>
 class __hash_map_iterator {
@@ -941,8 +853,8 @@ public:
 
   _LIBCPP_HIDE_FROM_ABI __hash_map_iterator(_HashIterator __i) _NOEXCEPT : __i_(__i) {}
 
-  _LIBCPP_HIDE_FROM_ABI reference operator*() const { return __i_->__get_value(); }
-  _LIBCPP_HIDE_FROM_ABI pointer operator->() const { return pointer_traits<pointer>::pointer_to(__i_->__get_value()); }
+  _LIBCPP_HIDE_FROM_ABI reference operator*() const { return *__i_; }
+  _LIBCPP_HIDE_FROM_ABI pointer operator->() const { return pointer_traits<pointer>::pointer_to(*__i_); }
 
   _LIBCPP_HIDE_FROM_ABI __hash_map_iterator& operator++() {
     ++__i_;
@@ -995,8 +907,8 @@ public:
   __hash_map_const_iterator(__hash_map_iterator<typename _HashIterator::__non_const_iterator> __i) _NOEXCEPT
       : __i_(__i.__i_) {}
 
-  _LIBCPP_HIDE_FROM_ABI reference operator*() const { return __i_->__get_value(); }
-  _LIBCPP_HIDE_FROM_ABI pointer operator->() const { return pointer_traits<pointer>::pointer_to(__i_->__get_value()); }
+  _LIBCPP_HIDE_FROM_ABI reference operator*() const { return *__i_; }
+  _LIBCPP_HIDE_FROM_ABI pointer operator->() const { return pointer_traits<pointer>::pointer_to(*__i_); }
 
   _LIBCPP_HIDE_FROM_ABI __hash_map_const_iterator& operator++() {
     ++__i_;
@@ -1053,8 +965,8 @@ public:
 
 private:
   typedef __hash_value_type<key_type, mapped_type> __value_type;
-  typedef __unordered_map_hasher<key_type, __value_type, hasher, key_equal> __hasher;
-  typedef __unordered_map_equal<key_type, __value_type, key_equal, hasher> __key_equal;
+  typedef __unordered_map_hasher<key_type, value_type, hasher, key_equal> __hasher;
+  typedef __unordered_map_equal<key_type, value_type, key_equal, hasher> __key_equal;
   typedef __rebind_alloc<allocator_traits<allocator_type>, __value_type> __allocator_type;
 
   typedef __hash_table<__value_type, __hasher, __key_equal, __allocator_type> __table;
@@ -1073,9 +985,6 @@ private:
 
   static_assert(__check_valid_allocator<allocator_type>::value, "");
 
-  static_assert(is_same<typename __table::__container_value_type, value_type>::value, "");
-  static_assert(is_same<typename __table::__node_value_type, __value_type>::value, "");
-
 public:
   typedef typename __alloc_traits::pointer pointer;
   typedef typename __alloc_traits::const_pointer const_pointer;
@@ -1680,9 +1589,8 @@ unordered_map<_Key, _Tp, _Hash, _Pred, _Alloc>::unordered_map(unordered_map&& __
     : __table_(std::move(__u.__table_), typename __table::allocator_type(__a)) {
   if (__a != __u.get_allocator()) {
     iterator __i = __u.begin();
-    while (__u.size() != 0) {
-      __table_.__emplace_unique(__u.__table_.remove((__i++).__i_)->__get_value().__move());
-    }
+    while (__u.size() != 0)
+      __table_.__insert_multi_from_orphaned_node(std::move(__u.__table_.remove((__i++).__i_)->__get_value()));
   }
 }
 
@@ -1741,8 +1649,7 @@ template <class _Key, class _Tp, class _Hash, class _Pred, class _Alloc>
 _Tp& unordered_map<_Key, _Tp, _Hash, _Pred, _Alloc>::operator[](const key_type& __k) {
   return __table_
       .__emplace_unique_key_args(__k, piecewise_construct, std::forward_as_tuple(__k), std::forward_as_tuple())
-      .first->__get_value()
-      .second;
+      .first->second;
 }
 
 template <class _Key, class _Tp, class _Hash, class _Pred, class _Alloc>
@@ -1750,8 +1657,7 @@ _Tp& unordered_map<_Key, _Tp, _Hash, _Pred, _Alloc>::operator[](key_type&& __k)
   return __table_
       .__emplace_unique_key_args(
           __k, piecewise_construct, std::forward_as_tuple(std::move(__k)), std::forward_as_tuple())
-      .first->__get_value()
-      .second;
+      .first->second;
 }
 #  else // _LIBCPP_CXX03_LANG
 
@@ -1760,9 +1666,9 @@ typename unordered_map<_Key, _Tp, _Hash, _Pred, _Alloc>::__node_holder
 unordered_map<_Key, _Tp, _Hash, _Pred, _Alloc>::__construct_node_with_key(const key_type& __k) {
   __node_allocator& __na = __table_.__node_alloc();
   __node_holder __h(__node_traits::allocate(__na, 1), _Dp(__na));
-  __node_traits::construct(__na, std::addressof(__h->__get_value().__get_value().first), __k);
+  __node_traits::construct(__na, std::addressof(__h->__get_value().first), __k);
   __h.get_deleter().__first_constructed = true;
-  __node_traits::construct(__na, std::addressof(__h->__get_value().__get_value().second));
+  __node_traits::construct(__na, std::addressof(__h->__get_value().second));
   __h.get_deleter().__second_constructed = true;
   return __h;
 }
@@ -1869,8 +1775,8 @@ public:
 
 private:
   typedef __hash_value_type<key_type, mapped_type> __value_type;
-  typedef __unordered_map_hasher<key_type, __value_type, hasher, key_equal> __hasher;
-  typedef __unordered_map_equal<key_type, __value_type, key_equal, hasher> __key_equal;
+  typedef __unordered_map_hasher<key_type, value_type, hasher, key_equal> __hasher;
+  typedef __unordered_map_equal<key_type, value_type, key_equal, hasher> __key_equal;
   typedef __rebind_alloc<allocato...
[truncated]

Michael137 · 2025-06-16T13:17:28Z

@Michael137 Looks like I'm breaking the formatters again. Could you take a look? I'm basically removing an extra indirection though __hash_value_type. A very similar change to __tree (i.e. {multi,}map) worked just fine, so maybe they should use the same approach.

FWIW I think the problem is that GetElementType in the unordered_map formatter looks at a template argument.

Thanks for the ping

Will have a look. I suspect your hunch is correct

Michael137 · 2025-06-16T15:56:08Z

I think what's confusing LLDB is that __hash_value_type is just a plain forward declaration. Is there a definition anywhere? I don't see it defined anywhere (apart from the cxx03 headers). How is clang happy with it?
We don't emit template arguments in DWARF for forward declared templates.

philnik777 · 2025-06-16T16:28:24Z

@Michael137 This patch removes the definition on purpose. With this patch it's only ever used as a tag type and a pair<const Key, Value> is used instead. __get_hash_node_value_type unwraps __hash_value_type to a pair now. We have to keep __hash_value_type as the template argument for ABI reasons unfortunately.

Michael137 · 2025-06-17T11:21:43Z

@Michael137 This patch removes the definition on purpose. With this patch it's only ever used as a tag type and a pair<const Key, Value> is used instead. __get_hash_node_value_type unwraps __hash_value_type to a pair now. We have to keep __hash_value_type as the template argument for ABI reasons unfortunately.

Coincidentally I adjusted this code couple of weeks ago: #140256 because of a similar issue where the node_type was a forward declaration.

Maybe we need to mimick whatever __get_hash_node_value_type is doing in LLDB. Checking...

Michael137 · 2025-06-17T11:41:02Z

I think we might be able to get away with using __hash_table::value_type:

0x0000030c:         DW_TAG_typedef
                      DW_AT_type        (0x00000807 "std::__1::pair<const int, int>")
                      DW_AT_name        ("value_type")
                      DW_AT_accessibility       (DW_ACCESS_public)
                      DW_AT_decl_file   ("/Users/jonas/Git/llvm-worktrees/llvm-project/builds-lldb/release/include/c++/v1/__hash_table")
                      DW_AT_decl_line   (686)

…le::value_type llvm#143501 changes usage of `__hash_value_type` in libcxx to an empty tag type. This type will no longer have a definition in DWARF. Currently the LLDB unordered_map formatter deduces the map's `element_type` by looking at the `__cc_` member of `__hash_value_type`. But that will no longer work because we only have its forward declaration. Since what we're really after is the type that `__hash_value_type` is wrapping, we can just look at the `__hash_table::value_type` typedef. With llvm#143501 that will now point to the `std::pair` element type (which used to be what we got from `__cc_`). TBD: need to double-check this works for older layouts. Quick glance at the code makes me suspicious of cases like `unordered_map<std::pair<int, int>, int>`

Michael137 · 2025-06-17T12:14:54Z

Preliminary patch: #144517

…le::value_type (#144517) #143501 changes usage of `__hash_value_type` in libcxx to an empty tag type. This type will no longer have a definition in DWARF. Currently the LLDB unordered_map formatter deduces the map's `element_type` by looking at the `__cc_` member of `__hash_value_type`. But that will no longer work because we only have its forward declaration. Since what we're really after is the type that `__hash_value_type` is wrapping, we can just look at the `__hash_table::value_type` typedef. With #143501 that will now point to the `std::pair` element type (which used to be what we got from `__cc_`). TBD: need to double-check this works for older layouts. Quick glance at the code makes me suspicious of cases like `unordered_map<std::pair<int, int>, int>`

Michael137 · 2025-06-17T16:37:58Z

Fix merged, you should be able to rebase and re-run CI

… __hash_table::value_type (#144517) llvm/llvm-project#143501 changes usage of `__hash_value_type` in libcxx to an empty tag type. This type will no longer have a definition in DWARF. Currently the LLDB unordered_map formatter deduces the map's `element_type` by looking at the `__cc_` member of `__hash_value_type`. But that will no longer work because we only have its forward declaration. Since what we're really after is the type that `__hash_value_type` is wrapping, we can just look at the `__hash_table::value_type` typedef. With llvm/llvm-project#143501 that will now point to the `std::pair` element type (which used to be what we got from `__cc_`). TBD: need to double-check this works for older layouts. Quick glance at the code makes me suspicious of cases like `unordered_map<std::pair<int, int>, int>`

…le::value_type (llvm#144517) llvm#143501 changes usage of `__hash_value_type` in libcxx to an empty tag type. This type will no longer have a definition in DWARF. Currently the LLDB unordered_map formatter deduces the map's `element_type` by looking at the `__cc_` member of `__hash_value_type`. But that will no longer work because we only have its forward declaration. Since what we're really after is the type that `__hash_value_type` is wrapping, we can just look at the `__hash_table::value_type` typedef. With llvm#143501 that will now point to the `std::pair` element type (which used to be what we got from `__cc_`). TBD: need to double-check this works for older layouts. Quick glance at the code makes me suspicious of cases like `unordered_map<std::pair<int, int>, int>` (cherry picked from commit 382e3fd)

…le::value_type (llvm#144517) llvm#143501 changes usage of `__hash_value_type` in libcxx to an empty tag type. This type will no longer have a definition in DWARF. Currently the LLDB unordered_map formatter deduces the map's `element_type` by looking at the `__cc_` member of `__hash_value_type`. But that will no longer work because we only have its forward declaration. Since what we're really after is the type that `__hash_value_type` is wrapping, we can just look at the `__hash_table::value_type` typedef. With llvm#143501 that will now point to the `std::pair` element type (which used to be what we got from `__cc_`). TBD: need to double-check this works for older layouts. Quick glance at the code makes me suspicious of cases like `unordered_map<std::pair<int, int>, int>`

libcxx/include/__hash_table

libcxx/include/unordered_map

philnik777 force-pushed the hash_table_avoid_type_punning branch 2 times, most recently from 20fcdd4 to 1376bde Compare June 10, 2025 21:19

philnik777 changed the title ~~[libc++] Avoid type-punning betwee __hash_value_type and pair~~ [libc++] Avoid type-punning between __hash_value_type and pair Jun 11, 2025

philnik777 marked this pull request as ready for review June 15, 2025 13:54

philnik777 requested a review from a team as a code owner June 15, 2025 13:54

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Jun 15, 2025

Michael137 mentioned this pull request Jun 17, 2025

[lldb][Formatter] Get element type for unordered_maps from __hash_table::value_type #144517

Merged

Michael137 mentioned this pull request Jun 18, 2025

[lldb] Assortment of cherry-picks from upstream LLVM swiftlang/llvm-project#10866

Merged

ldionne requested changes Jun 18, 2025

View reviewed changes

libcxx/include/__hash_table Show resolved Hide resolved

libcxx/include/unordered_map Outdated Show resolved Hide resolved

[libc++] Avoid type-punning betwee __hash_value_type and pair

53d1e23

philnik777 force-pushed the hash_table_avoid_type_punning branch from 0264040 to 53d1e23 Compare June 19, 2025 08:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[libc++] Avoid type-punning between __hash_value_type and pair #143501

[libc++] Avoid type-punning between __hash_value_type and pair #143501

Uh oh!

philnik777 commented Jun 10, 2025

Uh oh!

github-actions bot commented Jun 10, 2025 •

edited

Loading

Uh oh!

philnik777 commented Jun 12, 2025

Uh oh!

llvmbot commented Jun 15, 2025

Uh oh!

Michael137 commented Jun 16, 2025

Uh oh!

Michael137 commented Jun 16, 2025

Uh oh!

philnik777 commented Jun 16, 2025

Uh oh!

Michael137 commented Jun 17, 2025

Uh oh!

Michael137 commented Jun 17, 2025

Uh oh!

Michael137 commented Jun 17, 2025

Uh oh!

Michael137 commented Jun 17, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

[libc++] Avoid type-punning between __hash_value_type and pair #143501

Are you sure you want to change the base?

[libc++] Avoid type-punning between __hash_value_type and pair #143501

Uh oh!

Conversation

philnik777 commented Jun 10, 2025

Uh oh!

github-actions bot commented Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

philnik777 commented Jun 12, 2025

Uh oh!

llvmbot commented Jun 15, 2025

Uh oh!

Michael137 commented Jun 16, 2025

Uh oh!

Michael137 commented Jun 16, 2025

Uh oh!

philnik777 commented Jun 16, 2025

Uh oh!

Michael137 commented Jun 17, 2025

Uh oh!

Michael137 commented Jun 17, 2025

Uh oh!

Michael137 commented Jun 17, 2025

Uh oh!

Michael137 commented Jun 17, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Jun 10, 2025 •

edited

Loading