fix(codegen): region Number proof judges loop condition and update writes - #11782
Conversation
…ites
A loop region proves a loop-carried local Number with one preheader entry
test and a fixed point over the local's writes. The fixed point walked only
the loop body, so a write in the `for` update clause or in the condition
(`for (...; i++, s = "a") { o.x = o.x + s; }`) was never judged: with no
per-iteration recheck, F added the string's NaN-box bits as a double and
stored the result into the pointer-free F64 lane. fadd propagates the
payload, so the lane held the live string pointer (o.x printed `a`, node
`1aa`), untraced by the collector.
RegionNumberAssumptions now carries the region's loop control, and the
collector feeds its writes into the same shared write inventory as the
body's. A local written there is judged by the same fixed point: a
number-producing write keeps the fact, anything else drops the local to the
generic route. The planner and the preheader pass the loop's condition and
update; a body region has none.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (6)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe numeric-local proof now considers writes in loop conditions and ChangesLoop numeric-local proof
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The change is mergeable after normal checks; no actionable issue remains in the supplied review evidence. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change tightens an existing safety check rather than adding access or privileges. No introduced security issue was established in the reviewed paths, but incomplete coverage limits assurance. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 32.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 25 functions across 5 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
1 participant
Fixes a miscompile in default-on numeric receiver regions, introduced by #11680.
The bug. The Number proof for a loop-carried local only looked at writes in the loop body. A local written in the
forupdate clause or the loop condition was proven Number by a single test before the loop. When the region needs no per-iteration recheck, that test never runs again. Soo.x + sbecomes a raw float add on a string's bits, and the result goes through the pointer-free raw store. Wrong output, plus a live heap pointer in an F64 field that the GC never traces.s = "a"1aaa1aafor (...; (s = gen(i)), i < n; ...)5g1g2g25g1g2(i++, s = t, t = "c")3ccc3cco.x < swiths = "9"313s = {tag}1[object Object]…{ tag: 'live' }PERRY_REGIONS=0matches node, so the bug is region-only. The GC verifier doesn't catch the stray pointer.The fix. The region's assumptions now carry the loop's condition and update. Their writes go into the same write inventory and the same fixed point as body writes. A local with any non-Number write drops to the generic route. No special case.
Tests
test_gap_region_loop_control_writes.ts(byte-identical to node);Verification
manifest_consistency(net::writableCorked), fails on main since perf: batch corked sockets and streamline interpreter scope access #11757.region9/9,numeric17/17,field34/35 (one flaky compile, passes on rerun).Summary by CodeRabbit
forandwhileloops, including conditions and update clauses that change loop-carried values.