From 4e99c1bad79f542965f31b080cc62d77c203b4b0 Mon Sep 17 00:00:00 2001 From: John-David Dalton Date: Sat, 1 Aug 2026 21:01:36 -0400 Subject: [PATCH 1/5] fix(gc): root the interned typeof strings, the rawJSON key, and every saved implicit `this` Three unrooted-value bugs behind #7154's registry crash, found by pointing #7196's from-space reporter at `sfw-registry --help` rather than by grinding the static checker's tail. 1. RUNTIME CACHES (the one that mattered). `js_value_typeof` interned its eight result strings in thread-local `Cell<*mut StringHeader>`s that nothing registered as GC roots, so the FIRST minor collection swept or evacuated them and every later `typeof x === "..."` compared against from-space. `json/raw_json.rs`'s cached `"rawJSON"` key had the identical defect. Both now go through `gc_register_mutable_root_scanner`. This is why the registry failed 10/10 rather than intermittently: an unrooted register goes bad only when a collection lands in its window, an unrooted cache goes bad at collection #0 and stays bad. It is also structurally invisible to `scripts/gc_root_dominance_check.py`, which reads emitted LLVM IR and cannot see a runtime table. 2. CODEGEN, implicit `this`. `js_implicit_this_set` returns the value it displaced from the `IMPLICIT_THIS` cell -- a scanned MUTABLE root -- and that value was then held in a bare SSA register across the whole call the bind exists to scope. The restore published a pre-move address back INTO a root. #7214 found this in `js_closure_callN` and left it; it was in fact seven lowerings with seven copies of the same three lines, now one shared `temp_root::implicit_this_save` / `implicit_this_restore` pair. 3. CODEGEN, ClassExprFresh (#7211). Its `protect_handle` predicate asked only whether the AUTHOR's static initializers could collect, never whether the `js_object_set_field_by_name` the lowering itself emits per static could -- and that allocates. The four allowlist entries it covered are deleted, which is the ratchet working: the fix made them match nothing. Checker: `js_implicit_this_set` is now a root READ (being non-collecting is what makes a call one), which takes its sink from 214 hits to 0 and keeps the class gated. `--stale-registers` now honours `--min-binds`, and `--any-def` with it is a usage error -- both the "a silently ignored knob is a disarmed knob" rule the mode already applied to `--max-stale` and `--fatal-sinks`. Refs #7154, #7211, #7213, #7196, #7206, #7214, #7161 --- CLAUDE.md | 3 +- .../7219-registry-gc-unrooted-caches.md | 180 ++++++++++++++++++ .../src/expr/static_field_meta.rs | 35 +++- crates/perry-codegen/src/expr/temp_root.rs | 56 ++++++ .../src/lower_call/console_promise.rs | 72 ++++++- .../src/lower_call/early_branches.rs | 41 ++-- .../perry-codegen/src/lower_call/func_ref.rs | 13 +- .../src/lower_call/method_override.rs | 9 +- .../property_get/dynamic_dispatch.rs | 22 +-- .../property_get/static_dispatch.rs | 9 +- .../perry-runtime/src/builtins/arithmetic.rs | 143 +++++++++++--- crates/perry-runtime/src/builtins/mod.rs | 5 +- crates/perry-runtime/src/gc/mod.rs | 12 ++ crates/perry-runtime/src/json/mod.rs | 4 +- crates/perry-runtime/src/json/raw_json.rs | 33 +++- crates/perry-runtime/src/string/alloc.rs | 31 +++ scripts/gc_root_dominance_allowlist.json | 41 ++-- scripts/gc_root_dominance_check.py | 108 ++++++++++- ...t_gap_gc_closure_call_prev_this_rooting.ts | 66 +++++++ ...test_gap_gc_typeof_string_cache_rooting.ts | 69 +++++++ 20 files changed, 814 insertions(+), 138 deletions(-) create mode 100644 changelog.d/7219-registry-gc-unrooted-caches.md create mode 100644 test-files/test_gap_gc_closure_call_prev_this_rooting.ts create mode 100644 test-files/test_gap_gc_typeof_string_cache_rooting.ts diff --git a/CLAUDE.md b/CLAUDE.md index 65580f82a7..aa5ffdbf9d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -250,4 +250,5 @@ 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`, 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. +- **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) — **that list is now empty**, which #7211 emptied by fixing the fifth shape: `ClassExprFresh` rooted only when it thought the static *initializers* could collect and never asked whether its own emitted `js_object_set_field_by_name` could. The sophisticated version of the mistake — the author wrote a rooting predicate and it asked the wrong question. +- **A runtime-side cache of a raw heap pointer is a GC root, and the static checker cannot see it.** This is the sibling class, and it is the one that actually kept `sfw-registry --help` red after every codegen register in #7192/#7206/#7214 was closed. `js_value_typeof` interned its eight result strings in thread-local `Cell<*mut StringHeader>`s that nothing registered, so the FIRST minor collection invalidated them and every later `typeof x === "…"` compared against from-space (#7211). Two things to carry forward. **The failure signature is different**: an unrooted register goes bad only when a collection lands in its window, so it is intermittent; an unrooted cache goes bad at collection #0 and stays bad, so it fails 10/10 — if a GC bug is perfectly reproducible, suspect a table, not a register. And **`scripts/gc_root_dominance_check.py` reads emitted LLVM IR, so it is structurally blind to this**; the detector is `PERRY_GC_ZEAL=1 PERRY_GC_PROTECT_FROMSPACE=1 PERRY_GC_PROTECT_FROMSPACE_DEPTH=800` on a real workload, whose reporter names the address, `obj_type`, size and retiring cycle. Point the runtime instruments at the workload *before* grinding the static checker's tail. The root registry is `gc_register_mutable_root_scanner` in `gc/mod.rs` (~55 entries); when you add a cache of a heap pointer, add it there in the same commit. diff --git a/changelog.d/7219-registry-gc-unrooted-caches.md b/changelog.d/7219-registry-gc-unrooted-caches.md new file mode 100644 index 0000000000..7643dfbae3 --- /dev/null +++ b/changelog.d/7219-registry-gc-unrooted-caches.md @@ -0,0 +1,180 @@ +### Fixed + +- **`gc`: the interned `typeof` strings and `JSON.rawJSON`'s interned key are GC + roots, and are now registered as such** (#7211). `js_value_typeof` caches its + eight possible results in thread-local `Cell<*mut StringHeader>`s so each is + built once. Those cells held a **raw pointer into the nursery** with nothing + referencing the string, so the first minor collection swept or evacuated it + and the cache named abandoned memory for the rest of the process — every + later `typeof x === "string"` handed `js_string_equals` a from-space address. + `json/raw_json.rs`'s cached `"rawJSON"` key had the identical defect and is + fixed with it. Both now go through `gc_register_mutable_root_scanner`, so they + are marked (never swept) and rewritten (never stale after a copying minor). + + **This is what kept `sfw-registry --help` red** after #7192, #7206 and #7214 + had closed every stale *register* they could find, and the difference in + failure signature is the lesson worth keeping: + + | | unrooted register (#7154 class) | unrooted cache (this) | + |---|---|---| + | goes bad | only if a collection lands in a few-instruction window | at collection #0, permanently | + | reproduces | intermittently; needed a zod workload and ten rounds | **10/10** | + | found by | `scripts/gc_root_dominance_check.py` over emitted IR | nothing static — the tool cannot see a runtime table | + + A perfectly reproducible GC bug is evidence *against* a stale register. The + detector here was `PERRY_GC_ZEAL=1 PERRY_GC_PROTECT_FROMSPACE=1 + PERRY_GC_PROTECT_FROMSPACE_DEPTH=800`, whose reporter named it outright: + + ``` + [gc-fromspace-protect] FAULT: signal 10 at 0x…558a4 + last-known object: user_ptr=0x…558a0 obj_type=3 size=40 + retired_by_minor=#0 + … perry_closure_node_modules_zod_src_v4_core_schemas_ts__222 + 100 + ``` + + `obj_type` 3 is a string and 40 bytes is a 32-byte header plus `"string"`; + `retired_by_minor=#0` is the tell for a table rather than a register. + +- **`codegen`: the saved implicit `this` is rooted across every dispatch that + binds one** (#7211). `js_implicit_this_set` swaps the `IMPLICIT_THIS` cell and + returns what was there. That cell is a registered *mutable* root + (`scan_implicit_this_roots_mut`, `object/this_binding.rs:176`) and the swap has + already overwritten it, so the returned value is held only in an SSA register + — across the whole call the bind exists to scope. A minor inside that call + moves the object and rewrites every root naming it, leaving the register on + from-space; the restore then writes that pre-move address **back into a root + the collector scans**, so the corruption outlives the call that caused it. + + #7214 identified this in `js_closure_callN` and left it measured but unfixed. + It was in fact **seven** lowerings with seven copies of the same three lines: + `js_closure_callN` (`lower_call/console_promise.rs`), the + `js_native_call_value` override arms in `lower_call/method_override.rs` and + both `lower_call/property_get` dispatchers, the static-dispatch arm, the + #3576 receiverless reset in `lower_call/func_ref.rs`, and both closure-call + arms in `lower_call/early_branches.rs`. They are now one shared pair, + `temp_root::implicit_this_save` / `implicit_this_restore`, so the eighth + lowering that needs it gets the root for free rather than the bug. + +- **`codegen`: `Expr::ClassExprFresh` roots the fresh class object across the + `js_object_set_field_by_name` calls it emits itself** (#7211). The old + `protect_handle` predicate had four disjuncts and every one asked whether + something the *author* supplied could collect — a captured argument, a symbol + static, a `static { … }` body, an initializer expression. None asked whether + the lowering's own emitted field-store could, and it can: that helper performs + the keys-array transition and allocates. So `class C { static tag = tag }`, + one inert `LocalGet`, got no root at all. + + `js_object_mark_class` does not rescue it, and that is the reason this + survived review once already: it files the pointer in `CLASS_OBJECT_VALUES`, + which is a registered root and *is* forwarded — which keeps the OBJECT alive + and the side table's copy correct, and does nothing for the register. + Reachability is not the invariant; the invariant is that the register you are + still going to use was rewritten. + +### Changed + +- **`scripts/gc_root_dominance_check.py`: `js_implicit_this_set` is a root + READ.** Being non-collecting is exactly what makes a call a root read — the + same rule `js_closure_get_capture_bits` is listed under — and this one was + `NONCOLLECTING` without being a source, so the checker saw a safe call, never + classified the result as a heap value, and reported nothing at either end. + That blind spot is why `prev_this` survived #7206 and #7214. It reports the + class now: **214 hits on the parent, 0 after**, the largest single named sink + in the corpus. + +- **`scripts/gc_root_dominance_allowlist.json` is empty.** The four #7211 + entries were deleted because the fix made every one of them match nothing — + rule 1 working as designed: the gate goes red on a stale entry, and that red + is the instruction to delete it. An empty list is not a disarmed gate; rule 3 + still fails any violation, the `--min-files`/`--min-binds`/`--min-funcs` + floors still refuse a corpus that did not exercise the subject, and + `--self-test` still proves the checker can report a planted violation. + +- **`--stale-registers` now honours `--min-binds`**, and `--any-def` with + `--stale-registers` is a usage error. Both are the "a knob that is silently + ignored is a disarmed knob" rule the mode already applied to `--max-stale` + and `--fatal-sinks`, applied in the other direction. `--min-binds` is not a + bind-anchored-check detail: `run_stale` classifies a shadow-slot load as a + heap-value source by looking it up in the same `BIND_RE`-derived map, so a + corpus compiled with `PERRY_INLINE_SHADOW_SLOT=1` loses those sources + entirely, reports `total 0`, exits 0, and is indistinguishable from a clean + corpus. That is hazard 4 — the gate runs but its subject never did. Both + directions are asserted in `--self-test`, over-budget and under. + +## Verification + +Everything below is measured against the parent commit (`7d1dc9ca2`), built +from its own worktree rather than borrowed from another one. + +`sfw-registry --help`, 141 modules, compiled **and** run with +`PERRY_GC_MOVING_LOOP_POLLS=1` (it is compile-time since #7161 *and* +runtime-armed at `gc/policy.rs:1759`; arming only one is the false green that +cost #7214 a round): + +| | parent | this PR | +|---|---|---| +| `POLLS=1` | **0/10** — SIGSEGV every run | **10/10 clean** | +| default (no polls) | 10/10 clean | **10/10 clean** | + +Gap tests: + +| | parent | this PR | +|---|---|---| +| `test_gap_gc_typeof_string_cache_rooting.ts`, `POLLS=1` | `bad 444` **10/10** | `bad 0` **10/10** | +| same, `POLLS=1` + `PERRY_GEN_GC=0` | `bad 0` | `bad 0` | +| `test_gap_gc_closure_call_prev_this_rooting.ts`, `POLLS=1` + `PERRY_GC_ZEAL=1` | `bad 400` **5/5** | `bad 0` **10/10** | +| same, `PERRY_GEN_GC=0` + zeal | `bad 0` | `bad 0` | + +The `prev_this` test needs zeal and the reason is worth recording: the window +is a user call, so the collection that exploits it has to be a *moving* one, +and the only moving collections are the loop back-edge poll and the +microtask-pump safepoint. Allocation-triggered collections take +`ManualGcScanGuard::force_full_scan`, which makes the copying minor ineligible. +Zeal is the sanctioned instrument for exactly that, and the `PERRY_GEN_GC=0` +arm proves the test tracks collector mode rather than merely being flaky. + +`ClassExprFresh` has **no runtime gap test, deliberately**, and the same +argument is why: its window contains only `js_object_set_field_by_name` — no +user code and no loop, therefore no moving collection can land in it today. The +subject is the invariant, so the gate is the gap test, and it moves: + +| checker, `--moving-only`, 130 modules / 2170 functions | parent | this PR | +|---|---|---| +| bind-anchored violations | 5 (all 4 allowlist entries used) | **0**, allowlist empty | +| `--stale-registers`, total | 2912 | **2817** | +| `--stale-registers`, `sink=js_implicit_this_set` | 214 | **0** | +| `--stale-registers --fatal-sinks` | 279 | 279 (untouched by this change) | + +`cargo test -p perry-codegen`: **6 failures on the parent, the identical 6 +here** — all `loop_safepoint_purity`, which is #7161's default flip, measured on +the parent rather than assumed. + +Cost, over the 141-module `sfw-registry` corpus. The implicit-`this` root is on +the dynamic-dispatch path, so it was measured rather than argued: + +| | parent | this PR | delta | +|---|---|---|---| +| linked binary | 26,848,048 B | 26,881,088 B | **+0.12 %** | + +## Not fixed here, and why + +`js_get_string_pointer_unified` hands generated code a **raw** `*StringHeader`, +and its SSO branch allocates. Every `unbox_str_handle` site in +`expr/compare.rs`, `lower_string_method.rs` and `lower_array_method.rs` unboxes +its operands back-to-back into bare registers before one consuming call, so the +first handle is live across the second unbox with no root describing it. It is +**not exploitable today** — that allocation reaches the alloc-point arm of +`gc_check_trigger`, which forces a conservative stack scan, which makes the +copying minor ineligible, so the collection it can cause never moves anything +and the same scan keeps the handle alive. Both halves are closed by accident, +and by accident is the point: it rests on the alloc-point arm staying +non-moving, which is the property the moving-GC work keeps eroding. + +Filed as **#7213** rather than fixed here. A `GcSuppressScope` around that +allocation makes the window sound and costs nothing measurable, and it was +written and then reverted: shipping a GC-trigger change with no test that can +fail without it is exactly what CLAUDE.md's knob-kill policy exists to stop. +`js_get_string_pointer_unified` is likewise left out of the checker's +`ROOT_READ_CALLS` — classifying it would report the whole family, roughly forty +sites, and that population deserves its own measured count rather than being +folded in here. diff --git a/crates/perry-codegen/src/expr/static_field_meta.rs b/crates/perry-codegen/src/expr/static_field_meta.rs index 9616a4225f..cb77254c3e 100644 --- a/crates/perry-codegen/src/expr/static_field_meta.rs +++ b/crates/perry-codegen/src/expr/static_field_meta.rs @@ -471,10 +471,39 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { // and the block's body could then relocate the object out from under // the register the final `nanbox_pointer_inline` reads. let block_fns = static_block_fns(ctx, template); - let protect_handle = !captured_args.is_empty() + // #7211: `!named_statics.is_empty()` is the disjunct the original + // predicate was missing, and its absence is the interesting part. + // + // Every other clause here asks the same question — "can something + // the AUTHOR wrote collect?" — about a captured argument, a symbol + // static, a `static { … }` body, or an initializer expression. + // None of them asks whether the lowering's OWN emitted calls can, + // and the loop directly below unconditionally emits one + // `js_object_set_field_by_name` per named static. That helper + // performs the keys-array transition and allocates. So + // `class C { static tag = tag }` — a single inert `LocalGet` + // initializer — took `protect_handle == false`, kept the fresh + // object in a bare SSA register across a collection point, and + // then bound a shadow slot to the pre-move address. + // + // `js_object_mark_class` does NOT cover this, and it is the + // natural reason to wave it off: it files the pointer in + // `CLASS_OBJECT_VALUES`, which is a registered root and IS + // forwarded (`class_registry/gc_roots.rs:138`). That keeps the + // OBJECT alive and the side table's copy correct — and does + // nothing for `%obj`, a separate copy the collector cannot see. + // Reachability is not the invariant; the invariant is that the + // register you are still going to use was rewritten. + // The old `any_may_trigger_gc(named_statics)` disjunct is gone + // rather than kept alongside: it is now strictly subsumed — it can + // only be true when `named_statics` is non-empty, which is the new + // clause. Leaving it would read as a second, narrower opinion + // about the same operand and invite someone to "restore" the + // narrow one. + let protect_handle = !named_statics.is_empty() + || !captured_args.is_empty() || !symbol_statics.is_empty() - || !block_fns.is_empty() - || super::temp_root::any_may_trigger_gc(ctx, named_statics.iter().map(|(_, v)| v)); + || !block_fns.is_empty(); let rooted = super::temp_root::rooted_handle_begin(ctx, &obj, protect_handle); for (name, init) in named_statics { let key_idx = ctx.strings.intern(name); diff --git a/crates/perry-codegen/src/expr/temp_root.rs b/crates/perry-codegen/src/expr/temp_root.rs index 00bcaa3797..3c9bed3de6 100644 --- a/crates/perry-codegen/src/expr/temp_root.rs +++ b/crates/perry-codegen/src/expr/temp_root.rs @@ -102,6 +102,62 @@ pub(crate) fn temp_root_truncate(ctx: &mut FnCtx<'_>, idx: &str) { .call_void("js_gc_temp_root_truncate", &[(I32, idx)]); } +/// A saved implicit `this`, held in a temp-root slot for the duration of a +/// dispatch (#7211). +/// +/// `js_implicit_this_set` swaps the `IMPLICIT_THIS` cell and returns what was +/// there. That cell is a registered MUTABLE root — `scan_implicit_this_roots_mut` +/// (`object/this_binding.rs:176`) marks it and rewrites it on an evacuating +/// cycle — and the swap has already overwritten it, so the returned value is +/// now held ONLY in an SSA register, across the whole call the bind exists to +/// scope. A minor inside that call moves the object and rewrites every root +/// that names it, leaving this register on from-space; the restore then writes +/// that pre-move address BACK INTO the cell, so the corruption outlives the +/// call and lands on whatever reads `this` next. +/// +/// Seven lowerings emit this save/restore pair — `js_closure_callN`, the +/// `js_native_call_value` override arms in `method_override.rs` and both +/// `property_get` dispatchers, the static-dispatch arm, the direct-call +/// `#3576` reset in `func_ref.rs` and the two closure-call arms in +/// `early_branches.rs`. They had seven copies of the same three lines and +/// therefore seven copies of the same bug, which is why this is a helper +/// rather than seven edits: the next lowering that needs the pair gets the +/// root for free. +/// +/// Unconditional, unlike [`RootedOperands`]: the window is a user or native +/// call, so [`operand_protection`]'s "can this window collect?" test has +/// exactly one answer and there is nothing to gate on. +pub(crate) struct ImplicitThisSave { + slot: String, +} + +/// Bind `new_this` as the implicit `this` and root the value it displaced. +pub(crate) fn implicit_this_save(ctx: &mut FnCtx<'_>, new_this: &str) -> ImplicitThisSave { + let prev = ctx + .block() + .call(DOUBLE, "js_implicit_this_set", &[(DOUBLE, new_this)]); + let slot = temp_root_push_double(ctx, &prev); + ImplicitThisSave { slot } +} + +/// Restore the saved implicit `this`, re-read from its root. +/// +/// Reading the slot rather than the register is the fix, not a precaution: the +/// slot is a mutable root, so an evacuating cycle inside the dispatch rewrote +/// it and the register pushed beforehand names from-space. +/// +/// The truncate is emitted BEFORE the restore call so that nested saves — an +/// override arm inside an outer bind — release inner to outer. +/// `js_gc_temp_root_truncate` drops everything at or above its argument, so a +/// caller holding a LOWER group (`RootedOperands`) may release it afterwards +/// and drop this slot again harmlessly. +pub(crate) fn implicit_this_restore(ctx: &mut FnCtx<'_>, save: ImplicitThisSave) { + let prev = temp_root_get_double(ctx, &save.slot); + temp_root_truncate(ctx, &save.slot); + ctx.block() + .call(DOUBLE, "js_implicit_this_set", &[(DOUBLE, &prev)]); +} + /// Push `value` onto the array held in temp-root slot `idx`, writing the /// possibly-reallocated array pointer back into the slot. pub(crate) fn temp_rooted_array_push(ctx: &mut FnCtx<'_>, idx: &str, value: &str) { diff --git a/crates/perry-codegen/src/lower_call/console_promise.rs b/crates/perry-codegen/src/lower_call/console_promise.rs index dba208d8ee..d2734e0a27 100644 --- a/crates/perry-codegen/src/lower_call/console_promise.rs +++ b/crates/perry-codegen/src/lower_call/console_promise.rs @@ -1413,18 +1413,57 @@ pub fn try_lower_closure_call_fallthrough( }; 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)])) + // #7211: the value `js_implicit_this_set` hands back is the PREVIOUS + // implicit `this`, read straight out of the `IMPLICIT_THIS` cell — which + // `object/this_binding.rs:176` registers as a scanned MUTABLE root the + // collector rewrites in place (`scan_implicit_this_roots_mut`). The swap + // has already overwritten the cell by the time we hold it, so this + // register is now the only copy this frame has, and it stays live across + // the allocating rebind unbox below AND the entire user-code dispatch. + // + // Two ways that hurts, and the second is the one that makes this worse + // than an ordinary stale read: + // + // * the enclosing frame still roots the same object (its own operand + // group, one temp-root frame down), so an evacuating minor inside the + // callee MOVES it and rewrites that root — leaving this register + // naming from-space. The restore then publishes a pre-move address + // back INTO a root the collector scans, so the corruption outlives the + // call that caused it and surfaces in whatever reads `this` next. + // * where no other root holds it, the object is simply collected. + // + // It WAS invisible to `scripts/gc_root_dominance_check.py` at both ends, + // which is how it survived #7206 and #7214: `js_implicit_this_set` was + // NONCOLLECTING but not in `ROOT_READ_CALLS`, so the register had no + // recognised heap-value source, and the restore is not a `RECEIVER_SINKS` + // fatal sink, so it would not have ranked even if it had. Being + // non-collecting is precisely what makes a call a root READ — the same + // rule `js_closure_get_capture_bits` is listed under — and it is now + // classified that way, so the checker reports any lowering that + // reintroduces this. + // + // Unconditional, unlike the operand groups above: the window is the user + // call itself, so `operand_protection`'s "can this window collect?" test + // has exactly one answer here and there is nothing to gate on. + // + // Six sibling lowerings emit the same pair; `implicit_this_save` / + // `implicit_this_restore` is the shared form, so a seventh cannot + // reintroduce this by copy-paste. + let prev_this_root = if let Some(ref this_val) = method_recv { + Some(crate::expr::temp_root::implicit_this_save(ctx, this_val)) } else if !matches!(callee, Expr::PropertyGet { .. }) { // Receiverless closure-value call (`fn()`, IIFE, `curry(1)(2)`): // OrdinaryCallBindThis binds `this` to undefined — without the // reset the enclosing method dispatch's IMPLICIT_THIS leaks into // the callee (#3576). Member-shaped callees keep their existing // receiver/skip behavior above. + // + // The value pushed here is the ENCLOSING method's receiver, not + // `undefined`: this arm is the one that runs for `helper()` called + // from inside `o.m()`, and dropping that object is #3576's leak with + // the sign flipped. let undef = double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED)); - let blk = ctx.block(); - Some(blk.call(DOUBLE, "js_implicit_this_set", &[(DOUBLE, &undef)])) + Some(crate::expr::temp_root::implicit_this_save(ctx, &undef)) } else { None }; @@ -1518,13 +1557,26 @@ pub fn try_lower_closure_call_fallthrough( ) }; + // #7211: re-read the saved implicit `this` from its slot. Mandatory, not + // defensive, and for the same reason the operand re-reads above are: the + // temp-root slot is a MUTABLE root, so an evacuating cycle anywhere inside + // the dispatch rewrote the slot and left the register that was pushed + // naming from-space. Restoring the register instead of the slot is the + // whole bug. + // + // Ordered inner-to-outer: this slot was pushed above `roots`' first slot, + // so it is dropped first and `roots.release` then drops the group below + // it. `js_gc_temp_root_truncate` drops everything at or above its + // argument, so `roots.release` alone would in fact take this slot with it + // — but only when `roots` actually pushed one, and a receiverless call on + // inert arguments pushes nothing at all. Releasing this one explicitly is + // what makes the order correct in both shapes rather than in the common + // one. + if let Some(prev) = prev_this_root { + crate::expr::temp_root::implicit_this_restore(ctx, prev); + } // 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)]); - } Ok(Some(result)) } diff --git a/crates/perry-codegen/src/lower_call/early_branches.rs b/crates/perry-codegen/src/lower_call/early_branches.rs index 34a57dfd7e..0682ec1ec9 100644 --- a/crates/perry-codegen/src/lower_call/early_branches.rs +++ b/crates/perry-codegen/src/lower_call/early_branches.rs @@ -401,12 +401,13 @@ pub fn try_lower_closure_typed_local_call( &format!("closure:{}", func_id), TypedFeedbackContract::closure_direct_call(), ); + // #7211: rooted save/restore. The displaced value is the + // enclosing method's receiver and it is live across the + // callee body; the restore below sits in the merge block, + // so the slot index crosses the diamond exactly as the + // bare register used to. let prev_this = if callee_reads_this { - Some(ctx.block().call( - DOUBLE, - "js_implicit_this_set", - &[(DOUBLE, &undef_this)], - )) + Some(crate::expr::temp_root::implicit_this_save(ctx, &undef_this)) } else { None }; @@ -928,11 +929,7 @@ pub fn try_lower_closure_typed_local_call( // body codegen never saw — reset `this` here (and only // here) when the static gating skipped the outer reset. let fallback_prev_this = if prev_this.is_none() { - Some(ctx.block().call( - DOUBLE, - "js_implicit_this_set", - &[(DOUBLE, &undef_this)], - )) + Some(crate::expr::temp_root::implicit_this_save(ctx, &undef_this)) } else { None }; @@ -943,10 +940,11 @@ pub fn try_lower_closure_typed_local_call( fallback_args.push((DOUBLE, v.as_str())); } let fallback_value = ctx.block().call(DOUBLE, &runtime_fn, &fallback_args); - if let Some(prev) = &fallback_prev_this { - let _ = ctx - .block() - .call(DOUBLE, "js_implicit_this_set", &[(DOUBLE, prev)]); + // Inner save, released inside its own arm — so the outer + // slot (restored in the merge block) is still live and the + // temp-root depth matches on both paths into the merge. + if let Some(prev) = fallback_prev_this { + crate::expr::temp_root::implicit_this_restore(ctx, prev); } let after_fallback = ctx.block().label.clone(); if !ctx.block().is_terminated() { @@ -961,10 +959,8 @@ pub fn try_lower_closure_typed_local_call( (fallback_value.as_str(), after_fallback.as_str()), ], ); - if let Some(prev) = &prev_this { - let _ = ctx - .block() - .call(DOUBLE, "js_implicit_this_set", &[(DOUBLE, prev)]); + if let Some(prev) = prev_this { + crate::expr::temp_root::implicit_this_restore(ctx, prev); } return Ok(Some(merged)); } @@ -972,18 +968,15 @@ pub fn try_lower_closure_typed_local_call( // Generic js_closure_callN dispatch (unknown func id, rest // params, or arity mismatch): the runtime-resolved callee may // read `this`, so the reset is unconditional here. - let prev_this = - ctx.block() - .call(DOUBLE, "js_implicit_this_set", &[(DOUBLE, &undef_this)]); + // #7211: rooted save/restore across the runtime-resolved callee. + let prev_this = crate::expr::temp_root::implicit_this_save(ctx, &undef_this); let runtime_fn = format!("js_closure_call{}", lowered_args.len()); 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())); } let result = ctx.block().call(DOUBLE, &runtime_fn, &call_args); - let _ = ctx - .block() - .call(DOUBLE, "js_implicit_this_set", &[(DOUBLE, &prev_this)]); + crate::expr::temp_root::implicit_this_restore(ctx, prev_this); return Ok(Some(result)); } } diff --git a/crates/perry-codegen/src/lower_call/func_ref.rs b/crates/perry-codegen/src/lower_call/func_ref.rs index f1f477345a..145fc99522 100644 --- a/crates/perry-codegen/src/lower_call/func_ref.rs +++ b/crates/perry-codegen/src/lower_call/func_ref.rs @@ -488,12 +488,11 @@ pub fn try_lower_func_ref_call( // are lowered BEFORE the reset: `this` inside an argument expression // still sees the enclosing binding. let resets_this = ctx.funcs_reading_dynamic_this.contains(fid); + // #7211: rooted save/restore. The value displaced here is the ENCLOSING + // method's receiver, held across the callee body — arbitrary user code. let prev_this = if resets_this { let undef = double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED)); - Some( - ctx.block() - .call(DOUBLE, "js_implicit_this_set", &[(DOUBLE, &undef)]), - ) + Some(crate::expr::temp_root::implicit_this_save(ctx, &undef)) } else { None }; @@ -905,10 +904,8 @@ pub fn try_lower_func_ref_call( } else { ctx.block().call(DOUBLE, &fname, &arg_slices) }; - if let Some(prev) = &prev_this { - let _ = ctx - .block() - .call(DOUBLE, "js_implicit_this_set", &[(DOUBLE, prev)]); + if let Some(prev) = prev_this { + crate::expr::temp_root::implicit_this_restore(ctx, prev); } if ctx.local_generator_funcs.contains(fid) { let wrap_ptr = format!("@__perry_wrap_{}", fname); diff --git a/crates/perry-codegen/src/lower_call/method_override.rs b/crates/perry-codegen/src/lower_call/method_override.rs index 39f93ba0d8..3e863e000d 100644 --- a/crates/perry-codegen/src/lower_call/method_override.rs +++ b/crates/perry-codegen/src/lower_call/method_override.rs @@ -119,9 +119,9 @@ pub(super) fn emit_own_method_override_check( } else { this_box.to_string() }; - let prev_this = ctx - .block() - .call(DOUBLE, "js_implicit_this_set", &[(DOUBLE, &recv_for_this)]); + // #7211: rooted save/restore — the displaced implicit `this` is live + // across `js_native_call_value`, which runs arbitrary user code. + let prev_this = crate::expr::temp_root::implicit_this_save(ctx, &recv_for_this); let v_override = ctx.block().call( DOUBLE, "js_native_call_value", @@ -131,8 +131,7 @@ pub(super) fn emit_own_method_override_check( (I64, &args_len), ], ); - ctx.block() - .call(DOUBLE, "js_implicit_this_set", &[(DOUBLE, &prev_this)]); + crate::expr::temp_root::implicit_this_restore(ctx, prev_this); let after_override = ctx.block().label.clone(); if !ctx.block().is_terminated() { ctx.block().br(&merge_label); diff --git a/crates/perry-codegen/src/lower_call/property_get/dynamic_dispatch.rs b/crates/perry-codegen/src/lower_call/property_get/dynamic_dispatch.rs index 35197f21d4..91ef851377 100644 --- a/crates/perry-codegen/src/lower_call/property_get/dynamic_dispatch.rs +++ b/crates/perry-codegen/src/lower_call/property_get/dynamic_dispatch.rs @@ -392,11 +392,9 @@ pub(crate) fn try_lower_instance_method_call( // garbage. Mirrors `lower_call.rs:2607` for the closure- // call fallthrough pattern (#519). let recv_for_this_probe = recv_box.clone(); - let prev_this_probe = ctx.block().call( - DOUBLE, - "js_implicit_this_set", - &[(DOUBLE, &recv_for_this_probe)], - ); + // #7211: rooted save/restore across the user-code dispatch. + let prev_this_probe = + crate::expr::temp_root::implicit_this_save(ctx, &recv_for_this_probe); let v_override_probe = ctx.block().call( DOUBLE, "js_native_call_value", @@ -406,11 +404,7 @@ pub(crate) fn try_lower_instance_method_call( (I64, &probe_args_len_str), ], ); - ctx.block().call( - DOUBLE, - "js_implicit_this_set", - &[(DOUBLE, &prev_this_probe)], - ); + crate::expr::temp_root::implicit_this_restore(ctx, prev_this_probe); let after_override_probe = ctx.block().label.clone(); if !ctx.block().is_terminated() { ctx.block().br(&probe_outer_merge_label); @@ -1372,9 +1366,8 @@ fn emit_collapsed_instance_dispatch( // Override arm: bind IMPLICIT_THIS to the receiver and call the stored // function value (#632 — a class-field non-arrow function reads `this`). ctx.current_block = override_idx; - let prev_this = ctx - .block() - .call(DOUBLE, "js_implicit_this_set", &[(DOUBLE, recv_box)]); + // #7211: rooted save/restore across the user-code dispatch. + let prev_this = crate::expr::temp_root::implicit_this_save(ctx, recv_box); let v_override = ctx.block().call( DOUBLE, "js_native_call_value", @@ -1384,8 +1377,7 @@ fn emit_collapsed_instance_dispatch( (I64, &args_len), ], ); - ctx.block() - .call(DOUBLE, "js_implicit_this_set", &[(DOUBLE, &prev_this)]); + crate::expr::temp_root::implicit_this_restore(ctx, prev_this); let after_override = ctx.block().label.clone(); if !ctx.block().is_terminated() { ctx.block().br(&merge_label); diff --git a/crates/perry-codegen/src/lower_call/property_get/static_dispatch.rs b/crates/perry-codegen/src/lower_call/property_get/static_dispatch.rs index 021dee0d53..e40467ddfd 100644 --- a/crates/perry-codegen/src/lower_call/property_get/static_dispatch.rs +++ b/crates/perry-codegen/src/lower_call/property_get/static_dispatch.rs @@ -213,9 +213,9 @@ pub(crate) fn try_lower_static_dispatch( lowered.push(undef.clone()); } } - let prev_this = - ctx.block() - .call(DOUBLE, "js_implicit_this_set", &[(DOUBLE, &recv_box)]); + // #7211: rooted save/restore — the displaced implicit `this` is + // live across the static method body below, which is user code. + let prev_this = crate::expr::temp_root::implicit_this_save(ctx, &recv_box); // Receiver-sensitive static `this`: arm the one-shot override with // the ACTUAL receiver box so the callee prologue's // `js_static_this_resolve` binds `this` to it (spec @@ -240,8 +240,7 @@ pub(crate) fn try_lower_static_dispatch( let arg_slices: Vec<(crate::types::LlvmType, &str)> = lowered.iter().map(|s| (DOUBLE, s.as_str())).collect(); let result = ctx.block().call(DOUBLE, &fn_name, &arg_slices); - ctx.block() - .call(DOUBLE, "js_implicit_this_set", &[(DOUBLE, &prev_this)]); + crate::expr::temp_root::implicit_this_restore(ctx, prev_this); return Ok(Some(result)); } // #1787 / #321: the call target is a static FIELD holding a callable, diff --git a/crates/perry-runtime/src/builtins/arithmetic.rs b/crates/perry-runtime/src/builtins/arithmetic.rs index 1e773466c5..cda3970325 100644 --- a/crates/perry-runtime/src/builtins/arithmetic.rs +++ b/crates/perry-runtime/src/builtins/arithmetic.rs @@ -495,43 +495,126 @@ pub extern "C" fn js_ge(a: JSValue, b: JSValue) -> JSValue { JSValue::from_bits(js_rel_ge(f64::from_bits(a.bits()), f64::from_bits(b.bits())).to_bits()) } +// `typeof` returns one of eight strings, so each is allocated once and cached +// rather than rebuilt per call. +// +// #7211: these cells are GC ROOTS and are registered as such +// (`scan_typeof_string_roots_mut`, wired in `gc/mod.rs`). They are at module +// scope rather than inside `js_value_typeof` for exactly that reason — a +// scanner has to be able to reach them. +// +// Before that registration this cache was a deterministic use-after-free, and +// it is the bug that killed `sfw-registry --help` 10/10 under a +// `PERRY_GC_MOVING_LOOP_POLLS=1` build. `js_string_from_bytes` allocates in the +// NURSERY; nothing else references the result; so the first minor either swept +// it or evacuated it, and this raw pointer named the abandoned bytes from then +// on. Every later `typeof x === "string"` handed `js_string_equals` a from-space +// address. `PERRY_GC_PROTECT_FROMSPACE=1 PERRY_GC_PROTECT_FROMSPACE_DEPTH=800` +// reported it precisely: `obj_type=3 size=40 retired_by_minor=#0` — a string, +// 32-byte header plus `"string"`, retired by the very first collection. +// +// `retired_by_minor=#0` is the tell for this whole shape, and it is worth +// recognising: an ordinary #7154-class stale register goes bad at whichever +// collection lands inside a few-instruction window, so it is timing-dependent. +// A cache that is never rooted goes bad at the FIRST collection and stays bad, +// which is why this reproduced 10/10 while the register bugs needed a zod +// workload and ten rounds. +// +// It is invisible to `scripts/gc_root_dominance_check.py` by construction: that +// tool reads emitted LLVM IR, and this is a runtime-side table. The static +// checker could never have found it, which is why the runtime instruments had +// to be pointed at the registry first. +thread_local! { + static TYPEOF_UNDEFINED: std::cell::Cell<*mut StringHeader> = const { std::cell::Cell::new(std::ptr::null_mut()) }; + static TYPEOF_OBJECT: std::cell::Cell<*mut StringHeader> = const { std::cell::Cell::new(std::ptr::null_mut()) }; + static TYPEOF_BOOLEAN: std::cell::Cell<*mut StringHeader> = const { std::cell::Cell::new(std::ptr::null_mut()) }; + static TYPEOF_NUMBER: std::cell::Cell<*mut StringHeader> = const { std::cell::Cell::new(std::ptr::null_mut()) }; + static TYPEOF_STRING: std::cell::Cell<*mut StringHeader> = const { std::cell::Cell::new(std::ptr::null_mut()) }; + static TYPEOF_FUNCTION: std::cell::Cell<*mut StringHeader> = const { std::cell::Cell::new(std::ptr::null_mut()) }; + static TYPEOF_BIGINT: std::cell::Cell<*mut StringHeader> = const { std::cell::Cell::new(std::ptr::null_mut()) }; + static TYPEOF_SYMBOL: std::cell::Cell<*mut StringHeader> = const { std::cell::Cell::new(std::ptr::null_mut()) }; +} + +/// Get or initialize a cached `typeof` string. +fn get_cached( + cache: &'static std::thread::LocalKey>, + s: &str, +) -> *mut StringHeader { + cache.with(|cell| { + let ptr = cell.get(); + if !ptr.is_null() { + return ptr; + } + let new_ptr = crate::string::js_string_from_bytes(s.as_ptr(), s.len() as u32); + cell.set(new_ptr); + new_ptr + }) +} + +/// GC mutable-root scanner for the eight cached `typeof` strings (#7211). +/// +/// Marks them, so an unreferenced cache entry is never swept, AND rewrites +/// them, so an evacuating minor that relocates one leaves the cell naming the +/// new address instead of from-space. Both halves matter: marking alone would +/// still hand out a pre-move pointer after a copying minor, which is the +/// distinction `gc-rooting-invariant.md` keeps having to make. +/// +/// `STRING_TAG` rather than the default `POINTER_TAG` because these are +/// `StringHeader`s, matching `json::scan_parse_roots_mut`'s interned-key +/// treatment. +pub fn scan_typeof_string_roots_mut(visitor: &mut crate::gc::RuntimeRootVisitor<'_>) { + fn visit( + cache: &'static std::thread::LocalKey>, + visitor: &mut crate::gc::RuntimeRootVisitor<'_>, + ) { + cache.with(|cell| { + let mut ptr = cell.get() as *const StringHeader; + if ptr.is_null() { + return; + } + if visitor.visit_tagged_raw_const_ptr_slot(&mut ptr, crate::value::STRING_TAG) { + cell.set(ptr as *mut StringHeader); + } + }); + } + visit(&TYPEOF_UNDEFINED, visitor); + visit(&TYPEOF_OBJECT, visitor); + visit(&TYPEOF_BOOLEAN, visitor); + visit(&TYPEOF_NUMBER, visitor); + visit(&TYPEOF_STRING, visitor); + visit(&TYPEOF_FUNCTION, visitor); + visit(&TYPEOF_BIGINT, visitor); + visit(&TYPEOF_SYMBOL, visitor); +} + +/// Drop every cached `typeof` string. Test-only: the unit-test harness resets +/// arenas between tests while thread-locals persist, so a cache entry from a +/// previous test names memory the new arena does not own. +#[cfg(test)] +pub(crate) fn reset_typeof_string_cache_for_test() { + for cache in [ + &TYPEOF_UNDEFINED, + &TYPEOF_OBJECT, + &TYPEOF_BOOLEAN, + &TYPEOF_NUMBER, + &TYPEOF_STRING, + &TYPEOF_FUNCTION, + &TYPEOF_BIGINT, + &TYPEOF_SYMBOL, + ] { + cache.with(|cell| cell.set(std::ptr::null_mut())); + } +} + /// Return the typeof a value as a string /// Takes an f64 that uses NaN-boxing to distinguish types. /// Returns a pointer to a string: "undefined", "boolean", "number", "string", "object", "function" /// -/// Optimization: typeof only returns 7 possible strings, so we cache them as +/// Optimization: typeof only returns 8 possible strings, so we cache them as /// pre-allocated StringHeader pointers to avoid heap allocation on every call. +/// The cache is a registered GC root — see the `thread_local!` above. #[no_mangle] pub extern "C" fn js_value_typeof(value: f64) -> *mut StringHeader { - use std::cell::Cell; - - thread_local! { - static TYPEOF_UNDEFINED: Cell<*mut StringHeader> = const { Cell::new(std::ptr::null_mut()) }; - static TYPEOF_OBJECT: Cell<*mut StringHeader> = const { Cell::new(std::ptr::null_mut()) }; - static TYPEOF_BOOLEAN: Cell<*mut StringHeader> = const { Cell::new(std::ptr::null_mut()) }; - static TYPEOF_NUMBER: Cell<*mut StringHeader> = const { Cell::new(std::ptr::null_mut()) }; - static TYPEOF_STRING: Cell<*mut StringHeader> = const { Cell::new(std::ptr::null_mut()) }; - static TYPEOF_FUNCTION: Cell<*mut StringHeader> = const { Cell::new(std::ptr::null_mut()) }; - static TYPEOF_BIGINT: Cell<*mut StringHeader> = const { Cell::new(std::ptr::null_mut()) }; - static TYPEOF_SYMBOL: Cell<*mut StringHeader> = const { Cell::new(std::ptr::null_mut()) }; - } - - /// Get or initialize a cached typeof string. - fn get_cached( - cache: &'static std::thread::LocalKey>, - s: &str, - ) -> *mut StringHeader { - cache.with(|cell| { - let ptr = cell.get(); - if !ptr.is_null() { - return ptr; - } - let new_ptr = crate::string::js_string_from_bytes(s.as_ptr(), s.len() as u32); - cell.set(new_ptr); - new_ptr - }) - } - let jsval = JSValue::from_bits(value.to_bits()); if jsval.is_undefined() { diff --git a/crates/perry-runtime/src/builtins/mod.rs b/crates/perry-runtime/src/builtins/mod.rs index bba2da3eb0..eaa13d8023 100644 --- a/crates/perry-runtime/src/builtins/mod.rs +++ b/crates/perry-runtime/src/builtins/mod.rs @@ -38,7 +38,10 @@ pub(crate) use println; pub(crate) use crate::string::{js_string_from_bytes, js_string_from_wtf8_bytes, StringHeader}; pub(crate) use crate::JSValue; -mod arithmetic; +// `pub(crate)` so `gc::mod` can register `scan_typeof_string_roots_mut` +// (#7211): the interned `typeof` strings are GC roots and the scanner has to +// be nameable from the registration list. +pub(crate) mod arithmetic; mod console; mod formatting; mod globals; diff --git a/crates/perry-runtime/src/gc/mod.rs b/crates/perry-runtime/src/gc/mod.rs index ba74e47e67..dbea304905 100644 --- a/crates/perry-runtime/src/gc/mod.rs +++ b/crates/perry-runtime/src/gc/mod.rs @@ -485,6 +485,18 @@ pub fn gc_init() { gc_register_mutable_root_scanner(async_hooks_mutable_root_scanner); gc_register_mutable_root_scanner(shape_cache_mutable_root_scanner); gc_register_mutable_root_scanner(crate::regex::scan_last_exec_groups_root_mut); + // #7211: the eight interned `typeof` result strings, and JSON.rawJSON's + // interned `"rawJSON"` key. Both are thread-local caches of a RAW + // `StringHeader*` allocated in the nursery and referenced by nothing else, + // so before this registration the FIRST minor collection sweeps or + // evacuates them and the cached pointer names abandoned memory forever + // after. Not a timing-dependent stale register: a permanently wrong cache, + // which is why `sfw-registry --help` under a + // `PERRY_GC_MOVING_LOOP_POLLS=1` build failed 10/10 rather than + // intermittently, and why the from-space reporter blamed + // `retired_by_minor=#0`. + gc_register_mutable_root_scanner(crate::builtins::arithmetic::scan_typeof_string_roots_mut); + gc_register_mutable_root_scanner(crate::json::raw_json::scan_raw_json_key_root_mut); gc_register_mutable_root_scanner(crate::object::scan_exotic_expando_roots_mut); gc_register_mutable_root_scanner(crate::array::scan_template_raw_roots_mut); // #6981: the memoized `Array.prototype` / `Object.prototype` addresses in diff --git a/crates/perry-runtime/src/json/mod.rs b/crates/perry-runtime/src/json/mod.rs index 62ebb33945..66cecfd598 100644 --- a/crates/perry-runtime/src/json/mod.rs +++ b/crates/perry-runtime/src/json/mod.rs @@ -23,7 +23,9 @@ use std::cell::RefCell; mod parse_api; mod parser; -mod raw_json; +// `pub(crate)` so `gc::mod` can register `scan_raw_json_key_root_mut` (#7211): +// the interned `"rawJSON"` key is a GC root. +pub(crate) mod raw_json; mod replacer; mod reviver; mod simd; diff --git a/crates/perry-runtime/src/json/raw_json.rs b/crates/perry-runtime/src/json/raw_json.rs index 4c52e8c0ea..55bc9fda33 100644 --- a/crates/perry-runtime/src/json/raw_json.rs +++ b/crates/perry-runtime/src/json/raw_json.rs @@ -78,13 +78,36 @@ pub(crate) unsafe fn ptr_is_raw_json_wrapper(ptr: *const u8) -> bool { && (*obj).class_id == RAW_JSON_CLASS_ID } +// #7211: same defect as the `typeof` string cache, found by grepping for the +// shape once that one was diagnosed — a thread-local holding a raw nursery +// `StringHeader*` with nothing registering it as a root. Allocated once, +// referenced by nothing else, so the first minor sweeps or evacuates it and +// every later `JSON.rawJSON(...)` writes its own property under a from-space +// key. Hoisted out of the function body so `scan_raw_json_key_root_mut` can +// reach it. +thread_local! { + static RAW_JSON_KEY: std::cell::Cell<*mut StringHeader> = + const { std::cell::Cell::new(std::ptr::null_mut()) }; +} + +/// GC mutable-root scanner for the cached `"rawJSON"` key (#7211). Marks it so +/// it is not swept and rewrites it so an evacuating minor cannot leave the cell +/// naming from-space. +pub fn scan_raw_json_key_root_mut(visitor: &mut crate::gc::RuntimeRootVisitor<'_>) { + RAW_JSON_KEY.with(|cell| { + let mut ptr = cell.get() as *const StringHeader; + if ptr.is_null() { + return; + } + if visitor.visit_tagged_raw_const_ptr_slot(&mut ptr, crate::value::STRING_TAG) { + cell.set(ptr as *mut StringHeader); + } + }); +} + /// Cached `"rawJSON"` key string used for the wrapper's own property. fn raw_json_key() -> *const StringHeader { - use std::cell::Cell; - thread_local! { - static KEY: Cell<*mut StringHeader> = const { Cell::new(std::ptr::null_mut()) }; - } - KEY.with(|c| { + RAW_JSON_KEY.with(|c| { let p = c.get(); if !p.is_null() { return p as *const StringHeader; diff --git a/crates/perry-runtime/src/string/alloc.rs b/crates/perry-runtime/src/string/alloc.rs index eac25168c5..9cfc5097e9 100644 --- a/crates/perry-runtime/src/string/alloc.rs +++ b/crates/perry-runtime/src/string/alloc.rs @@ -18,6 +18,37 @@ pub extern "C" fn js_string_from_bytes(data: *const u8, len: u32) -> *mut String /// materialized value, so use sparingly — only as a last-resort /// compatibility shim on paths that truly need the heap /// representation. +/// +/// # The SSO branch allocates, and its callers hold raw pointers (#7213) +/// +/// This branch is a collection point, and `js_get_string_pointer_unified` — +/// its main caller — hands generated code a **raw** `*mut StringHeader` that +/// no root describes. Every `unbox_str_handle` site in +/// `perry-codegen/src/expr/compare.rs`, `lower_string_method.rs` and +/// `lower_array_method.rs` lowers its operands first and then unboxes them +/// back-to-back before one consuming call: +/// +/// ```text +/// %l = call i64 @js_get_string_pointer_unified(double %lbox) +/// %r = call i64 @js_get_string_pointer_unified(double %rbox) +/// call i32 @js_string_equals(i64 %l, i64 %r) +/// ``` +/// +/// `%l` is live across the second unbox. **It is not exploitable today**, and +/// the reason is worth writing down rather than rediscovering: an allocation +/// here reaches the alloc-point arm of `gc_check_trigger`, which takes +/// `ManualGcScanGuard::force_full_scan`, and a forced conservative stack scan +/// makes the copying minor ineligible (`CopiedMinorFallbackReason:: +/// ConservativeStack`). So the collection this allocation can cause never +/// MOVES anything, and the same conservative scan finds `%l` on the stack and +/// keeps it alive. Both halves of the hazard are closed by accident. +/// +/// By accident is the operative phrase — it rests on the alloc-point arm +/// staying non-moving, which is exactly the property the moving-GC work keeps +/// eroding. Tracked as #7213 rather than pre-emptively fixed here: a +/// `GcSuppressScope` around this allocation makes the window sound and costs +/// nothing measurable, but shipping a GC-trigger change with no test that can +/// fail without it is the thing CLAUDE.md's knob-kill policy exists to stop. #[no_mangle] pub extern "C" fn js_string_materialize_to_heap(value: f64) -> *mut StringHeader { let bits = value.to_bits(); diff --git a/scripts/gc_root_dominance_allowlist.json b/scripts/gc_root_dominance_allowlist.json index 936b642598..e14e9b36f7 100644 --- a/scripts/gc_root_dominance_allowlist.json +++ b/scripts/gc_root_dominance_allowlist.json @@ -20,32 +20,19 @@ "new violation was introduced.", "", "Fingerprint format: .ll::::->", - "Get it from the checker's own output (`-v` prints one per violation)." + "Get it from the checker's own output (`-v` prints one per violation).", + "", + "EMPTY IS THE GOAL AND IT IS CURRENTLY MET. The four #7211 entries that", + "lived here -- ClassExprFresh holding its fresh class object across the", + "js_object_set_field_by_name calls that install its own statics -- were", + "deleted when that predicate was fixed. That is rule 1 working as designed:", + "the fix made every one of them match nothing, the gate went red, and the", + "red is what says 'now delete the entries'.", + "", + "An empty list is not a disarmed gate. Rule 3 still fails any violation,", + "the --min-files / --min-binds / --min-funcs floors still refuse a corpus", + "that did not actually exercise the subject, and --self-test still proves", + "the checker reports a planted violation and clears the matching control." ], - "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." - } - ] + "entries": [] } diff --git a/scripts/gc_root_dominance_check.py b/scripts/gc_root_dominance_check.py index a6214460d4..be88869222 100755 --- a/scripts/gc_root_dominance_check.py +++ b/scripts/gc_root_dominance_check.py @@ -272,6 +272,10 @@ def build_cfg(f): # 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", + # `js_get_string_pointer_unified` is deliberately NOT here. Its SSO branch + # calls `js_string_materialize_to_heap`, which allocates (value/nanbox.rs:268), + # so it is a collection point by this file's one-sided rule even though the + # collection it can cause is currently always non-moving. See #7213. } # The single site where an evacuating (moving) minor runs. @@ -1166,6 +1170,22 @@ def seeded_violation_test(paths, moving_only, anchor, want_sites, verbose=False) "js_box_get_bits", # box.rs mutable-capture cell read "js_implicit_this_get", # object/this_binding.rs:160 thread-local "js_new_target_get", + # object/this_binding.rs:159 -- `js_implicit_this_SET` is a swap, so its + # RETURN value is a read of the same scanned mutable cell + # (`scan_implicit_this_roots_mut`, this_binding.rs:176) and the swap has + # already overwritten the only other copy. Listing the setter as a reader + # looks odd, which is exactly why #7214 left `prev_this` unrooted for a + # whole PR: the checker saw a call it knew could not collect, never + # classified the result as a heap value, and reported nothing at either + # end. Being non-collecting is what makes a call a root READ. + "js_implicit_this_set", + # `js_get_string_pointer_unified` is a candidate and is deliberately left + # out for now: its result IS a raw heap address in a bare register, but it + # is not in NONCOLLECTING (its SSO branch allocates), so classifying it as + # a source would report the whole `unbox_str_handle` family in one go -- + # roughly forty sites across lower_string_method.rs alone. That is a real + # population and it needs its own measured count and its own triage rather + # than being folded into this change. #7213. } # Argument positions that make a stale pointer FATAL rather than merely wrong: @@ -1646,6 +1666,27 @@ def _stale_probe(path, max_stale): return (int(m.group(1)) if m else -1), rc +def _main_probe(argv): + """Exit status of a full `main()` run over `argv` (no `sys.exit`). + + The guards these arms cover live in `main()`'s argument handling rather + than in a scannable function, and argparse reports a usage error by + raising `SystemExit(2)`. Driving `main()` is the only way to assert them; + calling the inner helpers would skip exactly the code under test. + """ + saved = sys.argv + buf = io.StringIO() + try: + sys.argv = ["gc_root_dominance_check.py"] + list(argv) + with contextlib.redirect_stdout(buf), contextlib.redirect_stderr(buf): + try: + return main() + except SystemExit as exc: + return exc.code if isinstance(exc.code, int) else 1 + finally: + sys.argv = saved + + def self_test(): """Assert the checker reports the planted violation and clears the control. @@ -1716,6 +1757,41 @@ def self_test(): "report 0 uses and exit 0", file=sys.stderr) ok = False + # --- the stale mode's own subject-liveness assertion ---------------- + # + # `--stale-registers` classifies a shadow-slot load as a heap-value + # source by looking the alloca up in a map built from + # `js_shadow_slot_bind`. A corpus with no binds therefore has almost no + # sources, reports `total 0`, and is indistinguishable from a corpus + # with no stale registers. `--min-binds` is what makes that case an + # error, and before #7211 the stale path returned above the guard. + # Both directions, so the guard cannot be "always 2". + if _main_probe(["--stale-registers", "--min-binds", "50", planted]) != 2: + print("self-test FAIL: --stale-registers over a corpus with fewer " + "than --min-binds root stores must exit 2. The scan's " + "shadow-slot sources come from those stores, so a clean " + "verdict over zero of them proves nothing.", file=sys.stderr) + ok = False + if _main_probe(["--stale-registers", "--min-binds", "2", planted]) != 0: + print("self-test FAIL: --stale-registers over a corpus that MEETS " + "--min-binds must still exit 0 (diagnostic). The guard " + "rejects every corpus, which is a gate that always fails.", + file=sys.stderr) + ok = False + # A knob that is silently ignored is a disarmed knob -- the same rule + # `--max-stale` and `--fatal-sinks` already carry, read the other way. + if _main_probe(["--stale-registers", "--any-def", planted]) != 2: + print("self-test FAIL: --any-def with --stale-registers must be a " + "usage error; the stale scan never consults the anchor, so " + "accepting it promises a widening that never happens", + file=sys.stderr) + ok = False + if _main_probe(["--any-def", planted]) != 1: + print("self-test FAIL: --any-def WITHOUT --stale-registers must " + "still run the bind-anchored check and report the planted " + "violations", file=sys.stderr) + ok = False + try: _scan([broken], False, "alloc") except MalformedIR: @@ -1948,6 +2024,15 @@ def main(): ap.error("--max-stale requires --stale-registers") if ns.fatal_sinks and not ns.stale_registers: ap.error("--fatal-sinks requires --stale-registers") + # Same rule read the other way. `--any-def` only selects the bind-anchored + # check's ANCHOR (`anchor = "any" if ns.any_def else "alloc"`, below), and + # the stale-register path never consults it -- it anchors on every + # heap-value source by construction, so there is no narrower or wider + # setting for it to pick. Passing both reads like "widen the stale scan" + # and does nothing at all. + if ns.any_def and ns.stale_registers: + ap.error("--any-def has no effect with --stale-registers " + "(the stale scan already anchors on every heap-value source)") if ns.self_test: return self_test() @@ -1990,9 +2075,6 @@ def main(): parsed.append((os.path.basename(p), parse_file(p))) poll_reaching, _known = compute_poll_reaching( [f for _m, fs in parsed for f in fs]) - if ns.stale_registers: - return run_stale(parsed, poll_reaching, verbose, moving_only, - ns.fatal_sinks, ns.max_stale) n_binds = sum( 1 for _m, fs in parsed @@ -2002,6 +2084,26 @@ def main(): if BIND_RE.search(ins.text) ) + if ns.stale_registers: + # `--min-binds` is a CORPUS-sanity assertion, not a bind-anchored-check + # detail, so it has to be honoured here too. `heap_source_kind` decides + # a `slotload` is a heap-value source by looking the pointer up in + # `slot_of_alloca`, and that map is built from the same `BIND_RE` + # (`stale_uses_in_function`). Compile the corpus with + # PERRY_INLINE_SHADOW_SLOT=1 (or with a broken `--trace llvm`) and + # every shadow-slot source vanishes: `run_stale` reports `total 0`, + # exits 0, and looks exactly like a corpus with no stale registers in + # it. That is hazard 4 -- the gate runs but its subject never did. + if n_binds < ns.min_binds: + print(f"error: {n_binds} root store(s) in the corpus, need at " + f"least {ns.min_binds}. The stale-register scan derives its " + "shadow-slot sources from those stores, so 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 run_stale(parsed, poll_reaching, verbose, moving_only, + ns.fatal_sinks, ns.max_stale) + if ns.unrooted_allocas: total = 0 moving_total = 0 diff --git a/test-files/test_gap_gc_closure_call_prev_this_rooting.ts b/test-files/test_gap_gc_closure_call_prev_this_rooting.ts new file mode 100644 index 0000000000..0d6c9978ac --- /dev/null +++ b/test-files/test_gap_gc_closure_call_prev_this_rooting.ts @@ -0,0 +1,66 @@ +// #7211: the PREVIOUS implicit `this` that `js_closure_callN` saves across a +// dynamic call must be rooted, because the restore publishes it back into a +// root the collector scans. +// +// `js_implicit_this_set(v)` swaps the `IMPLICIT_THIS` cell and returns what was +// there. That cell is a registered MUTABLE root — `scan_implicit_this_roots_mut` +// (object/this_binding.rs:176) marks it and rewrites it on an evacuating cycle. +// The swap has already overwritten it, so the returned value is now held only +// in a bare SSA register, and it stays there across the allocating rebind unbox +// AND the entire user call. A minor inside the callee moves the object, rewrites +// the enclosing frame's copy, and leaves this register naming from-space. The +// restore then writes that pre-move address BACK INTO the cell — so the damage +// outlives the call and lands on whatever reads `this` next. +// +// #7214 fixed the callee, the receiver and every argument of this same lowering +// and deliberately left this one, measured but unfixed. It was invisible to +// scripts/gc_root_dominance_check.py at both ends: `js_implicit_this_set` was +// NONCOLLECTING but not a root READ, so the register had no recognised +// heap-value source, and the restore is not a fatal sink. Both ends are now +// classified, so this shape stays gated as well as fixed. +// +// LIVE BY CONSTRUCTION. `outer` is a non-arrow function assigned onto an object +// literal, so `obj.outer(1)` takes the generic `js_closure_callN` fallthrough +// and binds `obj` as the implicit `this`. `h(v)` inside it is a RECEIVERLESS +// dynamic call, which per #3576 must reset `this` to undefined and restore it — +// that restore is the subject. `this.tag` is then read AFTER the restore, so a +// corrupted cell is observable rather than merely present. + +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 helper(v: number): number { + return churn(v); +} + +function outer(this: any, v: number): number { + const before = this.tag; + // Receiverless dynamic call: saves the current implicit `this` (= this + // object), binds undefined, and restores the saved value afterwards. + const h: any = helper; + const got = h(v); + // Read `this` again BELOW the restore. If the restore republished a + // pre-move address, this reads out of abandoned memory — a wrong number if + // the bytes were recycled, a SIGSEGV if the page was retired. + const after = this.tag; + return before === after ? after + got - v : -1; +} + +function run(): number { + let bad = 0; + for (let r = 0; r < 400; r++) { + const obj: any = { tag: r, outer: outer }; + const got = obj.outer(1); + if (got !== r) { + bad++; + } + } + return bad; +} + +console.log("bad", run()); diff --git a/test-files/test_gap_gc_typeof_string_cache_rooting.ts b/test-files/test_gap_gc_typeof_string_cache_rooting.ts new file mode 100644 index 0000000000..75d3275f5d --- /dev/null +++ b/test-files/test_gap_gc_typeof_string_cache_rooting.ts @@ -0,0 +1,69 @@ +// #7211: the interned `typeof` result strings are GC roots and must be +// registered as such. +// +// `js_value_typeof` (builtins/arithmetic.rs) returns one of eight strings and +// caches each in a thread-local `Cell<*mut StringHeader>` so it is built once +// rather than per call. Those cells held a RAW pointer into the NURSERY and +// nothing else referenced the string, so the first minor collection either +// swept it or evacuated it — and the cache kept naming the abandoned bytes for +// the rest of the process. Every later `typeof x === "…"` then handed +// `js_string_equals` a from-space address. +// +// This is the bug that kept `sfw-registry --help` red under a genuine +// `PERRY_GC_MOVING_LOOP_POLLS=1` build after #7206 and #7214 had closed every +// stale register they could find. It is worth naming the difference, because it +// is why the previous rounds could not find it: +// +// * a #7154-class stale REGISTER goes bad only if a collection happens to +// land inside a few-instruction window, so it is timing-dependent and needs +// a workload plus repetition to surface; +// * an unregistered CACHE goes bad at the first collection and stays bad, so +// it fails 10/10 — and it is invisible to +// `scripts/gc_root_dominance_check.py`, which reads emitted LLVM IR and +// cannot see a runtime-side table at all. +// +// The from-space reporter named it exactly: `obj_type=3 size=40 +// retired_by_minor=#0` — a string, a 32-byte header plus `"string"`, retired by +// the very first collection. +// +// LIVE BY CONSTRUCTION. Every `typeof` here is applied to a value read out of an +// array at a runtime index, so none of it folds at compile time and each call +// really does reach `js_value_typeof` and really does read the cache. The churn +// forces minor collections between the first population of the cache and the +// later reads, which is the whole subject: the FIRST iteration primes the cache +// and the ones after it are the test. + +function churn(x: number): number { + const bits: any[] = []; + for (let i = 0; i < 400; i++) { + bits.push({ i: i, s: "x" }); + } + return x + bits.length - 400; +} + +function run(): number { + let bad = 0; + const vals: any[] = ["a", 1, true, {}, undefined, run]; + const want: string[] = [ + "string", + "number", + "boolean", + "object", + "undefined", + "function", + ]; + for (let r = 0; r < 500; r++) { + churn(r); + for (let k = 0; k < 6; k++) { + // Reads the cached string and compares it against a literal. A cache + // entry the collector moved but never rewrote makes this compare read + // from-space. + if (typeof vals[k] !== want[k]) { + bad++; + } + } + } + return bad; +} + +console.log("bad", run()); From 28dc70d27b00e43aed35241daadc38783fe0943b Mon Sep 17 00:00:00 2001 From: jdalton Date: Sat, 1 Aug 2026 23:39:10 -0400 Subject: [PATCH 2/5] fix(gc): root the regexp receiver across ToString, and audit ALLOC_RE MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Expr::RegExpTest` / `Expr::RegExpExec` unboxed the receiver to a raw `RegExpHeader*` before emitting `js_jsvalue_to_string_coerce`, which allocates and dispatches a user `toString`. Under a moving minor the register named from-space by the time `js_regexp_test` dereferenced it — #7154's residual, at `defineApiCall + 404` in the registry. Both take the established `guard_store_operand_across` / `reread_store_operand` pair and the unbox moves below the coerce. The checker missed it because `ALLOC_RE` spelled the allocator `regexp_alloc\w*` and the call is `js_regexp_new`. Reconciling every alternative against the runtime's real symbol table found four that match nothing at all (`regexp_alloc`, `promise_alloc`, `bigint_alloc`, `typed_array_alloc`): the runtime materializes fresh GC objects under three naming conventions (`_alloc*`, `_new*`, `_create*`) and the pattern modelled one. All three are now matched as conventions, with the non-conforming constructors enumerated explicitly. Second blind spot, and the one that matters for CI: the ToPrimitive family was not `POLL_CAPABLE_RUNTIME`, so even with `ALLOC_RE` widened the site was invisible to `--moving-only`, which is the mode the gate runs. Refs #7154, #7226, #7211, #7161 --- ...exp-receiver-rooting-and-alloc-re-audit.md | 201 ++++++++++++++++++ .../perry-codegen/src/expr/instance_misc1.rs | 46 +++- scripts/gc_root_dominance_check.py | 115 +++++++++- .../test_gap_gc_regexp_receiver_rooting.ts | 80 +++++++ 4 files changed, 429 insertions(+), 13 deletions(-) create mode 100644 changelog.d/7227-regexp-receiver-rooting-and-alloc-re-audit.md create mode 100644 test-files/test_gap_gc_regexp_receiver_rooting.ts diff --git a/changelog.d/7227-regexp-receiver-rooting-and-alloc-re-audit.md b/changelog.d/7227-regexp-receiver-rooting-and-alloc-re-audit.md new file mode 100644 index 0000000000..2a66dd37ce --- /dev/null +++ b/changelog.d/7227-regexp-receiver-rooting-and-alloc-re-audit.md @@ -0,0 +1,201 @@ +### Fixed + +- **`codegen`: the receiver of `re.test(s)` / `re.exec(s)` is rooted across the + `ToString(s)` coercion the lowering emits below it** (#7154). Both + `Expr::RegExpTest` and `Expr::RegExpExec` lowered the receiver first, unboxed + it to a raw `RegExpHeader*` in a bare SSA register, and only then emitted + `js_jsvalue_to_string_coerce`. That coerce is not a bystander: it allocates, + and on an object argument it dispatches a user `[Symbol.toPrimitive]` / + `toString` / `valueOf`, which is arbitrary JS with its own loop back-edge + polls. Under `PERRY_GC_MOVING_LOOP_POLLS=1` one of those polls runs an + evacuating minor while the regexp is live only in that register. + + This is the residual #7226 measured and named rather than fixed. In the + `sfw-registry` reproducer it is `src/lib/api/shared.ts:67`, + `/\[[a-zA-Z]+\]/.test(url)`, faulting at + `perry_fn_src_lib_api_shared_ts__defineApiCall + 404`: + + ```asm + bl js_regexp_new ; ALLOCATES + and x20, x0, #0xffffffffffff ; raw regexp pointer -> bare register + ldr d0, [sp, #0x28] + bl js_jsvalue_to_string_coerce ; ALLOCATES, runs user toString + mov x0, x20 ; STALE + bl js_regexp_test ; faults here + ``` + + The receiver now takes the established `guard_store_operand_across` / + `reread_store_operand` pair, and the unbox moves BELOW the coerce — unboxing + above it is what parked the pre-move address in a register in the first + place. `RegExpExec` had the identical defect and is fixed with it. + +### Changed + +- **`scripts/gc_root_dominance_check.py`: `ALLOC_RE` audited against the + runtime's real symbol table instead of an assumed naming convention.** This + is the more valuable half of the change, because the miss above was the + *second* allocator to escape this pattern (`js_implicit_this_set` was the + first, #7226) and each one has cost a full investigation round. + + The old pattern carried an alternative spelled `regexp_alloc\w*`. **No such + symbol has ever existed.** It was not a typo — it was an extrapolation: + whoever wrote it knew a RegExp allocates and inferred the `_alloc` suffix + from its neighbours. Reconciling every alternative against + `extern "C" fn js_\w+` over perry-runtime + perry-stdlib, intersected with + the names perry-codegen actually declares, found **four alternatives matching + nothing at all** — `regexp_alloc\w*`, `promise_alloc\w*`, `bigint_alloc\w*` + and `typed_array_alloc\w*`. A quarter of the pattern was decorative. + + The root cause is that the runtime materializes fresh GC objects under + **three** naming conventions and the pattern modelled one: + + | convention | examples | matched before | + |---|---|---| + | `*_alloc*` | `js_object_alloc`, `js_array_alloc`, `js_closure_alloc`, `js_uint8array_alloc`, `js_inline_arena_slow_alloc` | yes | + | `*_new*` | `js_regexp_new`, `js_promise_new`, `js_symbol_new`, `js_date_new`, `js_error_new`, `js_typed_array_new`, `js_weakmap_new`, `js_url_new`, `js_boxed_string_new`, ~140 more | **no** | + | `*_create*` | `js_object_create`, `js_array_create`, `js_vm_create_context`, `js_crypto_create_hash`, ~40 more | only `object_create*` | + + All three are now matched as conventions, and the constructors that use none + of them are enumerated explicitly: the `_construct*` ctor forms, the fresh + string producers (`string_coerce`, `jsvalue_to_string*`, `string_slice`, + `string_to_*_case`, `string_pad_*`, `string_trim*`, …), the copy-on-read and + ES2023 change-by-copy array family (`array_to_sorted*`, `array_to_spliced`, + `array_with`, `array_flat*`, `array_like_to_array`, `iterator_to_array`, …), + the whole-object producers (`object_keys*`, `object_entries*`, + `object_from_entries`, `object_get_own_property_descriptor*`, + `structured_clone*`, the Set-methods family), the namespace/class-shape + helpers, and BigInt's `bigint_from*` (which has neither `_alloc` nor `_new`). + + Widening is safe in the checker's one-sided direction: a name that turns out + not to allocate costs a false positive to triage, while a missing one costs a + shipped use-after-free plus the round it takes to find by hand. The file now + says so, so the next person extends it rather than guessing. + +- **`scripts/gc_root_dominance_check.py`: the ToPrimitive / ToString / ToNumber + coercion family is `POLL_CAPABLE_RUNTIME`.** This is the second half of the + same blind spot and it is the half that matters for CI, because + `--moving-only` is the mode `gc-root-dominance.yml` gates on. With `ALLOC_RE` + widened but the coercions unmodelled, the `/re/.test(s)` site was reported by + the raw `--stale-registers` count and **still invisible to `--moving-only`**: + nothing in its window was classified as reaching a moving minor. A coercion + does not look like a call into user code, but ToPrimitive is exactly that — + and `js_string_coerce`'s own doc comment already said so ("a `POINTER_TAG` + object routes through `js_jsvalue_to_string`, which can invoke a user + `toString` / `valueOf`"). The checker just never read it. + +- **`scripts/gc_root_dominance_check.py`: `js_regexp_test` / `js_regexp_exec` + are fatal sinks.** A stale `RegExpHeader*` is dereferenced immediately by + both, and this one faulted rather than merely answering wrong, so it belongs + in the `--fatal-sinks` ranking and not only in the raw count. + +## Verification + +Measured against the parent (`4e99c1bad`, #7226's head), built from this +worktree rather than borrowed from another one. + +The checker change is what makes the codegen change checkable, so it is +reported first. Over the gap-test IR for the new reproducer: + +| `--stale-registers` over `test_gap_gc_regexp_receiver_rooting.ts` | parent | this PR | +|---|---|---| +| base checker (`regexp_alloc\w*`) | **0 reported** | — | +| widened `ALLOC_RE`, raw count | 3 | **0** | +| widened + `--moving-only` (the gate's mode) | **3**, `MOVING: YES via js_jsvalue_to_string_coerce` | **0** | + +All three are named exactly: `source (alloc): call i64 @js_regexp_new`, +`stale use: call i32 @js_regexp_test` / `@js_regexp_exec`, with +`js_jsvalue_to_string_coerce` in the window. + +Over the 130-module / 2170-function gap corpus, both checker widenings +together: + +| `--moving-only` | parent | this PR | +|---|---|---| +| bind-anchored violations (**the gate**) | 0 | **0**, allowlist still empty | +| `--stale-registers`, total | 2730 | 2738 | +| `--stale-registers --moving-only` | 2 | **62** | +| `--stale-registers --fatal-sinks` | 279 | 282 | +| `--stale-registers --moving-only --fatal-sinks` | 0 | **0** | +| `--unrooted-allocas`, moving-reachable | 57 | 85 | + +The gate does not move. The 60 newly-*moving* stale-register leads were all +already in the 2730-entry diagnostic list; modelling the coercions is what +reclassified their windows. Triaged mechanically by the shape of the stale +use: + +| stale use | count | verdict | +|---|---|---| +| `lshr … , 48` | 37 | NaN-box **tag** read. Relocation rewrites the low 48 bits; the tag is unchanged. Not a bug. | +| `fadd double` | 3 | float arithmetic on a value the mode could not prove non-pointer. Not a bug. | +| `getelementptr i8, …, 32` | 15 | direct field access in the `*__pshape` pointer-shape specializations. A real dereference shape and a real population — **left for its own PR**, since it is the `PERRY_PTR_SHAPE_LOCALS` family and wants its own measured count. | +| call argument | 7 | typed-feedback array receivers held across `js_number_coerce`. Same call: real shape, own population, own PR. | + +Nothing in the newly-visible set is a fatal sink, which is why the fatal count +moves only by the three regexp entries this PR then fixes. + +Gap test — `test_gap_gc_regexp_receiver_rooting.ts`, compiled **and** run with +`PERRY_GC_MOVING_LOOP_POLLS=1`: + +| | parent | this PR | +|---|---|---| +| `POLLS=1` + `PERRY_GC_ZEAL=1` | **0/10 — SIGSEGV/SIGBUS every run** | `bad 0` **10/10** | +| `POLLS=1` + zeal + `PERRY_GEN_GC=0` | `bad 0` | `bad 0` | + +The parent arm is a hard fault rather than a nonzero `bad`, and that is the +honest signature: with a regex **literal** receiver — the registry's shape — +`js_regexp_new`'s result is held only in the register, so the evacuating minor +retires the block under it and the deref lands in from-space. The +`PERRY_GEN_GC=0` arm proves the test tracks collector mode rather than being +flaky. Zeal is required for the same structural reason #7226 recorded for +`prev_this`: the window is a user call, so only a *moving* collection exploits +it, and allocation-triggered collections take +`ManualGcScanGuard::force_full_scan`, which makes the copying minor ineligible. + +## `sfw-registry` moves, and does NOT reach 30/30 + +`sfw-registry --help`, 141 modules, `PERRY_FORCE_WELL_KNOWN=iovalkey`, compiled +**and** run with `PERRY_GC_MOVING_LOOP_POLLS=1`: + +| | parent (`4e99c1bad`) | this PR | +|---|---|---| +| `POLLS=1`, **30 runs** | **0/30** | **28/30** | + +Both arms use the same runtime archives (the codegen fix is the only +difference) and the same firewall tree, so the comparison is like-for-like. +The parent's 30 failures are **not** crashes — every one is a deterministic +`TypeError: Cannot convert undefined or null to object`, which is what a stale +regexp receiver produces here: `defineApiCall` computes +`urlRequiresInterpolation = /\[[a-zA-Z]+\]/.test(url)` at definition time, the +stale read returns the wrong boolean, and the wrong branch hands `undefined` +to a downstream `Object` operation. The fix removes that failure mode +completely. + +Note that #7226 reported 26/30 for this same parent commit. That measurement +was taken against a different firewall checkout; on the tree measured here the +parent is 0/30. The delta this PR is responsible for is the one measured above, +on one tree, with one runtime. + +**This does not close #7154 and #7161's stopgap stays.** Two runs in thirty +still SIGSEGV, and the residual is a *different* object from the one this PR +fixes. Under `PERRY_GC_MOVING_LOOP_POLLS=1 PERRY_GC_PROTECT_FROMSPACE=1 +PERRY_GC_PROTECT_FROMSPACE_DEPTH=800` it is deterministic — **40/40** — and the +reporter names it: + +``` +[gc-fromspace-protect] FAULT: signal 10 at 0x…5d5c + block=0x…20000 +220508 retired_bytes=253416 retired_by_minor=#155 + last-known object: user_ptr=0x…5d58 obj_type=3 size=80 +``` + +`obj_type=3` is a **string**, not a `RegExpHeader`, so it is not the receiver +this PR rooted. Disassembling the faulting frame confirms it: the return +address is `defineApiCall + 428`, and `+424` is `bl js_regexp_test` — the same +call site, twenty bytes further along, which is exactly the size of the +`js_gc_temp_root_push` / `js_gc_temp_root_get` pair this PR inserts. The +receiver operand is now correct; the surviving stale value is a string reaching +the same call. + +That the protected arm is 40/40 while the unprotected arm is 2/30 is the useful +part: the next round has a deterministic reproducer instead of a 7 % one. It is +**not** fixed speculatively here — no edit ships without a test that can fail +without it. diff --git a/crates/perry-codegen/src/expr/instance_misc1.rs b/crates/perry-codegen/src/expr/instance_misc1.rs index 0a206a77ab..c68ca6b52d 100644 --- a/crates/perry-codegen/src/expr/instance_misc1.rs +++ b/crates/perry-codegen/src/expr/instance_misc1.rs @@ -1064,22 +1064,44 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { // a NaN-tagged string. Both must be unboxed before the call. Expr::RegExpTest { regex, string } => { let regex_box = lower_expr(ctx, regex)?; + // #7154: the receiver is live across BOTH the string operand's own + // lowering and the `js_jsvalue_to_string_coerce` below it, and the + // coerce is unconditional — it allocates, and on an object argument + // it runs a user `toString`, which is arbitrary JS with its own + // back-edge polls. So the window always exists and `collects` is + // `true` rather than a `expr_may_trigger_gc(string)` test. + // + // This is the site the registry reproducer faults at + // (`defineApiCall + 404`, `obj_type=3 size=80`): `js_regexp_new`'s + // raw result went into a bare `x20`, the coerce drove an evacuating + // minor that moved it, and `js_regexp_test` dereferenced from-space. + // The static checker could not see it because `ALLOC_RE` spelled the + // allocator `regexp_alloc\w*` and the call is `js_regexp_new`. + let guard = super::temp_root::guard_store_operand_across(ctx, regex, ®ex_box, true); let str_box = lower_expr(ctx, string)?; - let blk = ctx.block(); - let regex_handle = unbox_to_i64(blk, ®ex_box); // Per spec `RegExp.prototype.test` does `ToString(argument)`, so a // String wrapper (`re.test(new String("x"))`), a number // (`re.test(123)`), or an object with a custom `toString` must be // coerced — and a throwing `toString`/`valueOf` must propagate. // `js_get_string_pointer_unified` only unwraps real strings, so use // the coercing ToString that dispatches `toString` on objects. - let str_handle = blk.call(I64, "js_jsvalue_to_string_coerce", &[(DOUBLE, &str_box)]); + let str_handle = + ctx.block() + .call(I64, "js_jsvalue_to_string_coerce", &[(DOUBLE, &str_box)]); + // Re-read BELOW the coerce, then unbox. Unboxing above it is what + // parked the pre-move address in a register in the first place. + let regex_box = super::temp_root::reread_store_operand(ctx, &guard, regex, ®ex_box)?; + let blk = ctx.block(); + let regex_handle = unbox_to_i64(blk, ®ex_box); let i32_v = blk.call( I32, "js_regexp_test", &[(I64, ®ex_handle), (I64, &str_handle)], ); - Ok(i32_bool_to_nanbox(blk, &i32_v)) + let out = i32_bool_to_nanbox(ctx.block(), &i32_v); + // After the call: `js_regexp_test` allocates while reading these. + super::temp_root::release_store_operand(ctx, guard); + Ok(out) } Expr::RegExpExec { regex, string } => { // Returns ArrayHeader* or null. For a null (0) result we must @@ -1088,18 +1110,28 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { // non-null pointer value that compares unequal to null, causing // infinite loops + segfaults when callers IndexGet on the result. let regex_box = lower_expr(ctx, regex)?; + // #7154, identical shape to `RegExpTest` above and found with it: + // the receiver is live across the string operand's lowering and + // across the unconditional coerce, which allocates and can run a + // user `toString`. + let guard = super::temp_root::guard_store_operand_across(ctx, regex, ®ex_box, true); let str_box = lower_expr(ctx, string)?; - let blk = ctx.block(); - let regex_handle = unbox_to_i64(blk, ®ex_box); // `RegExp.prototype.exec` does `ToString(argument)` — coerce String // wrappers / numbers / objects (and propagate a throwing toString) // rather than only unwrapping real strings (see RegExpTest above). - let str_handle = blk.call(I64, "js_jsvalue_to_string_coerce", &[(DOUBLE, &str_box)]); + let str_handle = + ctx.block() + .call(I64, "js_jsvalue_to_string_coerce", &[(DOUBLE, &str_box)]); + let regex_box = super::temp_root::reread_store_operand(ctx, &guard, regex, ®ex_box)?; + let blk = ctx.block(); + let regex_handle = unbox_to_i64(blk, ®ex_box); let result = blk.call( I64, "js_regexp_exec", &[(I64, ®ex_handle), (I64, &str_handle)], ); + super::temp_root::release_store_operand(ctx, guard); + let blk = ctx.block(); // Branch on result == 0 → TAG_NULL; else NaN-box as pointer. let is_null = blk.icmp_eq(I64, &result, "0"); let ptr_boxed = nanbox_pointer_inline(ctx.block(), &result); diff --git a/scripts/gc_root_dominance_check.py b/scripts/gc_root_dominance_check.py index be88869222..7b37f2fb9a 100755 --- a/scripts/gc_root_dominance_check.py +++ b/scripts/gc_root_dominance_check.py @@ -282,13 +282,90 @@ def build_cfg(f): MOVING_POLL = "js_gc_loop_safepoint" # Result-producing calls that materialize a fresh GC object. +# +# ------------------------------------------------------------------ THE AUDIT +# This list is enumerated from the runtime's actual exported entry points, NOT +# guessed from a naming convention, because guessing a convention is precisely +# how it has failed twice: +# +# * `js_implicit_this_set` (#7226) -- a root READ that was not modelled, so +# `prev_this` survived two PRs that were looking straight at it; +# * `js_regexp_new` (#7154, this change) -- the regex carried an alternative +# spelled `regexp_alloc\w*`, and **no such symbol has ever existed**. The +# `/re/.test(s)` lowering holds `js_regexp_new`'s raw result in a register +# across `js_jsvalue_to_string_coerce`, and the checker reported nothing +# because the register had no recognised heap-value source. +# +# The second one is the instructive one. `regexp_alloc\w*` was not a typo, it +# was an ASSUMED convention: whoever wrote it knew a RegExp allocates and +# extrapolated the `_alloc` suffix from its neighbours. Reconciling every +# alternative below against the real symbol table (`grep -rhoE 'extern "C" fn +# js_\w+'` over perry-runtime + perry-stdlib, intersected with the names +# perry-codegen actually declares) found FOUR alternatives in the same state -- +# `regexp_alloc\w*`, `promise_alloc\w*`, `bigint_alloc\w*` and +# `typed_array_alloc\w*` matched nothing whatsoever. A quarter of the pattern +# was decorative. +# +# The root cause is that the runtime materializes fresh GC objects under THREE +# naming conventions and the old pattern modelled one: +# +# `_alloc*` js_object_alloc, js_array_alloc, js_closure_alloc, js_box_alloc, +# js_map_alloc, js_set_alloc, js_buffer_alloc, js_uint8array_alloc, +# js_arguments_object_alloc, js_inline_arena_slow_alloc, ... +# `_new*` js_regexp_new, js_promise_new, js_symbol_new, js_date_new, +# js_error_new, js_typed_array_new, js_weakmap_new, js_weakset_new, +# js_url_new, js_boxed_string_new, js_array_buffer_new, ~140 more +# `_create*` js_object_create, js_array_create, js_vm_create_context, +# js_crypto_create_hash, js_readline_create_interface, ~40 more +# +# So the three conventions are matched as conventions, and the constructors +# that use none of them are enumerated explicitly below. Widening is SAFE in +# the checker's one-sided direction: a name that is in fact not an allocation +# costs a false positive to triage, while a missing one costs a shipped +# use-after-free plus the investigation round it takes to find it by hand. +# When in doubt, add it. ALLOC_RE = re.compile( r"^js_(" - r"object_alloc\w*|array_alloc\w*|closure_alloc\w*|box_alloc\w*|" - r"string_alloc\w*|string_concat\w*|string_coerce|string_from\w*|" - r"map_alloc\w*|set_alloc\w*|promise_alloc\w*|bigint_alloc\w*|" - r"typed_array_alloc\w*|buffer_alloc\w*|regexp_alloc\w*|" - r"object_create\w*|array_from\w*|build_class_keys_array" + # -- convention 1: `*_alloc*`, including the arena's own slow path. + r"\w*_alloc\w*|" + # -- convention 2: `*_new*`. `js_regexp_new` is the #7154 residual. + r"\w*_new\w*|" + # -- convention 3: `*_create*`. + r"\w*_create\w*|create_\w+|" + # -- constructors using none of the three conventions ------------------- + # `new X(...)` / `X(...)` forms folded at HIR into a direct ctor call. + r"\w*_construct|\w*_construct_call|\w*_construct_apply|" + r"reflect_construct|new_function_construct\w*|super_construct_apply|" + # fresh strings. Every one of these returns a *mut StringHeader the + # caller holds raw; `string_coerce` and `jsvalue_to_string_coerce` are + # the two the `/re/` lowerings feed. + r"string_concat\w*|string_coerce|string_from\w*|string_append|" + r"jsvalue_to_string\w*|value_to_string\w*|number_to_string\w*|" + r"string_repeat|string_slice|string_substring|string_substr|" + r"string_pad_\w+|string_trim\w*|string_to_\w+_case|string_replace\w*|" + r"string_split|string_normalize|string_at|string_char_at|" + r"string_index_get_boxed|boxed_string\w*|" + # fresh arrays: the copy-on-read Array.prototype methods, the ES2023 + # change-by-copy family, and the iterable/array-like converters. + r"array_from\w*|array_clone\w*|array_concat\w*|array_slice|array_splice|" + r"array_to_spliced|array_to_sorted\w*|array_to_reversed|array_with|" + r"array_flat\w*|array_map|array_filter|array_like_to_array|" + r"iterator_to_array|array_of|array_group_by|" + # fresh objects/collections handed back as a whole + r"object_keys\w*|object_values\w*|object_entries\w*|object_from_entries|" + r"object_assign\w*|object_group_by|object_coerce|" + r"object_get_own_property_descriptor\w*|object_get_own_property_names|" + r"object_get_own_property_symbols|" + r"map_from_iterable|set_from_iterable|map_group_by|" + r"set_union|set_intersection|set_difference|set_symmetric_difference|" + r"structured_clone\w*|" + # module namespace objects, class-shape side tables, generator plumbing + r"build_class_keys_array|create_namespace|create_native_module_namespace|" + r"generator_attach_prototype|proxy_revocable|" + # BigInt: no `_alloc` and no `_new`; the constructors are `_from_*`. + r"bigint_from\w*|bigint_\w+_op|" + # Buffers / typed arrays that spell it neither way. + r"buffer_from\w*|typed_array_from\w*|array_buffer_slice" r")$" ) @@ -362,6 +439,21 @@ def is_collecting(callee): # ------------------------------------------------- interprocedural poll reach # Runtime helpers that re-enter compiled JS (and therefore its back-edge polls). +# +# The COERCION family is the half of this set that is easy to leave out, and +# leaving it out is what kept `--moving-only` blind to #7154's `/re/.test(s)` +# residual even once `ALLOC_RE` had been widened to recognise `js_regexp_new`. +# A coercion does not *look* like a call into user code, but ToPrimitive is +# exactly that: `js_jsvalue_to_string_coerce` runs `to_string_method_impl(…, +# skip_to_primitive = false)`, which consults `[Symbol.toPrimitive]`, then +# `toString`, then `valueOf` — arbitrary JS, with its own loop back-edge polls. +# `js_string_coerce`'s own doc comment already said so ("a `POINTER_TAG` object +# routes through `js_jsvalue_to_string`, which can invoke a user `toString` / +# `valueOf`"); the checker just never read it. +# +# This matters more than the raw-count modes suggest, because `--moving-only` +# is the mode the `gc-root-dominance.yml` gate runs. A source the gate cannot +# classify as reaching a moving minor is a source the gate cannot fail on. POLL_CAPABLE_RUNTIME = { "js_call_function", "js_call_closure", "js_invoke_closure", "js_call_value", "js_apply_function", "js_function_call", @@ -370,6 +462,13 @@ def is_collecting(callee): "js_array_sort", "js_array_map", "js_array_filter", "js_array_for_each", "js_array_reduce", "js_json_stringify", "js_string_replace", "js_promise_run_microtasks", "js_gc_loop_safepoint", + # ToPrimitive / ToString / ToNumber: every one of these dispatches a user + # `[Symbol.toPrimitive]` / `toString` / `valueOf` on an object operand. + "js_to_primitive", + "js_jsvalue_to_string", "js_jsvalue_to_string_coerce", + "js_jsvalue_to_string_method", "js_jsvalue_to_string_radix", + "js_string_coerce", "js_string_coerce_method_this", + "js_number_coerce", "js_object_coerce", } @@ -1198,7 +1297,11 @@ def seeded_violation_test(paths, moving_only, anchor, want_sites, verbose=False) r"object_get_property|object_set_property|put_value_set_dyn_ic|" r"get_value_dyn_ic|closure_call\w*|call_closure|call_function|" r"call_value|invoke_closure|apply_function|" - r"array_\w+|map_\w+|set_\w+|typed_feedback_\w*call\w*" + r"array_\w+|map_\w+|set_\w+|typed_feedback_\w*call\w*|" + # A stale RegExpHeader* is dereferenced immediately by both of these — + # this is #7154's residual, and it faulted rather than merely answering + # wrong, so it belongs in the fatal ranking and not just the raw count. + r"regexp_test|regexp_exec|regexp_match\w*|regexp_replace\w*" r")$" ) diff --git a/test-files/test_gap_gc_regexp_receiver_rooting.ts b/test-files/test_gap_gc_regexp_receiver_rooting.ts new file mode 100644 index 0000000000..c30e12b737 --- /dev/null +++ b/test-files/test_gap_gc_regexp_receiver_rooting.ts @@ -0,0 +1,80 @@ +// #7154: the RECEIVER of `re.test(s)` / `re.exec(s)` must be rooted across the +// `ToString(s)` coercion the lowering emits below it. +// +// `Expr::RegExpTest` / `Expr::RegExpExec` lowered the receiver first, unboxed it +// to a raw `RegExpHeader*` in a bare SSA register, and only THEN emitted +// `js_jsvalue_to_string_coerce`. That coerce is not a bystander: it allocates, +// and on an object argument it dispatches a user `toString`, which is arbitrary +// JS with its own loop back-edge polls. Under `PERRY_GC_MOVING_LOOP_POLLS=1` one +// of those polls runs an evacuating minor while the regexp is live only in that +// register, and `js_regexp_test` then dereferences abandoned from-space memory. +// +// This is the residual #7226 measured and named rather than fixed. In the +// `sfw-registry` reproducer it is `src/lib/api/shared.ts:67`, +// `/\[[a-zA-Z]+\]/.test(url)`, faulting at +// `perry_fn_src_lib_api_shared_ts__defineApiCall + 404`: +// +// bl js_regexp_new ; ALLOCATES +// and x20, x0, #0xffffffffffff ; raw regexp pointer -> bare register +// bl js_jsvalue_to_string_coerce ; ALLOCATES, runs user toString +// mov x0, x20 ; STALE +// bl js_regexp_test ; faults here +// +// The static checker could not see it, and that is the other half of the bug: +// `ALLOC_RE` carried an alternative spelled `regexp_alloc\w*` while the call is +// named `js_regexp_new`, so the register had no recognised heap-value source. +// No such symbol as `js_regexp_alloc` has ever existed. +// +// LIVE BY CONSTRUCTION. Both arms use a regex LITERAL receiver, which is the +// registry's shape and the strongest one: `js_regexp_new`'s result is held +// ONLY in the register, so nothing else keeps it alive across the coerce. The +// coercion allocates long enough that the minor runs early inside it and the +// abandoned bytes are then reused by the rest of the coercion's own work. The +// answers are checked against known-correct booleans and match text, so a stale +// read is observable rather than latent. Clean under `PERRY_GEN_GC=0`, so the +// evacuating arms are the ones that bite. + +// The loop is what matters, not its trip count: under `PERRY_GC_ZEAL=1` the +// FIRST back-edge poll inside it already runs an evacuating minor, which is +// the collection the receiver has to survive. The count is kept modest on +// purpose — zeal collects at every safepoint, so a 4000-trip churn (what the +// sibling #7154 tests use, where the collection has to arrive on its own +// budget) turns this file into a multi-hour run for no extra coverage. +function churn(tag: string): string { + const bits: any[] = []; + for (let i = 0; i < 120; i++) { + bits.push({ i: i, s: "x", pad: [i, i + 1, i + 2] }); + } + return bits.length === 120 ? tag : "unreachable"; +} + +class Coercer { + tag: string; + constructor(tag: string) { + this.tag = tag; + } + toString(): string { + return churn(this.tag); + } +} + +function run(): number { + let bad = 0; + for (let r = 0; r < 40; r++) { + // `.test` — the exact registry site. + if (!/^tag-[0-9]+$/.test(new Coercer("tag-" + r) as any)) { + bad++; + } + if (/^tag-[0-9]+$/.test(new Coercer("nope-" + r) as any)) { + bad++; + } + // `.exec` — the same lowering with the same defect, fixed with it. + const m = /^tag-([0-9]+)$/.exec(new Coercer("tag-" + r) as any); + if (m === null || m[1] !== String(r)) { + bad++; + } + } + return bad; +} + +console.log("bad", run()); From d8a3a354091c49322e255177cbd814e51e6af73e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sun, 2 Aug 2026 05:50:01 +0200 Subject: [PATCH 3/5] fix(ci): apply the --min-funcs breadth floor in the stale-register and unrooted-alloca modes Both modes return before the floor was evaluated, so `--stale-registers --min-funcs 1200` and `--unrooted-allocas --min-funcs 1200` ran to a verdict over a corpus too thin to have exercised anything and exited 0. A documented liveness control silently disabled by a mode flag, in the script whose job is to catch gates that cannot fail. Reproduced on the self-test fixture (2 functions): stale and alloca modes both exited 0 at --min-funcs 50 while the bind-anchored default correctly exited 2. After the fix all three exit 2. n_funcs is computed above the mode branches and the message lives in one helper. self_test() gains a failing and a passing arm for each of the three modes, so neither a skipped floor nor an always-on one can regress silently. --- scripts/gc_root_dominance_check.py | 64 ++++++++++++++++++++++++++---- 1 file changed, 57 insertions(+), 7 deletions(-) diff --git a/scripts/gc_root_dominance_check.py b/scripts/gc_root_dominance_check.py index 634616cdc4..5c75ba8cb5 100755 --- a/scripts/gc_root_dominance_check.py +++ b/scripts/gc_root_dominance_check.py @@ -1778,6 +1778,33 @@ def self_test(): "rejects every corpus, which is a gate that always fails.", file=sys.stderr) ok = False + # `--min-funcs` is the BREADTH floor, and it used to be evaluated below + # the mode branches -- so `--stale-registers` and `--unrooted-allocas` + # both returned before reaching it and ran to a verdict over a corpus + # too thin to have exercised anything. A documented liveness control + # disarmed by a mode flag, inside the script whose job is to catch + # exactly that. The planted fixture has 2 functions; `--min-funcs 50` + # must be an error in every mode and `--min-funcs 2` must not be one in + # any of them, so the guard can be neither skipped nor always-on. + for mode in (["--stale-registers"], ["--unrooted-allocas"], []): + label = mode[0] if mode else "(bind-anchored default)" + if _main_probe(mode + ["--min-funcs", "50", "--min-binds", "1", + planted]) != 2: + print(f"self-test FAIL: {label} over a 2-function corpus must " + "exit 2 at --min-funcs 50. A mode that returns above the " + "breadth floor silently disables it.", file=sys.stderr) + ok = False + for mode, want in ((["--stale-registers"], 0), + (["--unrooted-allocas"], 0), + ([], 1)): + label = mode[0] if mode else "(bind-anchored default)" + if _main_probe(mode + ["--min-funcs", "2", "--min-binds", "1", + planted]) != want: + print(f"self-test FAIL: {label} over a corpus that MEETS " + f"--min-funcs must exit {want}, not be rejected by the " + "breadth floor. A guard that rejects every corpus is a " + "gate that always fails.", file=sys.stderr) + ok = False # A knob that is silently ignored is a disarmed knob -- the same rule # `--max-stale` and `--fatal-sinks` already carry, read the other way. if _main_probe(["--stale-registers", "--any-def", planted]) != 2: @@ -2090,6 +2117,29 @@ def main(): if BIND_RE.search(ins.text) ) + # `--min-funcs` is a corpus-BREADTH floor that belongs to every mode, so it + # is computed HERE, above the mode branches, and not below them. + # + # It used to be computed below. `--stale-registers` and `--unrooted-allocas` + # both return before that point, so `--stale-registers --min-funcs 1200` ran + # to a verdict over a two-function corpus and exited 0 -- a documented + # liveness control silently disarmed by a mode flag, in the very script whose + # job is to catch gates that cannot fail. Reproduced on the self-test + # fixture (2 functions): stale mode and alloca mode both exited 0 at + # `--min-funcs 50` while the default bind-anchored mode correctly exited 2. + # Both modes consult it before returning now, and `self_test()` carries a + # failing and a passing arm for each so it cannot regress silently. + n_funcs = sum(len(fs) for _m, fs in parsed) + + def funcs_floor_violated(): + if n_funcs >= ns.min_funcs: + return False + 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 True + if ns.stale_registers: # `--min-binds` is a CORPUS-sanity assertion, not a bind-anchored-check # detail, so it has to be honoured here too. `heap_source_kind` decides @@ -2107,6 +2157,10 @@ def main(): "here means the IR was not the IR you think it is " "(compile with PERRY_INLINE_SHADOW_SLOT=0).", file=sys.stderr) return 2 + # Breadth, same as the bind-anchored path applies below. A corpus can + # carry plenty of root stores and still be one module deep. + if funcs_floor_violated(): + return 2 return run_stale(parsed, poll_reaching, verbose, moving_only, ns.fatal_sinks, ns.max_stale) @@ -2153,10 +2207,10 @@ def main(): print(f"error: {n_allocas} gc-capable alloca(s) in the corpus, need " f"at least {ns.min_binds}. Nothing was checked.", file=sys.stderr) return 2 + if funcs_floor_violated(): + return 2 return 1 if total else 0 - n_funcs = sum(len(fs) for _m, fs in parsed) - found = [] for mod, fs in parsed: for f in fs: @@ -2208,11 +2262,7 @@ def render(v): "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 - 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) + if funcs_floor_violated(): return 2 # --- allowlist hygiene -------------------------------------------------- From 68824377b2c4863ab13f36e7fe9461035306c5be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sun, 2 Aug 2026 05:55:32 +0200 Subject: [PATCH 4/5] ci(gc): gate ALLOC_RE's alternatives against the runtime's real symbol table MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The audit this PR did by hand found four alternatives matching nothing. Running the same reconciliation against the result found FIVE more that the audit itself introduced or left: `array_of`, `array_group_by`, `bigint_\w+_op`, `typed_array_from\w*`, `array_buffer_slice`. That is the argument for automating it rather than repeating it — a prose audit does not survive its own next edit. `--audit-alloc-re` decomposes ALLOC_RE into its alternatives and requires each to match at least one `extern "C" fn js_*` exported by perry-runtime or perry-stdlib. It refuses to render a verdict over a symbol scan that found implausibly few symbols, so it cannot pass by measuring itself, and it raises rather than silently checking zero alternatives if ALLOC_RE is rewritten into a shape it cannot decompose. Wired into gc-root-dominance.yml next to --self-test: static, no toolchain, no corpus. Verified both directions: green on the cleaned pattern (70 alternatives vs 3775 symbols), exit 2 with `regexp_alloc\w*` planted back in. self_test() gains arms for detection and for the absence of false positives. The five dead alternatives are deleted, which changes nothing about what the checker matches — that is what "matches no symbol" means. BigInt arithmetic (`js_bigint_add` and friends) allocates and is genuinely uncovered; widening for it is a coverage decision with its own false positives and is left to a change that can measure the new hits. --- .github/workflows/gc-root-dominance.yml | 16 +++ scripts/gc_root_dominance_check.py | 168 +++++++++++++++++++++++- 2 files changed, 180 insertions(+), 4 deletions(-) diff --git a/.github/workflows/gc-root-dominance.yml b/.github/workflows/gc-root-dominance.yml index 541f28c2c6..572a2836c5 100644 --- a/.github/workflows/gc-root-dominance.yml +++ b/.github/workflows/gc-root-dominance.yml @@ -88,6 +88,22 @@ jobs: - name: Checker self-test (can this gate still fail?) run: python3 scripts/gc_root_dominance_check.py --self-test + # The checker only reports a stale register when it recognises what + # MATERIALIZED the value, and that recognition is one regex. An + # alternative in it that matches no real symbol reads as coverage and is + # not -- the gate-can't-fail pattern in regex form. It has shipped nine + # times across two rounds: four (`regexp_alloc\w*`, `promise_alloc\w*`, + # `bigint_alloc\w*`, `typed_array_alloc\w*`) found only because + # `js_regexp_new` cost #7154 a whole investigation round, and five more + # introduced by the change that removed those four. A prose audit does not + # survive its own next edit, so it is a gate now: every alternative must + # match at least one `extern "C" fn js_*` the runtime actually exports. + # + # Static and instant — no toolchain, no corpus — so it runs before the + # build with the self-test. + - name: ALLOC_RE alternatives must match real runtime symbols + run: python3 scripts/gc_root_dominance_check.py --audit-alloc-re + - name: Install Rust toolchain uses: dtolnay/rust-toolchain@stable diff --git a/scripts/gc_root_dominance_check.py b/scripts/gc_root_dominance_check.py index 2a05cbef46..b68843ec9b 100755 --- a/scripts/gc_root_dominance_check.py +++ b/scripts/gc_root_dominance_check.py @@ -350,7 +350,7 @@ def build_cfg(f): r"array_from\w*|array_clone\w*|array_concat\w*|array_slice|array_splice|" r"array_to_spliced|array_to_sorted\w*|array_to_reversed|array_with|" r"array_flat\w*|array_map|array_filter|array_like_to_array|" - r"iterator_to_array|array_of|array_group_by|" + r"iterator_to_array|" # fresh objects/collections handed back as a whole r"object_keys\w*|object_values\w*|object_entries\w*|object_from_entries|" r"object_assign\w*|object_group_by|object_coerce|" @@ -363,12 +363,126 @@ def build_cfg(f): r"build_class_keys_array|create_namespace|create_native_module_namespace|" r"generator_attach_prototype|proxy_revocable|" # BigInt: no `_alloc` and no `_new`; the constructors are `_from_*`. - r"bigint_from\w*|bigint_\w+_op|" - # Buffers / typed arrays that spell it neither way. - r"buffer_from\w*|typed_array_from\w*|array_buffer_slice" + # + # NOT COVERED, AND DELIBERATELY LEFT UNCOVERED HERE: BigInt *arithmetic* + # (`js_bigint_add`, `js_bigint_and`, `js_bigint_mul`, ...) allocates a fresh + # BigInt and matches none of the three conventions. An alternative spelled + # `bigint_\w+_op` used to sit here and matched nothing -- the same + # extrapolated-suffix mistake as `regexp_alloc\w*`, in the same list, added + # by the change that removed the first four. It is deleted rather than + # widened because widening it is a coverage decision with its own false + # positives (`js_bigint_equals` returns a bool), and that belongs in a + # change that can measure the new hits. Tracked in the PR thread. + r"bigint_from\w*|" + # Buffers / typed arrays that spell it neither way. `array_buffer_slice` + # and `typed_array_from\w*` were removed for the same reason: no such + # symbols. The real one, `js_buffer_from_arraybuffer_slice`, is already + # matched by `buffer_from\w*`. + r"buffer_from\w*" r")$" ) +# --------------------------------------------------------------------- AUDIT +# +# A regex alternative that matches nothing is the gate-can't-fail pattern in +# regex form: it reads as coverage, it costs nothing to keep, and it is +# indistinguishable from real coverage until a bug slips through the hole it +# was supposed to close. This has now happened twice in this one pattern: +# +# * `regexp_alloc\w*`, `promise_alloc\w*`, `bigint_alloc\w*`, +# `typed_array_alloc\w*` -- four dead alternatives, found only because +# `js_regexp_new` cost #7154 a full investigation round; +# * `array_of`, `array_group_by`, `bigint_\w+_op`, `typed_array_from\w*`, +# `array_buffer_slice` -- five MORE, introduced by the change that removed +# the first four, and found by running this auditor against it. +# +# The second round is the argument for automating it. A prose audit does not +# survive its own next edit; the enumeration below is checked by CI instead +# (`--audit-alloc-re`), so an alternative cannot go dead without going red. +# +# Deleting a dead alternative changes NOTHING about what the checker matches -- +# that is what "matches no symbol" means -- so this is not a narrowing. + +_EXTERN_C_FN_RE = re.compile(r'extern\s+"C"\s+fn\s+(js_\w+)') + +# The runtime crates that export the C-ABI surface perry-codegen calls. +SYMBOL_ROOTS = ("crates/perry-runtime/src", "crates/perry-stdlib/src") + + +def runtime_symbols(roots=SYMBOL_ROOTS): + """Every `extern "C" fn js_*` the runtime actually exports.""" + syms = set() + for root in roots: + if not os.path.isdir(root): + continue + for dirpath, _dirs, files in os.walk(root): + for name in files: + if not name.endswith(".rs"): + continue + with open(os.path.join(dirpath, name), + encoding="utf-8", errors="replace") as fh: + syms.update(_EXTERN_C_FN_RE.findall(fh.read())) + return syms + + +def alloc_re_alternatives(): + """The top-level alternatives inside ALLOC_RE's `js_(...)` group. + + Asserts the pattern's shape rather than assuming it: if ALLOC_RE is ever + rewritten into a form this cannot decompose, that must be a loud failure, + not an audit that silently checks zero alternatives. + """ + pat = ALLOC_RE.pattern + if not (pat.startswith("^js_(") and pat.endswith(")$")): + raise MalformedIR( + "ALLOC_RE is no longer of the form ^js_(...)$; the alternative " + "audit cannot decompose it. Update alloc_re_alternatives() rather " + "than leaving the audit silently vacuous.") + inner = pat[len("^js_("):-len(")$")] + if "(" in inner: + raise MalformedIR( + "ALLOC_RE now contains a nested group; splitting on '|' would " + "produce bogus alternatives. Update alloc_re_alternatives().") + return [a for a in inner.split("|") if a] + + +def dead_alloc_alternatives(symbols): + """ALLOC_RE alternatives matching none of `symbols`, in pattern order.""" + dead = [] + for alt in alloc_re_alternatives(): + probe = re.compile("^js_(?:%s)$" % alt) + if not any(probe.match(s) for s in symbols): + dead.append(alt) + return dead + + +def audit_alloc_re(roots=SYMBOL_ROOTS): + """Exit status for `--audit-alloc-re`. 0 clean, 2 on a dead alternative.""" + syms = runtime_symbols(roots) + # Non-vacuity first: an empty symbol set would report every alternative + # dead, and a tiny one would report a plausible-looking few. Either way the + # verdict would be about the scan, not about the pattern. + if len(syms) < 500: + print(f"error: found only {len(syms)} `extern \"C\" fn js_*` symbols " + f"under {', '.join(roots)}. The audit is measuring its own scan, " + "not ALLOC_RE. Run it from the repository root.", file=sys.stderr) + return 2 + dead = dead_alloc_alternatives(syms) + print(f"=== ALLOC_RE: {len(alloc_re_alternatives())} alternatives vs " + f"{len(syms)} exported js_* symbols") + if dead: + print("error: ALLOC_RE alternatives that match no runtime symbol:", + file=sys.stderr) + for a in dead: + print(f" {a}", file=sys.stderr) + print("An alternative that matches nothing reads as coverage and is " + "not. Either it is misspelled -- check the real name in the " + "runtime -- or the symbol was renamed or removed and the " + "alternative should go with it.", file=sys.stderr) + return 2 + print("=== every alternative matches at least one exported symbol") + return 0 + # Bit-level / identity producers a heap address flows through unchanged. TRANSPARENT_OPS = ("or i64", "and i64", "bitcast", "inttoptr", "ptrtoint", "select", "phi", "add i64", "sub i64") @@ -1908,6 +2022,41 @@ def self_test(): "breadth floor. A guard that rejects every corpus is a " "gate that always fails.", file=sys.stderr) ok = False + # --- the ALLOC_RE alternative audit, both directions ---------------- + # + # The auditor is itself a gate, so it gets the same treatment as the + # rest: prove it reports a planted dead alternative AND clears a live + # one. Driven off a synthetic symbol set so the arm does not depend on + # the repository's current contents -- an arm that only passes because + # today's runtime happens to export the right names would go quiet the + # moment someone renamed a symbol, which is the case it exists for. + live_alts = [a for a in alloc_re_alternatives() + if re.compile("^js_(?:%s)$" % a).match("js_object_alloc") + or re.compile("^js_(?:%s)$" % a).match("js_regexp_new")] + if not live_alts: + print("self-test FAIL: neither js_object_alloc nor js_regexp_new " + "matches any ALLOC_RE alternative; the audit arm below would " + "be testing nothing", file=sys.stderr) + ok = False + if dead_alloc_alternatives({"js_object_alloc", "js_regexp_new"}) == []: + print("self-test FAIL: audited against a two-symbol table, almost " + "every ALLOC_RE alternative must be reported dead. An empty " + "result means the auditor cannot detect a dead alternative " + "at all.", file=sys.stderr) + ok = False + # And it must clear an alternative that IS covered: build a symbol set + # from one probe per alternative, and require a clean verdict. + synthetic = set() + for a in alloc_re_alternatives(): + # `\w*` -> `x`, `\w+` -> `x`: a concrete name the alternative matches. + synthetic.add("js_" + a.replace(r"\w*", "x").replace(r"\w+", "x")) + residue = dead_alloc_alternatives(synthetic) + if residue: + print(f"self-test FAIL: the auditor reported {len(residue)} " + "alternative(s) dead against a symbol set synthesised from " + f"the alternatives themselves: {residue}. That is a bug in " + "the auditor, not in ALLOC_RE.", file=sys.stderr) + ok = False # A knob that is silently ignored is a disarmed knob -- the same rule # `--max-stale` and `--fatal-sinks` already carry, read the other way. if _main_probe(["--stale-registers", "--any-def", planted]) != 2: @@ -2144,8 +2293,19 @@ def main(): "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.") + ap.add_argument("--audit-alloc-re", action="store_true", + help="check every ALLOC_RE alternative against the runtime's " + "real `extern \"C\" fn js_*` symbols and fail on any " + "that matches nothing. A dead alternative reads as " + "coverage and is not -- nine of them have shipped in " + "this pattern across two rounds. Takes no corpus.") ns = ap.parse_args() + # Standalone static audit: it reads the crates, not an IR corpus, so it is + # checked before the corpus arguments are. + if ns.audit_alloc_re: + return audit_alloc_re() + # A knob that is silently ignored is a disarmed knob: `--max-stale 0` # without `--stale-registers` would run the bind-anchored check and look # like it enforced a budget, and `--fatal-sinks` alone would look like it From c75def99586004f5fa7c394ce42cf5c68e919dfe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sun, 2 Aug 2026 06:19:33 +0200 Subject: [PATCH 5/5] test(gc): register the regexp witness, and triage its allocation-point reds test_gap_gc_regexp_receiver_rooting.ts was added without a corpus entry, so nothing ran it. Registered, which makes it a hard gate on `loop_polls` -- the route this PR's fix is verified on: `bad 0` 8/8, byte-exact against the oracle, with the copying minor live. It is red on all TEN allocation-point arms (exit=139, deterministic, identical evidence on every one). That route forces the collection inside the allocating helper rather than at a loop safepoint, and this PR claims no fix for it. Triaged to #7217 with the measurements rather than left to turn gc-stress red. Second file with this exact signature -- #7216's assign_string_source witness is triaged to #7217 for the same reason. Two independent sites where a rooting fix that holds at a safepoint does not hold at the allocation point says something about the route, not about either fix. gc-stress verified green with the registration: --arms pr FAIL=0 XFAIL=2. --- test-parity/gc_repsel_corpus.txt | 23 ++++++++++++++++++++ test-parity/gc_repsel_triage.txt | 37 ++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+) diff --git a/test-parity/gc_repsel_corpus.txt b/test-parity/gc_repsel_corpus.txt index 083b437818..2ccddc3be0 100644 --- a/test-parity/gc_repsel_corpus.txt +++ b/test-parity/gc_repsel_corpus.txt @@ -262,3 +262,26 @@ test_gap_gc_index_get_receiver_rooting test_gap_gc_closure_call_callee_rooting test_gap_gc_closure_call_this_rooting test_gap_gc_closure_call_argument_rooting + +# --- The regexp receiver across ToString (#7154 residual) -------------------- +# `re.test(s)` / `re.exec(s)` unboxed the receiver to a raw `RegExpHeader*` and +# only THEN emitted `js_jsvalue_to_string_coerce`, which allocates and can +# dispatch a user `toString` -- arbitrary JS with its own back-edge polls. #7226 +# measured this residual and named it rather than claiming closure; this file is +# it, and it is the last thing between `sfw-registry --help` and 30/30 under a +# genuine polls build. +# +# It is also the witness for the CHECKER half of the same bug. `ALLOC_RE` spelled +# the allocator `regexp_alloc\w*` while the real call is `js_regexp_new`, so the +# static pass had no recognised heap-value source for the register and reported +# nothing at all. That hole is gated now (`--audit-alloc-re`, which requires +# every alternative to match a real exported symbol), but a static gate cannot +# prove the FIX -- only running the program under a moving collector does, and +# only the `requires=move` arms do that. +# +# Measured on this branch, release, `--arms loop_polls --filter test_gap_gc_`: +# PASS on `loop_polls` 8/8 (`bad 0`, matching the oracle), and exit=139 on all +# TEN allocation-point arms, also deterministic. See gc_repsel_triage.txt: the +# fix in this PR is verified on the safepoint route and is NOT claimed on the +# allocation-point route, which is #7217's open defect class at a second site. +test_gap_gc_regexp_receiver_rooting diff --git a/test-parity/gc_repsel_triage.txt b/test-parity/gc_repsel_triage.txt index a7c1437ab8..721663b63f 100644 --- a/test-parity/gc_repsel_triage.txt +++ b/test-parity/gc_repsel_triage.txt @@ -15,3 +15,40 @@ # temporary. Do not re-triage it without a new issue number and a reason. test_gap_repsel_ptr_shape_locals | rep_ptr_shape_off | #6976 -- REGRESSION IN THE REPRESENTATION'S OWN OFF-SWITCH, not a defect in this PR. Bisected: passes at 8327ced52, fails at 1a533a3a8 (#6925, repsel Phase 5a proven `this`). With PERRY_PTR_SHAPE_LOCALS=0 the program dies partway with `TypeError: Cannot read properties of undefined (reading 'area')`, losing its last five output lines. It was invisible until now because #6925 also left test_gap_repsel_proven_this_frozen.ts unregistered, which makes this script exit 3 before it runs anything -- the gate was dark, not green. REMOVE THIS ENTRY when #6976 is fixed; an OFF arm is supposed to be the safest cell in the matrix. + +# --- The regexp receiver on the ALLOCATION-POINT arms (#7217 class) ---------- +# `test_gap_gc_regexp_receiver_rooting` is a HARD GATE on `loop_polls`, where the +# fix it ships with is claimed and verified: `bad 0`, 8/8, byte-exact against the +# oracle, with the copying minor live. These ten entries cover the arms where no +# fix is claimed -- the ones that force the collection at the register-imprecise +# ALLOCATION point (`%E%` without the compile-time PERRY_GC_MOVING_LOOP_POLLS=1) +# rather than at a loop safepoint. +# +# Measured on this branch, release, oracle node 26.5.1 (`bad 0`): +# PERRY_GC_HEAP_LIMIT=8 PERRY_GC_INCREMENTAL=0 PERRY_CONSERVATIVE_STACK_SCAN=off +# -> exit=139, no output, 8/8 deterministic +# compiled+run PERRY_GC_MOVING_LOOP_POLLS=1 -> `bad 0`, 8/8 +# shipped default -> `bad 0` +# `--arms all` reports the SAME `exit=139 cycles=1 scavenged=14` on all ten, and +# PASS on `loop_polls`. The split is the route, not the arm's other knobs. +# +# THIS IS THE SECOND FILE WITH EXACTLY THIS SIGNATURE. #7216's +# `test_gap_gc_assign_string_source_rooting` behaves the same way and is triaged +# to #7217 for the same reason. Two independent sites where a rooting fix that +# holds at a safepoint does not hold when the collection is forced inside the +# allocating helper is a statement about the route, not about either fix -- and +# it is what #7217 says in words about `object_assign_set_string_key`'s interning +# and keys-array growth. Whoever closes #7217 should check this file too; if it +# turns out to need its own fix, split these entries onto their own issue. +# +# DELETE ALL TEN when the allocation-point route is fixed. +test_gap_gc_regexp_receiver_rooting | evac_minor | #7217 -- allocation-point relocation; the fix is verified on the safepoint route (loop_polls, 8/8) and not claimed here. exit=139, deterministic. +test_gap_gc_regexp_receiver_rooting | force_evac | #7217 -- same allocation-point window, with force-evacuate on top. +test_gap_gc_regexp_receiver_rooting | force_verify | #7217 -- same allocation-point window, force + verify. +test_gap_gc_regexp_receiver_rooting | rep_i32_off | #7217 -- %E% allocation-point window; the repsel knob is not the discriminator (identical evidence on all ten). +test_gap_gc_regexp_receiver_rooting | rep_str_off | #7217 -- %E% allocation-point window; see rep_i32_off. +test_gap_gc_regexp_receiver_rooting | rep_str_static_off | #7217 -- %E% allocation-point window; see rep_i32_off. +test_gap_gc_regexp_receiver_rooting | rep_ptr_shape_off | #7217 -- %E% allocation-point window; see rep_i32_off. +test_gap_gc_regexp_receiver_rooting | rep_ptr_numarray_off | #7217 -- %E% allocation-point window; see rep_i32_off. +test_gap_gc_regexp_receiver_rooting | rep_spec_abi_off | #7217 -- %E% allocation-point window; see rep_i32_off. +test_gap_gc_regexp_receiver_rooting | rep_int_valued_off | #7217 -- %E% allocation-point window; see rep_i32_off.