fix(codegen): make every GC root store dominate the collection points after it

[jdalton]

The invariant

A GC-managed value's root store must DOMINATE every subsequent site that can trigger a collection.

#7184 fixed one way to break it: the store is emitted, but its shadow-slot index falls outside the pushed frame, so js_shadow_slot_bind (and #7088's inline store) bounds-checks it and silently no-ops. This PR fixes its sibling: the store is emitted in-frame, but late — after a call that allocates, which under PERRY_GC_MOVING_LOOP_POLLS=1 reaches a back-edge poll and an evacuating minor.

Both present identically, and both are invisible to every runtime GC probe: at the moment of the collection there is nothing for the collector to find. That is why the #7154 hunt kept measuring correct layout coverage on offenders whose targets had already died — the miss is a mutator window, not a heap, root, or remembered-set defect.

The four sites

1. new C(…) — lower_call/new.rs (the load-bearing one)

%inst = call i64 @js_object_alloc_class_inline_keys(...)   ; fresh Eden object
%box  = <nanbox %inst>
%ret  = call double @C_constructor(double %box, ...)       ; allocates; POLLS
call void @js_gc_init_typed_shape_layout(i64 %inst, ...)   ; %inst is now STALE
%v    = call double @js_ctor_return_override(%box, %ret, 0); publishes the STALE addr
store double %v, ptr %local
call void @js_shadow_slot_bind(i32 N, ptr %local)          ; a ROOTED dangling pointer

The instance is rooted inside the callee — the this parameter has a shadow slot — so the minor does not free it. It moves it and rewrites the callee's root. The caller's register is not a root, so it keeps the from-space address; js_gc_init_typed_shape_layout installs the layout descriptor on the abandoned copy, and js_ctor_return_override writes that dead address into the caller's shadow slot.

That is exactly #7154's fingerprint: a rooted slot holding a dangling pointer, so the from-space scan only ever sees offenders one or more cycles after the target died, with correct layout coverage, injected by the mutator between cycles.

Fixed by temp-rooting the instance across the constructor and re-reading it afterwards, on both the standalone-<Class>_constructor symbol path (the default since PERRY_INLINE_CTOR was inverted) and the inline-ctor path. A class with no constructor, no fields and no heritage runs no user code in that window and keeps its previous IR exactly — js_gc_init_typed_shape_layout is the only thing emitted in between and it does not allocate.

2. Expr::ObjectSpread — expr/logical_collections.rs

{ ...a, k: f() } allocated the object and then wrote every field through a register held across each part's lowering, with no rooting at all. Expr::Object has used RootedHandle for this since #6951; the spread form's own comment says it takes "the same js_object_set_field_by_name path as Expr::Object" but it never copied the rooting.

3. Expr::ClassExprFresh — expr/static_field_meta.rs

Same shape for the fresh class object a class-expression factory returns, across its static-field initializers, captured-argument snapshot, symbol statics and static { … } blocks. The capture snapshot forces protection on its own: it allocates a js_array_alloc accumulator and grows it with js_array_push_f64 per element, which are collection points even when every element is an inert LocalGet.

4. Property / element stores — expr/property_set.rs, expr/index_set.rs

o.k = f() and o[k] = f() evaluate the reference first and the value second (spec order), leaving the receiver in a register across f(). The slot it was loaded from is a root and gets rewritten; the register does not, so the store lands in from-space and the field never appears on the object the program keeps.

This is #7114 with a receiver instead of a string literal — the same "property (2), a rewritten location, is worthless without property (3), reading that location again below the collection point" that temp_root.rs's own module header describes.

A new temp_root::ReceiverGuard roots the receiver only when the value expression can collect, so stores with an inert RHS keep their previous IR. A temp root, not a re-load, is required: re-lowering object would observe an assignment made by f() itself, which is a miscompile rather than a rooting fix.

temp_root_scope_begin now takes the caller's extra reason to open a scope — new C() with no arguments still needs a marker to cut the instance root against.

The instrument: scripts/gc_root_dominance_check.py

The checker that found all four, included as a debug tool. It parses perry-emitted LLVM IR, builds each function's CFG, computes real Cooper/Harvey/Kennedy dominance, and reports every shadow-slot root store that does not dominate a preceding collection point, naming the intervening call.

Real dominance and path-based windows matter: a naive line-order scan produced 8 false positives on this corpus where this reports none, because a loop back edge is not an intra-iteration path. It is one-sided by design — NONCOLLECTING is the only place a call is declared safe and every entry cites the runtime source line that proves it, so a missing entry costs a false positive and never a missed bug. Exits non-zero on any violation, so it can gate.

PERRY_GC_MOVING_LOOP_POLLS=1 PERRY_INLINE_SHADOW_SLOT=0 perry compile app.ts -o /tmp/app --trace llvm
python3 scripts/gc_root_dominance_check.py .perry-trace/llvm -v

Verification

	result
test_gap_gc_new_instance_rooting.ts, POLLS=1, pure 73a9084ea	bad 9 (expected 0) — 3/3, deterministic
same, default (no polls), pure 73a9084ea	bad 0 — 5/5
same, POLLS=1, this PR	bad 0 — 10/10
same, default, this PR	bad 0 — 5/5
static checker over the 196-module sfw-registry IR corpus	234 → 1 violation
cargo test -p perry-codegen	6 failures, identical to main: the loop_safepoint_purity set from #7161's default flip
cargo test -p perry-runtime	unchanged — this PR touches only perry-codegen, which perry-runtime does not depend on
sfw-registry --help, default arm	clean 5/5 (no regression)
sfw-registry --help, POLLS=1 arm	still red 5/5

New codegen regression test the_new_instance_is_rooted_across_the_constructor_body pins the def-use chain — the value js_ctor_return_override publishes must be re-derived from the instance's temp root — plus a negative test that a class running no user code emits no instance root at all. Assertions are on the def-use chain rather than textual order, because the override is emitted into the ctor.return.after block, which the writer appends below the block that re-reads the root.

What this does NOT close

sfw-registry --help under PERRY_GC_MOVING_LOOP_POLLS=1 is still red, so this does not close #7154 and stopgap #7161 stays. At least one more site of this class remains.

Two concrete leads for whoever picks it up:

1. The residual is not another instance the alloc-anchored checker can see — it is down to 1 violation on the whole corpus. The remaining shapes it does not model are (a) heap values held in plain, non-shadow allocas (the inline-ctor path's this_slot/ctor_result_slot, and the [N x i64] closure-capture staging array), and (b) the general stale-register invariant — any register holding a heap value that is live across a collecting call and then used without being re-read from a root. A prototype of (b) is straightforward on top of the shipped checker's CFG/dominance layer; it reported ~1000 raw hits on the corpus before allowlist tuning, so it needs the NONCOLLECTING set extended before it is actionable.
2. Separately worth a look while in this area: a repro of the form { ...src, k: churn() } read back with typeof o.k !== "number" reports a false typeof while String(o.k) prints the correct number, under POLLS=1 only and clean under PERRY_GEN_GC=0. The value survives; the typeof/string-comparison path does not. That is collector-mode-dependent and looks like its own bug, not this class.

Refs #7154, #7184, #7161, #7114, #6951.

Summary by CodeRabbit

• Bug Fixes

  • Improved reliability during garbage collection for object construction, class initialization, object spreading, and property or element updates.
  • Prevented newly created objects and receivers from being lost or corrupted while allocation-heavy operations are in progress.
  • Added safeguards for constructors and static initialization that perform additional allocations.

• Tests

  • Added regression coverage for object construction and garbage-collection scenarios.

• Chores

  • Updated the application version to 0.5.1277.

[coderabbitai]

📝 Walkthrough

Walkthrough

The change adds temporary rooting for receivers, object spreads, fresh classes, and constructed instances across GC-triggering operations. It adds regression tests, an LLVM IR dominance checker, changelog documentation, and version updates.

Changes

GC root relocation protection

Layer / File(s)	Summary
Temporary-root infrastructure crates/perry-codegen/src/expr/temp_root.rs	Added receiver guards, reread helpers, release helpers, and conditional temporary-root scopes.
Constructor instance rooting crates/perry-codegen/src/lower_call/new.rs, crates/perry-codegen/tests/temp_root_operand_temporaries.rs	Constructed instances are rooted across user code and reloaded before layout and return handling. Tests cover rooting and the no-user-code path.
Store and expression rooting crates/perry-codegen/src/expr/index_set.rs, crates/perry-codegen/src/expr/property_set.rs, crates/perry-codegen/src/expr/logical_collections.rs, crates/perry-codegen/src/expr/static_field_meta.rs	Property stores, index stores, object spreads, and fresh class expressions reload rooted objects after potentially collecting evaluations.
Validation and diagnostics scripts/gc_root_dominance_check.py, test-files/test_gap_gc_new_instance_rooting.ts, changelog.d/7192-root-store-dominance.md, CLAUDE.md, Cargo.toml	Added moving-GC regression coverage and an LLVM IR dominance checker. Documented the changes and updated the version to 0.5.1277.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Codegen
  participant TempRootStack
  participant Runtime
  participant MovingGC
  Codegen->>TempRootStack: push receiver or allocated object
  Codegen->>Runtime: evaluate allocating RHS, initializer, or constructor
  Runtime->>MovingGC: trigger collection
  MovingGC-->>TempRootStack: relocate rooted value
  Codegen->>TempRootStack: reread relocated value
  Codegen->>Runtime: perform store or return operation
  Codegen->>TempRootStack: release temporary root

Loading

Possibly related issues

• gc: constructor arguments on lower_new's non-class branches (Readline / imported ctor / new Function) and builtin.rs are still not precise roots #6986 — Both changes modify constructor GC-rooting paths in lower_call/new.rs.
• gc: PERRY_GC_VERIFY_EVACUATION reports clean on a demonstrable missing rewrite (verifier only self-checks the rewrite pass's own enumeration) #7035 — The new LLVM IR dominance checker addresses GC verification tooling concerns.

Possibly related PRs

• PerryTS/perry#6972 — Both changes add precise temporary-rooting helpers in temp_root.rs.
• PerryTS/perry#6983 — Both changes modify temporary-root handling, constructor lowering, and regression tests.
• PerryTS/perry#7161 — Both changes address GC relocation safety in code generation.

Suggested labels: bug

Suggested reviewers: proggeramlug

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The PR addresses a related code-generation failure but does not fix the linked issue's remembered-set or write-barrier defect and explicitly leaves the reproduction failing.	Link this PR to a codegen-specific issue or implement the remaining #7154 remembered-set or write-barrier fix and pass the original reproduction.
Out of Scope Changes check	⚠️ Warning	The PR includes unrelated release metadata changes to Cargo.toml, CLAUDE.md, and changelog.d/7192-root-store-dominance.md.	Remove the version and release-metadata changes; maintainers should apply them during the merge or release process.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the code-generation change that makes GC root stores dominate later collection points.
Description check	✅ Passed	The description clearly covers the scope, implementation, related issues, verification, and known limitations, despite omitting some template headings.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.}

[coderabbitai]

Actionable comments posted: 5

🧹 Nitpick comments (4)

crates/perry-codegen/tests/temp_root_operand_temporaries.rs (2)

849-857: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Scope the gate assertion to the init function, and assert the scope marker.

The assertion searches the whole module IR. Any unrelated function that later emits js_gc_temp_root_push fails this test for the wrong reason. The other test in this file already narrows with init_ir.

The doc comment also promises "no scope marker either", but no assertion covers the scope marker.

♻️ Proposed refactor

 fn a_class_that_runs_no_user_code_emits_no_instance_root() {
     let ir = ir_for_new("new_inst_no_ctor.ts", vec![Expr::Number(1.0)]);
+    let f = init_ir(&ir);
     assert!(
-        !ir.contains("call i32 `@js_gc_temp_root_push`"),
+        !f.contains("call i32 `@js_gc_temp_root_push`"),
         "nothing can collect between the allocation and the `new` value, so \
-         rooting the instance would be pure TLS traffic:\n{ir}"
+         rooting the instance would be pure TLS traffic:\n{f}"
     );
+    assert!(
+        !f.contains("call void `@js_gc_temp_root_truncate`"),
+        "no temp-root scope marker may be opened for this class either:\n{f}"
+    );
 }

🤖 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-codegen/tests/temp_root_operand_temporaries.rs` around lines 849
- 857, Update a_class_that_runs_no_user_code_emits_no_instance_root to inspect
the init function IR via the existing init_ir helper rather than searching the
whole module. Assert that init_ir contains neither js_gc_temp_root_push nor the
promised scope marker, preserving the test’s no-root and no-scope behavior.

---

782-798: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Use a trim-based def lookup and include phi operands in the use chain.

def_of can miss valid two-space-emitted definitions if any later IR transformation changes spacing, and first_operand_reg drops all but the first operand. phi instructions use multiple operands, so the walk must push every operand onto the worklist or the test panics even for correct IR; report the visited set on failure.

🤖 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-codegen/tests/temp_root_operand_temporaries.rs` around lines 782
- 798, Update the temporary-register analysis helpers def_of and
first_operand_reg: make definition lookup trim each IR line before matching the
register assignment, and collect every register operand from phi instructions
rather than only the first. Ensure the worklist traverses all phi operands and
include the visited-register set in failure diagnostics.

scripts/gc_root_dominance_check.py (2)

514-514: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Remove the unused order map, and record the binds the checker cannot pair with a store.

Two points in check_func:

• Line 514 builds order, and nothing reads it.
• Lines 583-590 search for the activating store only inside bind_ins.block, at indices below the bind. If the emitter puts the store in a predecessor block, store_ins stays None and the bind is dropped without any record. That is another silent false negative against the one-sided soundness claim.

Count the skipped binds and report the count, so a coverage gap is visible instead of invisible.

Also applies to: 583-590

🤖 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_root_dominance_check.py` at line 514, In check_func, remove the
unused order map. Update the bind-to-store search around bind_ins.block so binds
with no matching store, including stores in predecessor blocks, are counted
rather than silently dropped; report the skipped-bind count while preserving
existing pairing behavior.

---

84-84: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Clear the three Ruff findings.

Ruff 0.16.0 reports:

• Line 84: Insn.__slots__ is not sorted (RUF023).
• Line 257: and is chained with or without parentheses (RUF021). The precedence here is easy to misread, and the trailing startswith("%") and "= load" in d.text clause is already covered by the " load " in d.text test.
• Line 338: node is unpacked but never used (RUF059).

🧹 Proposed fixes

-    __slots__ = ("text", "block", "idx", "result", "callee")
+    __slots__ = ("block", "callee", "idx", "result", "text")

-        if d.callee is not None or " load " in d.text or d.text.strip().startswith("%") and "= load" in d.text:
+        if d.callee is not None or " load " in d.text:
             origins.append(d)

-            node, it = stack[-1]
+            _node, it = stack[-1]

Also applies to: 257-257, 338-338

🤖 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_root_dominance_check.py` at line 84, Clear the three Ruff findings
in the affected code: sort Insn.__slots__ lexicographically; parenthesize the
mixed and/or condition at line 257 and remove the redundant trailing
startswith("%") and "= load" clause, since the existing " load " check covers
it; and replace the unused node binding in the unpacking at line 338 with an
ignored binding.

Source: Linters/SAST tools

🤖 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-codegen/src/expr/index_set.rs`:
- Around line 1482-1494: Root the dynamically lowered key in the string-key arm
of crates/perry-codegen/src/expr/index_set.rs at lines 1482-1494 across
lower_value_for_dynamic_index_set, re-read it beside reread_store_receiver, and
release it before recv_guard at line 1530; the literal-key arm needs no change.
In crates/perry-codegen/src/expr/static_field_meta.rs lines 504-510, root k
across init lowering and re-read it before js_object_set_symbol_property,
alongside the existing obj rooted-handle handling.

In `@crates/perry-codegen/src/expr/logical_collections.rs`:
- Around line 925-927: Update the protection predicates at
crates/perry-codegen/src/expr/logical_collections.rs#L925-L927 and
crates/perry-codegen/src/expr/static_field_meta.rs#L439-L442 to account for
emitted collection points, not only operand expressions. In
logical_collections.rs, force protection when any spread part exists or when
there is more than one part. In static_field_meta.rs, hoist the block_fns
computation before rooted_handle_begin, include !block_fns.is_empty(), and force
protection when more than one named static is emitted.

In `@crates/perry-codegen/src/lower_call/new.rs`:
- Around line 1866-1870: Refresh the constructor result value after the inlined
constructor body may trigger GC: in the fall-through path around
reload_instance, reload ret.result_slot and use that refreshed value for
js_ctor_return_override, or store it back into ctor_result_slot before the call.
If retaining the slot, root it with the established nanbox root-store mechanism
so it cannot hold a pre-move obj_box.

In `@scripts/gc_root_dominance_check.py`:
- Around line 638-655: Update main to use argparse for the supported flags
instead of manually filtering sys.argv, and require at least one discovered .ll
file from the supplied paths. If no valid .ll files are found—including for
missing, invalid-directory, or --help-only invocations—emit an error and return
a nonzero status before printing the zero-file summary.
- Around line 124-135: Update the parser’s block-handling logic around LABEL_RE
and curblk so implicit numeric LLVM basic-block labels are either represented as
valid blocks with correct CFG relationships or cause parsing to reject the IR.
Do not map implicit labels to %entry.implicit or treat that synthetic name as
the real entry block; ensure explicit blocks with that name and explicit numeric
labels remain unambiguous for dominance analysis.

---

Nitpick comments:
In `@crates/perry-codegen/tests/temp_root_operand_temporaries.rs`:
- Around line 849-857: Update
a_class_that_runs_no_user_code_emits_no_instance_root to inspect the init
function IR via the existing init_ir helper rather than searching the whole
module. Assert that init_ir contains neither js_gc_temp_root_push nor the
promised scope marker, preserving the test’s no-root and no-scope behavior.
- Around line 782-798: Update the temporary-register analysis helpers def_of and
first_operand_reg: make definition lookup trim each IR line before matching the
register assignment, and collect every register operand from phi instructions
rather than only the first. Ensure the worklist traverses all phi operands and
include the visited-register set in failure diagnostics.

In `@scripts/gc_root_dominance_check.py`:
- Line 514: In check_func, remove the unused order map. Update the bind-to-store
search around bind_ins.block so binds with no matching store, including stores
in predecessor blocks, are counted rather than silently dropped; report the
skipped-bind count while preserving existing pairing behavior.
- Line 84: Clear the three Ruff findings in the affected code: sort
Insn.__slots__ lexicographically; parenthesize the mixed and/or condition at
line 257 and remove the redundant trailing startswith("%") and "= load" clause,
since the existing " load " check covers it; and replace the unused node binding
in the unpacking at line 338 with an ignored binding.

🪄 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: c9ba5bd3-f744-47d8-8f67-e0e6667577ec

📥 Commits

Reviewing files that changed from the base of the PR and between 5ef83bb and 7eb2aeb.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (12)

• CLAUDE.md
• Cargo.toml
• changelog.d/7192-root-store-dominance.md
• crates/perry-codegen/src/expr/index_set.rs
• crates/perry-codegen/src/expr/logical_collections.rs
• crates/perry-codegen/src/expr/property_set.rs
• crates/perry-codegen/src/expr/static_field_meta.rs
• crates/perry-codegen/src/expr/temp_root.rs
• crates/perry-codegen/src/lower_call/new.rs
• crates/perry-codegen/tests/temp_root_operand_temporaries.rs
• scripts/gc_root_dominance_check.py
• test-files/test_gap_gc_new_instance_rooting.ts

[proggeramlug]

Post-merge verification of this PR's review round is in #7198. Summary of what the four findings turned out to be, so the threads above are not the only record:

• lower_call/new.rs:1870 — real, and it defeated this PR's fix at its last instruction. ctor_result_slot is a plain alloca_entry the collector never rewrites, so seeding it with obj_box parked the pre-constructor instance address in unrooted memory; on fall-through js_ctor_return_override saw an object in raw and returned that, discarding the value reload_instance had just re-read. Not gated on PERRY_INLINE_CTOR — force_ctor_call requires class.constructor.is_some(), so any class with fields or heritage but no own constructor takes the inline path by default. the_new_instance_is_rooted_across_the_constructor_body could not catch it: it walks the first operand of the override, which is the re-read one.
• index_set.rs:1494 — real. The computed key sits in the same window as the receiver.
• logical_collections.rs:927 — half real. User-code re-entry inside the helper (an accessor on a spread source, a static { … } body) is the route; "N js_object_set_field_by_name calls" is not — an allocation inside a runtime helper cannot initiate a moving collection in any shipped configuration.
• gc_root_dominance_check.py:135 — rejected as written, but the adjacent defect was worse: a label-less function parsed to zero blocks and was silently skipped, so a planted violation vanished and the run exited 0. That plus four other ways the checker could not fail are fixed in fix(codegen): close #7192's own residual root-store hole, and make the dominance checker able to fail #7198, along with PERRY_SAVE_LL never being honoured for split modules (so --trace llvm emitted nothing for exactly the largest modules).

Two corrections to this PR's fragment, both in #7198:

1. Expr::ObjectSpread is JSX-only. Since perry-codegen: object literal with computed-key props + computed-key methods + cross-module spread drops keys & mis-resolves methods — Effect HashRing.ts blocker (post-#740) #809 an object literal containing a spread lowers to a source-ordered IIFE built on js_object_assign_one (lower/expr_object.rs:844); the sole construction site of ObjectSpread is a JSX spread attribute (crates/perry-hir/src/jsx.rs:67). The zod-269-key-spread claim is wrong — the fix is still right, its blast radius is JSX.
2. The version bump collided. main reached 0.5.1277 via fix(hir): fold an imported binding's array-named method only with array evidence #7188 while this was in review, so both sides matched and this merged with no net bump.

Verified as merged, on origin/main + #7198, release build, Node 26.5.1: test_gap_gc_new_instance_rooting.ts is bad 0 5/5 under PERRY_GC_MOVING_LOOP_POLLS=1 and bad 0 3/3 under the shipped default, byte-exact against the oracle — and it is bad 9 deterministically 3/3 at 73a9084ea under polls, so the =1 arm is demonstrably not dark.

#7161 is not revertible yet, and not only because of sfw-registry. Two locally-reproducible SIGSEGVs (exit=139) remain under polls, both present at 73a9084ea and both clean under the default: { ...src, k: v } where src carries an accessor, and a class expression with a static { … } block. Details and reproducers in #7198.