Merge pull request #4595 from RalfJung/tb-terms

TB: update terminology to match paper & MiniRust

Author: RalfJung

src/borrow_tracker/tree_borrows/diagnostics.rs

@@ -244,8 +244,8 @@ pub(super) enum TransitionError {
     ChildAccessForbidden(Permission),
     /// A protector was triggered due to an invalid transition that loses
     /// too much permissions.
-    /// For example, if a protected tag goes from `Active` to `Disabled` due
-    /// to a foreign write this will produce a `ProtectedDisabled(Active)`.
+    /// For example, if a protected tag goes from `Unique` to `Disabled` due
+    /// to a foreign write this will produce a `ProtectedDisabled(Unique)`.
     /// This kind of error can only occur on foreign accesses.
     ProtectedDisabled(Permission),
     /// Cannot deallocate because some tag in the allocation is strongly protected.
@@ -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/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/borrow_tracker/tree_borrows/perms.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;
@@ -57,7 +57,7 @@ pub(super) struct LocationState {
 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`.
+    /// The permission is not allowed to be `Unique`.
     /// `sifa` is the (strongest) idempotent foreign access, see `foreign_access_skipping.rs`
     pub fn new_non_accessed(permission: Permission, sifa: IdempotentForeignAccess) -> Self {
         assert!(permission.is_initial() || permission.is_disabled());
@@ -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 `Active` 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
     }
@@ -170,7 +157,7 @@ impl LocationState {
             }
             if self.permission.is_frozen() && access_kind == AccessKind::Read {
                 // A foreign read to a `Frozen` tag will have almost no observable effect.
-                // It's a theorem that `Frozen` nodes have no active children, so all children
+                // It's a theorem that `Frozen` nodes have no `Unique` children, so all children
                 // already survive foreign reads. Foreign reads in general have almost no
                 // effect, the only further thing they could do is make protected `Reserved`
                 // nodes become conflicted, i.e. make them reject child writes for the further
@@ -265,7 +252,7 @@ pub(super) struct Node {
     pub children: SmallVec<[UniIndex; 4]>,
     /// Either `Reserved`,  `Frozen`, or `Disabled`, it is the permission this tag will
     /// lazily be initialized to on the first access.
-    /// It is only ever `Disabled` for a tree root, since the root is initialized to `Active` by
+    /// It is only ever `Disabled` for a tree root, since the root is initialized to `Unique` by
     /// its own separate mechanism.
     default_initial_perm: Permission,
     /// The default initial (strongest) idempotent foreign access.
@@ -598,14 +585,14 @@ impl Tree {
         };
         let rperms = {
             let mut perms = UniValMap::default();
-            // We manually set it to `Active` on all in-bounds positions.
-            // We also ensure that it is accessed, so that no `Active` but
+            // We manually set it to `Unique` on all in-bounds positions.
+            // We also ensure that it is accessed, so that no `Unique` but
             // not yet accessed nodes exist. Essentially, we pretend there
-            // was a write that initialized these to `Active`.
+            // was a write that initialized these to `Unique`.
             perms.insert(
                 root_idx,
                 LocationState::new_accessed(
-                    Permission::new_active(),
+                    Permission::new_unique(),
                     IdempotentForeignAccess::None,
                 ),
             );
@@ -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 {
@@ -790,7 +772,7 @@ impl<'tcx> Tree {
     /// - the access will be applied only to accessed locations of the allocation,
     /// - it will not be visible to children,
     /// - it will be recorded as a `FnExit` diagnostic access
-    /// - and it will be a read except if the location is `Active`, i.e. has been written to,
+    /// - and it will be a read except if the location is `Unique`, i.e. has been written to,
     ///   in which case it will be a write.
     ///
     /// `LocationState::perform_access` will take care of raising transition

src/borrow_tracker/tree_borrows/tree.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, ()> {
@@ -420,7 +420,7 @@ mod spurious_read {
     /// `(LocStateProt, LocStateProt)` where the two states are not guaranteed
     /// to be updated at the same time.
     /// Some `LocStateProtPair` may be unreachable through normal means
-    /// such as `x: Active, y: Active` in the case of mutually foreign pointers.
+    /// such as `x: Unique, y: Unique` in the case of mutually foreign pointers.
     struct LocStateProtPair {
         xy_rel: RelPosXY,
         x: LocStateProt,
@@ -709,7 +709,7 @@ mod spurious_read {
         let mut err = 0;
         for pat in Pattern::exhaustive() {
             let Ok(initial_source) = pat.initial_state() else {
-                // Failed to retag `x` in the source (e.g. `y` was protected Active)
+                // Failed to retag `x` in the source (e.g. `y` was protected Unique)
                 continue;
             };
             // `x` must stay protected, but the function protecting `y` might return here

src/borrow_tracker/tree_borrows/tree/tests.rs

@@ -16,7 +16,7 @@ LL | |             Poll::<()>::Pending
 LL | |         })
 LL | |         .await
    | |______________^
-help: the accessed tag <TAG> later transitioned to Active due to a child write access at offsets [OFFSET]
+help: the accessed tag <TAG> later transitioned to Unique due to a child write access at offsets [OFFSET]
   --> tests/fail/async-shared-mutable.rs:LL:CC
    |
 LL |             *x = 1;

tests/fail/async-shared-mutable.tree.stderr

@@ -7,7 +7,7 @@ LL |     *y
    = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental
    = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/tree-borrows.md for further information
    = help: the accessed tag <TAG> is foreign to the protected tag <TAG> (i.e., it is not a child)
-   = help: this foreign read access would cause the protected tag <TAG> (currently Active) to become Disabled
+   = help: this foreign read access would cause the protected tag <TAG> (currently Unique) to become Disabled
    = help: protected tags must never be Disabled
 help: the accessed tag <TAG> was created here
   --> tests/fail/both_borrows/box_noalias_violation.rs:LL:CC
@@ -19,7 +19,7 @@ help: the protected tag <TAG> was created here, in the initial state Reserved
    |
 LL | unsafe fn test(mut x: Box<i32>, y: *const i32) -> i32 {
    |                ^^^^^
-help: the protected tag <TAG> later transitioned to Active due to a child write access at offsets [0x0..0x4]
+help: the protected tag <TAG> later transitioned to Unique due to a child write access at offsets [0x0..0x4]
   --> tests/fail/both_borrows/box_noalias_violation.rs:LL:CC
    |
 LL |     *x = 5;

tests/fail/both_borrows/box_noalias_violation.tree.stderr

@@ -7,7 +7,7 @@ LL |     unsafe { *y = 2 };
    = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental
    = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/tree-borrows.md for further information
    = help: the accessed tag <TAG> is foreign to the protected tag <TAG> (i.e., it is not a child)
-   = help: this foreign write access would cause the protected tag <TAG> (currently Active) to become Disabled
+   = help: this foreign write access would cause the protected tag <TAG> (currently Unique) to become Disabled
    = help: protected tags must never be Disabled
 help: the accessed tag <TAG> was created here
   --> tests/fail/both_borrows/illegal_write6.rs:LL:CC
@@ -19,7 +19,7 @@ help: the protected tag <TAG> was created here, in the initial state Reserved
    |
 LL | fn foo(a: &mut u32, y: *mut u32) -> u32 {
    |        ^
-help: the protected tag <TAG> later transitioned to Active due to a child write access at offsets [0x0..0x4]
+help: the protected tag <TAG> later transitioned to Unique due to a child write access at offsets [0x0..0x4]
   --> tests/fail/both_borrows/illegal_write6.rs:LL:CC
    |
 LL |     *a = 1;