gc: no-move window for lazy intrinsic towers (#7251) + CI arm for ZEAL+VERIFY_EVACUATION (#7254) - #7723
Conversation
📝 WalkthroughWalkthroughThe runtime now protects lazy generator and typed-array intrinsic construction from moving GC, isolates intrinsic roots in tests, and adds GC-pressure and evacuation regression coverage. Workspace and documentation versions were updated to ChangesGC intrinsic construction safety
Release metadata
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TestThread
participant IntrinsicBuilder
participant GcSuppressScope
participant gc_check_trigger
participant OldReclaim
TestThread->>IntrinsicBuilder: Build intrinsic tower
IntrinsicBuilder->>GcSuppressScope: Enter no-move window
IntrinsicBuilder->>gc_check_trigger: Trigger allocation check
gc_check_trigger-->>OldReclaim: Keep collection pending
IntrinsicBuilder->>GcSuppressScope: Close scope
TestThread->>OldReclaim: Service collection on next allocation
Possibly related issues
Possibly related PRs
Suggested labels: 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 |
Wrap build_generator_tower and ensure_typed_array_intrinsic in GcSuppressScope, matching #7217's fix for populate_global_this_builtins. Convert the six intrinsic-tower AtomicI64 statics to per_test_global! so a gate test can rely on first-touch state regardless of libtest's run order (the ordering hazard that sank two of #7251's three prior gate attempts). Add crates/perry-runtime/src/gc/tests/lazy_intrinsic_towers.rs.
#7254) No CI arm previously set this pairing: gc_repsel_matrix.sh's verify_evac never sets ZEAL, its zeal-adjacent arms never set VERIFY_EVACUATION, and this script's own zeal arms never set VERIFY_EVACUATION either. Add arm 5: the pairing on the existing small fixture (non-vacuity + no false positive on known-good code) plus a pinned-regression witness against #7254's exact open reproducer (test_gap_repsel_p4a3_ptr_numarray), which must keep panicking with "stale forwarded pointer" until the underlying defect is fixed, at which point this block is the thing to delete. Not routed through gc_repsel_matrix.sh's arm registry: a sizing sweep of all 59 gc_repsel_corpus.txt files under this exact pairing found 3 confirmed panics (same signature) but also a striking concentration of multi-minute-plus runs on the test_gap_gc_* reproducer corpus -- ZEAL forces an evacuating minor at every back-edge poll, which no other matrix arm does. That population is not yet triaged (investigated under sustained host load of 30-55, which makes timeout-vs-genuine-cost undecidable), so this stays a small bounded script rather than joining --arms all today.
500b2aa to
82525e9
Compare
Merging as v0.5.1419 — and the best thing here is the gate you threw awayYou caught your own vacuous gate. The first draft armed the ArenaBytes trigger under That is the single most common way a gate ships unable to fail, and it is the third time today a first draft of a check turned out to be satisfied by "nothing happened". Discarding it rather than shipping it is the right instinct. #7251 — fixed and gated
#7254 — reproduced, gated rather than fixed, and the issue's own label is staleReproduced 3/3, with liveness proven rather than assumed: 14,072 copying-minor runs, 10,775 objects copied under The finding that isn't in the issue: the panic surface has moved. It now reads Gating rather than fixing is the right call here — this is the #7154/#7341 statepoint-liveness class, and #7211 is a direct precedent against a rushed fix. Arm 5a proves the pairing is non-vacuous and false-positive-free on known-good code; 5b is a pinned regression witness against the exact reproducer that fails loudly if the panic ever stops (fixed → promote it; changed shape → investigate). All 5 arms verified locally. The restraint I want to noteYou sized the population across all 59 corpus files — 20 PASS, 3 correctly INERT, 3 confirmed panics — and then refused to treat the 33 timeouts as a finding, because the sweep ran under sustained host load of 30–55 and timeout-vs-genuine-cost-vs-noise is undecidable from that data. Saying so plainly, and keeping the gate out of Gates 19/19. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/7723-gc-gate-arms.md`:
- Line 1: Update the changelog entry to remove the claim that both GC issues are
closed or that `#7254` is fixed. Describe the added regression coverage, while
accurately noting that the stale native-root defect remains unfixed.
In `@crates/perry-runtime/src/gc/tests/lazy_intrinsic_towers.rs`:
- Around line 50-55: Correct the intrinsic-root count from six to eight in the
documentation comment near the listed per_test_global! statics in
crates/perry-runtime/src/gc/tests/lazy_intrinsic_towers.rs lines 50-55 and in
changelog.d/7723-gc-gate-arms.md lines 24-34; make no other changes.
In `@scripts/gc_instrument_smoke.sh`:
- Line 274: Update the grep assertion in the arm 5b verification block to match
the verifier’s complete error prefix, rather than accepting any output
containing “stale forwarded pointer.” Preserve the existing failure behavior
while ensuring unrelated failures with that text do not satisfy the check.
🪄 Autofix
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: 02c89708-2195-4ae0-ba7c-f7372c47eaff
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
CLAUDE.mdCargo.tomlchangelog.d/7723-gc-gate-arms.mdcrates/perry-runtime/src/gc/tests/lazy_intrinsic_towers.rscrates/perry-runtime/src/gc/tests/mod.rscrates/perry-runtime/src/object/global_this/generator.rscrates/perry-runtime/src/object/global_this/typed_array.rscrates/perry-runtime/src/object/mod.rsscripts/gc_instrument_smoke.sh
| @@ -0,0 +1,82 @@ | |||
| Closed two GC issues in the "a real hazard exists and nothing exercises it" class. | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Do not state that #7254 is closed.
This PR does not fix the stale native-root defect. Lines 65-77 state that it remains unfixed. Describe the added regression coverage instead.
🤖 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 `@changelog.d/7723-gc-gate-arms.md` at line 1, Update the changelog entry to
remove the claim that both GC issues are closed or that `#7254` is fixed. Describe
the added regression coverage, while accurately noting that the stale
native-root defect remains unfixed.
| //! one and touch `globalThis`. FIXED HERE by converting the six statics | ||
| //! (`TYPED_ARRAY_INTRINSIC_PTR`, `TYPED_ARRAY_INTRINSIC_PROTO_PTR`, | ||
| //! `GENERATOR_FUNCTION_INTRINSIC_PTR`, `GENERATOR_INTRINSIC_PROTO_PTR`, | ||
| //! `GENERATOR_PROTOTYPE_PTR`, `ASYNC_GENERATOR_FUNCTION_INTRINSIC_PTR`, | ||
| //! `ASYNC_GENERATOR_INTRINSIC_PROTO_PTR`, `ASYNC_GENERATOR_PROTOTYPE_PTR`) | ||
| //! in `object/mod.rs` from a bare `static` to `per_test_global!` — the |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the intrinsic-root count.
The lists contain eight statics, not six: two typed-array roots and six generator-related roots.
crates/perry-runtime/src/gc/tests/lazy_intrinsic_towers.rs#L50-L55: replace “six” with “eight”.changelog.d/7723-gc-gate-arms.md#L24-L34: replace “six” with “eight”.
📍 Affects 2 files
crates/perry-runtime/src/gc/tests/lazy_intrinsic_towers.rs#L50-L55(this comment)changelog.d/7723-gc-gate-arms.md#L24-L34
🤖 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/gc/tests/lazy_intrinsic_towers.rs` around lines 50 -
55, Correct the intrinsic-root count from six to eight in the documentation
comment near the listed per_test_global! statics in
crates/perry-runtime/src/gc/tests/lazy_intrinsic_towers.rs lines 50-55 and in
changelog.d/7723-gc-gate-arms.md lines 24-34; make no other changes.
| echo " fixing it (e.g. the verifier stopped seeing the stale slot)." >&2 | ||
| exit 1 | ||
| fi | ||
| if ! grep -q 'stale forwarded pointer' <<<"$repro_out"; then |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Match the verifier failure prefix.
Line 274 accepts any nonzero failure that contains stale forwarded pointer. This can pass arm 5b for an unrelated failure that emits that text. Match the verifier error prefix so the arm enforces its stated failure mode.
Proposed fix
-if ! grep -q 'stale forwarded pointer' <<<"$repro_out"; then
+if ! grep -q 'gc evacuation verification failed: stale forwarded pointer' <<<"$repro_out"; then📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ! grep -q 'stale forwarded pointer' <<<"$repro_out"; then | |
| if ! grep -q 'gc evacuation verification failed: stale forwarded pointer' <<<"$repro_out"; then |
🤖 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 `@scripts/gc_instrument_smoke.sh` at line 274, Update the grep assertion in the
arm 5b verification block to match the verifier’s complete error prefix, rather
than accepting any output containing “stale forwarded pointer.” Preserve the
existing failure behavior while ensuring unrelated failures with that text do
not satisfy the check.
… no-op crates/perry-runtime/src/gc/tests/runtime_roots/generator_attach_prototype.rs's three tests were red on main: js_generator_attach_prototype and js_generator_attach_closure_prototype no longer moved their receiver under an alloc-point copying minor, and the shipped-default witness never saw its trigger armed. warm_generator_intrinsics() called js_generator_attach_prototype(TAG_UNDEFINED, 0) to pre-build the generator intrinsic tower before the timed call under test. That never worked: js_generator_attach_prototype returns at its very first line for any non-pointer obj, so the "warm-up" touched nothing. It went unnoticed because GENERATOR_FUNCTION_INTRINSIC_PTR and its five siblings were plain process-global AtomicI64s pre-#7723 - some earlier test in the same binary had almost always already built the tower, so the real call under test found it cached regardless of what warm_generator_intrinsics() did. #7723 converted those six statics to per_test_global! specifically so each test starts from a guaranteed first-touch state (crates/perry-runtime/src/gc/tests/lazy_intrinsic_towers.rs's whole point). That is a correct, deliberate change - it took away the accidental cross-test priming these three tests had been relying on. With nothing pre-built, the real call now pays the dozens-of-allocations tower build itself, inside build_generator_tower's GcSuppressScope (#7251's no-move window for that build). That suppression window swallows the arena trigger the test injected via arm_collection_on_next_block for the rest of the call: no copying minor ever runs before the tower build's own scope closes, and by then intermediate's own allocation no longer needs a new arena block, so the trigger is never serviced. Confirmed with instrumented gc_check_trigger / GcSuppressScope traces comparing the last-good commit against #7723: on the last-good commit the real call's first allocation reaches gc_check_trigger unsuppressed and services the trigger directly; on #7723 the entire ~1800-call tower build runs suppressed first and nothing ever re-triggers afterward. Fix warm_generator_intrinsics() to call crate::object::ensure_generator_intrinsics() directly - the same builder lazy_intrinsic_towers.rs uses - so it does what its name and doc comment always claimed. This does not touch the liveness/deferral assertions those tests make; it only repairs the test's own setup helper. Bisected via git checkout of each of today's three merges in an isolated worktree: c907953 (pre-#7721) passes; ca8c0d6 (#7721, moving-loop poll default flip) passes; cbb682d (#7723, no-move window + per_test_global towers) is the first commit where all three fail. #7724 is uninvolved.
… no-op (#7731) * fix(gc): warm_generator_intrinsics must call the tower builder, not a no-op crates/perry-runtime/src/gc/tests/runtime_roots/generator_attach_prototype.rs's three tests were red on main: js_generator_attach_prototype and js_generator_attach_closure_prototype no longer moved their receiver under an alloc-point copying minor, and the shipped-default witness never saw its trigger armed. warm_generator_intrinsics() called js_generator_attach_prototype(TAG_UNDEFINED, 0) to pre-build the generator intrinsic tower before the timed call under test. That never worked: js_generator_attach_prototype returns at its very first line for any non-pointer obj, so the "warm-up" touched nothing. It went unnoticed because GENERATOR_FUNCTION_INTRINSIC_PTR and its five siblings were plain process-global AtomicI64s pre-#7723 - some earlier test in the same binary had almost always already built the tower, so the real call under test found it cached regardless of what warm_generator_intrinsics() did. #7723 converted those six statics to per_test_global! specifically so each test starts from a guaranteed first-touch state (crates/perry-runtime/src/gc/tests/lazy_intrinsic_towers.rs's whole point). That is a correct, deliberate change - it took away the accidental cross-test priming these three tests had been relying on. With nothing pre-built, the real call now pays the dozens-of-allocations tower build itself, inside build_generator_tower's GcSuppressScope (#7251's no-move window for that build). That suppression window swallows the arena trigger the test injected via arm_collection_on_next_block for the rest of the call: no copying minor ever runs before the tower build's own scope closes, and by then intermediate's own allocation no longer needs a new arena block, so the trigger is never serviced. Confirmed with instrumented gc_check_trigger / GcSuppressScope traces comparing the last-good commit against #7723: on the last-good commit the real call's first allocation reaches gc_check_trigger unsuppressed and services the trigger directly; on #7723 the entire ~1800-call tower build runs suppressed first and nothing ever re-triggers afterward. Fix warm_generator_intrinsics() to call crate::object::ensure_generator_intrinsics() directly - the same builder lazy_intrinsic_towers.rs uses - so it does what its name and doc comment always claimed. This does not touch the liveness/deferral assertions those tests make; it only repairs the test's own setup helper. Bisected via git checkout of each of today's three merges in an isolated worktree: c907953 (pre-#7721) passes; ca8c0d6 (#7721, moving-loop poll default flip) passes; cbb682d (#7723, no-move window + per_test_global towers) is the first commit where all three fail. #7724 is uninvolved. * changelog: add fragment for #7731 (generator-attach-pacing) * chore: bump version to 0.5.1422 Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
1 participant
Summary
Two GC issues, both "a real hazard exists and nothing exercises it" — closes #7251, closes #7254.
#7251 — no-move window for the two lazy intrinsic-tower builders
build_generator_tower(object/global_this/generator.rs) andensure_typed_array_intrinsic(object/global_this/typed_array.rs) build the same shape of immortal object graph #7217 fixed forpopulate_global_this_builtins: raw*mut ObjectHeader/*mut ClosureHeaderlocals threaded across a dozen-plus allocating installs, with no root the collector knows about until the finalAtomicI64::store. Both now open aGcSuppressScopefor the whole build, matching #7217's fix exactly.The issue's own blocker was the gate, not the fix — three prior attempts all passed with the window deleted:
populate_global_this_builtinsgate's own approach) never reachesgc_check_trigger()at all: a tower is three orders of magnitude smaller than an arena block. Fixed by forcing the current block (very nearly) full before calling the builder, so its first allocation unconditionally takes the slow path.TYPED_ARRAY_INTRINSIC_PTR,GENERATOR_FUNCTION_INTRINSIC_PTR, …) were plain process-globalAtomicI64s — built once per process, so whichever test ran first on that binary poisoned every later test's "never built" precondition. Fixed by converting them toper_test_global!(the mechanism Process-global side tables are cleared by test guards that readers are not required to take — three flakes in two days #7672 built for exactly this ordering hazard): each libtest thread gets its own zeroed instance, and non-test builds expand to the identical barestatic.gc_is_suppressed()from inside the builder readtrueeven with the scope removed, because it was leaking attempt 2's ordering hazard in a different shape (an earlier test'spopulate_global_this_builtinscall opens its own outer window around both builders). Sidestepped by calling the tower builders directly, never through the bootstrap.force_legacy_gc_pacing, and it also passed with the fix deleted — vacuously. Under that pacing, an ArenaBytes trigger being "due" only starts a budgeted cycle that needs an explicit host-safepoint pump to advance; nothing pumps one inline, sogc_collection_count()never moves whether or not the builder is suppressed. Fixed by armingGC_OLD_RECLAIM_PENDINGinstead — the one branchgc_check_triggerservices synchronously regardless of pacing, and the exact leverglobal_bootstrap.rs's own GC: #7207's spread-accessor reproducer still SIGSEGVs on main under the evac_minor arm (allocation-point relocation, not the safepoint route) #7217 gate uses.New test:
crates/perry-runtime/src/gc/tests/lazy_intrinsic_towers.rs, two tests (generator_tower_runs_in_a_no_move_window,typed_array_intrinsic_tower_runs_in_a_no_move_window), each asserting both halves (CLAUDE.md's fourth way a gate can't fail): no collection ran during the builder call, AND the same armed request is serviced once the window closes.Sabotage, run by hand: commenting out either
GcSuppressScope::new()line turns its test red (a collection ran inside …), confirmed on this branch after the pacing fix (attempt 4's false start would not have caught it — verified that too, by running the vacuous first draft against the sabotaged tree and watching it pass).#7254 — PERRY_GC_ZEAL + PERRY_GC_VERIFY_EVACUATION, exercised in CI for the first time
Reproduced, 3/3, on current
main(post-#7719). Repro exactly as filed:Liveness proven via
PERRY_GC_DIAG=1, not merely a clean exit: 14,072 copying-minor runs, 10,775 objects actually copied across the run. The instrument was genuinely armed.New finding, not in the issue: the panic surface has moved. It now reads
"stale forwarded pointer in native stack-map roots", not"shadow stack roots".gc/roots/stack_maps.rs'svisit_stack_map_root_slotsis the statepoint/stack-map native root walker (docs/src/internals/rfc-rooting-by-construction.md's layer 2), which is live on this aarch64 host. So on currentmainthis is a layer-2 (emitted-code-liveness) defect, not layer-3 (hand-written runtime Rust) as the issue's own comment classified it — the classification predates the statepoint route becoming reachable here. Filing this as new evidence on #7254 rather than silently changing the issue's framing.Sized the population (the issue's own suggested first step), all 59
test-parity/gc_repsel_corpus.txtfiles under the exact pairing:test_gap_specabi_{reassign,recursion_escape,view_detach}— zeal correctly self-reported no collection ran; not a defecttest_gap_repsel_p4a_inline_tiers,test_gap_repsel_p4a3_ptr_numarray(the filed repro),test_gap_repsel_element_shape_loop_clone— all three the identicalnative stack-map rootssignatureCaveat on the 33 timeouts, stated plainly rather than folded into a claim: this sweep ran under sustained host load of 30–55 (20 concurrent users, confirmed via
uptimethroughout, not a transient spike) and competed with this same PR's own concurrent rebuilds. I cannot currently separate "host contention" from "ZEAL forcing an evacuating minor at every back-edge poll is just legitimately much more expensive than any other matrix arm on tight-loop reproducers" from "a genuine second defect" for those 33 files. The concentration is suspicious — it is almost the entiretest_gap_gc_*rooting-reproducer corpus specifically — so it is worth a dedicated follow-up on a quiet host with a longer per-file budget, but asserting more than that here would be exactly the overclaim CLAUDE.md's knob-kill policy warns about.What's fixed vs. gated, and why: the panic itself is NOT fixed in this PR. This is a statepoint/native-root-walker liveness bug (layer 2), the same class CLAUDE.md documents as needing careful, disassembly-driven campaigns (the
#7154/#7341family) — not something to rush without conclusive root-causing, given the project's own cautionary precedent (#7211: an author actively thinking about rooting still shipped a wrong four-clause predicate). A wrong fix here risks masking a real bug rather than closing it. A gate that pins the defect is shipped instead, per the task's own explicit allowance: "a gate with no fix is still better than today's nothing."Why not
gc_repsel_matrix.sh: azeal_verifyarm registered there joins every corpus file via--arms all(nightly/push), and the 90-minute job budget cannot responsibly absorb an unsized population of multi-minute-plus cells discovered under contested conditions. Extendedscripts/gc_instrument_smoke.shinstead (already wired intogc-stress's required-ish per-PR step) with a new arm 5, bounded and fast:copied_objects>0under the verifier) — proves the pairing is non-vacuous and produces no false positive on known-good code.test_gap_repsel_p4a3_ptr_numarray: asserts it panics withstale forwarded pointer, and fails loudly (with instructions) if that ever stops being true — either because the defect got fixed (delete the block, promote the file) or because the failure silently changed shape (needs a fresh look, not silence).Verified locally end-to-end (
./scripts/gc_instrument_smoke.sh <perry-dev-binary>): all 5 arms pass, including 5a (3,894 objects copied, live) and 5b (reproduces#7254as pinned).What this PR does NOT do
scripts/gc_gate_wiring_check.py'sGATESregistry — both additions extend scripts already invoked by the existinggc-stressjob (test.yml), so no new registration is needed.Test plan
crates/perry-runtime/src/gc/tests/lazy_intrinsic_towers.rs: 2/2 passing on this branch (rebased ontoorigin/main@ 72ef47a)GcSuppressScopeis removed (verified twice — once before the pacing-mechanism fix, which incorrectly stayed green, and once after, which correctly went red)./scripts/gc_instrument_smoke.sh <perry-dev binary>: all 5 arms pass locally, including the new gc: PERRY_GC_ZEAL + PERRY_GC_VERIFY_EVACUATION reports a stale forwarded pointer in shadow-stack roots (10/10, pre-existing, no CI arm sets the pair) #7254 pairing (5a non-vacuous + correct, 5b pins the known reproducer)bash -n+shellcheckclean on the modified scriptcargo check -p perry-runtime -p perry-runtime-static -p perry-stdlib -p perry-stdlib-static --tests: 0 errorscargo fmt --all -- --check: cleancargo test --release --workspace(not run in this session — scoped--libruns above cover the changed surface; left for CI)Summary by CodeRabbit
Bug Fixes
Tests
Chores