fix(gc): root the interned typeof strings, the rawJSON key, and every saved implicit this - #7226
Merged
proggeramlug merged 3 commits intoAug 2, 2026
Conversation
… saved implicit `this` Three unrooted-value bugs behind PerryTS#7154's registry crash, found by pointing PerryTS#7196's from-space reporter at `sfw-registry --help` rather than by grinding the static checker's tail. 1. RUNTIME CACHES (the one that mattered). `js_value_typeof` interned its eight result strings in thread-local `Cell<*mut StringHeader>`s that nothing registered as GC roots, so the FIRST minor collection swept or evacuated them and every later `typeof x === "..."` compared against from-space. `json/raw_json.rs`'s cached `"rawJSON"` key had the identical defect. Both now go through `gc_register_mutable_root_scanner`. This is why the registry failed 10/10 rather than intermittently: an unrooted register goes bad only when a collection lands in its window, an unrooted cache goes bad at collection #0 and stays bad. It is also structurally invisible to `scripts/gc_root_dominance_check.py`, which reads emitted LLVM IR and cannot see a runtime table. 2. CODEGEN, implicit `this`. `js_implicit_this_set` returns the value it displaced from the `IMPLICIT_THIS` cell -- a scanned MUTABLE root -- and that value was then held in a bare SSA register across the whole call the bind exists to scope. The restore published a pre-move address back INTO a root. PerryTS#7214 found this in `js_closure_callN` and left it; it was in fact seven lowerings with seven copies of the same three lines, now one shared `temp_root::implicit_this_save` / `implicit_this_restore` pair. 3. CODEGEN, ClassExprFresh (PerryTS#7211). Its `protect_handle` predicate asked only whether the AUTHOR's static initializers could collect, never whether the `js_object_set_field_by_name` the lowering itself emits per static could -- and that allocates. The four allowlist entries it covered are deleted, which is the ratchet working: the fix made them match nothing. Checker: `js_implicit_this_set` is now a root READ (being non-collecting is what makes a call one), which takes its sink from 214 hits to 0 and keeps the class gated. `--stale-registers` now honours `--min-binds`, and `--any-def` with it is a usage error -- both the "a silently ignored knob is a disarmed knob" rule the mode already applied to `--max-stale` and `--fatal-sinks`. Refs PerryTS#7154, PerryTS#7211, PerryTS#7213, PerryTS#7196, PerryTS#7206, PerryTS#7214, PerryTS#7161
📝 WalkthroughWalkthroughThe PR fixes GC rooting for fresh class expressions, implicit ChangesGC rooting fixes
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant ClosureDispatch
participant TempRoot
participant GC
participant Callee
ClosureDispatch->>TempRoot: Save displaced implicit this
TempRoot->>GC: Register mutable temp root
ClosureDispatch->>Callee: Execute collecting call
GC-->>TempRoot: Rewrite relocated value
ClosureDispatch->>TempRoot: Restore implicit this
sequenceDiagram
participant gc_init
participant GC
participant TypeofCache
participant RawJSONCache
gc_init->>GC: Register typeof root scanner
gc_init->>GC: Register rawJSON root scanner
GC->>TypeofCache: Visit and rewrite cached strings
GC->>RawJSONCache: Visit and rewrite rawJSON key
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/perry-runtime/src/json/raw_json.rs (1)
88-117: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReset
RAW_JSON_KEYwhen test arenas reset.
RAW_JSON_KEYpersists in TLS after the test harness resets arenas. A later collection invokesscan_raw_json_key_root_muton the stale pointer. Add a#[cfg(test)]reset helper and call it from the same reset sequence asreset_typeof_string_cache_for_test.Proposed local reset helper
+#[cfg(test)] +pub(crate) fn reset_raw_json_key_cache_for_test() { + RAW_JSON_KEY.with(|cell| cell.set(std::ptr::null_mut())); +}🤖 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/json/raw_json.rs` around lines 88 - 117, Reset the thread-local RAW_JSON_KEY cache during test arena resets to avoid scanning a stale pointer. Add a #[cfg(test)] helper near scan_raw_json_key_root_mut or raw_json_key that clears the cell, then invoke it from the existing reset sequence alongside reset_typeof_string_cache_for_test.
🤖 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 `@CLAUDE.md`:
- Around line 253-254: Remove the detailed incident history and issue-specific
changelog narrative from CLAUDE.md, retaining only concise, durable operational
guidance relevant to contributors. Preserve the full rooting and runtime-cache
details in changelog.d/7219-registry-gc-unrooted-caches.md, and do not alter
release metadata or external release documentation.
In `@scripts/gc_root_dominance_check.py`:
- Around line 2087-2105: Enforce the function-count threshold before stale-mode
dispatch: calculate n_funcs before the ns.stale_registers branch, reject when
n_funcs < ns.min_funcs, and only then call run_stale. Extend self_test() with
stale-register cases covering both below-threshold failure and meeting-threshold
success, while preserving the existing min-binds validation.
In `@test-files/test_gap_gc_typeof_string_cache_rooting.ts`:
- Around line 46-65: Extend the typeof cache test around the vals and want
arrays to include BigInt(1) with "bigint" and Symbol() with "symbol", then
update the inner loop to iterate across all eight entries instead of six. Run
the parity/gap tests using the Node version specified by .node-version.
---
Outside diff comments:
In `@crates/perry-runtime/src/json/raw_json.rs`:
- Around line 88-117: Reset the thread-local RAW_JSON_KEY cache during test
arena resets to avoid scanning a stale pointer. Add a #[cfg(test)] helper near
scan_raw_json_key_root_mut or raw_json_key that clears the cell, then invoke it
from the existing reset sequence alongside reset_typeof_string_cache_for_test.
🪄 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: 28622230-4a0b-4253-9c92-871f8e6bac55
📒 Files selected for processing (20)
CLAUDE.mdchangelog.d/7219-registry-gc-unrooted-caches.mdcrates/perry-codegen/src/expr/static_field_meta.rscrates/perry-codegen/src/expr/temp_root.rscrates/perry-codegen/src/lower_call/console_promise.rscrates/perry-codegen/src/lower_call/early_branches.rscrates/perry-codegen/src/lower_call/func_ref.rscrates/perry-codegen/src/lower_call/method_override.rscrates/perry-codegen/src/lower_call/property_get/dynamic_dispatch.rscrates/perry-codegen/src/lower_call/property_get/static_dispatch.rscrates/perry-runtime/src/builtins/arithmetic.rscrates/perry-runtime/src/builtins/mod.rscrates/perry-runtime/src/gc/mod.rscrates/perry-runtime/src/json/mod.rscrates/perry-runtime/src/json/raw_json.rscrates/perry-runtime/src/string/alloc.rsscripts/gc_root_dominance_allowlist.jsonscripts/gc_root_dominance_check.pytest-files/test_gap_gc_closure_call_prev_this_rooting.tstest-files/test_gap_gc_typeof_string_cache_rooting.ts
Comment on lines
+253
to
+254
| - **Root-store dominance in codegen.** *A GC-managed value's root store must **dominate** every subsequent site that can collect.* Three ways it has broken, all shipped: the store's slot index fell outside the pushed shadow frame so `js_shadow_slot_bind` bounds-checked it into a silent no-op (#7184); the store was emitted in-frame but **after** a call that allocates (#7192); and the value lives in a plain `alloca_entry` that is neither a shadow slot nor a temp root, so the collector never rewrites it (`lower_call/new.rs`'s inline-ctor `this_slot`, closed by #7207; `--unrooted-allocas` is the detector for that shape, and its remaining hits are #7210's). All three present identically — a *rooted* slot holding a dangling pointer, surfacing cycles later as `TypeError: value is not a function` — and **none is visible to any runtime GC probe**, because at the moment of the collection there is nothing for the collector to find. That is why #7154's from-space scan only ever saw offenders whose targets had already died. The instrument is static: `scripts/gc_root_dominance_check.py` over `--trace llvm` output (`--self-test` proves it can still fail). Only bites under `PERRY_GC_MOVING_LOOP_POLLS=1`, off by default since #7161 — so a green default run says nothing about this class. **Full writeup, every known shape and how to check your work: `docs/src/internals/gc-rooting-invariant.md`.** The CI gate is `gc-root-dominance.yml` over `scripts/gc_root_dominance_corpus.sh`; known-remaining hits are named one-per-entry in `scripts/gc_root_dominance_allowlist.json` (an entry that matches nothing FAILS, so a fix must delete its entry) — **that list is now empty**, which #7211 emptied by fixing the fifth shape: `ClassExprFresh` rooted only when it thought the static *initializers* could collect and never asked whether its own emitted `js_object_set_field_by_name` could. The sophisticated version of the mistake — the author wrote a rooting predicate and it asked the wrong question. | ||
| - **A runtime-side cache of a raw heap pointer is a GC root, and the static checker cannot see it.** This is the sibling class, and it is the one that actually kept `sfw-registry --help` red after every codegen register in #7192/#7206/#7214 was closed. `js_value_typeof` interned its eight result strings in thread-local `Cell<*mut StringHeader>`s that nothing registered, so the FIRST minor collection invalidated them and every later `typeof x === "…"` compared against from-space (#7211). Two things to carry forward. **The failure signature is different**: an unrooted register goes bad only when a collection lands in its window, so it is intermittent; an unrooted cache goes bad at collection #0 and stays bad, so it fails 10/10 — if a GC bug is perfectly reproducible, suspect a table, not a register. And **`scripts/gc_root_dominance_check.py` reads emitted LLVM IR, so it is structurally blind to this**; the detector is `PERRY_GC_ZEAL=1 PERRY_GC_PROTECT_FROMSPACE=1 PERRY_GC_PROTECT_FROMSPACE_DEPTH=800` on a real workload, whose reporter names the address, `obj_type`, size and retiring cycle. Point the runtime instruments at the workload *before* grinding the static checker's tail. The root registry is `gc_register_mutable_root_scanner` in `gc/mod.rs` (~55 entries); when you add a cache of a heap pointer, add it there in the same commit. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the detailed changelog material from CLAUDE.md.
These additions make CLAUDE.md less concise. Keep only durable operational guidance here. Keep the incident details in changelog.d/7219-registry-gc-unrooted-caches.md.
As per coding guidelines, “Keep CLAUDE.md concise; put detailed changelog entries in changelog.d/.” Based on learnings, contributors should not modify CLAUDE.md release metadata or release documentation in external PRs.
🤖 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 253 - 254, Remove the detailed incident history and
issue-specific changelog narrative from CLAUDE.md, retaining only concise,
durable operational guidance relevant to contributors. Preserve the full rooting
and runtime-cache details in changelog.d/7219-registry-gc-unrooted-caches.md,
and do not alter release metadata or external release documentation.
Sources: Coding guidelines, Learnings
Comment on lines
+46
to
+65
| const vals: any[] = ["a", 1, true, {}, undefined, run]; | ||
| const want: string[] = [ | ||
| "string", | ||
| "number", | ||
| "boolean", | ||
| "object", | ||
| "undefined", | ||
| "function", | ||
| ]; | ||
| for (let r = 0; r < 500; r++) { | ||
| churn(r); | ||
| for (let k = 0; k < 6; k++) { | ||
| // Reads the cached string and compares it against a literal. A cache | ||
| // entry the collector moved but never rewrote makes this compare read | ||
| // from-space. | ||
| if (typeof vals[k] !== want[k]) { | ||
| bad++; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Node version file =="
cat -n .node-version || true
echo "== Locate target file =="
fd -a 'test-gap-gc-typeof-string-cache-rooting\.ts$|test_gap_gc_typeof_string_cache_rooting\.ts$' . || true
echo "== Target file with line numbers =="
if [ -f test-files/test_gap_gc_typeof_string_cache_rooting.ts ]; then
cat -n test-files/test_gap_gc_typeof_string_cache_rooting.ts
fi
echo "== Search for TYPEOF cache cells =="
rg -n "TYPEOF_|typeof|typeof_gc|gap|parity" . --glob '*.ts' --glob '!**/node_modules/**' | head -200Repository: PerryTS/perry
Length of output: 24466
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== JavaScript typeof probe for untested types under available Node =="
node --version
node - <<'JS'
const vals = [BigInt(1), Symbol()];
const expected = ["bigint", "symbol"];
for (let k = 0; k < vals.length; k++) {
console.log(k, typeof vals[k], expected[k], typeof vals[k] === expected[k]);
}
JS
echo "== Find js_value_typeof and typeof cache definitions =="
rg -n "js_value_typeof|TYPEOF_|typeof.*cache|Cell<\\*mut StringHeader>|STRING_BIGINT|STRING_SYMBOL" . --glob '!**/node_modules/**' --glob '!test-files/*' | head -200
echo "== Inspect likely arithmetic source containing js_value_typeof =="
for f in $(rg -l "js_value_typeof|TYPEOF_" . --glob '!**/node_modules/**' --glob '!test-files/*' | head -20); do
echo "--- $f"
wc -l "$f"
rg -n "js_value_typeof|TYPEOF_|Cell|cache|typeof" "$f" -C 4
doneRepository: PerryTS/perry
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Node parity probe for untested typeof cache candidates =="
node - <<'JS'
const untested = [BigInt(1), Symbol()];
const wants = ["bigint", "symbol"];
for (let k = 0; k < untested.length; k++) {
const v = untested[k];
const t = typeof v;
console.log(`${wants[k]}=${t === wants[k]} actual=${t}`);
}
JS
echo "== focused type-of related source =="
rg -n "js_value_typeof|TYPEOF_BIGINT|TYPEOF_SYMBOL|TYPEOF_STRING|TYPEOF_undefined|Cell<\\*mut StringHeader>|stringHeaderCache|typeof.*cache|cache.*typeof" \
crates packages types test-files docs --glob '!**/node_modules/**' --glob '!**/.git/**' --max-count 200
echo "== candidate files around likely js_value_typeof implementations =="
rg -l "js_value_typeof|TYPEOF_BIGINT|TYPEOF_SYMBOL" . --glob '!**/node_modules/**' --glob '!test-files/*' --max-count 20 | while IFS= read -r f; do
echo "--- $f"
wc -l "$f"
sed -n '1,240p' "$f"
doneRepository: PerryTS/perry
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate JavaScript typeof cache candidates with file list =="
rg -l "js_value_typeof|CACHE|Cell<\\*mut" crates/perry-runtime/src crates/perry-codegen/src test-files test-parity --glob '!**/node_modules/**' | head -100
echo "== arithmetic.rs JSValue typeof section =="
sed -n '680,820p' crates/perry-runtime/src/builtins/arithmetic.rs
echo "== StringHeader cache references =="
rg -n "StringHeader|js_string_cache|cache|CACHE|Cell<" crates/perry-runtime/src/builtins/arithmetic.rs crates/perry-runtime/src/string.rs crates/perry-runtime/src/value.rs crates/perry-codegen/src/runtime_decls/strings.rs --max-count 200
echo "== focused typeof cache enum/list candidates =="
rg -n "bigint|symbol|TYPEOF_BIGINT|TYPEOF_SYMBOL|TYPEOF_|js_value_typeof|TAG_STRING|STRING_SHORT|SHORT_STRING" crates/perry-runtime/src/builtins/arithmetic.rs crates/perry-runtime/src/value.rs crates/perry-runtime/src/string.rs crates/perry-codegen/src | head -240Repository: PerryTS/perry
Length of output: 16562
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Run gap suite with pinned Node, and check the exact test =="
if command -v node >/dev/null 2>&1; then
node --version
fi
node --version | sed -n '1p' > /tmp/node-ver 2>/dev/null || true
node test-files/test_gap_gc_typeof_string_cache_rooting.ts 2>&1 || true
echo "== Determine whether test exercises all 8 typeof cache cells =="
python3 - <<'PY'
from pathlib import Path
p = Path("test-files/test_gap_gc_typeof_string_cache_rooting.ts")
text = p.read_text()
line6 = text.splitlines()[5]
line9 = text.splitlines()[8]
print(f"line6={line6.strip()}")
print(f"line9={line9.strip()}")
print(f"loop_bounded_by_6={line9.strip()}.startswith('for (let k = 0; k < 6; k++)') or '; k < 6' in line9 or 'k < 6' in line9 or 'k<6' in line9 or '"k < 6"' in line9 or '"k<6"' in line9")
print(f"vals_length_guess='vals: any[] = [\"a\", 1, true, {{}}, undefined, run]' in text")
print(f"six_literal_string_comparisons=text.count('typeof vals[k]')>=1 and text.count('\"string\"' if False else '')")
PY
echo "== Exact typeof cache constants and runtime symbols =="
sed -n '500,690p' crates/perry-runtime/src/builtins/arithmetic.rs
echo "== Node probe for untested cache targets =="
node - <<'JS'
const vals = [BigInt(1), Symbol()];
const expected = ["bigint", "symbol"];
for (let k = 0; k < vals.length; k++) {
console.log(expected[k], typeof vals[k], typeof vals[k] === expected[k]);
}
JSRepository: PerryTS/perry
Length of output: 693
Cover the missing typeof cache cells.
js_value_typeof still has TYPEOF_BIGINT and TYPEOF_SYMBOL cache cells. Add BigInt(1) and Symbol() to the input, add "bigint" and "symbol" to the expected values, and loop over all eight entries. Use the Node version from .node-version for parity/gap runs.
🤖 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_typeof_string_cache_rooting.ts` around lines 46 - 65,
Extend the typeof cache test around the vals and want arrays to include
BigInt(1) with "bigint" and Symbol() with "symbol", then update the inner loop
to iterate across all eight entries instead of six. Run the parity/gap tests
using the Node version specified by .node-version.
Source: Coding guidelines
jdalton
added a commit
to jdalton/perry
that referenced
this pull request
Aug 2, 2026
`Expr::RegExpTest` / `Expr::RegExpExec` unboxed the receiver to a raw `RegExpHeader*` before emitting `js_jsvalue_to_string_coerce`, which allocates and dispatches a user `toString`. Under a moving minor the register named from-space by the time `js_regexp_test` dereferenced it — PerryTS#7154's residual, at `defineApiCall + 404` in the registry. Both take the established `guard_store_operand_across` / `reread_store_operand` pair and the unbox moves below the coerce. The checker missed it because `ALLOC_RE` spelled the allocator `regexp_alloc\w*` and the call is `js_regexp_new`. Reconciling every alternative against the runtime's real symbol table found four that match nothing at all (`regexp_alloc`, `promise_alloc`, `bigint_alloc`, `typed_array_alloc`): the runtime materializes fresh GC objects under three naming conventions (`_alloc*`, `_new*`, `_create*`) and the pattern modelled one. All three are now matched as conventions, with the non-conforming constructors enumerated explicitly. Second blind spot, and the one that matters for CI: the ToPrimitive family was not `POLL_CAPABLE_RUNTIME`, so even with `ALLOC_RE` widened the site was invisible to `--moving-only`, which is the mode the gate runs. Refs PerryTS#7154, PerryTS#7226, PerryTS#7211, PerryTS#7161
jdalton
added a commit
to jdalton/perry
that referenced
this pull request
Aug 2, 2026
`Expr::RegExpTest` / `Expr::RegExpExec` unboxed the receiver to a raw `RegExpHeader*` before emitting `js_jsvalue_to_string_coerce`, which allocates and dispatches a user `toString`. Under a moving minor the register named from-space by the time `js_regexp_test` dereferenced it — PerryTS#7154's residual, at `defineApiCall + 404` in the registry. Both take the established `guard_store_operand_across` / `reread_store_operand` pair and the unbox moves below the coerce. The checker missed it because `ALLOC_RE` spelled the allocator `regexp_alloc\w*` and the call is `js_regexp_new`. Reconciling every alternative against the runtime's real symbol table found four that match nothing at all (`regexp_alloc`, `promise_alloc`, `bigint_alloc`, `typed_array_alloc`): the runtime materializes fresh GC objects under three naming conventions (`_alloc*`, `_new*`, `_create*`) and the pattern modelled one. All three are now matched as conventions, with the non-conforming constructors enumerated explicitly. Second blind spot, and the one that matters for CI: the ToPrimitive family was not `POLL_CAPABLE_RUNTIME`, so even with `ALLOC_RE` widened the site was invisible to `--moving-only`, which is the mode the gate runs. Refs PerryTS#7154, PerryTS#7226, PerryTS#7211, PerryTS#7161
added 2 commits
August 2, 2026 05:49
# Conflicts: # scripts/gc_root_dominance_allowlist.json # scripts/gc_root_dominance_check.py
…d unrooted-alloca modes Both modes return before the floor was evaluated, so `--stale-registers --min-funcs 1200` and `--unrooted-allocas --min-funcs 1200` ran to a verdict over a corpus too thin to have exercised anything and exited 0. A documented liveness control silently disabled by a mode flag, in the script whose job is to catch gates that cannot fail. Reproduced on the self-test fixture (2 functions): stale and alloca modes both exited 0 at --min-funcs 50 while the bind-anchored default correctly exited 2. After the fix all three exit 2. n_funcs is computed above the mode branches and the message lives in one helper. self_test() gains a failing and a passing arm for each of the three modes, so neither a skipped floor nor an always-on one can regress silently.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/gc_root_dominance_check.py (1)
2054-2062: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject
--any-defwith--unrooted-allocas.The current guard rejects
--any-defonly with--stale-registers. The unrooted branch callscheck_func_unrooted_allocasat Line 2181 withoutanchor, so it ignores--any-def. A caller can believe the requested option was applied while running a different scan.Reject this combination and add a self-test expecting status
2.Proposed validation and self-test
if ns.any_def and ns.stale_registers: ap.error("--any-def has no effect with --stale-registers " "(the stale scan already anchors on every heap-value source)") + if ns.any_def and ns.unrooted_allocas: + ap.error("--any-def has no effect with --unrooted-allocas " + "(the unrooted scan does not use an anchor)")+ if _main_probe(["--unrooted-allocas", "--any-def", planted]) != 2: + print("self-test FAIL: --any-def with --unrooted-allocas " + "must be a usage error", file=sys.stderr) + ok = False🤖 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` around lines 2054 - 2062, Extend the argument validation near the existing ns.any_def and ns.stale_registers guard to also reject ns.any_def combined with ns.unrooted_allocas, using an error that states --any-def has no effect for the unrooted-allocas scan. Add a self-test covering this combination and assert it exits with status 2.
🧹 Nitpick comments (1)
scripts/gc_root_dominance_check.py (1)
1791-1792: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse iterable unpacking in both mode probes.
Ruff reports RUF005 for
mode + [...]. Replace both expressions with[*mode, ...]. This keeps the arguments unchanged and removes the warning.Proposed refactor
- if _main_probe(mode + ["--min-funcs", "50", "--min-binds", "1", + if _main_probe([*mode, "--min-funcs", "50", "--min-binds", "1", planted]) != 2: - if _main_probe(mode + ["--min-funcs", "2", "--min-binds", "1", + if _main_probe([*mode, "--min-funcs", "2", "--min-binds", "1", planted]) != want:Also applies to: 1801-1802
🤖 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` around lines 1791 - 1792, Update both _main_probe calls in the mode probe checks to construct their argument lists with iterable unpacking ([*mode, ...]) instead of mode + [...], preserving the existing arguments and return-code checks.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.
Outside diff comments:
In `@scripts/gc_root_dominance_check.py`:
- Around line 2054-2062: Extend the argument validation near the existing
ns.any_def and ns.stale_registers guard to also reject ns.any_def combined with
ns.unrooted_allocas, using an error that states --any-def has no effect for the
unrooted-allocas scan. Add a self-test covering this combination and assert it
exits with status 2.
---
Nitpick comments:
In `@scripts/gc_root_dominance_check.py`:
- Around line 1791-1792: Update both _main_probe calls in the mode probe checks
to construct their argument lists with iterable unpacking ([*mode, ...]) instead
of mode + [...], preserving the existing arguments and return-code checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 356b4e9e-4b38-4442-9ea6-665e7c9120bb
📒 Files selected for processing (3)
CLAUDE.mdscripts/gc_root_dominance_allowlist.jsonscripts/gc_root_dominance_check.py
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/gc_root_dominance_allowlist.json
- CLAUDE.md
proggeramlug
pushed a commit
to proggeramlug/perry
that referenced
this pull request
Aug 2, 2026
…tered PerryTS#7226 added test_gap_gc_typeof_string_cache_rooting.ts and test_gap_gc_closure_call_prev_this_rooting.ts without corpus entries, so they ran nowhere -- the same omission this PR's registration check exists to catch, landing while the PR was open. With these, all 17 test_gap_gc_*.ts on main are registered and the check is satisfiable. Both PASS on --arms loop_polls with the copying minor live. They are also the class that most needs this arm: an unrooted CACHE goes bad at collection #0 and is invisible to the static IR pass, so a workload run under PERRY_GC_MOVING_LOOP_POLLS=1 is the only instrument that sees it.
proggeramlug
added a commit
that referenced
this pull request
Aug 2, 2026
#7228) * ci(gc): make the moving-GC stale-root witnesses able to fail `test-files/test_gap_gc_*.ts` are reproducers, not parity tests. Each was written for a specific stale-root defect -- #6981, #7114, #7154, #7192, #7200/#7201/#7202, #7206, #7208/#7209, #7214, #7216 -- and every one of their headers says the same thing: the bug is only expressible under a moving collector, which since #7161 means `PERRY_GC_MOVING_LOOP_POLLS=1` at COMPILE and RUN time. No CI job ran them that way. `gc-root-dominance` compiles with the flag but is a static pass and never runs a program; `gc_instrument_smoke.sh` runs with it but against its own synthetic fixture. The witnesses reach CI only through `gc-stress`, whose PR arm subset contains no arm that compiles with the flag -- so on every pull request they were compiled into IR in which their bug is not expressible, then run to completion, green, proving nothing. CLAUDE.md's hazard 4: the job runs, its subject does not. The arm is one line of existing machinery: scripts/gc_repsel_matrix.sh --arms loop_polls --filter test_gap_gc_ plus a checker that rejects UNVER as hard as FAIL. That second half is the point: the matrix reports a cell that matched the oracle under an inert arm as UNVER, but its own exit status counts only FAIL, so an all-UNVER table exits 0 -- a green gate over a subject that never ran. On a requires=move arm UNVER means nothing relocated, and a stale-root witness that never sees a relocation cannot fail however broken its rooting is. RED-THEN-GREEN, on this branch, release build, idle host, node 26.5.1: main 15/15 PASS, copy-minor 15/15, exit 0 #7214's two production codegen files reverted (tests untouched) 12 PASS / 3 FAIL, checker exit 1 restored 15/15 PASS, copy-minor 15/15, exit 0 The three reds are exactly #7214's three witnesses -- `TypeError: value is not a function`, exit 1, with the copying minor demonstrably live (scavenged=408, 3931, 3934). Nothing else moved. SCOPE. `--filter test_gap_gc_` excludes the representation corpus and with it #7194. `--arms loop_polls` is the safepoint route only, so #7217's allocation-point defect is out of scope rather than papered over. Both exclusions fall out of the arm and the filter; there is no allowlist to rot. TWO DARK WITNESSES, FOUND BY THE CHECKER'S REGISTRATION RULE. The matrix auto-detects unregistered `test_gap_repsel_*`/`test_gap_specabi_*` files but not this prefix, and `test_gap_gc_new_instance_rooting` (#7192) and `test_gap_gc_assign_string_source_rooting` (#7216) were never registered -- they ran nowhere at all. Both are registered here and both pass on `loop_polls`. Registering the second surfaced a pre-existing red on every allocation-point arm, which is #7217's mechanism on a second file; triaged with measurements, ten entries that die together when #7217 is fixed. NOT REQUIRED YET, deliberately: a new gate has never been green, so promoting it immediately blocks every open PR. Promotion after its first green run on `main` is the follow-up -- and per CLAUDE.md's corollary, leaving that step undone is itself hazard 2. Also fixes #7205 in gc-ratchet.yml and gc-root-dominance.yml: keying push runs on the commit. `cancel-in-progress: false` does not protect a queued `main` run, because GitHub allows at most one PENDING run per group and cancels the previous one when a new run enters. Three consecutive gc-ratchet main runs ended `cancelled` with `jobs: []`. * docs(changelog): fragment for #7228 * test(gc): register #7226's two witnesses, which landed unregistered #7226 added test_gap_gc_typeof_string_cache_rooting.ts and test_gap_gc_closure_call_prev_this_rooting.ts without corpus entries, so they ran nowhere -- the same omission this PR's registration check exists to catch, landing while the PR was open. With these, all 17 test_gap_gc_*.ts on main are registered and the check is satisfiable. Both PASS on --arms loop_polls with the copying minor live. They are also the class that most needs this arm: an unrooted CACHE goes bad at collection #0 and is invisible to the static IR pass, so a workload run under PERRY_GC_MOVING_LOOP_POLLS=1 is the only instrument that sees it. --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
proggeramlug
pushed a commit
that referenced
this pull request
Aug 2, 2026
… against the runtime's real symbol table (#7227) * fix(gc): root the interned typeof strings, the rawJSON key, and every saved implicit `this` Three unrooted-value bugs behind #7154's registry crash, found by pointing #7196's from-space reporter at `sfw-registry --help` rather than by grinding the static checker's tail. 1. RUNTIME CACHES (the one that mattered). `js_value_typeof` interned its eight result strings in thread-local `Cell<*mut StringHeader>`s that nothing registered as GC roots, so the FIRST minor collection swept or evacuated them and every later `typeof x === "..."` compared against from-space. `json/raw_json.rs`'s cached `"rawJSON"` key had the identical defect. Both now go through `gc_register_mutable_root_scanner`. This is why the registry failed 10/10 rather than intermittently: an unrooted register goes bad only when a collection lands in its window, an unrooted cache goes bad at collection #0 and stays bad. It is also structurally invisible to `scripts/gc_root_dominance_check.py`, which reads emitted LLVM IR and cannot see a runtime table. 2. CODEGEN, implicit `this`. `js_implicit_this_set` returns the value it displaced from the `IMPLICIT_THIS` cell -- a scanned MUTABLE root -- and that value was then held in a bare SSA register across the whole call the bind exists to scope. The restore published a pre-move address back INTO a root. #7214 found this in `js_closure_callN` and left it; it was in fact seven lowerings with seven copies of the same three lines, now one shared `temp_root::implicit_this_save` / `implicit_this_restore` pair. 3. CODEGEN, ClassExprFresh (#7211). Its `protect_handle` predicate asked only whether the AUTHOR's static initializers could collect, never whether the `js_object_set_field_by_name` the lowering itself emits per static could -- and that allocates. The four allowlist entries it covered are deleted, which is the ratchet working: the fix made them match nothing. Checker: `js_implicit_this_set` is now a root READ (being non-collecting is what makes a call one), which takes its sink from 214 hits to 0 and keeps the class gated. `--stale-registers` now honours `--min-binds`, and `--any-def` with it is a usage error -- both the "a silently ignored knob is a disarmed knob" rule the mode already applied to `--max-stale` and `--fatal-sinks`. Refs #7154, #7211, #7213, #7196, #7206, #7214, #7161 * fix(gc): root the regexp receiver across ToString, and audit ALLOC_RE `Expr::RegExpTest` / `Expr::RegExpExec` unboxed the receiver to a raw `RegExpHeader*` before emitting `js_jsvalue_to_string_coerce`, which allocates and dispatches a user `toString`. Under a moving minor the register named from-space by the time `js_regexp_test` dereferenced it — #7154's residual, at `defineApiCall + 404` in the registry. Both take the established `guard_store_operand_across` / `reread_store_operand` pair and the unbox moves below the coerce. The checker missed it because `ALLOC_RE` spelled the allocator `regexp_alloc\w*` and the call is `js_regexp_new`. Reconciling every alternative against the runtime's real symbol table found four that match nothing at all (`regexp_alloc`, `promise_alloc`, `bigint_alloc`, `typed_array_alloc`): the runtime materializes fresh GC objects under three naming conventions (`_alloc*`, `_new*`, `_create*`) and the pattern modelled one. All three are now matched as conventions, with the non-conforming constructors enumerated explicitly. Second blind spot, and the one that matters for CI: the ToPrimitive family was not `POLL_CAPABLE_RUNTIME`, so even with `ALLOC_RE` widened the site was invisible to `--moving-only`, which is the mode the gate runs. Refs #7154, #7226, #7211, #7161 * fix(ci): apply the --min-funcs breadth floor in the stale-register and unrooted-alloca modes Both modes return before the floor was evaluated, so `--stale-registers --min-funcs 1200` and `--unrooted-allocas --min-funcs 1200` ran to a verdict over a corpus too thin to have exercised anything and exited 0. A documented liveness control silently disabled by a mode flag, in the script whose job is to catch gates that cannot fail. Reproduced on the self-test fixture (2 functions): stale and alloca modes both exited 0 at --min-funcs 50 while the bind-anchored default correctly exited 2. After the fix all three exit 2. n_funcs is computed above the mode branches and the message lives in one helper. self_test() gains a failing and a passing arm for each of the three modes, so neither a skipped floor nor an always-on one can regress silently. * ci(gc): gate ALLOC_RE's alternatives against the runtime's real symbol table The audit this PR did by hand found four alternatives matching nothing. Running the same reconciliation against the result found FIVE more that the audit itself introduced or left: `array_of`, `array_group_by`, `bigint_\w+_op`, `typed_array_from\w*`, `array_buffer_slice`. That is the argument for automating it rather than repeating it — a prose audit does not survive its own next edit. `--audit-alloc-re` decomposes ALLOC_RE into its alternatives and requires each to match at least one `extern "C" fn js_*` exported by perry-runtime or perry-stdlib. It refuses to render a verdict over a symbol scan that found implausibly few symbols, so it cannot pass by measuring itself, and it raises rather than silently checking zero alternatives if ALLOC_RE is rewritten into a shape it cannot decompose. Wired into gc-root-dominance.yml next to --self-test: static, no toolchain, no corpus. Verified both directions: green on the cleaned pattern (70 alternatives vs 3775 symbols), exit 2 with `regexp_alloc\w*` planted back in. self_test() gains arms for detection and for the absence of false positives. The five dead alternatives are deleted, which changes nothing about what the checker matches — that is what "matches no symbol" means. BigInt arithmetic (`js_bigint_add` and friends) allocates and is genuinely uncovered; widening for it is a coverage decision with its own false positives and is left to a change that can measure the new hits. * test(gc): register the regexp witness, and triage its allocation-point reds test_gap_gc_regexp_receiver_rooting.ts was added without a corpus entry, so nothing ran it. Registered, which makes it a hard gate on `loop_polls` -- the route this PR's fix is verified on: `bad 0` 8/8, byte-exact against the oracle, with the copying minor live. It is red on all TEN allocation-point arms (exit=139, deterministic, identical evidence on every one). That route forces the collection inside the allocating helper rather than at a loop safepoint, and this PR claims no fix for it. Triaged to #7217 with the measurements rather than left to turn gc-stress red. Second file with this exact signature -- #7216's assign_string_source witness is triaged to #7217 for the same reason. Two independent sites where a rooting fix that holds at a safepoint does not hold at the allocation point says something about the route, not about either fix. gc-stress verified green with the registration: --arms pr FAIL=0 XFAIL=2. --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
This was referenced Aug 2, 2026
Merged
Closed
proggeramlug
pushed a commit
that referenced
this pull request
Aug 2, 2026
…hat walked 1 of 3 sibling slots The #7231 enumeration, verified and closed. This is #7226's class -- a runtime table holding a GC pointer that is not a registered root -- which is strictly worse than #7154's stale-register class (it goes bad at collection #0 and stays bad, rather than needing a collection to land in a narrow window) and which no static instrument can find: `gc_root_dominance_check.py` reads emitted LLVM IR, and a runtime table is not in it. ★ `CACHED_ENV`, the `process.env` object, is the load-bearing one and it is a hard crash rather than a subtle wrong answer. `js_process_env_impl` builds it once with `js_object_alloc` -- the NURSERY -- and caches it in a thread-local `Cell<f64>` that is the ENTIRE reference graph: `process.env` is a `js_process_env()` call, not a field of the `process` object. So the first minor swept or evacuated it and every later `process.env.X = v` wrote through a dangling pointer. The sibling `PROCESS_FINALIZATION_OBJECT` uses the identical materialize-once idiom and was already rooted, which is what makes this an omission rather than a design. The observable is ENUMERATION -- `Object.keys(process.env)`, `for…in`, spread, which is how `@next/env` and `dotenv` consume it. A direct `process.env.KEY` read lowers to `js_getenv` and asks the OS, so a witness built on the read would be a gate that cannot fail. Also rooted: `CACHED_PERMISSION` and `CACHED_REPORT` (same shape; the `runtime_write_barrier_root_nanbox` beside the first is an incremental MARK barrier, not a root registration, and is now labelled as such); `ERROR_CONSTRUCTOR_PTR` (a raw duplicate of a `globalThis` closure, outside the object graph, stale after a move); `INPUT_HANDLER` (the inline `useInput` arrow, which nothing else refers to); `RESIZE_CALLBACK` (a native slot that bypasses the rooted EventEmitter listener array); `FRAME_CALLBACKS` (rooted only transiently during registration -- its `unsafe impl Send` SAFETY comment asserted the opposite and is corrected in place); `CURRENT_NEW_TARGET`; `ACCESSOR_RECEIVER_OVERRIDE`; and `PENDING_FETCH_SIGNAL`. Scanner gap, the shape #7230 found twice: `worker_threads.rs`'s `scan_parent_port_event_roots_mut` visited `MESSAGE_EVENT_CALLBACKS` and neither `MESSAGE_CALLBACK` nor `CLOSE_CALLBACK` -- three slots in the same `thread_local!` block holding the same raw `ClosureHeader*`. The box/visit/ unbox dance is factored into one helper, because three copies of it is how the fourth gets forgotten. Two further windows closed in `frame.rs` while rooting its queue. `js_frame_tick` drained into an unrooted local `Vec` and rooted each callback only as it invoked it, leaving #2..#N naked while #1 ran arbitrary user code (#7230's staging-buffer shape); one batch `RuntimeHandleScope` now covers the set. And `js_on_frame_callback` held the queue mutex across an allocating `capture_context()` -- harmless before, a self-deadlock once a scanner locks the same mutex. Verified against c9cd73b with `test_gap_gc_process_env_cache_rooting.ts`, registered in `test-parity/gc_repsel_corpus.txt`. Compiled AND run under `PERRY_GC_MOVING_LOOP_POLLS=1` with the evacuating base: SIGBUS (exit 138) 10/10 at base with no output at all, `bad 0` 10/10 after, byte-exact vs node 26.5.1. Clean 5/5 on the shipped default both sides, so this is a `requires=move` witness rather than a pre-existing failure. Under `ZEAL=1 PROTECT_FROMSPACE=1 DEPTH=800` the base arm faults at the stale dereference (`obj_type=2 size=416`, RETIRED FROM-SPACE) and this build is clean, so the instrument is a detector here and not a noise generator. `scripts/gc_repsel_matrix.sh --arms loop_polls --filter test_gap_gc_`: PASS=21 UNVER=0 FAIL=0, copy-minor live 21/21. Refs #7231, #7226, #7230, #7210, #7154, #7196.
proggeramlug
added a commit
that referenced
this pull request
Aug 2, 2026
…hat walked 1 of 3 sibling slots (#7239) The #7231 enumeration, verified and closed. This is #7226's class -- a runtime table holding a GC pointer that is not a registered root -- which is strictly worse than #7154's stale-register class (it goes bad at collection #0 and stays bad, rather than needing a collection to land in a narrow window) and which no static instrument can find: `gc_root_dominance_check.py` reads emitted LLVM IR, and a runtime table is not in it. ★ `CACHED_ENV`, the `process.env` object, is the load-bearing one and it is a hard crash rather than a subtle wrong answer. `js_process_env_impl` builds it once with `js_object_alloc` -- the NURSERY -- and caches it in a thread-local `Cell<f64>` that is the ENTIRE reference graph: `process.env` is a `js_process_env()` call, not a field of the `process` object. So the first minor swept or evacuated it and every later `process.env.X = v` wrote through a dangling pointer. The sibling `PROCESS_FINALIZATION_OBJECT` uses the identical materialize-once idiom and was already rooted, which is what makes this an omission rather than a design. The observable is ENUMERATION -- `Object.keys(process.env)`, `for…in`, spread, which is how `@next/env` and `dotenv` consume it. A direct `process.env.KEY` read lowers to `js_getenv` and asks the OS, so a witness built on the read would be a gate that cannot fail. Also rooted: `CACHED_PERMISSION` and `CACHED_REPORT` (same shape; the `runtime_write_barrier_root_nanbox` beside the first is an incremental MARK barrier, not a root registration, and is now labelled as such); `ERROR_CONSTRUCTOR_PTR` (a raw duplicate of a `globalThis` closure, outside the object graph, stale after a move); `INPUT_HANDLER` (the inline `useInput` arrow, which nothing else refers to); `RESIZE_CALLBACK` (a native slot that bypasses the rooted EventEmitter listener array); `FRAME_CALLBACKS` (rooted only transiently during registration -- its `unsafe impl Send` SAFETY comment asserted the opposite and is corrected in place); `CURRENT_NEW_TARGET`; `ACCESSOR_RECEIVER_OVERRIDE`; and `PENDING_FETCH_SIGNAL`. Scanner gap, the shape #7230 found twice: `worker_threads.rs`'s `scan_parent_port_event_roots_mut` visited `MESSAGE_EVENT_CALLBACKS` and neither `MESSAGE_CALLBACK` nor `CLOSE_CALLBACK` -- three slots in the same `thread_local!` block holding the same raw `ClosureHeader*`. The box/visit/ unbox dance is factored into one helper, because three copies of it is how the fourth gets forgotten. Two further windows closed in `frame.rs` while rooting its queue. `js_frame_tick` drained into an unrooted local `Vec` and rooted each callback only as it invoked it, leaving #2..#N naked while #1 ran arbitrary user code (#7230's staging-buffer shape); one batch `RuntimeHandleScope` now covers the set. And `js_on_frame_callback` held the queue mutex across an allocating `capture_context()` -- harmless before, a self-deadlock once a scanner locks the same mutex. Verified against c9cd73b with `test_gap_gc_process_env_cache_rooting.ts`, registered in `test-parity/gc_repsel_corpus.txt`. Compiled AND run under `PERRY_GC_MOVING_LOOP_POLLS=1` with the evacuating base: SIGBUS (exit 138) 10/10 at base with no output at all, `bad 0` 10/10 after, byte-exact vs node 26.5.1. Clean 5/5 on the shipped default both sides, so this is a `requires=move` witness rather than a pre-existing failure. Under `ZEAL=1 PROTECT_FROMSPACE=1 DEPTH=800` the base arm faults at the stale dereference (`obj_type=2 size=416`, RETIRED FROM-SPACE) and this build is clean, so the instrument is a detector here and not a noise generator. `scripts/gc_repsel_matrix.sh --arms loop_polls --filter test_gap_gc_`: PASS=21 UNVER=0 FAIL=0, copy-minor live 21/21. Refs #7231, #7226, #7230, #7210, #7154, #7196. Co-authored-by: Ralph Küpper <ralph@skelpo.com>
jdalton
added a commit
to jdalton/perry
that referenced
this pull request
Aug 2, 2026
An argument list is evaluated left to right and each finished value sits in
a bare SSA register while the later ones are lowered.
`lower_call/extern_func.rs`'s generic `perry_fn_<src>__<name>` path lowered
the whole list in a plain `for a in args` loop with no protection at all, so
`f(A, B, {…}, Schema.array(), body => …)` leaves A and B naming
pre-collection addresses the moment an evacuating minor lands in argument 3,
4 or 5 — and it does: argument 3 allocates an object, argument 4 runs user
code with its own back-edge polls, argument 5 allocates a closure.
This is PerryTS#7227's residual. In the registry it is `src/lib/api/alerts.ts`'s
module init calling `defineApiCall(url, method, …)` across the module
boundary; the fault surfaces one frame down inside `js_regexp_test`, because
the stale `url` is what `/\[[a-zA-Z]+\]/.test(url)` hands it. Measured at the
fault rather than read off the disassembly: the string literal's
`__perry_init_strings_*` handle global held the post-move address evacuation
wrote back, while the register held the retired from-space one.
A string-literal argument therefore takes `OperandProtection::Reload` — its
handle global is a registered root, so the string is never swept, and
re-emitting the load below the collection point costs no runtime call. Other
arguments take a real temp root. Both come from `lower_exprs_rooted`, which
already does this for the `new C(…)` list, and each argument is gated on
`any_later_ref_may_trigger_gc` so lists with nothing allocating after them
emit the IR they emitted before.
The static checker missed it because it classifies a heap-value source as an
`ALLOC_RE` call or a shadow-slot load; a load of a string-literal handle
global is neither. Over the new gap test's IR it reports 24 `--moving-only`
stale uses at the call and names none of them `%r9`/`%r10`, the two literals
that actually faulted.
Refs PerryTS#7154, PerryTS#7226, PerryTS#7227, PerryTS#7161
proggeramlug
added a commit
that referenced
this pull request
Aug 2, 2026
An argument list is evaluated left to right and each finished value sits in
a bare SSA register while the later ones are lowered.
`lower_call/extern_func.rs`'s generic `perry_fn_<src>__<name>` path lowered
the whole list in a plain `for a in args` loop with no protection at all, so
`f(A, B, {…}, Schema.array(), body => …)` leaves A and B naming
pre-collection addresses the moment an evacuating minor lands in argument 3,
4 or 5 — and it does: argument 3 allocates an object, argument 4 runs user
code with its own back-edge polls, argument 5 allocates a closure.
This is #7227's residual. In the registry it is `src/lib/api/alerts.ts`'s
module init calling `defineApiCall(url, method, …)` across the module
boundary; the fault surfaces one frame down inside `js_regexp_test`, because
the stale `url` is what `/\[[a-zA-Z]+\]/.test(url)` hands it. Measured at the
fault rather than read off the disassembly: the string literal's
`__perry_init_strings_*` handle global held the post-move address evacuation
wrote back, while the register held the retired from-space one.
A string-literal argument therefore takes `OperandProtection::Reload` — its
handle global is a registered root, so the string is never swept, and
re-emitting the load below the collection point costs no runtime call. Other
arguments take a real temp root. Both come from `lower_exprs_rooted`, which
already does this for the `new C(…)` list, and each argument is gated on
`any_later_ref_may_trigger_gc` so lists with nothing allocating after them
emit the IR they emitted before.
The static checker missed it because it classifies a heap-value source as an
`ALLOC_RE` call or a shadow-slot load; a load of a string-literal handle
global is neither. Over the new gap test's IR it reports 24 `--moving-only`
stale uses at the call and names none of them `%r9`/`%r10`, the two literals
that actually faulted.
Refs #7154, #7226, #7227, #7161
Co-authored-by: jdalton <john.david.dalton@gmail.com>
This was referenced Aug 2, 2026
jdalton
added a commit
to jdalton/perry
that referenced
this pull request
Aug 2, 2026
…source `--stale-registers` classified a heap-value SOURCE as an `ALLOC_RE` call or a shadow-slot load. A `load double, ptr @...str.N.handle` is neither, so the register it defines was never tracked and no stale use could be attributed to it -- which is the blind spot PerryTS#7240 shipped its fix through, as that PR's own writeup says. The pattern already existed and was defined twice, in effect: `--unrooted- allocas` had `REWRITTEN_LOAD_RE` and used it, while `--stale-registers` had only `GLOBAL_ROOT_RE` and knew about `@perry_global_*` alone. The two modes disagreed about what a collector-rewritten load is, and the narrower one was wrong. There is now one definition and both modes read it. Unlike PerryTS#7226's `js_implicit_this_set` and PerryTS#7227's `js_regexp_new`, this could not be closed by adding a name to `ALLOC_RE`: the source is a `load`, not a `call`. Strictly additive by construction -- `GLOBAL_ROOT_RE` is consulted first, so no previously reported source changes kind. Measured over the 116-source / 136-module corpus, emitted twice, once by the parent compiler and once by the commit below, so the checker delta and the codegen delta can be read separately: corpus from mode parent this parent codegen --stale-registers 2914 4805 (+1891 strh) parent codegen --moving-only 110 158 (+48 strh) parent codegen --moving-only --fatal-sinks 32 32 this codegen --stale-registers 2858 4693 (+1835 strh) this codegen --moving-only 62 62 (+0) this codegen --moving-only --fatal-sinks 0 0 Read the two middle rows together, because that is the whole result. On the parent's IR the widening exposes 48 stale uses that reach a moving minor, and ALL 48 are in the two gap tests added in the commit below -- every one a `load double, ptr @...str.N.handle` feeding `joinRest` or `joinSameRest` below the rest-array construction, which is precisely the defect that commit fixes. There are none anywhere else in the corpus. On the fixed IR the same widening adds ZERO `--moving-only` uses. The modelling is therefore not too broad: it found one population, that population was real, and it is now empty. The CI gate is untouched -- `gc-root-dominance.yml` runs the bind-anchored mode, not `--stale-registers`, and exits 0 with 0 violations and 40/40 seeded violations caught on both corpora with both checkers. `--self-test` asserts the new source in both directions and under `--moving-only`, so the widening cannot silently stop working. Recorded rather than hidden: the shared `REWRITTEN_LOAD_RE` also names `@perry_class_keys_*`, which `--unrooted-allocas` has always used. It contributes 0 hits in `--stale-registers` over this corpus, so that arm is currently carried by the shared definition rather than exercised by it. Refs PerryTS#7154.
proggeramlug
added a commit
that referenced
this pull request
Aug 2, 2026
…alton) (#7271) * fix(gc): root the rest-argument and same-module direct-call paths #7240 fixed `lower_call/extern_func.rs`'s cross-module NON-rest arm and named two siblings it would not ship unmeasured. These are those two. `extern_func.rs`'s `has_rest` arm had TWO unprotected registers where the non-rest arm had one. The fixed parameters, as before -- except their window does not close when the last argument is lowered, because the rest array is materialized afterwards and materializing it runs `js_array_alloc` plus one `js_array_push_f64` per trailing argument. And the ACCUMULATOR, which has no analogue in the non-rest arm: `current` is a raw `*mut ArrayHeader` in a bare SSA register, threaded through the push loop, holding the only reference to every argument pushed so far while the next argument's expression -- arbitrary user code -- is lowered. Nothing rooted it, so a minor landing in that window was free to SWEEP the array, not merely move it. `func_ref.rs`'s same-module arms, all four, had the identical defect. #7240's regression test needed a two-file fixture precisely because a same-file callee does not reach `extern_func.rs` at all: it resolves through `Expr::FuncRef(fid)` into `func_ref.rs`, so the bug sat one `else` away, unreached by that PR's test. It was not folded into #7240 because `func_ref.rs` threads its lowered arguments through four specialized-ABI dispatch paths, each a fast/fallback diamond with a phi at the merge; the temp-root release has to sit in the merge block that post-dominates all five call sites. The release is emitted AFTER `implicit_this_restore`, and that order is load-bearing. `implicit_this_save` (#7211) runs below the argument lowering, so its slot sits ABOVE this group, and `js_gc_temp_root_truncate` drops `base` and everything above it. Releasing first drops the saved receiver, and `js_gc_temp_root_get` answers an out-of-range read with `0` -- so the restore would rebind the enclosing method's `this` to the NUMBER 0. That is a miscompile, not a rooting bug, and it fires whenever a same-module callee reads dynamic `this` and at least one argument takes a real slot. All five arms now share one `lower_call/mod.rs` helper. Each argument is still gated by `temp_root::operand_protection`, so a list of scalars emits the IR it emitted before. Measured per gap test, compiled AND run with `PERRY_GC_MOVING_LOOP_POLLS=1`: arm parent (6aeef5b) this commit polls only bad 0 10/10 bad 0 10/10 polls + PERRY_GC_ZEAL=1 0/10, SIGSEGV bad 0 10/10 polls + zeal + PERRY_GEN_GC=0 bad 0 10/10 bad 0 10/10 The first row is why both test files carry a `parity-env:` line: without it the harness runs them in the default configuration, the broken compiler prints `bad 0`, and the files gate nothing. Polls are off by default since #7161, so the IR has no back-edge safepoint to collect on, and without zeal the only collections are allocation-triggered, which take `ManualGcScanGuard::force_full_scan` and make the copying minor ineligible -- nothing moves, so a stale register still names a live object. `run_parity_tests.sh` applies `parity-env` to the perry compile AND the perry run, which is what `PERRY_GC_MOVING_LOOP_POLLS` needs, since it is read at both. The `PERRY_GEN_GC=0` row is the control that proves the tests track collector mode rather than being flaky. Statically, over the 116-source corpus emitted by the parent compiler and read with the parent checker (so this is the codegen delta alone): `--stale-registers --moving-only` 110 -> 62, and `--moving-only --fatal-sinks` 32 -> 0. Those 32 were all `source=alloc sink=js_array_push_f64` -- the unrooted rest accumulator. Refs #7154. * feat(gc-checker): model a string-literal handle load as a heap-value source `--stale-registers` classified a heap-value SOURCE as an `ALLOC_RE` call or a shadow-slot load. A `load double, ptr @...str.N.handle` is neither, so the register it defines was never tracked and no stale use could be attributed to it -- which is the blind spot #7240 shipped its fix through, as that PR's own writeup says. The pattern already existed and was defined twice, in effect: `--unrooted- allocas` had `REWRITTEN_LOAD_RE` and used it, while `--stale-registers` had only `GLOBAL_ROOT_RE` and knew about `@perry_global_*` alone. The two modes disagreed about what a collector-rewritten load is, and the narrower one was wrong. There is now one definition and both modes read it. Unlike #7226's `js_implicit_this_set` and #7227's `js_regexp_new`, this could not be closed by adding a name to `ALLOC_RE`: the source is a `load`, not a `call`. Strictly additive by construction -- `GLOBAL_ROOT_RE` is consulted first, so no previously reported source changes kind. Measured over the 116-source / 136-module corpus, emitted twice, once by the parent compiler and once by the commit below, so the checker delta and the codegen delta can be read separately: corpus from mode parent this parent codegen --stale-registers 2914 4805 (+1891 strh) parent codegen --moving-only 110 158 (+48 strh) parent codegen --moving-only --fatal-sinks 32 32 this codegen --stale-registers 2858 4693 (+1835 strh) this codegen --moving-only 62 62 (+0) this codegen --moving-only --fatal-sinks 0 0 Read the two middle rows together, because that is the whole result. On the parent's IR the widening exposes 48 stale uses that reach a moving minor, and ALL 48 are in the two gap tests added in the commit below -- every one a `load double, ptr @...str.N.handle` feeding `joinRest` or `joinSameRest` below the rest-array construction, which is precisely the defect that commit fixes. There are none anywhere else in the corpus. On the fixed IR the same widening adds ZERO `--moving-only` uses. The modelling is therefore not too broad: it found one population, that population was real, and it is now empty. The CI gate is untouched -- `gc-root-dominance.yml` runs the bind-anchored mode, not `--stale-registers`, and exits 0 with 0 violations and 40/40 seeded violations caught on both corpora with both checkers. `--self-test` asserts the new source in both directions and under `--moving-only`, so the widening cannot silently stop working. Recorded rather than hidden: the shared `REWRITTEN_LOAD_RE` also names `@perry_class_keys_*`, which `--unrooted-allocas` has always used. It contributes 0 hits in `--stale-registers` over this corpus, so that arm is currently carried by the shared definition rather than exercised by it. Refs #7154. * docs(changelog): fragment for the #7154 rest/same-module rooting follow-ups * test(gc): register #7270's two witnesses, and PR-key its changelog fragment --------- Co-authored-by: jdalton <john.david.dalton@gmail.com> Co-authored-by: Ralph Küpper <ralph@skelpo.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Base
main. Stacks on nothing — #7206, #7212, #7214 and #7216 are all merged.Step 1 of the brief was "run #7196's instruments on the registry first, nobody has done it." That was the whole job. The static checker had been ground down through #7192 → #7206 → #7214 without the registry moving, because the bug that was actually killing it is structurally invisible to a tool that reads emitted LLVM IR.
What the instruments named
Built the registry with polls on, ran it under
PERRY_GC_ZEAL=1 PERRY_GC_PROTECT_FROMSPACE=1 PERRY_GC_PROTECT_FROMSPACE_DEPTH=800:obj_type3 is a string; 40 bytes is a 32-byteStringHeaderplus"string". Disassembling+100lands just afterjs_string_equals, fed byjs_value_typeof.js_value_typeofinterns its eight result strings in thread-localCell<*mut StringHeader>s that nothing registers as a GC root. Allocated in the nursery, referenced by nothing else — so the first minor sweeps or evacuates it and the cache names abandoned memory for the rest of the process.json/raw_json.rs's cached"rawJSON"key has the identical defect and is fixed with it.The generalisable part
retired_by_minor=#0is the tell, and it is worth internalising:gc_root_dominance_check.pyover emitted IRA perfectly reproducible GC bug is evidence against a stale register. The registry failed 0/10 on
main, and that determinism was the clue — it had been read as "a worse instance of the same thing" rather than "a different thing".Also fixed
prev_this, the follow-up #7214 measured and left.js_implicit_this_setreturns the value it displaced from theIMPLICIT_THIScell — a scanned mutable root — which is then held in a bare register across the whole call, and republished into that root by the restore. #7214 found it injs_closure_callN; it is in fact seven lowerings with seven copies of the same three lines (console_promise.rs,method_override.rs, bothproperty_getdispatchers,static_dispatch.rs,func_ref.rs, and bothearly_branches.rsarms). Now one sharedtemp_root::implicit_this_save/implicit_this_restore, so an eighth gets the root rather than the bug.#7211,
ClassExprFresh. The predicate asked only whether the AUTHOR's initializers could collect, never whether thejs_object_set_field_by_namethe lowering emits per static could — and it allocates. All four allowlist entries are deleted, which is the ratchet working: the fix made them match nothing, the gate went red, and that red is the instruction to remove them. The allowlist is now empty.Checker
js_implicit_this_setis a root READ. Being non-collecting is exactly what makes a call one — the rulejs_closure_get_capture_bitsis listed under. It wasNONCOLLECTINGwithout being a source, so the checker saw a safe call, never classified the result as a heap value, and reported nothing at either end. That blind spot is precisely whyprev_thissurvived fix(codegen): root a call receiver and a computed-read base across their sibling operands #7206 and fix(codegen): root the callee,thisand every argument of js_closure_callN #7214.--stale-registersnow honours--min-binds, and--any-defwith it is a usage error (review feedback on fix(codegen): root the callee,thisand every argument of js_closure_callN #7214:1439 and fix(codegen): root a call receiver and a computed-read base across their sibling operands #7206:1453 — both valid, verified against the code).--min-bindsis not a bind-anchored detail:run_staleclassifies a shadow-slot load as a heap-value source from the sameBIND_REmap, so a corpus built withPERRY_INLINE_SHADOW_SLOT=1loses those sources, reportstotal 0and exits 0 — hazard 4, the gate runs but its subject never did. Both directions asserted in--self-test.Verification
Measured against
7d1dc9ca2, built from its own worktree (not borrowed from another one — CLAUDE.md's rule).sfw-registry --help, compiled and run withPERRY_GC_MOVING_LOOP_POLLS=1(compile-time since #7161 and runtime-armed atgc/policy.rs:1759; arming only one is the false green that cost #7214 a round):POLLS=1, 10 runsPOLLS=1, 30 runsThe 30-run number is the honest one, and it is why this does not close #7154. A 10-run sample said 10/10; widening it found the residual. See below.
test_gap_gc_typeof_string_cache_rooting.ts,POLLS=1bad 44410/10bad 010/10PERRY_GEN_GC=0bad 0bad 0test_gap_gc_closure_call_prev_this_rooting.ts,POLLS=1+ zealbad 4005/5bad 010/10PERRY_GEN_GC=0+ zealbad 0bad 0prev_thisneeds zeal, and the reason is structural rather than incidental: its window is a user call, so only a moving collection exploits it, and the only moving collections are the loop back-edge poll and the microtask-pump safepoint — allocation-triggered ones takeManualGcScanGuard::force_full_scan, which makes the copying minor ineligible. ThePERRY_GEN_GC=0arms prove both tests track collector mode rather than merely being flaky.ClassExprFreshhas no runtime gap test, deliberately — same argument: its window holds onlyjs_object_set_field_by_name, no user code and no loop, so no moving collection can land in it today. The invariant is the subject, so the gate is the test:--moving-only, 130 modules / 2170 functions--stale-registerstotal--stale-registers,sink=js_implicit_this_set--stale-registers --fatal-sinkscargo test -p perry-codegen: 6 failures on the parent, the identical 6 here — allloop_safepoint_purity, which is #7161's default flip. Measured on the parent rather than assumed.Cost, over the 141-module registry corpus (the implicit-
thisroot is on the dynamic-dispatch path, so it was measured):26/30, not 30/30. There is one more stale register and the reporter names it exactly —
perry_fn_src_lib_api_shared_ts__defineApiCall + 404,obj_type=3 size=80,retired_by_minor=#161:A textbook #7154 stale register in the
/re/.test(s)lowering. The checker misses it becauseALLOC_REmatchesregexp_alloc\w*but the call isjs_regexp_new— so the register has no recognised heap-value source, exactly the same shape of blind spot asjs_implicit_this_sethad. Addingjs_regexp_newtoALLOC_REshould surface it, and the fix is the establishedguard_store_operand/reread_store_operandpair. Left out of this PR because it wants its own before/after rather than being appended to a change already this size.Verified pre-existing rather than introduced here: the identical fault — same site,
obj_type, size — reproduces on the build without the seven implicit-thisroots.Deliberately not fixed: #7213
js_get_string_pointer_unifiedhands generated code a raw*StringHeader, and its SSO branch allocates. Everyunbox_str_handlesite unboxes its operands back-to-back into bare registers before one consuming call, so the first handle is live across the second unbox with no root describing it. It is not exploitable today — that allocation reaches the alloc-point arm ofgc_check_trigger, which forces a conservative stack scan, which makes the copying minor ineligible, so the collection it can cause never moves anything, and the same scan keeps the handle alive. Both halves are closed by accident, and by accident is the point: it rests on the alloc-point arm staying non-moving, which is the property the moving-GC work keeps eroding.A
GcSuppressScopearound that allocation was written, built, and reverted: shipping a GC-trigger change with no test that can fail without it is what CLAUDE.md's knob-kill policy exists to stop. Filed as #7213 with the analysis, and the reasoning is recorded instring/alloc.rsso it is not rediscovered.js_get_string_pointer_unifiedis likewise kept out ofROOT_READ_CALLS— classifying it reports the whole family (~40 sites), which deserves its own measured count.Refs #7154, #7211, #7213, #7196, #7206, #7214, #7161
Summary by CodeRabbit
Bug Fixes
typeofandJSON.rawJSONstrings.thiscorrectly across closure calls, dispatches, and allocating operations.Tests
typeofresults.Documentation