fix(gc): root rest-args and same-module call arguments; teach the checker about string-literal handles (#7154) - #7270
Closed
jdalton wants to merge 3 commits into
Conversation
PerryTS#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. PerryTS#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 PerryTS#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` (PerryTS#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 PerryTS#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 PerryTS#7154.
…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.
📝 WalkthroughWalkthroughThe change adds shared GC-rooted call-argument lowering for rest and same-module calls. It defers temporary-root cleanup until after dispatch and implicit- ChangesGC-rooted call lowering
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant FuncRef
participant RootedLowering
participant CallDispatch
participant ImplicitThis
FuncRef->>RootedLowering: lower and root call arguments
RootedLowering-->>FuncRef: arguments and cleanup guard
FuncRef->>CallDispatch: dispatch call
CallDispatch-->>FuncRef: return
FuncRef->>ImplicitThis: restore implicit this
FuncRef->>RootedLowering: release argument roots
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perry-codegen/src/lower_call/func_ref.rs`:
- Around line 402-422: Update the second RestBundle in the combined
rest-and-synthetic-arguments branch of the lower call flow to set
mark_arguments_object: true, while keeping it false for the first real-rest
bundle, so the synthetic arguments array follows the same runtime contract as
the simpler synthetic arguments path.
In `@crates/perry-codegen/src/lower_call/mod.rs`:
- Around line 288-293: Ensure both re-read sites in
crates/perry-codegen/src/lower_call/mod.rs#L288-L293 and
crates/perry-codegen/src/lower_call/mod.rs#L300-L320 rely on a documented
no-collection Reload path. In operand_is_reloadable, admit only side-effect-free
expressions so reread_one may safely re-lower each operand multiple times; also
confirm Reload cannot allocate, or move bundle boxing below the fixed-parameter
loop so no bare boxed pointer remains live across a re-read. Document this
invariant in the relevant function’s doc comment.
🪄 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: ad0072e1-b307-4964-a809-c8f775f06745
📒 Files selected for processing (9)
changelog.d/7241-rest-and-same-module-call-argument-rooting.mdcrates/perry-codegen/src/expr/temp_root.rscrates/perry-codegen/src/lower_call/extern_func.rscrates/perry-codegen/src/lower_call/func_ref.rscrates/perry-codegen/src/lower_call/mod.rsscripts/gc_root_dominance_check.pytest-files/fixtures/gc_call_arg_rooting_pkg/rest_callee.tstest-files/test_gap_gc_rest_argument_rooting.tstest-files/test_gap_gc_same_module_call_argument_rooting.ts
Comment on lines
402
to
+422
| if ctx.func_synthetic_arguments.contains(fid) && has_rest && !synthetic_is_rest { | ||
| let lowered_args: Vec<String> = args | ||
| .iter() | ||
| .map(|arg| lower_expr(ctx, arg)) | ||
| .collect::<Result<_>>()?; | ||
| // #1816: a real `...rest` AND a synthetic `arguments`, over the same | ||
| // argument list at two different offsets. | ||
| let fixed_count = declared_count.saturating_sub(2); | ||
| let undef_lit = double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED)); | ||
| for idx in 0..fixed_count { | ||
| if let Some(arg) = lowered_args.get(idx) { | ||
| lowered.push(arg.clone()); | ||
| } else { | ||
| lowered.push(undef_lit.clone()); | ||
| } | ||
| } | ||
|
|
||
| let rest_count = args.len().saturating_sub(fixed_count); | ||
| let cap = (rest_count as u32).to_string(); | ||
| let mut current = ctx.block().call(I64, "js_array_alloc", &[(I32, &cap)]); | ||
| for v in lowered_args.iter().skip(fixed_count) { | ||
| let blk = ctx.block(); | ||
| current = blk.call( | ||
| I64, | ||
| "js_array_push_f64", | ||
| &[(I64, ¤t), (DOUBLE, v.as_str())], | ||
| ); | ||
| } | ||
| let rest_box = nanbox_pointer_inline(ctx.block(), ¤t); | ||
| lowered.push(rest_box); | ||
|
|
||
| let cap = (args.len() as u32).to_string(); | ||
| let mut current = ctx.block().call(I64, "js_array_alloc", &[(I32, &cap)]); | ||
| for v in &lowered_args { | ||
| let blk = ctx.block(); | ||
| current = blk.call( | ||
| I64, | ||
| "js_array_push_f64", | ||
| &[(I64, ¤t), (DOUBLE, v.as_str())], | ||
| ); | ||
| } | ||
| let arguments_box = nanbox_pointer_inline(ctx.block(), ¤t); | ||
| lowered.push(arguments_box); | ||
| let (values, guard) = super::lower_rest_call_args_rooted( | ||
| ctx, | ||
| args, | ||
| fixed_count, | ||
| &[ | ||
| super::RestBundle { | ||
| from: fixed_count, | ||
| mark_arguments_object: false, | ||
| }, | ||
| super::RestBundle { | ||
| from: 0, | ||
| mark_arguments_object: false, | ||
| }, | ||
| ], | ||
| )?; | ||
| arg_guard = guard; | ||
| lowered.extend(values); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Compare the pre-change arm with the new one and inspect what the flag controls.
git log --oneline -3 -- crates/perry-codegen/src/lower_call/func_ref.rs
git diff HEAD~1 -- crates/perry-codegen/src/lower_call/func_ref.rs | sed -n '1,200p'
rg -nP -C 6 'js_array_mark_arguments_object' crates/perry-codegen/src crates/perry-runtime/src
rg -nP -C 10 '\bfn js_array_mark_arguments_object\b' crates/perry-runtime/src
rg -nP -C 6 'is_arguments_object|ARGUMENTS_OBJECT|arguments_object' crates/perry-runtime/srcRepository: PerryTS/perry
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== current func_ref region =="
sed -n '370,445p' crates/perry-codegen/src/lower_call/func_ref.rs
echo
echo "== lower_call rest bundle implementation =="
sed -n '200,312p' crates/perry-codegen/src/lower_call/mod.rs
echo
echo "== git diff for just the relevant region across all parents =="
git diff HEAD~1..HEAD -- crates/perry-codegen/src/lower_call/func_ref.rs | sed -n '1,260p'
echo
echo "== build-rest-array call sites =="
rg -nP -C 8 'build_rest_array|arguments_object: (true|false)|RestBundle|mark_arguments_object' crates/perry-codegen/src/lower_call crates/perry-runtime/src/closure/registry.rs crates/perry-runtime/src/object/arguments.rs | sed -n '1,260p'Repository: PerryTS/perry
Length of output: 28423
Mark the synthetic arguments array in the combined rest+arguments arm.
The second bundle bundles from index 0, so it is the synthetic arguments array. Set mark_arguments_object: true for that bundle, and the rest bundle to false, so this path emits the same runtime argument-object contract as the simpler synthetic arguments arm.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-codegen/src/lower_call/func_ref.rs` around lines 402 - 422,
Update the second RestBundle in the combined rest-and-synthetic-arguments branch
of the lower call flow to set mark_arguments_object: true, while keeping it
false for the first real-rest bundle, so the synthetic arguments array follows
the same runtime contract as the simpler synthetic arguments path.
Comment on lines
+288
to
+293
| for i in bundle.from..refs.len() { | ||
| // Re-read per element: the previous push allocated, so the register | ||
| // this argument was lowered into is already stale. | ||
| let value = rooted.reread_one(ctx, &refs, i)?; | ||
| temp_root::temp_rooted_array_push(ctx, &acc, &value); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Both re-read sites depend on the Reload path emitting no collecting call. reread_one re-lowers an operand classified Reload by calling lower_expr. This function calls it in two places, and each place assumes that re-lowering neither duplicates a side effect nor reaches a collection point. Establish that property once, then record it in the function doc.
crates/perry-codegen/src/lower_call/mod.rs#L288-L293: confirmoperand_is_reloadableadmits only side-effect-free forms, because the#1816shape re-reads each operand at least twice.crates/perry-codegen/src/lower_call/mod.rs#L300-L320: either confirm theReloadpath cannot allocate, or move the bundle boxing below the fixed-parameter loop so no bare boxed pointer is live across a re-read.
📍 Affects 1 file
crates/perry-codegen/src/lower_call/mod.rs#L288-L293(this comment)crates/perry-codegen/src/lower_call/mod.rs#L300-L320
🤖 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/mod.rs` around lines 288 - 293, Ensure
both re-read sites in crates/perry-codegen/src/lower_call/mod.rs#L288-L293 and
crates/perry-codegen/src/lower_call/mod.rs#L300-L320 rely on a documented
no-collection Reload path. In operand_is_reloadable, admit only side-effect-free
expressions so reread_one may safely re-lower each operand multiple times; also
confirm Reload cannot allocate, or move bundle boxing below the fixed-parameter
loop so no bare boxed pointer remains live across a re-read. Document this
invariant in the relevant function’s doc comment.
proggeramlug
pushed a commit
that referenced
this pull request
Aug 2, 2026
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>
Contributor
|
Landed as #7271 — your commits, restacked onto The accumulator finding is the good one: a raw Three fixes at merge time:
One thing I did not do: build and run the two witnesses locally. CI will be their first real execution. If they go red, that is them working on first contact. Thanks — this and #7240 together closed a class the static checker previously couldn't see. |
Contributor
Author
Re-verification on the rebased treeThe evidence in the description was measured against Gap tests, rebuilt on the rebased tree, compiled and run with
Checker, over the rebased corpus (now 121 sources / 141 modules — five more than before, from the other merges):
The load-bearing line is the middle one: on a tree where the codegen defect is fixed, the widening adds zero stale uses that reach a moving minor. That is the same result as on the pre-rebase corpus and it is the answer to "is the modelling too broad". The single The gate command from Still outstandingThe |
jdalton
added a commit
to jdalton/perry
that referenced
this pull request
Aug 2, 2026
Four PRs in a row shipped a test file that ran nowhere. PerryTS#7192 and PerryTS#7216 each added a `test_gap_gc_*` stale-root witness and no corpus line; PerryTS#7252 added a third; PerryTS#7270/PerryTS#7271 added two more, caught by the maintainer at merge. Four occurrences of one mistake is a missing gate, not carelessness. An unregistered file is not a failing test, it is no test at all. The PR is green, the reviewer sees a witness in the diff next to a passing run and reads the two together as "covered", and nothing says otherwise because nothing ran. This is CLAUDE.md's fourth hazard in its purest form: the gate runs, its subject never does. Registration checks already existed for two of the three prefixes (`gc_repsel_matrix.sh` auto-detects `test_gap_repsel_*`/`test_gap_specabi_*`, `gc-moving-witnesses.yml` adds `test_gap_gc_*`) and neither could catch the pull request that needed it: both sit behind a 90-minute release build of the compiler, behind a changed-paths relevance filter, and in workflows that are not in branch protection's required contexts. `scripts/check_test_registration.py` is the cheap half of those checks, pulled out to where it can block, and generalised past that one corpus. Pure filesystem and text — no compiler, no Node, ~0.2s — over four mechanisms: gc-repsel-corpus test-files/test_gap_{gc,repsel,specabi}_*.ts -> test-parity/gc_repsel_corpus.txt feature-matrix-probes test-features/probes/**/*.ts -> test-features/feature_matrix.toml compiler-output-workloads benchmarks/compiler_output/fixtures/**/*.ts -> benchmarks/compiler_output/workloads.toml rust-test-modules crates/*/**/tests/**/*.rs below a suite root -> the `mod` declaration in the parent module The last is the Rust analogue and is worth naming: cargo auto-discovers `crates/<c>/tests/<suite>.rs`, but a file one level deeper only compiles if a `mod` names it. Without one rustc never parses it — not dead code, not code, no warning. Built to be able to fail, against all four hazards: 1. no `continue-on-error`, no `|| true`; the step's exit status is the gate. 2. it is a step in `lint`, which is ALREADY a required context. That placement is the point: forgetting to add a new job to branch protection is hazard 2, and it is what left `gc-root-dominance` red and blocking nothing for days. No admin action is needed here because the step that gets forgotten does not exist. 3. `lint`'s concurrency already cancels pull-request runs only. 4. the subject is asserted live. Each mechanism floors its candidate set and FAILS if the glob stops matching, so "0 dark over 0 candidates" cannot print the same verdict as "0 dark over 157". Every run states `checked N files against M registries`. Exclusions are named with reasons rather than counted, because a threshold cannot tell a new dark file from an old one — fix one, add one, tally unchanged. A stale exclusion, one matching no file on disk, is itself a failure, so an excuse cannot outlive the file it excuses. Same for the mirror image: a registry entry whose file is gone fails as a rotted entry. `--self-test` (32 cases, also run in `lint`) plants an unregistered file into each of the four mechanisms over the REAL registries via an in-memory overlay, asserts the gate names it, then removes it and asserts green. It also pins the one false positive found while writing this: `resolve/tests/ declaration_sidecar_tests/compile_package.rs` IS declared, by a `mod` inside an inline `mod … { }` block two levels up, and the first draft condemned it. A gate that cries wolf gets deleted. Verified end to end on disk, not just through the overlay: planted a real unregistered file in each of the four mechanisms, watched the gate go red and name it; registered one and watched it go green; deleted the file leaving the line and watched the rotted-entry arm go red; restored and watched it go green. Today's dark set is empty for all four, so this is green on `main` from the first run and safe in a required context. Three feature probes and two compiler-output fixtures are excluded, each with its reason: four are helper modules imported by a registered test, and `benchmarks/compiler_output/fixtures/raw_numeric_layout_smoke.ts` is registered in a different registry (the `raw_numeric_layouts` target-collector workload in `scripts/run_memory_stability_tests.sh`). Deliberately out of scope: `tests/*.sh|py|ts`, where 143 of 171 files are referenced by nothing in the tree. There is no registry there to diff against, so "unregistered" is not even well defined; that is an archaeology problem (triage each, wire it up or delete it) and inventing a registry for it retroactively would make this gate red on day one for reasons unrelated to the four dark witnesses. `--list` says so out loud rather than leaving the silence. Docs where an author will actually meet the rule: a new `docs/src/testing/test-registration.md`, a bullet in CONTRIBUTING.md's "what goes in a PR", and a rewritten header on each of the three registry files. Refs PerryTS#7192, PerryTS#7216, PerryTS#7252, PerryTS#7270, PerryTS#7271.
jdalton
added a commit
to jdalton/perry
that referenced
this pull request
Aug 2, 2026
Four PRs in a row shipped a test file that ran nowhere. PerryTS#7192 and PerryTS#7216 each added a `test_gap_gc_*` stale-root witness and no corpus line; PerryTS#7252 added a third; PerryTS#7270/PerryTS#7271 added two more, caught by the maintainer at merge. Four occurrences of one mistake is a missing gate, not carelessness. An unregistered file is not a failing test, it is no test at all. The PR is green, the reviewer sees a witness in the diff next to a passing run and reads the two together as "covered", and nothing says otherwise because nothing ran. This is CLAUDE.md's fourth hazard in its purest form: the gate runs, its subject never does. Registration checks already existed for two of the three prefixes (`gc_repsel_matrix.sh` auto-detects `test_gap_repsel_*`/`test_gap_specabi_*`, `gc-moving-witnesses.yml` adds `test_gap_gc_*`) and neither could catch the pull request that needed it: both sit behind a 90-minute release build of the compiler, behind a changed-paths relevance filter, and in workflows that are not in branch protection's required contexts. `scripts/check_test_registration.py` is the cheap half of those checks, pulled out to where it can block, and generalised past that one corpus. Pure filesystem and text — no compiler, no Node, ~0.2s — over four mechanisms: gc-repsel-corpus test-files/test_gap_{gc,repsel,specabi}_*.ts -> test-parity/gc_repsel_corpus.txt feature-matrix-probes test-features/probes/**/*.ts -> test-features/feature_matrix.toml compiler-output-workloads benchmarks/compiler_output/fixtures/**/*.ts -> benchmarks/compiler_output/workloads.toml rust-test-modules crates/*/**/tests/**/*.rs below a suite root -> the `mod` declaration in the parent module The last is the Rust analogue and is worth naming: cargo auto-discovers `crates/<c>/tests/<suite>.rs`, but a file one level deeper only compiles if a `mod` names it. Without one rustc never parses it — not dead code, not code, no warning. Built to be able to fail, against all four hazards: 1. no `continue-on-error`, no `|| true`; the step's exit status is the gate. 2. it is a step in `lint`, which is ALREADY a required context. That placement is the point: forgetting to add a new job to branch protection is hazard 2, and it is what left `gc-root-dominance` red and blocking nothing for days. No admin action is needed here because the step that gets forgotten does not exist. 3. `lint`'s concurrency already cancels pull-request runs only. 4. the subject is asserted live. Each mechanism floors its candidate set and FAILS if the glob stops matching, so "0 dark over 0 candidates" cannot print the same verdict as "0 dark over 157". Every run states `checked N files against M registries`. Exclusions are named with reasons rather than counted, because a threshold cannot tell a new dark file from an old one — fix one, add one, tally unchanged. A stale exclusion, one matching no file on disk, is itself a failure, so an excuse cannot outlive the file it excuses. Same for the mirror image: a registry entry whose file is gone fails as a rotted entry. `--self-test` (32 cases, also run in `lint`) plants an unregistered file into each of the four mechanisms over the REAL registries via an in-memory overlay, asserts the gate names it, then removes it and asserts green. It also pins the one false positive found while writing this: `resolve/tests/ declaration_sidecar_tests/compile_package.rs` IS declared, by a `mod` inside an inline `mod … { }` block two levels up, and the first draft condemned it. A gate that cries wolf gets deleted. Verified end to end on disk, not just through the overlay: planted a real unregistered file in each of the four mechanisms, watched the gate go red and name it; registered one and watched it go green; deleted the file leaving the line and watched the rotted-entry arm go red; restored and watched it go green. Today's dark set is empty for all four, so this is green on `main` from the first run and safe in a required context. Three feature probes and two compiler-output fixtures are excluded, each with its reason: four are helper modules imported by a registered test, and `benchmarks/compiler_output/fixtures/raw_numeric_layout_smoke.ts` is registered in a different registry (the `raw_numeric_layouts` target-collector workload in `scripts/run_memory_stability_tests.sh`). Deliberately out of scope: `tests/*.sh|py|ts`, where 143 of 171 files are referenced by nothing in the tree. There is no registry there to diff against, so "unregistered" is not even well defined; that is an archaeology problem (triage each, wire it up or delete it) and inventing a registry for it retroactively would make this gate red on day one for reasons unrelated to the four dark witnesses. `--list` says so out loud rather than leaving the silence. Docs where an author will actually meet the rule: a new `docs/src/testing/test-registration.md`, a bullet in CONTRIBUTING.md's "what goes in a PR", and a rewritten header on each of the three registry files. Refs PerryTS#7192, PerryTS#7216, PerryTS#7252, PerryTS#7270, PerryTS#7271.
jdalton
added a commit
to jdalton/perry
that referenced
this pull request
Aug 2, 2026
Four PRs in a row shipped a test file that ran nowhere. PerryTS#7192 and PerryTS#7216 each added a `test_gap_gc_*` stale-root witness and no corpus line; PerryTS#7252 added a third; PerryTS#7270/PerryTS#7271 added two more, caught by the maintainer at merge. Four occurrences of one mistake is a missing gate, not carelessness. An unregistered file is not a failing test, it is no test at all. The PR is green, the reviewer sees a witness in the diff next to a passing run and reads the two together as "covered", and nothing says otherwise because nothing ran. This is CLAUDE.md's fourth hazard in its purest form: the gate runs, its subject never does. Registration checks already existed for two of the three prefixes (`gc_repsel_matrix.sh` auto-detects `test_gap_repsel_*`/`test_gap_specabi_*`, `gc-moving-witnesses.yml` adds `test_gap_gc_*`) and neither could catch the pull request that needed it: both sit behind a 90-minute release build of the compiler, behind a changed-paths relevance filter, and in workflows that are not in branch protection's required contexts. `scripts/check_test_registration.py` is the cheap half of those checks, pulled out to where it can block, and generalised past that one corpus. Pure filesystem and text — no compiler, no Node, ~0.2s — over four mechanisms: gc-repsel-corpus test-files/test_gap_{gc,repsel,specabi}_*.ts -> test-parity/gc_repsel_corpus.txt feature-matrix-probes test-features/probes/**/*.ts -> test-features/feature_matrix.toml compiler-output-workloads benchmarks/compiler_output/fixtures/**/*.ts -> benchmarks/compiler_output/workloads.toml rust-test-modules crates/*/**/tests/**/*.rs below a suite root -> the `mod` declaration in the parent module The last is the Rust analogue and is worth naming: cargo auto-discovers `crates/<c>/tests/<suite>.rs`, but a file one level deeper only compiles if a `mod` names it. Without one rustc never parses it — not dead code, not code, no warning. Built to be able to fail, against all four hazards: 1. no `continue-on-error`, no `|| true`; the step's exit status is the gate. 2. it is a step in `lint`, which is ALREADY a required context. That placement is the point: forgetting to add a new job to branch protection is hazard 2, and it is what left `gc-root-dominance` red and blocking nothing for days. No admin action is needed here because the step that gets forgotten does not exist. 3. `lint`'s concurrency already cancels pull-request runs only. 4. the subject is asserted live. Each mechanism floors its candidate set and FAILS if the glob stops matching, so "0 dark over 0 candidates" cannot print the same verdict as "0 dark over 157". Every run states `checked N files against M registries`. Exclusions are named with reasons rather than counted, because a threshold cannot tell a new dark file from an old one — fix one, add one, tally unchanged. A stale exclusion, one matching no file on disk, is itself a failure, so an excuse cannot outlive the file it excuses. Same for the mirror image: a registry entry whose file is gone fails as a rotted entry. `--self-test` (32 cases, also run in `lint`) plants an unregistered file into each of the four mechanisms over the REAL registries via an in-memory overlay, asserts the gate names it, then removes it and asserts green. It also pins the one false positive found while writing this: `resolve/tests/ declaration_sidecar_tests/compile_package.rs` IS declared, by a `mod` inside an inline `mod … { }` block two levels up, and the first draft condemned it. A gate that cries wolf gets deleted. Verified end to end on disk, not just through the overlay: planted a real unregistered file in each of the four mechanisms, watched the gate go red and name it; registered one and watched it go green; deleted the file leaving the line and watched the rotted-entry arm go red; restored and watched it go green. Today's dark set is empty for all four, so this is green on `main` from the first run and safe in a required context. Three feature probes and two compiler-output fixtures are excluded, each with its reason: four are helper modules imported by a registered test, and `benchmarks/compiler_output/fixtures/raw_numeric_layout_smoke.ts` is registered in a different registry (the `raw_numeric_layouts` target-collector workload in `scripts/run_memory_stability_tests.sh`). Deliberately out of scope: `tests/*.sh|py|ts`, where 143 of 171 files are referenced by nothing in the tree. There is no registry there to diff against, so "unregistered" is not even well defined; that is an archaeology problem (triage each, wire it up or delete it) and inventing a registry for it retroactively would make this gate red on day one for reasons unrelated to the four dark witnesses. `--list` says so out loud rather than leaving the silence. Docs where an author will actually meet the rule: a new `docs/src/testing/test-registration.md`, a bullet in CONTRIBUTING.md's "what goes in a PR", and a rewritten header on each of the three registry files. Refs PerryTS#7192, PerryTS#7216, PerryTS#7252, PerryTS#7270, PerryTS#7271.
jdalton
added a commit
to jdalton/perry
that referenced
this pull request
Aug 3, 2026
Four PRs in a row shipped a test file that ran nowhere. PerryTS#7192 and PerryTS#7216 each added a `test_gap_gc_*` stale-root witness and no corpus line; PerryTS#7252 added a third; PerryTS#7270/PerryTS#7271 added two more, caught by the maintainer at merge. Four occurrences of one mistake is a missing gate, not carelessness. An unregistered file is not a failing test, it is no test at all. The PR is green, the reviewer sees a witness in the diff next to a passing run and reads the two together as "covered", and nothing says otherwise because nothing ran. This is CLAUDE.md's fourth hazard in its purest form: the gate runs, its subject never does. Registration checks already existed for two of the three prefixes (`gc_repsel_matrix.sh` auto-detects `test_gap_repsel_*`/`test_gap_specabi_*`, `gc-moving-witnesses.yml` adds `test_gap_gc_*`) and neither could catch the pull request that needed it: both sit behind a 90-minute release build of the compiler, behind a changed-paths relevance filter, and in workflows that are not in branch protection's required contexts. `scripts/check_test_registration.py` is the cheap half of those checks, pulled out to where it can block, and generalised past that one corpus. Pure filesystem and text — no compiler, no Node, ~0.2s — over four mechanisms: gc-repsel-corpus test-files/test_gap_{gc,repsel,specabi}_*.ts -> test-parity/gc_repsel_corpus.txt feature-matrix-probes test-features/probes/**/*.ts -> test-features/feature_matrix.toml compiler-output-workloads benchmarks/compiler_output/fixtures/**/*.ts -> benchmarks/compiler_output/workloads.toml rust-test-modules crates/*/**/tests/**/*.rs below a suite root -> the `mod` declaration in the parent module The last is the Rust analogue and is worth naming: cargo auto-discovers `crates/<c>/tests/<suite>.rs`, but a file one level deeper only compiles if a `mod` names it. Without one rustc never parses it — not dead code, not code, no warning. Built to be able to fail, against all four hazards: 1. no `continue-on-error`, no `|| true`; the step's exit status is the gate. 2. it is a step in `lint`, which is ALREADY a required context. That placement is the point: forgetting to add a new job to branch protection is hazard 2, and it is what left `gc-root-dominance` red and blocking nothing for days. No admin action is needed here because the step that gets forgotten does not exist. 3. `lint`'s concurrency already cancels pull-request runs only. 4. the subject is asserted live. Each mechanism floors its candidate set and FAILS if the glob stops matching, so "0 dark over 0 candidates" cannot print the same verdict as "0 dark over 157". Every run states `checked N files against M registries`. Exclusions are named with reasons rather than counted, because a threshold cannot tell a new dark file from an old one — fix one, add one, tally unchanged. A stale exclusion, one matching no file on disk, is itself a failure, so an excuse cannot outlive the file it excuses. Same for the mirror image: a registry entry whose file is gone fails as a rotted entry. `--self-test` (32 cases, also run in `lint`) plants an unregistered file into each of the four mechanisms over the REAL registries via an in-memory overlay, asserts the gate names it, then removes it and asserts green. It also pins the one false positive found while writing this: `resolve/tests/ declaration_sidecar_tests/compile_package.rs` IS declared, by a `mod` inside an inline `mod … { }` block two levels up, and the first draft condemned it. A gate that cries wolf gets deleted. Verified end to end on disk, not just through the overlay: planted a real unregistered file in each of the four mechanisms, watched the gate go red and name it; registered one and watched it go green; deleted the file leaving the line and watched the rotted-entry arm go red; restored and watched it go green. Today's dark set is empty for all four, so this is green on `main` from the first run and safe in a required context. Three feature probes and two compiler-output fixtures are excluded, each with its reason: four are helper modules imported by a registered test, and `benchmarks/compiler_output/fixtures/raw_numeric_layout_smoke.ts` is registered in a different registry (the `raw_numeric_layouts` target-collector workload in `scripts/run_memory_stability_tests.sh`). Deliberately out of scope: `tests/*.sh|py|ts`, where 143 of 171 files are referenced by nothing in the tree. There is no registry there to diff against, so "unregistered" is not even well defined; that is an archaeology problem (triage each, wire it up or delete it) and inventing a registry for it retroactively would make this gate red on day one for reasons unrelated to the four dark witnesses. `--list` says so out loud rather than leaving the silence. Docs where an author will actually meet the rule: a new `docs/src/testing/test-registration.md`, a bullet in CONTRIBUTING.md's "what goes in a PR", and a rewritten header on each of the three registry files. Refs PerryTS#7192, PerryTS#7216, PerryTS#7252, PerryTS#7270, PerryTS#7271.
proggeramlug
pushed a commit
that referenced
this pull request
Aug 3, 2026
Four PRs in a row shipped a test file that ran nowhere. #7192 and #7216 each added a `test_gap_gc_*` stale-root witness and no corpus line; #7252 added a third; #7270/#7271 added two more, caught by the maintainer at merge. Four occurrences of one mistake is a missing gate, not carelessness. An unregistered file is not a failing test, it is no test at all. The PR is green, the reviewer sees a witness in the diff next to a passing run and reads the two together as "covered", and nothing says otherwise because nothing ran. This is CLAUDE.md's fourth hazard in its purest form: the gate runs, its subject never does. Registration checks already existed for two of the three prefixes (`gc_repsel_matrix.sh` auto-detects `test_gap_repsel_*`/`test_gap_specabi_*`, `gc-moving-witnesses.yml` adds `test_gap_gc_*`) and neither could catch the pull request that needed it: both sit behind a 90-minute release build of the compiler, behind a changed-paths relevance filter, and in workflows that are not in branch protection's required contexts. `scripts/check_test_registration.py` is the cheap half of those checks, pulled out to where it can block, and generalised past that one corpus. Pure filesystem and text — no compiler, no Node, ~0.2s — over four mechanisms: gc-repsel-corpus test-files/test_gap_{gc,repsel,specabi}_*.ts -> test-parity/gc_repsel_corpus.txt feature-matrix-probes test-features/probes/**/*.ts -> test-features/feature_matrix.toml compiler-output-workloads benchmarks/compiler_output/fixtures/**/*.ts -> benchmarks/compiler_output/workloads.toml rust-test-modules crates/*/**/tests/**/*.rs below a suite root -> the `mod` declaration in the parent module The last is the Rust analogue and is worth naming: cargo auto-discovers `crates/<c>/tests/<suite>.rs`, but a file one level deeper only compiles if a `mod` names it. Without one rustc never parses it — not dead code, not code, no warning. Built to be able to fail, against all four hazards: 1. no `continue-on-error`, no `|| true`; the step's exit status is the gate. 2. it is a step in `lint`, which is ALREADY a required context. That placement is the point: forgetting to add a new job to branch protection is hazard 2, and it is what left `gc-root-dominance` red and blocking nothing for days. No admin action is needed here because the step that gets forgotten does not exist. 3. `lint`'s concurrency already cancels pull-request runs only. 4. the subject is asserted live. Each mechanism floors its candidate set and FAILS if the glob stops matching, so "0 dark over 0 candidates" cannot print the same verdict as "0 dark over 157". Every run states `checked N files against M registries`. Exclusions are named with reasons rather than counted, because a threshold cannot tell a new dark file from an old one — fix one, add one, tally unchanged. A stale exclusion, one matching no file on disk, is itself a failure, so an excuse cannot outlive the file it excuses. Same for the mirror image: a registry entry whose file is gone fails as a rotted entry. `--self-test` (32 cases, also run in `lint`) plants an unregistered file into each of the four mechanisms over the REAL registries via an in-memory overlay, asserts the gate names it, then removes it and asserts green. It also pins the one false positive found while writing this: `resolve/tests/ declaration_sidecar_tests/compile_package.rs` IS declared, by a `mod` inside an inline `mod … { }` block two levels up, and the first draft condemned it. A gate that cries wolf gets deleted. Verified end to end on disk, not just through the overlay: planted a real unregistered file in each of the four mechanisms, watched the gate go red and name it; registered one and watched it go green; deleted the file leaving the line and watched the rotted-entry arm go red; restored and watched it go green. Today's dark set is empty for all four, so this is green on `main` from the first run and safe in a required context. Three feature probes and two compiler-output fixtures are excluded, each with its reason: four are helper modules imported by a registered test, and `benchmarks/compiler_output/fixtures/raw_numeric_layout_smoke.ts` is registered in a different registry (the `raw_numeric_layouts` target-collector workload in `scripts/run_memory_stability_tests.sh`). Deliberately out of scope: `tests/*.sh|py|ts`, where 143 of 171 files are referenced by nothing in the tree. There is no registry there to diff against, so "unregistered" is not even well defined; that is an archaeology problem (triage each, wire it up or delete it) and inventing a registry for it retroactively would make this gate red on day one for reasons unrelated to the four dark witnesses. `--list` says so out loud rather than leaving the silence. Docs where an author will actually meet the rule: a new `docs/src/testing/test-registration.md`, a bullet in CONTRIBUTING.md's "what goes in a PR", and a rewritten header on each of the three registry files. Refs #7192, #7216, #7252, #7270, #7271.
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.
The two follow-ups #7240 (landed as #7252) named and would not ship unmeasured, plus the checker change that would have made #7240 visible to the static gate in the first place.
1.
extern_func.rs'shas_restarm#7240 fixed the cross-module NON-rest arm. The rest arm has two unprotected registers where that one 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_allocplus onejs_array_push_f64per trailing argument, with each trailing argument's own expression lowered in between.The accumulator, which has no analogue in the non-rest arm and is the more dangerous of the two.
currentwas a raw*mut ArrayHeaderin 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 — was lowered. Nothing rooted it, so a minor landing in that window was free to sweep the array, not merely move it.temp_root::rooted_array_begin's doc has named this exact shape as "the shape behind every variadic / spread / rest argument list" since #6951 andconsole_promise.rshas used it since; this path never adopted it.2.
func_ref.rs's same-module arms — all four#7240's regression test needed a two-file fixture precisely because a same-file callee does not reach
extern_func.rsat all: it resolves throughExpr::FuncRef(fid)intofunc_ref.rs, so the identical defect sat oneelseaway, unreached by that PR's test.It was not folded into #7240 because
func_ref.rsthreads 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, since releasing on one side of a diamond leaves the other side's call reading dropped slots.One ordering detail is load-bearing and was wrong in the first draft of this branch. The release must be emitted after
implicit_this_restore, not before.implicit_this_save(#7211) runs below the argument lowering, so its slot sits above this group, andjs_gc_temp_root_truncatedropsbaseand everything above it. Releasing first drops the saved receiver, andjs_gc_temp_root_getanswers an out-of-range read with0— so the restore would rebind the enclosing method'sthisto the number 0. That is a miscompile rather than a rooting bug, and it fires whenever a same-module callee reads dynamicthisand at least one argument takes a real slot.3. The checker could not see any of this
--stale-registersclassified a heap-value SOURCE as anALLOC_REcall or a shadow-slot load. Aload double, ptr @…_.str.N.handleis neither, so the register it defines was never tracked and no stale use could be attributed to it. That is the blind spot #7240 shipped its fix through, and that PR's own writeup says so.The pattern already existed and was, in effect, defined twice:
--unrooted-allocashadREWRITTEN_LOAD_REand used it, while--stale-registershad onlyGLOBAL_ROOT_REand knew about@perry_global_*alone. The two modes disagreed about what a collector-rewritten load is, and the narrower one was wrong. One definition now, both modes read it. Unlike #7226'sjs_implicit_this_setand #7227'sjs_regexp_newthis could not be closed by adding a name toALLOC_RE— the source is aload, not acall.Evidence
Everything below was measured against
6aeef5baf, the pre-restack form of the commit that landed as #7252, on one tree with one set of runtime archives. The branch has since been rebased ontomain; a re-verification on the rebased tree is running and I will post it as a comment rather than edit these numbers.Gap tests
Both compiled and run with
PERRY_GC_MOVING_LOOP_POLLS=1, 10 runs per cell, for each oftest_gap_gc_rest_argument_rooting.tsandtest_gap_gc_same_module_call_argument_rooting.ts:6aeef5baf)bad 010/10bad 010/10PERRY_GC_ZEAL=1bad 010/10PERRY_GEN_GC=0bad 010/10bad 010/10The first row is why both files carry a
parity-env:line, and it is the most important thing in this PR to review. Without it the harness runs them in its default configuration, the broken compiler printsbad 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 takeManualGcScanGuard::force_full_scanand make the copying minor ineligible — nothing moves, so a stale register still names a live object.run_parity_tests.shappliesparity-envto the perry compile and the perry run (lines 955/1001), which is whatPERRY_GC_MOVING_LOOP_POLLSneeds, since it is read at both. ThePERRY_GEN_GC=0row is the control that proves the tests track collector mode rather than being flaky.The same hole exists in #7240's own
test_gap_gc_call_argument_rooting.ts, which has noparity-envline. I have not touched it here — flagging it rather than editing another PR's test.The checker, before and after
Over the 116-source / 136-module corpus from
scripts/gc_root_dominance_corpus.sh, emitted twice — once by the parent compiler and once by this branch's — so the checker delta and the codegen delta can be read separately. Columns are the checker; rows are the compiler that emitted the IR.--stale-registersstrhandle)--moving-onlystrhandle)--moving-only --fatal-sinks--stale-registersstrhandle)--moving-only--moving-only --fatal-sinksRead 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 here — every one a
load double, ptr @…_.str.N.handlefeedingjoinRestorjoinSameRestbelow the rest-array construction, which is precisely the defect the codegen half of this PR fixes. There are none anywhere else in the corpus. On this branch's IR the same widening adds zero--moving-onlyuses.So the modelling is not too broad. It found one population, that population was real, and it is now empty.
The codegen change reads out of the same table down the parent-checker column, which is an apples-to-apples measurement of the fix alone:
--moving-only110 → 62, and--moving-only --fatal-sinks32 → 0. Those 32 were allsource=alloc sink=js_array_push_f64— the unrooted rest accumulator, reported as an allocation held across the next push.The CI gate is untouched:
gc-root-dominance.ymlruns 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-testasserts the new source in both directions and under--moving-only.Recorded rather than hidden: the shared
REWRITTEN_LOAD_REalso names@perry_class_keys_*, which--unrooted-allocashas always used. It contributes 0 hits in--stale-registersover this corpus, so that arm is currently carried by the shared definition rather than exercised by it.sfw-registry --help— the existing win is not given backPERRY_FORCE_WELL_KNOWN=iovalkey, compiled and run withPERRY_GC_MOVING_LOOP_POLLS=1, both arms linking the same runtime archives and taking the same well-known routing decisions (verified in the compile logs, afterPERRY_WORKSPACE_ROOTwas pinned — a perry binary invoked from outside the workspace silently falls back to the prebuilt full stdlib and is not a comparable arm):6aeef5baf)This PR is not offered as a registry improvement — #7252 already took that workload to 30/30, and the point here is that these three edits do not give it back.
The one failure is reported, not rounded away. One run in 91 SIGSEGVed on this branch and none in 90 did on the parent. That is not enough to separate "regression" from "the known residual this effort has been chasing lands on one arm's sample and not the other's" — at a ~1 % rate, 90 clean parent runs are unsurprising either way. It is also consistent with the site named in #7154's follow-up list (
…zod…util_ts__clone + 1996,obj_type=2 size=72), which is unfixed and which nothing in this PR touches. I did not have budget to run thePERRY_GC_ZEAL=1 PERRY_GC_PROTECT_FROMSPACE=1 PERRY_GC_PROTECT_FROMSPACE_DEPTH=800arm on both binaries, which is the experiment that would settle it, and I would rather say that than present 89/91 as 30/30.What is deliberately NOT in here
Refs #7154. Stacked on #7252 (merged), so this now targets
maindirectly.Summary by CodeRabbit
Bug Fixes
arguments, and empty rest arrays.Tests