Skip to content
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

Use MIR's Offset for pointer add too #110837

Merged
merged 2 commits into from
Apr 28, 2023
Merged
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
11 changes: 9 additions & 2 deletions compiler/rustc_codegen_ssa/src/mir/rvalue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -819,8 +819,15 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
.builtin_deref(true)
.unwrap_or_else(|| bug!("deref of non-pointer {:?}", input_ty))
.ty;
let llty = bx.cx().backend_type(bx.cx().layout_of(pointee_type));
bx.inbounds_gep(llty, lhs, &[rhs])
let pointee_layout = bx.cx().layout_of(pointee_type);
if pointee_layout.is_zst() {
// `Offset` works in terms of the size of pointee,
// so offsetting a pointer to ZST is a noop.
lhs
} else {
let llty = bx.cx().backend_type(pointee_layout);
bx.inbounds_gep(llty, lhs, &[rhs])
}
}
mir::BinOp::Shl => common::build_unchecked_lshift(bx, lhs, rhs),
mir::BinOp::Shr => common::build_unchecked_rshift(bx, input_ty, lhs, rhs),
Expand Down
3 changes: 2 additions & 1 deletion compiler/rustc_hir_analysis/src/check/intrinsic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,8 @@ pub fn check_intrinsic_type(tcx: TyCtxt<'_>, it: &hir::ForeignItem<'_>) {

sym::type_name => (1, Vec::new(), tcx.mk_static_str()),
sym::type_id => (1, Vec::new(), tcx.types.u64),
sym::offset | sym::arith_offset => (
sym::offset => (2, vec![param(0), param(1)], param(0)),
sym::arith_offset => (
Copy link
Member

Choose a reason for hiding this comment

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

Is arith_offset not changed because we don't have a version of it in MIR?

Makes me wander if we should try intrinsicing other offset like functions like {byte,wrapping,wrapping_byte}_{add,sub,offset}

Copy link
Member Author

Choose a reason for hiding this comment

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

@WaffleLapkin The weird thing about byte_add is that it works on fat pointers. So it ends up quite a bit more complex than offset. I'm not sure if that's worth doing as an intrinsic or not. For the data part of it, cast-then-offset-then-cast of course works fine.

As for sub, maybe we should start by changing it from wrapping_neg to unchecked_neg, especially after #112238...

Copy link
Member

Choose a reason for hiding this comment

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

Hm, but wouldn't byte_add be just self.data += x? Doesn't sound like it's too hard for an intrinsic...

Copy link
Member Author

Choose a reason for hiding this comment

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

@WaffleLapkin Is perhaps your email client catching up on some ancient messages? Or are looking at old stuff again?

Copy link
Member

Choose a reason for hiding this comment

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

@scottmcm I cleared up old notifications that I were putting off for a long time 😄

1,
vec![
tcx.mk_ptr(ty::TypeAndMut { ty: param(0), mutbl: hir::Mutability::Not }),
Expand Down
12 changes: 12 additions & 0 deletions library/core/src/intrinsics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1413,6 +1413,10 @@ extern "rust-intrinsic" {
/// This is implemented as an intrinsic to avoid converting to and from an
/// integer, since the conversion would throw away aliasing information.
///
/// This can only be used with `Ptr` as a raw pointer type (`*mut` or `*const`)
/// to a `Sized` pointee and with `Delta` as `usize` or `isize`. Any other
/// instantiations may arbitrarily misbehave, and that's *not* a compiler bug.
///
/// # Safety
///
/// Both the starting and resulting pointer must be either in bounds or one
Expand All @@ -1421,6 +1425,14 @@ extern "rust-intrinsic" {
/// returned value will result in undefined behavior.
///
/// The stabilized version of this intrinsic is [`pointer::offset`].
#[cfg(not(bootstrap))]
#[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;
Copy link
Member

@RalfJung RalfJung Sep 11, 2024

Choose a reason for hiding this comment

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

When changing intrinsics, please make sure to always Cc the opsem and miri teams! These are language primitives, a bunch of places need to typically be adjusted to make everything fit again. In this case, this PR made the Miri logic all wrong since it will still always assume the offset to be an isize -- thus missing UB, which is a critical bug in Miri.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry about that, Ralf! I'll keep it in mind next time.

When the validator was explicitly allowing both isize and usize as argument types, I'd assumed it was handling the usize in the "obvious" way, but clearly I should have checked.


/// The bootstrap version of this is more restricted.
#[cfg(bootstrap)]
#[must_use = "returns a new pointer rather than modifying its argument"]
#[rustc_const_stable(feature = "const_ptr_offset", since = "1.61.0")]
#[rustc_nounwind]
Expand Down
10 changes: 9 additions & 1 deletion library/core/src/ptr/const_ptr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -916,8 +916,16 @@ impl<T: ?Sized> *const T {
where
T: Sized,
{
#[cfg(bootstrap)]
// SAFETY: the caller must uphold the safety contract for `offset`.
unsafe { self.offset(count as isize) }
unsafe {
self.offset(count as isize)
}
#[cfg(not(bootstrap))]
// SAFETY: the caller must uphold the safety contract for `offset`.
unsafe {
intrinsics::offset(self, count)
}
}

/// Calculates the offset from a pointer in bytes (convenience for `.byte_offset(count as isize)`).
Expand Down
22 changes: 20 additions & 2 deletions library/core/src/ptr/mut_ptr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -473,10 +473,20 @@ impl<T: ?Sized> *mut T {
where
T: Sized,
{
#[cfg(bootstrap)]
// SAFETY: the caller must uphold the safety contract for `offset`.
// The obtained pointer is valid for writes since the caller must
// guarantee that it points to the same allocated object as `self`.
unsafe { intrinsics::offset(self, count) as *mut T }
unsafe {
intrinsics::offset(self, count) as *mut T
}
#[cfg(not(bootstrap))]
// SAFETY: the caller must uphold the safety contract for `offset`.
// The obtained pointer is valid for writes since the caller must
// guarantee that it points to the same allocated object as `self`.
unsafe {
intrinsics::offset(self, count)
}
}

/// Calculates the offset from a pointer in bytes.
Expand Down Expand Up @@ -1016,8 +1026,16 @@ impl<T: ?Sized> *mut T {
where
T: Sized,
{
#[cfg(bootstrap)]
// SAFETY: the caller must uphold the safety contract for `offset`.
unsafe {
self.offset(count as isize)
}
#[cfg(not(bootstrap))]
// SAFETY: the caller must uphold the safety contract for `offset`.
unsafe { self.offset(count as isize) }
unsafe {
intrinsics::offset(self, count)
}
}

/// Calculates the offset from a pointer in bytes (convenience for `.byte_offset(count as isize)`).
Expand Down
2 changes: 1 addition & 1 deletion src/doc/unstable-book/src/language-features/intrinsics.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ via a declaration like
extern "rust-intrinsic" {
fn transmute<T, U>(x: T) -> U;

fn offset<T>(dst: *const T, offset: isize) -> *const T;
fn arith_offset<T>(dst: *const T, offset: isize) -> *const T;
}
```

Expand Down
34 changes: 34 additions & 0 deletions tests/codegen/intrinsics/offset.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
// compile-flags: -O -C no-prepopulate-passes
// min-llvm-version: 15.0 (because we're using opaque pointers)

#![crate_type = "lib"]
#![feature(core_intrinsics)]

use std::intrinsics::offset;

// CHECK-LABEL: ptr @offset_zst
// CHECK-SAME: (ptr noundef %p, [[SIZE:i[0-9]+]] noundef %d)
#[no_mangle]
pub unsafe fn offset_zst(p: *const (), d: usize) -> *const () {
// CHECK-NOT: getelementptr
// CHECK: ret ptr %p
offset(p, d)
}

// CHECK-LABEL: ptr @offset_isize
// CHECK-SAME: (ptr noundef %p, [[SIZE]] noundef %d)
#[no_mangle]
pub unsafe fn offset_isize(p: *const u32, d: isize) -> *const u32 {
// CHECK: %[[R:.*]] = getelementptr inbounds i32, ptr %p, [[SIZE]] %d
// CHECK-NEXT: ret ptr %[[R]]
offset(p, d)
}

// CHECK-LABEL: ptr @offset_usize
// CHECK-SAME: (ptr noundef %p, [[SIZE]] noundef %d)
#[no_mangle]
pub unsafe fn offset_usize(p: *const u64, d: usize) -> *const u64 {
// CHECK: %[[R:.*]] = getelementptr inbounds i64, ptr %p, [[SIZE]] %d
// CHECK-NEXT: ret ptr %[[R]]
offset(p, d)
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,10 @@
_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::<i32>(move _3, move _4) -> [return: bb1, unwind unreachable]; // scope 0 at $DIR/lower_intrinsics.rs:+1:5: +1:35
- _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:140:5: 140:29
- // + literal: Const { ty: unsafe extern "rust-intrinsic" fn(*const i32, isize) -> *const i32 {offset::<i32>}, val: Value(<ZST>) }
- // + 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
27 changes: 27 additions & 0 deletions tests/mir-opt/pre-codegen/slice_index.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
// compile-flags: -O -C debuginfo=0 -Zmir-opt-level=2
// only-64bit
// ignore-debug

#![crate_type = "lib"]

use std::ops::Range;

// EMIT_MIR slice_index.slice_index_usize.PreCodegen.after.mir
pub fn slice_index_usize(slice: &[u32], index: usize) -> u32 {
slice[index]
}

// EMIT_MIR slice_index.slice_get_mut_usize.PreCodegen.after.mir
pub fn slice_get_mut_usize(slice: &mut [u32], index: usize) -> Option<&mut u32> {
slice.get_mut(index)
}

// EMIT_MIR slice_index.slice_index_range.PreCodegen.after.mir
pub fn slice_index_range(slice: &[u32], index: Range<usize>) -> &[u32] {
&slice[index]
}

// EMIT_MIR slice_index.slice_get_unchecked_mut_range.PreCodegen.after.mir
pub unsafe fn slice_get_unchecked_mut_range(slice: &mut [u32], index: Range<usize>) -> &mut [u32] {
slice.get_unchecked_mut(index)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
// MIR for `slice_get_mut_usize` after PreCodegen

fn slice_get_mut_usize(_1: &mut [u32], _2: usize) -> Option<&mut u32> {
debug slice => _1; // in scope 0 at $DIR/slice_index.rs:+0:28: +0:33
debug index => _2; // in scope 0 at $DIR/slice_index.rs:+0:47: +0:52
let mut _0: std::option::Option<&mut u32>; // return place in scope 0 at $DIR/slice_index.rs:+0:64: +0:80
scope 1 (inlined core::slice::<impl [u32]>::get_mut::<usize>) { // at $DIR/slice_index.rs:16:11: 16:25
debug self => _1; // in scope 1 at $SRC_DIR/core/src/slice/mod.rs:LL:COL
debug index => _2; // in scope 1 at $SRC_DIR/core/src/slice/mod.rs:LL:COL
scope 2 (inlined <usize as SliceIndex<[u32]>>::get_mut) { // at $SRC_DIR/core/src/slice/mod.rs:LL:COL
debug self => _2; // in scope 2 at $SRC_DIR/core/src/slice/index.rs:LL:COL
debug slice => _1; // in scope 2 at $SRC_DIR/core/src/slice/index.rs:LL:COL
let mut _3: bool; // in scope 2 at $SRC_DIR/core/src/slice/index.rs:LL:COL
let mut _4: usize; // in scope 2 at $SRC_DIR/core/src/slice/index.rs:LL:COL
let mut _5: &[u32]; // in scope 2 at $SRC_DIR/core/src/slice/index.rs:LL:COL
let mut _6: &mut u32; // in scope 2 at $SRC_DIR/core/src/slice/index.rs:LL:COL
let mut _7: *mut u32; // in scope 2 at $SRC_DIR/core/src/slice/index.rs:LL:COL
let mut _8: *mut [u32]; // in scope 2 at $SRC_DIR/core/src/slice/index.rs:LL:COL
scope 3 {
scope 4 (inlined <usize as SliceIndex<[u32]>>::get_unchecked_mut) { // at $SRC_DIR/core/src/slice/index.rs:LL:COL
debug self => _2; // in scope 4 at $SRC_DIR/core/src/slice/index.rs:LL:COL
debug slice => _8; // in scope 4 at $SRC_DIR/core/src/slice/index.rs:LL:COL
let mut _9: *mut u32; // in scope 4 at $SRC_DIR/core/src/slice/index.rs:LL:COL
let mut _10: usize; // in scope 4 at $SRC_DIR/core/src/intrinsics.rs:LL:COL
let mut _11: *mut [u32]; // in scope 4 at $SRC_DIR/core/src/intrinsics.rs:LL:COL
scope 5 {
debug this => _2; // in scope 5 at $SRC_DIR/core/src/slice/index.rs:LL:COL
scope 6 {
scope 7 (inlined <usize as SliceIndex<[T]>>::get_unchecked_mut::runtime::<u32>) { // at $SRC_DIR/core/src/intrinsics.rs:LL:COL
debug this => _10; // in scope 7 at $SRC_DIR/core/src/intrinsics.rs:LL:COL
debug slice => _11; // in scope 7 at $SRC_DIR/core/src/intrinsics.rs:LL:COL
scope 8 (inlined ptr::mut_ptr::<impl *mut [u32]>::len) { // at $SRC_DIR/core/src/slice/index.rs:LL:COL
debug self => _11; // in scope 8 at $SRC_DIR/core/src/ptr/mut_ptr.rs:LL:COL
let mut _12: *const [u32]; // in scope 8 at $SRC_DIR/core/src/ptr/mut_ptr.rs:LL:COL
scope 9 (inlined std::ptr::metadata::<[u32]>) { // at $SRC_DIR/core/src/ptr/mut_ptr.rs:LL:COL
debug ptr => _12; // in scope 9 at $SRC_DIR/core/src/ptr/metadata.rs:LL:COL
scope 10 {
}
}
}
}
scope 11 (inlined ptr::mut_ptr::<impl *mut [u32]>::as_mut_ptr) { // at $SRC_DIR/core/src/slice/index.rs:LL:COL
debug self => _8; // in scope 11 at $SRC_DIR/core/src/ptr/mut_ptr.rs:LL:COL
}
scope 12 (inlined ptr::mut_ptr::<impl *mut u32>::add) { // at $SRC_DIR/core/src/slice/index.rs:LL:COL
debug self => _9; // in scope 12 at $SRC_DIR/core/src/ptr/mut_ptr.rs:LL:COL
debug count => _2; // in scope 12 at $SRC_DIR/core/src/ptr/mut_ptr.rs:LL:COL
scope 13 {
}
}
}
}
}
}
}
}

bb0: {
StorageLive(_6); // scope 1 at $SRC_DIR/core/src/slice/mod.rs:LL:COL
StorageLive(_3); // scope 2 at $SRC_DIR/core/src/slice/index.rs:LL:COL
StorageLive(_4); // scope 2 at $SRC_DIR/core/src/slice/index.rs:LL:COL
StorageLive(_5); // scope 2 at $SRC_DIR/core/src/slice/index.rs:LL:COL
_5 = &(*_1); // scope 2 at $SRC_DIR/core/src/slice/index.rs:LL:COL
_4 = Len((*_5)); // scope 2 at $SRC_DIR/core/src/slice/index.rs:LL:COL
StorageDead(_5); // scope 2 at $SRC_DIR/core/src/slice/index.rs:LL:COL
_3 = Lt(_2, move _4); // scope 2 at $SRC_DIR/core/src/slice/index.rs:LL:COL
StorageDead(_4); // scope 2 at $SRC_DIR/core/src/slice/index.rs:LL:COL
switchInt(move _3) -> [0: bb2, otherwise: bb1]; // scope 2 at $SRC_DIR/core/src/slice/index.rs:LL:COL
}

bb1: {
StorageLive(_7); // scope 3 at $SRC_DIR/core/src/slice/index.rs:LL:COL
StorageLive(_8); // scope 3 at $SRC_DIR/core/src/slice/index.rs:LL:COL
_8 = &raw mut (*_1); // scope 3 at $SRC_DIR/core/src/slice/index.rs:LL:COL
StorageLive(_10); // scope 3 at $SRC_DIR/core/src/slice/index.rs:LL:COL
StorageLive(_11); // scope 3 at $SRC_DIR/core/src/slice/index.rs:LL:COL
StorageLive(_12); // scope 3 at $SRC_DIR/core/src/slice/index.rs:LL:COL
StorageLive(_9); // scope 6 at $SRC_DIR/core/src/slice/index.rs:LL:COL
_9 = _8 as *mut u32 (PtrToPtr); // scope 11 at $SRC_DIR/core/src/ptr/mut_ptr.rs:LL:COL
_7 = Offset(_9, _2); // scope 13 at $SRC_DIR/core/src/ptr/mut_ptr.rs:LL:COL
StorageDead(_9); // scope 6 at $SRC_DIR/core/src/slice/index.rs:LL:COL
StorageDead(_12); // scope 3 at $SRC_DIR/core/src/slice/index.rs:LL:COL
StorageDead(_11); // scope 3 at $SRC_DIR/core/src/slice/index.rs:LL:COL
StorageDead(_10); // scope 3 at $SRC_DIR/core/src/slice/index.rs:LL:COL
StorageDead(_8); // scope 3 at $SRC_DIR/core/src/slice/index.rs:LL:COL
_6 = &mut (*_7); // scope 3 at $SRC_DIR/core/src/slice/index.rs:LL:COL
_0 = Option::<&mut u32>::Some(_6); // scope 3 at $SRC_DIR/core/src/slice/index.rs:LL:COL
StorageDead(_7); // scope 2 at $SRC_DIR/core/src/slice/index.rs:LL:COL
goto -> bb3; // scope 2 at $SRC_DIR/core/src/slice/index.rs:LL:COL
}

bb2: {
_0 = const Option::<&mut u32>::None; // scope 2 at $SRC_DIR/core/src/slice/index.rs:LL:COL
// mir::Constant
// + span: no-location
// + literal: Const { ty: Option<&mut u32>, val: Value(Scalar(0x0000000000000000)) }
goto -> bb3; // scope 2 at $SRC_DIR/core/src/slice/index.rs:LL:COL
}

bb3: {
StorageDead(_3); // scope 2 at $SRC_DIR/core/src/slice/index.rs:LL:COL
StorageDead(_6); // scope 1 at $SRC_DIR/core/src/slice/mod.rs:LL:COL
return; // scope 0 at $DIR/slice_index.rs:+2:2: +2:2
}
}
Loading