[libc++] Remove UB from std::__tree_node construction
#153908
Conversation
|
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
vinay-deshmukh
changed the title
| __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)...); |
There was a problem hiding this comment.
N.B. This is the line that prompts this change due to needing workarounds when implementing P3372 for map (Add constexpr)
|
Failed checks seem to have timed out after 2 hours: 
|
my bad, false alarm I do get test failures if I don't update set and multiset... |
…roject into vinay-issue-128660-map-ub
vinay-deshmukh
changed the title
std::map by updating std::__tree_node construction / Update std::__hash_node construction
philnik777
changed the title
std::map by updating std::__tree_node construction / Update std::__hash_node constructionstd::map by updating std::__tree_node construction
libcxx/include/__hash_table
Outdated
| // Cpp17FooInsertable named requirements don't require the allocator's construct() method | ||
| // to work on anything other than the value_type. | ||
| std::__construct_at(std::addressof(*__h), /* next = */ nullptr, /* hash = */ 0); | ||
| std::__construct_at(std::addressof(*__h), /* next = */ nullptr, /* hash = */ 0, __na, std::forward<_Args>(__args)...); |
There was a problem hiding this comment.
Do we ever construct the node with anything but nullptr and 0? If not, I'd just move that into the constructor.
There was a problem hiding this comment.
nullptr was used in all instances, but the hash = arg can be 0 or a variable for the second occurrence, so that is still a constructor arg.
one more thing, for the second instance this was 0, but then in the next line, it was being reassigned with a variable, so I changed that to be assigned through the constructor arg.
Update: as it needs to be computed after the __value_ is inititalized, we can't put that as a constructor arg unless we also send in the hash_function as a constructor arg..
| allocator_traits<_Alloc>::construct(__na, std::addressof(__get_value()), std::forward<_Args>(__args)...); | ||
| } | ||
|
|
||
| _LIBCPP_HIDE_FROM_ABI ~__hash_node() {} |
There was a problem hiding this comment.
Should this be changed to = default; as well? (while I'm changing this code)
There was a problem hiding this comment.
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.
frederick-vs-ja
changed the title
std::map by updating std::__tree_node constructionstd::__tree_node and std::__hash_node construction
|
@vinay-deshmukh I modified the title and description to conclude the changes more clearly and associate this PR with the corresponding issue. Please double check the changes. |
philnik777
changed the title
std::__tree_node and std::__hash_node constructionstd::__tree_node construction
|
|
I don't have merge permissions currently, so will need you to hit merge as well! Thanks! Arm ci appears to have timed out. |
|
Looks like that we shouldn't generally "fix" another PR in a PR description... |
3 participants
This patch also updates
__hash_tableto match what we do in__treenow.Fixes #102547
Fixes #134330 (comment)