fix(codegen): a taken throw out of a packed loop no longer loses loop-carried locals - #9215
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughPacked-f64 loop fast clones now reject ChangesPacked-loop throw handling
macOS unsafe-scope cleanup
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to Loops containing throws now use the established generic path, preventing stale loop-carried values after unwinding while preserving correct behavior for other abrupt exits. No actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Description checkExplanation The description is detailed and covers the defect, root cause, behavioral impact, related issues, tests, validation results, and performance tradeoff. It does not follow the template headings or include the checklist, but the required substantive information is mostly present. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…-carried locals PerryTS#9185 admitted `Stmt::Throw` to the packed-f64 versioned loop on the reasoning that the throw block ends in `unreachable`, so "control never returns to the loop and nothing reads what the clone cached". The second half of that is false: an unwind lands in a `catch`, which can read anything the loop wrote. `break`, `continue` and `return` leave the clone through normal CFG edges that flush the loop-carried locals back to their frame slots. An unwind edge does not, so a local the loop updated read back stale: let s = 0; try { for (let i = 0; i < arr.length; i++) { if (arr[i] === 40) throw PRE; s += arr[i]; } } catch (e) { console.log(s); } // perry 0, node 780 Wrong in both orderings (780 and 820 expected, 0 observed), correct under PERRY_PACKED_LOOP_ABRUPT=0, and correct on the 0.5.1220 release, which confirms PerryTS#9185 as the regression. This restores `Stmt::Throw` to unadmitted. Re-admitting needs the writeback emitted at the throw site rather than only at the exit block; that is PerryTS#9210, and the admission predicate is not the missing piece. PerryTS#9185's own tests missed this because both taken-throw cases read the thrown value or an untouched variable — the clone's live SSA value, which was correct — never the frame slot left behind. A closure-captured accumulator was also correct, being boxed rather than register-promoted, which is what kept the defect this narrow. The added test reads a loop-carried local AFTER each abrupt exit; its break and continue rows are what establish that the defect is specific to the unwind edge. Cost: a loop containing a throw returns to 16.00 ns/op against node's 1.17 (13.7x), the pre-PerryTS#9185 gap. Correctness first; PerryTS#9210 is the way back. Size: __text unchanged (10,968,212 B, 0.00%), __const +64 B, file +8 B. Refs PerryTS#9151, PerryTS#9185, PerryTS#9210
Six inner unsafe blocks nested inside an outer one, each wrapping a single str_from_header call. rustc reports 12 unnecessary-unsafe warnings across the lib and test targets, which fails the workspace warnings gate under -D warnings on any PR that invalidates this crate's check cache. Pre-existing since PerryTS#8453.
|
Superseded by #9230, which contains this revert as its first commit and then fixes the underlying defect — the writeback is emitted at the throw site, so the loop stays on the fast path AND is correct (7.99 → 0.95 ns/op, against node's 1.10). Merging either one resolves the wrong answer on |
cdc5211 to
9376282
Compare
|
Merged. I merged #9185, so this is my miss as much as yours — and the reason it got through is worth writing down, because my review specifically went looking for this hazard and cleared it on a wrong distinction. I reasoned: an unwind could return control to the loop, but Verified the premise rather than taking it, because a peer running a nine-shape packed-loop battery could not reproduce it on
Two things that came out of chasing the non-reproduction, both worth having in the regression test:
Validation: Agreed that reverting the admission is right and #9210 is the real fix. Worth carrying this table into #9210 as its acceptance criteria — particularly the nested partial-flush row, since that's the one a "does the value survive" test would pass while still being wrong. |
|
Correction to the record: this squash carried one commit that isn't yours. A What landed on Both changes were validated together on the branch I actually built — The |
…4 PRs through this hole in one day) (#9256) * docs(contributing): do not cancel the CI run of the PR being merged A cancelled job is neither a pass nor a failure, and two protections go quiet together: pr-gate never reports (so the required context is absent rather than red, which is what invites the bypass), and the changelog fragment check — a step inside lint, conditioned on pull_request — is skipped silently, so the omission stays invisible until release notes are cut. Both were observed on the same day. #9169 merged with lint failing and five jobs cancelled, breaking method dispatch and property lookup on main for four and a half hours (#9247). #9215, #9230 and #9235 each merged with lint CANCELLED; all three touched crates/, none carried a fragment, and the work is absent from its release notes. States explicitly that the gate is correct and should not be changed: gate in test.yml runs if: always() and treats cancelled as failure, exactly so a cancelled dependency cannot read as green. Every incident has been a bypass of a working gate. Docs only. * docs(contributing): teach 'pr-gate present and passing', not 'nothing red' A gate that never ran is absent from the status list, so it reads as clean under any failure filter — the same way CANCELLED does. 'pr-gate: pass' is a positive assertion that the fan-in ran and every dependency was success or skipped; '0 failing' is satisfied equally by a PR whose gate never executed. Extends the note to release automation, where the same hole exists one level up: a skipped or absent required context satisfies 'not failing', so the dispatch condition has to require conclusion == success. --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
….5.1519 (#9255) #9215, #9230 and #9235 merged without changelog fragments, so the v0.5.1519 notes carry no mention of the change. Only one entry is actually missing: #9215 and #9230 fix a regression from #9185 that was introduced and repaired entirely within the unreleased window, so no released version ever exhibited it and describing it as a fix would tell readers their current version is affected when none ever was. The fragment states the shipping release explicitly, so folding it into the next set of notes reads as a correction rather than as a new change. Docs only; no code, and no effect on the frozen v0.5.1519 candidate. Co-authored-by: Ralph Küpper <ralph@skelpo.com>
1 participant
This is a correctness fix for a silent wrong answer currently on
main, from my own #9185. Reverting the admission is the safe move; the real fix is #9210.The defect
#9185 admitted
Stmt::Throwto the packed-f64 versioned loop, reasoning that the throw block ends inunreachable, so "control never returns to the loop and nothing reads what the clone cached". The second half is false — an unwind lands in acatch, which can read anything the loop wrote.break,continueandreturnleave the clone through normal CFG edges that flush the loop-carried locals back to their frame slots. An unwind edge does not:84185b5656ABRUPT=0breakcontinuethrow, accumulate afterthrow, accumulate beforethrow, read via closureThe 0.5.1220 release (pre-#9185) also gives 780, which confirms #9185 as the regression rather than something older.
Why #9185's tests passed
Both of its taken-throw tests are blind to this by construction:
throwPrereads the thrown value and an untouched parameter,throwValuethrowssitself. Both observe the clone's live SSA value, which was correct — never the frame slot left behind. The closure row above is correct for the mirror-image reason: a captured accumulator is boxed rather than register-promoted, so there is no promoted copy to lose. That is what kept the defect narrow enough to slip through.The added test reads a loop-carried local after each abrupt exit. Its
breakandcontinuerows are not padding — they are what establish that the defect is specific to the unwind edge rather than to abrupt exits in general.The rule, which #9154's labeled-
breakbug also obeyed: an abrupt-exit optimisation is only tested by an exit that is actually taken AND a subsequent read of something the loop wrote. Satisfying one of those two conditions is what every test here did.Cost, stated plainly
This gives back #9185's win. A counted loop containing a throw returns to 16.00 ns/op against node's 1.17 (13.7× slower) — the pre-#9185 gap, and now the worst shape in that benchmark;
break(1.00) and the throw-free loop (1.00) both beat node (1.32, 2.26). I would rather be 13.7× slower than silently wrong, and #9210 is the way back: emit the writeback at the throw site, which is the piece #9185 was missing. The admission predicate was never the problem.Validation
packed_loop_abrupt_statements9/9,loop_property_array_hoist12/12,issue_8690_loop_versioned_arraylike3/3Error(user class,constfunction, function-valued binding, block-scoped class) — all node-identicalRUSTFLAGS="-D warnings" cargo check --workspace --all-targets: 8 errors, all inperry-ui-macos, all present on a pristineorigin/mainwith this diff stashed__textunchanged (10,968,212 B, 0.00%),__const+64 B, file +8 BRefs #9151, #9185, #9210
Summary by CodeRabbit
Bug Fixes
throwstatements occur inside optimized packed-array loops.catchblock.throwstatements.Tests