fix(gc): typed-array constructor sources are precise roots (#6981) - #6990
Merged
Conversation
|
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 (6)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughTyped-array constructor and source-conversion paths now root NaN-boxed inputs and re-read relocated values across allocation points. TaPtr documentation describes header residency precisely, and a GC regression test covers constructor, iterable, array-like, copy, and churn scenarios. ChangesTyped-array GC rooting
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Constructor as js_typed_array_new
participant RootScope as RuntimeHandleScope
participant SourceHelper as typed_array_new_from_heap_source
participant Materializer as typed_array_plain_object_values
Constructor->>RootScope: Root constructor source
Constructor->>SourceHelper: Classify rooted source
SourceHelper->>RootScope: Re-read source after allocating probes
SourceHelper->>Materializer: Materialize iterable or array-like source
Materializer->>RootScope: Read rooted source during allocation
Possibly related issues
🚥 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 |
proggeramlug
pushed a commit
that referenced
this pull request
Jul 29, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/perry-runtime/src/typedarray/construct.rs (1)
238-256: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRoot the array-like source inside
object_like_get.
object_like_getonly re-readsrooted.get_nanbox_f64()at call sites, but it stores that into a barerawlocal, then allocates the key string viajs_string_from_bytes(name, ...)before dereferencingrawinjs_object_get_field_by_name. If that allocation triggers collection and the plain-object/array-like receiver is nursery/movable,rawcan be stale on the field read. Preserve a handle around the receiver through the allocation/read inside this helper.🤖 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-runtime/src/typedarray/construct.rs` around lines 238 - 256, Update object_like_get to keep the receiver rooted across js_string_from_bytes and the subsequent js_object_get_field_by_name call, rather than storing rooted.get_nanbox_f64() in an unrooted raw local. Preserve the existing property lookup behavior while ensuring movable nursery receivers remain valid if key-string allocation triggers collection.
🧹 Nitpick comments (1)
crates/perry-runtime/src/typedarray/construct.rs (1)
166-172: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse the canonical heap-address guard before
GcHeaderreads.
bytes_to_typed_arrayandtyped_array_from_source_raw_valuesboth re-implementraw_addr >= GC_HEADER_SIZE + 0x1000, so handle-band payloads can reach theGcHeaderderef. Replace these inline probes withcrate::value::addr_class::try_read_gc_header(raw_addr)/is_plausible_heap_addr(raw_addr)and then checkobj_type.🤖 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-runtime/src/typedarray/construct.rs` around lines 166 - 172, Update bytes_to_typed_array and typed_array_from_source_raw_values to replace the inline raw_addr threshold and direct GcHeader dereference with the canonical addr_class helpers, using try_read_gc_header or is_plausible_heap_addr before reading obj_type. Preserve the existing plain-object conversion only when the validated header reports GC_TYPE_OBJECT.Source: Learnings
🤖 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.
Outside diff comments:
In `@crates/perry-runtime/src/typedarray/construct.rs`:
- Around line 238-256: Update object_like_get to keep the receiver rooted across
js_string_from_bytes and the subsequent js_object_get_field_by_name call, rather
than storing rooted.get_nanbox_f64() in an unrooted raw local. Preserve the
existing property lookup behavior while ensuring movable nursery receivers
remain valid if key-string allocation triggers collection.
---
Nitpick comments:
In `@crates/perry-runtime/src/typedarray/construct.rs`:
- Around line 166-172: Update bytes_to_typed_array and
typed_array_from_source_raw_values to replace the inline raw_addr threshold and
direct GcHeader dereference with the canonical addr_class helpers, using
try_read_gc_header or is_plausible_heap_addr before reading obj_type. Preserve
the existing plain-object conversion only when the validated header reports
GC_TYPE_OBJECT.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 692a90f2-acb1-41a6-b93b-6171b1e832fd
📒 Files selected for processing (6)
changelog.d/6990-typed-array-ctor-source-rooting.mdcrates/perry-codegen/src/codegen/function.rscrates/perry-codegen/src/codegen/spec_abi.rscrates/perry-runtime/src/typedarray/construct.rstest-files/test_gap_gc_ta_ctor_source_rooting.tstest-parity/gc_repsel_corpus.txt
added 2 commits
July 29, 2026 15:37
`new Int32Array([7, 8])` silently produced a LENGTH-0 array under a relocating minor with precise roots: `a[0]` read `undefined`, no crash. That is the failure in #6981's minimal reproducer, `test_gap_specabi_reassign.ts`. The mechanism is NOT the spec-ABI `TaPtr` shortcut the issue analysis proposed. Measured: the reproducer emits no specialized entry at all (`nm` finds no spec symbol) and fails identically with `PERRY_SPECIALIZED_ABI=0`. The never-reassigned proof in `collectors/spec_abi_sites.rs` correctly rejects `P` — `GlobalSet` puts it in `ModuleScan::writes`, so `judge_arg` returns `Boxed` and the tuple is not viable. The real defect is one layer out and on the runtime side: a typed-array constructor SOURCE reaches `js_typed_array_new` only as a bare NaN-boxed C-ABI argument, which is not a precise root, and the helper allocates before it ever dereferences the source. Instrumented ordering shows the collection landing inside the source-classification chain (the `is_registered_map || is_registered_set || is_builtin_iterator_class_id || js_util_types_is_generator_object` step), after which `clean_arr_ptr` nulls the swept source and the constructor falls through to `typed_array_alloc(kind, 0)`. Fix: root the observed value in a `RuntimeHandleScope` and re-read it after every allocating step, in `js_typed_array_new`'s heap-source arm, `js_typed_array_new_from_array`, `typed_array_from_source_raw_values` and `typed_array_plain_object_values`. The handle is a SNAPSHOT of the argument, never the caller's binding, so re-reading recovers the post-move address without ever observing a later reassignment of the source variable — the distinction that matters for a reproducer whose whole point is `P = new Int32Array([7, 8])`. Also corrects the two comments that made the `TaPtr` shortcut look settled (`codegen/function.rs`, `codegen/spec_abi.rs`). Their conclusion holds but their stated reason was wrong: what is passed and hoisted through is the typed-array HEADER, which is an object. The address is stable because `typed_array_alloc` puts header + inline payload in the OLD arena with `GC_FLAG_TENURED` (never relocated by the nursery copying minor, skipped by old-page defrag since `gc_type_is_movable` is false), not because "typed-array storage is non-movable". Measured on the evacuating precise-roots arm (`PERRY_GC_HEAP_LIMIT=8 PERRY_GC_INCREMENTAL=0 PERRY_CONSERVATIVE_STACK_SCAN=off`), oracle = pinned Node 26.5.0: representation corpus 14 FAIL -> 12 FAIL test_gap_specabi_reassign FAIL -> PASS (9 670 copied) test_gap_specabi_polymorphic_coexist FAIL -> PASS (9 648 copied) Every cell above relocated thousands of objects, so the arm was live. The PR-gated arms (default, verify_evac, cons_scan_off, shipped_default) are 21/21 green.
proggeramlug
force-pushed
the
fix/6981-taptr-spec-abi-rooting
branch
from
July 29, 2026 13:37
110e613 to
2b8758d
Compare
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 the runtime-side half of #6981, and corrects the analysis that issue
carried.
The issue's theory did not hold — measured
#6981 proposed that the minimal reproducer (
test_gap_specabi_reassign.ts)breaks because the spec-ABI
TaPtrparam binding atcodegen/function.rs:484omitsjs_shadow_slot_bind. That is not whathappens:
nmon the compiled reproducer finds no specialized symbol at all.PERRY_SPECIALIZED_ABI=0reproduces the failureidentically (
after: undefined).The never-reassigned proof is correct and it IS consulted on this route.
collect_spec_abi_factswalks the module forLocalSet/GlobalSet/Updateat any depth (
ModuleScan::writes);P = new Int32Array([7, 8])putsPinthat set, so
Pnever entersta_bindings,judge_argreturnsBoxed, thetuple
[Boxed]failsspec_tuple_is_viable, and no specialized entry isemitted. Neither "strengthen the proof" nor "remove the shortcut" applies —
the reproducer never reaches the shortcut. Its own header comment says so
("a reassigned binding must never prove
TaPtr"); it is a negative testfor the spec-ABI, and it was failing for an unrelated reason.
Claim 1 of the issue analysis is also wrong, though the comment it attacks was
misleading: a typed-array header does not move.
typed_array_allocplacesheader + inline payload in the OLD arena with
GC_FLAG_TENURED, which thenursery copying minor never relocates and old-page defrag skips
(
gc_type_is_movable(GC_TYPE_TYPED_ARRAY)isfalse). The comment's reason("typed-array storage is non-movable") named the wrong object — what is passed
and hoisted through is the header, which is an object — so both comments are
rewritten here to state the actual invariant and to say it does not generalize.
What actually breaks
A typed-array constructor source reaches the runtime only as a bare
NaN-boxed C-ABI argument, which is not a precise root, and the helper
allocates before it ever dereferences the source.
Instrumented ordering (probe at each classification step, interleaved with
PERRY_GC_TRACEon the same stream):clean_arr_ptrnulls the swept source and the constructor falls through totyped_array_alloc(kind, 0). Hencenew Int32Array([7, 8])yields alength-0 array and
a[0]readsundefinedwith no crash.Fix
Root the observed value in a
RuntimeHandleScopeand re-read it after everyallocating step, in
js_typed_array_new's heap-source arm,js_typed_array_new_from_array,typed_array_from_source_raw_valuesandtyped_array_plain_object_values.The handle holds a snapshot of the argument, never the caller's binding.
That matters here specifically: re-deriving from
P's slot after a safepointwould hand the constructor the new array, turning a stale-pointer bug into a
silent wrong-answer bug. A temp root buys liveness, a rewritable location, and
the value the call observed; only a snapshot buys the third.
Measurement
Acceptance arm —
PERRY_GC_HEAP_LIMIT=8 PERRY_GC_INCREMENTAL=0 PERRY_CONSERVATIVE_STACK_SCAN=off, release build, macOS arm64, oracle =pinned Node 26.5.0. Local harness reproduces #6981's published table exactly
(PASS=5 / FAIL=14 / UNVER=1, same
copied_objectscounts).test_gap_specabi_reassigntest_gap_specabi_polymorphic_coexistCorpus totals: 14 FAIL → 12 FAIL. Every repaired cell relocated thousands
of objects, so the arm was genuinely live — not a green-because-inert result.
PR-gated arms (
default,verify_evac,cons_scan_off,shipped_default)are 21/21 green, as is the shipped default configuration with no GC env at
all.
The remaining 12, attributed
Not fixed here, and not mine:
ta_param_numeric_read,typedarray_param_read,repsel_canonical_i32,repsel_ptr_shape_locals,repsel_proven_this_frozen— crashes insidethe collector. Bisected
typedarray_param_readtoconsole.log("u8", xorU8(u8, 8)); the faulting stack is frame-for-frameidentical to an unrelated probe's, i.e. one shared GC-internal defect
reached from many sites, not a typed-array-read bug. Its own typed-array
init sequence and param-read shape both pass in isolation.
repsel_p4a_inline_tiers,repsel_p4a_logical_numeric,repsel_p4a3_numarray_barriers,repsel_p4a3_ptr_numarray,repsel_p4b_field_store_elision,repsel_ptr_shape_barriers,repsel_gc_stress— output mismatches with no typed-array-from-arrayconstruction site at all. Consistent with gc: scalar-replaced object/array locals holding heap values are not precise roots #6968 (scalar-replaced
object/array locals are not precise roots).
None of them are the spec-ABI
TaPtrbinding. I found no case where thatshortcut is unsound; its two soundness obligations (liveness through the
caller's proven rooted binding, address stability through old-arena
residency) both hold, and the comments now say why.
Testing
test-files/test_gap_gc_ta_ctor_source_rooting.ts, registered intest-parity/gc_repsel_corpus.txt. Covers the literal temporary, thereassigned binding, a named plain-array source,
%TypedArray%.from,array-like and iterable object sources, typed-array copy construction, and
sustained churn.
Verified to discriminate: on the unfixed build it fails on the acceptance
arm with
literal: 0 undefined undefinedwhile copying 9 675 objects; itpasses after. It is green on the PR arm set because
cons_scan_offalone doesnot reach the evacuating minor, so it starts gating per-PR when
evac_minorjoins
PR_ARMS— whichgc_repsel_matrix.shsays happens when #6981 closes.A test I wrote and then deleted
I twice tried to add a cargo-test-visible unit test in
gc/tests/runtime_roots/, driving the collection withforce_next_general_arena_alloc_slow()+make_arena_trigger_due(). Bothversions passed against the unfixed runtime — the armed trigger fires at
typed_array_alloc, which is after the element snapshot, so neitherpressured the window the fix is about. A test that is green either way is
worse than no test, so I removed it rather than ship false confidence. The
gap test above is the gate, and it is verified to discriminate.
Not touched
Per the #6983 fences:
lower_new,lower_string_method, collection-methodlowering,
temp_roots.rsscanner registration, and the evacuation triggerpath.
Summary by CodeRabbit
new Int32Array(src),TypedArray.from(src), iterables, array-like objects, and typed-array copy cases) to reliably preserve correct lengths and element values across garbage collections.