Auto merge of #106180 - RalfJung:dereferenceable-generators, r=nbdd0121

make &mut !Unpin not dereferenceable, and Box<!Unpin> not noalias

See https://github.com/rust-lang/unsafe-code-guidelines/issues/381 and [this LLVM discussion](https://discourse.llvm.org/t/interaction-of-noalias-and-dereferenceable/66979). The exact semantics of how `noalias` and `dereferenceable` interact are unclear, and `@comex` found a case of LLVM actually exploiting that ambiguity for optimizations. I think for now we should treat LLVM `dereferenceable` as implying a "fake read" to happen immediately at the top of the function (standing in for the spurious reads that LLVM might introduce), and that fake read is subject to all the usual `noalias` restrictions. This means we cannot put `dereferenceable` on `&mut !Unpin` references as those references can alias with other references that are being read and written inside the function (e.g. for self-referential generators), meaning the fake read introduces aliasing conflicts with those other accesses.

For `&` this is already not a problem due to https://github.com/rust-lang/rust/pull/98017 which removed the `dereferenceable` attribute for other reasons.

Regular `&mut Unpin` references are unaffected, so I hope the impact of this is going to be tiny.

The first commit does some refactoring of the `PointerKind` enum since I found the old code very confusing each time I had to touch it. It doesn't change behavior.

Fixes https://github.com/rust-lang/miri/issues/2714

EDIT: Turns out our `Box<!Unpin>` treatment was incorrect, too, so the PR also fixes that now (in codegen and Miri): we do not put `noalias` on these boxes any more.

5 files changed, 95 insertions, 76 deletions

diff --git a/src/tools/miri/src/borrow_tracker/stacked_borrows/mod.rs b/src/tools/miri/src/borrow_tracker/stacked_borrows/mod.rs
index ec555ba2895..106e93751d2 100644
--- a/src/tools/miri/src/borrow_tracker/stacked_borrows/mod.rs
+++ b/src/tools/miri/src/borrow_tracker/stacked_borrows/mod.rs
@@ -81,21 +81,18 @@ impl NewPermission {
                         protector: None,
                     }
                 } else if pointee.is_unpin(*cx.tcx, cx.param_env()) {
-                    // A regular full mutable reference.
+                    // A regular full mutable reference. On `FnEntry` this is `noalias` and `dereferenceable`.
                     NewPermission::Uniform {
                         perm: Permission::Unique,
                         access: Some(AccessKind::Write),
                         protector,
                     }
                 } else {
+                    // `!Unpin` dereferences do not get `noalias` nor `dereferenceable`.
                     NewPermission::Uniform {
                         perm: Permission::SharedReadWrite,
-                        // FIXME: We emit `dereferenceable` for `!Unpin` mutable references, so we
-                        // should do fake accesses here. But then we run into
-                        // <https://github.com/rust-lang/unsafe-code-guidelines/issues/381>, so for now
-                        // we don't do that.
                         access: None,
-                        protector,
+                        protector: None,
                     }
                 }
             }
@@ -109,6 +106,7 @@ impl NewPermission {
                 }
             }
             ty::Ref(_, _pointee, Mutability::Not) => {
+                // Shared references. If frozen, these get `noalias` and `dereferenceable`; otherwise neither.
                 NewPermission::FreezeSensitive {
                     freeze_perm: Permission::SharedReadOnly,
                     freeze_access: Some(AccessKind::Read),
@@ -137,6 +135,32 @@ impl NewPermission {
         }
     }
 
+    fn from_box_ty<'tcx>(
+        ty: Ty<'tcx>,
+        kind: RetagKind,
+        cx: &crate::MiriInterpCx<'_, 'tcx>,
+    ) -> Self {
+        // `ty` is not the `Box` but the field of the Box with this pointer (due to allocator handling).
+        let pointee = ty.builtin_deref(true).unwrap().ty;
+        if pointee.is_unpin(*cx.tcx, cx.param_env()) {
+            // A regular box. On `FnEntry` this is `noalias`, but not `dereferenceable` (hence only
+            // a weak protector).
+            NewPermission::Uniform {
+                perm: Permission::Unique,
+                access: Some(AccessKind::Write),
+                protector: (kind == RetagKind::FnEntry)
+                    .then_some(ProtectorKind::WeakProtector),
+            }
+        } else {
+            // `!Unpin` boxes do not get `noalias` nor `dereferenceable`.
+            NewPermission::Uniform {
+                perm: Permission::SharedReadWrite,
+                access: None,
+                protector: None,
+            }
+        }
+    }
+
     fn protector(&self) -> Option<ProtectorKind> {
         match self {
             NewPermission::Uniform { protector, .. } => *protector,
@@ -916,12 +940,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
 
             fn visit_box(&mut self, place: &PlaceTy<'tcx, Provenance>) -> InterpResult<'tcx> {
                 // Boxes get a weak protectors, since they may be deallocated.
-                let new_perm = NewPermission::Uniform {
-                    perm: Permission::Unique,
-                    access: Some(AccessKind::Write),
-                    protector: (self.kind == RetagKind::FnEntry)
-                        .then_some(ProtectorKind::WeakProtector),
-                };
+                let new_perm = NewPermission::from_box_ty(place.layout.ty, self.kind, self.ecx);
                 self.retag_ptr_inplace(place, new_perm, self.retag_cause)
             }
 
diff --git a/src/tools/miri/tests/fail/stacked_borrows/deallocate_against_protector2.rs b/src/tools/miri/tests/fail/stacked_borrows/deallocate_against_protector2.rs
deleted file mode 100644
index fd67dccd14d..00000000000
--- a/src/tools/miri/tests/fail/stacked_borrows/deallocate_against_protector2.rs
+++ /dev/null
@@ -1,16 +0,0 @@
-//@error-pattern: /deallocating while item \[SharedReadWrite for .*\] is strongly protected/
-use std::marker::PhantomPinned;
-
-pub struct NotUnpin(i32, PhantomPinned);
-
-fn inner(x: &mut NotUnpin, f: fn(&mut NotUnpin)) {
-    // `f` may mutate, but it may not deallocate!
-    f(x)
-}
-
-fn main() {
-    inner(Box::leak(Box::new(NotUnpin(0, PhantomPinned))), |x| {
-        let raw = x as *mut _;
-        drop(unsafe { Box::from_raw(raw) });
-    });
-}
diff --git a/src/tools/miri/tests/fail/stacked_borrows/deallocate_against_protector2.stderr b/src/tools/miri/tests/fail/stacked_borrows/deallocate_against_protector2.stderr
deleted file mode 100644
index 47cfa0de725..00000000000
--- a/src/tools/miri/tests/fail/stacked_borrows/deallocate_against_protector2.stderr
+++ /dev/null
@@ -1,38 +0,0 @@
-error: Undefined Behavior: deallocating while item [SharedReadWrite for <TAG>] is strongly protected by call ID
-  --> RUSTLIB/alloc/src/alloc.rs:LL:CC
-   |
-LL |     unsafe { __rust_dealloc(ptr, layout.size(), layout.align()) }
-   |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ deallocating while item [SharedReadWrite for <TAG>] is strongly protected by 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
-   = note: BACKTRACE:
-   = note: inside `std::alloc::dealloc` at RUSTLIB/alloc/src/alloc.rs:LL:CC
-   = note: inside `<std::alloc::Global as std::alloc::Allocator>::deallocate` at RUSTLIB/alloc/src/alloc.rs:LL:CC
-   = note: inside `alloc::alloc::box_free::<NotUnpin, std::alloc::Global>` at RUSTLIB/alloc/src/alloc.rs:LL:CC
-   = note: inside `std::ptr::drop_in_place::<std::boxed::Box<NotUnpin>> - shim(Some(std::boxed::Box<NotUnpin>))` at RUSTLIB/core/src/ptr/mod.rs:LL:CC
-   = note: inside `std::mem::drop::<std::boxed::Box<NotUnpin>>` at RUSTLIB/core/src/mem/mod.rs:LL:CC
-note: inside closure
-  --> $DIR/deallocate_against_protector2.rs:LL:CC
-   |
-LL |         drop(unsafe { Box::from_raw(raw) });
-   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-   = note: inside `<[closure@$DIR/deallocate_against_protector2.rs:LL:CC] as std::ops::FnOnce<(&mut NotUnpin,)>>::call_once - shim` at RUSTLIB/core/src/ops/function.rs:LL:CC
-note: inside `inner`
-  --> $DIR/deallocate_against_protector2.rs:LL:CC
-   |
-LL |     f(x)
-   |     ^^^^
-note: inside `main`
-  --> $DIR/deallocate_against_protector2.rs:LL:CC
-   |
-LL | /     inner(Box::leak(Box::new(NotUnpin(0, PhantomPinned))), |x| {
-LL | |         let raw = x as *mut _;
-LL | |         drop(unsafe { Box::from_raw(raw) });
-LL | |     });
-   | |______^
-
-note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
-
-error: aborting due to previous error
-
diff --git a/src/tools/miri/tests/pass/stacked-borrows/future-self-referential.rs b/src/tools/miri/tests/pass/stacked-borrows/future-self-referential.rs
index 96fc0be344d..6994def16a1 100644
--- a/src/tools/miri/tests/pass/stacked-borrows/future-self-referential.rs
+++ b/src/tools/miri/tests/pass/stacked-borrows/future-self-referential.rs
@@ -26,6 +26,19 @@ impl Future for Delay {
     }
 }
 
+fn mk_waker() -> Waker {
+    use std::sync::Arc;
+
+    struct MyWaker;
+    impl Wake for MyWaker {
+        fn wake(self: Arc<Self>) {
+            unimplemented!()
+        }
+    }
+
+    Waker::from(Arc::new(MyWaker))
+}
+
 async fn do_stuff() {
     (&mut Delay::new(1)).await;
 }
@@ -73,16 +86,7 @@ impl Future for DoStuff {
 }
 
 fn run_fut<T>(fut: impl Future<Output = T>) -> T {
-    use std::sync::Arc;
-
-    struct MyWaker;
-    impl Wake for MyWaker {
-        fn wake(self: Arc<Self>) {
-            unimplemented!()
-        }
-    }
-
-    let waker = Waker::from(Arc::new(MyWaker));
+    let waker = mk_waker();
     let mut context = Context::from_waker(&waker);
 
     let mut pinned = pin!(fut);
@@ -94,7 +98,37 @@ fn run_fut<T>(fut: impl Future<Output = T>) -> T {
     }
 }
 
+fn self_referential_box() {
+    let waker = mk_waker();
+    let cx = &mut Context::from_waker(&waker);
+
+    async fn my_fut() -> i32 {
+        let val = 10;
+        let val_ref = &val;
+
+        let _ = Delay::new(1).await;
+
+        *val_ref
+    }
+
+    fn box_poll<F: Future>(
+        mut f: Pin<Box<F>>,
+        cx: &mut Context<'_>,
+    ) -> (Pin<Box<F>>, Poll<F::Output>) {
+        let p = f.as_mut().poll(cx);
+        (f, p)
+    }
+
+    let my_fut = Box::pin(my_fut());
+    let (my_fut, p1) = box_poll(my_fut, cx);
+    assert!(p1.is_pending());
+    let (my_fut, p2) = box_poll(my_fut, cx);
+    assert!(p2.is_ready());
+    drop(my_fut);
+}
+
 fn main() {
     run_fut(do_stuff());
     run_fut(DoStuff::new());
+    self_referential_box();
 }
diff --git a/src/tools/miri/tests/pass/stacked-borrows/stacked-borrows.rs b/src/tools/miri/tests/pass/stacked-borrows/stacked-borrows.rs
index ef6eb346c17..8e78efa73c7 100644
--- a/src/tools/miri/tests/pass/stacked-borrows/stacked-borrows.rs
+++ b/src/tools/miri/tests/pass/stacked-borrows/stacked-borrows.rs
@@ -19,6 +19,7 @@ fn main() {
     array_casts();
     mut_below_shr();
     wide_raw_ptr_in_tuple();
+    not_unpin_not_protected();
 }
 
 // Make sure that reading from an `&mut` does, like reborrowing to `&`,
@@ -219,3 +220,22 @@ fn wide_raw_ptr_in_tuple() {
     // Make sure the fn ptr part of the vtable is still fine.
     r.type_id();
 }
+
+fn not_unpin_not_protected() {
+    // `&mut !Unpin`, at least for now, does not get `noalias` nor `dereferenceable`, so we also
+    // don't add protectors. (We could, but until we have a better idea for where we want to go with
+    // the self-referntial-generator situation, it does not seem worth the potential trouble.)
+    use std::marker::PhantomPinned;
+
+    pub struct NotUnpin(i32, PhantomPinned);
+
+    fn inner(x: &mut NotUnpin, f: fn(&mut NotUnpin)) {
+        // `f` may mutate, but it may not deallocate!
+        f(x)
+    }
+
+    inner(Box::leak(Box::new(NotUnpin(0, PhantomPinned))), |x| {
+        let raw = x as *mut _;
+        drop(unsafe { Box::from_raw(raw) });
+    });
+}