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

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

Merged
merged 3 commits into from
Feb 7, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
21 changes: 6 additions & 15 deletions compiler/rustc_abi/src/lib.rs
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.
Box,
}

/// Note that this information is advisory only, and backends are free to ignore it.
Expand Down
39 changes: 12 additions & 27 deletions compiler/rustc_middle/src/ty/layout.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()),
Copy link
Contributor

Choose a reason for hiding this comment

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

This changed the behaviour of mutable ref in no-opt builds from SharedMutable to UniqueBorrowedPinned, as opposed to what is claimed in the commit message.

Copy link
Member Author

Choose a reason for hiding this comment

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

That doesn't actually change the emitted flags though, so the commit message is still correct. Or did I miss a case?

Copy link
Contributor

Choose a reason for hiding this comment

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

It changes the emitted flags if you only look at that commit, but of course the latter commits unify the behaviour of these two so the flags ended up being the same. I pointed this out because I'm reviewing per commit.

Copy link
Member Author

Choose a reason for hiding this comment

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

It changes the emitted flags if you only look at that commit,

I don't think it does... in no-opt builds we don't emit any flags either way.

Copy link
Contributor

Choose a reason for hiding this comment

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

If only this commit is applied then we will start emitting dereferenceable for mutable refs? Or am I missing something obvious?

Copy link
Member Author

@RalfJung RalfJung Feb 6, 2023

Choose a reason for hiding this comment

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

When optimizations are disabled, we don't emit any of these attributes, no matter what happens here. That logic is... somewhere, I honestly don't know where exactly.

},
};

tcx.layout_of(param_env.and(ty)).ok().map(|layout| PointeeInfo {
Expand Down Expand Up @@ -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);
}
}
}
Expand Down
33 changes: 16 additions & 17 deletions compiler/rustc_ty_utils/src/abi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
// <https://github.com/rust-lang/rust/pull/98017>). 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<T>` are still not decided, but currently we emit
Expand All @@ -278,23 +278,22 @@ fn adjust_for_rust_scalar<'tcx>(
// `&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
// `&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.
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 <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
6 changes: 6 additions & 0 deletions tests/codegen/function-arguments-noopt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down