From 0b22c9d42ca3283221787729d4ccdb3361a1ddc8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sun, 2 Aug 2026 10:18:18 +0200 Subject: [PATCH 1/5] fix(gc): a Symbol-typed local is a GC-heap reference, and gets a shadow slot (#7236) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `collectors/pointer_locals.rs` classified `Type::Symbol` as a non-pointer, so a `Symbol`-typed local never got a shadow-stack slot and sat in a plain `alloca` across every collection point in its scope. `alloc_symbol` is `gc_malloc(size_of::(), GC_TYPE_STRING)` and `js_symbol_new` returns it POINTER_TAG-boxed; nothing else holds a fresh symbol, so with no slot the malloc sweep inside the copying minor frees one that is still live. The drift had already happened: `typed_shape::type_is_pointer_bearing` — an exhaustive match, and the function that lays out the GC's own field masks — answered `true` for `Symbol` while this predicate answered "non-pointer". That is exactly what the doc comment over `is_definitely_non_pointer_type` predicted would cost a use-after-move. So the three copies of the question are now one exhaustive definition, and the other two delegate. Third site, a separate hazard from the missing slot: the same `Symbol` entry in `expr/shadow_slot.rs`'s `expr_is_known_non_pointer_shadow_value` suppressed temp-root protection for a symbol operand held across an allocating call. --- .../src/collectors/pointer_locals.rs | 195 +++++++++++++----- crates/perry-codegen/src/expr/shadow_slot.rs | 33 +-- crates/perry-codegen/src/typed_shape.rs | 45 ++++ 3 files changed, 210 insertions(+), 63 deletions(-) diff --git a/crates/perry-codegen/src/collectors/pointer_locals.rs b/crates/perry-codegen/src/collectors/pointer_locals.rs index a7163bfd22..311c94553d 100644 --- a/crates/perry-codegen/src/collectors/pointer_locals.rs +++ b/crates/perry-codegen/src/collectors/pointer_locals.rs @@ -184,26 +184,23 @@ impl HirTypeFacts for PointerAnalysisFacts<'_> { /// Types that can NEVER hold a heap pointer, and therefore cost a local its /// shadow-stack slot in [`collect_pointer_typed_locals`]. /// -/// **This is the single definition.** It used to be a nested `fn` inside -/// `collect_pointer_typed_locals`; it is module-level and `pub(crate)` because -/// anything that decides a value may be treated as a rooted pointer has to -/// agree with the pass that actually assigns the root slot. A second copy -/// drifting by one `Type` variant would mean a value this collector left -/// unrooted while another pass treated it as a live, movable pointer — a -/// use-after-move under the evacuating minor (#7019), not a cosmetic -/// inconsistency. `collectors/ptr_shape_returns.rs` (#7034 §4) is the current -/// second caller. +/// **Exactly the negation of [`crate::typed_shape::type_is_pointer_bearing`], +/// by construction rather than by review.** This used to be a second hand- +/// maintained `matches!` list, and it had already drifted from that one by a +/// single variant: `Type::Symbol`. `typed_shape` said pointer (correctly — +/// `js_symbol_new` NaN-boxes a movable `gc_malloc`), this said non-pointer, so +/// a `Symbol`-typed local got no shadow-stack slot and sat in a plain `alloca` +/// across every collection point in its scope (#7236). That is precisely the +/// failure the previous comment here predicted: "a second copy drifting by one +/// `Type` variant … a use-after-move under the evacuating minor (#7019), not a +/// cosmetic inconsistency." +/// +/// So there is now one copy, it is exhaustive, and a new `Type` variant is a +/// compile error over there instead of a silent "not a pointer" here. +/// `collectors/ptr_shape_returns.rs` (#7034 §4) is the second caller of this +/// negation and inherits the answer. pub(crate) fn is_definitely_non_pointer_type(ty: &Type) -> bool { - matches!( - ty, - Type::Number - | Type::Int32 - | Type::Boolean - | Type::Null - | Type::Void - | Type::Never - | Type::Symbol - ) || matches!(ty, Type::Union(variants) if variants.iter().all(is_definitely_non_pointer_type)) + !crate::typed_shape::type_is_pointer_bearing(ty) } pub fn collect_pointer_typed_locals( @@ -212,39 +209,18 @@ pub fn collect_pointer_typed_locals( flat_const_ids: &HashSet, ) -> std::collections::HashMap { use perry_hir::Stmt; + /// Does this local need a shadow-stack slot? + /// + /// The third copy of the pointer question, now routed to the one + /// definition (#7236). It was the exact complement of + /// [`is_definitely_non_pointer_type`] over every `Type` variant *except* + /// `Symbol`, which neither of them claimed — so a `Symbol` local was + /// simultaneously "not a pointer" (no slot here) and "pointer-bearing" + /// (`typed_shape`, which lays out the GC's own field masks). The + /// per-variant rationale that used to live here moved to + /// `type_is_pointer_bearing`'s doc comment with it. fn is_ptr_typed(ty: &Type) -> bool { - matches!( - ty, - Type::String - // A string-LITERAL type (`"foo"`, or a `"a" | "b"` discriminant - // union member) is a heap String at runtime — it needs a root - // slot exactly like `Type::String`, or the moving-GC precise scan - // reaps a live string → silent corruption. - | Type::StringLiteral(_) - | Type::Array(_) - | Type::Tuple(_) - | Type::Object(_) - | Type::Named(_) - // An unresolved generic type parameter (`T`) can bind to any - // heap type; treat it as a pointer (fail-safe). - | Type::TypeVar(_) - | Type::Promise(_) - | Type::Function(_) - // A generic instantiation (`Map`, `Set`, `WeakMap`, - // `Box`, `Array`, a user generic class, …) is always a - // heap-reference type. Without this, a `Map`/`Set`-typed local - // got NO shadow-stack slot, so the PRECISE moving-GC root scan - // never saw it — the object was reaped as dead while still live - // (crash: "grown Map must retain its side-allocation owner - // record"). The non-moving default GC hid this via its - // conservative C-stack scan. Treating a rare non-pointer generic - // value as a root is harmless: the GC decode rejects any slot - // value that isn't a live heap pointer. - | Type::Generic { .. } - | Type::BigInt - | Type::Any - | Type::Unknown - ) || matches!(ty, Type::Union(variants) if variants.iter().any(is_ptr_typed)) + crate::typed_shape::type_is_pointer_bearing(ty) } fn expr_value_type( @@ -1165,4 +1141,121 @@ mod tests { assert!(!slots.contains_key(&1)); assert!(slots.contains_key(&2)); } + + /// Every `Type` variant, with the answer pinned. + /// + /// #7236: `is_definitely_non_pointer_type` and `typed_shape`'s + /// `type_is_pointer_bearing` are two names for one question, and they had + /// drifted by exactly one variant — `Symbol` — which is how a + /// `POINTER_TAG`-boxed, `gc_malloc`'d, MOVABLE object came to be classified + /// as an immediate. They are now the same function, so this test is about + /// the ANSWERS: it enumerates the enum and fails if any classification + /// flips. `type_is_pointer_bearing`'s exhaustive `match` covers the other + /// half (a new variant is a compile error, not a silent "non-pointer"). + #[test] + fn every_type_variant_has_its_pointer_classification_pinned() { + // The six immediates: a raw f64, INT32_TAG, TAG_TRUE/TAG_FALSE, + // TAG_NULL, TAG_UNDEFINED, and the uninhabited type. No allocator + // produces any of them, so there is nothing for the collector to see. + let non_pointers = [ + Type::Number, + Type::Int32, + Type::Boolean, + Type::Null, + Type::Void, + Type::Never, + ]; + // Everything else is, or can be, a heap reference. `Symbol` is in this + // list and not the one above: that IS #7236. + let pointers = [ + Type::Symbol, + Type::String, + Type::StringLiteral("foo".to_string()), + Type::BigInt, + Type::Array(Box::new(Type::Number)), + Type::Tuple(vec![Type::Number]), + Type::Object(Default::default()), + Type::Function(FunctionType { + params: Vec::new(), + return_type: Box::new(Type::Any), + is_async: false, + is_generator: false, + }), + Type::Promise(Box::new(Type::Number)), + Type::Named("C".to_string()), + Type::Generic { + base: "Map".to_string(), + type_args: vec![Type::String, Type::Number], + }, + Type::TypeVar("T".to_string()), + Type::Any, + Type::Unknown, + ]; + for ty in &non_pointers { + assert!( + is_definitely_non_pointer_type(ty), + "{ty:?} must be a non-pointer" + ); + assert!(!crate::typed_shape::type_is_pointer_bearing(ty)); + } + for ty in &pointers { + assert!( + !is_definitely_non_pointer_type(ty), + "{ty:?} must be pointer-possible — a local of this type needs a \ + shadow-stack slot or the precise moving-GC root scan cannot see it" + ); + assert!(crate::typed_shape::type_is_pointer_bearing(ty)); + } + // A union is a non-pointer only when EVERY member is. + assert!(is_definitely_non_pointer_type(&Type::Union(vec![ + Type::Number, + Type::Boolean + ]))); + assert!(!is_definitely_non_pointer_type(&Type::Union(vec![ + Type::Number, + Type::Symbol + ]))); + } + + /// #7236's reproducer, at the collector: `const s = Symbol("x")` must get a + /// slot. `alloc_symbol` is `gc_malloc(_, GC_TYPE_STRING)` and + /// `GC_TYPE_STRING` is `movable: true`, so without a slot the local sits in + /// a plain `alloca` across every collection point in its scope — exactly + /// what `gc_root_dominance_check.py --unrooted-allocas --moving-only` + /// reported twice on `test_gap_class_forward_capture_6523`. + #[test] + fn a_symbol_typed_local_gets_a_shadow_slot() { + let stmts = vec![Stmt::Let { + id: 1, + name: "s".to_string(), + ty: Type::Symbol, + mutable: false, + init: Some(Expr::SymbolNew(Some(Box::new(Expr::String( + "x".to_string(), + ))))), + }]; + let slots = collect_pointer_typed_locals(&[], &stmts, &HashSet::new()); + assert!(slots.contains_key(&1), "Symbol local must be shadow-rooted"); + } + + /// The other half, and the sharper one: the declared type is `any`, so the + /// slot survives only if the write-refinement fixpoint ALSO agrees that + /// `Symbol` is a pointer. `is_definitely_non_pointer_type` is what that + /// loop consults (`all_non_pointer`), so a fix applied to `is_ptr_typed` + /// alone would hand out the slot and then take it away again. + #[test] + fn an_inferred_symbol_local_keeps_its_shadow_slot() { + let stmts = vec![Stmt::Let { + id: 1, + name: "s".to_string(), + ty: Type::Any, + mutable: false, + init: Some(Expr::SymbolNew(None)), + }]; + let slots = collect_pointer_typed_locals(&[], &stmts, &HashSet::new()); + assert!( + slots.contains_key(&1), + "a local refined to Symbol must keep its shadow slot" + ); + } } diff --git a/crates/perry-codegen/src/expr/shadow_slot.rs b/crates/perry-codegen/src/expr/shadow_slot.rs index 4fca697ffd..00d03be235 100644 --- a/crates/perry-codegen/src/expr/shadow_slot.rs +++ b/crates/perry-codegen/src/expr/shadow_slot.rs @@ -44,19 +44,28 @@ pub(crate) fn expr_is_known_non_pointer_shadow_value(ctx: &FnCtx<'_>, expr: &Exp Expr::LocalGet(id) => { // A reserved shadow slot means the local is pointer-possible even // if its initializer refined `local_types` to a scalar. + // + // #7236: this list also carried `HirType::Symbol`, and it is a + // SEPARATE hazard from the missing shadow slot — a `true` here + // suppresses `temp_root`'s argument/operand rooting, so + // `obj[s] = alloc()` with `s: symbol` left the movable symbol + // unrooted across the allocating call even once the local itself + // had a slot. `lower_call/closure_analysis.rs`'s + // `local_is_inert_primitive` never listed `Symbol`; this copy and + // `collectors/pointer_locals.rs` were the two that did. + // + // Derived from the one definition rather than restated, but kept + // NON-`Union` on purpose: `type_is_pointer_bearing` answers `false` + // for an all-scalar union (`number | undefined`), which would + // WIDEN this suppression to locals it never covered. That is a + // plausible optimisation and an unmeasured one; it is not this + // change. The guard makes the arm exactly the old list minus + // `Symbol`. !ctx.shadow_slot_map.contains_key(id) - && matches!( - ctx.local_types.get(id), - Some( - HirType::Number - | HirType::Int32 - | HirType::Boolean - | HirType::Null - | HirType::Void - | HirType::Never - | HirType::Symbol - ) - ) + && ctx.local_types.get(id).is_some_and(|ty| { + !matches!(ty, HirType::Union(_)) + && !crate::typed_shape::type_is_pointer_bearing(ty) + }) } Expr::Compare { .. } | Expr::Void(_) => true, Expr::Unary { .. } => true, diff --git a/crates/perry-codegen/src/typed_shape.rs b/crates/perry-codegen/src/typed_shape.rs index b38518fa4d..fc2424ca67 100644 --- a/crates/perry-codegen/src/typed_shape.rs +++ b/crates/perry-codegen/src/typed_shape.rs @@ -1,5 +1,50 @@ use perry_hir::types::Type; +/// ★ Can a value of this type be a heap reference the collector must see? +/// +/// **This is the single definition of "pointer" for the whole codegen crate**, +/// and every consumer of that question routes here: +/// [`crate::collectors::pointer_locals::is_definitely_non_pointer_type`] (which +/// is its exact negation, and through it the shadow-slot assignment pass and +/// `collectors/ptr_shape_returns.rs`), the typed-shape pointer masks below, and +/// `expr/object_literal.rs`. +/// +/// It is an **exhaustive `match`, deliberately**, not a `matches!` list. A +/// `matches!` list silently defaults a newly added `Type` variant to one answer, +/// and here the two answers are not symmetric: defaulting to "not a pointer" +/// means a local with **no shadow-stack slot**, i.e. a heap object the precise +/// moving-GC root scan cannot see. That is a use-after-move / premature sweep, +/// not a missed optimisation. The exhaustive match makes the compiler ask. +/// +/// Per-variant notes, kept here because this is the only copy: +/// +/// * `Symbol` — **a pointer.** `js_symbol_new` returns `POINTER_TAG`-boxed +/// storage from `alloc_symbol`, which is +/// `gc_malloc(size_of::(), GC_TYPE_STRING)`, and +/// `GC_TYPE_STRING`'s type-info entry is `movable: true`. It was listed as a +/// non-pointer by `is_definitely_non_pointer_type` (#7236) while this +/// function already answered `true` — the exact one-variant drift the doc +/// comment over there warned about, which is why there is now one copy. +/// * `StringLiteral` — a string-LITERAL type (`"foo"`, or a `"a" | "b"` +/// discriminant union member) is an ordinary heap `String` at runtime. +/// * `TypeVar` — an unresolved generic type parameter (`T`) can bind to any +/// heap type; treat it as a pointer (fail-safe). +/// * `Generic` — a generic instantiation (`Map`, `Set`, `WeakMap`, +/// `Box`, `Array`, a user generic class, …) is always a heap-reference +/// type. Without this a `Map`/`Set`-typed local got no shadow-stack slot and +/// was reaped while live (#7019, crash: "grown Map must retain its +/// side-allocation owner record"); the non-moving default GC hid it behind +/// its conservative C-stack scan. +/// * `Number` / `Int32` / `Boolean` / `Null` / `Void` — NaN-boxed immediates +/// (a raw `f64`, `INT32_TAG`, `TAG_TRUE`/`TAG_FALSE`, `TAG_NULL`, +/// `TAG_UNDEFINED`): no allocator produces one, so there is nothing to root. +/// `Never` has no value at all. These six are the complete non-pointer set. +/// * `Union` — pointer-bearing if ANY member is, so the negation is "every +/// member is a non-pointer", which is what the root-slot decision needs. +/// +/// Over-answering `true` is harmless: the GC decode rejects any slot value that +/// is not a live heap pointer, so a spurious root costs one slot, never +/// correctness. Under-answering is #7019 / #7236. pub(crate) fn type_is_pointer_bearing(ty: &Type) -> bool { match ty { Type::String From 259c30a19b6389f6af2ec2390f14ed7f2c80aaa1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sun, 2 Aug 2026 10:37:59 +0200 Subject: [PATCH 2/5] test(gc): witness for the unrooted Symbol local (#7236) Red at base (`A 30 B 20` on loop_polls AND on the shipped default, 3/3 deterministic), green after (`A 0 B 0`), byte-exact vs node 26.5.1. Registered in test-parity/gc_repsel_corpus.txt, which #7228's gc-moving-witnesses arm gates and which rejects UNVER as hard as FAIL. --- .../test_gap_gc_symbol_local_rooting.ts | 115 ++++++++++++++++++ test-parity/gc_repsel_corpus.txt | 35 ++++++ 2 files changed, 150 insertions(+) create mode 100644 test-files/test_gap_gc_symbol_local_rooting.ts diff --git a/test-files/test_gap_gc_symbol_local_rooting.ts b/test-files/test_gap_gc_symbol_local_rooting.ts new file mode 100644 index 0000000000..2945ccc46c --- /dev/null +++ b/test-files/test_gap_gc_symbol_local_rooting.ts @@ -0,0 +1,115 @@ +// #7236: a `Symbol`-typed local names a GC-heap object, and needs a +// shadow-stack slot like any other heap reference. +// +// `collectors/pointer_locals.rs` listed `Type::Symbol` in +// `is_definitely_non_pointer_type`, so `const s = Symbol("w")` got NO +// shadow-stack slot and lived in a plain `alloca` for its whole scope — +// invisible to the precise root walk. `typed_shape::type_is_pointer_bearing`, +// which lays out the GC's own field masks, said `Symbol` IS a pointer, so the +// two copies of one question had drifted by exactly one variant. +// +// ★ WHAT ACTUALLY GOES WRONG IS A PREMATURE FREE, NOT A STALE ADDRESS. +// `alloc_symbol` is `gc_malloc(size_of::(), GC_TYPE_STRING)`, and +// `gc_malloc` (gc/malloc.rs) is the SYSTEM allocator with a `GcHeader` in front +// — not an arena allocation. So a fresh symbol is not in the nursery and the +// copying minor cannot relocate it; what it can do is `dealloc` it. Under the +// #7235 taxonomy the local is RECLAIMABLE-but-not-MOVABLE, the #7230 class. +// `alloc_symbol`'s own comment concedes the liveness half — a fresh symbol is +// kept alive "through the SYMBOL_REGISTRY (for registered symbols) or NOT AT +// ALL" — and `SYMBOL_POINTERS` does not save it either: +// `scan_symbol_pointer_metadata_roots_mut` visits it with +// `visit_metadata_usize_slot`, which rewrites a recorded address WITHOUT +// marking. With no shadow slot the unrooted `alloca` is the only reference in +// the world, the collector cannot see it, and `sweep_malloc_objects` frees a +// live symbol. +// +// ★ WHY THE PRESSURE IS SYMBOLS AND NOT OBJECTS. The malloc sweep inside the +// copying minor is gated: `copied_minor_malloc_sweep_due` is true only for a +// `GcTriggerKind::MallocCount` collection or once `malloc_object_count()` +// passes its trigger. Driving pressure with object/array churn reaches the +// ARENA trigger instead, so the malloc sweep is only occasionally due and the +// defect reproduces intermittently — measured at 1 failure in 80 probes, and at +// 0 on four of five repeats of another shape. Allocating symbols is what makes +// the malloc-count trigger the one that fires, and with it the reproduction +// deterministic. It also makes the reuse deterministic: the freed block is +// exactly the size class the next `Symbol()` asks for, so the recycled bytes +// are what the stale local reads back. +// +// Measured on `f8f1e7188` (before the fix), `--release`: `A 34` on the +// `loop_polls` arm, identical on three consecutive runs, and `A 34` on the +// SHIPPED DEFAULT with no GC env at all. Oracle and fixed build both print +// `A 0`. This one does not need a moving arm to bite — the conservative stack +// scan is what has been hiding it, and it stops hiding it as soon as the +// collection is malloc-count driven. +// +// ★ WHY THE PROBES ARE NOT IDENTITY COMPARISONS. `js_symbol_equals` +// (symbol/constructors.rs) falls back from a bits comparison to dereferencing +// both headers and comparing `id`, and `js_is_symbol` falls back to reading +// `magic` — both answer correctly off a freed-but-not-yet-recycled header, so +// an identity probe reports nothing. What a reaped symbol cannot survive is +// having its own description read back after its storage is reused. +// +// The observable is byte-for-byte identical to `node --experimental-strip-types`. + +function symChurn(n: number): number { + let k = 0; + for (let i = 0; i < n; i++) { + const t = Symbol("t"); + if (typeof t === "symbol") { + k++; + } + } + return k; +} + +// A: the symbol is referenced by NOTHING but the local across the collection, +// and is then asked what it is. +function heldAcrossCollection(): number { + let bad = 0; + for (let r = 0; r < 20; r++) { + const s = Symbol("w"); + if (symChurn(50000) !== 50000) { + bad++; + } + if (typeof s !== "symbol") { + bad++; + } + if (String(s) !== "Symbol(w)") { + bad++; + } + if (s.description !== "w") { + bad++; + } + } + return bad; +} + +// B: the same local, then used as a COMPUTED KEY — the issue's other named +// shape. The store must land on a symbol key whose description round-trips, so +// a local whose storage was recycled into one of `symChurn`'s throwaway +// `Symbol("t")`s is caught by the description rather than by the count. +function usedAsComputedKey(): number { + let bad = 0; + for (let r = 0; r < 20; r++) { + const s = Symbol("k"); + if (symChurn(50000) !== 50000) { + bad++; + } + const o: any = {}; + o[s] = r; + if (o[s] !== r) { + bad++; + } + const keys = Object.getOwnPropertySymbols(o); + if (keys.length !== 1) { + bad++; + } + if (String(keys[0]) !== "Symbol(k)") { + bad++; + } + } + return bad; +} + +console.log("A", heldAcrossCollection()); +console.log("B", usedAsComputedKey()); diff --git a/test-parity/gc_repsel_corpus.txt b/test-parity/gc_repsel_corpus.txt index 5c469558bd..5f824fe107 100644 --- a/test-parity/gc_repsel_corpus.txt +++ b/test-parity/gc_repsel_corpus.txt @@ -390,3 +390,38 @@ test_gap_gc_interval_args_rooting # bad at collection #0 and stays bad, where an unrooted REGISTER needs a # collection to land in a narrow window. test_gap_gc_process_env_cache_rooting + +# --- #7236: a Symbol-typed local had no shadow slot at all ------------------ +# Not a representation file, so registered explicitly per the header rule. +# `collectors/pointer_locals.rs` classified `Type::Symbol` as a NON-pointer, so +# `const s = Symbol("w")` got no shadow-stack slot and lived in a plain +# `alloca` — the last violation `gc_root_dominance_check.py --unrooted-allocas +# --moving-only` reported on the #7235 corpus, and the one thing standing +# between `gc-root-dominance` and promotion to a required context (#7198). +# +# ★ THE FAILURE IS A PREMATURE FREE, NOT A STALE ADDRESS, and that dictates the +# shape of the file. `alloc_symbol` is `gc_malloc(_, GC_TYPE_STRING)` and +# `gc_malloc` is the SYSTEM allocator with a GcHeader in front, not an arena +# allocation — so the copying minor cannot relocate a symbol, it can only +# `dealloc` it. Under #7235's taxonomy the local is RECLAIMABLE and not +# MOVABLE. `SYMBOL_POINTERS` does not save it: +# `scan_symbol_pointer_metadata_roots_mut` uses `visit_metadata_usize_slot`, +# which rewrites without marking, exactly as `alloc_symbol`'s own comment says +# ("kept alive ... or NOT AT ALL"). +# +# So the pressure is SYMBOLS, not objects. `copied_minor_malloc_sweep_due` +# gates the malloc sweep on a `MallocCount` trigger or the malloc-object +# threshold; object churn reaches the ARENA trigger instead, and with it the +# defect reproduces intermittently — measured at 1 failure in 80 probes on one +# object-churn shape, and 0 on four of five repeats of another. Symbol churn +# makes the malloc-count trigger the one that fires AND makes the freed block +# the exact size class the next `Symbol()` reuses. +# +# Measured at base (`f8f1e7188`), `--release`, oracle node 26.5.1 (`A 0 B 0`): +# `A 30 B 20` on `loop_polls`, 3/3 identical, and `A 30 B 20` on the SHIPPED +# DEFAULT with no GC env at all, 2/2. `A 0 B 0` after the fix on both, 3/3. +# +# It therefore does NOT need a `requires=move` arm — the conservative stack +# scan is what was hiding it, and a malloc-count-driven collection stops it +# hiding. Expect PASS rather than UNVER wherever the arm collects at all. +test_gap_gc_symbol_local_rooting From a9a83e5847eaf7cef438831134087f91b379703a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sun, 2 Aug 2026 10:45:39 +0200 Subject: [PATCH 3/5] ci(gc): gate the unrooted-alloca mode, now that it reads 0 (#7236) --unrooted-allocas --moving-only was 98 before #7235, 2 after, and 0 once Type::Symbol stopped being classified as an immediate. That number is the stated condition for promoting gc-root-dominance to a required context (#7198); a number that is an acceptance criterion and is checked by nothing regresses silently. Verified it can fail: exit 1 at f8f1e7188, exit 0 here. --- .github/workflows/gc-root-dominance.yml | 46 +++++++++++++++++-- .../src/collectors/pointer_locals.rs | 33 +++++++------ crates/perry-codegen/src/typed_shape.rs | 19 ++++++-- docs/src/internals/gc-rooting-invariant.md | 34 +++++++++++--- 4 files changed, 105 insertions(+), 27 deletions(-) diff --git a/.github/workflows/gc-root-dominance.yml b/.github/workflows/gc-root-dominance.yml index 973d4a69b6..e54815f4b1 100644 --- a/.github/workflows/gc-root-dominance.yml +++ b/.github/workflows/gc-root-dominance.yml @@ -28,9 +28,17 @@ name: GC Root Dominance # corollary in CLAUDE.md warns about. # # **ACTION REQUIRED, and it is not something this workflow can do to -# itself**: with the allowlist below the job is green on `main`, so a -# repo admin must add `gc-root-dominance` to branch protection's required -# contexts. Until that happens this file is documentation, not a gate. +# itself**: a repo admin must add `gc-root-dominance` to branch +# protection's required contexts. Until that happens this file is +# documentation, not a gate. +# +# Both conditions #7198 named are met as of #7236: the dominance check is +# green with an EMPTY allowlist, and `--unrooted-allocas --moving-only` -- +# which was 98 before #7235 and 2 after -- now reads 0 and is a step +# below. Promote after this job's first green run on `main` WITH that +# step, not before: a gate that has never been green in its current shape +# blocks every open PR the day it becomes required, which is the corollary +# that produced this whole paragraph. # See docs/src/internals/gc-rooting-invariant.md, "Promoting this gate". # 3. `concurrency` cancels pull-request runs only, never `main` runs; # 4. the subject is ASSERTED live, not assumed, at three levels. @@ -181,6 +189,38 @@ jobs: --seeded-violations 40 \ -v + # ★ The OTHER mode, and until #7236 it had never been at zero. + # + # `--unrooted-allocas` asks a different question from the step above: not + # "is the root store late" but "is there a root store AT ALL". #7235 split + # its heap-source predicate by movability and got the count from 98 to 2, + # and both residuals were one bug -- `Type::Symbol` classified as an + # immediate, so a `Symbol` local got no shadow slot. That is now fixed and + # the corpus reads 0, which is the condition #7198 named for promoting + # this job to a required context. + # + # A number that is the acceptance criterion for a promotion and is checked + # by nothing will regress silently -- CLAUDE.md hazard 4, applied to the + # measurement rather than to the job. So it is a step. It shares the + # corpus, the exemptions and the allowlist file with the step above, and + # carries the same liveness floors: "0 violations" over a corpus that did + # not compile means nothing. + # + # If this arm ever goes red for a hazard that is real but deliberately + # deferred, the answer is an entry in gc_root_dominance_allowlist.json + # with an issue number, NOT deleting the step -- rule 1 in that file + # (an entry matching nothing fails the build) is what makes the tolerance + # temporary. + - name: Check that every GC value in an alloca has a root store + run: | + set -euo pipefail + python3 scripts/gc_root_dominance_check.py ir-corpus \ + --unrooted-allocas \ + --moving-only \ + --min-files 90 --min-binds 1500 --min-funcs 1200 \ + --allowlist scripts/gc_root_dominance_allowlist.json \ + -v + - name: Upload the IR corpus on failure if: failure() uses: actions/upload-artifact@v7 diff --git a/crates/perry-codegen/src/collectors/pointer_locals.rs b/crates/perry-codegen/src/collectors/pointer_locals.rs index 311c94553d..357a6fc82b 100644 --- a/crates/perry-codegen/src/collectors/pointer_locals.rs +++ b/crates/perry-codegen/src/collectors/pointer_locals.rs @@ -188,12 +188,16 @@ impl HirTypeFacts for PointerAnalysisFacts<'_> { /// by construction rather than by review.** This used to be a second hand- /// maintained `matches!` list, and it had already drifted from that one by a /// single variant: `Type::Symbol`. `typed_shape` said pointer (correctly — -/// `js_symbol_new` NaN-boxes a movable `gc_malloc`), this said non-pointer, so -/// a `Symbol`-typed local got no shadow-stack slot and sat in a plain `alloca` -/// across every collection point in its scope (#7236). That is precisely the -/// failure the previous comment here predicted: "a second copy drifting by one -/// `Type` variant … a use-after-move under the evacuating minor (#7019), not a -/// cosmetic inconsistency." +/// `js_symbol_new` NaN-boxes a `gc_malloc`'d `SymbolHeader` that a malloc +/// sweep inside the copying minor frees when nothing marks it), this said +/// non-pointer, so a `Symbol`-typed local got no shadow-stack slot and sat in a +/// plain `alloca` across every collection point in its scope (#7236). That is +/// precisely the failure the previous comment here predicted: "a second copy +/// drifting by one `Type` variant … a use-after-move under the evacuating +/// minor (#7019), not a cosmetic inconsistency." The realised failure turned +/// out to be the sibling one — a premature free rather than a stale address, +/// because `gc_malloc` is outside the arena — which is the distinction #7235 +/// drew between RECLAIMABLE and MOVABLE and the reason it drew it. /// /// So there is now one copy, it is exhaustive, and a new `Type` variant is a /// compile error over there instead of a silent "not a pointer" here. @@ -1147,8 +1151,9 @@ mod tests { /// #7236: `is_definitely_non_pointer_type` and `typed_shape`'s /// `type_is_pointer_bearing` are two names for one question, and they had /// drifted by exactly one variant — `Symbol` — which is how a - /// `POINTER_TAG`-boxed, `gc_malloc`'d, MOVABLE object came to be classified - /// as an immediate. They are now the same function, so this test is about + /// `POINTER_TAG`-boxed, `gc_malloc`'d, collector-freed object came to be + /// classified as an immediate. They are now the same function, so this test + /// is about /// the ANSWERS: it enumerates the enum and fails if any classification /// flips. `type_is_pointer_bearing`'s exhaustive `match` covers the other /// half (a new variant is a compile error, not a silent "non-pointer"). @@ -1218,11 +1223,13 @@ mod tests { } /// #7236's reproducer, at the collector: `const s = Symbol("x")` must get a - /// slot. `alloc_symbol` is `gc_malloc(_, GC_TYPE_STRING)` and - /// `GC_TYPE_STRING` is `movable: true`, so without a slot the local sits in - /// a plain `alloca` across every collection point in its scope — exactly - /// what `gc_root_dominance_check.py --unrooted-allocas --moving-only` - /// reported twice on `test_gap_class_forward_capture_6523`. + /// slot. `alloc_symbol` is `gc_malloc(_, GC_TYPE_STRING)`, and a fresh + /// symbol is reachable from nothing else (`SYMBOL_POINTERS` is visited + /// metadata-only), so without a slot the local sits in a plain `alloca` + /// across every collection point in its scope and the malloc sweep inside + /// the copying minor frees it while it is live — exactly what + /// `gc_root_dominance_check.py --unrooted-allocas --moving-only` reported + /// twice on `test_gap_class_forward_capture_6523`. #[test] fn a_symbol_typed_local_gets_a_shadow_slot() { let stmts = vec![Stmt::Let { diff --git a/crates/perry-codegen/src/typed_shape.rs b/crates/perry-codegen/src/typed_shape.rs index fc2424ca67..b6da04c634 100644 --- a/crates/perry-codegen/src/typed_shape.rs +++ b/crates/perry-codegen/src/typed_shape.rs @@ -20,11 +20,20 @@ use perry_hir::types::Type; /// /// * `Symbol` — **a pointer.** `js_symbol_new` returns `POINTER_TAG`-boxed /// storage from `alloc_symbol`, which is -/// `gc_malloc(size_of::(), GC_TYPE_STRING)`, and -/// `GC_TYPE_STRING`'s type-info entry is `movable: true`. It was listed as a -/// non-pointer by `is_definitely_non_pointer_type` (#7236) while this -/// function already answered `true` — the exact one-variant drift the doc -/// comment over there warned about, which is why there is now one copy. +/// `gc_malloc(size_of::(), GC_TYPE_STRING)`. Note *which* way +/// it is dangerous: `gc_malloc` is the SYSTEM allocator with a `GcHeader` in +/// front, not an arena allocation, so the copying minor cannot relocate a +/// symbol — it can `dealloc` it (`sweep_malloc_objects`, reached from the +/// copying minor whenever `copied_minor_malloc_sweep_due`). Under #7235's +/// taxonomy a symbol is RECLAIMABLE and not MOVABLE, and nothing else holds a +/// fresh one: `alloc_symbol`'s own comment says it is kept alive "through the +/// SYMBOL_REGISTRY … or NOT AT ALL", and `SYMBOL_POINTERS` is visited with +/// `visit_metadata_usize_slot`, which rewrites without marking. So an +/// unrooted `Symbol` local is a premature FREE (#7230's class), not a stale +/// address. It was listed as a non-pointer by +/// `is_definitely_non_pointer_type` (#7236) while this function already +/// answered `true` — the exact one-variant drift the doc comment over there +/// warned about, which is why there is now one copy. /// * `StringLiteral` — a string-LITERAL type (`"foo"`, or a `"a" | "b"` /// discriminant union member) is an ordinary heap `String` at runtime. /// * `TypeVar` — an unresolved generic type parameter (`T`) can bind to any diff --git a/docs/src/internals/gc-rooting-invariant.md b/docs/src/internals/gc-rooting-invariant.md index 1502597ae1..0f3b12bd06 100644 --- a/docs/src/internals/gc-rooting-invariant.md +++ b/docs/src/internals/gc-rooting-invariant.md @@ -192,9 +192,13 @@ bind-anchored check is structurally blind to: the value lives in a plain `alloca_entry` for its whole lifetime, so there is no `js_shadow_slot_bind` to anchor on and a scan that starts from binds calls the function clean. It found `lower_call/new.rs`'s inline-ctor `this_slot` independently of any runtime -probe. **The gate does not run this mode yet** — its remaining hits are the -caches, staging arrays and inlined-callee params tracked as #7210, and they are -deliberately not in the allowlist, which covers the bind-anchored shape only. +probe. + +**The gate runs this mode as of #7236, and the corpus reads 0.** It could not +before: #7210 measured 66 hits and triaged every one as a false positive, #7235 +split the heap-source predicate by movability (98 → 2 on a grown corpus), and +the 2 residuals were one bug — `collectors/pointer_locals.rs` classified +`Type::Symbol` as an immediate, so a `Symbol` local got no shadow slot at all. Run it by hand when you touch an `alloca_entry` site: ```bash @@ -269,9 +273,27 @@ the exact thing this file exists to prevent. which means it cannot turn a merge red — hazard 2, and the reason the #7211 hits sat unread on `main` from #7198 onward while the job was visibly failing. -With the allowlist the job is green on `main`, so the remaining step is for a -repo admin to add `gc-root-dominance` to the required contexts. A workflow -cannot do this to itself. Until it is done, this is documentation. +Both of the conditions #7198 named are now met: + +- the bind-anchored dominance check is green on `main` with an **empty** + allowlist (the #7211 entries were deleted when that predicate was fixed); +- `--unrooted-allocas --moving-only` reads **0** and is a step in the job + (#7236). That was the outstanding one: it was 98 before #7235, 2 after, and 0 + once `Type::Symbol` stopped being classified as an immediate. + +So the remaining step is for a **repo admin** to add `gc-root-dominance` to +branch protection's required contexts: + +``` +Settings → Branches → main → Require status checks to pass + → add: gc-root-dominance +``` + +A workflow cannot do this to itself, and neither can a PR. Until it is done, +this is documentation. Per CLAUDE.md's corollary, promote it **after** the job's +first green run on `main` with the `--unrooted-allocas` step included — a gate +that has never been green in its current shape blocks every open PR the day it +becomes required. ## Rules of thumb From 7961314fe0a97bd42bee6d8d6dfb354d7c62999b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sun, 2 Aug 2026 10:51:12 +0200 Subject: [PATCH 4/5] docs(changelog): fragment for #7236 (PR #7243) --- changelog.d/7243-symbol-local-shadow-slot.md | 59 ++++++++++++++++++++ 1 file changed, 59 insertions(+) create mode 100644 changelog.d/7243-symbol-local-shadow-slot.md diff --git a/changelog.d/7243-symbol-local-shadow-slot.md b/changelog.d/7243-symbol-local-shadow-slot.md new file mode 100644 index 0000000000..e0ee8b4da8 --- /dev/null +++ b/changelog.d/7243-symbol-local-shadow-slot.md @@ -0,0 +1,59 @@ +Fixed `collectors/pointer_locals.rs` classifying **`Type::Symbol` as a +non-pointer**, so a `Symbol`-typed local got no shadow-stack slot and lived in a +plain `alloca` — invisible to the precise root walk (#7236). `alloc_symbol` is +`gc_malloc(size_of::(), GC_TYPE_STRING)` and `js_symbol_new` +returns it `POINTER_TAG`-boxed; nothing else holds a fresh symbol, so the malloc +sweep inside the copying minor frees one that is still live. + +**The drift had already happened, one variant wide, inside one crate.** +`typed_shape::type_is_pointer_bearing` — an exhaustive match, and the function +that lays out the GC's own inline-field pointer masks — already answered `true` +for `Symbol` while `is_definitely_non_pointer_type` answered "non-pointer", so a +`Symbol` local was simultaneously not-a-pointer (no slot) and pointer-bearing +(scanned as one). That is verbatim what the doc comment over +`is_definitely_non_pointer_type` predicted would cost a use-after-move. There +was a third copy in `expr/shadow_slot.rs` carrying the same entry — a separate +hazard, since a `true` there suppresses `temp_root`'s protection of a symbol +operand held across an allocating call — and a fourth in +`lower_call/closure_analysis.rs` that correctly did not. All three now route to +one exhaustive definition, so a new `Type` variant is a compile error rather +than a silent "not a pointer". The full 21-variant audit found `Symbol` to be +the only misclassification; the other six non-pointers are NaN-boxed immediates +with no allocator. + +**The failure is a premature FREE, not a stale address**, and the correction +matters because it dictates the witness. `gc_malloc` is the SYSTEM allocator +with a `GcHeader` in front, not an arena allocation, so the copying minor cannot +relocate a symbol — under #7235's taxonomy a `Symbol` local is RECLAIMABLE and +not MOVABLE (#7230's class, not #7019's). `SYMBOL_POINTERS` does not save it +either: `scan_symbol_pointer_metadata_roots_mut` uses +`visit_metadata_usize_slot`, which rewrites without marking, exactly as +`alloc_symbol`'s own comment says ("kept alive … or **not at all**"). Two +consequences are baked into the new witness: the pressure must be **symbols**, +because `copied_minor_malloc_sweep_due` gates the malloc sweep on a +`MallocCount` trigger while object churn reaches the *arena* trigger (object +churn reproduced 1 failure in 80 probes, and 0 on four of five repeats of +another shape); and identity probes report nothing, because `js_symbol_equals` +falls back to comparing `id` off the freed header and `js_is_symbol` to reading +`magic`. + +New witness `test-files/test_gap_gc_symbol_local_rooting.ts`, registered in +`test-parity/gc_repsel_corpus.txt`: `A 30 B 20` at `f8f1e7188` on the +`loop_polls` arm (3/3 identical) **and on the shipped default** (2/2), `A 0 B 0` +after, byte-exact against node 26.5.1. + +`gc_root_dominance_check.py --unrooted-allocas --moving-only` over a freshly +generated 118-source / 136-module corpus: **4 → 0** (the two #7236 hits in +`test_gap_class_forward_capture_6523` plus the new witness's two). That was 98 +before #7235 and 2 after, and 0 is the condition #7198 named for promoting +`gc-root-dominance` to a required context. **The mode is now a step in that +job** rather than a number living in issue bodies — verified able to fail (exit +1 at `f8f1e7188`, exit 0 after) and sharing the corpus, exemptions, allowlist +and liveness floors with the existing dominance step. The promotion itself +remains a repo-admin action; `docs/src/internals/gc-rooting-invariant.md` now +carries the exact steps. + +The representation-selection promotion census +(`compiler_output_regression.py census --gate`) is **byte-identical** across all +18 workloads before and after, so no promotion floor moved and no ratchet was +taken. From ae82386e1bc401e1c16a8bc55b99e76a49aa1816 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sun, 2 Aug 2026 12:07:53 +0200 Subject: [PATCH 5/5] test(gc): narrow the Symbol witness to the defect #7236 fixes The first shape mixed two defects. A symbol WITH a description stayed red 2/20 after the fix on every PERRY_GC_INCREMENTAL=0 + PERRY_CONSERVATIVE_STACK_SCAN=off arm, because GC_TYPE_STRING is a pointer-free Leaf and a symbol's description StringHeader is never traced -- filed as #7246. A descriptionless Symbol() has no such pointer, so what the file measures is exactly the lifetime of the symbol object, which is what a shadow slot decides. --arms all --pressure 8: base 21/21 cells FAIL (0/21 byte-exact, 5 exit=1), with the fix 0 FAIL and 21/21 byte-exact (PASS=17 UNVER=4). --- .../test_gap_gc_symbol_local_rooting.ts | 115 ++++++++---------- test-parity/gc_repsel_corpus.txt | 25 ++-- 2 files changed, 69 insertions(+), 71 deletions(-) diff --git a/test-files/test_gap_gc_symbol_local_rooting.ts b/test-files/test_gap_gc_symbol_local_rooting.ts index 2945ccc46c..e03b9857b3 100644 --- a/test-files/test_gap_gc_symbol_local_rooting.ts +++ b/test-files/test_gap_gc_symbol_local_rooting.ts @@ -2,52 +2,65 @@ // shadow-stack slot like any other heap reference. // // `collectors/pointer_locals.rs` listed `Type::Symbol` in -// `is_definitely_non_pointer_type`, so `const s = Symbol("w")` got NO +// `is_definitely_non_pointer_type`, so `const s = Symbol("k")` got NO // shadow-stack slot and lived in a plain `alloca` for its whole scope — // invisible to the precise root walk. `typed_shape::type_is_pointer_bearing`, // which lays out the GC's own field masks, said `Symbol` IS a pointer, so the // two copies of one question had drifted by exactly one variant. // -// ★ WHAT ACTUALLY GOES WRONG IS A PREMATURE FREE, NOT A STALE ADDRESS. -// `alloc_symbol` is `gc_malloc(size_of::(), GC_TYPE_STRING)`, and -// `gc_malloc` (gc/malloc.rs) is the SYSTEM allocator with a `GcHeader` in front -// — not an arena allocation. So a fresh symbol is not in the nursery and the -// copying minor cannot relocate it; what it can do is `dealloc` it. Under the -// #7235 taxonomy the local is RECLAIMABLE-but-not-MOVABLE, the #7230 class. -// `alloc_symbol`'s own comment concedes the liveness half — a fresh symbol is -// kept alive "through the SYMBOL_REGISTRY (for registered symbols) or NOT AT -// ALL" — and `SYMBOL_POINTERS` does not save it either: -// `scan_symbol_pointer_metadata_roots_mut` visits it with -// `visit_metadata_usize_slot`, which rewrites a recorded address WITHOUT -// marking. With no shadow slot the unrooted `alloca` is the only reference in -// the world, the collector cannot see it, and `sweep_malloc_objects` frees a -// live symbol. +// ★ THE FAILURE IS A PREMATURE FREE, NOT A STALE ADDRESS. `alloc_symbol` is +// `gc_malloc(size_of::(), GC_TYPE_STRING)`, and `gc_malloc` +// (gc/malloc.rs) is the SYSTEM allocator with a `GcHeader` in front — not an +// arena allocation. So a fresh symbol is not in the nursery and the copying +// minor cannot relocate it; what it can do is `dealloc` it, via +// `sweep_malloc_objects`. Under #7235's taxonomy the local is +// RECLAIMABLE-but-not-MOVABLE, the #7230 class. Nothing else holds a fresh +// symbol — `alloc_symbol`'s own comment says it is kept alive "through the +// SYMBOL_REGISTRY … or NOT AT ALL", and `scan_symbol_pointer_metadata_roots_mut` +// visits `SYMBOL_POINTERS` with `visit_metadata_usize_slot`, which rewrites a +// recorded address WITHOUT marking. With no shadow slot the `alloca` is the +// only reference in the world and a live symbol is freed. // // ★ WHY THE PRESSURE IS SYMBOLS AND NOT OBJECTS. The malloc sweep inside the // copying minor is gated: `copied_minor_malloc_sweep_due` is true only for a // `GcTriggerKind::MallocCount` collection or once `malloc_object_count()` -// passes its trigger. Driving pressure with object/array churn reaches the -// ARENA trigger instead, so the malloc sweep is only occasionally due and the -// defect reproduces intermittently — measured at 1 failure in 80 probes, and at -// 0 on four of five repeats of another shape. Allocating symbols is what makes -// the malloc-count trigger the one that fires, and with it the reproduction -// deterministic. It also makes the reuse deterministic: the freed block is -// exactly the size class the next `Symbol()` asks for, so the recycled bytes -// are what the stale local reads back. +// passes its trigger. Object/array churn reaches the ARENA trigger instead, so +// the defect reproduces intermittently — measured at 1 failure in 80 probes on +// one object-churn shape, and at 0 on four of five repeats of another after a +// first run showed 72. Symbol churn makes the malloc-count trigger the one that +// fires, and makes the reuse deterministic too: the freed block is exactly the +// size class the next `Symbol()` asks for, so the recycled bytes are one of +// `symChurn`'s throwaway `Symbol("t")`s and the stale local reads ITS +// description back. // -// Measured on `f8f1e7188` (before the fix), `--release`: `A 34` on the -// `loop_polls` arm, identical on three consecutive runs, and `A 34` on the -// SHIPPED DEFAULT with no GC env at all. Oracle and fixed build both print -// `A 0`. This one does not need a moving arm to bite — the conservative stack -// scan is what has been hiding it, and it stops hiding it as soon as the -// collection is malloc-count driven. -// -// ★ WHY THE PROBES ARE NOT IDENTITY COMPARISONS. `js_symbol_equals` +// ★ WHY THE PROBE IS NOT AN IDENTITY COMPARISON. `js_symbol_equals` // (symbol/constructors.rs) falls back from a bits comparison to dereferencing // both headers and comparing `id`, and `js_is_symbol` falls back to reading -// `magic` — both answer correctly off a freed-but-not-yet-recycled header, so -// an identity probe reports nothing. What a reaped symbol cannot survive is -// having its own description read back after its storage is reused. +// `magic` — and the block that recycles a freed symbol is ANOTHER SYMBOL, so +// both answer "yes, a symbol" and `typeof` never moves (measured: `typeof` +// 0/20 at base). What a reaped symbol cannot survive is being used as a +// computed KEY and then having that key read back and identified, which is +// #7236's own named shape. +// +// ★ WHY THE PROBE SYMBOL HAS NO DESCRIPTION. `Symbol()` rather than +// `Symbol("k")`, on purpose, to keep this file gating ONE defect. +// `GC_TYPE_STRING`'s type-info entry is `pointer_free: true` / +// `GcRewriteDescriptorKind::Leaf` — `alloc_symbol` says so in as many words +// ("the GC won't try to scan internal pointers") — so a symbol's +// `description` `StringHeader*` is never traced or rewritten, and a symbol +// that is itself perfectly rooted can still have its description reaped out +// from under it. That is a SEPARATE, runtime-side defect (#7246) +// that this codegen change does not touch, and with `Symbol("k")` it left this +// file red 2/20 on +// the `PERRY_GC_INCREMENTAL=0 PERRY_CONSERVATIVE_STACK_SCAN=off` arms even +// after the fix. A descriptionless symbol has no such pointer, so what remains +// is exactly the lifetime of the symbol object — which is what a shadow slot +// decides. The recycled block is one of `symChurn`'s `Symbol("t")`s, so the +// stale local reports `Symbol(t)` where the oracle says `Symbol()`. +// +// Measured at base (`f8f1e7188`), release, oracle node 26.5.1 (`B 0`): +// `B 34` of a possible 80, 3/3 identical, on `loop_polls` and on the shipped +// default; `B 0` after the fix, 3/3. // // The observable is byte-for-byte identical to `node --experimental-strip-types`. @@ -62,36 +75,10 @@ function symChurn(n: number): number { return k; } -// A: the symbol is referenced by NOTHING but the local across the collection, -// and is then asked what it is. -function heldAcrossCollection(): number { - let bad = 0; - for (let r = 0; r < 20; r++) { - const s = Symbol("w"); - if (symChurn(50000) !== 50000) { - bad++; - } - if (typeof s !== "symbol") { - bad++; - } - if (String(s) !== "Symbol(w)") { - bad++; - } - if (s.description !== "w") { - bad++; - } - } - return bad; -} - -// B: the same local, then used as a COMPUTED KEY — the issue's other named -// shape. The store must land on a symbol key whose description round-trips, so -// a local whose storage was recycled into one of `symChurn`'s throwaway -// `Symbol("t")`s is caught by the description rather than by the count. function usedAsComputedKey(): number { let bad = 0; for (let r = 0; r < 20; r++) { - const s = Symbol("k"); + const s = Symbol(); if (symChurn(50000) !== 50000) { bad++; } @@ -104,12 +91,14 @@ function usedAsComputedKey(): number { if (keys.length !== 1) { bad++; } - if (String(keys[0]) !== "Symbol(k)") { + if (String(keys[0]) !== "Symbol()") { + bad++; + } + if (keys[0].description !== undefined) { bad++; } } return bad; } -console.log("A", heldAcrossCollection()); console.log("B", usedAsComputedKey()); diff --git a/test-parity/gc_repsel_corpus.txt b/test-parity/gc_repsel_corpus.txt index 5f824fe107..9e0ffe56c6 100644 --- a/test-parity/gc_repsel_corpus.txt +++ b/test-parity/gc_repsel_corpus.txt @@ -394,8 +394,8 @@ test_gap_gc_process_env_cache_rooting # --- #7236: a Symbol-typed local had no shadow slot at all ------------------ # Not a representation file, so registered explicitly per the header rule. # `collectors/pointer_locals.rs` classified `Type::Symbol` as a NON-pointer, so -# `const s = Symbol("w")` got no shadow-stack slot and lived in a plain -# `alloca` — the last violation `gc_root_dominance_check.py --unrooted-allocas +# `const s = Symbol()` got no shadow-stack slot and lived in a plain `alloca` — +# the last violation `gc_root_dominance_check.py --unrooted-allocas # --moving-only` reported on the #7235 corpus, and the one thing standing # between `gc-root-dominance` and promotion to a required context (#7198). # @@ -417,11 +417,20 @@ test_gap_gc_process_env_cache_rooting # makes the malloc-count trigger the one that fires AND makes the freed block # the exact size class the next `Symbol()` reuses. # -# Measured at base (`f8f1e7188`), `--release`, oracle node 26.5.1 (`A 0 B 0`): -# `A 30 B 20` on `loop_polls`, 3/3 identical, and `A 30 B 20` on the SHIPPED -# DEFAULT with no GC env at all, 2/2. `A 0 B 0` after the fix on both, 3/3. +# ★ THE PROBE SYMBOL DELIBERATELY HAS NO DESCRIPTION, so this file gates ONE +# defect. `GC_TYPE_STRING` is `pointer_free: true` / `Leaf`, so a symbol's +# `description` StringHeader is never traced or rewritten and can be reaped out +# from under a perfectly rooted symbol — a SEPARATE runtime-side defect (#7246) +# that #7236 does not touch. With `Symbol("k")` this file stayed red 2/20 on the +# `PERRY_GC_INCREMENTAL=0 PERRY_CONSERVATIVE_STACK_SCAN=off` arms AFTER the +# fix; with `Symbol()` there is no such pointer and what remains is exactly the +# lifetime of the symbol object. If those two arms ever go red here again, +# suspect the description defect before suspecting the classification. # -# It therefore does NOT need a `requires=move` arm — the conservative stack -# scan is what was hiding it, and a malloc-count-driven collection stops it -# hiding. Expect PASS rather than UNVER wherever the arm collects at all. +# Measured with `--arms all --pressure 8`, oracle node 26.5.1 (`B 0`): +# f8f1e7188 (base) 21/21 cells FAIL, 0/21 byte-exact (5 of them exit=1) +# with the fix 0 FAIL, 21/21 byte-exact, PASS=17 UNVER=4 +# It does NOT need a `requires=move` arm: the conservative stack scan is what +# was hiding it, and a malloc-count-driven collection stops it hiding, so it is +# red on the SHIPPED DEFAULT at base too. test_gap_gc_symbol_local_rooting