Auto merge of #103957 - JakobDegen:drop-retag, r=RalfJung

bors · bors · commit cca80b9a81d4 · 2022-12-22T17:48:43.000Z
Retag as FnEntry on `drop_in_place` This commit changes the mir drop shim to always retag its argument as if it were a `&mut`. cc rust-lang/unsafe-code-guidelines#373
diff --git a/compiler/rustc_const_eval/src/interpret/terminator.rs b/compiler/rustc_const_eval/src/interpret/terminator.rs
@@ -119,11 +119,20 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
             }
 
             Drop { place, target, unwind } => {
+                let frame = self.frame();
+                let ty = place.ty(&frame.body.local_decls, *self.tcx).ty;
+                let ty = self.subst_from_frame_and_normalize_erasing_regions(frame, ty)?;
+                let instance = Instance::resolve_drop_in_place(*self.tcx, ty);
+                if let ty::InstanceDef::DropGlue(_, None) = instance.def {
+                    // This is the branch we enter if and only if the dropped type has no drop glue
+                    // whatsoever. This can happen as a result of monomorphizing a drop of a
+                    // generic. In order to make sure that generic and non-generic code behaves
+                    // roughly the same (and in keeping with Mir semantics) we do nothing here.
+                    self.go_to_block(target);
+                    return Ok(());
+                }
                 let place = self.eval_place(place)?;
-                let ty = place.layout.ty;
                 trace!("TerminatorKind::drop: {:?}, type {}", place, ty);
-
-                let instance = Instance::resolve_drop_in_place(*self.tcx, ty);
                 self.drop_in_place(&place, instance, target, unwind)?;
             }
 
diff --git a/compiler/rustc_middle/src/mir/syntax.rs b/compiler/rustc_middle/src/mir/syntax.rs
@@ -564,14 +564,13 @@ pub enum TerminatorKind<'tcx> {
     Unreachable,
 
     /// The behavior of this statement differs significantly before and after drop elaboration.
-    /// After drop elaboration, `Drop` executes the drop glue for the specified place, after which
-    /// it continues execution/unwinds at the given basic blocks. It is possible that executing drop
-    /// glue is special - this would be part of Rust's memory model. (**FIXME**: due we have an
-    /// issue tracking if drop glue has any interesting semantics in addition to those of a function
-    /// call?)
-    ///
-    /// `Drop` before drop elaboration is a *conditional* execution of the drop glue. Specifically, the
-    /// `Drop` will be executed if...
+    ///
+    /// After drop elaboration: `Drop` terminators are a complete nop for types that have no drop
+    /// glue. For other types, `Drop` terminators behave exactly like a call to
+    /// `core::mem::drop_in_place` with a pointer to the given place.
+    ///
+    /// `Drop` before drop elaboration is a *conditional* execution of the drop glue. Specifically,
+    /// the `Drop` will be executed if...
     ///
     /// **Needs clarification**: End of that sentence. This in effect should document the exact
     /// behavior of drop elaboration. The following sounds vaguely right, but I'm not quite sure:
diff --git a/compiler/rustc_mir_transform/src/shim.rs b/compiler/rustc_mir_transform/src/shim.rs
@@ -174,9 +174,36 @@ fn build_drop_shim<'tcx>(tcx: TyCtxt<'tcx>, def_id: DefId, ty: Option<Ty<'tcx>>)
     let mut body =
         new_body(source, blocks, local_decls_for_sig(&sig, span), sig.inputs().len(), span);
 
+    // The first argument (index 0), but add 1 for the return value.
+    let mut dropee_ptr = Place::from(Local::new(1 + 0));
+    if tcx.sess.opts.unstable_opts.mir_emit_retag {
+        // We want to treat the function argument as if it was passed by `&mut`. As such, we
+        // generate
+        // ```
+        // temp = &mut *arg;
+        // Retag(temp, FnEntry)
+        // ```
+        // It's important that we do this first, before anything that depends on `dropee_ptr`
+        // has been put into the body.
+        let reborrow = Rvalue::Ref(
+            tcx.lifetimes.re_erased,
+            BorrowKind::Mut { allow_two_phase_borrow: false },
+            tcx.mk_place_deref(dropee_ptr),
+        );
+        let ref_ty = reborrow.ty(body.local_decls(), tcx);
+        dropee_ptr = body.local_decls.push(LocalDecl::new(ref_ty, span)).into();
+        let new_statements = [
+            StatementKind::Assign(Box::new((dropee_ptr, reborrow))),
+            StatementKind::Retag(RetagKind::FnEntry, Box::new(dropee_ptr)),
+        ];
+        for s in new_statements {
+            body.basic_blocks_mut()[START_BLOCK]
+                .statements
+                .push(Statement { source_info, kind: s });
+        }
+    }
+
     if ty.is_some() {
-        // The first argument (index 0), but add 1 for the return value.
-        let dropee_ptr = Place::from(Local::new(1 + 0));
         let patch = {
             let param_env = tcx.param_env_reveal_all_normalized(def_id);
             let mut elaborator =
diff --git a/src/test/mir-opt/retag.core.ptr-drop_in_place.Test.SimplifyCfg-make_shim.after.mir b/src/test/mir-opt/retag.core.ptr-drop_in_place.Test.SimplifyCfg-make_shim.after.mir
@@ -3,11 +3,14 @@
 fn std::ptr::drop_in_place(_1: *mut Test) -> () {
     let mut _0: ();                      // return place in scope 0 at $SRC_DIR/core/src/ptr/mod.rs:+0:1: +0:56
     let mut _2: &mut Test;               // in scope 0 at $SRC_DIR/core/src/ptr/mod.rs:+0:1: +0:56
-    let mut _3: ();                      // in scope 0 at $SRC_DIR/core/src/ptr/mod.rs:+0:1: +0:56
+    let mut _3: &mut Test;               // in scope 0 at $SRC_DIR/core/src/ptr/mod.rs:+0:1: +0:56
+    let mut _4: ();                      // in scope 0 at $SRC_DIR/core/src/ptr/mod.rs:+0:1: +0:56
 
     bb0: {
         _2 = &mut (*_1);                 // scope 0 at $SRC_DIR/core/src/ptr/mod.rs:+0:1: +0:56
-        _3 = <Test as Drop>::drop(move _2) -> bb1; // scope 0 at $SRC_DIR/core/src/ptr/mod.rs:+0:1: +0:56
+        Retag([fn entry] _2);            // scope 0 at $SRC_DIR/core/src/ptr/mod.rs:+0:1: +0:56
+        _3 = &mut (*_2);                 // scope 0 at $SRC_DIR/core/src/ptr/mod.rs:+0:1: +0:56
+        _4 = <Test as Drop>::drop(move _3) -> bb1; // scope 0 at $SRC_DIR/core/src/ptr/mod.rs:+0:1: +0:56
                                          // mir::Constant
                                          // + span: $SRC_DIR/core/src/ptr/mod.rs:LL:COL
                                          // + literal: Const { ty: for<'a> fn(&'a mut Test) {<Test as Drop>::drop}, val: Value(<ZST>) }
diff --git a/src/tools/miri/tests/fail/stacked_borrows/drop_in_place_protector.rs b/src/tools/miri/tests/fail/stacked_borrows/drop_in_place_protector.rs
@@ -0,0 +1,27 @@
+//! Test that drop_in_place retags the entire place,
+//! invalidating all aliases to it.
+
+// A zero-sized drop type -- the retagging of `fn drop` itself won't
+// do anything (since it is zero-sized); we are entirely relying on the retagging
+// in `drop_in_place` here.
+#[repr(transparent)]
+struct HasDrop;
+impl Drop for HasDrop {
+    fn drop(&mut self) {
+        unsafe {
+            let _val = *P;
+            //~^ ERROR: /not granting access .* because that would remove .* which is strongly protected/
+        }
+    }
+}
+
+static mut P: *mut u8 = core::ptr::null_mut();
+
+fn main() {
+    unsafe {
+        let mut x = (HasDrop, 0u8);
+        let x = core::ptr::addr_of_mut!(x);
+        P = x.cast();
+        core::ptr::drop_in_place(x);
+    }
+}
diff --git a/src/tools/miri/tests/fail/stacked_borrows/drop_in_place_protector.stderr b/src/tools/miri/tests/fail/stacked_borrows/drop_in_place_protector.stderr
@@ -0,0 +1,33 @@
+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
+  --> $DIR/drop_in_place_protector.rs:LL:CC
+   |
+LL |             let _val = *P;
+   |                        ^^ 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
+   |
+   = 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..0x1]
+  --> $DIR/drop_in_place_protector.rs:LL:CC
+   |
+LL |         let x = core::ptr::addr_of_mut!(x);
+   |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^
+help: <TAG> is this argument
+  --> $DIR/drop_in_place_protector.rs:LL:CC
+   |
+LL |         core::ptr::drop_in_place(x);
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   = note: BACKTRACE:
+   = note: inside `<HasDrop as std::ops::Drop>::drop` at $DIR/drop_in_place_protector.rs:LL:CC
+   = note: inside `std::ptr::drop_in_place::<HasDrop> - shim(Some(HasDrop))` at RUSTLIB/core/src/ptr/mod.rs:LL:CC
+   = note: inside `std::ptr::drop_in_place::<(HasDrop, u8)> - shim(Some((HasDrop, u8)))` at RUSTLIB/core/src/ptr/mod.rs:LL:CC
+note: inside `main`
+  --> $DIR/drop_in_place_protector.rs:LL:CC
+   |
+LL |         core::ptr::drop_in_place(x);
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   = note: this error originates in the macro `core::ptr::addr_of_mut` (in Nightly builds, run with -Z macro-backtrace for more info)
+
+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/fail/stacked_borrows/drop_in_place_retag.rs b/src/tools/miri/tests/fail/stacked_borrows/drop_in_place_retag.rs
@@ -0,0 +1,12 @@
+//! Test that drop_in_place mutably retags the entire place, even for a type that does not need
+//! dropping, ensuring among other things that it is writeable
+
+//@error-pattern: /retag .* for Unique permission .* only grants SharedReadOnly permission/
+
+fn main() {
+    unsafe {
+        let x = 0u8;
+        let x = core::ptr::addr_of!(x);
+        core::ptr::drop_in_place(x.cast_mut());
+    }
+}
diff --git a/src/tools/miri/tests/fail/stacked_borrows/drop_in_place_retag.stderr b/src/tools/miri/tests/fail/stacked_borrows/drop_in_place_retag.stderr
@@ -0,0 +1,29 @@
+error: Undefined Behavior: trying to retag from <TAG> for Unique permission at ALLOC[0x0], but that tag only grants SharedReadOnly permission for this location
+  --> RUSTLIB/core/src/ptr/mod.rs:LL:CC
+   |
+LL | pub unsafe fn drop_in_place<T: ?Sized>(to_drop: *mut T) {
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   | |
+   | trying to retag from <TAG> for Unique permission at ALLOC[0x0], but that tag only grants SharedReadOnly permission for this location
+   | this error occurs as part of retag at ALLOC[0x0..0x1]
+   |
+   = 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 SharedReadOnly retag at offsets [0x0..0x1]
+  --> $DIR/drop_in_place_retag.rs:LL:CC
+   |
+LL |         let x = core::ptr::addr_of!(x);
+   |                 ^^^^^^^^^^^^^^^^^^^^^^
+   = note: BACKTRACE:
+   = note: inside `std::ptr::drop_in_place::<u8> - shim(None)` at RUSTLIB/core/src/ptr/mod.rs:LL:CC
+note: inside `main`
+  --> $DIR/drop_in_place_retag.rs:LL:CC
+   |
+LL |         core::ptr::drop_in_place(x.cast_mut());
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   = note: this error originates in the macro `core::ptr::addr_of` (in Nightly builds, run with -Z macro-backtrace for more info)
+
+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/fail/unaligned_pointers/drop_in_place.rs b/src/tools/miri/tests/fail/unaligned_pointers/drop_in_place.rs
@@ -0,0 +1,25 @@
+#[repr(transparent)]
+struct HasDrop(u8);
+
+impl Drop for HasDrop {
+    fn drop(&mut self) {}
+}
+
+#[repr(C, align(2))]
+struct PartialDrop {
+    a: HasDrop,
+    b: u8,
+}
+
+//@error-pattern: /alignment 2 is required/
+fn main() {
+    unsafe {
+        // Create an unaligned pointer
+        let mut x = [0_u16; 2];
+        let p = core::ptr::addr_of_mut!(x).cast::<u8>();
+        let p = p.add(1);
+        let p = p.cast::<PartialDrop>();
+
+        core::ptr::drop_in_place(p);
+    }
+}
diff --git a/src/tools/miri/tests/fail/unaligned_pointers/drop_in_place.stderr b/src/tools/miri/tests/fail/unaligned_pointers/drop_in_place.stderr
@@ -0,0 +1,20 @@
+error: Undefined Behavior: accessing memory with alignment ALIGN, but alignment ALIGN is required
+  --> RUSTLIB/core/src/ptr/mod.rs:LL:CC
+   |
+LL | pub unsafe fn drop_in_place<T: ?Sized>(to_drop: *mut T) {
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ accessing memory with alignment ALIGN, but alignment ALIGN is required
+   |
+   = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
+   = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
+   = note: BACKTRACE:
+   = note: inside `std::ptr::drop_in_place::<PartialDrop> - shim(Some(PartialDrop))` at RUSTLIB/core/src/ptr/mod.rs:LL:CC
+note: inside `main`
+  --> $DIR/drop_in_place.rs:LL:CC
+   |
+LL |         core::ptr::drop_in_place(p);
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
+
+error: aborting due to previous error
+
