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

[rustc_ty_utils] Treat drop_in_place's *mut argument like &mut when adding LLVM attributes #111807

Merged
merged 11 commits into from
May 23, 2023
Merged
35 changes: 31 additions & 4 deletions compiler/rustc_ty_utils/src/abi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,7 @@ fn adjust_for_rust_scalar<'tcx>(
layout: TyAndLayout<'tcx>,
offset: Size,
is_return: bool,
drop_target_pointee: Option<Ty<'tcx>>,
) {
// Booleans are always a noundef i1 that needs to be zero-extended.
if scalar.is_bool() {
Expand All @@ -251,14 +252,24 @@ fn adjust_for_rust_scalar<'tcx>(
}

// Only pointer types handled below.
let Scalar::Initialized { value: Pointer(_), valid_range} = scalar else { return };
let Scalar::Initialized { value: Pointer(_), valid_range } = scalar else { return };

if !valid_range.contains(0) {
// Set `nonnull` if the validity range excludes zero, or for the argument to `drop_in_place`,
// which must be nonnull per its documented safety requirements.
if !valid_range.contains(0) || drop_target_pointee.is_some() {
attrs.set(ArgAttribute::NonNull);
}

if let Some(pointee) = layout.pointee_info_at(&cx, offset) {
if let Some(kind) = pointee.safe {
let kind = if let Some(kind) = pointee.safe {
Some(kind)
} else if let Some(pointee) = drop_target_pointee {
// The argument to `drop_in_place` is semantically equivalent to a mutable reference.
Some(PointerKind::MutableRef { unpin: pointee.is_unpin(cx.tcx, cx.param_env()) })
} else {
None
};
if let Some(kind) = kind {
attrs.pointee_align = Some(pointee.align);

// `Box` are not necessarily dereferenceable for the entire duration of the function as
Expand Down Expand Up @@ -362,10 +373,18 @@ fn fn_abi_new_uncached<'tcx>(
use SpecAbi::*;
let rust_abi = matches!(sig.abi, RustIntrinsic | PlatformIntrinsic | Rust | RustCall);

let is_drop_in_place =
fn_def_id.is_some() && fn_def_id == cx.tcx.lang_items().drop_in_place_fn();

let arg_of = |ty: Ty<'tcx>, arg_idx: Option<usize>| -> Result<_, FnAbiError<'tcx>> {
let span = tracing::debug_span!("arg_of");
let _entered = span.enter();
let is_return = arg_idx.is_none();
let is_drop_target = is_drop_in_place && arg_idx == Some(0);
let drop_target_pointee = is_drop_target.then(|| match ty.kind() {
ty::RawPtr(ty::TypeAndMut { ty, .. }) => *ty,
_ => bug!("argument to drop_in_place is not a raw ptr: {:?}", ty),
});

let layout = cx.layout_of(ty)?;
let layout = if force_thin_self_ptr && arg_idx == Some(0) {
Expand All @@ -379,7 +398,15 @@ fn fn_abi_new_uncached<'tcx>(

let mut arg = ArgAbi::new(cx, layout, |layout, scalar, offset| {
let mut attrs = ArgAttributes::new();
adjust_for_rust_scalar(*cx, &mut attrs, scalar, *layout, offset, is_return);
adjust_for_rust_scalar(
*cx,
&mut attrs,
scalar,
*layout,
offset,
is_return,
drop_target_pointee,
);
attrs
});

Expand Down
16 changes: 11 additions & 5 deletions library/core/src/ptr/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -441,19 +441,25 @@ mod mut_ptr;
///
/// * `to_drop` must be [valid] for both reads and writes.
///
/// * `to_drop` must be properly aligned.
/// * `to_drop` must be properly aligned, even if `T` has size 0.
///
/// * The value `to_drop` points to must be valid for dropping, which may mean it must uphold
/// additional invariants - this is type-dependent.
/// * `to_drop` must be nonnull, even if `T` has size 0.
///
/// * The value `to_drop` points to must be valid for dropping, which may mean
Copy link
Member

Choose a reason for hiding this comment

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

This particular requirement is ill-placed in the list since it is not a consequence of the things said above the enumeration -- but the docs claim that all list items follow from that prior information.

I think it is better to just state the requirement, we don't need to document hiw they come about because from the internal implementation details.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, undid the change to the introduction of this section

/// it must uphold additional invariants. These invariants depend on the type
/// of the value being dropped. For instance, when dropping a Box, the box's
/// pointer to the heap must be valid.
///
/// * While `drop_in_place` is executing, the only way to access parts of
/// `to_drop` is through the `&mut self` references supplied to the
/// `Drop::drop` methods that `drop_in_place` invokes.
///
/// Additionally, if `T` is not [`Copy`], using the pointed-to value after
/// calling `drop_in_place` can cause undefined behavior. Note that `*to_drop =
/// foo` counts as a use because it will cause the value to be dropped
/// again. [`write()`] can be used to overwrite data without causing it to be
/// dropped.
///
/// Note that even if `T` has size `0`, the pointer must be non-null and properly aligned.
///
/// [valid]: self#safety
///
/// # Examples
Expand Down
38 changes: 38 additions & 0 deletions tests/codegen/drop-in-place-noalias.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
// compile-flags: -O -C no-prepopulate-passes

// Tests that the compiler can apply `noalias` and other &mut attributes to `drop_in_place`.
// Note that non-Unpin types should not get `noalias`, matching &mut behavior.

#![crate_type="lib"]

use std::marker::PhantomPinned;

// CHECK: define internal void @{{.*}}core{{.*}}ptr{{.*}}drop_in_place{{.*}}StructUnpin{{.*}}({{.*\*|ptr}} noalias noundef align 4 dereferenceable(12) %{{.+}})

// CHECK: define internal void @{{.*}}core{{.*}}ptr{{.*}}drop_in_place{{.*}}StructNotUnpin{{.*}}({{.*\*|ptr}} noundef nonnull align 4 %{{.+}})

pub struct StructUnpin {
a: i32,
b: i32,
c: i32,
}

impl Drop for StructUnpin {
fn drop(&mut self) {}
}

pub struct StructNotUnpin {
a: i32,
b: i32,
c: i32,
p: PhantomPinned,
}

impl Drop for StructNotUnpin {
fn drop(&mut self) {}
}

pub unsafe fn main(x: StructUnpin, y: StructNotUnpin) {
drop(x);
drop(y);
}
5 changes: 4 additions & 1 deletion tests/codegen/noalias-box-off.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,8 @@

// CHECK-LABEL: @box_should_not_have_noalias_if_disabled(
// CHECK-NOT: noalias
// CHECK-SAME: %foo)
#[no_mangle]
pub fn box_should_not_have_noalias_if_disabled(_b: Box<u8>) {}
pub fn box_should_not_have_noalias_if_disabled(foo: Box<u8>) {
drop(foo);
}