From 102040ce76d588c0605e29577cdf8307acb4bb10 Mon Sep 17 00:00:00 2001 From: Jakob Degen Date: Fri, 4 Nov 2022 00:05:15 -0700 Subject: [PATCH 1/4] Retag as FnEntry on `drop_in_place` --- compiler/rustc_mir_transform/src/shim.rs | 28 +++++++++++++++- ...place.Test.SimplifyCfg-make_shim.after.mir | 7 ++-- .../drop_in_place_protector.rs | 27 +++++++++++++++ .../drop_in_place_protector.stderr | 33 +++++++++++++++++++ .../stacked_borrows/drop_in_place_retag.rs | 19 +++++++++++ .../drop_in_place_retag.stderr | 29 ++++++++++++++++ .../fail/unaligned_pointers/drop_in_place.rs | 25 ++++++++++++++ .../unaligned_pointers/drop_in_place.stderr | 20 +++++++++++ .../miri/tests/pass/drop_in_place_null.rs | 7 ++++ 9 files changed, 192 insertions(+), 3 deletions(-) create mode 100644 src/tools/miri/tests/fail/stacked_borrows/drop_in_place_protector.rs create mode 100644 src/tools/miri/tests/fail/stacked_borrows/drop_in_place_protector.stderr create mode 100644 src/tools/miri/tests/fail/stacked_borrows/drop_in_place_retag.rs create mode 100644 src/tools/miri/tests/fail/stacked_borrows/drop_in_place_retag.stderr create mode 100644 src/tools/miri/tests/fail/unaligned_pointers/drop_in_place.rs create mode 100644 src/tools/miri/tests/fail/unaligned_pointers/drop_in_place.stderr create mode 100644 src/tools/miri/tests/pass/drop_in_place_null.rs diff --git a/compiler/rustc_mir_transform/src/shim.rs b/compiler/rustc_mir_transform/src/shim.rs index f8b55c862875e..aa89ff0038102 100644 --- a/compiler/rustc_mir_transform/src/shim.rs +++ b/compiler/rustc_mir_transform/src/shim.rs @@ -176,7 +176,33 @@ fn build_drop_shim<'tcx>(tcx: TyCtxt<'tcx>, def_id: DefId, ty: Option>) 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 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 }); + } + } 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 index 14f297e948bec..f495f147be3df 100644 --- 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 = ::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 = ::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) {::drop}, val: Value() } 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 new file mode 100644 index 0000000000000..883361d05fc4d --- /dev/null +++ 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 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 new file mode 100644 index 0000000000000..6d122ade47727 --- /dev/null +++ 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 because that would remove [Unique for ] which is 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 because that would remove [Unique for ] 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 +help: 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: is this argument + --> $DIR/drop_in_place_protector.rs:LL:CC + | +LL | core::ptr::drop_in_place(x); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ + = note: BACKTRACE: + = note: inside `::drop` at $DIR/drop_in_place_protector.rs:LL:CC + = note: inside `std::ptr::drop_in_place:: - 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` at $DIR/drop_in_place_protector.rs:LL:CC + --> $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 new file mode 100644 index 0000000000000..4cb870f1d97d0 --- /dev/null +++ b/src/tools/miri/tests/fail/stacked_borrows/drop_in_place_retag.rs @@ -0,0 +1,19 @@ +//! Test that drop_in_place mutably retags the entire place, +//! ensuring it is writeable + +//@error-pattern: /retag .* for Unique permission .* only grants SharedReadOnly permission/ + +#[repr(transparent)] +struct HasDrop; + +impl Drop for HasDrop { + fn drop(&mut self) {} +} + +fn main() { + unsafe { + let x = (0u8, HasDrop); + 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 new file mode 100644 index 0000000000000..022b27d69b2c7 --- /dev/null +++ b/src/tools/miri/tests/fail/stacked_borrows/drop_in_place_retag.stderr @@ -0,0 +1,29 @@ +error: Undefined Behavior: trying to retag from 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(to_drop: *mut T) { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | | + | trying to retag from for Unique permission at ALLOC[0x0], but that tag only grants SharedReadOnly permission for this location + | this error occurs as part of FnEntry 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: 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, HasDrop)> - shim(Some((u8, HasDrop)))` at RUSTLIB/core/src/ptr/mod.rs:LL:CC +note: inside `main` at $DIR/drop_in_place_retag.rs:LL:CC + --> $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 new file mode 100644 index 0000000000000..cf3a558bb994a --- /dev/null +++ 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::(); + let p = p.add(1); + let p = p.cast::(); + + 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 new file mode 100644 index 0000000000000..1081964987c07 --- /dev/null +++ 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(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:: - shim(Some(PartialDrop))` at RUSTLIB/core/src/ptr/mod.rs:LL:CC +note: inside `main` at $DIR/drop_in_place.rs:LL:CC + --> $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 + diff --git a/src/tools/miri/tests/pass/drop_in_place_null.rs b/src/tools/miri/tests/pass/drop_in_place_null.rs new file mode 100644 index 0000000000000..aab070b69757c --- /dev/null +++ b/src/tools/miri/tests/pass/drop_in_place_null.rs @@ -0,0 +1,7 @@ +// Make sure that dropping types with no drop glue is DB even for invalid pointers. + +fn main() { + unsafe { + core::ptr::drop_in_place::(core::ptr::null_mut()); + } +} From c359ab0b5d0dbaab1a995d7aa9fb9c6512bb837b Mon Sep 17 00:00:00 2001 From: Jakob Degen Date: Tue, 6 Dec 2022 01:18:24 -0800 Subject: [PATCH 2/4] Retag argument to `drop_in_place` unconditionally --- compiler/rustc_mir_transform/src/shim.rs | 57 ++++++++++--------- .../drop_in_place_protector.rs | 2 +- .../drop_in_place_protector.stderr | 4 +- .../stacked_borrows/drop_in_place_retag.rs | 9 +-- .../drop_in_place_retag.stderr | 2 +- .../miri/tests/pass/drop_in_place_null.rs | 7 --- 6 files changed, 34 insertions(+), 47 deletions(-) delete mode 100644 src/tools/miri/tests/pass/drop_in_place_null.rs diff --git a/compiler/rustc_mir_transform/src/shim.rs b/compiler/rustc_mir_transform/src/shim.rs index aa89ff0038102..dace540fa29d2 100644 --- a/compiler/rustc_mir_transform/src/shim.rs +++ b/compiler/rustc_mir_transform/src/shim.rs @@ -174,35 +174,36 @@ fn build_drop_shim<'tcx>(tcx: TyCtxt<'tcx>, def_id: DefId, ty: Option>) let mut body = new_body(source, blocks, local_decls_for_sig(&sig, span), sig.inputs().len(), span); - if ty.is_some() { - // 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 }); - } + // 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() { let patch = { let param_env = tcx.param_env_reveal_all_normalized(def_id); let mut elaborator = 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 index 883361d05fc4d..8cf63ee700b85 100644 --- 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 @@ -10,7 +10,7 @@ impl Drop for HasDrop { fn drop(&mut self) { unsafe { let _val = *P; - //~^ ERROR: /not granting access .* because that would remove .* which is protected/ + //~^ ERROR: /not granting access .* because that would remove .* which is strongly protected/ } } } 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 index 6d122ade47727..8b1740cd81be3 100644 --- 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 @@ -1,8 +1,8 @@ -error: Undefined Behavior: not granting access to tag because that would remove [Unique for ] which is protected because it is an argument of call ID +error: Undefined Behavior: not granting access to tag because that would remove [Unique for ] 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 because that would remove [Unique for ] which is protected because it is an argument of call ID + | ^^ not granting access to tag because that would remove [Unique for ] 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 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 index 4cb870f1d97d0..e7d256b686d1d 100644 --- 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 @@ -3,16 +3,9 @@ //@error-pattern: /retag .* for Unique permission .* only grants SharedReadOnly permission/ -#[repr(transparent)] -struct HasDrop; - -impl Drop for HasDrop { - fn drop(&mut self) {} -} - fn main() { unsafe { - let x = (0u8, HasDrop); + 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 index 022b27d69b2c7..05648e44be97e 100644 --- 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 @@ -15,7 +15,7 @@ help: was created by a SharedReadOnly retag at offsets [0x0..0x1] LL | let x = core::ptr::addr_of!(x); | ^^^^^^^^^^^^^^^^^^^^^^ = note: BACKTRACE: - = note: inside `std::ptr::drop_in_place::<(u8, HasDrop)> - shim(Some((u8, HasDrop)))` at RUSTLIB/core/src/ptr/mod.rs:LL:CC + = note: inside `std::ptr::drop_in_place:: - shim(None)` at RUSTLIB/core/src/ptr/mod.rs:LL:CC note: inside `main` at $DIR/drop_in_place_retag.rs:LL:CC --> $DIR/drop_in_place_retag.rs:LL:CC | diff --git a/src/tools/miri/tests/pass/drop_in_place_null.rs b/src/tools/miri/tests/pass/drop_in_place_null.rs deleted file mode 100644 index aab070b69757c..0000000000000 --- a/src/tools/miri/tests/pass/drop_in_place_null.rs +++ /dev/null @@ -1,7 +0,0 @@ -// Make sure that dropping types with no drop glue is DB even for invalid pointers. - -fn main() { - unsafe { - core::ptr::drop_in_place::(core::ptr::null_mut()); - } -} From 37e00165e408c0e0bca0072f282b790888254bc2 Mon Sep 17 00:00:00 2001 From: Jakob Degen Date: Fri, 9 Dec 2022 19:05:49 -0800 Subject: [PATCH 3/4] Bless tests --- .../tests/fail/stacked_borrows/drop_in_place_protector.stderr | 2 +- .../tests/fail/stacked_borrows/drop_in_place_retag.stderr | 4 ++-- .../miri/tests/fail/unaligned_pointers/drop_in_place.stderr | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) 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 index 8b1740cd81be3..bd51a6645a676 100644 --- 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 @@ -20,7 +20,7 @@ LL | core::ptr::drop_in_place(x); = note: inside `::drop` at $DIR/drop_in_place_protector.rs:LL:CC = note: inside `std::ptr::drop_in_place:: - 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` at $DIR/drop_in_place_protector.rs:LL:CC +note: inside `main` --> $DIR/drop_in_place_protector.rs:LL:CC | LL | core::ptr::drop_in_place(x); 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 index 05648e44be97e..3f9e6708bdaad 100644 --- 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 @@ -5,7 +5,7 @@ LL | pub unsafe fn drop_in_place(to_drop: *mut T) { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | | | trying to retag from for Unique permission at ALLOC[0x0], but that tag only grants SharedReadOnly permission for this location - | this error occurs as part of FnEntry retag at ALLOC[0x0..0x1] + | 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 @@ -16,7 +16,7 @@ LL | let x = core::ptr::addr_of!(x); | ^^^^^^^^^^^^^^^^^^^^^^ = note: BACKTRACE: = note: inside `std::ptr::drop_in_place:: - shim(None)` at RUSTLIB/core/src/ptr/mod.rs:LL:CC -note: inside `main` at $DIR/drop_in_place_retag.rs:LL:CC +note: inside `main` --> $DIR/drop_in_place_retag.rs:LL:CC | LL | core::ptr::drop_in_place(x.cast_mut()); 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 index 1081964987c07..ef20b43c118ff 100644 --- a/src/tools/miri/tests/fail/unaligned_pointers/drop_in_place.stderr +++ b/src/tools/miri/tests/fail/unaligned_pointers/drop_in_place.stderr @@ -8,7 +8,7 @@ LL | pub unsafe fn drop_in_place(to_drop: *mut T) { = 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:: - shim(Some(PartialDrop))` at RUSTLIB/core/src/ptr/mod.rs:LL:CC -note: inside `main` at $DIR/drop_in_place.rs:LL:CC +note: inside `main` --> $DIR/drop_in_place.rs:LL:CC | LL | core::ptr::drop_in_place(p); From 0229281d03d92eec1b167377fec1e9978af1f704 Mon Sep 17 00:00:00 2001 From: Jakob Degen Date: Wed, 21 Dec 2022 15:22:17 -0800 Subject: [PATCH 4/4] Don't run `Drop` terminators on types that do not have drop glue in const eval --- .../rustc_const_eval/src/interpret/terminator.rs | 15 ++++++++++++--- compiler/rustc_middle/src/mir/syntax.rs | 15 +++++++-------- .../fail/stacked_borrows/drop_in_place_retag.rs | 4 ++-- 3 files changed, 21 insertions(+), 13 deletions(-) diff --git a/compiler/rustc_const_eval/src/interpret/terminator.rs b/compiler/rustc_const_eval/src/interpret/terminator.rs index 0e7ffcdffc97a..550c7a44c4199 100644 --- 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 index 99e59c770d754..6e7d84410da95 100644 --- a/compiler/rustc_middle/src/mir/syntax.rs +++ b/compiler/rustc_middle/src/mir/syntax.rs @@ -562,14 +562,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/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 index e7d256b686d1d..8180e2f03a79c 100644 --- 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 @@ -1,5 +1,5 @@ -//! Test that drop_in_place mutably retags the entire place, -//! ensuring it is writeable +//! 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/