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

[oli-obk]

I think you should be able to do your check even earlier (in mir_drops_elaborated_and_const_checked), as long as you do it after borrowck is ensured

View changes since this review

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[oli-obk]

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.

That query modifier is for large query results: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/query/modifiers/struct.arena_cache.html

I think using it may even cause perf regressions

[Human9000-bit]

It looks like tainting a body inside mir_drops_elaborated_and_const_checked disables some const eval analysis later on.
The error disappears if I move transmutes check into optimized_mir_inner

[Human9000-bit]

Though missing error messages don't seem very much informative to me, so we wouldn't lose much if we just bless these tests ig
@oli-obk what do you think?

[rustbot]

Some changes occurred in src/tools/cargo

cc @ehuss

[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.