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.

src/borrow_tracker/tree_borrows/tree.rs

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.

tests/pass/tree_borrows/wildcard/reborrow.rs

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

tests/fail/tree_borrows/wildcard/cross_tree_update_main_invalid_exposed.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.

royAmmerschuber · 2025-11-27T15:37:37Z

@rustbot ready

RalfJung

Thanks! Looks good overall but I have plenty of minor comments.

View changes since this review

src/borrow_tracker/tree_borrows/diagnostics.rs

RalfJung · 2025-11-28T15:18:41Z

src/borrow_tracker/tree_borrows/tree.rs

+    /// If we reborrow a reference which has wildcard provenance, then we do not know where in
+    /// the tree to attach them. Instead we create a new additional tree for this allocation
+    /// with this new reference as a root. We call this additional tree a wildcard subtree.
+    ///
+    /// Sorted according to `BorTag` from low to high. This also means the main root is `root[0]`.
+    ///
+    /// The actual structure should be a single tree but with wildcard provenance we approximate
+    /// this with this ordered set of trees. Each wildcard subtree could be the direct child of
+    /// any exposed tag (that is smaller than the root). This also means that it can only be the
+    /// child of a tree that comes before it in the vec ensuring we don't have any cycles in our
+    /// approximated tree.
+    ///


Suggested change

/// If we reborrow a reference which has wildcard provenance, then we do not know where in

/// the tree to attach them. Instead we create a new additional tree for this allocation

/// with this new reference as a root. We call this additional tree a wildcard subtree.

///

/// Sorted according to `BorTag` from low to high. This also means the main root is `root[0]`.

///

/// The actual structure should be a single tree but with wildcard provenance we approximate

/// this with this ordered set of trees. Each wildcard subtree could be the direct child of

/// any exposed tag (that is smaller than the root). This also means that it can only be the

/// child of a tree that comes before it in the vec ensuring we don't have any cycles in our

/// approximated tree.

///

/// If we reborrow a reference which has wildcard provenance, then we do not know where in

/// the tree to attach them. Instead we create a new additional tree for this allocation

/// with this new reference as a root. We call this additional tree a wildcard subtree.

///

/// The actual structure should be a single tree but with wildcard provenance we approximate

/// this with this ordered set of trees. Each wildcard subtree is the direct child of *some* exposed

/// tag (that is smaller than the root), but we do not know which. This also means that it can only be the

/// child of a tree that comes before it in the vec ensuring we don't have any cycles in our

/// approximated tree.

///

/// Sorted according to `BorTag` from low to high. This also means the main root is `root[0]`.

RalfJung · 2025-11-28T15:19:11Z

src/borrow_tracker/tree_borrows/tree.rs

        accessed_node: UniIndex,
        visit_children: ChildrenVisitMode,
-    ) -> Result<(), Err> {
+    ) -> Result<UniIndex, Err> {


Please explain the return value in a doc comment for this function.

RalfJung · 2025-11-28T15:21:56Z

src/borrow_tracker/tree_borrows/tree.rs

    ) -> InterpResult<'tcx> {
        let idx = self.tag_mapping.insert(new_tag);
-        let parent_idx = self.tag_mapping.get(&parent_tag).unwrap();
+


Suggested change

RalfJung · 2025-11-28T15:24:25Z

src/borrow_tracker/tree_borrows/tree.rs

+            for root in self.roots.iter().rev() {
+                if Some(*root) == accessed_root {
+                    continue;
+                }
+                check_tree(*root)?;


Suggested change

for root in self.roots.iter().rev() {

if Some(*root) == accessed_root {

continue;

}

check_tree(*root)?;

for &root in self.roots.iter().rev() {

if Some(root) == accessed_root {

continue;

}

check_tree(root)?;

RalfJung · 2025-11-28T15:50:45Z

tests/pass/tree_borrows/wildcard/reborrow.rs

+    // Performs a foreign read on ref3 and either read on reb1.
+    // This temporarily treats ref3 as only foreign.
+    let _x = *reb2;
+    // Performs an either write on ref3 and reb1.


What is an "either write"?

RalfJung · 2025-11-28T15:50:53Z

tests/pass/tree_borrows/wildcard/reborrow.rs

+    // This temporarily treats ref3 as only foreign.
+    let _x = *reb2;
+    // Performs an either write on ref3 and reb1.
+    // This should stop treating ref3 as only foreign.


tests/pass/tree_borrows/wildcard/undetected_ub.rs

RalfJung · 2025-11-28T15:53:09Z

tests/pass/tree_borrows/wildcard/undetected_ub.rs

+    // This access doesn't freeze reb1 even though no access could have come from its
+    // child ref3 (since ref3>reb2).
+    //
+    // ref3 doesn't get frozen, because It's still reserved.


Suggested change

// ref3 doesn't get frozen, because It's still reserved.

// ref3 doesn't get frozen because it's still reserved.

RalfJung · 2025-11-28T15:53:21Z

tests/pass/tree_borrows/wildcard/undetected_ub.rs

+    //                      └────────────┘
+
+    // This access doesn't freeze reb1 even though no access could have come from its
+    // child ref3 (since ref3>reb2).


Why does the max_local_access logic not detect this?

royAmmerschuber · 2025-11-29T18:36:31Z

@rustbot ready

royAmmerschuber · 2025-11-29T18:41:30Z

src/borrow_tracker/tree_borrows/tree.rs

        access_kind: AccessKind,
-        access_cause: AccessCause,
-        access_range: Option<AllocRange>,
+        access_cause: AccessCause,        //diagnostics
+        access_range: Option<AllocRange>, //diagnostics
        relatedness: AccessRelatedness,
-        span: Span,
-        location_range: Range<u64>,
+        span: Span,                 //diagnostics
+        location_range: Range<u64>, //diagnostics
        protected: bool,


It might make sense to move all the diagnostics arguments into a single struct. Since these just get passed through unchanged all the way from Tree::perform_access.

That seems like a good idea for a future PR. :)

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

royAmmerschuber added 2 commits November 25, 2025 15:10

further factor out get_relatedness

d40d674

update tests & make iteration order clearer

40ed9a1

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

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

update comments & dont clone roots

5881323

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

royAmmerschuber commented Nov 29, 2025

View reviewed changes

		// 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);

	// ref3 doesn't get frozen, because It's still reserved.
	// ref3 doesn't get frozen because it's still reserved.

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

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

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!

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!

royAmmerschuber commented Nov 27, 2025

Uh oh!

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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

royAmmerschuber commented Nov 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

RalfJung left a comment •

edited by rustbot

Loading

RalfJung left a comment •

edited by rustbot

Loading