Lower transmutes from int to pointer type as gep on null

[saethlin]

I thought of this while looking at #121242. See that PR's description for why this lowering is preferable.

The UI test that's being changed here crashes without changing the transmutes into casts. Based on that, this PR should not be merged without a crater build-and-test run.

[rustbot]

r? @fmease

rustbot has assigned @fmease.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[saethlin]

@bors try @rust-timer queue

[bors]

⌛ Trying commit 903dd09 with merge d073071...

[bors]

☀️ Try build successful - checks-actions
Build commit: d073071 (d073071d77ce0f93b4fd8cc567a1e2b9e1b22126)

[rust-timer]

Finished benchmarking commit (d073071): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.5%	[3.5%, 3.5%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.1%	[2.1%, 2.2%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.1%	[0.1%, 0.1%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.1%	[0.1%, 0.1%]	1

Bootstrap: 640.336s -> 641.418s (0.17%)
Artifact size: 308.82 MiB -> 308.83 MiB (0.00%)

[saethlin]

@craterbot run mode=build-and-test

[craterbot]

👌 Experiment pr-121282 created and queued.
🤖 Automatically detected try build d073071
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[saethlin]

@craterbot abort

[craterbot]

🗑️ Experiment pr-121282 deleted!

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[saethlin]

@craterbot run mode=build-and-test start=master#61223975d46f794466efa832bc7562b9707ecc46+rustflags=-Copt-level=3 end=try#d073071d77ce0f93b4fd8cc567a1e2b9e1b22126+rustflags=-Copt-level=3

[craterbot]

👌 Experiment pr-121282 created and queued.
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[scottmcm]

I think to make this pass on non-64-bit you need

Suggested change

	// CHECK: %_0 = getelementptr i8, ptr null, i64 %i
	// CHECK: %_0 = getelementptr i8, ptr null, [[USIZE]] %i

[craterbot]

🚧 Experiment pr-121282 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[bors]

☀️ Test successful - checks-actions
Approved by: scottmcm
Pushing 0fa7fea to master...

[rust-timer]

Finished benchmarking commit (0fa7fea): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.6%	[2.6%, 2.6%]	1
Regressions ❌ (secondary)	2.8%	[2.8%, 2.8%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-7.8%	[-7.8%, -7.8%]	1
All ❌✅ (primary)	2.6%	[2.6%, 2.6%]	1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 672.319s -> 672.448s (0.02%)
Artifact size: 310.04 MiB -> 310.05 MiB (0.00%)

[Liorst4]

Is there a difference between casting an isize to a pointer using as and using std::mem::transmute?

@saethlin @scottmcm

When this change was proposed it crashed on one of my projects #121282 (comment)

@saethlin discussed this with me here: Liorst4/liorforth#1

The discussion didn't go very deep into the issue, and it left me kind of confused.
I assumed its a bug in the patch and @saethlin was going to fix it before pushing it into master.

Now when I run the test in release the test crashes.
To fix it, all I needed to do is change a std::mem::transmute cast into an as cast.
Liorst4/liorforth@fba984a
But I don't really understand why, shouldn't these be the same thing? Is there something I'm missing? Alignment issues?

Code that fails now

type Byte = u8;

type Cell = isize;

#[repr(packed(1))]
struct CountedString {
    len: Byte,
    data: [Byte; 0],
}

#[no_mangle]
fn test_breaks() {
    let buffer: [u8; 6] = [5, b'h', b'e', b'l', b'l', b'o'];
    let counted_string_address: Cell = (&buffer).as_ptr() as Cell;
    
    // Using `transmute` to cast
    let counted_string: &CountedString = unsafe {
        std::mem::transmute::<Cell, *const CountedString>(counted_string_address)
            .as_ref()
            .unwrap()
    };

    let counted_string_as_slice: &[u8] = unsafe {
        std::slice::from_raw_parts(counted_string.data.as_ptr(), counted_string.len as usize)
    };
    let counted_string_as_rust_string = std::str::from_utf8(counted_string_as_slice).unwrap();
    println!("{}", counted_string_as_rust_string);
}

#[no_mangle]
fn test_works() {
    let buffer: [u8; 6] = [5, b'h', b'e', b'l', b'l', b'o'];
    let counted_string_address: Cell = (&buffer).as_ptr() as Cell;
    
    // Using `as` to cast
    let counted_string: &CountedString = unsafe {
        (counted_string_address as *const CountedString)
            .as_ref()
            .unwrap()
    };

    let counted_string_as_slice: &[u8] = unsafe {
        std::slice::from_raw_parts(counted_string.data.as_ptr(), counted_string.len as usize)
    };
    let counted_string_as_rust_string = std::str::from_utf8(counted_string_as_slice).unwrap();
    println!("{}", counted_string_as_rust_string);
}

pub fn main() {
    test_works();
    test_breaks();
}

https://gcc.godbolt.org/z/b8ba9Y7G5

[saethlin]

Yes there is a difference. To quote the RFC https://rust-lang.github.io/rfcs/3559-rust-has-provenance.html

Furthermore, the section on “Integer types” is extended to say that integers do not have provenance, and therefore transmuting (via transmute or type punning) from a pointer to an integer is a lossy operation and might even be UB. (The exact semantics of that operation involve some subtle trade-offs and are not decided by this RFC.)

as is a different operation from transmute for other types as well. Conversions between integers and floating point, for example.

[Liorst4]

Yes there is a difference. To quote the RFC https://rust-lang.github.io/rfcs/3559-rust-has-provenance.html

Furthermore, the section on “Integer types” is extended to say that integers do not have provenance, and therefore transmuting (via transmute or type punning) from a pointer to an integer is a lossy operation and might even be UB. (The exact semantics of that operation involve some subtle trade-offs and are not decided by this RFC.)

as is a different operation from transmute for other types as well. Conversions between integers and floating point, for example.

OK, thanks, this clears stuff up 👍

[scottmcm]

@Liorst4 Reminder that you can also confirm this using Miri. This PR changed only codegen, which doesn't affect Miri, but your test_breaks example also fails in Miri: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=46c3f8c80ae90b5fb22dd59023628e88

error: Undefined Behavior: out-of-bounds pointer use: 0x25d44[noalloc] is a dangling pointer (it has no provenance)
   --> /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/const_ptr.rs:358:57
    |
358 |         if self.is_null() { None } else { unsafe { Some(&*self) } }
    |                                                         ^^^^^^ out-of-bounds pointer use: 0x25d44[noalloc] is a dangling pointer (it has no provenance)
    |
    = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
    = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
    = note: BACKTRACE:
    = note: inside `std::ptr::const_ptr::<impl *const CountedString>::as_ref::<'_>` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/const_ptr.rs:358:57: 358:63
note: inside `test_breaks`
   --> src/main.rs:18:9
    |
18  | /         std::mem::transmute::<Cell, *const CountedString>(counted_string_address)
19  | |             .as_ref()
    | |_____________________^
note: inside `main`
   --> src/main.rs:31:5
    |
31  |     test_breaks();
    |     ^^^^^^^^^^^^^

(Miri has diagnosed this for a long time; this PR just means that LLVM also knows about the UB.)