Skip to content

Commit 9ac8046

Browse files
committed
test(gc): cover every interned string-cache root cell, and trim CLAUDE.md
Review follow-ups from PerryTS#7226, PerryTS#7227 and PerryTS#7214, all merged, so they land together against main. The interned `typeof` strings and the `JSON.rawJSON` key are registered GC roots since PerryTS#7226, but nothing tested the registration from the Rust side and the `.ts` gap test drove only six of the eight `typeof` cells. `scan_typeof_string_roots_mut` is eight hand-written `visit(...)` calls, so `bigint` and `symbol` could have lost theirs without a red test. - `gc/tests/runtime_roots/interned_string_caches.rs`: mark, rewrite and registration tests for both scanners. The rewrite test forwards all eight `typeof` cells, so a dropped `visit(...)` line fails there. Marking alone is not enough and is asserted separately: a marked but un-rewritten cell still hands out a pre-move address after a copying minor, which is the whole PerryTS#7211 failure. - `test_gap_gc_typeof_string_cache_rooting.ts` now drives all eight cells. Unregistering the scanner takes it from `bad 0` to `bad 592` 5/5 under a genuine `POLLS=1` build; the six-cell version reported `bad 444`, and 592/8 == 444/6 == 74, so the two added cells fail at the same collection as the rest rather than being decorative. - `reset_typeof_string_cache_for_test` had no callers and its doc described an arena-reset teardown that does not exist. It is now driven by the tests above, its eight-cell list is shared with the new populate/peek helpers instead of being written out twice, and `raw_json.rs` gets the matching trio. - CLAUDE.md: fold PerryTS#7226's additions back toward the length of the entries around them. The file's own opening note says to keep it concise and put detail in `changelog.d/`, and the incident narrative is already in `changelog.d/7219-registry-gc-unrooted-caches.md`. The detector knobs it re-listed are documented in full two sections above. - `changelog.d/7214-...md`: rewrap so the line does not open with `PerryTS#7161`, which markdownlint reads as a malformed ATX heading (MD018). No runtime behavior changes: every new Rust symbol is `#[cfg(test)]`.
1 parent 61410a2 commit 9ac8046

8 files changed

Lines changed: 408 additions & 19 deletions

File tree

CLAUDE.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -250,5 +250,5 @@ Corollary: a *new* gate has never been green, so promoting it to required immedi
250250
- **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.
251251
- **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.
252252
- **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.
253-
- **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.
254-
- **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.
253+
- **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.
254+
- **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`.

changelog.d/7214-closure-calln-stale-registers.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -113,8 +113,8 @@ moving-reachable).
113113

114114
## What this does NOT close
115115

116-
**`sfw-registry --help` under a genuine `POLLS=1` build is still red, so
117-
#7161's stopgap stays.** Measured on this build, compiled *and* run with the
116+
**`sfw-registry --help` under a genuine `POLLS=1` build is still red, so the
117+
stopgap from #7161 stays.** Measured on this build, compiled *and* run with the
118118
flag: **3/10 pass, 7/10 SIGSEGV**. Its default arm is clean **10/10**, so
119119
nothing was traded away. The three fixed registers were real and are now
120120
provably rooted, but they are not the last thing standing between the registry
Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
1+
### Fixed
2+
3+
- **The `typeof` string-cache rooting test drove six of the eight cache
4+
cells.** `scan_typeof_string_roots_mut` is eight hand-written `visit(...)`
5+
calls, one per interned `typeof` result, so `TYPEOF_BIGINT` and
6+
`TYPEOF_SYMBOL` could have lost theirs and no test would have noticed.
7+
`test_gap_gc_typeof_string_cache_rooting.ts` now drives all eight.
8+
9+
Measured by unregistering the scanner and rebuilding, rather than assumed:
10+
11+
| | default | `POLLS=1`, compiled *and* run with the flag |
12+
|---|---|---|
13+
| registered | `bad 0` | `bad 0` 5/5 |
14+
| unregistered || `bad 592` 5/5 |
15+
16+
The six-cell version of this test reported `bad 444`, and
17+
`592 / 8 == 444 / 6 == 74` — the two added cells go bad at the same
18+
collection as the other six, which is what says they are really covered
19+
rather than decorative.
20+
21+
### Added
22+
23+
- **Rust-side mark, rewrite and registration tests for both interned-string
24+
root scanners** (`gc/tests/runtime_roots/interned_string_caches.rs`):
25+
`builtins::arithmetic::scan_typeof_string_roots_mut` and
26+
`json::raw_json::scan_raw_json_key_root_mut`. Neither had one; #7211
27+
registered them and the `.ts` gap test covered only the `typeof` side, from
28+
one direction, at six cells.
29+
30+
Marking and rewriting are asserted separately on purpose. Marking alone
31+
keeps the string alive but still hands out a pre-move address after a
32+
copying minor, which is the #7211 failure in full — the distinction
33+
`docs/src/internals/gc-rooting-invariant.md` keeps having to make. The
34+
registration test is separate again, because either scanner can be called
35+
directly from a test whether or not `gc_init` ever names it, and an
36+
unregistered scanner is a no-op in production.
37+
38+
Sabotage-tested, per the project's own rule that a gate must be shown able
39+
to fail:
40+
41+
| sabotage | result |
42+
|---|---|
43+
| drop `visit(&TYPEOF_BIGINT, visitor)` | mark and rewrite tests red, naming `cell 6` |
44+
| drop the `gc_init` registration of `scan_raw_json_key_root_mut` | registration test red |
45+
46+
### Changed
47+
48+
- **`reset_typeof_string_cache_for_test` was dead code.** It had no callers,
49+
and its doc comment described a shared arena-reset teardown that does not
50+
exist in this repo — every other `_for_test` helper in `perry-runtime` is
51+
called. It is now driven by the tests above, and its eight-cell list is
52+
shared with the new `populate_*` / `*_cells_for_test` helpers instead of
53+
being written out a second time. `json/raw_json.rs` gets the matching trio
54+
(`reset_`, `populate_`, `peek_`), which is what made the rawJSON scanner
55+
testable at all.
56+
57+
All `#[cfg(test)]`; no runtime behavior changes.
58+
59+
- **`CLAUDE.md`: the two #7226 entries are folded back toward the length of
60+
the entries around them**, 2006 → 1722 and 1358 → 903 characters, in a
61+
section whose other bullets run 242-355. The file's own opening note says
62+
to keep it concise and put detail in `changelog.d/`. Nothing operational
63+
was dropped: the incident narrative is already in
64+
`changelog.d/7219-registry-gc-unrooted-caches.md`, and the detector knobs
65+
the new bullet re-listed (`PERRY_GC_ZEAL`, `PERRY_GC_PROTECT_FROMSPACE`,
66+
`PERRY_GC_PROTECT_FROMSPACE_DEPTH`) are documented in full, with their
67+
exact gating, two sections above under "Rooting-bug instruments".
68+
69+
- **`changelog.d/7214-closure-calln-stale-registers.md` line 117 opened with
70+
`#7161`**, which markdownlint reads as a malformed ATX heading (MD018).
71+
Rewrapped so the reference is not the first thing on the line. The
72+
rendered text is unchanged — CommonMark requires a space after `#`, so it
73+
was never a heading, and the line is a paragraph continuation besides.
74+
75+
## Not changed, and why
76+
77+
**`SYMBOL_ROOTS` in `scripts/gc_root_dominance_check.py` does not need the
78+
`crates/perry-ext-*` crates.** `--audit-alloc-re` is a liveness check on
79+
`ALLOC_RE`'s alternatives — it asks whether each alternative matches at least
80+
one real exported symbol — so widening the symbol corpus can only make it
81+
more permissive, never less. Measured: 3775 symbols under the current two
82+
roots, 394 more that exist only in the 38 ext crates, and the dead-alternative
83+
verdict is the empty list with or without them. No alternative is kept alive
84+
only by an ext symbol. The 26 ext-only allocating symbols already match
85+
`ALLOC_RE` through the `_new` / `_create` conventions, so what the checker
86+
detects is unchanged either way.
87+
88+
A symbol that allocates and matches no alternative would be a real hole, but
89+
it is a hole in `ALLOC_RE` and this audit runs the other direction, so adding
90+
roots would not surface it.

crates/perry-runtime/src/builtins/arithmetic.rs

Lines changed: 42 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -587,25 +587,54 @@ pub fn scan_typeof_string_roots_mut(visitor: &mut crate::gc::RuntimeRootVisitor<
587587
visit(&TYPEOF_SYMBOL, visitor);
588588
}
589589

590-
/// Drop every cached `typeof` string. Test-only: the unit-test harness resets
591-
/// arenas between tests while thread-locals persist, so a cache entry from a
592-
/// previous test names memory the new arena does not own.
590+
/// The eight cells and their payloads, in `scan_typeof_string_roots_mut`
591+
/// order. Test-only. It exists so a test can assert the scanner reaches EVERY
592+
/// cell: that scanner is eight hand-written `visit(...)` calls, and a dropped
593+
/// line is invisible to any test that exercises only some of them.
594+
#[cfg(test)]
595+
type TypeofCacheCell = &'static std::thread::LocalKey<std::cell::Cell<*mut StringHeader>>;
596+
597+
#[cfg(test)]
598+
fn typeof_cache_entries_for_test() -> [(TypeofCacheCell, &'static str); 8] {
599+
[
600+
(&TYPEOF_UNDEFINED, "undefined"),
601+
(&TYPEOF_OBJECT, "object"),
602+
(&TYPEOF_BOOLEAN, "boolean"),
603+
(&TYPEOF_NUMBER, "number"),
604+
(&TYPEOF_STRING, "string"),
605+
(&TYPEOF_FUNCTION, "function"),
606+
(&TYPEOF_BIGINT, "bigint"),
607+
(&TYPEOF_SYMBOL, "symbol"),
608+
]
609+
}
610+
611+
/// Drop every cached `typeof` string. Test-only: a rooting test has to start
612+
/// from an empty cache so the strings it then allocates are its own, in a
613+
/// known arena, rather than survivors of whichever test ran first on this
614+
/// thread.
593615
#[cfg(test)]
594616
pub(crate) fn reset_typeof_string_cache_for_test() {
595-
for cache in [
596-
&TYPEOF_UNDEFINED,
597-
&TYPEOF_OBJECT,
598-
&TYPEOF_BOOLEAN,
599-
&TYPEOF_NUMBER,
600-
&TYPEOF_STRING,
601-
&TYPEOF_FUNCTION,
602-
&TYPEOF_BIGINT,
603-
&TYPEOF_SYMBOL,
604-
] {
617+
for (cache, _) in typeof_cache_entries_for_test() {
605618
cache.with(|cell| cell.set(std::ptr::null_mut()));
606619
}
607620
}
608621

622+
/// Allocate all eight cached strings, exactly as eight `typeof` calls of eight
623+
/// different value shapes would. Test-only; reaching `bigint` and `symbol`
624+
/// from Rust otherwise means building a BigInt and a registered Symbol.
625+
#[cfg(test)]
626+
pub(crate) fn populate_typeof_string_cache_for_test() {
627+
for (cache, text) in typeof_cache_entries_for_test() {
628+
get_cached(cache, text);
629+
}
630+
}
631+
632+
/// Read the eight cells without populating them. Test-only.
633+
#[cfg(test)]
634+
pub(crate) fn typeof_string_cache_cells_for_test() -> [*mut StringHeader; 8] {
635+
typeof_cache_entries_for_test().map(|(cache, _)| cache.with(|cell| cell.get()))
636+
}
637+
609638
/// Return the typeof a value as a string
610639
/// Takes an f64 that uses NaN-boxing to distinguish types.
611640
/// Returns a pointer to a string: "undefined", "boolean", "number", "string", "object", "function"

crates/perry-runtime/src/gc/tests/runtime_roots.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ use super::support::*;
33
use std::cell::Cell;
44
mod callback_scanners;
55
mod hook_dispatch_handles;
6+
mod interned_string_caches;
67
mod prototype_addr_cache;
78
mod side_table_scanners;
89
mod string_slice;

0 commit comments

Comments
 (0)