Skip to content

Change int-to-ptr transmute lowering back to inttoptr #147541

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+28 −8
Open

Change int-to-ptr transmute lowering back to inttoptr #147541

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions compiler/rustc_codegen_ssa/src/mir/rvalue.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1010,14 +1010,14 @@ pub(super) fn transmute_scalar<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
imm = match (from_scalar.primitive(), to_scalar.primitive()) {
(Int(..) | Float(_), Int(..) | Float(_)) => bx.bitcast(imm, to_backend_ty),
(Pointer(..), Pointer(..)) => bx.pointercast(imm, to_backend_ty),
(Int(..), Pointer(..)) => bx.ptradd(bx.const_null(bx.type_ptr()), imm),
(Int(..), Pointer(..)) => bx.inttoptr(imm, to_backend_ty),
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, maybe we could add a BuilderMethods::inttoptr_noprovenance for this, as that would give us a place to have a -Z flag that affects what it does so we could put it back to the better version more easily later?

Copy link
Member

Choose a reason for hiding this comment

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

(Maybe skip the -Z flag in this PR, if we want something that could go to beta, but if it existed we could test both ways in the codegen test.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Writing the actual implementation here took me about a minute, and I expect it will be similarly near-zero effort to swap it back again. Adding a helper would be more effort.

Copy link
Member

Choose a reason for hiding this comment

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

It just feels like a -Z mutable-noalias situation, where being able to test whether something is still broken without a new build is the reason to do it, not because the actual code change is hard.

Copy link
Member

Choose a reason for hiding this comment

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

A dedicated method would also allow backends which don't miscompile it to keep avoiding the provenance.

(Pointer(..), Int(..)) => {
// FIXME: this exposes the provenance, which shouldn't be necessary.
bx.ptrtoint(imm, to_backend_ty)
}
(Float(_), Pointer(..)) => {
let int_imm = bx.bitcast(imm, bx.cx().type_isize());
bx.ptradd(bx.const_null(bx.type_ptr()), int_imm)
bx.inttoptr(int_imm, to_backend_ty)
}
(Pointer(..), Float(_)) => {
// FIXME: this exposes the provenance, which shouldn't be necessary.
Expand Down
2 changes: 1 addition & 1 deletion tests/codegen-llvm/common_prim_int_ptr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
#[no_mangle]
pub fn insert_int(x: usize) -> Result<usize, Box<()>> {
// CHECK: start:
// CHECK-NEXT: %[[WO_PROV:.+]] = getelementptr i8, ptr null, [[USIZE:i[0-9]+]] %x
// CHECK-NEXT: %[[WO_PROV:.+]] = inttoptr [[USIZE:i[0-9]+]] %x to ptr
// CHECK-NEXT: %[[R:.+]] = insertvalue { [[USIZE]], ptr } { [[USIZE]] 0, ptr poison }, ptr %[[WO_PROV]], 1
// CHECK-NEXT: ret { [[USIZE]], ptr } %[[R]]
Ok(x)
Expand Down
2 changes: 1 addition & 1 deletion tests/codegen-llvm/enum/enum-aggregate.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ fn make_ok_ptr(x: NonNull<u16>) -> Result<NonNull<u16>, usize> {
fn make_ok_int(x: usize) -> Result<usize, NonNull<u16>> {
// CHECK-LABEL: { i64, ptr } @make_ok_int(i64 %x)
// CHECK-NEXT: start:
// CHECK-NEXT: %[[NOPROV:.+]] = getelementptr i8, ptr null, i64 %x
// CHECK-NEXT: %[[NOPROV:.+]] = inttoptr i64 %x to ptr
// CHECK-NEXT: %[[R:.+]] = insertvalue { i64, ptr } { i64 0, ptr poison }, ptr %[[NOPROV]], 1
// CHECK-NEXT: ret { i64, ptr } %[[R]]
Ok(x)
Expand Down
20 changes: 20 additions & 0 deletions tests/codegen-llvm/int-ptr-int-enum-miscompile.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// This is a regression test for https://github.com/rust-lang/rust/issues/147265.

//@ compile-flags: -Copt-level=3

#![crate_type = "lib"]

#[no_mangle]
pub fn mk_result(a: usize) -> Result<u8, *const u8> {
// CHECK-LABEL: @mk_result
// CHECK-NOT: unreachable
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: it's always good to have positive checks too, just in case the negative thing changes, so maybe

Suggested change
// CHECK-NOT: unreachable
// CHECK-NOT: unreachable
// CHECK: load i8,
// CHECK-NOT: unreachable

match g(a) {
Ok(b) => Ok(unsafe { *(b as *const u8) }),
Err(c) => Err(c),
}
}

#[cold]
fn g(a: usize) -> Result<usize, *const u8> {
Ok(a)
}
2 changes: 1 addition & 1 deletion tests/codegen-llvm/intrinsics/transmute-niched.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,7 @@ pub unsafe fn check_four_or_eight_to_nonnull(x: FourOrEight) -> NonNull<u8> {
// OPT: call void @llvm.assume(i1 %1)
// CHECK-NOT: icmp
// CHECK-NOT: assume
// CHECK: %[[RET:.+]] = getelementptr i8, ptr null, i64 %x
// CHECK: %[[RET:.+]] = inttoptr i64 %x to ptr
// CHECK-NEXT: ret ptr %[[RET]]

transmute(x)
Expand Down
4 changes: 2 additions & 2 deletions tests/codegen-llvm/intrinsics/transmute.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,7 @@ pub unsafe fn check_pair_with_bool(x: (u8, bool)) -> (bool, i8) {
pub unsafe fn check_float_to_pointer(x: f64) -> *const () {
// CHECK-NOT: alloca
// CHECK: %0 = bitcast double %x to i64
// CHECK: %_0 = getelementptr i8, ptr null, i64 %0
// CHECK: %_0 = inttoptr i64 %0 to ptr
// CHECK: ret ptr %_0
transmute(x)
}
Expand Down Expand Up @@ -378,7 +378,7 @@ pub unsafe fn check_issue_110005(x: (usize, bool)) -> Option<Box<[u8]>> {
// CHECK-LABEL: @check_pair_to_dst_ref(
#[no_mangle]
pub unsafe fn check_pair_to_dst_ref<'a>(x: (usize, usize)) -> &'a [u8] {
// CHECK: %_0.0 = getelementptr i8, ptr null, i64 %x.0
// CHECK: %_0.0 = inttoptr i64 %x.0 to ptr
// CHECK: %0 = icmp ne ptr %_0.0, null
// CHECK: call void @llvm.assume(i1 %0)
// CHECK: %1 = insertvalue { ptr, i64 } poison, ptr %_0.0, 0
Expand Down
2 changes: 1 addition & 1 deletion tests/codegen-llvm/transmute-scalar.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ pub fn ptr_to_int(p: *mut u16) -> usize {
}

// CHECK: define{{.*}}ptr @int_to_ptr([[USIZE]] %i)
// CHECK: %_0 = getelementptr i8, ptr null, [[USIZE]] %i
// CHECK: %_0 = inttoptr [[USIZE]] %i to ptr
// CHECK-NEXT: ret ptr %_0
#[no_mangle]
pub fn int_to_ptr(i: usize) -> *mut u16 {
Expand Down
Loading