From b2c717fa338dd3e917008484c4bf55886041f743 Mon Sep 17 00:00:00 2001 From: Scott McMurray Date: Sat, 11 Mar 2023 15:32:54 -0800 Subject: [PATCH 1/6] `MaybeUninit::assume_init_read` should have `noundef` load metadata I was looking into `array::IntoIter` optimization, and noticed that it wasn't annotating the loads with `noundef` for simple things like `array::IntoIter`. Turned out to be a more general problem as `MaybeUninit::assume_init_read` isn't marking the load as initialized (), which is unfortunate since that's basically its reason to exist. This PR lowers `ptr::read(p)` to `copy *p` in MIR, which fortuitiously also improves the IR we give to LLVM for things like `mem::replace`. --- .../rustc_hir_analysis/src/check/intrinsic.rs | 2 + compiler/rustc_lint_defs/src/builtin.rs | 7 +-- .../src/lower_intrinsics.rs | 24 +++++++++ compiler/rustc_span/src/symbol.rs | 1 + library/core/src/intrinsics.rs | 14 +++++ library/core/src/ptr/mod.rs | 20 +++++--- tests/codegen/mem-replace-direct-memcpy.rs | 41 +++++++++++++-- tests/codegen/read-noundef-metadata.rs | 51 +++++++++++++++++++ ...ad_via_copy_primitive.LowerIntrinsics.diff | 27 ++++++++++ ..._via_copy_uninhabited.LowerIntrinsics.diff | 22 ++++++++ tests/mir-opt/lower_intrinsics.rs | 12 +++++ tests/ui/const-ptr/out_of_bounds_read.stderr | 6 +-- tests/ui/consts/const-eval/ub-ref-ptr.stderr | 24 ++------- tests/ui/consts/issue-miri-1910.stderr | 2 +- 14 files changed, 214 insertions(+), 39 deletions(-) create mode 100644 tests/codegen/read-noundef-metadata.rs create mode 100644 tests/mir-opt/lower_intrinsics.read_via_copy_primitive.LowerIntrinsics.diff create mode 100644 tests/mir-opt/lower_intrinsics.read_via_copy_uninhabited.LowerIntrinsics.diff diff --git a/compiler/rustc_hir_analysis/src/check/intrinsic.rs b/compiler/rustc_hir_analysis/src/check/intrinsic.rs index 20b6561f8b256..8c364a4f3b2b8 100644 --- a/compiler/rustc_hir_analysis/src/check/intrinsic.rs +++ b/compiler/rustc_hir_analysis/src/check/intrinsic.rs @@ -363,6 +363,8 @@ pub fn check_intrinsic_type(tcx: TyCtxt<'_>, it: &hir::ForeignItem<'_>) { sym::likely => (0, vec![tcx.types.bool], tcx.types.bool), sym::unlikely => (0, vec![tcx.types.bool], tcx.types.bool), + sym::read_via_copy => (1, vec![tcx.mk_imm_ptr(param(0))], param(0)), + sym::discriminant_value => { let assoc_items = tcx.associated_item_def_ids( tcx.require_lang_item(hir::LangItem::DiscriminantKind, None), diff --git a/compiler/rustc_lint_defs/src/builtin.rs b/compiler/rustc_lint_defs/src/builtin.rs index 46ec1a2dca1f7..91966e75b5fa2 100644 --- a/compiler/rustc_lint_defs/src/builtin.rs +++ b/compiler/rustc_lint_defs/src/builtin.rs @@ -1026,12 +1026,13 @@ declare_lint! { /// ### Example /// /// ```rust,compile_fail - /// #![feature(const_ptr_read)] + /// #![feature(const_mut_refs)] /// const FOO: () = unsafe { /// let x = &[0_u8; 4]; /// let y = x.as_ptr().cast::(); - /// y.read(); // the address of a `u8` array is unknown and thus we don't know if - /// // it is aligned enough for reading a `u32`. + /// let mut z = 123; + /// y.copy_to_nonoverlapping(&mut z, 1); // the address of a `u8` array is unknown + /// // and thus we don't know if it is aligned enough for copying a `u32`. /// }; /// ``` /// diff --git a/compiler/rustc_mir_transform/src/lower_intrinsics.rs b/compiler/rustc_mir_transform/src/lower_intrinsics.rs index f596cc1808fa2..151fff27d14ad 100644 --- a/compiler/rustc_mir_transform/src/lower_intrinsics.rs +++ b/compiler/rustc_mir_transform/src/lower_intrinsics.rs @@ -149,6 +149,30 @@ impl<'tcx> MirPass<'tcx> for LowerIntrinsics { terminator.kind = TerminatorKind::Goto { target }; } } + sym::read_via_copy => { + let Ok([arg]) = <[_; 1]>::try_from(std::mem::take(args)) else { + span_bug!(terminator.source_info.span, "Wrong number of arguments"); + }; + let derefed_place = + if let Some(place) = arg.place() && let Some(local) = place.as_local() { + tcx.mk_place_deref(local.into()) + } else { + span_bug!(terminator.source_info.span, "Only passing a local is supported"); + }; + block.statements.push(Statement { + source_info: terminator.source_info, + kind: StatementKind::Assign(Box::new(( + *destination, + Rvalue::Use(Operand::Copy(derefed_place)), + ))), + }); + if let Some(target) = *target { + terminator.kind = TerminatorKind::Goto { target }; + } else { + // Reading something uninhabited means this is unreachable. + terminator.kind = TerminatorKind::Unreachable; + } + } sym::discriminant_value => { if let (Some(target), Some(arg)) = (*target, args[0].place()) { let arg = tcx.mk_place_deref(arg); diff --git a/compiler/rustc_span/src/symbol.rs b/compiler/rustc_span/src/symbol.rs index 4e626fd9f30c0..45f79c9d80592 100644 --- a/compiler/rustc_span/src/symbol.rs +++ b/compiler/rustc_span/src/symbol.rs @@ -1153,6 +1153,7 @@ symbols! { read_enum_variant_arg, read_struct, read_struct_field, + read_via_copy, readonly, realloc, reason, diff --git a/library/core/src/intrinsics.rs b/library/core/src/intrinsics.rs index c6321587adc62..d77c2a00eb780 100644 --- a/library/core/src/intrinsics.rs +++ b/library/core/src/intrinsics.rs @@ -2020,6 +2020,20 @@ extern "rust-intrinsic" { #[rustc_safe_intrinsic] pub fn saturating_sub(a: T, b: T) -> T; + /// This is a *typed* read, `copy *p` in MIR. + /// + /// The stabilized form of this intrinsic is [`crate::ptr::read`], so + /// that can be implemented without needing to do an *untyped* copy + /// via [`copy_nonoverlapping`], and thus can get proper metadata. + /// + /// This intrinsic can *only* be called with a copy or move of a local. + /// (It allows neither constants nor projections.) + /// + /// To avoid introducing any `noalias` requirements, it just takes a pointer. + #[cfg(not(bootstrap))] + #[rustc_const_unstable(feature = "const_ptr_read", issue = "80377")] + pub fn read_via_copy(p: *const T) -> T; + /// Returns the value of the discriminant for the variant in 'v'; /// if `T` has no discriminant, returns `0`. /// diff --git a/library/core/src/ptr/mod.rs b/library/core/src/ptr/mod.rs index f41be46abc96f..04b67a56db59b 100644 --- a/library/core/src/ptr/mod.rs +++ b/library/core/src/ptr/mod.rs @@ -1137,25 +1137,33 @@ pub const unsafe fn replace(dst: *mut T, mut src: T) -> T { pub const unsafe fn read(src: *const T) -> T { // We are calling the intrinsics directly to avoid function calls in the generated code // as `intrinsics::copy_nonoverlapping` is a wrapper function. + #[cfg(bootstrap)] extern "rust-intrinsic" { #[rustc_const_stable(feature = "const_intrinsic_copy", since = "1.63.0")] fn copy_nonoverlapping(src: *const T, dst: *mut T, count: usize); } - let mut tmp = MaybeUninit::::uninit(); // SAFETY: the caller must guarantee that `src` is valid for reads. // `src` cannot overlap `tmp` because `tmp` was just allocated on // the stack as a separate allocated object. - // - // Also, since we just wrote a valid value into `tmp`, it is guaranteed - // to be properly initialized. unsafe { assert_unsafe_precondition!( "ptr::read requires that the pointer argument is aligned and non-null", [T](src: *const T) => is_aligned_and_not_null(src) ); - copy_nonoverlapping(src, tmp.as_mut_ptr(), 1); - tmp.assume_init() + + #[cfg(bootstrap)] + { + let mut tmp = MaybeUninit::::uninit(); + copy_nonoverlapping(src, tmp.as_mut_ptr(), 1); + tmp.assume_init() + } + #[cfg(not(bootstrap))] + { + // This uses a dedicated intrinsic, not `copy_nonoverlapping`, + // so that it gets a *typed* copy, not an *untyped* one. + crate::intrinsics::read_via_copy(src) + } } } diff --git a/tests/codegen/mem-replace-direct-memcpy.rs b/tests/codegen/mem-replace-direct-memcpy.rs index e8bbf0e1bbd61..3b01a621b5680 100644 --- a/tests/codegen/mem-replace-direct-memcpy.rs +++ b/tests/codegen/mem-replace-direct-memcpy.rs @@ -12,13 +12,44 @@ pub fn replace_byte(dst: &mut u8, src: u8) -> u8 { std::mem::replace(dst, src) } +#[repr(C, align(8))] +pub struct Big([u64; 7]); +pub fn replace_big(dst: &mut Big, src: Big) -> Big { + // Before the `read_via_copy` intrinsic, this emitted six `memcpy`s. + std::mem::replace(dst, src) +} + // NOTE(eddyb) the `CHECK-NOT`s ensure that the only calls of `@llvm.memcpy` in -// the entire output, are the two direct calls we want, from `ptr::replace`. +// the entire output, are the direct calls we want, from `ptr::replace`. // CHECK-NOT: call void @llvm.memcpy -// CHECK: ; core::mem::replace -// CHECK-NOT: call void @llvm.memcpy -// CHECK: call void @llvm.memcpy.{{.+}}({{i8\*|ptr}} align 1 %{{.*}}, {{i8\*|ptr}} align 1 %{{.*}}, i{{.*}} 1, i1 false) + +// For a large type, we expect exactly three `memcpy`s +// CHECK-LABEL: define internal void @{{.+}}mem{{.+}}replace{{.+}}sret(%Big) + // CHECK-NOT: alloca + // CHECK: alloca %Big + // CHECK-NOT: alloca + // CHECK-NOT: call void @llvm.memcpy + // CHECK: call void @llvm.memcpy.{{.+}}({{i8\*|ptr}} align 8 %{{.*}}, {{i8\*|ptr}} align 8 %{{.*}}, i{{.*}} 56, i1 false) + // CHECK-NOT: call void @llvm.memcpy + // CHECK: call void @llvm.memcpy.{{.+}}({{i8\*|ptr}} align 8 %{{.*}}, {{i8\*|ptr}} align 8 %{{.*}}, i{{.*}} 56, i1 false) + // CHECK-NOT: call void @llvm.memcpy + // CHECK: call void @llvm.memcpy.{{.+}}({{i8\*|ptr}} align 8 %{{.*}}, {{i8\*|ptr}} align 8 %{{.*}}, i{{.*}} 56, i1 false) + // CHECK-NOT: call void @llvm.memcpy + // CHECK-NOT: call void @llvm.memcpy -// CHECK: call void @llvm.memcpy.{{.+}}({{i8\*|ptr}} align 1 %{{.*}}, {{i8\*|ptr}} align 1 %{{.*}}, i{{.*}} 1, i1 false) + +// For a small type, we expect one each of `load`/`store`/`memcpy` instead +// CHECK-LABEL: define internal noundef i8 @{{.+}}mem{{.+}}replace + // CHECK-NOT: alloca + // CHECK: alloca i8 + // CHECK-NOT: alloca + // CHECK-NOT: call void @llvm.memcpy + // CHECK: load i8 + // CHECK-NOT: call void @llvm.memcpy + // CHECK: store i8 + // CHECK-NOT: call void @llvm.memcpy + // CHECK: call void @llvm.memcpy.{{.+}}({{i8\*|ptr}} align 1 %{{.*}}, {{i8\*|ptr}} align 1 %{{.*}}, i{{.*}} 1, i1 false) + // CHECK-NOT: call void @llvm.memcpy + // CHECK-NOT: call void @llvm.memcpy diff --git a/tests/codegen/read-noundef-metadata.rs b/tests/codegen/read-noundef-metadata.rs new file mode 100644 index 0000000000000..03386921c43e3 --- /dev/null +++ b/tests/codegen/read-noundef-metadata.rs @@ -0,0 +1,51 @@ +// compile-flags: -O -Z merge-functions=disabled +// no-system-llvm +// ignore-debug (the extra assertions get in the way) + +#![crate_type = "lib"] + +// Ensure that various forms of reading pointers correctly annotate the `load`s +// with `!noundef` metadata to enable extra optimization. The functions return +// `MaybeUninit` to keep it from being inferred from the function type. + +use std::mem::MaybeUninit; + +// CHECK-LABEL: define i8 @copy_byte( +#[no_mangle] +pub unsafe fn copy_byte(p: *const u8) -> MaybeUninit { + // CHECK-NOT: load + // CHECK: load i8, ptr %p, align 1 + // CHECK-SAME: !noundef ! + // CHECK-NOT: load + MaybeUninit::new(*p) +} + +// CHECK-LABEL: define i8 @read_byte( +#[no_mangle] +pub unsafe fn read_byte(p: *const u8) -> MaybeUninit { + // CHECK-NOT: load + // CHECK: load i8, ptr %p, align 1 + // CHECK-SAME: !noundef ! + // CHECK-NOT: load + MaybeUninit::new(p.read()) +} + +// CHECK-LABEL: define i8 @read_byte_maybe_uninit( +#[no_mangle] +pub unsafe fn read_byte_maybe_uninit(p: *const MaybeUninit) -> MaybeUninit { + // CHECK-NOT: load + // CHECK: load i8, ptr %p, align 1 + // CHECK-NOT: noundef + // CHECK-NOT: load + p.read() +} + +// CHECK-LABEL: define i8 @read_byte_assume_init( +#[no_mangle] +pub unsafe fn read_byte_assume_init(p: &MaybeUninit) -> MaybeUninit { + // CHECK-NOT: load + // CHECK: load i8, ptr %p, align 1 + // CHECK-SAME: !noundef ! + // CHECK-NOT: load + MaybeUninit::new(p.assume_init_read()) +} diff --git a/tests/mir-opt/lower_intrinsics.read_via_copy_primitive.LowerIntrinsics.diff b/tests/mir-opt/lower_intrinsics.read_via_copy_primitive.LowerIntrinsics.diff new file mode 100644 index 0000000000000..27fceeedf6e2c --- /dev/null +++ b/tests/mir-opt/lower_intrinsics.read_via_copy_primitive.LowerIntrinsics.diff @@ -0,0 +1,27 @@ +- // MIR for `read_via_copy_primitive` before LowerIntrinsics ++ // MIR for `read_via_copy_primitive` after LowerIntrinsics + + fn read_via_copy_primitive(_1: &i32) -> i32 { + debug r => _1; // in scope 0 at $DIR/lower_intrinsics.rs:+0:32: +0:33 + let mut _0: i32; // return place in scope 0 at $DIR/lower_intrinsics.rs:+0:44: +0:47 + let mut _2: *const i32; // in scope 0 at $DIR/lower_intrinsics.rs:+1:46: +1:47 + scope 1 { + } + + bb0: { + StorageLive(_2); // scope 1 at $DIR/lower_intrinsics.rs:+1:46: +1:47 + _2 = &raw const (*_1); // scope 1 at $DIR/lower_intrinsics.rs:+1:46: +1:47 +- _0 = read_via_copy::(move _2) -> bb1; // scope 1 at $DIR/lower_intrinsics.rs:+1:14: +1:48 +- // mir::Constant +- // + span: $DIR/lower_intrinsics.rs:85:14: 85:45 +- // + literal: Const { ty: unsafe extern "rust-intrinsic" fn(*const i32) -> i32 {read_via_copy::}, val: Value() } ++ _0 = (*_2); // scope 1 at $DIR/lower_intrinsics.rs:+1:14: +1:48 ++ goto -> bb1; // scope 1 at $DIR/lower_intrinsics.rs:+1:14: +1:48 + } + + bb1: { + StorageDead(_2); // scope 1 at $DIR/lower_intrinsics.rs:+1:47: +1:48 + return; // scope 0 at $DIR/lower_intrinsics.rs:+2:2: +2:2 + } + } + diff --git a/tests/mir-opt/lower_intrinsics.read_via_copy_uninhabited.LowerIntrinsics.diff b/tests/mir-opt/lower_intrinsics.read_via_copy_uninhabited.LowerIntrinsics.diff new file mode 100644 index 0000000000000..faf3f9c31b82f --- /dev/null +++ b/tests/mir-opt/lower_intrinsics.read_via_copy_uninhabited.LowerIntrinsics.diff @@ -0,0 +1,22 @@ +- // MIR for `read_via_copy_uninhabited` before LowerIntrinsics ++ // MIR for `read_via_copy_uninhabited` after LowerIntrinsics + + fn read_via_copy_uninhabited(_1: &Never) -> Never { + debug r => _1; // in scope 0 at $DIR/lower_intrinsics.rs:+0:34: +0:35 + let mut _0: Never; // return place in scope 0 at $DIR/lower_intrinsics.rs:+0:48: +0:53 + let mut _2: *const Never; // in scope 0 at $DIR/lower_intrinsics.rs:+1:46: +1:47 + scope 1 { + } + + bb0: { + StorageLive(_2); // scope 1 at $DIR/lower_intrinsics.rs:+1:46: +1:47 + _2 = &raw const (*_1); // scope 1 at $DIR/lower_intrinsics.rs:+1:46: +1:47 +- _0 = read_via_copy::(move _2); // scope 1 at $DIR/lower_intrinsics.rs:+1:14: +1:48 +- // mir::Constant +- // + span: $DIR/lower_intrinsics.rs:90:14: 90:45 +- // + literal: Const { ty: unsafe extern "rust-intrinsic" fn(*const Never) -> Never {read_via_copy::}, val: Value() } ++ _0 = (*_2); // scope 1 at $DIR/lower_intrinsics.rs:+1:14: +1:48 ++ unreachable; // scope 1 at $DIR/lower_intrinsics.rs:+1:14: +1:48 + } + } + diff --git a/tests/mir-opt/lower_intrinsics.rs b/tests/mir-opt/lower_intrinsics.rs index 7147be43ca5e3..a0a1df4e5ca86 100644 --- a/tests/mir-opt/lower_intrinsics.rs +++ b/tests/mir-opt/lower_intrinsics.rs @@ -79,3 +79,15 @@ pub fn with_overflow(a: i32, b: i32) { let _y = core::intrinsics::sub_with_overflow(a, b); let _z = core::intrinsics::mul_with_overflow(a, b); } + +// EMIT_MIR lower_intrinsics.read_via_copy_primitive.LowerIntrinsics.diff +pub fn read_via_copy_primitive(r: &i32) -> i32 { + unsafe { core::intrinsics::read_via_copy(r) } +} + +// EMIT_MIR lower_intrinsics.read_via_copy_uninhabited.LowerIntrinsics.diff +pub fn read_via_copy_uninhabited(r: &Never) -> Never { + unsafe { core::intrinsics::read_via_copy(r) } +} + +pub enum Never {} diff --git a/tests/ui/const-ptr/out_of_bounds_read.stderr b/tests/ui/const-ptr/out_of_bounds_read.stderr index 3e7b09a5982d9..89536f53f08b0 100644 --- a/tests/ui/const-ptr/out_of_bounds_read.stderr +++ b/tests/ui/const-ptr/out_of_bounds_read.stderr @@ -1,7 +1,7 @@ error[E0080]: evaluation of constant value failed --> $SRC_DIR/core/src/ptr/mod.rs:LL:COL | - = note: memory access failed: alloc5 has size 4, so pointer to 4 bytes starting at offset 4 is out-of-bounds + = note: dereferencing pointer failed: alloc5 has size 4, so pointer to 4 bytes starting at offset 4 is out-of-bounds | note: inside `std::ptr::read::` --> $SRC_DIR/core/src/ptr/mod.rs:LL:COL @@ -14,7 +14,7 @@ LL | const _READ: u32 = unsafe { ptr::read(PAST_END_PTR) }; error[E0080]: evaluation of constant value failed --> $SRC_DIR/core/src/ptr/mod.rs:LL:COL | - = note: memory access failed: alloc5 has size 4, so pointer to 4 bytes starting at offset 4 is out-of-bounds + = note: dereferencing pointer failed: alloc5 has size 4, so pointer to 4 bytes starting at offset 4 is out-of-bounds | note: inside `std::ptr::read::` --> $SRC_DIR/core/src/ptr/mod.rs:LL:COL @@ -29,7 +29,7 @@ LL | const _CONST_READ: u32 = unsafe { PAST_END_PTR.read() }; error[E0080]: evaluation of constant value failed --> $SRC_DIR/core/src/ptr/mod.rs:LL:COL | - = note: memory access failed: alloc5 has size 4, so pointer to 4 bytes starting at offset 4 is out-of-bounds + = note: dereferencing pointer failed: alloc5 has size 4, so pointer to 4 bytes starting at offset 4 is out-of-bounds | note: inside `std::ptr::read::` --> $SRC_DIR/core/src/ptr/mod.rs:LL:COL diff --git a/tests/ui/consts/const-eval/ub-ref-ptr.stderr b/tests/ui/consts/const-eval/ub-ref-ptr.stderr index 6bd367b646902..080568b51ef71 100644 --- a/tests/ui/consts/const-eval/ub-ref-ptr.stderr +++ b/tests/ui/consts/const-eval/ub-ref-ptr.stderr @@ -148,11 +148,11 @@ LL | const DATA_FN_PTR: fn() = unsafe { mem::transmute(&13) }; HEX_DUMP } -error: accessing memory with alignment 1, but alignment 4 is required +error[E0080]: evaluation of constant value failed --> $SRC_DIR/core/src/ptr/mod.rs:LL:COL | - = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! - = note: for more information, see issue #68585 + = note: accessing memory with alignment 1, but alignment 4 is required + | note: inside `std::ptr::read::` --> $SRC_DIR/core/src/ptr/mod.rs:LL:COL note: inside `ptr::const_ptr::::read` @@ -162,25 +162,7 @@ note: inside `UNALIGNED_READ` | LL | ptr.read(); | ^^^^^^^^^^ - = note: `#[deny(invalid_alignment)]` on by default error: aborting due to 15 previous errors For more information about this error, try `rustc --explain E0080`. -Future incompatibility report: Future breakage diagnostic: -error: accessing memory with alignment 1, but alignment 4 is required - --> $SRC_DIR/core/src/ptr/mod.rs:LL:COL - | - = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! - = note: for more information, see issue #68585 -note: inside `std::ptr::read::` - --> $SRC_DIR/core/src/ptr/mod.rs:LL:COL -note: inside `ptr::const_ptr::::read` - --> $SRC_DIR/core/src/ptr/const_ptr.rs:LL:COL -note: inside `UNALIGNED_READ` - --> $DIR/ub-ref-ptr.rs:67:5 - | -LL | ptr.read(); - | ^^^^^^^^^^ - = note: `#[deny(invalid_alignment)]` on by default - diff --git a/tests/ui/consts/issue-miri-1910.stderr b/tests/ui/consts/issue-miri-1910.stderr index 61865b1dad764..a10eea9de114f 100644 --- a/tests/ui/consts/issue-miri-1910.stderr +++ b/tests/ui/consts/issue-miri-1910.stderr @@ -1,7 +1,7 @@ error[E0080]: evaluation of constant value failed --> $SRC_DIR/core/src/ptr/mod.rs:LL:COL | - = note: unable to copy parts of a pointer from memory at ALLOC + = note: unable to turn pointer into raw bytes | = help: this code performed an operation that depends on the underlying bytes representing a pointer = help: the absolute address of a pointer is not known at compile-time, so such operations are not supported From 0b96fee34315b75c8f05ce338d3beb6c85772056 Mon Sep 17 00:00:00 2001 From: Scott McMurray Date: Sun, 12 Mar 2023 12:57:40 -0700 Subject: [PATCH 2/6] Add a codegen test to confirm this fixes 106369 --- tests/codegen/issues/issue-106369.rs | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 tests/codegen/issues/issue-106369.rs diff --git a/tests/codegen/issues/issue-106369.rs b/tests/codegen/issues/issue-106369.rs new file mode 100644 index 0000000000000..3fe7be4f1442e --- /dev/null +++ b/tests/codegen/issues/issue-106369.rs @@ -0,0 +1,15 @@ +// compile-flags: -O +// ignore-debug (the extra assertions get in the way) + +#![crate_type = "lib"] + +// From + +// CHECK-LABEL: @issue_106369( +#[no_mangle] +pub unsafe fn issue_106369(ptr: *const &i32) -> bool { + // CHECK-NOT: icmp + // CHECK: ret i1 true + // CHECK-NOT: icmp + Some(std::ptr::read(ptr)).is_some() +} From 1f70bb8c43054d3948ffb579ba3809822a889c62 Mon Sep 17 00:00:00 2001 From: Scott McMurray Date: Sun, 12 Mar 2023 13:23:22 -0700 Subject: [PATCH 3/6] Add a codegen test to confirm this fixes 73258 --- tests/codegen/issues/issue-73258.rs | 38 +++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) create mode 100644 tests/codegen/issues/issue-73258.rs diff --git a/tests/codegen/issues/issue-73258.rs b/tests/codegen/issues/issue-73258.rs new file mode 100644 index 0000000000000..0134f929b2961 --- /dev/null +++ b/tests/codegen/issues/issue-73258.rs @@ -0,0 +1,38 @@ +// compile-flags: -O +// ignore-debug (the extra assertions get in the way) + +#![crate_type = "lib"] + +// Adapted from + +#[derive(Clone, Copy)] +#[repr(u8)] +pub enum Foo { + A, B, C, D, +} + +// CHECK-LABEL: @issue_73258( +#[no_mangle] +pub unsafe fn issue_73258(ptr: *const Foo) -> Foo { + // CHECK-NOT: icmp + // CHECK-NOT: call + // CHECK-NOT: br + // CHECK-NOT: select + + // CHECK: %[[R:.+]] = load i8 + // CHECK-SAME: !range ! + + // CHECK-NOT: icmp + // CHECK-NOT: call + // CHECK-NOT: br + // CHECK-NOT: select + + // CHECK: ret i8 %[[R]] + + // CHECK-NOT: icmp + // CHECK-NOT: call + // CHECK-NOT: br + // CHECK-NOT: select + let k: Option = Some(ptr.read()); + return k.unwrap(); +} From 87696fd5a1a1b7cd75cd9a66896deae0ab56cfb5 Mon Sep 17 00:00:00 2001 From: Scott McMurray Date: Sun, 12 Mar 2023 15:52:34 -0700 Subject: [PATCH 4/6] Add a better approach comment in `ptr::read` to justify the intrinsic --- library/core/src/ptr/mod.rs | 43 +++++++++++++++++++++++++++---------- 1 file changed, 32 insertions(+), 11 deletions(-) diff --git a/library/core/src/ptr/mod.rs b/library/core/src/ptr/mod.rs index 04b67a56db59b..86929e2c4889c 100644 --- a/library/core/src/ptr/mod.rs +++ b/library/core/src/ptr/mod.rs @@ -1135,17 +1135,31 @@ pub const unsafe fn replace(dst: *mut T, mut src: T) -> T { #[rustc_const_unstable(feature = "const_ptr_read", issue = "80377")] #[cfg_attr(miri, track_caller)] // even without panics, this helps for Miri backtraces pub const unsafe fn read(src: *const T) -> T { - // We are calling the intrinsics directly to avoid function calls in the generated code - // as `intrinsics::copy_nonoverlapping` is a wrapper function. - #[cfg(bootstrap)] - extern "rust-intrinsic" { - #[rustc_const_stable(feature = "const_intrinsic_copy", since = "1.63.0")] - fn copy_nonoverlapping(src: *const T, dst: *mut T, count: usize); - } + // It would be semantically correct to implement this via `copy_nonoverlapping` + // and `MaybeUninit`, as was done before PR #109035. + + // However, it switched to intrinsic that lowers to `_0 = *src` in MIR in + // order to address a few implementation issues: + // + // - Using `MaybeUninit::assume_init` after a `copy_nonoverlapping` was not + // turning the untyped copy into a typed load. As such, the generated + // `load` in LLVM didn't get various metadata, such as `!range` (#73258), + // `!nonnull`, and `!noundef`, resulting in poorer optimization. + // - Going through the extra local resulted in multiple extra copies, even + // in optimized MIR. (Ignoring StorageLive/Dead, the intrinsic is one + // MIR statement, while the previous implementation was eight.) LLVM + // could sometimes optimize them away, but because `read` is at the core + // of so many things, not having them in the first place improves what we + // hand off to the backend. For example, `mem::replace::` previously + // emitted 4 `alloca` and 6 `memcpy`s, but is now 1 `alloc` and 3 `memcpy`s. + // - In general, this approach keeps us from getting any more bugs (like + // #106369) that boil down to "`read(p)` is worse than `*p`", as this + // makes them look identical to the backend (or other MIR consumers). + // + // Future enhancements to MIR optimizations might well allow this to return + // to the previous implementation, rather than using an intrinsic. // SAFETY: the caller must guarantee that `src` is valid for reads. - // `src` cannot overlap `tmp` because `tmp` was just allocated on - // the stack as a separate allocated object. unsafe { assert_unsafe_precondition!( "ptr::read requires that the pointer argument is aligned and non-null", @@ -1154,14 +1168,21 @@ pub const unsafe fn read(src: *const T) -> T { #[cfg(bootstrap)] { + // We are calling the intrinsics directly to avoid function calls in the + // generated code as `intrinsics::copy_nonoverlapping` is a wrapper function. + extern "rust-intrinsic" { + #[rustc_const_stable(feature = "const_intrinsic_copy", since = "1.63.0")] + fn copy_nonoverlapping(src: *const T, dst: *mut T, count: usize); + } + + // `src` cannot overlap `tmp` because `tmp` was just allocated on + // the stack as a separate allocated object. let mut tmp = MaybeUninit::::uninit(); copy_nonoverlapping(src, tmp.as_mut_ptr(), 1); tmp.assume_init() } #[cfg(not(bootstrap))] { - // This uses a dedicated intrinsic, not `copy_nonoverlapping`, - // so that it gets a *typed* copy, not an *untyped* one. crate::intrinsics::read_via_copy(src) } } From e7c6ad89cfd937d741b9d256e950ec0de96a7142 Mon Sep 17 00:00:00 2001 From: Scott McMurray Date: Tue, 14 Mar 2023 22:24:28 -0700 Subject: [PATCH 5/6] Improved implementation and comments after code review feedback --- .../src/lower_intrinsics.rs | 31 +++--- library/core/src/intrinsics.rs | 14 +-- library/core/src/ptr/mod.rs | 8 +- tests/codegen/ptr-read-metadata.rs | 96 +++++++++++++++++++ tests/codegen/read-noundef-metadata.rs | 51 ---------- ..._via_copy_uninhabited.LowerIntrinsics.diff | 1 - 6 files changed, 124 insertions(+), 77 deletions(-) create mode 100644 tests/codegen/ptr-read-metadata.rs delete mode 100644 tests/codegen/read-noundef-metadata.rs diff --git a/compiler/rustc_mir_transform/src/lower_intrinsics.rs b/compiler/rustc_mir_transform/src/lower_intrinsics.rs index 151fff27d14ad..5d7382305ae14 100644 --- a/compiler/rustc_mir_transform/src/lower_intrinsics.rs +++ b/compiler/rustc_mir_transform/src/lower_intrinsics.rs @@ -150,7 +150,7 @@ impl<'tcx> MirPass<'tcx> for LowerIntrinsics { } } sym::read_via_copy => { - let Ok([arg]) = <[_; 1]>::try_from(std::mem::take(args)) else { + let [arg] = args.as_slice() else { span_bug!(terminator.source_info.span, "Wrong number of arguments"); }; let derefed_place = @@ -159,18 +159,23 @@ impl<'tcx> MirPass<'tcx> for LowerIntrinsics { } else { span_bug!(terminator.source_info.span, "Only passing a local is supported"); }; - block.statements.push(Statement { - source_info: terminator.source_info, - kind: StatementKind::Assign(Box::new(( - *destination, - Rvalue::Use(Operand::Copy(derefed_place)), - ))), - }); - if let Some(target) = *target { - terminator.kind = TerminatorKind::Goto { target }; - } else { - // Reading something uninhabited means this is unreachable. - terminator.kind = TerminatorKind::Unreachable; + terminator.kind = match *target { + None => { + // No target means this read something uninhabited, + // so it must be unreachable, and we don't need to + // preserve the assignment either. + TerminatorKind::Unreachable + } + Some(target) => { + block.statements.push(Statement { + source_info: terminator.source_info, + kind: StatementKind::Assign(Box::new(( + *destination, + Rvalue::Use(Operand::Copy(derefed_place)), + ))), + }); + TerminatorKind::Goto { target } + } } } sym::discriminant_value => { diff --git a/library/core/src/intrinsics.rs b/library/core/src/intrinsics.rs index d77c2a00eb780..ee8846675ce25 100644 --- a/library/core/src/intrinsics.rs +++ b/library/core/src/intrinsics.rs @@ -2020,16 +2020,12 @@ extern "rust-intrinsic" { #[rustc_safe_intrinsic] pub fn saturating_sub(a: T, b: T) -> T; - /// This is a *typed* read, `copy *p` in MIR. + /// This is an implementation detail of [`crate::ptr::read`] and should + /// not be used anywhere else. See its comments for why this exists. /// - /// The stabilized form of this intrinsic is [`crate::ptr::read`], so - /// that can be implemented without needing to do an *untyped* copy - /// via [`copy_nonoverlapping`], and thus can get proper metadata. - /// - /// This intrinsic can *only* be called with a copy or move of a local. - /// (It allows neither constants nor projections.) - /// - /// To avoid introducing any `noalias` requirements, it just takes a pointer. + /// This intrinsic can *only* be called where the argument is a local without + /// projections (`read_via_copy(p)`, not `read_via_copy(*p)`) so that it + /// trivially obeys runtime-MIR rules about derefs in operands. #[cfg(not(bootstrap))] #[rustc_const_unstable(feature = "const_ptr_read", issue = "80377")] pub fn read_via_copy(p: *const T) -> T; diff --git a/library/core/src/ptr/mod.rs b/library/core/src/ptr/mod.rs index 86929e2c4889c..5884a8ca30807 100644 --- a/library/core/src/ptr/mod.rs +++ b/library/core/src/ptr/mod.rs @@ -1136,10 +1136,12 @@ pub const unsafe fn replace(dst: *mut T, mut src: T) -> T { #[cfg_attr(miri, track_caller)] // even without panics, this helps for Miri backtraces pub const unsafe fn read(src: *const T) -> T { // It would be semantically correct to implement this via `copy_nonoverlapping` - // and `MaybeUninit`, as was done before PR #109035. + // and `MaybeUninit`, as was done before PR #109035. Calling `assume_init` + // provides enough information to know that this is a typed operation. - // However, it switched to intrinsic that lowers to `_0 = *src` in MIR in - // order to address a few implementation issues: + // However, as of March 2023 the compiler was not capable of taking advantage + // of that information. Thus the implementation here switched to an intrinsic, + // which lowers to `_0 = *src` in MIR, to address a few issues: // // - Using `MaybeUninit::assume_init` after a `copy_nonoverlapping` was not // turning the untyped copy into a typed load. As such, the generated diff --git a/tests/codegen/ptr-read-metadata.rs b/tests/codegen/ptr-read-metadata.rs new file mode 100644 index 0000000000000..e1e3272662c4b --- /dev/null +++ b/tests/codegen/ptr-read-metadata.rs @@ -0,0 +1,96 @@ +// compile-flags: -O -Z merge-functions=disabled +// no-system-llvm +// ignore-debug (the extra assertions get in the way) + +#![crate_type = "lib"] + +// Ensure that various forms of reading pointers correctly annotate the `load`s +// with `!noundef` and `!range` metadata to enable extra optimization. + +use std::mem::MaybeUninit; + +// CHECK-LABEL: define noundef i8 @copy_byte( +#[no_mangle] +pub unsafe fn copy_byte(p: *const u8) -> u8 { + // CHECK-NOT: load + // CHECK: load i8, ptr %p, align 1 + // CHECK-SAME: !noundef ! + // CHECK-NOT: load + *p +} + +// CHECK-LABEL: define noundef i8 @read_byte( +#[no_mangle] +pub unsafe fn read_byte(p: *const u8) -> u8 { + // CHECK-NOT: load + // CHECK: load i8, ptr %p, align 1 + // CHECK-SAME: !noundef ! + // CHECK-NOT: load + p.read() +} + +// CHECK-LABEL: define i8 @read_byte_maybe_uninit( +#[no_mangle] +pub unsafe fn read_byte_maybe_uninit(p: *const MaybeUninit) -> MaybeUninit { + // CHECK-NOT: load + // CHECK: load i8, ptr %p, align 1 + // CHECK-NOT: noundef + // CHECK-NOT: load + p.read() +} + +// CHECK-LABEL: define noundef i8 @read_byte_assume_init( +#[no_mangle] +pub unsafe fn read_byte_assume_init(p: &MaybeUninit) -> u8 { + // CHECK-NOT: load + // CHECK: load i8, ptr %p, align 1 + // CHECK-SAME: !noundef ! + // CHECK-NOT: load + p.assume_init_read() +} + +// CHECK-LABEL: define noundef i32 @copy_char( +#[no_mangle] +pub unsafe fn copy_char(p: *const char) -> char { + // CHECK-NOT: load + // CHECK: load i32, ptr %p + // CHECK-SAME: !range ![[RANGE:[0-9]+]] + // CHECK-SAME: !noundef ! + // CHECK-NOT: load + *p +} + +// CHECK-LABEL: define noundef i32 @read_char( +#[no_mangle] +pub unsafe fn read_char(p: *const char) -> char { + // CHECK-NOT: load + // CHECK: load i32, ptr %p + // CHECK-SAME: !range ![[RANGE]] + // CHECK-SAME: !noundef ! + // CHECK-NOT: load + p.read() +} + +// CHECK-LABEL: define i32 @read_char_maybe_uninit( +#[no_mangle] +pub unsafe fn read_char_maybe_uninit(p: *const MaybeUninit) -> MaybeUninit { + // CHECK-NOT: load + // CHECK: load i32, ptr %p + // CHECK-NOT: range + // CHECK-NOT: noundef + // CHECK-NOT: load + p.read() +} + +// CHECK-LABEL: define noundef i32 @read_char_assume_init( +#[no_mangle] +pub unsafe fn read_char_assume_init(p: &MaybeUninit) -> char { + // CHECK-NOT: load + // CHECK: load i32, ptr %p + // CHECK-SAME: !range ![[RANGE]] + // CHECK-SAME: !noundef ! + // CHECK-NOT: load + p.assume_init_read() +} + +// CHECK: ![[RANGE]] = !{i32 0, i32 1114112} diff --git a/tests/codegen/read-noundef-metadata.rs b/tests/codegen/read-noundef-metadata.rs deleted file mode 100644 index 03386921c43e3..0000000000000 --- a/tests/codegen/read-noundef-metadata.rs +++ /dev/null @@ -1,51 +0,0 @@ -// compile-flags: -O -Z merge-functions=disabled -// no-system-llvm -// ignore-debug (the extra assertions get in the way) - -#![crate_type = "lib"] - -// Ensure that various forms of reading pointers correctly annotate the `load`s -// with `!noundef` metadata to enable extra optimization. The functions return -// `MaybeUninit` to keep it from being inferred from the function type. - -use std::mem::MaybeUninit; - -// CHECK-LABEL: define i8 @copy_byte( -#[no_mangle] -pub unsafe fn copy_byte(p: *const u8) -> MaybeUninit { - // CHECK-NOT: load - // CHECK: load i8, ptr %p, align 1 - // CHECK-SAME: !noundef ! - // CHECK-NOT: load - MaybeUninit::new(*p) -} - -// CHECK-LABEL: define i8 @read_byte( -#[no_mangle] -pub unsafe fn read_byte(p: *const u8) -> MaybeUninit { - // CHECK-NOT: load - // CHECK: load i8, ptr %p, align 1 - // CHECK-SAME: !noundef ! - // CHECK-NOT: load - MaybeUninit::new(p.read()) -} - -// CHECK-LABEL: define i8 @read_byte_maybe_uninit( -#[no_mangle] -pub unsafe fn read_byte_maybe_uninit(p: *const MaybeUninit) -> MaybeUninit { - // CHECK-NOT: load - // CHECK: load i8, ptr %p, align 1 - // CHECK-NOT: noundef - // CHECK-NOT: load - p.read() -} - -// CHECK-LABEL: define i8 @read_byte_assume_init( -#[no_mangle] -pub unsafe fn read_byte_assume_init(p: &MaybeUninit) -> MaybeUninit { - // CHECK-NOT: load - // CHECK: load i8, ptr %p, align 1 - // CHECK-SAME: !noundef ! - // CHECK-NOT: load - MaybeUninit::new(p.assume_init_read()) -} diff --git a/tests/mir-opt/lower_intrinsics.read_via_copy_uninhabited.LowerIntrinsics.diff b/tests/mir-opt/lower_intrinsics.read_via_copy_uninhabited.LowerIntrinsics.diff index faf3f9c31b82f..610c67d2fecd6 100644 --- a/tests/mir-opt/lower_intrinsics.read_via_copy_uninhabited.LowerIntrinsics.diff +++ b/tests/mir-opt/lower_intrinsics.read_via_copy_uninhabited.LowerIntrinsics.diff @@ -15,7 +15,6 @@ - // mir::Constant - // + span: $DIR/lower_intrinsics.rs:90:14: 90:45 - // + literal: Const { ty: unsafe extern "rust-intrinsic" fn(*const Never) -> Never {read_via_copy::}, val: Value() } -+ _0 = (*_2); // scope 1 at $DIR/lower_intrinsics.rs:+1:14: +1:48 + unreachable; // scope 1 at $DIR/lower_intrinsics.rs:+1:14: +1:48 } } From dfc3377954f9e03172fed57ca890141006a0d82e Mon Sep 17 00:00:00 2001 From: Scott McMurray Date: Wed, 15 Mar 2023 00:57:08 -0700 Subject: [PATCH 6/6] Split the mem-replace codegen test Apparently in CI it's getting generated in the opposite order, one function per file will make the test pass either way. --- tests/codegen/mem-replace-big-type.rs | 36 ++++++++++++++++++++++ tests/codegen/mem-replace-direct-memcpy.rs | 22 ------------- 2 files changed, 36 insertions(+), 22 deletions(-) create mode 100644 tests/codegen/mem-replace-big-type.rs diff --git a/tests/codegen/mem-replace-big-type.rs b/tests/codegen/mem-replace-big-type.rs new file mode 100644 index 0000000000000..f6898e2f75814 --- /dev/null +++ b/tests/codegen/mem-replace-big-type.rs @@ -0,0 +1,36 @@ +// This test ensures that `mem::replace::` only ever calls `@llvm.memcpy` +// with `size_of::()` as the size, and never goes through any wrapper that +// may e.g. multiply `size_of::()` with a variable "count" (which is only +// known to be `1` after inlining). + +// compile-flags: -C no-prepopulate-passes -Zinline-mir=no +// ignore-debug: the debug assertions get in the way + +#![crate_type = "lib"] + +#[repr(C, align(8))] +pub struct Big([u64; 7]); +pub fn replace_big(dst: &mut Big, src: Big) -> Big { + // Before the `read_via_copy` intrinsic, this emitted six `memcpy`s. + std::mem::replace(dst, src) +} + +// NOTE(eddyb) the `CHECK-NOT`s ensure that the only calls of `@llvm.memcpy` in +// the entire output, are the direct calls we want, from `ptr::replace`. + +// CHECK-NOT: call void @llvm.memcpy + +// For a large type, we expect exactly three `memcpy`s +// CHECK-LABEL: define internal void @{{.+}}mem{{.+}}replace{{.+}}sret(%Big) + // CHECK-NOT: alloca + // CHECK: alloca %Big + // CHECK-NOT: alloca + // CHECK-NOT: call void @llvm.memcpy + // CHECK: call void @llvm.memcpy.{{.+}}({{i8\*|ptr}} align 8 %{{.*}}, {{i8\*|ptr}} align 8 %{{.*}}, i{{.*}} 56, i1 false) + // CHECK-NOT: call void @llvm.memcpy + // CHECK: call void @llvm.memcpy.{{.+}}({{i8\*|ptr}} align 8 %{{.*}}, {{i8\*|ptr}} align 8 %{{.*}}, i{{.*}} 56, i1 false) + // CHECK-NOT: call void @llvm.memcpy + // CHECK: call void @llvm.memcpy.{{.+}}({{i8\*|ptr}} align 8 %{{.*}}, {{i8\*|ptr}} align 8 %{{.*}}, i{{.*}} 56, i1 false) + // CHECK-NOT: call void @llvm.memcpy + +// CHECK-NOT: call void @llvm.memcpy diff --git a/tests/codegen/mem-replace-direct-memcpy.rs b/tests/codegen/mem-replace-direct-memcpy.rs index 3b01a621b5680..83babab4f847b 100644 --- a/tests/codegen/mem-replace-direct-memcpy.rs +++ b/tests/codegen/mem-replace-direct-memcpy.rs @@ -12,33 +12,11 @@ pub fn replace_byte(dst: &mut u8, src: u8) -> u8 { std::mem::replace(dst, src) } -#[repr(C, align(8))] -pub struct Big([u64; 7]); -pub fn replace_big(dst: &mut Big, src: Big) -> Big { - // Before the `read_via_copy` intrinsic, this emitted six `memcpy`s. - std::mem::replace(dst, src) -} - // NOTE(eddyb) the `CHECK-NOT`s ensure that the only calls of `@llvm.memcpy` in // the entire output, are the direct calls we want, from `ptr::replace`. // CHECK-NOT: call void @llvm.memcpy -// For a large type, we expect exactly three `memcpy`s -// CHECK-LABEL: define internal void @{{.+}}mem{{.+}}replace{{.+}}sret(%Big) - // CHECK-NOT: alloca - // CHECK: alloca %Big - // CHECK-NOT: alloca - // CHECK-NOT: call void @llvm.memcpy - // CHECK: call void @llvm.memcpy.{{.+}}({{i8\*|ptr}} align 8 %{{.*}}, {{i8\*|ptr}} align 8 %{{.*}}, i{{.*}} 56, i1 false) - // CHECK-NOT: call void @llvm.memcpy - // CHECK: call void @llvm.memcpy.{{.+}}({{i8\*|ptr}} align 8 %{{.*}}, {{i8\*|ptr}} align 8 %{{.*}}, i{{.*}} 56, i1 false) - // CHECK-NOT: call void @llvm.memcpy - // CHECK: call void @llvm.memcpy.{{.+}}({{i8\*|ptr}} align 8 %{{.*}}, {{i8\*|ptr}} align 8 %{{.*}}, i{{.*}} 56, i1 false) - // CHECK-NOT: call void @llvm.memcpy - -// CHECK-NOT: call void @llvm.memcpy - // For a small type, we expect one each of `load`/`store`/`memcpy` instead // CHECK-LABEL: define internal noundef i8 @{{.+}}mem{{.+}}replace // CHECK-NOT: alloca