fix(gc): restore SIDE_MASK when recording a pointer into an array's existing element mask

[jdalton]

Status: DRAFT — partial soundness fix. Does NOT by itself resolve the sfw-registry crash.

What this fixes (verified)

A real moving-minor soundness bug in the same class as #6831. layout_note_slot's pointer branch set a mask bit for an existing element mask without restoring SIDE_MASK layout state:

if let Some(mask) = masks.get_mut(&parent_user) {
    mask.set_slot(slot_index);            // <- left a stale POINTER_FREE state in place
}

heap_payload_slot_selection treats POINTER_FREE as "no pointers" and skips the entire payload without consulting the mask. So an array that had flipped to POINTER_FREE (e.g. truncated to a numeric/empty prefix) while retaining its element mask had all its recorded pointer elements skipped by the evacuating young-gen minor — the live child was reclaimed/relocated out from under the slot and later read as a garbage pointer (TypeError: value is not a function).

Fix (crates/perry-runtime/src/gc/layout.rs, layout_note_slot): recording a pointer proves the object is not pointer-free, so restore SIDE_MASK.

Verification of this fix

• PERRY_GC_FROMSPACE_SCAN=1 on the sfw-registry workload: within-length stale array→young edges (owner in Survivor, POINTER_FREE state, elem_index < length) drop 202 → 0.
• cargo test -p perry-runtime --lib layout 78 pass / 0 fail; array:: 73 pass / 0 fail. (gc::tests::teardown::map_set_* fail identically on the clean baseline — pre-existing, order-dependent; unrelated.)
• Change is strictly conservative: it only ever adds tracing, never removes it.

What this does NOT fix (still open)

The sfw-registry TypeError: value is not a function crash persists after this fix (tested under both default moving loop and PERRY_GC_FORCE_EVACUATE=1). Its primary remaining cause is a separate class: live array→young edges at element indices ≥ length (21,888 remaining scan offenders across ~11k small arrays). The length-bounded copying-minor trace (gc_element_slot_range → [0, length)) does not cover them.

That one is NOT safe to fix by "just trace more": a diagnostic that additionally evacuated beyond-length capacity slots decoding as heap pointers turned the crash into a SIGSEGV — the beyond-length region is contaminated with uninitialized capacity whose bits coincidentally decode as from-space pointers. So a correct fix must be at the producers (keep length in sync with the filled/live extent, or clear vacated tail slots in pop/shift/unshift), not in the collector. Root producer not yet pinned to a single site.

Relates to #6831 (born-old arrays losing old→young remembered-set edges — the same "array element edge invisible to the minor → value is not a function" failure).

[coderabbitai]

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 0cd086aa-14cb-4302-b5db-1dc58a74ab6e

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[proggeramlug]

Thanks — this is a real soundness fix and it lands as-is. I've pushed the missing regression coverage on top of your commit (your commit is untouched) and I'm merging.

On the fix itself

Correct, and correct for the reason you give: recording a pointer proves the object is not pointer-free, and heap_payload_slot_selection skips the whole payload on POINTER_FREE without ever consulting the mask, so a populated mask under that state is not a cosmetic desync — it is an invisible payload. The sibling branch (no mask yet, state POINTER_FREE) already restored SIDE_MASK; this one just didn't. Strictly conservative, as you say: it can only add tracing.

I also checked the one interaction that could have made it non-orthogonal — your new set_layout_state(header, GC_LAYOUT_SIDE_MASK) against the #6893 typed-layout eviction machinery (layout_set_typed_unknown is one-way, GC_OBJ_TYPED_LAYOUT_INTACT is bit 12 of GcHeader._reserved). It is orthogonal, three ways: set_layout_state masks only GC_LAYOUT_STATE_MASK | GC_LAYOUT_ALL_POINTERS (0xE000), so bit 12 is preserved and can never be set by it; an evicted object has state GC_LAYOUT_UNKNOWN, which layout_note_slot early-returns on well before your branch; and eviction also removes the object's LAYOUT_SLOT_MASKS entry, so the masks.get_mut(..) guard your code sits behind is None for exactly the objects eviction has claimed. Lock discipline is likewise unchanged: your call sits inside the same LAYOUT_SLOT_MASKS.with(|m| { let mut masks = m.borrow_mut(); … }) borrow as the pre-existing set_layout_state in the sibling else if branch, and set_layout_state is a plain _reserved bit write that takes no lock and touches no thread-local, so there is no re-entrant borrow and no double-take of the global side-table lock.

Your beyond-length finding — you reproduced an established result

This is the part worth your time, because it's context you couldn't have had from the issue tracker alone.

Your second finding — 21,888 remaining scan offenders at element indices ≥ length, and the observation that a diagnostic which evacuated them turned the crash into a SIGSEGV — independently reproduces the fromspace-scan instrument's own validation from #7050. Measured there by extending the scan with two extra axes (is the offender's owner itself GC_FLAG_FORWARDED, and is the offending slot inside visit_gc_rewrite_slots for that owner), on every cycle of every run:

[gc-fromspace-scan OFFENDERS] missing_rewrites=2054 dangling=0 owners=3 | never_dirty=6 lost_dirty=0 dirty_but_missed=2048
[... split] live_owner_refs=6 (enumerated=0 not_enumerated=6) live_owners=2  stub_owner_refs=2048 stub_owners=1 stub_forwarding_words=0

enumerated=0 throughout. 99%+ of those offenders are (1) payload words of GC_FLAG_FORWARDED array growth stubs (#233) — dead storage by construction, only word 0 is live — and (2) uninitialized bytes inside an array's unused capacity: arena blocks are recycled rather than zeroed and move_young copies the whole payload, so a len=1 cap=16 array carries 15 slots of the previous occupant's bytes into to-space, complete with plausible-looking GcHeaders and forwarding addresses. That is exactly why evacuating them SIGSEGVs — they aren't references, they're garbage that happens to decode. The consequence recorded in #7050 is that "PERRY_GC_FROMSPACE_SCAN → zero offenders" is not an attainable acceptance criterion and never was; what is zero on every cycle is the population that would be a genuine collector defect (live owner, enumerated slot).

So your conclusion is the same one that campaign reached, arrived at independently: fix it at the producers, never by tracing more in the collector. Keeping length in sync with the filled/live extent and clearing vacated tail slots in pop / shift / unshift is the right shape — js_array_pop_f64 today just decrements length and leaves the slot bits in place, which is precisely the producer half of what you measured. That follow-up is real, wanted, and yours if you want it.

What I added on top

Your PR shipped fix + changelog only, so on top of your commit:

• crates/perry-runtime/src/gc/tests/layout_pointer_free_mask.rs — two tests. test_pointer_store_into_retained_mask_restores_side_mask pins the invariant; test_retained_mask_pointer_elements_survive_copying_minor drives the same store end to end through a real evacuating young-gen minor, asserts liveness first (copied_objects > 0, array actually relocated) so an inert collection can't produce a green cell, then asserts both masked children were evacuated and their slots rewritten, then reads a field back off each relocated child.

  Verified red against the unfixed runtime — your commit reverted locally, separate CARGO_TARGET_DIR per arm, test binaries hashed and confirmed different:

  test_pointer_store_into_retained_mask_restores_side_mask ... FAILED
    left: Some(0)   right: Some(2)
  test_retained_mask_pointer_elements_survive_copying_minor ... FAILED
    slot 0 still points at the from-space child: the minor skipped this
    array's payload, so the live child was never evacuated (#7138)
    left: 2199037411368   right: 2199037411368
  

  Both pass with your fix. Note the second one reaches the collector assertion on the unfixed arm — i.e. the copying minor ran and moved objects in both arms, so the red is the payload being skipped, not an inert collector.

• test-files/test_gap_gc_array_truncate_append.ts, registered in test-parity/gc_repsel_corpus.txt — the TypeScript-level truncate-then-append shape (length = 0, truncation to a numeric prefix, pop/shift/splice, callable elements, and a long-lived array), sized to be GC-live so the relocating arms of scripts/gc_repsel_matrix.sh aren't inert against it.

• Renamed the changelog fragment to the PR-keyed convention (changelog.d/7138-gc-pointer-free-mask-invariant.md); content is yours, unchanged.

Gates (run locally on the pinned macOS arm64 host — the CI runner backlog is hours deep)

scripts/gc_repsel_matrix.sh --arms all --pressure 8, 24 corpus files × 20 arms = 480 cells:

byte-exact vs node 26.5.1: 479/480 cells
summary: PASS=379 UNVER=100 XFAIL=1 FAIL=0

FAIL=0, and the single XFAIL is the pre-existing repsel_ptr_shape_locals × rep_ptr_shape_off entry (#6976). Liveness holds — every requires=move arm reports copy-minor 23/24, every requires=scavenge arm 14/24. The new row is PASS on all 20 arms, UNVER on none, and it is the most GC-live member of the corpus:

default          cycles=196  scavenged=8 756 303
evac_minor       cycles=149  scavenged=8 682 359
cons_scan_off    cycles=148  scavenged=8 682 359
shipped_default  cycles=274  scavenged=12 766 892   <- no pressure knob, no GC env at all

main moved three corpus rows underneath this PR while it was open, so the matrix was re-run on the merged state (current main + this PR, 27 files × 21 arms = 567 cells):

byte-exact vs node 26.5.1: 566/567 cells
summary: PASS=447 UNVER=119 XFAIL=1 FAIL=0

Still FAIL=0, same single pre-existing XFAIL, new row PASS on all 21 arms, and main's three new rows — including gc_string_literal_operand_rooting (#7114), the other evacuation-sensitive one — are unaffected by the fix.

gc-ratchet (measure --repeats 7 then check), against the pinned darwin-arm64 baseline 88dcee83b, on the same host it was captured on:

• --profile shared_ci → OK, exit 0
• --profile pinned_host → OK, exit 0 (this profile additionally gates RSS and wall time)

Gated columns (heap_used/heap_total/minor_cycles/step_cycles/copied_/promoted_/freed_bytes, 72 rows) are +0.00% on retention and cycles and within ±0.06% on the evacuation counters. That is the right answer, but bit-identical gated counters are also exactly what a vacuous measurement looks like, so the ungated columns are the proof a fresh binary was actually measured: rss_bytes −0.28%…+1.48%, wall_ms −13.17%…+3.61% across the 8 probes. Runtime archives rebuilt via -p perry -p perry-runtime-static -p perry-stdlib-static with .a mtimes confirmed to have moved (the crates are rlib-only, so building them alone links a stale archive and makes the whole measurement vacuous).

cargo test --release -p perry-runtime --lib -- --test-threads=1: 1576 passed, 0 failed, 3 ignored — including both new tests. (Worth knowing if you run the suite locally: in the dev profile it aborts on gc::tests::shadow_stack_ops::out_of_range_frame_pop_is_ignored, because js_shadow_frame_pop's corrupted-handle guard uses debug_assert!(false, …) inside an extern "C" fn — a non-unwinding panic, so SIGABRT. Pre-existing on main, invisible to CI because CI runs --release, where debug_assert! compiles out. Nothing to do with this PR; use --release.)

cargo fmt --all -- --check clean. lint's other gates (check_file_size.sh, gc_store_site_inventory.py, addr_class_inventory.py) are red — but they are red with identical offender counts on pristine origin/main (16 oversize files, 17 store sites, 4 addr-class sites), and none of the offenders is in a file this PR touches. Pre-existing repo debt, not introduced here.

One note on the fixture: it builds the desynced state with a single set_layout_state call rather than replaying a producer sequence. That's deliberate and documented in the module header — every in-crate writer that sets POINTER_FREE today also drops the LAYOUT_SLOT_MASKS entry, so no single call sequence in this crate reproduces it; the masks table is keyed by address, so the 202 occurrences you measured come from side-table reuse. The invariant under test ("a populated mask entry must never be left under POINTER_FREE") is address-local and doesn't depend on how the state got there, and pinning the test to one speculative producer route would have made it weaker.

Thanks again — good find, good writeup, and the draft framing (partial fix, named remaining cause, explicit "do not fix this in the collector") is exactly the right way to hand off a partially-understood soundness bug.