Auto merge of #142170 - RalfJung:sb-raw-retag, r=<try>

Stacked Borrows: raw pointers inherit the tag from their parent pointer

The biggest design mistake of SB in my opinion was the decision to have raw pointers be distinct from the reference they are derived from. This has multiple undesirable consequences:
- Within a function, a `let mut x` and `&raw mut x` are not allowed to be used in an interleaving way, which is quite surprising and requires dedicated work-arounds e.g. in c2rust.
- Casting a `&mut T` to a `*const T` results in a read-only pointer, which regularly startles people.
- On the implementation side, this means we have to recognize all places where "safe" pointers turn into raw pointers, which turns out to be tricky for `Box`.

TB fully mitigates this by having a raw pointer inherit the tag from its parent pointer. I think we should do the same for SB. However, this requires a bit of hackery to prevent code like this from becoming UB:
```rust
fn foo(x: &mut [i32]) {
  let ptr = x.as_mut_ptr();
  let len = x.len();
  assert!(len > 0);
  ptr.write(0);
}
```
Under SB, `x.len()` causes a read of the entire slice which invalidates `ptr` since that is derived from a child of `x` (created as part of the implicit retag when calling `as_mut_ptr`).

So this experiment adds some special magic hackery to `len` to avoid `x.len()` from retagging anything. `len` is already special in invisible ways by having the function call be replaced by a MIR op; this extends that magic for the purposes of our alias tracking.

This is definitely not the final answer, but it is a minimal step that lets us make SB slightly cleaner, thus unlocking some patterns in the ecosystem that Miri has so far rejected, and some cleanup in the compiler. Longer-term plans include a new attribute one can put on functions to avoid retags -- putting that attribute on `len` *or* `as_mut_ptr` would fix the above example.

TODO:
- Check which further TB tests should be moved to "both borrows" now.
- Entirely remove `RetagKind::Raw`.
- Add mir-opt tests checking the MIR around `x.len()` looks the way we want it to.

try-job: `*gnu*aux`

Author: rust-bors[bot]

compiler/rustc_middle/src/mir/mod.rs

@@ -1108,7 +1108,7 @@ pub enum LocalInfo<'tcx> {
     /// A temporary created during evaluating `if` predicate, possibly for pattern matching for `let`s,
     /// and subject to Edition 2024 temporary lifetime rules
     IfThenRescopeTemp { if_then: HirId },
-    /// A temporary created during the pass `Derefer` to avoid it's retagging
+    /// A temporary created during the pass `Derefer` to avoid its retagging
     DerefTemp,
     /// A temporary created for borrow checking.
     FakeBorrow,

compiler/rustc_mir_transform/src/add_retag.rs

@@ -4,7 +4,6 @@
 //! of MIR building, and only after this pass we think of the program has having the
 //! normal MIR semantics.
 
-use rustc_hir::LangItem;
 use rustc_middle::mir::*;
 use rustc_middle::ty::{self, Ty, TyCtxt};
 
@@ -28,7 +27,6 @@ fn may_contain_reference<'tcx>(ty: Ty<'tcx>, depth: u32, tcx: TyCtxt<'tcx>) -> b
         // References and Boxes (`noalias` sources)
         ty::Ref(..) => true,
         ty::Adt(..) if ty.is_box() => true,
-        ty::Adt(adt, _) if tcx.is_lang_item(adt.did(), LangItem::PtrUnique) => true,
         // Compound types: recurse
         ty::Array(ty, _) | ty::Slice(ty) => {
             // This does not branch so we keep the depth the same.
@@ -153,6 +151,13 @@ impl<'tcx> crate::MirPass<'tcx> for AddRetag {
                                 }
                             }
                             Rvalue::Ref(..) => None,
+                            // Also skip coercions whose source is already a local. This is crucial
+                            // to avoid unnecessary retaggs in `my_array.len()` calls.
+                            Rvalue::Cast(
+                                CastKind::PointerCoercion(_, CoercionSource::Implicit),
+                                Operand::Copy(p) | Operand::Move(p),
+                                _,
+                            ) if p.as_local().is_some() => None,
                             _ => {
                                 if needs_retag(place) {
                                     Some(RetagKind::Default)

compiler/rustc_mir_transform/src/lib.rs

@@ -628,6 +628,7 @@ fn run_runtime_lowering_passes<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
 fn run_runtime_cleanup_passes<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
     let passes: &[&dyn MirPass<'tcx>] = &[
         &lower_intrinsics::LowerIntrinsics,
+        &lower_slice_len::LowerSliceLenCalls,
         &remove_place_mention::RemovePlaceMention,
         &simplify::SimplifyCfg::PreOptimizations,
     ];
@@ -671,9 +672,6 @@ pub(crate) fn run_optimization_passes<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'
             &check_null::CheckNull,
             // Before inlining: trim down MIR with passes to reduce inlining work.
 
-            // Has to be done before inlining, otherwise actual call will be almost always inlined.
-            // Also simple, so can just do first.
-            &lower_slice_len::LowerSliceLenCalls,
             // Perform instsimplify before inline to eliminate some trivial calls (like clone
             // shims).
             &instsimplify::InstSimplify::BeforeInline,

compiler/rustc_mir_transform/src/lower_slice_len.rs

@@ -2,14 +2,18 @@
 //! It should run before inlining!
 
 use rustc_hir::def_id::DefId;
+use rustc_index::IndexVec;
 use rustc_middle::mir::*;
-use rustc_middle::ty::TyCtxt;
+use rustc_middle::ty::adjustment::PointerCoercion;
+use rustc_middle::ty::{Ty, TyCtxt};
 
 pub(super) struct LowerSliceLenCalls;
 
 impl<'tcx> crate::MirPass<'tcx> for LowerSliceLenCalls {
     fn is_enabled(&self, sess: &rustc_session::Session) -> bool {
-        sess.mir_opt_level() > 0
+        // This pass also removes some otherwise-problematic retags, so we
+        // always enable it when retags matter.
+        sess.mir_opt_level() > 0 || sess.opts.unstable_opts.mir_emit_retag
     }
 
     fn run_pass(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
@@ -23,7 +27,7 @@ impl<'tcx> crate::MirPass<'tcx> for LowerSliceLenCalls {
         let basic_blocks = body.basic_blocks.as_mut_preserves_cfg();
         for block in basic_blocks {
             // lower `<[_]>::len` calls
-            lower_slice_len_call(block, slice_len_fn_item_def_id);
+            lower_slice_len_call(tcx, &mut body.local_decls, block, slice_len_fn_item_def_id);
         }
     }
 
@@ -32,7 +36,12 @@ impl<'tcx> crate::MirPass<'tcx> for LowerSliceLenCalls {
     }
 }
 
-fn lower_slice_len_call<'tcx>(block: &mut BasicBlockData<'tcx>, slice_len_fn_item_def_id: DefId) {
+fn lower_slice_len_call<'tcx>(
+    tcx: TyCtxt<'tcx>,
+    local_decls: &mut IndexVec<Local, LocalDecl<'tcx>>,
+    block: &mut BasicBlockData<'tcx>,
+    slice_len_fn_item_def_id: DefId,
+) {
     let terminator = block.terminator();
     if let TerminatorKind::Call {
         func,
@@ -50,11 +59,12 @@ fn lower_slice_len_call<'tcx>(block: &mut BasicBlockData<'tcx>, slice_len_fn_ite
         // perform modifications from something like:
         //     _5 = core::slice::<impl [u8]>::len(move _6) -> bb1
         // into:
-        //     _5 = PtrMetadata(move _6)
+        //     _5 = PtrMetadata(move _6);
         //     goto bb1
 
         // make new RValue for Len
-        let r_value = Rvalue::UnaryOp(UnOp::PtrMetadata, arg.node.clone());
+        let arg = arg.node.clone();
+        let r_value = Rvalue::UnaryOp(UnOp::PtrMetadata, arg.clone());
         let len_statement_kind = StatementKind::Assign(Box::new((*destination, r_value)));
         let add_statement =
             Statement { kind: len_statement_kind, source_info: terminator.source_info };
@@ -64,5 +74,75 @@ fn lower_slice_len_call<'tcx>(block: &mut BasicBlockData<'tcx>, slice_len_fn_ite
 
         block.statements.push(add_statement);
         block.terminator_mut().kind = new_terminator_kind;
+
+        if tcx.sess.opts.unstable_opts.mir_emit_retag {
+            // If we care about retags, we want there to be *no retag* for this `len` call.
+            // That means we have to clean up the MIR a little: the MIR will now often be:
+            //     _6 = &*_4;
+            //     _5 = PtrMetadata(move _6);
+            // Change that to:
+            //     _6 = &raw const *_4;
+            //     _5 = PtrMetadata(move _6);
+            let len = block.statements.len();
+            if len >= 2
+                && let &mut StatementKind::Assign(box (rebor_dest, ref mut rebor_r)) =
+                    &mut block.statements[len - 2].kind
+                && let Some(retag_local) = rebor_dest.as_local()
+                && !local_decls[retag_local].is_user_variable()
+                // The LHS of this previous assignment is the `arg` from above.
+                && matches!(arg, Operand::Copy(p) | Operand::Move(p) if p.as_local() == Some(retag_local))
+                // The RHS is a reference-taking operation.
+                && let Rvalue::Ref(_, BorrowKind::Shared, rebor_orig) = *rebor_r
+            {
+                let orig_inner_ty = rebor_orig.ty(local_decls, tcx).ty;
+                // Change the previous statement to use `&raw` instead of `&`.
+                *rebor_r = Rvalue::RawPtr(RawPtrKind::FakeForPtrMetadata, rebor_orig);
+                // Change the type of the local to match.
+                local_decls[retag_local].ty = Ty::new_ptr(tcx, orig_inner_ty, Mutability::Not);
+            }
+            // There's a second pattern we need to recognize for `array.len()` calls. There,
+            // the MIR is:
+            //     _4 = &*_3;
+            //     _6 = move _4 as &[T] (PointerCoercion(Unsize, Implicit));
+            //     StorageDead(_4);
+            //     _5 = PtrMetadata(move _6);
+            // Change that to:
+            //     _4 = &raw const *_3;
+            //     _6 = move _4 as *const [T] (PointerCoercion(Unsize, Implicit));
+            //     StorageDead(_4);
+            //     _5 = PtrMetadata(move _6);
+            if len >= 4
+                && let [reborrow_stmt, unsize_stmt, storage_dead_stmt] =
+                    block.statements.get_disjoint_mut([len - 4, len - 3, len - 2]).unwrap()
+                // Check that the 3 statements have the right shape.
+                && let &mut StatementKind::Assign(box (rebor_dest, ref mut rebor_r)) = &mut reborrow_stmt.kind
+                && let Some(retag_local) = rebor_dest.as_local()
+                && !local_decls[retag_local].is_user_variable()
+                && let &mut StatementKind::Assign(box (unsized_dest, ref mut unsize_r)) = &mut unsize_stmt.kind
+                && let Some(unsized_local) = unsized_dest.as_local()
+                && !local_decls[unsized_local].is_user_variable()
+                && storage_dead_stmt.kind == StatementKind::StorageDead(retag_local)
+                // And check that they have the right operands.
+                && matches!(arg, Operand::Copy(p) | Operand::Move(p) if p.as_local() == Some(unsized_local))
+                && let &mut Rvalue::Cast(
+                    CastKind::PointerCoercion(PointerCoercion::Unsize, CoercionSource::Implicit),
+                    ref cast_op,
+                    ref mut cast_ty,
+                ) = unsize_r
+                && let Operand::Copy(unsize_src) | Operand::Move(unsize_src) = cast_op
+                && unsize_src.as_local() == Some(retag_local)
+                && let Rvalue::Ref(_, BorrowKind::Shared, rebor_orig) = *rebor_r
+            {
+                let orig_inner_ty = rebor_orig.ty(local_decls, tcx).ty;
+                let unsize_inner_ty = cast_ty.builtin_deref(true).unwrap();
+                // Change the reborrow to use `&raw` instead of `&`.
+                *rebor_r = Rvalue::RawPtr(RawPtrKind::FakeForPtrMetadata, rebor_orig);
+                // Change the unsize coercion in the same vein.
+                *cast_ty = Ty::new_ptr(tcx, unsize_inner_ty, Mutability::Not);
+                // Change the type of the locals to match.
+                local_decls[retag_local].ty = Ty::new_ptr(tcx, orig_inner_ty, Mutability::Not);
+                local_decls[unsized_local].ty = *cast_ty;
+            }
+        }
     }
 }

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

@@ -86,15 +86,6 @@ impl NewPermission {
                     }
                 }
             }
-            ty::RawPtr(_, 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,
-                    access: None,
-                    protector: None,
-                }
-            }
             ty::Ref(_, _pointee, Mutability::Not) => {
                 // Shared references. If frozen, these get `noalias` and `dereferenceable`; otherwise neither.
                 NewPermission::FreezeSensitive {
@@ -110,17 +101,6 @@ impl NewPermission {
                     // This fixes https://github.com/rust-lang/rust/issues/55005.
                 }
             }
-            ty::RawPtr(_, 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,
-                    nonfreeze_perm: Permission::SharedReadWrite,
-                    nonfreeze_access: None,
-                }
-            }
             _ => unreachable!(),
         }
     }
@@ -196,11 +176,7 @@ impl<'tcx> Stack {
         match perm {
             Permission::SharedReadOnly => bug!("Cannot use SharedReadOnly for writing"),
             Permission::Disabled => bug!("Cannot use Disabled for anything"),
-            Permission::Unique => {
-                // On a write, everything above us is incompatible.
-                granting + 1
-            }
-            Permission::SharedReadWrite => {
+            Permission::Unique | Permission::SharedReadWrite => {
                 // The SharedReadWrite *just* above us are compatible, to skip those.
                 let mut idx = granting + 1;
                 while let Some(item) = self.get(idx) {
@@ -863,12 +839,13 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         val: &ImmTy<'tcx>,
     ) -> InterpResult<'tcx, ImmTy<'tcx>> {
         let this = self.eval_context_mut();
-        let new_perm = NewPermission::from_ref_ty(val.layout.ty, kind, this);
         let cause = match kind {
             RetagKind::TwoPhase => RetagCause::TwoPhase,
             RetagKind::FnEntry => unreachable!(),
-            RetagKind::Raw | RetagKind::Default => RetagCause::Normal,
+            RetagKind::Default => RetagCause::Normal,
+            RetagKind::Raw => return interp_ok(val.clone()), // no raw retags!
         };
+        let new_perm = NewPermission::from_ref_ty(val.layout.ty, kind, this);
         this.sb_retag_reference(val, new_perm, RetagInfo { cause, in_field: false })
     }
 
@@ -942,14 +919,16 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
 
                 // Check the type of this value to see what to do with it (retag, or recurse).
                 match place.layout.ty.kind() {
-                    ty::Ref(..) | ty::RawPtr(..) => {
-                        if matches!(place.layout.ty.kind(), ty::Ref(..))
-                            || self.kind == RetagKind::Raw
-                        {
-                            let new_perm =
-                                NewPermission::from_ref_ty(place.layout.ty, self.kind, self.ecx);
-                            self.retag_ptr_inplace(place, new_perm)?;
-                        }
+                    ty::Ref(..) => {
+                        let new_perm =
+                            NewPermission::from_ref_ty(place.layout.ty, self.kind, self.ecx);
+                        self.retag_ptr_inplace(place, new_perm)?;
+                    }
+                    ty::RawPtr(_, _) => {
+                        // We definitely do *not* want to recurse into raw pointers -- wide raw
+                        // pointers have fields, and for dyn Trait pointees those can have reference
+                        // type!
+                        // We also do not want to reborrow them.
                     }
                     ty::Adt(adt, _) if adt.is_box() => {
                         // Recurse for boxes, they require some tricky handling and will end up in `visit_box` above.

src/tools/miri/tests/fail/both_borrows/aliasing_mut3.stack.stderr

@@ -12,8 +12,8 @@ LL | pub fn safe(x: &mut i32, y: &i32) {
 help: <TAG> was created by a SharedReadOnly retag at offsets [0x0..0x4]
   --> tests/fail/both_borrows/aliasing_mut3.rs:LL:CC
    |
-LL |     safe_raw(xraw, xshr);
-   |                    ^^^^
+LL |     let xshr = &*xref;
+   |                ^^^^^^
 help: <TAG> was later invalidated at offsets [0x0..0x4] by a Unique function-entry retag inside this call
   --> tests/fail/both_borrows/aliasing_mut3.rs:LL:CC
    |

src/tools/miri/tests/fail/both_borrows/box_exclusive_violation1.stack.stderr

@@ -12,8 +12,8 @@ LL |         *LEAK = 7;
 help: <TAG> was created by a SharedReadOnly retag at offsets [0x0..0x4]
   --> tests/fail/both_borrows/box_exclusive_violation1.rs:LL:CC
    |
-LL |         LEAK = x as *const _ as *mut _;
-   |                ^
+LL | fn unknown_code_1(x: &i32) {
+   |                   ^
 help: <TAG> was later invalidated at offsets [0x0..0x4] by a write access
   --> tests/fail/both_borrows/box_exclusive_violation1.rs:LL:CC
    |

src/tools/miri/tests/fail/both_borrows/box_noalias_violation.stack.stderr

@@ -6,7 +6,7 @@ LL |     *y
    |
    = 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
-help: <TAG> was created by a SharedReadWrite retag at offsets [0x0..0x4]
+help: <TAG> was created by a Unique retag at offsets [0x0..0x4]
   --> tests/fail/both_borrows/box_noalias_violation.rs:LL:CC
    |
 LL |         let ptr = &mut v as *mut i32;

src/tools/miri/tests/fail/both_borrows/illegal_write1.stack.stderr

@@ -12,8 +12,8 @@ LL |         unsafe { *x = 42 };
 help: <TAG> was created by a SharedReadOnly retag at offsets [0x0..0x4]
   --> tests/fail/both_borrows/illegal_write1.rs:LL:CC
    |
-LL |         let x: *mut u32 = xref as *const _ as *mut _;
-   |                           ^^^^
+LL |     let xref = &*target;
+   |                ^^^^^^^^
    = note: BACKTRACE (of the first span):
    = note: inside `main` at tests/fail/both_borrows/illegal_write1.rs:LL:CC
 

src/tools/miri/tests/fail/both_borrows/illegal_write6.stack.stderr

@@ -6,11 +6,11 @@ LL |     unsafe { *y = 2 };
    |
    = 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
-help: <TAG> was created by a SharedReadWrite retag at offsets [0x0..0x4]
+help: <TAG> was created by a Unique retag at offsets [0x0..0x4]
   --> tests/fail/both_borrows/illegal_write6.rs:LL:CC
    |
-LL |     let p = x as *mut u32;
-   |             ^
+LL |     let x = &mut 0u32;
+   |             ^^^^^^^^^
 help: <TAG> is this argument
   --> tests/fail/both_borrows/illegal_write6.rs:LL:CC
    |