From 5ce1a00e812dc6d4512dcaf8d9ff41986904c515 Mon Sep 17 00:00:00 2001 From: Patrick Walton Date: Wed, 26 Oct 2022 15:12:19 -0700 Subject: [PATCH 1/5] [rustc_ty_utils] Add the LLVM `noalias` parameter attribute to `drop_in_place` in certain cases. LLVM can make use of the `noalias` parameter attribute on the parameter to `drop_in_place` in areas like argument promotion. Because the Rust compiler fully controls the code for `drop_in_place`, it can soundly deduce parameter attributes on it. In the case of a value that has a programmer-defined Drop implementation, we know that the first thing `drop_in_place` will do is pass a pointer to the object to `Drop::drop`. `Drop::drop` takes `&mut`, so it must be guaranteed that there are no pointers to the object upon entering that function. Therefore, it should be safe to mark `noalias` there. With this patch, we mark `noalias` only when the type is a value with a programmer-defined Drop implementation. This is possibly overly conservative, but I thought that proceeding cautiously was best in this instance. --- compiler/rustc_ty_utils/src/abi.rs | 36 ++++++++++++++++++++++- src/test/codegen/drop-in-place-noalias.rs | 34 +++++++++++++++++++++ 2 files changed, 69 insertions(+), 1 deletion(-) create mode 100644 src/test/codegen/drop-in-place-noalias.rs diff --git a/compiler/rustc_ty_utils/src/abi.rs b/compiler/rustc_ty_utils/src/abi.rs index 73c7eb6992f07..1f2714434170b 100644 --- a/compiler/rustc_ty_utils/src/abi.rs +++ b/compiler/rustc_ty_utils/src/abi.rs @@ -199,6 +199,7 @@ fn adjust_for_rust_scalar<'tcx>( layout: TyAndLayout<'tcx>, offset: Size, is_return: bool, + is_drop_target: bool, ) { // Booleans are always a noundef i1 that needs to be zero-extended. if scalar.is_bool() { @@ -276,6 +277,25 @@ fn adjust_for_rust_scalar<'tcx>( } } } + + // If this is the argument to `drop_in_place`, the contents of which we fully control as the + // compiler, then we may be able to mark that argument `noalias`. Currently, we're conservative + // and do so only if `drop_in_place` results in a direct call to the programmer's `drop` method. + // The `drop` method requires `&mut self`, so we're effectively just propagating the `noalias` + // guarantee from `drop` upward to `drop_in_place` in this case. + if is_drop_target { + match *layout.ty.kind() { + ty::RawPtr(inner) => { + if let ty::Adt(adt_def, _) = inner.ty.kind() { + if adt_def.destructor(cx.tcx()).is_some() { + debug!("marking drop_in_place argument as noalias"); + attrs.set(ArgAttribute::NoAlias); + } + } + } + _ => bug!("drop target isn't a raw pointer"), + } + } } // FIXME(eddyb) perhaps group the signature/type-containing (or all of them?) @@ -331,10 +351,16 @@ fn fn_abi_new_uncached<'tcx>( use SpecAbi::*; let rust_abi = matches!(sig.abi, RustIntrinsic | PlatformIntrinsic | Rust | RustCall); + let is_drop_in_place = match (cx.tcx.lang_items().drop_in_place_fn(), fn_def_id) { + (Some(drop_in_place_fn), Some(fn_def_id)) => drop_in_place_fn == fn_def_id, + _ => false, + }; + let arg_of = |ty: Ty<'tcx>, arg_idx: Option| -> 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 layout = cx.layout_of(ty)?; let layout = if force_thin_self_ptr && arg_idx == Some(0) { @@ -348,7 +374,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, + is_drop_target, + ); attrs }); diff --git a/src/test/codegen/drop-in-place-noalias.rs b/src/test/codegen/drop-in-place-noalias.rs new file mode 100644 index 0000000000000..bd9de4ad9728b --- /dev/null +++ b/src/test/codegen/drop-in-place-noalias.rs @@ -0,0 +1,34 @@ +// Tests that the compiler can mark `drop_in_place` as `noalias` when safe to do so. + +#![crate_type="lib"] + +use std::hint::black_box; + +// CHECK: define{{.*}}drop_in_place{{.*}}Foo{{.*}}({{.*}}noalias{{.*}}) + +#[repr(C)] +pub struct Foo { + a: i32, + b: i32, + c: i32, +} + +impl Drop for Foo { + #[inline(never)] + fn drop(&mut self) { + black_box(self.a); + } +} + +extern { + fn bar(); + fn baz(foo: Foo); +} + +pub fn haha() { + let foo = Foo { a: 1, b: 2, c: 3 }; + unsafe { + bar(); + baz(foo); + } +} From ecfb33238c927633311aa9624963c406ef9b241b Mon Sep 17 00:00:00 2001 From: Patrick Walton Date: Wed, 26 Oct 2022 21:49:23 -0700 Subject: [PATCH 2/5] Fix noalias box test --- src/test/codegen/noalias-box-off.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/codegen/noalias-box-off.rs b/src/test/codegen/noalias-box-off.rs index afd17c7c16074..aef3fd73994f7 100644 --- a/src/test/codegen/noalias-box-off.rs +++ b/src/test/codegen/noalias-box-off.rs @@ -3,6 +3,6 @@ #![crate_type = "lib"] // CHECK-LABEL: @box_should_not_have_noalias_if_disabled( -// CHECK-NOT: noalias +// CHECK-NOT: noalias{{.*}}% #[no_mangle] pub fn box_should_not_have_noalias_if_disabled(_b: Box) {} From 67ddb33d49b6eb3428486c96fa6b4e0640eb5676 Mon Sep 17 00:00:00 2001 From: Patrick Walton Date: Thu, 17 Nov 2022 15:24:39 -0800 Subject: [PATCH 3/5] Apply `noalias`, `nonnull`, `dereferenceable`, and `align` attributes unconditionally. We've done measurements with Miri and have determined that `noalias` shouldn't break code. The requirements that allow us to add dereferenceable and align have been long documented in the standard library documentation. --- compiler/rustc_ty_utils/src/abi.rs | 27 +++++++++-------------- src/test/codegen/drop-in-place-noalias.rs | 2 +- 2 files changed, 11 insertions(+), 18 deletions(-) diff --git a/compiler/rustc_ty_utils/src/abi.rs b/compiler/rustc_ty_utils/src/abi.rs index 1f2714434170b..6c5be946392d3 100644 --- a/compiler/rustc_ty_utils/src/abi.rs +++ b/compiler/rustc_ty_utils/src/abi.rs @@ -276,24 +276,17 @@ fn adjust_for_rust_scalar<'tcx>( attrs.set(ArgAttribute::NoAliasMutRef); } } - } - // If this is the argument to `drop_in_place`, the contents of which we fully control as the - // compiler, then we may be able to mark that argument `noalias`. Currently, we're conservative - // and do so only if `drop_in_place` results in a direct call to the programmer's `drop` method. - // The `drop` method requires `&mut self`, so we're effectively just propagating the `noalias` - // guarantee from `drop` upward to `drop_in_place` in this case. - if is_drop_target { - match *layout.ty.kind() { - ty::RawPtr(inner) => { - if let ty::Adt(adt_def, _) = inner.ty.kind() { - if adt_def.destructor(cx.tcx()).is_some() { - debug!("marking drop_in_place argument as noalias"); - attrs.set(ArgAttribute::NoAlias); - } - } - } - _ => bug!("drop target isn't a raw pointer"), + // If this is the argument to `drop_in_place`, the contents of which we fully control as the + // compiler, then we mark this argument as `noalias`, aligned, and dereferenceable. (The + // standard library documents the necessary requirements to uphold these attributes for code + // that calls this method directly.) This can enable better optimizations, such as argument + // promotion. + if is_drop_target { + attrs.set(ArgAttribute::NoAlias); + attrs.set(ArgAttribute::NonNull); + attrs.pointee_size = pointee.size; + attrs.pointee_align = Some(pointee.align); } } } diff --git a/src/test/codegen/drop-in-place-noalias.rs b/src/test/codegen/drop-in-place-noalias.rs index bd9de4ad9728b..64ac8760456cf 100644 --- a/src/test/codegen/drop-in-place-noalias.rs +++ b/src/test/codegen/drop-in-place-noalias.rs @@ -4,7 +4,7 @@ use std::hint::black_box; -// CHECK: define{{.*}}drop_in_place{{.*}}Foo{{.*}}({{.*}}noalias{{.*}}) +// CHECK: define{{.*}}core{{.*}}ptr{{.*}}drop_in_place{{.*}}Foo{{.*}}({{.*}}noalias {{.*}} align 4 dereferenceable(12){{.*}}) #[repr(C)] pub struct Foo { From 02cfabe6710020df56c306b873cfa7f111504ac6 Mon Sep 17 00:00:00 2001 From: Patrick Walton Date: Thu, 17 Nov 2022 15:40:46 -0800 Subject: [PATCH 4/5] Update documentation for `drop_in_place()` --- library/core/src/ptr/mod.rs | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-) diff --git a/library/core/src/ptr/mod.rs b/library/core/src/ptr/mod.rs index 9d4bae3eaa5f7..c126098e28176 100644 --- a/library/core/src/ptr/mod.rs +++ b/library/core/src/ptr/mod.rs @@ -436,14 +436,34 @@ mod mut_ptr; /// /// # Safety /// -/// Behavior is undefined if any of the following conditions are violated: +/// Immediately upon executing, `drop_in_place` takes out a mutable borrow on the +/// pointed-to-value. Effectively, this function is implemented like so: +/// +/// ``` +/// # struct Foo { x: i32 } +/// fn drop_in_place(to_drop: *mut Foo) { +/// let mut value = &mut *to_drop; +/// // ... drop the fields of `value` ... +/// } +/// ``` +/// +/// This implies that the behavior is undefined if any of the following +/// conditions are violated: /// /// * `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. +/// +/// * `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 +/// 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. /// -/// * The value `to_drop` points to must be valid for dropping, which may mean it must uphold -/// additional invariants - this is type-dependent. +/// * 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 = @@ -451,8 +471,6 @@ mod mut_ptr; /// 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 From 53f21aaa99a33fbf8731818bb4826b62537f2986 Mon Sep 17 00:00:00 2001 From: Patrick Walton Date: Thu, 17 Nov 2022 21:08:52 -0800 Subject: [PATCH 5/5] Add missing "unsafe" to fix doctest --- library/core/src/ptr/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/core/src/ptr/mod.rs b/library/core/src/ptr/mod.rs index c126098e28176..afa4f5ef6ce74 100644 --- a/library/core/src/ptr/mod.rs +++ b/library/core/src/ptr/mod.rs @@ -441,7 +441,7 @@ mod mut_ptr; /// /// ``` /// # struct Foo { x: i32 } -/// fn drop_in_place(to_drop: *mut Foo) { +/// unsafe fn drop_in_place(to_drop: *mut Foo) { /// let mut value = &mut *to_drop; /// // ... drop the fields of `value` ... /// }