diff --git a/.github/workflows/gc-root-dominance.yml b/.github/workflows/gc-root-dominance.yml index 9774979c33..541f28c2c6 100644 --- a/.github/workflows/gc-root-dominance.yml +++ b/.github/workflows/gc-root-dominance.yml @@ -19,16 +19,37 @@ name: GC Root Dominance # # 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`; +# 2. NOT yet in branch protection's required contexts. This was deliberate at +# first — a new gate has never been green, so promoting it immediately +# blocks every open PR — but the second step was never taken, and in the +# meantime the job WAS red on `main` and blocked nothing: the five +# ClassExprFresh hits now tracked in #7211 have been reported on every run +# since #7198 and went unread. That is hazard 2 doing exactly what the +# corollary in CLAUDE.md warns about. +# +# **ACTION REQUIRED, and it is not something this workflow can do to +# itself**: with the allowlist below the job is green on `main`, so a +# repo admin must add `gc-root-dominance` to branch protection's required +# contexts. Until that happens this file is documentation, not a gate. +# See docs/src/internals/gc-rooting-invariant.md, "Promoting this gate". # 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. +# 4. the subject is ASSERTED live, not assumed, at three levels. +# `--self-test` proves the checker still reports a planted violation in +# hand-written IR and still clears the control. `--seeded-violations` +# goes further and plants collection points into the REAL corpus, +# requiring every one to be reported -- that is what catches the case +# where perry's emitted IR drifts to a shape the parser can no longer +# read, which frozen fixtures cannot detect. And `--min-files` / +# `--min-binds` / `--min-funcs` refuse a clean verdict over a corpus with +# too few modules, root stores or functions to have exercised anything. +# 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. +# +# Known-remaining violations live in scripts/gc_root_dominance_allowlist.json, +# one named entry each with an issue and a written justification -- NOT a +# numeric threshold, which cannot tell a new violation from an old one. An +# entry that matches nothing fails the build, so a fixed bug's entry must be +# deleted rather than left to widen coverage later. on: pull_request: @@ -93,60 +114,35 @@ jobs: || { echo "::error::target/release/$artifact was not produced"; exit 1; } done + # The source list and the env knobs live in the script, not here, so that + # reproducing a CI failure is one command rather than a re-read of this + # YAML. A retyped invocation that drops PERRY_GC_MOVING_LOOP_POLLS + # produces IR in which the bug is not expressible at all, and the local + # run then "cannot reproduce" a real finding. - 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" + run: ./scripts/gc_root_dominance_corpus.sh ir-corpus - 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. + # No pipe: the checker's own exit status is the job's. + # + # The floors are asserted, not hoped for. On the corpus as of this + # commit the run reports ~1993 functions / 117 modules / 2501 root + # stores, so these sit below that with room for churn and well above + # "something compiled". Raise them when the corpus grows; never lower + # one to make a run pass -- a shrinking corpus is the finding. + # + # --seeded-violations plants 40 collection points into this very IR + # and requires all 40 to be reported. That is the arm that fails if + # the checker has stopped understanding perry's output, which is the + # only way a green verdict here could be a lie. python3 scripts/gc_root_dominance_check.py ir-corpus \ - --moving-only --min-files 5 --min-binds 50 -v + --moving-only \ + --min-files 90 --min-binds 1500 --min-funcs 1200 \ + --allowlist scripts/gc_root_dominance_allowlist.json \ + --seeded-violations 40 \ + -v - name: Upload the IR corpus on failure if: failure() diff --git a/.gitignore b/.gitignore index 227196cd75..f1a078cbc8 100644 --- a/.gitignore +++ b/.gitignore @@ -40,6 +40,8 @@ benchmarks/suite/assets/ # `perry compile --trace llvm` dumps per-module .ll files here. .perry-trace/ +# scripts/gc_root_dominance_corpus.sh output (regenerate, never commit) +ir-corpus/ # Compiled test executables in the project root (no extension) /test_* diff --git a/CLAUDE.md b/CLAUDE.md index 29fa33cb2a..65580f82a7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -250,4 +250,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. +- **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`, closed by #7207; `--unrooted-allocas` is the detector for that shape, and its remaining hits are #7210's). 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. **Full writeup, every known shape and how to check your work: `docs/src/internals/gc-rooting-invariant.md`.** The CI gate is `gc-root-dominance.yml` over `scripts/gc_root_dominance_corpus.sh`; known-remaining hits are named one-per-entry in `scripts/gc_root_dominance_allowlist.json` (an entry that matches nothing FAILS, so a fix must delete its entry). A fifth shape is open as #7211: `ClassExprFresh` roots only when it thinks the static *initializers* collect, and never asks whether its own emitted `js_object_set_field_by_name` does — the sophisticated version of the mistake, where the author wrote a rooting predicate and it asked the wrong question. diff --git a/changelog.d/7212-gc-root-dominance-gate.md b/changelog.d/7212-gc-root-dominance-gate.md new file mode 100644 index 0000000000..1ddf161a7f --- /dev/null +++ b/changelog.d/7212-gc-root-dominance-gate.md @@ -0,0 +1,65 @@ +### CI: the GC root-dominance gate can now fail, and is baselined honestly + +The static root-dominance checker added in #7198 had been **red on `main` ever +since it merged**, and blocked nothing: `gc-root-dominance` is not in branch +protection's required contexts, so the job reported failure without being able +to turn a merge red. That is hazard 2 from CLAUDE.md's "four ways a gate can be +unable to fail", and the corollary it warns about — run a new gate once, *then* +promote it; leaving the second step undone is how hazard 2 happens. + +The five violations it had been reporting all along are real, and are now +tracked as #7211: `Expr::ClassExprFresh` roots its class object only when it +believes the static *initializers* can collect, and never asks whether the +lowering's own emitted `js_object_set_field_by_name` can. A class expression +whose statics are inert (`class C { static tag = tag }`) therefore holds the +object in a register across a collection point. `js_object_mark_class` does not +rescue it: that helper roots `CLASS_OBJECT_VALUES`' own copy, which keeps the +object alive and forwarded while leaving the register stale. Reachability is not +the invariant. + +**Corpus** (`scripts/gc_root_dominance_corpus.sh`, new) — emission moves out of +the workflow so that reproducing a CI failure is one command rather than a +re-read of the YAML; an invocation retyped without `PERRY_GC_MOVING_LOOP_POLLS=1` +produces IR in which the bug is not expressible at all. Grown from 41 to 117 +`.ll` files / 1993 functions / 2501 root stores over 99 sources, selected for the +lowerings this invariant runs through. A stale glob is a hard error and the +compiled-source count has an explicit floor. + +**Allowlist** (`scripts/gc_root_dominance_allowlist.json`, new) — one named entry +per known-remaining hit, each with an issue and a written justification, instead +of a numeric threshold. A threshold cannot distinguish a new violation from an +old one, and the cheapest way to green a red build is to raise it by one. The +checker enforces that an entry matching nothing **fails** (so a fix must delete +its entry — that is the ratchet), that an entry suppresses at most its `count`, +and that an unnamed violation fails regardless of the total. + +**Proof of failure** — `--seeded-violations N` splices synthetic collection +points into the *real* corpus IR between an allocation and its root store and +requires every one to be reported. `--self-test` only proves the checker fires on +frozen fixtures, which keeps passing even if perry's emitted IR drifts to a shape +the parser can no longer read; that is the case where the gate reports a serene +`violations: 0` over IR it is not analysing. `--self-test` additionally gained +arms covering the allowlist's anti-absorption properties. + +**Visibility** — `--min-funcs` plus a `checked N functions / M modules` summary +line, so a silently-empty or silently-shrunken run is impossible to mistake for a +clean one. + +Verified: green on `main` with the allowlist; red without it; red with any single +entry removed or its `count` lowered; exit 2 on a stale entry or an empty corpus; +40/40 seeded violations caught. Corpus emission ~80s, the check ~3s. + +**Still required, and not something the workflow can do to itself:** a repo admin +must add `gc-root-dominance` to branch protection's required contexts. + +### Docs + +- `docs/src/internals/gc-rooting-invariant.md` — the rule stated plainly for + codegen authors, with all five real bugs as case studies, the symptom each + produces, and how to check your work. Includes the false-green caveat on + `PERRY_GC_PROTECT_FROMSPACE_DEPTH` (the default of 4 is not enough; use 800). +- `docs/src/internals/rfc-rooting-by-construction.md` — design proposal for + making the bug unrepresentable: V8's `Handle`/`HandleScope` discipline + expressed through Rust's borrow checker, so that using an unrooted value + across a collection point is a compile error. Four of the five real bugs would + be caught by construction. Proposal only; nothing implemented. diff --git a/changelog.d/7214-closure-calln-stale-registers.md b/changelog.d/7214-closure-calln-stale-registers.md new file mode 100644 index 0000000000..82c22a04a3 --- /dev/null +++ b/changelog.d/7214-closure-calln-stale-registers.md @@ -0,0 +1,143 @@ +### Fixed + +- **The generic dynamic-call lowering no longer holds its callee, its `this` + receiver or its already-lowered arguments in bare registers across the + argument list.** `js_closure_callN` is the central dispatch path — `f(g())`, + `o.m(g())`, `curry(1)(2)`, every call whose callee is a value rather than a + statically resolved function — and it held **three** classes of GC value in + SSA registers across work that can collect. This is the site #7206 named and + deliberately left open, and the last known instance of #7192's + root-store-dominance class. + + An SSA register is not a GC root. Under `PERRY_GC_MOVING_LOOP_POLLS=1` a + back-edge poll inside an argument runs an evacuating minor: each held value + *survives* — the capture cell, shadow slot or module global it was read from + is a root — and therefore **moves**. The collector rewrites that location; + the register keeps naming from-space. + + - **the callee**, held across the whole argument list. The checked unbox then + masks a pre-move address and `js_closure_callN` reads a closure header out + of abandoned memory: `TypeError: value is not a function`, the failure + shape #7154 has worn since #7184. + - **the `this` receiver**, held across the read of the callee off it *and* + the argument list. #7206 fixed this operand on the sibling + `js_native_call_method_by_id` dispatch; this is the same operand on the + generic one — the dispatch a closure-valued property takes (hono's + `RegExpRouter.match = match`, the #519 shape). + - **each already-lowered argument**, held across the arguments after it *and* + across the rebind unbox. + + The three live windows are different, so they are computed separately rather + than protected as one block: + + | operand | window | + |---|---| + | receiver | the callee read + every argument | + | callee | every argument | + | argument *i* | the arguments after *i* + the rebind unbox | + + That last window is the subtle one, and it is why this could not be a + copy of #7206's fix. `js_closure_unbox_callee_checked_rebind` calls + `clone_closure_rebind_this`, which **allocates** a replacement closure + (`closure/dynamic_props.rs:1040`) when the callee captures `this`. It sits + *below* the last argument and *above* `js_closure_callN`, so the arguments + are re-read below it — hence `RootedOperands::reread_one`, which re-reads one + operand at a caller-chosen point instead of re-reading the whole group at + one. Hoisting the unbox above the argument list would remove the window + instead, but its throw is observable and the spec evaluates arguments before + it. For the >16-arity path the argument stores into the stack buffer moved + below the unbox for the same reason: a stack buffer is not a root, so filling + it above an allocating rebind just freezes pre-move addresses one indirection + further out. + + The receiverless path takes `js_closure_unbox_callee_checked`, which is a tag + check and a mask and allocates nothing, so `f(x, y)` on inert operands emits + exactly the IR it emitted before. Temp roots, not re-lowering: re-lowering + the callee or receiver would observe an assignment made by an argument, which + is a miscompile rather than a rooting fix. + + Three new gap tests, one per held value, each red on the parent under a + genuine `POLLS=1` build and green after. **The flag is compile-time since + #7161 *and* runtime-armed (`gc_moving_loop_polls_enabled()`, + `gc/policy.rs:1759`) — setting only one of the two is a false green, and the + first cut of these tests passed 10/10 for exactly that reason.** + + | | parent | this change | + |---|---|---| + | `test_gap_gc_closure_call_callee_rooting.ts`, `POLLS=1` | `TypeError: value is not a function` **10/10** | `bad 0` **10/10** | + | `test_gap_gc_closure_call_this_rooting.ts`, `POLLS=1` | `TypeError: value is not a function` **10/10** | `bad 0` **10/10** | + | `test_gap_gc_closure_call_argument_rooting.ts`, `POLLS=1` | `TypeError: value is not a function` **10/10** | `bad 0` **10/10** | + | all three, `POLLS=1` + `PERRY_GEN_GC=0` | `bad 0` | `bad 0` 5/5 | + | all three, default (no polls) | `bad 0` | `bad 0` 5/5 | + + **Cost, measured over the 141-module `sfw-registry` corpus** — this is the + hottest call path in the compiler's output, so it was measured rather than + assumed. The `operand_protection` gate means an operand whose window cannot + collect emits nothing at all, which is why the delta is this small: + + | | before | after | delta | + |---|---|---|---| + | linked binary | 39,216,688 B | 39,233,200 B | **+0.042 %** | + | emitted IR | 1,999,570 lines | 2,001,607 lines | **+0.10 %** | + | `js_gc_temp_root_push` sites | 8,394 | 8,885 | +491 | + +### Changed + +- **`scripts/gc_root_dominance_check.py`: `js_closure_unbox_callee_checked` is + now in `NONCOLLECTING`**, citing `closure/unbox.rs:25` — it is a tag check on + the NaN-boxed callee and a low-48 mask, with no allocation, no user code and + no poll. It sits between every dynamic call's last argument and its + `js_closure_callN`, so its absence reported the entire argument list of every + 1-arg dynamic call as stale: **372 of the 729 fatal-sink hits were this one + false positive**, all of them marked `MOVING: no`. This is the checker's + stated one-sided discipline working as designed — a missing entry costs false + positives, never a missed bug — and it is why the raw before/after counts + below are quoted against the corrected list. + + `js_closure_unbox_callee_checked_rebind` is deliberately **not** added: it + allocates, and the fix above depends on it counting as a collection point. + +## Verification + +Over the 141-module `sfw-registry` corpus, fatal-sink slice, with the corrected +`NONCOLLECTING`: **231 → 205**. + +`cargo test -p perry-codegen`: failing set **identical to the parent** — +6 `loop_safepoint_purity` (#7161's default flip), 16 `native_proof_regressions`, +3 `native_proof_buffer_views`, 1 `shadow_slot_hygiene`, 1 +`typed_shape_descriptors`, all pre-existing and measured directly on the parent +commit rather than assumed. One `perry-codegen` lib unit test that is red on the +parent passes here. The bind-anchored gate reports the same single non-moving +residual #7192 left (`js_closure_alloc_with_captures_singleton`, 0 +moving-reachable). + +## What this does NOT close + +**`sfw-registry --help` under a genuine `POLLS=1` build is still red, so +#7161's stopgap stays.** Measured on this build, compiled *and* run with the +flag: **3/10 pass, 7/10 SIGSEGV**. Its default arm is clean **10/10**, so +nothing was traded away. The three fixed registers were real and are now +provably rooted, but they are not the last thing standing between the registry +and a clean evacuating minor. + +Two concrete leads for whoever picks this up, both found while fixing the above +and neither speculative: + +1. **`prev_this` in the same lowering is the same bug, unfixed.** + `js_implicit_this_set` returns the *previous* implicit `this`, read out of + the `IMPLICIT_THIS` cell — which `object/this_binding.rs:176` documents as a + scanned mutable root the collector rewrites. That value is then held in a + bare register across the entire user call and written back afterwards, so a + collection anywhere inside the callee makes the restore publish a from-space + pointer back into a root. It is invisible to the current checker on both + ends: `js_implicit_this_set` is not in `ROOT_READ_CALLS`, and it is not a + `RECEIVER_SINKS` fatal sink. Fixing it costs a temp root on every dynamic + call, which is why it was measured and left rather than folded in here. +2. **205 fatal-sink hits remain**, no longer dominated by any single class — + 37 `js_closure_call1`, 22 `js_closure_call2`, 18 + `js_closure_call_apply_with_spread`, 17 `js_array_spread_append`, 15 + `js_object_set_field_by_name`, 15 `js_array_concat`, and a long tail. The + spread path (`expr/call_spread.rs`) is the obvious next one: it is the same + dispatch family and was never touched by #7206 or this change. + +Refs #7154, #7206, #7192, #7198, #7184, #7161, #7114, #6951, #519. diff --git a/crates/perry-codegen/src/expr/temp_root.rs b/crates/perry-codegen/src/expr/temp_root.rs index 81bec6d35a..00bcaa3797 100644 --- a/crates/perry-codegen/src/expr/temp_root.rs +++ b/crates/perry-codegen/src/expr/temp_root.rs @@ -524,20 +524,42 @@ impl RootedOperands { operands: &[&Expr], ) -> anyhow::Result> { let mut out = Vec::with_capacity(self.values.len()); - for (i, original) in self.values.iter().enumerate() { - let value = match &self.slots[i] { - Some(idx) => { - let idx = idx.clone(); - temp_root_get_double(ctx, &idx) - } - None if self.reloadable[i] => super::lower_expr(ctx, operands[i])?, - None => original.clone(), - }; - out.push(value); + for i in 0..self.values.len() { + out.push(self.reread_one(ctx, operands, i)?); } Ok(out) } + /// Re-read ONE operand, at a point the caller picks. + /// + /// [`RootedOperands::reread`] re-reads the whole group at a single point, + /// which is right when one collection point separates the group from its + /// consumer. It is wrong when the operands are consumed by *different* + /// instructions with a collection point between them — the generic + /// dynamic-call lowering is exactly that shape (#7154): the callee and the + /// `this` receiver are consumed by `js_closure_unbox_callee_checked_rebind`, + /// that rebind CLONES a `this`-capturing closure and therefore allocates, + /// and only then does `js_closure_callN` consume the arguments. Re-reading + /// the arguments above the rebind would put them right back in the window + /// the roots exist to close. + /// + /// Same three cases as [`RootedOperands::reread`]; see its documentation. + pub(crate) fn reread_one( + &self, + ctx: &mut FnCtx<'_>, + operands: &[&Expr], + i: usize, + ) -> anyhow::Result { + Ok(match &self.slots[i] { + Some(idx) => { + let idx = idx.clone(); + temp_root_get_double(ctx, &idx) + } + None if self.reloadable[i] => super::lower_expr(ctx, operands[i])?, + None => self.values[i].clone(), + }) + } + /// True when this group actually pushed slots — the signal a caller uses to /// keep an eager unbox (and therefore its exact register numbering) on the /// unprotected path. diff --git a/crates/perry-codegen/src/lower_call/console_promise.rs b/crates/perry-codegen/src/lower_call/console_promise.rs index 4bc662bb1f..dba208d8ee 100644 --- a/crates/perry-codegen/src/lower_call/console_promise.rs +++ b/crates/perry-codegen/src/lower_call/console_promise.rs @@ -1257,6 +1257,65 @@ pub fn try_lower_closure_call_fallthrough( // throw an `at :` frame under `--debug-symbols`. `0` (and the // default build) → no emission, unchanged `` frame. let call_byte_offset = ctx.strings.pending_call_offset(); + + // #7154: this lowering held THREE classes of GC value in bare SSA + // registers across work that can collect, and a bare register is not a GC + // root. Under `PERRY_GC_MOVING_LOOP_POLLS=1` a back-edge poll inside any of + // that work runs an evacuating minor: each value SURVIVES (the capture + // cell, shadow slot or module global it was read from is a root) and + // therefore MOVES, the collector rewrites that location, and the register + // keeps naming from-space. + // + // * the CALLEE, held across the whole argument list. The checked unbox + // then masks a pre-move address and `js_closure_callN` reads a closure + // header out of abandoned memory — `TypeError: value is not a + // function`, the failure shape #7154 has worn since #7184. + // * the `this` RECEIVER, held across the read of the callee off it AND + // the argument list. #7206 fixed this operand on the sibling + // `js_native_call_method_by_id` dispatch; this is the same operand on + // the generic one. + // * each already-lowered ARGUMENT, held across the arguments that follow + // it and across the rebind unbox below. + // + // The three windows are NOT the same, which is why they are computed + // separately rather than protected as one block: + // + // receiver | the callee read + every argument + // callee | every argument + // argument | the arguments after it + the rebind unbox + // + // The rebind unbox is a collection point that only exists on the + // member-shaped path: `js_closure_unbox_callee_checked_rebind` calls + // `clone_closure_rebind_this`, which allocates a fresh closure + // (`closure/dynamic_props.rs:1040`) when the callee captures `this`. It + // sits BELOW the last argument and ABOVE `js_closure_callN`, so the + // arguments are re-read after it, not before — see + // `RootedOperands::reread_one`. The receiverless path takes + // `js_closure_unbox_callee_checked`, which is a tag check and a mask and + // allocates nothing, so `f(x, y)` on inert operands emits exactly the IR it + // emitted before this change. + // + // Temp roots, not re-lowering: re-lowering the callee or the receiver would + // observe an assignment made by an argument, which is a miscompile rather + // than a rooting fix (`temp_root::operand_is_reloadable`). + let arg_collects: Vec = args + .iter() + .map(|a| crate::expr::temp_root::expr_may_trigger_gc(ctx, a)) + .collect(); + let any_arg_collects = arg_collects.iter().any(|&c| c); + // Reading the callee off the receiver: a by-name property get walks a + // prototype chain and can run an accessor, so it is a collection point in + // the receiver's window (and only in the receiver's). + let callee_read_collects = crate::expr::temp_root::expr_may_trigger_gc(ctx, callee); + + // Operands are recorded in the order their values are produced — receiver, + // callee, then arguments — because `RootedOperands` roots each one BEFORE + // the next is lowered. Rooting a finished list afterwards is worse than + // doing nothing: by then an earlier operand may already have been swept and + // the push publishes a dangling pointer into a slot the collector scans. + let mut roots = crate::expr::temp_root::root_operands_begin(args.len() + 2); + let mut operand_exprs: Vec<&Expr> = Vec::with_capacity(args.len() + 2); + let prelowered_recv: Option<(String, String)> = if let Expr::PropertyGet { object, property, .. @@ -1271,23 +1330,43 @@ pub fn try_lower_closure_call_fallthrough( None }; - let method_recv: Option = if let Some((ref obj_v, _)) = prelowered_recv { - Some(obj_v.clone()) - } else if let Expr::PropertyGet { object, .. } = callee { - // Skip the method-binding when the receiver is a global, - // namespace import, or NativeModuleRef — those aren't - // user objects and shouldn't influence `this`. - if matches!( - object.as_ref(), - Expr::GlobalGet(_) | Expr::NativeModuleRef(_) | Expr::ExternFuncRef { .. } - ) { - None - } else { - Some(lower_expr(ctx, object)?) + // The receiver expression, when this call binds one. Skip the + // method-binding when the receiver is a global, namespace import, or + // NativeModuleRef — those aren't user objects and shouldn't influence + // `this`. (`receiver_must_eval_once` never matches those forms, so the + // prelowered arm cannot disagree with this test.) + let method_recv_expr: Option<&Expr> = match callee { + Expr::PropertyGet { object, .. } => { + if prelowered_recv.is_some() { + Some(object.as_ref()) + } else if matches!( + object.as_ref(), + Expr::GlobalGet(_) | Expr::NativeModuleRef(_) | Expr::ExternFuncRef { .. } + ) { + None + } else { + Some(object.as_ref()) + } } - } else { - None + _ => None, + }; + + let method_recv: Option = match method_recv_expr { + Some(obj_expr) => { + let v = match prelowered_recv { + Some((ref obj_v, _)) => obj_v.clone(), + None => lower_expr(ctx, obj_expr)?, + }; + // Rooted here, before the callee read below: nothing has collected + // between the lowering above and this push. + roots.push(ctx, obj_expr, &v, callee_read_collects || any_arg_collects); + operand_exprs.push(obj_expr); + Some(v) + } + None => None, }; + let recv_slot = method_recv.as_ref().map(|_| 0usize); + let callee_slot = if recv_slot.is_some() { 1 } else { 0 }; let recv_box = if let Some((ref obj_v, ref property)) = prelowered_recv { // Read `property` off the once-lowered receiver value via the @@ -1309,10 +1388,31 @@ pub fn try_lower_closure_call_fallthrough( } else { lower_expr(ctx, callee)? }; - let mut lowered_args: Vec = Vec::with_capacity(args.len()); - for a in args { - lowered_args.push(lower_expr(ctx, a)?); + roots.push(ctx, callee, &recv_box, any_arg_collects); + operand_exprs.push(callee); + + // The rebind unbox allocates (see the header above), and it sits between + // the last argument and `js_closure_callN`, so every argument's window + // includes it. Receiverless calls take the non-allocating unbox and keep + // their old IR. + let rebind_allocates = method_recv.is_some(); + for (i, a) in args.iter().enumerate() { + let v = lower_expr(ctx, a)?; + let collects = rebind_allocates || arg_collects[i + 1..].iter().any(|&c| c); + roots.push(ctx, a, &v, collects); + operand_exprs.push(a); } + + // Re-read the receiver and the callee HERE: below every argument, above the + // unbox that consumes them. Mandatory, not defensive — the temp-root slot + // is a MUTABLE root, so an evacuating cycle rewrites it and the register + // pushed beforehand is stale. + let method_recv: Option = match recv_slot { + Some(i) => Some(roots.reread_one(ctx, &operand_exprs, i)?), + None => None, + }; + let recv_box = roots.reread_one(ctx, &operand_exprs, callee_slot)?; + let prev_this: Option = if let Some(ref this_val) = method_recv { let blk = ctx.block(); Some(blk.call(DOUBLE, "js_implicit_this_set", &[(DOUBLE, this_val)])) @@ -1334,38 +1434,58 @@ pub fn try_lower_closure_call_fallthrough( // argument's location no longer shadows this one. Applies to both arity // branches below (the checked unbox throws in either). No-op default build. crate::expr::calls::emit_call_location_at(ctx, call_byte_offset); - let result = if lowered_args.len() <= 16 { + + // #5504: tag-check the callee before masking to a closure pointer. + // A non-callable value (number/string/bool/null/undefined) whose + // low-48 bits form an in-range address would otherwise be handed to + // `js_closure_callN` as a wild `*const ClosureHeader` and SIGSEGV on + // the header read. The checked unbox throws `TypeError: value is not + // a function` for any non-`POINTER_TAG` value. + // #6475: a member-shaped call (`o.m(args)`) must rebind an + // object-literal method's baked `this` capture slot to the receiver — + // the slot wins over the IMPLICIT_THIS cell set above, so a method + // inherited via `Object.setPrototypeOf(obj, proto)` otherwise runs + // with `this` bound to the proto literal (effect's Pipeable + // `TagClass.pipe(...)` composed against the wrong `this` and + // HttpApiBuilder.group returned a curried function instead of a + // Layer). The rebind variant is a no-op for closures that don't + // capture `this`, so plain functions and arrows are untouched; + // receiverless calls keep the plain checked unbox. + // + // #7154: this is also the collection point that sits between the arguments + // and the dispatch. `clone_closure_rebind_this` ALLOCATES a replacement + // closure when the callee captures `this`, so the arguments are re-read + // below it — hoisting the unbox above the argument list instead is not an + // option, because its throw is observable and the spec evaluates arguments + // before it. + let closure_handle = { let blk = ctx.block(); - // #5504: tag-check the callee before masking to a closure pointer. - // A non-callable value (number/string/bool/null/undefined) whose - // low-48 bits form an in-range address would otherwise be handed to - // `js_closure_callN` as a wild `*const ClosureHeader` and SIGSEGV on - // the header read. The checked unbox throws `TypeError: value is not - // a function` for any non-`POINTER_TAG` value. - // #6475: a member-shaped call (`o.m(args)`) must rebind an - // object-literal method's baked `this` capture slot to the receiver — - // the slot wins over the IMPLICIT_THIS cell set above, so a method - // inherited via `Object.setPrototypeOf(obj, proto)` otherwise runs - // with `this` bound to the proto literal (effect's Pipeable - // `TagClass.pipe(...)` composed against the wrong `this` and - // HttpApiBuilder.group returned a curried function instead of a - // Layer). The rebind variant is a no-op for closures that don't - // capture `this`, so plain functions and arrows are untouched; - // receiverless calls keep the plain checked unbox. - let closure_handle = if let Some(ref this_val) = method_recv { - blk.call( + match method_recv { + Some(ref this_val) => blk.call( I64, "js_closure_unbox_callee_checked_rebind", &[(DOUBLE, &recv_box), (DOUBLE, this_val)], - ) - } else { - blk.call( + ), + None => blk.call( I64, "js_closure_unbox_callee_checked", &[(DOUBLE, &recv_box)], - ) - }; + ), + } + }; + + // Re-read the arguments BELOW the unbox. `closure_handle` itself is a raw + // pointer in a register, but nothing between here and the dispatch can + // collect, so it needs no protection of its own. + let arg_base = callee_slot + 1; + let mut lowered_args: Vec = Vec::with_capacity(args.len()); + for i in 0..args.len() { + lowered_args.push(roots.reread_one(ctx, &operand_exprs, arg_base + i)?); + } + + let result = if lowered_args.len() <= 16 { let runtime_fn = format!("js_closure_call{}", lowered_args.len()); + let blk = ctx.block(); let mut call_args: Vec<(crate::types::LlvmType, &str)> = vec![(I64, &closure_handle)]; for v in &lowered_args { call_args.push((DOUBLE, v.as_str())); @@ -1377,6 +1497,12 @@ pub fn try_lower_closure_call_fallthrough( // variadic `js_closure_call_array(closure_i64, args_ptr, argc)`. This // mirrors the `js_native_call_value` marshaling used elsewhere in // lower_call. `args_ptr` is non-null here since argc > 16 > 0. + // + // #7154: the stores happen below the unbox now. A stack buffer is not a + // GC root, so filling it above an allocating rebind would freeze + // pre-move addresses into it — the same staleness one indirection + // further out. The stores have no observable effect, so moving them + // below the throw-capable unbox changes nothing else. let n = lowered_args.len(); let buf = ctx.func.alloca_entry_array(DOUBLE, n); let blk = ctx.block(); @@ -1384,35 +1510,6 @@ pub fn try_lower_closure_call_fallthrough( let slot = blk.gep(DOUBLE, &buf, &[(I64, &format!("{}", i))]); blk.store(DOUBLE, v, &slot); } - // #5504: tag-check the callee before masking to a closure pointer. - // A non-callable value (number/string/bool/null/undefined) whose - // low-48 bits form an in-range address would otherwise be handed to - // `js_closure_callN` as a wild `*const ClosureHeader` and SIGSEGV on - // the header read. The checked unbox throws `TypeError: value is not - // a function` for any non-`POINTER_TAG` value. - // #6475: a member-shaped call (`o.m(args)`) must rebind an - // object-literal method's baked `this` capture slot to the receiver — - // the slot wins over the IMPLICIT_THIS cell set above, so a method - // inherited via `Object.setPrototypeOf(obj, proto)` otherwise runs - // with `this` bound to the proto literal (effect's Pipeable - // `TagClass.pipe(...)` composed against the wrong `this` and - // HttpApiBuilder.group returned a curried function instead of a - // Layer). The rebind variant is a no-op for closures that don't - // capture `this`, so plain functions and arrows are untouched; - // receiverless calls keep the plain checked unbox. - let closure_handle = if let Some(ref this_val) = method_recv { - blk.call( - I64, - "js_closure_unbox_callee_checked_rebind", - &[(DOUBLE, &recv_box), (DOUBLE, this_val)], - ) - } else { - blk.call( - I64, - "js_closure_unbox_callee_checked", - &[(DOUBLE, &recv_box)], - ) - }; let argc = n.to_string(); blk.call( DOUBLE, @@ -1421,6 +1518,10 @@ pub fn try_lower_closure_call_fallthrough( ) }; + // Released AFTER the dispatch, not before: the dispatcher allocates while + // it reads these values. + roots.release(ctx); + if let Some(prev) = prev_this { ctx.block() .call(DOUBLE, "js_implicit_this_set", &[(DOUBLE, &prev)]); diff --git a/docs/src/SUMMARY.md b/docs/src/SUMMARY.md index 6c1d7678ac..9b5cc5cbdc 100644 --- a/docs/src/SUMMARY.md +++ b/docs/src/SUMMARY.md @@ -174,6 +174,8 @@ - [Memory Model](internals/memory-model.md) - [Explicit Memory Control](internals/explicit-memory.md) +- [The GC rooting invariant (codegen)](internals/gc-rooting-invariant.md) +- [RFC: rooting by construction](internals/rfc-rooting-by-construction.md) # Contributing diff --git a/docs/src/internals/gc-rooting-invariant.md b/docs/src/internals/gc-rooting-invariant.md new file mode 100644 index 0000000000..df8ee4beda --- /dev/null +++ b/docs/src/internals/gc-rooting-invariant.md @@ -0,0 +1,265 @@ +# The GC rooting invariant (codegen) + +Read this before you emit a call from a lowering. + +## The rule + +> **Any GC-managed value that is live across a collection point must be +> reachable from a root before that point.** +> +> A value read out of a root and held in an SSA register across a call **is not +> rooted**. It is a copy, and the collector cannot see copies. + +Perry's GC moves objects. When an evacuating minor runs, it walks the roots, +copies live objects to old-gen, and **rewrites every reference it can reach**. +Anything it cannot reach keeps the old address. That address now points into +from-space, which is about to be reused. + +A "collection point" is any of: + +- an allocation (`js_object_alloc`, `js_array_alloc`, `js_closure_alloc`, string + concatenation, boxing — anything that can take an arena block); +- a call that can allocate, which in practice means **almost every runtime + helper**. `js_object_set_field_by_name` allocates: it performs the keys-array + transition. `js_object_get_property` allocates: it can run a getter, which is + user code; +- `js_gc_loop_safepoint`, the back-edge poll (only emitted under + `PERRY_GC_MOVING_LOOP_POLLS=1`, off by default since #7161). + +The safe default is that **a call collects unless you have read the runtime +source and proved otherwise**. The checker described below encodes exactly this +bias: its `NONCOLLECTING` set is the only place a call is declared safe, and +every entry names the runtime line that proves it. + +## Why this class of bug is so expensive + +Every violation presents the same way and none of it points at the code that is +wrong: + +- the symptom is `TypeError: value is not a function`, or a SIGSEGV, **cycles + later and somewhere else** — wherever the stale pointer is finally + dereferenced; +- **no runtime GC probe can see it.** At the moment of the collection there is + nothing for the collector to find, so a from-space scan, a verify-roots pass + and a zeal run all come back clean. `PERRY_GC_VERIFY_EVACUATION` checks that + reachable slots were forwarded; it cannot check a register it does not know + exists; +- it is **invisible by default**, because the back-edge poll that triggers it is + off. A green default test run says nothing about this class. + +Four instances shipped in a single day. The detection lag, not the fix, was the +cost every time. + +## The four ways it has actually broken + +### 1. Slot index past the frame (#7184) + +The root store was emitted, and it looked right. But the slot index fell outside +the frame pushed by `js_shadow_frame_enter`, so `js_shadow_slot_bind` +bounds-checked it and **silently returned**. The value was never rooted; the IR +says it was. + +*Tell:* a `js_shadow_slot_bind(i32 N, …)` where `N >= the frame size`. There is +no diagnostic — the bind is a no-op by design, because a bounds-check that +panicked would be worse. + +### 2. Root store after a collecting call (#7192) + +The store was in-frame and correct, but emitted **after** a call that allocates. +Between the allocation and the store, the value lived only in a register. + +```llvm +%obj = call ptr @js_object_alloc(i32 4) +%ret = call double @js_call_function(double %a) ; can evacuate %obj +store ptr %obj, ptr %slot ; stores the OLD address +call void @js_shadow_slot_bind(i32 0, ptr %slot) ; roots a dangling pointer +``` + +*Tell:* the resulting slot is *rooted* and *dangling* at the same time, which is +why it survives every "is it rooted?" check. + +### 3. Method receiver across the argument list (#7206) + +The receiver was loaded out of its root, then the argument expressions were +lowered — each of which can allocate — and only then was the call emitted with +the receiver still in the register loaded before the arguments. + +*Tell:* a `load` from a root slot, followed by any lowering of a sub-expression, +followed by a use of the loaded register. **Re-read the root after every +collection point** instead of caching the load. + +### 4. Computed-read base across the key expression (#7206) + +`base[key]` — the base was materialized, then the *key* expression was lowered +(allocating a string, say), then the element read used the stale base. + +*Tell:* two operands where one is evaluated first and used last. + +### And the one that is still open (#7211) + +`Expr::ClassExprFresh` roots its class object only when it believes the static +*initializers* can collect: + +```rust +let protect_handle = !captured_args.is_empty() + || !symbol_statics.is_empty() + || !block_fns.is_empty() + || any_may_trigger_gc(ctx, named_statics.iter().map(|(_, v)| v)); +``` + +Every disjunct asks about code the *author* supplied. None asks whether the +lowering's **own emitted calls** collect — and the loop below unconditionally +emits one `js_object_set_field_by_name` per static, which does. So +`class C { static tag = tag }`, whose only initializer is an inert `LocalGet`, +takes `protect_handle == false` and goes stale. + +This one is worth internalising, because it is the sophisticated version of the +mistake: the author *did* think about rooting, wrote a predicate for it, and the +predicate asked the wrong question. + +> **`js_object_mark_class` does not save it.** That helper puts the object in +> `CLASS_OBJECT_VALUES`, which is a registered root and *is* forwarded. The +> object stays alive and the side table's copy stays correct — and the register +> is still stale, because the collector rewrote a different copy. +> +> **Reachability is not the invariant.** The invariant is that the register you +> are still going to use was rewritten. A side table roots *its* pointer, not +> yours. + +## How to check your work + +### 1. The static checker — run this one + +It is the only instrument that sees this class before it crashes. + +```bash +cargo build --release -p perry -p perry-runtime-static -p perry-stdlib-static +./scripts/gc_root_dominance_corpus.sh ir-corpus +python3 scripts/gc_root_dominance_check.py ir-corpus --moving-only \ + --allowlist scripts/gc_root_dominance_allowlist.json -v +``` + +It parses the emitted LLVM IR, builds per-function CFGs, computes real +Cooper/Harvey/Kennedy dominance, and reports any root store that does not +dominate a preceding collection point. It is one-sided: an unrecognised call +counts as collecting, so a gap in its model costs a false positive, never a +missed bug. + +For a single file you are iterating on: + +```bash +PERRY_GC_MOVING_LOOP_POLLS=1 PERRY_INLINE_SHADOW_SLOT=0 \ + ./target/release/perry compile mycase.ts -o /tmp/mycase --trace llvm +python3 scripts/gc_root_dominance_check.py .perry-trace/llvm -v +``` + +Both env knobs matter. `PERRY_GC_MOVING_LOOP_POLLS=1` is what puts +`js_gc_loop_safepoint` in the IR, which the `MOVING` classification keys on; +without it the corpus **cannot express the bug**. `PERRY_INLINE_SHADOW_SLOT=0` +makes every root store the `js_shadow_slot_bind` call form the checker anchors +on. + +`--stale-registers` (#7206) additionally catches values that are *never* rooted +— read out of a root and held in a register across a collection point. That is +the mode that found cases 3 and 4. + +`--unrooted-allocas` (#7207) covers the remaining shape, and is the one the +bind-anchored check is structurally blind to: the value lives in a plain +`alloca_entry` for its whole lifetime, so there is no `js_shadow_slot_bind` to +anchor on and a scan that starts from binds calls the function clean. It found +`lower_call/new.rs`'s inline-ctor `this_slot` independently of any runtime +probe. **The gate does not run this mode yet** — its remaining hits are the +caches, staging arrays and inlined-callee params tracked as #7210, and they are +deliberately not in the allowlist, which covers the bind-anchored shape only. +Run it by hand when you touch an `alloca_entry` site: + +```bash +python3 scripts/gc_root_dominance_check.py .perry-trace/llvm \ + --unrooted-allocas --moving-only -v +``` + +### 2. The runtime instruments — second, and mind the depth + +From #7196: + +- `PERRY_GC_ZEAL=1` — collect at every safepoint. Slow, thorough. +- `PERRY_GC_PROTECT_FROMSPACE=1` — `mprotect` from-space after evacuation so a + stale read faults immediately instead of reading plausible garbage. +- `PERRY_GC_FROMSPACE_SCAN_ABORT` — now actually runs. + +> **`PERRY_GC_PROTECT_FROMSPACE_DEPTH` defaults to 4, and that default produces +> FALSE GREENS.** Four levels of retained from-space is not enough to still be +> holding the block your stale pointer is in by the time it is dereferenced. +> **Use 800.** A clean run at the default depth means nothing. + +And remember the ceiling on all of these: if the collection happens while the +only copy is in a register, there is nothing at that moment for any runtime +probe to notice. These instruments catch the *consequence*, later. The static +checker catches the *cause*, now. + +## The CI gate + +`.github/workflows/gc-root-dominance.yml` runs the checker on every PR over a +versioned corpus of ~99 `test-files/` sources chosen for the lowerings they +exercise (class expressions, constructors, object/array literals, computed +reads, property stores, closures, dynamic dispatch). The whole job is a few +minutes; the check itself is about three seconds over ~2000 functions. + +It is built to be able to fail, against all four hazards in CLAUDE.md: + +- the checker's exit status is the job's — no `continue-on-error`, no pipe; +- `concurrency` cancels pull-request runs only, so `main` runs are never starved; +- `--min-files` / `--min-binds` / `--min-funcs` refuse a clean verdict over a + corpus too thin to have exercised anything, and the run prints + `checked N functions / M modules` so a silently-empty run is visible; +- `--self-test` proves it still fires on planted fixtures, and + `--seeded-violations 40` splices collection points into the **real** corpus IR + and requires all 40 to be reported — that is the arm that catches the checker + silently losing the ability to read perry's output. + +### The allowlist, and why it is not a number + +Known-remaining violations live in `scripts/gc_root_dominance_allowlist.json`, +one entry each with a fingerprint, an issue, and a written justification. + +A numeric threshold cannot tell a new violation from an old one: fix one bug, +introduce another, and the total is unchanged and the gate stays green. Worse, +under deadline the cheapest way to green a red build is to raise the number by +one, and nothing in the diff says what was conceded. + +So the checker enforces three properties: + +1. **an entry that matches nothing fails the build.** When you fix the bug, + delete the entry in the same PR. That is the ratchet, and it is why a fixed + bug cannot leave a tombstone that quietly widens coverage later. +2. **an entry suppresses at most its `count`.** A second violation of the same + shape in the same function is new, and fails. +3. **a violation with no entry fails**, regardless of how many entries exist. + +Adding an entry is a code-review event. Bumping a `count` to green a build is +the exact thing this file exists to prevent. + +### Promoting this gate + +**As of this writing the job is NOT in branch protection's required contexts**, +which means it cannot turn a merge red — hazard 2, and the reason the #7211 hits +sat unread on `main` from #7198 onward while the job was visibly failing. + +With the allowlist the job is green on `main`, so the remaining step is for a +repo admin to add `gc-root-dominance` to the required contexts. A workflow +cannot do this to itself. Until it is done, this is documentation. + +## Rules of thumb + +- **Root before you call, not after.** If a value must survive a call, its root + store belongs above the call, unconditionally. Do not predicate it on a + cleverness about which callees collect — that is bug #5. +- **Re-read the root after every collection point.** Never cache a load out of a + root slot across a call. `rooted_handle_get` exists for this. +- **Evaluate-then-allocate is the hazard.** Any lowering with two or more + operands where one is materialized before another is lowered needs the first + one rooted. +- **`--trace llvm` and read it.** Three seconds of the checker beats a day of + bisecting a `not a function` five cycles downstream. +- **When in doubt, root it.** A redundant shadow slot costs a store. A missing + one costs a day, and it costs it to whoever hits the crash, not to you. diff --git a/docs/src/internals/rfc-rooting-by-construction.md b/docs/src/internals/rfc-rooting-by-construction.md new file mode 100644 index 0000000000..3132f9d891 --- /dev/null +++ b/docs/src/internals/rfc-rooting-by-construction.md @@ -0,0 +1,248 @@ +# RFC: rooting by construction + +**Status:** proposal. Nothing in this document is implemented. +**Problem:** [The GC rooting invariant](gc-rooting-invariant.md) — #7154, #7184, +#7192, #7206, #7211. + +## The case + +Five instances of one bug in about a day. Each was found by a different means, +each took hours to localise, and each fix was two lines. The fixes are not the +cost; the *representability* is. Today a lowering author can write the wrong +thing, and nothing between their keyboard and a crash five GC cycles later +objects. + +The current defences are all detection, and they run at increasing distance from +the mistake: + +| defence | catches | latency | +|---|---|---| +| code review | what a reviewer happens to notice | minutes, unreliable | +| `gc_root_dominance_check.py` | dominance violations in emitted IR | one CI run | +| `PERRY_GC_ZEAL` / from-space protect | the *consequence*, if timing cooperates | a test run, flaky | +| a user's crash | everything, eventually | days | + +The static checker is a genuine improvement and should stay. But it is still a +post-hoc pass over generated artefacts: it tells you the IR you produced is +wrong, not that the code you wrote cannot produce it. V8 made the opposite +choice with `Handle` / `HandleScope` / `DisallowGarbageCollection`, and the +reason is instructive — V8 has far more GC-touching call sites than perry, and +manages them with a type discipline rather than with a linter. + +**The question this RFC answers: can perry's Rust codegen make an unrooted live +value across a collection point fail to compile?** + +Short answer: yes, for four of the five real bugs, with a change that is +mechanical but wide. + +## Why the type system is currently absent + +Perry's codegen represents an SSA value as a **`String`**: + +```rust +let obj = ctx.block().call(I64, "js_object_alloc", &[(I32, &tcid), (I32, &n)]); +// obj: String -- the register name, e.g. "%r10" +ctx.block().call_void("js_object_set_field_by_name", &[(I64, &obj), ...]); +``` + +`String` is `Clone`, has no lifetime, and carries no information about what it +holds or whether it is still valid. Every value in the emitter — an `i32` loop +counter, a `double`, a GC pointer, a slot index — has the same type. There is +nothing for a rule to attach to. That is the root cause of the *class*, as +distinct from the root cause of any one bug. + +There are ~2500 builder call sites (`~2080` `.call(`, `~416` `.call_void(`) +across 35 files in `crates/perry-codegen/src`. + +## Proposed API + +Three types and one rule. + +```rust +/// A register holding a GC-managed value that is NOT rooted. +/// +/// Borrows the emitter immutably. Not Clone, not Copy. +pub struct Raw<'e> { + reg: String, + _emitter: PhantomData<&'e Emitter>, +} + +/// A shadow-slot root. Outlives collection points; cannot be read directly. +pub struct Rooted { + slot: SlotIdx, // only obtainable from ShadowFrame::alloc_slot +} + +/// A register holding something the GC does not manage: i32, double, bool, +/// a slot index. Freely copyable, no lifetime. +pub struct Plain(String); +``` + +The whole design rests on **splitting the emitter's methods by whether they can +collect**: + +```rust +impl Emitter { + /// Cannot collect. Takes &self, so outstanding `Raw` handles stay valid. + pub fn emit_pure(&self, ...) -> Plain { ... } + + /// CAN collect. Takes &mut self, which ends every outstanding `Raw` borrow. + pub fn emit_call(&mut self, sig: CollectingCall, args: &[Arg]) -> Raw<'_> { ... } +} + +impl Rooted { + /// Re-read the slot. The returned Raw is valid until the next &mut emit. + pub fn get<'e>(&self, e: &'e Emitter) -> Raw<'e> { ... } +} + +impl<'e> Raw<'e> { + /// Consume this register into a root. The only way to make a Rooted. + pub fn root(self, e: &mut Emitter, frame: &mut ShadowFrame) -> Rooted { ... } +} +``` + +The rule falls out of the borrow checker with no new machinery: + +> A `Raw<'e>` holds a shared borrow of the emitter. Emitting anything that can +> collect requires `&mut`. Therefore **a `Raw` cannot be used across a +> collection point** — the compiler rejects it. + +```rust +let obj = e.emit_call(OBJECT_ALLOC, &[..]); // Raw<'_>, borrows e +e.emit_call(SET_FIELD, &[obj.arg(), ..]); // needs &mut e +let boxed = obj.nanbox(&e); // ERROR: obj borrows e, + // which is mutably + // borrowed above +``` + +The fix is the correct code, and it is the shortest path out of the error: + +```rust +let obj = e.emit_call(OBJECT_ALLOC, &[..]).root(&mut e, &mut frame); +e.emit_call(SET_FIELD, &[obj.get(&e).arg(), ..]); +let boxed = obj.get(&e).nanbox(&e); // re-read, correct +``` + +Note that `Rooted::get` returning a fresh `Raw<'e>` also enforces the *second* +half of the contract that `temp_root.rs` documents today in prose: **re-read +after every collection point**, never cache the load. A cached `Raw` simply does +not survive the next `&mut`. + +### Implementation note + +`emit_pure` taking `&self` while appending to the instruction buffer needs +interior mutability — a `RefCell>` inside `Emitter`. That is the one +piece of real machinery this design requires, and it is contained to the +builder. The `RefCell` is never held across a call into user code, so the +runtime borrow panics are not a practical hazard. + +`CollectingCall` vs pure is decided by a table with the same one-sided bias the +checker already uses: **a callee is collecting unless it appears in a +`NON_COLLECTING` list whose every entry names the runtime line that proves it.** +That list already exists, in `gc_root_dominance_check.py`. It should move into +Rust and become the single source of truth both consume. + +## Would it have caught the real bugs? + +| bug | shape | caught? | +|---|---|---| +| **#7192** root store after a collecting call | `%obj` used after `js_call_function` | **Yes.** `Raw` used after `&mut` emit — borrow error. | +| **#7206a** method receiver across the argument list | receiver loaded, args lowered, receiver used | **Yes.** Lowering an argument is an `&mut` emit; the receiver `Raw` is dead. Author must hold a `Rooted` and `get()` after. | +| **#7206b** computed-read base across the key expression | base materialized, key lowered, base used | **Yes.** Identical mechanism. | +| **#7211** `ClassExprFresh` predicate asks the wrong question | rooted only if *initializers* can collect | **Yes, and most valuably.** There is no predicate to get wrong: `js_object_set_field_by_name` is a `CollectingCall`, so the class object's `Raw` cannot survive the loop. The author is forced to `root()` — the cleverness that caused the bug becomes unexpressible. | +| **#7184** slot index outside the pushed frame | `js_shadow_slot_bind(i32 N)` with `N >= frame size` | **Partly.** Not a liveness bug, so the borrow checker is silent. It *is* fixed by construction if `SlotIdx` is only obtainable from `ShadowFrame::alloc_slot()` and the frame's `enter(n)` count is derived from the number allocated, rather than both being written by hand. That is a worthwhile companion change and is cheap. | + +Four of five by construction, the fifth by making the frame own its own slot +numbering. That is a strong enough result to justify the work. + +## Migration cost + +The honest number is large but the distribution is favourable. + +- **~2500 builder call sites**, 35 files. Most are *not* GC-managed: loop + counters, `double` arithmetic, NaN-box bit twiddling, slot indices. Those + become `Plain`, which is `Copy` and imposes nothing. A rough read of the call + sites suggests **300–500 genuinely handle GC pointers** — the ones in + `expr/`, `lower_call/`, and the object/array/closure literal paths. +- **8 `rooted_handle_begin` sites** exist today, so the *explicit* rooting + surface is currently tiny. That is the point: the sites that need rooting and + do not have it are the bugs. +- The work is mechanical and the compiler drives it: change a signature, follow + the errors. It does not require understanding each lowering, only the local + data flow the compiler points at. + +**Incremental path, which matters more than the total:** + +1. Land the types with an explicit, greppable escape hatch: + `Raw::from_untrusted_register(String)` / `Raw::into_untrusted_register()`. + Every un-migrated caller uses it. Zero behaviour change, zero risk. +2. Migrate one family at a time, highest-risk first: `expr/temp_root.rs`'s + clients, then `lower_call/*`, then the literal paths. Each is its own PR. +3. `#[deny]` the escape hatch per-module as each module finishes, so migrated + code cannot regress. +4. Keep `gc_root_dominance_check.py` in CI permanently as the backstop for + whatever still goes through the escape hatch — and as the check on the + `NON_COLLECTING` table itself, which the type system trusts and cannot + verify. + +Steps 1 and 2-for-one-family are a plausible next PR. There is no point at which +a half-migrated tree is worse than today's. + +## Performance + +- **Emitted code: identical.** These are compile-time wrappers over register + names; the IR is unchanged. +- **Compiler runtime: neutral to slightly negative.** `Raw` is a newtype over + `String`, so no extra allocation. The `RefCell` adds a borrow flag check per + `emit_pure`, which is noise next to the `format!` calls already in the + builder. +- **Compiler build time: slightly up**, from monomorphisation over the added + lifetime. `perry-codegen` is already one of the slow crates; this is worth + measuring before the wide migration, not assuming. +- **Risk of *more* rooting than today:** yes, and that is a real cost worth + naming. When the borrow checker forces a `root()`, the author will insert one + rather than reason about whether it was needed, and some will be redundant. + A redundant shadow slot is one store and one bind. Given that the alternative + is the bug this document exists about, that is the right trade — but it should + be measured on the benchmark suite after the first family migrates, not waved + through. + +## What it cannot catch + +Stating these plainly, because a safety mechanism believed to be total is worse +than one known to be partial: + +- **A miscategorised callee.** If a genuinely-allocating helper is listed in + `NON_COLLECTING`, the type system will cheerfully allow a `Raw` across it. The + table is trusted input. This is why the checker must stay: it derives its + verdict from the emitted IR, so the two failure modes are not correlated. +- **The escape hatch**, for as long as any caller uses it. +- **Runtime-side rooting.** `RuntimeHandleScope` in `perry-runtime` is a + separate discipline over hand-written Rust; nothing here touches it. +- **Anything interprocedural.** A lowering that returns a `Raw` to a caller that + then collects is caught only if the lifetime actually propagates — which it + does for direct returns, but not across a `String` boundary or a struct field + that erases the lifetime. +- **Correctness of the shadow frame itself** — that `enter(n)` matches the slots + used, that the frame is popped on every path including unwinds. The + `SlotIdx`-from-`alloc_slot` change addresses the first; the rest is separate. +- **Values rooted in a side table rather than a slot.** As #7211 shows, + `CLASS_OBJECT_VALUES` roots *its own copy* and leaves the register stale. The + type system would treat such a value as unrooted, which is the correct and + conservative answer — but it means some code that is arguably fine today will + be forced to add a slot. + +## Recommendation + +Adopt, incrementally, starting with step 1 and one family. The decisive argument +is #7211: an author who was *actively thinking about GC rooting*, who wrote a +four-clause predicate to decide whether to root, still got it wrong — because +the predicate asked about the user's expressions and not about the lowering's +own emitted calls. No amount of care or review reliably catches that. A type +that makes the value unusable after the call does, and it does so at the moment +the mistake is made rather than five GC cycles later in someone else's program. + +**Not prototyped here.** `crates/perry-codegen/src/expr/` and `lower_call/` are +under concurrent edit (#7206 and the `js_closure_callN` work), and a +proof-of-concept worth anything has to touch exactly those files. The right +sequencing is: land the CI gate, let the in-flight lowering fixes merge, then +open step 1 as its own PR against a quiet tree. diff --git a/scripts/gc_root_dominance_allowlist.json b/scripts/gc_root_dominance_allowlist.json new file mode 100644 index 0000000000..936b642598 --- /dev/null +++ b/scripts/gc_root_dominance_allowlist.json @@ -0,0 +1,51 @@ +{ + "_readme": [ + "Known-remaining GC root-dominance violations, one entry per hit.", + "", + "This is deliberately NOT a numeric threshold. A count cannot tell a new", + "violation from an old one: fix one bug, introduce another, and the total is", + "unchanged and the gate stays green. Each entry names a fingerprint, the", + "issue tracking it, and why it is being tolerated.", + "", + "Three rules the checker enforces, so this file cannot decay into a blanket", + "suppression:", + " 1. an entry that matches nothing FAILS the build. When the bug is fixed,", + " delete the entry in the same PR. That is the ratchet.", + " 2. an entry suppresses at most `count` hits. A second violation of the", + " same shape in the same function is new, and fails.", + " 3. a violation with no entry fails, no matter how many entries exist.", + "", + "Adding an entry is a code-review event. Bumping `count` to make a red build", + "green is the thing this file exists to prevent -- if the count went up, a", + "new violation was introduced.", + "", + "Fingerprint format: .ll::::->", + "Get it from the checker's own output (`-v` prints one per violation)." + ], + "entries": [ + { + "fingerprint": "test_gap_class_expr_identity__test_gap_class_expr_identity_ts.ll::main::js_object_alloc->js_object_set_field_by_name", + "issue": "#7211", + "count": 2, + "justification": "Expr::ClassExprFresh (expr/static_field_meta.rs:432) holds the fresh class object in an SSA register across the js_object_set_field_by_name calls that install its named statics. Its protect_handle predicate only asks whether the AUTHOR'S initializer expressions can collect, never whether the lowering's own emitted field-store can -- so a class expression with inert statics gets no temp root. Two evaluations in this module, hence count 2. Pre-existing on main and reported red by this same gate since #7198; not fixed here because crates/perry-codegen/src/expr/ is being actively edited under #7206." + }, + { + "fingerprint": "test_gap_class_expr_instance_fields__test_gap_class_expr_instance_fields_ts.ll::main::js_object_alloc->js_object_set_field_by_name", + "issue": "#7211", + "count": 1, + "justification": "Same ClassExprFresh predicate gap as the entry above, reached through a class expression carrying instance fields. Tracked as one fix in #7211; listed separately because the fingerprint is per-module and a per-module entry is what keeps a NEW violation in this module from being absorbed." + }, + { + "fingerprint": "test_gap_class_expr_new_instanceof__test_gap_class_expr_new_instanceof_ts.ll::main::js_object_alloc->js_object_set_field_by_name", + "issue": "#7211", + "count": 1, + "justification": "Same ClassExprFresh predicate gap as the entries above, reached via `new`/`instanceof` on a class-expression value. Note that js_object_mark_class does put this object in CLASS_OBJECT_VALUES, which is scanned and forwarded -- that keeps the OBJECT alive but does not rewrite the register, so the violation is real rather than a false positive." + }, + { + "fingerprint": "test_gap_class_expr_static_this__test_gap_class_expr_static_this_ts.ll::main::js_object_alloc->js_object_set_field_by_name", + "issue": "#7211", + "count": 1, + "justification": "Same ClassExprFresh predicate gap as the entries above, reached through a static method that reads `this`. Same fix in #7211 retires all four entries at once; the checker will then fail on the stale entries, which is the intended prompt to delete them." + } + ] +} diff --git a/scripts/gc_root_dominance_check.py b/scripts/gc_root_dominance_check.py index 9dcd084a18..a6214460d4 100755 --- a/scripts/gc_root_dominance_check.py +++ b/scripts/gc_root_dominance_check.py @@ -87,6 +87,7 @@ import argparse import contextlib import io +import json import os import re import sys @@ -257,6 +258,17 @@ def build_cfg(f): "js_array_length", # array/indexing.rs:537 "js_object_mark_class", "js_class_object_pin_parent", "js_new_target_get", "js_new_target_set", + # closure/unbox.rs:25 -- a tag check on the NaN-boxed callee and a low-48 + # mask. No allocation, no user code, no poll. It sits between every + # dynamic call's last argument and its `js_closure_callN`, so leaving it + # out reported the whole argument list of every 1-arg dynamic call as + # stale (372 of the 729 fatal sinks) with `MOVING: no`. + # + # `js_closure_unbox_callee_checked_rebind` is deliberately NOT here: it + # calls `clone_closure_rebind_this`, which allocates a replacement closure + # (closure/dynamic_props.rs:1040) when the callee captures `this`. That one + # IS a collection point, and #7154's fix re-reads the arguments below it. + "js_closure_unbox_callee_checked", # object/this_binding.rs:160 -- a thread-local cell swap "js_implicit_this_set", "js_implicit_this_get", "js_gc_note_slot_layout", "js_string_addref_if_heap_string", @@ -570,6 +582,30 @@ def movers(self): def moving(self): return bool(self.movers) + @property + def fingerprint(self): + """Identity of this violation for allowlist matching. + + Deliberately built from names, never from SSA registers, slot indices + or line numbers: `%12`, `slot 3` and "line 480" all move when an + unrelated lowering above them changes by one instruction, and a + fingerprint that churns is a fingerprint someone will replace with a + numeric threshold. What stays put across a recompile is *which + function*, *what it allocated*, and *what could collect before the + root store landed* -- which is also exactly the triple a human needs + to judge whether an entry is still the bug they signed off on. + + Coarser than one-per-instruction on purpose: two violations in the + same function with the same alloc/collector pair collapse to one + fingerprint, and the allowlist entry's `count` is what pins the + multiplicity. A third one appearing later therefore still fails. + """ + return "{}::{}::{}->{}".format( + self.module, self.func, self.alloc.callee or "?", + sorted({c.callee for c in self.collectors})[0] + if self.collectors else "?", + ) + def check_func(module, f, want_moving_only=False, poll_reaching=frozenset(), anchor_mode="alloc"): @@ -699,6 +735,337 @@ def scan(blk, lo, hi): return violations +# -------------------------------------------------------------- allowlist --- +# +# Why a file of named entries and not `--max-violations N`: +# +# A numeric threshold cannot tell a new violation from an old one. Fix one bug, +# introduce a different one, and the count is unchanged and the gate is green -- +# which is the failure mode this whole exercise exists to prevent. Worse, the +# number ratchets the wrong way under pressure: the cheapest way to make a red +# build green is to raise it by one, and nothing in the diff says what was +# conceded. +# +# So: one entry per known-remaining violation, each naming the issue that tracks +# it and carrying a written justification, and three properties that keep the +# file from decaying into a blanket suppression: +# +# * an entry that matches NOTHING is an error. When the bug is fixed the entry +# must be deleted in the same PR, so the allowlist cannot accumulate +# tombstones that quietly widen its coverage later. +# * an entry suppresses at most `count` violations. A second violation of the +# same shape in the same function is new, and fails. +# * a violation with no entry fails, regardless of how many entries exist. + +ALLOWLIST_MIN_JUSTIFICATION = 40 + + +class AllowlistError(Exception): + """The allowlist itself is malformed, stale, or under-documented.""" + + +class AllowEntry: + __slots__ = ("fingerprint", "count", "issue", "justification", "matched") + + def __init__(self, fingerprint, count, issue, justification): + self.fingerprint = fingerprint + self.count = count + self.issue = issue + self.justification = justification + self.matched = 0 + + +def load_allowlist(path): + """Parse and VALIDATE the allowlist. Every failure here is exit 2. + + Validation is strict because an allowlist is the one part of a gate whose + whole purpose is to make it not fail; a typo'd field that silently parses + to "suppress everything" would be indistinguishable from a working gate. + """ + try: + with open(path, "r") as fh: + doc = json.load(fh) + except FileNotFoundError: + raise AllowlistError(f"{path}: no such file") + except json.JSONDecodeError as exc: + raise AllowlistError(f"{path}: not valid JSON: {exc}") + + if not isinstance(doc, dict) or "entries" not in doc: + raise AllowlistError( + f"{path}: expected a JSON object with an \"entries\" array") + raw = doc["entries"] + if not isinstance(raw, list): + raise AllowlistError(f"{path}: \"entries\" must be an array") + + entries = {} + for i, e in enumerate(raw): + where = f"{path}: entries[{i}]" + if not isinstance(e, dict): + raise AllowlistError(f"{where}: must be an object") + for field in ("fingerprint", "issue", "justification"): + if not isinstance(e.get(field), str) or not e[field].strip(): + raise AllowlistError( + f"{where}: \"{field}\" is required and must be a " + "non-empty string") + count = e.get("count", 1) + if not isinstance(count, int) or isinstance(count, bool) or count < 1: + raise AllowlistError( + f"{where}: \"count\" must be a positive integer (got {count!r})") + # A justification of "known issue" is not a justification. The length + # floor is crude, but it is the difference between a reviewer having to + # read a sentence and having to go excavate the history themselves. + if len(e["justification"].strip()) < ALLOWLIST_MIN_JUSTIFICATION: + raise AllowlistError( + f"{where}: \"justification\" must be at least " + f"{ALLOWLIST_MIN_JUSTIFICATION} characters explaining WHY this " + "violation is known-safe or known-tracked. Suppressing a " + "GC-rooting violation without saying why is how the next one " + "gets waved through.") + fp = e["fingerprint"].strip() + if fp in entries: + raise AllowlistError( + f"{where}: duplicate fingerprint {fp!r}; merge the two entries " + "and set \"count\" instead, so the suppressed multiplicity is " + "stated in one place") + entries[fp] = AllowEntry(fp, count, e["issue"].strip(), + e["justification"].strip()) + return entries + + +def apply_allowlist(violations, entries): + """Split `violations` into (suppressed, remaining) and mark entries used. + + Over-quota violations land in `remaining`: an entry is a licence for a + stated number of hits, not for a shape. + """ + seen = defaultdict(int) + suppressed, remaining = [], [] + for v in violations: + fp = v.fingerprint + entry = entries.get(fp) + if entry is None: + remaining.append(v) + continue + seen[fp] += 1 + if seen[fp] <= entry.count: + entry.matched += 1 + suppressed.append(v) + else: + remaining.append(v) + return suppressed, remaining + + +def stale_entries(entries): + """Entries that matched nothing. Always an error -- see the header note.""" + return [e for e in entries.values() if e.matched == 0] + + +# --------------------------------------------------- seeded-violation proof --- +# +# `--self-test` proves the checker fires on HAND-WRITTEN IR. Necessary, not +# sufficient: the fixtures are frozen text, so they keep passing even if perry's +# emitted IR drifts to a shape the parser can no longer read -- at which point +# the checker reports a serene `violations: 0` over a corpus it is not actually +# analysing. That is hazard 4 (CLAUDE.md) wearing the self-test's clothes, and +# it is the failure mode this repo has hit most often. +# +# So the gate also seeds violations into the REAL corpus and requires every one +# to be caught. For each sampled site we take a value that IS correctly rooted +# today and splice a collection point into the gap between the allocation and +# its root store -- mechanically manufacturing #7192's exact shape out of code +# that is currently right: +# +# %v = call ptr @js_object_alloc(...) <-- real +# call double @js_call_function(...) <-- spliced in +# store ptr %v, ptr %slot <-- real +# call void @js_shadow_slot_bind(...) <-- real +# +# Only the collection point is synthetic. The function, its CFG, the allocation, +# the value's provenance chain through `or`/`bitcast`, the alloca, the store and +# the bind are all perry's own output, so the exercise covers the parser, the +# CFG builder, the provenance walk and the dominance computation against IR as +# it is actually emitted. Splicing rather than reordering also means a site +# always exists wherever a rooted allocation exists, instead of depending on the +# scheduler happening to leave a collecting call in a convenient place. +# +# If perry's IR stops having rooted allocations in the shape this matches, the +# gate goes red for "I could not seed a violation" rather than green for "I +# found none". + +_ANY_BIND_RE = re.compile( + r"^\s*call void @js_shadow_slot_bind\(i32 (\d+), ptr %([\w.$]+)\)") +_ANY_STORE_RE = re.compile( + r"^\s*store\s+[\w\[\]x* ]+?\s+%([\w.$]+),\s*ptr %([\w.$]+)") +_ANY_ASSIGN_RE = re.compile(r"^\s*%([\w.$]+)\s*=\s*(.*)$") +_ANY_CALL_RHS_RE = re.compile(r"^call\s+[^@]*@([\w.$]+)\(") + +# The spliced collection point. `js_call_function` is in POLL_CAPABLE_RUNTIME, +# so the planted violation is classified MOVING and survives `--moving-only` -- +# i.e. the proof is about the configuration the gate actually ships with, not a +# laxer one. The mutant only has to be parseable by this checker, never +# compiled, so no declaration is needed. +_SEED_CALL = " %gcseed.probe = call double @js_call_function(double 0.000000e+00)" + + +def _block_defs(lines, lo, hi): + """reg -> right-hand side, for definitions in lines[lo:hi].""" + defs = {} + for j in range(lo, hi): + m = _ANY_ASSIGN_RE.match(lines[j]) + if m: + defs[m.group(1)] = m.group(2) + return defs + + +def _reaches_alloc(defs, reg, limit=32): + """Does `reg` trace back to an allocation through transparent ops only? + + Mirrors `provenance()` deliberately: the seeded site has to be one the + checker's own anchoring would accept, otherwise a non-report is correct + behaviour being counted as a miss. + """ + seen = set() + q = deque([reg]) + while q and len(seen) < limit: + r = q.popleft() + if r in seen: + continue + seen.add(r) + rhs = defs.get(r) + if rhs is None: + continue + cm = _ANY_CALL_RHS_RE.match(rhs) + if cm: + if ALLOC_RE.match(cm.group(1)): + return True + continue + if any(op in rhs for op in TRANSPARENT_OPS): + q.extend(re.findall(r"%([\w.$]+)", rhs)) + return False + + +def _seed_sites(lines): + """Yield line indices at which to splice a collection point. + + A site is a `store`/`js_shadow_slot_bind` pair whose stored value traces + back to an allocation earlier in the same basic block, where: + + * the slot is bound exactly once in the enclosing function and never + `js_shadow_slot_set`, so it cannot already be active at the store -- + an already-active slot publishes through `bound_ptr` and the checker + correctly reports nothing; + * nothing in the gap roots the value another way, which would likewise + make a non-report correct. + + Both exclusions matter: without them the "misses" this test reports would + be the checker behaving properly, and the first person to see a red run + would learn to distrust it. + """ + fn_lo, fn_hi = 0, 0 + block_start = 0 + fn_text = "" + for i, line in enumerate(lines): + if line.startswith("define "): + fn_lo = i + fn_hi = next((k for k in range(i + 1, len(lines)) + if lines[k].startswith("}")), len(lines)) + fn_text = "\n".join(lines[fn_lo:fn_hi]) + block_start = i + 1 + continue + if LABEL_RE.match(line) or line.startswith("}"): + block_start = i + 1 + continue + if i < fn_lo or i >= fn_hi: + continue + bm = _ANY_BIND_RE.match(line) + if not bm: + continue + slot, alloca = bm.group(1), bm.group(2) + if i == 0: + continue + sm = _ANY_STORE_RE.match(lines[i - 1]) + if not sm or sm.group(2) != alloca: + continue + # the slot must not already be live at the store + if fn_text.count(f"js_shadow_slot_bind(i32 {slot}, ") != 1: + continue + if f"js_shadow_slot_set(i32 {slot}," in fn_text: + continue + defs = _block_defs(lines, block_start, i - 1) + if not _reaches_alloc(defs, sm.group(1)): + continue + # a rooting call already in the block would protect the mutant + if any(rc in lines[j] for j in range(block_start, i - 1) + for rc in ROOTING_CALLS): + continue + yield i - 1 + + +def _mutate(lines, at): + """Splice a collection point immediately above line `at`.""" + return lines[:at] + [_SEED_CALL] + lines[at:] + + +def seeded_violation_test(paths, moving_only, anchor, want_sites, verbose=False): + """Return 0 if every seeded violation was caught, non-zero otherwise.""" + caught = missed = sites = 0 + misses = [] + with tempfile.TemporaryDirectory() as td: + mutant = os.path.join(td, "mutant.ll") + for p in sorted(paths): + if sites >= want_sites: + break + with open(p, "r", errors="replace") as fh: + lines = fh.read().splitlines() + try: + base, _ = _scan([p], moving_only, anchor) + except MalformedIR: + continue + for at in _seed_sites(lines): + if sites >= want_sites: + break + with open(mutant, "w") as fh: + fh.write("\n".join(_mutate(lines, at)) + "\n") + try: + after, _ = _scan([mutant], moving_only, anchor) + except MalformedIR: + continue + sites += 1 + if len(after) > len(base): + caught += 1 + if verbose: + print(f" seeded {os.path.basename(p)}:{at + 1} -> caught") + else: + missed += 1 + misses.append(f"{p}:{at + 1}") + + print(f"=== seeded violations: {sites} planted, {caught} caught, " + f"{missed} MISSED") + if sites == 0: + print("error: could not seed a single violation into the corpus. The " + "mutator looks for a store/bind pair whose value traces back to " + "an allocation in the same block; finding none means perry's " + "emitted IR no longer has the shape this checker anchors on, so " + "a clean verdict from it would be meaningless.", file=sys.stderr) + return 2 + if sites < want_sites: + print(f"error: only {sites} seed site(s) found, wanted {want_sites}. " + "Too narrow a sample to claim the checker still fires.", + file=sys.stderr) + return 2 + if missed: + print("error: the checker did NOT report these seeded violations:", + file=sys.stderr) + for m in misses[:20]: + print(f" {m}", file=sys.stderr) + print("A planted collection point between an allocation and its root " + "store went unreported. The checker is not reading this IR the " + "way it thinks it is; a clean verdict from it means nothing " + "until this is explained.", file=sys.stderr) + return 1 + return 0 + + # ------------------------------------------------------------- self-test --- # # The gate's own "can it fail?" arm. `PLANTED` is the #7186 shape: the instance @@ -1257,7 +1624,7 @@ def _scan(paths, moving_only, anchor): if BIND_RE.search(ins.text) ) found = [ - (mod, v) + v for mod, fs in parsed for f in fs for v in check_func(mod, f, moving_only, poll_reaching, anchor) @@ -1307,7 +1674,7 @@ def self_test(): 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): + if ok and not all(v.moving for 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) @@ -1397,6 +1764,122 @@ def self_test(): f"bind for every alloca, so the unrooted check must report 0, " f"got {len(found)}", file=sys.stderr) ok = False + # ---- the allowlist must not be able to absorb a new violation ------ + # + # These arms exist because the allowlist is the only component whose + # job is to stop the gate failing. Every one of them is a way it could + # quietly become a blanket suppression. + planted_hits, _ = _scan([planted], False, "alloc") + fps = sorted({v.fingerprint for v in planted_hits}) + if len(fps) != 2: + print(f"self-test FAIL: planted fixture -> {len(fps)} distinct " + "fingerprints, expected 2 (the two functions differ, so the " + "fingerprint must distinguish them)", file=sys.stderr) + ok = False + + def _write_allowlist(entries): + p = os.path.join(td, "allow.json") + with open(p, "w") as fh: + json.dump({"entries": entries}, fh) + return p + + good_reason = ("tracked in the issue above; the receiver is rooted by " + "the caller one frame up, verified by hand") + + if len(fps) == 2: + # 1. Full coverage suppresses. + al = load_allowlist(_write_allowlist([ + {"fingerprint": fp, "issue": "#0000", + "justification": good_reason} for fp in fps])) + _sup, rem = apply_allowlist(planted_hits, al) + if rem or stale_entries(al): + print("self-test FAIL: an allowlist naming every planted " + "fingerprint should suppress all of them and go stale on " + "none", file=sys.stderr) + ok = False + + # 2. Partial coverage still fails -- the uncovered one is "new". + al = load_allowlist(_write_allowlist([ + {"fingerprint": fps[0], "issue": "#0000", + "justification": good_reason}])) + _sup, rem = apply_allowlist(planted_hits, al) + if len(rem) != 1: + print(f"self-test FAIL: one entry covering one of two " + f"violations left {len(rem)} unallowed, expected 1. An " + "allowlist that suppresses a violation it does not name " + "is a blanket suppression.", file=sys.stderr) + ok = False + + # 3. `count` is a quota, not a licence for the shape. Feed the same + # violation twice against a count of 1 and the second must + # survive -- this is the "new violation while the total is + # unchanged" case that a numeric threshold cannot see. + al = load_allowlist(_write_allowlist([ + {"fingerprint": fps[0], "issue": "#0000", "count": 1, + "justification": good_reason}])) + same = [v for v in planted_hits if v.fingerprint == fps[0]] + _sup, rem = apply_allowlist(same + same, al) + if len(rem) != 1: + print(f"self-test FAIL: count=1 against two hits of the same " + f"fingerprint left {len(rem)} unallowed, expected 1", + file=sys.stderr) + ok = False + + # 4. An entry that matches nothing is an error, so a fixed bug's + # tombstone cannot linger and widen coverage later. + al = load_allowlist(_write_allowlist([ + {"fingerprint": "nosuch::nosuch::js_object_alloc->js_x", + "issue": "#0000", "justification": good_reason}])) + apply_allowlist(planted_hits, al) + if not stale_entries(al): + print("self-test FAIL: an allowlist entry matching nothing must " + "be reported stale", file=sys.stderr) + ok = False + + # 5. Under-documented and malformed entries are refused outright. + for bad, why in ( + ([{"fingerprint": "a::b::c->d", "issue": "#1", + "justification": "known"}], "a too-short justification"), + ([{"fingerprint": "a::b::c->d", "justification": good_reason}], + "a missing issue"), + ([{"fingerprint": "a::b::c->d", "issue": "#1", "count": 0, + "justification": good_reason}], "count=0"), + ([{"fingerprint": "a::b::c->d", "issue": "#1", + "justification": good_reason}, + {"fingerprint": "a::b::c->d", "issue": "#2", + "justification": good_reason}], "a duplicate fingerprint"), + ): + try: + load_allowlist(_write_allowlist(bad)) + except AllowlistError: + pass + else: + print(f"self-test FAIL: {why} must be rejected", file=sys.stderr) + ok = False + + # ---- the corpus mutator must be able to manufacture a violation ---- + # (proves seeded_violation_test's machinery works even before it is + # pointed at real IR, so a CI run that finds no sites is a real signal + # about the IR rather than a broken mutator.) + with open(clean, "r") as fh: + clean_lines = fh.read().splitlines() + sites = list(_seed_sites(clean_lines)) + if not sites: + print("self-test FAIL: the mutator found no seed site in the clean " + "fixture, which is a rooted alloc followed by a collecting " + "call — the exact shape it exists to find", file=sys.stderr) + ok = False + else: + mutant = os.path.join(td, "mutant.ll") + with open(mutant, "w") as fh: + fh.write("\n".join(_mutate(clean_lines, sites[0])) + "\n") + got, _ = _scan([mutant], False, "alloc") + if not got: + print("self-test FAIL: sinking the root store below the " + "collecting call in the CLEAN fixture must produce a " + "violation; the mutator or the checker is broken", + file=sys.stderr) + ok = False print("self-test OK" if ok else "self-test FAILED") return 0 if ok else 1 @@ -1440,6 +1923,21 @@ def main(): "holds a heap value across a collecting call and is " "loaded below it, with no js_shadow_slot_bind anywhere. " "Disjoint from the bind-anchored check by construction.") + ap.add_argument("--min-funcs", type=int, default=1, metavar="N", + help="fail unless at least N functions were checked (default 1). " + "Files and binds can both look healthy while the corpus " + "is one module deep; this asserts breadth.") + ap.add_argument("--allowlist", metavar="PATH", + help="JSON file of known-remaining violations, one entry each " + "with an issue and a written justification. An entry that " + "matches nothing is an error, and an entry suppresses at " + "most its `count` hits -- so a NEW violation fails even " + "when the total is below some previous number.") + ap.add_argument("--seeded-violations", type=int, default=0, metavar="N", + help="after checking, plant N synthetic violations into the " + "real corpus IR and require every one to be reported. " + "Proves the checker can still fail against the IR perry " + "actually emits, not just against frozen fixtures.") ns = ap.parse_args() # A knob that is silently ignored is a disarmed knob: `--max-stale 0` @@ -1454,6 +1952,14 @@ def main(): if ns.self_test: return self_test() + allowlist = {} + if ns.allowlist: + try: + allowlist = load_allowlist(ns.allowlist) + except AllowlistError as exc: + print(f"error: {exc}", file=sys.stderr) + return 2 + moving_only = ns.moving_only anchor = "any" if ns.any_def else "alloc" verbose = ns.verbose @@ -1541,42 +2047,98 @@ def main(): return 2 return 1 if total else 0 - total = 0 - moving_total = 0 - per_kind = defaultdict(int) - per_kind_moving = defaultdict(int) - out = [] + n_funcs = sum(len(fs) for _m, fs in parsed) + + found = [] for mod, fs in parsed: for f in fs: - for v in check_func(mod, f, moving_only, poll_reaching, anchor): - total += 1 - per_kind[v.alloc.callee] += 1 - if v.moving: - moving_total += 1 - per_kind_moving[v.alloc.callee] += 1 - cs = sorted({c.callee for c in v.collectors}) - out.append( - f"{mod}::{v.func}\n" - f" alloc : {v.alloc.text.strip()}\n" - f" store : {v.store.text.strip()}\n" - f" bind : slot {v.slot} {v.bind.text.strip()}\n" - f" between: {', '.join(cs[:8])}" - f"{' (+%d more)' % (len(cs) - 8) if len(cs) > 8 else ''}\n" - f" MOVING : {('YES via ' + ', '.join(v.movers[:3])) if v.moving else 'no'}\n" - ) + found.extend(check_func(mod, f, moving_only, poll_reaching, anchor)) + + suppressed, remaining = apply_allowlist(found, allowlist) + + def render(v): + cs = sorted({c.callee for c in v.collectors}) + return ( + f"{v.module}::{v.func}\n" + f" alloc : {v.alloc.text.strip()}\n" + f" store : {v.store.text.strip()}\n" + f" bind : slot {v.slot} {v.bind.text.strip()}\n" + f" between: {', '.join(cs[:8])}" + f"{' (+%d more)' % (len(cs) - 8) if len(cs) > 8 else ''}\n" + f" MOVING : {('YES via ' + ', '.join(v.movers[:3])) if v.moving else 'no'}\n" + f" fingerprint: {v.fingerprint}\n" + ) + + total = len(found) + moving_total = sum(1 for v in found if v.moving) + per_kind = defaultdict(int) + per_kind_moving = defaultdict(int) + for v in found: + per_kind[v.alloc.callee] += 1 + if v.moving: + per_kind_moving[v.alloc.callee] += 1 + if verbose: - print("\n".join(out)) - print(f"=== files: {len(paths)} root stores: {n_binds} violations: {total}" - f" (moving-minor reachable: {moving_total})") + print("\n".join(render(v) for v in found)) + + # The breadth line CI reads. Printing functions and modules -- not just a + # violation count -- is what makes a silently-empty or silently-shrunken run + # visible in the log instead of indistinguishable from a clean one. + print(f"=== checked {n_funcs} functions / {len(parsed)} modules " + f"({len(paths)} .ll files, {n_binds} root stores)") + print(f"=== violations: {total} (moving-minor reachable: {moving_total})") + if allowlist: + print(f"=== allowlisted: {len(suppressed)} hit(s) across " + f"{len(allowlist)} entr(y/ies); unallowed: {len(remaining)}") 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}") + + # --- subject-liveness assertions, before any verdict is announced ------- 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 n_funcs < ns.min_funcs: + print(f"error: checked {n_funcs} function(s), need at least " + f"{ns.min_funcs}. The corpus compiled but is too thin to have " + "exercised the lowerings this invariant runs through.", + file=sys.stderr) + return 2 + + # --- allowlist hygiene -------------------------------------------------- + stale = stale_entries(allowlist) + if stale: + print("error: allowlist entries matched nothing:", file=sys.stderr) + for e in stale: + print(f" {e.fingerprint} ({e.issue})", file=sys.stderr) + print("Either the violation was fixed — delete the entry in the same " + "PR, that is the ratchet — or the corpus shrank and no longer " + "contains the module it names, which means this run checked less " + "than it claims to.", file=sys.stderr) + return 2 + + if remaining: + if not verbose: + print("\n".join(render(v) for v in remaining)) + print(f"error: {len(remaining)} GC root-dominance violation(s) not " + "covered by the allowlist.", file=sys.stderr) + print("A GC-managed value is live across a collection point without " + "being reachable from a root. See docs/src/internals/" + "gc-rooting-invariant.md. If this is genuinely known and tracked, " + "add an entry with an issue and a justification — never a count " + "bump.", file=sys.stderr) + return 1 + + # --- can this gate still fail? ------------------------------------------ + if ns.seeded_violations: + rc = seeded_violation_test(paths, moving_only, anchor, + ns.seeded_violations, verbose) + if rc: + return rc + + return 0 if __name__ == "__main__": diff --git a/scripts/gc_root_dominance_corpus.sh b/scripts/gc_root_dominance_corpus.sh new file mode 100755 index 0000000000..e06ba84714 --- /dev/null +++ b/scripts/gc_root_dominance_corpus.sh @@ -0,0 +1,147 @@ +#!/usr/bin/env bash +# +# Emit the LLVM IR corpus that scripts/gc_root_dominance_check.py gates on. +# +# This lives in a script rather than inline in the workflow so that the corpus +# CI checks and the corpus you check locally are the same corpus. The previous +# arrangement had the source list, the env knobs and the failure budget spelled +# out only in gc-root-dominance.yml, which made "reproduce the CI failure" +# mean "reread the YAML and retype it" -- and a retyped PERRY_GC_MOVING_LOOP_POLLS +# that gets dropped produces IR in which the bug is not expressible at all. +# +# ./scripts/gc_root_dominance_corpus.sh [OUTDIR] +# +# Requires target/release/perry plus the runtime archives (see the workflow, or +# `cargo build --release -p perry -p perry-runtime-static -p perry-stdlib-static`). +# +# Exit status is non-zero if fewer than MIN_COMPILED sources produced IR. A +# source that stops compiling is allowed -- test-files/ churns, and this gate is +# not the gap suite -- but the corpus is NOT allowed to quietly shrink to +# nothing, because "0 violations over 3 files" and "0 violations over 300 files" +# print the same verdict and mean opposite things. + +set -euo pipefail + +OUTDIR="${1:-ir-corpus}" +PERRY_BIN="${PERRY_BIN:-target/release/perry}" + +# Sources are chosen for the LOWERINGS they exercise, not for coverage of the +# language. Every one of #7154/#7184/#7192/#7206 was in one of these paths: +# +# gc/repsel - the rooting machinery itself, and representation selection +# (a value that changes representation changes who must root it) +# class - method receivers, `super()`, static initialisers. #7206's +# receiver-across-the-argument-list bug lives here. +# new - inline-constructor `this_slot`, the still-open alloca case +# object/ - object and array literals: long runs of element stores, each +# array one an allocation the accumulating literal must survive +# computed/ - computed reads. #7206's other bug is the base held in a +# index register across the key expression's own allocations +# prop - property stores +# static - static field initialisers, which run in a synthetic frame +# closure - captures, the js_closure_callN family +# dynamic - dynamic dispatch, where the receiver's type is not proven +# spread - spread/rest, which allocate per element +# map/set - collection literals +# +# Keep this list in sync with MIN_COMPILED below when you add a prefix. +PATTERNS=( + 'test_gap_gc_*.ts' + 'test_gap_repsel*.ts' + 'test_gap_class*.ts' + 'test_gap_new*.ts' + 'test_gap_object*.ts' + 'test_gap_array*.ts' + 'test_gap_computed*.ts' + 'test_gap_prop*.ts' + 'test_gap_static*.ts' + 'test_gap_closure*.ts' + 'test_gap_dynamic*.ts' + 'test_gap_map*.ts' + 'test_gap_set*.ts' +) + +# The floor, not a target. Raise it when the corpus grows; never lower it to +# make a run pass -- a shrinking corpus is the finding, not the obstacle. +MIN_COMPILED="${MIN_COMPILED:-90}" + +if [ ! -x "$PERRY_BIN" ]; then + echo "error: $PERRY_BIN not found or not executable." >&2 + echo " cargo build --release -p perry -p perry-runtime-static -p perry-stdlib-static" >&2 + exit 2 +fi + +shopt -s nullglob +sources=() +for pat in "${PATTERNS[@]}"; do + # shellcheck disable=SC2206 + matches=(test-files/$pat) + if [ "${#matches[@]}" -eq 0 ]; then + # Loud, because a stale pattern is how a corpus silently loses a whole + # lowering: the run stays green and nobody re-reads the glob list. + echo "::error::pattern '$pat' matched nothing; it is stale. Remove it or fix it." >&2 + exit 2 + fi + sources+=("${matches[@]}") +done + +if [ "${#sources[@]}" -eq 0 ]; then + echo "::error::no corpus sources matched any pattern; the glob list is stale" >&2 + exit 2 +fi + +rm -rf "$OUTDIR" +mkdir -p "$OUTDIR" + +compiled=0 +skipped=0 +skipped_names=() +scratch="$(mktemp -d)" +trap 'rm -rf "$scratch"' EXIT + +for src in "${sources[@]}"; do + name="$(basename "$src" .ts)" + rm -rf .perry-trace/llvm + # 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), so without it this corpus cannot express the bug at all. + # PERRY_INLINE_SHADOW_SLOT=0 makes every root store the @js_shadow_slot_bind + # call form; the #7088 inline diamond is equivalent but harder to anchor on. + if ! env PERRY_GC_MOVING_LOOP_POLLS=1 \ + PERRY_INLINE_SHADOW_SLOT=0 \ + PERRY_NO_AUTO_OPTIMIZE=1 \ + "$PERRY_BIN" compile "$src" -o "$scratch/$name" --trace llvm \ + >/dev/null 2>&1; then + skipped=$((skipped + 1)) + skipped_names+=("$name") + continue + fi + emitted=0 + for ll in .perry-trace/llvm/*.ll; do + cp "$ll" "$OUTDIR/${name}__$(basename "$ll")" + emitted=1 + done + if [ "$emitted" -eq 1 ]; then + compiled=$((compiled + 1)) + else + # Compiled but emitted no IR: --trace llvm is wired to codegen, so this + # means codegen did not run (a fully cached build). Counting it as a + # success would let a cache hit stand in for a check. + skipped=$((skipped + 1)) + skipped_names+=("$name (no .ll emitted)") + fi +done + +files="$(find "$OUTDIR" -name '*.ll' | wc -l | tr -d ' ')" +echo "corpus: $compiled/${#sources[@]} sources compiled, $skipped skipped, $files .ll files" +if [ "$skipped" -gt 0 ]; then + printf ' skipped: %s\n' "${skipped_names[*]}" +fi + +if [ "$compiled" -lt "$MIN_COMPILED" ]; then + echo "::error::only $compiled sources compiled, need at least $MIN_COMPILED." >&2 + echo "The corpus shrank. Either fix the sources that stopped compiling, or -- if" >&2 + echo "they were removed on purpose -- adjust PATTERNS and MIN_COMPILED together" >&2 + echo "in this file, in the same PR, so the reduction is reviewable." >&2 + exit 1 +fi diff --git a/test-files/test_gap_gc_closure_call_argument_rooting.ts b/test-files/test_gap_gc_closure_call_argument_rooting.ts new file mode 100644 index 0000000000..3a55ad5467 --- /dev/null +++ b/test-files/test_gap_gc_closure_call_argument_rooting.ts @@ -0,0 +1,52 @@ +// #7154: an already-lowered ARGUMENT of a dynamic value-call must be rooted +// across the evaluation of the arguments that follow it. +// +// `f(a, g())` lowers `a` into a bare SSA register and then lowers `g()`. `g()` +// allocates, and under `PERRY_GC_MOVING_LOOP_POLLS=1` a loop back-edge poll +// inside it runs an evacuating minor: `a` survives (the capture cell holding +// it is a root) and therefore MOVES, the collector rewrites the cell, and the +// register keeps naming from-space. `js_closure_callN` then passes the +// pre-move address as argument 0 and the callee reads its fields out of +// abandoned memory. +// +// This is the third register the generic dynamic-call lowering held across its +// own argument list; #7206 named all three and fixed none of them. +// +// ISOLATED ON PURPOSE. The callee is a module-level binding assigned a plain +// function declaration, so its function object is created once at module init +// and is long tenured by the time the loop runs — it is not what moves. `inst` +// is allocated fresh per iteration and is squarely in the nursery, so argument +// 0 is the operand under test. + +function churn(x: number): number { + const bits: any[] = []; + for (let i = 0; i < 600; i++) { + bits.push({ i: i, s: "x" }); + } + return x + bits.length - 600; +} + +function add(o: any, v: number): number { + return o.tag + v; +} + +const fn: any = add; + +function make(t: number): (p: number) => number { + const inst: any = { tag: t }; + return (p: number) => fn(inst, churn(p)); +} + +function run(): number { + let bad = 0; + for (let r = 0; r < 400; r++) { + const f = make(r); + const got = f(1); + if (got !== r + 1) { + bad++; + } + } + return bad; +} + +console.log("bad", run()); diff --git a/test-files/test_gap_gc_closure_call_callee_rooting.ts b/test-files/test_gap_gc_closure_call_callee_rooting.ts new file mode 100644 index 0000000000..13aef97b14 --- /dev/null +++ b/test-files/test_gap_gc_closure_call_callee_rooting.ts @@ -0,0 +1,50 @@ +// #7154: the CALLEE of a dynamic value-call must be rooted across the +// evaluation of the call's arguments. +// +// `f(g())` evaluates the callee first and the arguments second — spec order, +// and codegen follows it — which left the callee in a bare SSA register while +// `g()` was lowered. `g()` allocates, and under `PERRY_GC_MOVING_LOOP_POLLS=1` +// a loop back-edge poll inside it runs an evacuating minor. The callee +// SURVIVES that minor (the closure capture cell holding it is a root), which +// means it MOVES: the collector rewrites the capture cell but not the caller's +// register. `js_closure_unbox_callee_checked` then masks a from-space address +// and `js_closure_callN` reads its header out of abandoned memory — +// `TypeError: value is not a function`. +// +// Same invariant as #7206, #7192, #7184 and #7114, one operand over: a GC +// value's root must dominate every subsequent collection point, and a +// rewritten location is worthless unless the code below the collection point +// READS that location again. +// +// LIVE BY CONSTRUCTION. `fn` is an `any`-typed closure read out of a capture +// cell, so the call takes the generic `js_closure_callN` fallthrough rather +// than a static direct call, and the argument allocates hard enough to reach +// the collector. A non-moving collection cannot expose this, so the evacuating +// arms are the ones that bite. + +function churn(x: number): number { + const bits: any[] = []; + for (let i = 0; i < 600; i++) { + bits.push({ i: i, s: "x" }); + } + return x + bits.length - 600; +} + +function make(t: number): (p: number) => number { + const fn: any = (v: number): number => t + v; + return (p: number) => fn(churn(p)); +} + +function run(): number { + let bad = 0; + for (let r = 0; r < 400; r++) { + const f = make(r); + const got = f(1); + if (got !== r + 1) { + bad++; + } + } + return bad; +} + +console.log("bad", run()); diff --git a/test-files/test_gap_gc_closure_call_this_rooting.ts b/test-files/test_gap_gc_closure_call_this_rooting.ts new file mode 100644 index 0000000000..b8fc4c07ac --- /dev/null +++ b/test-files/test_gap_gc_closure_call_this_rooting.ts @@ -0,0 +1,54 @@ +// #7154: the `this` RECEIVER of a dynamic value-call must be rooted across the +// read of the callee off it and across the call's argument list. +// +// `recv.m(g())` where `recv.m` is a closure-VALUED property (not a native +// method) lowers through the generic `js_closure_callN` fallthrough. That path +// evaluates the receiver, reads the callee off it, lowers the arguments, and +// only then binds the receiver as the implicit `this` and hands it to +// `js_closure_unbox_callee_checked_rebind`. The receiver sat in a bare SSA +// register for that whole span: an evacuating minor inside `g()` rewrites the +// capture cell it was read from and leaves the register naming from-space, so +// the rebind clones captures out of abandoned memory and the body's `this.tag` +// reads garbage. +// +// #7206 fixed this same operand on the sibling `js_native_call_method_by_id` +// dispatch. This is the `js_closure_callN` one — the dispatch a closure-valued +// property takes (hono's `RegExpRouter.match = match`, the #519 shape). +// +// LIVE BY CONSTRUCTION. `m` is a non-arrow function declaration assigned onto +// an object literal, so it reads `this` through the implicit-this binding and +// the call takes the fallthrough rather than the by-name method dispatch. The +// receiver, the callee and the argument list are all in flight at once here — +// this lowering held all three in registers, and the three tests in this group +// name them one at a time. + +function churn(x: number): number { + const bits: any[] = []; + for (let i = 0; i < 600; i++) { + bits.push({ i: i, s: "x" }); + } + return x + bits.length - 600; +} + +function meth(this: any, v: number): number { + return this.tag + v; +} + +function make(t: number): (p: number) => number { + const inst: any = { tag: t, m: meth }; + return (p: number) => inst.m(churn(p)); +} + +function run(): number { + let bad = 0; + for (let r = 0; r < 400; r++) { + const f = make(r); + const got = f(1); + if (got !== r + 1) { + bad++; + } + } + return bad; +} + +console.log("bad", run()); diff --git a/test-parity/gc_repsel_corpus.txt b/test-parity/gc_repsel_corpus.txt index 2b953831e1..083b437818 100644 --- a/test-parity/gc_repsel_corpus.txt +++ b/test-parity/gc_repsel_corpus.txt @@ -232,3 +232,33 @@ test_gap_gc_closure_this_capture_rooting # the `requires=move` arms and prove nothing on `default`. test_gap_gc_method_receiver_rooting test_gap_gc_index_get_receiver_rooting + +# --- Stale REGISTERS on the generic dynamic call (#7214) --------------------- +# The same class as #7206 above, on the hottest call path the compiler emits. +# `js_closure_callN` held THREE classes of GC value in bare registers, each +# across a DIFFERENT window: +# +# receiver | the callee read off it + every argument +# callee | every argument +# argument | the arguments after it + the rebind unbox +# +# The rebind unbox is a collection point that exists only on the member-shaped +# path: `js_closure_unbox_callee_checked_rebind` calls +# `clone_closure_rebind_this`, which allocates a replacement closure when the +# callee captures `this`. It sits BELOW the last argument and ABOVE the +# dispatch, so the arguments are re-read after it. The receiverless path takes +# `js_closure_unbox_callee_checked` -- a tag check and a mask that allocates +# nothing -- so `f(x, y)` on inert operands emits byte-identical IR. +# +# Measured with #7206 applied but NOT #7214, i.e. these are exactly the sites +# #7206 left open. Compiled AND run with `PERRY_GC_MOVING_LOOP_POLLS=1`, +# oracle node 26.5.1 (`bad 0` for all three): +# closure_call_callee_rooting `TypeError: value is not a function` +# closure_call_this_rooting `TypeError: value is not a function` +# closure_call_argument_rooting `TypeError: value is not a function` +# All three are `bad 0` with #7214 applied, including under PERRY_GC_ZEAL=1, +# and clean on the shipped default on BOTH sides -- so they certify nothing on +# the `default` arm and belong with the `requires=move` rows. +test_gap_gc_closure_call_callee_rooting +test_gap_gc_closure_call_this_rooting +test_gap_gc_closure_call_argument_rooting