fix(codegen): resolve #7494's four lowering-independent proof-test failures - #7730
Conversation
|
Warning Review limit reached
Next review available in: 23 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (10)
✨ Finishing Touches📝 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 |
…ilures Per-test disposition, decided by whether the lowering legitimately changed or a proof regressed: - reassigned_typed_array_store_records_runtime_fallback: live regression. A local reassigned to a fresh typed array (array = new Int32Array(8)) lost its typed-array dispatch (receiver_class_name correctly answers None for a reassigned local, #6906) and fell through to is_array_expr's plain-number[] fast store, writing the element at the wrong byte offset against a real typed-array object. Fixed by excluding reassigned locals from is_array_expr (mirroring receiver_class_name's own guard) and by moving is_width_tracked_typed_array_receiver off receiver_class_name onto static_type_of, since every tier it gates is independently safe against a stale/reassigned view. - proven_buffer_and_typed_array_reads_are_numeric_operands: live gap (perf). A buffer-view-tracked typed-array read in arithmetic context had no number-context entry point, so it picked up a redundant js_number_coerce despite being a proven native read. Added try_lower_typed_array_f64_read_for_number_context. - integer_arithmetic_array_push_omits_inbounds_layout_note_and_barrier and integer_modulo::i32_counter_mod_unsafe_or_nonliteral_divisors_keep_frem: stale assertions. Both were pinned to a lowering shape #6915 and #7404/#7416 legitimately (and soundly) superseded. Re-pointed at the properties they exist to protect instead of the old textual windows. typed_f64_receiver_method_clone_raw_loads_after_composed_guards (#7506) and large_local_array_push_inbounds_store_emits_precise_slot_barrier (#7708) remain red, unrelated and untouched.
064f321 to
7b9fa76
Compare
Merging as v0.5.1424Four tests, and the disposition question was answered per test rather than in bulk — which is what makes this landable. #1 is a real memory-corruption bug, not a test problem
That is an unboxed-pointer-plus-wrong-offset write. The dynamic-fallback arm's own comment already promised what should have happened — "aliases, reassigned locals, and unknown bounds stay on the runtime helper" — and The two-part fix is the right shape: exclude reassigned locals from The other three
Re-pointing two assertions at "the property" rather than "the textual window" is the fix #7494 asked for, and the reason the other two were caught as real bugs is that the question was asked per test instead of assumed. VerificationFull |
Summary
Resolves #7494 — the four lowering-independent
perry-codegenintegration-test failures. For each, the question was the same and had to be answered before touching the test: did the lowering legitimately change, or did a proof regress?reassigned_typed_array_store_records_runtime_fallback— live regression, fixed.array[i] = von a local reassigned to a fresh typed array (array = new Int32Array(8)) skipped the typed-array dispatch —is_width_tracked_typed_array_receiveris gated onreceiver_class_name, which correctly answersNonefor any reassigned local (let binding reassigned from Int32Array to plain array keeps typed-array read lowering (wrong values: 0/undefined instead of elements) #6906) — and fell through tois_array_expr's plain-number[]fast store (lower_index_set_fast). That path writes the element atarr_handle + 8; a real typed-array object's data region starts atarr_handle + 16. The store landed on the typed array's own header bytes instead of element 0: a type-confusion write, not a missed optimization — the severity the issue flagged as "guilty until proven innocent." Fixed in two places:is_array_exprnow excludes reassigned locals up front (mirroringreceiver_class_name's own guard), andis_width_tracked_typed_array_receiver(in bothindex_get.rsandindex_set.rs) now resolves the receiver viastatic_type_ofinstead ofreceiver_class_name— every tier it gates is eitherctx.buffer_view_slots-tracked (which reassignment already invalidates via the existingupdate_buffer_view_for_assignment) or a genuinely dynamic runtime call that re-validates the object's actual GC kind at runtime, so the stricter guard was only ever stopping this branch's own documented "aliases, reassigned locals, and unknown bounds stay on the runtime helper" fallback from being reached.proven_buffer_and_typed_array_reads_are_numeric_operands— live gap, fixed (perf, not correctness). Asum += typedArray[i]read tracked viactx.buffer_view_slots(the stronger-bounds native path thatta_param_f64_read.rsexplicitly declines to shadow) had no number-context entry point of its own, so it fell through to the genericlower_exprtier withfallback_coerced = falseand picked up a redundantjs_number_coerce— the residual-coerce rule can't see that this specific lowering path never produces a boxed value. Addedbuffer_access::try_lower_typed_array_f64_read_for_number_context, wired intobinary::lower_arithmetic_operand.integer_arithmetic_array_push_omits_inbounds_layout_note_and_barrier— stale assertion, re-pointed at the property. perf(codegen+runtime): representation-selection Phase 4a — fast plain-array numeric elements (inline tiers, holes axis, self-healing bindings) #6915 (Repsel 4a.1, merged 2026-07-28 — five versions before perf(typed-feedback): stop emitting recording calls into default builds (#7480) #7702, which is often but wrongly blamed for this one) added an inline-store tier for array pushes whose value is canonical-raw-f64 by construction with typed-feedback recording off (the default), skipping the guarded tier'sjs_typed_feedback_numeric_array_push_guard/js_array_numeric_push_f64_unboxedpair entirely — so the test'sblock_between("apush.numeric_fast.", …)window no longer existed. The property the test protects — no slot-layout note, no write barrier, for a value proven non-pointer by construction — holds under either tier (array_store_needs_layout_note/array_store_needs_write_barrierare consulted before the tier is picked), so the test now asserts that property directly over the whole function, plus two sanity checks that the push and its arithmetic weren't optimized away.integer_modulo::i32_counter_mod_unsafe_or_nonliteral_divisors_keep_frem— stale assertion, re-pointed at the property. perf: the % fast path gates on the i32 set, but only needs integer-valued-in-i64 (20.4× on bench_bitwise) #7404/perf(codegen): route % on i64-range integer locals to srem, not fmod #7416 added a second integer-modulo fast-path hand-off for a dividend proven integer-valued outsidei32_counter_slots, gated on the divisor being any non-zeroExpr::Integerliteral — not, like the older hand-off, one that is also positive and fitsi32. The widening is sound: the dividend side is independently capped at 62 bits (a full bit of headroom belowi64::MIN), and ECMA-262's%sign rule depends only on the dividend's sign, which is exactly what the existing IEEE-754 negative-zero repair already tests.negativeandout_of_i32move to a new test asserting they now reach the integer path with the repair intact; the still-genuinely-ineligible cases (Expr::Numberliterals, zero-divisor UB avoidance) stay in the original test.typed_f64_receiver_method_clone_raw_loads_after_composed_guards(#7506) andlarge_local_array_push_inbounds_store_emits_precise_slot_barrier(#7708) remain red on this branch — both are separate, open investigations, left untouched per scope.Test plan
cargo test -p perry-codegen --no-fail-fast --tests(full integration + lib suite) — same 2 pre-existing, out-of-scope failures as a cleanmainbaseline (typed_f64_receiver_method_clone_raw_loads_after_composed_guards,large_local_array_push_inbounds_store_emits_precise_slot_barrier); all four target tests green; zero new failures.cargo test -p perry-codegen --no-fail-fast --test native_proof_buffer_views --test native_proof_regressions --test typed_shape_descriptors— reconfirmed clean aftercargo fmt.cargo fmt --all -- --checkclean.