Skip to content

Commit

Permalink
Auto merge of rust-lang#3833 - JoJoDeveloping:tb-fix-stack-overflow, …
Browse files Browse the repository at this point in the history
…r=RalfJung

Make Tree Borrows Provenance GC no longer produce stack overflows

Most functions operating on Tree Borrows' trees are carefully written to not cause stack overflows due to too much recursion. The one exception is [`Tree::keep_only_needed`](https://github.com/rust-lang/miri/blob/94f5588fafcc7d59fce60ca8f7af0208e6f618d4/src/borrow_tracker/tree_borrows/tree.rs#L724), which just uses regular recursion.
This function is part of the provenance GC, so it is called regularly for every allocation in the program.

Tests show that this is a problem in practice. For example, the test `fill::horizontal_line` in crate `tiny-skia` (version 0.11.4) is such a test.

This PR changes this, this test no now longer crashes. Instead, it succeeds (after a _long_ time).
  • Loading branch information
bors committed Aug 22, 2024
2 parents 695a4f9 + b5d77d8 commit 8b10bda
Show file tree
Hide file tree
Showing 3 changed files with 74 additions and 47 deletions.
2 changes: 1 addition & 1 deletion src/tools/miri/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ doctest = false # and no doc tests
[dependencies]
getrandom = { version = "0.2", features = ["std"] }
rand = "0.8"
smallvec = "1.7"
smallvec = { version = "1.7", features = ["drain_filter"] }
aes = { version = "0.8.3", features = ["hazmat"] }
measureme = "11"
ctrlc = "3.2.5"
Expand Down
104 changes: 64 additions & 40 deletions src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
//! and the relative position of the access;
//! - idempotency properties asserted in `perms.rs` (for optimizations)
use std::fmt;
use std::{fmt, mem};

use smallvec::SmallVec;

Expand Down Expand Up @@ -699,18 +699,24 @@ impl<'tcx> Tree {
/// Integration with the BorTag garbage collector
impl Tree {
pub fn remove_unreachable_tags(&mut self, live_tags: &FxHashSet<BorTag>) {
let root_is_needed = self.keep_only_needed(self.root, live_tags); // root can't be removed
assert!(root_is_needed);
self.remove_useless_children(self.root, live_tags);
// Right after the GC runs is a good moment to check if we can
// merge some adjacent ranges that were made equal by the removal of some
// tags (this does not necessarily mean that they have identical internal representations,
// see the `PartialEq` impl for `UniValMap`)
self.rperms.merge_adjacent_thorough();
}

/// Checks if a node is useless and should be GC'ed.
/// A node is useless if it has no children and also the tag is no longer live.
fn is_useless(&self, idx: UniIndex, live: &FxHashSet<BorTag>) -> bool {
let node = self.nodes.get(idx).unwrap();
node.children.is_empty() && !live.contains(&node.tag)
}

/// Traverses the entire tree looking for useless tags.
/// Returns true iff the tag it was called on is still live or has live children,
/// and removes from the tree all tags that have no live children.
/// Removes from the tree all useless child nodes of root.
/// It will not delete the root itself.
///
/// NOTE: This leaves in the middle of the tree tags that are unreachable but have
/// reachable children. There is a potential for compacting the tree by reassigning
Expand All @@ -721,42 +727,60 @@ impl Tree {
/// `child: Reserved`. This tree can exist. If we blindly delete `parent` and reassign
/// `child` to be a direct child of `root` then Writes to `child` are now permitted
/// whereas they were not when `parent` was still there.
fn keep_only_needed(&mut self, idx: UniIndex, live: &FxHashSet<BorTag>) -> bool {
let node = self.nodes.get(idx).unwrap();
// FIXME: this function does a lot of cloning, a 2-pass approach is possibly
// more efficient. It could consist of
// 1. traverse the Tree, collect all useless tags in a Vec
// 2. traverse the Vec, remove all tags previously selected
// Bench it.
let children: SmallVec<_> = node
.children
.clone()
.into_iter()
.filter(|child| self.keep_only_needed(*child, live))
.collect();
let no_children = children.is_empty();
let node = self.nodes.get_mut(idx).unwrap();
node.children = children;
if !live.contains(&node.tag) && no_children {
// All of the children and this node are unreachable, delete this tag
// from the tree (the children have already been deleted by recursive
// calls).
// Due to the API of UniMap we must absolutely call
// `UniValMap::remove` for the key of this tag on *all* maps that used it
// (which are `self.nodes` and every range of `self.rperms`)
// before we can safely apply `UniValMap::forget` to truly remove
// the tag from the mapping.
let tag = node.tag;
self.nodes.remove(idx);
for (_perms_range, perms) in self.rperms.iter_mut_all() {
perms.remove(idx);
fn remove_useless_children(&mut self, root: UniIndex, live: &FxHashSet<BorTag>) {
// To avoid stack overflows, we roll our own stack.
// Each element in the stack consists of the current tag, and the number of the
// next child to be processed.

// The other functions are written using the `TreeVisitorStack`, but that does not work here
// since we need to 1) do a post-traversal and 2) remove nodes from the tree.
// Since we do a post-traversal (by deleting nodes only after handling all children),
// we also need to be a bit smarter than "pop node, push all children."
let mut stack = vec![(root, 0)];
while let Some((tag, nth_child)) = stack.last_mut() {
let node = self.nodes.get(*tag).unwrap();
if *nth_child < node.children.len() {
// Visit the child by pushing it to the stack.
// Also increase `nth_child` so that when we come back to the `tag` node, we
// look at the next child.
let next_child = node.children[*nth_child];
*nth_child += 1;
stack.push((next_child, 0));
continue;
} else {
// We have processed all children of `node`, so now it is time to process `node` itself.
// First, get the current children of `node`. To appease the borrow checker,
// we have to temporarily move the list out of the node, and then put the
// list of remaining children back in.
let mut children_of_node =
mem::take(&mut self.nodes.get_mut(*tag).unwrap().children);
// Remove all useless children, and save them for later.
// The closure needs `&self` and the loop below needs `&mut self`, so we need to `collect`
// in to a temporary list.
let to_remove: Vec<_> =
children_of_node.drain_filter(|x| self.is_useless(*x, live)).collect();
// Put back the now-filtered vector.
self.nodes.get_mut(*tag).unwrap().children = children_of_node;
// Now, all that is left is unregistering the children saved in `to_remove`.
for idx in to_remove {
// Note: In the rest of this comment, "this node" refers to `idx`.
// This node has no more children (if there were any, they have already been removed).
// It is also unreachable as determined by the GC, so we can remove it everywhere.
// Due to the API of UniMap we must make sure to call
// `UniValMap::remove` for the key of this node on *all* maps that used it
// (which are `self.nodes` and every range of `self.rperms`)
// before we can safely apply `UniKeyMap::remove` to truly remove
// this tag from the `tag_mapping`.
let node = self.nodes.remove(idx).unwrap();
for (_perms_range, perms) in self.rperms.iter_mut_all() {
perms.remove(idx);
}
self.tag_mapping.remove(&node.tag);
}
// We are done, the parent can continue.
stack.pop();
continue;
}
self.tag_mapping.remove(&tag);
// The tag has been deleted, inform the caller
false
} else {
// The tag is still live or has live children, it must be kept
true
}
}
}
Expand Down
15 changes: 9 additions & 6 deletions src/tools/miri/src/borrow_tracker/tree_borrows/unimap.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
#![allow(dead_code)]

use std::hash::Hash;
use std::{hash::Hash, mem};

use rustc_data_structures::fx::FxHashMap;

Expand Down Expand Up @@ -187,13 +187,16 @@ impl<V> UniValMap<V> {
self.data.get_mut(idx.idx as usize).and_then(Option::as_mut)
}

/// Delete any value associated with this index. Ok even if the index
/// has no associated value.
pub fn remove(&mut self, idx: UniIndex) {
/// Delete any value associated with this index.
/// Returns None if the value was not present, otherwise
/// returns the previously stored value.
pub fn remove(&mut self, idx: UniIndex) -> Option<V> {
if idx.idx as usize >= self.data.len() {
return;
return None;
}
self.data[idx.idx as usize] = None;
let mut res = None;
mem::swap(&mut res, &mut self.data[idx.idx as usize]);
res
}
}

Expand Down

0 comments on commit 8b10bda

Please sign in to comment.