Fix malformed transmute handling during mir build

[Human9000-bit]

View all comments

Running dataflow const prop optimization on std::mem::transmute of mismatched sizes failed the assertion in interpreter and caused ICE.

So it's better to not let those transmutations into the interpreter at all

Fixes #149920

[rustbot]

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

[fmease]

r? mir

[oli-obk]

This error is emitted in typeck iirc. It should be tainting the body. If it isnt, it's likely that tcx.dcx() is used instead of infcx.dcx()

Please check if we can avoid changing anything in const prop by properly tainting wherever this error is emitted

[Human9000-bit]

At first glance there isn't infcx where the error is emitted, and no obvious way to get one

[oli-obk]

Where is it being emitted? If it is within the typeck query, we can probably change something further up the call stack ( you can try using -Ztreat-err-as-bug to find out where the error is emitted and what the call stack is while emitting it

[Human9000-bit]

It is emitted in check_transmutes ran by run_required_analyses:

rust/compiler/rustc_interface/src/passes.rs

Lines 1146 to 1149 in 981cf69

	if not_typeck_child {
	tcx.ensure_ok().mir_borrowck(def_id);
	tcx.ensure_ok().check_transmutes(def_id);
	}

and the very error emission happens here:

rust/compiler/rustc_hir_typeck/src/intrinsicck.rs

Lines 138 to 142 in 981cf69

	} else {
	err.note(format!("source type: `{}` ({})", from, skeleton_string(from, sk_from)));
	err.note(format!("target type: `{}` ({})", to, skeleton_string(to, sk_to)));
	err.emit();
	}

[oli-obk]

Oh wow. Didn't know that was changed (#145469)

I'll need to think on it. Unsure why it was necessary to live in a separate query

[oli-obk]

Yes that would fix this optimizer ICE, but i didn't suggest that because i assumed we'd get the same ICE in CTFE, and the only fix for that is to run the query earlier, where it would likely cycle again

[Human9000-bit]

We could check at the beginning of optimized_mir that check_transmutes does not emit an error.

I tried to do that at the start of run_pass of dataflow const prop. It ICEd for def_id being a typeck child.
So we can't just slap check_transmutes for every def_id (at least in mir opts)

[oli-obk]

You can invoke is_typeck_child to invoke check_transmutes on the parent (there's also a method for getting the typeck parent).

This will fix the ICE (if you also make the check_transmute query return a Result<(), ErrorReported> and don't do anything if it's an error).

Tho at that point it may be better to do what cjgillot suggested and check it at the start of thr optimized_mir query and just return a dummy body or the unoptimized body.

I'm just sceptical it fixed the ICE in general as it should be possibe to produce the ICE in const eval by evaluating code that has the same transmute problem

[oli-obk]

Hmm apparently not: https://play.rust-lang.org/?version=nightly&mode=release&edition=2024&gist=31291deac2b05c040502d266b33e2f69

It seems very weird that we report an error here, but that's probably an oversight. The ctfe error should be an ICE

[Human9000-bit]

Apparently your example is getting caught in well-formedness checking during HIR analysis, which uses different methods of interpreter compared to dataflow const prop opt

[Human9000-bit]

Follow-up idea: I am thinking it would be nice to add arena_cache modifier for check_trasnmutes query: Result<(), ErrorGuaranteed> is 1 byte size, and the query is now being reused.
Though it requires Result<(), ErrorGuaranteed> impl ArenaCache, so that's why it should better be in another PR.

[JonathanBrouwer]

@bors try @rust-timer queue

[Human9000-bit]

@oli-obk I tried the approach with checking transmutes another time before optimizing MIR, the perf results should be ready soon

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[cjgillot]

r=me if perf is clean

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 3cd4ee9 (3cd4ee9bcdc9171640486660ba26e388f1c35ebe, parent: 20e19eeac0c548e31f84d3914e0ce17cde618119)

[rust-timer]

Finished benchmarking commit (3cd4ee9): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

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

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

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

Max RSS (memory usage)

Results (primary -3.9%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-3.9%	[-7.5%, -1.0%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-3.9%	[-7.5%, -1.0%]	3

Cycles

Results (secondary 2.9%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

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

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 511.673s -> 511.063s (-0.12%)
Artifact size: 400.63 MiB -> 400.67 MiB (0.01%)

[oli-obk]

@bors r=cjgillot

[rust-bors]

📌 Commit fa6e163 has been approved by cjgillot

It is now in the queue for this repository.

🌲 The tree is currently closed for pull requests below priority 5. This pull request will be tested once the tree is reopened.

[JonathanBrouwer]

@bors p=6
Allowing this to run with closed tree

[rust-bors]

☀️ Test successful - CI
Approved by: cjgillot
Duration: 3h 11m 6s
Pushing 53509ca to main...

[github-actions]

What is this?

This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing d595fce (parent) -> 53509ca (this PR)

Test differences

Show 4 test diffs

Stage 1

• [ui] tests/ui/transmute/transmute-in-async-fn.rs: [missing] -> pass (J0)

Stage 2

• [ui] tests/ui/transmute/transmute-in-async-fn.rs: [missing] -> pass (J1)

Additionally, 2 doctest diffs were found. These are ignored, as they are noisy.

Job group index

• J0: x86_64-gnu-llvm-21-3, x86_64-gnu-llvm-22-3
• J1: aarch64-apple, aarch64-gnu, aarch64-gnu-llvm-21-1, aarch64-msvc-1, arm-android, armhf-gnu, dist-i586-gnu-i586-i686-musl, i686-gnu-1, i686-gnu-nopt-1, i686-msvc-1, optional-x86_64-gnu-parallel-frontend, test-various, x86_64-gnu, x86_64-gnu-debug, x86_64-gnu-gcc, x86_64-gnu-llvm-21, x86_64-gnu-llvm-21-2, x86_64-gnu-llvm-22-2, x86_64-gnu-nopt, x86_64-gnu-stable, x86_64-mingw-1, x86_64-msvc-1

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard 53509ca37e3b507887607c2f4a7f23bd4838f099 --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

1. dist-aarch64-apple: 2h 17m -> 1h 26m (-36.8%)
2. dist-powerpc64le-linux-gnu: 1h 16m -> 1h 44m (+36.8%)
3. dist-powerpc64-linux-musl: 1h 14m -> 1h 41m (+36.3%)
4. dist-x86_64-apple: 2h 13m -> 1h 35m (-28.4%)
5. x86_64-msvc-2: 1h 56m -> 2h 29m (+28.3%)
6. dist-sparcv9-solaris: 1h 15m -> 1h 35m (+26.6%)
7. x86_64-msvc-ext1: 2h 11m -> 1h 40m (-23.3%)
8. x86_64-gnu-aux: 2h 19m -> 1h 49m (-21.3%)
9. dist-various-2: 35m 24s -> 42m 30s (+20.1%)
10. aarch64-apple: 3h 27m -> 2h 46m (-19.7%)

How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

[rust-timer]

Finished benchmarking commit (53509ca): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This perf run didn't have relevant results for this metric.

Max RSS (memory usage)

Results (primary -0.9%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	1.9%	[1.9%, 1.9%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-3.8%	[-3.8%, -3.8%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.9%	[-3.8%, 1.9%]	2

Cycles

Results (secondary 6.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

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

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 509.701s -> 509.672s (-0.01%)
Artifact size: 400.67 MiB -> 400.72 MiB (0.01%)