[libcxx-commits] [libcxx] [libc++] Remove UB from std::map __tree_node constructor (PR #153908)

[Vinay Deshmukh via libcxx-commits]

https://github.com/vinay-deshmukh updated https://github.com/llvm/llvm-project/pull/153908

>From 2f689f97b6c729f03f37e51c941c32186e9d3565 Mon Sep 17 00:00:00 2001
From: Vinay Deshmukh <32487576+vinay-deshmukh at users.noreply.github.com>
Date: Wed, 13 Aug 2025 06:49:44 -0400
Subject: [PATCH 1/4] use allocator as template arg and allocator_traits, tests
 passed in cpp26

---
 libcxx/include/__tree | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/libcxx/include/__tree b/libcxx/include/__tree
index d154ce1616b93..3fef3efcf78a7 100644
--- a/libcxx/include/__tree
+++ b/libcxx/include/__tree
@@ -582,10 +582,16 @@ class _LIBCPP_STANDALONE_DEBUG __tree_node : public __tree_node_base<_VoidPtr> {
 public:
   using __node_value_type _LIBCPP_NODEBUG = __get_node_value_type_t<_Tp>;
 
+  union {
   __node_value_type __value_;
+  };
 
   _LIBCPP_HIDE_FROM_ABI __node_value_type& __get_value() { return __value_; }
 
+  template <class _Alloc, class... _Args>
+  explicit __tree_node(_Alloc& __na, _Args&&... __args) {
+    allocator_traits<_Alloc>::construct(__na, std::addressof(__value_), std::forward<_Args>(__args)...);
+  }
   ~__tree_node()                             = delete;
   __tree_node(__tree_node const&)            = delete;
   __tree_node& operator=(__tree_node const&) = delete;
@@ -1808,7 +1814,7 @@ typename __tree<_Tp, _Compare, _Allocator>::__node_holder
 __tree<_Tp, _Compare, _Allocator>::__construct_node(_Args&&... __args) {
   __node_allocator& __na = __node_alloc();
   __node_holder __h(__node_traits::allocate(__na, 1), _Dp(__na));
-  __node_traits::construct(__na, std::addressof(__h->__value_), std::forward<_Args>(__args)...);
+  std::__construct_at(std::addressof(*__h), __na, std::forward<_Args>(__args)...);
   __h.get_deleter().__value_constructed = true;
   return __h;
 }

>From bbc637ad50a0051e1f46da96bdbef6205e7f846d Mon Sep 17 00:00:00 2001
From: Vinay Deshmukh <32487576+vinay-deshmukh at users.noreply.github.com>
Date: Fri, 15 Aug 2025 20:46:20 -0400
Subject: [PATCH 2/4] remove UB using list

---
 libcxx/include/__tree | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/libcxx/include/__tree b/libcxx/include/__tree
index 3fef3efcf78a7..d860f4c96f467 100644
--- a/libcxx/include/__tree
+++ b/libcxx/include/__tree
@@ -582,11 +582,25 @@ class _LIBCPP_STANDALONE_DEBUG __tree_node : public __tree_node_base<_VoidPtr> {
 public:
   using __node_value_type _LIBCPP_NODEBUG = __get_node_value_type_t<_Tp>;
 
-  union {
-  __node_value_type __value_;
-  };
+  // We allow starting the lifetime of nodes without initializing the value held by the node,
+  // since that is handled by the map itself in order to be allocator-aware.
+  #  ifndef _LIBCPP_CXX03_LANG
+
+  private:
+    union {
+      __node_value_type __value_;
+    };
+
+  public:
+    _LIBCPP_HIDE_FROM_ABI __node_value_type& __get_value() { return __value_; }
+  #  else
+
+  private:
+    _ALIGNAS_TYPE(__node_value_type) unsigned char __buffer_[sizeof(__node_value_type)];
 
-  _LIBCPP_HIDE_FROM_ABI __node_value_type& __get_value() { return __value_; }
+  public:
+    _LIBCPP_HIDE_FROM_ABI __node_value_type& __get_value() { return *std::__launder(reinterpret_cast<_Tp*>(&__buffer_)); }
+  #  endif
 
   template <class _Alloc, class... _Args>
   explicit __tree_node(_Alloc& __na, _Args&&... __args) {

>From 99c2829166a093ac57479a0b8cb7fbed806cf00c Mon Sep 17 00:00:00 2001
From: Vinay Deshmukh <32487576+vinay-deshmukh at users.noreply.github.com>
Date: Fri, 15 Aug 2025 20:53:57 -0400
Subject: [PATCH 3/4] cf

---
 libcxx/include/__tree | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/libcxx/include/__tree b/libcxx/include/__tree
index d860f4c96f467..5245dcdb23434 100644
--- a/libcxx/include/__tree
+++ b/libcxx/include/__tree
@@ -582,25 +582,25 @@ class _LIBCPP_STANDALONE_DEBUG __tree_node : public __tree_node_base<_VoidPtr> {
 public:
   using __node_value_type _LIBCPP_NODEBUG = __get_node_value_type_t<_Tp>;
 
-  // We allow starting the lifetime of nodes without initializing the value held by the node,
-  // since that is handled by the map itself in order to be allocator-aware.
-  #  ifndef _LIBCPP_CXX03_LANG
+// We allow starting the lifetime of nodes without initializing the value held by the node,
+// since that is handled by the map itself in order to be allocator-aware.
+#ifndef _LIBCPP_CXX03_LANG
 
-  private:
-    union {
-      __node_value_type __value_;
-    };
+private:
+  union {
+    __node_value_type __value_;
+  };
 
-  public:
-    _LIBCPP_HIDE_FROM_ABI __node_value_type& __get_value() { return __value_; }
-  #  else
+public:
+  _LIBCPP_HIDE_FROM_ABI __node_value_type& __get_value() { return __value_; }
+#else
 
-  private:
-    _ALIGNAS_TYPE(__node_value_type) unsigned char __buffer_[sizeof(__node_value_type)];
+private:
+  _ALIGNAS_TYPE(__node_value_type) unsigned char __buffer_[sizeof(__node_value_type)];
 
-  public:
-    _LIBCPP_HIDE_FROM_ABI __node_value_type& __get_value() { return *std::__launder(reinterpret_cast<_Tp*>(&__buffer_)); }
-  #  endif
+public:
+  _LIBCPP_HIDE_FROM_ABI __node_value_type& __get_value() { return *std::__launder(reinterpret_cast<_Tp*>(&__buffer_)); }
+#endif
 
   template <class _Alloc, class... _Args>
   explicit __tree_node(_Alloc& __na, _Args&&... __args) {

>From 5afe3a4b2ca319491fe05e57e6a46eae4add233b Mon Sep 17 00:00:00 2001
From: Vinay Deshmukh <32487576+vinay-deshmukh at users.noreply.github.com>
Date: Fri, 15 Aug 2025 20:54:06 -0400
Subject: [PATCH 4/4] doc-fix

---
 libcxx/include/__tree | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libcxx/include/__tree b/libcxx/include/__tree
index 5245dcdb23434..e1da5e65e3cde 100644
--- a/libcxx/include/__tree
+++ b/libcxx/include/__tree
@@ -582,8 +582,8 @@ class _LIBCPP_STANDALONE_DEBUG __tree_node : public __tree_node_base<_VoidPtr> {
 public:
   using __node_value_type _LIBCPP_NODEBUG = __get_node_value_type_t<_Tp>;
 
-// We allow starting the lifetime of nodes without initializing the value held by the node,
-// since that is handled by the map itself in order to be allocator-aware.
+// We use a union to avoid initialization during member initialization, which allows us
+// to use the allocator from the container to allocate the node itself
 #ifndef _LIBCPP_CXX03_LANG
 
 private: