Skip to content

Allow reading a *mut without an internal cast #111233

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

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion compiler/rustc_hir_analysis/src/check/intrinsic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -380,7 +380,7 @@ 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::read_via_copy => (2, vec![param(0)], param(1)),
sym::write_via_move => (1, vec![tcx.mk_mut_ptr(param(0)), param(0)], tcx.mk_unit()),

sym::discriminant_value => {
Expand Down
21 changes: 17 additions & 4 deletions library/core/src/intrinsics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,9 @@
)]
#![allow(missing_docs)]

use crate::marker::DiscriminantKind;
use crate::marker::Tuple;
#[cfg(not(bootstrap))]
use crate::marker::PointerLike;
use crate::marker::{DiscriminantKind, Tuple};
use crate::mem;

pub mod mir;
Expand Down Expand Up @@ -1429,7 +1430,7 @@ extern "rust-intrinsic" {
#[must_use = "returns a new pointer rather than modifying its argument"]
#[rustc_const_stable(feature = "const_ptr_offset", since = "1.61.0")]
#[rustc_nounwind]
pub fn offset<Ptr, Delta>(dst: Ptr, offset: Delta) -> Ptr;
pub fn offset<Ptr: PointerLike, Delta>(dst: Ptr, offset: Delta) -> Ptr;

/// The bootstrap version of this is more restricted.
#[cfg(bootstrap)]
Expand Down Expand Up @@ -2257,9 +2258,21 @@ extern "rust-intrinsic" {
/// This is an implementation detail of [`crate::ptr::read`] and should
/// not be used anywhere else. See its comments for why this exists.
///
/// This intrinsic can *only* be called where the pointer is a local without
/// `Ptr` must be some kind of pointer to `T`.
///
/// This intrinsic can *only* be called where the `ptr` is a local without
/// projections (`read_via_copy(ptr)`, not `read_via_copy(*ptr)`) so that it
/// trivially obeys runtime-MIR rules about derefs in operands.
///
/// Not meeting the above requirements may arbitrarily misbehave, and
/// per MCP#620 that's *not* a compiler bug.
#[cfg(not(bootstrap))]
#[rustc_const_unstable(feature = "const_ptr_read", issue = "80377")]
#[rustc_nounwind]
pub fn read_via_copy<Ptr: PointerLike, T>(ptr: Ptr) -> T;

/// The bootstrap version of this intrinsic is more limited.
#[cfg(bootstrap)]
#[rustc_const_unstable(feature = "const_ptr_read", issue = "80377")]
#[rustc_nounwind]
pub fn read_via_copy<T>(ptr: *const T) -> T;
Expand Down
12 changes: 8 additions & 4 deletions library/core/src/mem/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -738,7 +738,7 @@ pub const fn swap<T>(x: &mut T, y: &mut T) {
// tends to copy the whole thing to stack rather than doing it one part
// at a time, so instead treat them as one-element slices and piggy-back
// the slice optimizations that will split up the swaps.
if size_of::<T>() / align_of::<T>() > 4 {
if const { size_of::<T>() / align_of::<T>() > 4 } {
// SAFETY: exclusive references always point to one non-overlapping
// element and are non-null and properly aligned.
return unsafe { ptr::swap_nonoverlapping(x, y, 1) };
Expand Down Expand Up @@ -774,11 +774,14 @@ pub(crate) const fn swap_simple<T>(x: &mut T, y: &mut T) {
// asymmetry to the behaviour where one value went through read+write
// whereas the other was copied over by the intrinsic (see #94371).

let x: *mut T = x;
let y: *mut T = y;

// SAFETY: exclusive references are always valid to read/write,
// including being aligned, and nothing here panics so it's drop-safe.
unsafe {
let a = ptr::read(x);
let b = ptr::read(y);
let a = ptr::read_mut(x);
let b = ptr::read_mut(y);
ptr::write(x, b);
ptr::write(y, a);
}
Expand Down Expand Up @@ -909,11 +912,12 @@ pub fn take<T: Default>(dest: &mut T) -> T {
#[rustc_const_unstable(feature = "const_replace", issue = "83164")]
#[cfg_attr(not(test), rustc_diagnostic_item = "mem_replace")]
pub const fn replace<T>(dest: &mut T, src: T) -> T {
let dest: *mut T = dest;
// SAFETY: We read from `dest` but directly write `src` into it afterwards,
// such that the old value is not duplicated. Nothing is dropped and
// nothing here can panic.
unsafe {
let result = ptr::read(dest);
let result = ptr::read_mut(dest);
Copy link
Member

Choose a reason for hiding this comment

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

Compared to ptr::read, this just avoids a single MIR statement to turn the *mut into a *const, right? I am quite surprised that this would be a change worth doing. And if it is worth doing, a more holistic approach might be to just not do that cast and instead allow *mut everywhere that *const is allowed?

Copy link
Member Author

Choose a reason for hiding this comment

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

This does that a bit more for the method versions -- *mut has had read for 5 years now, and this PR makes that 1 statement shorter, which is certainly unimportant on its own but impacts every time it's mir-inlined. I suppose we can compatibly add impl Trait parameters to functions now, so we could consider letting ptr::read take more things too, but I don't know how disruptive that would be -- or if libs-api would even want it.

But I'm also not certain I understand what you were imagining with "allow *mut everywhere that *const is allowed". Could you sketch it out a bit more? I'm curious. (AFAIK MIR is typed enough that we can't omit these casts, even though they're NOPs?)

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking MIR optimizations could remove the *mut to *const cast that was introduced by MIR building, and our notion of "well-typed MIR" could be weakened to say that this is fine.

ptr::write(dest, src);
result
}
Expand Down
18 changes: 17 additions & 1 deletion library/core/src/ptr/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1168,7 +1168,23 @@ pub const unsafe fn read<T>(src: *const T) -> T {
"ptr::read requires that the pointer argument is aligned and non-null",
[T](src: *const T) => is_aligned_and_not_null(src)
);
crate::intrinsics::read_via_copy(src)
intrinsics::read_via_copy(src)
}
}

/// Like [`read`], but on `*mut` to avoid a `&raw const*`.
///
/// # Safety
///
/// Same as [`read`].
pub(crate) const unsafe fn read_mut<T>(src: *mut T) -> T {
// SAFETY: see `read` above
unsafe {
assert_unsafe_precondition!(
"ptr::read requires that the pointer argument is aligned and non-null",
[T](src: *mut T) => is_aligned_and_not_null(src)
);
intrinsics::read_via_copy(src)
}
}

Expand Down
4 changes: 2 additions & 2 deletions library/core/src/ptr/mut_ptr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1312,8 +1312,8 @@ impl<T: ?Sized> *mut T {
where
T: Sized,
{
// SAFETY: the caller must uphold the safety contract for ``.
unsafe { read(self) }
// SAFETY: the caller must uphold the safety contract for `read_mut`.
unsafe { read_mut(self) }
}

/// Performs a volatile read of the value from `self` without moving it. This
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ LL | pub fn catch_unwind<F: FnOnce() -> R + UnwindSafe, R>(f: F) -> Result<R> {
help: the conflicting tag <TAG> then transitioned from Reserved to Frozen due to a foreign read access at offsets [0x0..0x1]
--> RUSTLIB/core/src/ptr/mod.rs:LL:CC
|
LL | crate::intrinsics::read_via_copy(src)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
LL | intrinsics::read_via_copy(src)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
= help: this corresponds to a loss of write permissions
= note: BACKTRACE (of the first span):
= note: inside `main` at $DIR/fragile-data-race.rs:LL:CC
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
_4 = &raw const (*_1); // scope 1 at $DIR/lower_intrinsics.rs:+2:55: +2:56
- _3 = option_payload_ptr::<usize>(move _4) -> [return: bb1, unwind unreachable]; // scope 1 at $DIR/lower_intrinsics.rs:+2:18: +2:57
- // mir::Constant
- // + span: $DIR/lower_intrinsics.rs:137:18: 137:54
- // + span: $DIR/lower_intrinsics.rs:142:18: 142:54
- // + literal: Const { ty: unsafe extern "rust-intrinsic" fn(*const Option<usize>) -> *const usize {option_payload_ptr::<usize>}, val: Value(<ZST>) }
+ _3 = &raw const (((*_4) as Some).0: usize); // scope 1 at $DIR/lower_intrinsics.rs:+2:18: +2:57
+ goto -> bb1; // scope 1 at $DIR/lower_intrinsics.rs:+2:18: +2:57
Expand All @@ -37,7 +37,7 @@
_6 = &raw const (*_2); // scope 2 at $DIR/lower_intrinsics.rs:+3:55: +3:56
- _5 = option_payload_ptr::<String>(move _6) -> [return: bb2, unwind unreachable]; // scope 2 at $DIR/lower_intrinsics.rs:+3:18: +3:57
- // mir::Constant
- // + span: $DIR/lower_intrinsics.rs:138:18: 138:54
- // + span: $DIR/lower_intrinsics.rs:143:18: 143:54
- // + literal: Const { ty: unsafe extern "rust-intrinsic" fn(*const Option<String>) -> *const String {option_payload_ptr::<String>}, val: Value(<ZST>) }
+ _5 = &raw const (((*_6) as Some).0: std::string::String); // scope 2 at $DIR/lower_intrinsics.rs:+3:18: +3:57
+ goto -> bb2; // scope 2 at $DIR/lower_intrinsics.rs:+3:18: +3:57
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
_4 = _2; // scope 0 at $DIR/lower_intrinsics.rs:+1:33: +1:34
- _0 = offset::<*const i32, isize>(move _3, move _4) -> [return: bb1, unwind unreachable]; // scope 0 at $DIR/lower_intrinsics.rs:+1:5: +1:35
- // mir::Constant
- // + span: $DIR/lower_intrinsics.rs:144:5: 144:29
- // + span: $DIR/lower_intrinsics.rs:149:5: 149:29
- // + literal: Const { ty: unsafe extern "rust-intrinsic" fn(*const i32, isize) -> *const i32 {offset::<*const i32, isize>}, val: Value(<ZST>) }
+ _0 = Offset(move _3, move _4); // scope 0 at $DIR/lower_intrinsics.rs:+1:5: +1:35
+ goto -> bb1; // scope 0 at $DIR/lower_intrinsics.rs:+1:5: +1:35
Expand Down
30 changes: 30 additions & 0 deletions tests/mir-opt/lower_intrinsics.ptr_offset_mut.LowerIntrinsics.diff
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
- // MIR for `ptr_offset_mut` before LowerIntrinsics
+ // MIR for `ptr_offset_mut` after LowerIntrinsics

fn ptr_offset_mut(_1: *mut i32, _2: usize) -> *mut i32 {
debug p => _1; // in scope 0 at $DIR/lower_intrinsics.rs:+0:30: +0:31
debug d => _2; // in scope 0 at $DIR/lower_intrinsics.rs:+0:43: +0:44
let mut _0: *mut i32; // return place in scope 0 at $DIR/lower_intrinsics.rs:+0:56: +0:64
let mut _3: *mut i32; // in scope 0 at $DIR/lower_intrinsics.rs:+1:30: +1:31
let mut _4: usize; // in scope 0 at $DIR/lower_intrinsics.rs:+1:33: +1:34

bb0: {
StorageLive(_3); // scope 0 at $DIR/lower_intrinsics.rs:+1:30: +1:31
_3 = _1; // scope 0 at $DIR/lower_intrinsics.rs:+1:30: +1:31
StorageLive(_4); // scope 0 at $DIR/lower_intrinsics.rs:+1:33: +1:34
_4 = _2; // scope 0 at $DIR/lower_intrinsics.rs:+1:33: +1:34
- _0 = offset::<*mut i32, usize>(move _3, move _4) -> [return: bb1, unwind unreachable]; // scope 0 at $DIR/lower_intrinsics.rs:+1:5: +1:35
- // mir::Constant
- // + span: $DIR/lower_intrinsics.rs:154:5: 154:29
- // + literal: Const { ty: unsafe extern "rust-intrinsic" fn(*mut i32, usize) -> *mut i32 {offset::<*mut i32, usize>}, val: Value(<ZST>) }
+ _0 = Offset(move _3, move _4); // scope 0 at $DIR/lower_intrinsics.rs:+1:5: +1:35
+ goto -> bb1; // scope 0 at $DIR/lower_intrinsics.rs:+1:5: +1:35
Copy link
Member

Choose a reason for hiding this comment

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

Similarly to the other comment, why do we goto to bb1, where bb1 can be simply inlined?

Copy link
Contributor

Choose a reason for hiding this comment

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

Separation of concerns: LowerIntrinsics is just tasked with replacing the call terminator. Merging the blocks is left to SimplifyCfg pass.

}

bb1: {
StorageDead(_4); // scope 0 at $DIR/lower_intrinsics.rs:+1:34: +1:35
StorageDead(_3); // scope 0 at $DIR/lower_intrinsics.rs:+1:34: +1:35
return; // scope 0 at $DIR/lower_intrinsics.rs:+2:2: +2:2
}
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
- // MIR for `read_via_copy_mut` before LowerIntrinsics
+ // MIR for `read_via_copy_mut` after LowerIntrinsics

fn read_via_copy_mut(_1: *mut i64) -> i64 {
debug r => _1; // in scope 0 at $DIR/lower_intrinsics.rs:+0:33: +0:34
let mut _0: i64; // return place in scope 0 at $DIR/lower_intrinsics.rs:+0:49: +0:52
let mut _2: *mut i64; // in scope 0 at $DIR/lower_intrinsics.rs:+1:37: +1:38

bb0: {
StorageLive(_2); // scope 0 at $DIR/lower_intrinsics.rs:+1:37: +1:38
_2 = _1; // scope 0 at $DIR/lower_intrinsics.rs:+1:37: +1:38
- _0 = read_via_copy::<*mut i64, i64>(move _2) -> [return: bb1, unwind unreachable]; // scope 0 at $DIR/lower_intrinsics.rs:+1:5: +1:39
- // mir::Constant
- // + span: $DIR/lower_intrinsics.rs:129:5: 129:36
- // + literal: Const { ty: unsafe extern "rust-intrinsic" fn(*mut i64) -> i64 {read_via_copy::<*mut i64, i64>}, val: Value(<ZST>) }
+ _0 = (*_2); // scope 0 at $DIR/lower_intrinsics.rs:+1:5: +1:39
+ goto -> bb1; // scope 0 at $DIR/lower_intrinsics.rs:+1:5: +1:39
}

bb1: {
StorageDead(_2); // scope 0 at $DIR/lower_intrinsics.rs:+1:38: +1:39
return; // scope 0 at $DIR/lower_intrinsics.rs:+2:2: +2:2
}
}

Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,17 @@
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
let mut _2: &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::<i32>(move _2) -> [return: bb1, unwind unreachable]; // scope 1 at $DIR/lower_intrinsics.rs:+1:14: +1:48
_2 = _1; // scope 1 at $DIR/lower_intrinsics.rs:+1:46: +1:47
- _0 = read_via_copy::<&i32, i32>(move _2) -> [return: bb1, unwind unreachable]; // scope 1 at $DIR/lower_intrinsics.rs:+1:14: +1:48
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that this also allows directly reading a &, though it's not used in library in this PR.

- // mir::Constant
- // + span: $DIR/lower_intrinsics.rs:119:14: 119:45
- // + literal: Const { ty: unsafe extern "rust-intrinsic" fn(*const i32) -> i32 {read_via_copy::<i32>}, val: Value(<ZST>) }
- // + literal: Const { ty: unsafe extern "rust-intrinsic" fn(&i32) -> i32 {read_via_copy::<&i32, i32>}, val: Value(<ZST>) }
+ _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
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,17 @@
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
let mut _2: &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::<Never>(move _2) -> unwind unreachable; // scope 1 at $DIR/lower_intrinsics.rs:+1:14: +1:48
_2 = _1; // scope 1 at $DIR/lower_intrinsics.rs:+1:46: +1:47
- _0 = read_via_copy::<&Never, Never>(move _2) -> unwind unreachable; // scope 1 at $DIR/lower_intrinsics.rs:+1:14: +1:48
- // mir::Constant
- // + span: $DIR/lower_intrinsics.rs:124:14: 124:45
- // + literal: Const { ty: unsafe extern "rust-intrinsic" fn(*const Never) -> Never {read_via_copy::<Never>}, val: Value(<ZST>) }
- // + literal: Const { ty: unsafe extern "rust-intrinsic" fn(&Never) -> Never {read_via_copy::<&Never, Never>}, val: Value(<ZST>) }
+ unreachable; // scope 1 at $DIR/lower_intrinsics.rs:+1:14: +1:48
}
}
Expand Down
10 changes: 10 additions & 0 deletions tests/mir-opt/lower_intrinsics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,11 @@ pub fn read_via_copy_uninhabited(r: &Never) -> Never {
unsafe { core::intrinsics::read_via_copy(r) }
}

// EMIT_MIR lower_intrinsics.read_via_copy_mut.LowerIntrinsics.diff
pub unsafe fn read_via_copy_mut(r: *mut i64) -> i64 {
core::intrinsics::read_via_copy(r)
}

// EMIT_MIR lower_intrinsics.write_via_move_string.LowerIntrinsics.diff
pub fn write_via_move_string(r: &mut String, v: String) {
unsafe { core::intrinsics::write_via_move(r, v) }
Expand All @@ -143,3 +148,8 @@ pub fn option_payload(o: &Option<usize>, p: &Option<String>) {
pub unsafe fn ptr_offset(p: *const i32, d: isize) -> *const i32 {
core::intrinsics::offset(p, d)
}

// EMIT_MIR lower_intrinsics.ptr_offset_mut.LowerIntrinsics.diff
pub unsafe fn ptr_offset_mut(p: *mut i32, d: usize) -> *mut i32 {
core::intrinsics::offset(p, d)
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
_4 = move _2; // scope 1 at $DIR/lower_intrinsics.rs:+1:50: +1:51
- _0 = write_via_move::<String>(move _3, move _4) -> [return: bb1, unwind unreachable]; // scope 1 at $DIR/lower_intrinsics.rs:+1:14: +1:52
- // mir::Constant
- // + span: $DIR/lower_intrinsics.rs:129:14: 129:46
- // + span: $DIR/lower_intrinsics.rs:134:14: 134:46
- // + literal: Const { ty: unsafe extern "rust-intrinsic" fn(*mut String, String) {write_via_move::<String>}, val: Value(<ZST>) }
+ (*_3) = move _4; // scope 1 at $DIR/lower_intrinsics.rs:+1:14: +1:52
+ goto -> bb1; // scope 1 at $DIR/lower_intrinsics.rs:+1:14: +1:52
Expand Down
Loading