From 784f9f131c3c6d9b1220541367c67ab64e2f3907 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Wed, 29 Jul 2026 08:48:00 +0200 Subject: [PATCH 01/15] fix(gc): root Map/Set collection-method operands across allocating operands (#6970) --- crates/perry-codegen/src/expr/math_simple.rs | 92 ++++++- crates/perry-codegen/src/expr/temp_root.rs | 87 +++++++ .../src/lower_call/property_get/map_set.rs | 231 +++++++++++------- 3 files changed, 308 insertions(+), 102 deletions(-) diff --git a/crates/perry-codegen/src/expr/math_simple.rs b/crates/perry-codegen/src/expr/math_simple.rs index 9cc593a92d..ae71d70c29 100644 --- a/crates/perry-codegen/src/expr/math_simple.rs +++ b/crates/perry-codegen/src/expr/math_simple.rs @@ -8,6 +8,7 @@ use anyhow::Result; use perry_hir::types::Type as HirType; use perry_hir::{BinaryOp, Expr}; +use crate::expr::temp_root; use crate::type_analysis::{is_definitely_string_expr, is_numeric_expr, map_static_type_args}; use crate::types::{DOUBLE, F32, I1, I32, I64}; @@ -264,6 +265,33 @@ fn guarded_map_number_key_set( ) } +/// Re-read the `MapSet` receiver + key after `value` has been lowered (#6970). +/// +/// Every `Expr::MapSet` branch lowers `value` before it touches the receiver +/// handle or the key, and that lowering is the collection point. On the +/// protected path this hands back values read out of their temp-root slots — +/// mandatory, since an evacuating cycle rewrites the slot in place — and +/// derives the receiver handle from the re-read box. On the unprotected path +/// `RootedOperands::reread` returns the original registers and +/// `m_handle_unrooted` is the eagerly computed handle, so nothing is emitted. +fn reread_map_set_receiver_and_key( + ctx: &mut FnCtx<'_>, + roots: &temp_root::RootedOperands, + m_handle_unrooted: &Option, +) -> (String, String) { + let values = roots.reread(ctx); + let k_box = values[1].clone(); + let m_handle = match m_handle_unrooted { + Some(handle) => handle.clone(), + None => { + let m_box = values[0].clone(); + let blk = ctx.block(); + unbox_to_i64(blk, &m_box) + } + }; + (m_handle, k_box) +} + fn guarded_map_number_key_get(ctx: &mut FnCtx<'_>, map_handle: &str, key_box: &str) -> String { let guard_raw = ctx .block() @@ -523,13 +551,29 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { && is_definitely_string_expr(ctx, value); let m_box = lower_expr(ctx, map)?; let k_box = lower_expr(ctx, key)?; - let m_handle = { + // #6970: the receiver and the key are finished, but every branch + // below lowers `value` next, and that lowering can collect. Until + // the runtime call both live only in SSA registers, so a collection + // there sweeps them: `m.set(fresh(k), churn(N))` aborted inside + // `js_map_set` on a key whose header had been recycled. + let roots = temp_root::root_operands( + ctx, + &[&m_box, &k_box], + temp_root::expr_may_trigger_gc(value), + ); + // Unbox eagerly only on the unprotected path, so its IR — including + // register numbering — is exactly what it was before this change. + // On the protected path the handle has to come from the *re-read* + // box, so it is derived after `value` is lowered instead. + let m_handle_unrooted = (!roots.is_rooted()).then(|| { let blk = ctx.block(); unbox_to_i64(blk, &m_box) - }; + }); let new_handle = if use_string_i32_map { let value_i32 = lower_expr_native(ctx, value, crate::native_value::ExpectedNativeRep::I32)?; + let (m_handle, k_box) = + reread_map_set_receiver_and_key(ctx, &roots, &m_handle_unrooted); let (k_handle, new_handle) = { let blk = ctx.block(); let k_handle = unbox_str_handle(blk, &k_box); @@ -561,6 +605,8 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { } else if use_string_u32_map { let value_u32 = lower_expr_native(ctx, value, crate::native_value::ExpectedNativeRep::U32)?; + let (m_handle, k_box) = + reread_map_set_receiver_and_key(ctx, &roots, &m_handle_unrooted); let (k_handle, new_handle) = { let blk = ctx.block(); let k_handle = unbox_str_handle(blk, &k_box); @@ -592,6 +638,8 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { } else if use_string_f32_map { let value_f32 = lower_expr_native(ctx, value, crate::native_value::ExpectedNativeRep::F32)?; + let (m_handle, k_box) = + reread_map_set_receiver_and_key(ctx, &roots, &m_handle_unrooted); let (k_handle, new_handle) = { let blk = ctx.block(); let k_handle = unbox_str_handle(blk, &k_box); @@ -622,6 +670,8 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { new_handle } else if use_string_number_map { let v_box = lower_expr(ctx, value)?; + let (m_handle, k_box) = + reread_map_set_receiver_and_key(ctx, &roots, &m_handle_unrooted); let (k_handle, new_handle) = { let blk = ctx.block(); let k_handle = unbox_str_handle(blk, &k_box); @@ -644,6 +694,8 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { } else if use_string_boolean_map { let value_i1 = lower_expr_native(ctx, value, crate::native_value::ExpectedNativeRep::I1)?; + let (m_handle, k_box) = + reread_map_set_receiver_and_key(ctx, &roots, &m_handle_unrooted); let (k_handle, new_handle) = { let blk = ctx.block(); let k_handle = unbox_str_handle(blk, &k_box); @@ -675,6 +727,8 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { new_handle } else if use_string_string_map { let v_box = lower_expr(ctx, value)?; + let (m_handle, k_box) = + reread_map_set_receiver_and_key(ctx, &roots, &m_handle_unrooted); let (k_handle, v_handle, new_handle) = { let blk = ctx.block(); let k_handle = unbox_str_handle(blk, &k_box); @@ -707,6 +761,8 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { new_handle } else if has_string_key_map { let v_box = lower_expr(ctx, value)?; + let (m_handle, k_box) = + reread_map_set_receiver_and_key(ctx, &roots, &m_handle_unrooted); let (k_handle, new_handle) = { let blk = ctx.block(); let k_handle = unbox_str_handle(blk, &k_box); @@ -740,6 +796,8 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { new_handle } else if use_number_string_map { let v_box = lower_expr(ctx, value)?; + let (m_handle, k_box) = + reread_map_set_receiver_and_key(ctx, &roots, &m_handle_unrooted); let (v_handle, v_slot_box) = { let blk = ctx.block(); let v_handle = unbox_str_handle(blk, &v_box); @@ -760,6 +818,8 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { guarded_map_number_key_set(ctx, &m_handle, &k_box, &v_slot_box) } else if use_number_key_map { let v_box = lower_expr(ctx, value)?; + let (m_handle, k_box) = + reread_map_set_receiver_and_key(ctx, &roots, &m_handle_unrooted); if static_number_string_map { record_collection_typed_value_fallback( ctx, @@ -775,6 +835,8 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { guarded_map_number_key_set(ctx, &m_handle, &k_box, &v_box) } else { let v_box = lower_expr(ctx, value)?; + let (m_handle, k_box) = + reread_map_set_receiver_and_key(ctx, &roots, &m_handle_unrooted); let new_handle = { let blk = ctx.block(); blk.call( @@ -794,6 +856,9 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { ); new_handle }; + // Released only now: the runtime call above allocates while it + // reads the key, so the group has to stay rooted across it. + roots.release(ctx); // map.set returns the (possibly-realloc'd) map. Re-NaN-box // and return. The caller may need to write this back to a // local; that's the caller's problem if Map is held in a @@ -807,13 +872,14 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { let use_number_key_map = !use_string_key_map && is_static_number_key_map(ctx, map) && is_numeric_expr(ctx, key); - let m_box = lower_expr(ctx, map)?; - let k_box = lower_expr(ctx, key)?; + // #6970: `key` is lowered after the receiver and can collect, so the + // receiver would otherwise sit unrooted in an SSA register across it. + let (m_box, k_box, guard) = temp_root::lower_operand_pair_rooted(ctx, map, key)?; let m_handle = { let blk = ctx.block(); unbox_to_i64(blk, &m_box) }; - if use_string_key_map { + let value = if use_string_key_map { let (k_handle, value) = { let blk = ctx.block(); let k_handle = unbox_str_handle(blk, &k_box); @@ -832,9 +898,9 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { "map", "js_map_get_string_key", ); - Ok(value) + value } else if use_number_key_map { - Ok(guarded_map_number_key_get(ctx, &m_handle, &k_box)) + guarded_map_number_key_get(ctx, &m_handle, &k_box) } else { let value = { let blk = ctx.block(); @@ -849,8 +915,10 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { "js_map_get", "receiver_or_key_not_static_string", ); - Ok(value) - } + value + }; + temp_root::temp_root_release(ctx, guard); + Ok(value) } Expr::MapHas { map, key } => { let use_string_key_map = @@ -858,8 +926,9 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { let use_number_key_map = !use_string_key_map && is_static_number_key_map(ctx, map) && is_numeric_expr(ctx, key); - let m_box = lower_expr(ctx, map)?; - let k_box = lower_expr(ctx, key)?; + // #6970: same hazard as `MapGet` — the key's lowering can collect + // while the receiver is live only in an SSA register. + let (m_box, k_box, guard) = temp_root::lower_operand_pair_rooted(ctx, map, key)?; let m_handle = { let blk = ctx.block(); unbox_to_i64(blk, &m_box) @@ -902,6 +971,7 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { ); i32_v }; + temp_root::temp_root_release(ctx, guard); // NaN-tagged boolean for "true"/"false" printing. let blk = ctx.block(); let bit = blk.icmp_ne(I32, &i32_v, "0"); diff --git a/crates/perry-codegen/src/expr/temp_root.rs b/crates/perry-codegen/src/expr/temp_root.rs index 85c32bfd88..e9bdf53b93 100644 --- a/crates/perry-codegen/src/expr/temp_root.rs +++ b/crates/perry-codegen/src/expr/temp_root.rs @@ -261,6 +261,93 @@ pub(crate) fn lower_operand_pair_rooted( Ok((left_value, right_value, guard)) } +/// Already-lowered operand values kept alive across work whose shape the +/// caller controls — a later operand whose *representation* is chosen per +/// branch (`Expr::MapSet`, #6970) or an allocation that happens after the whole +/// list is lowered (`new C(a, b)`, #6969). +/// +/// [`lower_exprs_rooted`] cannot serve those: it decides what to protect from +/// the expressions it is handed and re-reads immediately, whereas these sites +/// need the re-read to happen *after* a step the helper never sees. So the +/// caller supplies the protection decision and picks the re-read point. +/// +/// When `protect` is false this emits nothing at all and [`RootedOperands::reread`] +/// hands the original registers straight back, so unprotected sites keep their +/// pre-#6951 IR byte for byte. +pub(crate) struct RootedOperands { + /// Slot index per operand, or `None` when the operand was not rooted. + slots: Vec>, + /// The registers as originally lowered — the answer when nothing is rooted. + values: Vec, + /// First slot pushed; truncating it drops the whole group. + guard: Option, +} + +/// Root each of `values` (NaN-boxed `double` registers) when `protect` says +/// something between here and the consuming call can collect. +/// +/// `protect` is the caller's judgement precisely because the hazard is not +/// visible in an expression list: for `m.set(k, v)` it is `v`'s lowering, for +/// `new C(a, b)` it is the instance allocation. +pub(crate) fn root_operands( + ctx: &mut FnCtx<'_>, + values: &[&str], + protect: bool, +) -> RootedOperands { + let mut slots = Vec::with_capacity(values.len()); + let mut guard: Option = None; + for value in values { + if protect { + let idx = temp_root_push_double(ctx, value); + if guard.is_none() { + guard = Some(idx.clone()); + } + slots.push(Some(idx)); + } else { + slots.push(None); + } + } + RootedOperands { + slots, + values: values.iter().map(|v| (*v).to_string()).collect(), + guard, + } +} + +impl RootedOperands { + /// Re-read every rooted operand. Mandatory after the collection point, not + /// defensive: the slot is a *mutable* root, so an evacuating cycle rewrites + /// it and the register pushed beforehand is stale. + /// + /// Emits nothing when nothing was rooted. + pub(crate) fn reread(&self, ctx: &mut FnCtx<'_>) -> Vec { + self.slots + .iter() + .zip(self.values.iter()) + .map(|(slot, original)| match slot { + Some(idx) => { + let idx = idx.clone(); + temp_root_get_double(ctx, &idx) + } + None => original.clone(), + }) + .collect() + } + + /// True when this group actually pushed slots — the signal a caller uses to + /// keep an eager unbox (and therefore its exact register numbering) on the + /// unprotected path. + pub(crate) fn is_rooted(&self) -> bool { + self.guard.is_some() + } + + /// Drop the group. Call it *after* the consuming call: the consumer + /// allocates while reading these values. + pub(crate) fn release(self, ctx: &mut FnCtx<'_>) { + temp_root_release(ctx, self.guard); + } +} + /// Release a guard returned by [`lower_exprs_rooted`]. Call it *after* the /// consuming call, not before: the consumer allocates while reading these /// values. diff --git a/crates/perry-codegen/src/lower_call/property_get/map_set.rs b/crates/perry-codegen/src/lower_call/property_get/map_set.rs index a403503acc..e523a57e06 100644 --- a/crates/perry-codegen/src/lower_call/property_get/map_set.rs +++ b/crates/perry-codegen/src/lower_call/property_get/map_set.rs @@ -4,7 +4,7 @@ use anyhow::Result; use perry_hir::Expr; -use crate::expr::{lower_expr, unbox_to_i64, FnCtx}; +use crate::expr::{lower_expr, temp_root, unbox_to_i64, FnCtx}; use crate::nanbox::double_literal; use crate::type_analysis::{is_map_expr, is_set_expr, is_url_search_params_expr}; use crate::types::{DOUBLE, I64}; @@ -22,51 +22,70 @@ pub(crate) fn try_lower_map_set_methods( if is_map_expr(ctx, object) { match property { "set" if args.len() == 2 => { - let m_box = lower_expr(ctx, object)?; - let k_box = lower_expr(ctx, &args[0])?; - let v_box = lower_expr(ctx, &args[1])?; - let blk = ctx.block(); - let m_handle = unbox_to_i64(blk, &m_box); - blk.call_void( - "js_map_set", - &[(I64, &m_handle), (DOUBLE, &k_box), (DOUBLE, &v_box)], - ); + // #6970: each finished operand is live in an SSA register + // across the ones that follow, and those can collect. + let (vals, guard) = + temp_root::lower_exprs_rooted(ctx, &[object, &args[0], &args[1]])?; + let (m_box, k_box, v_box) = (vals[0].clone(), vals[1].clone(), vals[2].clone()); + { + let blk = ctx.block(); + let m_handle = unbox_to_i64(blk, &m_box); + blk.call_void( + "js_map_set", + &[(I64, &m_handle), (DOUBLE, &k_box), (DOUBLE, &v_box)], + ); + } + temp_root::temp_root_release(ctx, guard); return Ok(Some(m_box)); } "get" if args.len() == 1 => { - let m_box = lower_expr(ctx, object)?; - let k_box = lower_expr(ctx, &args[0])?; - let blk = ctx.block(); - let m_handle = unbox_to_i64(blk, &m_box); - return Ok(Some(blk.call( - DOUBLE, - "js_map_get", - &[(I64, &m_handle), (DOUBLE, &k_box)], - ))); + // #6970: the argument's lowering can collect while the + // receiver is live only in an SSA register. + let (m_box, k_box, guard) = + temp_root::lower_operand_pair_rooted(ctx, object, &args[0])?; + let value = { + let blk = ctx.block(); + let m_handle = unbox_to_i64(blk, &m_box); + blk.call(DOUBLE, "js_map_get", &[(I64, &m_handle), (DOUBLE, &k_box)]) + }; + temp_root::temp_root_release(ctx, guard); + return Ok(Some(value)); } "has" if args.len() == 1 => { - let m_box = lower_expr(ctx, object)?; - let k_box = lower_expr(ctx, &args[0])?; - let blk = ctx.block(); - let m_handle = unbox_to_i64(blk, &m_box); - let i32_v = blk.call( - crate::types::I32, - "js_map_has", - &[(I64, &m_handle), (DOUBLE, &k_box)], - ); - return Ok(Some(crate::expr::i32_bool_to_nanbox(blk, &i32_v))); + // #6970: the argument's lowering can collect while the + // receiver is live only in an SSA register. + let (m_box, k_box, guard) = + temp_root::lower_operand_pair_rooted(ctx, object, &args[0])?; + let result = { + let blk = ctx.block(); + let m_handle = unbox_to_i64(blk, &m_box); + let i32_v = blk.call( + crate::types::I32, + "js_map_has", + &[(I64, &m_handle), (DOUBLE, &k_box)], + ); + crate::expr::i32_bool_to_nanbox(blk, &i32_v) + }; + temp_root::temp_root_release(ctx, guard); + return Ok(Some(result)); } "delete" if args.len() == 1 => { - let m_box = lower_expr(ctx, object)?; - let k_box = lower_expr(ctx, &args[0])?; - let blk = ctx.block(); - let m_handle = unbox_to_i64(blk, &m_box); - let i32_v = blk.call( - crate::types::I32, - "js_map_delete", - &[(I64, &m_handle), (DOUBLE, &k_box)], - ); - return Ok(Some(crate::expr::i32_bool_to_nanbox(blk, &i32_v))); + // #6970: the argument's lowering can collect while the + // receiver is live only in an SSA register. + let (m_box, k_box, guard) = + temp_root::lower_operand_pair_rooted(ctx, object, &args[0])?; + let result = { + let blk = ctx.block(); + let m_handle = unbox_to_i64(blk, &m_box); + let i32_v = blk.call( + crate::types::I32, + "js_map_delete", + &[(I64, &m_handle), (DOUBLE, &k_box)], + ); + crate::expr::i32_bool_to_nanbox(blk, &i32_v) + }; + temp_root::temp_root_release(ctx, guard); + return Ok(Some(result)); } "clear" if args.is_empty() => { let m_box = lower_expr(ctx, object)?; @@ -112,36 +131,53 @@ pub(crate) fn try_lower_map_set_methods( if is_set_expr(ctx, object) { match property { "add" if args.len() == 1 => { - let s_box = lower_expr(ctx, object)?; - let v_box = lower_expr(ctx, &args[0])?; - let blk = ctx.block(); - let s_handle = unbox_to_i64(blk, &s_box); - blk.call_void("js_set_add", &[(I64, &s_handle), (DOUBLE, &v_box)]); + // #6970: the argument's lowering can collect while the + // receiver is live only in an SSA register. + let (s_box, v_box, guard) = + temp_root::lower_operand_pair_rooted(ctx, object, &args[0])?; + { + let blk = ctx.block(); + let s_handle = unbox_to_i64(blk, &s_box); + blk.call_void("js_set_add", &[(I64, &s_handle), (DOUBLE, &v_box)]); + } + temp_root::temp_root_release(ctx, guard); return Ok(Some(s_box)); } "has" if args.len() == 1 => { - let s_box = lower_expr(ctx, object)?; - let v_box = lower_expr(ctx, &args[0])?; - let blk = ctx.block(); - let s_handle = unbox_to_i64(blk, &s_box); - let i32_v = blk.call( - crate::types::I32, - "js_set_has", - &[(I64, &s_handle), (DOUBLE, &v_box)], - ); - return Ok(Some(crate::expr::i32_bool_to_nanbox(blk, &i32_v))); + // #6970: the argument's lowering can collect while the + // receiver is live only in an SSA register. + let (s_box, v_box, guard) = + temp_root::lower_operand_pair_rooted(ctx, object, &args[0])?; + let result = { + let blk = ctx.block(); + let s_handle = unbox_to_i64(blk, &s_box); + let i32_v = blk.call( + crate::types::I32, + "js_set_has", + &[(I64, &s_handle), (DOUBLE, &v_box)], + ); + crate::expr::i32_bool_to_nanbox(blk, &i32_v) + }; + temp_root::temp_root_release(ctx, guard); + return Ok(Some(result)); } "delete" if args.len() == 1 => { - let s_box = lower_expr(ctx, object)?; - let v_box = lower_expr(ctx, &args[0])?; - let blk = ctx.block(); - let s_handle = unbox_to_i64(blk, &s_box); - let i32_v = blk.call( - crate::types::I32, - "js_set_delete", - &[(I64, &s_handle), (DOUBLE, &v_box)], - ); - return Ok(Some(crate::expr::i32_bool_to_nanbox(blk, &i32_v))); + // #6970: the argument's lowering can collect while the + // receiver is live only in an SSA register. + let (s_box, v_box, guard) = + temp_root::lower_operand_pair_rooted(ctx, object, &args[0])?; + let result = { + let blk = ctx.block(); + let s_handle = unbox_to_i64(blk, &s_box); + let i32_v = blk.call( + crate::types::I32, + "js_set_delete", + &[(I64, &s_handle), (DOUBLE, &v_box)], + ); + crate::expr::i32_bool_to_nanbox(blk, &i32_v) + }; + temp_root::temp_root_release(ctx, guard); + return Ok(Some(result)); } "clear" if args.is_empty() => { let s_box = lower_expr(ctx, object)?; @@ -184,37 +220,50 @@ pub(crate) fn try_lower_map_set_methods( // a boolean. The runtime fns receive the receiver as an I64 set // handle and `other` as a NaN-boxed f64. "union" | "intersection" | "difference" | "symmetricDifference" if args.len() == 1 => { - let s_box = lower_expr(ctx, object)?; - let other_box = lower_expr(ctx, &args[0])?; - let blk = ctx.block(); - let s_handle = unbox_to_i64(blk, &s_box); - let runtime_fn = match property { - "union" => "js_set_union", - "intersection" => "js_set_intersection", - "difference" => "js_set_difference", - "symmetricDifference" => "js_set_symmetric_difference", - _ => unreachable!(), + // #6970: the argument's lowering can collect while the + // receiver is live only in an SSA register. + let (s_box, other_box, guard) = + temp_root::lower_operand_pair_rooted(ctx, object, &args[0])?; + let boxed = { + let blk = ctx.block(); + let s_handle = unbox_to_i64(blk, &s_box); + let runtime_fn = match property { + "union" => "js_set_union", + "intersection" => "js_set_intersection", + "difference" => "js_set_difference", + "symmetricDifference" => "js_set_symmetric_difference", + _ => unreachable!(), + }; + let result = + blk.call(I64, runtime_fn, &[(I64, &s_handle), (DOUBLE, &other_box)]); + crate::expr::nanbox_pointer_inline_pub(blk, &result) }; - let result = blk.call(I64, runtime_fn, &[(I64, &s_handle), (DOUBLE, &other_box)]); - return Ok(Some(crate::expr::nanbox_pointer_inline_pub(blk, &result))); + temp_root::temp_root_release(ctx, guard); + return Ok(Some(boxed)); } "isSubsetOf" | "isSupersetOf" | "isDisjointFrom" if args.len() == 1 => { - let s_box = lower_expr(ctx, object)?; - let other_box = lower_expr(ctx, &args[0])?; - let blk = ctx.block(); - let s_handle = unbox_to_i64(blk, &s_box); - let runtime_fn = match property { - "isSubsetOf" => "js_set_is_subset_of", - "isSupersetOf" => "js_set_is_superset_of", - "isDisjointFrom" => "js_set_is_disjoint_from", - _ => unreachable!(), + // #6970: the argument's lowering can collect while the + // receiver is live only in an SSA register. + let (s_box, other_box, guard) = + temp_root::lower_operand_pair_rooted(ctx, object, &args[0])?; + let result = { + let blk = ctx.block(); + let s_handle = unbox_to_i64(blk, &s_box); + let runtime_fn = match property { + "isSubsetOf" => "js_set_is_subset_of", + "isSupersetOf" => "js_set_is_superset_of", + "isDisjointFrom" => "js_set_is_disjoint_from", + _ => unreachable!(), + }; + let i32_v = blk.call( + crate::types::I32, + runtime_fn, + &[(I64, &s_handle), (DOUBLE, &other_box)], + ); + crate::expr::i32_bool_to_nanbox(blk, &i32_v) }; - let i32_v = blk.call( - crate::types::I32, - runtime_fn, - &[(I64, &s_handle), (DOUBLE, &other_box)], - ); - return Ok(Some(crate::expr::i32_bool_to_nanbox(blk, &i32_v))); + temp_root::temp_root_release(ctx, guard); + return Ok(Some(result)); } _ => {} } From 9b5fe36e92a1911cdd924b3c0bfa2e5d6580dd67 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Wed, 29 Jul 2026 08:48:27 +0200 Subject: [PATCH 02/15] fix(gc): root collection forEach receiver across callback lowering (#6970) --- .../src/lower_call/property_get/map_set.rs | 108 +++++++++++------- 1 file changed, 69 insertions(+), 39 deletions(-) diff --git a/crates/perry-codegen/src/lower_call/property_get/map_set.rs b/crates/perry-codegen/src/lower_call/property_get/map_set.rs index e523a57e06..10cd6530d9 100644 --- a/crates/perry-codegen/src/lower_call/property_get/map_set.rs +++ b/crates/perry-codegen/src/lower_call/property_get/map_set.rs @@ -287,37 +287,57 @@ pub(crate) fn try_lower_collection_foreach( // with the full `(value, key, collection)` triple. Map.forEach // returns `undefined`. if is_map_expr(ctx, object) { - let m_box = lower_expr(ctx, object)?; - let cb_box = lower_expr(ctx, &args[0])?; - let this_arg = if args.len() >= 2 { - lower_expr(ctx, &args[1])? - } else { - double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED)) - }; - let blk = ctx.block(); - let m_handle = unbox_to_i64(blk, &m_box); - blk.call_void( - "js_map_foreach", - &[(I64, &m_handle), (DOUBLE, &cb_box), (DOUBLE, &this_arg)], - ); + // #6970: the callback (a closure allocation) and the optional + // `thisArg` are lowered after the receiver, so the receiver would + // otherwise be live only in an SSA register across them. + let mut operands: Vec<&Expr> = vec![object, &args[0]]; + if args.len() >= 2 { + operands.push(&args[1]); + } + let (vals, guard) = temp_root::lower_exprs_rooted(ctx, &operands)?; + let m_box = vals[0].clone(); + let cb_box = vals[1].clone(); + let this_arg = vals + .get(2) + .cloned() + .unwrap_or_else(|| double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED))); + { + let blk = ctx.block(); + let m_handle = unbox_to_i64(blk, &m_box); + blk.call_void( + "js_map_foreach", + &[(I64, &m_handle), (DOUBLE, &cb_box), (DOUBLE, &this_arg)], + ); + } + temp_root::temp_root_release(ctx, guard); return Ok(Some(double_literal(f64::from_bits( crate::nanbox::TAG_UNDEFINED, )))); } if is_set_expr(ctx, object) { - let s_box = lower_expr(ctx, object)?; - let cb_box = lower_expr(ctx, &args[0])?; - let this_arg = if args.len() >= 2 { - lower_expr(ctx, &args[1])? - } else { - double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED)) - }; - let blk = ctx.block(); - let s_handle = unbox_to_i64(blk, &s_box); - blk.call_void( - "js_set_foreach", - &[(I64, &s_handle), (DOUBLE, &cb_box), (DOUBLE, &this_arg)], - ); + // #6970: the callback (a closure allocation) and the optional + // `thisArg` are lowered after the receiver, so the receiver would + // otherwise be live only in an SSA register across them. + let mut operands: Vec<&Expr> = vec![object, &args[0]]; + if args.len() >= 2 { + operands.push(&args[1]); + } + let (vals, guard) = temp_root::lower_exprs_rooted(ctx, &operands)?; + let s_box = vals[0].clone(); + let cb_box = vals[1].clone(); + let this_arg = vals + .get(2) + .cloned() + .unwrap_or_else(|| double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED))); + { + let blk = ctx.block(); + let s_handle = unbox_to_i64(blk, &s_box); + blk.call_void( + "js_set_foreach", + &[(I64, &s_handle), (DOUBLE, &cb_box), (DOUBLE, &this_arg)], + ); + } + temp_root::temp_root_release(ctx, guard); return Ok(Some(double_literal(f64::from_bits( crate::nanbox::TAG_UNDEFINED, )))); @@ -330,19 +350,29 @@ pub(crate) fn try_lower_collection_foreach( // runtime entry so the callback gets the string `(value, key)` // pair instead of `(NaN, 0)` from the Array.forEach fast path. if is_url_search_params_expr(ctx, object) { - let p_box = lower_expr(ctx, object)?; - let cb_box = lower_expr(ctx, &args[0])?; - let this_arg = if args.len() >= 2 { - lower_expr(ctx, &args[1])? - } else { - double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED)) - }; - let blk = ctx.block(); - let p_handle = unbox_to_i64(blk, &p_box); - blk.call_void( - "js_url_search_params_for_each", - &[(I64, &p_handle), (DOUBLE, &cb_box), (DOUBLE, &this_arg)], - ); + // #6970: the callback (a closure allocation) and the optional + // `thisArg` are lowered after the receiver, so the receiver would + // otherwise be live only in an SSA register across them. + let mut operands: Vec<&Expr> = vec![object, &args[0]]; + if args.len() >= 2 { + operands.push(&args[1]); + } + let (vals, guard) = temp_root::lower_exprs_rooted(ctx, &operands)?; + let p_box = vals[0].clone(); + let cb_box = vals[1].clone(); + let this_arg = vals + .get(2) + .cloned() + .unwrap_or_else(|| double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED))); + { + let blk = ctx.block(); + let p_handle = unbox_to_i64(blk, &p_box); + blk.call_void( + "js_url_search_params_for_each", + &[(I64, &p_handle), (DOUBLE, &cb_box), (DOUBLE, &this_arg)], + ); + } + temp_root::temp_root_release(ctx, guard); return Ok(Some(double_literal(0.0))); } } From 70042a9cb6b62573f61f85f709bf1089974c95f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Wed, 29 Jul 2026 08:54:41 +0200 Subject: [PATCH 03/15] fix(gc): root string-method receiver and concat accumulator (#6971) --- crates/perry-codegen/src/expr/temp_root.rs | 10 ++ .../perry-codegen/src/lower_string_method.rs | 125 ++++++++++++++++-- 2 files changed, 124 insertions(+), 11 deletions(-) diff --git a/crates/perry-codegen/src/expr/temp_root.rs b/crates/perry-codegen/src/expr/temp_root.rs index e9bdf53b93..0a5c04da5a 100644 --- a/crates/perry-codegen/src/expr/temp_root.rs +++ b/crates/perry-codegen/src/expr/temp_root.rs @@ -52,6 +52,16 @@ pub(crate) fn temp_root_get_double(ctx: &mut FnCtx<'_>, idx: &str) -> String { ctx.block().bitcast_i64_to_double(&bits) } +/// Overwrite slot `idx` with a new raw `i64`. +/// +/// For producers that hand back a *different* address each round — the +/// `concat` accumulator (#6971), where every `js_string_concat` yields a new +/// string and the old one stops being the value that must stay alive. +pub(crate) fn temp_root_set_i64(ctx: &mut FnCtx<'_>, idx: &str, value_i64: &str) { + ctx.block() + .call_void("js_gc_temp_root_set", &[(I32, idx), (I64, value_i64)]); +} + /// Drop slot `idx` and everything pushed above it. pub(crate) fn temp_root_truncate(ctx: &mut FnCtx<'_>, idx: &str) { ctx.block() diff --git a/crates/perry-codegen/src/lower_string_method.rs b/crates/perry-codegen/src/lower_string_method.rs index b9f17f405d..48fc9dcba6 100644 --- a/crates/perry-codegen/src/lower_string_method.rs +++ b/crates/perry-codegen/src/lower_string_method.rs @@ -8,8 +8,9 @@ use perry_hir::types::Type as HirType; use perry_hir::Expr; use crate::expr::temp_root::{ - lower_exprs_rooted, lower_operand_pair_rooted, temp_root_get_double, temp_root_get_i64, - temp_root_push_double, temp_root_push_i64, temp_root_release, temp_root_truncate, + self, lower_exprs_rooted, lower_operand_pair_rooted, temp_root_get_double, temp_root_get_i64, + temp_root_push_double, temp_root_push_i64, temp_root_release, temp_root_set_i64, + temp_root_truncate, }; use crate::expr::{ i32_bool_to_nanbox, lower_expr, nanbox_pointer_inline, nanbox_string_inline, unbox_str_handle, @@ -156,6 +157,51 @@ pub(crate) fn lower_string_method( nanbox_string_inline(blk, &coerced) }; + // #6971: the receiver is lowered BEFORE the arguments, and an argument's + // lowering can collect — `fresh(k).concat("|" + churn(N))` dropped the + // receiver, whose unboxed form is a BARE string address that only the + // `gc::root_words` bare form covers. Root it across the whole dispatch; + // the truncate below is the single release point for every one of the + // match's ~60 return paths. + let recv_root = args + .iter() + .any(temp_root::expr_may_trigger_gc) + .then(|| temp_root_push_double(ctx, &recv_box)); + let result = lower_string_method_dispatch(ctx, object, property, args, &recv_box, &recv_root); + // Released only after the dispatch's consuming runtime call has run: that + // call allocates while it reads the receiver. + if let Some(idx) = &recv_root { + temp_root_truncate(ctx, idx); + } + result +} + +/// Re-read the string-method receiver out of its temp root (#6971). +/// +/// Mandatory rather than defensive: the slot is a *mutable* root, so an +/// evacuating cycle rewrites it and the register pushed beforehand is stale. +/// Returns the original register when nothing was rooted, emitting no IR — so a +/// method whose arguments cannot collect keeps its previous code byte for byte. +fn reread_recv(ctx: &mut FnCtx<'_>, recv_root: &Option, recv_box: &str) -> String { + match recv_root { + Some(idx) => { + let idx = idx.clone(); + temp_root_get_double(ctx, &idx) + } + None => recv_box.to_string(), + } +} + +#[allow(clippy::too_many_lines)] +fn lower_string_method_dispatch( + ctx: &mut FnCtx<'_>, + object: &Expr, + property: &str, + args: &[Expr], + recv_box: &str, + recv_root: &Option, +) -> Result { + let recv_box = recv_box.to_string(); match property { "indexOf" => { if args.len() > 2 { @@ -182,6 +228,7 @@ pub(crate) fn lower_string_method( } else { None }; + let recv_box = reread_recv(ctx, recv_root, &recv_box); let blk = ctx.block(); let recv_handle = unbox_str_handle(blk, &recv_box); let needle_handle = if needle_is_str { @@ -230,6 +277,7 @@ pub(crate) fn lower_string_method( } else { None }; + let recv_box = reread_recv(ctx, recv_root, &recv_box); let blk = ctx.block(); let recv_handle = unbox_str_handle(blk, &recv_box); // String length (i32 at header offset 0). Used as the default end @@ -282,6 +330,7 @@ pub(crate) fn lower_string_method( // its coercion/undefined/RegExp checks for this common hot path. if args.len() == 1 && matches!(&args[0], Expr::String(_) | Expr::WtfString(_)) { let delim_box = lower_expr(ctx, &args[0])?; + let recv_box = reread_recv(ctx, recv_root, &recv_box); let blk = ctx.block(); let recv_handle = unbox_str_handle(blk, &recv_box); let delim_handle = unbox_str_handle(blk, &delim_box); @@ -310,6 +359,7 @@ pub(crate) fn lower_string_method( } else { None }; + let recv_box = reread_recv(ctx, recv_root, &recv_box); let blk = ctx.block(); let recv_handle = unbox_str_handle(blk, &recv_box); // No separator → pass `undefined`, which `js_string_split_value` @@ -354,6 +404,7 @@ pub(crate) fn lower_string_method( } else { Some(lower_expr(ctx, &args[0])?) }; + let recv_box = reread_recv(ctx, recv_root, &recv_box); let blk = ctx.block(); let locales_box = match locales_box { Some(v) => v, @@ -380,6 +431,7 @@ pub(crate) fn lower_string_method( for extra in args.iter() { let _ = lower_expr(ctx, extra)?; } + let recv_box = reread_recv(ctx, recv_root, &recv_box); let blk = ctx.block(); let recv_handle = unbox_str_handle(blk, &recv_box); let runtime_fn = match property { @@ -399,6 +451,7 @@ pub(crate) fn lower_string_method( for extra in args.iter() { let _ = lower_expr(ctx, extra)?; } + let recv_box = reread_recv(ctx, recv_root, &recv_box); let blk = ctx.block(); let recv_handle = unbox_str_handle(blk, &recv_box); let runtime_fn = match property { @@ -427,6 +480,7 @@ pub(crate) fn lower_string_method( for extra in args.iter().skip(1) { let _ = lower_expr(ctx, extra)?; } + let recv_box = reread_recv(ctx, recv_root, &recv_box); let blk = ctx.block(); let recv_handle = unbox_str_handle(blk, &recv_box); let value_handle = blk.call(I64, "js_string_coerce", &[(DOUBLE, &value_d)]); @@ -459,6 +513,7 @@ pub(crate) fn lower_string_method( for extra in args.iter().skip(1) { let _ = lower_expr(ctx, extra)?; } + let recv_box = reread_recv(ctx, recv_root, &recv_box); let blk = ctx.block(); let recv_handle = unbox_str_handle(blk, &recv_box); let idx_i32 = blk.call(I32, "js_string_index_to_i32", &[(DOUBLE, &idx_d)]); @@ -477,6 +532,7 @@ pub(crate) fn lower_string_method( ); } let count_d = lower_expr(ctx, &args[0])?; + let recv_box = reread_recv(ctx, recv_root, &recv_box); let blk = ctx.block(); let recv_handle = unbox_str_handle(blk, &recv_box); let result = blk.call( @@ -518,6 +574,7 @@ pub(crate) fn lower_string_method( let repl_is_str = is_string_expr(ctx, &args[1]); let needle_box = lower_expr(ctx, &args[0])?; let repl_box = lower_expr(ctx, &args[1])?; + let recv_box = reread_recv(ctx, recv_root, &recv_box); let blk = ctx.block(); let recv_handle = unbox_str_handle(blk, &recv_box); // #4871: a `searchValue` codegen can't type (an object-property @@ -641,6 +698,7 @@ pub(crate) fn lower_string_method( for extra in args.iter().skip(1) { let _ = lower_expr(ctx, extra)?; } + let recv_box = reread_recv(ctx, recv_root, &recv_box); let recv_handle = str_operand_handle_tag_dispatched(ctx, object, &recv_box); let blk = ctx.block(); let idx_i32 = blk.call(I32, "js_string_index_to_i32", &[(DOUBLE, &idx_d)]); @@ -662,6 +720,7 @@ pub(crate) fn lower_string_method( for extra in args.iter().skip(1) { let _ = lower_expr(ctx, extra)?; } + let recv_box = reread_recv(ctx, recv_root, &recv_box); let recv_handle = str_operand_handle_tag_dispatched(ctx, object, &recv_box); let blk = ctx.block(); let idx_i32 = blk.call(I32, "js_string_index_to_i32", &[(DOUBLE, &idx_d)]); @@ -683,6 +742,7 @@ pub(crate) fn lower_string_method( for extra in args.iter().skip(1) { let _ = lower_expr(ctx, extra)?; } + let recv_box = reread_recv(ctx, recv_root, &recv_box); let recv_handle = str_operand_handle_tag_dispatched(ctx, object, &recv_box); let blk = ctx.block(); let idx_i32 = blk.call(I32, "js_string_index_to_i32", &[(DOUBLE, &idx_d)]); @@ -720,6 +780,7 @@ pub(crate) fn lower_string_method( } else { None }; + let recv_box = reread_recv(ctx, recv_root, &recv_box); let blk = ctx.block(); let recv_handle = unbox_str_handle(blk, &recv_box); let needle_handle = if needle_is_str { @@ -788,6 +849,7 @@ pub(crate) fn lower_string_method( let sp_box = blk.load(DOUBLE, &sp_global); unbox_str_handle(blk, &sp_box) }; + let recv_box = reread_recv(ctx, recv_root, &recv_box); let blk = ctx.block(); let recv_handle = unbox_str_handle(blk, &recv_box); // Pass `target_length` as raw DOUBLE — the runtime does the @@ -824,6 +886,7 @@ pub(crate) fn lower_string_method( } form }; + let recv_box = reread_recv(ctx, recv_root, &recv_box); let blk = ctx.block(); let recv_handle = unbox_str_handle(blk, &recv_box); let result = blk.call( @@ -884,6 +947,7 @@ pub(crate) fn lower_string_method( &[(DOUBLE, loc), (DOUBLE, opts_ref)], ); } + let recv_box = reread_recv(ctx, recv_root, &recv_box); let blk = ctx.block(); let recv_handle = unbox_str_handle(blk, &recv_box); // A non-string `that` (undefined/number/object) must be @@ -926,6 +990,7 @@ pub(crate) fn lower_string_method( } else { crate::nanbox::double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED)) }; + let recv_box = reread_recv(ctx, recv_root, &recv_box); let blk = ctx.block(); let recv_handle = unbox_str_handle(blk, &recv_box); let i32_v = blk.call( @@ -950,6 +1015,7 @@ pub(crate) fn lower_string_method( } else { crate::nanbox::double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED)) }; + let recv_box = reread_recv(ctx, recv_root, &recv_box); let blk = ctx.block(); let recv_handle = unbox_str_handle(blk, &recv_box); let result = blk.call( @@ -979,6 +1045,7 @@ pub(crate) fn lower_string_method( } else { crate::nanbox::double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED)) }; + let recv_box = reread_recv(ctx, recv_root, &recv_box); let blk = ctx.block(); let recv_handle = unbox_str_handle(blk, &recv_box); let result = blk.call( @@ -994,6 +1061,7 @@ pub(crate) fn lower_string_method( for extra in args.iter() { let _ = lower_expr(ctx, extra)?; } + let recv_box = reread_recv(ctx, recv_root, &recv_box); let blk = ctx.block(); let recv_handle = unbox_str_handle(blk, &recv_box); // Returns a NaN-tagged boolean directly. @@ -1004,6 +1072,7 @@ pub(crate) fn lower_string_method( for extra in args.iter() { let _ = lower_expr(ctx, extra)?; } + let recv_box = reread_recv(ctx, recv_root, &recv_box); let blk = ctx.block(); let recv_handle = unbox_str_handle(blk, &recv_box); let result = blk.call(I64, "js_string_to_well_formed", &[(I64, &recv_handle)]); @@ -1015,24 +1084,54 @@ pub(crate) fn lower_string_method( // arg (`undefined`, a boolean, a `{ toString }` object) must render // as its string form, not be bit-cast as a string handle (which // dropped `undefined`/booleans). A static string arg skips coercion. - let blk = ctx.block(); - let mut acc_handle = unbox_str_handle(blk, &recv_box); + // #6971: unlike every other arm, `concat` unboxes the receiver + // BEFORE it lowers its arguments, then keeps threading the running + // accumulator through an SSA register across each one. That + // accumulator is a bare `StringHeader*`, so the generic + // `reread_recv` above cannot help: it has to be rooted in its own + // right and written back after every concat, because each iteration + // produces a NEW address. + let mut acc_handle = { + let blk = ctx.block(); + unbox_str_handle(blk, &recv_box) + }; + let acc_root = args + .iter() + .any(temp_root::expr_may_trigger_gc) + .then(|| temp_root_push_i64(ctx, &acc_handle)); for a in args { let a_is_str = is_string_expr(ctx, a); let s_box = lower_expr(ctx, a)?; - let blk = ctx.block(); - let s_handle = if a_is_str { - unbox_str_handle(blk, &s_box) - } else { - blk.call(I64, "js_string_coerce", &[(DOUBLE, &s_box)]) + // The ToString coercion allocates too, so re-read only after it. + let s_handle = { + let blk = ctx.block(); + if a_is_str { + unbox_str_handle(blk, &s_box) + } else { + blk.call(I64, "js_string_coerce", &[(DOUBLE, &s_box)]) + } }; - acc_handle = blk.call( + if let Some(idx) = &acc_root { + let idx = idx.clone(); + acc_handle = temp_root_get_i64(ctx, &idx); + } + acc_handle = ctx.block().call( I64, "js_string_concat", &[(I64, &acc_handle), (I64, &s_handle)], ); + // Write the new accumulator back, so the NEXT argument's + // lowering keeps *this* string alive rather than its input. + if let Some(idx) = &acc_root { + let idx = idx.clone(); + temp_root_set_i64(ctx, &idx, &acc_handle); + } + } + let boxed = nanbox_string_inline(ctx.block(), &acc_handle); + if let Some(idx) = &acc_root { + temp_root_truncate(ctx, idx); } - Ok(nanbox_string_inline(ctx.block(), &acc_handle)) + Ok(boxed) } "substr" => { // Legacy substr(start, length) — distinct from substring/slice: @@ -1053,6 +1152,7 @@ pub(crate) fn lower_string_method( } else { None }; + let recv_box = reread_recv(ctx, recv_root, &recv_box); let blk = ctx.block(); let recv_handle = unbox_str_handle(blk, &recv_box); // Pass the raw NaN-boxed args straight through: `js_string_substr` @@ -1088,6 +1188,7 @@ pub(crate) fn lower_string_method( } else { None }; + let recv_box = reread_recv(ctx, recv_root, &recv_box); let blk = ctx.block(); let recv_handle = unbox_str_handle(blk, &recv_box); let method_id = regexp_search_method_id(property); @@ -1148,6 +1249,7 @@ pub(crate) fn lower_string_method( } else { None }; + let recv_box = reread_recv(ctx, recv_root, &recv_box); let blk = ctx.block(); let recv_handle = unbox_str_handle(blk, &recv_box); let method_id = regexp_search_method_id(property); @@ -1188,6 +1290,7 @@ pub(crate) fn lower_string_method( for a in args { let _ = lower_expr(ctx, a)?; } + let recv_box = reread_recv(ctx, recv_root, &recv_box); let blk = ctx.block(); let handle = blk.call(I64, "js_jsvalue_to_string", &[(DOUBLE, &recv_box)]); Ok(nanbox_string_inline(blk, &handle)) From eb641642ed11ef8af53e6694a1c71f764dc5f46f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Wed, 29 Jul 2026 08:58:46 +0200 Subject: [PATCH 04/15] fix(gc): root constructor arguments across the instance allocation (#6969) --- crates/perry-codegen/src/expr/temp_root.rs | 47 ++++++++++++++++++++++ crates/perry-codegen/src/lower_call/new.rs | 45 ++++++++++++++++++++- 2 files changed, 91 insertions(+), 1 deletion(-) diff --git a/crates/perry-codegen/src/expr/temp_root.rs b/crates/perry-codegen/src/expr/temp_root.rs index 0a5c04da5a..8d69239fff 100644 --- a/crates/perry-codegen/src/expr/temp_root.rs +++ b/crates/perry-codegen/src/expr/temp_root.rs @@ -417,3 +417,50 @@ pub(crate) fn any_may_trigger_gc<'a>( ) -> bool { exprs.into_iter().any(|e| expr_may_trigger_gc(ctx, e)) } + +/// Would `expr`'s lowered value need a temp root, assuming everything after it +/// reaches a collection point? +/// +/// Three suppressions, each meaning "already rooted, or nothing to root": +/// +/// - provably not a heap reference — a slot for it is pure TLS traffic; +/// - a string literal — a load from a module global `__perry_init_strings_*` +/// registered with `js_gc_register_global_root`; +/// - a plain local or module-global read — the shadow stack and the module-var +/// scanners already hold those for as long as generated code can see them. +/// +/// The last one is why `new C(a, b)` on plain locals stays at its old IR even +/// though the instance allocation that follows always collects. +pub(crate) fn operand_needs_root(ctx: &FnCtx<'_>, expr: &Expr) -> bool { + !super::expr_is_known_non_pointer_shadow_value(ctx, expr) + && !matches!( + expr, + Expr::String(_) | Expr::LocalGet(_) | Expr::GlobalGet(_) + ) +} + +/// Open an expression-scope temp-root barrier for a call/constructor whose +/// operands are `args`. +/// +/// Pushes a null marker slot and returns its index. Because +/// [`temp_root_truncate`] is a stack *cut*, [`temp_root_scope_end`] drops the +/// marker and every slot pushed above it — no matter which of the callee's +/// return paths ran. That is what makes rooting tractable in +/// `lower_call/new.rs`, where `lowered_args` is consumed at a dozen sites +/// spread over ~20 return paths (#6969); the alternative is a `temp_root_release` +/// at each, which is exactly the bookkeeping that gets missed. +/// +/// A null word decodes to nothing, so the marker itself roots no object. +/// Emits nothing when no operand could ever need rooting. +pub(crate) fn temp_root_scope_begin(ctx: &mut FnCtx<'_>, args: &[Expr]) -> Option { + args.iter() + .any(|a| operand_needs_root(ctx, a)) + .then(|| temp_root_push_i64(ctx, "0")) +} + +/// Close a barrier opened by [`temp_root_scope_begin`]. +pub(crate) fn temp_root_scope_end(ctx: &mut FnCtx<'_>, scope: Option) { + if let Some(idx) = scope { + temp_root_truncate(ctx, &idx); + } +} diff --git a/crates/perry-codegen/src/lower_call/new.rs b/crates/perry-codegen/src/lower_call/new.rs index 0c7f321eea..8320a52f7f 100644 --- a/crates/perry-codegen/src/lower_call/new.rs +++ b/crates/perry-codegen/src/lower_call/new.rs @@ -20,7 +20,7 @@ use super::new_helpers::{ ctor_body_has_value_return, ctor_body_uses_this, ctor_chain_uses_new_target, emit_promise_subclass_init, local_constructor_symbol_exists, node_stream_parent_kind, }; -use crate::expr::{lower_expr, lower_js_args_array, nanbox_pointer_inline, FnCtx}; +use crate::expr::{lower_expr, lower_js_args_array, nanbox_pointer_inline, temp_root, FnCtx}; use crate::nanbox::{double_literal, POINTER_MASK_I64}; use crate::types::{DOUBLE, I32, I64, I8, PTR}; @@ -136,6 +136,24 @@ fn lower_new_impl( class_name: &str, args: &[Expr], caps_absent_from_args: bool, +) -> Result { + // #6969: expression-scope temp-root barrier. The body below roots its + // constructor arguments across the instance allocation, and it has ~20 + // return paths with `lowered_args` consumed at a dozen of them — one cut + // here releases the group whichever path ran, instead of a + // `temp_root_release` at each that reviewers and future edits must keep + // balanced. + let scope = temp_root::temp_root_scope_begin(ctx, args); + let result = lower_new_impl_inner(ctx, class_name, args, caps_absent_from_args); + temp_root::temp_root_scope_end(ctx, scope); + result +} + +fn lower_new_impl_inner( + ctx: &mut FnCtx<'_>, + class_name: &str, + args: &[Expr], + caps_absent_from_args: bool, ) -> Result { // Built-in Web classes that the runtime provides constructors for. // These are checked BEFORE the ctx.classes lookup because the user @@ -328,6 +346,21 @@ fn lower_new_impl( for a in args { lowered_args.push(lower_constructor_arg(ctx, a)?); } + // #6969: every argument is now finished, but the instance allocation below + // — and, for a multi-argument list, the later arguments' own lowering — + // collects while they live in nothing but SSA registers. + // `new Pair(fresh(0), churn(N))` lost `fresh(0)` that way. Root them here; + // the re-read is immediately after the allocation (see `obj_box`), and the + // scope cut in `lower_new_impl` is the release. + let mut arg_roots: Vec> = Vec::with_capacity(lowered_args.len()); + for (a, value) in args.iter().zip(lowered_args.iter()) { + let slot = if temp_root::operand_needs_root(ctx, a) { + Some(temp_root::temp_root_push_double(ctx, value)) + } else { + None + }; + arg_roots.push(slot); + } // Compute total field count including inherited parent fields. // The runtime allocates at least 8 inline slots regardless, so this @@ -768,6 +801,16 @@ fn lower_new_impl( ) }; let obj_box = nanbox_pointer_inline(ctx.block(), &obj_handle); + // #6969: the allocation above has run, so re-read every rooted argument. + // Mandatory rather than defensive — the slots are *mutable* roots, so an + // evacuating cycle rewrote them and the registers pushed earlier are stale. + // Every `lowered_args` consumer below this point sees the re-read values. + for (value, slot) in lowered_args.iter_mut().zip(arg_roots.iter()) { + if let Some(idx) = slot { + let idx = idx.clone(); + *value = temp_root::temp_root_get_double(ctx, &idx); + } + } // Constructor bodies may contain terminating recursive construction // shapes such as `if (typeof opts === "function") return new C(...)`. From 422651e875607730c088d1242c553b8c9b94c46f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Wed, 29 Jul 2026 09:03:44 +0200 Subject: [PATCH 05/15] fix(gc): root constructor args as they are lowered, not after the loop (#6969) --- crates/perry-codegen/src/lower_call/new.rs | 24 ++++++++++++---------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/crates/perry-codegen/src/lower_call/new.rs b/crates/perry-codegen/src/lower_call/new.rs index 8320a52f7f..0cf3c8bf69 100644 --- a/crates/perry-codegen/src/lower_call/new.rs +++ b/crates/perry-codegen/src/lower_call/new.rs @@ -342,23 +342,25 @@ fn lower_new_impl_inner( // user args happened to equal its captured locals. // Lower the args first (constructor params). + // + // #6969: each argument is rooted as soon as it is lowered, NOT after the + // loop — `new Pair(fresh(0), churn(N))` collects inside `churn`, which is + // argument 1's lowering, and by then argument 0 exists only in an SSA + // register. (Rooting after the loop is worse than not rooting at all: it + // publishes an already-dangling pointer to the scanner.) The roots also + // carry the arguments across the instance allocation below, which always + // collects; the re-read is immediately after it (see `obj_box`), and the + // scope cut in `lower_new_impl` is the release. let mut lowered_args: Vec = Vec::with_capacity(args.len()); + let mut arg_roots: Vec> = Vec::with_capacity(args.len()); for a in args { - lowered_args.push(lower_constructor_arg(ctx, a)?); - } - // #6969: every argument is now finished, but the instance allocation below - // — and, for a multi-argument list, the later arguments' own lowering — - // collects while they live in nothing but SSA registers. - // `new Pair(fresh(0), churn(N))` lost `fresh(0)` that way. Root them here; - // the re-read is immediately after the allocation (see `obj_box`), and the - // scope cut in `lower_new_impl` is the release. - let mut arg_roots: Vec> = Vec::with_capacity(lowered_args.len()); - for (a, value) in args.iter().zip(lowered_args.iter()) { + let value = lower_constructor_arg(ctx, a)?; let slot = if temp_root::operand_needs_root(ctx, a) { - Some(temp_root::temp_root_push_double(ctx, value)) + Some(temp_root::temp_root_push_double(ctx, &value)) } else { None }; + lowered_args.push(value); arg_roots.push(slot); } From 1dcc50d55685067a4882e6a5d4d66cbd6a513735 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Wed, 29 Jul 2026 09:12:11 +0200 Subject: [PATCH 06/15] test(gc): pin the operand-rooting contract for #6969/#6970/#6971 --- crates/perry-codegen/src/expr/math_simple.rs | 6 +- crates/perry-codegen/src/expr/temp_root.rs | 19 +- .../tests/temp_root_operand_temporaries.rs | 387 ++++++++++++++++++ .../perry-runtime/src/gc/tests/temp_roots.rs | 84 ++++ 4 files changed, 487 insertions(+), 9 deletions(-) create mode 100644 crates/perry-codegen/tests/temp_root_operand_temporaries.rs diff --git a/crates/perry-codegen/src/expr/math_simple.rs b/crates/perry-codegen/src/expr/math_simple.rs index ae71d70c29..13cd88f1a5 100644 --- a/crates/perry-codegen/src/expr/math_simple.rs +++ b/crates/perry-codegen/src/expr/math_simple.rs @@ -556,10 +556,14 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { // the runtime call both live only in SSA registers, so a collection // there sweeps them: `m.set(fresh(k), churn(N))` aborted inside // `js_map_set` on a key whose header had been recycled. + let value_collects = temp_root::expr_may_trigger_gc(value); let roots = temp_root::root_operands( ctx, &[&m_box, &k_box], - temp_root::expr_may_trigger_gc(value), + &[ + value_collects && temp_root::operand_needs_root(ctx, map), + value_collects && temp_root::operand_needs_root(ctx, key), + ], ); // Unbox eagerly only on the unprotected path, so its IR — including // register numbering — is exactly what it was before this change. diff --git a/crates/perry-codegen/src/expr/temp_root.rs b/crates/perry-codegen/src/expr/temp_root.rs index 8d69239fff..004238b904 100644 --- a/crates/perry-codegen/src/expr/temp_root.rs +++ b/crates/perry-codegen/src/expr/temp_root.rs @@ -293,21 +293,24 @@ pub(crate) struct RootedOperands { guard: Option, } -/// Root each of `values` (NaN-boxed `double` registers) when `protect` says -/// something between here and the consuming call can collect. +/// Root each of `values` (NaN-boxed `double` registers) whose corresponding +/// `protect` flag says something between here and the consuming call can +/// collect *and* the operand is not already rooted elsewhere. /// -/// `protect` is the caller's judgement precisely because the hazard is not -/// visible in an expression list: for `m.set(k, v)` it is `v`'s lowering, for -/// `new C(a, b)` it is the instance allocation. +/// The caller supplies the flags precisely because the hazard is not visible in +/// an expression list: for `m.set(k, v)` it is `v`'s lowering, for +/// `new C(a, b)` it is the instance allocation. Pair each flag with +/// [`operand_needs_root`] so a plain local receiver — already held by the +/// shadow stack — keeps its old IR. pub(crate) fn root_operands( ctx: &mut FnCtx<'_>, values: &[&str], - protect: bool, + protect: &[bool], ) -> RootedOperands { let mut slots = Vec::with_capacity(values.len()); let mut guard: Option = None; - for value in values { - if protect { + for (i, value) in values.iter().enumerate() { + if protect.get(i).copied().unwrap_or(false) { let idx = temp_root_push_double(ctx, value); if guard.is_none() { guard = Some(idx.clone()); diff --git a/crates/perry-codegen/tests/temp_root_operand_temporaries.rs b/crates/perry-codegen/tests/temp_root_operand_temporaries.rs new file mode 100644 index 0000000000..baaa0f25cd --- /dev/null +++ b/crates/perry-codegen/tests/temp_root_operand_temporaries.rs @@ -0,0 +1,387 @@ +//! #6969 / #6970 / #6971 — the operand temporaries that #6951 did not reach. +//! +//! #6951 rooted variadic argument accumulators, concat operand pairs and +//! literal element lists. Three sibling lowering paths kept their operands in +//! bare LLVM SSA registers across a collection point: +//! +//! - **#6970** native collection-method arguments (`m.set(fresh(), churn())`) — +//! this one *aborted*, inside `js_map_set`, on a key whose header had been +//! recycled; +//! - **#6969** constructor arguments, held across the instance allocation +//! (which always collects) as well as across each other; +//! - **#6971** the string-method receiver, and `concat`'s accumulator — a bare +//! `StringHeader*`, the form only `gc::root_words`' bare case covers. +//! +//! These tests pin the *codegen contract*. The end-to-end proof is the +//! `cons_scan_off` arm (`PERRY_CONSERVATIVE_STACK_SCAN=off`), the only +//! configuration where the bug is observable — every other automatic collection +//! forces a conservative native-stack scan that pins the temporary by accident. +//! Equally important is the negative half: the shapes that were always safe +//! must still emit no rooting calls at all. + +use perry_codegen::{compile_module, AppMetadata, CompileOptions}; +use perry_hir::{Class, 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") +} + + +/// An allocating operand: an object literal is a collection point, which is all +/// `expr_may_trigger_gc` needs to see. +fn allocating() -> Expr { + Expr::Object(Vec::new()) +} + +// ---------------------------------------------------------------- #6970 ---- + +/// `m.set(key, value)` where `value` allocates: `key` is finished but lives in +/// an SSA register across `value`'s lowering. +/// +/// This is the abort in #6970. `js_map_set` ran with a key pointer whose block +/// the sweep had already returned and `churn` had reused, and the Map's +/// side-allocation owner record no longer matched — `grown Map must retain its +/// side-allocation owner record`, exit 134. +#[test] +fn map_set_key_is_rooted_across_an_allocating_value() { + let ir = ir_for( + "map_set_rooted.ts", + vec![Stmt::Expr(Expr::MapSet { + map: Box::new(Expr::MapNew), + key: Box::new(allocating()), + value: Box::new(allocating()), + })], + ); + + assert!( + ir.contains("call i32 @js_gc_temp_root_push"), + "the receiver and key must be pushed onto the temp-root stack before \ + the value's lowering, which collects (#6970):\n{ir}" + ); + assert!( + ir.contains("call i64 @js_gc_temp_root_get"), + "they must be RE-READ after the value is lowered — the slot is a \ + mutable root and an evacuating cycle rewrites it:\n{ir}" + ); + + let push = ir.find("call i32 @js_gc_temp_root_push").unwrap(); + let get = ir.find("call i64 @js_gc_temp_root_get").unwrap(); + let consume = ir.find("call i64 @js_map_set(").unwrap(); + let truncate = ir.find("call void @js_gc_temp_root_truncate").unwrap(); + assert!( + push < get && get < consume && consume < truncate, + "order must be push → re-read → consuming call → release; the release \ + comes last because js_map_set allocates while it reads the key:\n{ir}" + ); +} + +/// The gate: a `map.set` whose value cannot collect must emit no rooting at all. +#[test] +fn map_set_with_a_non_allocating_value_emits_no_rooting_calls() { + let ir = ir_for( + "map_set_no_gc.ts", + vec![Stmt::Expr(Expr::MapSet { + map: Box::new(Expr::MapNew), + key: Box::new(Expr::String("k".to_string())), + value: Box::new(Expr::Number(1.0)), + })], + ); + + assert!( + !ir.contains("call i32 @js_gc_temp_root_push"), + "nothing after the key can collect, so this must cost exactly what it \ + cost before (the `declare` line is unconditional; only a CALL counts):\n{ir}" + ); +} + +// ---------------------------------------------------------------- #6971 ---- + +/// `s.concat(x)` threads a bare `StringHeader*` accumulator through an SSA +/// register across every argument, and each `js_string_concat` returns a NEW +/// address — so the slot has to be written back, not just re-read. +#[test] +fn concat_accumulator_is_rooted_and_written_back() { + let ir = ir_for( + "concat_rooted.ts", + vec![Stmt::Expr(Expr::Call { + callee: Box::new(Expr::PropertyGet { + object: Box::new(Expr::Binary { + op: perry_hir::BinaryOp::Add, + left: Box::new(Expr::String("a".to_string())), + right: Box::new(Expr::String("b".to_string())), + }), + property: "concat".to_string(), + byte_offset: 0, + }), + args: vec![allocating()], + type_args: Vec::new(), + byte_offset: 0, + })], + ); + + assert!( + ir.contains("call i32 @js_gc_temp_root_push"), + "the concat accumulator must be rooted across an allocating argument \ + (#6971):\n{ir}" + ); + assert!( + ir.contains("call void @js_gc_temp_root_set"), + "each js_string_concat yields a NEW address, so the accumulator must be \ + written back into its slot — otherwise the next argument's lowering \ + keeps the INPUT alive and sweeps the string under construction:\n{ir}" + ); + assert!( + ir.contains("call i64 @js_gc_temp_root_get"), + "the accumulator must be re-read after the argument (and its ToString \ + coercion, which also allocates):\n{ir}" + ); +} + +/// The gate for the whole string-method family: a receiver whose arguments +/// cannot collect must not pay for the dispatch-wide root. +#[test] +fn string_method_with_non_allocating_args_emits_no_rooting_calls() { + let ir = ir_for( + "string_method_no_gc.ts", + vec![Stmt::Expr(Expr::Call { + callee: Box::new(Expr::PropertyGet { + object: Box::new(Expr::Binary { + op: perry_hir::BinaryOp::Add, + left: Box::new(Expr::String("a".to_string())), + right: Box::new(Expr::String("b".to_string())), + }), + property: "slice".to_string(), + byte_offset: 0, + }), + args: vec![Expr::Number(1.0)], + type_args: Vec::new(), + byte_offset: 0, + })], + ); + + assert!( + !ir.contains("call i32 @js_gc_temp_root_push"), + "a numeric argument cannot collect, so the receiver needs no root:\n{ir}" + ); +} + +// ---------------------------------------------------------------- #6969 ---- + +/// A module declaring `class Pair {}` plus a top-level `new Pair(a, b)`. +fn module_with_new(name: &str, args: Vec) -> Module { + let mut module = module_with_init( + name, + vec![Stmt::Expr(Expr::New { + class_name: "Pair".to_string(), + args, + type_args: Vec::new(), + byte_offset: 0, + cap_args_appended: 0, + })], + ); + module.classes = vec![Class { + id: 1, + name: "Pair".to_string(), + type_params: Vec::new(), + extends: None, + extends_name: None, + native_extends: None, + extends_expr: None, + heritage_lexically_shadowed: false, + fields: Vec::new(), + constructor: None, + methods: Vec::new(), + getters: Vec::new(), + setters: Vec::new(), + static_accessor_names: Vec::new(), + static_accessor_fn_ids: Vec::new(), + computed_members: Vec::new(), + static_fields: Vec::new(), + static_methods: Vec::new(), + decorators: Vec::new(), + is_exported: false, + aliases: Vec::new(), + is_nested: false, + alloc_width_hint: 0, + }]; + module +} + +fn ir_for_new(name: &str, args: Vec) -> String { + String::from_utf8(compile_module(&module_with_new(name, args), entry_opts()).unwrap()) + .expect("LLVM IR should be UTF-8") +} + +/// Constructor arguments are all lowered before the instance is allocated, and +/// that allocation always collects — so every heap-valued argument must be +/// rooted, and re-read after the allocation. +/// +/// The rooting must also be interleaved with the lowering, not appended after +/// it: argument 0 is live across argument 1's evaluation. Pushing the whole +/// list afterwards is strictly worse than not rooting at all — it publishes an +/// already-dangling pointer to the scanner, which turned the #6969 silent DIFF +/// into a SIGSEGV while this fix was being written. +#[test] +fn constructor_arguments_are_rooted_across_the_instance_allocation() { + let ir = ir_for_new("ctor_args_rooted.ts", vec![allocating(), allocating()]); + + assert!( + ir.contains("call i32 @js_gc_temp_root_push"), + "constructor arguments must be rooted (#6969):\n{ir}" + ); + assert!( + ir.contains("call i64 @js_gc_temp_root_get"), + "they must be re-read after the instance allocation:\n{ir}" + ); + + // Push order: the scope marker, then one root per heap argument — and + // argument 0's push must precede argument 1's *lowering*, not merely + // precede the instance allocation. + let pushes: Vec = ir + .match_indices("call i32 @js_gc_temp_root_push") + .map(|(i, _)| i) + .collect(); + assert!( + pushes.len() >= 3, + "expected the scope marker plus a root for each of the two heap \ + arguments, got {} pushes:\n{ir}", + pushes.len() + ); + // Each argument is an object literal, so each lowers to its own + // `js_object_alloc`; the SECOND one is argument 1's, i.e. the collection + // point argument 0 has to survive. + let arg_allocs: Vec = ir + .match_indices("call i64 @js_object_alloc(") + .map(|(i, _)| i) + .collect(); + assert!(arg_allocs.len() >= 2, "both arguments allocate:\n{ir}"); + assert!( + pushes[1] < arg_allocs[1], + "argument 0 must be rooted BEFORE argument 1 is lowered — rooting the \ + whole list after the loop publishes an already-dangling pointer (#6969):\n{ir}" + ); + + let instance_alloc = ir + .find("call i64 @js_object_alloc_class_inline_keys") + .expect("the instance allocation"); + let get = ir.find("call i64 @js_gc_temp_root_get").unwrap(); + assert!( + pushes[pushes.len() - 1] < instance_alloc && instance_alloc < get, + "every argument must still be rooted across the instance allocation, \ + and re-read after it:\n{ir}" + ); + + let truncate = ir.find("call void @js_gc_temp_root_truncate").unwrap(); + assert!( + get < truncate, + "the scope cut must come after the arguments are consumed:\n{ir}" + ); +} + +/// The gate: `new Pair(a, b)` on plain locals must emit no rooting. +/// +/// A `LocalGet` is already a precise root — codegen binds every pointer-typed +/// local to a shadow-stack slot — so paying for a temp root there would be a +/// cost on a shape that was never broken. +#[test] +fn constructor_arguments_on_plain_locals_emit_no_rooting_calls() { + let ir = ir_for_new( + "ctor_args_locals.ts", + vec![Expr::Number(1.0), Expr::String("s".to_string())], + ); + + assert!( + !ir.contains("call i32 @js_gc_temp_root_push"), + "a number and a string literal need no root — the literal is a load \ + from a module global already registered with js_gc_register_global_root:\n{ir}" + ); +} diff --git a/crates/perry-runtime/src/gc/tests/temp_roots.rs b/crates/perry-runtime/src/gc/tests/temp_roots.rs index d58f460c8f..afb46e1aa0 100644 --- a/crates/perry-runtime/src/gc/tests/temp_roots.rs +++ b/crates/perry-runtime/src/gc/tests/temp_roots.rs @@ -191,3 +191,87 @@ fn shadow_savepoint_restores_the_temp_root_depth() { js_gc_temp_root_truncate(outer); } + +/// Rewriting a slot must re-aim the root: the NEW value becomes reachable and +/// the replaced one does not stay alive on the strength of having once been in +/// that slot. +/// +/// This is the semantics `String.prototype.concat` depends on (#6971). Its +/// accumulator is a bare `StringHeader*` that changes address on every +/// iteration — each `js_string_concat` returns a *new* string — so codegen +/// writes the result back with `js_gc_temp_root_set` before lowering the next +/// argument. If a write-back rooted the old address instead of the new one, +/// the accumulator under construction would be the thing swept. +/// +/// Pins `ConservativeStackScanMode::Disabled`: with the unit-test default +/// (`Full`) the native-stack scan finds both raw pointers in these Rust locals +/// and the test passes without proving anything about precise roots. +#[test] +fn rewriting_a_slot_roots_the_new_value_and_releases_the_replaced_one() { + let _guard = CopyingNurseryTestGuard::new(1); + let _scan = ConservativeScanDisabledGuard::new(); + let _trigger_guard = GcTriggerThresholdTestGuard::suppress_automatic_triggers(); + register_temp_root_scanner_for_tests(); + reset_temp_roots(); + reset_old_reclaim_pressure(); + + let dead_headers = allocate_dead_malloc_churn_headers(8); + + let replaced = gc_malloc( + std::mem::size_of::(), + GC_TYPE_CLOSURE, + ); + let successor = gc_malloc( + std::mem::size_of::(), + GC_TYPE_CLOSURE, + ); + unsafe { + init_test_closure(replaced); + init_test_closure(successor); + } + assert!( + malloc_user_ptr_tracked(replaced) && malloc_user_ptr_tracked(successor), + "precondition: both objects are tracked" + ); + + // The accumulator pattern: root the first value, then hand the slot the + // successor the way `js_string_concat`'s result is written back. + let slot = js_gc_temp_root_push(replaced as u64); + js_gc_temp_root_set(slot, successor as u64); + + GC_NEXT_MALLOC_TRIGGER.with(|trigger| trigger.set(malloc_object_count().saturating_sub(1))); + gc_check_trigger(); + let completed = complete_budgeted_gc_cycle(); + assert_eq!(completed.status, JS_GC_STEP_STATUS_COMPLETED); + + assert_eq!( + tracked_malloc_headers_matching(&dead_headers), + 0, + "the sweep must actually have run for this test to mean anything" + ); + + let survivor = js_gc_temp_root_get(slot); + assert_eq!( + survivor, successor as u64, + "the slot must still hold the value written back into it" + ); + assert!( + malloc_user_ptr_tracked(survivor as *mut u8), + "the value a slot was re-aimed at must be marked, not swept (#6971)" + ); + unsafe { + assert_eq!( + (*(survivor as *mut crate::closure::ClosureHeader)).type_tag, + crate::closure::CLOSURE_MAGIC, + "the surviving accumulator must still be intact" + ); + } + assert!( + !malloc_user_ptr_tracked(replaced), + "the replaced value must NOT be retained by a slot that no longer \ + points at it — otherwise every concat round leaks its input" + ); + + js_gc_temp_root_truncate(slot); + assert_eq!(temp_root_depth(), 0); +} From fc09be4db71e1166b778eaa0ebe8955473a9ce2a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Wed, 29 Jul 2026 09:27:14 +0200 Subject: [PATCH 07/15] fix(gc): operand_needs_root must check the shadow slot, not just LocalGet --- crates/perry-codegen/src/expr/temp_root.rs | 29 +++++++++++++++------- 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/crates/perry-codegen/src/expr/temp_root.rs b/crates/perry-codegen/src/expr/temp_root.rs index 004238b904..1d6c99fd8c 100644 --- a/crates/perry-codegen/src/expr/temp_root.rs +++ b/crates/perry-codegen/src/expr/temp_root.rs @@ -429,17 +429,28 @@ pub(crate) fn any_may_trigger_gc<'a>( /// - provably not a heap reference — a slot for it is pure TLS traffic; /// - a string literal — a load from a module global `__perry_init_strings_*` /// registered with `js_gc_register_global_root`; -/// - a plain local or module-global read — the shadow stack and the module-var -/// scanners already hold those for as long as generated code can see them. +/// - a module-global read — `@perry_global_*` are registered GC roots; +/// - a local that **has a reserved shadow slot**, and is therefore already a +/// precise root for as long as generated code can observe it. /// -/// The last one is why `new C(a, b)` on plain locals stays at its old IR even -/// though the instance allocation that follows always collects. +/// The last one is why `new C(a, b)` on ordinary locals stays at its old IR +/// even though the instance allocation that follows always collects. +/// +/// The shadow-slot check is load-bearing, not decoration. Suppressing every +/// `LocalGet` looks equivalent and is not: a local can be pointer-valued and +/// have *no* shadow slot, in which case it lives in a bare alloca that the root +/// walk never visits (that is the #6968 defect). `m.set(fresh(), churn())` +/// regressed straight back to an abort when this was written as a blanket +/// `LocalGet` suppression — the Map receiver was exactly such a local. pub(crate) fn operand_needs_root(ctx: &FnCtx<'_>, expr: &Expr) -> bool { - !super::expr_is_known_non_pointer_shadow_value(ctx, expr) - && !matches!( - expr, - Expr::String(_) | Expr::LocalGet(_) | Expr::GlobalGet(_) - ) + if super::expr_is_known_non_pointer_shadow_value(ctx, expr) { + return false; + } + match expr { + Expr::String(_) | Expr::GlobalGet(_) => false, + Expr::LocalGet(id) => !ctx.shadow_slot_map.contains_key(id), + _ => true, + } } /// Open an expression-scope temp-root barrier for a call/constructor whose From 75af94f4ce88a1a311cb2b7006bf76e5b23ddd39 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Wed, 29 Jul 2026 09:28:33 +0200 Subject: [PATCH 08/15] fix(gc): adopt the #6975 expr_may_trigger_gc signature --- crates/perry-codegen/src/expr/math_simple.rs | 2 +- crates/perry-codegen/src/lower_string_method.rs | 12 ++++-------- .../tests/temp_root_operand_temporaries.rs | 1 - 3 files changed, 5 insertions(+), 10 deletions(-) diff --git a/crates/perry-codegen/src/expr/math_simple.rs b/crates/perry-codegen/src/expr/math_simple.rs index 13cd88f1a5..7e2af6efcf 100644 --- a/crates/perry-codegen/src/expr/math_simple.rs +++ b/crates/perry-codegen/src/expr/math_simple.rs @@ -556,7 +556,7 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { // the runtime call both live only in SSA registers, so a collection // there sweeps them: `m.set(fresh(k), churn(N))` aborted inside // `js_map_set` on a key whose header had been recycled. - let value_collects = temp_root::expr_may_trigger_gc(value); + let value_collects = temp_root::expr_may_trigger_gc(ctx, value); let roots = temp_root::root_operands( ctx, &[&m_box, &k_box], diff --git a/crates/perry-codegen/src/lower_string_method.rs b/crates/perry-codegen/src/lower_string_method.rs index 48fc9dcba6..613c8bd3ee 100644 --- a/crates/perry-codegen/src/lower_string_method.rs +++ b/crates/perry-codegen/src/lower_string_method.rs @@ -163,10 +163,8 @@ pub(crate) fn lower_string_method( // `gc::root_words` bare form covers. Root it across the whole dispatch; // the truncate below is the single release point for every one of the // match's ~60 return paths. - let recv_root = args - .iter() - .any(temp_root::expr_may_trigger_gc) - .then(|| temp_root_push_double(ctx, &recv_box)); + let args_can_collect = args.iter().any(|a| temp_root::expr_may_trigger_gc(ctx, a)); + let recv_root = args_can_collect.then(|| temp_root_push_double(ctx, &recv_box)); let result = lower_string_method_dispatch(ctx, object, property, args, &recv_box, &recv_root); // Released only after the dispatch's consuming runtime call has run: that // call allocates while it reads the receiver. @@ -1095,10 +1093,8 @@ fn lower_string_method_dispatch( let blk = ctx.block(); unbox_str_handle(blk, &recv_box) }; - let acc_root = args - .iter() - .any(temp_root::expr_may_trigger_gc) - .then(|| temp_root_push_i64(ctx, &acc_handle)); + let args_can_collect = args.iter().any(|a| temp_root::expr_may_trigger_gc(ctx, a)); + let acc_root = args_can_collect.then(|| temp_root_push_i64(ctx, &acc_handle)); for a in args { let a_is_str = is_string_expr(ctx, a); let s_box = lower_expr(ctx, a)?; diff --git a/crates/perry-codegen/tests/temp_root_operand_temporaries.rs b/crates/perry-codegen/tests/temp_root_operand_temporaries.rs index baaa0f25cd..e16b94cfde 100644 --- a/crates/perry-codegen/tests/temp_root_operand_temporaries.rs +++ b/crates/perry-codegen/tests/temp_root_operand_temporaries.rs @@ -115,7 +115,6 @@ fn ir_for(name: &str, init: Vec) -> String { .expect("LLVM IR should be UTF-8") } - /// An allocating operand: an object literal is a collection point, which is all /// `expr_may_trigger_gc` needs to see. fn allocating() -> Expr { From c87390b9b2a9ba31fae769b35b906672e8055e0b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Wed, 29 Jul 2026 09:34:52 +0200 Subject: [PATCH 09/15] docs: changelog fragment for #6969/#6970/#6971 --- .../6977-operand-temporaries-precise-roots.md | 67 +++++++++++++++++++ 1 file changed, 67 insertions(+) create mode 100644 changelog.d/6977-operand-temporaries-precise-roots.md diff --git a/changelog.d/6977-operand-temporaries-precise-roots.md b/changelog.d/6977-operand-temporaries-precise-roots.md new file mode 100644 index 0000000000..904d498e3e --- /dev/null +++ b/changelog.d/6977-operand-temporaries-precise-roots.md @@ -0,0 +1,67 @@ +### Fixed + +- **GC: operand temporaries in three more lowering paths are precise roots (#6969, #6970, #6971).** + #6972 rooted variadic argument accumulators, concat operand pairs and literal + element lists; #6975 closed the coercion hole in the gate. Three sibling paths + still kept an evaluated operand in a bare LLVM SSA register across a + collection point, which under precise-roots-only + (`PERRY_CONSERVATIVE_STACK_SCAN=off`) is a live use-after-free: + + - **#6970 — collection-method operands.** `m.set(fresh(0), churn(N))` + **aborted** (exit 134, `grown Map must retain its side-allocation owner + record`): the key was finished and live only in a register across the + value's lowering, so `js_map_set` ran against a header the sweep had freed + and `churn` had reused. Fixed in the `Expr::MapSet` / `MapGet` / `MapHas` + lowering and in the `PropertyGet` dispatch that handles non-`Ident` + receivers (`this.field.set(…)`), including `Map`/`Set`/`URLSearchParams` + `forEach`, whose callback closure is itself an allocation the receiver has + to survive. + - **#6969 — constructor arguments.** `new Pair(fresh(0), churn(N))` held + argument 0 across argument 1's lowering *and* across the instance + allocation, which always collects. + - **#6971 — string-method receiver and the `concat` accumulator.** + `fresh(0).concat("|" + churn(N))` dropped its receiver. `concat` is the + dangerous form: its accumulator is a bare `StringHeader*`, the word form + only `gc::root_words`' bare case covers, and every `js_string_concat` + returns a *new* address — so the slot is written back with + `js_gc_temp_root_set`, not merely re-read. + + Two mechanism additions, both built from #6972's primitives: + `RootedOperands` (root already-lowered operands when the *caller* knows what + follows collects — a per-branch operand representation for `MapSet`, an + allocation for `new`), and `temp_root_scope_begin`/`_end`, an + expression-scope barrier. The barrier is what makes `lower_new` tractable: + truncation is a stack *cut*, so one marker slot releases the whole group on + whichever of that function's ~20 return paths ran, instead of a + `temp_root_release` at each that future edits must keep balanced. + + A new suppression, `operand_needs_root`, keeps this free where it was already + safe: literals, module globals, provable non-pointers, and locals that + **have a reserved shadow slot**. The shadow-slot check is load-bearing — a + blanket `LocalGet` suppression regressed #6970 straight back to an abort, + because a local can be pointer-valued with no shadow slot and therefore no + precise root at all (that is #6968). + + Cost: on a probe of already-safe shapes (`"user_" + i`, `[1,2,3]`, + `{a:i,b:total}`, all-local argument lists, `m.set(k, 1)`, `m.get(k)`, + `label.slice(1,3)`, `new Pair(label, i)`) the emitted LLVM IR is + **byte-identical** to `main`, md5 included. The three protected shapes add 11 + runtime calls and 14 IR lines in total. + + Verification: each issue's reproducer is byte-exact over 4 runs under + `PERRY_CONSERVATIVE_STACK_SCAN=off PERRY_GC_HEAP_LIMIT=8` with the arm + measurably live (`PERRY_GC_TRACE=1`: 22 completed cycles), against exit-134 / + silent-DIFF before. Rooting a constructor's argument list *after* the lowering + loop rather than interleaved turned #6969's silent DIFF into a SIGSEGV — it + publishes an already-dangling pointer to the scanner — so the interleaving is + pinned by a test. + +### Notes + +- `gc::tests::temp_roots::rewriting_a_slot_roots_the_new_value_and_releases_the_replaced_one` + pins `ConservativeStackScanMode::Disabled`, as every test in that module must: + with the unit-test default (`Full`) the native-stack scan finds the raw + pointers in the test's own Rust locals and the test passes without proving + anything about precise roots. +- Six codegen IR tests pin the emission contract and, just as importantly, the + *absence* of rooting on the shapes that were never broken. From 5b715e6cca1c71b6c1017d593c903b7b56fa85e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Wed, 29 Jul 2026 09:36:09 +0200 Subject: [PATCH 10/15] docs: key the changelog fragment to PR #6983 --- ...precise-roots.md => 6983-operand-temporaries-precise-roots.md} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename changelog.d/{6977-operand-temporaries-precise-roots.md => 6983-operand-temporaries-precise-roots.md} (100%) diff --git a/changelog.d/6977-operand-temporaries-precise-roots.md b/changelog.d/6983-operand-temporaries-precise-roots.md similarity index 100% rename from changelog.d/6977-operand-temporaries-precise-roots.md rename to changelog.d/6983-operand-temporaries-precise-roots.md From 5b485380875435d01a860256842e3076544c5dc1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Wed, 29 Jul 2026 10:12:19 +0200 Subject: [PATCH 11/15] fix(gc): re-load registered-root operands instead of reusing a stale register A shadow-slotted local, a module global and a string literal are all marked by the collector, so a suppressed operand can never be swept. But an evacuating cycle REWRITES their storage, and the register loaded beforehand keeps the pre-move address -- the same staleness #6981 reports for a raw typed-array pointer under the specialized ABI. Re-lowering the operand at the re-read point emits the load again and costs no runtime call. --- crates/perry-codegen/src/expr/math_simple.rs | 94 +++++++++---- crates/perry-codegen/src/expr/temp_root.rs | 133 +++++++++++++----- crates/perry-codegen/src/lower_call/new.rs | 19 ++- .../tests/temp_root_operand_temporaries.rs | 55 +++++++- 4 files changed, 234 insertions(+), 67 deletions(-) diff --git a/crates/perry-codegen/src/expr/math_simple.rs b/crates/perry-codegen/src/expr/math_simple.rs index 7e2af6efcf..f8db5217a3 100644 --- a/crates/perry-codegen/src/expr/math_simple.rs +++ b/crates/perry-codegen/src/expr/math_simple.rs @@ -277,9 +277,10 @@ fn guarded_map_number_key_set( fn reread_map_set_receiver_and_key( ctx: &mut FnCtx<'_>, roots: &temp_root::RootedOperands, + operands: &[&Expr; 2], m_handle_unrooted: &Option, -) -> (String, String) { - let values = roots.reread(ctx); +) -> Result<(String, String)> { + let values = roots.reread(ctx, operands)?; let k_box = values[1].clone(); let m_handle = match m_handle_unrooted { Some(handle) => handle.clone(), @@ -289,7 +290,7 @@ fn reread_map_set_receiver_and_key( unbox_to_i64(blk, &m_box) } }; - (m_handle, k_box) + Ok((m_handle, k_box)) } fn guarded_map_number_key_get(ctx: &mut FnCtx<'_>, map_handle: &str, key_box: &str) -> String { @@ -557,13 +558,12 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { // there sweeps them: `m.set(fresh(k), churn(N))` aborted inside // `js_map_set` on a key whose header had been recycled. let value_collects = temp_root::expr_may_trigger_gc(ctx, value); + let map_key_operands: [&Expr; 2] = [map, key]; let roots = temp_root::root_operands( ctx, + &map_key_operands, &[&m_box, &k_box], - &[ - value_collects && temp_root::operand_needs_root(ctx, map), - value_collects && temp_root::operand_needs_root(ctx, key), - ], + &[value_collects, value_collects], ); // Unbox eagerly only on the unprotected path, so its IR — including // register numbering — is exactly what it was before this change. @@ -576,8 +576,12 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { let new_handle = if use_string_i32_map { let value_i32 = lower_expr_native(ctx, value, crate::native_value::ExpectedNativeRep::I32)?; - let (m_handle, k_box) = - reread_map_set_receiver_and_key(ctx, &roots, &m_handle_unrooted); + let (m_handle, k_box) = reread_map_set_receiver_and_key( + ctx, + &roots, + &map_key_operands, + &m_handle_unrooted, + )?; let (k_handle, new_handle) = { let blk = ctx.block(); let k_handle = unbox_str_handle(blk, &k_box); @@ -609,8 +613,12 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { } else if use_string_u32_map { let value_u32 = lower_expr_native(ctx, value, crate::native_value::ExpectedNativeRep::U32)?; - let (m_handle, k_box) = - reread_map_set_receiver_and_key(ctx, &roots, &m_handle_unrooted); + let (m_handle, k_box) = reread_map_set_receiver_and_key( + ctx, + &roots, + &map_key_operands, + &m_handle_unrooted, + )?; let (k_handle, new_handle) = { let blk = ctx.block(); let k_handle = unbox_str_handle(blk, &k_box); @@ -642,8 +650,12 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { } else if use_string_f32_map { let value_f32 = lower_expr_native(ctx, value, crate::native_value::ExpectedNativeRep::F32)?; - let (m_handle, k_box) = - reread_map_set_receiver_and_key(ctx, &roots, &m_handle_unrooted); + let (m_handle, k_box) = reread_map_set_receiver_and_key( + ctx, + &roots, + &map_key_operands, + &m_handle_unrooted, + )?; let (k_handle, new_handle) = { let blk = ctx.block(); let k_handle = unbox_str_handle(blk, &k_box); @@ -674,8 +686,12 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { new_handle } else if use_string_number_map { let v_box = lower_expr(ctx, value)?; - let (m_handle, k_box) = - reread_map_set_receiver_and_key(ctx, &roots, &m_handle_unrooted); + let (m_handle, k_box) = reread_map_set_receiver_and_key( + ctx, + &roots, + &map_key_operands, + &m_handle_unrooted, + )?; let (k_handle, new_handle) = { let blk = ctx.block(); let k_handle = unbox_str_handle(blk, &k_box); @@ -698,8 +714,12 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { } else if use_string_boolean_map { let value_i1 = lower_expr_native(ctx, value, crate::native_value::ExpectedNativeRep::I1)?; - let (m_handle, k_box) = - reread_map_set_receiver_and_key(ctx, &roots, &m_handle_unrooted); + let (m_handle, k_box) = reread_map_set_receiver_and_key( + ctx, + &roots, + &map_key_operands, + &m_handle_unrooted, + )?; let (k_handle, new_handle) = { let blk = ctx.block(); let k_handle = unbox_str_handle(blk, &k_box); @@ -731,8 +751,12 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { new_handle } else if use_string_string_map { let v_box = lower_expr(ctx, value)?; - let (m_handle, k_box) = - reread_map_set_receiver_and_key(ctx, &roots, &m_handle_unrooted); + let (m_handle, k_box) = reread_map_set_receiver_and_key( + ctx, + &roots, + &map_key_operands, + &m_handle_unrooted, + )?; let (k_handle, v_handle, new_handle) = { let blk = ctx.block(); let k_handle = unbox_str_handle(blk, &k_box); @@ -765,8 +789,12 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { new_handle } else if has_string_key_map { let v_box = lower_expr(ctx, value)?; - let (m_handle, k_box) = - reread_map_set_receiver_and_key(ctx, &roots, &m_handle_unrooted); + let (m_handle, k_box) = reread_map_set_receiver_and_key( + ctx, + &roots, + &map_key_operands, + &m_handle_unrooted, + )?; let (k_handle, new_handle) = { let blk = ctx.block(); let k_handle = unbox_str_handle(blk, &k_box); @@ -800,8 +828,12 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { new_handle } else if use_number_string_map { let v_box = lower_expr(ctx, value)?; - let (m_handle, k_box) = - reread_map_set_receiver_and_key(ctx, &roots, &m_handle_unrooted); + let (m_handle, k_box) = reread_map_set_receiver_and_key( + ctx, + &roots, + &map_key_operands, + &m_handle_unrooted, + )?; let (v_handle, v_slot_box) = { let blk = ctx.block(); let v_handle = unbox_str_handle(blk, &v_box); @@ -822,8 +854,12 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { guarded_map_number_key_set(ctx, &m_handle, &k_box, &v_slot_box) } else if use_number_key_map { let v_box = lower_expr(ctx, value)?; - let (m_handle, k_box) = - reread_map_set_receiver_and_key(ctx, &roots, &m_handle_unrooted); + let (m_handle, k_box) = reread_map_set_receiver_and_key( + ctx, + &roots, + &map_key_operands, + &m_handle_unrooted, + )?; if static_number_string_map { record_collection_typed_value_fallback( ctx, @@ -839,8 +875,12 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { guarded_map_number_key_set(ctx, &m_handle, &k_box, &v_box) } else { let v_box = lower_expr(ctx, value)?; - let (m_handle, k_box) = - reread_map_set_receiver_and_key(ctx, &roots, &m_handle_unrooted); + let (m_handle, k_box) = reread_map_set_receiver_and_key( + ctx, + &roots, + &map_key_operands, + &m_handle_unrooted, + )?; let new_handle = { let blk = ctx.block(); blk.call( diff --git a/crates/perry-codegen/src/expr/temp_root.rs b/crates/perry-codegen/src/expr/temp_root.rs index 1d6c99fd8c..0473e86cc5 100644 --- a/crates/perry-codegen/src/expr/temp_root.rs +++ b/crates/perry-codegen/src/expr/temp_root.rs @@ -287,30 +287,47 @@ pub(crate) fn lower_operand_pair_rooted( pub(crate) struct RootedOperands { /// Slot index per operand, or `None` when the operand was not rooted. slots: Vec>, - /// The registers as originally lowered — the answer when nothing is rooted. + /// The registers as originally lowered — the answer when nothing is rooted + /// and the operand cannot be re-loaded. values: Vec, + /// Whether an unrooted operand must be re-loaded from its own storage + /// rather than reused from its register. See [`RootedOperands::reread`]. + reloadable: Vec, /// First slot pushed; truncating it drops the whole group. guard: Option, } -/// Root each of `values` (NaN-boxed `double` registers) whose corresponding -/// `protect` flag says something between here and the consuming call can -/// collect *and* the operand is not already rooted elsewhere. +/// Protect `values` across a collection point the caller knows about. /// -/// The caller supplies the flags precisely because the hazard is not visible in -/// an expression list: for `m.set(k, v)` it is `v`'s lowering, for -/// `new C(a, b)` it is the instance allocation. Pair each flag with -/// [`operand_needs_root`] so a plain local receiver — already held by the -/// shadow stack — keeps its old IR. +/// `collects[i]` says "something between operand `i` and the consuming call can +/// reach a collection point". The caller supplies it because the hazard is not +/// visible in an expression list: for `m.set(k, v)` it is `v`'s lowering, for +/// `new C(a, b)` it is the instance allocation. +/// +/// From that one flag two decisions follow, and an operand needs exactly one of +/// them: +/// +/// - [`operand_needs_root`] → push a temp-root slot, because nothing else keeps +/// this value alive; +/// - otherwise [`operand_is_reloadable`] → emit no runtime call, but re-load the +/// value at the re-read point, because its storage is a registered root that +/// evacuation *rewrites* while the cached register keeps the old address. +/// +/// When `collects[i]` is false neither applies: nothing can be swept and nothing +/// can move, so the register is reused and the IR is exactly what it was. pub(crate) fn root_operands( ctx: &mut FnCtx<'_>, + operands: &[&Expr], values: &[&str], - protect: &[bool], + collects: &[bool], ) -> RootedOperands { let mut slots = Vec::with_capacity(values.len()); + let mut reloadable = Vec::with_capacity(values.len()); let mut guard: Option = None; for (i, value) in values.iter().enumerate() { - if protect.get(i).copied().unwrap_or(false) { + let collects_here = collects.get(i).copied().unwrap_or(false); + let needs_root = collects_here && operand_needs_root(ctx, operands[i]); + if needs_root { let idx = temp_root_push_double(ctx, value); if guard.is_none() { guard = Some(idx.clone()); @@ -319,32 +336,70 @@ pub(crate) fn root_operands( } else { slots.push(None); } + reloadable.push(!needs_root && collects_here && operand_is_reloadable(operands[i])); } RootedOperands { slots, values: values.iter().map(|v| (*v).to_string()).collect(), + reloadable, guard, } } +/// Does this operand read a location the collector *rewrites in place*, so that +/// re-lowering it after a collection yields the corrected address? +/// +/// A local with a shadow slot, a module global and a string-literal handle are +/// all registered roots — they are marked, and on an evacuating cycle they are +/// **rewritten**. That keeps the object alive and the *storage* correct, but it +/// says nothing about a register loaded from that storage beforehand: after +/// relocation the register holds the pre-move address. Re-loading is the fix, +/// and it is free — no temp-root traffic, just the load that would have been +/// emitted anyway. +/// +/// This is the same staleness #6981 reports one layer in (a raw typed-array +/// pointer passed under the specialized ABI). +pub(crate) fn operand_is_reloadable(expr: &Expr) -> bool { + matches!( + expr, + Expr::LocalGet(_) | Expr::GlobalGet(_) | Expr::String(_) + ) +} + impl RootedOperands { - /// Re-read every rooted operand. Mandatory after the collection point, not - /// defensive: the slot is a *mutable* root, so an evacuating cycle rewrites - /// it and the register pushed beforehand is stale. + /// Re-read every operand after the collection point. + /// + /// Three cases, and the third is the subtle one: /// - /// Emits nothing when nothing was rooted. - pub(crate) fn reread(&self, ctx: &mut FnCtx<'_>) -> Vec { - self.slots - .iter() - .zip(self.values.iter()) - .map(|(slot, original)| match slot { + /// - **rooted** → read the slot. Mandatory, not defensive: the slot is a + /// *mutable* root, so an evacuating cycle rewrites it and the register + /// pushed beforehand is stale. + /// - **unrooted but re-loadable** → re-lower it. A local/global/literal is + /// already a registered root, so it was never at risk of being *swept* — + /// but an evacuating cycle rewrote its storage, so the register loaded + /// before the collection points at where the object *used to be*. Emitting + /// the load again is correct and costs no runtime call. + /// - **unrooted and not re-loadable** → keep the register. This is only + /// reached for values `expr_is_known_non_pointer_shadow_value` proved are + /// not heap references, which relocation cannot invalidate. + pub(crate) fn reread( + &self, + ctx: &mut FnCtx<'_>, + operands: &[&Expr], + ) -> anyhow::Result> { + let mut out = Vec::with_capacity(self.values.len()); + for (i, original) in self.values.iter().enumerate() { + let value = match &self.slots[i] { Some(idx) => { let idx = idx.clone(); temp_root_get_double(ctx, &idx) } + None if self.reloadable[i] => super::lower_expr(ctx, operands[i])?, None => original.clone(), - }) - .collect() + }; + out.push(value); + } + Ok(out) } /// True when this group actually pushed slots — the signal a caller uses to @@ -424,24 +479,38 @@ pub(crate) fn any_may_trigger_gc<'a>( /// Would `expr`'s lowered value need a temp root, assuming everything after it /// reaches a collection point? /// -/// Three suppressions, each meaning "already rooted, or nothing to root": +/// A temp root buys two distinct things, and the suppressions here only give +/// up the first: +/// +/// 1. **liveness** — the object is marked instead of swept; +/// 2. **a re-readable location** — a slot the collector rewrites, so the value +/// can be recovered after relocation. +/// +/// Suppressed operands already have (1) from somewhere else, and get (2) from +/// [`operand_is_reloadable`] instead, which re-emits the load rather than +/// reusing the pre-collection register. Both halves are required: dropping the +/// second is exactly the staleness #6981 reports one layer in. /// -/// - provably not a heap reference — a slot for it is pure TLS traffic; +/// - provably not a heap reference — a slot for it is pure TLS traffic, and +/// relocation cannot invalidate it either; /// - a string literal — a load from a module global `__perry_init_strings_*` /// registered with `js_gc_register_global_root`; -/// - a module-global read — `@perry_global_*` are registered GC roots; -/// - a local that **has a reserved shadow slot**, and is therefore already a -/// precise root for as long as generated code can observe it. +/// - a module-global read — `@perry_global_*` are registered GC roots +/// (marked *and* rewritten on evacuation); +/// - a local that **has a reserved shadow slot**, which binds the collector to +/// the local's own alloca — so evacuation rewrites the alloca in place. /// -/// The last one is why `new C(a, b)` on ordinary locals stays at its old IR -/// even though the instance allocation that follows always collects. +/// Together these are why `new C(a, b)` on ordinary locals emits no runtime +/// rooting calls even though the instance allocation that follows always +/// collects. /// /// The shadow-slot check is load-bearing, not decoration. Suppressing every /// `LocalGet` looks equivalent and is not: a local can be pointer-valued and /// have *no* shadow slot, in which case it lives in a bare alloca that the root -/// walk never visits (that is the #6968 defect). `m.set(fresh(), churn())` -/// regressed straight back to an abort when this was written as a blanket -/// `LocalGet` suppression — the Map receiver was exactly such a local. +/// walk never visits (that is the #6968 defect) — so it has neither (1) nor +/// (2). `m.set(fresh(), churn())` regressed straight back to an abort when this +/// was written as a blanket `LocalGet` suppression; the Map receiver was +/// exactly such a local. pub(crate) fn operand_needs_root(ctx: &FnCtx<'_>, expr: &Expr) -> bool { if super::expr_is_known_non_pointer_shadow_value(ctx, expr) { return false; diff --git a/crates/perry-codegen/src/lower_call/new.rs b/crates/perry-codegen/src/lower_call/new.rs index 0cf3c8bf69..9f9c1dd272 100644 --- a/crates/perry-codegen/src/lower_call/new.rs +++ b/crates/perry-codegen/src/lower_call/new.rs @@ -807,10 +807,21 @@ fn lower_new_impl_inner( // Mandatory rather than defensive — the slots are *mutable* roots, so an // evacuating cycle rewrote them and the registers pushed earlier are stale. // Every `lowered_args` consumer below this point sees the re-read values. - for (value, slot) in lowered_args.iter_mut().zip(arg_roots.iter()) { - if let Some(idx) = slot { - let idx = idx.clone(); - *value = temp_root::temp_root_get_double(ctx, &idx); + for (i, (value, slot)) in lowered_args.iter_mut().zip(arg_roots.iter()).enumerate() { + match slot { + Some(idx) => { + let idx = idx.clone(); + *value = temp_root::temp_root_get_double(ctx, &idx); + } + // Not rooted because it reads a registered root (a shadow-slotted + // local, a module global, a string literal). Those are never swept + // — but an evacuating cycle REWROTE their storage, so the register + // loaded before the allocation points at the pre-move address. + // Re-lowering emits the load again and costs no runtime call. + None if temp_root::operand_is_reloadable(&args[i]) => { + *value = lower_constructor_arg(ctx, &args[i])?; + } + None => {} } } diff --git a/crates/perry-codegen/tests/temp_root_operand_temporaries.rs b/crates/perry-codegen/tests/temp_root_operand_temporaries.rs index e16b94cfde..695a36715b 100644 --- a/crates/perry-codegen/tests/temp_root_operand_temporaries.rs +++ b/crates/perry-codegen/tests/temp_root_operand_temporaries.rs @@ -366,11 +366,13 @@ fn constructor_arguments_are_rooted_across_the_instance_allocation() { ); } -/// The gate: `new Pair(a, b)` on plain locals must emit no rooting. +/// The gate: `new Pair(a, b)` on immediates must emit no rooting. /// -/// A `LocalGet` is already a precise root — codegen binds every pointer-typed -/// local to a shadow-stack slot — so paying for a temp root there would be a -/// cost on a shape that was never broken. +/// A number roots nothing at all, and a string literal is already a registered +/// root, so neither needs a temp-root slot. (The literal is still *re-loaded* +/// after the allocation — see +/// `registered_root_operands_are_reloaded_rather_than_rooted` — but that is a +/// plain load, not a runtime call.) #[test] fn constructor_arguments_on_plain_locals_emit_no_rooting_calls() { let ir = ir_for_new( @@ -384,3 +386,48 @@ fn constructor_arguments_on_plain_locals_emit_no_rooting_calls() { from a module global already registered with js_gc_register_global_root:\n{ir}" ); } + +/// An operand that reads a *registered root* is not rooted again — but it must +/// be **re-loaded**, not reused from its pre-collection register. +/// +/// A string literal, a module global and a shadow-slotted local are all marked +/// by the collector, so they are never swept. But an evacuating cycle +/// **rewrites their storage**, and the register loaded before the collection +/// still holds the pre-move address. Emitting the load again is correct and +/// costs no runtime call — the same staleness #6981 reports one layer in, for a +/// raw typed-array pointer under the specialized ABI. +#[test] +fn registered_root_operands_are_reloaded_rather_than_rooted() { + let ir = ir_for_new( + "ctor_args_reload.ts", + vec![Expr::String("lit".to_string()), allocating()], + ); + + let handle_load = "load double, ptr @ctor_args_reload_ts_.str."; + let loads: Vec = ir.match_indices(handle_load).map(|(i, _)| i).collect(); + assert!( + loads.len() >= 2, + "the literal operand must be loaded a SECOND time after the instance \ + allocation — reusing the first register leaves it pointing at where \ + the string used to be once evacuation moves it:\n{ir}" + ); + + let alloc = ir + .find("call i64 @js_object_alloc_class_inline_keys") + .expect("the instance allocation"); + assert!( + loads[0] < alloc && loads.iter().any(|&l| l > alloc), + "one load before the allocation (the original lowering) and one after \ + it (the re-load):\n{ir}" + ); + + // And it must be a re-LOAD, not a temp root: a registered root needs no + // second liveness mechanism, so this must cost zero runtime calls for it. + // (The allocating operand still gets a real root — hence >= 1 push.) + let pushes = ir.matches("call i32 @js_gc_temp_root_push").count(); + assert!( + pushes <= 2, + "expected at most the scope marker plus the allocating operand's root; \ + the literal must not get a slot of its own, got {pushes}:\n{ir}" + ); +} From 75f2170f20d985bca35ca4cb538d39db37e0a21c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Wed, 29 Jul 2026 10:12:33 +0200 Subject: [PATCH 12/15] docs: record the re-load half of the fix --- .../6983-operand-temporaries-precise-roots.md | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/changelog.d/6983-operand-temporaries-precise-roots.md b/changelog.d/6983-operand-temporaries-precise-roots.md index 904d498e3e..637044de89 100644 --- a/changelog.d/6983-operand-temporaries-precise-roots.md +++ b/changelog.d/6983-operand-temporaries-precise-roots.md @@ -42,11 +42,23 @@ because a local can be pointer-valued with no shadow slot and therefore no precise root at all (that is #6968). + A temp root buys two things — liveness, and a location the collector rewrites + — and the suppressions only give up the first. A suppressed operand reads a + registered root, so it can never be *swept*; but evacuation **rewrites that + storage**, leaving the pre-collection register pointing at where the object + used to be. `operand_is_reloadable` therefore re-emits the load at the + re-read point instead of reusing the register: correct under relocation, and + a plain `load` rather than a runtime call. This is the same staleness #6981 + reports one layer in, for a raw typed-array pointer under the specialized + ABI. + Cost: on a probe of already-safe shapes (`"user_" + i`, `[1,2,3]`, `{a:i,b:total}`, all-local argument lists, `m.set(k, 1)`, `m.get(k)`, `label.slice(1,3)`, `new Pair(label, i)`) the emitted LLVM IR is **byte-identical** to `main`, md5 included. The three protected shapes add 11 - runtime calls and 14 IR lines in total. + runtime calls and 14 IR lines in total. Where a real allocation does intervene + over a registered-root operand, the cost is one extra `load` per operand and + no runtime call. Verification: each issue's reproducer is byte-exact over 4 runs under `PERRY_CONSERVATIVE_STACK_SCAN=off PERRY_GC_HEAP_LIMIT=8` with the arm From cf02fa71685b379c579aa3a746ffac96bda5fcf3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Wed, 29 Jul 2026 10:24:47 +0200 Subject: [PATCH 13/15] fix(gc): root the MapSet receiver before the key is lowered, and refresh ctor args at late consumers Two review findings from CodeRabbit on #6983, both real: 1. The MapSet receiver was rooted only AFTER the key had been lowered, so its exposure window (the key's own lowering, which allocates in `m.set(fresh(k), churn(N))`) was unprotected -- and pushing an already-dead register into a scanned slot is strictly worse than not rooting, the same escalation that turned #6969 into a SIGSEGV. RootedOperands is now built incrementally so each operand is rooted before the next is lowered. 2. lower_new's single post-allocation re-read did not cover the consumers that sit behind field initializers and an inlined constructor body (marshal_imported_ctor_args, the dynamic-parent super args buffer). Those values stay rooted, so this was staleness under evacuation rather than a use-after-free, but it is the same class this PR treats as mandatory. Extracted refresh_rooted_args and called it at those sites too. --- .../6983-operand-temporaries-precise-roots.md | 4 +- crates/perry-codegen/src/expr/math_simple.rs | 29 +++-- crates/perry-codegen/src/expr/temp_root.rs | 112 ++++++++++-------- crates/perry-codegen/src/lower_call/new.rs | 70 +++++++---- 4 files changed, 130 insertions(+), 85 deletions(-) diff --git a/changelog.d/6983-operand-temporaries-precise-roots.md b/changelog.d/6983-operand-temporaries-precise-roots.md index 637044de89..ee79e76665 100644 --- a/changelog.d/6983-operand-temporaries-precise-roots.md +++ b/changelog.d/6983-operand-temporaries-precise-roots.md @@ -1,8 +1,8 @@ ### Fixed - **GC: operand temporaries in three more lowering paths are precise roots (#6969, #6970, #6971).** - #6972 rooted variadic argument accumulators, concat operand pairs and literal - element lists; #6975 closed the coercion hole in the gate. Three sibling paths + #6951 (via #6972) rooted variadic argument accumulators, concat operand pairs + and literal element lists; #6975 closed the coercion hole in the gate. Three sibling paths still kept an evaluated operand in a bare LLVM SSA register across a collection point, which under precise-roots-only (`PERRY_CONSERVATIVE_STACK_SCAN=off`) is a live use-after-free: diff --git a/crates/perry-codegen/src/expr/math_simple.rs b/crates/perry-codegen/src/expr/math_simple.rs index f8db5217a3..2f6efb9908 100644 --- a/crates/perry-codegen/src/expr/math_simple.rs +++ b/crates/perry-codegen/src/expr/math_simple.rs @@ -550,21 +550,24 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { let use_string_string_map = is_static_string_string_map(ctx, map) && is_definitely_string_expr(ctx, key) && is_definitely_string_expr(ctx, value); - let m_box = lower_expr(ctx, map)?; - let k_box = lower_expr(ctx, key)?; - // #6970: the receiver and the key are finished, but every branch - // below lowers `value` next, and that lowering can collect. Until - // the runtime call both live only in SSA registers, so a collection - // there sweeps them: `m.set(fresh(k), churn(N))` aborted inside - // `js_map_set` on a key whose header had been recycled. + // #6970: each operand is finished before the next is lowered, and + // both are live in nothing but SSA registers until the runtime + // call. `m.set(fresh(k), churn(N))` aborted inside `js_map_set` on + // a key whose header had been recycled. + // + // Root each one BEFORE lowering the next, not after the whole list: + // the receiver's exposure starts at `key`'s lowering, not `value`'s, + // and rooting a list that is already lowered can publish an + // already-dangling pointer into a scanned slot — strictly worse + // than not rooting at all. + let key_collects = temp_root::expr_may_trigger_gc(ctx, key); let value_collects = temp_root::expr_may_trigger_gc(ctx, value); let map_key_operands: [&Expr; 2] = [map, key]; - let roots = temp_root::root_operands( - ctx, - &map_key_operands, - &[&m_box, &k_box], - &[value_collects, value_collects], - ); + let mut roots = temp_root::root_operands_begin(2); + let m_box = lower_expr(ctx, map)?; + roots.push(ctx, map, &m_box, key_collects || value_collects); + let k_box = lower_expr(ctx, key)?; + roots.push(ctx, key, &k_box, value_collects); // Unbox eagerly only on the unprotected path, so its IR — including // register numbering — is exactly what it was before this change. // On the protected path the handle has to come from the *re-read* diff --git a/crates/perry-codegen/src/expr/temp_root.rs b/crates/perry-codegen/src/expr/temp_root.rs index 0473e86cc5..fee9bdd818 100644 --- a/crates/perry-codegen/src/expr/temp_root.rs +++ b/crates/perry-codegen/src/expr/temp_root.rs @@ -297,55 +297,6 @@ pub(crate) struct RootedOperands { guard: Option, } -/// Protect `values` across a collection point the caller knows about. -/// -/// `collects[i]` says "something between operand `i` and the consuming call can -/// reach a collection point". The caller supplies it because the hazard is not -/// visible in an expression list: for `m.set(k, v)` it is `v`'s lowering, for -/// `new C(a, b)` it is the instance allocation. -/// -/// From that one flag two decisions follow, and an operand needs exactly one of -/// them: -/// -/// - [`operand_needs_root`] → push a temp-root slot, because nothing else keeps -/// this value alive; -/// - otherwise [`operand_is_reloadable`] → emit no runtime call, but re-load the -/// value at the re-read point, because its storage is a registered root that -/// evacuation *rewrites* while the cached register keeps the old address. -/// -/// When `collects[i]` is false neither applies: nothing can be swept and nothing -/// can move, so the register is reused and the IR is exactly what it was. -pub(crate) fn root_operands( - ctx: &mut FnCtx<'_>, - operands: &[&Expr], - values: &[&str], - collects: &[bool], -) -> RootedOperands { - let mut slots = Vec::with_capacity(values.len()); - let mut reloadable = Vec::with_capacity(values.len()); - let mut guard: Option = None; - for (i, value) in values.iter().enumerate() { - let collects_here = collects.get(i).copied().unwrap_or(false); - let needs_root = collects_here && operand_needs_root(ctx, operands[i]); - if needs_root { - let idx = temp_root_push_double(ctx, value); - if guard.is_none() { - guard = Some(idx.clone()); - } - slots.push(Some(idx)); - } else { - slots.push(None); - } - reloadable.push(!needs_root && collects_here && operand_is_reloadable(operands[i])); - } - RootedOperands { - slots, - values: values.iter().map(|v| (*v).to_string()).collect(), - reloadable, - guard, - } -} - /// Does this operand read a location the collector *rewrites in place*, so that /// re-lowering it after a collection yields the corrected address? /// @@ -366,7 +317,70 @@ pub(crate) fn operand_is_reloadable(expr: &Expr) -> bool { ) } +/// Build the protection **incrementally**, one operand at a time, so each is +/// rooted before the next one is lowered. +/// +/// That ordering is the whole point. Lowering every operand first and rooting +/// the finished list afterwards is not merely late, it is *worse than doing +/// nothing*: by then an earlier operand may already have been swept, and the +/// push publishes a dangling pointer into a slot the collector scans. That is +/// what turned #6969 from a silent wrong answer into a SIGSEGV, and it is why +/// `m.set(k, v)` roots `map` before `key` is lowered rather than after. +/// +/// See [`RootedOperands::push`] for the per-operand contract. +pub(crate) fn root_operands_begin(capacity: usize) -> RootedOperands { + RootedOperands { + slots: Vec::with_capacity(capacity), + values: Vec::with_capacity(capacity), + reloadable: Vec::with_capacity(capacity), + guard: None, + } +} + impl RootedOperands { + /// Record one already-lowered operand. + /// + /// `collects` says "something between this operand and the consuming call + /// can reach a collection point" — the caller supplies it because the + /// hazard is not visible in an expression list: for `m.set(k, v)` the + /// receiver's window covers both `key`'s lowering and `value`'s, while the + /// key's covers only `value`'s. + /// + /// From that flag two decisions follow, and an operand needs exactly one: + /// + /// - [`operand_needs_root`] → push a temp-root slot, because nothing else + /// keeps this value alive; + /// - otherwise [`operand_is_reloadable`] → emit no runtime call, but + /// re-load the value at the re-read point, because its storage is a + /// registered root that evacuation *rewrites* while the cached register + /// keeps the old address. + /// + /// When `collects` is false neither applies: nothing can be swept and + /// nothing can move, so the register is reused and the IR is unchanged. + pub(crate) fn push( + &mut self, + ctx: &mut FnCtx<'_>, + operand: &Expr, + value: &str, + collects: bool, + ) { + let needs_root = collects && operand_needs_root(ctx, operand); + if needs_root { + let idx = temp_root_push_double(ctx, value); + // The FIRST slot pushed is the guard: truncating it drops every + // slot above it too, so one call releases the whole group. + if self.guard.is_none() { + self.guard = Some(idx.clone()); + } + self.slots.push(Some(idx)); + } else { + self.slots.push(None); + } + self.reloadable + .push(!needs_root && collects && operand_is_reloadable(operand)); + self.values.push(value.to_string()); + } + /// Re-read every operand after the collection point. /// /// Three cases, and the third is the subtle one: diff --git a/crates/perry-codegen/src/lower_call/new.rs b/crates/perry-codegen/src/lower_call/new.rs index 9f9c1dd272..e1fafd2bd4 100644 --- a/crates/perry-codegen/src/lower_call/new.rs +++ b/crates/perry-codegen/src/lower_call/new.rs @@ -131,6 +131,43 @@ pub(crate) fn lower_new_member_captured( lower_new_impl(ctx, class_name, args, true) } +/// Refresh `lowered_args` after something that may have collected (#6969). +/// +/// Two cases, and both are mandatory rather than defensive: +/// +/// - a **rooted** argument is re-read from its slot, because the slot is a +/// *mutable* root that an evacuating cycle rewrites in place, leaving the +/// register pushed beforehand stale; +/// - an argument that was NOT rooted because it reads a registered root (a +/// shadow-slotted local, a module global, a string literal) is **re-loaded**. +/// Those are never swept, but evacuation rewrote their storage too, so the +/// cached register points at where the value used to be. Re-lowering emits +/// the load again and costs no runtime call. +/// +/// Called after the instance allocation and again before the late consumers +/// that sit behind further arbitrary lowering (field initializers, an inlined +/// constructor body) — each of those is another chance to relocate. +fn refresh_rooted_args( + ctx: &mut FnCtx<'_>, + args: &[Expr], + lowered_args: &mut [String], + arg_roots: &[Option], +) -> Result<()> { + for (i, (value, slot)) in lowered_args.iter_mut().zip(arg_roots.iter()).enumerate() { + match slot { + Some(idx) => { + let idx = idx.clone(); + *value = temp_root::temp_root_get_double(ctx, &idx); + } + None if temp_root::operand_is_reloadable(&args[i]) => { + *value = lower_constructor_arg(ctx, &args[i])?; + } + None => {} + } + } + Ok(()) +} + fn lower_new_impl( ctx: &mut FnCtx<'_>, class_name: &str, @@ -803,27 +840,9 @@ fn lower_new_impl_inner( ) }; let obj_box = nanbox_pointer_inline(ctx.block(), &obj_handle); - // #6969: the allocation above has run, so re-read every rooted argument. - // Mandatory rather than defensive — the slots are *mutable* roots, so an - // evacuating cycle rewrote them and the registers pushed earlier are stale. - // Every `lowered_args` consumer below this point sees the re-read values. - for (i, (value, slot)) in lowered_args.iter_mut().zip(arg_roots.iter()).enumerate() { - match slot { - Some(idx) => { - let idx = idx.clone(); - *value = temp_root::temp_root_get_double(ctx, &idx); - } - // Not rooted because it reads a registered root (a shadow-slotted - // local, a module global, a string literal). Those are never swept - // — but an evacuating cycle REWROTE their storage, so the register - // loaded before the allocation points at the pre-move address. - // Re-lowering emits the load again and costs no runtime call. - None if temp_root::operand_is_reloadable(&args[i]) => { - *value = lower_constructor_arg(ctx, &args[i])?; - } - None => {} - } - } + // #6969: the instance allocation has run, so refresh every argument before + // the constructor consumes them. + refresh_rooted_args(ctx, args, &mut lowered_args, &arg_roots)?; // Constructor bodies may contain terminating recursive construction // shapes such as `if (typeof opts === "function") return new C(...)`. @@ -1541,6 +1560,9 @@ fn lower_new_impl_inner( // Walked to an ancestor — call its ctor with this and forwarded args. // `...rest` ctors get the trailing args packed into one array // for the final slot (mirrors method_has_rest, #672). + // Field initializers / an inlined constructor body were lowered + // between the instance allocation and here, so refresh again. + refresh_rooted_args(ctx, args, &mut lowered_args, &arg_roots)?; let marshalled = marshal_imported_ctor_args(ctx, &ctor, &lowered_args); let mut ctor_args: Vec<(crate::types::LlvmType, &str)> = Vec::with_capacity(1 + marshalled.len()); @@ -1578,6 +1600,9 @@ fn lower_new_impl_inner( // Pad missing optional args with TAG_UNDEFINED so the constructor // doesn't read garbage from stale registers, and pack the rest // slot into an array when the ctor's last param is `...rest`. + // Field initializers / an inlined constructor body were lowered + // between the instance allocation and here, so refresh again. + refresh_rooted_args(ctx, args, &mut lowered_args, &arg_roots)?; let marshalled = marshal_imported_ctor_args(ctx, &ctor, &lowered_args); // Pass `this` as NaN-boxed double (same as compile_method's this_arg). let mut ctor_args: Vec<(crate::types::LlvmType, &str)> = @@ -1664,6 +1689,9 @@ fn lower_new_impl_inner( "js_get_dynamic_parent_value", &[(I32, &cid.to_string())], ); + // Same here: the dynamic-parent `super(...)` buffer is filled long + // after the allocation, behind further lowering. + refresh_rooted_args(ctx, args, &mut lowered_args, &arg_roots)?; let (args_ptr, args_len) = if lowered_args.is_empty() { ("null".to_string(), "0".to_string()) } else { From d8e07a2d65bc01ab5077485991503ced0b8bac32 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Wed, 29 Jul 2026 10:38:06 +0200 Subject: [PATCH 14/15] fix(gc): snapshot mutable constructor operands instead of re-lowering them CodeRabbit review finding on #6983, and a miscompile I introduced rather than a rooting gap. operand_is_reloadable admitted LocalGet and GlobalGet on the grounds that both are registered roots whose storage evacuation rewrites, so re-lowering recovers the post-move address for free. It does -- but it also reads the variable's CURRENT value, and 'current' is after the later arguments, the field initializers and possibly an inlined constructor body have run. Any of those may have reassigned it. let g = 'before'; function bump() { g = 'after'; return 1; } const c = new C(g, bump()); // c.p must be 'before' printed 'after' on this branch; node and main print 'before'. Only provably immutable sources may be re-loaded, so operand_is_reloadable is now string literals alone. Locals and globals get a real temp-root slot, which preserves the call-time value AND is rewritten on evacuation -- the only option that satisfies both. Regression test: test_gap_ctor_arg_capture_order.ts, verified to fail with the old predicate ('global captured: after'). --- .../6983-operand-temporaries-precise-roots.md | 22 +++++---- crates/perry-codegen/src/expr/temp_root.rs | 32 +++++++++---- test-files/test_gap_ctor_arg_capture_order.ts | 45 +++++++++++++++++++ 3 files changed, 81 insertions(+), 18 deletions(-) create mode 100644 test-files/test_gap_ctor_arg_capture_order.ts diff --git a/changelog.d/6983-operand-temporaries-precise-roots.md b/changelog.d/6983-operand-temporaries-precise-roots.md index ee79e76665..d1d3a17bdc 100644 --- a/changelog.d/6983-operand-temporaries-precise-roots.md +++ b/changelog.d/6983-operand-temporaries-precise-roots.md @@ -42,15 +42,19 @@ because a local can be pointer-valued with no shadow slot and therefore no precise root at all (that is #6968). - A temp root buys two things — liveness, and a location the collector rewrites - — and the suppressions only give up the first. A suppressed operand reads a - registered root, so it can never be *swept*; but evacuation **rewrites that - storage**, leaving the pre-collection register pointing at where the object - used to be. `operand_is_reloadable` therefore re-emits the load at the - re-read point instead of reusing the register: correct under relocation, and - a plain `load` rather than a runtime call. This is the same staleness #6981 - reports one layer in, for a raw typed-array pointer under the specialized - ABI. + A temp root buys **three** things, and an operand needs all of them: liveness + (not swept), a location the collector rewrites (survives relocation), and the + value the call actually observed (not a later one). A registered root — a + local, a module global, a string literal — supplies the first for free, which + tempts you to skip the slot. Skipping it is only safe when the source is also + *immutable*: re-loading a local or global recovers the right address after + evacuation but reads its value **now**, after later arguments, field + initializers and possibly an inlined constructor body have run, any of which + may have reassigned it. `new C(g, bump())` where `bump()` sets `g` then + captured the post-`bump()` value — a miscompile, not a rooting bug, caught in + review and covered by `test_gap_ctor_arg_capture_order.ts`. So only string + literals are re-loaded; locals and globals get a real slot, which preserves + the call-time value and is rewritten on evacuation. Cost: on a probe of already-safe shapes (`"user_" + i`, `[1,2,3]`, `{a:i,b:total}`, all-local argument lists, `m.set(k, 1)`, `m.get(k)`, diff --git a/crates/perry-codegen/src/expr/temp_root.rs b/crates/perry-codegen/src/expr/temp_root.rs index fee9bdd818..b1a11e9deb 100644 --- a/crates/perry-codegen/src/expr/temp_root.rs +++ b/crates/perry-codegen/src/expr/temp_root.rs @@ -311,10 +311,20 @@ pub(crate) struct RootedOperands { /// This is the same staleness #6981 reports one layer in (a raw typed-array /// pointer passed under the specialized ABI). pub(crate) fn operand_is_reloadable(expr: &Expr) -> bool { - matches!( - expr, - Expr::LocalGet(_) | Expr::GlobalGet(_) | Expr::String(_) - ) + // ONLY provably immutable sources. A string literal always re-lowers to a + // load of the same `__perry_init_strings_*` handle, so re-reading it can + // never observe a different value. + // + // A local or a module global must NOT be here, even though both are + // registered roots whose storage evacuation rewrites. Re-lowering one reads + // its value *now*, and "now" is after the later arguments, the field + // initializers and possibly an inlined constructor body have run — any of + // which may have reassigned it. `new C(g, bump())` where `bump()` sets + // `g` must capture `g`'s value at call time; re-lowering produced the + // post-`bump()` value, a miscompile rather than a rooting bug. Those + // operands get a real temp root instead: the slot preserves the call-time + // value AND the collector rewrites it on evacuation. + matches!(expr, Expr::String(_)) } /// Build the protection **incrementally**, one operand at a time, so each is @@ -529,11 +539,15 @@ pub(crate) fn operand_needs_root(ctx: &FnCtx<'_>, expr: &Expr) -> bool { if super::expr_is_known_non_pointer_shadow_value(ctx, expr) { return false; } - match expr { - Expr::String(_) | Expr::GlobalGet(_) => false, - Expr::LocalGet(id) => !ctx.shadow_slot_map.contains_key(id), - _ => true, - } + // Only a string literal is suppressed: it is a registered root AND + // immutable, so `operand_is_reloadable` can recover it with a plain load. + // + // Locals and module globals are deliberately NOT suppressed. Being a + // registered root buys liveness, but the value has to survive relocation + // *and* stay the value the call actually observed — and a re-load gives up + // the second. Rooting is the only thing that gives both, so they pay for a + // slot. + !matches!(expr, Expr::String(_)) } /// Open an expression-scope temp-root barrier for a call/constructor whose diff --git a/test-files/test_gap_ctor_arg_capture_order.ts b/test-files/test_gap_ctor_arg_capture_order.ts new file mode 100644 index 0000000000..a11dd22a41 --- /dev/null +++ b/test-files/test_gap_ctor_arg_capture_order.ts @@ -0,0 +1,45 @@ +// Constructor arguments must capture the value they had at `new` time, even +// when a LATER argument, a field initializer or the constructor body reassigns +// the variable they came from. +// +// Regression guard for the GC-rooting work (#6969/#6983): an argument that +// reads a registered root (a local, a module global) must be SNAPSHOT into a +// temp-root slot, not re-lowered after the fact. Re-lowering recovers the right +// address after an evacuating cycle but reads the variable's *current* value — +// which is a miscompile, not a rooting fix. Only immutable sources (string +// literals) may be re-loaded. +let g = "before"; +function bump(): number { + g = "after"; + return 1; +} + +class C { + p: unknown; + q: unknown; + constructor(p: unknown, q: unknown) { + this.p = p; + this.q = q; + } +} + +const sink: unknown[] = []; + +// Argument 0 reads `g` before bump() reassigns it. +const c = new C(g, bump()); +sink.push(c); // force a real allocation (defeat scalar replacement) +console.log("global captured:", c.p, "now:", g); + +// Same, with a local reassigned by the constructor body itself. +let local = "L0"; +class D { + v: unknown; + constructor(v: unknown) { + local = "L1"; + this.v = v; + } +} +const d = new D(local); +sink.push(d); +console.log("local captured:", d.v, "now:", local); +console.log("allocated:", sink.length); From a10818316b8871ab415d0edf62627d5dc71081e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Wed, 29 Jul 2026 16:13:51 +0200 Subject: [PATCH 15/15] fix(codegen): don't emit the string-method root release after a terminator The unknown-property arm of lower_string_method_dispatch throws via js_throw_type_error_not_a_function, emits `unreachable`, then still returns Ok(placeholder) so the caller has a register to phi against. Control therefore returns to the receiver's release site with the block already terminated, and appending js_gc_temp_root_truncate there emits an instruction after the terminator -- invalid IR. Guard with is_terminated(), the idiom already used in codegen/{closure,entry}.rs. Skipping the release is sound: unreachable means no path resumes, and the temp-root stack is cut by the enclosing scope regardless. No regression test: an attempted HIR-level reproducer never reached the throwing arm (no `unreachable` in the emitted IR), so it passed with and without the guard. A green-either-way test is worse than none, so it was removed and the reachability is recorded as unproven. Reported by CodeRabbit on #6983. --- .../perry-codegen/src/lower_string_method.rs | 23 ++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/crates/perry-codegen/src/lower_string_method.rs b/crates/perry-codegen/src/lower_string_method.rs index 613c8bd3ee..9bceb71729 100644 --- a/crates/perry-codegen/src/lower_string_method.rs +++ b/crates/perry-codegen/src/lower_string_method.rs @@ -168,8 +168,29 @@ pub(crate) fn lower_string_method( let result = lower_string_method_dispatch(ctx, object, property, args, &recv_box, &recv_root); // Released only after the dispatch's consuming runtime call has run: that // call allocates while it reads the receiver. + // + // The `is_terminated` guard is load-bearing, not defensive. The + // unknown-property arm of the dispatch throws and emits `unreachable`, then + // still returns `Ok(placeholder)` so callers have a register to phi against + // — so control reaches here with the block already terminated. Appending the + // truncate there would emit an instruction after the terminator: invalid IR, + // reachable from `("a" + churn()).nope(obj)` (an unrecognized string method + // whose arguments can collect, which is what sets `recv_root` at all). + // Skipping the release is sound: `unreachable` means no path resumes, and + // the temp-root stack is cut by the enclosing scope regardless. + // + // NOT covered by a regression test, deliberately: an attempted HIR-level + // reproducer (`("a" + "b").nope({})`) never reached the throwing arm — the + // emitted IR contained no `unreachable` at all — so the test passed with + // and without this guard. A test that is green either way is worse than no + // test, so it was removed rather than shipped. The guard is kept as + // defense-in-depth: emitting after a terminator is never correct, and the + // check is free. Reachability of the arm from a TypeScript source remains + // unproven; see the PR body. if let Some(idx) = &recv_root { - temp_root_truncate(ctx, idx); + if !ctx.block().is_terminated() { + temp_root_truncate(ctx, idx); + } } result }