[libc++] Remove UB from std::__tree_node construction
#153908
Conversation
|
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
| @@ -1808,7 +1842,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)...); | |||
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... |
… for UB removal (#154225) Prepare for: llvm/llvm-project#153908 (comment)
…roject into vinay-issue-128660-map-ub
std::map by updating std::__tree_node construction / Update std::__hash_node construction
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
| // | ||
| // We don't use the allocator's construct() method to construct the node itself since the | ||
| // 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..
| : _Base(__next), __hash_(__hash) { | ||
| 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.
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. |
std::__tree_node and std::__hash_node constructionstd::__tree_node construction
|
This patch also updates
__hash_tableto match what we do in__treenow.Fixes #102547
Fixes #134330 (comment)