Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -250,5 +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) — **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.
- **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), and that list is currently **empty** — every new hit is a red build.
- **A runtime-side cache of a raw heap pointer is a GC root, and the static checker cannot see it.** `scripts/gc_root_dominance_check.py` reads emitted LLVM IR, so a thread-local or side table holding a `*mut` into the heap is structurally invisible to it — the runtime instruments above are the only detector, and they go at the workload *before* you grind the static checker's tail. Two tells. 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 **a perfectly reproducible GC bug means a table, not a register**. And the 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. Worked examples: `changelog.d/7219-registry-gc-unrooted-caches.md`, `changelog.d/7239-gc-unrooted-runtime-caches.md`.
Comment thread
coderabbitai[bot] marked this conversation as resolved.
4 changes: 2 additions & 2 deletions changelog.d/7214-closure-calln-stale-registers.md
Original file line number Diff line number Diff line change
Expand Up @@ -113,8 +113,8 @@ moving-reachable).

## What this does NOT close

**`sfw-registry --help` under a genuine `POLLS=1` build is still red, so
#7161's stopgap stays.** Measured on this build, compiled *and* run with the
**`sfw-registry --help` under a genuine `POLLS=1` build is still red, so the
stopgap from #7161 stays.** Measured on this build, compiled *and* run with the
flag: **3/10 pass, 7/10 SIGSEGV**. Its default arm is clean **10/10**, so
nothing was traded away. The three fixed registers were real and are now
provably rooted, but they are not the last thing standing between the registry
Expand Down
90 changes: 90 additions & 0 deletions changelog.d/7276-interned-string-cache-root-coverage.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
### Fixed

- **The `typeof` string-cache rooting test drove six of the eight cache
cells.** `scan_typeof_string_roots_mut` is eight hand-written `visit(...)`
calls, one per interned `typeof` result, so `TYPEOF_BIGINT` and
`TYPEOF_SYMBOL` could have lost theirs and no test would have noticed.
`test_gap_gc_typeof_string_cache_rooting.ts` now drives all eight.

Measured by unregistering the scanner and rebuilding, rather than assumed:

| | default | `POLLS=1`, compiled *and* run with the flag |
|---|---|---|
| registered | `bad 0` | `bad 0` 5/5 |
| unregistered | — | `bad 592` 5/5 |

The six-cell version of this test reported `bad 444`, and
`592 / 8 == 444 / 6 == 74` — the two added cells go bad at the same
collection as the other six, which is what says they are really covered
rather than decorative.

### Added

- **Rust-side mark, rewrite and registration tests for both interned-string
root scanners** (`gc/tests/runtime_roots/interned_string_caches.rs`):
`builtins::arithmetic::scan_typeof_string_roots_mut` and
`json::raw_json::scan_raw_json_key_root_mut`. Neither had one; #7211
registered them and the `.ts` gap test covered only the `typeof` side, from
one direction, at six cells.

Marking and rewriting are asserted separately on purpose. Marking alone
keeps the string alive but still hands out a pre-move address after a
copying minor, which is the #7211 failure in full — the distinction
`docs/src/internals/gc-rooting-invariant.md` keeps having to make. The
registration test is separate again, because either scanner can be called
directly from a test whether or not `gc_init` ever names it, and an
unregistered scanner is a no-op in production.

Sabotage-tested, per the project's own rule that a gate must be shown able
to fail:

| sabotage | result |
|---|---|
| drop `visit(&TYPEOF_BIGINT, visitor)` | mark and rewrite tests red, naming `cell 6` |
| drop the `gc_init` registration of `scan_raw_json_key_root_mut` | registration test red |

### Changed

- **`reset_typeof_string_cache_for_test` was dead code.** It had no callers,
and its doc comment described a shared arena-reset teardown that does not
exist in this repo — every other `_for_test` helper in `perry-runtime` is
called. It is now driven by the tests above, and its eight-cell list is
shared with the new `populate_*` / `*_cells_for_test` helpers instead of
being written out a second time. `json/raw_json.rs` gets the matching trio
(`reset_`, `populate_`, `peek_`), which is what made the rawJSON scanner
testable at all.

All `#[cfg(test)]`; no runtime behavior changes.

- **`CLAUDE.md`: the two #7226 entries are folded back toward the length of
the entries around them**, 2006 → 1722 and 1358 → 903 characters, in a
section whose other bullets run 242-355. The file's own opening note says
to keep it concise and put detail in `changelog.d/`. Nothing operational
was dropped: the incident narrative is already in
`changelog.d/7219-registry-gc-unrooted-caches.md`, and the detector knobs
the new bullet re-listed (`PERRY_GC_ZEAL`, `PERRY_GC_PROTECT_FROMSPACE`,
`PERRY_GC_PROTECT_FROMSPACE_DEPTH`) are documented in full, with their
exact gating, two sections above under "Rooting-bug instruments".

- **`changelog.d/7214-closure-calln-stale-registers.md` line 117 opened with
`#7161`**, which markdownlint reads as a malformed ATX heading (MD018).
Rewrapped so the reference is not the first thing on the line. The
rendered text is unchanged — CommonMark requires a space after `#`, so it
was never a heading, and the line is a paragraph continuation besides.

## Not changed, and why

**`SYMBOL_ROOTS` in `scripts/gc_root_dominance_check.py` does not need the
`crates/perry-ext-*` crates.** `--audit-alloc-re` is a liveness check on
`ALLOC_RE`'s alternatives — it asks whether each alternative matches at least
one real exported symbol — so widening the symbol corpus can only make it
more permissive, never less. Measured: 3775 symbols under the current two
roots, 394 more that exist only in the 38 ext crates, and the dead-alternative
verdict is the empty list with or without them. No alternative is kept alive
only by an ext symbol. The 26 ext-only allocating symbols already match
`ALLOC_RE` through the `_new` / `_create` conventions, so what the checker
detects is unchanged either way.

A symbol that allocates and matches no alternative would be a real hole, but
it is a hole in `ALLOC_RE` and this audit runs the other direction, so adding
roots would not surface it.
55 changes: 42 additions & 13 deletions crates/perry-runtime/src/builtins/arithmetic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -587,25 +587,54 @@ pub fn scan_typeof_string_roots_mut(visitor: &mut crate::gc::RuntimeRootVisitor<
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.
/// The eight cells and their payloads, in `scan_typeof_string_roots_mut`
/// order. Test-only. It exists so a test can assert the scanner reaches EVERY
/// cell: that scanner is eight hand-written `visit(...)` calls, and a dropped
/// line is invisible to any test that exercises only some of them.
#[cfg(test)]
type TypeofCacheCell = &'static std::thread::LocalKey<std::cell::Cell<*mut StringHeader>>;

#[cfg(test)]
fn typeof_cache_entries_for_test() -> [(TypeofCacheCell, &'static str); 8] {
[
(&TYPEOF_UNDEFINED, "undefined"),
(&TYPEOF_OBJECT, "object"),
(&TYPEOF_BOOLEAN, "boolean"),
(&TYPEOF_NUMBER, "number"),
(&TYPEOF_STRING, "string"),
(&TYPEOF_FUNCTION, "function"),
(&TYPEOF_BIGINT, "bigint"),
(&TYPEOF_SYMBOL, "symbol"),
]
}

/// Drop every cached `typeof` string. Test-only: a rooting test has to start
/// from an empty cache so the strings it then allocates are its own, in a
/// known arena, rather than survivors of whichever test ran first on this
/// thread.
#[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,
] {
for (cache, _) in typeof_cache_entries_for_test() {
cache.with(|cell| cell.set(std::ptr::null_mut()));
}
}

/// Allocate all eight cached strings, exactly as eight `typeof` calls of eight
/// different value shapes would. Test-only; reaching `bigint` and `symbol`
/// from Rust otherwise means building a BigInt and a registered Symbol.
#[cfg(test)]
pub(crate) fn populate_typeof_string_cache_for_test() {
for (cache, text) in typeof_cache_entries_for_test() {
get_cached(cache, text);
}
}

/// Read the eight cells without populating them. Test-only.
#[cfg(test)]
pub(crate) fn typeof_string_cache_cells_for_test() -> [*mut StringHeader; 8] {
typeof_cache_entries_for_test().map(|(cache, _)| cache.with(|cell| cell.get()))
}

/// 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"
Expand Down
1 change: 1 addition & 0 deletions crates/perry-runtime/src/gc/tests/runtime_roots.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use super::support::*;
use std::cell::Cell;
mod callback_scanners;
mod hook_dispatch_handles;
mod interned_string_caches;
mod prototype_addr_cache;
mod side_table_scanners;
mod string_slice;
Expand Down
Loading
Loading