From ea541bc2ee00c955e3f7eb3ddd5c3ab80146ab71 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 27 Dec 2022 12:46:08 +0100 Subject: [PATCH] make &mut !Unpin not dereferenceable See https://github.com/rust-lang/unsafe-code-guidelines/issues/381 for discussion. --- compiler/rustc_ty_utils/src/abi.rs | 15 +++++--- .../src/borrow_tracker/stacked_borrows/mod.rs | 10 ++--- .../deallocate_against_protector2.rs | 16 -------- .../deallocate_against_protector2.stderr | 38 ------------------- .../pass/stacked-borrows/stacked-borrows.rs | 20 ++++++++++ tests/codegen/function-arguments.rs | 16 +++++++- 6 files changed, 48 insertions(+), 67 deletions(-) delete mode 100644 src/tools/miri/tests/fail/stacked_borrows/deallocate_against_protector2.rs delete mode 100644 src/tools/miri/tests/fail/stacked_borrows/deallocate_against_protector2.stderr diff --git a/compiler/rustc_ty_utils/src/abi.rs b/compiler/rustc_ty_utils/src/abi.rs index 884087987ce..4ee3202292a 100644 --- a/compiler/rustc_ty_utils/src/abi.rs +++ b/compiler/rustc_ty_utils/src/abi.rs @@ -256,13 +256,16 @@ fn adjust_for_rust_scalar<'tcx>( // `Box` are not necessarily dereferenceable for the entire duration of the function as // they can be deallocated at any time. Same for non-frozen shared references (see - // ). If LLVM had a way to say - // "dereferenceable on entry" we could use it here. + // ), and for mutable references to + // potentially self-referential types (see + // ). If LLVM had a way + // to say "dereferenceable on entry" we could use it here. attrs.pointee_size = match kind { - PointerKind::Box | PointerKind::SharedRef { frozen: false } => Size::ZERO, - PointerKind::SharedRef { frozen: true } | PointerKind::MutableRef { .. } => { - pointee.size - } + PointerKind::Box + | PointerKind::SharedRef { frozen: false } + | PointerKind::MutableRef { unpin: false } => Size::ZERO, + PointerKind::SharedRef { frozen: true } + | PointerKind::MutableRef { unpin: true } => pointee.size, }; // The aliasing rules for `Box` are still not decided, but currently we emit 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..3b3a41c2f03 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 @@ fn from_ref_ty<'tcx>( 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 - // , so for now - // we don't do that. access: None, - protector, + protector: None, } } } @@ -109,6 +106,7 @@ fn from_ref_ty<'tcx>( } } 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), 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 ] 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 ] 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 `::deallocate` at RUSTLIB/alloc/src/alloc.rs:LL:CC - = note: inside `alloc::alloc::box_free::` at RUSTLIB/alloc/src/alloc.rs:LL:CC - = note: inside `std::ptr::drop_in_place::> - shim(Some(std::boxed::Box))` at RUSTLIB/core/src/ptr/mod.rs:LL:CC - = note: inside `std::mem::drop::>` 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/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) }); + }); +} diff --git a/tests/codegen/function-arguments.rs b/tests/codegen/function-arguments.rs index 1f979d7b90a..0f4639086b8 100644 --- a/tests/codegen/function-arguments.rs +++ b/tests/codegen/function-arguments.rs @@ -85,6 +85,12 @@ pub fn option_nonzero_int(x: Option) -> Option { pub fn readonly_borrow(_: &i32) { } +// CHECK: noundef align 4 dereferenceable(4) {{i32\*|ptr}} @readonly_borrow_ret() +#[no_mangle] +pub fn readonly_borrow_ret() -> &'static i32 { + loop {} +} + // CHECK: @static_borrow({{i32\*|ptr}} noalias noundef readonly align 4 dereferenceable(4) %_1) // static borrow may be captured #[no_mangle] @@ -115,9 +121,17 @@ pub fn mutable_unsafe_borrow(_: &mut UnsafeInner) { pub fn mutable_borrow(_: &mut i32) { } +// CHECK: noundef align 4 dereferenceable(4) {{i32\*|ptr}} @mutable_borrow_ret() +#[no_mangle] +pub fn mutable_borrow_ret() -> &'static mut i32 { + loop {} +} + #[no_mangle] -// CHECK: @mutable_notunpin_borrow({{i32\*|ptr}} noundef align 4 dereferenceable(4) %_1) +// CHECK: @mutable_notunpin_borrow({{i32\*|ptr}} noundef nonnull align 4 %_1) // This one is *not* `noalias` because it might be self-referential. +// It is also not `dereferenceable` due to +// . pub fn mutable_notunpin_borrow(_: &mut NotUnpin) { } -- 2.44.0