From 00adb653f2e3be26174cabb0cbceccefa6052567 Mon Sep 17 00:00:00 2001 From: Johannes Hostert Date: Mon, 25 Mar 2024 17:35:29 +0100 Subject: [PATCH] Tree Borrows: Make tree root always be initialized There should never be an `Active` but not initialized node in the borrow tree. If such a node exists, and if it has a protector, then on a foreign read, it could become disabled. This leads to some problems when formally proving that read moving optimizations are sound. The root node is the only node for which this can happen, since all other nodes can only become `Active` when actually used. But special- casing the root node here is annoying to reason about, everything becomes much nicer if we can simply say that *all* `Active` nodes must be initialized. This requires making the root node default- initialized. This is also more intuitive, since the root arguably becomes ini- tialized during the allocation, which can be considered a write. --- src/borrow_tracker/tree_borrows/tree.rs | 20 ++++++++++++++----- src/borrow_tracker/tree_borrows/tree/tests.rs | 4 ++-- 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/src/borrow_tracker/tree_borrows/tree.rs b/src/borrow_tracker/tree_borrows/tree.rs index dda1c7cca1..d7ea4ec99b 100644 --- a/src/borrow_tracker/tree_borrows/tree.rs +++ b/src/borrow_tracker/tree_borrows/tree.rs @@ -42,6 +42,7 @@ pub(super) struct LocationState { /// protected, that is *not* the case for uninitialized locations. Instead we just have a latent /// "future initial permission" of `Disabled`, causing UB only if an access is ever actually /// performed. + /// Note that the tree root is also always initialized, as if the allocation was a write access. initialized: bool, /// This pointer's current permission / future initial permission. permission: Permission, @@ -57,8 +58,8 @@ pub(super) struct LocationState { impl LocationState { /// Default initial state has never been accessed and has been subjected to no /// foreign access. - fn new(permission: Permission) -> Self { - Self { permission, initialized: false, latest_foreign_access: None } + fn new(initialized: bool, permission: Permission) -> Self { + Self { permission, initialized, latest_foreign_access: None } } /// Record that this location was accessed through a child pointer by @@ -240,7 +241,12 @@ pub(super) struct Node { pub children: SmallVec<[UniIndex; 4]>, /// Either `Reserved` or `Frozen`, the permission this tag will be lazily initialized /// to on the first access. + /// Can also be `Active` for the root node, in which case we consider every node + /// to be already initialized (see `is_default_initialized`). default_initial_perm: Permission, + /// Whether this node is always considered to be initialized. This is `true` for the + /// root node, which is also the only by-default `Active` node. + is_default_initialized: bool, /// Some extra information useful only for debugging purposes pub debug_info: NodeDebugInfo, } @@ -445,6 +451,7 @@ impl Tree { /// Create a new tree, with only a root pointer. pub fn new(root_tag: BorTag, size: Size, span: Span) -> Self { let root_perm = Permission::new_active(); + let root_is_init = true; let mut tag_mapping = UniKeyMap::default(); let root_idx = tag_mapping.insert(root_tag); let nodes = { @@ -459,6 +466,7 @@ impl Tree { parent: None, children: SmallVec::default(), default_initial_perm: root_perm, + is_default_initialized: root_is_init, debug_info, }, ); @@ -466,7 +474,7 @@ impl Tree { }; let rperms = { let mut perms = UniValMap::default(); - perms.insert(root_idx, LocationState::new(root_perm).with_access()); + perms.insert(root_idx, LocationState::new(root_is_init, root_perm).with_access()); RangeMap::new(size, perms) }; Self { root: root_idx, nodes, rperms, tag_mapping } @@ -483,6 +491,7 @@ impl<'tcx> Tree { reborrow_range: AllocRange, span: Span, ) -> InterpResult<'tcx> { + let is_default_initialized = false; 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(); @@ -494,13 +503,14 @@ impl<'tcx> Tree { parent: Some(parent_idx), children: SmallVec::default(), default_initial_perm, + is_default_initialized, 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(default_initial_perm).with_access(); + let perm = LocationState::new(is_default_initialized, default_initial_perm).with_access(); for (_perms_range, perms) in self.rperms.iter_mut(reborrow_range.start, reborrow_range.size) { perms.insert(idx, perm); @@ -599,7 +609,7 @@ impl<'tcx> Tree { -> Result { let NodeAppArgs { node, mut perm, rel_pos } = args; - let old_state = perm.or_insert(LocationState::new(node.default_initial_perm)); + let old_state = perm.or_insert(LocationState::new(node.is_default_initialized, node.default_initial_perm)); match old_state.skip_if_known_noop(access_kind, rel_pos) { ContinueTraversal::SkipChildren => return Ok(ContinueTraversal::SkipChildren), diff --git a/src/borrow_tracker/tree_borrows/tree/tests.rs b/src/borrow_tracker/tree_borrows/tree/tests.rs index 35f3b53afd..5501942814 100644 --- a/src/borrow_tracker/tree_borrows/tree/tests.rs +++ b/src/borrow_tracker/tree_borrows/tree/tests.rs @@ -516,11 +516,11 @@ mod spurious_read { let source = LocStateProtPair { xy_rel: RelPosXY::MutuallyForeign, x: LocStateProt { - state: LocationState::new(Permission::new_frozen()).with_access(), + state: LocationState::new(false, Permission::new_frozen()).with_access(), prot: true, }, y: LocStateProt { - state: LocationState::new(Permission::new_reserved(false)), + state: LocationState::new(false, Permission::new_reserved(false)), prot: true, }, };