Skip to content

Use MaybeUninit in liballoc #54924

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.

Sign up for GitHub

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
merged 5 commits into from
Oct 12, 2018
Merged

Use MaybeUninit in liballoc #54924

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
aeea992
alloc: fix deprecated warnings
japaric Aug 19, 2018
6644e18
address RalfJung's comment
japaric Aug 23, 2018
4d8310c
gdb_rust_pretty_printing: adapt to the changes in the layout of btree…
japaric Sep 1, 2018
faa733d
fix tidy
japaric Sep 1, 2018
e4434be
remove a now outdated comment
RalfJung Oct 10, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions src/etc/gdb_rust_pretty_printing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,8 +322,11 @@ def to_string(self):
def children(self):
(length, data_ptr) = \
rustpp.extract_length_and_ptr_from_std_btreeset(self.__val)
val = GdbValue(data_ptr.get_wrapped_value().dereference()).get_child_at_index(3)
gdb_ptr = val.get_wrapped_value()
leaf_node = GdbValue(data_ptr.get_wrapped_value().dereference())
maybe_uninit_keys = leaf_node.get_child_at_index(3)
manually_drop_keys = maybe_uninit_keys.get_child_at_index(1)
keys = manually_drop_keys.get_child_at_index(0)
gdb_ptr = keys.get_wrapped_value()
for index in xrange(length):
yield (str(index), gdb_ptr[index])

Expand All @@ -345,9 +348,14 @@ def to_string(self):
def children(self):
(length, data_ptr) = \
rustpp.extract_length_and_ptr_from_std_btreemap(self.__val)
keys = GdbValue(data_ptr.get_wrapped_value().dereference()).get_child_at_index(3)
leaf_node = GdbValue(data_ptr.get_wrapped_value().dereference())
maybe_uninit_keys = leaf_node.get_child_at_index(3)
manually_drop_keys = maybe_uninit_keys.get_child_at_index(1)
keys = manually_drop_keys.get_child_at_index(0)
keys_ptr = keys.get_wrapped_value()
vals = GdbValue(data_ptr.get_wrapped_value().dereference()).get_child_at_index(4)
maybe_uninit_vals = leaf_node.get_child_at_index(4)
manually_drop_vals = maybe_uninit_vals.get_child_at_index(1)
vals = manually_drop_vals.get_child_at_index(0)
vals_ptr = vals.get_wrapped_value()
for index in xrange(length):
yield (str(index), keys_ptr[index])
Expand Down
43 changes: 20 additions & 23 deletions src/liballoc/collections/btree/node.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@
// This implies that even an empty internal node has at least one edge.

use core::marker::PhantomData;
use core::mem;
use core::mem::{self, MaybeUninit};
use core::ptr::{self, Unique, NonNull};
use core::slice;

Expand All @@ -58,9 +58,6 @@ pub const CAPACITY: usize = 2 * B - 1;
/// these should always be put behind pointers, and specifically behind `BoxedNode` in the owned
/// case.
///
/// See also rust-lang/rfcs#197, which would make this structure significantly more safe by
/// avoiding accidentally dropping unused and uninitialized keys and values.
///
/// We put the metadata first so that its position is the same for every `K` and `V`, in order
/// to statically allocate a single dummy node to avoid allocations. This struct is `repr(C)` to
/// prevent them from being reordered.
Expand All @@ -73,7 +70,7 @@ struct LeafNode<K, V> {
/// This node's index into the parent node's `edges` array.
/// `*node.parent.edges[node.parent_idx]` should be the same thing as `node`.
/// This is only guaranteed to be initialized when `parent` is nonnull.
parent_idx: u16,
parent_idx: MaybeUninit<u16>,

/// The number of keys and values this node stores.
///
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you confirm that despite introducing MaybeUninit we are still the same size? (i.e., same 32-bit word layout as indicated by the comment on len)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MaybeUninit won't introduce any additional alignment requirements that aren't present in the type it wraps, and will only increase the size of the resulting type as a result of inhibiting niche-filling optimizations (and u16 has not niches, so this shouldn't be an issue).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be a good way to test this? We have a way to "hack" a static_assert!, but the size alone won't do it here because there's a ptr right before this, so on 64bit system it'll be 2 ptr-words in size at least. And we cannot yet do an offset! macro in CTFE. I could add a debug_assert! to test the offset at run-time?

However, I agree with what @cramertj said and this is repr(C), so I am not expecting any surprises. Still, would be nice to be sure.

Expand All @@ -83,8 +80,8 @@ struct LeafNode<K, V> {

/// The arrays storing the actual data of the node. Only the first `len` elements of each
/// array are initialized and valid.
keys: [K; CAPACITY],
vals: [V; CAPACITY],
keys: MaybeUninit<[K; CAPACITY]>,
vals: MaybeUninit<[V; CAPACITY]>,
}

impl<K, V> LeafNode<K, V> {
Expand All @@ -94,10 +91,10 @@ impl<K, V> LeafNode<K, V> {
LeafNode {
// As a general policy, we leave fields uninitialized if they can be, as this should
// be both slightly faster and easier to track in Valgrind.
keys: mem::uninitialized(),
vals: mem::uninitialized(),
keys: MaybeUninit::uninitialized(),
vals: MaybeUninit::uninitialized(),
parent: ptr::null(),
parent_idx: mem::uninitialized(),
parent_idx: MaybeUninit::uninitialized(),
len: 0
}
}
Expand All @@ -115,10 +112,10 @@ unsafe impl Sync for LeafNode<(), ()> {}
// ever take a pointer past the first key.
static EMPTY_ROOT_NODE: LeafNode<(), ()> = LeafNode {
parent: ptr::null(),
parent_idx: 0,
parent_idx: MaybeUninit::uninitialized(),
len: 0,
keys: [(); CAPACITY],
vals: [(); CAPACITY],
keys: MaybeUninit::uninitialized(),
vals: MaybeUninit::uninitialized(),
};

/// The underlying representation of internal nodes. As with `LeafNode`s, these should be hidden
Expand Down Expand Up @@ -430,7 +427,7 @@ impl<BorrowType, K, V, Type> NodeRef<BorrowType, K, V, Type> {
root: self.root,
_marker: PhantomData
},
idx: self.as_leaf().parent_idx as usize,
idx: unsafe { usize::from(*self.as_leaf().parent_idx.get_ref()) },
_marker: PhantomData
})
} else {
Expand Down Expand Up @@ -567,7 +564,7 @@ impl<'a, K: 'a, V: 'a, Type> NodeRef<marker::Immut<'a>, K, V, Type> {
// the node, which is allowed by LLVM.
unsafe {
slice::from_raw_parts(
self.as_leaf().keys.as_ptr(),
self.as_leaf().keys.as_ptr() as *const K,
self.len()
)
}
Expand All @@ -578,7 +575,7 @@ impl<'a, K: 'a, V: 'a, Type> NodeRef<marker::Immut<'a>, K, V, Type> {
debug_assert!(!self.is_shared_root());
unsafe {
slice::from_raw_parts(
self.as_leaf().vals.as_ptr(),
self.as_leaf().vals.as_ptr() as *const V,
self.len()
)
}
Expand All @@ -605,7 +602,7 @@ impl<'a, K: 'a, V: 'a, Type> NodeRef<marker::Mut<'a>, K, V, Type> {
} else {
unsafe {
slice::from_raw_parts_mut(
&mut self.as_leaf_mut().keys as *mut [K] as *mut K,
self.as_leaf_mut().keys.get_mut() as *mut [K] as *mut K,
self.len()
)
}
Expand All @@ -616,7 +613,7 @@ impl<'a, K: 'a, V: 'a, Type> NodeRef<marker::Mut<'a>, K, V, Type> {
debug_assert!(!self.is_shared_root());
unsafe {
slice::from_raw_parts_mut(
&mut self.as_leaf_mut().vals as *mut [V] as *mut V,
self.as_leaf_mut().vals.get_mut() as *mut [V] as *mut V,
self.len()
)
}
Expand Down Expand Up @@ -1013,7 +1010,7 @@ impl<'a, K, V> Handle<NodeRef<marker::Mut<'a>, K, V, marker::Internal>, marker::
let ptr = self.node.as_internal_mut() as *mut _;
let mut child = self.descend();
child.as_leaf_mut().parent = ptr;
child.as_leaf_mut().parent_idx = idx;
child.as_leaf_mut().parent_idx.set(idx);
}

/// Unsafely asserts to the compiler some static information about whether the underlying
Expand Down Expand Up @@ -1152,12 +1149,12 @@ impl<'a, K, V> Handle<NodeRef<marker::Mut<'a>, K, V, marker::Leaf>, marker::KV>

ptr::copy_nonoverlapping(
self.node.keys().as_ptr().add(self.idx + 1),
new_node.keys.as_mut_ptr(),
new_node.keys.as_mut_ptr() as *mut K,
new_len
);
ptr::copy_nonoverlapping(
self.node.vals().as_ptr().add(self.idx + 1),
new_node.vals.as_mut_ptr(),
new_node.vals.as_mut_ptr() as *mut V,
new_len
);

Expand Down Expand Up @@ -1210,12 +1207,12 @@ impl<'a, K, V> Handle<NodeRef<marker::Mut<'a>, K, V, marker::Internal>, marker::

ptr::copy_nonoverlapping(
self.node.keys().as_ptr().add(self.idx + 1),
new_node.data.keys.as_mut_ptr(),
new_node.data.keys.as_mut_ptr() as *mut K,
new_len
);
ptr::copy_nonoverlapping(
self.node.vals().as_ptr().add(self.idx + 1),
new_node.data.vals.as_mut_ptr(),
new_node.data.vals.as_mut_ptr() as *mut V,
new_len
);
ptr::copy_nonoverlapping(
Expand Down
1 change: 1 addition & 0 deletions src/liballoc/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@
#![feature(rustc_const_unstable)]
#![feature(const_vec_new)]
#![feature(slice_partition_dedup)]
#![feature(maybe_uninit)]

// Allow testing this library

Expand Down