Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

do not mark interior mutable shared refs as dereferenceable #98017

Merged
merged 3 commits into from
Jul 23, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 12 additions & 7 deletions compiler/rustc_middle/src/ty/layout.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2618,14 +2618,14 @@ where
// 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::Shared
PointerKind::SharedMutable
} else {
match mt {
hir::Mutability::Not => {
if ty.is_freeze(tcx.at(DUMMY_SP), cx.param_env()) {
PointerKind::Frozen
} else {
PointerKind::Shared
PointerKind::SharedMutable
}
}
hir::Mutability::Mut => {
Expand All @@ -2636,7 +2636,7 @@ where
if ty.is_unpin(tcx.at(DUMMY_SP), cx.param_env()) {
PointerKind::UniqueBorrowed
} else {
PointerKind::Shared
PointerKind::UniqueBorrowedPinned
}
}
}
Expand Down Expand Up @@ -3255,10 +3255,13 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> {

// `Box` (`UniqueBorrowed`) are not necessarily dereferenceable
// for the entire duration of the function as they can be deallocated
// at any time. Set their valid size to 0.
// at any time. Same for shared mutable references. If LLVM had a
// way to say "dereferenceable on entry" we could use it here.
attrs.pointee_size = match kind {
PointerKind::UniqueOwned => Size::ZERO,
_ => pointee.size,
PointerKind::UniqueBorrowed
| PointerKind::UniqueBorrowedPinned
| PointerKind::Frozen => pointee.size,
PointerKind::SharedMutable | PointerKind::UniqueOwned => Size::ZERO,
};

// `Box`, `&T`, and `&mut T` cannot be undef.
Expand All @@ -3285,7 +3288,9 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> {
// or not to actually emit the attribute. It can also be controlled with the
// `-Zmutable-noalias` debugging option.
let no_alias = match kind {
PointerKind::Shared | PointerKind::UniqueBorrowed => false,
PointerKind::SharedMutable
| PointerKind::UniqueBorrowed
| PointerKind::UniqueBorrowedPinned => false,
PointerKind::UniqueOwned => noalias_for_box,
PointerKind::Frozen => !is_return,
};
Expand Down
12 changes: 8 additions & 4 deletions compiler/rustc_target/src/abi/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1350,15 +1350,19 @@ impl<'a, Ty> Deref for TyAndLayout<'a, Ty> {
#[derive(Copy, Clone, PartialEq, Eq, Debug)]
pub enum PointerKind {
/// Most general case, we know no restrictions to tell LLVM.
Shared,
SharedMutable,

/// `&T` where `T` contains no `UnsafeCell`, is `noalias` and `readonly`.
/// `&T` where `T` contains no `UnsafeCell`, is `dereferenceable`, `noalias` and `readonly`.
Frozen,

/// `&mut T` which is `noalias` but not `readonly`.
/// `&mut T` which is `dereferenceable` and `noalias` but not `readonly`.
UniqueBorrowed,

/// `Box<T>`, unlike `UniqueBorrowed`, it also has `noalias` on returns.
/// `&mut !Unpin`, which is `dereferenceable` but neither `noalias` nor `readonly`.
UniqueBorrowedPinned,

/// `Box<T>`, which is `noalias` (even on return types, unlike the above) but neither `readonly`
/// nor `dereferenceable`.
UniqueOwned,
}

Expand Down
27 changes: 18 additions & 9 deletions library/core/src/cell.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1766,15 +1766,24 @@ impl<T: ?Sized + fmt::Display> fmt::Display for RefMut<'_, T> {
///
/// The precise Rust aliasing rules are somewhat in flux, but the main points are not contentious:
///
/// - If you create a safe reference with lifetime `'a` (either a `&T` or `&mut T`
/// reference) that is accessible by safe code (for example, because you returned it),
/// then you must not access the data in any way that contradicts that reference for the
/// remainder of `'a`. For example, this means that if you take the `*mut T` from an
/// `UnsafeCell<T>` and cast it to an `&T`, then the data in `T` must remain immutable
/// (modulo any `UnsafeCell` data found within `T`, of course) until that reference's
/// lifetime expires. Similarly, if you create a `&mut T` reference that is released to
/// safe code, then you must not access the data within the `UnsafeCell` until that
/// reference expires.
/// - If you create a safe reference with lifetime `'a` (either a `&T` or `&mut T` reference), then
/// you must not access the data in any way that contradicts that reference for the remainder of
/// `'a`. For example, this means that if you take the `*mut T` from an `UnsafeCell<T>` and cast it
/// to an `&T`, then the data in `T` must remain immutable (modulo any `UnsafeCell` data found
/// within `T`, of course) until that reference's lifetime expires. Similarly, if you create a `&mut
/// T` reference that is released to safe code, then you must not access the data within the
/// `UnsafeCell` until that reference expires.
Copy link
Member Author

@RalfJung RalfJung Jun 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have also reworded this paragraph because the previous wording made it sound like whether the reference is "accessible by safe code" matters -- it does not; mutating the data that a live &i32 points to is always UB even if all your code is unsafe.

///
/// - For both `&T` without `UnsafeCell<_>` and `&mut T`, you must also not deallocate the data
/// until the reference expires. As a special exception, given an `&T`, any part of it that is
/// inside an `UnsafeCell<_>` may be deallocated during the lifetime of the reference, after the
/// last time the reference is used (dereferenced or reborrowed). Since you cannot deallocate a part
/// of what a reference points to, this means the memory an `&T` points to can be deallocted only if
/// *every part of it* (including padding) is inside an `UnsafeCell`.
///
/// However, whenever a `&UnsafeCell<T>` is constructed or dereferenced, it must still point to
/// live memory and the compiler is allowed to insert spurious reads if it can prove that this
/// memory has not yet been deallocated.
///
/// - At all times, you must avoid data races. If multiple threads have access to
/// the same `UnsafeCell`, then any writes must have a proper happens-before relation to all other
Expand Down
20 changes: 19 additions & 1 deletion src/test/codegen/function-arguments.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

use std::mem::MaybeUninit;
use std::num::NonZeroU64;
use std::marker::PhantomPinned;

pub struct S {
_field: [i32; 8],
Expand All @@ -14,6 +15,11 @@ pub struct UnsafeInner {
_field: std::cell::UnsafeCell<i16>,
}

pub struct NotUnpin {
_field: i32,
_marker: PhantomPinned,
}

pub enum MyBool {
True,
False,
Expand Down Expand Up @@ -91,7 +97,7 @@ pub fn static_borrow(_: &'static i32) {
pub fn named_borrow<'r>(_: &'r i32) {
}

// CHECK: @unsafe_borrow({{i16\*|ptr}} noundef align 2 dereferenceable(2) %_1)
// CHECK: @unsafe_borrow({{i16\*|ptr}} noundef nonnull align 2 %_1)
// unsafe interior means this isn't actually readonly and there may be aliases ...
#[no_mangle]
pub fn unsafe_borrow(_: &UnsafeInner) {
Expand All @@ -109,6 +115,18 @@ pub fn mutable_unsafe_borrow(_: &mut UnsafeInner) {
pub fn mutable_borrow(_: &mut i32) {
}

#[no_mangle]
// CHECK: @mutable_notunpin_borrow({{i32\*|ptr}} noundef align 4 dereferenceable(4) %_1)
// This one is *not* `noalias` because it might be self-referential.
pub fn mutable_notunpin_borrow(_: &mut NotUnpin) {
}

// CHECK: @notunpin_borrow({{i32\*|ptr}} noalias noundef readonly align 4 dereferenceable(4) %_1)
// But `&NotUnpin` behaves perfectly normal.
#[no_mangle]
pub fn notunpin_borrow(_: &NotUnpin) {
}

// CHECK: @indirect_struct({{%S\*|ptr}} noalias nocapture noundef dereferenceable(32) %_1)
#[no_mangle]
pub fn indirect_struct(_: S) {
Expand Down