Support retagging of wildcard references in tree borrows #4707

royAmmerschuber · 2025-11-17T16:42:55Z

Adds proper suport for reborrowing wildcard pointers to tree borrows.

rustbot · 2025-11-17T16:42:58Z

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 · 2025-11-17T17:19:51Z

src/borrow_tracker/tree_borrows/tree.rs

+                        // then check if we can skip the entire subtree because the access might not
+                        // change any permissions here anyway.


I'm confused by the "might not" here. (Isn't that something you already had before and we removed it in the previous PR?)

Yes, your right. This got lost in a rebase.

RalfJung

Overall approach looks good. Here's a bunch of comments. :)

Please also add the 3 SB tests in this area: illegal_read/write_despite_exposed*.

View changes since this review

RalfJung · 2025-11-24T09:06:21Z

src/borrow_tracker/tree_borrows/tree.rs

-
-            let state = perm.or_insert(node.default_location_state());
+        #[cfg(feature = "expensive-consistency-checks")]
+        self.verify_wildcard_consistency(global);


We should only need this if a wildcard is involved in the access, right? I.e. if the tag is wildcard, or if there are multiple roots.

RalfJung · 2025-11-24T09:12:00Z

src/borrow_tracker/tree_borrows/tree.rs

-    pub(super) root: UniIndex,
+    /// Contains both the root of the main tree as well as the roots of the wildcard subtrees.
+    /// Sorted according to `BorTag` from low to high. This also means the main root is `root[0]`.
+    pub(super) roots: SmallVec<[UniIndex; 2]>,


This seems like a good place to describe what these multiple trees even mean.

RalfJung · 2025-11-24T09:13:19Z

src/borrow_tracker/tree_borrows/tree.rs

+            parent_node.children.push(idx);
+        } else {
+            // If the parent had wildcard provenance, then register the idx
+            // as a new wildcard root.


Please explain why this preserves sortedness.

RalfJung · 2025-11-24T09:15:36Z

src/borrow_tracker/tree_borrows/tree.rs

        }

        // We need to ensure the consistency of the wildcard access tracking data structure.
        // For this, we insert the correct entry for this tag based on its parent, if it exists.


The comment does not cover the else branch.

RalfJung · 2025-11-24T09:19:48Z

src/borrow_tracker/tree_borrows/tree.rs

+        // Foreign accesses are always possible on wildcard subtrees, as they have
+        // `max_foreign_access==Write` on their root. However on the main tree this can fail
+        // resulting in an access error.
+        let accessed_root_tag = accessed_root.map(|idx| nodes.get(idx).unwrap().tag);


This doesn't seem to directly relate to the comment. You can just move it up.

RalfJung · 2025-11-24T10:02:48Z

src/borrow_tracker/tree_borrows/diagnostics.rs

+
+    /// Print extra text if the tag is exposed.
+    fn print_exposed(&self, exposed: bool) -> S {
+        if exposed { " Exposed" } else { "" }


Suggested change

if exposed { " Exposed" } else { "" }

if exposed { " (exposed)" } else { "" }

RalfJung · 2025-11-24T10:06:29Z

tests/pass/tree_borrows/wildcard/reborrow.rs

+    // We can still access both ref1, ref2.
+    assert_eq!(*ref2, 13);


Comment and code don't quite match... we can access both, after all.
You can just explain that this is technically UB we don't catch.

RalfJung · 2025-11-24T10:07:16Z

tests/pass/tree_borrows/wildcard/reborrow.rs

+    // We should be able to access any of the references.
+    assert_eq!(*ref2, 13);


Again this doesn't quite test what the comment says.

RalfJung · 2025-11-24T10:08:33Z

tests/pass/tree_borrows/wildcard/reborrow.rs

+    //                      │            │
+    //                      └────────────┘
+
+    // This is the only valid ordering these accesses can happen in.


This should refer back to the comment for the test: accessing reb2 doesn't invalidate reb1 because ref1 exists. (Also, please pick more distinguishable names.)

RalfJung · 2025-11-24T10:15:46Z

tests/fail/tree_borrows/wildcard/reborrow8.rs

Please explain in the comment that this tests the max_local_tag logic.

rustbot · 2025-11-24T10:24:23Z

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

RalfJung · 2025-11-25T10:05:59Z

src/borrow_tracker/tree_borrows/tree.rs

+            };
+            // Afterwards we check all tags in arbitrary order, so that we also catch
+            // protectors on different subtrees.
+            // (This unnecessarily checks the tags of `start_idx`s subtree again)


This comment seems outdated now.

RalfJung · 2025-11-25T10:08:14Z

src/borrow_tracker/tree_borrows/tree.rs

+                TreeVisitor { nodes: &mut self.nodes, loc }.traverse_this_parents_children_other(
+                    *root,
+                    // Visit all children, skipping none.
+                    |_| ContinueTraversal::Recurse,
+                    check_strong_protector,
+                )?;


This part is still duplicated between the two branches... maybe it makes more sense to share the entire visitor invocation, not just the check_strong_protector closure?

rustbot · 2025-11-25T14:10:12Z

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

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

RalfJung reviewed Nov 17, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

royAmmerschuber force-pushed the feature/wildcard-root branch from 99dacc6 to 52f2087 Compare November 21, 2025 10:23

This comment has been minimized.

Sign in to view

RalfJung requested changes Nov 24, 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 Nov 24, 2025

royAmmerschuber mentioned this pull request Nov 25, 2025

preliminary refactor for wildcard reborrows #4723

Merged

RalfJung reviewed Nov 25, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

royAmmerschuber added 16 commits November 25, 2025 15:00

refactor perform_access to suppoert wildcard roots

2251b42

fix access pattern

6c5f166

fix only_foreign & normal wildcard access

04780bb

add tests

3455c18

fix strong-protector

ba50563

add wildcard subtree support to print_state

2caf2ee

fine grained tag ordering checks

1de4072

fix protector release & make more precise

2f1530b

add formatting test

c53b037

fix strict check & tests

3fe046b

add comments

10d452e

formatting & remove outdated comments

fff256b

unify root and wildcard_roots into single Vec

214e8ba

fix formatting

5347312

report correct accessed tag on protector violation

0beebdc

start improving test documentation

b412011

royAmmerschuber added 6 commits November 25, 2025 15:01

move stacked borrows tests to both_borrows

8edaf3d

make tree visitor return root index

720294d

factor out root f_continue & get_relatedness in wildcard_access

08bc656

other changes from review

9a5b2fd

remove skip child logic

d8089dd

expand roots comment & refactor check_strong_protector

4097b0a

royAmmerschuber force-pushed the feature/wildcard-root branch from 8c7ac81 to 4097b0a Compare November 25, 2025 14:10

further factor out get_relatedness

d40d674

		// then check if we can skip the entire subtree because the access might not
		// change any permissions here anyway.

	if exposed { " Exposed" } else { "" }
	if exposed { " (exposed)" } else { "" }

		// We can still access both ref1, ref2.
		assert_eq!(*ref2, 13);

		// We should be able to access any of the references.
		assert_eq!(*ref2, 13);

Support retagging of wildcard references in tree borrows #4707

Are you sure you want to change the base?

Support retagging of wildcard references in tree borrows #4707

Conversation

royAmmerschuber commented Nov 17, 2025

Uh oh!

rustbot commented Nov 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

RalfJung left a comment • edited by rustbot 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 Nov 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

rustbot commented Nov 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

RalfJung left a comment •

edited by rustbot

Loading