Skip to content

fix(gc): close #7210's two remaining unrooted-alloca sites + #7640 A/C findings - #7736

Open
proggeramlug wants to merge 3 commits into
mainfrom
gc/7210-7640-rooting-residue
Open

fix(gc): close #7210's two remaining unrooted-alloca sites + #7640 A/C findings#7736
proggeramlug wants to merge 3 commits into
mainfrom
gc/7210-7640-rooting-residue

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Summary

Closes out #7210's two named remaining sites, plus part of #7640's sections
A and C. Full accounting below, including what was not reached — several
things turned out not to be hazards, reported as findings rather than
shortfalls.

#7210 — both named sites, fixed

  • crates/perry-codegen/src/codegen/helpers.rs's emit_namespace_populator
    (§2's flagship site): vals_buf staged every exported binding's NaN-boxed
    value in a plain stack alloca while the per-entry loop called allocating
    helpers (js_closure_alloc_singleton, arbitrary cross-module getters). An
    already-staged entry had no root and could go stale before
    js_create_namespace read the whole buffer. Fixed by rooting each value in
    a RootedGroup as it's produced and deferring every vals_buf store to a
    second, call-free pass that runs immediately before the consuming call.
  • crates/perry-codegen/src/lower_call/early_branches.rs's
    obj[strKey](args) computed-key dispatch (§3's flagship site): receiver,
    key and every argument were lowered into bare registers in sequence, and in
    the static-string-key arm unbox_str_handle (an allocating SSO
    materialisation) ran between the args buffer's stores and the consuming
    call. Fixed by rooting [object, index, ...args] in one RootedGroup and
    building the args buffer last, in each branch, after unbox_str_handle.

#7640 section A — 3 of ~7 arms fixed

Not reached: the #5525 recv_unknown inline dyn-TA store, and the TA
runtime-key / TA final-fallback / Uint8Array runtime-key arms.

#7640 section C — resolved (not a mechanical fix)

Two property_set.rs comments claimed a class-field store's receiver
survives an allocating RHS via "the same statepoint re-read" a sibling arm
relies on. That mechanism doesn't exist (RS4GC only relocates values still
ptr addrspace(1)-typed and live across the safepoint; the receiver crosses
it as a plain double, per function/precise_roots.rs). What's actually
going on:

  • Bare Expr::LocalGet/Expr::This receiver: the claim is TRUE, via
    root_reload.rs (GC: #7154's residual is NOT fixed — the loop-polls config is red 0/30, and stock zod alone fails 5/40 #7280) — a front-end pass, independent of RS4GC, that
    re-materialises a value derived from a shadow-slot or handle-global load
    below any collection point it doesn't dominate. Verified with
    scripts/gc_root_dominance_check.py --stale-registers/--statepoints on
    both lowerings against a new fixture — zero hazards.
  • Compound receiver — this.target.x = allocPoint(n).x, receiver itself a
    class-field READ: the claim is FALSE
    , and invisible to both root_reload
    (the receiver is a phi over two field-get paths, not a direct shadow-slot
    load) and the --stale-registers checker (its pattern match only anchors
    on a direct load double, ptr <root> source). Confirmed by hand in the
    emitted IR for exactly this shape.

Left unfixed on purpose: rooting the receiver unconditionally would tax the
dominant LocalGet/This case this investigation just proved needs
nothing, on what #7640 itself calls "the hottest store path in the
compiler" — a measured-cost tradeoff the issue explicitly deferred to a
follow-up that can benchmark it. All four property_set.rs sites that made
or relied on the original claim now say this precisely.

New fixture: test-files/test_gap_gc_class_field_receiver_rooting.ts
exercises both halves under PERRY_GC_MOVING_LOOP_POLLS=1 allocation
pressure, and is also a real Node-comparison functional-correctness check
independent of either checker.

#7640 section E — triaged, not fixed

All seven named callees
(ptr_numarray_access::try_lower_num_array_guard_free_{set,get},
try_lower_proven_view_checked_store, lower_typed_array_store,
lower_buffer_store, lower_index_set_fast,
masked_window::lower_masked_window_index_get,
property_get/generic_dispatch.rs::lower_generic_property_get) were read,
not skipped. Each looks like a hazard that is not one:

  1. Single, already-rooted callerlower_index_set_fast's only call
    site is inside the with_operands_rooted group gc(layer 3): from-space quarantine catches 55 stale dereferences across the gap suite — the instrument is in CI but aimed at one synthetic fixture #7341 already
    established; try_lower_num_array_guard_free_{get,set}'s object is
    structurally constrained to Expr::LocalGet and its one caller each
    doesn't interleave any other lowering around it.
  2. Address derived after the allocating step, not before
    try_lower_proven_view_checked_store lowers value first, then derives
    the data pointer.
  3. Typed-array immovability, per CLAUDE.md's own GC section —
    lower_typed_array_store/lower_buffer_store cache a data pointer across
    an allocating RHS, but into a GC_FLAG_TENURED, non-movable backing
    store (same category GC: the remaining unrooted-alloca hazards after #7207 — class-keys pointer caches, interleaved staging arrays, inlined-callee param slots #7210 section 5 already flagged, not a new finding).
  4. lower_generic_property_get/lower_masked_window_index_get have no
    second allocating operand in their own window at all.

This triage is static, not checker-verified per callee — treat it as a
lead for whoever picks up the rest of E, not a closed finding.

What I verified, and against which commit

The branch is rebased onto origin/main@f28657da0 (v0.5.1426 — post #7724's
phi-false-positive fix, post #7732's native-lowering-to-0-unrooted,
--max-unrooted deleted from the gate). A prior verification pass (both
gated checker modes, PERRY_RUNTIME_DIR pinned, full corpus, empty
allowlist) was clean on an earlier commit of this branch, but that was
measured against v0.5.1420 — before #7732, so it is not the number this
PR should be judged on. Re-running now on the current HEAD and will post
the fresh before/after (both lowerings, gated invocations) as a PR comment
as soon as the release rebuild finishes
— opening the PR itself rather
than holding it for that run to land, per the standing guidance that an
unpushed/unopened branch is worth nothing.

Codegen test baseline

cargo test -p perry-codegen --no-fail-fast will be re-run against the same
HEAD and posted as a comment. The last full run (also pre-#7732, pre-#7730)
showed 4 failing integration-test targets matching #7708's pre-existing set;
#7730 ("resolve #7494's four lowering-independent proof-test failures",
merged since) looks likely to have fixed exactly those, which the fresh run
will confirm or correct.

Test plan

  • cargo check -p perry-codegen
  • cargo fmt --all -- --check
  • cargo test -p perry-codegen --no-fail-fast, diffed against the
    current origin/main baseline (see above)
  • scripts/gc_root_dominance_check.py, gated invocations
    (--unrooted-allocas --moving-only, --statepoints --moving-only),
    both lowerings, PERRY_RUNTIME_DIR pinned
  • New gap fixtures compile and match node --experimental-strip-types
    output

Summary by CodeRabbit

  • Bug Fixes

    • Improved reliability of namespace exports, computed-key method calls, array and typed-array assignments, and global property updates during garbage collection.
    • Prevented incorrect values and stale references when assignments or method calls trigger memory allocation.
    • Improved stability for class-field assignments, setters, and compound receivers under allocation pressure.
  • Tests

    • Added regression coverage for namespace exports, computed dispatch, indexed stores, typed arrays, global properties, and class fields.

proggeramlug pushed a commit that referenced this pull request Aug 9, 2026
@coderabbitai

coderabbitai Bot commented Aug 9, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The changes root and reread operands across namespace construction, index stores, and computed-key dispatch. They document class-field receiver limits, add moving-GC regression tests, and update release metadata.

Changes

GC rooting coverage

Layer / File(s) Summary
Namespace population rooting
crates/perry-codegen/src/codegen/helpers.rs, test-files/fixtures/gc_namespace_rooting_pkg/*, test-files/test_gap_gc_namespace_and_computed_dispatch_rooting.ts
Namespace values are rooted during materialization and reread before namespace creation. Fixtures and tests cover constants, functions, classes, re-exports, and computed-key dispatch.
Index and property store rooting
crates/perry-codegen/src/expr/index_set.rs, crates/perry-codegen/src/expr/property_set.rs, test-files/test_gap_gc_index_set_bounded_globalthis_ta_rooting.ts
Global-object, typed-array, and bounded-array stores root receivers, keys, and values across allocating evaluation. Comments document safe direct receivers and unresolved compound receivers.
Computed method dispatch rooting
crates/perry-codegen/src/lower_call/early_branches.rs
Computed dispatch rebuilds native argument buffers from reread rooted operands for static and dynamic keys.
Class-field receiver regression coverage
test-files/test_gap_gc_class_field_receiver_rooting.ts
Regression cases cover raw-f64 fields, boxed fields, setters, and compound this.target receivers with allocating right-hand sides.
Rooting scope and release metadata
changelog.d/7736-rooting-residue.md, CLAUDE.md, Cargo.toml
The changelog records fixed and unresolved rooting cases. Version markers advance from 0.5.1429 to 0.5.1430.

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

Possibly related issues

  • Issue 7210 — The changes address the namespace-population and computed-key dispatch hazards identified in the issue.
  • Issue 7640 — The changes address several rooting gaps in indexed stores and document the remaining class-field receiver hazard.

Possibly related PRs

Sequence Diagram(s)

sequenceDiagram
  participant GeneratedCode
  participant RootedGroup
  participant Runtime
  participant MovingGC
  GeneratedCode->>RootedGroup: lower receiver, key, value, or arguments
  GeneratedCode->>MovingGC: execute allocation-capable operation
  MovingGC->>RootedGroup: relocate managed values
  RootedGroup->>GeneratedCode: reread relocated values
  GeneratedCode->>Runtime: perform namespace creation, store, or dispatch
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 29.41% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the GC rooting fixes and the related issue findings addressed by the pull request.
Description check ✅ Passed The description provides detailed changes, related issues, test coverage, and pending verification status, despite omitting some template headings.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch gc/7210-7640-rooting-residue

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

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (2)
test-files/fixtures/gc_namespace_rooting_pkg/lib.ts (1)

20-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider adding a nested-namespace export to cover the fifth entry kind.

The fixture covers LocalVar, LocalFunction, LocalClass, ForeignVar, and ForeignFunction. NamespaceEntryKind::NestedNamespace loads @__perry_ns_<prefix> and is also rooted by this change, but no export here produces it. Add export * as other from "./other.ts"; to exercise that arm.

🤖 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 `@test-files/fixtures/gc_namespace_rooting_pkg/lib.ts` around lines 20 - 22,
Add a namespace alias export alongside the existing re-exports in the fixture
module, using the nested namespace form that exposes the other module as `other`
(for example, `export * as other from "./other.ts"`), so
`NamespaceEntryKind::NestedNamespace` is exercised while preserving the existing
`churnFromOther` and `CHURN_TAG` exports.
test-files/test_gap_gc_index_set_bounded_globalthis_ta_rooting.ts (1)

49-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a read-back assertion for the typed-array symbol-key store.

The other two cases compare the stored value against the expected value and increment bad. The typed-array symbol case only performs the store. A stale key after relocation stores the value under a different symbol, and this fixture still prints 0. Read the symbol property back and compare it.

🧪 Proposed assertion
-function widthTrackedTaSymbolStore(ta: Int32Array, n: number): void {
+function widthTrackedTaSymbolStore(ta: Int32Array, n: number): number {
   const sym = Symbol.for("gc7640a_" + n);
   (ta as unknown as Record<symbol, number>)[sym] = churn(n);
+  return (ta as unknown as Record<symbol, number>)[sym];
 }
   const ta = new Int32Array(8);
   for (let r = 0; r < 200; r++) {
-    widthTrackedTaSymbolStore(ta, r);
+    if (widthTrackedTaSymbolStore(ta, r) !== r) bad++;
   }

Also applies to: 72-75

🤖 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 `@test-files/test_gap_gc_index_set_bounded_globalthis_ta_rooting.ts` around
lines 49 - 52, Update widthTrackedTaSymbolStore and the corresponding second
typed-array symbol case to read back the value at the same symbol key after
storing it, compare it with churn(n), and increment bad when it differs,
matching the assertions used by the other cases.
🤖 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/property_set.rs`:
- Around line 196-202: Update the stale changelog reference in the comment near
the property-set emitted IR discussion to point from 7640-abc-rooting-residue.md
to 7736-rooting-residue.md, without changing the surrounding explanation.

In `@test-files/test_gap_gc_class_field_receiver_rooting.ts`:
- Around line 92-99: Update the gc-moving-witnesses corpus handling for
test_gap_gc_class_field_receiver_rooting.ts so the Runner.run case is treated as
unprotected. Restrict assertions to safe arms or encode the expected failure for
Runner.run, preventing its relocated-witness mismatch from blocking the
loop_polls suite.

---

Nitpick comments:
In `@test-files/fixtures/gc_namespace_rooting_pkg/lib.ts`:
- Around line 20-22: Add a namespace alias export alongside the existing
re-exports in the fixture module, using the nested namespace form that exposes
the other module as `other` (for example, `export * as other from
"./other.ts"`), so `NamespaceEntryKind::NestedNamespace` is exercised while
preserving the existing `churnFromOther` and `CHURN_TAG` exports.

In `@test-files/test_gap_gc_index_set_bounded_globalthis_ta_rooting.ts`:
- Around line 49-52: Update widthTrackedTaSymbolStore and the corresponding
second typed-array symbol case to read back the value at the same symbol key
after storing it, compare it with churn(n), and increment bad when it differs,
matching the assertions used by the other cases.
🪄 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: 50c57554-6978-4f6d-83e8-d2967d05b9ef

📥 Commits

Reviewing files that changed from the base of the PR and between f28657d and a0be89f.

📒 Files selected for processing (10)
  • changelog.d/7736-rooting-residue.md
  • crates/perry-codegen/src/codegen/helpers.rs
  • crates/perry-codegen/src/expr/index_set.rs
  • crates/perry-codegen/src/expr/property_set.rs
  • crates/perry-codegen/src/lower_call/early_branches.rs
  • test-files/fixtures/gc_namespace_rooting_pkg/lib.ts
  • test-files/fixtures/gc_namespace_rooting_pkg/other.ts
  • test-files/test_gap_gc_class_field_receiver_rooting.ts
  • test-files/test_gap_gc_index_set_bounded_globalthis_ta_rooting.ts
  • test-files/test_gap_gc_namespace_and_computed_dispatch_rooting.ts

Comment on lines +196 to +202
// emitted IR. Left unfixed here: rooting it unconditionally would add
// a real store+bind+reread to the common `LocalGet`/`This` case this
// comment just proved needs none, on what the issue that tracks this
// (#7640) calls "the hottest store path in the compiler" — a
// measured-cost change, not a rooting-API mechanical one, and
// deliberately left for a follow-up that can benchmark it. See
// changelog.d/7640-abc-rooting-residue.md.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: List changelog fragments and check the referenced file.
set -euo pipefail

fd . changelog.d -t f
rg -n '7640-abc-rooting-residue' crates changelog.d test-files || echo "no other references"

Repository: PerryTS/perry

Length of output: 25645


Update the stale changelog pathname.

changelog.d/7640-abc-rooting-residue.md is not present; the relevant changelog fragment is changelog.d/7736-rooting-residue.md. Update this comment so readers find the right note.

🤖 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/src/expr/property_set.rs` around lines 196 - 202, Update
the stale changelog reference in the comment near the property-set emitted IR
discussion to point from 7640-abc-rooting-residue.md to 7736-rooting-residue.md,
without changing the surrounding explanation.

Comment on lines +92 to +99
class Runner {
run(p: Point, n: number): void {
const h = new Holder();
h.target = p;
h.target.x = allocPoint(churn(n)).x;
h.target.next = allocPoint(churn(n + 1));
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Check how this fixture is executed and whether output is compared.
set -euo pipefail

rg -n 'test_gap_gc_class_field_receiver_rooting' -g '!**/target/**' .
rg -n 'PERRY_GC_MOVING_LOOP_POLLS' -g '!**/target/**' . | head -30

Repository: PerryTS/perry

Length of output: 9891


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "## candidate files"
git ls-files | rg '(^|/)test_gap_gc_class_field_receiver_rooting\.ts$|property_set\.rs$|gc_repsel_corpus\.txt$|memory-model\.md$|CLAUDE\.md$' || true

echo
echo "## fixture excerpts"
if [ -f test-files/test_gap_gc_class_field_receiver_rooting.ts ]; then
  wc -l test-files/test_gap_gc_class_field_receiver_rooting.ts
  sed -n '1,180p' test-files/test_gap_gc_class_field_receiver_rooting.ts | cat -n
fi

echo
echo "## property_set.rs relevant excerpt"
if [ -f crates/perry-codegen/src/expr/property_set.rs ]; then
  wc -l crates/perry-codegen/src/expr/property_set.rs
  sed -n '150,220p' crates/perry-codegen/src/expr/property_set.rs | cat -n
fi

Repository: PerryTS/perry

Length of output: 10639


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "## gc_repsel_corpus text"
if [ -f test-parity/gc_repsel_corpus.txt ]; then
  rg -n '7640|class_field|receiver|gap_gc|PERRY_GC_MOVING_LOOP_POLLS|test_gap_gc_class_field_receiver_rooting' test-parity/gc_repsel_corpus.txt -C 2 || true
fi

echo
echo "## root/repsel script excerpts"
for f in scripts.gc_repsel_matrix.sh scripts/gc_root_dominance_check.py docs/src/internals/memory-model.md changelog.d/7640*; do
  if [ -f "$f" ]; then
    echo "### $f"
    wc -l "$f"
    rg -n 'test_gap_gc_class_field_receiver_rooting|7640|receiver|PERRY_GC_MOVING_LOOP_POLLS|PERRY_GC_ZEAL|moving|assert|bad' "$f" -C 3 || true
  fi
done

Repository: PerryTS/perry

Length of output: 50369


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "## workflow files referencing gc-moving-witnesses or test_gap_gc_class_field_receiver_rooting"
git ls-files .github test-parity scripts | paste -sd'\n' | rg '(^\.github/workflows/|test-parity/gc_repsel_corpus\.txt|scripts/)' || true
for f in $(git ls-files .github/workflows | sort); do
  if rg -q 'gc-moving-witnesses|test-gap-gc|PERRY_GC_MOVING_LOOP_POLLS|PERRY_GC_ZEAL|PERRY_GC_PROTECT_FROMSPACE|PERRY_GC_HEAP_LIMIT|loop_polls|force_evac|evac_minor' "$f"; then
    echo "### $f"
    rg -n 'gc-moving-witnesses|test-gap-gc|PERRY_GC_MOVING_LOOP_POLLS|PERRY_GC_ZEAL|PERRY_GC_PROTECT_FROMSPACE|PERRY_GC_HEAP_LIMIT|loop_polls|force_evac|evac_minor|test_gap_gc_class_field_receiver_rooting|PERRY_GC_DIAG|PERRY_GC_MOVING' "$f" -C 2
  fi
done

echo
echo "## corpus entry context exactly"
perl -ne 'print "$. $_" if $. >= 723 && $. <= 732' test-parity/gc_repsel_corpus.txt | cat -n

echo
echo "## targeted policy/collector references only"
rg -n 'evacuation_policy_allowed|move|copying|PERRY_GC_ZEAL|PERRY_GC_PROTECT_FROMSPACE|PERRY_GC_HEAP_LIMIT|PERRY_GC_MOVING_LOOP_POLLS|gc_check_trigger|js_gc_loop_safepoint' crates -g '*.rs' -C 1 | head -240

Repository: PerryTS/perry

Length of output: 31188


Mark the Runner.run branch as unprotected in the corpus flow.

test_gap_gc_class_field_receiver_rooting.ts documents h.target.x = … and h.target.next = … as genuinely unprotected compound receivers, and crates/perry-codegen/src/expr/property_set.rs states the same. The gc-moving-witnesses circuit runs this file on --arms loop_polls and gates on relocated witnesses matching oracle output; assert only the safe arms, or carry the failure expectation for Runner.run so this open case does not block the moving-witness suite.

🤖 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 `@test-files/test_gap_gc_class_field_receiver_rooting.ts` around lines 92 - 99,
Update the gc-moving-witnesses corpus handling for
test_gap_gc_class_field_receiver_rooting.ts so the Runner.run case is treated as
unprotected. Restrict assertions to safe arms or encode the expected failure for
Runner.run, preventing its relocated-witness mismatch from blocking the
loop_polls suite.

Ralph Küpper added 3 commits August 9, 2026 23:53
…C findings

#7210: root the two flagship interleaved-staging sites the earlier alloca
enumeration left open. `codegen/helpers.rs`'s `emit_namespace_populator`
staged every re-exported binding's value into a plain stack alloca while the
per-entry loop called allocating helpers (closure singleton alloc, arbitrary
cross-module getters); an already-staged entry had no root and could go
stale before `js_create_namespace` read the whole buffer. Fixed by rooting
each value in a RootedGroup as it's produced and deferring every store to a
second, call-free pass that runs immediately before the consuming call.
`lower_call/early_branches.rs`'s `obj[strKey](args)` computed-key dispatch
lowered receiver/key/args into bare registers in sequence, and
`unbox_str_handle` (an allocating SSO materialisation) ran between the args
buffer's stores and the call in the static-key arm. Fixed the same way:
root `[object, index, ...args]` in one RootedGroup, build the args buffer
last in each branch.

#7640 section A: three more index_set.rs arms with no rooting decision at
all, now fixed — the bounded-index-pair array store (the issue's sharpest
repro), `globalThis[k] = v`, and the width-tracked typed-array
non-numeric-index store. Not reached: the #5525 recv_unknown inline dyn-TA
store, and the TA runtime-key / TA final-fallback / Uint8Array runtime-key
arms.

#7640 section C: resolved, not mechanically fixed. Two property_set.rs
comments claimed a class-field store's receiver survives an allocating RHS
via "the same statepoint re-read" a sibling arm relies on. That mechanism
doesn't exist. Traced to ground: for a bare LocalGet/This receiver the claim
is true, via root_reload.rs (#7280) — a front-end pass independent of RS4GC
that re-materialises a value derived from a shadow-slot or handle-global
load below any collection point it doesn't dominate; verified with the
checker on both lowerings against a new fixture. For a compound receiver
(`this.target.x = allocPoint(n).x`, the receiver itself a class-field READ)
the claim is false, and the gap is invisible to both root_reload (no shadow
slot to re-derive from — the value is a phi over two field-get paths) and
the stale-register checker (its pattern match only anchors on a direct
`load double, ptr <root>` source). Left unfixed deliberately: rooting the
receiver unconditionally would tax the dominant plain-local case this
investigation just proved needs nothing, on what the issue calls the
hottest store path in the compiler — a measured-cost tradeoff for a
follow-up, not a mechanical gap. All four sites that made or relied on the
original claim now say this precisely.

#7640 section E: triaged, not fixed. All seven named callees were read, not
skipped — each looks like a hazard that is not one (single already-rooted
caller, address derived after the allocating step rather than before, or
typed-array immovability per CLAUDE.md's own GC section). Static triage
only, not checker-verified per-callee; a lead for a follow-up.

New corpus fixtures: test_gap_gc_namespace_and_computed_dispatch_rooting.ts
(+ fixtures/gc_namespace_rooting_pkg/), test_gap_gc_index_set_bounded_globalthis_ta_rooting.ts,
and an expanded test_gap_gc_class_field_receiver_rooting.ts covering both
halves of the section C finding.

Verified: scripts/gc_root_dominance_check.py's gated invocations
(--unrooted-allocas --moving-only; --statepoints --moving-only) read clean
on the full corpus, both lowerings, with an empty allowlist. cargo test
-p perry-codegen has pre-existing failures unrelated to this change (see PR
description for the exact set against the current main baseline).
@proggeramlug
proggeramlug force-pushed the gc/7210-7640-rooting-residue branch from a0be89f to 2e868d7 Compare August 9, 2026 21:55

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
CLAUDE.md (1)

143-145: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Move release-history detail out of CLAUDE.md.

Lines 143-145 and 147 add issue history, former behavior, and performance history to CLAUDE.md. Keep only current GC behavior and operator guidance here. Move the historical detail to changelog.d/7736-rooting-residue.md.

As per coding guidelines, CLAUDE.md must stay concise and detailed change history belongs in changelog.d/. The PR objectives identify changelog.d/7736-rooting-residue.md as this change’s fragment.

Also applies to: 147-147

🤖 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 `@CLAUDE.md` around lines 143 - 145, Trim the GC guidance rows for
PERRY_GC_ZEAL, PERRY_GC_ZEAL_ALLOC_KB, and PERRY_GC_FROMSPACE_SCAN_ABORT in
CLAUDE.md to retain only current behavior and operator usage. Move issue
references, former behavior, performance measurements, and release-history
details into changelog.d/7736-rooting-residue.md, preserving the current
operational instructions in CLAUDE.md.

Source: Coding guidelines

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

Outside diff comments:
In `@CLAUDE.md`:
- Around line 143-145: Trim the GC guidance rows for PERRY_GC_ZEAL,
PERRY_GC_ZEAL_ALLOC_KB, and PERRY_GC_FROMSPACE_SCAN_ABORT in CLAUDE.md to retain
only current behavior and operator usage. Move issue references, former
behavior, performance measurements, and release-history details into
changelog.d/7736-rooting-residue.md, preserving the current operational
instructions in CLAUDE.md.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 5748e015-dee4-44c0-b3a3-3e4ea1de75c0

📥 Commits

Reviewing files that changed from the base of the PR and between a0be89f and 2e868d7.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (2)
  • CLAUDE.md
  • Cargo.toml

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant