fix(codegen): don't root typed-array element reads as GC temporaries (#6996) - #6997
Merged
Merged
Conversation
…6996) `compiler-output-regression` has been red on main since 760db2f (#6972, "argument temporaries are precise roots"): the `native-abi-proof` workload `native_abi_packet_control` fails `hot_loops_no_runtime_calls` with one unexpected call in its hot loop, `js_gc_temp_root_push`. Bisected, not guessed: 1a533a3 (#6972's parent) is green, 760db2f is red, and the after-opt IR delta is exactly that one call in the hot-loop blocks. It is still red at aa1c150. Root cause. The fixture's kernel is `(buf[i] + packet.tag + i) & 255`. The right operand is a property get on an `any`, so the add lowers through `js_dynamic_string_or_number_add` and #6972 roots the operand pair across the property get -- correctly, in general: a heap operand sitting in an SSA register while its sibling allocates is exactly the #6951 use-after-free. But the left operand here is `buf[i]`, a byte. #6972 anticipated this and gated on `expr_is_known_non_pointer_shadow_value`; that predicate has no arm for typed-array / Buffer element reads, so every iteration of the workload's hottest loop (4096 x 64) paid a push + re-read + truncate to root a value that can never be collected. Fix. Teach the predicate the element-read family. The proof is about the LOWERING, not the declared type -- annotations are unenforced, so `buf: Buffer` holding something else must not be load-bearing, and it isn't: `js_uint8array_index_get_value` and `js_buffer_index_get_value` answer `undefined` for a receiver that is not a Uint8Array/Buffer, and `lower_buffer_load`'s inline arm reads a raw byte. The two lowerings of `Uint8ArrayGet` that CAN yield a heap value are excluded by construction: a symbol key (`js_object_get_symbol_property` hands back a prototype accessor) and a key without the integer-array-index proof (`js_typed_array_index_get_dynamic` falls through to string-keyed property lookup, and an expando can hold anything). `BufferIndexGet` has neither path. Because the gate's soundness argument is "the index proof that routes this read to a byte accessor is the same proof the gate tests", the verbatim copy of `numeric_index_has_integer_array_index_proof` in `expr::arrays_finds` is folded into the `expr::index_get` one (identical arms and thresholds; the copy's `BitAnd` arm was the inlined body of `bitand_has_nonnegative_i32_mask`). Two copies that could drift would make the gate unsound the moment one was edited. This is a skip, not a relaxation: #6972's rooting is untouched everywhere it protects something. In this fixture the emitted push count goes 5 -> 4 -- the console.log accumulator pair and the two `js_dynamic_string_or_number_add` results in the return expression (which really can be strings) all stay rooted -- and the hot loop returns to its pre-#6972 IR. Tests. Four cases in the #6951 suite, both directions: the proven-index `Uint8ArrayGet` and `BufferIndexGet` operands emit no rooting call (both fail without this change), and the symbol-keyed and unproven-key reads still emit one. Verified: `native-abi-proof` and `native-region-proof` both report `"status": "pass"` with every workload green (full harness, link + run, not just the IR probe), both harness unittest modules pass, `cargo fmt` is clean, and `cargo test -p perry-codegen` is unchanged against main (its 3 `native_proof_regressions` failures reproduce identically on a pristine main checkout).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughTyped-array and Buffer index lowering now share integer-index proof logic. Proven byte reads are classified as non-pointer values, avoiding temporary GC roots, while symbol-keyed and unproven reads remain rooted. Regression tests and a changelog entry document the behavior. ChangesTyped-array temp-root precision
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/perry-codegen/tests/temp_root_argument_temporaries.rs (1)
286-405: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd equivalent cargo-test-visible unit coverage.
These regression cases live in an integration suite; place them in, or duplicate them into, a unit-test module that runs in the standard codegen test target.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-codegen/tests/temp_root_argument_temporaries.rs` around lines 286 - 405, Add equivalent regression coverage for the typed-array and Buffer operand-rooting cases in a unit-test module included by the standard codegen test target. Preserve all four behaviors from the existing tests: proven numeric element reads are not temp-rooted, while symbol-keyed and unproven-key reads remain temp-rooted, and ensure the new tests are discoverable by cargo test.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@changelog.d/6997-temp-root-typed-element-reads.md`:
- Around line 31-35: Update the unproven-key lowering description to name
js_object_get_index_polymorphic, which is the helper emitted for the Any-key
Uint8ArrayGet fallback. Preserve the existing explanation that this path falls
through to string-keyed property lookup, and leave the symbol-key and
BufferIndexGet descriptions unchanged.
---
Nitpick comments:
In `@crates/perry-codegen/tests/temp_root_argument_temporaries.rs`:
- Around line 286-405: Add equivalent regression coverage for the typed-array
and Buffer operand-rooting cases in a unit-test module included by the standard
codegen test target. Preserve all four behaviors from the existing tests: proven
numeric element reads are not temp-rooted, while symbol-keyed and unproven-key
reads remain temp-rooted, and ensure the new tests are discoverable by cargo
test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b7ceca36-5c8c-4b88-8b9e-083cb228266c
📒 Files selected for processing (4)
changelog.d/6997-temp-root-typed-element-reads.mdcrates/perry-codegen/src/expr/arrays_finds.rscrates/perry-codegen/src/expr/shadow_slot.rscrates/perry-codegen/tests/temp_root_argument_temporaries.rs
…oot skip CodeRabbit on #6997: the unproven-key regression actually emits `js_object_get_index_polymorphic`, not `js_typed_array_index_get_dynamic`. That is a THIRD heap-capable lowering of `Uint8ArrayGet` -- the i32-context one in `lower_uint8array_get_i32` -- and it is gated on `is_numeric_expr`, not on the integer-array-index proof, so testing the proof alone would not have covered it. Require both predicates (`is_numeric_expr` only ever narrows the skip: every arm it admits still reads a byte). Changelog fragment names all three arms.
This was referenced Jul 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #6996. Unblocks the required
compiler-output-regressioncheck, which has been red onmainsince the 2026-07-28→29 merge window.What was red
compiler-output-regression→native-abi-proof→native_abi_packet_control→hot_loops_no_runtime_calls. The truncated CI message hid the offending name; it is a single call:Everything else in those loops is on the workload's allowlist. This is not a lost native region — the control packet is meant to keep its dynamic calls (that is the point of the typed-vs-control delta). One new call appeared inside the hot loop.
Bisect
Window
95960e0df..760db2fd8(28 commits, the scheduled run is daily). Bisected against this one workload — compiler rebuilt at each endpoint from source in a dedicated worktree and target dir, same cargo package set (cargo build -p perry, debug, as the gate does), never a prebuilt binary from elsewhere:95960e0df(#6911, the last green scheduled run's SHA)1a533a3a8(#6925, parent of the suspect)760db2fd8(#6972, "argument temporaries are precise roots")aa1c15028(currentmain)The good endpoint was verified green before any hop was trusted. The delta between the last two is exactly
js_gc_temp_root_pushin the hot-loop blocks; nothing else in the after-opt IR of that workload changes.So the culprit is one of the GC rooting PRs — #6972, which is a legitimate fix (#6951). It is not being reverted, and it is not the wrong fix; it over-emits in one shape.
Why it happened
The fixture's kernel is
packetisany, sobuf[i] + packet.taglowers throughjs_dynamic_string_or_number_add, and #6972 roots the operand pair across the property get. In general that is exactly right — a heap operand held only in an SSA register while its sibling allocates is the #6951 use-after-free. But the left operand here is a byte. #6972 anticipated this and gated the emission onexpr_is_known_non_pointer_shadow_value; that predicate has no arm for typed-array / Buffer element reads, so the workload's hottest loop (4096 × 64 = 262 144 iterations) paid a push + re-read + truncate per iteration to root a value that can never be collected.The fix — a skip, not a relaxed contract
The contract stays exactly as it is.
expr_is_known_non_pointer_shadow_valuelearns the element-read family instead.The soundness argument is about the lowering, not the declared type — type annotations are unenforced in Perry, so
buf: Bufferholding something else must not be load-bearing, and it isn't:js_uint8array_index_get_valueandjs_buffer_index_get_valueboth answerundefinedfor a receiver that is not a Uint8Array / Uint8ClampedArray / Buffer, and a byte otherwise;lower_buffer_load's inline arm reads a raw byte.The two lowerings of
Uint8ArrayGetthat can yield a heap value are excluded by construction, and both have a test holding them from the other side:u8[Symbol.iterator]) →js_object_get_symbol_property, which returns a%TypedArray%.prototypeaccessor;js_typed_array_index_get_dynamic, which falls through to string-keyed property lookup, and an expando can hold anything.BufferIndexGethas neither path (every arm coerces the key to i32 and reads a byte), so it needs no condition.Because that argument depends on "the index proof that routes this read to a byte accessor is the same proof the gate tests", the verbatim copy of
numeric_index_has_integer_array_index_proofinexpr::arrays_findsis folded into theexpr::index_getone. The two were identical (same arms, same thresholds; the copy'sBitAndarm was the inlined body ofbitand_has_nonnegative_i32_mask) — but two copies that could drift would make the gate unsound the moment one of them was edited.What it costs #6972
Nothing it protects. In this fixture the emitted push count goes 5 → 4: the
console.logaccumulator pair and the twojs_dynamic_string_or_number_addresults in the return expression — which really can be strings — all stay rooted. Only the byte loses its slot, and the hot loop returns to its pre-#6972 IR.Tests
Four cases added to the #6951 suite (
crates/perry-codegen/tests/temp_root_argument_temporaries.rs), in both directions:buffer_element_operand_is_not_temp_rootedandbuffer_index_get_operand_is_not_temp_rooted— no rooting call. Both fail without this change (verified by reverting the predicate and re-running).symbol_keyed_typed_array_read_is_still_temp_rootedandunproven_key_typed_array_read_is_still_temp_rooted— the rooting call is still emitted. These are the soundness boundary, and they pass in both directions by design.Verification
native-abi-proof:"status": "pass",failed_workloads: []— the full harness (compile + link + run + every contract), not just an IR probe.native-region-proof:"status": "pass", all 11 workloads.python3 -m unittest tests.test_compiler_output_regression(66 tests) andtests.test_native_abi_evidence_report(19 tests): OK.cargo test -p perry-codegen: unchanged againstmain. Threenative_proof_regressionscases fail —mutable_string_key_rejects_static_write_pic,nested_same_shape_object_writes_version_one_through_four_fields,static_put_value_rejects_write_pic_when_rhs_can_allocate— and they reproduce identically on a pristinemaincheckout on the same machine, so they are pre-existing and unrelated (worth a separate look).cargo fmt --all -- --check: clean.Not verified: the
fma_contractsteps of that job, which pass-march=haswelland cannot run on the arm64 box this was validated on. They are untouched by this diff.One thing found on the way, not fixed here
crates/perry-codegen/src/collectors/pointer_locals.rsclassifiesExpr::Uint8ArrayGetasType::Numberunconditionally, including the symbol-key and unproven-key lowerings that this PR deliberately excludes. That decides shadow-slot reservation, soconst x = u8[Symbol.iterator]binds a heap value into a local with no GC root — the same class of hazard #6951 was about, on a rarer shape. This PR does not widen that; it just does not copy it. Worth its own issue.Summary by CodeRabbit
Bug Fixes
codegenregression where typed-array and Buffer element reads could emit GC-unsafe temporary values.Tests