diff --git a/.github/workflows/gc-root-dominance.yml b/.github/workflows/gc-root-dominance.yml new file mode 100644 index 0000000000..9774979c33 --- /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: $(find ir-corpus -name '*.ll' | 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@v7 + 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/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. 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)