Skip to content

fix(ci): restore the gc-root-dominance gate — its corpus lost its subject to the statepoint default - #7452

Merged
proggeramlug merged 2 commits into
mainfrom
fix/gc-root-dominance-corpus-vacuous
Aug 5, 2026
Merged

fix(ci): restore the gc-root-dominance gate — its corpus lost its subject to the statepoint default#7452
proggeramlug merged 2 commits into
mainfrom
fix/gc-root-dominance-corpus-vacuous

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

gc-root-dominance has been red on main since statepoints became the default root lowering, and it is a required context — so every merge in that window bypassed the gate that protects the GC rooting invariant.

What broke

The checker anchors on @js_shadow_slot_bind call sites. #7370 made statepoints the default, and they express roots as gc.statepoint relocation bundles instead. The corpus therefore contains:

count
gc.statepoint occurrences 1251
js_gc_loop_safepoint 526
js_shadow_slot_bind call sites 0
js_shadow_slot_bind declare lines 144 (one per module)

PERRY_INLINE_SHADOW_SLOT=0 is still set and still correct — it just has nothing to act on, because the bind form is not what codegen emits any more.

The checker caught this itself rather than reporting a false green, which is the design working:

error: 0 root store(s) in the corpus, need at least 1500. The subject of
this check never ran — a clean verdict here means the IR was not the IR
you think it is (compile with PERRY_INLINE_SHADOW_SLOT=0).
=== checked 2378 functions / 144 modules (144 .ll files, 0 root stores)

That is CI's own log from run 30990401930 on main, and my local reproduction matches it to within one function (2379 vs 2378).

Fix

One env var on the corpus compile: PERRY_RS4GC=0, selecting the shadow-stack lowering the checker understands.

before after
root stores in corpus 0 3151
dominance arm exit 2 exit 0, 0 violations
seeded-violations arm never reached 40 planted / 40 caught / 0 MISSED
--unrooted-allocas --moving-only exit 2 exit 0, 0 violations, 6223 gc-capable allocas

Both gated arms run the exact CI invocations, floors included.

The gap this leaves, stated rather than papered over

This gates the shadow-stack lowering, which since #7370 is no longer the default on targets whose frames the runtime can walk. The statepoint lowering currently has no equivalent static check. Teaching the checker to read relocation bundles is a separate and larger change.

The tempting alternative — lowering or deleting --min-binds — would have produced a green tick over a corpus with no subject in it, which is CLAUDE.md's hazard 4 exactly. The comment in the script says all of this at the call site so the next person meets it before the next default flip.

Note on the 7 latent hits

Running --unrooted-allocas without --moving-only (which CI does not do) reports 7 violations in the shadow-stack corpus, all MOVING: no — in class_expr_dynamic_parent_ctor and three repsel_gc_stress functions, between js_array_numeric_value_to_raw_f64 and friends. They are not new regressions: CI has never produced this corpus, so they have simply never been visible. They are outside the gated arm and I have not touched them; worth an issue if the statepoint-side checker ever lands and makes them reachable.

Summary by CodeRabbit

  • Tests
    • Improved garbage-collection root dominance validation using a dedicated corpus and shadow-stack configuration.
    • Both validation paths now pass, including complete coverage of 40 adversarial detection cases.
    • Added clearer checks around root binding coverage.
  • Documentation
    • Documented current validation coverage and the remaining gap for statepoint-based root lowering.

Ralph Küpper added 2 commits August 5, 2026 16:27
…t binds

The checker anchors on `@js_shadow_slot_bind` call sites. Statepoints
became the default root lowering in #7370 and express roots as
`gc.statepoint` relocation bundles instead, so the corpus has carried
1251 statepoints and ZERO binds ever since. The checker's own vacuity
floor then fails the job:

  error: 0 root store(s) in the corpus, need at least 1500.
  The subject of this check never ran.

Selecting the shadow-stack lowering for the corpus restores the subject:
0 -> 3151 root stores, both gated arms exit 0, and the adversarial arm
reports 40 planted / 40 caught / 0 missed.

This gates the shadow-stack lowering only. The statepoint lowering now
has no equivalent static check; that gap is real and is named in the
script rather than hidden by lowering the floor.
@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 859c606f-5d00-47be-8078-049d541474a5

📥 Commits

Reviewing files that changed from the base of the PR and between 91487ff and 29a9ba8.

📒 Files selected for processing (2)
  • changelog.d/7452-gc-root-dominance-corpus.md
  • scripts/gc_root_dominance_corpus.sh

📝 Walkthrough

Walkthrough

The corpus build now forces shadow-stack root lowering for gc-root-dominance. Comments document checker coverage and the statepoint limitation. A changelog records the restored gate and corpus results.

Changes

GC dominance corpus

Layer / File(s) Summary
Configure shadow-stack lowering
scripts/gc_root_dominance_corpus.sh, changelog.d/7452-gc-root-dominance-corpus.md
The corpus build sets PERRY_RS4GC=0 while retaining moving-loop polls and inline shadow slots. Comments and the changelog document root-bind requirements, coverage results, and the unchecked statepoint path.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

  • PerryTS/perry#7415: Both changes set PERRY_RS4GC=0 in scripts/gc_root_dominance_corpus.sh.
  • PerryTS/perry#7370: Both changes use PERRY_RS4GC=0 to select shadow-stack root lowering.
  • PerryTS/perry#7289: Both changes document coverage limitations in the gc-root-dominance checker.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/gc-root-dominance-corpus-vacuous

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@proggeramlug
proggeramlug merged commit 45b9f79 into main Aug 5, 2026
0 of 11 checks passed
@proggeramlug
proggeramlug deleted the fix/gc-root-dominance-corpus-vacuous branch August 5, 2026 14:28
proggeramlug added a commit that referenced this pull request Aug 5, 2026
* fix(ci): the dependency-scale dominance corpus was vacuous too

#7452 fixed the curated corpus and missed this one. Same cause: the
checker anchors on @js_shadow_slot_bind call sites, statepoints became
the default root lowering in #7370 and express roots as gc.statepoint
relocation bundles instead, so the corpus carried 81 modules with ZERO
of the checker's subject.

  before: 81 modules, 0 bind call sites
  after:  81 modules, 7719 bind call sites

CI's own floors say what the corpus is supposed to look like -- the step
comment reads '81 modules, ~12900 functions, ~7700 root stores' and sets
--min-binds 4000. The fixed corpus measures 81 / 12899 / 7719, i.e. the
floors were written against the shadow-stack lowering and this restores
exactly the state they were set from.

Both gated arms exit 0 with 40/40 seeded violations caught.

This is the corpus #7280 created BECAUSE the curated one reads zero while
twenty lines of stock zod fault, so leaving it measuring nothing defeats
the reason it exists.

* docs: changelog fragment for #7460

---------

Co-authored-by: Ralph Küpper <ralph@skelpo.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant