diff --git a/changelog.d/7065-closure-self-pointer-gc-root.md b/changelog.d/7065-closure-self-pointer-gc-root.md new file mode 100644 index 0000000000..ca81ceb4c0 --- /dev/null +++ b/changelog.d/7065-closure-self-pointer-gc-root.md @@ -0,0 +1,25 @@ +### Fixed + +- **A relocating young collection no longer invalidates a closure's own `this_closure` pointer** (#7055). A closure body reaches its captures through `js_closure_get_capture_bits(%this_closure, idx)`, and `%this_closure` is an LLVM parameter — a register value no root enumeration can see. The shipped default runs an evacuating young collection at loop back-edge polls (`js_gc_loop_safepoint`) with precise roots and no conservative native-stack scan, so a closure relocated while its own body was on the stack left that register pointing into from-space; the same cycle resets from-space and hands it straight back to the mutator, after which `js_closure_get_capture_bits` read a foreign object's `capture_count`, judged the index out of range, and returned **0**. Pointer 0 is not a registered box, so every later boxed-capture read yielded `undefined` and every boxed-capture write was silently dropped. + + In an `async fn` the casualty was the generator state machine itself: the async-to-generator transform boxes every body local, `__gen_state` included, so the `state = ` store at the end of a resumed state body went nowhere and the following `await` resumed into the state it had just finished — **replaying one loop iteration** and folding its contribution into the accumulator twice. That is the shape #7055 reported on an ordinary event-loop request pump: a deterministic checksum error of *fixed* magnitude at 150 / 300 / 600 / 1200 requests, present only when `copied_objects > 0` and absent under `PERRY_GEN_GC=0`. + + Closure bodies with captures now spill `%this_closure` into a NaN-boxed entry alloca bound to a shadow-stack slot (`reserve_shadow_slot` + `js_shadow_slot_bind`), and `expr::current_closure_ptr_value` reloads the pointer from that slot at every capture access — so the collector marks and rewrites it like any other precise root. Capture-less closures emit no capture access and keep their frame-free prologue, so `(a, b) => a - b` costs nothing. The typed-ABI clone is untouched and is not a hole: `lower_typed_f64_body_*` bails on anything that is not straight-line arithmetic, so a typed body contains no poll and no allocation. No runtime change — `crates/perry-runtime` is byte-identical. + + Minimal reproducer (11 lines; `calls` must be 3, printed 4 under any relocating arm): + + ```ts + let calls = 0; + async function main(): Promise { + for (let r = 0; r < 3; r++) { + calls = calls + 1; + let o: any = null; + for (let i = 0; i < 50; i++) { o = { id: i }; } + await Promise.resolve(); + } + console.log("calls:" + calls); + } + main(); + ``` + + The issue's `w5_srv_scale.ts` is now byte-exact against node 26.5.0 at 150 / 300 / 600 requests in the shipped default, and across `PERRY_GC_SCAVENGE_NURSERY_MB` 1–16, `PERRY_GC_MOVING_SAFEPOINT=0`, `PERRY_GC_SCAVENGE=1`, `PERRY_GC_FORCE_EVACUATE=1` and `PERRY_GEN_GC=0`. Covered by a `cargo-test`-visible codegen IR-shape test and `crates/perry/tests/gc_closure_self_pointer_root_7055.rs`, both sabotage-verified in both directions. diff --git a/crates/perry-codegen/src/codegen/arguments.rs b/crates/perry-codegen/src/codegen/arguments.rs index 7c02668667..61628d510b 100644 --- a/crates/perry-codegen/src/codegen/arguments.rs +++ b/crates/perry-codegen/src/codegen/arguments.rs @@ -67,7 +67,13 @@ pub(crate) fn materialize_arguments_object( .call(I64, "js_closure_alloc_singleton", &[(PTR, &wrap_ref)]); nanbox_pointer_inline(ctx.block(), &closure_ptr) } - ArgumentsCallee::CurrentClosure => nanbox_pointer_inline(ctx.block(), "%this_closure"), + ArgumentsCallee::CurrentClosure => { + // #7055: through the shadow-rooted slot when there is one — the + // raw `%this_closure` register is not a GC root. + let ptr = crate::expr::try_current_closure_ptr_value(ctx) + .unwrap_or_else(|| "%this_closure".to_string()); + nanbox_pointer_inline(ctx.block(), &ptr) + } } }; let args_obj = ctx.block().call( diff --git a/crates/perry-codegen/src/codegen/closure.rs b/crates/perry-codegen/src/codegen/closure.rs index ac8daf9a27..aef884acbf 100644 --- a/crates/perry-codegen/src/codegen/closure.rs +++ b/crates/perry-codegen/src/codegen/closure.rs @@ -785,6 +785,49 @@ pub(super) fn compile_closure( let mut reassigned_locals = module_reassigned_locals.clone(); reassigned_locals.extend(crate::collectors::reassigned_locals(body)); + // #7055: spill the closure's own `%this_closure` pointer into a + // shadow-rooted entry alloca and read every capture back through it. + // + // `%this_closure` is an LLVM parameter — a register value no root + // enumeration can see. The shipped moving young collection runs at a loop + // back-edge poll (`js_gc_loop_safepoint`) with PRECISE roots and no + // conservative native-stack scan, so a closure relocated while its own body + // is running leaves that register pointing into from-space. From-space is + // reset at the end of the same cycle and immediately reused by the mutator, + // after which `js_closure_get_capture_bits` reads a foreign object's + // `capture_count`, decides the index is out of range, and returns **0** — + // turning every later boxed-capture read into `undefined` and every write + // into a silent no-op. In an `async fn` that swallowed the generator's own + // `__gen_state` store, so the next `await` resumed into the state it had + // just finished and one loop iteration ran twice. + // + // Rooting it here makes the closure a first-class precise root: the + // collector rewrites this slot along with every other shadow slot, and + // `current_closure_ptr_value` reloads from it at each capture access. + // + // Only closures that actually read captures pay for it. A capture-less + // closure (`(a, b) => a - b` handed to `sort`) never emits a + // `js_closure_get_capture_bits` call in its body, so the pointer is dead on + // arrival — and reserving a slot there would force a `js_shadow_frame_push` + // /`pop` pair onto bodies that need no frame at all. The `this` / + // `new.target` capture reads are exempt for a different reason: they run in + // the entry-block prologue, ahead of any statement that could collect. + let current_closure_slot = if closure_captures.is_empty() { + None + } else { + lf.reserve_shadow_slot().map(|idx| { + let blk = lf.block_mut(0).expect("closure body has an entry block"); + let slot = blk.alloca(I64); + let tagged = blk.or(I64, "%this_closure", crate::nanbox::POINTER_TAG_I64); + blk.store(I64, &tagged, &slot); + blk.call_void( + "js_shadow_slot_bind", + &[(I32, &idx.to_string()), (PTR, &slot)], + ); + slot + }) + }; + let mut ctx = FnCtx { func: lf, module_slug: crate::expr::native_region_slug(strings.module_prefix()), @@ -820,6 +863,7 @@ pub(super) fn compile_closure( namespace_v8_specifiers: &cross_module.namespace_v8_specifiers, closure_captures, current_closure_ptr: Some("%this_closure".to_string()), + current_closure_slot, enums, // Async closures (arrow functions declared `async () => ...`) // must wrap their return values in `js_promise_resolved` so the @@ -1052,3 +1096,324 @@ pub(super) fn compile_closure( } Ok(()) } + +#[cfg(test)] +mod tests { + use perry_hir::types::Type; + use perry_hir::{Expr, Function, Module as HirModule, Stmt, UpdateOp}; + + /// Compile a one-closure module to LLVM IR text. + /// + /// `outer() { let x; const f = () => { x = 2; x++; return x; }; return f; }` + /// — the smallest shape that exercises all three capture accessors: a read + /// (`js_closure_get_capture_bits`), a write + /// (`js_closure_set_capture_bits`), and a read-modify-write whose coercion + /// (`js_to_numeric`) can run a user `valueOf` and therefore collect between + /// the read and the write. + fn one_capture_closure_ir() -> String { + let closure = Expr::Closure { + func_id: 1, + params: Vec::new(), + return_type: Type::Any, + body: vec![ + Stmt::Expr(Expr::LocalSet(0, Box::new(Expr::Number(2.0)))), + Stmt::Expr(Expr::Update { + id: 0, + op: UpdateOp::Increment, + prefix: false, + }), + Stmt::Return(Some(Expr::LocalGet(0))), + ], + captures: vec![0], + mutable_captures: vec![0], + captures_this: false, + captures_new_target: false, + enclosing_class: None, + is_arrow: true, + is_async: false, + is_generator: false, + is_strict: true, + }; + let mut hir = HirModule::new("closure_self_root_test"); + hir.functions.push(Function { + id: 0, + name: "outer".to_string(), + type_params: Vec::new(), + params: Vec::new(), + return_type: Type::Any, + body: vec![ + Stmt::Let { + id: 0, + name: "x".to_string(), + ty: Type::Any, + mutable: true, + init: None, + }, + Stmt::Let { + id: 2, + name: "f".to_string(), + ty: Type::Any, + mutable: false, + init: Some(closure), + }, + Stmt::Return(Some(Expr::LocalGet(2))), + ], + is_async: false, + is_generator: false, + is_strict: true, + is_exported: false, + captures: Vec::new(), + decorators: Vec::new(), + was_plain_async: false, + was_unrolled: false, + }); + let opts = crate::CompileOptions { + emit_ir_only: true, + ..Default::default() + }; + let bytes = crate::compile_module(&hir, opts).expect("closure test module compiles"); + String::from_utf8(bytes).expect("LLVM IR is UTF-8") + } + + /// Same module, but the captured id has **no declaration site** in the + /// enclosing function, so `collect_boxed_vars` does not box it and the + /// closure body takes the *unboxed* capture accessors — the pair whose + /// writer (`js_closure_set_capture_bits`) does no bounds check at all. + fn unboxed_capture_update_ir() -> String { + let closure = Expr::Closure { + func_id: 1, + params: Vec::new(), + return_type: Type::Any, + body: vec![ + Stmt::Expr(Expr::Update { + id: 0, + op: UpdateOp::Increment, + prefix: false, + }), + Stmt::Return(Some(Expr::LocalGet(0))), + ], + captures: vec![0], + mutable_captures: Vec::new(), + captures_this: false, + captures_new_target: false, + enclosing_class: None, + is_arrow: true, + is_async: false, + is_generator: false, + is_strict: true, + }; + let mut hir = HirModule::new("closure_unboxed_update_test"); + hir.functions.push(Function { + id: 0, + name: "outer".to_string(), + type_params: Vec::new(), + params: Vec::new(), + return_type: Type::Any, + body: vec![ + Stmt::Let { + id: 2, + name: "f".to_string(), + ty: Type::Any, + mutable: false, + init: Some(closure), + }, + Stmt::Return(Some(Expr::LocalGet(2))), + ], + is_async: false, + is_generator: false, + is_strict: true, + is_exported: false, + captures: Vec::new(), + decorators: Vec::new(), + was_plain_async: false, + was_unrolled: false, + }); + let opts = crate::CompileOptions { + emit_ir_only: true, + ..Default::default() + }; + let bytes = crate::compile_module(&hir, opts).expect("unboxed-capture module compiles"); + String::from_utf8(bytes).expect("LLVM IR is UTF-8") + } + + /// #7055: the closure's own `%this_closure` pointer must be a PRECISE GC + /// root, and capture accesses must read it back from that root. + /// + /// `%this_closure` is an LLVM parameter, i.e. a register the collector + /// cannot see. The shipped moving young collection runs at loop back-edge + /// polls with precise roots and no conservative native-stack scan, so a + /// closure relocated while its own body runs leaves that register pointing + /// into from-space — which is reset and reused before the body's next + /// capture access. `js_closure_get_capture_bits` then reads a foreign + /// object's `capture_count`, judges the index out of range and returns 0, + /// so every later boxed-capture read yields `undefined` and every write is + /// dropped. + /// + /// Teeth: with the fix reverted the body reads captures straight off the + /// parameter and the `js_closure_get_capture_bits(i64 %this_closure` + /// assertion below fails. + #[test] + fn closure_body_roots_its_own_closure_pointer_and_reads_captures_through_it() { + let ir = one_capture_closure_ir(); + // The public `perry_closure_*` symbol can be a typed trampoline over a + // straight-line `__typed_f64` clone; the real body is the one that + // carries a shadow frame. (The typed clone lowers arithmetic-only, + // loop-free, call-free statements — `lower_typed_f64_body_*` bails on + // anything else — so it contains no safepoint and its `%this_closure` + // register cannot go stale.) + let body = ir + .split("define ") + .find(|f| { + let name_starts_here = f.starts_with("double @perry_closure_") + || f.starts_with("internal double @perry_closure_"); + name_starts_here && f.contains("@js_shadow_frame_push") + }) + .unwrap_or_else(|| panic!("no shadow-framed closure body in IR:\n{ir}")); + + // The prologue NaN-boxes `%this_closure` into an alloca and binds that + // alloca to a shadow-stack slot, so the collector marks and rewrites it. + let tagged = format!("or i64 %this_closure, {}", crate::nanbox::POINTER_TAG_I64); + assert!( + body.contains(&tagged), + "closure prologue must NaN-box %this_closure for the shadow slot; body:\n{body}" + ); + assert!( + body.contains("@js_shadow_slot_bind"), + "closure prologue must bind the closure-pointer slot as a GC root; body:\n{body}" + ); + + // And no capture access may use the raw (unrooted) parameter. + assert!( + !body.contains("@js_closure_get_capture_bits(i64 %this_closure"), + "capture reads must reload the closure pointer from its rooted \ + slot, not use the %this_closure register; body:\n{body}" + ); + // Belt and braces, and honestly labelled: this fixture does NOT emit the + // unboxed capture writer. `collect_boxed_vars` boxes every declared + // local a nested closure mutates (and `collect_boxed_param_ids` covers + // params), so a mutating capture always lowers to `get_capture_bits` + + // `js_box_set_bits`, never to `js_closure_set_capture_bits`. This + // assertion guards the unboxed writer against a future change to that + // boxing rule; it is not evidence about today's output. + assert!( + !body.contains("@js_closure_set_capture_bits(i64 %this_closure"), + "capture writes must reload the closure pointer from its rooted \ + slot, not use the %this_closure register; body:\n{body}" + ); + + // Not vacuous: the fixture really does read, write and read-modify-write + // the capture, and the read-modify-write really does emit the ToNumeric + // coercion — the collect-capable call the reload below has to survive. + let accesses = body.matches("@js_closure_get_capture_bits(").count(); + assert!( + accesses >= 2, + "fixture must access the capture more than once (read + write), or \ + the per-access reload assertion below is vacuous; body:\n{body}" + ); + assert!( + body.contains("@js_box_set_bits"), + "fixture must WRITE the capture, not only read it; body:\n{body}" + ); + assert!( + body.contains("@js_to_numeric"), + "the captured `x++` must emit its ToNumeric coercion; body:\n{body}" + ); + + // The invariant, stated positively: EVERY capture access re-reads the + // rooted slot, so no access can be reached through a pointer loaded + // before an intervening collection. Pre-fix this count is 0. + let slot = { + let bind = body + .find("@js_shadow_slot_bind(") + .map(|i| &body[i..]) + .and_then(|t| t.split_once("ptr ")) + .map(|(_, rest)| rest) + .unwrap_or_else(|| panic!("no shadow-slot bind in body:\n{body}")); + bind.split(')').next().expect("bind operand").to_string() + }; + let reload = format!("load i64, ptr {slot}"); + assert!( + body.matches(reload.as_str()).count() >= accesses, + "every one of the {accesses} capture accesses must reload the \ + closure pointer from its rooted slot {slot}; body:\n{body}" + ); + + // #7055 (CodeRabbit): the sharp case — `js_to_numeric` runs a user + // `valueOf`, i.e. arbitrary JS that can reach a loop poll and relocate + // this closure. The capture access that FOLLOWS it must come from a + // fresh load, never from a pointer materialized before the call. + let coerce_at = body.find("@js_to_numeric").expect("coercion call"); + let next_access = body[coerce_at..] + .find("@js_closure_get_capture_bits(") + .map(|i| coerce_at + i) + .unwrap_or_else(|| { + panic!("fixture must access the capture after the coercion; body:\n{body}") + }); + assert!( + body[coerce_at..next_access].contains(reload.as_str()), + "a capture access after `js_to_numeric` (which can run a user \ + `valueOf` and relocate the closure) must re-read the closure \ + pointer from its rooted slot {slot}; body:\n{body}" + ); + } + + /// #7055 (CodeRabbit 🟠 Major): `js_to_numeric` runs a user `valueOf` — + /// arbitrary JS that can reach a `js_gc_loop_safepoint` and relocate this + /// closure — and the *unboxed* capture writer + /// `js_closure_set_capture_bits` does NOT validate its pointer (unlike the + /// reader, which bounds-checks and returns 0). Writing through a closure + /// pointer materialized before the coercion would therefore store into + /// whatever the mutator has since placed at that recycled from-space + /// address. The write must use a pointer re-read from the rooted slot. + /// + /// Reachability, stated plainly: today `collect_boxed_vars` boxes every + /// declared local a nested closure mutates and `collect_boxed_param_ids` + /// covers params, so TypeScript cannot currently produce a mutating + /// *unboxed* capture — the fixture reaches this arm by omitting the + /// declaration site. The arm is live code with a memory-corrupting failure + /// mode if that boxing rule ever narrows, which is what this pins. + #[test] + fn unboxed_capture_write_reloads_the_closure_pointer_after_the_coercion() { + let ir = unboxed_capture_update_ir(); + let body = ir + .split("define ") + .find(|f| { + let name_starts_here = f.starts_with("double @perry_closure_") + || f.starts_with("internal double @perry_closure_"); + name_starts_here && f.contains("@js_closure_set_capture_bits(") + }) + .unwrap_or_else(|| panic!("no unboxed capture write in IR:\n{ir}")); + + // Non-vacuous: this really is the unboxed arm, and the coercion really + // is emitted between the capture read and the capture write. + assert!( + !body.contains("@js_box_set_bits"), + "fixture must take the UNBOXED capture arm; body:\n{body}" + ); + let coerce_at = body + .find("@js_to_numeric") + .unwrap_or_else(|| panic!("captured `x++` must emit its coercion; body:\n{body}")); + let write_at = body + .find("@js_closure_set_capture_bits(") + .expect("capture write"); + assert!( + coerce_at < write_at, + "fixture must coerce before it writes; body:\n{body}" + ); + + let slot = { + let bind = body + .find("@js_shadow_slot_bind(") + .map(|i| &body[i..]) + .and_then(|t| t.split_once("ptr ")) + .map(|(_, rest)| rest) + .unwrap_or_else(|| panic!("no shadow-slot bind in body:\n{body}")); + bind.split(')').next().expect("bind operand").to_string() + }; + assert!( + body[coerce_at..write_at].contains(&format!("load i64, ptr {slot}")), + "the unboxed capture write must re-read the closure pointer from \ + its rooted slot {slot} after `js_to_numeric`; body:\n{body}" + ); + } +} diff --git a/crates/perry-codegen/src/codegen/entry.rs b/crates/perry-codegen/src/codegen/entry.rs index 85dd373928..6a569b66fa 100644 --- a/crates/perry-codegen/src/codegen/entry.rs +++ b/crates/perry-codegen/src/codegen/entry.rs @@ -706,6 +706,7 @@ pub(super) fn compile_module_entry( namespace_v8_specifiers: &cross_module.namespace_v8_specifiers, closure_captures: HashMap::new(), current_closure_ptr: None, + current_closure_slot: None, enums, is_async_fn: false, is_strict_fn: false, @@ -1333,6 +1334,7 @@ pub(super) fn compile_module_entry( namespace_v8_specifiers: &cross_module.namespace_v8_specifiers, closure_captures: HashMap::new(), current_closure_ptr: None, + current_closure_slot: None, enums, is_async_fn: false, is_strict_fn: false, diff --git a/crates/perry-codegen/src/codegen/function.rs b/crates/perry-codegen/src/codegen/function.rs index 3508942711..c6985ce6d0 100644 --- a/crates/perry-codegen/src/codegen/function.rs +++ b/crates/perry-codegen/src/codegen/function.rs @@ -703,6 +703,7 @@ pub(super) fn compile_function( namespace_v8_specifiers: &cross_module.namespace_v8_specifiers, closure_captures: HashMap::new(), current_closure_ptr: None, + current_closure_slot: None, enums, is_async_fn: f.is_async, is_strict_fn: f.is_strict, diff --git a/crates/perry-codegen/src/codegen/method.rs b/crates/perry-codegen/src/codegen/method.rs index 4bebde0c0b..b71f487ea7 100644 --- a/crates/perry-codegen/src/codegen/method.rs +++ b/crates/perry-codegen/src/codegen/method.rs @@ -449,6 +449,7 @@ pub(super) fn compile_method( namespace_v8_specifiers: &cross_module.namespace_v8_specifiers, closure_captures: HashMap::new(), current_closure_ptr: None, + current_closure_slot: None, enums, is_async_fn: method.is_async, is_strict_fn: true, @@ -1498,6 +1499,7 @@ pub(super) fn compile_static_method( namespace_v8_specifiers: &cross_module.namespace_v8_specifiers, closure_captures: HashMap::new(), current_closure_ptr: None, + current_closure_slot: None, enums, is_async_fn: f.is_async, is_strict_fn: f.is_strict, diff --git a/crates/perry-codegen/src/expr/array_push.rs b/crates/perry-codegen/src/expr/array_push.rs index 5a0e1c21aa..43a57f91bf 100644 --- a/crates/perry-codegen/src/expr/array_push.rs +++ b/crates/perry-codegen/src/expr/array_push.rs @@ -432,9 +432,8 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { if ctx.boxed_vars.contains(array_id) { // Captured-through-closure boxed var. if let Some(&capture_idx) = ctx.closure_captures.get(array_id) { - let closure_ptr = ctx.current_closure_ptr.clone().ok_or_else(|| { - anyhow!("ArrayPush boxed captured but no current_closure_ptr") - })?; + let closure_ptr = + super::current_closure_ptr_value(ctx, "ArrayPush boxed captured")?; let idx_str = capture_idx.to_string(); let blk = ctx.block(); let box_ptr = blk.call( @@ -477,10 +476,7 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { // module-global store-back below instead of returning. } if let Some(&capture_idx) = ctx.closure_captures.get(array_id) { - let closure_ptr = ctx - .current_closure_ptr - .clone() - .ok_or_else(|| anyhow!("ArrayPush captured but no current_closure_ptr"))?; + let closure_ptr = super::current_closure_ptr_value(ctx, "ArrayPush captured")?; let idx_str = capture_idx.to_string(); let new_bits = ctx.block().bitcast_double_to_i64(&new_box); ctx.block().call_void( @@ -526,9 +522,8 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { let new_box = nanbox_pointer_inline(blk, &new_handle); if ctx.boxed_vars.contains(array_id) { if let Some(&capture_idx) = ctx.closure_captures.get(array_id) { - let closure_ptr = ctx.current_closure_ptr.clone().ok_or_else(|| { - anyhow!("ArrayPushSpread boxed captured but no current_closure_ptr") - })?; + let closure_ptr = + super::current_closure_ptr_value(ctx, "ArrayPushSpread boxed captured")?; let idx_str = capture_idx.to_string(); let blk = ctx.block(); let box_ptr = blk.call( @@ -562,9 +557,8 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { // GC-root slot (see the matching note in `Expr::ArrayPush`). } if let Some(&capture_idx) = ctx.closure_captures.get(array_id) { - let closure_ptr = ctx.current_closure_ptr.clone().ok_or_else(|| { - anyhow!("ArrayPushSpread captured but no current_closure_ptr") - })?; + let closure_ptr = + super::current_closure_ptr_value(ctx, "ArrayPushSpread captured")?; let idx_str = capture_idx.to_string(); let new_bits = ctx.block().bitcast_double_to_i64(&new_box); ctx.block().call_void( diff --git a/crates/perry-codegen/src/expr/closure.rs b/crates/perry-codegen/src/expr/closure.rs index 40a178c350..329e3df40a 100644 --- a/crates/perry-codegen/src/expr/closure.rs +++ b/crates/perry-codegen/src/expr/closure.rs @@ -4,7 +4,7 @@ //! Pure mechanical move — match arm bodies are verbatim copies, called from //! `lower_expr`'s outer dispatch. -use anyhow::{anyhow, Result}; +use anyhow::Result; use perry_hir::Expr; use crate::type_analysis::compute_auto_captures; @@ -91,9 +91,8 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { // transitively-captured box. Read the // capture slot RAW (it holds the box ptr // bits) and propagate directly. - let closure_ptr = ctx.current_closure_ptr.clone().ok_or_else(|| { - anyhow!("nested boxed capture but no current_closure_ptr") - })?; + let closure_ptr = + super::current_closure_ptr_value(ctx, "nested boxed capture")?; let idx_str = _capture_idx.to_string(); let v = ctx.block().call( I64, diff --git a/crates/perry-codegen/src/expr/instance_misc1.rs b/crates/perry-codegen/src/expr/instance_misc1.rs index 20cebc4351..57256779c5 100644 --- a/crates/perry-codegen/src/expr/instance_misc1.rs +++ b/crates/perry-codegen/src/expr/instance_misc1.rs @@ -4,7 +4,7 @@ //! Pure mechanical move — match arm bodies are verbatim copies, called from //! `lower_expr`'s outer dispatch. -use anyhow::{anyhow, Result}; +use anyhow::Result; use perry_hir::{BinaryOp, Expr, WithSetFallback}; use crate::nanbox::{double_literal, i64_literal, POINTER_MASK_I64}; @@ -82,10 +82,7 @@ fn emit_with_key(ctx: &mut FnCtx<'_>, property: &str) -> (String, String) { fn store_prelowered_local(ctx: &mut FnCtx<'_>, id: u32, value: &str) -> Result { super::invalidate_local_write_facts(ctx, id); if let Some(&capture_idx) = ctx.closure_captures.get(&id) { - let closure_ptr = ctx - .current_closure_ptr - .clone() - .ok_or_else(|| anyhow!("captured with-fallback set but no current_closure_ptr"))?; + let closure_ptr = super::current_closure_ptr_value(ctx, "captured with-fallback set")?; let idx_str = capture_idx.to_string(); if ctx.boxed_vars.contains(&id) { let blk = ctx.block(); diff --git a/crates/perry-codegen/src/expr/literals_vars.rs b/crates/perry-codegen/src/expr/literals_vars.rs index 339feff0b3..8c0c7aa707 100644 --- a/crates/perry-codegen/src/expr/literals_vars.rs +++ b/crates/perry-codegen/src/expr/literals_vars.rs @@ -4,7 +4,7 @@ //! Pure mechanical move — match arm bodies are verbatim copies, called from //! `lower_expr`'s outer dispatch. -use anyhow::{anyhow, Result}; +use anyhow::Result; use perry_hir::types::Type as HirType; use perry_hir::{BinaryOp, Expr, UpdateOp}; @@ -391,10 +391,7 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { } // Captured by closure (from outer scope): if let Some(&capture_idx) = ctx.closure_captures.get(id) { - let closure_ptr = ctx - .current_closure_ptr - .clone() - .ok_or_else(|| anyhow!("captured local but no current_closure_ptr"))?; + let closure_ptr = super::current_closure_ptr_value(ctx, "captured local")?; let idx_str = capture_idx.to_string(); // If the captured id is a boxed var, the capture slot holds a // raw box pointer. Read the capture, extract the box pointer, @@ -578,10 +575,7 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { // Closure captures first (write through the runtime), then // locals, then module globals. if let Some(&capture_idx) = ctx.closure_captures.get(id) { - let closure_ptr = ctx - .current_closure_ptr - .clone() - .ok_or_else(|| anyhow!("captured local set but no current_closure_ptr"))?; + let closure_ptr = super::current_closure_ptr_value(ctx, "captured local set")?; let idx_str = capture_idx.to_string(); // Boxed captured var: read the box pointer from the // capture slot, then js_box_set_bits to update the shared @@ -709,12 +703,18 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { }; // Closure capture path: runtime get + add/sub + runtime set. if let Some(&capture_idx) = ctx.closure_captures.get(id) { - let closure_ptr = ctx - .current_closure_ptr - .clone() - .ok_or_else(|| anyhow!("captured local update but no current_closure_ptr"))?; + let closure_ptr = super::current_closure_ptr_value(ctx, "captured local update")?; let idx_str = capture_idx.to_string(); // Boxed captured var: deref box bits, modify, store back. + // + // `box_ptr` deliberately survives the `coerce_old`/`step_new` + // calls below even though those can collect: a box is + // `std::alloc::alloc`'d by `js_box_alloc_bits`, is never freed + // and is never relocated (`scan_box_roots_mut` rewrites the + // JSValue *inside* the box, not the box's address), so an + // address read before a collection still names the same live + // cell after it. The closure pointer has no such guarantee, + // which is why the non-boxed arm below re-reads it. if ctx.boxed_vars.contains(id) { let blk = ctx.block(); let box_ptr = blk.call( @@ -743,7 +743,23 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { let old = coerce_old(blk, &old); let new = step_new(blk, &old); let new_bits = blk.bitcast_double_to_i64(&new); - blk.call_void( + // #7055: `coerce_old` emits `js_to_numeric`, which runs a user + // `valueOf` — arbitrary JS, including allocating loops that + // reach a `js_gc_loop_safepoint` and relocate this very + // closure. `js_closure_set_capture_bits` does NOT validate its + // pointer (unlike `js_closure_get_capture_bits`, which bounds- + // checks and returns 0), so writing through a pre-coercion + // `closure_ptr` would store into whatever the mutator has since + // put at that recycled from-space address. Re-read the rooted + // slot after the coercion; the collector has rewritten it. + // Only the coercing path pays the reload — a statically + // integer-typed counter emits no call in between. + let closure_ptr = if needs_numeric_coerce { + super::current_closure_ptr_value(ctx, "captured local update")? + } else { + closure_ptr + }; + ctx.block().call_void( "js_closure_set_capture_bits", &[(I64, &closure_ptr), (I32, &idx_str), (I64, &new_bits)], ); diff --git a/crates/perry-codegen/src/expr/mod.rs b/crates/perry-codegen/src/expr/mod.rs index c0a9b52e49..7ebcaad4e1 100644 --- a/crates/perry-codegen/src/expr/mod.rs +++ b/crates/perry-codegen/src/expr/mod.rs @@ -8,7 +8,7 @@ //! error so a user running `--backend llvm` on richer TypeScript gets a //! one-line explanation instead of a silent broken binary. -use anyhow::{anyhow, Result}; +use anyhow::Result; use perry_hir::types::Type as HirType; use perry_hir::{BinaryOp, CompareOp, Expr, UnaryOp}; @@ -147,9 +147,10 @@ pub(crate) use scalar_slot_root::{ root_scalar_replaced_slot, root_scalar_replaced_slot_unconditional, }; pub(crate) use shadow_slot::{ - 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, + current_closure_ptr_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, + try_current_closure_ptr_value, }; /// One in-flight inline-constructor return target. See @@ -343,7 +344,23 @@ pub(crate) struct FnCtx<'a> { /// Inside a closure body, the LLVM SSA value name for the current /// closure pointer (`%this_closure`). `Expr::LocalGet` of a captured /// id uses this as the first arg to `js_closure_get_capture_bits`. + /// + /// Prefer [`current_closure_ptr_value`] over reading this directly — the + /// raw SSA parameter is NOT a GC root and goes stale the moment a + /// relocating collection runs inside the body (#7055). pub current_closure_ptr: Option, + /// Inside a closure body, the entry-block alloca holding the NaN-boxed + /// `%this_closure` pointer, bound to a shadow-stack slot so the moving + /// collector marks AND rewrites it (#7055). + /// + /// `%this_closure` itself lives only in a register, and the copying minor + /// at a loop back-edge poll (`js_gc_loop_safepoint`) runs with precise + /// roots and no conservative stack scan — so a closure relocated mid-body + /// left every later `js_closure_get_capture_bits` reading recycled + /// from-space memory, silently returning 0 and turning every capture + /// read/write into a no-op. Reload through this slot at every capture + /// access instead. + pub current_closure_slot: Option, /// Map from (enum_name, member_name) → enum value. Built once in /// `compile_module` from `hir.enums`. Used by `Expr::EnumMember` /// to lower enum references to constants. @@ -1864,10 +1881,7 @@ pub(crate) fn is_compiler_private_async_i1_control_local(ctx: &FnCtx<'_>, id: u3 pub(crate) fn load_boxed_local_pointer(ctx: &mut FnCtx<'_>, id: u32) -> Result> { if let Some(&capture_idx) = ctx.closure_captures.get(&id) { - let closure_ptr = ctx - .current_closure_ptr - .clone() - .ok_or_else(|| anyhow!("boxed local capture but no current_closure_ptr"))?; + let closure_ptr = current_closure_ptr_value(ctx, "boxed local capture")?; let cap_bits = ctx.block().call( I64, "js_closure_get_capture_bits", diff --git a/crates/perry-codegen/src/expr/shadow_slot.rs b/crates/perry-codegen/src/expr/shadow_slot.rs index 92c52455f1..3d1b2b70db 100644 --- a/crates/perry-codegen/src/expr/shadow_slot.rs +++ b/crates/perry-codegen/src/expr/shadow_slot.rs @@ -5,11 +5,39 @@ //! `crate::expr::X` call paths resolve unchanged. use super::*; +use anyhow::{anyhow, Result}; + use perry_hir::types::Type as HirType; use perry_hir::{BinaryOp, Expr}; use crate::types::{I32, I64, PTR}; +/// The current closure pointer to feed to `js_closure_get/set_capture_bits`, +/// re-read from the shadow-rooted slot when one exists (#7055). +/// +/// Every capture access MUST go through here rather than reusing the +/// `%this_closure` SSA parameter. The parameter is a register value the +/// collector cannot see: an evacuating young collection at a loop back-edge +/// poll inside the body relocates the closure, rewrites every root it knows +/// about, and then resets from-space — after which the register points at +/// recycled memory whose `capture_count` no longer covers the index, so +/// `js_closure_get_capture_bits` returns 0 and every subsequent boxed-capture +/// read/write silently no-ops. Returns `None` when the body has no closure +/// pointer at all (top-level functions/methods). +pub(crate) fn try_current_closure_ptr_value(ctx: &mut FnCtx<'_>) -> Option { + if let Some(slot) = ctx.current_closure_slot.clone() { + let bits = ctx.block().load(I64, &slot); + return Some(ctx.block().and(I64, &bits, crate::nanbox::POINTER_MASK_I64)); + } + ctx.current_closure_ptr.clone() +} + +/// [`try_current_closure_ptr_value`] with the shared "no current_closure_ptr" +/// diagnostic; `what` names the lowering that needed it. +pub(crate) fn current_closure_ptr_value(ctx: &mut FnCtx<'_>, what: &str) -> Result { + try_current_closure_ptr_value(ctx).ok_or_else(|| anyhow!("{what} but no current_closure_ptr")) +} + pub(crate) fn expr_is_known_non_pointer_shadow_value(ctx: &FnCtx<'_>, expr: &Expr) -> bool { match expr { Expr::Undefined | Expr::Null | Expr::Bool(_) | Expr::Number(_) | Expr::Integer(_) => true, diff --git a/crates/perry-codegen/src/expr/static_field_meta.rs b/crates/perry-codegen/src/expr/static_field_meta.rs index d8ce50d2cc..2711bfc0ce 100644 --- a/crates/perry-codegen/src/expr/static_field_meta.rs +++ b/crates/perry-codegen/src/expr/static_field_meta.rs @@ -541,7 +541,7 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { // relink with a concrete closure when they have one. Expr::LinkGeneratorPrototype { obj, is_async } => { let obj_val = lower_expr(ctx, obj)?; - if let Some(closure_ptr) = ctx.current_closure_ptr.clone() { + if let Some(closure_ptr) = crate::expr::try_current_closure_ptr_value(ctx) { return Ok(ctx.block().call( DOUBLE, "js_generator_attach_closure_prototype", diff --git a/crates/perry-codegen/src/lower_array_method.rs b/crates/perry-codegen/src/lower_array_method.rs index 6f034a3a74..906c405343 100644 --- a/crates/perry-codegen/src/lower_array_method.rs +++ b/crates/perry-codegen/src/lower_array_method.rs @@ -34,9 +34,7 @@ pub(crate) fn emit_grow_mutator_writeback( // Boxed var: the slot / capture holds the BOX pointer; update the box // content so every closure sharing the box sees the new head. if let Some(&capture_idx) = ctx.closure_captures.get(&array_id) { - let closure_ptr = ctx.current_closure_ptr.clone().ok_or_else(|| { - anyhow::anyhow!("unshift boxed capture but no current_closure_ptr") - })?; + let closure_ptr = crate::expr::current_closure_ptr_value(ctx, "unshift boxed capture")?; let idx_str = capture_idx.to_string(); let blk = ctx.block(); let box_ptr = blk.call( @@ -60,10 +58,7 @@ pub(crate) fn emit_grow_mutator_writeback( // directly from a nested fn) — fall through to the global store. } if let Some(&capture_idx) = ctx.closure_captures.get(&array_id) { - let closure_ptr = ctx - .current_closure_ptr - .clone() - .ok_or_else(|| anyhow::anyhow!("unshift capture but no current_closure_ptr"))?; + let closure_ptr = crate::expr::current_closure_ptr_value(ctx, "unshift capture")?; let idx_str = capture_idx.to_string(); let new_bits = ctx.block().bitcast_double_to_i64(new_box); ctx.block().call_void( diff --git a/crates/perry/tests/gc_closure_self_pointer_root_7055.rs b/crates/perry/tests/gc_closure_self_pointer_root_7055.rs new file mode 100644 index 0000000000..16114cb967 --- /dev/null +++ b/crates/perry/tests/gc_closure_self_pointer_root_7055.rs @@ -0,0 +1,186 @@ +//! #7055 — the relocating young collection must not invalidate a closure's +//! own `this_closure` pointer. +//! +//! A closure body reaches its captured variables through +//! `js_closure_get_capture_bits(%this_closure, idx)`, where `%this_closure` is +//! the LLVM parameter the caller passed — a register value no root enumeration +//! can see. The shipped default runs an evacuating young collection at loop +//! back-edge polls (`js_gc_loop_safepoint`) with PRECISE roots and no +//! conservative native-stack scan, so a closure relocated while its own body is +//! on the stack leaves that register pointing into from-space. From-space is +//! reset at the end of the same cycle and immediately handed back to the +//! mutator, so the next capture access reads a *different* object's header: +//! `js_closure_get_capture_bits` finds the index beyond that object's +//! `capture_count` and returns **0**. Box pointer 0 is not a registered box, so +//! every later boxed-capture read yields `undefined` and every boxed-capture +//! write is silently dropped. +//! +//! In an `async fn` the casualty is the generator state machine itself. Its +//! `__gen_state` local is a boxed capture, so the `state = ` store at the +//! end of a resumed state body went nowhere; the following `await` then resumed +//! into the state it had just finished and **ran one loop iteration twice**. +//! That is #7055's "deterministic wrong answer in the shipped default": a +//! checksum off by exactly one request's fold, the same constant at every +//! workload length, only when `copied_objects > 0`. +//! +//! The program below is the reduced form of the issue's `w5_srv_scale.ts`. +//! Every arm must reproduce node 26.5.0 exactly: +//! +//! ```text +//! $ node --experimental-strip-types main.ts +//! calls:150 +//! checksum:-342133776 +//! ``` +//! +//! # Why the arms are a nursery-cap sweep +//! +//! Whether a *relocating* minor lands inside the state-machine body at all +//! depends on where the nursery happens to fill, so any single configuration is +//! a coin flip on a given host — precisely the "passes either way" trap. The +//! sweep removes the coin flip: `PERRY_GC_SCAVENGE_NURSERY_MB` is a pacing knob +//! only (it moves the trigger threshold, it does not change what the collector +//! does), so one binary run under a spread of caps puts a relocating minor at +//! many different points of the program. Every arm must be node-exact; the test +//! is red if **any** of them is not. +//! +//! Measured against the unfixed compiler on this program: caps 1, 2 and 8 MB +//! print `calls:151`, caps 4 and 16 MB and the default do not. With the fix all +//! of them are node-exact. `PERRY_GEN_GC=0` (full mark-sweep, never moves) is +//! the control — correct before and after, so a green run cannot be inertness. + +use std::path::PathBuf; +use std::process::Command; + +fn perry_bin() -> PathBuf { + PathBuf::from(env!("CARGO_BIN_EXE_perry")) +} + +/// Reduced `w5_srv_scale.ts`: an event-loop request pump. `handle` allocates +/// enough per request to trip a relocating minor at a loop back-edge poll while +/// the async function's state-machine closure is suspended one frame up, and +/// the poll is followed by boxed-capture accesses (`__gen_state` among them). +const SOURCE: &str = r#" +let calls = 0; +let checksum = 0; + +function handle(req: number): number { + const rows: any[] = []; + for (let i = 0; i < 1500; i++) { + rows.push({ + id: req * 1500 + i, + name: "row-" + i + "-" + req, + tags: ["a" + (i & 15), "b" + (i & 7), "c" + (i % 5)], + score: (i * 7919 + req * 104729) % 1000, + }); + } + let acc = 0; + for (let i = 0; i < rows.length; i++) { + const row = rows[i]; + acc = (acc + row.id + row.score + row.name.length + row.tags[0].length) | 0; + } + return acc; +} + +async function main(): Promise { + for (let r = 0; r < 150; r++) { + calls = calls + 1; + checksum = (checksum + handle(r)) | 0; + await new Promise((resolve) => { setImmediate(resolve); }); + } + console.log("calls:" + calls); + console.log("checksum:" + checksum); +} + +main(); +"#; + +/// node 26.5.0 (`.node-version`) on the program above. +const NODE_ORACLE: &str = "calls:150\nchecksum:-342133776\n"; + +#[test] +fn relocating_minor_does_not_replay_an_async_loop_iteration() { + let dir = tempfile::tempdir().expect("tempdir"); + let entry = dir.path().join("main.ts"); + let output = dir.path().join("main_bin"); + std::fs::write(&entry, SOURCE).expect("write entry"); + + let compile = Command::new(perry_bin()) + .current_dir(dir.path()) + .arg("compile") + .arg(&entry) + .arg("-o") + .arg(&output) + .arg("--no-cache") + .output() + .expect("run perry compile"); + assert!( + compile.status.success(), + "perry compile failed\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&compile.stdout), + String::from_utf8_lossy(&compile.stderr) + ); + + // One binary, many collector configurations: `PERRY_GC_SCAVENGE_NURSERY_MB` + // and `PERRY_GEN_GC` are runtime-only knobs, so every arm runs exactly the + // same generated code. + let mut arms: Vec> = vec![vec![]]; + for mb in ["1", "2", "3", "4", "5", "8", "12", "16"] { + arms.push(vec![("PERRY_GC_SCAVENGE_NURSERY_MB", mb)]); + } + arms.push(vec![("PERRY_GEN_GC", "0")]); + + // The test runner's own environment is inherited by `Command`, so a + // developer (or a bisect script) exporting `PERRY_GEN_GC=0` — or any other + // collector kill switch — would silently turn every arm into the + // never-relocates control and the suite would pass against the unfixed + // compiler. Clear the whole family first, then apply only this arm's own + // settings. + const GC_ENV_OVERRIDES: &[&str] = &[ + "PERRY_GEN_GC", + "PERRY_GEN_GC_EVACUATE", + "PERRY_GC_SCAVENGE", + "PERRY_GC_SCAVENGE_NURSERY_MB", + "PERRY_GC_MOVING_SAFEPOINT", + "PERRY_GC_MOVING_LOOP_POLLS", + "PERRY_GC_FORCE_EVACUATE", + "PERRY_CONSERVATIVE_STACK_SCAN", + "PERRY_WRITE_BARRIERS", + "PERRY_GC_INCREMENTAL", + "PERRY_GC_HEAP_LIMIT", + ]; + + for arm in &arms { + let mut cmd = Command::new(&output); + cmd.current_dir(dir.path()); + for key in GC_ENV_OVERRIDES { + cmd.env_remove(key); + } + for (k, v) in arm { + cmd.env(k, v); + } + let run = cmd.output().expect("run compiled binary"); + let label = if arm.is_empty() { + "default".to_string() + } else { + arm.iter() + .map(|(k, v)| format!("{k}={v}")) + .collect::>() + .join(" ") + }; + assert!( + run.status.success(), + "[{label}] compiled binary failed (exit {:?})\nstderr:\n{}", + run.status.code(), + String::from_utf8_lossy(&run.stderr) + ); + assert_eq!( + String::from_utf8_lossy(&run.stdout), + NODE_ORACLE, + "[{label}] the async loop body must run exactly once per iteration \ + and fold the node-oracle checksum. `calls:151` means a relocating \ + minor invalidated the state-machine closure's own `this_closure` \ + pointer mid-body, so the `__gen_state` store was dropped and the \ + next `await` resumed into the state that had just finished (#7055)." + ); + } +}