Skip to content

Commit 75291bc

Browse files
committed
Auto merge of rust-lang#109035 - scottmcm:ptr-read-should-know-undef, r=WaffleLapkin,JakobDegen
Ensure `ptr::read` gets all the same LLVM `load` metadata that dereferencing does 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>`. Trying to narrow it down, it seems that was because `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. The root cause is that `ptr::read` is currently implemented via the *untyped* `copy_nonoverlapping`, and thus the `load` doesn't get any type-aware metadata: no `noundef`, no `!range`. This PR solves that by lowering `ptr::read(p)` to `copy *p` in MIR, for which the backends already do the right thing. Fortuitiously, this also improves the IR we give to LLVM for things like `mem::replace`, and fixes a couple of long-standing bugs where `ptr::read` on `Copy` types was worse than `*`ing them. Zulip conversation: <https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/Move.20array.3A.3AIntoIter.20to.20ManuallyDrop/near/341189936> cc `@erikdesjardins` `@JakobDegen` `@workingjubilee` `@the8472` Fixes rust-lang#106369 Fixes rust-lang#73258
2 parents 4125ee5 + 07af188 commit 75291bc

File tree

2 files changed

+55
-14
lines changed

2 files changed

+55
-14
lines changed

core/src/intrinsics.rs

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2020,6 +2020,16 @@ extern "rust-intrinsic" {
20202020
#[rustc_safe_intrinsic]
20212021
pub fn saturating_sub<T: Copy>(a: T, b: T) -> T;
20222022

2023+
/// This is an implementation detail of [`crate::ptr::read`] and should
2024+
/// not be used anywhere else. See its comments for why this exists.
2025+
///
2026+
/// This intrinsic can *only* be called where the argument is a local without
2027+
/// projections (`read_via_copy(p)`, not `read_via_copy(*p)`) so that it
2028+
/// trivially obeys runtime-MIR rules about derefs in operands.
2029+
#[cfg(not(bootstrap))]
2030+
#[rustc_const_unstable(feature = "const_ptr_read", issue = "80377")]
2031+
pub fn read_via_copy<T>(p: *const T) -> T;
2032+
20232033
/// Returns the value of the discriminant for the variant in 'v';
20242034
/// if `T` has no discriminant, returns `0`.
20252035
///

core/src/ptr/mod.rs

Lines changed: 45 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1135,27 +1135,58 @@ pub const unsafe fn replace<T>(dst: *mut T, mut src: T) -> T {
11351135
#[rustc_const_unstable(feature = "const_ptr_read", issue = "80377")]
11361136
#[cfg_attr(miri, track_caller)] // even without panics, this helps for Miri backtraces
11371137
pub const unsafe fn read<T>(src: *const T) -> T {
1138-
// We are calling the intrinsics directly to avoid function calls in the generated code
1139-
// as `intrinsics::copy_nonoverlapping` is a wrapper function.
1140-
extern "rust-intrinsic" {
1141-
#[rustc_const_stable(feature = "const_intrinsic_copy", since = "1.63.0")]
1142-
fn copy_nonoverlapping<T>(src: *const T, dst: *mut T, count: usize);
1143-
}
1138+
// It would be semantically correct to implement this via `copy_nonoverlapping`
1139+
// and `MaybeUninit`, as was done before PR #109035. Calling `assume_init`
1140+
// provides enough information to know that this is a typed operation.
11441141

1145-
let mut tmp = MaybeUninit::<T>::uninit();
1146-
// SAFETY: the caller must guarantee that `src` is valid for reads.
1147-
// `src` cannot overlap `tmp` because `tmp` was just allocated on
1148-
// the stack as a separate allocated object.
1142+
// However, as of March 2023 the compiler was not capable of taking advantage
1143+
// of that information. Thus the implementation here switched to an intrinsic,
1144+
// which lowers to `_0 = *src` in MIR, to address a few issues:
11491145
//
1150-
// Also, since we just wrote a valid value into `tmp`, it is guaranteed
1151-
// to be properly initialized.
1146+
// - Using `MaybeUninit::assume_init` after a `copy_nonoverlapping` was not
1147+
// turning the untyped copy into a typed load. As such, the generated
1148+
// `load` in LLVM didn't get various metadata, such as `!range` (#73258),
1149+
// `!nonnull`, and `!noundef`, resulting in poorer optimization.
1150+
// - Going through the extra local resulted in multiple extra copies, even
1151+
// in optimized MIR. (Ignoring StorageLive/Dead, the intrinsic is one
1152+
// MIR statement, while the previous implementation was eight.) LLVM
1153+
// could sometimes optimize them away, but because `read` is at the core
1154+
// of so many things, not having them in the first place improves what we
1155+
// hand off to the backend. For example, `mem::replace::<Big>` previously
1156+
// emitted 4 `alloca` and 6 `memcpy`s, but is now 1 `alloc` and 3 `memcpy`s.
1157+
// - In general, this approach keeps us from getting any more bugs (like
1158+
// #106369) that boil down to "`read(p)` is worse than `*p`", as this
1159+
// makes them look identical to the backend (or other MIR consumers).
1160+
//
1161+
// Future enhancements to MIR optimizations might well allow this to return
1162+
// to the previous implementation, rather than using an intrinsic.
1163+
1164+
// SAFETY: the caller must guarantee that `src` is valid for reads.
11521165
unsafe {
11531166
assert_unsafe_precondition!(
11541167
"ptr::read requires that the pointer argument is aligned and non-null",
11551168
[T](src: *const T) => is_aligned_and_not_null(src)
11561169
);
1157-
copy_nonoverlapping(src, tmp.as_mut_ptr(), 1);
1158-
tmp.assume_init()
1170+
1171+
#[cfg(bootstrap)]
1172+
{
1173+
// We are calling the intrinsics directly to avoid function calls in the
1174+
// generated code as `intrinsics::copy_nonoverlapping` is a wrapper function.
1175+
extern "rust-intrinsic" {
1176+
#[rustc_const_stable(feature = "const_intrinsic_copy", since = "1.63.0")]
1177+
fn copy_nonoverlapping<T>(src: *const T, dst: *mut T, count: usize);
1178+
}
1179+
1180+
// `src` cannot overlap `tmp` because `tmp` was just allocated on
1181+
// the stack as a separate allocated object.
1182+
let mut tmp = MaybeUninit::<T>::uninit();
1183+
copy_nonoverlapping(src, tmp.as_mut_ptr(), 1);
1184+
tmp.assume_init()
1185+
}
1186+
#[cfg(not(bootstrap))]
1187+
{
1188+
crate::intrinsics::read_via_copy(src)
1189+
}
11591190
}
11601191
}
11611192

0 commit comments

Comments
 (0)