From 9374131ccfc1d7d2339ea711590336254b87b750 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Wed, 29 Jul 2026 18:08:33 +0200 Subject: [PATCH] wip: hoist scalar-slot GC root binds --- crates/perry-codegen/src/expr/mod.rs | 5 +- .../src/expr/scalar_slot_root.rs | 91 ++++-- crates/perry-codegen/src/expr/shadow_slot.rs | 12 +- crates/perry-codegen/src/stmt/let_stmt.rs | 19 +- .../tests/scalar_replaced_slot_roots.rs | 285 ++++++++++++++++++ 5 files changed, 380 insertions(+), 32 deletions(-) diff --git a/crates/perry-codegen/src/expr/mod.rs b/crates/perry-codegen/src/expr/mod.rs index 0538606d07..d83940d4da 100644 --- a/crates/perry-codegen/src/expr/mod.rs +++ b/crates/perry-codegen/src/expr/mod.rs @@ -147,8 +147,9 @@ 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, + emit_persistent_shadow_root_barrier, 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, }; /// One in-flight inline-constructor return target. See diff --git a/crates/perry-codegen/src/expr/scalar_slot_root.rs b/crates/perry-codegen/src/expr/scalar_slot_root.rs index ebbf0fd62f..23f96ae80d 100644 --- a/crates/perry-codegen/src/expr/scalar_slot_root.rs +++ b/crates/perry-codegen/src/expr/scalar_slot_root.rs @@ -32,11 +32,31 @@ //! 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 bind runs **once, in the function-entry setup** — not at each store. +//! What a bind does is `slot_ptrs[idx] = alloca; stack[idx] = *alloca; +//! active[idx] = true; root_barrier(*alloca)`. For an entry-hoisted alloca the +//! first three are loop-invariant: the address never changes, and every reader +//! of a bound slot (`visit_shadow_stack_root_slots`, `js_shadow_slot_get`) +//! dereferences `slot_ptrs[idx]` in preference to the `stack[idx]` mirror, so +//! the mirror is dead storage. Only the root barrier is per-store work, and it +//! is emitted inline and guarded (`emit_persistent_shadow_root_barrier`). +//! +//! This is the same treatment `enable_persistent_shadow_slot_for_array_alias` +//! already gives a `const item = arr[i]` alias, for the same reason. +//! +//! The hoist does **not** move when the rooted value is read. The collector +//! reads the alloca at collection time, exactly as it did when the bind sat at +//! the store; nothing is snapshotted into a register and re-read later. What +//! disappears is a redundant copy, not an observation point. +//! +//! # Why the slot must be initialized before the bind +//! +//! Binding at entry makes the slot `active` from function entry, so the +//! collector starts dereferencing the alloca *before* any store reaches it. An +//! uninitialized alloca would hand the root-word decoder stack garbage that +//! can pass `is_plausible_heap_addr`. Every path that hoists a bind therefore +//! initializes its alloca to `undefined` in `entry_allocas`, ahead of the +//! `entry_post_init_setup` region the bind lands in. //! //! # The gate //! @@ -51,17 +71,17 @@ use super::*; use perry_hir::Expr; -use crate::types::{I32, PTR}; +use crate::types::{I32, I64, 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. +/// Call *after* the `store` — the emitted root barrier reads the alloca back, +/// 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; @@ -80,20 +100,35 @@ pub(crate) fn root_scalar_replaced_slot_unconditional(ctx: &mut FnCtx<'_>, 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)], - ); + if !ctx.scalar_slot_shadow_slots.contains_key(slot) { + // `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); + // One bind for the whole frame. `reserve_shadow_slot` runs first so a + // lazily-created `js_shadow_frame_push` is already in + // `entry_post_init_setup` when this call is appended after it — a bind + // that ran before the push would land in the caller's frame. + ctx.func.entry_setup_call_void( + "js_shadow_slot_bind", + &[(I32, &idx.to_string()), (PTR, slot)], + ); + } + emit_scalar_slot_store_barrier(ctx, slot); +} + +/// The per-store remainder of a bind: shade the newly stored value so an +/// in-flight incremental mark cannot miss it. +/// +/// The operand is read back from the alloca rather than threaded down from the +/// caller's value register **because that is precisely what the bind it +/// replaces did** (`js_shadow_slot_bind` dereferences `value_slot`). The load +/// sits in the same block, immediately after the store that produced the value, +/// with nothing in between — it cannot observe a later write, and LLVM forwards +/// it to the stored register. +fn emit_scalar_slot_store_barrier(ctx: &mut FnCtx<'_>, slot: &str) { + let value_bits = ctx.block().load(I64, slot); + crate::expr::emit_persistent_shadow_root_barrier(ctx, &value_bits); } diff --git a/crates/perry-codegen/src/expr/shadow_slot.rs b/crates/perry-codegen/src/expr/shadow_slot.rs index 1c4d9b0398..92c52455f1 100644 --- a/crates/perry-codegen/src/expr/shadow_slot.rs +++ b/crates/perry-codegen/src/expr/shadow_slot.rs @@ -170,7 +170,17 @@ pub(crate) fn emit_shadow_slot_bind_for_local(ctx: &mut FnCtx<'_>, local_id: u32 ); } -fn emit_persistent_shadow_root_barrier(ctx: &mut FnCtx<'_>, value_bits: &str) { +/// Emit the incremental-mark root shading barrier for a value that has just +/// been written into an already-bound (persistent) root slot. +/// +/// This is the only part of `js_shadow_slot_bind` that is genuinely per-store: +/// re-recording `slot_ptrs[idx]` and re-mirroring the value are loop-invariant +/// for an entry-hoisted alloca, but a pointer stored into a root *after* the +/// collector scanned roots still has to be shaded. Guarding on +/// `PERRY_INCREMENTAL_MARK_BARRIER_ACTIVE_COUNT` inline keeps the common +/// (no incremental cycle in flight) path down to a load, a compare, and a +/// not-taken branch instead of a TLS-touching call. +pub(crate) fn emit_persistent_shadow_root_barrier(ctx: &mut FnCtx<'_>, value_bits: &str) { let active = ctx.block() .load_atomic_seq_cst(I32, "@PERRY_INCREMENTAL_MARK_BARRIER_ACTIVE_COUNT", 4); diff --git a/crates/perry-codegen/src/stmt/let_stmt.rs b/crates/perry-codegen/src/stmt/let_stmt.rs index 4e5d78fd18..ab3f14fec9 100644 --- a/crates/perry-codegen/src/stmt/let_stmt.rs +++ b/crates/perry-codegen/src/stmt/let_stmt.rs @@ -452,6 +452,13 @@ pub(crate) fn lower_let( }; let source = lower_expr(ctx, object)?; let source_slot = ctx.func.alloca_entry(DOUBLE); + // See the array-element slots below: the root bind is hoisted to + // function entry, so this alloca is a live root before the store + // below runs. Give it a decodable `undefined` first. + let source_undef = + crate::nanbox::double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED)); + ctx.func + .entry_allocas_push_store(DOUBLE, &source_undef, &source_slot); 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 @@ -599,8 +606,18 @@ pub(crate) fn lower_let( if ctx.non_escaping_arrays.contains_key(&id) { let n = elements.len(); let mut slots: Vec = Vec::with_capacity(n); + // Initialize to `undefined` in the entry block, like the + // object-literal field slots below. `root_scalar_replaced_slot` + // binds a pointer-capable element's alloca as a GC root once at + // function entry, which makes the collector dereference it from + // entry onward — before the element store runs, and on paths where + // it never runs at all. An uninitialized alloca would feed the + // root-word decoder stack garbage. + let undef = crate::nanbox::double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED)); for _ in 0..n { - slots.push(ctx.func.alloca_entry(DOUBLE)); + let slot = ctx.func.alloca_entry(DOUBLE); + ctx.func.entry_allocas_push_store(DOUBLE, &undef, &slot); + slots.push(slot); } // Evaluate each element expression first; store the // result into its slot. Order matches source, so any diff --git a/crates/perry-codegen/tests/scalar_replaced_slot_roots.rs b/crates/perry-codegen/tests/scalar_replaced_slot_roots.rs index f8de4d74ea..4f9f4675e1 100644 --- a/crates/perry-codegen/tests/scalar_replaced_slot_roots.rs +++ b/crates/perry-codegen/tests/scalar_replaced_slot_roots.rs @@ -160,6 +160,33 @@ fn bind_calls(ir: &str) -> usize { ir.matches("call void @js_shadow_slot_bind(").count() } +/// Count of emitted incremental-mark root-shading barriers. This is the +/// per-store remainder left behind once the bind is hoisted to entry. +fn root_barriers(ir: &str) -> usize { + ir.matches("call void @js_write_barrier_root_nanbox(") + .count() +} + +/// The whole `define … { … }` body of the function containing `needle`. +/// +/// The tiny modules these tests build put everything in module init, but +/// scoping the assertions to one function keeps ordering claims meaningful if +/// that ever stops being true. +fn enclosing_function<'a>(ir: &'a str, needle: &str) -> &'a str { + let at = ir + .find(needle) + .unwrap_or_else(|| panic!("no `{needle}` in:\n{ir}")); + let start = ir[..at] + .rfind("\ndefine ") + .map(|i| i + 1) + .unwrap_or_else(|| panic!("`{needle}` is outside any function in:\n{ir}")); + let end = ir[start..] + .find("\n}") + .map(|i| start + i + 2) + .unwrap_or(ir.len()); + &ir[start..end] +} + /// 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 "; @@ -411,3 +438,261 @@ fn later_store_into_a_scalar_replaced_field_is_bound() { must be rooted as well (#6968):\n{ir}" ); } + +// --------------------------------------------------------------------------- +// The bind is per SLOT, not per STORE (#7013). +// +// #7007 emitted `js_shadow_slot_bind` at every store into a scalar-replacement +// alloca. That call is loop-invariant apart from its root barrier: the alloca +// is entry-hoisted, so `slot_ptrs[idx]` never changes, and every reader of a +// bound slot dereferences `slot_ptrs[idx]` rather than the `stack[idx]` mirror +// the bind refreshes. In a loop it cost ~4 ns per iteration for nothing. +// --------------------------------------------------------------------------- + +/// Two heap stores into the SAME scalar-replaced field must emit exactly one +/// bind — hoisted to function entry — not one per store. +/// +/// Teeth: pre-hoist this IR carried one bind per store site (2), so the +/// equality fails on the old compiler. It also fails if a future change drops +/// the bind altogether (0), which would un-root the alloca and reopen #6968. +#[test] +fn repeated_stores_into_one_scalar_slot_bind_once() { + let ir = ir_for( + "scalar_field_two_stores.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()), + }), + 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_eq!( + bind_calls(&ir), + 1, + "two heap stores into one scalar-replaced field must share a single \ + entry-hoisted bind — the alloca address is loop-invariant, so \ + re-binding is pure per-store cost (#7013):\n{ir}" + ); +} + +/// Every store still shades its value, so an in-flight incremental mark cannot +/// miss a pointer written into an already-scanned root. +/// +/// This is the part of the bind that is genuinely per-store, and dropping it +/// while hoisting the rest would be a silent incremental-GC miscompile. +/// +/// Teeth: pre-hoist the scalar-slot path emitted no +/// `js_write_barrier_root_nanbox` at all (the shading happened inside +/// `js_shadow_slot_bind`), so the old compiler produces 0 and fails. +#[test] +fn every_store_into_a_hoisted_scalar_slot_shades_its_value() { + let ir = ir_for( + "scalar_field_two_stores_barrier.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()), + }), + 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_eq!( + root_barriers(&ir), + 2, + "each of the two heap stores must shade the value it wrote; the \ + hoisted bind only shades what the alloca held at function entry \ + (#7013):\n{ir}" + ); + + // The barrier must be the guarded form, not an unconditional call: the + // whole point of hoisting is that the common path (no incremental cycle in + // flight) stays a load + compare + not-taken branch. + assert!( + ir.contains("@PERRY_INCREMENTAL_MARK_BARRIER_ACTIVE_COUNT"), + "the per-store shading barrier must be guarded on the incremental-mark \ + active count, otherwise the hoist just trades one unconditional call \ + for another (#7013):\n{ir}" + ); +} + +/// The bind must sit in the ENTRY block, ahead of the loop that stores through +/// the slot — and after `js_shadow_frame_push`. +/// +/// This is the property the whole change exists for: a store inside a loop must +/// not re-bind per iteration. It is also where the one real ordering hazard +/// lives — `reserve_shadow_slot` can create the frame push lazily, into the very +/// region the hoisted bind is appended to, and a bind emitted before the push +/// would write a slot in the CALLER's frame. +/// +/// Teeth: pre-hoist the bind was emitted at the store site, i.e. inside the +/// loop body, which is past the entry block's terminator — so the +/// `bind < first_branch` claim fails on the old compiler. +#[test] +fn bind_is_hoisted_into_the_entry_block_ahead_of_the_storing_loop() { + let ir = ir_for( + "scalar_field_loop_bind.ts", + vec![ + let_stmt(9, "n", Expr::Number(3.0)), + Stmt::While { + condition: Expr::Compare { + op: perry_hir::CompareOp::Lt, + left: Box::new(Expr::LocalGet(9)), + right: Box::new(Expr::Number(3.0)), + }, + body: 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_eq!( + bind_calls(&ir), + 1, + "a scalar-replaced field stored once per iteration must be bound once, \ + not once per iteration (#7013):\n{ir}" + ); + + let body = enclosing_function(&ir, "call void @js_shadow_slot_bind("); + let push = body + .find("call i64 @js_shadow_frame_push(") + .unwrap_or_else(|| panic!("no frame push in the binding function:\n{body}")); + let bind = body + .find("call void @js_shadow_slot_bind(") + .expect("bind was located by enclosing_function"); + let first_branch = body + .find("\n br label %") + .unwrap_or_else(|| panic!("no entry-block terminator in:\n{body}")); + + assert!( + push < bind, + "the hoisted bind must follow the frame push — binding before it would \ + write a slot belonging to the caller's frame (#7013):\n{body}" + ); + assert!( + bind < first_branch, + "the bind must sit in the entry block, ahead of the loop that stores \ + through the slot; emitting it at the store site is the per-iteration \ + cost this change removes (#7013):\n{body}" + ); +} + +/// A scalar-replaced ARRAY element alloca must be initialized before the +/// hoisted bind makes it a live root. +/// +/// Binding at entry makes the collector dereference the alloca from function +/// entry, i.e. before the element store runs and on paths where it never runs. +/// The object-literal path already stored `undefined` into its field slots at +/// entry; the array path did not, because pre-hoist nothing read those allocas +/// before their store. +/// +/// Teeth: pre-hoist the array element slots got a bare `alloca` with no entry +/// store, so the `undef_stores > 0` claim fails on the old compiler. +#[test] +fn scalar_replaced_array_element_slots_are_initialized_before_the_bind() { + let ir = ir_for( + "scalar_array_element_init.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)), + }, + ]), + ], + ); + + let body = enclosing_function(&ir, "call void @js_shadow_slot_bind("); + let bind = body + .find("call void @js_shadow_slot_bind(") + .expect("bind was located by enclosing_function"); + + // TAG_UNDEFINED as the double literal codegen emits for it. + let undef = + perry_codegen::nanbox::double_literal(f64::from_bits(perry_codegen::nanbox::TAG_UNDEFINED)); + let undef_store = format!("store double {undef}, ptr "); + let stores_before_bind = body[..bind].matches(undef_store.as_str()).count(); + + assert!( + stores_before_bind > 0, + "a scalar-replaced array element alloca is a live GC root from the \ + hoisted bind onward, so it must hold a decodable `undefined` before \ + that bind rather than uninitialized stack bytes (#7013). Looked for \ + `{undef_store}` ahead of the bind in:\n{body}" + ); +} + +/// The gate still holds after hoisting: a numeric-only literal must emit +/// neither a bind nor a shading barrier. +/// +/// Hoisting moves work to function entry, where it is easy to stop noticing — +/// this keeps the #6997 "a proven-numeric field costs nothing" property honest +/// for the entry region too. +#[test] +fn numeric_only_scalar_replaced_literal_emits_no_entry_rooting() { + let ir = ir_for( + "scalar_numeric_no_entry_rooting.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 proven-numeric literal must not acquire an entry-hoisted bind \ + (#7013):\n{ir}" + ); + assert_eq!( + root_barriers(&ir), + 0, + "a proven-numeric literal must not emit a store-site shading barrier \ + (#7013):\n{ir}" + ); +}