-
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
Changes from 29 commits
2f689f9
bbc637a
99c2829
5afe3a4
ca72744
47b71f8
d16fabf
dcd4cb4
08f5328
4b5609c
d24dbc2
9c47574
8b1278e
b241636
f5cc9c3
c1a776e
4e905fe
225b03c
1b01b80
62df872
e035df9
675a0ca
54951dd
77c1569
0698243
5cd0195
881284a
f7332e6
8e9153e
3ded535
ccc9292
a59fb89
117e150
f0dfc29
603b60b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -166,7 +166,12 @@ public: | |
} | ||
#endif | ||
|
||
_LIBCPP_HIDE_FROM_ABI explicit __hash_node(__next_pointer __next, size_t __hash) : _Base(__next), __hash_(__hash) {} | ||
template <class _Alloc, class... _Args> | ||
_LIBCPP_HIDE_FROM_ABI explicit __hash_node(__next_pointer __next, size_t __hash, _Alloc& __na, _Args&&... __args) | ||
: _Base(__next), __hash_(__hash) { | ||
allocator_traits<_Alloc>::construct(__na, std::addressof(__get_value()), std::forward<_Args>(__args)...); | ||
} | ||
|
||
_LIBCPP_HIDE_FROM_ABI ~__hash_node() {} | ||
}; | ||
|
||
|
@@ -1874,16 +1879,13 @@ __hash_table<_Tp, _Hash, _Equal, _Alloc>::__construct_node(_Args&&... __args) { | |
__node_allocator& __na = __node_alloc(); | ||
__node_holder __h(__node_traits::allocate(__na, 1), _Dp(__na)); | ||
|
||
// Begin the lifetime of the node itself. Note that this doesn't begin the lifetime of the value | ||
// held inside the node, since we need to use the allocator's construct() method for that. | ||
// Begin the lifetime of the node itself and the value_type contained within. | ||
// | ||
// 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)...); | ||
|
||
|
||
// Now construct the value_type using the allocator's construct() method. | ||
__node_traits::construct(__na, std::addressof(__h->__get_value()), std::forward<_Args>(__args)...); | ||
__h.get_deleter().__value_constructed = true; | ||
|
||
__h->__hash_ = hash_function()(__h->__get_value()); | ||
|
@@ -1897,8 +1899,8 @@ __hash_table<_Tp, _Hash, _Equal, _Alloc>::__construct_node_hash(size_t __hash, _ | |
static_assert(!__is_hash_value_type<_Args...>::value, "Construct cannot be called with a hash value type"); | ||
__node_allocator& __na = __node_alloc(); | ||
__node_holder __h(__node_traits::allocate(__na, 1), _Dp(__na)); | ||
std::__construct_at(std::addressof(*__h), /* next = */ nullptr, /* hash = */ __hash); | ||
__node_traits::construct(__na, std::addressof(__h->__get_value()), std::forward<_Args>(__args)...); | ||
std::__construct_at( | ||
std::addressof(*__h), /* next = */ nullptr, /* hash = */ __hash, __na, std::forward<_Args>(__args)...); | ||
__h.get_deleter().__value_constructed = true; | ||
return __h; | ||
} | ||
|
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> | ||
|
@@ -558,8 +560,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> >; | ||
|
@@ -572,22 +573,40 @@ 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 allocate the node itself | ||
#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; | ||
|
@@ -1793,7 +1812,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; | ||
} | ||
|
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.