From 67fac9bb5a6ebf5c0b5c9921398498510d40ddbf Mon Sep 17 00:00:00 2001 From: Johannes Hostert Date: Sat, 26 Oct 2024 16:35:56 +0200 Subject: [PATCH 1/5] Add benchmark showing effectivity of subtree skipping --- bench-cargo-miri/string-replace/Cargo.lock | 7 +++++++ bench-cargo-miri/string-replace/Cargo.toml | 8 ++++++++ bench-cargo-miri/string-replace/data.json | 1 + bench-cargo-miri/string-replace/src/main.rs | 7 +++++++ 4 files changed, 23 insertions(+) create mode 100644 bench-cargo-miri/string-replace/Cargo.lock create mode 100644 bench-cargo-miri/string-replace/Cargo.toml create mode 100644 bench-cargo-miri/string-replace/data.json create mode 100644 bench-cargo-miri/string-replace/src/main.rs diff --git a/bench-cargo-miri/string-replace/Cargo.lock b/bench-cargo-miri/string-replace/Cargo.lock new file mode 100644 index 0000000000..443115c126 --- /dev/null +++ b/bench-cargo-miri/string-replace/Cargo.lock @@ -0,0 +1,7 @@ +# This file is automatically @generated by Cargo. +# It is not intended for manual editing. +version = 3 + +[[package]] +name = "string-replace" +version = "0.1.0" diff --git a/bench-cargo-miri/string-replace/Cargo.toml b/bench-cargo-miri/string-replace/Cargo.toml new file mode 100644 index 0000000000..f0785cd693 --- /dev/null +++ b/bench-cargo-miri/string-replace/Cargo.toml @@ -0,0 +1,8 @@ +[package] +name = "string-replace" +version = "0.1.0" +edition = "2021" + +# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html + +[dependencies] diff --git a/bench-cargo-miri/string-replace/data.json b/bench-cargo-miri/string-replace/data.json new file mode 100644 index 0000000000..7e074cd695 --- /dev/null +++ b/bench-cargo-miri/string-replace/data.json @@ -0,0 +1 @@ +[{"_id":"6724e6fc58417687afba2b85","index":0,"guid":"5c1bd108-2ee2-40bd-bce8-895c206409df","isActive":true,"balance":"$2,927.88","picture":"http://placehold.it/32x32","age":40,"eyeColor":"green","name":"Wynn Bradshaw","gender":"male","company":"ROTODYNE","email":"wynnbradshaw@rotodyne.com","phone":"+1 (904) 559-3130","address":"287 Bergen Avenue, Sperryville, Alaska, 5392","about":"Adipisicing fugiat aute adipisicing qui esse cillum. Lorem consequat consectetur voluptate id pariatur nostrud incididunt aliquip incididunt laboris aliqua. Magna nulla adipisicing cupidatat ea velit aliquip magna duis duis sunt ipsum. Cillum labore mollit fugiat tempor dolor sit.\r\n","registered":"2017-01-26T01:28:10 -01:00","latitude":46.089504,"longitude":51.763723,"greeting":"Hello, Wynn Bradshaw! You have 6 unread messages.","favoriteFruit":"banana"},{"_id":"6724e6fce8619d86c0389ccf","index":1,"guid":"ced7fbb7-3b1a-419b-9fc7-d47582bbb3ea","isActive":true,"balance":"$1,856.38","picture":"http://placehold.it/32x32","age":21,"eyeColor":"brown","name":"Olsen Larsen","gender":"male","company":"VERBUS","email":"olsenlarsen@verbus.com","phone":"+1 (936) 480-3749","address":"370 Losee Terrace, Churchill, Maine, 4040","about":"Consequat Lorem in laboris fugiat veniam tempor eiusmod eu incididunt enim do et qui. Sit commodo eu excepteur cillum ex tempor commodo ex ex laboris esse. Aute aute nulla dolore dolor do. Irure esse proident nostrud non. Incididunt velit reprehenderit incididunt laboris do. Consequat nulla est id ex veniam tempor. Sit Lorem magna cillum aliquip irure quis sit minim anim.\r\n","registered":"2016-07-12T02:08:39 -02:00","latitude":-12.843628,"longitude":-124.143829,"greeting":"Hello, Olsen Larsen! You have 1 unread messages.","favoriteFruit":"apple"},{"_id":"6724e6fc01b471965ea560cf","index":2,"guid":"21fde9a3-13ba-46be-baed-fb503f668b9e","isActive":false,"balance":"$2,025.88","picture":"http://placehold.it/32x32","age":29,"eyeColor":"green","name":"Ramirez Kinney","gender":"male","company":"QUAREX","email":"ramirezkinney@quarex.com","phone":"+1 (852) 447-2930","address":"986 Cornelia Street, Oberlin, Texas, 362","about":"Minim ea proident quis eiusmod aliquip duis excepteur velit minim aute cupidatat. Esse qui ex aliquip laborum id reprehenderit. Anim dolore commodo deserunt laborum nulla duis. Sint quis anim mollit fugiat sit incididunt reprehenderit occaecat aliqua dolor. Ullamco ipsum eiusmod incididunt proident qui exercitation adipisicing voluptate elit aliquip. Tempor duis aute incididunt adipisicing.\r\n","registered":"2016-02-23T05:34:14 -01:00","latitude":-56.21645,"longitude":44.048129,"greeting":"Hello, Ramirez Kinney! You have 9 unread messages.","favoriteFruit":"banana"},{"_id":"6724e6fc3ea8e4182b9e170f","index":3,"guid":"46b20637-eecc-40db-87d7-03da9bcd1cea","isActive":true,"balance":"$3,399.31","picture":"http://placehold.it/32x32","age":39,"eyeColor":"brown","name":"Hansen Kaufman","gender":"male","company":"EVENTAGE","email":"hansenkaufman@eventage.com","phone":"+1 (827) 483-2303","address":"916 Brighton Court, Sunbury, New Mexico, 3804","about":"Nisi in voluptate aute ullamco ipsum proident fugiat veniam anim reprehenderit. In ad irure dolor labore culpa incididunt veniam mollit Lorem deserunt cupidatat incididunt. Aliquip aliquip proident ut culpa.\r\n","registered":"2023-10-18T07:03:48 -02:00","latitude":-40.239135,"longitude":49.802049,"greeting":"Hello, Hansen Kaufman! You have 10 unread messages.","favoriteFruit":"apple"},{"_id":"6724e6fc721f83a10cf2aa37","index":4,"guid":"3d23743b-1e82-474e-8f7a-855fa46170d1","isActive":false,"balance":"$1,967.87","picture":"http://placehold.it/32x32","age":35,"eyeColor":"green","name":"Imelda Stephens","gender":"female","company":"OHMNET","email":"imeldastephens@ohmnet.com","phone":"+1 (893) 523-2400","address":"391 Wilson Street, Glidden, Kansas, 7226","about":"Officia sunt magna adipisicing id exercitation deserunt deserunt aliquip excepteur Lorem enim fugiat. Nulla culpa ut cupidatat excepteur do deserunt labore id eu laboris ullamco adipisicing ad. Et non nisi adipisicing minim aliquip ea ut qui adipisicing do laboris ex dolore duis.\r\n","registered":"2020-10-20T07:03:38 -02:00","latitude":0.348698,"longitude":-157.961956,"greeting":"Hello, Imelda Stephens! You have 2 unread messages.","favoriteFruit":"strawberry"},{"_id":"6724e6fc7ad7274b9f4c406c","index":5,"guid":"626292b1-ae84-4887-9e29-78e548cd24e6","isActive":true,"balance":"$1,577.44","picture":"http://placehold.it/32x32","age":40,"eyeColor":"brown","name":"Lynne Jarvis","gender":"female","company":"CORECOM","email":"lynnejarvis@corecom.com","phone":"+1 (899) 556-3876","address":"465 National Drive, Davenport, Palau, 9786","about":"Aliquip elit dolore sint quis do laboris exercitation elit aliqua eiusmod. Excepteur ad aliqua eiusmod incididunt tempor laboris officia consectetur sit. Cupidatat voluptate deserunt ut consectetur qui laborum duis elit incididunt occaecat laborum. Mollit aute velit officia amet aute minim fugiat sit laborum Lorem deserunt in. Exercitation eu sunt nulla adipisicing quis ea aute est. Lorem ea cillum ad labore quis minim et est laboris deserunt proident. Amet ut tempor laborum occaecat exercitation ullamco laborum adipisicing fugiat ea voluptate quis fugiat.\r\n","registered":"2018-11-03T03:53:15 -01:00","latitude":89.827087,"longitude":-136.882799,"greeting":"Hello, Lynne Jarvis! You have 3 unread messages.","favoriteFruit":"strawberry"},{"_id":"6724e6fcef1a1db2cf170762","index":6,"guid":"b8777c06-b90f-49a4-8737-96712fc504a3","isActive":false,"balance":"$2,285.03","picture":"http://placehold.it/32x32","age":37,"eyeColor":"green","name":"Price Bolton","gender":"male","company":"IMANT","email":"pricebolton@imant.com","phone":"+1 (825) 424-2873","address":"237 Aberdeen Street, Sattley, Montana, 2918","about":"Non cillum irure fugiat consequat ad ex. Magna magna tempor excepteur irure quis. Duis in laboris ipsum adipisicing culpa magna reprehenderit nisi incididunt est veniam quis. Labore culpa ut culpa veniam est est consectetur ipsum ex esse.\r\n","registered":"2014-04-26T01:20:19 -02:00","latitude":70.349258,"longitude":126.810102,"greeting":"Hello, Price Bolton! You have 10 unread messages.","favoriteFruit":"banana"},{"_id":"6724e6fc8bcb952208c159f9","index":7,"guid":"a4e6c6c8-3fe3-42de-ae28-79c16956d309","isActive":false,"balance":"$1,298.07","picture":"http://placehold.it/32x32","age":28,"eyeColor":"blue","name":"Gretchen Wynn","gender":"female","company":"TERASCAPE","email":"gretchenwynn@terascape.com","phone":"+1 (882) 447-2895","address":"973 Suydam Place, Shindler, Nebraska, 8094","about":"Anim mollit labore magna proident ipsum culpa enim deserunt dolore sunt veniam fugiat. Ad fugiat cupidatat nisi commodo dolore duis commodo nostrud est. Enim proident ullamco non adipisicing magna consequat mollit ad reprehenderit laboris. Ex quis duis anim id non commodo amet sunt est magna officia.\r\n","registered":"2021-08-13T08:51:32 -02:00","latitude":14.551848,"longitude":-27.142242,"greeting":"Hello, Gretchen Wynn! You have 7 unread messages.","favoriteFruit":"apple"},{"_id":"6724e6fcc8243c2dfa47f5d4","index":8,"guid":"27df20d5-c1d8-419b-ad38-bdd1e6094775","isActive":true,"balance":"$3,005.40","picture":"http://placehold.it/32x32","age":33,"eyeColor":"blue","name":"Chen Travis","gender":"male","company":"MEMORA","email":"chentravis@memora.com","phone":"+1 (980) 500-2406","address":"182 Dahlgreen Place, Baker, South Carolina, 9817","about":"Ad nisi consequat aliquip eiusmod aute pariatur est sint magna. Ad magna anim esse qui Lorem nulla veniam dolore eiusmod. Cillum consequat sit aliqua est proident exercitation eiusmod irure. Minim eu laboris ad incididunt enim sunt. Sunt in excepteur aute non tempor irure mollit laboris. Eu et duis ullamco dolor sint occaecat officia culpa ipsum anim anim eu veniam aliquip. Exercitation ipsum dolor sint cillum duis incididunt minim quis irure enim reprehenderit do do incididunt.\r\n","registered":"2019-09-01T02:57:37 -02:00","latitude":25.442301,"longitude":48.381036,"greeting":"Hello, Chen Travis! You have 1 unread messages.","favoriteFruit":"banana"}] \ No newline at end of file diff --git a/bench-cargo-miri/string-replace/src/main.rs b/bench-cargo-miri/string-replace/src/main.rs new file mode 100644 index 0000000000..73bf4a850e --- /dev/null +++ b/bench-cargo-miri/string-replace/src/main.rs @@ -0,0 +1,7 @@ +const TCB_INFO_JSON: &str = include_str!("../data.json"); + +fn main() { + let tcb_json = TCB_INFO_JSON; + let bad_tcb_json = tcb_json.replace("female", "male"); + std::hint::black_box(bad_tcb_json); +} From 04630e0a52351428e258804c72943cc5a5f48295 Mon Sep 17 00:00:00 2001 From: Johannes Hostert Date: Mon, 26 Aug 2024 22:40:17 +0200 Subject: [PATCH 2/5] Properly fix #3846 by resetting parents on lazy node creation This commit supplies a real fix, which makes retags more complicated, at the benefit of making accesses more performant. --- src/borrow_tracker/tree_borrows/mod.rs | 5 + src/borrow_tracker/tree_borrows/perms.rs | 28 ++++ src/borrow_tracker/tree_borrows/tree.rs | 185 ++++++++++++++++++----- 3 files changed, 181 insertions(+), 37 deletions(-) diff --git a/src/borrow_tracker/tree_borrows/mod.rs b/src/borrow_tracker/tree_borrows/mod.rs index 40467aa4bc..cd3822a35f 100644 --- a/src/borrow_tracker/tree_borrows/mod.rs +++ b/src/borrow_tracker/tree_borrows/mod.rs @@ -297,6 +297,11 @@ trait EvalContextPrivExt<'tcx>: crate::MiriInterpCxExt<'tcx> { )?; // 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( + new_tag, + new_perm.initial_state, + 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 28f9dec7bb..1d0b5dc930 100644 --- a/src/borrow_tracker/tree_borrows/perms.rs +++ b/src/borrow_tracker/tree_borrows/perms.rs @@ -87,6 +87,28 @@ impl PermissionPriv { fn compatible_with_protector(&self) -> bool { !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 { + match self { + // The read will make it conflicted, so it is not invariant under it. + ReservedFrz { conflicted } if prot && !conflicted => None, + // Otherwise, foreign reads do not affect it + ReservedFrz { .. } => Some(AccessKind::Read), + // Famously, ReservedIM survives foreign writes. It is never protected. + ReservedIM => Some(AccessKind::Write), + // Active changes on any foreign access (becomes Frozen/Disabled). + Active => None, + // Frozen survives foreign reads, but not writes. + Frozen => Some(AccessKind::Read), + // 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), + } + } } /// This module controls how each permission individually reacts to an access. @@ -305,6 +327,12 @@ impl Permission { (Disabled, _) => false, } } + + /// 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) + } } impl PermTransition { diff --git a/src/borrow_tracker/tree_borrows/tree.rs b/src/borrow_tracker/tree_borrows/tree.rs index a551b017df..116cdf0d40 100644 --- a/src/borrow_tracker/tree_borrows/tree.rs +++ b/src/borrow_tracker/tree_borrows/tree.rs @@ -51,6 +51,11 @@ pub(super) struct LocationState { /// `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, } @@ -143,30 +148,25 @@ impl LocationState { } } - // Helper to optimize the tree traversal. - // The optimization here consists of observing thanks to the tests - // `foreign_read_is_noop_after_foreign_write` and `all_transitions_idempotent`, - // that there are actually just three possible sequences of events that can occur - // in between two child accesses that produce different results. - // - // Indeed, - // - applying any number of foreign read accesses is the same as applying - // exactly one foreign read, - // - applying any number of foreign read or write accesses is the same - // as applying exactly one foreign write. - // therefore the three sequences of events that can produce different - // outcomes are - // - an empty sequence (`self.latest_foreign_access = None`) - // - a nonempty read-only sequence (`self.latest_foreign_access = Some(Read)`) - // - a nonempty sequence with at least one write (`self.latest_foreign_access = Some(Write)`) - // - // This function not only determines if skipping the propagation right now - // is possible, it also updates the internal state to keep track of whether - // the propagation can be skipped next time. - // It is a performance loss not to call this function when a foreign access occurs. - // FIXME: This optimization is wrong, and is currently disabled (by ignoring the - // result returned here). Since we presumably want an optimization like this, - // we should add it back. See #3864 for more information. + /// 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. fn skip_if_known_noop( &self, access_kind: AccessKind, @@ -207,21 +207,82 @@ impl LocationState { } /// Records a new access, so that future access can potentially be skipped - /// by `skip_if_known_noop`. - /// The invariants for this function are closely coupled to the function above: - /// It MUST be called on child accesses, and on foreign accesses MUST be called - /// when `skip_if_know_noop` returns `Recurse`, and MUST NOT be called otherwise. - /// FIXME: This optimization is wrong, and is currently disabled (by ignoring the - /// result returned here). Since we presumably want an optimization like this, - /// we should add it back. See #3864 for more information. - #[allow(unused)] + /// 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). 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, + } + } + + /// 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. + fn reset_last_foreign_access_after_retag( + &mut self, + strongest_survivable: Option, + ) -> 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 + } } impl fmt::Display for LocationState { @@ -640,6 +701,56 @@ impl<'tcx> Tree { 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( + &mut self, + child_tag: BorTag, + new_perm: Permission, + prot: bool, + ) { + 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; + // 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, + } + }); + if any_change { + continue; + } else { + break; + } + } + } + /// Deallocation requires /// - a pointer that permits write accesses /// - the absence of Strong Protectors anywhere in the allocation @@ -735,9 +846,7 @@ impl<'tcx> Tree { .get() .copied() .unwrap_or_else(|| LocationState::new_uninit(node.default_initial_perm)); - // FIXME: See #3684 - let _would_skip_if_not_for_fixme = old_state.skip_if_known_noop(access_kind, *rel_pos); - ContinueTraversal::Recurse + old_state.skip_if_known_noop(access_kind, *rel_pos) }; let node_app = |perms_range: Range, access_kind: AccessKind, @@ -748,8 +857,10 @@ impl<'tcx> Tree { let old_state = perm.or_insert(LocationState::new_uninit(node.default_initial_perm)); - // FIXME: See #3684 - // old_state.record_new_access(access_kind, rel_pos); + // Call this function now, which ensures it is only called when + // `skip_if_known_noop` returns `Recurse`, due to the contract of + // `traverse_this_parents_children_other`. + old_state.record_new_access(access_kind, rel_pos); let protected = global.borrow().protected_tags.contains_key(&node.tag); let transition = old_state.perform_access(access_kind, rel_pos, protected)?; From a7f5fac9f62365b5cc8c8906a2b9e3f0c90945d3 Mon Sep 17 00:00:00 2001 From: Johannes Hostert Date: Tue, 19 Nov 2024 17:29:17 +0100 Subject: [PATCH 3/5] refactor foreign access skipping --- .../tree_borrows/foreign_access_skipping.rs | 94 ++++++++ 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, 220 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..c0a10f720f --- /dev/null +++ b/src/borrow_tracker/tree_borrows/foreign_access_skipping.rs @@ -0,0 +1,94 @@ +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 + } + } +} + +mod tests { + use super::LastForeignAccess; + + #[test] + fn test_order() { + assert!(LastForeignAccess::LocalAccess < LastForeignAccess::ForeignRead); + assert!(LastForeignAccess::ForeignRead < LastForeignAccess::ForeignWrite); + } +} 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, }, }; From c61eb7f8f5b829e9d6af2a6dd3985802a0a7340f Mon Sep 17 00:00:00 2001 From: Johannes Hostert Date: Tue, 19 Nov 2024 21:15:03 +0100 Subject: [PATCH 4/5] Add mising cfg test --- src/borrow_tracker/tree_borrows/foreign_access_skipping.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/borrow_tracker/tree_borrows/foreign_access_skipping.rs b/src/borrow_tracker/tree_borrows/foreign_access_skipping.rs index c0a10f720f..9978674b3a 100644 --- a/src/borrow_tracker/tree_borrows/foreign_access_skipping.rs +++ b/src/borrow_tracker/tree_borrows/foreign_access_skipping.rs @@ -83,6 +83,7 @@ impl LastForeignAccess { } } +#[cfg(test)] mod tests { use super::LastForeignAccess; From dff1270e687a7aa018f33f245b212e6c0ce875fe Mon Sep 17 00:00:00 2001 From: Johannes Hostert Date: Tue, 19 Nov 2024 21:18:39 +0100 Subject: [PATCH 5/5] Address reviews 1: add an assertion to ReservedIM --- src/borrow_tracker/tree_borrows/perms.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/borrow_tracker/tree_borrows/perms.rs b/src/borrow_tracker/tree_borrows/perms.rs index 34fecb6752..f8e81e9cfd 100644 --- a/src/borrow_tracker/tree_borrows/perms.rs +++ b/src/borrow_tracker/tree_borrows/perms.rs @@ -97,6 +97,7 @@ impl PermissionPriv { // Otherwise, foreign reads do not affect it ReservedFrz { .. } => LastForeignAccess::ForeignRead, // Famously, ReservedIM survives foreign writes. It is never protected. + ReservedIM if prot => unreachable!("Protected ReservedIM should not exist!"), ReservedIM => LastForeignAccess::ForeignWrite, // Active changes on any foreign access (becomes Frozen/Disabled). Active => LastForeignAccess::LocalAccess, @@ -668,7 +669,9 @@ mod propagation_optimization_checks { // 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)); + if perm.compatible_with_protector() { + 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());