Tree Borrows: improve protector end access child skipping #4766

royAmmerschuber · 2025-12-16T14:27:39Z

Currently, if we do a protector release access we assume that any other subtree (including the main subtree) could be a child of the accessed node, meaning we conservatively don't update them based on the access.

This PR determines as accurately as possible (under the current model), which other subtrees could be children of the accessed tag.
This means we now accurately reflect:

the main subtree is never a child of the accessed tag
if the accessed tag doesn't have exposed children, then no other subtree is a child of it
a subtree can only be a child of the accessed tag, if there exists an exposed child of the accessed tag with a tag less than the root of the subtree

This reverts commit d8089dd.

rustbot · 2025-12-16T14:27:42Z

Thank you for contributing to Miri! A reviewer will take a look at your PR, typically within a week or two.
Please remember to not force-push to the PR branch except when you need to rebase due to a conflict or when the reviewer asks you for it.

RalfJung

Mostly comments about comments, as usual. :)

View changes since this review

RalfJung · 2025-12-18T11:23:42Z

src/borrow_tracker/tree_borrows/tree.rs

                global,
                ChildrenVisitMode::VisitChildrenOfAccessed,
                &diagnostics,
+                None, // min_exposed_child only matters for protector end access,


Suggested change

None, // min_exposed_child only matters for protector end access,

/* min_exposed_child */ None, // only matters for protector end access,

RalfJung · 2025-12-18T11:24:20Z

src/borrow_tracker/tree_borrows/tree.rs

+        let min_exposed_child = if self.roots.len() > 1 {
+            LocationTree::get_min_exposed_child(source_idx, &self.nodes)
+        } else {
+            None


Is this correct?

Suggested change

None

// There's no point in computing this when there is just one tree.

None

RalfJung · 2025-12-18T11:25:19Z

src/borrow_tracker/tree_borrows/tree.rs

+        while let Some(idx) = stack.pop() {
+            let node = nodes.get(idx).unwrap();
+            if min_tag.is_some_and(|min| min < node.tag) {
+                continue;


Suggested change

continue;

// The minimum we found before is bigger than this tag, and therefore

// also bigger than all its children, so we can skip this subtree.

continue;

RalfJung · 2025-12-18T11:29:06Z

src/borrow_tracker/tree_borrows/tree.rs

+            if let Some(min_exposed_child) = min_exposed_child
+                && tag > min_exposed_child


Shouldn't this condition check visit_children?

RalfJung · 2025-12-18T11:29:28Z

src/borrow_tracker/tree_borrows/tree.rs

+            // On a protector release access we skip the children of the accessed tag so that
+            // we can correctly return references from functions. However, if the tag has


Suggested change

// On a protector release access we skip the children of the accessed tag so that

// we can correctly return references from functions. However, if the tag has

// On a protector release access we have to skip the children of the accessed tag.

// However, if the tag has

RalfJung · 2025-12-18T11:31:05Z

tests/fail/tree_borrows/wildcard/protector_release.rs

+
+/// This is a variant of the test in `../protector-write-lazy.rs`, but with
+/// wildcard references.
+/// Checks that a protector release transitions a foreign reference on a reference,


What does "transitions a foreign reference on a reference" mean?

RalfJung · 2025-12-18T11:31:19Z

tests/fail/tree_borrows/wildcard/protector_release.rs

+/// wildcard references.
+/// Checks that a protector release transitions a foreign reference on a reference,
+/// if we can determine that it's not a child of the released tag.
+/// For this version we know the tag is not a child, because its wildcard root has


Suggested change

/// For this version we know the tag is not a child, because its wildcard root has

/// For this version we know the tag is not a child because its wildcard root has

RalfJung · 2025-12-18T11:32:51Z

tests/fail/tree_borrows/wildcard/protector_release.rs

+        //          │                 │
+        //          ▼                 ▼
+        //    ┌───────────┐     ┌───────────┐
+        //    │ arg4      │     │       ref6│


Does the horizontal alignment of ref6 mean anything?

RalfJung · 2025-12-18T11:33:58Z

tests/fail/tree_borrows/wildcard/protector_release2.rs

This test seems very similar to the previous one, why did you add it as a separate test? Is there some plausible bug in the code that could make a difference between these tests?

RalfJung · 2025-12-18T12:56:05Z

src/borrow_tracker/tree_borrows/tree.rs

+    /// * `min_exposed_child`: any subtree that could be a child of `access_source` must
+    ///   have a root with a larger tag than this. If None then no other subtree can be
+    ///   a child of `access_source`.
+    ///   This only gets read if `visit_children==SkipChildrenOfAccessed`.


The comment doesn't match the name. What about something like this?

Suggested change

/// * `min_exposed_child`: any subtree that could be a child of `access_source` must

/// have a root with a larger tag than this. If None then no other subtree can be

/// a child of `access_source`.

/// This only gets read if `visit_children==SkipChildrenOfAccessed`.

/// * `min_exposed_child`: The tag of the smallest exposed (transitive) child of the accessed node.

/// This is useful with `visit_children == SkipChildrenOfAccessed`, where we need to skip children

/// of the accessed.

rustbot · 2025-12-18T12:56:31Z

Reminder, once the PR becomes ready for a review, use @rustbot ready.

royAmmerschuber added 4 commits December 10, 2025 09:43

Revert "remove skip child logic"

203cb1d

This reverts commit d8089dd.

improve get_min_exposed

817d46b

move min_exposed_child outside of location loop

41c6ce9

add more comments

69336b4

rustbot added the S-waiting-on-review Status: Waiting for a review to complete label Dec 16, 2025

RalfJung requested changes Dec 18, 2025

View reviewed changes

rustbot added S-waiting-on-author Status: Waiting for the PR author to address review comments and removed S-waiting-on-review Status: Waiting for a review to complete labels Dec 18, 2025

	None, // min_exposed_child only matters for protector end access,
	/* min_exposed_child */ None, // only matters for protector end access,

	None
	// There's no point in computing this when there is just one tree.
	None

		if let Some(min_exposed_child) = min_exposed_child
		&& tag > min_exposed_child

		// On a protector release access we skip the children of the accessed tag so that
		// we can correctly return references from functions. However, if the tag has

	/// For this version we know the tag is not a child, because its wildcard root has
	/// For this version we know the tag is not a child because its wildcard root has

Tree Borrows: improve protector end access child skipping #4766

Are you sure you want to change the base?

Tree Borrows: improve protector end access child skipping #4766

Conversation

royAmmerschuber commented Dec 16, 2025

Uh oh!

rustbot commented Dec 16, 2025

Uh oh!

RalfJung left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rustbot commented Dec 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

RalfJung left a comment •

edited

Loading