Lower transmutes from int to pointer type as gep on null

[saethlin]

I thought of this while looking at https://github.com/rust-lang/rust/pull/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

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

:hourglass: Trying commit 903dd092bc984a1e992e5ec7010df8595acd9b51 with merge d073071d77ce0f93b4fd8cc567a1e2b9e1b22126...

[bors]

:sunny: Try build successful - checks-actions Build commit: d073071d77ce0f93b4fd8cc567a1e2b9e1b22126 (d073071d77ce0f93b4fd8cc567a1e2b9e1b22126)

[rust-timer]

Queued d073071d77ce0f93b4fd8cc567a1e2b9e1b22126 with parent 61223975d46f794466efa832bc7562b9707ecc46, future comparison URL. There are currently 0 preceding artifacts in the queue. It will probably take at least ~1.2 hours until the benchmark run finishes.

[rust-timer]

Finished benchmarking commit (d073071d77ce0f93b4fd8cc567a1e2b9e1b22126): 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]

:ok_hand: Experiment pr-121282 created and queued. :robot: Automatically detected try build d073071d77ce0f93b4fd8cc567a1e2b9e1b22126 :mag: You can check out the queue and this experiment's details.

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

[saethlin]

@craterbot abort

[craterbot]

:wastebasket: Experiment pr-121282 deleted!

:information_source: 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]

:ok_hand: Experiment pr-121282 created and queued. :mag: You can check out the queue and this experiment's details.

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

[craterbot]

:construction: Experiment pr-121282 is now running

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

[fmease]

Not my area of expertise r? compiler

[craterbot]

:tada: Experiment pr-121282 is completed! :bar_chart: 146 regressed and 123 fixed (419370 total) :newspaper: Open the full report.

:warning: If you notice any spurious failure please add them to the blacklist! :information_source: 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 p=1 crates=https://crater-reports.s3.amazonaws.com/pr-121282/retry-regressed-list.txt

[craterbot]

:ok_hand: Experiment pr-121282-1 created and queued. :mag: You can check out the queue and this experiment's details.

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

[craterbot]

:construction: Experiment pr-121282-1 is now running

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

[craterbot]

:tada: Experiment pr-121282-1 is completed! :bar_chart: 38 regressed and 13 fixed (146 total) :newspaper: Open the full report.

:warning: If you notice any spurious failure please add them to the blacklist! :information_source: Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[saethlin]

All the build-fail results in the re-run are from the MIR inliner's dependency on crate hash. I don't know if that makes the crates qualify as flaky or rustc.

Only 2 of the regressions are UB I didn't catch initially by searching for SIGILL|SIGSEGV. All the rest are flaky test suites.

But I do now have 5 builds that have failed in crater (now twice) and I cannot reproduce. That might be some aspect of how my dev setup is slightly different from crater. Going to keep working on those.

[saethlin]

Here's a paste of my notes, I'll update this later:

Regressions (I will file issues):
reg/faktory/0.12.1 # the int -> ptr transmute is in a dependency, atomic-option: https://github.com/reem/rust-atomic-option/pull/5
reg/gazebo/0.8.1  # UB is in a test, not the actual library (PR rejected by CLA request)
reg/context_bind/0.0.2 # # ptr::read(transmute(some_usize)): https://github.com/valarauca/context_bind/issues/2
reg/unbounded-spsc/0.1.9 # Arc -> usize -> Arc transmutes: https://github.com/spearman/unbounded-spsc/issues/1
gh/Liorst4/liorforth/390a61582bb18cc289570632006d829d5823eb2d # int -> ptr transmute then as_ref: https://github.com/Liorst4/liorforth/issues/1
reg/vtable_gen/0.2.1  # int -> ref transmute (caught by checks)
reg/crt0stack/0.1.1 # int -> ref transmute (caught by checks)
gh/perlindgren/fastmem/c578c9d796cfb294fad05f2ed02aedd64ee1ad44 # int -> ref transmute (caught by checks)
gh/yamgent/rusty-jam-2-dog-chicken/e2b43cfe6e1a140f425244818c7e74bda77603f9 # UB in buddy-alloc

out-of-disk -> segfault:
gh/fracture91/pong-rust/2ba36af7aa7f6528add353fa162f9484f21e6c4a # out of disk -> compiler sefault
gh/slitchfield/aoc2023/0e53faaa5d57131d442ab35f7a0b901c5886a7ce # out of disk -> compiler segfault
gh/sphaerophoria/jira_cloud_review/dc1aac5c5e2d52af03299692cac6bf10ec7243aa  # out of disk -> compiler segfault
gh/tgnottingham/valgrind-ebpf-rodata-bug/3b590c87e27f3546ec539c13b5317513638836d9 # out of disk -> compiler segfault
gh/vbarrielle/aaacs/8e4a5e3f7030c14af322dc5b5c6263ef99e16519 # out of disk -> compiler segfault
gh/wrsturgeon/gamecontroller3-nix/482e4a35e9def86057c53631dfb5951d42f6395bA # out of disk -> compiler segfault
gh/wyatt-herkamp/auto_project/354145fc67b768b6965e2a9eececf8560d16e11c # out of disk -> compiler segfault

Crates that are already broken but sometimes happen to pass crater:
gh/aharisu/navi/38558dc0bb326124e273ff73d87f43f970dc4f62 # SIGABRT -> SIGILL (malloc() corrupted top size on master)
reg/knetrs/0.1.1  # Segfaults on nightly too
reg/yices2/0.1.4 # timed out in master build by chance. Always segfaults.
reg/scheduled-executor/0.4.0 # Allocator corruption on nightly (malloc(): unaligned tcache chunk detected)
gh/fkjogu/stochasticsampling/cc3a287c0eaf06dd4dd871d4e8d4ec242451cbde # SIGABRT -> SIGILL double free or corruption (!prev)
reg/skippy-rs/0.0.0-alpha.4 # Also does out-of-bounds get_unchecked. Boring.
gh/senrust/toy_compiler2/3325a5d71a27e4498c21573ba00c038e2c470d5e # test suite does data races
gh/Yogaflre/leetcode/44e3cb2e60802cd38871208f09425ef0213caf09 # Parent task killed on master, always segfaults
gh/UndeadRat22/kzg10-rust/b9222a332cd5b7bfa64a17f1666655b472591db9 # Lots of bad unsafe, passess when single-threaded, sometimes crashes with an abort about foreign exceptions. Probably wrapping a non-thread-safe C/C++ library
gh/JakeDawkins/graphql-client-302-repro/0378a702e84d04a3d3a7b20fdfbbf3ce2d2e728c # stack overflow in a proc macro
gh/cdstanford/hydroflow-hackathon/f6ffa6d86f3a8e2630203089d2aa874aade63abb # buggy proc macro? SIGILL on nightly
gh/hydro-project/compute-pi/d1026ae20fb61f4db4596d59c7b7315b5cddb22c # buggy proc macro? SIGILL on nightly
reg/pepe-telemetry/0.2.0 # uncoordinated env management
reg/puid/0.2.0 # thread::spawn without join oin
reg/watchable/1.1.1 # thread::spawn without join
reg/lispi/0.3.3 # test suite does data races when run concurrently
reg/cargo-ramdisk/0.2.2 # filesystem races when run concurrently
gh/KatsukiFujimoto/git-rs/1f2f6ee5bd32bc12255d092c7f6d2b0ee9a3ff2e # tests depend on HashSet iteration order being right
gh/MarkChuCarroll/schism/47c9a0303fe4f51cc066adc3c2585db2cf7f3ccb # tests have shared atomic state, unsynchronized
gh/Osrepnay/algae/c687c8a6c2f7c426c823b9f262f256c2fda77cb9 # not entirely sure but I see some odd use of thread::spawn without join and channels. Tests fail in Miri without UB.
gh/Yichangcs/Ruscheme/d9456af5f880ee3b5ee8600412a55173b489e3e8 # test suite does data races when run concurrently
gh/bakaq/rust-blackhole/53a2b81ae13e1a728d9328bdf60e9bd227fe5323 # test suite uses an RNG from SDL2, unlucky randomness fail
gh/diegoasanch/advent-of-code-2023/9f1891131cf2b878de32dee05868a7cd15b9746b # hashmap iteration order
gh/grnmeira/Snake/7270d2715ae206ded530e53868362bd2ccb87ee7 # test suite uses thread_rng, unlucky randomness fails
gh/wojciechkepka/pkgera5f1ea1384494b1fe63282fdf3483563bf495c6f # uncoordinated env management
gh/xffxff/muzero-rs/21ee68a6d276ee52ea59361e99809f5ff23fbfa8 # test suite uses thread_rng, unlucky randomness fails
gh/maxjeffos/rs_dynamic_args/aa6b0631ef1c19fe3d304378b44c621f68899a9c # flaky, concurrent env manipulation
reg/njord/0.1.0 # tests only pass the first time and also rely on the target dir being in a very specific location that happens to be set up in crater by accident
reg/matecito/0.1.6 # thread::spawn + sleep after with no join makes the test flaky. Miri doesn't report UB, but only seed 15 reports a test failure.
reg/namaste/0.23.2 # Managed to repro the test failure by running two instances of the test suite in parallel. This crate does some stuff with unix sockets so I wouldn't be surprised if there's some state leakage somehow?

[DianQK]

I'm not sure this is the right conversion, see:

Godbolt: https://llvm.godbolt.org/z/aeco9Y36b Alive2: https://alive2.llvm.org/ce/z/FS7pkR

[scottmcm]

@DianQK I'm not sure what you're suggesting from those examples? It being UB to load through a transmute-integer-to-pointer is exactly what this PR is intentionally doing. transmute is not as.

[DianQK]

@DianQK I'm not sure what you're suggesting from those examples? It being UB to load through a transmute-integer-to-pointer is exactly what this PR is intentionally doing. transmute is not as.

I don't think we can generate std::mem::transmute(i) to GEP null, offset. IIUC, we only can do this with a const context.

[workingjubilee]

I don't think we can generate std::mem::transmute(i) to GEP null, offset. IIUC, we only can do this with a const context.

Why do you say that?

[DianQK]

I don't think we can generate std::mem::transmute(i) to GEP null, offset. IIUC, we only can do this with a const context.

Why do you say that?

OK, I misread the document at https://doc.rust-lang.org/std/mem/fn.transmute.html, but it doesn't matter. Casting isize to pointer using std::mem::transmute looks like an allowed operation. But getelementptr i8, ptr null, i64 %i is UB. Or I misunderstand something.

[scottmcm]

@DianQK The transmute itself is not UB, but the read afterwards is UB. https://rust-lang.github.io/rfcs/3559-rust-has-provenance.html#guide-level-explanation has some explanation about how transmuting an integer is different from casting it.

Alternatively, run MIRI on this demonstration: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=8836f4de012cca3c2696038bb276b5eb

fn main() {
    let f = 123.456_f32;
    let a = (&f) as *const f32 as usize;
    
    let p1 = a as *const f32;
    unsafe { dbg!(*p1) }; // Fine, it was a cast
    
    let p2 = unsafe { std::mem::transmute::<usize, *const f32>(a) };
    unsafe { dbg!(*p2) }; // UB, transmute doesn't restore provenance
}

and you'll see that the second read is already UB today:

error: Undefined Behavior: memory access failed: 0x24d4c[noalloc] is a dangling pointer (it has no provenance)
 --> src/main.rs:9:14

so this PR is just giving LLVM the chance to use that UB as well -- as your godbolt example shows.

[workingjubilee]

I don't think we can generate std::mem::transmute(i) to GEP null, offset. IIUC, we only can do this with a const context.

Why do you say that?

OK, I misread the document at https://doc.rust-lang.org/std/mem/fn.transmute.html, but it doesn't matter. Casting isize to pointer using std::mem::transmute looks like an allowed operation. But getelementptr i8, ptr null, i64 %i is UB. Or I misunderstand something.

Why is getelementptr i8, ptr null, i64 %i UB? Even with the inbounds annotation, it "only" produces a poison value.

[DianQK]

I don't think we can generate std::mem::transmute(i) to GEP null, offset. IIUC, we only can do this with a const context.

Why do you say that?

OK, I misread the document at https://doc.rust-lang.org/std/mem/fn.transmute.html, but it doesn't matter. Casting isize to pointer using std::mem::transmute looks like an allowed operation. But getelementptr i8, ptr null, i64 %i is UB. Or I misunderstand something.

Why is getelementptr i8, ptr null, i64 %i UB? Even with the inbounds annotation, it "only" produces a poison value.

Sorry :3 Yes. It produces a poison value. But inttoptr won't produce a poison value.