From bbbec3a15116a2532dc81eb01bbeb1c0b40e1cd6 Mon Sep 17 00:00:00 2001 From: Johannes Hostert Date: Tue, 19 Nov 2024 17:29:17 +0100 Subject: [PATCH] refactor foreign access skipping --- .../tree_borrows/foreign_access_skipping.rs | 84 +++++++ src/borrow_tracker/tree_borrows/mod.rs | 9 +- src/borrow_tracker/tree_borrows/perms.rs | 49 +++- src/borrow_tracker/tree_borrows/tree.rs | 223 ++++++------------ src/borrow_tracker/tree_borrows/tree/tests.rs | 16 +- 5 files changed, 210 insertions(+), 171 deletions(-) create mode 100644 src/borrow_tracker/tree_borrows/foreign_access_skipping.rs diff --git a/src/borrow_tracker/tree_borrows/foreign_access_skipping.rs b/src/borrow_tracker/tree_borrows/foreign_access_skipping.rs new file mode 100644 index 0000000000..8bb256d339 --- /dev/null +++ b/src/borrow_tracker/tree_borrows/foreign_access_skipping.rs @@ -0,0 +1,84 @@ +use super::AccessKind; +use super::tree::AccessRelatedness; + +/// To speed up tree traversals, we want to skip traversing subtrees when we know the traversal will have no effect. +/// This is often the case for foreign accesses, since usually foreign accesses happen several times in a row, but also +/// foreign accesses are idempotent. In particular, see tests `foreign_read_is_noop_after_foreign_write` and `all_transitions_idempotent`. +/// Thus, for each node we keep track of the "strongest idempotent foreign access," (SIFA) i.e. which foreign access can be skipped. +/// This enum represents the kinds of values we store: +/// `LocalAccess` means that the last access was local, and thus the next foreign access needs to happen, it can not be skipped. +/// `ForeignRead` means that we are idempotent under foreign reads, but not (yet) under foreign writes. +/// `ForeignWrite` means that we are idempotent under foreign writes. In other words, this node can be skipped for all foreign accesses. +/// Due to the subtree property of traversals, this also means that all our children can be skipped, which makes this optimization +/// worthwhile. In order for this to work, the invariant for multiple nodes in a tree is that at each location, the SIFA at each +/// child must be stronger than that at the parent. However, note that this is quite invariant, due to retags inserting nodes across +/// the entire range, which can violate this invariant without explicitly causing a local access which would reset things. +/// So this needs to be done manually, thus `ensure_no_stronger_than`, and also the test `permission_sifa_is_correct` in `perms.rs` +#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, Debug, Default)] +pub enum LastForeignAccess { + #[default] + LocalAccess, + ForeignRead, + ForeignWrite, +} + +impl LastForeignAccess { + /// Returns true if a node where the strongest idempotent foreign access is `self` + /// can skip the access `happening_next`. Note that if this returns + /// `true`, then the entire subtree will be skipped. + pub fn can_skip_foreign_access(self, happening_next: LastForeignAccess) -> bool { + debug_assert!(happening_next.is_foreign()); + // This ordering is correct. Intuitively, if the last access here was + // a foreign write, everything can be skipped, since after a foreign write, + // all further foreign accesses are idempotent + happening_next <= self + } + + /// Updates `self` to account for a foreign access. + pub fn record_new(&mut self, just_happened: LastForeignAccess) { + if just_happened.is_local() { + // If the access is local, reset it. + *self = LastForeignAccess::LocalAccess; + } else { + // Otherwise, keep it or stengthen it. + *self = just_happened.max(*self); + } + } + + /// Returns true if this access is local. + pub fn is_local(self) -> bool { + matches!(self, LastForeignAccess::LocalAccess) + } + + /// Returns true if this access is foreign, i.e. not local. + pub fn is_foreign(self) -> bool { + !self.is_local() + } + + /// Constructs a foreign access from an `AccessKind` + pub fn foreign(acc: AccessKind) -> LastForeignAccess { + match acc { + AccessKind::Read => Self::ForeignRead, + AccessKind::Write => Self::ForeignWrite, + } + } + + /// Usually, tree traversals have an `AccessKind` and an `AccessRelatedness`. + /// This methods converts these into the corresponding `LastForeignAccess`, to be used + /// to e.g. invoke `can_skip_foreign_access`. + pub fn of_acc_and_rel(acc: AccessKind, rel: AccessRelatedness) -> LastForeignAccess { + if rel.is_foreign() { Self::foreign(acc) } else { Self::LocalAccess } + } + + /// During retags, the SIFA needs to be weakened to account for children with weaker SIFAs being inserted. + /// Thus, this method is called from the bottom up on each parent, until it returns false, which means the + /// "children have stronger SIFAs" invariant is restored. + pub fn ensure_no_stronger_than(&mut self, strongest_allowed: LastForeignAccess) -> bool { + if *self > strongest_allowed { + *self = strongest_allowed; + true + } else { + false + } + } +} diff --git a/src/borrow_tracker/tree_borrows/mod.rs b/src/borrow_tracker/tree_borrows/mod.rs index cd3822a35f..37fbcc7e3a 100644 --- a/src/borrow_tracker/tree_borrows/mod.rs +++ b/src/borrow_tracker/tree_borrows/mod.rs @@ -9,6 +9,7 @@ use crate::concurrency::data_race::NaReadType; use crate::*; pub mod diagnostics; +mod foreign_access_skipping; mod perms; mod tree; mod unimap; @@ -296,12 +297,14 @@ trait EvalContextPrivExt<'tcx>: crate::MiriInterpCxExt<'tcx> { this.machine.current_span(), )?; // Record the parent-child pair in the tree. - tree_borrows.new_child(orig_tag, new_tag, new_perm.initial_state, range, span)?; - tree_borrows.update_last_accessed_after_retag( + tree_borrows.new_child( + orig_tag, new_tag, new_perm.initial_state, + range, + span, new_perm.protector.is_some(), - ); + )?; drop(tree_borrows); // Also inform the data race model (but only if any bytes are actually affected). diff --git a/src/borrow_tracker/tree_borrows/perms.rs b/src/borrow_tracker/tree_borrows/perms.rs index 1d0b5dc930..34fecb6752 100644 --- a/src/borrow_tracker/tree_borrows/perms.rs +++ b/src/borrow_tracker/tree_borrows/perms.rs @@ -48,6 +48,7 @@ enum PermissionPriv { Disabled, } use self::PermissionPriv::*; +use super::foreign_access_skipping::LastForeignAccess; impl PartialOrd for PermissionPriv { /// PermissionPriv is ordered by the reflexive transitive closure of @@ -88,25 +89,23 @@ impl PermissionPriv { !matches!(self, ReservedIM) } - /// Returns the strongest foreign action this node survives (without change), - /// where `prot` indicates if it is protected. - /// They are ordered as `None` < `Some(Read)` < `Some(Write)`, in order of power. - pub fn strongest_survivable_foreign_action(&self, prot: bool) -> Option { + /// See `foreign_access_skipping.rs`. Compotes the SIFA of a permission. + fn strongest_idempotent_foreign_access(&self, prot: bool) -> LastForeignAccess { match self { // The read will make it conflicted, so it is not invariant under it. - ReservedFrz { conflicted } if prot && !conflicted => None, + ReservedFrz { conflicted } if prot && !conflicted => LastForeignAccess::LocalAccess, // Otherwise, foreign reads do not affect it - ReservedFrz { .. } => Some(AccessKind::Read), + ReservedFrz { .. } => LastForeignAccess::ForeignRead, // Famously, ReservedIM survives foreign writes. It is never protected. - ReservedIM => Some(AccessKind::Write), + ReservedIM => LastForeignAccess::ForeignWrite, // Active changes on any foreign access (becomes Frozen/Disabled). - Active => None, + Active => LastForeignAccess::LocalAccess, // Frozen survives foreign reads, but not writes. - Frozen => Some(AccessKind::Read), + Frozen => LastForeignAccess::ForeignRead, // Disabled survives foreign reads and writes. It survives them // even if protected, because a protected `Disabled` is not initialized // and does therefore not trigger UB. - Disabled => Some(AccessKind::Write), + Disabled => LastForeignAccess::ForeignWrite, } } } @@ -330,8 +329,9 @@ impl Permission { /// Returns the strongest foreign action this node survives (without change), /// where `prot` indicates if it is protected. - pub fn strongest_survivable_foreign_action(&self, prot: bool) -> Option { - self.inner.strongest_survivable_foreign_action(prot) + /// See `foreign_access_skipping` + pub fn strongest_idempotent_foreign_access(&self, prot: bool) -> LastForeignAccess { + self.inner.strongest_idempotent_foreign_access(prot) } } @@ -662,6 +662,31 @@ mod propagation_optimization_checks { } } + #[test] + #[rustfmt::skip] + fn permission_sifa_is_correct() { + // Tests that `strongest_idempotent_foreign_access` is correct. See `foreign_access_skipping.rs`. + for perm in PermissionPriv::exhaustive() { + // Assert that adding a protector makes is less idempotent. + assert!(perm.strongest_idempotent_foreign_access(true) <= perm.strongest_idempotent_foreign_access(false)); + for prot in bool::exhaustive() { + if prot { + precondition!(perm.compatible_with_protector()); + } + let access = perm.strongest_idempotent_foreign_access(prot); + // We now assert it is idempotent, and never causes UB. + // First, if the SIFA includes foreign reads, assert it is idempotent under foreign reads. + if LastForeignAccess::ForeignRead <= access { + assert_eq!(perm, transition::perform_access(AccessKind::Read, AccessRelatedness::DistantAccess, perm, prot).unwrap()); + } + // Then, if the SIFA includes foreign writes, assert it is idempotent under foreign writes. + if LastForeignAccess::ForeignWrite <= access { + assert_eq!(perm, transition::perform_access(AccessKind::Write, AccessRelatedness::DistantAccess, perm, prot).unwrap()); + } + } + } + } + #[test] // Check that all transitions are consistent with the order on PermissionPriv, // i.e. Reserved -> Active -> Frozen -> Disabled diff --git a/src/borrow_tracker/tree_borrows/tree.rs b/src/borrow_tracker/tree_borrows/tree.rs index 116cdf0d40..d369ebdec9 100644 --- a/src/borrow_tracker/tree_borrows/tree.rs +++ b/src/borrow_tracker/tree_borrows/tree.rs @@ -21,6 +21,7 @@ use crate::borrow_tracker::tree_borrows::Permission; use crate::borrow_tracker::tree_borrows::diagnostics::{ self, NodeDebugInfo, TbError, TransitionError, }; +use crate::borrow_tracker::tree_borrows::foreign_access_skipping::LastForeignAccess; use crate::borrow_tracker::tree_borrows::perms::PermTransition; use crate::borrow_tracker::tree_borrows::unimap::{UniEntry, UniIndex, UniKeyMap, UniValMap}; use crate::borrow_tracker::{GlobalState, ProtectorKind}; @@ -45,33 +46,27 @@ pub(super) struct LocationState { initialized: bool, /// This pointer's current permission / future initial permission. permission: Permission, - /// Strongest foreign access whose effects have already been applied to - /// this node and all its children since the last child access. - /// This is `None` if the most recent access is a child access, - /// `Some(Write)` if at least one foreign write access has been applied - /// since the previous child access, and `Some(Read)` if at least one - /// foreign read and no foreign write have occurred since the last child access. - /// It is an invariant that all children of this node have an access that is - /// at least as strong, if not stronger, where the strength order is: - /// `None` < `Some(Read)` < `Some(Write)`, and `Some(Write)` is strongest. - /// Further, for correctness, it is important that this is `None` when a node - /// is default-created. - latest_foreign_access: Option, + /// See `foreign_access_skipping.rs` + /// Stores the strongest idempotent foreign access for this location. + /// Invariant is that at all children, the strongest idempotent foreign access is stronger (greater). + strongest_idempotent_foreign_access: LastForeignAccess, } impl LocationState { /// Constructs a new initial state. It has neither been accessed, nor been subjected /// to any foreign access yet. /// The permission is not allowed to be `Active`. - fn new_uninit(permission: Permission) -> Self { + /// `sifa` is the strongest idempotent foreign access, see `foreign_access_skipping.rs` + fn new_uninit(permission: Permission, sifa: LastForeignAccess) -> Self { assert!(permission.is_initial() || permission.is_disabled()); - Self { permission, initialized: false, latest_foreign_access: None } + Self { permission, initialized: false, strongest_idempotent_foreign_access: sifa } } /// Constructs a new initial state. It has not yet been subjected /// to any foreign access. However, it is already marked as having been accessed. - fn new_init(permission: Permission) -> Self { - Self { permission, initialized: true, latest_foreign_access: None } + /// `sifa` is the strongest idempotent foreign access, see `foreign_access_skipping.rs` + fn new_init(permission: Permission, sifa: LastForeignAccess) -> Self { + Self { permission, initialized: true, strongest_idempotent_foreign_access: sifa } } /// Check if the location has been initialized, i.e. if it has @@ -148,45 +143,19 @@ impl LocationState { } } - /// Tree traversal optimizations. - /// It turns out that all repeated accesses are idempotent, and further, - /// that foreign write accesses to a node are "stronger" than foreign read - /// accesses, insofar that a foreign read after a foreign write is a no-op. - /// The tests `foreign_read_is_noop_after_foreign_write` and - /// `all_transitions_idempotent` ensure this. - /// - /// Thus, if this access is a foreign access, and we already had such a foreign - /// access happen in the past, without a child access in-between, then we can - /// skip this access, since it is a known no-op. Further, since it is an - /// invariant of `latest_foreign_access` that all children survive stronger accesses, - /// we can not only skip this node, but the entire subtree rooted at this node. - /// - /// This method now checks whether this is the case, and the access to the entire - /// subtree (including this node) can be skipped. - /// For correctness, it is crucial that the information about the last access is - /// updated whenever an update occurs, using `record_new_access` or - /// `reset_last_foreign_access_after_reborrow`. If not, this function can cause - /// subtrees to be skipped that should be updated. + /// Tree traversal optimizations. See `foreign_access_skipping.rs`. + /// This checks if such a foreign access can be skipped. fn skip_if_known_noop( &self, access_kind: AccessKind, rel_pos: AccessRelatedness, ) -> ContinueTraversal { if rel_pos.is_foreign() { - let new_access_noop = match (self.latest_foreign_access, access_kind) { - // Previously applied transition makes the new one a guaranteed - // noop in the two following cases: - // (1) justified by `foreign_read_is_noop_after_foreign_write` - (Some(AccessKind::Write), AccessKind::Read) => true, - // (2) justified by `all_transitions_idempotent` - (Some(old), new) if old == new => true, - // In all other cases there has been a recent enough - // child access that the effects of the new foreign access - // need to be applied to this subtree. - _ => false, - }; + let happening_now = LastForeignAccess::foreign(access_kind); + let new_access_noop = + self.strongest_idempotent_foreign_access.can_skip_foreign_access(happening_now); if new_access_noop { - // Abort traversal if the new transition is indeed guaranteed + // Abort traversal if the new access is indeed guaranteed // to be noop. // No need to update `self.latest_foreign_access`, // the type of the current streak among nonempty read-only @@ -207,81 +176,24 @@ impl LocationState { } /// Records a new access, so that future access can potentially be skipped - /// by `skip_if_known_noop`. It needs to be called after some accesses, in order - /// to record this new access. - /// The rules for when it needs to be called are tightly coupled to `skip_if_known_noop` above: - /// If `skip_if_known_noop` says that a node should be visited, this function MUST be called afterwards. - /// If `skip_if_known_noop` says that a node (and its subtree) should be skipped, this function - /// MUST NOT be called for this node (nor any of its children). + /// by `skip_if_known_noop`. This must be called if `skip_if_known_noop` returned `Recurse`. + /// See `foreign_access_skipping.rs` fn record_new_access(&mut self, access_kind: AccessKind, rel_pos: AccessRelatedness) { debug_assert!(matches!( self.skip_if_known_noop(access_kind, rel_pos), ContinueTraversal::Recurse )); - if rel_pos.is_foreign() { - self.latest_foreign_access = Some(access_kind); - } else { - self.latest_foreign_access = None; - } - } - - /// Check if the latest recorded foreign update needs to be updated after a retag. - /// Retags can violate the invariant on `LocationState::latest_foreign_access`, - /// which states that a node's children survive accesses at least as strong as that node itself. - /// While retags act as a read for the offsets specified in the retag, it is also present at the other - /// offsets, but marked as "lazy." Such lazy nodes can, however, still be affected by foreign accesses. - /// This would violate the invariants that children always survive accesses at least as strong as the last - /// recorded foreign access at the parent. - /// To restore the invariant, we need to weaken the information stored in the parent, until it is weak enough - /// to encompass the newly added lazy node. - /// This function compares two accesses, and indicates if the parents needs to be updated. - /// It returns `true` iff `last_recorded` is stronger than `happening_now`, where - /// `None` is weakest and `Some(Write)` is strongest. - pub fn foreign_access_requires_update( - last_recorded: Option, - happening_now: Option, - ) -> bool { - match (last_recorded, happening_now) { - // if the last access here was a child access, we need to do nothing - (None, _) => false, - // if the last access here was a foreign access, but our new child does not - // survive any foreign access, we must let the next foreign access through again - (_, None) => true, - // last access was a read, the child survives reads -> no change needed - (Some(AccessKind::Read), Some(AccessKind::Read)) => false, - // last access was a read, the child survives writes -> no change needed - (Some(AccessKind::Read), Some(AccessKind::Write)) => false, - // last access was a write, the child survives writes -> no change needed - (Some(AccessKind::Write), Some(AccessKind::Write)) => false, - // last access was a write, the child does not survive writes -> reset it to read - (Some(AccessKind::Write), Some(AccessKind::Read)) => true, - } + self.strongest_idempotent_foreign_access + .record_new(LastForeignAccess::of_acc_and_rel(access_kind, rel_pos)); } /// Restores the "children are stronger" invariant of `latest_foreign_access` after a retag. - /// See also `foreign_access_requires_update`. - /// Retags can violate the invariant on `latest_foreign_access`, - /// which states that a node's children survive accesses at least as strong as that node itself. - /// While retags act as a read for the offsets specified in the retag, it is also present at the other - /// offsets, but marked as "lazy." Such lazy nodes can, however, still be affected by foreign accesses. - /// This would violate the invariants that children always survive accesses at least as strong as the last - /// recorded foreign access at the parent. - /// To restore the invariant, we need to weaken the information stored in the parent, until it is weak enough - /// to encompass the newly added lazy node. - /// This function updates the `latest_foreign_access` for this node, by weakening it if necessary. - /// It returns `true` if such a weakening was necessary. If this method returns `true`, then this node's parent - /// also needs to be updated. If it returns `false`, then the invariant is restored globally, since all of `self`'s - /// parents are already weaker than `self` itself. + /// See also `foreign_access_requires_update`. See `foreign_access_skipping.rs` fn reset_last_foreign_access_after_retag( &mut self, - strongest_survivable: Option, + strongest_allowed: LastForeignAccess, ) -> bool { - let needs_update = - Self::foreign_access_requires_update(self.latest_foreign_access, strongest_survivable); - if needs_update { - self.latest_foreign_access = strongest_survivable; - } - needs_update + self.strongest_idempotent_foreign_access.ensure_no_stronger_than(strongest_allowed) } } @@ -338,6 +250,10 @@ pub(super) struct Node { /// It is only ever `Disabled` for a tree root, since the root is initialized to `Active` by /// its own separate mechanism. default_initial_perm: Permission, + /// The default initial strongest idempotent foreign access. + /// This participates in the invariant for `LocationState::last_foreign_access` + /// in cases where there is no location store yet. See `foreign_access_skipping.rs` + default_initial_foreign_access: LastForeignAccess, /// Some extra information useful only for debugging purposes pub debug_info: NodeDebugInfo, } @@ -652,6 +568,8 @@ impl Tree { parent: None, children: SmallVec::default(), default_initial_perm: root_default_perm, + // The root may never be skipped, all accesses will be local. + default_initial_foreign_access: LastForeignAccess::LocalAccess, debug_info, }); nodes @@ -662,7 +580,10 @@ impl Tree { // We also ensure that it is initialized, so that no `Active` but // not yet initialized nodes exist. Essentially, we pretend there // was a write that initialized these to `Active`. - perms.insert(root_idx, LocationState::new_init(Permission::new_active())); + perms.insert( + root_idx, + LocationState::new_init(Permission::new_active(), LastForeignAccess::LocalAccess), + ); RangeMap::new(size, perms) }; Self { root: root_idx, nodes, rperms, tag_mapping } @@ -678,72 +599,69 @@ impl<'tcx> Tree { default_initial_perm: Permission, reborrow_range: AllocRange, span: Span, + prot: bool, ) -> InterpResult<'tcx> { assert!(!self.tag_mapping.contains_key(&new_tag)); let idx = self.tag_mapping.insert(new_tag); let parent_idx = self.tag_mapping.get(&parent_tag).unwrap(); + let strongest_idempotent = default_initial_perm.strongest_idempotent_foreign_access(prot); // Create the node self.nodes.insert(idx, Node { tag: new_tag, parent: Some(parent_idx), children: SmallVec::default(), default_initial_perm, + default_initial_foreign_access: strongest_idempotent, debug_info: NodeDebugInfo::new(new_tag, default_initial_perm, span), }); // Register new_tag as a child of parent_tag self.nodes.get_mut(parent_idx).unwrap().children.push(idx); // Initialize perms - let perm = LocationState::new_init(default_initial_perm); + let perm = LocationState::new_init(default_initial_perm, strongest_idempotent); for (_perms_range, perms) in self.rperms.iter_mut(reborrow_range.start, reborrow_range.size) { perms.insert(idx, perm); } + + // Inserting the new perms might have broken the SIFA invariant (see `foreign_access_skipping.rs`). + // We now weaken the recorded SIFA for our parents, until the invariant is restored. + // We could weaken them all to `LocalAccess`, but it is more efficient to compute the SIFA + // for the new permission statically, and use that. + self.update_last_accessed_after_retag(parent_idx, strongest_idempotent); + interp_ok(()) } - /// Restores the "children are stronger" invariant of `latest_foreign_access` after a retag. - /// See also `reset_last_foreign_access_after_reborrow`. - /// Retags can violate the invariant on `latest_foreign_access`, - /// which states that a node's children survive accesses at least as strong as that node itself. - /// While retags act as a read for the offsets specified in the retag, it is also present at the other - /// offsets, but marked as "lazy." Such lazy nodes can, however, still be affected by foreign accesses. - /// This would violate the invariants that children always survive accesses at least as strong as the last - /// recorded foreign access at the parent. - /// To restore the invariant, we need to weaken the information stored in all parents, until it is weak enough - /// to encompass the newly added lazy node. - /// This function walks upwards from the newly inserted node, and ensures that all its parents are aware that the subtree - /// rooted at that parent might now survive fewer foreign accesses than it did before this node was inserted. - pub fn update_last_accessed_after_retag( + /// Restores the SIFA "children are stronger" invariant after a retag. + /// See `foreign_access_skipping` and `new_child`. + fn update_last_accessed_after_retag( &mut self, - child_tag: BorTag, - new_perm: Permission, - prot: bool, + mut current: UniIndex, + strongest_allowed: LastForeignAccess, ) { - let mut current = self.tag_mapping.get(&child_tag).unwrap(); - let strongest_survivable = new_perm.strongest_survivable_foreign_action(prot); // We walk the tree upwards, until the invariant is restored - while let Some(next) = self.nodes.get(current).unwrap().parent { - current = next; + loop { + let current_node = self.nodes.get_mut(current).unwrap(); // record whether we did any change (if not, the invariant is restored). let any_change = self.rperms.iter_mut_all().any(|(_, map)| { match map.get_mut(current) { // if there is a permission, ensure that it's latest recorded foreign access is not stronger // than the `strongest_survivable` by the newly inserted child. // Returns `true` if it changed something, and in that case, we need to continue with the parent. - Some(perm) => perm.reset_last_foreign_access_after_retag(strongest_survivable), - // if there is no permission yet, it is still "default lazy". - // in that case, we would need to compare with that node's original default permission. - // But that depended on whether that node was protected when it was created, which is no - // longer the case now. - // Anyways, since the node is "default", its latest recorded foreign access will default to `None`, - // but we might need to continue weakening the parent. - // So we return `true` here, which is definitely correct, but maybe not fully optimal. - // But in cases where we are imprecise, the iteration stops at the next initialized parent anyway, - // so it is likely not a huge loss in practice. - None => true, + Some(perm) => perm.reset_last_foreign_access_after_retag(strongest_allowed), + // All which are not yet lazily initialized use the Node::default_initial_foreign_access, which is handled below + // So we return false, which is the identity in `any()` + None => false, } - }); + }) || (current_node + .default_initial_foreign_access + .ensure_no_stronger_than(strongest_allowed)); + if any_change { + let Some(next) = self.nodes.get(current).unwrap().parent else { + break; + }; + current = next; continue; } else { break; @@ -842,10 +760,7 @@ impl<'tcx> Tree { let node_skipper = |access_kind: AccessKind, args: &NodeAppArgs<'_>| -> ContinueTraversal { let NodeAppArgs { node, perm, rel_pos } = args; - let old_state = perm - .get() - .copied() - .unwrap_or_else(|| LocationState::new_uninit(node.default_initial_perm)); + let old_state = perm.get().copied().unwrap_or_else(|| node.default_location_state()); old_state.skip_if_known_noop(access_kind, *rel_pos) }; let node_app = |perms_range: Range, @@ -855,7 +770,7 @@ impl<'tcx> Tree { -> Result<(), TransitionError> { let NodeAppArgs { node, mut perm, rel_pos } = args; - let old_state = perm.or_insert(LocationState::new_uninit(node.default_initial_perm)); + let old_state = perm.or_insert(node.default_location_state()); // Call this function now, which ensures it is only called when // `skip_if_known_noop` returns `Recurse`, due to the contract of @@ -1087,6 +1002,12 @@ impl Tree { } } +impl Node { + pub fn default_location_state(&self) -> LocationState { + LocationState::new_uninit(self.default_initial_perm, self.default_initial_foreign_access) + } +} + impl VisitProvenance for Tree { fn visit_provenance(&self, visit: &mut VisitWith<'_>) { // To ensure that the root never gets removed, we visit it diff --git a/src/borrow_tracker/tree_borrows/tree/tests.rs b/src/borrow_tracker/tree_borrows/tree/tests.rs index 5d51a72852..16f857ddd2 100644 --- a/src/borrow_tracker/tree_borrows/tree/tests.rs +++ b/src/borrow_tracker/tree_borrows/tree/tests.rs @@ -10,7 +10,11 @@ impl Exhaustive for LocationState { fn exhaustive() -> Box> { // We keep `latest_foreign_access` at `None` as that's just a cache. Box::new(<(Permission, bool)>::exhaustive().map(|(permission, initialized)| { - Self { permission, initialized, latest_foreign_access: None } + Self { + permission, + initialized, + strongest_idempotent_foreign_access: Default::default(), + } })) } } @@ -598,13 +602,15 @@ mod spurious_read { let source = LocStateProtPair { xy_rel: RelPosXY::MutuallyForeign, x: LocStateProt { - state: LocationState::new_init(Permission::new_frozen()), + // For the tests, the strongest idempotent foreign access does not matter, so we use `Default::default` + state: LocationState::new_init(Permission::new_frozen(), Default::default()), prot: true, }, y: LocStateProt { - state: LocationState::new_uninit(Permission::new_reserved( - /* freeze */ true, /* protected */ true, - )), + state: LocationState::new_uninit( + Permission::new_reserved(/* freeze */ true, /* protected */ true), + Default::default(), + ), prot: true, }, };