now we do not need weak protectors any more

Author: RalfJung

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

@@ -95,7 +95,7 @@ pub struct GlobalStateInner {
     /// We add tags to this when they are created with a protector in `reborrow`, and
     /// we remove tags from this when the call which is protecting them returns, in
     /// `GlobalStateInner::end_call`. See `Stack::item_popped` for more details.
-    pub protected_tags: FxHashMap<BorTag, ProtectorKind>,
+    pub protected_tags: FxHashSet<BorTag>,
     /// The pointer ids to trace
     pub tracked_pointer_tags: FxHashSet<BorTag>,
     /// The call ids to trace
@@ -142,26 +142,6 @@ pub enum RetagFields {
     OnlyScalar,
 }
 
-/// The flavor of the protector.
-#[derive(Copy, Clone, Debug, PartialEq, Eq)]
-pub enum ProtectorKind {
-    /// Protected against aliasing violations from other pointers.
-    ///
-    /// Items protected like this cause UB when they are invalidated, *but* the pointer itself may
-    /// still be used to issue a deallocation.
-    ///
-    /// This is required for LLVM IR pointers that are `noalias` but *not* `dereferenceable`.
-    WeakProtector,
-
-    /// Protected against any kind of invalidation.
-    ///
-    /// Items protected like this cause UB when they are invalidated or the memory is deallocated.
-    /// This is strictly stronger protection than `WeakProtector`.
-    ///
-    /// This is required for LLVM IR pointers that are `dereferenceable` (and also allows `noalias`).
-    StrongProtector,
-}
-
 /// Utilities for initialization and ID generation
 impl GlobalStateInner {
     pub fn new(
@@ -175,7 +155,7 @@ impl GlobalStateInner {
             next_ptr_tag: BorTag::one(),
             base_ptr_tags: FxHashMap::default(),
             next_call_id: NonZeroU64::new(1).unwrap(),
-            protected_tags: FxHashMap::default(),
+            protected_tags: FxHashSet::default(),
             tracked_pointer_tags,
             tracked_call_ids,
             retag_fields,

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

@@ -6,8 +6,7 @@ use rustc_span::{Span, SpanData};
 use rustc_target::abi::Size;
 
 use crate::borrow_tracker::{
-    stacked_borrows::{err_sb_ub, Permission},
-    AccessKind, GlobalStateInner, ProtectorKind,
+    stacked_borrows::{err_sb_ub, Permission}, AccessKind, GlobalStateInner,
 };
 use crate::*;
 
@@ -398,11 +397,7 @@ impl<'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'history, 'ecx, 'mir, 'tcx> {
     }
 
     #[inline(never)] // This is only called on fatal code paths
-    pub(super) fn protector_error(&self, item: &Item, kind: ProtectorKind) -> InterpError<'tcx> {
-        let protected = match kind {
-            ProtectorKind::WeakProtector => "weakly protected",
-            ProtectorKind::StrongProtector => "strongly protected",
-        };
+    pub(super) fn protector_error(&self, item: &Item) -> InterpError<'tcx> {
         let call_id = self
             .machine
             .threads
@@ -417,15 +412,15 @@ impl<'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'history, 'ecx, 'mir, 'tcx> {
         match self.operation {
             Operation::Dealloc(_) =>
                 err_sb_ub(
-                    format!("deallocating while item {item:?} is {protected} by call {call_id:?}",),
+                    format!("deallocating while item {item:?} is protected by call {call_id:?}",),
                     None,
                     None,
                 ),
             Operation::Retag(RetagOp { orig_tag: tag, .. })
             | Operation::Access(AccessOp { tag, .. }) =>
                 err_sb_ub(
                     format!(
-                        "not granting access to tag {tag:?} because that would remove {item:?} which is {protected} because it is an argument of call {call_id:?}",
+                        "not granting access to tag {tag:?} because that would remove {item:?} which is protected because it is an argument of call {call_id:?}",
                     ),
                     None,
                     tag.and_then(|tag| self.get_logs_relevant_to(tag, Some(item.tag()))),

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

@@ -20,7 +20,7 @@ use rustc_target::abi::{Abi, Size};
 
 use crate::borrow_tracker::{
     stacked_borrows::diagnostics::{AllocHistory, DiagnosticCx, DiagnosticCxBuilder, TagHistory},
-    AccessKind, GlobalStateInner, ProtectorKind, RetagFields,
+    AccessKind, GlobalStateInner, RetagFields,
 };
 use crate::*;
 
@@ -51,12 +51,12 @@ enum NewPermission {
         /// If true, indicates that the stack should be cleared of all other tags.
         clear: bool,
         access: Option<AccessKind>,
-        protector: Option<ProtectorKind>,
+        protector: bool,
     },
     FreezeSensitive {
         freeze_perm: Permission,
         freeze_access: Option<AccessKind>,
-        freeze_protector: Option<ProtectorKind>,
+        freeze_protector: bool,
         nonfreeze_perm: Permission,
         nonfreeze_access: Option<AccessKind>,
         // nonfreeze_protector must always be None
@@ -71,25 +71,23 @@ impl NewPermission {
         kind: RetagKind,
         cx: &crate::MiriInterpCx<'_, 'tcx>,
     ) -> Self {
-        let protector = (kind == RetagKind::FnEntry).then_some(ProtectorKind::StrongProtector);
         match ty.kind() {
             ty::Ref(_, pointee, Mutability::Mut) => {
                 if kind == RetagKind::TwoPhase {
                     // We mostly just give up on 2phase-borrows, and treat these exactly like raw pointers.
-                    assert!(protector.is_none()); // RetagKind can't be both FnEntry and TwoPhase.
                     NewPermission::Uniform {
                         perm: Permission::SharedReadWrite,
                         clear: false,
                         access: None,
-                        protector: None,
+                        protector: false,
                     }
                 } else if pointee.is_unpin(*cx.tcx, cx.param_env()) {
                     // A regular full mutable reference.
                     NewPermission::Uniform {
                         perm: Permission::Unique,
                         clear: false,
                         access: Some(AccessKind::Write),
-                        protector,
+                        protector: kind == RetagKind::FnEntry,
                     }
                 } else {
                     NewPermission::Uniform {
@@ -100,25 +98,24 @@ impl NewPermission {
                         // <https://github.com/rust-lang/unsafe-code-guidelines/issues/381>, so for now
                         // we don't do that.
                         access: None,
-                        protector,
+                        protector: kind == RetagKind::FnEntry,
                     }
                 }
             }
             ty::RawPtr(ty::TypeAndMut { mutbl: Mutability::Mut, .. }) => {
-                assert!(protector.is_none()); // RetagKind can't be both FnEntry and Raw.
                 // Mutable raw pointer. No access, not protected.
                 NewPermission::Uniform {
                     perm: Permission::SharedReadWrite,
                     clear: false,
                     access: None,
-                    protector: None,
+                    protector: false,
                 }
             }
             ty::Ref(_, _pointee, Mutability::Not) => {
                 NewPermission::FreezeSensitive {
                     freeze_perm: Permission::SharedReadOnly,
                     freeze_access: Some(AccessKind::Read),
-                    freeze_protector: protector,
+                    freeze_protector: kind == RetagKind::FnEntry,
                     nonfreeze_perm: Permission::SharedReadWrite,
                     // Inside UnsafeCell, this does *not* count as an access, as there
                     // might actually be mutable references further up the stack that
@@ -129,12 +126,11 @@ impl NewPermission {
                 }
             }
             ty::RawPtr(ty::TypeAndMut { mutbl: Mutability::Not, .. }) => {
-                assert!(protector.is_none()); // RetagKind can't be both FnEntry and Raw.
                 // `*const T`, when freshly created, are read-only in the frozen part.
                 NewPermission::FreezeSensitive {
                     freeze_perm: Permission::SharedReadOnly,
                     freeze_access: Some(AccessKind::Read),
-                    freeze_protector: None,
+                    freeze_protector: false,
                     nonfreeze_perm: Permission::SharedReadWrite,
                     nonfreeze_access: None,
                 }
@@ -143,7 +139,7 @@ impl NewPermission {
         }
     }
 
-    fn protector(&self) -> Option<ProtectorKind> {
+    fn protector(&self) -> bool {
         match self {
             NewPermission::Uniform { protector, .. } => *protector,
             NewPermission::FreezeSensitive { freeze_protector, .. } => *freeze_protector,
@@ -186,13 +182,6 @@ impl Permission {
     }
 }
 
-/// Determines whether an item was invalidated by a conflicting access, or by deallocation.
-#[derive(Copy, Clone, Debug)]
-enum ItemInvalidationCause {
-    Conflict,
-    Dealloc,
-}
-
 /// Core per-location operations: access, dealloc, reborrow.
 impl<'tcx> Stack {
     /// Find the first write-incompatible item above the given one --
@@ -228,7 +217,6 @@ impl<'tcx> Stack {
         item: &Item,
         global: &GlobalStateInner,
         dcx: &mut DiagnosticCx<'_, '_, '_, 'tcx>,
-        cause: ItemInvalidationCause,
     ) -> InterpResult<'tcx> {
         if !global.tracked_pointer_tags.is_empty() {
             dcx.check_tracked_tag_popped(item, global);
@@ -251,14 +239,8 @@ impl<'tcx> Stack {
         // 2. Most frames protect only one or two tags. So this duplicative global turns a search
         //    which ends up about linear in the number of protected tags in the program into a
         //    constant time check (and a slow linear, because the tags in the frames aren't contiguous).
-        if let Some(&protector_kind) = global.protected_tags.get(&item.tag()) {
-            // The only way this is okay is if the protector is weak and we are deallocating with
-            // the right pointer.
-            let allowed = matches!(cause, ItemInvalidationCause::Dealloc)
-                && matches!(protector_kind, ProtectorKind::WeakProtector);
-            if !allowed {
-                return Err(dcx.protector_error(item, protector_kind).into());
-            }
+        if global.protected_tags.contains(&item.tag()) {
+            return Err(dcx.protector_error(item).into());
         }
         Ok(())
     }
@@ -298,7 +280,7 @@ impl<'tcx> Stack {
                 0
             };
             self.pop_items_after(first_incompatible_idx, |item| {
-                Stack::item_invalidated(&item, global, dcx, ItemInvalidationCause::Conflict)?;
+                Stack::item_invalidated(&item, global, dcx)?;
                 dcx.log_invalidation(item.tag());
                 Ok(())
             })?;
@@ -319,7 +301,7 @@ impl<'tcx> Stack {
                 0
             };
             self.disable_uniques_starting_at(first_incompatible_idx, |item| {
-                Stack::item_invalidated(&item, global, dcx, ItemInvalidationCause::Conflict)?;
+                Stack::item_invalidated(&item, global, dcx)?;
                 dcx.log_invalidation(item.tag());
                 Ok(())
             })?;
@@ -366,10 +348,10 @@ impl<'tcx> Stack {
         // As part of this we do regular protector checking, i.e. even weakly protected items cause UB when popped.
         self.access(AccessKind::Write, tag, global, dcx, exposed_tags)?;
 
-        // Step 2: Pretend we remove the remaining items, checking if any are strongly protected.
+        // Step 2: Pretend we remove the remaining items, checking if any are protected.
         for idx in (0..self.len()).rev() {
             let item = self.get(idx).unwrap();
-            Stack::item_invalidated(&item, global, dcx, ItemInvalidationCause::Dealloc)?;
+            Stack::item_invalidated(&item, global, dcx)?;
         }
 
         Ok(())
@@ -404,7 +386,7 @@ impl<'tcx> Stack {
                 // Remove all items, also checking their protectors.
                 for idx in (0..self.len()).rev() {
                     let item = self.get(idx).unwrap();
-                    Stack::item_invalidated(&item, global, dcx, ItemInvalidationCause::Conflict)?;
+                    Stack::item_invalidated(&item, global, dcx)?;
                 }
                 self.clear();
             }
@@ -669,7 +651,7 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<'
                     );
                     let mut dcx = dcx.build(&mut stacked_borrows.history, base_offset);
                     dcx.log_creation();
-                    if new_perm.protector().is_some() {
+                    if new_perm.protector() {
                         dcx.log_protector();
                     }
                 },
@@ -727,7 +709,7 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<'
             size.bytes()
         );
 
-        if let Some(protect) = new_perm.protector() {
+        if new_perm.protector() {
             // See comment in `Stack::item_invalidated` for why we store the tag twice.
             this.frame_mut().extra.borrow_tracker.as_mut().unwrap().protected_tags.push(new_tag);
             this.machine
@@ -736,7 +718,7 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<'
                 .unwrap()
                 .get_mut()
                 .protected_tags
-                .insert(new_tag, protect);
+                .insert(new_tag);
         }
 
         // Update the stacks, according to the new permission information we are given.
@@ -748,7 +730,7 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<'
                 // mutable pointer, that seems reasonable.
                 let (alloc_extra, machine) = this.get_alloc_extra_mut(alloc_id)?;
                 let stacked_borrows = alloc_extra.borrow_tracker_sb_mut().get_mut();
-                let item = Item::new(new_tag, perm, protector.is_some());
+                let item = Item::new(new_tag, perm, protector);
                 let range = alloc_range(base_offset, size);
                 let global = machine.borrow_tracker.as_ref().unwrap().borrow();
                 let dcx = DiagnosticCxBuilder::retag(
@@ -790,9 +772,9 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<'
                     let (perm, access, protector) = if frozen {
                         (freeze_perm, freeze_access, freeze_protector)
                     } else {
-                        (nonfreeze_perm, nonfreeze_access, None)
+                        (nonfreeze_perm, nonfreeze_access, /*protector*/ false)
                     };
-                    let item = Item::new(new_tag, perm, protector.is_some());
+                    let item = Item::new(new_tag, perm, protector);
                     let global = this.machine.borrow_tracker.as_ref().unwrap().borrow();
                     let dcx = DiagnosticCxBuilder::retag(
                         &this.machine,
@@ -937,14 +919,14 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                 // We cannot protect boxes since they might get invalidated if passed to an `id`
                 // function (due to the return-position `noalias`). However, doing a `clear` on
                 // `FnEntry` also has the effect that using any other pointer to access this memory
-                // is *immediate* UB.
+                // is *immediate* UB, thus ensuring `noalias` scoped for this function.
                 // FIXME: should we `clear` on *all* `Box` retags? Currently (2022-12-27) this leads
                 // to UB somewhere in thread-local dtor handling.
                 let new_perm = NewPermission::Uniform {
                     perm: Permission::Unique,
                     clear: matches!(self.kind, RetagKind::FnEntry | RetagKind::FnReturn),
                     access: Some(AccessKind::Write),
-                    protector: None,
+                    protector: false,
                 };
                 self.retag_ptr_inplace(place, new_perm, self.retag_cause)
             }
@@ -1021,7 +1003,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
             perm: Permission::Unique,
             clear: false,
             access: Some(AccessKind::Write),
-            protector: Some(ProtectorKind::StrongProtector),
+            protector: true,
         };
         let val = this.sb_retag_reference(&val, new_perm, RetagCause::FnReturnPlace)?;
         // And use reborrowed pointer for return place.

src/tools/miri/tests/fail/stacked_borrows/aliasing_mut1.stderr

@@ -1,8 +1,8 @@
-error: Undefined Behavior: not granting access to tag <TAG> because that would remove [Unique for <TAG>] which is strongly protected because it is an argument of call ID
+error: Undefined Behavior: not granting access to tag <TAG> because that would remove [Unique for <TAG>] which is protected because it is an argument of call ID
   --> $DIR/aliasing_mut1.rs:LL:CC
    |
 LL | pub fn safe(_x: &mut i32, _y: &mut i32) {}
-   |                           ^^ not granting access to tag <TAG> because that would remove [Unique for <TAG>] which is strongly protected because it is an argument of call ID
+   |                           ^^ not granting access to tag <TAG> because that would remove [Unique for <TAG>] which is protected because it is an argument of call ID
    |
    = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
    = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information

src/tools/miri/tests/fail/stacked_borrows/aliasing_mut2.stderr

@@ -1,8 +1,8 @@
-error: Undefined Behavior: not granting access to tag <TAG> because that would remove [SharedReadOnly for <TAG>] which is strongly protected because it is an argument of call ID
+error: Undefined Behavior: not granting access to tag <TAG> because that would remove [SharedReadOnly for <TAG>] which is protected because it is an argument of call ID
   --> $DIR/aliasing_mut2.rs:LL:CC
    |
 LL | pub fn safe(_x: &i32, _y: &mut i32) {}
-   |                       ^^ not granting access to tag <TAG> because that would remove [SharedReadOnly for <TAG>] which is strongly protected because it is an argument of call ID
+   |                       ^^ not granting access to tag <TAG> because that would remove [SharedReadOnly for <TAG>] which is protected because it is an argument of call ID
    |
    = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
    = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information

src/tools/miri/tests/fail/stacked_borrows/aliasing_mut4.stderr

@@ -1,8 +1,8 @@
-error: Undefined Behavior: not granting access to tag <TAG> because that would remove [SharedReadOnly for <TAG>] which is strongly protected because it is an argument of call ID
+error: Undefined Behavior: not granting access to tag <TAG> because that would remove [SharedReadOnly for <TAG>] which is protected because it is an argument of call ID
   --> $DIR/aliasing_mut4.rs:LL:CC
    |
 LL | pub fn safe(_x: &i32, _y: &mut Cell<i32>) {}
-   |                       ^^ not granting access to tag <TAG> because that would remove [SharedReadOnly for <TAG>] which is strongly protected because it is an argument of call ID
+   |                       ^^ not granting access to tag <TAG> because that would remove [SharedReadOnly for <TAG>] which is protected because it is an argument of call ID
    |
    = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
    = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information

src/tools/miri/tests/fail/stacked_borrows/deallocate_against_protector1.rs

@@ -1,4 +1,4 @@
-//@error-pattern: /deallocating while item \[Unique for .*\] is strongly protected/
+//@error-pattern: /deallocating while item \[Unique for .*\] is protected/
 use std::alloc::{dealloc, Layout};
 
 fn inner(x: &mut i32, f: fn(&mut i32)) {