-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[libc++] Remove UB from std::__tree_node
construction
#153908
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
philnik777
merged 35 commits into
llvm:main
from
vinay-deshmukh:vinay-issue-128660-map-ub
Sep 11, 2025
Merged
Changes from all commits
Commits
Show all changes
35 commits
Select commit
Hold shift + click to select a range
2f689f9
use allocator as template arg and allocator_traits, tests passed in c…
vinay-deshmukh bbc637a
remove UB using list
vinay-deshmukh 99c2829
cf
vinay-deshmukh 5afe3a4
doc-fix
vinay-deshmukh ca72744
Replace __value_ with __get_value() for __tree_node
vinay-deshmukh 47b71f8
miss typo
vinay-deshmukh d16fabf
fix ctor for cpp03 buffer
vinay-deshmukh dcd4cb4
cf
vinay-deshmukh 08f5328
rename in map as well
vinay-deshmukh 4b5609c
cf
vinay-deshmukh d24dbc2
include
vinay-deshmukh 9c47574
more?
vinay-deshmukh 8b1278e
hide from abi
vinay-deshmukh b241636
terser
vinay-deshmukh f5cc9c3
redundant launder
vinay-deshmukh c1a776e
single branch
vinay-deshmukh 4e905fe
remove the & from buffer
vinay-deshmukh 225b03c
remove _LIBCPP_STANDALONE_DEBUG
vinay-deshmukh 1b01b80
cf
vinay-deshmukh 62df872
Merge remote-tracking branch 'upstream/main' into vinay-issue-128660-…
vinay-deshmukh e035df9
Merge branch 'main' into vinay-issue-128660-map-ub
vinay-deshmukh 675a0ca
Merge remote-tracking branch 'upstream/main' into vinay-issue-128660-…
vinay-deshmukh 54951dd
Merge branch 'vinay-issue-128660-map-ub' of out:vinay-deshmukh/llvm-p…
vinay-deshmukh 77c1569
Remove ~__tree_node_base() = default;
vinay-deshmukh 0698243
Update __hash_node constructor to be similar to __tree_node
vinay-deshmukh 5cd0195
cf
vinay-deshmukh 881284a
Merge remote-tracking branch 'upstream/main' into vinay-issue-128660-…
vinay-deshmukh f7332e6
delete ~__tree_node_base
vinay-deshmukh 8e9153e
Revert "delete ~__tree_node_base"
vinay-deshmukh 3ded535
Default to nullptr, and assign __hash during construction
vinay-deshmukh ccc9292
fix eager reduction...
vinay-deshmukh a59fb89
format
vinay-deshmukh 117e150
Fix doc comment, to say construct, not allocate
vinay-deshmukh f0dfc29
After public
vinay-deshmukh 603b60b
Merge remote-tracking branch 'upstream/main' into vinay-issue-128660-…
vinay-deshmukh File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,9 +20,11 @@ | |
#include <__memory/addressof.h> | ||
#include <__memory/allocator_traits.h> | ||
#include <__memory/compressed_pair.h> | ||
#include <__memory/construct_at.h> | ||
#include <__memory/pointer_traits.h> | ||
#include <__memory/swap_allocator.h> | ||
#include <__memory/unique_ptr.h> | ||
#include <__new/launder.h> | ||
#include <__type_traits/copy_cvref.h> | ||
#include <__type_traits/enable_if.h> | ||
#include <__type_traits/invoke.h> | ||
|
@@ -559,8 +561,7 @@ public: | |
}; | ||
|
||
template <class _VoidPtr> | ||
class _LIBCPP_STANDALONE_DEBUG | ||
__tree_node_base : public __tree_end_node<__rebind_pointer_t<_VoidPtr, __tree_node_base<_VoidPtr> > > { | ||
class __tree_node_base : public __tree_end_node<__rebind_pointer_t<_VoidPtr, __tree_node_base<_VoidPtr> > > { | ||
public: | ||
using pointer = __rebind_pointer_t<_VoidPtr, __tree_node_base>; | ||
using __end_node_pointer _LIBCPP_NODEBUG = __rebind_pointer_t<_VoidPtr, __tree_end_node<pointer> >; | ||
|
@@ -573,22 +574,41 @@ public: | |
|
||
_LIBCPP_HIDE_FROM_ABI void __set_parent(pointer __p) { __parent_ = static_cast<__end_node_pointer>(__p); } | ||
vinay-deshmukh marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
~__tree_node_base() = delete; | ||
_LIBCPP_HIDE_FROM_ABI __tree_node_base() = default; | ||
__tree_node_base(__tree_node_base const&) = delete; | ||
__tree_node_base& operator=(__tree_node_base const&) = delete; | ||
}; | ||
|
||
template <class _Tp, class _VoidPtr> | ||
class _LIBCPP_STANDALONE_DEBUG __tree_node : public __tree_node_base<_VoidPtr> { | ||
class __tree_node : public __tree_node_base<_VoidPtr> { | ||
public: | ||
using __node_value_type _LIBCPP_NODEBUG = __get_node_value_type_t<_Tp>; | ||
|
||
// We use a union to avoid initialization during member initialization, which allows us | ||
// to use the allocator from the container to construct the `__node_value_type` in the | ||
// memory provided by the union member | ||
#ifndef _LIBCPP_CXX03_LANG | ||
|
||
private: | ||
__node_value_type __value_; | ||
vinay-deshmukh marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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)]; | ||
|
||
public: | ||
_LIBCPP_HIDE_FROM_ABI __node_value_type& __get_value() { return *reinterpret_cast<__node_value_type*>(__buffer_); } | ||
#endif | ||
|
||
template <class _Alloc, class... _Args> | ||
_LIBCPP_HIDE_FROM_ABI explicit __tree_node(_Alloc& __na, _Args&&... __args) { | ||
vinay-deshmukh marked this conversation as resolved.
Show resolved
Hide resolved
|
||
allocator_traits<_Alloc>::construct(__na, std::addressof(__get_value()), std::forward<_Args>(__args)...); | ||
} | ||
~__tree_node() = delete; | ||
__tree_node(__tree_node const&) = delete; | ||
__tree_node& operator=(__tree_node const&) = delete; | ||
|
@@ -1816,7 +1836,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->__get_value()), std::forward<_Args>(__args)...); | ||
std::__construct_at(std::addressof(*__h), __na, std::forward<_Args>(__args)...); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. N.B. This is the line that prompts this change due to needing workarounds when implementing P3372 for map (Add |
||
__h.get_deleter().__value_constructed = true; | ||
return __h; | ||
} | ||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be changed to
= default;
as well? (while I'm changing this code)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A defaulted definition is possibly deleted because
__node_value_type
can be non-trivially destructible. I haven't check whether there's any code destroying the__hash_node
as a whole (i.e. calling the destructor). If so, we shouldn't default it.