diff --git a/changelog.d/7007-scalar-replaced-local-rooting.md b/changelog.d/7007-scalar-replaced-local-rooting.md new file mode 100644 index 0000000000..d053b8c08a --- /dev/null +++ b/changelog.d/7007-scalar-replaced-local-rooting.md @@ -0,0 +1,16 @@ +### Fixed + +- **GC: a heap value in a scalar-replaced object field or array element is now a precise root (#6968).** + + Scalar replacement deletes the object and keeps one entry-block alloca per field/element. Those allocas belong to no HIR local, so `collect_pointer_typed_locals` — which assigns shadow slots by walking `Stmt::Let` — never saw them and nothing bound them. With precise roots only (`PERRY_CONSERVATIVE_STACK_SCAN=off`) a collection landing between the store and the read swept the value out from under the alloca: `{ const o = { a: fresh(0), b: churn(N) }; console.log(o.a, o.b) }` printed an empty `o.a`, or a recycled one, with no crash and no diagnostic. The array form (`const a = [fresh(0), churn(N)]`) was identical. + + #6951/#6972's object-literal rooting could not reach this shape: that path roots the object *handle*, and scalar replacement leaves no handle. The object local *does* get a shadow slot reserved — it is pointer-typed — but lowering only ever **cleared** it. + + Each replaced slot is now shadow-bound at the store, the same treatment `emit_shadow_slot_update_for_expr` gives an ordinary pointer-typed local, at object-literal fields, array-literal elements, scalar-replaced `split()` parts, anonymous-shape constructor arguments, and both `expr::property_set` arms. Two properties keep it cheap: + + - **The frame grows on demand.** `LlFunction::reserve_shadow_slot` rewrites the slot-count operand of the already-emitted `js_shadow_frame_push` (creating the frame if the pre-lowering count was zero), because the escape facts that decide scalar replacement are not computed until after the frame is sized. + - **Reservation is lazy and gated on the lowering, never a declared type** (#6997): the predicate is `expr_is_known_non_pointer_shadow_value`. A literal whose fields are all numbers takes no slot, emits no call, and does not grow the frame. Measured: `{ x: i & 1023, y: (i>>3) & 1023 }` over 40 M iterations is unchanged (313–355 ms → 319–353 ms), as is the array twin. A pointer-capable field store costs **~2.6 ns** (118–121 ms → 144–148 ms over 10 M iterations) — against 4682–4997 ms for the heap object scalar replacement removes, so the optimization still wins by ~32× after paying for the root. + + Corpus effect, `scripts/gc_repsel_matrix.sh --pressure 8` on the evacuating precise-roots arm: `test_gap_repsel_gc_stress` goes FAIL → PASS (deterministic over 3 repeats, `moved=1 230 900` in both arms), and no cell regresses. Ten of the thirteen files #6981 lists compile to **byte-identical LLVM IR** with and without this change, so #6968 is provably not their cause; they belong to the argument-passing families (#6969/#6970/#6971) and their neighbours. + + New coverage: `test-files/test_gap_repsel_scalar_replaced_locals.ts` (registered in `test-parity/gc_repsel_corpus.txt`; red on `cons_scan_off` — a PR arm — before this change, green after) and `crates/perry-codegen/tests/scalar_replaced_slot_roots.rs` (5 codegen-contract tests, teeth verified in both directions, including the gate tests against a deliberately coarsened gate). diff --git a/crates/perry-codegen/src/codegen/closure.rs b/crates/perry-codegen/src/codegen/closure.rs index d9af39655e..23604f2ea0 100644 --- a/crates/perry-codegen/src/codegen/closure.rs +++ b/crates/perry-codegen/src/codegen/closure.rs @@ -903,6 +903,7 @@ pub(super) fn compile_closure( scalar_replaced_arrays: std::collections::HashMap::new(), scalar_replaced_split_part_lengths: std::collections::HashMap::new(), scalar_replaced_uppercase_sources: std::collections::HashMap::new(), + scalar_slot_shadow_slots: std::collections::HashMap::new(), scalar_ctor_target: Vec::new(), non_escaping_news: native_facts.non_escaping_news().clone(), non_escaping_new_used_fields: native_facts.non_escaping_new_used_fields().clone(), diff --git a/crates/perry-codegen/src/codegen/entry.rs b/crates/perry-codegen/src/codegen/entry.rs index a2a63b3094..0e90cb6a6b 100644 --- a/crates/perry-codegen/src/codegen/entry.rs +++ b/crates/perry-codegen/src/codegen/entry.rs @@ -793,6 +793,7 @@ pub(super) fn compile_module_entry( scalar_replaced_arrays: std::collections::HashMap::new(), scalar_replaced_split_part_lengths: std::collections::HashMap::new(), scalar_replaced_uppercase_sources: std::collections::HashMap::new(), + scalar_slot_shadow_slots: std::collections::HashMap::new(), scalar_ctor_target: Vec::new(), non_escaping_news: main_native_facts.non_escaping_news().clone(), non_escaping_new_used_fields: main_native_facts.non_escaping_new_used_fields().clone(), @@ -1412,6 +1413,7 @@ pub(super) fn compile_module_entry( scalar_replaced_arrays: std::collections::HashMap::new(), scalar_replaced_split_part_lengths: std::collections::HashMap::new(), scalar_replaced_uppercase_sources: std::collections::HashMap::new(), + scalar_slot_shadow_slots: std::collections::HashMap::new(), scalar_ctor_target: Vec::new(), non_escaping_news: init_native_facts.non_escaping_news().clone(), non_escaping_new_used_fields: init_native_facts.non_escaping_new_used_fields().clone(), diff --git a/crates/perry-codegen/src/codegen/function.rs b/crates/perry-codegen/src/codegen/function.rs index b0dab1814c..c72cd1ad8a 100644 --- a/crates/perry-codegen/src/codegen/function.rs +++ b/crates/perry-codegen/src/codegen/function.rs @@ -775,6 +775,7 @@ pub(super) fn compile_function( scalar_replaced_arrays: std::collections::HashMap::new(), scalar_replaced_split_part_lengths: std::collections::HashMap::new(), scalar_replaced_uppercase_sources: std::collections::HashMap::new(), + scalar_slot_shadow_slots: std::collections::HashMap::new(), scalar_ctor_target: Vec::new(), non_escaping_news: native_facts.non_escaping_news().clone(), non_escaping_new_used_fields: native_facts.non_escaping_new_used_fields().clone(), diff --git a/crates/perry-codegen/src/codegen/method.rs b/crates/perry-codegen/src/codegen/method.rs index f46cc86e79..69672fb5ac 100644 --- a/crates/perry-codegen/src/codegen/method.rs +++ b/crates/perry-codegen/src/codegen/method.rs @@ -535,6 +535,7 @@ pub(super) fn compile_method( scalar_replaced_arrays: std::collections::HashMap::new(), scalar_replaced_split_part_lengths: std::collections::HashMap::new(), scalar_replaced_uppercase_sources: std::collections::HashMap::new(), + scalar_slot_shadow_slots: std::collections::HashMap::new(), scalar_ctor_target: Vec::new(), non_escaping_news: native_facts.non_escaping_news().clone(), non_escaping_new_used_fields: native_facts.non_escaping_new_used_fields().clone(), @@ -1577,6 +1578,7 @@ pub(super) fn compile_static_method( scalar_replaced_arrays: std::collections::HashMap::new(), scalar_replaced_split_part_lengths: std::collections::HashMap::new(), scalar_replaced_uppercase_sources: std::collections::HashMap::new(), + scalar_slot_shadow_slots: std::collections::HashMap::new(), scalar_ctor_target: Vec::new(), non_escaping_news: native_facts.non_escaping_news().clone(), non_escaping_new_used_fields: native_facts.non_escaping_new_used_fields().clone(), diff --git a/crates/perry-codegen/src/expr/mod.rs b/crates/perry-codegen/src/expr/mod.rs index 9d01f95294..0538606d07 100644 --- a/crates/perry-codegen/src/expr/mod.rs +++ b/crates/perry-codegen/src/expr/mod.rs @@ -131,6 +131,7 @@ pub(crate) use write_barrier::{ // under 2000 lines. Inherent methods (`record_value`) need no re-export. mod dispatch; mod record_value; +mod scalar_slot_root; mod shadow_slot; mod slot_rep; pub(crate) mod temp_root; @@ -142,6 +143,9 @@ pub(crate) use slot_rep::{ }; pub(crate) use dispatch::{lower_expr, lower_math_operand}; +pub(crate) use scalar_slot_root::{ + root_scalar_replaced_slot, root_scalar_replaced_slot_unconditional, +}; pub(crate) use shadow_slot::{ emit_shadow_slot_bind_for_local, emit_shadow_slot_clear, emit_shadow_slot_update_for_expr, enable_persistent_shadow_slot_for_array_alias, expr_is_known_non_pointer_shadow_value, @@ -1006,6 +1010,14 @@ pub(crate) struct FnCtx<'a> { /// original receiver. Only fused string operations may consume it. pub scalar_replaced_uppercase_sources: std::collections::HashMap, + /// Shadow-frame slot reserved for a scalar-replacement alloca, keyed by + /// the alloca's SSA name (#6968). These allocas belong to no HIR local, + /// so `collect_pointer_typed_locals` cannot see them and the frame is + /// grown on demand — see `expr::scalar_slot_root`. Populated the first + /// time a possibly-pointer value is stored into the alloca; a field that + /// only ever holds numbers never appears here and costs nothing. + pub scalar_slot_shadow_slots: std::collections::HashMap, + /// Non-escaping array literals identified by escape analysis. Maps /// local_id → length. Used by the Stmt::Let lowering to intercept /// `let arr = [a, b, c]` and emit per-index allocas instead of a diff --git a/crates/perry-codegen/src/expr/property_set.rs b/crates/perry-codegen/src/expr/property_set.rs index db99969360..4fe3f85a86 100644 --- a/crates/perry-codegen/src/expr/property_set.rs +++ b/crates/perry-codegen/src/expr/property_set.rs @@ -219,6 +219,14 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { val_double.clone() }; ctx.block().store(DOUBLE, &stored_value, &slot); + // #6968: bind the field alloca as a precise GC root, the + // same treatment `emit_shadow_slot_update_for_expr` gives + // an ordinary pointer-typed local. Skipped for a + // `numeric_store`, whose stored bits are a canonicalized + // raw `f64` by construction. + if !numeric_store { + crate::expr::root_scalar_replaced_slot(ctx, &slot, value); + } // String-alias fix (mirror of `let y = x` in stmt/let_stmt.rs): // a string-typed local stored into a scalar-replaced field's // alloca slot aliases the same heap buffer. The runtime @@ -301,6 +309,12 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { val_double.clone() }; ctx.block().store(DOUBLE, &stored_value, &slot); + // #6968: see the `ScalarObjectFieldSet` path above — + // an inlined constructor's `this.f = …` writes the + // same kind of unrooted per-field alloca. + if !numeric_store { + crate::expr::root_scalar_replaced_slot(ctx, &slot, value); + } // String-alias fix: see the ScalarObjectFieldSet path // above. `this.field = s` into a scalar-replaced ctor slot // aliases the string buffer; mark it shared so a later diff --git a/crates/perry-codegen/src/expr/scalar_slot_root.rs b/crates/perry-codegen/src/expr/scalar_slot_root.rs new file mode 100644 index 0000000000..ebbf0fd62f --- /dev/null +++ b/crates/perry-codegen/src/expr/scalar_slot_root.rs @@ -0,0 +1,99 @@ +//! GC rooting for scalar-replaced object fields and array elements (#6968). +//! +//! # The hole this closes +//! +//! Scalar replacement turns `const o = { a: fresh(), b: n }` into one +//! entry-block alloca per field and deletes the object. Those allocas belong +//! to no HIR local, so `collect_pointer_typed_locals` — which assigns shadow +//! slots by walking `Stmt::Let` — never sees them, and nothing ever calls +//! `js_shadow_slot_bind` for them: +//! +//! ```llvm +//! %r13 = call double @perry_fn_m__fresh(double 0.0) +//! store double %r13, ptr %r10 ; o.a — a bare, unrooted alloca +//! %r16 = call double @perry_fn_m__churn(double %r15) ; collects; %r10 swept +//! ``` +//! +//! The object *local* does get a slot reserved (it is pointer-typed), but +//! lowering only ever clears it — there is no object handle to bind, which is +//! why #6951/#6972's object-literal rooting does not reach this shape. +//! +//! Until #6977 this was invisible: `gc_check_trigger` forces a conservative +//! native-stack scan, which finds the alloca. With precise roots only +//! (`PERRY_CONSERVATIVE_STACK_SCAN=off`) the value is swept out from under +//! the alloca and the program reads freed memory. +//! +//! # What is emitted +//! +//! At each store into such an alloca whose value may be a heap reference: +//! reserve one shadow-frame slot for that alloca (once) and bind it, exactly +//! as `expr::emit_shadow_slot_update_for_expr` does for an ordinary +//! pointer-typed local. `js_shadow_slot_bind` records `slot_ptrs[slot] = +//! alloca`, so both a mark-sweep root walk and an evacuating minor's +//! rewrite pass reach the real alloca rather than a stale mirror. +//! +//! The bind is not repeated per alloca-per-store *shape*, only per store +//! *site*: the alloca is entry-hoisted and never moves, so one bind covers +//! the rest of the frame's life. Re-binding at a later store to the same +//! field is what re-runs the incremental-mark root barrier, which is +//! required for the same reason an ordinary local's re-assignment re-binds. +//! +//! # The gate +//! +//! Emission is skipped when the stored value cannot be a heap reference. +//! That decision is made from the **lowering**, never the declared type +//! (#6997): the predicate is `expr_is_known_non_pointer_shadow_value`, the +//! very one that decides whether an ordinary pointer local's slot is bound +//! or cleared. A field whose values are all numbers therefore costs nothing +//! — no slot, no call, no growth of the frame. + +use super::*; + +use perry_hir::Expr; + +use crate::types::{I32, PTR}; + +/// Root the scalar-replacement alloca `slot` against the value expression +/// that was just stored into it. +/// +/// Call *after* the `store` — `js_shadow_slot_bind` reads the alloca to seed +/// the shadow mirror and to run the root write barrier, so the new value has +/// to be in place. Callers that store a canonicalized raw `f64` (the +/// `numeric_store` arm of `expr::property_set`) must not call this at all: +/// those bits are a plain double by construction, and the shared root-word +/// decoder rejects them, but reserving a slot for them would be pure waste. +pub(crate) fn root_scalar_replaced_slot(ctx: &mut FnCtx<'_>, slot: &str, value: &Expr) { + if expr_is_known_non_pointer_shadow_value(ctx, value) { + return; + } + bind_scalar_replaced_slot(ctx, slot); +} + +/// Root a scalar-replacement alloca whose stored value has no HIR expression +/// to gate on because codegen synthesized it. +/// +/// Used by the scalar-replaced `String.prototype.split` arm, whose element +/// slots receive `js_string_split_part_value` results — heap strings with +/// nothing else referring to them. +pub(crate) fn root_scalar_replaced_slot_unconditional(ctx: &mut FnCtx<'_>, slot: &str) { + bind_scalar_replaced_slot(ctx, slot); +} + +fn bind_scalar_replaced_slot(ctx: &mut FnCtx<'_>, slot: &str) { + let slot_idx = match ctx.scalar_slot_shadow_slots.get(slot).copied() { + Some(idx) => idx, + None => { + // `None` means shadow-stack emission is off for this build; the + // caller must not emit slot traffic either. + let Some(idx) = ctx.func.reserve_shadow_slot() else { + return; + }; + ctx.scalar_slot_shadow_slots.insert(slot.to_string(), idx); + idx + } + }; + ctx.block().call_void( + "js_shadow_slot_bind", + &[(I32, &slot_idx.to_string()), (PTR, slot)], + ); +} diff --git a/crates/perry-codegen/src/function.rs b/crates/perry-codegen/src/function.rs index e0da8c6373..b2c7509a87 100644 --- a/crates/perry-codegen/src/function.rs +++ b/crates/perry-codegen/src/function.rs @@ -103,12 +103,53 @@ pub struct LlFunction { /// land wiring incrementally (e.g. just `main`) before /// flipping the default across every user function. shadow_frame_slot: Option, + /// Whether shadow-frame emission was requested for this function at all + /// (i.e. `enable_shadow_frame` / `enable_post_init_shadow_frame` ran). + /// + /// Distinct from `shadow_frame_slot.is_some()`: a function whose locals + /// were all proven non-pointer requests a frame but gets none, because a + /// zero-slot frame is pure overhead. `reserve_shadow_slot` needs to tell + /// that case (grow it — there is now something to root) apart from "the + /// shadow stack is switched off for this build" (do nothing). + shadow_frame_requested: bool, + /// Which region the frame push belongs in — `entry_post_init_setup` when + /// `enable_post_init_shadow_frame` was used, `entry_allocas` otherwise. + /// Remembered so a lazily-created frame lands where the eager one would. + shadow_frame_post_init_region: bool, + /// Where the emitted `js_shadow_frame_push` line lives, so its slot count + /// can be rewritten when lowering discovers a root the pre-lowering + /// pointer analysis could not see (#6968: scalar-replaced object fields + /// and array elements, which have no HIR local of their own). + shadow_frame_push: Option, + /// Slot count currently baked into that push line. + shadow_frame_slot_count: u32, /// Runtime hooks emitted immediately before each non-pointer `ret`. /// Entry/module-init functions use this for process-level diagnostics /// that must run regardless of which block reaches the normal epilogue. pre_return_void_calls: Vec, } +/// Render the frame-push instruction. Kept in one place so the eager +/// emission and the later count rewrite cannot drift. +fn shadow_frame_push_line(handle_reg: &str, slot_count: u32) -> String { + format!( + " {} = call i64 @js_shadow_frame_push(i32 {})", + handle_reg, slot_count + ) +} + +/// Location of a function's `js_shadow_frame_push` line, so its slot-count +/// operand can be rewritten in place after the fact. +struct ShadowFramePush { + /// `true` when the line lives in `entry_post_init_setup` rather than + /// `entry_allocas`. + post_init: bool, + /// Index of the line within that region. + line_idx: usize, + /// SSA register the push result is assigned to, needed to re-render. + handle_reg: String, +} + impl LlFunction { pub fn new( name: impl Into, @@ -140,6 +181,10 @@ impl LlFunction { entry_post_init_setup: Vec::new(), entry_init_boundary: None, shadow_frame_slot: None, + shadow_frame_requested: false, + shadow_frame_post_init_region: false, + shadow_frame_push: None, + shadow_frame_slot_count: 0, pre_return_void_calls: Vec::new(), } } @@ -180,30 +225,85 @@ impl LlFunction { } fn enable_shadow_frame_inner(&mut self, slot_count: u32, post_init: bool) { - use crate::types::I64; if self.shadow_frame_slot.is_some() { return; } + // Record the request (and its region) even when no frame is emitted: + // `reserve_shadow_slot` uses it to tell "nothing to root yet" from + // "shadow stack disabled", and to place a lazily-created push line in + // the same region `enable_*_shadow_frame` would have used. + self.shadow_frame_requested = true; + self.shadow_frame_post_init_region = post_init; if slot_count == 0 { return; } + self.emit_shadow_frame_push(slot_count, post_init); + } + + fn emit_shadow_frame_push(&mut self, slot_count: u32, post_init: bool) { + use crate::types::I64; let handle_slot = self.alloca_entry(I64); let handle_reg = format!("%r{}", self.reg_counter.next()); - let push_line = format!( - " {} = call i64 @js_shadow_frame_push(i32 {})", - handle_reg, slot_count - ); + let push_line = shadow_frame_push_line(&handle_reg, slot_count); let store_line = format!(" store i64 {}, ptr {}", handle_reg, handle_slot); - if post_init { - self.entry_post_init_setup.push(push_line); - self.entry_post_init_setup.push(store_line); + let region = if post_init { + &mut self.entry_post_init_setup } else { - self.entry_allocas.push(push_line); - self.entry_allocas.push(store_line); - } + &mut self.entry_allocas + }; + let line_idx = region.len(); + region.push(push_line); + region.push(store_line); + self.shadow_frame_push = Some(ShadowFramePush { + post_init, + line_idx, + handle_reg, + }); + self.shadow_frame_slot_count = slot_count; self.shadow_frame_slot = Some(handle_slot); } + /// Reserve one more GC-root slot in this function's shadow frame and + /// return its index, rewriting the already-emitted + /// `js_shadow_frame_push` count in place. + /// + /// `collect_pointer_typed_locals` sizes the frame before lowering, from + /// the HIR locals it can see. Scalar replacement (#6968) creates storage + /// that has no HIR local of its own — one entry-block alloca per object + /// field / array element — so a heap value living in one of those is + /// invisible to the pre-lowering count. Rather than teach the collector + /// to predict every scalar-replacement decision (they are taken later, on + /// conditions the collector does not evaluate), the frame grows on demand + /// at the store site that actually needs the root. + /// + /// Returns `None` when shadow-stack emission is switched off for this + /// build, in which case the caller must not emit slot traffic either. + /// When the frame was skipped as empty, one is created here. + pub fn reserve_shadow_slot(&mut self) -> Option { + if !self.shadow_frame_requested { + return None; + } + if self.shadow_frame_push.is_none() { + let post_init = self.shadow_frame_post_init_region; + self.emit_shadow_frame_push(0, post_init); + } + let idx = self.shadow_frame_slot_count; + self.shadow_frame_slot_count += 1; + let count = self.shadow_frame_slot_count; + let Some(push) = &self.shadow_frame_push else { + return None; + }; + let (post_init, line_idx) = (push.post_init, push.line_idx); + let line = shadow_frame_push_line(&push.handle_reg, count); + let region = if post_init { + &mut self.entry_post_init_setup + } else { + &mut self.entry_allocas + }; + region[line_idx] = line; + Some(idx) + } + /// Mark the current end of the entry block as the boundary between /// the init prelude (`js_gc_init`, `__perry_init_strings_*`) and /// user code. Hoisted post-init setup (cached global loads) is diff --git a/crates/perry-codegen/src/stmt/let_stmt.rs b/crates/perry-codegen/src/stmt/let_stmt.rs index 566a41cd5e..4e5d78fd18 100644 --- a/crates/perry-codegen/src/stmt/let_stmt.rs +++ b/crates/perry-codegen/src/stmt/let_stmt.rs @@ -453,6 +453,12 @@ pub(crate) fn lower_let( let source = lower_expr(ctx, object)?; let source_slot = ctx.func.alloca_entry(DOUBLE); ctx.block().store(DOUBLE, &source, &source_slot); + // #6968: the whole point of capturing the receiver here is that the + // source local may be overwritten afterwards — at which moment this + // alloca holds the ONLY reference to that string, across every + // collection until the fused consumer reads it. Same unrooted-alloca + // hole as the object/array field slots below. + crate::expr::root_scalar_replaced_slot(ctx, &source_slot, object); let dummy_slot = ctx.func.alloca_entry(DOUBLE); let undef = crate::nanbox::double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED)); ctx.func @@ -562,7 +568,13 @@ pub(crate) fn lower_let( (I32, &index.to_string()), ], ); - ctx.block().store(DOUBLE, &value, &slots[index as usize]); + let part_slot = slots[index as usize].clone(); + ctx.block().store(DOUBLE, &value, &part_slot); + // #6968: `js_string_split_part_value` hands back a fresh + // heap string whose only reference is this alloca. There + // is no HIR expression to gate on — the value is + // synthesized by codegen — and it is always a string. + crate::expr::root_scalar_replaced_slot_unconditional(ctx, &part_slot); } } ctx.scalar_replaced_arrays.insert(id, slots); @@ -606,6 +618,11 @@ pub(crate) fn lower_let( } let v = lower_expr(ctx, elem)?; ctx.block().store(DOUBLE, &v, &slots[i]); + // #6968: same rooting hole as the object-literal fields — + // the element alloca is the only reference to a heap value + // stored here, and no HIR local names it. + let elem_slot = slots[i].clone(); + crate::expr::root_scalar_replaced_slot(ctx, &elem_slot, elem); // A uniquely-owned string captured into this scalar-replaced // array slot aliases its heap buffer; demote it to shared so a // later in-place `+=` on the source local doesn't mutate the @@ -687,6 +704,10 @@ pub(crate) fn lower_let( let v = lower_expr(ctx, value_expr)?; if let Some(slot) = field_slots.get(key).cloned() { ctx.block().store(DOUBLE, &v, &slot); + // #6968: the field alloca is this heap value's only + // reference — there is no object for #6951/#6972's + // handle rooting to cover — so bind it as a precise root. + crate::expr::root_scalar_replaced_slot(ctx, &slot, value_expr); let lowered = LoweredValue { semantic: SemanticKind::JsValue, rep: NativeRep::JsValue, @@ -797,6 +818,10 @@ pub(crate) fn lower_let( let arg_val = lower_expr(ctx, arg)?; if let Some(slot) = slot { ctx.block().store(DOUBLE, &arg_val, &slot); + // #6968: anonymous-shape scalar replacement stores + // constructor arguments straight into per-field + // allocas — same unrooted-heap-value hole. + crate::expr::root_scalar_replaced_slot(ctx, &slot, arg); let lowered = LoweredValue { semantic: SemanticKind::JsValue, rep: NativeRep::JsValue, diff --git a/crates/perry-codegen/tests/scalar_replaced_slot_roots.rs b/crates/perry-codegen/tests/scalar_replaced_slot_roots.rs new file mode 100644 index 0000000000..f8de4d74ea --- /dev/null +++ b/crates/perry-codegen/tests/scalar_replaced_slot_roots.rs @@ -0,0 +1,413 @@ +//! #6968: a heap value stored into a scalar-replaced object field or array +//! element must be a precise GC root. +//! +//! Scalar replacement deletes the object and keeps one entry-block alloca per +//! field. Those allocas belong to no HIR local, so the pre-lowering +//! `collect_pointer_typed_locals` pass — which assigns shadow slots by walking +//! `Stmt::Let` — cannot see them, and nothing bound them. A collection landing +//! between the store and the read swept the value out from under the alloca. +//! +//! The end-to-end proof is `test-files/test_gap_repsel_scalar_replaced_locals.ts` +//! on the `cons_scan_off` arm of `scripts/gc_repsel_matrix.sh` — the only +//! configuration where the bug is observable, because every automatic +//! collection otherwise forces a conservative native-stack scan that pins the +//! alloca by accident. These tests pin the *codegen contract* that arm depends +//! on, in-process, so a lowering path that goes back to emitting a bare alloca +//! fails here instead of under a later narrowing of the forced scan. +//! +//! Both directions are covered: the gate must stay silent for a +//! literal whose fields are numbers, or every scalar-replaced `{x, y}` in a hot +//! loop would pay for rooting a value that can never be collected (#6997). + +use perry_codegen::{compile_module, AppMetadata, CompileOptions}; +use perry_hir::types::Type; +use perry_hir::{Expr, Module, ModuleInitKind, Stmt}; + +fn entry_opts() -> CompileOptions { + CompileOptions { + target: None, + is_entry_module: true, + non_entry_module_prefixes: Vec::new(), + nextjs_path_init_modules: Vec::new(), + import_function_prefixes: std::collections::HashMap::new(), + import_function_ffi_aliases: std::collections::HashMap::new(), + import_function_origin_names: std::collections::HashMap::new(), + import_function_v8_specifiers: std::collections::HashMap::new(), + import_function_node_submodule: std::collections::HashMap::new(), + namespace_node_submodules: std::collections::HashMap::new(), + namespace_v8_specifiers: std::collections::HashMap::new(), + namespace_member_prefixes: std::collections::HashMap::new(), + namespace_member_origin_names: std::collections::HashMap::new(), + emit_ir_only: true, + verify_native_regions: false, + disable_buffer_fast_path: false, + namespace_imports: Vec::new(), + imported_classes: Vec::new(), + imported_enums: Vec::new(), + imported_async_funcs: std::collections::HashSet::new(), + type_aliases: std::collections::HashMap::new(), + imported_func_param_counts: std::collections::HashMap::new(), + imported_func_has_rest: std::collections::HashSet::new(), + imported_func_synthetic_arguments: std::collections::HashSet::new(), + imported_func_return_types: std::collections::HashMap::new(), + imported_vars: std::collections::HashSet::new(), + output_type: "executable".to_string(), + needs_stdlib: false, + needs_ui: false, + needs_geisterhand: false, + geisterhand_port: 7676, + enabled_features: Vec::new(), + native_module_init_names: Vec::new(), + js_module_specifiers: Vec::new(), + bundled_extensions: Vec::new(), + native_library_functions: Vec::new(), + i18n_table: None, + fast_math: false, + fp_contract_mode: perry_codegen::FpContractMode::Off, + app_metadata: AppMetadata::default(), + namespace_entries: Vec::new(), + dynamic_import_path_to_prefix: std::collections::HashMap::new(), + deferred_module_prefixes: std::collections::HashSet::new(), + module_init_deps: Vec::new(), + is_dynamic_import_target: false, + debug_locations: false, + module_source: None, + debug_source_line_offset: 0, + } +} + +fn module_with_init(name: &str, init: Vec) -> Module { + Module { + name: name.to_string(), + imports: Vec::new(), + exports: Vec::new(), + classes: Vec::new(), + interfaces: Vec::new(), + type_aliases: Vec::new(), + enums: Vec::new(), + globals: Vec::new(), + functions: Vec::new(), + script_global_functions: Vec::new(), + references_global_this: false, + annexb_global_undefined_names: Vec::new(), + init, + exported_native_instances: Vec::new(), + exported_func_return_native_instances: Vec::new(), + exported_objects: Vec::new(), + exported_functions: Vec::new(), + widgets: Vec::new(), + uses_fetch: false, + uses_webassembly: false, + extern_funcs: Vec::new(), + init_was_unrolled: false, + has_top_level_await: false, + init_kind: ModuleInitKind::Eager, + async_step_closures: std::collections::HashSet::new(), + closure_display_names: std::collections::HashMap::new(), + class_display_names: std::collections::HashMap::new(), + closure_source_text: std::collections::HashMap::new(), + async_generator_funcs: std::collections::HashSet::new(), + gen_param_prologue_len: std::collections::HashMap::new(), + } +} + +fn ir_for(name: &str, init: Vec) -> String { + String::from_utf8(compile_module(&module_with_init(name, init), entry_opts()).unwrap()) + .expect("LLVM IR should be UTF-8") +} + +fn let_stmt(id: u32, name: &str, init: Expr) -> Stmt { + Stmt::Let { + id, + name: name.to_string(), + ty: Type::Any, + mutable: false, + init: Some(init), + } +} + +/// A heap value with no other reference: a fresh object literal that is not +/// itself bound to a local, so it is allocated on the heap and the field +/// alloca is the only thing pointing at it. +fn heap_value() -> Expr { + Expr::Object(vec![("k".to_string(), Expr::Number(1.0))]) +} + +fn field_get(local: u32, field: &str) -> Expr { + Expr::PropertyGet { + object: Box::new(Expr::LocalGet(local)), + property: field.to_string(), + byte_offset: 0, + } +} + +fn console_log(args: Vec) -> Stmt { + Stmt::Expr(Expr::Call { + callee: Box::new(Expr::PropertyGet { + object: Box::new(Expr::GlobalGet(0)), + property: "log".to_string(), + byte_offset: 0, + }), + args, + type_args: Vec::new(), + byte_offset: 0, + }) +} + +/// Count of emitted `js_shadow_slot_bind` CALL sites. The `declare` line is +/// unconditional, so only calls count. +fn bind_calls(ir: &str) -> usize { + ir.matches("call void @js_shadow_slot_bind(").count() +} + +/// The slot count baked into this module-init function's frame push. +fn frame_slot_count(ir: &str) -> u32 { + let needle = "call i64 @js_shadow_frame_push(i32 "; + let start = ir + .find(needle) + .map(|i| i + needle.len()) + .unwrap_or_else(|| { + panic!("expected a shadow frame push in:\n{ir}"); + }); + let rest = &ir[start..]; + let end = rest.find(')').expect("malformed frame push"); + rest[..end] + .parse() + .expect("frame push count is not a number") +} + +/// A scalar-replaced object literal whose field holds a heap value must bind +/// that field's alloca as a precise root. +/// +/// Pre-fix the field store was `store double %v, ptr %slot` into a bare +/// entry-block alloca with no `js_shadow_slot_bind` anywhere in the function: +/// the object local's own reserved slot is only ever *cleared*, because scalar +/// replacement leaves no object handle to bind. A collection between the store +/// and the read therefore swept the value (#6968). +#[test] +fn scalar_replaced_object_field_holding_a_heap_value_is_bound() { + let ir = ir_for( + "scalar_object_field_root.ts", + vec![ + let_stmt( + 1, + "o", + Expr::Object(vec![ + ("a".to_string(), heap_value()), + ("b".to_string(), Expr::Number(2.0)), + ]), + ), + console_log(vec![field_get(1, "a"), field_get(1, "b")]), + ], + ); + + assert!( + bind_calls(&ir) > 0, + "the scalar-replaced field alloca holding a heap value must be bound \ + as a precise root (#6968):\n{ir}" + ); + + // Frame growth, stated DIFFERENTIALLY. `frame_slot_count(&ir) > 0` on its + // own certifies nothing: `o` is pointer-typed, so the pre-lowering pass + // already reserved it a slot and the frame is non-empty with or without + // this fix. The claim that has teeth is that the pointer-capable field + // takes an ADDITIONAL slot the pointer analysis could not have predicted, + // so compare against the structurally identical numeric-only literal — + // same local, same field count, same reads, no rooting. + let control = ir_for( + "scalar_object_field_root_control.ts", + vec![ + let_stmt( + 1, + "o", + Expr::Object(vec![ + ("a".to_string(), Expr::Number(1.0)), + ("b".to_string(), Expr::Number(2.0)), + ]), + ), + console_log(vec![field_get(1, "a"), field_get(1, "b")]), + ], + ); + assert!( + frame_slot_count(&ir) > frame_slot_count(&control), + "binding a scalar-replacement alloca must grow the shadow frame beyond \ + what the pre-lowering pointer analysis reserved: heap-field literal \ + has {} slots, the numeric-only control has {} — the pre-lowering pass \ + cannot see these allocas, so the extra slot can only come from \ + `reserve_shadow_slot` (#6968):\n{ir}", + frame_slot_count(&ir), + frame_slot_count(&control), + ); +} + +/// The gate, from the other side: a literal whose every field is a number +/// must emit no rooting at all. +/// +/// This is the #6997 lesson — rooting a value that can never be collected is +/// pure cost on the path that exists *because* it was optimized. The decision +/// is made from the lowering (`expr_is_known_non_pointer_shadow_value`), not +/// from a declared type, so it holds for `any`-typed locals too — which is +/// exactly what this module builds (`Type::Any`). +#[test] +fn numeric_only_scalar_replaced_object_emits_no_rooting() { + let ir = ir_for( + "scalar_object_numeric.ts", + vec![ + let_stmt( + 1, + "p", + Expr::Object(vec![ + ("x".to_string(), Expr::Number(1.0)), + ("y".to_string(), Expr::Number(2.0)), + ]), + ), + console_log(vec![field_get(1, "x"), field_get(1, "y")]), + ], + ); + + assert_eq!( + bind_calls(&ir), + 0, + "a scalar-replaced literal with only numeric fields must not pay for \ + GC rooting:\n{ir}" + ); +} + +/// The array-literal form of the same defect: `const a = [heap, n]` becomes +/// one alloca per element, and element 0 is the only reference to its value. +#[test] +fn scalar_replaced_array_element_holding_a_heap_value_is_bound() { + let ir = ir_for( + "scalar_array_element_root.ts", + vec![ + let_stmt(1, "a", Expr::Array(vec![heap_value(), Expr::Number(2.0)])), + console_log(vec![ + Expr::IndexGet { + object: Box::new(Expr::LocalGet(1)), + index: Box::new(Expr::Integer(0)), + }, + Expr::IndexGet { + object: Box::new(Expr::LocalGet(1)), + index: Box::new(Expr::Integer(1)), + }, + ]), + ], + ); + + assert!( + bind_calls(&ir) > 0, + "the scalar-replaced array element alloca holding a heap value must be \ + bound as a precise root (#6968):\n{ir}" + ); +} + +/// …and its numeric twin stays free. +#[test] +fn numeric_only_scalar_replaced_array_emits_no_rooting() { + let ir = ir_for( + "scalar_array_numeric.ts", + vec![ + let_stmt( + 1, + "a", + Expr::Array(vec![Expr::Number(1.0), Expr::Number(2.0)]), + ), + console_log(vec![Expr::IndexGet { + object: Box::new(Expr::LocalGet(1)), + index: Box::new(Expr::Integer(0)), + }]), + ], + ); + + assert_eq!( + bind_calls(&ir), + 0, + "a scalar-replaced numeric array literal must not pay for GC rooting:\n{ir}" + ); +} + +/// The scalar-replaced `split()` arm: its element slots receive +/// `js_string_split_part_value` results — fresh heap strings with nothing else +/// referring to them — so they are rooted unconditionally (there is no HIR +/// expression to gate on; the value is synthesized by codegen). +/// +/// Stated differentially against the same string local WITHOUT the split, +/// because the string local itself is pointer-typed and binds its own slot in +/// both compilers: only the extra binds can come from the part slots. +#[test] +fn scalar_replaced_split_parts_are_bound() { + let source = Stmt::Let { + id: 1, + name: "s".to_string(), + ty: Type::String, + mutable: false, + init: Some(Expr::String("a,b,c".to_string())), + }; + let split = Expr::Call { + callee: Box::new(Expr::PropertyGet { + object: Box::new(Expr::LocalGet(1)), + property: "split".to_string(), + byte_offset: 0, + }), + args: vec![Expr::String(",".to_string())], + type_args: Vec::new(), + byte_offset: 0, + }; + let ir = ir_for( + "scalar_split_parts.ts", + vec![ + source.clone(), + let_stmt(2, "parts", split), + console_log(vec![ + Expr::IndexGet { + object: Box::new(Expr::LocalGet(2)), + index: Box::new(Expr::Integer(0)), + }, + Expr::IndexGet { + object: Box::new(Expr::LocalGet(2)), + index: Box::new(Expr::Integer(1)), + }, + ]), + ], + ); + let control = ir_for( + "scalar_split_parts_control.ts", + vec![source, console_log(vec![Expr::LocalGet(1)])], + ); + + assert!( + bind_calls(&ir) > bind_calls(&control), + "the scalar-replaced split part slots must be bound as precise roots: \ + split IR has {} binds, the split-free control has {} (#6968):\n{ir}", + bind_calls(&ir), + bind_calls(&control), + ); +} + +/// A later `o.a = ` writes the same alloca and must root it too — the +/// object-literal initializer is not the only store site. +#[test] +fn later_store_into_a_scalar_replaced_field_is_bound() { + let ir = ir_for( + "scalar_object_field_reassign.ts", + vec![ + let_stmt( + 1, + "o", + Expr::Object(vec![("a".to_string(), Expr::Number(0.0))]), + ), + Stmt::Expr(Expr::PropertySet { + object: Box::new(Expr::LocalGet(1)), + property: "a".to_string(), + value: Box::new(heap_value()), + }), + console_log(vec![field_get(1, "a")]), + ], + ); + + assert!( + bind_calls(&ir) > 0, + "a heap value assigned into a scalar-replaced field after construction \ + must be rooted as well (#6968):\n{ir}" + ); +} diff --git a/test-files/test_gap_repsel_scalar_replaced_locals.ts b/test-files/test_gap_repsel_scalar_replaced_locals.ts new file mode 100644 index 0000000000..037abbd474 --- /dev/null +++ b/test-files/test_gap_repsel_scalar_replaced_locals.ts @@ -0,0 +1,65 @@ +// #6968: a heap value stored into a SCALAR-REPLACED object field or array +// element must be a precise GC root. +// +// Escape analysis deletes the object and keeps one entry-block alloca per +// field/element. Those allocas belong to no HIR local, so the shadow-slot +// assignment pass (which walks `Stmt::Let`) never saw them and nothing bound +// them. With precise roots only, the value was swept out from under the +// alloca and the read returned recycled memory: +// +// $ PERRY_CONSERVATIVE_STACK_SCAN=off PERRY_GC_HEAP_LIMIT=8 ./repro +// obj f0-0 417894 +// obj 417894 <- o.a is gone +// +// The failure is a use-after-free, so the freed block has to be RECYCLED +// before it shows: one statement passes, six in a row do not. Each `fresh()` +// result is held only by the field alloca while `churn()` collects. +// +// Registered in test-parity/gc_repsel_corpus.txt; the `cons_scan_off` arm of +// scripts/gc_repsel_matrix.sh is the configuration that observes this. + +let sink: unknown[] = []; + +function churn(n: number): number { + let acc = 0; + for (let i = 0; i < n; i++) { + sink.push({ i: i, s: "x" + (i & 255), a: [i, i + 1] }); + if (sink.length > 2048) { + acc = (acc + sink.length) | 0; + sink = []; + } + } + return acc | 0; +} + +function fresh(k: number): string { + return "f" + k + "-" + (k * 7); +} + +const N = 200000; + +// --- scalar-replaced object literal: field `a` holds the only reference ---- +{ const o = { a: fresh(0), b: churn(N) }; console.log("obj", o.a, o.b); } +{ const o = { a: fresh(1), b: churn(N) }; console.log("obj", o.a, o.b); } +{ const o = { a: fresh(2), b: churn(N) }; console.log("obj", o.a, o.b); } +{ const o = { a: fresh(3), b: churn(N) }; console.log("obj", o.a, o.b); } +{ const o = { a: fresh(4), b: churn(N) }; console.log("obj", o.a, o.b); } +{ const o = { a: fresh(5), b: churn(N) }; console.log("obj", o.a, o.b); } + +// --- scalar-replaced array literal: element 0 holds the only reference ----- +{ const a = [fresh(6), churn(N)]; console.log("arr", a[0], a[1]); } +{ const a = [fresh(7), churn(N)]; console.log("arr", a[0], a[1]); } +{ const a = [fresh(8), churn(N)]; console.log("arr", a[0], a[1]); } +{ const a = [fresh(9), churn(N)]; console.log("arr", a[0], a[1]); } +{ const a = [fresh(10), churn(N)]; console.log("arr", a[0], a[1]); } +{ const a = [fresh(11), churn(N)]; console.log("arr", a[0], a[1]); } + +// --- a store AFTER construction writes the same alloca -------------------- +{ const o = { a: "seed", b: 0 }; o.a = fresh(12); const n = churn(N); console.log("set", o.a, n); } +{ const o = { a: "seed", b: 0 }; o.a = fresh(13); const n = churn(N); console.log("set", o.a, n); } +{ const o = { a: "seed", b: 0 }; o.a = fresh(14); const n = churn(N); console.log("set", o.a, n); } +{ const o = { a: "seed", b: 0 }; o.a = fresh(15); const n = churn(N); console.log("set", o.a, n); } + +// --- the numeric-only twin must keep working (it needs no rooting at all) -- +{ const p = { x: 3, y: 4 }; console.log("num", (p.x * p.x + p.y * p.y) | 0); } +{ const q = [5, 12]; console.log("num", (q[0] * q[0] + q[1] * q[1]) | 0); } diff --git a/test-parity/gc_repsel_corpus.txt b/test-parity/gc_repsel_corpus.txt index a8d9d04ed4..deeb85818e 100644 --- a/test-parity/gc_repsel_corpus.txt +++ b/test-parity/gc_repsel_corpus.txt @@ -59,6 +59,15 @@ test_gap_repsel_proven_this_frozen # per-PR when `evac_minor` joins PR_ARMS. test_gap_gc_ta_ctor_source_rooting +# --- Scalar-replaced object/array locals (#6968) ----------------------------- +# Not a representation of its own: escape analysis DELETES the object and keeps +# one entry-block alloca per field/element. Those allocas belong to no HIR +# local, so the shadow-slot assignment pass could not see them and a heap value +# living in one was invisible to a precise-roots collection. Live by +# construction (it churns across every read), so the `cons_scan_off` and +# `evac_minor` arms both bite. +test_gap_repsel_scalar_replaced_locals + # --- The GC-live member ------------------------------------------------------ # Every file above performs ZERO collections (measured, #6950), which makes the # GC arms inert against them. This one holds each representation's local live