From 9ac804603c6dbbe52398eb956933528e8bf3faef Mon Sep 17 00:00:00 2001 From: jdalton Date: Sun, 2 Aug 2026 15:50:31 -0400 Subject: [PATCH] test(gc): cover every interned string-cache root cell, and trim CLAUDE.md Review follow-ups from #7226, #7227 and #7214, all merged, so they land together against main. The interned `typeof` strings and the `JSON.rawJSON` key are registered GC roots since #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 #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 #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 `#7161`, which markdownlint reads as a malformed ATX heading (MD018). No runtime behavior changes: every new Rust symbol is `#[cfg(test)]`. --- CLAUDE.md | 4 +- .../7214-closure-calln-stale-registers.md | 4 +- ...276-interned-string-cache-root-coverage.md | 90 +++++++ .../perry-runtime/src/builtins/arithmetic.rs | 55 +++- .../src/gc/tests/runtime_roots.rs | 1 + .../runtime_roots/interned_string_caches.rs | 238 ++++++++++++++++++ crates/perry-runtime/src/json/raw_json.rs | 22 ++ ...test_gap_gc_typeof_string_cache_rooting.ts | 13 +- 8 files changed, 408 insertions(+), 19 deletions(-) create mode 100644 changelog.d/7276-interned-string-cache-root-coverage.md create mode 100644 crates/perry-runtime/src/gc/tests/runtime_roots/interned_string_caches.rs diff --git a/CLAUDE.md b/CLAUDE.md index 14b4e42d02..e2638330dc 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -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`. diff --git a/changelog.d/7214-closure-calln-stale-registers.md b/changelog.d/7214-closure-calln-stale-registers.md index 82c22a04a3..3ad473ac7f 100644 --- a/changelog.d/7214-closure-calln-stale-registers.md +++ b/changelog.d/7214-closure-calln-stale-registers.md @@ -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 diff --git a/changelog.d/7276-interned-string-cache-root-coverage.md b/changelog.d/7276-interned-string-cache-root-coverage.md new file mode 100644 index 0000000000..56cdfea04d --- /dev/null +++ b/changelog.d/7276-interned-string-cache-root-coverage.md @@ -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. diff --git a/crates/perry-runtime/src/builtins/arithmetic.rs b/crates/perry-runtime/src/builtins/arithmetic.rs index cda3970325..012a19d3e3 100644 --- a/crates/perry-runtime/src/builtins/arithmetic.rs +++ b/crates/perry-runtime/src/builtins/arithmetic.rs @@ -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>; + +#[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" diff --git a/crates/perry-runtime/src/gc/tests/runtime_roots.rs b/crates/perry-runtime/src/gc/tests/runtime_roots.rs index 14f67214eb..aac4b7d8ab 100644 --- a/crates/perry-runtime/src/gc/tests/runtime_roots.rs +++ b/crates/perry-runtime/src/gc/tests/runtime_roots.rs @@ -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; diff --git a/crates/perry-runtime/src/gc/tests/runtime_roots/interned_string_caches.rs b/crates/perry-runtime/src/gc/tests/runtime_roots/interned_string_caches.rs new file mode 100644 index 0000000000..c4bfa7d386 --- /dev/null +++ b/crates/perry-runtime/src/gc/tests/runtime_roots/interned_string_caches.rs @@ -0,0 +1,238 @@ +//! #7211 — the runtime interns two sets of strings in thread-local +//! `Cell<*mut StringHeader>`s, and both are GC roots. +//! +//! * `builtins::arithmetic`'s eight `typeof` results, so `typeof x` does not +//! rebuild `"string"` on every call; +//! * `json::raw_json`'s `"rawJSON"` key, so every `JSON.rawJSON(...)` writes +//! its own property under the same key object. +//! +//! Both held a raw nursery pointer that nothing registered, so the first minor +//! collection swept or evacuated the string and the cell named abandoned bytes +//! for the rest of the process. That is a *deterministic* use-after-free +//! rather than the intermittent kind: it goes bad at collection #0, not when a +//! collection happens to land in a window. `scripts/gc_root_dominance_check.py` +//! cannot see either one — it reads emitted LLVM IR, and these are runtime-side +//! tables. +//! +//! Two halves have to hold for each, and marking alone is not enough: a marked +//! but un-rewritten cell still hands out a pre-move address after a copying +//! minor, which is the whole failure. So each scanner gets a MARK test and a +//! REWRITE test, plus the registration check — a scanner a test can call +//! directly is a no-op in production until `gc_init` names it. +//! +//! The `typeof` rewrite test drives all EIGHT cells. `scan_typeof_string_roots_mut` +//! is eight hand-written `visit(...)` calls; dropping one is invisible to a +//! test that only exercises a few, and `bigint`/`symbol` are the two hardest to +//! reach from a `.ts` gap test. + +use super::*; +use crate::StringHeader; + +/// Empties both caches on entry and on exit, so a test starts from cells it +/// populated itself and a later test on this thread does not inherit a cell +/// pointing into this test's arena. +/// +/// It also pins the GC triggers to `usize::MAX` for the body. That is not +/// tidiness: these tests hand-build the evacuation the collector would +/// perform, and `evacuate_string` holds `from` across an +/// `arena_alloc_gc_old`, which reaches `gc_check_trigger()` on its +/// block-full slow path. A real collection landing in that window would move +/// `from` out from under the forwarding-address write and the test would go +/// red for a reason that has nothing to do with the scanner under test. This +/// makes "no collection happens here" enforced rather than merely observed. +struct InternedStringCacheGuard { + _triggers: GcTriggerThresholdTestGuard, +} + +impl InternedStringCacheGuard { + fn new() -> Self { + let triggers = GcTriggerThresholdTestGuard::suppress_automatic_triggers(); + crate::builtins::arithmetic::reset_typeof_string_cache_for_test(); + crate::json::raw_json::reset_raw_json_key_cache_for_test(); + Self { + _triggers: triggers, + } + } +} + +impl Drop for InternedStringCacheGuard { + fn drop(&mut self) { + crate::builtins::arithmetic::reset_typeof_string_cache_for_test(); + crate::json::raw_json::reset_raw_json_key_cache_for_test(); + } +} + +/// Allocate an old-gen destination and forward `from` → `to`, the shape an +/// evacuating minor leaves behind. Only safe to call under +/// `InternedStringCacheGuard`, which is what keeps the allocation from +/// collecting `from` before the forwarding address is written. +fn evacuate_string(from: *mut StringHeader) -> *mut StringHeader { + let to = crate::arena::arena_alloc_gc_old(64, 8, GC_TYPE_STRING); + unsafe { + set_forwarding_address(header_from_user_ptr(from as *const u8), to); + } + to as *mut StringHeader +} + +/// MARK. The cache is the only reference to these strings, so an unmarked cell +/// is a swept cell — and the cache would then hand out freed memory forever. +#[test] +fn typeof_string_cache_is_marked_by_the_collector() { + let _cache_guard = InternedStringCacheGuard::new(); + clear_marks(); + clear_mark_seeds(); + + crate::builtins::arithmetic::populate_typeof_string_cache_for_test(); + let cells = crate::builtins::arithmetic::typeof_string_cache_cells_for_test(); + let valid_ptrs = build_valid_pointer_set(); + + crate::builtins::arithmetic::scan_typeof_string_roots_mut(&mut RuntimeRootVisitor::for_mark( + &valid_ptrs, + )); + + for (i, cell) in cells.iter().enumerate() { + assert!(!cell.is_null(), "cache cell {i} should have been populated"); + assert_marked_user_ptr( + *cell as usize, + &format!("typeof cache cell {i} (nothing else references it)"), + ); + } + + clear_marks(); + clear_mark_seeds(); +} + +/// REWRITE, all eight cells. Marking keeps the string alive; only the rewrite +/// makes the cell name the surviving copy. Every cell is forwarded, so a +/// `visit(...)` line dropped from the scanner fails here. +#[test] +fn every_typeof_string_cache_cell_is_rewritten_by_the_collector() { + let _cache_guard = InternedStringCacheGuard::new(); + + crate::builtins::arithmetic::populate_typeof_string_cache_for_test(); + let before = crate::builtins::arithmetic::typeof_string_cache_cells_for_test(); + // The from-space objects must exist before the valid-pointer set is built: + // that set is what tells the rewrite visitor an address is a real heap + // object, exactly as in a real cycle. + let valid_ptrs = build_valid_pointer_set(); + let expected: Vec<*mut StringHeader> = before.iter().map(|p| evacuate_string(*p)).collect(); + + crate::builtins::arithmetic::scan_typeof_string_roots_mut( + &mut RuntimeRootVisitor::for_rewrite(&valid_ptrs), + ); + + let after = crate::builtins::arithmetic::typeof_string_cache_cells_for_test(); + for i in 0..8 { + assert_eq!( + after[i], expected[i], + "typeof cache cell {i} must be rewritten to the relocated string — \ + a marked-but-stale cell still hands `js_string_equals` a from-space \ + address on every later `typeof x === \"…\"` (#7211). \ + scan_typeof_string_roots_mut is eight hand-written visit() calls; \ + a missing one shows up here and nowhere else." + ); + } +} + +/// MARK. Same shape, and the reason it is a separate scanner: the `"rawJSON"` +/// key is written into every wrapper object, so a swept key means +/// `JSON.rawJSON(...)` stores its text under freed memory. +#[test] +fn raw_json_key_cache_is_marked_by_the_collector() { + let _cache_guard = InternedStringCacheGuard::new(); + clear_marks(); + clear_mark_seeds(); + + let key = crate::json::raw_json::raw_json_key_populate_for_test(); + assert!(!key.is_null()); + let valid_ptrs = build_valid_pointer_set(); + + crate::json::raw_json::scan_raw_json_key_root_mut(&mut RuntimeRootVisitor::for_mark( + &valid_ptrs, + )); + + assert_marked_user_ptr( + key as usize, + "the interned \"rawJSON\" key (nothing else references it)", + ); + + clear_marks(); + clear_mark_seeds(); +} + +/// REWRITE. +#[test] +fn raw_json_key_cache_is_rewritten_by_the_collector() { + let _cache_guard = InternedStringCacheGuard::new(); + + let from = crate::json::raw_json::raw_json_key_populate_for_test(); + let valid_ptrs = build_valid_pointer_set(); + let to = evacuate_string(from); + + crate::json::raw_json::scan_raw_json_key_root_mut(&mut RuntimeRootVisitor::for_rewrite( + &valid_ptrs, + )); + + assert_eq!( + crate::json::raw_json::raw_json_key_peek_for_test(), + to, + "the RAW_JSON_KEY cell must be rewritten to the relocated key — otherwise \ + every later JSON.rawJSON(...) sets its own property under a from-space \ + key (#7211)" + ); +} + +/// An empty cache is the state between process start and the first `typeof`, +/// and every cycle in that window scans it. A null cell must be skipped, not +/// treated as an address. +#[test] +fn scanning_an_empty_cache_is_a_no_op() { + let _cache_guard = InternedStringCacheGuard::new(); + let valid_ptrs = build_valid_pointer_set(); + + crate::builtins::arithmetic::scan_typeof_string_roots_mut( + &mut RuntimeRootVisitor::for_rewrite(&valid_ptrs), + ); + crate::json::raw_json::scan_raw_json_key_root_mut(&mut RuntimeRootVisitor::for_rewrite( + &valid_ptrs, + )); + + assert!( + crate::builtins::arithmetic::typeof_string_cache_cells_for_test() + .iter() + .all(|p| p.is_null()), + "scanning must not populate the typeof cache" + ); + assert!( + crate::json::raw_json::raw_json_key_peek_for_test().is_null(), + "scanning must not populate the rawJSON key cache" + ); +} + +/// …and both must actually be REGISTERED. Either scanner can be called +/// directly from a test whether or not `gc_init` ever mentions it, so the +/// wiring is asserted separately: an unregistered scanner is a no-op in +/// production, which is precisely the #7211 bug. +#[test] +fn interned_string_cache_scanners_are_registered() { + crate::gc::gc_init(); + let registered = |scanner: MutableRootScanner| { + crate::gc::roots::MUTABLE_ROOT_SCANNERS.with(|scanners| { + scanners + .borrow() + .iter() + .any(|entry| entry.scanner as usize == scanner as usize) + }) + }; + + assert!( + registered(crate::builtins::arithmetic::scan_typeof_string_roots_mut as MutableRootScanner), + "scan_typeof_string_roots_mut must be registered in gc_init — unregistered, \ + the eight interned `typeof` strings are swept by the first minor and every \ + later `typeof x === \"…\"` compares against from-space (#7211)" + ); + assert!( + registered(crate::json::raw_json::scan_raw_json_key_root_mut as MutableRootScanner), + "scan_raw_json_key_root_mut must be registered in gc_init (#7211)" + ); +} diff --git a/crates/perry-runtime/src/json/raw_json.rs b/crates/perry-runtime/src/json/raw_json.rs index 55bc9fda33..8b6be86c54 100644 --- a/crates/perry-runtime/src/json/raw_json.rs +++ b/crates/perry-runtime/src/json/raw_json.rs @@ -105,6 +105,28 @@ pub fn scan_raw_json_key_root_mut(visitor: &mut crate::gc::RuntimeRootVisitor<'_ }); } +/// Populate the cache the way `js_json_raw_json` does, and hand back the +/// pointer it cached. Test-only. +#[cfg(test)] +pub(crate) fn raw_json_key_populate_for_test() -> *mut StringHeader { + raw_json_key() as *mut StringHeader +} + +/// Read the cell WITHOUT populating it. Test-only. +#[cfg(test)] +pub(crate) fn raw_json_key_peek_for_test() -> *mut StringHeader { + RAW_JSON_KEY.with(|c| c.get()) +} + +/// Drop the cached key. Test-only, and the mirror of +/// `builtins::arithmetic::reset_typeof_string_cache_for_test` — a rooting test +/// has to start from an empty cache so the string it then allocates is its +/// own, in a known arena. +#[cfg(test)] +pub(crate) fn reset_raw_json_key_cache_for_test() { + RAW_JSON_KEY.with(|c| c.set(std::ptr::null_mut())); +} + /// Cached `"rawJSON"` key string used for the wrapper's own property. fn raw_json_key() -> *const StringHeader { RAW_JSON_KEY.with(|c| { diff --git a/test-files/test_gap_gc_typeof_string_cache_rooting.ts b/test-files/test_gap_gc_typeof_string_cache_rooting.ts index 75d3275f5d..72f76c80e5 100644 --- a/test-files/test_gap_gc_typeof_string_cache_rooting.ts +++ b/test-files/test_gap_gc_typeof_string_cache_rooting.ts @@ -32,6 +32,13 @@ // 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. +// +// ALL EIGHT CELLS. `scan_typeof_string_roots_mut` is eight hand-written +// `visit(...)` calls, so a cell nothing exercises is a cell whose registration +// can be dropped without a red test. `bigint` and `symbol` are here for that +// reason and no other — they are the two the first cut of this test missed. +// The Rust side asserts the same thing from the other direction, in +// `gc/tests/runtime_roots/interned_string_caches.rs`. function churn(x: number): number { const bits: any[] = []; @@ -43,7 +50,7 @@ function churn(x: number): number { function run(): number { let bad = 0; - const vals: any[] = ["a", 1, true, {}, undefined, run]; + const vals: any[] = ["a", 1, true, {}, undefined, run, BigInt(1), Symbol()]; const want: string[] = [ "string", "number", @@ -51,10 +58,12 @@ function run(): number { "object", "undefined", "function", + "bigint", + "symbol", ]; for (let r = 0; r < 500; r++) { churn(r); - for (let k = 0; k < 6; k++) { + for (let k = 0; k < 8; 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.