fix(gc): root the regexp receiver across ToString, and audit ALLOC_RE against the runtime's real symbol table - #7227
Merged
proggeramlug merged 9 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
jdalton
force-pushed
the
fix/7154-regexp-new-stale-register
branch
from
August 2, 2026 03:42
8a22b07 to
6149ca1
Compare
|
Warning Review limit reached
Next review available in: 22 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR registers runtime string caches as mutable GC roots, preserves implicit ChangesGC rooting and validation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
`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
force-pushed
the
fix/7154-regexp-new-stale-register
branch
from
August 2, 2026 03:43
6149ca1 to
28dc70d
Compare
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/perry-codegen/src/lower_call/property_get/static_dispatch.rs (1)
216-243: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftRoot
recv_boxacross the argument lowering, not only the displacedthis.
recv_boxis computed at lines 94-120 before the argument lowering at lines 138-215. That lowering callslower_expron argument expressions and allocates rest/argumentsarrays, so it can run user code. For theClassExprFresh,Call,Sequence, andLocalGetarms,recv_boxnames a real heap class object thatimplicit_this_saveandjs_static_this_arm_valuecan publish before the collector moves it. Guardrecv_boxwith the store-operand helpers, re-read it below the argument lowering, and release the guard after the callee.🤖 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/lower_call/property_get/static_dispatch.rs` around lines 216 - 243, Root recv_box across argument lowering, not just the displaced implicit this: in the static-dispatch flow around implicit_this_save and the lowered argument expressions, use the store-operand guard helpers for receiver shapes that produce a heap class object (ClassExprFresh, Call, Sequence, and LocalGet), re-read the guarded receiver before js_static_this_arm_value, and release the guard only after the callee call and restoration complete.
🧹 Nitpick comments (3)
crates/perry-runtime/src/builtins/arithmetic.rs (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract a shared helper for cached-
StringHeader-pointer root scanning. Both scanners implement the identical mark-and-rewrite sequence for aCell<*mut StringHeader>, differing only in which cell they visit. The shared root cause is the lack of a single helper function for this shape.
crates/perry-runtime/src/builtins/arithmetic.rs#L565-588: replace the localvisitfunction body with a call to a shared helper, e.g.fn root_and_rewrite_string_cell(cell: &Cell<*mut StringHeader>, visitor: &mut RuntimeRootVisitor<'_>).crates/perry-runtime/src/json/raw_json.rs#L96-106: replacescan_raw_json_key_root_mut's body with a call to the same shared helper.Centralizing this logic reduces the risk that a future cache addition (the PR notes more may follow, since this defect class was found "by grepping for the shape") gets the tag or null-check wrong through copy-paste.
♻️ Proposed shared helper
/// GC mutable-root helper: mark and rewrite a cached `*mut StringHeader` cell. pub(crate) fn root_and_rewrite_string_cell( cell: &std::cell::Cell<*mut StringHeader>, visitor: &mut crate::gc::RuntimeRootVisitor<'_>, ) { let mut ptr = cell.get() as *const StringHeader; if ptr.is_null() { return; } if visitor.visit_tagged_raw_const_ptr_slot(&mut ptr, crate::value::STRING_TAG) { cell.set(ptr as *mut StringHeader); } }🤖 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/builtins/arithmetic.rs` at line 1, Extract a shared `root_and_rewrite_string_cell` helper for marking and rewriting cached `Cell<*mut StringHeader>` roots, preserving the existing null check, `STRING_TAG`, and pointer update behavior. Replace the local `visit` implementation in the arithmetic scanner and the body of `scan_raw_json_key_root_mut` with calls to this helper, importing it from its defining module as needed.scripts/gc_root_dominance_check.py (1)
2190-2208: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
--min-funcsstays unenforced on the--stale-registerspath.This change correctly moves the
--min-bindsassertion aboverun_stale. The--min-funcsassertion at Lines 2308-2313 is still below the earlyreturnat Line 2208, so the stale mode never reaches it. Per the flag's own help text at Lines 2105-2108, files and binds can look healthy while the corpus is one module deep. That is the same disarmed-knob hazard the--min-bindsmove just closed, and the--unrooted-allocasbranch has the same gap at Line 2249.Counting functions once, above both early returns, closes both.
♻️ Proposed hoist of the breadth assertion
n_binds = sum( 1 for _m, fs in parsed for f in fs for b in f.blocks for ins in f.insns[b] if BIND_RE.search(ins.text) ) + n_funcs = sum(len(fs) for _m, fs in parsed) + if n_funcs < ns.min_funcs: + print(f"error: checked {n_funcs} function(s), need at least " + f"{ns.min_funcs}. The corpus compiled but is too thin to have " + "exercised the lowerings this invariant runs through.", + file=sys.stderr) + return 2Then drop the duplicated
n_funcsbinding at Line 2255 and the duplicated check at Lines 2308-2313.Add a self-test arm next to the new
--min-bindsarms so the hoisted guard cannot be disarmed again:if _main_probe(["--stale-registers", "--min-funcs", "50", planted]) != 2: print("self-test FAIL: --stale-registers over a corpus with fewer than " "--min-funcs functions must exit 2", 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 2190 - 2208, Hoist the function-count calculation and --min-funcs validation before the early returns for --stale-registers and --unrooted-allocas, using the existing function-counting logic and error behavior. Remove the later duplicated n_funcs binding and check, then add the requested _main_probe self-test beside the --min-binds probes to verify stale-register mode exits 2 when below --min-funcs.CLAUDE.md (1)
253-254: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep
CLAUDE.mdconcise.Line 253 and Line 254 duplicate detailed incident history already suited to
changelog.d/and the rooting documentation. Keep only the durable invariants, detector commands, and links to the detailed records.As per coding guidelines: keep
CLAUDE.mdconcise and put detailed change history inchangelog.d/fragments.🤖 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, Condense the two GC-rooting bullets in CLAUDE.md by retaining only the durable rooting invariants, relevant detector commands, CI/allowlist references, and links to the detailed rooting documentation or changelog records. Remove the incident-by-incident history, issue numbers, failure timelines, and implementation-specific narrative; preserve the guidance to register runtime raw-pointer caches via gc_register_mutable_root_scanner.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.
Inline comments:
In `@crates/perry-runtime/src/json/raw_json.rs`:
- Around line 88-106: Add a cfg(test) reset_raw_json_key_cache_for_test function
alongside scan_raw_json_key_root_mut that clears RAW_JSON_KEY, then invoke it
from the shared test arena-reset teardown used by
reset_typeof_string_cache_for_test. Ensure the cache is cleared whenever test
arenas are reset.
In `@scripts/gc_root_dominance_check.py`:
- Around line 327-370: Update heap_source_kind to check ROOT_READ_CALLS before
applying ALLOC_RE, ensuring js_new_target_get and js_new_target_set remain
classified as root reads rather than allocations. Preserve the existing
allocation matching rules for all other names.
---
Outside diff comments:
In `@crates/perry-codegen/src/lower_call/property_get/static_dispatch.rs`:
- Around line 216-243: Root recv_box across argument lowering, not just the
displaced implicit this: in the static-dispatch flow around implicit_this_save
and the lowered argument expressions, use the store-operand guard helpers for
receiver shapes that produce a heap class object (ClassExprFresh, Call,
Sequence, and LocalGet), re-read the guarded receiver before
js_static_this_arm_value, and release the guard only after the callee call and
restoration complete.
---
Nitpick comments:
In `@CLAUDE.md`:
- Around line 253-254: Condense the two GC-rooting bullets in CLAUDE.md by
retaining only the durable rooting invariants, relevant detector commands,
CI/allowlist references, and links to the detailed rooting documentation or
changelog records. Remove the incident-by-incident history, issue numbers,
failure timelines, and implementation-specific narrative; preserve the guidance
to register runtime raw-pointer caches via gc_register_mutable_root_scanner.
In `@crates/perry-runtime/src/builtins/arithmetic.rs`:
- Line 1: Extract a shared `root_and_rewrite_string_cell` helper for marking and
rewriting cached `Cell<*mut StringHeader>` roots, preserving the existing null
check, `STRING_TAG`, and pointer update behavior. Replace the local `visit`
implementation in the arithmetic scanner and the body of
`scan_raw_json_key_root_mut` with calls to this helper, importing it from its
defining module as needed.
In `@scripts/gc_root_dominance_check.py`:
- Around line 2190-2208: Hoist the function-count calculation and --min-funcs
validation before the early returns for --stale-registers and
--unrooted-allocas, using the existing function-counting logic and error
behavior. Remove the later duplicated n_funcs binding and check, then add the
requested _main_probe self-test beside the --min-binds probes to verify
stale-register mode exits 2 when below --min-funcs.
🪄 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: 82bf97db-b60d-4732-9106-02ec1ccded7a
📒 Files selected for processing (23)
CLAUDE.mdchangelog.d/7219-registry-gc-unrooted-caches.mdchangelog.d/7227-regexp-receiver-rooting-and-alloc-re-audit.mdcrates/perry-codegen/src/expr/instance_misc1.rscrates/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_regexp_receiver_rooting.tstest-files/test_gap_gc_typeof_string_cache_rooting.ts
Comment on lines
+88
to
+106
| thread_local! { | ||
| static RAW_JSON_KEY: std::cell::Cell<*mut StringHeader> = | ||
| const { std::cell::Cell::new(std::ptr::null_mut()) }; | ||
| } | ||
|
|
||
| /// GC mutable-root scanner for the cached `"rawJSON"` key (#7211). Marks it so | ||
| /// it is not swept and rewrites it so an evacuating minor cannot leave the cell | ||
| /// naming from-space. | ||
| pub fn scan_raw_json_key_root_mut(visitor: &mut crate::gc::RuntimeRootVisitor<'_>) { | ||
| RAW_JSON_KEY.with(|cell| { | ||
| let mut ptr = cell.get() as *const StringHeader; | ||
| if ptr.is_null() { | ||
| return; | ||
| } | ||
| if visitor.visit_tagged_raw_const_ptr_slot(&mut ptr, crate::value::STRING_TAG) { | ||
| cell.set(ptr as *mut StringHeader); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for an existing raw_json_key test-reset function, and whether JSON.rawJSON
# is exercised by tests that reset arenas between runs.
rg -n "reset_typeof_string_cache_for_test|reset_raw_json_key_cache_for_test|RAW_JSON_KEY" --type=rust
rg -n "raw_json_key|rawJSON" --type=rust -g '*test*'Repository: PerryTS/perry
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -u
echo "tracked rust test files:"
git ls-files '*.rs' | rg '(^|/)(tests|test|.*test.*)\.rs$|(^|/)tests/|(^|/)test/' || true
echo
echo "search raw_json_key/RawJSON/rawJSON/RAW_JSON_KEY reset_typeof:"
rg -n "raw_json_key|RawJSON|rawJSON|RAW_JSON_KEY|reset_typeof_string_cache_for_test|reset_raw_json_key_cache_for_test|JSON" crates/perry-runtime --type=rust || true
echo
echo "changed files summary:"
git diff --stat 2>/dev/null || true
git diff -- crates/perry-runtime/src/json/raw_json.rs | sed -n '1,220p' 2>/dev/null || trueRepository: PerryTS/perry
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -u
echo "raw_json.rs:"
cat -n crates/perry-runtime/src/json/raw_json.rs | sed -n '1,220p'
echo
echo "arithmetic.rs typeof cache/reset:"
rg -n "typeof|TYPEOF|reset_typeof|cache|Cell" crates/perry-runtime/src/builtins/arithmetic.rs | sed -n '1,220p'
cat -n crates/perry-runtime/src/builtins/arithmetic.rs | sed -n '1,180p'
echo
echo "arena reset definitions/usages:"
rg -n "reset(_arena|_arenas)|Arena|arena|gc_reset|new_arena|thread_local|#[cfg\(test\)]" crates/perry-runtime/src crates/perry-runtime/tests 2>/dev/null | head -n 240
echo
echo "raw_json tests:"
rg -n "raw_json|rawJSON|isRawJSON|RawJSON|rawJSON" crates/perry crates/perry-runtime --type=rust || trueRepository: PerryTS/perry
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -u
echo "runtime roots registration:"
rg -n "scan_raw_json_key_root_mut|scan_typeof_string_roots_mut|RuntimeRootVisitor|register.*root|roots" crates/perry-runtime/src/gc crates/perry-runtime/src --type=rust | sed -n '1,220p'
echo
echo "raw_json tests exact:"
rg -n "raw_json|rawJSON|isRawJSON|RawJSON" crates/perry crates/perry-runtime --type=rust | rg "tests|test" || true
echo
echo "concrete reset call sites:"
rg -n "reset_typeof_string_cache_for_test|reset_.*cache_for_test|isolate|Arena|reset_arena|js_arena|reset_gc|clear.*arena" crates/perry crates/perry-runtime --type=rust | sed -n '1,240p'
echo
echo "raw_json implementation slice:"
cat -n crates/perry-runtime/src/json/raw_json.rs | sed -n '70,122p'Repository: PerryTS/perry
Length of output: 50369
Clear RAW_JSON_KEY in the test arena-reset path.
reset_typeof_string_cache_for_test() exists because test arenas can be reset while thread-local caches still hold arena pointers. RAW_JSON_KEY has the same shape, so add #[cfg(test)] reset_raw_json_key_cache_for_test() and call it from the shared test teardown if tests reset arenas.
🤖 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 - 106, Add a
cfg(test) reset_raw_json_key_cache_for_test function alongside
scan_raw_json_key_root_mut that clears RAW_JSON_KEY, then invoke it from the
shared test arena-reset teardown used by reset_typeof_string_cache_for_test.
Ensure the cache is cleared whenever test arenas are reset.
added 2 commits
August 2, 2026 05:51
…l 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.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@scripts/gc_root_dominance_check.py`:
- Around line 2190-2208: Before the stale-register return in the main validation
flow, count the corpus functions and enforce ns.min_funcs using the existing
function-count validation logic. Ensure --stale-registers with --min-funcs
rejects corpora below the threshold before invoking run_stale, while preserving
the existing stale-register and min-binds behavior.
🪄 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: 74e85644-981d-4934-b9cd-d0fca4a72dd8
📒 Files selected for processing (23)
CLAUDE.mdchangelog.d/7219-registry-gc-unrooted-caches.mdchangelog.d/7227-regexp-receiver-rooting-and-alloc-re-audit.mdcrates/perry-codegen/src/expr/instance_misc1.rscrates/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_regexp_receiver_rooting.tstest-files/test_gap_gc_typeof_string_cache_rooting.ts
🚧 Files skipped from review as they are similar to previous changes (18)
- crates/perry-codegen/src/lower_call/early_branches.rs
- scripts/gc_root_dominance_allowlist.json
- CLAUDE.md
- crates/perry-runtime/src/builtins/mod.rs
- crates/perry-codegen/src/expr/static_field_meta.rs
- crates/perry-runtime/src/gc/mod.rs
- test-files/test_gap_gc_typeof_string_cache_rooting.ts
- crates/perry-runtime/src/json/raw_json.rs
- crates/perry-codegen/src/lower_call/method_override.rs
- crates/perry-codegen/src/lower_call/property_get/static_dispatch.rs
- crates/perry-runtime/src/json/mod.rs
- crates/perry-codegen/src/lower_call/func_ref.rs
- crates/perry-codegen/src/expr/instance_misc1.rs
- test-files/test_gap_gc_closure_call_prev_this_rooting.ts
- crates/perry-runtime/src/builtins/arithmetic.rs
- crates/perry-runtime/src/string/alloc.rs
- crates/perry-codegen/src/lower_call/property_get/dynamic_dispatch.rs
- crates/perry-codegen/src/expr/temp_root.rs
Contributor
|
Conflicts resolved (merge of the resolved #7226 branch, no rebase, jdalton's The audit needed to be a gate, and here is the proofThe question was whether a dead So the score is four dead alternatives found by a use-after-free investigation, removed by hand, and five introduced or left by the same removal. That is not a criticism of the audit — it is the argument that a prose audit does not survive its own next edit. The durable fix is the check.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
scripts/gc_root_dominance_check.py (2)
2296-2307: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReject
--audit-alloc-recombined with corpus flags instead of ignoring them.The early return runs before every other usage check. Therefore
--audit-alloc-re ir-corpus --min-funcs 1200 --seeded-violations 40runs only the static audit, exits 0, and never scans the corpus. The command line claims a corpus run. The script already refuses this shape for--max-stale,--fatal-sinks,--any-def, and the two mode flags, with the stated rule that a silently ignored knob is a disarmed knob. Apply the same rule here.🛡️ Proposed usage guard
ns = ap.parse_args() # Standalone static audit: it reads the crates, not an IR corpus, so it is # checked before the corpus arguments are. if ns.audit_alloc_re: + # Same rule the other mode flags carry: this pass takes no corpus and + # consults none of the corpus knobs, so accepting them alongside it + # would promise a scan that never runs. + if ns.paths or ns.self_test or ns.stale_registers or \ + ns.unrooted_allocas or ns.allowlist or ns.seeded_violations: + ap.error("--audit-alloc-re takes no corpus and no corpus options; " + "run it on its own") return audit_alloc_re()The
--min-files,--min-binds, and--min-funcsfloors have argparse defaults, so test them against their defaults rather than truthiness if you include them.🤖 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 2296 - 2307, Update the argument validation around audit_alloc_re() so --audit-alloc-re rejects any corpus path or corpus-only options instead of silently ignoring them; compare --min-files, --min-binds, and --min-funcs against their argparse defaults rather than truthiness, then retain the early static-audit return only for a standalone invocation.
2025-2059: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover
audit_alloc_re()itself in the self-test, not onlydead_alloc_alternatives().These arms exercise the decomposition and the dead-alternative detection. They never call
audit_alloc_re(). Three behaviors that CI depends on stay unproven: thelen(syms) < 500non-vacuity floor, the exit code 2 on a dead alternative, and the exit code 0 on a clean verdict.audit_alloc_reaccepts arootsargument, so both directions are cheap to assert with temporary directories.♻️ Proposed additional self-test arms
residue = dead_alloc_alternatives(synthetic) if residue: print(f"self-test FAIL: the auditor reported {len(residue)} " "alternative(s) dead against a symbol set synthesised from " f"the alternatives themselves: {residue}. That is a bug in " "the auditor, not in ALLOC_RE.", file=sys.stderr) ok = False + # The exit-status surface CI actually calls. An empty scan must be + # rejected as a measurement of itself, not reported as a dead pattern. + empty_root = os.path.join(td, "no-such-crates") + if audit_alloc_re(roots=(empty_root,)) != 2: + print("self-test FAIL: --audit-alloc-re over a root with no " + "runtime sources must exit 2. A scan that finds nothing " + "would otherwise report every alternative dead.", + file=sys.stderr) + ok = FalseNote the
residuearm reports "a bug in the auditor" for any alternative whose regex metacharacters are not\w*or\w+. Consider widening the synthetic-name construction or the message when a new metacharacter is introduced.As per coding guidelines,
scripts/gc_root_dominance_check.py: "use its self-test".🤖 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 2025 - 2059, Extend the self-test around the existing ALLOC_RE checks to invoke audit_alloc_re() with temporary roots and synthetic symbol sets. Assert that a sufficiently small symbol set triggers the len(syms) < 500 non-vacuity failure with exit code 2, while a clean set covering every alternative returns exit code 0. Update synthetic-name generation or its diagnostic so newly introduced regex metacharacters are handled explicitly rather than misreported.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.
Inline comments:
In @.github/workflows/gc-root-dominance.yml:
- Around line 91-106: Update SYMBOL_ROOTS in gc_root_dominance_check.py to
include the perry-ext-* crates so the ALLOC_RE audit scans all exported extern
"C" js_* runtime symbols, or explicitly document and enforce the intended
extension exclusion without masking new allocations. Also add gc-root-dominance
to the branch protection required status contexts.
---
Nitpick comments:
In `@scripts/gc_root_dominance_check.py`:
- Around line 2296-2307: Update the argument validation around audit_alloc_re()
so --audit-alloc-re rejects any corpus path or corpus-only options instead of
silently ignoring them; compare --min-files, --min-binds, and --min-funcs
against their argparse defaults rather than truthiness, then retain the early
static-audit return only for a standalone invocation.
- Around line 2025-2059: Extend the self-test around the existing ALLOC_RE
checks to invoke audit_alloc_re() with temporary roots and synthetic symbol
sets. Assert that a sufficiently small symbol set triggers the len(syms) < 500
non-vacuity failure with exit code 2, while a clean set covering every
alternative returns exit code 0. Update synthetic-name generation or its
diagnostic so newly introduced regex metacharacters are handled explicitly
rather than misreported.
🪄 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: 55a3a3e9-4a97-46a0-9a61-2366f3fe470b
📒 Files selected for processing (4)
.github/workflows/gc-root-dominance.ymlCLAUDE.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
Comment on lines
+91
to
+106
| # The checker only reports a stale register when it recognises what | ||
| # MATERIALIZED the value, and that recognition is one regex. An | ||
| # alternative in it that matches no real symbol reads as coverage and is | ||
| # not -- the gate-can't-fail pattern in regex form. It has shipped nine | ||
| # times across two rounds: four (`regexp_alloc\w*`, `promise_alloc\w*`, | ||
| # `bigint_alloc\w*`, `typed_array_alloc\w*`) found only because | ||
| # `js_regexp_new` cost #7154 a whole investigation round, and five more | ||
| # introduced by the change that removed those four. A prose audit does not | ||
| # survive its own next edit, so it is a gate now: every alternative must | ||
| # match at least one `extern "C" fn js_*` the runtime actually exports. | ||
| # | ||
| # Static and instant — no toolchain, no corpus — so it runs before the | ||
| # build with the self-test. | ||
| - name: ALLOC_RE alternatives must match real runtime symbols | ||
| run: python3 scripts/gc_root_dominance_check.py --audit-alloc-re | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm the workflow's cancellation policy and locate every crate
# that exports the C-ABI js_* surface the audit scans.
set -euo pipefail
echo "=== concurrency / trigger block ==="
sed -n '1,80p' .github/workflows/gc-root-dominance.yml
echo
echo "=== continue-on-error anywhere in this workflow ==="
rg -n 'continue-on-error' .github/workflows/gc-root-dominance.yml || echo "none"
echo
echo "=== crates exporting `extern \"C\" fn js_*`, by crate ==="
rg -nP --glob '*.rs' 'extern\s+"C"\s+fn\s+js_\w+' crates \
| sed -E 's#^(crates/[^/]+)/.*#\1#' | sort | uniq -c | sort -rn
echo
echo "=== total symbols under the two roots the audit scans ==="
rg -oP --glob '*.rs' 'extern\s+"C"\s+fn\s+\Kjs_\w+' \
crates/perry-runtime/src crates/perry-stdlib/src | sort -u | wc -lRepository: PerryTS/perry
Length of output: 5864
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== all Rust files with exact extern C js_* declarations, grouped"
python3 - <<'PY'
import re, pathlib
pat = re.compile(r'''^[\s]*(pub\s+)?extern\s+"C"\s+fn\s+(js_\w+)\b''', re.M)
counts={}
for p in pathlib.Path('crates').glob('**/*.rs'):
s=p.read_text(errors='replace')
names=pat.findall(s)
real = [x[1] for x in names if isinstance(x, tuple)]
if not real:
names=pat.findall(s)
real = [x for x in names if not isinstance(x, tuple)]
if real:
counts[p]=real
for p, names in sorted(counts.items()):
print(len(names), p)
for n in names[:5]:
print(" ", n)
if len(names)>5:
print(" ...")
print("total_files", len(counts))
print("total_symbols", sum(len(v) for v in counts.values()))
PY
echo
echo "=== workflow references to SYMBOL_ROOTS and audit paths ==="
rg -n 'SYMBOL_ROOTS|perry-runtime|perry-stdlib|--audit-alloc-re|audit_alloc_re|crates/perry' scripts .github/workflows/gc-root-dominance.yml
echo
echo "=== checkout step and script step surrounding the audit"
sed -n '80,115p' .github/workflows/gc-root-dominance.yml
echo
echo "=== candidate script outline and relevant sections ==="
wc -l scripts/gc_root_dominance_check.py
ast-grep outline scripts/gc_root_dominance_check.py --view compact || true
sed -n '1,220p' scripts/gc_root_dominance_check.pyRepository: PerryTS/perry
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== workflow action names and required context hints ==="
sed -n '88,110p' .github/workflows/gc-root-dominance.yml
rg -n "gc-root-dominance|required contexts|branch protection" .github README.md CLAUDE.md docs/src/internals/gc-rooting-invariant.md 2>/dev/null || true
echo
echo "=== SYMBOL_ROOTS and audit implementation ==="
rg -n 'SYMBOL_ROOTS|audit_alloc_re|js_\w+' scripts/gc_root_dominance_check.py
sed -n '1,260p' scripts/gc_root_dominance_check.py
echo
echo "=== exact extern C js_* declarations in representative extension crate libs ==="
python3 - <<'PY'
import re, pathlib, collections
pat = re.compile(r'(?m)^(?P<vis>pub\s*)?extern\s+"C"\s+(?:#\[no_mangle\]\s*)?fn\s+(?P<name>js_[\w\d_-]+)\b')
roots = []
for crate in sorted(pathlib.Path('crates').glob('*')):
if not (crate / 'src' / 'lib.rs').is_file():
continue
p = crate / 'src' / 'lib.rs'
names = [m.group('name') for m in pat.finditer(p.read_text(errors='replace'))]
if names:
roots.append((crate, names))
for crate, names in roots[40:90]:
print(len(names), str(crate), names)
total = sum(len(names) for _, names in roots)
print(f"total_ext_lib_exports={total}")
PY
echo
echo "=== all crates with src/lib.rs and any js_* export declaration ==="
python3 - <<'PY'
import re, pathlib
pat = re.compile(r'(?m)^(?P<vis>pub\s*)?extern\s+"C"\s+(?:#\[no_mangle\]\s*)?fn\s+(?P<name>js_[\w\d_-]+)\b')
for crate in sorted(pathlib.Path('crates').glob('*')):
p = crate / 'src' / 'lib.rs'
if not p.exists():
continue
names = [m.group('name') for m in pat.finditer(p.read_text(errors='replace'))]
if names:
print(f"{crate}: {len(names)}")
PYRepository: PerryTS/perry
Length of output: 27225
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== exact symbol count from runtime_symbols implementation ==="
python3 - <<'PY'
from pathlib import Path
def runtime_symbols(roots):
syms = set()
for root in roots:
for path in Path(root).rglob("*.rs"):
text = path.read_text(errors="replace")
for line in text.splitlines():
if 'extern "C"' not in line:
continue
start = line.find('js_')
if start < 0:
continue
rest = line[start:]
for i in range(len(rest)):
if not rest[i].isalnum() and rest[i] not in '_'-:'14345d':
sym = rest[:i]
if sym.split('(')[0].split('<')[0].split('::')[-1] in ("js_", "__rust_abi"):
continue
syms.add(sym)
return syms
roots = ("crates/perry-runtime/src", "crates/perry-stdlib/src")
syms = runtime_symbols(roots)
print("total:", len(syms))
print("unique_samples:", sorted(syms)[:20])
pat_ext = None
pat_ext = re.compile(r'(?m)^(?P<vis>pub\s*)?extern\s+"C"\s+(?:#\[no_mangle\]\s*)?fn\s+(?P<name>js_[\w\d_-]+)\b')
ext_syms=set()
for crate in sorted(pathlib.Path('crates').glob('*')):
p = crate / 'src' / 'lib.rs'
if not p.exists():
continue
names = [m.group('name') for m in pat_ext.finditer(p.read_text(errors='replace'))]
if names:
print('EXPORTED_LIB:', str(crate), len(names))
# Final exact summary.
root_syms = sorted(read_exact_exported_js(
'SYMBOL_ROOTS = ("crates/perry-runtime/src", "crates/perry-stdlib/src")
for root in roots:
for path in Path(root).rglob("*.rs"):
text = path.read_text(errors="replace")
for line in text.splitlines():
m=pat_exact.match(line)
if m:
syms.add(m.group(2))Repository: scripts/gc_root_dominance_check.py
Length of output: 2037
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== audit_alloc_re implementation ==="
sed -n '409,515p' scripts/gc_root_dominance_check.py
echo
echo "=== runtime_symbols implementation ==="
sed -n '412,438p' scripts/gc_root_dominance_check.py
echo
echo "=== exact `extern "C" fn js_*` declarations only in crates/lib.rs ==="
rg -n --glob '*.rs' '^pub?\s*extern\s+"C"\s+(?:#\[no_mangle\]\s*)?fn\s+(js_[\w\d_-]+)\b' crates \
| cut -d: -f1,5 \
| sed -E 's#^([^:]+):\s*(.*)#\1 \2#' \
| sort \
| uniq
echo
echo "=== count total exact exported declarations ==="
rg -o --glob '*.rs' '^pub?\s*extern\s+"C"\s+(?:#\[no_mangle\]\s*)?fn\s+(js_[\w\d_-]+)\b' crates \
| sort -u \
| wc -l
echo
echo "=== count exact exported declarations under audit roots ==="
rg -o --glob '*.rs' '^pub?\s*extern\s+"C"\s+(?:#\[no_mangle\]\s*)?fn\s+(js_[\w\d_-]+)\b' \
crates/perry-runtime/src crates/perry-stdlib/src \
| sort -u \
| wc -lRepository: PerryTS/perry
Length of output: 46897
Expand the symbol scan or document why extension exports are excluded.
The audit only scans crates/perry-runtime/src and crates/perry-stdlib/src, but exported extern "C" js_* symbols also exist in extension crates under crates/perry-ext-*. Add extension symbol roots to SYMBOL_ROOTS in scripts/gc_root_dominance_check.py, or make this expected so the audit does not mask new exported allocations. Add gc-root-dominance to branch protection’s required contexts.
🤖 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 @.github/workflows/gc-root-dominance.yml around lines 91 - 106, Update
SYMBOL_ROOTS in gc_root_dominance_check.py to include the perry-ext-* crates so
the ALLOC_RE audit scans all exported extern "C" js_* runtime symbols, or
explicitly document and enforce the intended extension exclusion without masking
new allocations. Also add gc-root-dominance to the branch protection required
status contexts.
Source: Coding guidelines
added 2 commits
August 2, 2026 06:18
# Conflicts: # scripts/gc_root_dominance_check.py
…t 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 PerryTS#7217 with the measurements rather than left to turn gc-stress red. Second file with this exact signature -- PerryTS#7216's assign_string_source witness is triaged to PerryTS#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.
Contributor
|
Re-merged against The witness was registered nowhere, and registering it found a residual
It is red on all ten allocation-point arms, deterministically: So the split is the route, not any of the arms' other knobs: safepoint relocation is fine, relocation forced inside the allocating helper is not. This PR claims a fix for the first and not the second, so I triaged the ten cells to #7217 with the measurements rather than let them turn This is the second file with exactly this signature. #7216's Worth deciding whether #7217 should own both or whether the route deserves its own issue. Noted in the triage file so whoever closes #7217 checks these two. One thing I did not do
|
# Conflicts: # test-parity/gc_repsel_corpus.txt # test-parity/gc_repsel_triage.txt
Contributor
|
Re-merged against Two conflicts, both append-at-end collisions in Re-verified the two things a re-merge can break, plus the gates that now live on
The witness measurements are carried forward unchanged: the merge touched no Rust, so |
This was referenced Aug 2, 2026
Merged
proggeramlug
added a commit
that referenced
this pull request
Aug 2, 2026
…ility (#7210) (#7235) * fix(gc): split the dominance checker's heap-source predicate by movability `_is_heap_source` answered "does the collector REWRITE this location?" when the reportable question is "can the OBJECT this register names go bad while it sits in unrooted memory across a collection?". #7210 measured the consequence: every one of the 66 `--unrooted-allocas --moving-only` hits was a false positive, so the population could never reach 0 and #7198's promote-to-required clock could not start. The split models the two independent ways a held address goes bad -- the object MOVES, or the object is RECLAIMED -- and exempts a source only when both are false. Two exemptions, each with its premises machine-checked by a new `--audit-immovable-sources` and each reversible by its own flag so #7210's counterfactuals stay one command away instead of becoming tribal knowledge: class-keys `@perry_class_keys_*` / `js_build_class_keys_array`. Not movable: allocated through `js_array_alloc_with_length_longlived` (old arena), and the only thing that relocates an old-arena object is old-page defrag, which `select_old_page_defrag_pages` short-circuits off (#6206). Not reclaimable: the global is registered with `js_gc_register_global_root`. Re-report with `--assume-old-defrag`. box `js_box_alloc*`. Not movable: `std::alloc::alloc`, outside the GC heap -- `scan_box_roots_mut` rewrites the JSValue INSIDE the box, never the box's address. Not reclaimable: no dealloc, and `BOX_REGISTRY` is monotonic. Re-report with `--assume-boxes-in-gc-heap`. The exemptions are one-sided in the UNSAFE direction: an unlisted source keeps the old conservative classification, so forgetting one costs a false positive rather than a missed bug. Because an exemption is a suppression, it gets the strictest treatment in the file. `--self-test` now asserts, for each: it fires; its knob exactly reverses it; and -- the arm that matters -- a structurally IDENTICAL fixture whose only difference is a NURSERY allocator is still reported, under `--moving-only` and under both knobs. If a future widening ever swallows that, the exemption has stopped being about the allocator and become a rule about code shape. `--unrooted-allocas` also prints what each exemption suppressed, so "the corpus is clean" and "the corpus is entirely exempted" no longer print the same line. Measured over a freshly generated 134-file corpus (116 sources) at c9cd73b, `--unrooted-allocas --moving-only`: 98 before, 2 after, 96 suppressed (93 class-keys + 3 box). The count is above #7210's 66 because both the corpus and `ALLOC_RE` grew since -- #7227 added the `*_new*` convention, which is what makes the 2 residuals visible at all. Refs #7210, #7198, #7202, #7154. * docs(changelog): add fragment for #7235 * ci(gc): gate the immovable-source exemption premises * fix(gc): don't let an exemption tally end the per-alloca search Review of my own change: `check_func_unrooted_allocas` counted an exemption and then set `reported`, which ends the store loop for that alloca. One slot written from an exempt source AND from a nursery allocator would therefore have its real store never examined -- the accounting turning into a missed hazard, which is exactly the failure mode the exemption machinery exists to avoid. The tally is now collected per alloca and applied after the loop, so it never competes with a report. `--self-test` gains the arm that catches it: one slot, `@perry_class_keys_*` store first, `js_array_alloc_with_length` store second, both across a moving poll. Sabotage-checked in both directions -- reinstating the old control flow makes that arm, and only that arm, fail (exit 1); restoring it clears (exit 0). Corpus numbers unchanged (2 violations, 93 class-keys + 3 box suppressed), and the partition is now exact: `--assume-old-defrag` alone reports 95, `--assume-boxes-in-gc-heap` alone 5, both together **98** -- byte-identical to what origin/main's predicate reports over the same corpus. --------- 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>
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.
Stacks on #7226 (branch
fix/7211-registry-gc-stale-registers), which is still open — this branch is based on its head (4e99c1bad), so review it after that one.#7226 took
sfw-registry --helpfrom 0/30 to 26/30 under a genuine polls build and named its own residual rather than closing #7154:perry_fn_src_lib_api_shared_ts__defineApiCall + 404, the/re/.test(s)lowering, plus the reason the static checker could not see it. This PR does that item, and does the checker audit that the item asks for.The blind spot is the more valuable half, so it goes first
ALLOC_REcarried an alternative spelledregexp_alloc\w*. No such symbol has ever existed.It was not a typo. It was an extrapolation: whoever wrote it knew a RegExp allocates and inferred the
_allocsuffix from its neighbours. That is worth naming precisely, because it is the second allocator to escape this pattern (js_implicit_this_setwas the first, closed by #7226) and each miss has cost a full investigation round.So rather than adding
js_regexp_newand moving on, every alternative was reconciled against the runtime's real symbol table —extern "C" fn js_\w+over perry-runtime + perry-stdlib, intersected with the names perry-codegen actually declares. Four alternatives matched nothing whatsoever:regexp_alloc\w*,promise_alloc\w*,bigint_alloc\w*,typed_array_alloc\w*. A quarter of the pattern was decorative.The root cause is structural. The runtime materializes fresh GC objects under three naming conventions and the pattern modelled one:
*_alloc*js_object_alloc,js_array_alloc,js_closure_alloc,js_uint8array_alloc,js_inline_arena_slow_alloc*_new*js_regexp_new,js_promise_new,js_symbol_new,js_date_new,js_error_new,js_typed_array_new,js_weakmap_new,js_url_new,js_boxed_string_new, ~140 more*_create*js_object_create,js_array_create,js_vm_create_context,js_crypto_create_hash, ~40 moreobject_create*All three are now matched as conventions, so the next
js_foo_newis covered on the day it is written. The constructors that use none of the three are enumerated explicitly: the_construct*ctor forms; the fresh-string producers (string_coerce,jsvalue_to_string*,string_slice,string_to_*_case,string_pad_*,string_trim*, …); the copy-on-read and ES2023 change-by-copy array family (array_to_sorted*,array_to_spliced,array_with,array_flat*,array_like_to_array,iterator_to_array, …); the whole-object producers (object_keys*,object_entries*,object_from_entries,object_get_own_property_descriptor*,structured_clone*, the Set-methods family); the namespace/class-shape helpers; and BigInt'sbigint_from*, which has neither_allocnor_new.The second blind spot, and it is the one that matters for CI
Widening
ALLOC_REalone was not enough, and finding that out is the useful part. With it widened, the raw--stale-registerscount reported the site — and--moving-onlystill did not, because nothing in the window was classified as reaching a moving minor.--moving-onlyis the modegc-root-dominance.ymlgates on, so the gate would have stayed blind.js_jsvalue_to_string_coercerunsto_string_method_impl(…, skip_to_primitive = false), which consults[Symbol.toPrimitive], thentoString, thenvalueOf— arbitrary JS, with its own back-edge polls. A coercion does not look like a call into user code.js_string_coerce's own doc comment already said it was one ("aPOINTER_TAGobject routes throughjs_jsvalue_to_string, which can invoke a usertoString/valueOf"); the checker just never read it. The whole ToPrimitive / ToString / ToNumber family is nowPOLL_CAPABLE_RUNTIME.js_regexp_test/js_regexp_execare also added toRECEIVER_SINKS: a staleRegExpHeader*is dereferenced immediately by both, and this one faulted rather than merely answering wrong.The fix
Expr::RegExpTestandExpr::RegExpExeclowered the receiver first, unboxed it to a rawRegExpHeader*, and only then emitted the coerce:Both now take the established
guard_store_operand_across/reread_store_operandpair, and the unbox moves below the coerce — unboxing above it is what parked the pre-move address in a register in the first place. The window is unconditional (the coerce is always emitted), socollectsistruerather than a test on the string operand.Verification
Measured against the parent
4e99c1bad, built from this worktree rather than borrowed from another one.The checker change is what makes the codegen change checkable, so it is reported first — over the new reproducer's IR:
--stale-registersovertest_gap_gc_regexp_receiver_rooting.tsregexp_alloc\w*)ALLOC_RE, raw count--moving-only(the gate's mode)Named exactly:
source (alloc): call i64 @js_regexp_new→stale use: call i32 @js_regexp_test/@js_regexp_exec,MOVING: YES via js_jsvalue_to_string_coerce.Over the 131-module / 2170-function gap corpus, both checker widenings together:
--moving-only--stale-registers, total--stale-registers --moving-only--stale-registers --fatal-sinks--stale-registers --moving-only --fatal-sinks--unrooted-allocas, moving-reachableThe exact CI invocation (
--moving-only --min-files 90 --min-binds 1500 --min-funcs 1200 --allowlist … --seeded-violations 40) exits 0, 40 planted / 40 caught / 0 missed, with the new gap test in the corpus and participating in the seeding.Triage of the 60 newly-moving leads
They were all already in the 2730-entry diagnostic list; modelling the coercions reclassified their windows. Classified mechanically by the shape of the stale use:
lshr …, 48fadd doublegetelementptr i8, …, 32*__pshapespecializations. Real dereference shape, real population — thePERRY_PTR_SHAPE_LOCALSfamily, left for its own PR with its own measured count.js_number_coerce. Same call.None is a fatal sink, which is why the fatal count moves only by the three regexp entries this PR then fixes.
Gap test
test_gap_gc_regexp_receiver_rooting.ts, compiled and run withPERRY_GC_MOVING_LOOP_POLLS=1:POLLS=1+PERRY_GC_ZEAL=1bad 010/10PERRY_GEN_GC=0bad 0bad 0The parent arm is a hard fault rather than a nonzero
bad, and that is the honest signature: with a regex literal receiver — the registry's shape —js_regexp_new's result is held only in the register, so the evacuating minor retires the block under it and the deref lands in from-space. ThePERRY_GEN_GC=0arm proves the test tracks collector mode rather than being flaky. Zeal is needed for the structural reason #7226 recorded forprev_this: the window is a user call, so only a moving collection exploits it, and allocation-triggered collections takeManualGcScanGuard::force_full_scan, which makes the copying minor ineligible.cargo test -p perry-codegen: 6 failures, all*_keeps_the_back_edge_poll— the identicalloop_safepoint_purityset #7226 measured on this same parent, which is #7161's default flip. No new failures.sfw-registry --help, 141 modules,PERRY_FORCE_WELL_KNOWN=iovalkey, compiled and run withPERRY_GC_MOVING_LOOP_POLLS=1:4e99c1bad)POLLS=1, 30 runsBoth arms link the same runtime archives — the codegen fix is the only difference — and run against the same firewall checkout, so the comparison is like-for-like.
The parent's 30 failures are not crashes. Every one is a deterministic
TypeError: Cannot convert undefined or null to object, which is exactly what a stale regexp receiver produces at this site:defineApiCallcomputesurlRequiresInterpolation = /\[[a-zA-Z]+\]/.test(url)at definition time, the stale read returns the wrong boolean, the wrong branch runs, andundefinedreaches a downstreamObjectoperation. This PR removes that failure mode completely — the TypeError does not occur once in 30 runs afterwards.Two runs in thirty still SIGSEGV, so there is no #7161 revert here. The 30-run number is the one that counts; a 10-run sample on a ~7 % failure is worthless, which is the lesson #7226 already paid for.
The residual is a different object from the one this PR roots, and it now has a deterministic reproducer. Under
PERRY_GC_MOVING_LOOP_POLLS=1 PERRY_GC_PROTECT_FROMSPACE=1 PERRY_GC_PROTECT_FROMSPACE_DEPTH=800it fails 40/40:obj_type=3is a string, not aRegExpHeader, so it is not the receiver this PR rooted. The disassembly confirms it: the faulting frame's return address isdefineApiCall + 428, and+424isbl js_regexp_test— the same call site, twenty bytes further along, which is exactly the size of thejs_gc_temp_root_push/js_gc_temp_root_getpair this PR inserts. The receiver operand is now correct; a string reaching that same call is not.Notably the static checker reports nothing of that shape in
defineApiCall— its remaining hits there are alljs_closure_set_capture_bitsacrossjs_closure_alloc,MOVING: no. A runtime-visible fault that the IR checker cannot see is the #7211 signature (a runtime table, not a codegen register), and that is where the next round should look — with the protector, which now makes it 40/40 instead of 2/30.Nothing is fixed speculatively: no GC/codegen edit ships here without a test that fails without it.
Refs #7154, #7226, #7211, #7213, #7161
Summary by CodeRabbit
Bug Fixes
typeofand JSON values.Tests
typeofresults.Documentation