Merge pull request #4273 from yoctocell/new-cell-state

TB: add `Cell` state to support more fine-grained tracking of interior mutable data

Author: RalfJung

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

@@ -123,6 +123,10 @@ struct NewPermission {
     /// Whether this pointer is part of the arguments of a function call.
     /// `protector` is `Some(_)` for all pointers marked `noalias`.
     protector: Option<ProtectorKind>,
+    /// Whether a read should be performed on a retag.  This should be `false`
+    /// for `Cell` because this could cause data races when using thread-safe
+    /// data types like `Mutex<T>`.
+    initial_read: bool,
 }
 
 impl<'tcx> NewPermission {
@@ -141,18 +145,19 @@ impl<'tcx> NewPermission {
         // To eliminate the case of Protected Reserved IM we override interior mutability
         // in the case of a protected reference: protected references are always considered
         // "freeze" in their reservation phase.
-        let initial_state = match mutability {
-            Mutability::Mut if ty_is_unpin => Permission::new_reserved(ty_is_freeze, is_protected),
-            Mutability::Not if ty_is_freeze => Permission::new_frozen(),
+        let (initial_state, initial_read) = match mutability {
+            Mutability::Mut if ty_is_unpin =>
+                (Permission::new_reserved(ty_is_freeze, is_protected), true),
+            Mutability::Not if ty_is_freeze => (Permission::new_frozen(), true),
+            Mutability::Not if !ty_is_freeze => (Permission::new_cell(), false),
             // Raw pointers never enter this function so they are not handled.
             // However raw pointers are not the only pointers that take the parent
-            // tag, this also happens for `!Unpin` `&mut`s and interior mutable
-            // `&`s, which are excluded above.
+            // tag, this also happens for `!Unpin` `&mut`s, which are excluded above.
             _ => return None,
         };
 
         let protector = is_protected.then_some(ProtectorKind::StrongProtector);
-        Some(Self { zero_size: false, initial_state, protector })
+        Some(Self { zero_size: false, initial_state, protector, initial_read })
     }
 
     /// Compute permission for `Box`-like type (`Box` always, and also `Unique` if enabled).
@@ -175,6 +180,7 @@ impl<'tcx> NewPermission {
                 zero_size,
                 initial_state,
                 protector: protected.then_some(ProtectorKind::WeakProtector),
+                initial_read: true,
             }
         })
     }
@@ -289,13 +295,15 @@ trait EvalContextPrivExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         let mut tree_borrows = alloc_extra.borrow_tracker_tb().borrow_mut();
 
         // All reborrows incur a (possibly zero-sized) read access to the parent
-        tree_borrows.perform_access(
-            orig_tag,
-            Some((range, AccessKind::Read, diagnostics::AccessCause::Reborrow)),
-            this.machine.borrow_tracker.as_ref().unwrap(),
-            alloc_id,
-            this.machine.current_span(),
-        )?;
+        if new_perm.initial_read {
+            tree_borrows.perform_access(
+                orig_tag,
+                Some((range, AccessKind::Read, diagnostics::AccessCause::Reborrow)),
+                this.machine.borrow_tracker.as_ref().unwrap(),
+                alloc_id,
+                this.machine.current_span(),
+            )?;
+        }
         // Record the parent-child pair in the tree.
         tree_borrows.new_child(
             orig_tag,
@@ -308,7 +316,7 @@ trait EvalContextPrivExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         drop(tree_borrows);
 
         // Also inform the data race model (but only if any bytes are actually affected).
-        if range.size.bytes() > 0 {
+        if range.size.bytes() > 0 && new_perm.initial_read {
             if let Some(data_race) = alloc_extra.data_race.as_ref() {
                 data_race.read(
                     alloc_id,
@@ -535,6 +543,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
             initial_state: Permission::new_reserved(ty_is_freeze, /* protected */ true),
             zero_size: false,
             protector: Some(ProtectorKind::StrongProtector),
+            initial_read: true,
         };
         this.tb_retag_place(place, new_perm)
     }

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

@@ -8,6 +8,10 @@ use crate::borrow_tracker::tree_borrows::tree::AccessRelatedness;
 /// The activation states of a pointer.
 #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
 enum PermissionPriv {
+    /// represents: a shared reference to interior mutable data.
+    /// allows: all foreign and child accesses;
+    /// rejects: nothing
+    Cell,
     /// represents: a local mutable reference that has not yet been written to;
     /// allows: child reads, foreign reads;
     /// affected by: child writes (becomes Active),
@@ -60,6 +64,14 @@ impl PartialOrd for PermissionPriv {
         use Ordering::*;
         Some(match (self, other) {
             (a, b) if a == b => Equal,
+            // Versions of `Reserved` with different interior mutability are incomparable with each
+            // other.
+            (ReservedIM, ReservedFrz { .. })
+            | (ReservedFrz { .. }, ReservedIM)
+            // `Cell` is not comparable with any other permission
+            // since it never transitions to any other state and we
+            // can never get to `Cell` from another state.
+            | (Cell, _) | (_, Cell) => return None,
             (Disabled, _) => Greater,
             (_, Disabled) => Less,
             (Frozen, _) => Greater,
@@ -71,27 +83,29 @@ impl PartialOrd for PermissionPriv {
                 // `bool` is ordered such that `false <= true`, so this works as intended.
                 c1.cmp(c2)
             }
-            // Versions of `Reserved` with different interior mutability are incomparable with each
-            // other.
-            (ReservedIM, ReservedFrz { .. }) | (ReservedFrz { .. }, ReservedIM) => return None,
         })
     }
 }
 
 impl PermissionPriv {
     /// Check if `self` can be the initial state of a pointer.
     fn is_initial(&self) -> bool {
-        matches!(self, ReservedFrz { conflicted: false } | Frozen | ReservedIM)
+        matches!(self, ReservedFrz { conflicted: false } | Frozen | ReservedIM | Cell)
     }
 
     /// Reject `ReservedIM` that cannot exist in the presence of a protector.
     fn compatible_with_protector(&self) -> bool {
-        !matches!(self, ReservedIM)
+        // FIXME(TB-Cell): It is unclear what to do here.
+        // `Cell` will occur with a protector but won't provide the guarantees
+        // of noalias (it will fail the `protected_enforces_noalias` test).
+        !matches!(self, ReservedIM | Cell)
     }
 
     /// See `foreign_access_skipping.rs`. Computes the SIFA of a permission.
     fn strongest_idempotent_foreign_access(&self, prot: bool) -> IdempotentForeignAccess {
         match self {
+            // Cell survives any foreign access
+            Cell => IdempotentForeignAccess::Write,
             // A protected non-conflicted Reserved will become conflicted under a foreign read,
             // and is hence not idempotent under it.
             ReservedFrz { conflicted } if prot && !conflicted => IdempotentForeignAccess::None,
@@ -124,14 +138,16 @@ mod transition {
             Disabled => return None,
             // The inner data `ty_is_freeze` of `Reserved` is always irrelevant for Read
             // accesses, since the data is not being mutated. Hence the `{ .. }`.
-            readable @ (ReservedFrz { .. } | ReservedIM | Active | Frozen) => readable,
+            readable @ (Cell | ReservedFrz { .. } | ReservedIM | Active | Frozen) => readable,
         })
     }
 
     /// A non-child node was read-accessed: keep `Reserved` but mark it as `conflicted` if it
     /// is protected; invalidate `Active`.
     fn foreign_read(state: PermissionPriv, protected: bool) -> Option<PermissionPriv> {
         Some(match state {
+            // Cell ignores foreign reads.
+            Cell => Cell,
             // Non-writeable states just ignore foreign reads.
             non_writeable @ (Frozen | Disabled) => non_writeable,
             // Writeable states are more tricky, and depend on whether things are protected.
@@ -167,6 +183,8 @@ mod transition {
     /// write permissions, `Frozen` and `Disabled` cannot obtain such permissions and produce UB.
     fn child_write(state: PermissionPriv, protected: bool) -> Option<PermissionPriv> {
         Some(match state {
+            // Cell ignores child writes.
+            Cell => Cell,
             // If the `conflicted` flag is set, then there was a foreign read during
             // the function call that is still ongoing (still `protected`),
             // this is UB (`noalias` violation).
@@ -185,6 +203,8 @@ mod transition {
         // types receive a `ReservedFrz` instead of `ReservedIM` when retagged under a protector,
         // so the result of this function does indirectly depend on (past) protector status.
         Some(match state {
+            // Cell ignores foreign writes.
+            Cell => Cell,
             res @ ReservedIM => {
                 // We can never create a `ReservedIM` under a protector, only `ReservedFrz`.
                 assert!(!protected);
@@ -242,6 +262,11 @@ impl Permission {
         self.inner == Frozen
     }
 
+    /// Check if `self` is the shared-reference-to-interior-mutable-data state of a pointer.
+    pub fn is_cell(&self) -> bool {
+        self.inner == Cell
+    }
+
     /// Default initial permission of the root of a new tree at inbounds positions.
     /// Must *only* be used for the root, this is not in general an "initial" permission!
     pub fn new_active() -> Self {
@@ -278,11 +303,27 @@ impl Permission {
         Self { inner: Disabled }
     }
 
+    /// Default initial permission of a shared reference to interior mutable data.
+    pub fn new_cell() -> Self {
+        Self { inner: Cell }
+    }
+
     /// Reject `ReservedIM` that cannot exist in the presence of a protector.
     pub fn compatible_with_protector(&self) -> bool {
         self.inner.compatible_with_protector()
     }
 
+    /// What kind of access to perform before releasing the protector.
+    pub fn protector_end_access(&self) -> Option<AccessKind> {
+        match self.inner {
+            // Do not do perform access if it is a `Cell`, as this
+            // can cause data races when using thread-safe data types.
+            Cell => None,
+            Active => Some(AccessKind::Write),
+            _ => Some(AccessKind::Read),
+        }
+    }
+
     /// Apply the transition to the inner PermissionPriv.
     pub fn perform_access(
         kind: AccessKind,
@@ -306,30 +347,32 @@ impl Permission {
     /// remove protected parents.
     pub fn can_be_replaced_by_child(self, child: Self) -> bool {
         match (self.inner, child.inner) {
-            // ReservedIM can be replaced by anything, as it allows all
-            // transitions.
+            // Cell allows all transitions.
+            (Cell, _) => true,
+            // Cell is the most permissive, nothing can be replaced by Cell.
+            // (ReservedIM, Cell) => true,
+            (_, Cell) => false,
+            // ReservedIM can be replaced by anything besides Cell.
+            // ReservedIM allows all transitions, but unlike Cell, a local write
+            // to ReservedIM transitions to Active, while it is a no-op for Cell.
             (ReservedIM, _) => true,
+            (_, ReservedIM) => false,
             // Reserved (as parent, where conflictedness does not matter)
-            // can be replaced by all but ReservedIM,
-            // since ReservedIM alone would survive foreign writes
-            (ReservedFrz { .. }, ReservedIM) => false,
+            // can be replaced by all but ReservedIM and Cell,
+            // since ReservedIM and Cell alone would survive foreign writes
             (ReservedFrz { .. }, _) => true,
+            (_, ReservedFrz { .. }) => false,
             // Active can not be replaced by something surviving
-            // foreign reads and then remaining writable.
-            (Active, ReservedIM) => false,
-            (Active, ReservedFrz { .. }) => false,
+            // foreign reads and then remaining writable (i.e., Reserved*).
             // Replacing a state by itself is always okay, even if the child state is protected.
-            (Active, Active) => true,
             // Active can be replaced by Frozen, since it is not protected.
-            (Active, Frozen) => true,
-            (Active, Disabled) => true,
+            (Active, Active | Frozen | Disabled) => true,
+            (_, Active) => false,
             // Frozen can only be replaced by Disabled (and itself).
-            (Frozen, Frozen) => true,
-            (Frozen, Disabled) => true,
-            (Frozen, _) => false,
+            (Frozen, Frozen | Disabled) => true,
+            (_, Frozen) => false,
             // Disabled can not be replaced by anything else.
             (Disabled, Disabled) => true,
-            (Disabled, _) => false,
         }
     }
 
@@ -383,6 +426,7 @@ pub mod diagnostics {
                 f,
                 "{}",
                 match self {
+                    Cell => "Cell",
                     ReservedFrz { conflicted: false } => "Reserved",
                     ReservedFrz { conflicted: true } => "Reserved (conflicted)",
                     ReservedIM => "Reserved (interior mutable)",
@@ -413,6 +457,7 @@ pub mod diagnostics {
             // and also as `diagnostics::DisplayFmtPermission.uninit` otherwise
             // alignment will be incorrect.
             match self.inner {
+                Cell => "Cel ",
                 ReservedFrz { conflicted: false } => "Res ",
                 ReservedFrz { conflicted: true } => "ResC",
                 ReservedIM => "ReIM",
@@ -459,7 +504,7 @@ pub mod diagnostics {
         ///   (Reserved < Active < Frozen < Disabled);
         /// - between `self` and `err` the permission should also be increasing,
         ///   so all permissions inside `err` should be greater than `self.1`;
-        /// - `Active` and `Reserved(conflicted=false)` cannot cause an error
+        /// - `Active`, `Reserved(conflicted=false)`, and `Cell` cannot cause an error
         ///   due to insufficient permissions, so `err` cannot be a `ChildAccessForbidden(_)`
         ///   of either of them;
         /// - `err` should not be `ProtectedDisabled(Disabled)`, because the protected
@@ -492,13 +537,14 @@ pub mod diagnostics {
                         (ReservedFrz { conflicted: true } | Active | Frozen, Disabled) => false,
                         (ReservedFrz { conflicted: true }, Frozen) => false,
 
-                        // `Active` and `Reserved` have all permissions, so a
+                        // `Active`, `Reserved`, and `Cell` have all permissions, so a
                         // `ChildAccessForbidden(Reserved | Active)` can never exist.
-                        (_, Active) | (_, ReservedFrz { conflicted: false }) =>
+                        (_, Active) | (_, ReservedFrz { conflicted: false }) | (_, Cell) =>
                             unreachable!("this permission cannot cause an error"),
                         // No transition has `Reserved { conflicted: false }` or `ReservedIM`
-                        // as its `.to` unless it's a noop.
-                        (ReservedFrz { conflicted: false } | ReservedIM, _) =>
+                        // as its `.to` unless it's a noop. `Cell` cannot be in its `.to`
+                        // because all child accesses are a noop.
+                        (ReservedFrz { conflicted: false } | ReservedIM | Cell, _) =>
                             unreachable!("self is a noop transition"),
                         // All transitions produced in normal executions (using `apply_access`)
                         // change permissions in the order `Reserved -> Active -> Frozen -> Disabled`.
@@ -544,16 +590,17 @@ pub mod diagnostics {
                                 "permission that results in Disabled should not itself be Disabled in the first place"
                             ),
                         // No transition has `Reserved { conflicted: false }` or `ReservedIM` as its `.to`
-                        // unless it's a noop.
-                        (ReservedFrz { conflicted: false } | ReservedIM, _) =>
+                        // unless it's a noop. `Cell` cannot be in its `.to` because all child
+                        // accesses are a noop.
+                        (ReservedFrz { conflicted: false } | ReservedIM | Cell, _) =>
                             unreachable!("self is a noop transition"),
 
                         // Permissions only evolve in the order `Reserved -> Active -> Frozen -> Disabled`,
                         // so permissions found must be increasing in the order
                         // `self.from < self.to <= forbidden.from < forbidden.to`.
-                        (Disabled, ReservedFrz { .. } | ReservedIM | Active | Frozen)
-                        | (Frozen, ReservedFrz { .. } | ReservedIM | Active)
-                        | (Active, ReservedFrz { .. } | ReservedIM) =>
+                        (Disabled, Cell | ReservedFrz { .. } | ReservedIM | Active | Frozen)
+                        | (Frozen, Cell | ReservedFrz { .. } | ReservedIM | Active)
+                        | (Active, Cell | ReservedFrz { .. } | ReservedIM) =>
                             unreachable!("permissions between self and err must be increasing"),
                     }
                 }
@@ -590,7 +637,7 @@ mod propagation_optimization_checks {
     impl Exhaustive for PermissionPriv {
         fn exhaustive() -> Box<dyn Iterator<Item = Self>> {
             Box::new(
-                vec![Active, Frozen, Disabled, ReservedIM]
+                vec![Active, Frozen, Disabled, ReservedIM, Cell]
                     .into_iter()
                     .chain(<bool>::exhaustive().map(|conflicted| ReservedFrz { conflicted })),
             )

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

@@ -721,9 +721,14 @@ impl<'tcx> Tree {
                     // visit all children, skipping none
                     |_| ContinueTraversal::Recurse,
                     |args: NodeAppArgs<'_>| -> Result<(), TransitionError> {
-                        let NodeAppArgs { node, .. } = args;
+                        let NodeAppArgs { node, perm, .. } = args;
+                        let perm =
+                            perm.get().copied().unwrap_or_else(|| node.default_location_state());
                         if global.borrow().protected_tags.get(&node.tag)
                             == 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()
                         {
                             Err(TransitionError::ProtectedDealloc)
                         } else {
@@ -865,10 +870,9 @@ impl<'tcx> Tree {
                 let idx = self.tag_mapping.get(&tag).unwrap();
                 // Only visit initialized permissions
                 if let Some(p) = perms.get(idx)
+                    && let Some(access_kind) = p.permission.protector_end_access()
                     && p.initialized
                 {
-                    let access_kind =
-                        if p.permission.is_active() { AccessKind::Write } else { AccessKind::Read };
                     let access_cause = diagnostics::AccessCause::FnExit(access_kind);
                     TreeVisitor { nodes: &mut self.nodes, tag_mapping: &self.tag_mapping, perms }
                         .traverse_nonchildren(