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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 3 commits into from
Feb 7, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
201ae73
make PointerKind directly reflect pointer types
RalfJung Dec 27, 2022
ea541bc
make &mut !Unpin not dereferenceable
RalfJung Dec 27, 2022
1ef1687
also do not add noalias on not-Unpin Box
RalfJung Jan 2, 2023
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
21 changes: 6 additions & 15 deletions compiler/rustc_abi/src/lib.rs
View file
Open in desktop
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
Open in desktop
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() {
Copy link
Member Author

Choose a reason for hiding this comment

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

For some reason rustfmt decided to completely change how this match is formatted. Some recent other PR made the inverse change, and resolving those conflicts was quite painful (git is not great at handling whitespace differences during rebasing). @rust-lang/rustfmt any idea why this back-and-forth is happening? rustfmt seems to sometimes perefer

let var =
    match expr {
        pat => ...
    };

over

let val = match expr {
    pat => ...
};

even though I would think the latter is always preferable?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is where the inverse change recently happened.

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
Open in desktop
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
Open in desktop
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