From 201ae7387245caa4591d4c8db4c35c170c64faf0 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 27 Dec 2022 12:31:17 +0100 Subject: [PATCH 1/3] make PointerKind directly reflect pointer types The code that consumes PointerKind (`adjust_for_rust_scalar` in rustc_ty_utils) ended up using PointerKind variants to talk about Rust reference types (& and &mut) anyway, making the old code structure quite confusing: one always had to keep in mind which PointerKind corresponds to which type. So this changes PointerKind to directly reflect the type. This does not change behavior. --- compiler/rustc_abi/src/lib.rs | 21 ++++-------- compiler/rustc_middle/src/ty/layout.rs | 39 +++++++---------------- compiler/rustc_ty_utils/src/abi.rs | 33 ++++++++++--------- tests/codegen/function-arguments-noopt.rs | 6 ++++ 4 files changed, 40 insertions(+), 59 deletions(-) diff --git a/compiler/rustc_abi/src/lib.rs b/compiler/rustc_abi/src/lib.rs index 5af6206c0bb80..9126c516d381e 100644 --- a/compiler/rustc_abi/src/lib.rs +++ b/compiler/rustc_abi/src/lib.rs @@ -1439,21 +1439,12 @@ impl fmt::Debug for LayoutS { #[derive(Copy, Clone, PartialEq, Eq, Debug)] pub enum PointerKind { - /// Most general case, we know no restrictions to tell LLVM. - SharedMutable, - - /// `&T` where `T` contains no `UnsafeCell`, is `dereferenceable`, `noalias` and `readonly`. - Frozen, - - /// `&mut T` which is `dereferenceable` and `noalias` but not `readonly`. - UniqueBorrowed, - - /// `&mut !Unpin`, which is `dereferenceable` but neither `noalias` nor `readonly`. - UniqueBorrowedPinned, - - /// `Box`, which is `noalias` (even on return types, unlike the above) but neither `readonly` - /// nor `dereferenceable`. - UniqueOwned, + /// Shared reference. `frozen` indicates the absence of any `UnsafeCell`. + SharedRef { frozen: bool }, + /// Mutable reference. `unpin` indicates the absence of any pinned data. + MutableRef { unpin: bool }, + /// Box. + Box, } /// Note that this information is advisory only, and backends are free to ignore it. diff --git a/compiler/rustc_middle/src/ty/layout.rs b/compiler/rustc_middle/src/ty/layout.rs index cdcd6281f209b..1244922c6d4aa 100644 --- a/compiler/rustc_middle/src/ty/layout.rs +++ b/compiler/rustc_middle/src/ty/layout.rs @@ -833,32 +833,17 @@ where }) } ty::Ref(_, ty, mt) if offset.bytes() == 0 => { - let kind = if tcx.sess.opts.optimize == OptLevel::No { - // Use conservative pointer kind if not optimizing. This saves us the - // Freeze/Unpin queries, and can save time in the codegen backend (noalias - // attributes in LLVM have compile-time cost even in unoptimized builds). - PointerKind::SharedMutable - } else { - match mt { - hir::Mutability::Not => { - if ty.is_freeze(tcx, cx.param_env()) { - PointerKind::Frozen - } else { - PointerKind::SharedMutable - } - } - hir::Mutability::Mut => { - // References to self-referential structures should not be considered - // noalias, as another pointer to the structure can be obtained, that - // is not based-on the original reference. We consider all !Unpin - // types to be potentially self-referential here. - if ty.is_unpin(tcx, cx.param_env()) { - PointerKind::UniqueBorrowed - } else { - PointerKind::UniqueBorrowedPinned - } - } - } + // Use conservative pointer kind if not optimizing. This saves us the + // Freeze/Unpin queries, and can save time in the codegen backend (noalias + // attributes in LLVM have compile-time cost even in unoptimized builds). + let optimize = tcx.sess.opts.optimize != OptLevel::No; + let kind = match mt { + hir::Mutability::Not => PointerKind::SharedRef { + frozen: optimize && ty.is_freeze(tcx, cx.param_env()), + }, + hir::Mutability::Mut => PointerKind::MutableRef { + unpin: optimize && ty.is_unpin(tcx, cx.param_env()), + }, }; tcx.layout_of(param_env.and(ty)).ok().map(|layout| PointeeInfo { @@ -929,7 +914,7 @@ where if let Some(ref mut pointee) = result { if let ty::Adt(def, _) = this.ty.kind() { if def.is_box() && offset.bytes() == 0 { - pointee.safe = Some(PointerKind::UniqueOwned); + pointee.safe = Some(PointerKind::Box); } } } diff --git a/compiler/rustc_ty_utils/src/abi.rs b/compiler/rustc_ty_utils/src/abi.rs index 1c74aeca5ab1f..884087987ce08 100644 --- a/compiler/rustc_ty_utils/src/abi.rs +++ b/compiler/rustc_ty_utils/src/abi.rs @@ -254,15 +254,15 @@ fn adjust_for_rust_scalar<'tcx>( if let Some(kind) = pointee.safe { attrs.pointee_align = Some(pointee.align); - // `Box` (`UniqueBorrowed`) are not necessarily dereferenceable - // for the entire duration of the function as they can be deallocated - // at any time. Same for shared mutable references. If LLVM had a - // way to say "dereferenceable on entry" we could use it here. + // `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. attrs.pointee_size = match kind { - PointerKind::UniqueBorrowed - | PointerKind::UniqueBorrowedPinned - | PointerKind::Frozen => pointee.size, - PointerKind::SharedMutable | PointerKind::UniqueOwned => Size::ZERO, + PointerKind::Box | PointerKind::SharedRef { frozen: false } => Size::ZERO, + PointerKind::SharedRef { frozen: true } | PointerKind::MutableRef { .. } => { + pointee.size + } }; // The aliasing rules for `Box` are still not decided, but currently we emit @@ -278,15 +278,14 @@ fn adjust_for_rust_scalar<'tcx>( // `&mut` pointer parameters never alias other parameters, // or mutable global data // - // `&T` where `T` contains no `UnsafeCell` is immutable, - // and can be marked as both `readonly` and `noalias`, as - // LLVM's definition of `noalias` is based solely on memory - // dependencies rather than pointer equality + // `&T` where `T` contains no `UnsafeCell` is immutable, and can be marked as both + // `readonly` and `noalias`, as LLVM's definition of `noalias` is based solely on memory + // dependencies rather than pointer equality. However this only applies to arguments, + // not return values. let no_alias = match kind { - PointerKind::SharedMutable | PointerKind::UniqueBorrowedPinned => false, - PointerKind::UniqueBorrowed => noalias_mut_ref, - PointerKind::UniqueOwned => noalias_for_box, - PointerKind::Frozen => true, + PointerKind::SharedRef { frozen } => frozen, + PointerKind::MutableRef { unpin } => unpin && noalias_mut_ref, + PointerKind::Box => noalias_for_box, }; // We can never add `noalias` in return position; that LLVM attribute has some very surprising semantics // (see ). @@ -294,7 +293,7 @@ fn adjust_for_rust_scalar<'tcx>( attrs.set(ArgAttribute::NoAlias); } - if kind == PointerKind::Frozen && !is_return { + if matches!(kind, PointerKind::SharedRef { frozen: true }) && !is_return { attrs.set(ArgAttribute::ReadOnly); } } diff --git a/tests/codegen/function-arguments-noopt.rs b/tests/codegen/function-arguments-noopt.rs index ff76405a4ea32..0c62e0d35e36d 100644 --- a/tests/codegen/function-arguments-noopt.rs +++ b/tests/codegen/function-arguments-noopt.rs @@ -29,6 +29,12 @@ pub fn borrow(x: &i32) -> &i32 { x } +// CHECK: align 4 {{i32\*|ptr}} @borrow_mut({{i32\*|ptr}} align 4 %x) +#[no_mangle] +pub fn borrow_mut(x: &mut i32) -> &mut i32 { + x +} + // CHECK-LABEL: @borrow_call #[no_mangle] pub fn borrow_call(x: &i32, f: fn(&i32) -> &i32) -> &i32 { From ea541bc2ee00c955e3f7eb3ddd5c3ab80146ab71 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 27 Dec 2022 12:46:08 +0100 Subject: [PATCH 2/3] 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 884087987ce08..4ee3202292afd 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 ec555ba2895c8..3b3a41c2f0373 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 - // , 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), 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 fd67dccd14df6..0000000000000 --- 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 47cfa0de7258c..0000000000000 --- 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 ef6eb346c17b1..8e78efa73c751 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 1f979d7b90a70..0f4639086b885 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) { } From 1ef16874b5ef79e193526cd23eebd30d45826360 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 2 Jan 2023 14:09:01 +0100 Subject: [PATCH 3/3] also do not add noalias on not-Unpin Box --- compiler/rustc_abi/src/lib.rs | 4 +- compiler/rustc_middle/src/ty/layout.rs | 186 +++++++++--------- compiler/rustc_ty_utils/src/abi.rs | 9 +- .../src/borrow_tracker/stacked_borrows/mod.rs | 33 +++- .../future-self-referential.rs | 54 ++++- tests/codegen/function-arguments.rs | 10 +- 6 files changed, 180 insertions(+), 116 deletions(-) diff --git a/compiler/rustc_abi/src/lib.rs b/compiler/rustc_abi/src/lib.rs index 9126c516d381e..0306cb5ce6abd 100644 --- a/compiler/rustc_abi/src/lib.rs +++ b/compiler/rustc_abi/src/lib.rs @@ -1443,8 +1443,8 @@ pub enum PointerKind { SharedRef { frozen: bool }, /// Mutable reference. `unpin` indicates the absence of any pinned data. MutableRef { unpin: bool }, - /// Box. - Box, + /// Box. `unpin` indicates the absence of any pinned data. + Box { unpin: bool }, } /// Note that this information is advisory only, and backends are free to ignore it. diff --git a/compiler/rustc_middle/src/ty/layout.rs b/compiler/rustc_middle/src/ty/layout.rs index 1244922c6d4aa..4c2855821384b 100644 --- a/compiler/rustc_middle/src/ty/layout.rs +++ b/compiler/rustc_middle/src/ty/layout.rs @@ -818,110 +818,114 @@ where let tcx = cx.tcx(); let param_env = cx.param_env(); - let pointee_info = - match *this.ty.kind() { - ty::RawPtr(mt) if offset.bytes() == 0 => { - tcx.layout_of(param_env.and(mt.ty)).ok().map(|layout| PointeeInfo { - size: layout.size, - align: layout.align.abi, - safe: None, - }) - } - ty::FnPtr(fn_sig) if offset.bytes() == 0 => { - tcx.layout_of(param_env.and(tcx.mk_fn_ptr(fn_sig))).ok().map(|layout| { - PointeeInfo { size: layout.size, align: layout.align.abi, safe: None } - }) - } - ty::Ref(_, ty, mt) if offset.bytes() == 0 => { - // Use conservative pointer kind if not optimizing. This saves us the - // Freeze/Unpin queries, and can save time in the codegen backend (noalias - // attributes in LLVM have compile-time cost even in unoptimized builds). - let optimize = tcx.sess.opts.optimize != OptLevel::No; - let kind = match mt { - hir::Mutability::Not => PointerKind::SharedRef { - frozen: optimize && ty.is_freeze(tcx, cx.param_env()), - }, - hir::Mutability::Mut => PointerKind::MutableRef { - unpin: optimize && ty.is_unpin(tcx, cx.param_env()), - }, - }; + let pointee_info = match *this.ty.kind() { + ty::RawPtr(mt) if offset.bytes() == 0 => { + tcx.layout_of(param_env.and(mt.ty)).ok().map(|layout| PointeeInfo { + size: layout.size, + align: layout.align.abi, + safe: None, + }) + } + ty::FnPtr(fn_sig) if offset.bytes() == 0 => { + tcx.layout_of(param_env.and(tcx.mk_fn_ptr(fn_sig))).ok().map(|layout| PointeeInfo { + size: layout.size, + align: layout.align.abi, + safe: None, + }) + } + ty::Ref(_, ty, mt) if offset.bytes() == 0 => { + // Use conservative pointer kind if not optimizing. This saves us the + // Freeze/Unpin queries, and can save time in the codegen backend (noalias + // attributes in LLVM have compile-time cost even in unoptimized builds). + let optimize = tcx.sess.opts.optimize != OptLevel::No; + let kind = match mt { + hir::Mutability::Not => PointerKind::SharedRef { + frozen: optimize && ty.is_freeze(tcx, cx.param_env()), + }, + hir::Mutability::Mut => PointerKind::MutableRef { + unpin: optimize && ty.is_unpin(tcx, cx.param_env()), + }, + }; - tcx.layout_of(param_env.and(ty)).ok().map(|layout| PointeeInfo { - size: layout.size, - align: layout.align.abi, - safe: Some(kind), - }) - } + tcx.layout_of(param_env.and(ty)).ok().map(|layout| PointeeInfo { + size: layout.size, + align: layout.align.abi, + safe: Some(kind), + }) + } - _ => { - let mut data_variant = match this.variants { - // Within the discriminant field, only the niche itself is - // always initialized, so we only check for a pointer at its - // offset. - // - // If the niche is a pointer, it's either valid (according - // to its type), or null (which the niche field's scalar - // validity range encodes). This allows using - // `dereferenceable_or_null` for e.g., `Option<&T>`, and - // this will continue to work as long as we don't start - // using more niches than just null (e.g., the first page of - // the address space, or unaligned pointers). - Variants::Multiple { - tag_encoding: TagEncoding::Niche { untagged_variant, .. }, - tag_field, - .. - } if this.fields.offset(tag_field) == offset => { - Some(this.for_variant(cx, untagged_variant)) - } - _ => Some(this), - }; + _ => { + let mut data_variant = match this.variants { + // Within the discriminant field, only the niche itself is + // always initialized, so we only check for a pointer at its + // offset. + // + // If the niche is a pointer, it's either valid (according + // to its type), or null (which the niche field's scalar + // validity range encodes). This allows using + // `dereferenceable_or_null` for e.g., `Option<&T>`, and + // this will continue to work as long as we don't start + // using more niches than just null (e.g., the first page of + // the address space, or unaligned pointers). + Variants::Multiple { + tag_encoding: TagEncoding::Niche { untagged_variant, .. }, + tag_field, + .. + } if this.fields.offset(tag_field) == offset => { + Some(this.for_variant(cx, untagged_variant)) + } + _ => Some(this), + }; - if let Some(variant) = data_variant { - // We're not interested in any unions. - if let FieldsShape::Union(_) = variant.fields { - data_variant = None; - } + if let Some(variant) = data_variant { + // We're not interested in any unions. + if let FieldsShape::Union(_) = variant.fields { + data_variant = None; } + } - let mut result = None; - - if let Some(variant) = data_variant { - // FIXME(erikdesjardins): handle non-default addrspace ptr sizes - // (requires passing in the expected address space from the caller) - let ptr_end = offset + Pointer(AddressSpace::DATA).size(cx); - for i in 0..variant.fields.count() { - let field_start = variant.fields.offset(i); - if field_start <= offset { - let field = variant.field(cx, i); - result = field.to_result().ok().and_then(|field| { - if ptr_end <= field_start + field.size { - // We found the right field, look inside it. - let field_info = - field.pointee_info_at(cx, offset - field_start); - field_info - } else { - None - } - }); - if result.is_some() { - break; + let mut result = None; + + if let Some(variant) = data_variant { + // FIXME(erikdesjardins): handle non-default addrspace ptr sizes + // (requires passing in the expected address space from the caller) + let ptr_end = offset + Pointer(AddressSpace::DATA).size(cx); + for i in 0..variant.fields.count() { + let field_start = variant.fields.offset(i); + if field_start <= offset { + let field = variant.field(cx, i); + result = field.to_result().ok().and_then(|field| { + if ptr_end <= field_start + field.size { + // We found the right field, look inside it. + let field_info = + field.pointee_info_at(cx, offset - field_start); + field_info + } else { + None } + }); + if result.is_some() { + break; } } } + } - // FIXME(eddyb) This should be for `ptr::Unique`, not `Box`. - if let Some(ref mut pointee) = result { - if let ty::Adt(def, _) = this.ty.kind() { - if def.is_box() && offset.bytes() == 0 { - pointee.safe = Some(PointerKind::Box); - } + // FIXME(eddyb) This should be for `ptr::Unique`, not `Box`. + if let Some(ref mut pointee) = result { + if let ty::Adt(def, _) = this.ty.kind() { + if def.is_box() && offset.bytes() == 0 { + let optimize = tcx.sess.opts.optimize != OptLevel::No; + pointee.safe = Some(PointerKind::Box { + unpin: optimize && this.ty.boxed_ty().is_unpin(tcx, cx.param_env()), + }); } } - - result } - }; + + result + } + }; debug!( "pointee_info_at (offset={:?}, type kind: {:?}) => {:?}", diff --git a/compiler/rustc_ty_utils/src/abi.rs b/compiler/rustc_ty_utils/src/abi.rs index 4ee3202292afd..ad5527f5a778b 100644 --- a/compiler/rustc_ty_utils/src/abi.rs +++ b/compiler/rustc_ty_utils/src/abi.rs @@ -261,7 +261,7 @@ fn adjust_for_rust_scalar<'tcx>( // ). If LLVM had a way // to say "dereferenceable on entry" we could use it here. attrs.pointee_size = match kind { - PointerKind::Box + PointerKind::Box { .. } | PointerKind::SharedRef { frozen: false } | PointerKind::MutableRef { unpin: false } => Size::ZERO, PointerKind::SharedRef { frozen: true } @@ -278,17 +278,16 @@ fn adjust_for_rust_scalar<'tcx>( // versions at all anymore. We still support turning it off using -Zmutable-noalias. let noalias_mut_ref = cx.tcx.sess.opts.unstable_opts.mutable_noalias; - // `&mut` pointer parameters never alias other parameters, - // or mutable global data - // // `&T` where `T` contains no `UnsafeCell` is immutable, and can be marked as both // `readonly` and `noalias`, as LLVM's definition of `noalias` is based solely on memory // dependencies rather than pointer equality. However this only applies to arguments, // not return values. + // + // `&mut T` and `Box` where `T: Unpin` are unique and hence `noalias`. let no_alias = match kind { PointerKind::SharedRef { frozen } => frozen, PointerKind::MutableRef { unpin } => unpin && noalias_mut_ref, - PointerKind::Box => noalias_for_box, + PointerKind::Box { unpin } => unpin && noalias_for_box, }; // We can never add `noalias` in return position; that LLVM attribute has some very surprising semantics // (see ). 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 3b3a41c2f0373..106e93751d219 100644 --- a/src/tools/miri/src/borrow_tracker/stacked_borrows/mod.rs +++ b/src/tools/miri/src/borrow_tracker/stacked_borrows/mod.rs @@ -135,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 { match self { NewPermission::Uniform { protector, .. } => *protector, @@ -914,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/pass/stacked-borrows/future-self-referential.rs b/src/tools/miri/tests/pass/stacked-borrows/future-self-referential.rs index 96fc0be344dbf..6994def16a1da 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) { + 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(fut: impl Future) -> T { - use std::sync::Arc; - - struct MyWaker; - impl Wake for MyWaker { - fn wake(self: Arc) { - 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(fut: impl Future) -> 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( + mut f: Pin>, + cx: &mut Context<'_>, + ) -> (Pin>, Poll) { + 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/tests/codegen/function-arguments.rs b/tests/codegen/function-arguments.rs index 0f4639086b885..96dfde18683e3 100644 --- a/tests/codegen/function-arguments.rs +++ b/tests/codegen/function-arguments.rs @@ -181,6 +181,12 @@ pub fn _box(x: Box) -> Box { x } +// CHECK: noundef nonnull align 4 {{i32\*|ptr}} @notunpin_box({{i32\*|ptr}} noundef nonnull align 4 %x) +#[no_mangle] +pub fn notunpin_box(x: Box) -> Box { + x +} + // CHECK: @struct_return({{%S\*|ptr}} noalias nocapture noundef sret(%S) dereferenceable(32){{( %0)?}}) #[no_mangle] pub fn struct_return() -> S { @@ -247,12 +253,12 @@ pub fn trait_raw(_: *const dyn Drop) { // CHECK: @trait_box({{\{\}\*|ptr}} noalias noundef nonnull align 1{{( %0)?}}, {{.+}} noalias noundef readonly align {{.*}} dereferenceable({{.*}}){{( %1)?}}) #[no_mangle] -pub fn trait_box(_: Box) { +pub fn trait_box(_: Box) { } // CHECK: { {{i8\*|ptr}}, {{i8\*|ptr}} } @trait_option({{i8\*|ptr}} noalias noundef align 1 %x.0, {{i8\*|ptr}} %x.1) #[no_mangle] -pub fn trait_option(x: Option>) -> Option> { +pub fn trait_option(x: Option>) -> Option> { x }