Tree::new_child: remove SIFA precondition and sync terminology

Author: RalfJung

src/tools/miri/src/borrow_tracker/tree_borrows/diagnostics.rs

@@ -504,7 +504,7 @@ impl DisplayFmt {
         if let Some(perm) = perm {
             format!(
                 "{ac}{st}",
-                ac = if perm.is_accessed() { self.accessed.yes } else { self.accessed.no },
+                ac = if perm.accessed() { self.accessed.yes } else { self.accessed.no },
                 st = perm.permission().short_name(),
             )
         } else {

src/tools/miri/src/borrow_tracker/tree_borrows/mod.rs

@@ -294,24 +294,6 @@ trait EvalContextPrivExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
             return interp_ok(Some(Provenance::Concrete { alloc_id, tag: new_tag }));
         }
 
-        let span = this.machine.current_span();
-
-        // When adding a new node, the SIFA of its parents needs to be updated, potentially across
-        // the entire memory range. For the parts that are being accessed below, the access itself
-        // trivially takes care of that. However, we have to do some more work to also deal with the
-        // parts that are not being accessed. Specifically what we do is that we call
-        // `update_last_accessed_after_retag` on the SIFA of the permission set for the part of
-        // memory outside `perm_map` -- so that part is definitely taken care of. The remaining
-        // concern is the part of memory that is in the range of `perms_map`, but not accessed
-        // below. There we have two cases:
-        // * If the type is `!Freeze`, then the non-accessed part uses `nonfreeze_perm`, so the
-        //   `nonfreeze_perm` initialized parts are also fine. We enforce the `freeze_perm` parts to
-        //   be accessed via the assert below, and thus everything is taken care of.
-        // * If the type is `Freeze`, then `freeze_perm` is used everywhere (both inside and outside
-        //   the initial range), and we update everything to have the `freeze_perm`'s SIFA, so there
-        //   are no issues. (And this assert below is not actually needed in this case).
-        assert!(new_perm.freeze_access);
-
         let protected = new_perm.protector.is_some();
         let precise_interior_mut = this
             .machine
@@ -337,7 +319,7 @@ trait EvalContextPrivExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                 LocationState::new_non_accessed(perm, sifa)
             }
         };
-        let perms_map = if !precise_interior_mut {
+        let inside_perms = if !precise_interior_mut {
             // For `!Freeze` types, just pretend the entire thing is an `UnsafeCell`.
             let ty_is_freeze = place.layout.ty.is_freeze(*this.tcx, this.typing_env());
             let state = loc_state(ty_is_freeze);
@@ -364,8 +346,8 @@ trait EvalContextPrivExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         let alloc_extra = this.get_alloc_extra(alloc_id)?;
         let mut tree_borrows = alloc_extra.borrow_tracker_tb().borrow_mut();
 
-        for (perm_range, perm) in perms_map.iter_all() {
-            if perm.is_accessed() {
+        for (perm_range, perm) in inside_perms.iter_all() {
+            if perm.accessed() {
                 // Some reborrows incur a read access to the parent.
                 // Adjust range to be relative to allocation start (rather than to `place`).
                 let range_in_alloc = AllocRange {
@@ -401,10 +383,10 @@ trait EvalContextPrivExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
             base_offset,
             orig_tag,
             new_tag,
-            perms_map,
+            inside_perms,
             new_perm.outside_perm,
             protected,
-            span,
+            this.machine.current_span(),
         )?;
         drop(tree_borrows);
 

src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs

@@ -11,7 +11,7 @@
 //! - idempotency properties asserted in `perms.rs` (for optimizations)
 
 use std::ops::Range;
-use std::{fmt, mem};
+use std::{cmp, fmt, mem};
 
 use rustc_abi::Size;
 use rustc_data_structures::fx::FxHashSet;
@@ -73,23 +73,10 @@ impl LocationState {
 
     /// Check if the location has been accessed, i.e. if it has
     /// ever been accessed through a child pointer.
-    pub fn is_accessed(&self) -> bool {
+    pub fn accessed(&self) -> bool {
         self.accessed
     }
 
-    /// Check if the state can exist as the initial permission of a pointer.
-    ///
-    /// Do not confuse with `is_accessed`, the two are almost orthogonal
-    /// as apart from `Unique` which is not initial and must be accessed,
-    /// any other permission can have an arbitrary combination of being
-    /// initial/accessed.
-    /// FIXME: when the corresponding `assert` in `tree_borrows/mod.rs` finally
-    /// passes and can be uncommented, remove this `#[allow(dead_code)]`.
-    #[cfg_attr(not(test), allow(dead_code))]
-    pub fn is_initial(&self) -> bool {
-        self.permission.is_initial()
-    }
-
     pub fn permission(&self) -> Permission {
         self.permission
     }
@@ -618,67 +605,62 @@ impl Tree {
 impl<'tcx> Tree {
     /// Insert a new tag in the tree.
     ///
-    /// `initial_perms` defines the initial permissions for the part of memory
-    /// that is already considered "initialized" immediately. The ranges in this
-    /// map are relative to `base_offset`.
-    /// `default_perm` defines the initial permission for the rest of the allocation.
-    ///
-    /// For all non-accessed locations in the RangeMap (those that haven't had an
-    /// implicit read), their SIFA must be weaker than or as weak as the SIFA of
-    /// `default_perm`.
+    /// `inside_perm` defines the initial permissions for a block of memory starting at
+    /// `base_offset`. These may nor may not be already marked as "accessed".
+    /// `outside_perm` defines the initial permission for the rest of the allocation.
+    /// These are definitely not "accessed".
     pub(super) fn new_child(
         &mut self,
         base_offset: Size,
         parent_tag: BorTag,
         new_tag: BorTag,
-        initial_perms: DedupRangeMap<LocationState>,
-        default_perm: Permission,
+        inside_perms: DedupRangeMap<LocationState>,
+        outside_perm: Permission,
         protected: bool,
         span: Span,
     ) -> InterpResult<'tcx> {
         let idx = self.tag_mapping.insert(new_tag);
         let parent_idx = self.tag_mapping.get(&parent_tag).unwrap();
-        assert!(default_perm.is_initial());
+        assert!(outside_perm.is_initial());
 
         let default_strongest_idempotent =
-            default_perm.strongest_idempotent_foreign_access(protected);
+            outside_perm.strongest_idempotent_foreign_access(protected);
         // Create the node
         self.nodes.insert(
             idx,
             Node {
                 tag: new_tag,
                 parent: Some(parent_idx),
                 children: SmallVec::default(),
-                default_initial_perm: default_perm,
+                default_initial_perm: outside_perm,
                 default_initial_idempotent_foreign_access: default_strongest_idempotent,
-                debug_info: NodeDebugInfo::new(new_tag, default_perm, span),
+                debug_info: NodeDebugInfo::new(new_tag, outside_perm, span),
             },
         );
         // Register new_tag as a child of parent_tag
         self.nodes.get_mut(parent_idx).unwrap().children.push(idx);
 
+        // We need to know the biggest SIFA for `update_last_accessed_after_retag` below.
+        let mut max_sifa = default_strongest_idempotent;
         for (Range { start, end }, &perm) in
-            initial_perms.iter(Size::from_bytes(0), initial_perms.size())
+            inside_perms.iter(Size::from_bytes(0), inside_perms.size())
         {
-            assert!(perm.is_initial());
+            assert!(perm.permission.is_initial());
+            max_sifa = cmp::max(max_sifa, perm.idempotent_foreign_access);
             for (_perms_range, perms) in self
                 .rperms
                 .iter_mut(Size::from_bytes(start) + base_offset, Size::from_bytes(end - start))
             {
-                assert!(
-                    default_strongest_idempotent
-                        >= perm.permission.strongest_idempotent_foreign_access(protected)
-                );
                 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.
-        // See the comment in `tb_reborrow` for why it is correct to use the SIFA of `default_uninit_perm`.
-        self.update_last_accessed_after_retag(parent_idx, default_strongest_idempotent);
+        // 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. For this
+        // we need the *maximum* SIFA (`Write` needs more fixup than `None`).
+        self.update_last_accessed_after_retag(parent_idx, max_sifa);
 
         interp_ok(())
     }
@@ -755,9 +737,9 @@ impl<'tcx> Tree {
                             == Some(&ProtectorKind::StrongProtector)
                             // Don't check for protector if it is a Cell (see `unsafe_cell_deallocate` in `interior_mutability.rs`).
                             // Related to https://github.com/rust-lang/rust/issues/55005.
-                            && !perm.permission().is_cell()
+                            && !perm.permission.is_cell()
                             // Only trigger UB if the accessed bit is set, i.e. if the protector is actually protecting this offset. See #4579.
-                            && perm.is_accessed()
+                            && perm.accessed
                         {
                             Err(TransitionError::ProtectedDealloc)
                         } else {

src/tools/miri/src/borrow_tracker/tree_borrows/tree/tests.rs

@@ -106,7 +106,7 @@ fn tree_compacting_is_sound() {
                         as_foreign_or_child(rel),
                         kind,
                         parent.permission(),
-                        as_lazy_or_accessed(child.is_accessed()),
+                        as_lazy_or_accessed(child.accessed()),
                         child.permission(),
                         as_protected(child_protected),
                         np.permission(),
@@ -122,7 +122,7 @@ fn tree_compacting_is_sound() {
                         as_foreign_or_child(rel),
                         kind,
                         parent.permission(),
-                        as_lazy_or_accessed(child.is_accessed()),
+                        as_lazy_or_accessed(child.accessed()),
                         child.permission(),
                         as_protected(child_protected),
                         nc.permission()
@@ -375,7 +375,7 @@ mod spurious_read {
 
     impl LocStateProt {
         fn is_initial(&self) -> bool {
-            self.state.is_initial()
+            self.state.permission().is_initial()
         }
 
         fn perform_access(&self, kind: AccessKind, rel: AccessRelatedness) -> Result<Self, ()> {