Cap iteration count in ScalarReplacementOfAggregates#155290
Cap iteration count in ScalarReplacementOfAggregates#155290blackms wants to merge 1 commit intorust-lang:mainfrom
ScalarReplacementOfAggregates#155290Conversation
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
it says that 2 is nearly covers all cases and more than 10 is never needed, so you go 16? why? |
This comment has been minimized.
This comment has been minimized.
The SROA pass repeatedly flattens aggregate locals into their field locals, looping until a fixed point is reached. When the MIR body involves a type whose layout is cyclic (e.g. `struct T(T)` reached via an associated type projection), the layout cycle error is emitted but compilation continues into MIR optimization. Inside SROA, `iter_fields` then keeps returning a single field whose normalized type is the enclosing struct itself, so each iteration allocates a fresh replacement local and the fixed-point loop never terminates. Bound the loop to a small constant. In practice two iterations capture essentially all of SROA's value and non-contrived code does not exceed ten, so 10 is sufficient and guarantees termination on tainted input.
aa5cf73 to
e6d8c3c
Compare
|
Thanks for the review @Kivooeo!
Full disclosure: I used a Claude-based coding agent team for the initial investigation — reading the SROA pass, forming the root-cause hypothesis, drafting the fix from @scottmcm's comment, and sketching the test. I then validated manually before opening the PR: reproduced the hang locally with
Fair point, I was being overcautious. Dropped the cap to 10 in the force-push just now — that's the number @scottmcm explicitly called out as "never needed" in non-contrived code, so using exactly that value makes the relationship between comment and constant obvious. Comment updated to match. Also force-pushed a fix for the CI failure: when I renamed the test from |
Fixes #153205.
Problem
ScalarReplacementOfAggregates(compiler/rustc_mir_transform/src/sroa.rs) runs a fixed-pointloop { … }that keeps flattening aggregate locals until no further "dead" locals are produced. Inside,compute_flatteningcallsiter_fields, which normalizes each field type viatry_normalize_erasing_regionsbefore feeding it back as a new local.On the MCVE from the issue:
normalizing
Thing<Identity>'s field type yieldsThing<Identity>again — the enclosing struct itself. AnE0391layout-cycle error is emitted up front, but MIR optimization still runs, and every SROA iteration allocates exactly one fresh local of typeThing<Identity>which is itself a fresh flattening candidate.all_dead_localsis therefore non-empty forever, and the loop never terminates.Reproduced locally with
rustc 1.94.1 stable: after theE0391is printed the compiler hangs indefinitely (killed after 12 s during repro).Fix
Replace the unbounded
loopwithfor _ in 0..MAX_ITERATIONSwhereconst MAX_ITERATIONS: usize = 16. Per @scottmcm on the issue thread, 2 iterations typically capture all of SROA's value and non-contrived code never exceeds 10, so 16 leaves comfortable headroom while guaranteeing termination on tainted input. The loop body (including thebreakon emptyall_dead_locals) is unchanged, so healthy code reaches its fixed point exactly as before.I considered adding an early-bail on
body.tainted_by_errors/tcx.dcx().has_errors()instead, but opted against it: it's a larger behavioural change (skipping SROA entirely on tainted bodies) and the same non-termination shape could theoretically be reached without the tainting flag being set at pass entry. A structural iteration cap is a simpler, more local invariant and is robust to both.Test
Added
tests/ui/layout/hang-sroa-layout-cycle.rsnext to the existinglayout-cycle.rs/post-mono-layout-cycle.rstests. Uses//@ build-failwith//@ compile-flags: -Copt-level=3(SROA is gated onmir_opt_level >= 2) and asserts theE0391cycle error via//~^ ERROR. Without the fix compiletest times out on this test; with the fix it completes and matches the expected stderr.r? mir-opt