From 0794dc1e640f9a6943b6b51aedfb8f3b7c4b7f6e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 1 Aug 2026 17:59:25 +0200 Subject: [PATCH 1/2] fix(codegen): close three more root-store dominance holes, and make the checker able to fail MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Maintenance pass on #7192. Four review findings adjudicated reproducer-first; three were real and are fixed here, one was rejected with evidence. REAL — `Expr::ObjectSpread` / `Expr::ClassExprFresh` protect predicates. `protect_handle` was computed from the operand expressions alone, so `{ ...src, tail: 7 }` over inert parts pushed no root at all — and `js_object_copy_own_fields` reads every own key of the source, which runs a getter, which is arbitrary JS that can reach a back-edge poll. Reproduced at the PR head under PERRY_GC_MOVING_LOOP_POLLS=1: `bad 10` deterministic 3/3, clean by default. Same for a class expression carrying `static { … }` with otherwise-inert statics: `bad 4` 3/3. A spread part and a static block now force protection on their own; `block_fns` moves above `rooted_handle_begin` so the predicate can see it. The rest of the predicate stays byte-identical to `Expr::Object`'s, because an allocation inside a runtime helper provably cannot INITIATE a moving collection — `gc_check_trigger`'s minor arm defers to the loop safepoint under polls and is conservative-scanned or budgeted non-moving otherwise. REAL — the inline-ctor result slot. `ctor_result_slot` is a plain `alloca_entry`: not a shadow slot, not a temp root, never rewritten. Seeding it with `obj_box` parked the PRE-constructor instance address in unrooted memory for the whole body, and on fall-through `js_ctor_return_override` saw an *object* in `raw` and returned THAT — discarding the re-read instance the new `reload_instance` had just recovered. The dominance fix was defeated at its last instruction, and the PR's own regression test could not see it because it walks the FIRST operand, which is the re-read one. The slot now starts at `undefined`, which is exactly equivalent on all four paths (fall-through, bare `return;`, `return `, inherited-symbol ctor) and carries no address. A regression test pins it. REAL — the key operand of a computed store. `o[k] = f()` and a class expression's `[sym]: init` lower the key before the value, leaving it in an SSA register across the same window the receiver had. Rooted with the same guard, released inside-out so the temp-root cuts nest. No runtime reproducer found (the value survives; the staleness is latent until the abandoned memory is reused, which is #7154's fingerprint), so this rests on the same static argument as #7114. REJECTED — `%entry.implicit` in the checker. No such code exists; the parser drops pre-label instructions rather than synthesising a block, and numeric labels already parse correctly. Perry's IR writer provably cannot emit either shape (`function.rs` emits a label before every block, always `name.counter`), confirmed over 214 real functions / 3608 labels. But the adjacent defect is real and worse: a label-less function parsed to zero blocks and was SILENTLY SKIPPED — a planted violation vanished and the run exited 0. Both that and a label-shaped line the strict regex declines now raise `MalformedIR`, and the `; preds =` form LLVM itself prints is accepted rather than mis-parsed. The checker could not fail in three other ways, all fixed: it exited 0 on no arguments, on `--help`, on a typo'd flag and on a directory holding no `.ll`; it had no liveness assertion, so a clean verdict over zero root stores was indistinguishable from a clean one over the real corpus; and `PERRY_SAVE_LL` was never honoured for modules past `MIN_CALLABLES_TO_SPLIT`, so `--trace llvm` silently emitted nothing for exactly the largest modules. `main` now uses argparse, requires `--min-files` / `--min-binds`, and grows a `--self-test` that plants a violation and asserts the checker reports it. The split path writes one `.ll` per codegen unit. Wires the gate into a new `gc-root-dominance` workflow — deliberately NOT in branch protection's required contexts yet, since a gate that has never been green blocks every open PR the day it is promoted. CLAUDE.md gains the invariant itself under known-weak areas, including the third way it breaks that is still open: a heap value in a plain `alloca_entry` (`lower_call/new.rs`'s inline-ctor `this_slot`). Refs #7154, #7184, #7192, #7114, #6951. --- .github/workflows/gc-root-dominance.yml | 157 ++++++++++ CLAUDE.md | 1 + changelog.d/7192-root-store-dominance.md | 2 +- crates/perry-codegen/src/codegen/mod.rs | 21 +- crates/perry-codegen/src/expr/index_set.rs | 23 +- .../src/expr/logical_collections.rs | 19 +- crates/perry-codegen/src/expr/property_set.rs | 14 +- .../src/expr/static_field_meta.rs | 65 ++-- crates/perry-codegen/src/expr/temp_root.rs | 81 +++-- crates/perry-codegen/src/lower_call/new.rs | 31 +- crates/perry-codegen/src/stmt/mod.rs | 8 +- .../tests/temp_root_operand_temporaries.rs | 80 +++++ scripts/gc_root_dominance_check.py | 278 +++++++++++++++++- 13 files changed, 692 insertions(+), 88 deletions(-) create mode 100644 .github/workflows/gc-root-dominance.yml diff --git a/.github/workflows/gc-root-dominance.yml b/.github/workflows/gc-root-dominance.yml new file mode 100644 index 0000000000..8574fb4ca1 --- /dev/null +++ b/.github/workflows/gc-root-dominance.yml @@ -0,0 +1,157 @@ +name: GC Root Dominance + +# Static gate for the invariant a GC-managed value's root store must DOMINATE +# every subsequent site that can trigger a collection (#7154). +# +# Two bugs of this class shipped before there was an instrument for it. #7184: +# the root store was emitted but its shadow-slot index fell outside the pushed +# frame, so `js_shadow_slot_bind` bounds-checked it into a silent no-op. #7192: +# the store was emitted in-frame but AFTER a call that allocates. Both present +# identically — a *rooted* slot holding a dangling pointer, surfacing cycles +# later as "TypeError: value is not a function" — and neither is visible to any +# runtime GC probe, because at the moment of the collection there is nothing +# for the collector to find. A static pass over the emitted IR is the only +# instrument that sees them before they crash, which is why this is a gate and +# not a benchmark. +# +# THIS JOB IS DESIGNED TO BE ABLE TO FAIL, and is checked against all four ways +# a gate can be unable to (CLAUDE.md): +# +# 1. no `continue-on-error`, no `|| true`, no pipe between the checker and +# the shell's exit status; +# 2. NOT yet in branch protection's required contexts — deliberately, because +# a new gate has never been green and promoting it immediately blocks every +# open PR. Promote after one clean week on `main`; +# 3. `concurrency` cancels pull-request runs only, never `main` runs; +# 4. the subject is ASSERTED live, not assumed. `--self-test` proves the +# checker still reports a planted violation and still clears the control, +# and `--min-files` / `--min-binds` refuse a clean verdict over a corpus +# that contained no modules or no root stores. An empty `.perry-trace/llvm` +# is a routine outcome of a failed compile, so "0 violations" over 0 files +# must be an error rather than a pass. + +on: + pull_request: + push: + branches: [main] + workflow_dispatch: + +permissions: + contents: read + +concurrency: + # Per-event groups, cancelling PR runs only. A shared group with an + # unconditional cancel-in-progress starves `main`: on a deep runner queue + # every merge cancels the previous main run before it reaches a runner, and a + # gate that is always cancelled never fails. Same reasoning as gc-ratchet.yml. + group: gc-root-dominance-${{ github.event_name }}-${{ github.ref }} + cancel-in-progress: ${{ github.event_name == 'pull_request' }} + +env: + CARGO_TERM_COLOR: always + MACOSX_DEPLOYMENT_TARGET: "13.0" + +jobs: + gc-root-dominance: + runs-on: macos-14 + timeout-minutes: 90 + steps: + - uses: actions/checkout@v7 + with: + persist-credentials: false + + # Fast structural failure first: prove the checker can still fail before + # spending a compiler build on it. This is the arm that would have caught + # `PERRY_GC_FORCE_EVACUATE` being inert for every test that "exercised" + # it (#6942/#6946). + - name: Checker self-test (can this gate still fail?) + run: python3 scripts/gc_root_dominance_check.py --self-test + + - name: Install Rust toolchain + uses: dtolnay/rust-toolchain@stable + + - name: Cache cargo + uses: actions/cache@v6 + with: + path: | + ~/.cargo/registry + ~/.cargo/git + target + key: ${{ runner.os }}-cargo-gcdom-${{ hashFiles('**/Cargo.lock') }} + restore-keys: ${{ runner.os }}-cargo- + + - name: Build perry and the runtime archives + run: | + set -euo pipefail + # perry-runtime and perry-stdlib are rlib-only; the .a files come from + # the -static wrapper crates. The package set is fixed so cargo + # feature unification matches every other job that builds the + # compiler. + cargo build --release -p perry -p perry-runtime-static -p perry-stdlib-static + for artifact in perry libperry_runtime.a libperry_stdlib.a; do + test -s "target/release/$artifact" \ + || { echo "::error::target/release/$artifact was not produced"; exit 1; } + done + + - name: Emit the IR corpus + env: + # PERRY_GC_MOVING_LOOP_POLLS=1 is what puts `js_gc_loop_safepoint` in + # the IR, which is what the MOVING classification keys on. It is off + # by default (#7161 stopgap), so without it this gate would run over + # IR that cannot express the bug — hazard 4 again. + PERRY_GC_MOVING_LOOP_POLLS: "1" + # Makes every root store the @js_shadow_slot_bind call form. The #7088 + # inline diamond is equivalent but harder to anchor on. + PERRY_INLINE_SHADOW_SLOT: "0" + PERRY_NO_AUTO_OPTIMIZE: "1" + run: | + set -euo pipefail + mkdir -p ir-corpus + # A spread of shapes that exercise the lowerings this invariant runs + # through: construction, object/array literals and spreads, class + # expressions with statics, property and element stores, closures. + # Kept to test-files/ so the corpus is versioned with the repo rather + # than depending on a private workload. + shopt -s nullglob + sources=( + test-files/test_gap_gc_*.ts + test-files/test_gap_class*.ts + test-files/test_gap_object*.ts + test-files/test_gap_static*.ts + test-files/test_gap_prop*.ts + ) + if [ "${#sources[@]}" -eq 0 ]; then + echo "::error::no corpus sources matched; the glob is stale" + exit 1 + fi + for src in "${sources[@]}"; do + name="$(basename "$src" .ts)" + rm -rf .perry-trace/llvm + # A source that fails to compile must not silently shrink the + # corpus: --min-files below is the backstop, but say so here too. + if ! ./target/release/perry compile "$src" -o "/tmp/$name" --trace llvm >/dev/null 2>&1; then + echo "::warning::$src did not compile; skipping" + continue + fi + for ll in .perry-trace/llvm/*.ll; do + cp "$ll" "ir-corpus/${name}__$(basename "$ll")" + done + done + echo "corpus: $(ls ir-corpus | wc -l) .ll files" + + - name: Check root-store dominance + run: | + set -euo pipefail + # No pipe: the checker's own exit status is the job's. --min-binds + # asserts the corpus actually contained root stores, so a green + # verdict cannot come from IR that never had a subject. + python3 scripts/gc_root_dominance_check.py ir-corpus \ + --moving-only --min-files 5 --min-binds 50 -v + + - name: Upload the IR corpus on failure + if: failure() + uses: actions/upload-artifact@v4 + with: + name: gc-root-dominance-ir + path: ir-corpus + retention-days: 7 diff --git a/CLAUDE.md b/CLAUDE.md index 2869076f49..a8bbec8457 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -237,3 +237,4 @@ Corollary: a *new* gate has never been green, so promoting it to required immedi - **Async-to-generator transform, body locals.** It boxes every body local into a shared mutable cell typed `Any`. Two consequences seen in the wild: per-iteration `let`/`const` bindings collapse for closures created in a loop, and computed numeric-key calls (`arr[i](x)`) lose their type proof and silently resolve by *method name*, evaporating the call. - **Native base-class subclassing.** A native base's surface is installed at `super()` time and its parent edge lives in the class registry; keying any of that on a literal `extends` name loses it for fieldless classes, indirect subclasses, and class expressions. - **Two prototype-resolution paths.** `CLASS_PROTOTYPE_OBJECTS` (synthetic: `Object.create`, plain-function ctors) vs `CLASS_DECL_PROTOTYPE_OBJECTS` (declared classes). `in`/`for…in` and `getPrototypeOf` have disagreed about the same chain. +- **Root-store dominance in codegen.** *A GC-managed value's root store must **dominate** every subsequent site that can collect.* Three ways it has broken, all shipped: the store's slot index fell outside the pushed shadow frame so `js_shadow_slot_bind` bounds-checked it into a silent no-op (#7184); the store was emitted in-frame but **after** a call that allocates (#7192); and the value lives in a plain `alloca_entry` that is neither a shadow slot nor a temp root, so the collector never rewrites it (`lower_call/new.rs`'s inline-ctor `this_slot`, still open). All three present identically — a *rooted* slot holding a dangling pointer, surfacing cycles later as `TypeError: value is not a function` — and **none is visible to any runtime GC probe**, because at the moment of the collection there is nothing for the collector to find. That is why #7154's from-space scan only ever saw offenders whose targets had already died. The instrument is static: `scripts/gc_root_dominance_check.py` over `--trace llvm` output (`--self-test` proves it can still fail). Only bites under `PERRY_GC_MOVING_LOOP_POLLS=1`, off by default since #7161 — so a green default run says nothing about this class. diff --git a/changelog.d/7192-root-store-dominance.md b/changelog.d/7192-root-store-dominance.md index 65bf31b367..31d17c925d 100644 --- a/changelog.d/7192-root-store-dominance.md +++ b/changelog.d/7192-root-store-dominance.md @@ -4,7 +4,7 @@ - **`new C(…)` (`lower_call/new.rs`)** — the load-bearing one. The instance was a raw register while the constructor body ran. It is rooted inside the callee (the `this` parameter has a shadow slot), so the minor does not free it — it *moves* it and rewrites the callee's root, leaving the caller's register naming from-space. `js_gc_init_typed_shape_layout` then installed the layout descriptor on the abandoned copy and `js_ctor_return_override` published that dead address into the caller's shadow slot: a *rooted* slot holding a dangling pointer, which is exactly why #7154's from-space scan only ever saw offenders one or more cycles after the target died, with correct layout coverage. The instance is now temp-rooted across the constructor and re-read afterwards, on both the standalone-`_constructor` symbol path and the inline-ctor path. A class with no constructor, no fields and no heritage runs no user code in that window and keeps its previous IR exactly. - **`Expr::ObjectSpread` (`expr/logical_collections.rs`)** — `{ ...a, k: f() }` allocated the object and then wrote every field through a register held across each part's lowering, with no rooting at all. `Expr::Object` has used `RootedHandle` for this since #6951; the spread form never got it. - **`Expr::ClassExprFresh` (`expr/static_field_meta.rs`)** — same shape for the fresh class object built by a class-expression factory, across its static-field initializers, captured arguments, symbol statics and `static { … }` blocks. - - **property / element stores (`expr/property_set.rs`, `expr/index_set.rs`)** — `o.k = f()` and `o[k] = f()` evaluate the reference first and the value second (spec order), leaving the receiver in a register across `f()`. The *slot* it was loaded from is a root and gets rewritten; the register does not, so the store landed in from-space and the field never appeared on the object the program kept. This is #7114 with a receiver instead of a string literal. A new `temp_root::ReceiverGuard` roots the receiver only when the value expression can collect, so stores with an inert RHS keep their previous IR. + - **property / element stores (`expr/property_set.rs`, `expr/index_set.rs`)** — `o.k = f()` and `o[k] = f()` evaluate the reference first and the value second (spec order), leaving the receiver in a register across `f()`. The *slot* it was loaded from is a root and gets rewritten; the register does not, so the store landed in from-space and the field never appeared on the object the program kept. This is #7114 with a receiver instead of a string literal. A new `temp_root::StoreOperandGuard` roots the receiver only when the value expression can collect, so stores with an inert RHS keep their previous IR. `temp_root::temp_root_scope_begin` now takes the caller's extra reason to open a scope, because `new C()` with no arguments still needs a marker to cut the instance root against. diff --git a/crates/perry-codegen/src/codegen/mod.rs b/crates/perry-codegen/src/codegen/mod.rs index e9cb91d3f9..9df1d76da2 100644 --- a/crates/perry-codegen/src/codegen/mod.rs +++ b/crates/perry-codegen/src/codegen/mod.rs @@ -2653,9 +2653,9 @@ pub fn compile_module(hir: &HirModule, opts: CompileOptions) -> Result> // #5391 codegen units: large modules split their object compilation into N // independently-compiled units so clang's peak RSS stays ~whole/N instead of // OOMing on one giant TU. Gated to large modules (default 1 unit = unchanged - // behavior). `emit_ir_only` and `PERRY_SAVE_LL` want the whole-module text, - // so they take the single-text path; the split path avoids materializing the - // full ~1GB IR string at all (which would defeat the memory win). + // behavior). `emit_ir_only` wants the whole-module text, so it takes the + // single-text path; the split path avoids materializing the full ~1GB IR + // string at all (which would defeat the memory win). let n_units = if opts.emit_ir_only { 1 } else { @@ -2668,6 +2668,21 @@ pub fn compile_module(hir: &HirModule, opts: CompileOptions) -> Result> hir.name, units.len() ); + // #7154: dump the units. The comment above used to claim `PERRY_SAVE_LL` + // took the single-text path — it never did; this `return` fires before + // the `PERRY_SAVE_LL` write below. So `--trace llvm` silently emitted + // NOTHING for any module past `MIN_CALLABLES_TO_SPLIT`, i.e. exactly the + // largest modules, which is where a static IR audit + // (`scripts/gc_root_dominance_check.py`) most needs to look — a corpus + // that quietly omits its biggest members makes a clean verdict + // meaningless. One file per unit, not one concatenation: the units are + // already materialized here, so this adds no peak. + if let Ok(save_dir) = std::env::var("PERRY_SAVE_LL") { + for (i, unit) in units.iter().enumerate() { + let filename = format!("{}/{}.unit{}.ll", save_dir, module_prefix, i); + let _ = std::fs::write(&filename, unit); + } + } return crate::linker::compile_units_to_object(&units, opts.target.as_deref()); } diff --git a/crates/perry-codegen/src/expr/index_set.rs b/crates/perry-codegen/src/expr/index_set.rs index aa89eb277e..665e884d5f 100644 --- a/crates/perry-codegen/src/expr/index_set.rs +++ b/crates/perry-codegen/src/expr/index_set.rs @@ -1430,14 +1430,14 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { // minor inside it relocates the receiver out from under // `obj_box`. Root it across the evaluation and re-read below. let recv_guard = - super::temp_root::guard_store_receiver(ctx, object, &obj_box, value); + super::temp_root::guard_store_operand(ctx, object, &obj_box, value); let (val_double, _val_bits) = lower_value_for_dynamic_index_set( ctx, value, "index_set.literal_string_value_bits", "literal_string_index_set_helper_edge", )?; - let obj_box = super::temp_root::reread_store_receiver(ctx, &recv_guard, &obj_box); + let obj_box = super::temp_root::reread_store_operand(ctx, &recv_guard, &obj_box); let key_idx = ctx.strings.intern(literal); let key_handle_global = format!("@{}", ctx.strings.entry(key_idx).handle_global); let obj_bits = ctx.block().bitcast_double_to_i64(&obj_box); @@ -1476,22 +1476,33 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { (DOUBLE, &val_double), ], ); - super::temp_root::release_store_receiver(ctx, recv_guard); + super::temp_root::release_store_operand(ctx, recv_guard); return Ok(val_double); } if is_string_expr(ctx, index) { let obj_box = lower_expr(ctx, object)?; // #7154: see the literal-key arm above. let recv_guard = - super::temp_root::guard_store_receiver(ctx, object, &obj_box, value); + super::temp_root::guard_store_operand(ctx, object, &obj_box, value); let key_box = lower_expr(ctx, index)?; + // #7154: the KEY sits in the same window as the receiver. A + // non-literal string key is an ordinary heap string with no + // registered root of its own, so an evacuating minor inside the + // value's evaluation relocates it and leaves `key_box` naming + // from-space — `unbox_str_handle` below would hand the setter a + // pre-move `StringHeader*` and the field would land under a + // garbage key. Pushed AFTER `recv_guard` so the two cuts nest: + // `temp_root_truncate` drops everything above its index, so the + // key must be released first. + let key_guard = super::temp_root::guard_store_operand(ctx, index, &key_box, value); let (val_double, _val_bits) = lower_value_for_dynamic_index_set( ctx, value, "index_set.string_value_bits", "string_index_set_helper_edge", )?; - let obj_box = super::temp_root::reread_store_receiver(ctx, &recv_guard, &obj_box); + let key_box = super::temp_root::reread_store_operand(ctx, &key_guard, &key_box); + let obj_box = super::temp_root::reread_store_operand(ctx, &recv_guard, &obj_box); let obj_bits = ctx.block().bitcast_double_to_i64(&obj_box); super::property_set::emit_nullish_write_guard( ctx, @@ -1527,7 +1538,7 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { (DOUBLE, &val_double), ], ); - super::temp_root::release_store_receiver(ctx, recv_guard); + super::temp_root::release_store_operand(ctx, recv_guard); return Ok(val_double); } // Fallback with runtime STRING_TAG check, matching IndexGet. diff --git a/crates/perry-codegen/src/expr/logical_collections.rs b/crates/perry-codegen/src/expr/logical_collections.rs index 28a9f67a4b..805843790e 100644 --- a/crates/perry-codegen/src/expr/logical_collections.rs +++ b/crates/perry-codegen/src/expr/logical_collections.rs @@ -922,8 +922,23 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { // from-space memory, so the fields silently vanish from the copy // the caller receives. This is the same rooting contract // `Expr::Object` has used since #6951; `ObjectSpread` never got it. - let protect_handle = - super::temp_root::any_may_trigger_gc(ctx, parts.iter().map(|(_, v)| v)); + // + // A spread part forces protection on its own, independently of + // whether the spread *expression* collects. `js_object_copy_own_fields` + // reads every own key of the source, so a source carrying an accessor + // runs arbitrary user code inside the helper — `{ ...a }` over a plain + // `LocalGet` answers `false` to `any_may_trigger_gc` and is still a + // collection point. + // + // A plain `js_object_set_field_by_name` on an inert value is NOT one, + // which is why the rest of the predicate stays byte-identical to + // `Expr::Object`'s: an allocation inside a runtime helper can never + // *initiate* a moving collection. `gc_check_trigger`'s minor arm + // defers to the loop safepoint under `PERRY_GC_MOVING_LOOP_POLLS=1` + // (`gc/policy.rs`, `GC_SAFEPOINT_PENDING`) and is conservative-scanned + // or budgeted-non-moving otherwise, so the register stays valid. + let protect_handle = parts.iter().any(|(k, _)| k.is_none()) + || super::temp_root::any_may_trigger_gc(ctx, parts.iter().map(|(_, v)| v)); let rooted = super::temp_root::rooted_handle_begin(ctx, &obj_handle, protect_handle); for (key_opt, value_expr) in parts { if let Some(key) = key_opt { diff --git a/crates/perry-codegen/src/expr/property_set.rs b/crates/perry-codegen/src/expr/property_set.rs index cbe97f1d6d..a1e4c955b8 100644 --- a/crates/perry-codegen/src/expr/property_set.rs +++ b/crates/perry-codegen/src/expr/property_set.rs @@ -51,9 +51,9 @@ fn lower_runtime_property_set_by_name( ) -> Result { let recv_box = lower_expr(ctx, object)?; // #7154: root the receiver across the value's evaluation, which allocates. - let recv_guard = super::temp_root::guard_store_receiver(ctx, object, &recv_box, value); + let recv_guard = super::temp_root::guard_store_operand(ctx, object, &recv_box, value); let val_double = lower_expr(ctx, value)?; - let recv_box = super::temp_root::reread_store_receiver(ctx, &recv_guard, &recv_box); + let recv_box = super::temp_root::reread_store_operand(ctx, &recv_guard, &recv_box); let key_idx = ctx.strings.intern(property); let dispatch_global = ctx.strings.static_dispatch_global(key_idx); let blk = ctx.block(); @@ -63,7 +63,7 @@ fn lower_runtime_property_set_by_name( "js_object_set_field_by_property_id", &[(I64, &obj_bits), (I64, &property_id), (DOUBLE, &val_double)], ); - super::temp_root::release_store_receiver(ctx, recv_guard); + super::temp_root::release_store_operand(ctx, recv_guard); Ok(val_double) } @@ -1003,14 +1003,14 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { // inside it relocates the receiver out from under `obj_box` -- // `obj.k = f()` then writes `k` into abandoned from-space memory // and the field never appears on the object the program keeps. - let recv_guard = super::temp_root::guard_store_receiver(ctx, object, &obj_box, value); + let recv_guard = super::temp_root::guard_store_operand(ctx, object, &obj_box, value); let (val_double, _val_bits) = lower_value_for_dynamic_property_set( ctx, value, "property_set.dynamic_value_bits", "dynamic_property_set_helper_edge", )?; - let obj_box = super::temp_root::reread_store_receiver(ctx, &recv_guard, &obj_box); + let obj_box = super::temp_root::reread_store_operand(ctx, &recv_guard, &obj_box); // Intern the field name in the StringPool (same one the // matching getter uses, so they share the global string). let key_idx = ctx.strings.intern(property); @@ -1033,7 +1033,7 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { "js_object_set_field_by_name", &[(I64, &obj_bits), (I64, &key_raw), (DOUBLE, &val_double)], ); - super::temp_root::release_store_receiver(ctx, recv_guard); + super::temp_root::release_store_operand(ctx, recv_guard); return Ok(val_double); } let site_id = emit_typed_feedback_register_site( @@ -1051,7 +1051,7 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { (DOUBLE, &val_double), ], ); - super::temp_root::release_store_receiver(ctx, recv_guard); + super::temp_root::release_store_operand(ctx, recv_guard); Ok(val_double) } diff --git a/crates/perry-codegen/src/expr/static_field_meta.rs b/crates/perry-codegen/src/expr/static_field_meta.rs index 9304764f58..104c15b0b1 100644 --- a/crates/perry-codegen/src/expr/static_field_meta.rs +++ b/crates/perry-codegen/src/expr/static_field_meta.rs @@ -12,6 +12,32 @@ use crate::types::{DOUBLE, I32, I64, PTR}; use super::{emit_root_nanbox_store_on_block, lower_expr, nanbox_pointer_inline, FnCtx}; +/// The compiled symbols for `template`'s `static { … }` blocks, in declaration +/// order (#685). +/// +/// Lifted out of `Expr::ClassExprFresh`'s body so the #7154 rooting predicate +/// can ask "does this class expression run arbitrary user code?" *before* the +/// object is exposed, instead of discovering it at the loop that invokes them. +fn static_block_fns(ctx: &FnCtx<'_>, template: &str) -> Vec { + ctx.classes + .get(template) + .map(|c| { + c.static_methods + .iter() + .filter(|m| m.name.starts_with("__perry_static_init_")) + .filter_map(|m| { + ctx.methods + .get(&( + template.to_string(), + crate::codegen::static_method_registry_key(&m.name), + )) + .cloned() + }) + .collect() + }) + .unwrap_or_default() +} + pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { match expr { Expr::StaticFieldGet { @@ -436,8 +462,18 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { // allocates a `js_array_alloc` accumulator and grows it with // `js_array_push_f64` per element, and those are collection points // even when every element is an inert `LocalGet`. + // + // So does a `static { … }` block, for the plainer reason that its + // body is arbitrary user code — which is why `block_fns` is computed + // HERE rather than at its loop below: the predicate has to see it. + // A class expression whose only statics are inert (`static x = 1`) + // but which carries a static block otherwise pushed no root at all, + // and the block's body could then relocate the object out from under + // the register the final `nanbox_pointer_inline` reads. + let block_fns = static_block_fns(ctx, template); let protect_handle = !captured_args.is_empty() || !symbol_statics.is_empty() + || !block_fns.is_empty() || super::temp_root::any_may_trigger_gc(ctx, named_statics.iter().map(|(_, v)| v)); let rooted = super::temp_root::rooted_handle_begin(ctx, &obj, protect_handle); for (name, init) in named_statics { @@ -503,7 +539,12 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { } for (key, init) in symbol_statics { let k = lower_expr(ctx, key)?; + // #7154: `key` is lowered before `init`, so the Symbol sits in + // an SSA register across an arbitrary initializer — the same + // exposure the receiver has, one operand over. Root it. + let key_guard = super::temp_root::guard_store_operand(ctx, key, &k, init); let v = lower_expr(ctx, init)?; + let k = super::temp_root::reread_store_operand(ctx, &key_guard, &k); // #7154: both lowerings above can collect; re-derive the // receiver from the root rather than reusing the register. let obj = super::temp_root::rooted_handle_get(ctx, &rooted); @@ -513,6 +554,10 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { "js_object_set_symbol_property", &[(DOUBLE, &obj_box), (DOUBLE, &k), (DOUBLE, &v)], ); + // Cut per iteration rather than letting `rooted`'s release do it + // at the end: the setter this call may invoke is user code, and + // N statics would otherwise hold N slots across all of them. + super::temp_root::release_store_operand(ctx, key_guard); } // #685: run the class's `static { … }` blocks NOW — at the class // expression's evaluation, with `this` = THIS fresh class object. @@ -527,24 +572,8 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { // object, not the shared template). Blocks run after the named // static fields above — the source interleaving of fields and // blocks is not reproduced on this path (pre-existing limitation). - let block_fns: Vec = ctx - .classes - .get(template) - .map(|c| { - c.static_methods - .iter() - .filter(|m| m.name.starts_with("__perry_static_init_")) - .filter_map(|m| { - ctx.methods - .get(&( - template.clone(), - crate::codegen::static_method_registry_key(&m.name), - )) - .cloned() - }) - .collect() - }) - .unwrap_or_default(); + // + // `block_fns` is computed above, next to `protect_handle`. for fn_name in block_fns { // #7154: a static block runs arbitrary user code, so re-derive // the receiver from the root before each one. diff --git a/crates/perry-codegen/src/expr/temp_root.rs b/crates/perry-codegen/src/expr/temp_root.rs index a907d91953..6bf002c092 100644 --- a/crates/perry-codegen/src/expr/temp_root.rs +++ b/crates/perry-codegen/src/expr/temp_root.rs @@ -551,52 +551,69 @@ pub(crate) fn temp_root_release(ctx: &mut FnCtx<'_>, guard: Option) { } } -/// The receiver of a property/element STORE, kept valid across the evaluation -/// of the value being stored (#7154). -/// -/// `o.k = f()` and `o[k] = f()` evaluate the reference first and the value -/// second — spec order, and codegen follows it. That leaves the receiver in an -/// SSA register while `f()` runs, and `f()` allocates. A back-edge poll inside -/// it drives an evacuating minor which relocates the receiver; the *slot* the -/// register was loaded from is a root and gets rewritten, but the register does -/// not, so the store lands in abandoned from-space memory and the field never -/// appears on the object the program keeps. +/// An operand of a property/element STORE that is lowered *before* the value, +/// kept valid across the value's evaluation (#7154). +/// +/// Two operands are in that position, and both need it: +/// +/// - the **receiver**. `o.k = f()` and `o[k] = f()` evaluate the reference +/// first and the value second — spec order, and codegen follows it. That +/// leaves the receiver in an SSA register while `f()` runs, and `f()` +/// allocates. A back-edge poll inside it drives an evacuating minor which +/// relocates the receiver; the *slot* the register was loaded from is a root +/// and gets rewritten, but the register does not, so the store lands in +/// abandoned from-space memory and the field never appears on the object the +/// program keeps. +/// - the **computed key**. `o[k] = f()` lowers `k` before `f`, and a +/// non-literal string key is an ordinary heap string with the same exposure: +/// `unbox_str_handle` below the call then reads a pre-move `StringHeader*`, +/// so the field lands under a garbage key. Same for the `[sym]: init` pair of +/// a class expression's symbol statics, where the Symbol is lowered before +/// its initializer. /// /// This is the store-side instance of the [module invariant](self): property /// (2) — a rewritten location — is worthless without property (3), reading that -/// location again below the collection point. It is #7114 with a receiver -/// instead of a string literal. +/// location again below the collection point. It is #7114 with a store operand +/// instead of a call operand. +/// +/// A temp root (not a re-load) is the required strategy: re-lowering the +/// operand would observe an assignment made by `f()` itself, which is a +/// miscompile rather than a rooting fix — see [`operand_is_reloadable`]. /// -/// A temp root (not a re-load) is the required strategy: re-lowering `object` -/// would observe an assignment made by `f()` itself, which is a miscompile -/// rather than a rooting fix — see [`operand_is_reloadable`]. -pub(crate) struct ReceiverGuard { +/// Guards nest: push the receiver's first and the key's second, then release in +/// the opposite order, because [`temp_root_truncate`] is a stack *cut* and a +/// release of the outer one drops the inner. +pub(crate) struct StoreOperandGuard { slot: Option, } -/// Root `recv` (the lowered `object`) if evaluating `value` can collect. -/// Emits nothing otherwise, so stores with an inert RHS keep their old IR. -pub(crate) fn guard_store_receiver( +/// Root `lowered` (the already-lowered `operand`) if evaluating `value` can +/// collect. Emits nothing otherwise, so stores with an inert RHS keep their old +/// IR. +pub(crate) fn guard_store_operand( ctx: &mut FnCtx<'_>, - object: &Expr, - recv: &str, + operand: &Expr, + lowered: &str, value: &Expr, -) -> ReceiverGuard { +) -> StoreOperandGuard { let collects = expr_may_trigger_gc(ctx, value); - let slot = match operand_protection(ctx, object, collects) { - OperandProtection::Root => Some(temp_root_push_double(ctx, recv)), - // `Reload`/`Reuse` both mean the register survives: a string literal - // cannot be a store receiver, and a proven non-pointer is not movable. + let slot = match operand_protection(ctx, operand, collects) { + OperandProtection::Root => Some(temp_root_push_double(ctx, lowered)), + // `Reload` means the operand is a string literal: a registered, + // immutable global root, so re-deriving it below the collection point + // is exact — and the call sites here re-lower nothing, they simply keep + // the register, which for a literal is a load from that same global. + // `Reuse` means a proven non-pointer, which relocation cannot touch. OperandProtection::Reload | OperandProtection::Reuse => None, }; - ReceiverGuard { slot } + StoreOperandGuard { slot } } -/// Re-read the receiver below the value's evaluation. Returns `recv` unchanged -/// when nothing was rooted. -pub(crate) fn reread_store_receiver( +/// Re-read the operand below the value's evaluation. Returns `lowered` +/// unchanged when nothing was rooted. +pub(crate) fn reread_store_operand( ctx: &mut FnCtx<'_>, - guard: &ReceiverGuard, + guard: &StoreOperandGuard, recv: &str, ) -> String { match &guard.slot { @@ -610,7 +627,7 @@ pub(crate) fn reread_store_receiver( /// Drop the guard. Call it *after* the store, not before: the store helper /// allocates (key interning, field-array growth, shape transition). -pub(crate) fn release_store_receiver(ctx: &mut FnCtx<'_>, guard: ReceiverGuard) { +pub(crate) fn release_store_operand(ctx: &mut FnCtx<'_>, guard: StoreOperandGuard) { if let Some(idx) = guard.slot { temp_root_truncate(ctx, &idx); } diff --git a/crates/perry-codegen/src/lower_call/new.rs b/crates/perry-codegen/src/lower_call/new.rs index da0d4f7eba..4cea219ea2 100644 --- a/crates/perry-codegen/src/lower_call/new.rs +++ b/crates/perry-codegen/src/lower_call/new.rs @@ -1138,12 +1138,33 @@ fn lower_new_impl_inner( // inside the (about-to-be-inlined) ctor body must apply spec // return-override semantics and yield the `new` expression's value — // NOT emit a function-level `ret` that terminates the enclosing - // function. `ctor_result_slot` starts as `this`; `Stmt::Return` - // overwrites it with a returned object (or throws for a derived ctor - // returning a primitive), then branches to `after_idx`. Refs - // class/subclass/derived-class-return-override-*. + // function. `Stmt::Return` overwrites the slot with the returned value + // (or throws for a derived ctor returning a primitive), then branches to + // `after_idx`. Refs class/subclass/derived-class-return-override-*. + // + // #7154: the slot starts at `undefined`, NOT at `this`. + // + // It is a plain entry alloca — not a shadow slot, not a temp root — so the + // collector neither marks nor rewrites it. Seeding it with `obj_box` put + // the PRE-constructor instance address in unrooted memory for the whole + // body; on fall-through (no explicit `return`) `js_ctor_return_override` + // then saw an *object* in `raw` and returned THAT — the stale address — + // discarding the re-read `obj_box` the reload below just recovered. The + // instance-root fix was defeated at its last instruction. + // + // `undefined` is exactly equivalent for every path and carries no address: + // - fall-through → `raw` is undefined → the override yields `this_val`, + // i.e. the RE-READ instance (previously: `raw`, the + // stale one — same value only when nothing moved); + // - bare `return;` → the slot is untouched, so also `this_val`; + // - `return ` → `Stmt::Return` overwrote the slot; unchanged; + // - inherited-symbol ctor → the call's return value overwrote it; unchanged. let ctor_result_slot = ctx.func.alloca_entry(DOUBLE); - ctx.block().store(DOUBLE, &obj_box, &ctor_result_slot); + ctx.block().store( + DOUBLE, + &double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED)), + &ctor_result_slot, + ); let after_idx = ctx.new_block("ctor.return.after"); let after_label = ctx.block_label(after_idx); ctx.inline_ctor_return.push(crate::expr::InlineCtorReturn { diff --git a/crates/perry-codegen/src/stmt/mod.rs b/crates/perry-codegen/src/stmt/mod.rs index 7885e33520..ae65b69164 100644 --- a/crates/perry-codegen/src/stmt/mod.rs +++ b/crates/perry-codegen/src/stmt/mod.rs @@ -319,8 +319,12 @@ pub(crate) fn lower_stmt(ctx: &mut FnCtx<'_>, stmt: &Stmt) -> Result<()> { } Stmt::Return(None) => { // Inside an inlined constructor body, a bare `return;` keeps the - // implicit `this` (the result slot already holds it) and jumps to - // the shared after-block — never a function-level `ret`. + // implicit `this` and jumps to the shared after-block — never a + // function-level `ret`. Leaving the result slot untouched is what + // expresses that: it holds `undefined`, and the construction + // completion's `js_ctor_return_override` maps `undefined` to the + // re-read `this` (#7154 — the slot is a plain alloca the collector + // does not rewrite, so it must never carry an instance address). if let Some(target) = ctx.inline_ctor_return.last().cloned() { for _ in 0..ctx.try_depth { ctx.block().call_void("js_try_end", &[]); diff --git a/crates/perry-codegen/tests/temp_root_operand_temporaries.rs b/crates/perry-codegen/tests/temp_root_operand_temporaries.rs index de25117738..8e70337ab0 100644 --- a/crates/perry-codegen/tests/temp_root_operand_temporaries.rs +++ b/crates/perry-codegen/tests/temp_root_operand_temporaries.rs @@ -855,3 +855,83 @@ fn a_class_that_runs_no_user_code_emits_no_instance_root() { rooting the instance would be pure TLS traffic:\n{ir}" ); } + +/// #7154 follow-up: the inline-constructor **result slot** must not be seeded +/// with the instance address. +/// +/// `ctor_result_slot` is a plain entry alloca — not a shadow slot, not a temp +/// root — so the collector neither marks nor rewrites it. Seeding it with the +/// pre-constructor `obj_box` meant that on fall-through (no explicit `return`) +/// `js_ctor_return_override` received an *object* in `raw` and returned THAT, +/// discarding the re-read instance the reload had just recovered. The +/// dominance fix was defeated at its last instruction, and +/// `the_new_instance_is_rooted_across_the_constructor_body` could not see it: +/// that test walks the FIRST operand, which is the re-read one. +/// +/// Sabotage check: restore the `store double %obj_box, ptr %ctor_result_slot` +/// seed in `lower_new_impl_inner` and this fails. +#[test] +fn the_inline_ctor_result_slot_never_carries_an_instance_address() { + let ir = String::from_utf8( + compile_module( + &module_with_new_running_ctor("new_inst_result_slot.ts"), + entry_opts(), + ) + .unwrap(), + ) + .expect("LLVM IR should be UTF-8"); + let f = init_ir(&ir); + + let override_line = f + .lines() + .find(|l| l.contains("call double @js_ctor_return_override")) + .unwrap_or_else(|| panic!("the return-override:\n{f}")); + // `call double @js_ctor_return_override(double %a, double %b, i32 N)` — + // `%b` is `raw`, the value loaded out of the result slot. + let args = override_line + .split_once("js_ctor_return_override(") + .expect("argument list") + .1; + let raw_reg = args + .split(", ") + .nth(1) + .and_then(|a| a.trim().strip_prefix("double %")) + .unwrap_or_else(|| panic!("no `raw` register operand in `{override_line}`")) + .trim_end_matches(')') + .to_string(); + + let raw_def = f + .lines() + .find(|l| l.trim_start().starts_with(&format!("%{raw_reg} = "))) + .unwrap_or_else(|| panic!("no definition of %{raw_reg} in:\n{f}")) + .to_string(); + assert!( + raw_def.contains("load double"), + "`raw` should be a load from the inline-ctor result slot, got `{raw_def}`:\n{f}" + ); + let slot = raw_def + .rsplit_once("ptr ") + .expect("the slot pointer operand") + .1 + .trim() + .to_string(); + + // Every store into that slot must be a constant. A register operand means + // an address the collector cannot rewrite is sitting in unrooted memory + // across the constructor body. + for line in f + .lines() + .filter(|l| l.trim_end().ends_with(&format!("ptr {slot}")) && l.contains("store ")) + { + let stored = line + .split_once("store double ") + .map(|(_, rest)| rest.split(',').next().unwrap_or("").trim().to_string()) + .unwrap_or_default(); + assert!( + !stored.starts_with('%'), + "the inline-ctor result slot is a plain alloca the collector does \ + not rewrite, so it must never be seeded with a heap address — \ + found `{line}` (#7154):\n{f}" + ); + } +} diff --git a/scripts/gc_root_dominance_check.py b/scripts/gc_root_dominance_check.py index b5266bcec4..be3e39e537 100755 --- a/scripts/gc_root_dominance_check.py +++ b/scripts/gc_root_dominance_check.py @@ -60,16 +60,50 @@ where this reports none. Exit code is 1 when any violation is reported, so it can gate. + +Gating +------ +A dominance gate that cannot fail is worse than none, so three things are +enforced rather than assumed (CLAUDE.md, "four ways a gate can be unable to +fail"): + +* **empty input is an error, not a pass.** No paths, a path that holds no + `.ll`, or a typo'd flag exits 2 with a message instead of printing + `violations: 0`. +* **the subject must be live.** `--min-files` / `--min-binds` assert that the + corpus actually contained modules and actually contained root stores. A green + verdict over zero binds proves nothing and is refused. +* **malformed IR is an error, not a silent skip.** A function body whose first + line is not a label used to parse to zero blocks and be dropped without a + word — a planted violation vanished and the run exited 0. Both that and a + label-shaped line the parser cannot read now raise. + +`--self-test` compiles two in-file fixtures — one with a planted violation of +exactly this class, one identical but with the root store hoisted above the +collection point — and asserts the checker reports the first and clears the +second. It is the "assert the gate can fail" arm, and CI runs it next to the +real corpus. """ +import argparse import os import re import sys +import tempfile from collections import defaultdict, deque # ---------------------------------------------------------------- IR parsing DEFINE_RE = re.compile(r"^define\s+.*?@([\w.$]+)\(") -LABEL_RE = re.compile(r"^([\w.$][\w.$]*):\s*$") +# A trailing `; preds = %a, %b` comment is LLVM's own printed form (`llvm-dis`, +# `opt -S`, `clang -S -emit-llvm`). Perry's writer never emits it, but pointing +# this tool at LLVM-canonical IR is the obvious thing to try, and rejecting the +# label there used to append it to the PREVIOUS block — collapsing the whole +# function into one and producing exactly the line-order false positives the +# docstring above says real dominance avoids. +LABEL_RE = re.compile(r"^([\w.$][\w.$]*):\s*(?:;.*)?$") +# Anything that ends in `:` and is not an instruction is label-SHAPED. If the +# strict form above declines it, that is a parser gap and must be loud. +LABEL_SHAPED_RE = re.compile(r"^[^\s=]+:\s*(?:;.*)?$") ASSIGN_RE = re.compile(r"^\s*%([\w.$]+)\s*=\s*(.*)$") CALL_RE = re.compile(r"\bcall\s+[^@]*@([\w.$]+)\(") BIND_RE = re.compile(r"call void @js_shadow_slot_bind\(i32 (\d+), ptr %([\w.$]+)\)") @@ -102,12 +136,20 @@ def __init__(self, name): self.preds = defaultdict(set) +class MalformedIR(Exception): + """The parser cannot model this IR, so any verdict over it would be a lie. + + Raised rather than skipped: a function the parser drops reports zero + violations, which is indistinguishable from a clean one. + """ + + def parse_file(path): funcs = [] cur = None curblk = None with open(path, "r", errors="replace") as fh: - for raw in fh: + for lineno, raw in enumerate(fh, 1): line = raw.rstrip("\n") m = DEFINE_RE.match(line) if m: @@ -118,6 +160,12 @@ def parse_file(path): if cur is None: continue if line.startswith("}"): + if not cur.blocks: + raise MalformedIR( + f"{path}:{lineno}: @{cur.name} has no basic-block label. " + "The parser has no way to build a CFG for it, and a " + "silent skip would report it clean." + ) cur = None curblk = None continue @@ -128,10 +176,24 @@ def parse_file(path): cur.blocks.append(curblk) cur.insns[curblk] = [] continue - if curblk is None: - continue if not line.strip(): continue + if LABEL_SHAPED_RE.match(line.strip()): + raise MalformedIR( + f"{path}:{lineno}: label-shaped line the parser cannot read: " + f"{line.strip()!r}. Appending it to the previous block would " + "merge two blocks and fabricate an intra-block path." + ) + if curblk is None: + # An instruction before the first label is LLVM's implicit + # entry block (`%0`). Perry's writer never emits one, but if it + # ever does, or if this is run over `llvm-dis` output, dropping + # the instructions would hide every violation in the entry + # block — which is where allocations live. + raise MalformedIR( + f"{path}:{lineno}: instruction before the first label in " + f"@{cur.name}: {line.strip()!r} (implicit entry block)." + ) cur.insns[curblk].append(Insn(line, curblk, len(cur.insns[curblk]))) for f in funcs: build_cfg(f) @@ -635,26 +697,208 @@ def scan(blk, lo, hi): return violations +# ------------------------------------------------------------- self-test --- +# +# The gate's own "can it fail?" arm. `PLANTED` is the #7186 shape: the instance +# is materialized, a call that allocates runs, and only THEN is the slot bound. +# `CLEAN` is byte-identical with the store/bind pair hoisted above the call. + +_SELFTEST_PLANTED = """\ +define double @perry_fn_selftest__late(double %a) { +entry.0: + %slot = alloca i64 + call void @js_shadow_frame_enter(i32 1) + %obj = call ptr @js_object_alloc(i32 4) + %ret = call double @js_call_function(double %a) + store ptr %obj, ptr %slot + call void @js_shadow_slot_bind(i32 0, ptr %slot) + ret double %ret +} + +define double @perry_fn_selftest__branchy(double %a, i1 %c) { +entry.0: + %slot = alloca i64 + call void @js_shadow_frame_enter(i32 1) + %arr = call ptr @js_array_alloc(i32 8) + br i1 %c, label %if.then.1, label %if.merge.2 + +if.then.1: + %poll = call double @js_gc_loop_safepoint(double %a) + br label %if.merge.2 + +if.merge.2: + store ptr %arr, ptr %slot + call void @js_shadow_slot_bind(i32 1, ptr %slot) + ret double %a +} +""" + +_SELFTEST_CLEAN = """\ +define double @perry_fn_selftest__early(double %a) { +entry.0: + %slot = alloca i64 + call void @js_shadow_frame_enter(i32 1) + %obj = call ptr @js_object_alloc(i32 4) + store ptr %obj, ptr %slot + call void @js_shadow_slot_bind(i32 0, ptr %slot) + %ret = call double @js_call_function(double %a) + ret double %ret +} +""" + +_SELFTEST_MALFORMED = """\ +define double @perry_fn_selftest__nolabel(double %a) { + %slot = alloca i64 + %obj = call ptr @js_object_alloc(i32 4) + %ret = call double @js_call_function(double %a) + store ptr %obj, ptr %slot + call void @js_shadow_slot_bind(i32 0, ptr %slot) + ret double %ret +} +""" + + +def _scan(paths, moving_only, anchor): + """(violations, n_binds) over `paths`.""" + parsed = [(os.path.basename(p), parse_file(p)) for p in sorted(paths)] + poll_reaching, _known = compute_poll_reaching( + [f for _m, fs in parsed for f in fs]) + 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) + ) + found = [ + (mod, v) + for mod, fs in parsed + for f in fs + for v in check_func(mod, f, moving_only, poll_reaching, anchor) + ] + return found, binds + + +def self_test(): + """Assert the checker reports the planted violation and clears the control. + + Returns 0 on success. A gate that never demonstrates a failure is a gate + that has not been shown to work. + """ + ok = True + with tempfile.TemporaryDirectory() as td: + planted = os.path.join(td, "planted.ll") + clean = os.path.join(td, "clean.ll") + broken = os.path.join(td, "broken.ll") + for p, text in ( + (planted, _SELFTEST_PLANTED), + (clean, _SELFTEST_CLEAN), + (broken, _SELFTEST_MALFORMED), + ): + with open(p, "w") as fh: + fh.write(text) + + found, binds = _scan([planted], False, "alloc") + if len(found) != 2: + print(f"self-test FAIL: planted fixture -> {len(found)} violations, " + "expected 2 (same-block and cross-block forms)", file=sys.stderr) + ok = False + if binds != 2: + print(f"self-test FAIL: planted fixture -> {binds} binds, expected 2", + file=sys.stderr) + ok = False + if ok and not all(v.moving for _m, v in found): + print("self-test FAIL: both planted violations reach a moving minor " + "(js_call_function / js_gc_loop_safepoint) and must be " + "classified MOVING", file=sys.stderr) + ok = False + + found, binds = _scan([clean], False, "alloc") + if found: + print(f"self-test FAIL: control fixture -> {len(found)} violations, " + "expected 0", file=sys.stderr) + ok = False + if binds != 1: + print(f"self-test FAIL: control fixture -> {binds} binds, expected 1", + file=sys.stderr) + ok = False + + try: + _scan([broken], False, "alloc") + except MalformedIR: + pass + else: + print("self-test FAIL: a function with no basic-block label must " + "raise MalformedIR, not parse to zero blocks and report clean", + file=sys.stderr) + ok = False + + print("self-test OK" if ok else "self-test FAILED") + return 0 if ok else 1 + + def main(): - args = [a for a in sys.argv[1:] if not a.startswith("-")] - moving_only = "--moving-only" in sys.argv - anchor = "any" if "--any-def" in sys.argv else "alloc" - verbose = "-v" in sys.argv + ap = argparse.ArgumentParser( + description=__doc__.splitlines()[0], + formatter_class=argparse.RawDescriptionHelpFormatter, + ) + ap.add_argument("paths", nargs="*", help=".ll files or directories to scan") + ap.add_argument("--moving-only", action="store_true", + help="keep only violations whose window reaches a moving minor") + ap.add_argument("--any-def", action="store_true", + help="anchor on any call result, not just allocation intrinsics") + ap.add_argument("-v", "--verbose", action="store_true") + ap.add_argument("--self-test", action="store_true", + help="run the built-in planted/clean fixtures and exit") + ap.add_argument("--min-files", type=int, default=1, metavar="N", + help="fail unless at least N .ll files were scanned (default 1)") + ap.add_argument("--min-binds", type=int, default=1, metavar="N", + help="fail unless at least N root stores were seen (default 1). " + "A clean verdict over zero root stores proves nothing.") + ns = ap.parse_args() + + if ns.self_test: + return self_test() + + moving_only = ns.moving_only + anchor = "any" if ns.any_def else "alloc" + verbose = ns.verbose paths = [] - for a in args: + for a in ns.paths: if os.path.isdir(a): for root, _dirs, files in os.walk(a): for fn in files: if fn.endswith(".ll"): paths.append(os.path.join(root, fn)) - else: + elif os.path.isfile(a): paths.append(a) + else: + print(f"error: no such file or directory: {a}", file=sys.stderr) + return 2 + + # An empty corpus is a misconfigured run, never a pass. `--trace llvm` + # silently produces nothing for a failed compile, and `PERRY_SAVE_LL` is not + # written for modules that codegen splits into units, so "the directory is + # empty" is a routine outcome rather than an exotic one. + if len(paths) < ns.min_files: + print(f"error: scanned {len(paths)} .ll file(s), need at least " + f"{ns.min_files}. Nothing was checked.", file=sys.stderr) + return 2 parsed = [] for p in sorted(paths): parsed.append((os.path.basename(p), parse_file(p))) poll_reaching, _known = compute_poll_reaching( [f for _m, fs in parsed for f in fs]) + 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) + ) total = 0 moving_total = 0 @@ -681,12 +925,22 @@ def main(): ) if verbose: print("\n".join(out)) - print(f"=== files: {len(paths)} violations: {total}" + print(f"=== files: {len(paths)} root stores: {n_binds} violations: {total}" f" (moving-minor reachable: {moving_total})") for k, n in sorted(per_kind.items(), key=lambda kv: -kv[1]): print(f" {n:6d} ({per_kind_moving.get(k, 0):5d} moving) {k}") + if n_binds < ns.min_binds: + print(f"error: {n_binds} root store(s) in the corpus, need at least " + f"{ns.min_binds}. 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).", file=sys.stderr) + return 2 return 1 if total else 0 if __name__ == "__main__": - sys.exit(main()) + try: + sys.exit(main()) + except MalformedIR as exc: + print(f"error: {exc}", file=sys.stderr) + sys.exit(2) From 79db46d77f3b5fb73742f511aaf6c525f00f51ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 1 Aug 2026 18:11:15 +0200 Subject: [PATCH 2/2] docs(changelog): follow-up fragment, and correct #7192's ObjectSpread reachability claim #7192's fragment is restored to what merged, minus nothing: the corrections and the follow-up's own entries move into a separate PR-keyed fragment, per changelog.d/README.md. The two corrections it records: * Expr::ObjectSpread is reachable ONLY from a JSX spread attribute (crates/perry-hir/src/jsx.rs:67, its sole construction site). Since #809 an object literal containing a spread lowers to a source-ordered IIFE built on js_object_assign_one, so { ...a, k: v } never reaches that arm. The claim that zod's 269-key spread runs through it is wrong. * "At least one more site of this class remains" is replaced by three named residuals with reproducers, two of them SIGSEGVs under PERRY_GC_MOVING_LOOP_POLLS=1, all reproduced at 73a9084ea (inherited, not caused) and all clean under the shipped default. --- .github/workflows/gc-root-dominance.yml | 4 +-- changelog.d/7192-root-store-dominance.md | 2 +- .../7198-root-store-dominance-followup.md | 25 +++++++++++++++++++ 3 files changed, 28 insertions(+), 3 deletions(-) create mode 100644 changelog.d/7198-root-store-dominance-followup.md diff --git a/.github/workflows/gc-root-dominance.yml b/.github/workflows/gc-root-dominance.yml index 8574fb4ca1..9774979c33 100644 --- a/.github/workflows/gc-root-dominance.yml +++ b/.github/workflows/gc-root-dominance.yml @@ -137,7 +137,7 @@ jobs: cp "$ll" "ir-corpus/${name}__$(basename "$ll")" done done - echo "corpus: $(ls ir-corpus | wc -l) .ll files" + echo "corpus: $(find ir-corpus -name '*.ll' | wc -l) .ll files" - name: Check root-store dominance run: | @@ -150,7 +150,7 @@ jobs: - name: Upload the IR corpus on failure if: failure() - uses: actions/upload-artifact@v4 + uses: actions/upload-artifact@v7 with: name: gc-root-dominance-ir path: ir-corpus diff --git a/changelog.d/7192-root-store-dominance.md b/changelog.d/7192-root-store-dominance.md index 31d17c925d..65bf31b367 100644 --- a/changelog.d/7192-root-store-dominance.md +++ b/changelog.d/7192-root-store-dominance.md @@ -4,7 +4,7 @@ - **`new C(…)` (`lower_call/new.rs`)** — the load-bearing one. The instance was a raw register while the constructor body ran. It is rooted inside the callee (the `this` parameter has a shadow slot), so the minor does not free it — it *moves* it and rewrites the callee's root, leaving the caller's register naming from-space. `js_gc_init_typed_shape_layout` then installed the layout descriptor on the abandoned copy and `js_ctor_return_override` published that dead address into the caller's shadow slot: a *rooted* slot holding a dangling pointer, which is exactly why #7154's from-space scan only ever saw offenders one or more cycles after the target died, with correct layout coverage. The instance is now temp-rooted across the constructor and re-read afterwards, on both the standalone-`_constructor` symbol path and the inline-ctor path. A class with no constructor, no fields and no heritage runs no user code in that window and keeps its previous IR exactly. - **`Expr::ObjectSpread` (`expr/logical_collections.rs`)** — `{ ...a, k: f() }` allocated the object and then wrote every field through a register held across each part's lowering, with no rooting at all. `Expr::Object` has used `RootedHandle` for this since #6951; the spread form never got it. - **`Expr::ClassExprFresh` (`expr/static_field_meta.rs`)** — same shape for the fresh class object built by a class-expression factory, across its static-field initializers, captured arguments, symbol statics and `static { … }` blocks. - - **property / element stores (`expr/property_set.rs`, `expr/index_set.rs`)** — `o.k = f()` and `o[k] = f()` evaluate the reference first and the value second (spec order), leaving the receiver in a register across `f()`. The *slot* it was loaded from is a root and gets rewritten; the register does not, so the store landed in from-space and the field never appeared on the object the program kept. This is #7114 with a receiver instead of a string literal. A new `temp_root::StoreOperandGuard` roots the receiver only when the value expression can collect, so stores with an inert RHS keep their previous IR. + - **property / element stores (`expr/property_set.rs`, `expr/index_set.rs`)** — `o.k = f()` and `o[k] = f()` evaluate the reference first and the value second (spec order), leaving the receiver in a register across `f()`. The *slot* it was loaded from is a root and gets rewritten; the register does not, so the store landed in from-space and the field never appeared on the object the program kept. This is #7114 with a receiver instead of a string literal. A new `temp_root::ReceiverGuard` roots the receiver only when the value expression can collect, so stores with an inert RHS keep their previous IR. `temp_root::temp_root_scope_begin` now takes the caller's extra reason to open a scope, because `new C()` with no arguments still needs a marker to cut the instance root against. diff --git a/changelog.d/7198-root-store-dominance-followup.md b/changelog.d/7198-root-store-dominance-followup.md new file mode 100644 index 0000000000..f52148dbb6 --- /dev/null +++ b/changelog.d/7198-root-store-dominance-followup.md @@ -0,0 +1,25 @@ +### Fixed + +- **codegen: three more root-store dominance holes, and one that defeated #7192's own fix at its last instruction (#7154 partial)**. Post-merge follow-up to #7192, adjudicating its review round reproducer-first. + - **the inline-constructor result slot (`lower_call/new.rs`)** — the load-bearing one. `ctor_result_slot` is a plain `alloca_entry`: not a shadow slot, not a temp root, never rewritten by the collector. Seeding it with `obj_box` parked the **pre**-constructor instance address in unrooted memory for the whole body, and on fall-through (no explicit `return`) `js_ctor_return_override` saw an *object* in `raw` and returned **that** — discarding the instance `reload_instance` had just re-read. #7192's dominance fix was defeated one instruction after it landed. It is **not** gated on `PERRY_INLINE_CTOR`: `force_ctor_call` requires `class.constructor.is_some()`, so any class with fields or heritage but no own constructor (`class C { payload = mk() }`, `class C extends B {}`) takes the inline path by default. And #7192's own regression test could not see it — `the_new_instance_is_rooted_across_the_constructor_body` walks the *first* operand of the override, which is the re-read one. The slot now starts at `undefined`, exactly equivalent on all four paths (fall-through, bare `return;`, `return `, inherited-symbol ctor) and carrying no address. It also removes a latent spurious `TypeError`: a *derived* class whose instance's GC type is outside `constructor_return_overrides_this`'s set previously fell through to "Derived constructors may only return object or undefined" on a plain fall-through. + - **the computed key of a property/element store (`expr/index_set.rs`, `expr/static_field_meta.rs`)** — `o[k] = f()` and a class expression's `[sym]: init` lower the key *before* the value, leaving it in an SSA register across exactly the window #7192 closed for the receiver. A non-literal string key is an ordinary heap string with no registered root of its own, so `unbox_str_handle` below the call would hand the setter a pre-move `StringHeader*`. Rooted with the same guard, pushed after the receiver's and released before it so the `temp_root_truncate` cuts nest. The guard family is renamed `ReceiverGuard`/`guard_store_receiver` → `StoreOperandGuard`/`guard_store_operand`, because rooting a key through something called "receiver" is the naming drift that let #7114's two predicates diverge. + - **`Expr::ObjectSpread` and `Expr::ClassExprFresh` protection predicates (`expr/logical_collections.rs`, `expr/static_field_meta.rs`)** — both were computed from the operand expressions alone, so a construct whose parts are all inert pushed no root even though the lowering itself emits a user-code boundary. A **spread part** now forces protection on its own (`js_object_copy_own_fields` reads every own key of the source, so an accessor there runs arbitrary JS *inside* the helper), and so does a **`static { … }` block** (its body is user code by definition) — which is why `block_fns` moves above `rooted_handle_begin`. Verified in the emitted IR: a class expression with one inert named static, one static block, no captures and no symbol statics answered `false` to every term of the old predicate and emitted no instance root at all; it now pushes one and re-reads it before each block call, before the capture snapshot and before the final `nanbox_pointer_inline`. + + The rest of both predicates stays byte-identical to `Expr::Object`'s, deliberately. An allocation inside a runtime helper provably cannot *initiate* a moving collection: `gc_check_trigger()`'s minor arm defers to the loop safepoint under `PERRY_GC_MOVING_LOOP_POLLS=1` (`gc/policy.rs`, `GC_SAFEPOINT_PENDING`), falls back to a conservative-scanned non-moving minor with polls off, and reaches a budgeted `MutatorAssist` step with `evacuation_policy_allowed = false` on the shipped default; C4b skips non-tenured nursery objects outright. "N `js_object_set_field_by_name` calls, therefore force the root" is not a sound reason, and forking these predicates away from `Expr::Object`'s on it would recreate the two-copies-of-one-decision shape that produced #7114. + +### Fixed (tooling) + +- **`scripts/gc_root_dominance_check.py` could not fail in four ways.** It is a gate, so it is now audited against all four (CLAUDE.md): it exited 0 on no arguments, on `--help`, on a typo'd flag and on a directory holding no `.ll` — and the corpus is *generated*, so "the trace directory is empty" is a routine outcome of a failed compile rather than an exotic one (now argparse + `--min-files`, exit 2). It had no liveness assertion, so a clean verdict over **zero root stores** was indistinguishable from a clean verdict over the real corpus (now `--min-binds`, with `root stores: N` printed in the summary so a green run carries its own evidence — this is the arm that catches a forgotten `PERRY_INLINE_SHADOW_SLOT=0`). A function whose body had no basic-block label parsed to **zero blocks and was silently skipped**: a planted violation vanished and the run exited 0 (now `MalformedIR`, as is any label-shaped line the strict regex declines, which used to be appended to the previous block and merge two blocks into a fabricated intra-block path). And `--self-test` now plants a violation of exactly this class — same-block and cross-block-through-a-diamond forms — and asserts the checker reports both, clears an otherwise-identical control, and raises on the malformed fixture. + + While there: LLVM's own printed label form (`if.then.1: ; preds = %entry.0`) is now accepted rather than mis-parsed, so pointing the tool at `llvm-dis` / `opt -S` output either works or says why not. The `%entry.implicit` synthetic block a reviewer described does not exist and was never the defect. + +- **`PERRY_SAVE_LL` / `--trace llvm` silently emitted nothing for split modules.** `codegen/mod.rs` forces `n_units = 1` only for `emit_ir_only`, and the `n_units > 1` path `return`s before the `PERRY_SAVE_LL` write — so every module past `MIN_CALLABLES_TO_SPLIT` (8000 callables) was absent from `.perry-trace/llvm`, i.e. exactly the largest modules, which is where a static IR audit most needs to look. The comment above it claimed the opposite. The split path now writes one `.unitN.ll` per codegen unit, at no extra peak because the units are already materialized there. A corpus that quietly omits its biggest members makes a clean verdict meaningless. + +### Added + +- **`gc-root-dominance` workflow** — runs the checker's `--self-test` first (fast structural failure before a compiler build), then emits an IR corpus from `test-files/` under `PERRY_GC_MOVING_LOOP_POLLS=1 PERRY_INLINE_SHADOW_SLOT=0` and checks it with both liveness floors. No `continue-on-error`, no `|| true`, no pipe between the checker and the job's exit status; `concurrency` cancels pull-request runs only so a busy `main` queue cannot starve it. **Deliberately not a required context yet** — a gate that has never been green blocks every open PR the day it is promoted; promote after a clean week on `main`. + +### Notes + +- #7192's fragment claimed `Expr::ObjectSpread` is where zod's 269-key spread lowers. It is not: since #809 an object literal containing a spread lowers to a source-ordered IIFE built on `js_object_assign_one` (`lower/expr_object.rs:844`), and `Expr::ObjectSpread`'s **sole** construction site is a JSX spread attribute (`crates/perry-hir/src/jsx.rs:67`). The fix is still right; its blast radius is JSX. +- Three residuals of this class remain, all reproduced at `73a9084ea` (before #7184/#7192, so inherited rather than caused) and all clean under the shipped default. They are why #7161 cannot be reverted yet: heap values in plain `alloca_entry` slots the collector never rewrites (the inline-ctor `this_slot`, the `[N x i64]` closure-capture staging array); `{ ...src, k: v }` with an accessor source, which **SIGSEGVs** (`exit=139`) under polls on the `js_object_assign_one` path; and a class expression with a `static { … }` block, which **SIGSEGVs** under polls because the value `js_static_this_arm_value` parks in the runtime's static-`this` cell is not rooted.