Skip to content

Commit

Permalink
Auto merge of rust-lang#106180 - RalfJung:dereferenceable-generators,…
Browse files Browse the repository at this point in the history 
… r=nbdd0121

make &mut !Unpin not dereferenceable, and Box<!Unpin> not noalias

See rust-lang/unsafe-code-guidelines#381 and [this LLVM discussion](https://discourse.llvm.org/t/interaction-of-noalias-and-dereferenceable/66979). The exact semantics of how `noalias` and `dereferenceable` interact are unclear, and `@comex` found a case of LLVM actually exploiting that ambiguity for optimizations. I think for now we should treat LLVM `dereferenceable` as implying a "fake read" to happen immediately at the top of the function (standing in for the spurious reads that LLVM might introduce), and that fake read is subject to all the usual `noalias` restrictions. This means we cannot put `dereferenceable` on `&mut !Unpin` references as those references can alias with other references that are being read and written inside the function (e.g. for self-referential generators), meaning the fake read introduces aliasing conflicts with those other accesses.

For `&` this is already not a problem due to rust-lang#98017 which removed the `dereferenceable` attribute for other reasons.

Regular `&mut Unpin` references are unaffected, so I hope the impact of this is going to be tiny.

The first commit does some refactoring of the `PointerKind` enum since I found the old code very confusing each time I had to touch it. It doesn't change behavior.

Fixes rust-lang/miri#2714

EDIT: Turns out our `Box<!Unpin>` treatment was incorrect, too, so the PR also fixes that now (in codegen and Miri): we do not put `noalias` on these boxes any more.
  • Loading branch information
@bors
bors committed Feb 7, 2023
2 parents 35d6d70 + 1ef1687 commit dffea43
Show file tree
Hide file tree
Showing 10 changed files with 245 additions and 219 deletions.
21 changes: 6 additions & 15 deletions compiler/rustc_abi/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1439,21 +1439,12 @@ impl<V: Idx> fmt::Debug for LayoutS<V> {

#[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<T>`, 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. `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.
Expand Down
201 changes: 95 additions & 106 deletions compiler/rustc_middle/src/ty/layout.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -818,125 +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 => {
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
}
}
}
};
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<T>`, not `Box<T>`.
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);
}
// FIXME(eddyb) This should be for `ptr::Unique<T>`, not `Box<T>`.
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: {:?}) => {:?}",
Expand Down
39 changes: 20 additions & 19 deletions compiler/rustc_ty_utils/src/abi.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -254,15 +254,18 @@ 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
// <https://github.com/rust-lang/rust/pull/98017>), and for mutable references to
// potentially self-referential types (see
// <https://github.com/rust-lang/unsafe-code-guidelines/issues/381>). 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 }
| PointerKind::MutableRef { unpin: false } => Size::ZERO,
PointerKind::SharedRef { frozen: true }
| PointerKind::MutableRef { unpin: true } => pointee.size,
};

// The aliasing rules for `Box<T>` are still not decided, but currently we emit
Expand All @@ -275,26 +278,24 @@ 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<U>` 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.
//
// `&T` where `T` contains no `UnsafeCell<U>` 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
// `&mut T` and `Box<T>` where `T: Unpin` are unique and hence `noalias`.
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 { unpin } => unpin && noalias_for_box,
};
// We can never add `noalias` in return position; that LLVM attribute has some very surprising semantics
// (see <https://github.com/rust-lang/unsafe-code-guidelines/issues/385#issuecomment-1368055745>).
if no_alias && !is_return {
attrs.set(ArgAttribute::NoAlias);
}

if kind == PointerKind::Frozen && !is_return {
if matches!(kind, PointerKind::SharedRef { frozen: true }) && !is_return {
attrs.set(ArgAttribute::ReadOnly);
}
}
Expand Down
43 changes: 31 additions & 12 deletions src/tools/miri/src/borrow_tracker/stacked_borrows/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
// <https://github.com/rust-lang/unsafe-code-guidelines/issues/381>, so for now
// we don't do that.
access: None,
protector,
protector: None,
}
}
}
Expand All @@ -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),
Expand Down Expand Up @@ -137,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<ProtectorKind> {
match self {
NewPermission::Uniform { protector, .. } => *protector,
Expand Down Expand Up @@ -916,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)
}

Expand Down
Loading

0 comments on commit dffea43

Please sign in to comment.