fix(gc): root the inlined spread's [Symbol.iterator] walk (#7498) - #7527
Conversation
|
Warning Review limit reached
Next review available in: 1 minute 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 (2)
📝 WalkthroughWalkthroughThe runtime now roots receivers, iterator values, symbols, methods, and results across moving-GC operations. It copies borrowed property keys into owned storage. A regression witness covers repeated inlined array spread. ChangesMoving-GC rooting fixes
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Spread as array_from_spread_value
participant Lookup as js_object_get_symbol_property
participant Handles as RuntimeHandleScope
participant GC as Moving GC
participant Drain as js_iterator_to_array
Spread->>Handles: Root spread receiver and iterator values
Spread->>Lookup: Resolve Symbol.iterator
Lookup->>Handles: Root receiver and copy property key
Lookup->>GC: Allocation-capable prototype lookup
GC-->>Handles: Relocate rooted values
Handles-->>Lookup: Re-read relocated receiver and key
Lookup-->>Spread: Return iterator method
Spread->>Drain: Invoke iterator and drain results
Drain->>GC: Allocate during iteration
GC-->>Handles: Relocate iterator and accumulator
Drain-->>Spread: Return materialized array
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 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 |
`[...obj.arr]` has two lowerings. Out of line it calls `js_iterator_to_array` (rooted by #7495); inlined it routes through `array_from_spread_value`, which resolves `[Symbol.iterator]` through the whole prototype-walk tower first. Three frames on that walk held a GC-managed value the collector cannot see, and `PERRY_GC_PROTECT_FROMSPACE=1` faults on all of them. * `symbol::get::req_handle_symbol_fallback` read the receiver into a bare `usize`, interned a `"_req"` key -- an allocation -- and then read a field off the PRE-move address. It runs on every heap-object symbol read whose own-symbol lookup missed, so the window is unconditional; the reproducer faults 5/5, not intermittently. * `array_prototype_property_value`, and the array + object arms of `get_field_by_name_object_tail`, took the property name as a `&str` / `&[u8]` BORROWED OUT OF THE KEY'S `StringHeader`. No root fixes that shape: a borrow is not a slot the collector can rewrite. They copy the bytes off the heap once, before their first allocation, through the new `HeapKeyBytes` (stack buffer, spill only for a >64-byte key). * `array_from_spread_value` carried the spread receiver through a dozen classification probes and the entire symbol walk, then used it to rebind `this` for the `[Symbol.iterator]()` factory. Also roots `default_object_prototype_property_value`'s key/receiver and the two subclass-marker probes (`fetch_subclass_handle_id`, `temporal_subclass_cell`), each of which allocated a key string between reading its receiver and using it, and re-reads `js_object_get_symbol_property`'s receiver after the fallback. All handles are NaN-boxed, so `scripts/raw_handle_debt.py` stays at 999. Claude-Session: https://claude.ai/code/session_019EHcmXKArA7m42SihYCcgH
The corpus note on `test_gap_gc_iterator_drain_rooting` asked for exactly this
check ("if it does not go silent, there is a third site"). There is, and it now
has an issue with the repro, the lldb faulting instruction, the knob bisect and
the reason the one-line patch was reverted.
Claude-Session: https://claude.ai/code/session_019EHcmXKArA7m42SihYCcgH
50dcf94 to
906e8a8
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Rebased onto
The conflict was That rebase also settles the #7516 overlap empirically rather than by argument.
|
1 participant
Fixes #7498.
What was stale, and why no root could have saved two of them
[...obj.arr]has two lowerings. Out of line it callsjs_iterator_to_array—the drain #7495 rooted. Inlined, it routes through
array_from_spread_value,which resolves
[Symbol.iterator]through the whole prototype-walk towerfirst, and three frames on that walk held a GC-managed value in a place the
collector cannot see.
1.
symbol::get::req_handle_symbol_fallback— the fault in #7498's firsttrace. It reads the receiver into a bare
usize, interns a"_req"key —an allocation — and then reads a field off the pre-move address:
js_object_get_field_by_namethen reads that copy'skeys_arrayout ofretired from-space — a 40-byte
GC_TYPE_ARRAY, which is exactly what thequarantine reports. This helper runs on every heap-object symbol read whose
own-symbol lookup missed, so the window is unconditional; the reproducer faults
5/5 rather than intermittently. (CLAUDE.md's "deterministic ⇒ a table, not a
register" heuristic does not apply here: it is a register, in a code path with
no branch between the read and the allocation.)
2. A
&str/&[u8]borrowed out of the key'sStringHeader— the 56-byteGC_TYPE_STRINGin #7498's second trace.get_field_by_name_object_tailslices the property name straight out of the key's payload and hands it down:
and
array_prototype_property_valuethen allocates three times before readingit (
js_get_global_this_builtin_valueinterns"Array",closure_get_dynamic_propcan run an accessor, and
js_string_from_bytesreads its source bytesafter its own
string_storage_alloc).A
RuntimeHandleScopecannot fix this shape. Rooting the key keeps theobject alive and rewrites the slot; it does nothing for a
&stralreadypointing at the pre-move address, and there is no slot to rewrite. Confirmed
under lldb — the faulting instruction is the
ldrsbof the UTF-8 scan insidejs_string_from_bytes, i.e. the borrow itself:The only sound shape is to stop borrowing: the new
HeapKeyBytescopies thebytes off the heap once, before the arm's first allocation. Property names are
short, so the common case is a 64-byte stack buffer and no allocator traffic at
all; the spill keeps that total rather than "usually".
3.
array_from_spread_value's receiver — carried through a dozenclassification probes and the entire symbol walk, then used to rebind
thisforthe
[Symbol.iterator]()factory and as thejs_array_is_arrayfallback. Rootedfirst, before anything in the function allocates, and the argument is shadowed by
a reader so the pre-collection address is not nameable below.
Also rooted, same shape, same measured path:
default_object_prototype_property_value'skey/receiver (plus the displaced
thisand accessor-receiver override that itsown doc comment flagged as a residual), and the two subclass-marker probes
fetch_subclass_handle_id/temporal_subclass_cell, each of which allocates akey string between reading its receiver and using it.
Raw-handle debt: 999 (baseline 999), unchanged. None of the touched modules
has a ceiling entry, so a single bare
get_raw_{mut,const}_ptrin any of themwould turn the gate red. Every handle here is NaN-boxed and read back with
get_nanbox_f64/get_nanbox_u64.Overlap with #7516, declared rather than duplicated
#7516 (open) fixes
js_get_global_this_builtin_value— theglobalThisreadunder
array_prototype_property_value— with the same root-then-re-read shape.This branch had that fix and it has been dropped, so the two PRs do not
collide textually and the work is not done twice. Measured: the witness below is
clean 5/5 with it and clean 5/5 without it, so nothing here depends on #7516
landing first.
array/flat_clone.rs, #7516's other array-side fix, is untouchedhere.
Witness
test-files/test_gap_gc_spread_symbol_iterator_rooting.ts, registered intest-parity/gc_repsel_corpus.txt. It istest_gap_gc_iterator_drain_rooting'sshrunk twin: same shape, small enough that
cloneinlines, so the spread takesthe other lowering. Its size is load-bearing and the file says so.
Latent by construction, like
10_store_receiver_across_alloc.ts: evacuationcopies rather than zeroes, so the stale read returns the correct old bytes and
this file printed the right checksum before the fix on both links. Only
unmapping retired from-space makes it a signal.
PERRY_NO_AUTO_OPTIMIZE=1, plainPROTECT_FROMSPACE=1 DEPTH=200, no-auto linkPROTECT_FROMSPACE=1 DEPTH=200, auto-optimize linkThe before-fault, symbolicated through
PERRY_LINK_MAP(the perry link stripsthe symbol table, so
atos/nmreturn nothing):The clean verdict is not vacuous. The same run with
PERRY_GC_DIAG=1printsfour quarantine retirements and a live copying minor on each:
The two auto-optimize rows are a real A/B: the runtime archive was rebuilt from
source on both sides (
PERRY_WORKSPACE_ROOTset,target/perry-auto-*/…/libperry_runtime.aasserted on the
[link] invoking:line), so they differ only in runtime source.The instrument fires without
PERRY_GC_ZEALhere — these are ordinarytrigger=ArenaBytescopying minors — and it also faults at the defaultDEPTH=4, so the depth is not doing the work.The protected run is NOT clean everywhere, and that is filed, not absorbed
test_gap_gc_iterator_drain_rooting— the OUT-of-line sibling — still faults andstill prints
badLen 1instead ofbadLen 0(5/5) on this machine, onorigin/mainas well (there it is worse:TypeError: next is not a function,5/5). #7498's two faults are gone from its protected run; what remains is
#7528:
js_native_call_methodreading its rooted receiver into a local ~330 linesand a dozen allocating probes before
is_closure_ptrdereferences it:Knob bisect agrees it is the copying minor:
PERRY_GEN_GC=0,PERRY_GC_SCAVENGE=0andPERRY_WRITE_BARRIERS=0each make it correct,PERRY_GEN_GC_EVACUATE=0does not.Patching the one faulting line was tried and reverted. The fault moved 800
bytes further into the same function — which is the signal that the whole
receiver needs re-reading (99 uses), not one arm of it. That is a separate PR
against a different function; it is #7528, not a vague remainder here.
The corpus note on
test_gap_gc_iterator_drain_rootingis updated to say so:its own text said "when #7498 lands, a protected run should go silent — if it
does not, there is a third site", and there is.
Unrelated and unchanged by this branch:
test_gap_iterator_helpers_2874failsbyte-identically before and after (
TypeError: Cannot read properties of undefined (reading 'toArray')— theIteratorhelpers surface, not a rootingbug).
Validation
cargo test -p perry-runtime --no-fail-fast: 1744 passed, 0 failed.cargo fmt --all -- --check,scripts/raw_handle_debt.py(999/999),scripts/check_file_size.sh,scripts/addr_class_inventory.pyandscripts/check_test_registration.pyall green, re-run after the final commit.Summary by CodeRabbit
Bug Fixes
Tests