fix(gc): record layout + write barrier on raw pointer publishes into traced payload slots - #7179
Conversation
📝 WalkthroughWalkthroughThe PR replaces raw pointer stores with barriered updates, orders dynamic field publication before value storage, initializes closure captures to ChangesGC pointer publication
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 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: 2
🤖 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 `@crates/perry-runtime/src/gc/tests/copying/pointer_publish_7154.rs`:
- Around line 58-82: Root the closure before calling
js_object_set_method_by_name, which may trigger copying GC, by storing its
address in the existing shadow slot and reloading it through the test's live
helper afterward. Use the reloaded closure for capture_slot and
enumerated_child_slots, preserving the existing slot reuse in the later survival
phase.
In `@crates/perry-runtime/src/symbol/properties.rs`:
- Line 664: Update the field-write sequence in bind_reserved_this_slot so
obj_f64 remains rooted through js_closure_set_capture_f64, layout_note_slot, and
runtime_write_barrier_gc_slot. Before js_object_set_field_by_name dereferences
it, obtain or refresh the object pointer from the live handle scope rather than
using the potentially stale raw pointer.
🪄 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: dd419416-be06-415b-a156-7dff9312d5a1
📒 Files selected for processing (7)
changelog.d/7179-gc-pointer-publish-barrier.mdcrates/perry-runtime/src/closure/alloc.rscrates/perry-runtime/src/gc/tests/copying.rscrates/perry-runtime/src/gc/tests/copying/pointer_publish_7154.rscrates/perry-runtime/src/object/field_set_by_name.rscrates/perry-runtime/src/symbol/properties.rscrates/perry-runtime/src/weakref.rs
…ayload slots Refs #7154
…alue, live canary)
14d2aad to
2f4a7bb
Compare
|
Merge order: #7180 must land first. #7180 is the one-line fix (formatting only, no behaviour change). Once it lands, re-running this PR's checks against the updated base is enough — there is no conflict, this PR does not touch Review round is addressed (
The 4 |
1 participant
Summary
Two raw pointer stores made the default evacuating minor GC lose live objects. Both break the same invariant, and #7154's diagnostic signature (a NaN-boxed pointer at a traced slot, target header garbage,
PERRY_GC_MOVING_LOOP_POLLS=0makes it go away) is what each one produces.Perry allocates every heap object in the
POINTER_FREElayout state and only leaves that state when a store records a pointer throughlayout_note_slot.heap_payload_slot_selectionshort-circuits onPOINTER_FREEand skips the entire payload without consulting any mask — so a raw*slot = bitsinto a traced slot is not merely imprecise, it makes the child invisible. The evacuating young-generation minor (default-on since #7019) then neither keeps the child alive nor rewrites the slot when the child moves, and the next read returns a pointer into reclaimed nursery memory:TypeError: value is not a functionat a call site unrelated to the store, or a SIGSEGV inside the collector on a later cycle. The same raw store also skipsruntime_write_barrier_slot, so an old→young edge never reaches the remembered set.The invariant
Both fixed sites now say this in a comment at the store.
What was fixed
1.
bind_reserved_this_slot(symbol/properties.rs) — shared byjs_object_set_method_by_name(#809: the ordered object-literal lowering used when a...spreadprecedes athis-reading method) andjs_object_set_symbol_method. It patched the closure's reservedthiscapture slot with a raw*slot = f64, so the closure stayedPOINTER_FREEand the bound receiver was never traced. Now routes throughjs_closure_set_capture_f64.2.
js_weakmap_set's overwrite-existing-key path (weakref.rs) — wrote the new value into the entry's field 1, byte offset +40 (the exact offset #7154's diagnostic scan reports) with the unbarrieredwrite_object_field_bits_raw. The insert path (weak_entry_new) already usedjs_object_set_field; the overwrite now matches. For a tenured entry this was a genuine missing remembered-set insert.Two hardening changes in the same family:
3.
js_closure_allocundefined-initializes its capture slots. They were raw recycled arena bytes — invisible while the layout saysPOINTER_FREE, but decoded as references by anything that reads payload words (the conservative scan,PERRY_GC_FROMSPACE_SCAN, a later whole-range layout rebuild). Mirrors #7138's HOLE-initialisation of unused array capacity and whatjs_object_allocalready does for object fields.4.
js_object_set_field_by_name's three dynamic-key append paths bumpfield_countbefore publishing the value.gc_field_slot_rangebounds the collector's view of the payload byfield_count, so a slot the count does not yet cover is invisible to both tracing and evacuation rewriting; the old order leftmirror_class_object_static_writesitting inside that window. Latent today (nothing reachable from that window allocates from the arena) — this makes the ordering correct by construction rather than by audit. Every physical slot is undefined-initialized at allocation, so widening the count first can only expose non-pointer sentinels.Evidence
Reproducer: zod 4.4.3 compiled natively (
perry.compilePackages), 400 schemas × 20 rounds ofsafeParse. Measured on the pre-#7161main, where the evacuating minor was still default-on: it exits non-zero in the default configuration and is clean underPERRY_GC_MOVING_LOOP_POLLS=0— the #7154 fingerprint. (After #7161 the shipped default no longer moves, so the crash now needsPERRY_GC_MOVING_LOOP_POLLS=1; the fix and the counts below are unchanged either way.)PERRY_GC_FROMSPACE_SCAN=1, per cycle:type=4(closure capture, +16) edgesThe traced mechanism, established with temporary probes (all reverted):
type=4closures freshly copied intoSurvivor1, capture slot 0, pointing at un-evacuatedNurseryEdenobjects,ever_dirty=false._reserved = 0x4008→GC_LAYOUT_POINTER_FREE, with the collector's owngc_child_slotsenumerator emitting zero child slots and the offending slot falling inside the skippedPointerFreeRange.capture_count = 0x80000001→CAPTURES_THIS_FLAG | 1: the dangling slot is the reservedthisslot.#[track_caller]trap onset_layout_stateand onlayout_transfershowed the Eden original was alreadyPOINTER_FREEwith a live pointer capture and no per-object mask — i.e. the capture was never recorded, not lost on the move. That points at the one store path that writes that slot without the helper:bind_reserved_this_slot.Tests
crates/perry-runtime/src/gc/tests/copying/pointer_publish_7154.rs, three tests. Each first interrogates the collector's own child-slot enumerator (deterministic — immune to GC timing and conservative-stack pinning), then asserts end-to-end survival across a collection proven to have copied (copied_objects > 0, kept true by a rooted young canary so the liveness gate cannot be satisfied by the very edge under test).Sabotage, one fix reverted at a time:
test_bound_this_capture_is_traced_after_method_bind_7154[]"test_weakmap_overwrite_value_is_traced_7154test_fresh_closure_capture_slots_are_initialized_7154cargo test -p perry-runtime --lib -- --test-threads=1(perry-dev): 1603 passed, 0 failed, 3 ignored.cargo fmt --all --checkclean.scripts/check_file_size.shclean.GC x repsel matrix
New gap test
test-files/test_gap_gc_pointer_publish.ts, registered intest-parity/gc_repsel_corpus.txt. It exercises all three fixed publish sites (spread-then-this-method object literal, computedSymbol.toPrimitivemethod, WeakMap overwrite of a tenured entry), and every subject is read after the churn that drives collection.Re-run after rebasing onto the merged #7161,
--arms all --pressure 8:Every
requires=movearm bites —evac_minor,force_evac,force_verify,loop_pollsand all eightrep_*arms reportmoved-objects 1/1,copy-minor 1/1. 33 copying minors underPERRY_GC_MOVING_LOOP_POLLS=1.The four
UNVERcells (default,verify_evac,cons_scan_off,cons_scan_off_force) are #7161's doing, and are correct: with the evacuating minor off by default the file drives zero copying minors, and the bug it guards cannot manifest without a moving collector — a PASS there would be exactly the false confidence the harness exists to remove. This is not specific to this file:test_gap_repsel_gc_stress, the corpus's designated GC-live member, now reports the identicalPASS=17 UNVER=4shape. The corpus entry documents this and says what to do if the moving-loop knob is ever deleted.Collector-side coverage does not depend on that knob. The three Rust tests call
gc_collect_minordirectly, which takes the copying path regardless ofPERRY_GC_MOVING_LOOP_POLLS, and each assertscopied_objects > 0. Verified green on the flipped default: 1608 passed, 0 failed.Red-then-green on the gap file, same binary, fixes reverted: exit 138 (SIGBUS); byte-exact vs Node with the fixes.
Review round (both 🟠 Majors addressed)
Both were property 3 of the three-property contract — the value the call observed — and both are fixed in
1c15d441b.bind_reserved_this_slotheldobj_f64raw across the barriered capture store. I traced the reachability first: the store is not an arena allocator today (layout_note_slottouches only thread-local layout maps — a system-allocator insert, which does not feedGC_NEXT_MALLOC_TRIGGER;runtime_write_barrier_gc_slotends in a remembered-set insert), so nothing was miscompiled. I took the fix anyway, because that is a claim about three functions nobody is obliged to preserve and String literal operand is not GC-rooted across an allocating call in the same expression (stale handle after evacuation) #7114 is what it costs when one goes stale. Both entry points now root receiver / key-or-symbol / closure in aRuntimeHandleScopeand re-derive every raw pointer after the bind, including the returned receiver.bind_reserved_this_slotdocuments itself as a collection point.js_weakmap_set's overwrite arm is already correct for a structural reason: the barriered store is the last thing that arm does and it returns frommap_handle, so neitherentries_ptrnorentryis read afterwards. The enclosing loop's "this scan performs no allocation" comment was made narrower than it reads, so the invariant is now annotated with what a future editor must do if that arm ever falls through.Not fixed here
This is necessary but not sufficient for the zod workload: with the closure-capture population at zero, that binary still fails whenever the moving minor is enabled (pre-#7161 default;
PERRY_GC_MOVING_LOOP_POLLS=1after it) and is still clean when it is not, so at least one more offender remains. The residue the scan still reports is dominated bytype=1array owners whose own header carriesGC_FLAG_FORWARDED(stale relocated copies — probably scan false positives, sincescan_objectskips from-space owners but not forwarded ones) plus the documentedtype=3conservative-scan string false positives. Six genuine-lookingSHAPE_SHAREDkeys-array slots inSurvivor0are the most interesting remainder. The #7161 stopgap should not be dropped on the strength of this PR alone.scripts/addr_class_inventory.pyfails on this branch, inchild_process/value_util.rsandfs/dirent.rs— neither is touched here (git diff origin/main --name-onlyconfirms); pre-existing onmain.Fixes part of #7154. Refs #7161, #7138.