From e61f126fb48f3f21cc019b7dc005b5287cef79e5 Mon Sep 17 00:00:00 2001 From: Mark Rousskov Date: Wed, 18 Mar 2020 10:25:46 -0400 Subject: [PATCH 1/7] Replace shared root with optional root This simplifies the node manipulation, as we can (in later commits) always know when traversing nodes that we are not in a shared root. --- src/liballoc/collections/btree/map.rs | 189 +++++++++++++++----------- 1 file changed, 112 insertions(+), 77 deletions(-) diff --git a/src/liballoc/collections/btree/map.rs b/src/liballoc/collections/btree/map.rs index 9da324ba2d4f1..478fa8e15268e 100644 --- a/src/liballoc/collections/btree/map.rs +++ b/src/liballoc/collections/btree/map.rs @@ -122,7 +122,7 @@ use UnderflowResult::*; /// ``` #[stable(feature = "rust1", since = "1.0.0")] pub struct BTreeMap { - root: node::Root, + root: Option>, length: usize, } @@ -147,10 +147,11 @@ impl Clone for BTreeMap { { match node.force() { Leaf(leaf) => { - let mut out_tree = BTreeMap { root: node::Root::new_leaf(), length: 0 }; + let mut out_tree = BTreeMap { root: Some(node::Root::new_leaf()), length: 0 }; { - let mut out_node = match out_tree.root.as_mut().force() { + let root = out_tree.root.as_mut().unwrap(); + let mut out_node = match root.as_mut().force() { Leaf(leaf) => leaf, Internal(_) => unreachable!(), }; @@ -170,8 +171,13 @@ impl Clone for BTreeMap { Internal(internal) => { let mut out_tree = clone_subtree(internal.first_edge().descend()); + // Cannot call ensure_root_is_owned() because lacking K: Ord + if out_tree.root.is_none() { + out_tree.root = Some(node::Root::new_leaf()); + } + { - let mut out_node = out_tree.root.push_level(); + let mut out_node = out_tree.root.as_mut().unwrap().push_level(); let mut in_edge = internal.first_edge(); while let Ok(kv) = in_edge.right_kv() { let (k, v) = kv.into_kv(); @@ -190,7 +196,7 @@ impl Clone for BTreeMap { (root, length) }; - out_node.push(k, v, subroot); + out_node.push(k, v, subroot.unwrap_or_else(|| node::Root::new_leaf())); out_tree.length += 1 + sublength; } } @@ -203,9 +209,9 @@ impl Clone for BTreeMap { if self.is_empty() { // Ideally we'd call `BTreeMap::new` here, but that has the `K: // Ord` constraint, which this method lacks. - BTreeMap { root: node::Root::shared_empty_root(), length: 0 } + BTreeMap { root: None, length: 0 } } else { - clone_subtree(self.root.as_ref()) + clone_subtree(self.root.as_ref().unwrap().as_ref()) } } @@ -271,14 +277,14 @@ where type Key = K; fn get(&self, key: &Q) -> Option<&K> { - match search::search_tree(self.root.as_ref(), key) { + match search::search_tree(self.root.as_ref()?.as_ref(), key) { Found(handle) => Some(handle.into_kv().0), GoDown(_) => None, } } fn take(&mut self, key: &Q) -> Option { - match search::search_tree(self.root.as_mut(), key) { + match search::search_tree(self.root.as_mut()?.as_mut(), key) { Found(handle) => Some( OccupiedEntry { handle, length: &mut self.length, _marker: PhantomData } .remove_kv() @@ -290,7 +296,7 @@ where fn replace(&mut self, key: K) -> Option { self.ensure_root_is_owned(); - match search::search_tree::, K, (), K>(self.root.as_mut(), &key) { + match search::search_tree::, K, (), K>(self.root.as_mut()?.as_mut(), &key) { Found(handle) => Some(mem::replace(handle.into_kv_mut().0, key)), GoDown(handle) => { VacantEntry { key, handle, length: &mut self.length, _marker: PhantomData } @@ -344,15 +350,18 @@ pub struct IterMut<'a, K: 'a, V: 'a> { /// [`BTreeMap`]: struct.BTreeMap.html #[stable(feature = "rust1", since = "1.0.0")] pub struct IntoIter { - front: Handle, marker::Edge>, - back: Handle, marker::Edge>, + front: Option, marker::Edge>>, + back: Option, marker::Edge>>, length: usize, } #[stable(feature = "collection_debug", since = "1.17.0")] impl fmt::Debug for IntoIter { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - let range = Range { front: self.front.reborrow(), back: self.back.reborrow() }; + let range = Range { + front: self.front.as_ref().map(|f| f.reborrow()), + back: self.back.as_ref().map(|b| b.reborrow()), + }; f.debug_list().entries(range).finish() } } @@ -417,8 +426,8 @@ pub struct ValuesMut<'a, K: 'a, V: 'a> { /// [`BTreeMap`]: struct.BTreeMap.html #[stable(feature = "btree_range", since = "1.17.0")] pub struct Range<'a, K: 'a, V: 'a> { - front: Handle, K, V, marker::Leaf>, marker::Edge>, - back: Handle, K, V, marker::Leaf>, marker::Edge>, + front: Option, K, V, marker::Leaf>, marker::Edge>>, + back: Option, K, V, marker::Leaf>, marker::Edge>>, } #[stable(feature = "collection_debug", since = "1.17.0")] @@ -437,8 +446,8 @@ impl fmt::Debug for Range<'_, K, V> { /// [`BTreeMap`]: struct.BTreeMap.html #[stable(feature = "btree_range", since = "1.17.0")] pub struct RangeMut<'a, K: 'a, V: 'a> { - front: Handle, K, V, marker::Leaf>, marker::Edge>, - back: Handle, K, V, marker::Leaf>, marker::Edge>, + front: Option, K, V, marker::Leaf>, marker::Edge>>, + back: Option, K, V, marker::Leaf>, marker::Edge>>, // Be invariant in `K` and `V` _marker: PhantomData<&'a mut (K, V)>, @@ -447,7 +456,10 @@ pub struct RangeMut<'a, K: 'a, V: 'a> { #[stable(feature = "collection_debug", since = "1.17.0")] impl fmt::Debug for RangeMut<'_, K, V> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - let range = Range { front: self.front.reborrow(), back: self.back.reborrow() }; + let range = Range { + front: self.front.as_ref().map(|f| f.reborrow()), + back: self.back.as_ref().map(|b| b.reborrow()), + }; f.debug_list().entries(range).finish() } } @@ -544,7 +556,7 @@ impl BTreeMap { /// ``` #[stable(feature = "rust1", since = "1.0.0")] pub fn new() -> BTreeMap { - BTreeMap { root: node::Root::shared_empty_root(), length: 0 } + BTreeMap { root: None, length: 0 } } /// Clears the map, removing all elements. @@ -589,7 +601,7 @@ impl BTreeMap { K: Borrow, Q: Ord, { - match search::search_tree(self.root.as_ref(), key) { + match search::search_tree(self.root.as_ref()?.as_ref(), key) { Found(handle) => Some(handle.into_kv().1), GoDown(_) => None, } @@ -616,7 +628,7 @@ impl BTreeMap { K: Borrow, Q: Ord, { - match search::search_tree(self.root.as_ref(), k) { + match search::search_tree(self.root.as_ref()?.as_ref(), k) { Found(handle) => Some(handle.into_kv()), GoDown(_) => None, } @@ -645,7 +657,7 @@ impl BTreeMap { T: Ord, K: Borrow, { - let front = self.root.as_ref().first_leaf_edge(); + let front = self.root.as_ref()?.as_ref().first_leaf_edge(); front.right_kv().ok().map(Handle::into_kv) } @@ -674,7 +686,7 @@ impl BTreeMap { T: Ord, K: Borrow, { - let front = self.root.as_mut().first_leaf_edge(); + let front = self.root.as_mut()?.as_mut().first_leaf_edge(); if let Ok(kv) = front.right_kv() { Some(OccupiedEntry { handle: kv.forget_node_type(), @@ -708,7 +720,7 @@ impl BTreeMap { T: Ord, K: Borrow, { - let back = self.root.as_ref().last_leaf_edge(); + let back = self.root.as_ref()?.as_ref().last_leaf_edge(); back.left_kv().ok().map(Handle::into_kv) } @@ -737,7 +749,7 @@ impl BTreeMap { T: Ord, K: Borrow, { - let back = self.root.as_mut().last_leaf_edge(); + let back = self.root.as_mut()?.as_mut().last_leaf_edge(); if let Ok(kv) = back.left_kv() { Some(OccupiedEntry { handle: kv.forget_node_type(), @@ -801,7 +813,7 @@ impl BTreeMap { K: Borrow, Q: Ord, { - match search::search_tree(self.root.as_mut(), key) { + match search::search_tree(self.root.as_mut()?.as_mut(), key) { Found(handle) => Some(handle.into_kv_mut().1), GoDown(_) => None, } @@ -896,7 +908,7 @@ impl BTreeMap { K: Borrow, Q: Ord, { - match search::search_tree(self.root.as_mut(), key) { + match search::search_tree(self.root.as_mut()?.as_mut(), key) { Found(handle) => Some( OccupiedEntry { handle, length: &mut self.length, _marker: PhantomData } .remove_entry(), @@ -992,11 +1004,15 @@ impl BTreeMap { K: Borrow, R: RangeBounds, { - let root1 = self.root.as_ref(); - let root2 = self.root.as_ref(); - let (f, b) = range_search(root1, root2, range); + if let Some(root) = &self.root { + let root1 = root.as_ref(); + let root2 = root.as_ref(); + let (f, b) = range_search(root1, root2, range); - Range { front: f, back: b } + Range { front: Some(f), back: Some(b) } + } else { + Range { front: None, back: None } + } } /// Constructs a mutable double-ended iterator over a sub-range of elements in the map. @@ -1036,11 +1052,15 @@ impl BTreeMap { K: Borrow, R: RangeBounds, { - let root1 = self.root.as_mut(); - let root2 = unsafe { ptr::read(&root1) }; - let (f, b) = range_search(root1, root2, range); + if let Some(root) = &mut self.root { + let root1 = root.as_mut(); + let root2 = unsafe { ptr::read(&root1) }; + let (f, b) = range_search(root1, root2, range); - RangeMut { front: f, back: b, _marker: PhantomData } + RangeMut { front: Some(f), back: Some(b), _marker: PhantomData } + } else { + RangeMut { front: None, back: None, _marker: PhantomData } + } } /// Gets the given key's corresponding entry in the map for in-place manipulation. @@ -1065,7 +1085,7 @@ impl BTreeMap { pub fn entry(&mut self, key: K) -> Entry<'_, K, V> { // FIXME(@porglezomp) Avoid allocating if we don't insert self.ensure_root_is_owned(); - match search::search_tree(self.root.as_mut(), &key) { + match search::search_tree(self.root.as_mut().unwrap().as_mut(), &key) { Found(handle) => { Occupied(OccupiedEntry { handle, length: &mut self.length, _marker: PhantomData }) } @@ -1077,7 +1097,7 @@ impl BTreeMap { fn from_sorted_iter>(&mut self, iter: I) { self.ensure_root_is_owned(); - let mut cur_node = self.root.as_mut().last_leaf_edge().into_node(); + let mut cur_node = self.root.as_mut().unwrap().as_mut().last_leaf_edge().into_node(); // Iterate through all key-value pairs, pushing them into nodes at the right level. for (key, value) in iter { // Try to push key-value pair into the current leaf node. @@ -1126,7 +1146,7 @@ impl BTreeMap { fn fix_right_edge(&mut self) { // Handle underfull nodes, start from the top. - let mut cur_node = self.root.as_mut(); + let mut cur_node = self.root.as_mut().unwrap().as_mut(); while let Internal(internal) = cur_node.force() { // Check if right-most child is underfull. let mut last_edge = internal.last_edge(); @@ -1187,14 +1207,14 @@ impl BTreeMap { let total_num = self.len(); let mut right = Self::new(); - right.root = node::Root::new_leaf(); - for _ in 0..(self.root.as_ref().height()) { - right.root.push_level(); + right.root = Some(node::Root::new_leaf()); + for _ in 0..(self.root.as_ref().unwrap().as_ref().height()) { + right.root.as_mut().unwrap().push_level(); } { - let mut left_node = self.root.as_mut(); - let mut right_node = right.root.as_mut(); + let mut left_node = self.root.as_mut().unwrap().as_mut(); + let mut right_node = right.root.as_mut().unwrap().as_mut(); loop { let mut split_edge = match search::search_node(left_node, key) { @@ -1223,7 +1243,9 @@ impl BTreeMap { self.fix_right_border(); right.fix_left_border(); - if self.root.as_ref().height() < right.root.as_ref().height() { + if self.root.as_ref().unwrap().as_ref().height() + < right.root.as_ref().unwrap().as_ref().height() + { self.recalc_length(); right.length = total_num - self.len(); } else { @@ -1261,19 +1283,19 @@ impl BTreeMap { res } - self.length = dfs(self.root.as_ref()); + self.length = dfs(self.root.as_ref().unwrap().as_ref()); } /// Removes empty levels on the top. fn fix_top(&mut self) { loop { { - let node = self.root.as_ref(); + let node = self.root.as_ref().unwrap().as_ref(); if node.height() == 0 || node.len() > 0 { break; } } - self.root.pop_level(); + self.root.as_mut().unwrap().pop_level(); } } @@ -1281,7 +1303,7 @@ impl BTreeMap { self.fix_top(); { - let mut cur_node = self.root.as_mut(); + let mut cur_node = self.root.as_mut().unwrap().as_mut(); while let Internal(node) = cur_node.force() { let mut last_kv = node.last_kv(); @@ -1307,7 +1329,7 @@ impl BTreeMap { self.fix_top(); { - let mut cur_node = self.root.as_mut(); + let mut cur_node = self.root.as_mut().unwrap().as_mut(); while let Internal(node) = cur_node.force() { let mut first_kv = node.first_kv(); @@ -1329,8 +1351,8 @@ impl BTreeMap { /// If the root node is the shared root node, allocate our own node. fn ensure_root_is_owned(&mut self) { - if self.root.is_shared_root() { - self.root = node::Root::new_leaf(); + if self.root.is_none() { + self.root = Some(node::Root::new_leaf()); } } } @@ -1458,12 +1480,21 @@ impl IntoIterator for BTreeMap { type IntoIter = IntoIter; fn into_iter(self) -> IntoIter { - let root1 = unsafe { ptr::read(&self.root).into_ref() }; - let root2 = unsafe { ptr::read(&self.root).into_ref() }; + if self.root.is_none() { + mem::forget(self); + return IntoIter { front: None, back: None, length: 0 }; + } + + let root1 = unsafe { unwrap_unchecked(ptr::read(&self.root)).into_ref() }; + let root2 = unsafe { unwrap_unchecked(ptr::read(&self.root)).into_ref() }; let len = self.length; mem::forget(self); - IntoIter { front: root1.first_leaf_edge(), back: root2.last_leaf_edge(), length: len } + IntoIter { + front: Some(root1.first_leaf_edge()), + back: Some(root2.last_leaf_edge()), + length: len, + } } } @@ -1480,7 +1511,8 @@ impl Drop for IntoIter { // No need to avoid the shared root, because the tree was definitely not empty. unsafe { - let mut node = ptr::read(&self.0.front).into_node().forget_type(); + let mut node = + unwrap_unchecked(ptr::read(&self.0.front)).into_node().forget_type(); while let Some(parent) = node.deallocate_and_ascend() { node = parent.into_node().forget_type(); } @@ -1495,14 +1527,13 @@ impl Drop for IntoIter { } unsafe { - let mut node = ptr::read(&self.front).into_node().forget_type(); - if node.is_shared_root() { - return; - } - // Most of the nodes have been deallocated while traversing - // but one pile from a leaf up to the root is left standing. - while let Some(parent) = node.deallocate_and_ascend() { - node = parent.into_node().forget_type(); + if let Some(front) = ptr::read(&self.front) { + let mut node = front.into_node().forget_type(); + // Most of the nodes have been deallocated while traversing + // but one pile from a leaf up to the root is left standing. + while let Some(parent) = node.deallocate_and_ascend() { + node = parent.into_node().forget_type(); + } } } } @@ -1517,7 +1548,7 @@ impl Iterator for IntoIter { None } else { self.length -= 1; - Some(unsafe { self.front.next_unchecked() }) + Some(unsafe { self.front.as_mut().unwrap().next_unchecked() }) } } @@ -1533,7 +1564,7 @@ impl DoubleEndedIterator for IntoIter { None } else { self.length -= 1; - Some(unsafe { self.back.next_back_unchecked() }) + Some(unsafe { self.back.as_mut().unwrap().next_back_unchecked() }) } } } @@ -1683,7 +1714,7 @@ impl<'a, K, V> Range<'a, K, V> { } unsafe fn next_unchecked(&mut self) -> (&'a K, &'a V) { - self.front.next_unchecked() + unwrap_unchecked(self.front.as_mut()).next_unchecked() } } @@ -1696,7 +1727,7 @@ impl<'a, K, V> DoubleEndedIterator for Range<'a, K, V> { impl<'a, K, V> Range<'a, K, V> { unsafe fn next_back_unchecked(&mut self) -> (&'a K, &'a V) { - self.back.next_back_unchecked() + unwrap_unchecked(self.back.as_mut()).next_back_unchecked() } } @@ -1734,7 +1765,7 @@ impl<'a, K, V> RangeMut<'a, K, V> { } unsafe fn next_unchecked(&mut self) -> (&'a mut K, &'a mut V) { - self.front.next_unchecked() + unwrap_unchecked(self.front.as_mut()).next_unchecked() } } @@ -1755,7 +1786,7 @@ impl FusedIterator for RangeMut<'_, K, V> {} impl<'a, K, V> RangeMut<'a, K, V> { unsafe fn next_back_unchecked(&mut self) -> (&'a mut K, &'a mut V) { - self.back.next_back_unchecked() + unwrap_unchecked(self.back.as_mut()).next_back_unchecked() } } @@ -1969,8 +2000,8 @@ impl BTreeMap { pub fn iter(&self) -> Iter<'_, K, V> { Iter { range: Range { - front: self.root.as_ref().first_leaf_edge(), - back: self.root.as_ref().last_leaf_edge(), + front: self.root.as_ref().map(|r| r.as_ref().first_leaf_edge()), + back: self.root.as_ref().map(|r| r.as_ref().last_leaf_edge()), }, length: self.length, } @@ -1999,13 +2030,17 @@ impl BTreeMap { /// ``` #[stable(feature = "rust1", since = "1.0.0")] pub fn iter_mut(&mut self) -> IterMut<'_, K, V> { - let root1 = self.root.as_mut(); - let root2 = unsafe { ptr::read(&root1) }; IterMut { - range: RangeMut { - front: root1.first_leaf_edge(), - back: root2.last_leaf_edge(), - _marker: PhantomData, + range: if let Some(root) = &mut self.root { + let root1 = root.as_mut(); + let root2 = unsafe { ptr::read(&root1) }; + RangeMut { + front: Some(root1.first_leaf_edge()), + back: Some(root2.last_leaf_edge()), + _marker: PhantomData, + } + } else { + RangeMut { front: None, back: None, _marker: PhantomData } }, length: self.length, } From 1c44f852df205ae6d363cc9a2b12f8093b3ed342 Mon Sep 17 00:00:00 2001 From: Mark Rousskov Date: Wed, 18 Mar 2020 10:28:33 -0400 Subject: [PATCH 2/7] Remove shared root code and assertions from BTree nodes --- src/liballoc/collections/btree/node.rs | 53 +----------------------- src/liballoc/collections/btree/search.rs | 14 +++---- 2 files changed, 8 insertions(+), 59 deletions(-) diff --git a/src/liballoc/collections/btree/node.rs b/src/liballoc/collections/btree/node.rs index 1132ffdaf8005..5530c9593a51a 100644 --- a/src/liballoc/collections/btree/node.rs +++ b/src/liballoc/collections/btree/node.rs @@ -111,21 +111,6 @@ impl LeafNode { } } -impl NodeHeader { - fn is_shared_root(&self) -> bool { - ptr::eq(self, &EMPTY_ROOT_NODE as *const _ as *const _) - } -} - -// We need to implement Sync here in order to make a static instance. -unsafe impl Sync for NodeHeader<(), ()> {} - -// An empty node used as a placeholder for the root node, to avoid allocations. -// We use just a header in order to save space, since no operation on an empty tree will -// ever take a pointer past the first key. -static EMPTY_ROOT_NODE: NodeHeader<(), ()> = - NodeHeader { parent: ptr::null(), parent_idx: MaybeUninit::uninit(), len: 0 }; - /// The underlying representation of internal nodes. As with `LeafNode`s, these should be hidden /// behind `BoxedNode`s to prevent dropping uninitialized keys and values. Any pointer to an /// `InternalNode` can be directly casted to a pointer to the underlying `LeafNode` portion of the @@ -154,8 +139,8 @@ impl InternalNode { } /// A managed, non-null pointer to a node. This is either an owned pointer to -/// `LeafNode`, an owned pointer to `InternalNode`, or a (not owned) -/// pointer to `NodeHeader<(), ()` (more specifically, the pointer to EMPTY_ROOT_NODE). +/// `LeafNode` or an owned pointer to `InternalNode`. +/// /// All of these types have a `NodeHeader` prefix, meaning that they have at /// least the same size as `NodeHeader` and store the same kinds of data at the same /// offsets; and they have a pointer alignment at least as large as `NodeHeader`'s. @@ -196,20 +181,6 @@ unsafe impl Sync for Root {} unsafe impl Send for Root {} impl Root { - /// Whether the instance of `Root` wraps a shared, empty root node. If not, - /// the entire tree is uniquely owned by the owner of the `Root` instance. - pub fn is_shared_root(&self) -> bool { - self.as_ref().is_shared_root() - } - - /// Returns a shared tree, wrapping a shared root node that is eternally empty. - pub fn shared_empty_root() -> Self { - Root { - node: unsafe { BoxedNode::from_ptr(NonNull::from(&EMPTY_ROOT_NODE).cast()) }, - height: 0, - } - } - /// Returns a new owned tree, with its own root node that is initially empty. pub fn new_leaf() -> Self { Root { node: BoxedNode::from_leaf(Box::new(unsafe { LeafNode::new() })), height: 0 } @@ -245,7 +216,6 @@ impl Root { /// Adds a new internal node with a single edge, pointing to the previous root, and make that /// new node the root. This increases the height by 1 and is the opposite of `pop_level`. pub fn push_level(&mut self) -> NodeRef, K, V, marker::Internal> { - debug_assert!(!self.is_shared_root()); let mut new_node = Box::new(unsafe { InternalNode::new() }); new_node.edges[0].write(unsafe { BoxedNode::from_ptr(self.node.as_ptr()) }); @@ -381,7 +351,6 @@ impl NodeRef { /// Unsafe because the node must not be the shared root. For more information, /// see the `NodeRef` comments. unsafe fn as_leaf(&self) -> &LeafNode { - debug_assert!(!self.is_shared_root()); self.node.as_ref() } @@ -389,11 +358,6 @@ impl NodeRef { unsafe { &*(self.node.as_ptr() as *const NodeHeader) } } - /// Returns whether the node is the shared, empty root. - pub fn is_shared_root(&self) -> bool { - self.as_header().is_shared_root() - } - /// Borrows a view into the keys stored in the node. /// Unsafe because the caller must ensure that the node is not the shared root. pub unsafe fn keys(&self) -> &[K] { @@ -464,7 +428,6 @@ impl NodeRef { pub unsafe fn deallocate_and_ascend( self, ) -> Option, marker::Edge>> { - assert!(!self.is_shared_root()); let height = self.height; let node = self.node; let ret = self.ascend().ok(); @@ -527,14 +490,12 @@ impl<'a, K, V, Type> NodeRef, K, V, Type> { impl<'a, K: 'a, V: 'a, Type> NodeRef, K, V, Type> { /// Unsafe because the caller must ensure that the node is not the shared root. unsafe fn into_key_slice(self) -> &'a [K] { - debug_assert!(!self.is_shared_root()); // We cannot be the shared root, so `as_leaf` is okay. slice::from_raw_parts(MaybeUninit::first_ptr(&self.as_leaf().keys), self.len()) } /// Unsafe because the caller must ensure that the node is not the shared root. unsafe fn into_val_slice(self) -> &'a [V] { - debug_assert!(!self.is_shared_root()); // We cannot be the shared root, so `as_leaf` is okay. slice::from_raw_parts(MaybeUninit::first_ptr(&self.as_leaf().vals), self.len()) } @@ -555,7 +516,6 @@ impl<'a, K: 'a, V: 'a, Type> NodeRef, K, V, Type> { /// Unsafe because the caller must ensure that the node is not the shared root. unsafe fn into_key_slice_mut(mut self) -> &'a mut [K] { - debug_assert!(!self.is_shared_root()); // We cannot be the shared root, so `as_leaf_mut` is okay. slice::from_raw_parts_mut( MaybeUninit::first_ptr_mut(&mut (*self.as_leaf_mut()).keys), @@ -565,7 +525,6 @@ impl<'a, K: 'a, V: 'a, Type> NodeRef, K, V, Type> { /// Unsafe because the caller must ensure that the node is not the shared root. unsafe fn into_val_slice_mut(mut self) -> &'a mut [V] { - debug_assert!(!self.is_shared_root()); slice::from_raw_parts_mut( MaybeUninit::first_ptr_mut(&mut (*self.as_leaf_mut()).vals), self.len(), @@ -574,7 +533,6 @@ impl<'a, K: 'a, V: 'a, Type> NodeRef, K, V, Type> { /// Unsafe because the caller must ensure that the node is not the shared root. unsafe fn into_slices_mut(mut self) -> (&'a mut [K], &'a mut [V]) { - debug_assert!(!self.is_shared_root()); // We cannot use the getters here, because calling the second one // invalidates the reference returned by the first. // More precisely, it is the call to `len` that is the culprit, @@ -592,7 +550,6 @@ impl<'a, K, V> NodeRef, K, V, marker::Leaf> { /// Adds a key/value pair the end of the node. pub fn push(&mut self, key: K, val: V) { assert!(self.len() < CAPACITY); - debug_assert!(!self.is_shared_root()); let idx = self.len(); @@ -607,7 +564,6 @@ impl<'a, K, V> NodeRef, K, V, marker::Leaf> { /// Adds a key/value pair to the beginning of the node. pub fn push_front(&mut self, key: K, val: V) { assert!(self.len() < CAPACITY); - debug_assert!(!self.is_shared_root()); unsafe { slice_insert(self.keys_mut(), 0, key); @@ -624,7 +580,6 @@ impl<'a, K, V> NodeRef, K, V, marker::Internal> { pub fn push(&mut self, key: K, val: V, edge: Root) { assert!(edge.height == self.height - 1); assert!(self.len() < CAPACITY); - debug_assert!(!self.is_shared_root()); let idx = self.len(); @@ -658,7 +613,6 @@ impl<'a, K, V> NodeRef, K, V, marker::Internal> { pub fn push_front(&mut self, key: K, val: V, edge: Root) { assert!(edge.height == self.height - 1); assert!(self.len() < CAPACITY); - debug_assert!(!self.is_shared_root()); unsafe { slice_insert(self.keys_mut(), 0, key); @@ -904,7 +858,6 @@ impl<'a, K, V> Handle, K, V, marker::Leaf>, marker::Edge fn insert_fit(&mut self, key: K, val: V) -> *mut V { // Necessary for correctness, but in a private module debug_assert!(self.node.len() < CAPACITY); - debug_assert!(!self.node.is_shared_root()); unsafe { slice_insert(self.node.keys_mut(), self.idx, key); @@ -1081,7 +1034,6 @@ impl<'a, K, V> Handle, K, V, marker::Leaf>, marker::KV> /// - All the key/value pairs to the right of this handle are put into a newly /// allocated node. pub fn split(mut self) -> (NodeRef, K, V, marker::Leaf>, K, V, Root) { - assert!(!self.node.is_shared_root()); unsafe { let mut new_node = Box::new(LeafNode::new()); @@ -1113,7 +1065,6 @@ impl<'a, K, V> Handle, K, V, marker::Leaf>, marker::KV> pub fn remove( mut self, ) -> (Handle, K, V, marker::Leaf>, marker::Edge>, K, V) { - assert!(!self.node.is_shared_root()); unsafe { let k = slice_remove(self.node.keys_mut(), self.idx); let v = slice_remove(self.node.vals_mut(), self.idx); diff --git a/src/liballoc/collections/btree/search.rs b/src/liballoc/collections/btree/search.rs index 2ba5cebbdee74..f9354263864e5 100644 --- a/src/liballoc/collections/btree/search.rs +++ b/src/liballoc/collections/btree/search.rs @@ -72,14 +72,12 @@ where // Using `keys()` is fine here even if BorrowType is mutable, as all we return // is an index -- not a reference. let len = node.len(); - if len > 0 { - let keys = unsafe { node.keys() }; // safe because a non-empty node cannot be the shared root - for (i, k) in keys.iter().enumerate() { - match key.cmp(k.borrow()) { - Ordering::Greater => {} - Ordering::Equal => return (i, true), - Ordering::Less => return (i, false), - } + let keys = unsafe { node.keys() }; // safe because a non-empty node cannot be the shared root + for (i, k) in keys.iter().enumerate() { + match key.cmp(k.borrow()) { + Ordering::Greater => {} + Ordering::Equal => return (i, true), + Ordering::Less => return (i, false), } } (len, false) From 3c04fda751352fe00884782e7adf792a5e2a91c4 Mon Sep 17 00:00:00 2001 From: Mark Rousskov Date: Wed, 18 Mar 2020 10:45:35 -0400 Subject: [PATCH 3/7] Make functions dependent only on shared root avoidance safe --- src/liballoc/collections/btree/map.rs | 4 +- src/liballoc/collections/btree/node.rs | 107 ++++++++++++----------- src/liballoc/collections/btree/search.rs | 5 +- 3 files changed, 58 insertions(+), 58 deletions(-) diff --git a/src/liballoc/collections/btree/map.rs b/src/liballoc/collections/btree/map.rs index 478fa8e15268e..21f99257d81d0 100644 --- a/src/liballoc/collections/btree/map.rs +++ b/src/liballoc/collections/btree/map.rs @@ -1349,7 +1349,8 @@ impl BTreeMap { self.fix_top(); } - /// If the root node is the shared root node, allocate our own node. + /// If the root node is the empty (non-allocated) root node, allocate our + /// own node. fn ensure_root_is_owned(&mut self) { if self.root.is_none() { self.root = Some(node::Root::new_leaf()); @@ -1509,7 +1510,6 @@ impl Drop for IntoIter { // don't have to care about panics this time (they'll abort). while let Some(_) = self.0.next() {} - // No need to avoid the shared root, because the tree was definitely not empty. unsafe { let mut node = unwrap_unchecked(ptr::read(&self.0.front)).into_node().forget_type(); diff --git a/src/liballoc/collections/btree/node.rs b/src/liballoc/collections/btree/node.rs index 5530c9593a51a..99f531264ba20 100644 --- a/src/liballoc/collections/btree/node.rs +++ b/src/liballoc/collections/btree/node.rs @@ -169,8 +169,9 @@ impl BoxedNode { } } -/// Either an owned tree or a shared, empty tree. Note that this does not have a destructor, -/// and must be cleaned up manually if it is an owned tree. +/// An owned tree. +/// +/// Note that this does not have a destructor, and must be cleaned up manually. pub struct Root { node: BoxedNode, /// The number of levels below the root node. @@ -278,10 +279,7 @@ impl Root { /// `Leaf`, the `NodeRef` points to a leaf node, when this is `Internal` the /// `NodeRef` points to an internal node, and when this is `LeafOrInternal` the /// `NodeRef` could be pointing to either type of node. -/// Note that in case of a leaf node, this might still be the shared root! -/// Only turn this into a `LeafNode` reference if you know it is not the shared root! -/// Shared references must be dereferenceable *for the entire size of their pointee*, -/// so '&LeafNode` or `&InternalNode` pointing to the shared root is undefined behavior. +/// /// Turning this into a `NodeHeader` reference is always safe. pub struct NodeRef { /// The number of levels below the node. @@ -344,14 +342,15 @@ impl NodeRef { NodeRef { height: self.height, node: self.node, root: self.root, _marker: PhantomData } } - /// Exposes the leaf "portion" of any leaf or internal node that is not the shared root. + /// Exposes the leaf "portion" of any leaf or internal node. /// If the node is a leaf, this function simply opens up its data. /// If the node is an internal node, so not a leaf, it does have all the data a leaf has /// (header, keys and values), and this function exposes that. - /// Unsafe because the node must not be the shared root. For more information, - /// see the `NodeRef` comments. - unsafe fn as_leaf(&self) -> &LeafNode { - self.node.as_ref() + fn as_leaf(&self) -> &LeafNode { + // The node must be valid for at least the LeafNode portion. + // This is not a reference in the NodeRef type because we don't know if + // it should be unique or shared. + unsafe { self.node.as_ref() } } fn as_header(&self) -> &NodeHeader { @@ -359,14 +358,12 @@ impl NodeRef { } /// Borrows a view into the keys stored in the node. - /// Unsafe because the caller must ensure that the node is not the shared root. - pub unsafe fn keys(&self) -> &[K] { + pub fn keys(&self) -> &[K] { self.reborrow().into_key_slice() } /// Borrows a view into the values stored in the node. - /// Unsafe because the caller must ensure that the node is not the shared root. - unsafe fn vals(&self) -> &[V] { + fn vals(&self) -> &[V] { self.reborrow().into_val_slice() } @@ -470,39 +467,37 @@ impl<'a, K, V, Type> NodeRef, K, V, Type> { /// (header, keys and values), and this function exposes that. /// /// Returns a raw ptr to avoid asserting exclusive access to the entire node. - /// This also implies you can invoke this member on the shared root, but the resulting pointer - /// might not be properly aligned and definitely would not allow accessing keys and values. fn as_leaf_mut(&mut self) -> *mut LeafNode { self.node.as_ptr() } - /// Unsafe because the caller must ensure that the node is not the shared root. - unsafe fn keys_mut(&mut self) -> &mut [K] { - self.reborrow_mut().into_key_slice_mut() + fn keys_mut(&mut self) -> &mut [K] { + // SAFETY: the caller will not be able to call further methods on self + // until the key slice reference is dropped, as we have unique access + // for the lifetime of the borrow. + unsafe { self.reborrow_mut().into_key_slice_mut() } } - /// Unsafe because the caller must ensure that the node is not the shared root. - unsafe fn vals_mut(&mut self) -> &mut [V] { - self.reborrow_mut().into_val_slice_mut() + fn vals_mut(&mut self) -> &mut [V] { + // SAFETY: the caller will not be able to call further methods on self + // until the value slice reference is dropped, as we have unique access + // for the lifetime of the borrow. + unsafe { self.reborrow_mut().into_val_slice_mut() } } } impl<'a, K: 'a, V: 'a, Type> NodeRef, K, V, Type> { - /// Unsafe because the caller must ensure that the node is not the shared root. - unsafe fn into_key_slice(self) -> &'a [K] { - // We cannot be the shared root, so `as_leaf` is okay. - slice::from_raw_parts(MaybeUninit::first_ptr(&self.as_leaf().keys), self.len()) + fn into_key_slice(self) -> &'a [K] { + unsafe { slice::from_raw_parts(MaybeUninit::first_ptr(&self.as_leaf().keys), self.len()) } } - /// Unsafe because the caller must ensure that the node is not the shared root. - unsafe fn into_val_slice(self) -> &'a [V] { - // We cannot be the shared root, so `as_leaf` is okay. - slice::from_raw_parts(MaybeUninit::first_ptr(&self.as_leaf().vals), self.len()) + fn into_val_slice(self) -> &'a [V] { + unsafe { slice::from_raw_parts(MaybeUninit::first_ptr(&self.as_leaf().vals), self.len()) } } - /// Unsafe because the caller must ensure that the node is not the shared root. - unsafe fn into_slices(self) -> (&'a [K], &'a [V]) { - let k = ptr::read(&self); + fn into_slices(self) -> (&'a [K], &'a [V]) { + // SAFETY: equivalent to reborrow() except not requiring Type: 'a + let k = unsafe { ptr::read(&self) }; (k.into_key_slice(), self.into_val_slice()) } } @@ -514,25 +509,27 @@ impl<'a, K: 'a, V: 'a, Type> NodeRef, K, V, Type> { unsafe { &mut *(self.root as *mut Root) } } - /// Unsafe because the caller must ensure that the node is not the shared root. - unsafe fn into_key_slice_mut(mut self) -> &'a mut [K] { - // We cannot be the shared root, so `as_leaf_mut` is okay. - slice::from_raw_parts_mut( - MaybeUninit::first_ptr_mut(&mut (*self.as_leaf_mut()).keys), - self.len(), - ) + fn into_key_slice_mut(mut self) -> &'a mut [K] { + // SAFETY: The keys of a node must always be initialized up to length. + unsafe { + slice::from_raw_parts_mut( + MaybeUninit::first_ptr_mut(&mut (*self.as_leaf_mut()).keys), + self.len(), + ) + } } - /// Unsafe because the caller must ensure that the node is not the shared root. - unsafe fn into_val_slice_mut(mut self) -> &'a mut [V] { - slice::from_raw_parts_mut( - MaybeUninit::first_ptr_mut(&mut (*self.as_leaf_mut()).vals), - self.len(), - ) + fn into_val_slice_mut(mut self) -> &'a mut [V] { + // SAFETY: The values of a node must always be initialized up to length. + unsafe { + slice::from_raw_parts_mut( + MaybeUninit::first_ptr_mut(&mut (*self.as_leaf_mut()).vals), + self.len(), + ) + } } - /// Unsafe because the caller must ensure that the node is not the shared root. - unsafe fn into_slices_mut(mut self) -> (&'a mut [K], &'a mut [V]) { + fn into_slices_mut(mut self) -> (&'a mut [K], &'a mut [V]) { // We cannot use the getters here, because calling the second one // invalidates the reference returned by the first. // More precisely, it is the call to `len` that is the culprit, @@ -540,8 +537,13 @@ impl<'a, K: 'a, V: 'a, Type> NodeRef, K, V, Type> { // overlap with the keys (and even the values, for ZST keys). let len = self.len(); let leaf = self.as_leaf_mut(); - let keys = slice::from_raw_parts_mut(MaybeUninit::first_ptr_mut(&mut (*leaf).keys), len); - let vals = slice::from_raw_parts_mut(MaybeUninit::first_ptr_mut(&mut (*leaf).vals), len); + // SAFETY: The keys and values of a node must always be initialized up to length. + let keys = unsafe { + slice::from_raw_parts_mut(MaybeUninit::first_ptr_mut(&mut (*leaf).keys), len) + }; + let vals = unsafe { + slice::from_raw_parts_mut(MaybeUninit::first_ptr_mut(&mut (*leaf).vals), len) + }; (keys, vals) } } @@ -698,8 +700,7 @@ impl<'a, K, V> NodeRef, K, V, marker::LeafOrInternal> { } } - /// Unsafe because the caller must ensure that the node is not the shared root. - unsafe fn into_kv_pointers_mut(mut self) -> (*mut K, *mut V) { + fn into_kv_pointers_mut(mut self) -> (*mut K, *mut V) { (self.keys_mut().as_mut_ptr(), self.vals_mut().as_mut_ptr()) } } diff --git a/src/liballoc/collections/btree/search.rs b/src/liballoc/collections/btree/search.rs index f9354263864e5..4e80f7f21ebff 100644 --- a/src/liballoc/collections/btree/search.rs +++ b/src/liballoc/collections/btree/search.rs @@ -67,12 +67,11 @@ where Q: Ord, K: Borrow, { - // This function is defined over all borrow types (immutable, mutable, owned), - // and may be called on the shared root in each case. + // This function is defined over all borrow types (immutable, mutable, owned). // Using `keys()` is fine here even if BorrowType is mutable, as all we return // is an index -- not a reference. let len = node.len(); - let keys = unsafe { node.keys() }; // safe because a non-empty node cannot be the shared root + let keys = node.keys(); for (i, k) in keys.iter().enumerate() { match key.cmp(k.borrow()) { Ordering::Greater => {} From 54b7c38889ee7bb59f4d6449822fcfdfa3448ddb Mon Sep 17 00:00:00 2001 From: Mark Rousskov Date: Wed, 18 Mar 2020 11:10:21 -0400 Subject: [PATCH 4/7] Drop NodeHeader type from BTree code We no longer have a separate header because the shared root is gone; all code can work solely with leafs now. --- src/liballoc/collections/btree/node.rs | 46 +++----------------------- 1 file changed, 5 insertions(+), 41 deletions(-) diff --git a/src/liballoc/collections/btree/node.rs b/src/liballoc/collections/btree/node.rs index 99f531264ba20..6ebb98c42cd4f 100644 --- a/src/liballoc/collections/btree/node.rs +++ b/src/liballoc/collections/btree/node.rs @@ -44,34 +44,7 @@ const B: usize = 6; pub const MIN_LEN: usize = B - 1; pub const CAPACITY: usize = 2 * B - 1; -/// The underlying representation of leaf nodes. Note that it is often unsafe to actually store -/// these, since only the first `len` keys and values are assumed to be initialized. As such, -/// these should always be put behind pointers, and specifically behind `BoxedNode` in the owned -/// case. -/// -/// We have a separate type for the header and rely on it matching the prefix of `LeafNode`, in -/// order to statically allocate a single dummy node to avoid allocations. This struct is -/// `repr(C)` to prevent them from being reordered. `LeafNode` does not just contain a -/// `NodeHeader` because we do not want unnecessary padding between `len` and the keys. -/// Crucially, `NodeHeader` can be safely transmuted to different K and V. (This is exploited -/// by `as_header`.) -#[repr(C)] -struct NodeHeader { - /// We use `*const` as opposed to `*mut` so as to be covariant in `K` and `V`. - /// This either points to an actual node or is null. - parent: *const InternalNode, - - /// 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 non-null. - parent_idx: MaybeUninit, - - /// The number of keys and values this node stores. - /// - /// This next to `parent_idx` to encourage the compiler to join `len` and - /// `parent_idx` into the same 32-bit word, reducing space overhead. - len: u16, -} +/// The underlying representation of leaf nodes. #[repr(C)] struct LeafNode { /// We use `*const` as opposed to `*mut` so as to be covariant in `K` and `V`. @@ -141,10 +114,7 @@ impl InternalNode { /// A managed, non-null pointer to a node. This is either an owned pointer to /// `LeafNode` or an owned pointer to `InternalNode`. /// -/// All of these types have a `NodeHeader` prefix, meaning that they have at -/// least the same size as `NodeHeader` and store the same kinds of data at the same -/// offsets; and they have a pointer alignment at least as large as `NodeHeader`'s. -/// However, `BoxedNode` contains no information as to which of the three types +/// However, `BoxedNode` contains no information as to which of the two types /// of nodes it actually contains, and, partially due to this lack of information, /// has no destructor. struct BoxedNode { @@ -279,8 +249,6 @@ impl Root { /// `Leaf`, the `NodeRef` points to a leaf node, when this is `Internal` the /// `NodeRef` points to an internal node, and when this is `LeafOrInternal` the /// `NodeRef` could be pointing to either type of node. -/// -/// Turning this into a `NodeHeader` reference is always safe. pub struct NodeRef { /// The number of levels below the node. height: usize, @@ -322,7 +290,7 @@ impl NodeRef { /// Note that, despite being safe, calling this function can have the side effect /// of invalidating mutable references that unsafe code has created. pub fn len(&self) -> usize { - self.as_header().len as usize + self.as_leaf().len as usize } /// Returns the height of this node in the whole tree. Zero height denotes the @@ -353,10 +321,6 @@ impl NodeRef { unsafe { self.node.as_ref() } } - fn as_header(&self) -> &NodeHeader { - unsafe { &*(self.node.as_ptr() as *const NodeHeader) } - } - /// Borrows a view into the keys stored in the node. pub fn keys(&self) -> &[K] { self.reborrow().into_key_slice() @@ -377,7 +341,7 @@ impl NodeRef { pub fn ascend( self, ) -> Result, marker::Edge>, Self> { - let parent_as_leaf = self.as_header().parent as *const LeafNode; + let parent_as_leaf = self.as_leaf().parent as *const LeafNode; if let Some(non_zero) = NonNull::new(parent_as_leaf as *mut _) { Ok(Handle { node: NodeRef { @@ -386,7 +350,7 @@ impl NodeRef { root: self.root, _marker: PhantomData, }, - idx: unsafe { usize::from(*self.as_header().parent_idx.as_ptr()) }, + idx: unsafe { usize::from(*self.as_leaf().parent_idx.as_ptr()) }, _marker: PhantomData, }) } else { From 13f6d771bb7f3617e247d9806e1b9472022cdaf4 Mon Sep 17 00:00:00 2001 From: Mark Rousskov Date: Wed, 18 Mar 2020 19:00:15 -0400 Subject: [PATCH 5/7] Simplify ensure_root_is_owned callers This makes ensure_root_is_owned return a reference to the (now guaranteed to exist) root, allowing callers to operate on it without going through another unwrap. Unfortunately this is only rarely useful as it's frequently the case that both the length and the root need to be accessed and field-level borrows in methods don't yet exist. --- src/liballoc/collections/btree/map.rs | 28 +++++++++++++-------------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/src/liballoc/collections/btree/map.rs b/src/liballoc/collections/btree/map.rs index 21f99257d81d0..3ba7befc04609 100644 --- a/src/liballoc/collections/btree/map.rs +++ b/src/liballoc/collections/btree/map.rs @@ -170,13 +170,13 @@ impl Clone for BTreeMap { } Internal(internal) => { let mut out_tree = clone_subtree(internal.first_edge().descend()); - - // Cannot call ensure_root_is_owned() because lacking K: Ord - if out_tree.root.is_none() { - out_tree.root = Some(node::Root::new_leaf()); - } + out_tree.ensure_root_is_owned(); { + // Ideally we'd use the return of ensure_root_is_owned + // instead of re-unwrapping here but unfortunately that + // borrows all of out_tree and we need access to the + // length below. let mut out_node = out_tree.root.as_mut().unwrap().push_level(); let mut in_edge = internal.first_edge(); while let Ok(kv) = in_edge.right_kv() { @@ -1207,9 +1207,9 @@ impl BTreeMap { let total_num = self.len(); let mut right = Self::new(); - right.root = Some(node::Root::new_leaf()); + let right_root = right.ensure_root_is_owned(); for _ in 0..(self.root.as_ref().unwrap().as_ref().height()) { - right.root.as_mut().unwrap().push_level(); + right_root.push_level(); } { @@ -1348,14 +1348,6 @@ impl BTreeMap { self.fix_top(); } - - /// If the root node is the empty (non-allocated) root node, allocate our - /// own node. - fn ensure_root_is_owned(&mut self) { - if self.root.is_none() { - self.root = Some(node::Root::new_leaf()); - } - } } #[stable(feature = "rust1", since = "1.0.0")] @@ -2151,6 +2143,12 @@ impl BTreeMap { pub fn is_empty(&self) -> bool { self.len() == 0 } + + /// If the root node is the empty (non-allocated) root node, allocate our + /// own node. + fn ensure_root_is_owned(&mut self) -> &mut node::Root { + self.root.get_or_insert_with(|| node::Root::new_leaf()) + } } impl<'a, K: Ord, V> Entry<'a, K, V> { From 4d85314a0008ec809054854dff55c09913b1e80f Mon Sep 17 00:00:00 2001 From: Mark Rousskov Date: Fri, 20 Mar 2020 09:40:57 -0400 Subject: [PATCH 6/7] Update test commentary for shared root removal --- src/liballoc/tests/btree/map.rs | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/src/liballoc/tests/btree/map.rs b/src/liballoc/tests/btree/map.rs index d05eec19346de..3a3462d546f7a 100644 --- a/src/liballoc/tests/btree/map.rs +++ b/src/liballoc/tests/btree/map.rs @@ -67,7 +67,7 @@ fn test_basic_large() { #[test] fn test_basic_small() { let mut map = BTreeMap::new(); - // Empty, shared root: + // Empty, root is absent (None): assert_eq!(map.remove(&1), None); assert_eq!(map.len(), 0); assert_eq!(map.get(&1), None); @@ -123,7 +123,7 @@ fn test_basic_small() { assert_eq!(map.values().collect::>(), vec![&4]); assert_eq!(map.remove(&2), Some(4)); - // Empty but private root: + // Empty but root is owned (Some(...)): assert_eq!(map.len(), 0); assert_eq!(map.get(&1), None); assert_eq!(map.get_mut(&1), None); @@ -263,13 +263,6 @@ fn test_iter_mut_mutation() { do_test_iter_mut_mutation::(144); } -#[test] -fn test_into_key_slice_with_shared_root_past_bounds() { - let mut map: BTreeMap = BTreeMap::new(); - assert_eq!(map.get(&Align32(1)), None); - assert_eq!(map.get_mut(&Align32(1)), None); -} - #[test] fn test_values_mut() { let mut a = BTreeMap::new(); From bce7f6f3a09c0dc2e56c8b3a9f17c8a18cada009 Mon Sep 17 00:00:00 2001 From: Mark Rousskov Date: Wed, 18 Mar 2020 15:29:05 -0400 Subject: [PATCH 7/7] Fix debugger pretty printing of BTrees --- src/etc/gdb_rust_pretty_printing.py | 34 ++++++++++++-------- src/test/debuginfo/pretty-std-collections.rs | 24 ++++++++++---- 2 files changed, 38 insertions(+), 20 deletions(-) diff --git a/src/etc/gdb_rust_pretty_printing.py b/src/etc/gdb_rust_pretty_printing.py index 0914c22eb13f0..cae64ef6665bb 100755 --- a/src/etc/gdb_rust_pretty_printing.py +++ b/src/etc/gdb_rust_pretty_printing.py @@ -370,12 +370,17 @@ def to_string(self): ("(len: %i)" % self.__val.get_wrapped_value()['map']['length'])) def children(self): - root = self.__val.get_wrapped_value()['map']['root'] - node_ptr = root['node'] - i = 0 - for child in children_of_node(node_ptr, root['height'], False): - yield (str(i), child) - i = i + 1 + prev_idx = None + innermap = GdbValue(self.__val.get_wrapped_value()['map']) + if innermap.get_wrapped_value()['length'] > 0: + root = GdbValue(innermap.get_wrapped_value()['root']) + type_name = str(root.type.ty.name).replace('core::option::Option<', '')[:-1] + root = root.get_wrapped_value().cast(gdb.lookup_type(type_name)) + node_ptr = root['node'] + i = 0 + for child in children_of_node(node_ptr, root['height'], False): + yield (str(i), child) + i = i + 1 class RustStdBTreeMapPrinter(object): @@ -391,13 +396,16 @@ def to_string(self): ("(len: %i)" % self.__val.get_wrapped_value()['length'])) def children(self): - root = self.__val.get_wrapped_value()['root'] - node_ptr = root['node'] - i = 0 - for child in children_of_node(node_ptr, root['height'], True): - yield (str(i), child[0]) - yield (str(i), child[1]) - i = i + 1 + if self.__val.get_wrapped_value()['length'] > 0: + root = GdbValue(self.__val.get_wrapped_value()['root']) + type_name = str(root.type.ty.name).replace('core::option::Option<', '')[:-1] + root = root.get_wrapped_value().cast(gdb.lookup_type(type_name)) + node_ptr = root['node'] + i = 0 + for child in children_of_node(node_ptr, root['height'], True): + yield (str(i), child[0]) + yield (str(i), child[1]) + i = i + 1 class RustStdStringPrinter(object): diff --git a/src/test/debuginfo/pretty-std-collections.rs b/src/test/debuginfo/pretty-std-collections.rs index f8997fad9a53f..3d2d88a676d0d 100644 --- a/src/test/debuginfo/pretty-std-collections.rs +++ b/src/test/debuginfo/pretty-std-collections.rs @@ -17,35 +17,43 @@ // gdb-command: print btree_set // gdb-check:$1 = BTreeSet(len: 15) = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14} +// gdb-command: print empty_btree_set +// gdb-check:$2 = BTreeSet(len: 0) + // gdb-command: print btree_map -// gdb-check:$2 = BTreeMap(len: 15) = {[0] = 0, [1] = 1, [2] = 2, [3] = 3, [4] = 4, [5] = 5, [6] = 6, [7] = 7, [8] = 8, [9] = 9, [10] = 10, [11] = 11, [12] = 12, [13] = 13, [14] = 14} +// gdb-check:$3 = BTreeMap(len: 15) = {[0] = 0, [1] = 1, [2] = 2, [3] = 3, [4] = 4, [5] = 5, [6] = 6, [7] = 7, [8] = 8, [9] = 9, [10] = 10, [11] = 11, [12] = 12, [13] = 13, [14] = 14} + +// gdb-command: print empty_btree_map +// gdb-check:$4 = BTreeMap(len: 0) // gdb-command: print vec_deque -// gdb-check:$3 = VecDeque(len: 3, cap: 8) = {5, 3, 7} +// gdb-check:$5 = VecDeque(len: 3, cap: 8) = {5, 3, 7} // gdb-command: print vec_deque2 -// gdb-check:$4 = VecDeque(len: 7, cap: 8) = {2, 3, 4, 5, 6, 7, 8} +// gdb-check:$6 = VecDeque(len: 7, cap: 8) = {2, 3, 4, 5, 6, 7, 8} #![allow(unused_variables)] -use std::collections::BTreeSet; use std::collections::BTreeMap; +use std::collections::BTreeSet; use std::collections::VecDeque; - fn main() { - // BTreeSet let mut btree_set = BTreeSet::new(); for i in 0..15 { btree_set.insert(i); } + let mut empty_btree_set: BTreeSet = BTreeSet::new(); + // BTreeMap let mut btree_map = BTreeMap::new(); for i in 0..15 { btree_map.insert(i, i); } + let mut empty_btree_map: BTreeMap = BTreeMap::new(); + // VecDeque let mut vec_deque = VecDeque::new(); vec_deque.push_back(5); @@ -63,4 +71,6 @@ fn main() { zzz(); // #break } -fn zzz() { () } +fn zzz() { + () +}