Skip to content

Commit b2c717f

Browse files
scottmcmscottmcm
committed
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<i32, N>`. Turned out to be a more general problem as `MaybeUninit::assume_init_read` isn't marking the load as initialized (<https://rust.godbolt.org/z/Mxd8TPTnv>), 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`.
1 parent 19c5376 commit b2c717f

14 files changed

+214
-39
lines changed

‎compiler/rustc_hir_analysis/src/check/intrinsic.rs

+2
Original file line numberDiff line numberDiff line change
@@ -363,6 +363,8 @@ pub fn check_intrinsic_type(tcx: TyCtxt<'_>, it: &hir::ForeignItem<'_>) {
363363
sym::likely => (0, vec![tcx.types.bool], tcx.types.bool),
364364
sym::unlikely => (0, vec![tcx.types.bool], tcx.types.bool),
365365

366+
sym::read_via_copy => (1, vec![tcx.mk_imm_ptr(param(0))], param(0)),
367+
366368
sym::discriminant_value => {
367369
let assoc_items = tcx.associated_item_def_ids(
368370
tcx.require_lang_item(hir::LangItem::DiscriminantKind, None),

‎compiler/rustc_lint_defs/src/builtin.rs

+4-3
Original file line numberDiff line numberDiff line change
@@ -1026,12 +1026,13 @@ declare_lint! {
10261026
/// ### Example
10271027
///
10281028
/// ```rust,compile_fail
1029-
/// #![feature(const_ptr_read)]
1029+
/// #![feature(const_mut_refs)]
10301030
/// const FOO: () = unsafe {
10311031
/// let x = &[0_u8; 4];
10321032
/// let y = x.as_ptr().cast::<u32>();
1033-
/// y.read(); // the address of a `u8` array is unknown and thus we don't know if
1034-
/// // it is aligned enough for reading a `u32`.
1033+
/// let mut z = 123;
1034+
/// y.copy_to_nonoverlapping(&mut z, 1); // the address of a `u8` array is unknown
1035+
/// // and thus we don't know if it is aligned enough for copying a `u32`.
10351036
/// };
10361037
/// ```
10371038
///

‎compiler/rustc_mir_transform/src/lower_intrinsics.rs

+24
Original file line numberDiff line numberDiff line change
@@ -149,6 +149,30 @@ impl<'tcx> MirPass<'tcx> for LowerIntrinsics {
149149
terminator.kind = TerminatorKind::Goto { target };
150150
}
151151
}
152+
sym::read_via_copy => {
153+
let Ok([arg]) = <[_; 1]>::try_from(std::mem::take(args)) else {
154+
span_bug!(terminator.source_info.span, "Wrong number of arguments");
155+
};
156+
let derefed_place =
157+
if let Some(place) = arg.place() && let Some(local) = place.as_local() {
158+
tcx.mk_place_deref(local.into())
159+
} else {
160+
span_bug!(terminator.source_info.span, "Only passing a local is supported");
161+
};
162+
block.statements.push(Statement {
163+
source_info: terminator.source_info,
164+
kind: StatementKind::Assign(Box::new((
165+
*destination,
166+
Rvalue::Use(Operand::Copy(derefed_place)),
167+
))),
168+
});
169+
if let Some(target) = *target {
170+
terminator.kind = TerminatorKind::Goto { target };
171+
} else {
172+
// Reading something uninhabited means this is unreachable.
173+
terminator.kind = TerminatorKind::Unreachable;
174+
}
175+
}
152176
sym::discriminant_value => {
153177
if let (Some(target), Some(arg)) = (*target, args[0].place()) {
154178
let arg = tcx.mk_place_deref(arg);

‎compiler/rustc_span/src/symbol.rs

+1
Original file line numberDiff line numberDiff line change
@@ -1153,6 +1153,7 @@ symbols! {
11531153
read_enum_variant_arg,
11541154
read_struct,
11551155
read_struct_field,
1156+
read_via_copy,
11561157
readonly,
11571158
realloc,
11581159
reason,

‎library/core/src/intrinsics.rs

+14
Original file line numberDiff line numberDiff line change
@@ -2020,6 +2020,20 @@ extern "rust-intrinsic" {
20202020
#[rustc_safe_intrinsic]
20212021
pub fn saturating_sub<T: Copy>(a: T, b: T) -> T;
20222022

2023+
/// This is a *typed* read, `copy *p` in MIR.
2024+
///
2025+
/// The stabilized form of this intrinsic is [`crate::ptr::read`], so
2026+
/// that can be implemented without needing to do an *untyped* copy
2027+
/// via [`copy_nonoverlapping`], and thus can get proper metadata.
2028+
///
2029+
/// This intrinsic can *only* be called with a copy or move of a local.
2030+
/// (It allows neither constants nor projections.)
2031+
///
2032+
/// To avoid introducing any `noalias` requirements, it just takes a pointer.
2033+
#[cfg(not(bootstrap))]
2034+
#[rustc_const_unstable(feature = "const_ptr_read", issue = "80377")]
2035+
pub fn read_via_copy<T>(p: *const T) -> T;
2036+
20232037
/// Returns the value of the discriminant for the variant in 'v';
20242038
/// if `T` has no discriminant, returns `0`.
20252039
///

‎library/core/src/ptr/mod.rs

+14-6
Original file line numberDiff line numberDiff line change
@@ -1137,25 +1137,33 @@ pub const unsafe fn replace<T>(dst: *mut T, mut src: T) -> T {
11371137
pub const unsafe fn read<T>(src: *const T) -> T {
11381138
// We are calling the intrinsics directly to avoid function calls in the generated code
11391139
// as `intrinsics::copy_nonoverlapping` is a wrapper function.
1140+
#[cfg(bootstrap)]
11401141
extern "rust-intrinsic" {
11411142
#[rustc_const_stable(feature = "const_intrinsic_copy", since = "1.63.0")]
11421143
fn copy_nonoverlapping<T>(src: *const T, dst: *mut T, count: usize);
11431144
}
11441145

1145-
let mut tmp = MaybeUninit::<T>::uninit();
11461146
// SAFETY: the caller must guarantee that `src` is valid for reads.
11471147
// `src` cannot overlap `tmp` because `tmp` was just allocated on
11481148
// the stack as a separate allocated object.
1149-
//
1150-
// Also, since we just wrote a valid value into `tmp`, it is guaranteed
1151-
// to be properly initialized.
11521149
unsafe {
11531150
assert_unsafe_precondition!(
11541151
"ptr::read requires that the pointer argument is aligned and non-null",
11551152
[T](src: *const T) => is_aligned_and_not_null(src)
11561153
);
1157-
copy_nonoverlapping(src, tmp.as_mut_ptr(), 1);
1158-
tmp.assume_init()
1154+
1155+
#[cfg(bootstrap)]
1156+
{
1157+
let mut tmp = MaybeUninit::<T>::uninit();
1158+
copy_nonoverlapping(src, tmp.as_mut_ptr(), 1);
1159+
tmp.assume_init()
1160+
}
1161+
#[cfg(not(bootstrap))]
1162+
{
1163+
// This uses a dedicated intrinsic, not `copy_nonoverlapping`,
1164+
// so that it gets a *typed* copy, not an *untyped* one.
1165+
crate::intrinsics::read_via_copy(src)
1166+
}
11591167
}
11601168
}
11611169

‎tests/codegen/mem-replace-direct-memcpy.rs

+36-5
Original file line numberDiff line numberDiff line change
@@ -12,13 +12,44 @@ pub fn replace_byte(dst: &mut u8, src: u8) -> u8 {
1212
std::mem::replace(dst, src)
1313
}
1414

15+
#[repr(C, align(8))]
16+
pub struct Big([u64; 7]);
17+
pub fn replace_big(dst: &mut Big, src: Big) -> Big {
18+
// Before the `read_via_copy` intrinsic, this emitted six `memcpy`s.
19+
std::mem::replace(dst, src)
20+
}
21+
1522
// NOTE(eddyb) the `CHECK-NOT`s ensure that the only calls of `@llvm.memcpy` in
16-
// the entire output, are the two direct calls we want, from `ptr::replace`.
23+
// the entire output, are the direct calls we want, from `ptr::replace`.
1724

1825
// CHECK-NOT: call void @llvm.memcpy
19-
// CHECK: ; core::mem::replace
20-
// CHECK-NOT: call void @llvm.memcpy
21-
// CHECK: call void @llvm.memcpy.{{.+}}({{i8\*|ptr}} align 1 %{{.*}}, {{i8\*|ptr}} align 1 %{{.*}}, i{{.*}} 1, i1 false)
26+
27+
// For a large type, we expect exactly three `memcpy`s
28+
// CHECK-LABEL: define internal void @{{.+}}mem{{.+}}replace{{.+}}sret(%Big)
29+
// CHECK-NOT: alloca
30+
// CHECK: alloca %Big
31+
// CHECK-NOT: alloca
32+
// CHECK-NOT: call void @llvm.memcpy
33+
// CHECK: call void @llvm.memcpy.{{.+}}({{i8\*|ptr}} align 8 %{{.*}}, {{i8\*|ptr}} align 8 %{{.*}}, i{{.*}} 56, i1 false)
34+
// CHECK-NOT: call void @llvm.memcpy
35+
// CHECK: call void @llvm.memcpy.{{.+}}({{i8\*|ptr}} align 8 %{{.*}}, {{i8\*|ptr}} align 8 %{{.*}}, i{{.*}} 56, i1 false)
36+
// CHECK-NOT: call void @llvm.memcpy
37+
// CHECK: call void @llvm.memcpy.{{.+}}({{i8\*|ptr}} align 8 %{{.*}}, {{i8\*|ptr}} align 8 %{{.*}}, i{{.*}} 56, i1 false)
38+
// CHECK-NOT: call void @llvm.memcpy
39+
2240
// CHECK-NOT: call void @llvm.memcpy
23-
// CHECK: call void @llvm.memcpy.{{.+}}({{i8\*|ptr}} align 1 %{{.*}}, {{i8\*|ptr}} align 1 %{{.*}}, i{{.*}} 1, i1 false)
41+
42+
// For a small type, we expect one each of `load`/`store`/`memcpy` instead
43+
// CHECK-LABEL: define internal noundef i8 @{{.+}}mem{{.+}}replace
44+
// CHECK-NOT: alloca
45+
// CHECK: alloca i8
46+
// CHECK-NOT: alloca
47+
// CHECK-NOT: call void @llvm.memcpy
48+
// CHECK: load i8
49+
// CHECK-NOT: call void @llvm.memcpy
50+
// CHECK: store i8
51+
// CHECK-NOT: call void @llvm.memcpy
52+
// CHECK: call void @llvm.memcpy.{{.+}}({{i8\*|ptr}} align 1 %{{.*}}, {{i8\*|ptr}} align 1 %{{.*}}, i{{.*}} 1, i1 false)
53+
// CHECK-NOT: call void @llvm.memcpy
54+
2455
// CHECK-NOT: call void @llvm.memcpy

‎tests/codegen/read-noundef-metadata.rs

+51
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
// compile-flags: -O -Z merge-functions=disabled
2+
// no-system-llvm
3+
// ignore-debug (the extra assertions get in the way)
4+
5+
#![crate_type = "lib"]
6+
7+
// Ensure that various forms of reading pointers correctly annotate the `load`s
8+
// with `!noundef` metadata to enable extra optimization. The functions return
9+
// `MaybeUninit` to keep it from being inferred from the function type.
10+
11+
use std::mem::MaybeUninit;
12+
13+
// CHECK-LABEL: define i8 @copy_byte(
14+
#[no_mangle]
15+
pub unsafe fn copy_byte(p: *const u8) -> MaybeUninit<u8> {
16+
// CHECK-NOT: load
17+
// CHECK: load i8, ptr %p, align 1
18+
// CHECK-SAME: !noundef !
19+
// CHECK-NOT: load
20+
MaybeUninit::new(*p)
21+
}
22+
23+
// CHECK-LABEL: define i8 @read_byte(
24+
#[no_mangle]
25+
pub unsafe fn read_byte(p: *const u8) -> MaybeUninit<u8> {
26+
// CHECK-NOT: load
27+
// CHECK: load i8, ptr %p, align 1
28+
// CHECK-SAME: !noundef !
29+
// CHECK-NOT: load
30+
MaybeUninit::new(p.read())
31+
}
32+
33+
// CHECK-LABEL: define i8 @read_byte_maybe_uninit(
34+
#[no_mangle]
35+
pub unsafe fn read_byte_maybe_uninit(p: *const MaybeUninit<u8>) -> MaybeUninit<u8> {
36+
// CHECK-NOT: load
37+
// CHECK: load i8, ptr %p, align 1
38+
// CHECK-NOT: noundef
39+
// CHECK-NOT: load
40+
p.read()
41+
}
42+
43+
// CHECK-LABEL: define i8 @read_byte_assume_init(
44+
#[no_mangle]
45+
pub unsafe fn read_byte_assume_init(p: &MaybeUninit<u8>) -> MaybeUninit<u8> {
46+
// CHECK-NOT: load
47+
// CHECK: load i8, ptr %p, align 1
48+
// CHECK-SAME: !noundef !
49+
// CHECK-NOT: load
50+
MaybeUninit::new(p.assume_init_read())
51+
}

‎tests/mir-opt/lower_intrinsics.read_via_copy_primitive.LowerIntrinsics.diff

+27
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
- // MIR for `read_via_copy_primitive` before LowerIntrinsics
2+
+ // MIR for `read_via_copy_primitive` after LowerIntrinsics
3+
4+
fn read_via_copy_primitive(_1: &i32) -> i32 {
5+
debug r => _1; // in scope 0 at $DIR/lower_intrinsics.rs:+0:32: +0:33
6+
let mut _0: i32; // return place in scope 0 at $DIR/lower_intrinsics.rs:+0:44: +0:47
7+
let mut _2: *const i32; // in scope 0 at $DIR/lower_intrinsics.rs:+1:46: +1:47
8+
scope 1 {
9+
}
10+
11+
bb0: {
12+
StorageLive(_2); // scope 1 at $DIR/lower_intrinsics.rs:+1:46: +1:47
13+
_2 = &raw const (*_1); // scope 1 at $DIR/lower_intrinsics.rs:+1:46: +1:47
14+
- _0 = read_via_copy::<i32>(move _2) -> bb1; // scope 1 at $DIR/lower_intrinsics.rs:+1:14: +1:48
15+
- // mir::Constant
16+
- // + span: $DIR/lower_intrinsics.rs:85:14: 85:45
17+
- // + literal: Const { ty: unsafe extern "rust-intrinsic" fn(*const i32) -> i32 {read_via_copy::<i32>}, val: Value(<ZST>) }
18+
+ _0 = (*_2); // scope 1 at $DIR/lower_intrinsics.rs:+1:14: +1:48
19+
+ goto -> bb1; // scope 1 at $DIR/lower_intrinsics.rs:+1:14: +1:48
20+
}
21+
22+
bb1: {
23+
StorageDead(_2); // scope 1 at $DIR/lower_intrinsics.rs:+1:47: +1:48
24+
return; // scope 0 at $DIR/lower_intrinsics.rs:+2:2: +2:2
25+
}
26+
}
27+

‎tests/mir-opt/lower_intrinsics.read_via_copy_uninhabited.LowerIntrinsics.diff

+22
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
- // MIR for `read_via_copy_uninhabited` before LowerIntrinsics
2+
+ // MIR for `read_via_copy_uninhabited` after LowerIntrinsics
3+
4+
fn read_via_copy_uninhabited(_1: &Never) -> Never {
5+
debug r => _1; // in scope 0 at $DIR/lower_intrinsics.rs:+0:34: +0:35
6+
let mut _0: Never; // return place in scope 0 at $DIR/lower_intrinsics.rs:+0:48: +0:53
7+
let mut _2: *const Never; // in scope 0 at $DIR/lower_intrinsics.rs:+1:46: +1:47
8+
scope 1 {
9+
}
10+
11+
bb0: {
12+
StorageLive(_2); // scope 1 at $DIR/lower_intrinsics.rs:+1:46: +1:47
13+
_2 = &raw const (*_1); // scope 1 at $DIR/lower_intrinsics.rs:+1:46: +1:47
14+
- _0 = read_via_copy::<Never>(move _2); // scope 1 at $DIR/lower_intrinsics.rs:+1:14: +1:48
15+
- // mir::Constant
16+
- // + span: $DIR/lower_intrinsics.rs:90:14: 90:45
17+
- // + literal: Const { ty: unsafe extern "rust-intrinsic" fn(*const Never) -> Never {read_via_copy::<Never>}, val: Value(<ZST>) }
18+
+ _0 = (*_2); // scope 1 at $DIR/lower_intrinsics.rs:+1:14: +1:48
19+
+ unreachable; // scope 1 at $DIR/lower_intrinsics.rs:+1:14: +1:48
20+
}
21+
}
22+

‎tests/mir-opt/lower_intrinsics.rs

+12
Original file line numberDiff line numberDiff line change
@@ -79,3 +79,15 @@ pub fn with_overflow(a: i32, b: i32) {
7979
let _y = core::intrinsics::sub_with_overflow(a, b);
8080
let _z = core::intrinsics::mul_with_overflow(a, b);
8181
}
82+
83+
// EMIT_MIR lower_intrinsics.read_via_copy_primitive.LowerIntrinsics.diff
84+
pub fn read_via_copy_primitive(r: &i32) -> i32 {
85+
unsafe { core::intrinsics::read_via_copy(r) }
86+
}
87+
88+
// EMIT_MIR lower_intrinsics.read_via_copy_uninhabited.LowerIntrinsics.diff
89+
pub fn read_via_copy_uninhabited(r: &Never) -> Never {
90+
unsafe { core::intrinsics::read_via_copy(r) }
91+
}
92+
93+
pub enum Never {}

‎tests/ui/const-ptr/out_of_bounds_read.stderr

+3-3
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
error[E0080]: evaluation of constant value failed
22
--> $SRC_DIR/core/src/ptr/mod.rs:LL:COL
33
|
4-
= note: memory access failed: alloc5 has size 4, so pointer to 4 bytes starting at offset 4 is out-of-bounds
4+
= note: dereferencing pointer failed: alloc5 has size 4, so pointer to 4 bytes starting at offset 4 is out-of-bounds
55
|
66
note: inside `std::ptr::read::<u32>`
77
--> $SRC_DIR/core/src/ptr/mod.rs:LL:COL
@@ -14,7 +14,7 @@ LL | const _READ: u32 = unsafe { ptr::read(PAST_END_PTR) };
1414
error[E0080]: evaluation of constant value failed
1515
--> $SRC_DIR/core/src/ptr/mod.rs:LL:COL
1616
|
17-
= note: memory access failed: alloc5 has size 4, so pointer to 4 bytes starting at offset 4 is out-of-bounds
17+
= note: dereferencing pointer failed: alloc5 has size 4, so pointer to 4 bytes starting at offset 4 is out-of-bounds
1818
|
1919
note: inside `std::ptr::read::<u32>`
2020
--> $SRC_DIR/core/src/ptr/mod.rs:LL:COL
@@ -29,7 +29,7 @@ LL | const _CONST_READ: u32 = unsafe { PAST_END_PTR.read() };
2929
error[E0080]: evaluation of constant value failed
3030
--> $SRC_DIR/core/src/ptr/mod.rs:LL:COL
3131
|
32-
= note: memory access failed: alloc5 has size 4, so pointer to 4 bytes starting at offset 4 is out-of-bounds
32+
= note: dereferencing pointer failed: alloc5 has size 4, so pointer to 4 bytes starting at offset 4 is out-of-bounds
3333
|
3434
note: inside `std::ptr::read::<u32>`
3535
--> $SRC_DIR/core/src/ptr/mod.rs:LL:COL

‎tests/ui/consts/const-eval/ub-ref-ptr.stderr

+3-21
Original file line numberDiff line numberDiff line change
@@ -148,11 +148,11 @@ LL | const DATA_FN_PTR: fn() = unsafe { mem::transmute(&13) };
148148
HEX_DUMP
149149
}
150150

151-
error: accessing memory with alignment 1, but alignment 4 is required
151+
error[E0080]: evaluation of constant value failed
152152
--> $SRC_DIR/core/src/ptr/mod.rs:LL:COL
153153
|
154-
= warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
155-
= note: for more information, see issue #68585 <https://github.com/rust-lang/rust/issues/104616>
154+
= note: accessing memory with alignment 1, but alignment 4 is required
155+
|
156156
note: inside `std::ptr::read::<u32>`
157157
--> $SRC_DIR/core/src/ptr/mod.rs:LL:COL
158158
note: inside `ptr::const_ptr::<impl *const u32>::read`
@@ -162,25 +162,7 @@ note: inside `UNALIGNED_READ`
162162
|
163163
LL | ptr.read();
164164
| ^^^^^^^^^^
165-
= note: `#[deny(invalid_alignment)]` on by default
166165

167166
error: aborting due to 15 previous errors
168167

169168
For more information about this error, try `rustc --explain E0080`.
170-
Future incompatibility report: Future breakage diagnostic:
171-
error: accessing memory with alignment 1, but alignment 4 is required
172-
--> $SRC_DIR/core/src/ptr/mod.rs:LL:COL
173-
|
174-
= warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
175-
= note: for more information, see issue #68585 <https://github.com/rust-lang/rust/issues/104616>
176-
note: inside `std::ptr::read::<u32>`
177-
--> $SRC_DIR/core/src/ptr/mod.rs:LL:COL
178-
note: inside `ptr::const_ptr::<impl *const u32>::read`
179-
--> $SRC_DIR/core/src/ptr/const_ptr.rs:LL:COL
180-
note: inside `UNALIGNED_READ`
181-
--> $DIR/ub-ref-ptr.rs:67:5
182-
|
183-
LL | ptr.read();
184-
| ^^^^^^^^^^
185-
= note: `#[deny(invalid_alignment)]` on by default
186-

‎tests/ui/consts/issue-miri-1910.stderr

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
error[E0080]: evaluation of constant value failed
22
--> $SRC_DIR/core/src/ptr/mod.rs:LL:COL
33
|
4-
= note: unable to copy parts of a pointer from memory at ALLOC
4+
= note: unable to turn pointer into raw bytes
55
|
66
= help: this code performed an operation that depends on the underlying bytes representing a pointer
77
= help: the absolute address of a pointer is not known at compile-time, so such operations are not supported

0 commit comments

Comments
 (0)