Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 1, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 23 additions & 14 deletions src/borrow_tracker/tree_borrows/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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).
Expand All @@ -175,6 +180,7 @@ impl<'tcx> NewPermission {
zero_size,
initial_state,
protector: protected.then_some(ProtectorKind::WeakProtector),
initial_read: true,
}
})
}
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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)
}
Expand Down
111 changes: 79 additions & 32 deletions src/borrow_tracker/tree_borrows/perms.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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).
Expand All @@ -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);
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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,
Expand All @@ -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,
}
}

Expand Down Expand Up @@ -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)",
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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`.
Expand Down Expand Up @@ -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"),
}
}
Expand Down Expand Up @@ -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 })),
)
Expand Down
10 changes: 7 additions & 3 deletions src/borrow_tracker/tree_borrows/tree.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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(
Expand Down
Loading