From 140baf7ff3ef0cc00a7b155e1ef1f7521fec7e45 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Wed, 29 Jul 2026 07:15:32 +0200 Subject: [PATCH 1/3] fix(gc): make argument temporaries precise roots (#6951) With the conservative native-stack scan disabled -- precise/shadow-stack roots only -- a collection landing during argument evaluation dropped console.log's string-literal argument. No crash, no diagnostic. Harder shapes (string concat with an allocating right operand, `new C(str, f())`) segfaulted. Root cause: the shadow stack roots named locals. It has no slot for the values that exist only between two instructions, and an LLVM SSA register is not a GC root. `console.log(a, b)` lowers to `js_array_alloc(2)` plus one `js_array_push_f64` per argument, with the accumulator threaded through an SSA register. That register held the ONLY reference to everything pushed so far -- argument 0 included -- across argument 1's evaluation. A collection there swept the half-built array and the following push landed in recycled memory whose length had been reset to 0, so the label disappeared and only the number printed. Conservative stack scanning hid this: gc_check_trigger forces a full conservative scan on both automatic arms, while gc/roots.rs's nominal production default is Auto -> SkipDisabled. The scan was doing load-bearing correctness work, not acting as a safety net. New mechanism: a per-thread temp-root stack callable from generated code (gc/roots/temp_roots.rs), registered as a budgeted mutable root scanner, so slots are both MARKED and REWRITTEN. Slots decode through visit_heap_word_u64_slot, accepting both word forms the gc::root_words contract admits (NaN-boxed values and bare heap addresses -- generated code pushes both). Generated code pushes before the collection point, re-reads after (mandatory: an evacuating cycle rewrites the slot), and truncates after the consuming call. ShadowSavepoint carries the depth, so the longjmp unwind that already restores the shadow stack restores this stack with it. Rooted sites: the variadic argument accumulator (console log/info/warn/ error/debug/trace/assert/timeLog), the string-concat operand pair and the n-way concat chain (template literals, log lines), the object-literal handle across its initializers, and array-literal element values. Emission is gated on "does anything after this reach a collection point" -- `"a" + i`, `[1, 2, 3]` and all-local argument lists emit byte-identical IR to before. test-parity/gc_repsel_triage.txt: both triaged cells clear. The matrix's cons_scan_off arm (in the PR arm set) is now a hard gate on this shape. --- .../perry-codegen/src/expr/array_literal.rs | 38 ++- crates/perry-codegen/src/expr/binary.rs | 41 +-- crates/perry-codegen/src/expr/mod.rs | 1 + .../perry-codegen/src/expr/object_literal.rs | 31 ++- crates/perry-codegen/src/expr/temp_root.rs | 260 ++++++++++++++++++ .../src/lower_call/console_promise.rs | 80 +++--- .../perry-codegen/src/lower_string_method.rs | 63 +++-- .../perry-codegen/src/runtime_decls/arrays.rs | 15 + .../tests/temp_root_argument_temporaries.rs | 227 +++++++++++++++ crates/perry-runtime/src/gc/mod.rs | 10 + crates/perry-runtime/src/gc/roots.rs | 12 + .../src/gc/roots/shadow_stack.rs | 10 + .../perry-runtime/src/gc/roots/temp_roots.rs | 197 +++++++++++++ crates/perry-runtime/src/gc/tests/mod.rs | 1 + .../perry-runtime/src/gc/tests/temp_roots.rs | 193 +++++++++++++ test-parity/gc_repsel_triage.txt | 10 +- 16 files changed, 1102 insertions(+), 87 deletions(-) create mode 100644 crates/perry-codegen/src/expr/temp_root.rs create mode 100644 crates/perry-codegen/tests/temp_root_argument_temporaries.rs create mode 100644 crates/perry-runtime/src/gc/roots/temp_roots.rs create mode 100644 crates/perry-runtime/src/gc/tests/temp_roots.rs diff --git a/crates/perry-codegen/src/expr/array_literal.rs b/crates/perry-codegen/src/expr/array_literal.rs index b1ca9e7e21..a2ee480f4c 100644 --- a/crates/perry-codegen/src/expr/array_literal.rs +++ b/crates/perry-codegen/src/expr/array_literal.rs @@ -4,8 +4,9 @@ use anyhow::Result; use perry_hir::Expr; +use super::temp_root::{lower_exprs_rooted, temp_root_release}; use super::{ - emit_jsvalue_slot_store_on_block, expr_produces_non_pointer_bits_by_construction, lower_expr, + emit_jsvalue_slot_store_on_block, expr_produces_non_pointer_bits_by_construction, nanbox_pointer_inline, FnCtx, }; use crate::type_analysis::is_numeric_expr; @@ -33,8 +34,15 @@ use crate::types::{DOUBLE, I32, I64, I8, PTR}; /// allocated slot (offset hasn't advanced past the `fits` check) or a /// header with `length == capacity` and uninitialized elements. No /// allocator call runs between the header write and the element stores, -/// so GC can't run in that window. Element expressions with their own -/// allocations lower to SSA values pinned by conservative stack scanning. +/// so GC can't run in that window. +/// +/// #6951: element values themselves are a different matter. They are lowered +/// before the allocation and each one then sits in an SSA register across +/// every later element's evaluation — which is not a root, and was only ever +/// covered by conservative native-stack scanning. `[freshString(), f()]` lost +/// its first element as soon as `f` collected. `lower_exprs_rooted` roots each +/// value that has an allocating element after it, and emits nothing for the +/// all-literal / all-local shapes. pub(crate) fn lower_array_literal(ctx: &mut FnCtx<'_>, elements: &[Expr]) -> Result { let n = elements.len(); let all_numeric_elements = elements.iter().all(|e| is_numeric_expr(ctx, e)); @@ -45,18 +53,18 @@ pub(crate) fn lower_array_literal(ctx: &mut FnCtx<'_>, elements: &[Expr]) -> Res return Ok(nanbox_pointer_inline(ctx.block(), &arr)); } - // Evaluate all element expressions *before* allocating. This keeps each - // value in an SSA register (spilled to stack if needed; reachable by the - // conservative stack scanner) so nested allocations inside element - // expressions don't see a half-initialized outer array. - let mut vals = Vec::with_capacity(n); + // Evaluate all element expressions *before* allocating, so nested + // allocations inside element expressions don't see a half-initialized + // outer array. Each evaluated value is kept in a temp root until the last + // element has been lowered (#6951). let mut layout_notes_needed = Vec::with_capacity(n); for value_expr in elements { layout_notes_needed.push(!expr_produces_non_pointer_bits_by_construction( ctx, value_expr, )); - vals.push(lower_expr(ctx, value_expr)?); } + let element_refs: Vec<&Expr> = elements.iter().collect(); + let (vals, element_guard) = lower_exprs_rooted(ctx, &element_refs)?; // #5391: oversized modules outline array-literal construction. The inline // bump-alloc + N×(store + layout-note + barrier) sequence makes minified @@ -75,7 +83,9 @@ pub(crate) fn lower_array_literal(ctx: &mut FnCtx<'_>, elements: &[Expr]) -> Res let arr = ctx .block() .call(I64, "js_array_from_values", &[(PTR, &buf), (I32, &n_str)]); - return Ok(nanbox_pointer_inline(ctx.block(), &arr)); + let boxed = nanbox_pointer_inline(ctx.block(), &arr); + temp_root_release(ctx, element_guard); + return Ok(boxed); } // Inline bump-allocator path for small literals. Size threshold matches @@ -211,7 +221,9 @@ pub(crate) fn lower_array_literal(ctx: &mut FnCtx<'_>, elements: &[Expr]) -> Res ); } - return Ok(nanbox_pointer_inline(ctx.block(), &user_ptr_as_i64)); + let boxed = nanbox_pointer_inline(ctx.block(), &user_ptr_as_i64); + temp_root_release(ctx, element_guard); + return Ok(boxed); } // Fallback for N > INLINE_MAX_ELEMENTS: keep the extern call + N inline @@ -250,5 +262,7 @@ pub(crate) fn lower_array_literal(ctx: &mut FnCtx<'_>, elements: &[Expr]) -> Res .call(I32, "js_array_mark_numeric_f64_layout", &[(I64, &arr)]); } - Ok(nanbox_pointer_inline(ctx.block(), &arr)) + let boxed = nanbox_pointer_inline(ctx.block(), &arr); + temp_root_release(ctx, element_guard); + Ok(boxed) } diff --git a/crates/perry-codegen/src/expr/binary.rs b/crates/perry-codegen/src/expr/binary.rs index eedffaeb0d..47f0f5eac3 100644 --- a/crates/perry-codegen/src/expr/binary.rs +++ b/crates/perry-codegen/src/expr/binary.rs @@ -22,6 +22,7 @@ use crate::type_analysis::{ }; use crate::types::{DOUBLE, I1, I128, I32, I64}; +use super::temp_root::{lower_operand_pair_rooted, temp_root_release}; use super::{is_known_finite, lower_expr, FnCtx}; fn lower_arithmetic_operand(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result<(String, bool)> { @@ -411,13 +412,14 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { if other_known_primitive { return lower_string_coerce_concat(ctx, left, right, l_is_str, r_is_str); } - let l = lower_expr(ctx, left)?; - let r = lower_expr(ctx, right)?; - return Ok(ctx.block().call( + let (l, r, guard) = lower_operand_pair_rooted(ctx, left, right)?; + let sum = ctx.block().call( DOUBLE, "js_dynamic_string_or_number_add", &[(DOUBLE, &l), (DOUBLE, &r)], - )); + ); + temp_root_release(ctx, guard); + return Ok(sum); } if is_bigint_expr(ctx, left) && is_bigint_expr(ctx, right) { if let Some(value) = try_lower_small_bigint_literal_binary( @@ -428,13 +430,12 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { ) { return Ok(value); } - let l = lower_expr(ctx, left)?; - let r = lower_expr(ctx, right)?; - return Ok(ctx.block().call( - DOUBLE, - "js_dynamic_add", - &[(DOUBLE, &l), (DOUBLE, &r)], - )); + let (l, r, guard) = lower_operand_pair_rooted(ctx, left, right)?; + let sum = + ctx.block() + .call(DOUBLE, "js_dynamic_add", &[(DOUBLE, &l), (DOUBLE, &r)]); + temp_root_release(ctx, guard); + return Ok(sum); } // Refs #486: neither operand is statically known. Per JS // spec for `+`, if EITHER side is a string at runtime, the @@ -454,13 +455,14 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { && crate::type_analysis::is_numeric_expr(ctx, right)) || add_operands_have_pod_materialization_hazard(ctx, left, right) { - let l = lower_expr(ctx, left)?; - let r = lower_expr(ctx, right)?; - return Ok(ctx.block().call( + let (l, r, guard) = lower_operand_pair_rooted(ctx, left, right)?; + let sum = ctx.block().call( DOUBLE, "js_dynamic_string_or_number_add", &[(DOUBLE, &l), (DOUBLE, &r)], - )); + ); + temp_root_release(ctx, guard); + return Ok(sum); } } // BigInt arithmetic fast path. NaN-tagged bigints compare @@ -483,11 +485,12 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { { return Ok(value); } - let l = lower_expr(ctx, left)?; - let r = lower_expr(ctx, right)?; - return Ok(ctx + let (l, r, guard) = lower_operand_pair_rooted(ctx, left, right)?; + let value = ctx .block() - .call(DOUBLE, fname, &[(DOUBLE, &l), (DOUBLE, &r)])); + .call(DOUBLE, fname, &[(DOUBLE, &l), (DOUBLE, &r)]); + temp_root_release(ctx, guard); + return Ok(value); } // A non-primitive operand may `ToNumeric` to a BigInt at runtime // (`Object(1n)`, or an object with a BigInt-returning diff --git a/crates/perry-codegen/src/expr/mod.rs b/crates/perry-codegen/src/expr/mod.rs index 389127e1b4..30e7a27d6d 100644 --- a/crates/perry-codegen/src/expr/mod.rs +++ b/crates/perry-codegen/src/expr/mod.rs @@ -133,6 +133,7 @@ mod dispatch; mod record_value; mod shadow_slot; mod slot_rep; +pub(crate) mod temp_root; pub(crate) use slot_rep::{ canonical_i32_locals_enabled, canonical_local_i32_slot, canonical_str_locals_enabled, collect_canonical_str_ineligible_locals, collect_closure_referenced_locals, diff --git a/crates/perry-codegen/src/expr/object_literal.rs b/crates/perry-codegen/src/expr/object_literal.rs index 43f895ba5d..98509316df 100644 --- a/crates/perry-codegen/src/expr/object_literal.rs +++ b/crates/perry-codegen/src/expr/object_literal.rs @@ -5,6 +5,9 @@ use anyhow::Result; use perry_hir::types::Type as HirType; use perry_hir::Expr; +use super::temp_root::{ + any_may_trigger_gc, rooted_handle_begin, rooted_handle_get, rooted_handle_release, +}; use super::{lower_expr, nanbox_pointer_inline, FnCtx}; use crate::nanbox::POINTER_MASK_I64; use crate::type_analysis::{compute_auto_captures, is_numeric_expr}; @@ -308,6 +311,12 @@ pub(crate) fn lower_object_literal( props: &[(String, Expr)], expected_ty: Option<&HirType>, ) -> Result { + // #6951: the object handle is allocated BEFORE the property values are + // lowered and lives in an SSA register across all of them. `{ a: s, b: f() }` + // therefore had its half-built object swept by `f`'s collection, and the + // remaining field stores landed in recycled memory. Root the handle when any + // initializer can collect; literals of plain locals emit no extra IR. + let protect_handle = any_may_trigger_gc(props.iter().map(|(_, v)| v)); let field_count = props.len() as u32; let zero_str = "0".to_string(); let n_str = field_count.to_string(); @@ -365,16 +374,21 @@ pub(crate) fn lower_object_literal( ], ); + let rooted = rooted_handle_begin(ctx, &obj_handle, protect_handle); for (i, (_, value_expr)) in props.iter().enumerate() { let v = lower_expr(ctx, value_expr)?; let idx_str = i.to_string(); + let obj_handle = rooted_handle_get(ctx, &rooted); ctx.block().call_void( "js_object_set_unboxed_f64_field", &[(I64, &obj_handle), (I32, &idx_str), (DOUBLE, &v)], ); } + let obj_handle = rooted_handle_get(ctx, &rooted); emit_unboxed_object_layout_init(ctx, &obj_handle); - return Ok(nanbox_pointer_inline(ctx.block(), &obj_handle)); + let boxed = nanbox_pointer_inline(ctx.block(), &obj_handle); + rooted_handle_release(ctx, rooted); + return Ok(boxed); } if !any_method_closure && field_count > 0 { @@ -416,9 +430,11 @@ pub(crate) fn lower_object_literal( ], ); + let rooted = rooted_handle_begin(ctx, &obj_handle, protect_handle); for (i, (_, value_expr)) in props.iter().enumerate() { let v = lower_expr(ctx, value_expr)?; let idx_str = i.to_string(); + let obj_handle = rooted_handle_get(ctx, &rooted); // Issue #448: the runtime `js_object_set_field` takes its // value as `JSValue` (`#[repr(transparent)] u64`), which the // System V / AArch64 / Win64 ABIs all pass in a *general*- @@ -439,15 +455,19 @@ pub(crate) fn lower_object_literal( &[(I64, &obj_handle), (I32, &idx_str), (I64, &v_bits)], ); } + let obj_handle = rooted_handle_get(ctx, &rooted); if let Some(layout) = typed_layout.as_ref() { emit_object_typed_shape_init(ctx, &obj_handle, layout); } - return Ok(nanbox_pointer_inline(ctx.block(), &obj_handle)); + let boxed = nanbox_pointer_inline(ctx.block(), &obj_handle); + rooted_handle_release(ctx, rooted); + return Ok(boxed); } let obj_handle = ctx .block() .call(I64, "js_object_alloc", &[(I32, &zero_str), (I32, &n_str)]); + let rooted = rooted_handle_begin(ctx, &obj_handle, protect_handle); // Track `(closure_value_double, reserved_this_slot_idx)` for each // method closure that needs `this` patched after the object is @@ -472,6 +492,7 @@ pub(crate) fn lower_object_literal( let v = lower_expr(ctx, value_expr)?; this_patches.push((v.clone(), this_idx)); + let obj_handle = rooted_handle_get(ctx, &rooted); let blk = ctx.block(); let key_box = blk.load(DOUBLE, &key_handle_global); let key_bits = blk.bitcast_double_to_i64(&key_box); @@ -484,6 +505,7 @@ pub(crate) fn lower_object_literal( } let v = lower_expr(ctx, value_expr)?; + let obj_handle = rooted_handle_get(ctx, &rooted); let blk = ctx.block(); let key_box = blk.load(DOUBLE, &key_handle_global); let key_bits = blk.bitcast_double_to_i64(&key_box); @@ -497,6 +519,7 @@ pub(crate) fn lower_object_literal( // Patch each method closure's reserved `this` slot with the object // pointer (NaN-boxed). Done AFTER all fields are set so every // method sees the fully-initialized object. + let obj_handle = rooted_handle_get(ctx, &rooted); if !this_patches.is_empty() { let blk = ctx.block(); let obj_tagged = { @@ -519,5 +542,7 @@ pub(crate) fn lower_object_literal( emit_object_typed_shape_init(ctx, &obj_handle, layout); } - Ok(nanbox_pointer_inline(ctx.block(), &obj_handle)) + let boxed = nanbox_pointer_inline(ctx.block(), &obj_handle); + rooted_handle_release(ctx, rooted); + Ok(boxed) } diff --git a/crates/perry-codegen/src/expr/temp_root.rs b/crates/perry-codegen/src/expr/temp_root.rs new file mode 100644 index 0000000000..c9361bba6a --- /dev/null +++ b/crates/perry-codegen/src/expr/temp_root.rs @@ -0,0 +1,260 @@ +//! Precise rooting for expression temporaries (#6951). +//! +//! The shadow stack roots named locals. It has no slot for the values that +//! only exist between two instructions, and an LLVM SSA register is not a GC +//! root — so an accumulator array, or an already-evaluated operand waiting for +//! its sibling, dies if the sibling's evaluation collects. Conservative native +//! stack scanning hid that (see `perry-runtime/src/gc/roots/temp_roots.rs`); +//! with `PERRY_CONSERVATIVE_STACK_SCAN=off` it is a live use-after-free. +//! +//! The emission contract, in the order it must appear: +//! +//! ```text +//! %idx = call i32 @js_gc_temp_root_push(i64 ) ; before the collection point +//! ... ; anything that may collect +//! %v = call i64 @js_gc_temp_root_get(i32 %idx) ; ALWAYS re-read +//! call void @js_gc_temp_root_truncate(i32 %idx) ; after the last use +//! ``` +//! +//! Re-reading is mandatory, not defensive: the slot is a *mutable* root, so an +//! evacuating cycle rewrites it and the register pushed beforehand is stale. +//! That is also why this is preferable to widening conservative scanning — +//! conservative roots have to pin, precise ones can move. + +use perry_hir::Expr; + +use crate::types::{DOUBLE, I32, I64}; + +use super::FnCtx; + +/// Push `value_i64` (a bare heap pointer or NaN-boxed bits) and return the +/// slot-index register. +pub(crate) fn temp_root_push_i64(ctx: &mut FnCtx<'_>, value_i64: &str) -> String { + ctx.block() + .call(I32, "js_gc_temp_root_push", &[(I64, value_i64)]) +} + +/// Push a NaN-boxed `double` temporary and return the slot-index register. +pub(crate) fn temp_root_push_double(ctx: &mut FnCtx<'_>, value: &str) -> String { + let bits = ctx.block().bitcast_double_to_i64(value); + temp_root_push_i64(ctx, &bits) +} + +/// Re-read slot `idx` as a raw `i64`. +pub(crate) fn temp_root_get_i64(ctx: &mut FnCtx<'_>, idx: &str) -> String { + ctx.block().call(I64, "js_gc_temp_root_get", &[(I32, idx)]) +} + +/// Re-read slot `idx` as a NaN-boxed `double`. +pub(crate) fn temp_root_get_double(ctx: &mut FnCtx<'_>, idx: &str) -> String { + let bits = temp_root_get_i64(ctx, idx); + ctx.block().bitcast_i64_to_double(&bits) +} + +/// Drop slot `idx` and everything pushed above it. +pub(crate) fn temp_root_truncate(ctx: &mut FnCtx<'_>, idx: &str) { + ctx.block() + .call_void("js_gc_temp_root_truncate", &[(I32, idx)]); +} + +/// Push `value` onto the array held in temp-root slot `idx`, writing the +/// possibly-reallocated array pointer back into the slot. +pub(crate) fn temp_rooted_array_push(ctx: &mut FnCtx<'_>, idx: &str, value: &str) { + ctx.block().call_void( + "js_array_push_f64_temp_rooted", + &[(I32, idx), (DOUBLE, value)], + ); +} + +/// Allocate an argument-accumulator array and root it, returning the +/// temp-root slot index. +/// +/// This is the shape behind every variadic / spread / rest argument list: +/// `js_array_alloc(n)`, then one `js_array_push_f64` per argument, with the +/// accumulator threaded through in an SSA register. That register held the +/// only reference to everything pushed so far — including argument 0 — across +/// the evaluation of argument 1, which is exactly the #6951 repro +/// (`console.log("label", allocatingCall())`). +/// +/// Pair with [`temp_rooted_array_push`] per argument, then +/// [`rooted_array_read`] and [`temp_root_truncate`] — in that order, so the +/// array stays rooted across the call that consumes it. +pub(crate) fn rooted_array_begin(ctx: &mut FnCtx<'_>, cap: &str) -> String { + let arr = ctx.block().call(I64, "js_array_alloc", &[(I32, cap)]); + temp_root_push_i64(ctx, &arr) +} + +/// Read the accumulator back out of its temp-root slot. Does NOT truncate: +/// callers truncate after the consuming call, so the array is still rooted +/// while the consumer runs (formatting an argument list allocates). +pub(crate) fn rooted_array_read(ctx: &mut FnCtx<'_>, idx: &str) -> String { + temp_root_get_i64(ctx, idx) +} + +/// Can lowering `expr` reach a collection point? +/// +/// Deliberately one-sided: `false` must mean "provably allocates nothing", and +/// everything unrecognized answers `true`. A wrong `false` is a +/// use-after-free; a wrong `true` costs two runtime calls on a cold path. +pub(crate) fn expr_may_trigger_gc(expr: &Expr) -> bool { + match expr { + // Immediates and plain slot reads. `LocalGet` reads an alloca, + // `GlobalGet` a module global — neither allocates. + Expr::Undefined + | Expr::Null + | Expr::Bool(_) + | Expr::Number(_) + | Expr::Integer(_) + | Expr::LocalGet(_) + | Expr::GlobalGet(_) => false, + // A string literal is materialized once into a module-global handle by + // `__perry_init_strings_*` and registered as a GC root there; the use + // site is a load. + Expr::String(_) => false, + Expr::Unary { operand, .. } => expr_may_trigger_gc(operand), + Expr::Compare { left, right, .. } => { + expr_may_trigger_gc(left) || expr_may_trigger_gc(right) + } + // `+` on unknown operands can be string concatenation, which allocates; + // every other binary operator is numeric or bitwise. + Expr::Binary { + op, left, right, .. + } => { + matches!(op, perry_hir::BinaryOp::Add) + || expr_may_trigger_gc(left) + || expr_may_trigger_gc(right) + } + Expr::Conditional { + condition, + then_expr, + else_expr, + } => { + expr_may_trigger_gc(condition) + || expr_may_trigger_gc(then_expr) + || expr_may_trigger_gc(else_expr) + } + Expr::Sequence(exprs) => exprs.iter().any(expr_may_trigger_gc), + _ => true, + } +} + +/// Does any expression after index `i` reach a collection point? +/// +/// This is the gate for protecting argument `i`: a value that nothing +/// allocating follows cannot be collected before it is consumed, so the +/// rooting calls would be pure overhead. `"a" + i`, `f(x, y)` on plain locals +/// and `[1, 2, 3]` therefore emit exactly the IR they emitted before #6951. +pub(crate) fn any_later_arg_may_trigger_gc(args: &[Expr], i: usize) -> bool { + args.iter().skip(i + 1).any(expr_may_trigger_gc) +} + +fn any_later_ref_may_trigger_gc(exprs: &[&Expr], i: usize) -> bool { + exprs.iter().skip(i + 1).any(|e| expr_may_trigger_gc(e)) +} + +/// Lower `exprs` left to right, keeping each already-evaluated value precisely +/// rooted across the evaluation of the ones that follow (#6951). +/// +/// Returns the lowered values — **re-read from their roots**, so they are +/// valid after an evacuating cycle — and the guard index the caller must pass +/// to [`temp_root_release`] once the consuming call has run. `None` means +/// nothing needed protecting and no runtime calls were emitted. +pub(crate) fn lower_exprs_rooted( + ctx: &mut FnCtx<'_>, + exprs: &[&Expr], +) -> anyhow::Result<(Vec, Option)> { + let mut values = Vec::with_capacity(exprs.len()); + let mut slots: Vec> = Vec::with_capacity(exprs.len()); + let mut guard: Option = None; + for (i, expr) in exprs.iter().enumerate() { + let value = super::lower_expr(ctx, expr)?; + if any_later_ref_may_trigger_gc(exprs, i) { + 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 guard.is_none() { + guard = Some(idx.clone()); + } + slots.push(Some(idx)); + } else { + slots.push(None); + } + values.push(value); + } + for (value, slot) in values.iter_mut().zip(slots.iter()) { + if let Some(idx) = slot { + *value = temp_root_get_double(ctx, idx); + } + } + Ok((values, guard)) +} + +/// Lower a `left`/`right` operand pair with the same contract as +/// [`lower_exprs_rooted`]. +pub(crate) fn lower_operand_pair_rooted( + ctx: &mut FnCtx<'_>, + left: &Expr, + right: &Expr, +) -> anyhow::Result<(String, String, Option)> { + let (mut values, guard) = lower_exprs_rooted(ctx, &[left, right])?; + let right_value = values.pop().expect("pair lowering yields two values"); + let left_value = values.pop().expect("pair lowering yields two values"); + Ok((left_value, right_value, guard)) +} + +/// Release a guard returned by [`lower_exprs_rooted`]. Call it *after* the +/// consuming call, not before: the consumer allocates while reading these +/// values. +pub(crate) fn temp_root_release(ctx: &mut FnCtx<'_>, guard: Option) { + if let Some(idx) = guard { + temp_root_truncate(ctx, &idx); + } +} + +/// A freshly allocated container handle (object, array, …) that generated code +/// keeps writing into while it lowers the initializer expressions. +/// +/// The handle is a raw `i64` in an SSA register, and every initializer that +/// allocates is a chance for the half-built container to be swept out from +/// under it — the object-literal form of the #6951 accumulator bug. Re-read +/// the handle through [`rooted_handle_get`] before every use. +pub(crate) struct RootedHandle { + slot: Option, + value: String, +} + +/// Root `handle` when `protect` says an upcoming initializer can collect. +/// `protect == false` emits nothing and [`rooted_handle_get`] hands the +/// original register straight back, so unprotected sites keep their old IR. +pub(crate) fn rooted_handle_begin( + ctx: &mut FnCtx<'_>, + handle_i64: &str, + protect: bool, +) -> RootedHandle { + let slot = protect.then(|| temp_root_push_i64(ctx, handle_i64)); + RootedHandle { + slot, + value: handle_i64.to_string(), + } +} + +pub(crate) fn rooted_handle_get(ctx: &mut FnCtx<'_>, handle: &RootedHandle) -> String { + match &handle.slot { + Some(idx) => { + let idx = idx.clone(); + temp_root_get_i64(ctx, &idx) + } + None => handle.value.clone(), + } +} + +pub(crate) fn rooted_handle_release(ctx: &mut FnCtx<'_>, handle: RootedHandle) { + if let Some(idx) = handle.slot { + temp_root_truncate(ctx, &idx); + } +} + +/// Do any of an object literal's / call's initializer expressions collect? +pub(crate) fn any_may_trigger_gc<'a>(exprs: impl IntoIterator) -> bool { + exprs.into_iter().any(expr_may_trigger_gc) +} diff --git a/crates/perry-codegen/src/lower_call/console_promise.rs b/crates/perry-codegen/src/lower_call/console_promise.rs index 386249c20b..dc49d5cebd 100644 --- a/crates/perry-codegen/src/lower_call/console_promise.rs +++ b/crates/perry-codegen/src/lower_call/console_promise.rs @@ -13,6 +13,10 @@ use anyhow::Result; use perry_hir::types::Type as HirType; use perry_hir::Expr; +use crate::expr::temp_root::{ + rooted_array_begin, rooted_array_read, temp_root_get_double, temp_root_push_double, + temp_root_truncate, temp_rooted_array_push, +}; use crate::expr::{ emit_typed_feedback_register_site, lower_expr, nanbox_pointer_inline, FnCtx, TypedFeedbackContract, TypedFeedbackKind, @@ -278,18 +282,15 @@ pub fn try_lower_console_call( ctx.block().call_void("js_console_trace", &[(DOUBLE, &val)]); } else { let cap = (args.len() as u32).to_string(); - let mut current_arr = ctx.block().call(I64, "js_array_alloc", &[(I32, &cap)]); + let acc = rooted_array_begin(ctx, &cap); for arg in args.iter() { let v = lower_expr(ctx, arg)?; - let blk = ctx.block(); - current_arr = blk.call( - I64, - "js_array_push_f64", - &[(I64, ¤t_arr), (DOUBLE, &v)], - ); + temp_rooted_array_push(ctx, &acc, &v); } + let current_arr = rooted_array_read(ctx, &acc); ctx.block() .call_void("js_console_trace_spread", &[(I64, ¤t_arr)]); + temp_root_truncate(ctx, &acc); } return Ok(Some(double_literal(f64::from_bits( crate::nanbox::TAG_UNDEFINED, @@ -323,21 +324,23 @@ pub fn try_lower_console_call( { let v = lower_expr(ctx, &args[0])?; if property == "timeLog" && args.len() > 1 { + // `v` (the label) is itself an evaluated temporary held + // across the extra arguments' evaluation, so it needs a + // root of its own alongside the accumulator (#6951). + let label = temp_root_push_double(ctx, &v); let cap = ((args.len() - 1) as u32).to_string(); - let mut current_arr = ctx.block().call(I64, "js_array_alloc", &[(I32, &cap)]); + let acc = rooted_array_begin(ctx, &cap); for arg in args.iter().skip(1) { let extra = lower_expr(ctx, arg)?; - let blk = ctx.block(); - current_arr = blk.call( - I64, - "js_array_push_f64", - &[(I64, ¤t_arr), (DOUBLE, &extra)], - ); + temp_rooted_array_push(ctx, &acc, &extra); } + let current_arr = rooted_array_read(ctx, &acc); + let v = temp_root_get_double(ctx, &label); ctx.block().call_void( "js_console_time_log_spread", &[(DOUBLE, &v), (I64, ¤t_arr)], ); + temp_root_truncate(ctx, &label); return Ok(Some(double_literal(f64::from_bits( crate::nanbox::TAG_UNDEFINED, )))); @@ -403,21 +406,20 @@ pub fn try_lower_console_call( } else { // Multi-arg messages: bundle args[1..] into a heap // array and call the spread variant. + let cond_root = temp_root_push_double(ctx, &cond_v); let cap = ((args.len() - 1) as u32).to_string(); - let mut current_arr = ctx.block().call(I64, "js_array_alloc", &[(I32, &cap)]); + let acc = rooted_array_begin(ctx, &cap); for arg in args.iter().skip(1) { let v = lower_expr(ctx, arg)?; - let blk = ctx.block(); - current_arr = blk.call( - I64, - "js_array_push_f64", - &[(I64, ¤t_arr), (DOUBLE, &v)], - ); + temp_rooted_array_push(ctx, &acc, &v); } + let current_arr = rooted_array_read(ctx, &acc); + let cond_v = temp_root_get_double(ctx, &cond_root); ctx.block().call_void( "js_console_assert_spread", &[(DOUBLE, &cond_v), (I64, ¤t_arr)], ); + temp_root_truncate(ctx, &cond_root); } return Ok(Some(double_literal(f64::from_bits( crate::nanbox::TAG_UNDEFINED, @@ -459,12 +461,14 @@ pub fn try_lower_console_call( } else { lower_expr(ctx, arg)? }; - let mut current_arr = ctx.block().call(I64, "js_array_alloc", &[(I32, "1")]); - current_arr = ctx.block().call( - I64, - "js_array_push_f64", - &[(I64, ¤t_arr), (DOUBLE, &v)], - ); + // The accumulator is allocated AFTER the argument, so + // `js_array_alloc` is itself a collection point with `v` live + // only in an SSA register. Root `v` across it (#6951). + let v_root = temp_root_push_double(ctx, &v); + let acc = rooted_array_begin(ctx, "1"); + let v = temp_root_get_double(ctx, &v_root); + temp_rooted_array_push(ctx, &acc, &v); + let current_arr = rooted_array_read(ctx, &acc); let runtime_fn = match property.as_str() { "info" => "js_console_info_spread", "debug" => "js_console_debug_spread", @@ -473,6 +477,9 @@ pub fn try_lower_console_call( _ => "js_console_log_spread", }; ctx.block().call_void(runtime_fn, &[(I64, ¤t_arr)]); + // Drops `v_root` and everything above it, the accumulator + // included — a truncate is a stack cut, not a single pop. + temp_root_truncate(ctx, &v_root); return Ok(Some(double_literal(f64::from_bits( crate::nanbox::TAG_UNDEFINED, )))); @@ -483,21 +490,25 @@ pub fn try_lower_console_call( // objects/arrays). This is more accurate than // js_jsvalue_to_string which only does the JS toString // protocol (returns "[object Object]" for plain objects). + // + // #6951: the accumulator is the only reference to every argument + // already pushed, and it lives in an SSA register across the + // evaluation of every argument still to come. `console.log("label", + // allocatingCall())` therefore lost its label — the array was swept + // mid-statement and the following push landed in recycled memory. + // Keep it in a temp root and re-read it, so it survives and follows + // an evacuating cycle. let cap = (args.len() as u32).to_string(); - let mut current_arr = ctx.block().call(I64, "js_array_alloc", &[(I32, &cap)]); + let acc = rooted_array_begin(ctx, &cap); for arg in args.iter() { let v = if let Some(v) = lower_util_types_predicate_arg(ctx, arg)? { v } else { lower_expr(ctx, arg)? }; - let blk = ctx.block(); - current_arr = blk.call( - I64, - "js_array_push_f64", - &[(I64, ¤t_arr), (DOUBLE, &v)], - ); + temp_rooted_array_push(ctx, &acc, &v); } + let current_arr = rooted_array_read(ctx, &acc); let runtime_fn = match property.as_str() { "info" => "js_console_info_spread", "debug" => "js_console_debug_spread", @@ -506,6 +517,7 @@ pub fn try_lower_console_call( _ => "js_console_log_spread", }; ctx.block().call_void(runtime_fn, &[(I64, ¤t_arr)]); + temp_root_truncate(ctx, &acc); return Ok(Some(double_literal(f64::from_bits( crate::nanbox::TAG_UNDEFINED, )))); diff --git a/crates/perry-codegen/src/lower_string_method.rs b/crates/perry-codegen/src/lower_string_method.rs index 1ca05d4f3f..87440406d8 100644 --- a/crates/perry-codegen/src/lower_string_method.rs +++ b/crates/perry-codegen/src/lower_string_method.rs @@ -7,6 +7,10 @@ use anyhow::{anyhow, bail, Result}; 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_i64, temp_root_push_i64, + temp_root_release, temp_root_truncate, +}; use crate::expr::{ i32_bool_to_nanbox, lower_expr, nanbox_pointer_inline, nanbox_string_inline, unbox_str_handle, FnCtx, @@ -1586,8 +1590,12 @@ pub(crate) fn lower_string_coerce_concat( l_is_string: bool, r_is_string: bool, ) -> Result { - let l_box = lower_expr(ctx, left)?; - let r_box = lower_expr(ctx, right)?; + // #6951: `l_box` is a heap string in an SSA register while `right` is + // lowered. If `right` allocates (`"tag" + f()`), a collection sweeps the + // left operand and the concat reads freed memory — a segfault, not a + // dropped character. `lower_operand_pair_rooted` emits nothing at all when + // `right` provably cannot collect, which is the common `"user_" + i` case. + let (l_box, r_box, guard) = lower_operand_pair_rooted(ctx, left, right)?; // Issue #58: fused string+value concat — when one side is a string // and the other is not, use the fused runtime call that collapses @@ -1604,7 +1612,9 @@ pub(crate) fn lower_string_coerce_concat( "js_string_concat_value", &[(I64, &l_handle), (DOUBLE, &r_box)], ); - return Ok(nanbox_string_inline(blk, &result_handle)); + let boxed = nanbox_string_inline(blk, &result_handle); + temp_root_release(ctx, guard); + return Ok(boxed); } if !l_is_string && r_is_string { @@ -1616,21 +1626,34 @@ pub(crate) fn lower_string_coerce_concat( "js_value_concat_string", &[(DOUBLE, &l_box), (I64, &r_handle)], ); - return Ok(nanbox_string_inline(blk, &result_handle)); + let boxed = nanbox_string_inline(blk, &result_handle); + temp_root_release(ctx, guard); + return Ok(boxed); } // Both non-string (shouldn't normally reach here) — fall back to // the generic path. + let l_handle = ctx + .block() + .call(I64, "js_jsvalue_to_string", &[(DOUBLE, &l_box)]); + // The coercion of the right operand allocates, and `l_handle` is a bare + // string address in an SSA register — root it across that call (#6951). + let l_root = temp_root_push_i64(ctx, &l_handle); + let r_handle = ctx + .block() + .call(I64, "js_jsvalue_to_string", &[(DOUBLE, &r_box)]); + let l_handle = temp_root_get_i64(ctx, &l_root); let blk = ctx.block(); - let l_handle = blk.call(I64, "js_jsvalue_to_string", &[(DOUBLE, &l_box)]); - let r_handle = blk.call(I64, "js_jsvalue_to_string", &[(DOUBLE, &r_box)]); let result_handle = blk.call( I64, "js_string_concat", &[(I64, &l_handle), (I64, &r_handle)], ); - Ok(nanbox_string_inline(blk, &result_handle)) + let boxed = nanbox_string_inline(blk, &result_handle); + temp_root_truncate(ctx, &l_root); + temp_root_release(ctx, guard); + Ok(boxed) } /// Lower a static `s1 + s2` string concatenation. Both operands must @@ -1656,8 +1679,9 @@ pub(crate) fn lower_string_concat( left: &Expr, right: &Expr, ) -> Result { - let l_box = lower_expr(ctx, left)?; - let r_box = lower_expr(ctx, right)?; + // #6951: same hazard as `lower_string_coerce_concat` — the left operand is + // a heap string in an SSA register across the right operand's evaluation. + let (l_box, r_box, guard) = lower_operand_pair_rooted(ctx, left, right)?; let blk = ctx.block(); // SSO-aware fast path: pass operands as NaN-boxed f64s directly to // `js_string_concat_sso`, which keeps SSO operands inline (no @@ -1665,11 +1689,13 @@ pub(crate) fn lower_string_concat( // SSO when the total fits 5 bytes, heap-pointer otherwise. Saves up // to 3 heap allocations per concat on hot paths like ABC451D's // recursive `before + after` (1.4M concats with 1-9 byte operands). - Ok(blk.call( + let result = blk.call( DOUBLE, "js_string_concat_box", &[(DOUBLE, &l_box), (DOUBLE, &r_box)], - )) + ); + temp_root_release(ctx, guard); + Ok(result) } /// Cap the per-call part count for the n-way fold. Must match the @@ -1756,11 +1782,12 @@ pub(crate) fn lower_string_concat_chain(ctx: &mut FnCtx<'_>, parts: &[&Expr]) -> debug_assert!(parts.len() <= CONCAT_CHAIN_MAX_PARTS); // Lower each part first (in source order); side effects must fire - // left-to-right per JS spec. - let mut lowered: Vec = Vec::with_capacity(parts.len()); - for p in parts { - lowered.push(lower_expr(ctx, p)?); - } + // left-to-right per JS spec. #6951: that ordering is exactly what makes + // every earlier part a heap value in an SSA register across every later + // part's evaluation — this is the template-literal / log-line shape, and + // one allocating interpolation was enough to sweep the parts already + // lowered. Parts that nothing allocating follows emit no rooting calls. + let (lowered, guard) = lower_exprs_rooted(ctx, parts)?; let n = lowered.len(); // Hoist the buffer to the function entry block. Issue #167. @@ -1780,5 +1807,7 @@ pub(crate) fn lower_string_concat_chain(ctx: &mut FnCtx<'_>, parts: &[&Expr]) -> "js_string_concat_chain", &[(I64, &base_i64), (I32, &format!("{}", n))], ); - Ok(nanbox_string_inline(blk, &result_handle)) + let boxed = nanbox_string_inline(blk, &result_handle); + temp_root_release(ctx, guard); + Ok(boxed) } diff --git a/crates/perry-codegen/src/runtime_decls/arrays.rs b/crates/perry-codegen/src/runtime_decls/arrays.rs index f7a48e485c..64131dcacd 100644 --- a/crates/perry-codegen/src/runtime_decls/arrays.rs +++ b/crates/perry-codegen/src/runtime_decls/arrays.rs @@ -101,6 +101,21 @@ pub fn declare_phase_b_arrays(module: &mut LlModule) { module.declare_function("js_shadow_slot_set", VOID, &[I32, I64]); module.declare_function("js_shadow_slot_bind", VOID, &[I32, PTR]); module.declare_function("js_gc_write_barriers_emitted", VOID, &[I32]); + // #6951: precise roots for expression temporaries the shadow stack has no + // slot for — the argument accumulator of a variadic call, an operand + // waiting for its sibling. Push before the collection point, read back + // after (an evacuating cycle rewrites the slot, so the pre-collection SSA + // register is stale), truncate when the region ends. + // js_gc_temp_root_push(value: u64) -> u32 (slot index) + // js_gc_temp_root_get(idx: u32) -> u64 + // js_gc_temp_root_set(idx: u32, value: u64) + // js_gc_temp_root_truncate(base: u32) + // js_array_push_f64_temp_rooted(idx: u32, value: f64) + module.declare_function("js_gc_temp_root_push", I32, &[I64]); + module.declare_function("js_gc_temp_root_get", I64, &[I32]); + module.declare_function("js_gc_temp_root_set", VOID, &[I32, I64]); + module.declare_function("js_gc_temp_root_truncate", VOID, &[I32]); + module.declare_function("js_array_push_f64_temp_rooted", VOID, &[I32, DOUBLE]); // Phase 2 of the moving-GC project: emitted at loop back-edges (only when // compiled with the moving-safepoint opt-in) so a deferred nursery // collection can run at a precise-root safepoint. No-op at runtime unless diff --git a/crates/perry-codegen/tests/temp_root_argument_temporaries.rs b/crates/perry-codegen/tests/temp_root_argument_temporaries.rs new file mode 100644 index 0000000000..7a80a49399 --- /dev/null +++ b/crates/perry-codegen/tests/temp_root_argument_temporaries.rs @@ -0,0 +1,227 @@ +//! #6951: evaluated-but-not-yet-consumed argument temporaries must be precise +//! GC roots, not bare LLVM SSA registers. +//! +//! The end-to-end proof lives in the GC × representation matrix +//! (`scripts/gc_repsel_matrix.sh`, `cons_scan_off` arm), which runs the corpus +//! with `PERRY_CONSERVATIVE_STACK_SCAN=off` — the only configuration where the +//! bug is observable, because every automatic collection otherwise forces a +//! conservative native-stack scan that pins the temporary by accident. These +//! tests pin the *codegen contract* that arm depends on, in-process and in +//! `cargo-test`, so a lowering path that quietly goes back to threading an +//! accumulator through an SSA register fails here rather than three weeks +//! later under a narrowed forced scan. + +use perry_codegen::{compile_module, AppMetadata, CompileOptions}; +use perry_hir::{Expr, Module, ModuleInitKind, Stmt}; + +fn entry_opts() -> CompileOptions { + CompileOptions { + target: None, + is_entry_module: true, + non_entry_module_prefixes: Vec::new(), + nextjs_path_init_modules: Vec::new(), + import_function_prefixes: std::collections::HashMap::new(), + import_function_ffi_aliases: std::collections::HashMap::new(), + import_function_origin_names: std::collections::HashMap::new(), + import_function_v8_specifiers: std::collections::HashMap::new(), + import_function_node_submodule: std::collections::HashMap::new(), + namespace_node_submodules: std::collections::HashMap::new(), + namespace_v8_specifiers: std::collections::HashMap::new(), + namespace_member_prefixes: std::collections::HashMap::new(), + namespace_member_origin_names: std::collections::HashMap::new(), + emit_ir_only: true, + verify_native_regions: false, + disable_buffer_fast_path: false, + namespace_imports: Vec::new(), + imported_classes: Vec::new(), + imported_enums: Vec::new(), + imported_async_funcs: std::collections::HashSet::new(), + type_aliases: std::collections::HashMap::new(), + imported_func_param_counts: std::collections::HashMap::new(), + imported_func_has_rest: std::collections::HashSet::new(), + imported_func_synthetic_arguments: std::collections::HashSet::new(), + imported_func_return_types: std::collections::HashMap::new(), + imported_vars: std::collections::HashSet::new(), + output_type: "executable".to_string(), + needs_stdlib: false, + needs_ui: false, + needs_geisterhand: false, + geisterhand_port: 7676, + enabled_features: Vec::new(), + native_module_init_names: Vec::new(), + js_module_specifiers: Vec::new(), + bundled_extensions: Vec::new(), + native_library_functions: Vec::new(), + i18n_table: None, + fast_math: false, + fp_contract_mode: perry_codegen::FpContractMode::Off, + app_metadata: AppMetadata::default(), + namespace_entries: Vec::new(), + dynamic_import_path_to_prefix: std::collections::HashMap::new(), + deferred_module_prefixes: std::collections::HashSet::new(), + module_init_deps: Vec::new(), + is_dynamic_import_target: false, + debug_locations: false, + module_source: None, + debug_source_line_offset: 0, + } +} + +fn module_with_init(name: &str, init: Vec) -> Module { + Module { + name: name.to_string(), + imports: Vec::new(), + exports: Vec::new(), + classes: Vec::new(), + interfaces: Vec::new(), + type_aliases: Vec::new(), + enums: Vec::new(), + globals: Vec::new(), + functions: Vec::new(), + script_global_functions: Vec::new(), + references_global_this: false, + annexb_global_undefined_names: Vec::new(), + init, + exported_native_instances: Vec::new(), + exported_func_return_native_instances: Vec::new(), + exported_objects: Vec::new(), + exported_functions: Vec::new(), + widgets: Vec::new(), + uses_fetch: false, + uses_webassembly: false, + extern_funcs: Vec::new(), + init_was_unrolled: false, + has_top_level_await: false, + init_kind: ModuleInitKind::Eager, + async_step_closures: std::collections::HashSet::new(), + closure_display_names: std::collections::HashMap::new(), + class_display_names: std::collections::HashMap::new(), + closure_source_text: std::collections::HashMap::new(), + async_generator_funcs: std::collections::HashSet::new(), + gen_param_prologue_len: std::collections::HashMap::new(), + } +} + +fn ir_for(name: &str, init: Vec) -> String { + String::from_utf8(compile_module(&module_with_init(name, init), entry_opts()).unwrap()) + .expect("LLVM IR should be UTF-8") +} + +/// `console.log(a, b, …)` — the shape in the #6951 repro. +fn console_log(args: Vec) -> Stmt { + Stmt::Expr(Expr::Call { + callee: Box::new(Expr::PropertyGet { + object: Box::new(Expr::GlobalGet(0)), + property: "log".to_string(), + byte_offset: 0, + }), + args, + type_args: Vec::new(), + byte_offset: 0, + }) +} + +/// An allocating argument: an object literal is a collection point, which is +/// all `expr_may_trigger_gc` needs to see. +fn allocating() -> Expr { + Expr::Object(Vec::new()) +} + +/// The argument accumulator of a variadic call must live in a temp root, and +/// every use of it must be re-read from that root. +/// +/// Pre-fix the sequence was `js_array_alloc` → N × `js_array_push_f64` with the +/// accumulator threaded through an SSA register across each argument's +/// evaluation. That register held the ONLY reference to everything pushed so +/// far, so an allocating later argument swept the half-built array and the +/// next push landed in recycled memory — `console.log("label", churn())` lost +/// its label with no crash and no diagnostic. +#[test] +fn console_argument_accumulator_is_temp_rooted() { + let ir = ir_for( + "console_accumulator.ts", + vec![console_log(vec![ + Expr::String("label".to_string()), + allocating(), + ])], + ); + + assert!( + ir.contains("call i32 @js_gc_temp_root_push"), + "the accumulator must be pushed onto the temp-root stack:\n{ir}" + ); + assert!( + ir.contains("call void @js_array_push_f64_temp_rooted"), + "arguments must be appended through the rooted push, which reads the \ + accumulator out of its slot and writes the reallocated pointer back:\n{ir}" + ); + assert!( + ir.contains("call i64 @js_gc_temp_root_get"), + "the accumulator must be RE-READ before the consuming call — an \ + evacuating cycle rewrites the slot, so the pushed register is stale:\n{ir}" + ); + assert!( + ir.contains("call void @js_gc_temp_root_truncate"), + "the temp root must be released after the consuming call:\n{ir}" + ); + + // The accumulator must not survive as a threaded SSA register: the raw + // two-operand push is what made it unrooted in the first place. + assert!( + !ir.contains("call i64 @js_array_push_f64(i64"), + "the console argument list must not thread the accumulator through an \ + SSA register any more (#6951):\n{ir}" + ); + + // Ordering: read, consume, then release. + let get = ir.find("call i64 @js_gc_temp_root_get").unwrap(); + let consume = ir.find("call void @js_console_log_spread").unwrap(); + let truncate = ir.find("call void @js_gc_temp_root_truncate").unwrap(); + assert!( + get < consume && consume < truncate, + "the accumulator must be read before the consumer runs and released \ + only after it returns:\n{ir}" + ); +} + +/// The gate: rooting is emitted only when something that follows can collect. +/// +/// An array literal of plain string literals has no allocation between its +/// elements' evaluation and the array's construction, so it must emit exactly +/// the IR it emitted before #6951 — no runtime calls, no cost. +#[test] +fn non_allocating_element_list_emits_no_rooting_calls() { + let ir = ir_for( + "array_literal_no_gc.ts", + vec![Stmt::Expr(Expr::Array(vec![ + Expr::String("a".to_string()), + Expr::String("b".to_string()), + Expr::Number(1.0), + ]))], + ); + + assert!( + !ir.contains("call i32 @js_gc_temp_root_push"), + "an all-literal array literal must not pay for temp rooting (the \ + `declare` line is unconditional; only an emitted CALL counts):\n{ir}" + ); +} + +/// …and it IS emitted when a later element allocates: the earlier element's +/// value is in an SSA register across that allocation, which is not a root. +#[test] +fn array_literal_roots_elements_before_an_allocating_element() { + let ir = ir_for( + "array_literal_gc.ts", + vec![Stmt::Expr(Expr::Array(vec![ + Expr::String("a".to_string()), + allocating(), + ]))], + ); + + assert!( + ir.contains("call i32 @js_gc_temp_root_push"), + "an array literal with an allocating later element must root the \ + elements already evaluated (#6951):\n{ir}" + ); +} diff --git a/crates/perry-runtime/src/gc/mod.rs b/crates/perry-runtime/src/gc/mod.rs index 383e4ce562..a4d6cb5b82 100644 --- a/crates/perry-runtime/src/gc/mod.rs +++ b/crates/perry-runtime/src/gc/mod.rs @@ -377,6 +377,16 @@ pub fn gc_init() { new_runtime_handle_root_scan_state, MutableRootScannerSource::RuntimeHandles, ); + // #6951: expression temporaries generated code is holding in SSA registers + // across a collection point. Same standing as the shadow stack — a precise + // mutable root that is marked AND rewritten — and, like the shadow stack, + // load-bearing the moment the conservative native-stack scan is off. + gc_register_budgeted_mutable_root_scanner_with_source( + scan_temp_roots_mut, + scan_temp_roots_mut_step, + new_temp_root_scan_state, + MutableRootScannerSource::RuntimeMutableScanner, + ); gc_register_mutable_root_scanner(crate::promise::scan_native_async_completion_roots_mut); gc_register_budgeted_mutable_root_scanner_with_source( promise_mutable_root_scanner, diff --git a/crates/perry-runtime/src/gc/roots.rs b/crates/perry-runtime/src/gc/roots.rs index f7acc17ff6..485cb5100b 100644 --- a/crates/perry-runtime/src/gc/roots.rs +++ b/crates/perry-runtime/src/gc/roots.rs @@ -4,6 +4,7 @@ use std::any::Any; mod runtime_handles; mod scanner_shims; mod shadow_stack; +mod temp_roots; pub(super) use runtime_handles::{ new_runtime_handle_root_scan_state, scan_runtime_handle_roots_mut, @@ -27,6 +28,17 @@ pub use shadow_stack::{ js_shadow_slot_set, shadow_stack_depth, SHADOW_STACK_GROW_RESERVE, SHADOW_STACK_HEADER_SLOTS, }; pub(crate) use shadow_stack::{shadow_stack_restore, shadow_stack_savepoint, ShadowSavepoint}; +#[cfg(test)] +pub(crate) use temp_roots::reset_temp_roots; +#[cfg(test)] +pub(super) use temp_roots::temp_root_depth; +pub use temp_roots::{ + js_array_push_f64_temp_rooted, js_gc_temp_root_get, js_gc_temp_root_push, js_gc_temp_root_set, + js_gc_temp_root_truncate, +}; +pub(super) use temp_roots::{ + new_temp_root_scan_state, scan_temp_roots_mut, scan_temp_roots_mut_step, +}; pub type MutableRootScanner = for<'a> fn(&mut RuntimeRootVisitor<'a>); pub(crate) type BudgetedMutableRootScanner = diff --git a/crates/perry-runtime/src/gc/roots/shadow_stack.rs b/crates/perry-runtime/src/gc/roots/shadow_stack.rs index 10c1c24a74..aa107d22e8 100644 --- a/crates/perry-runtime/src/gc/roots/shadow_stack.rs +++ b/crates/perry-runtime/src/gc/roots/shadow_stack.rs @@ -246,10 +246,17 @@ pub(crate) fn shadow_stack_has_active_frame() -> bool { /// orphaned frames, reading — and, on the copying/evacuating path, /// *writing back into* — `slot_ptrs` that point into stack memory that /// has already been unwound and is being reused by the catch body. +/// +/// The same reasoning applies to the temp-root stack (#6951): generated code +/// pushes an expression temporary, evaluates something that throws, and never +/// reaches its `js_gc_temp_root_truncate`. That depth is therefore recorded +/// here and restored with the frames, so one savepoint covers both precise +/// root stacks and `crate::exception` needs no separate hook. #[derive(Copy, Clone)] pub(crate) struct ShadowSavepoint { frame_top: usize, len: usize, + temp_roots: usize, } impl ShadowSavepoint { @@ -258,6 +265,7 @@ impl ShadowSavepoint { pub(crate) const EMPTY: ShadowSavepoint = ShadowSavepoint { frame_top: usize::MAX, len: 0, + temp_roots: 0, }; } @@ -270,6 +278,7 @@ pub(crate) fn shadow_stack_savepoint() -> ShadowSavepoint { ShadowSavepoint { frame_top: s.frame_top, len: s.stack.len(), + temp_roots: super::temp_roots::temp_root_depth(), } }) } @@ -294,4 +303,5 @@ pub(crate) fn shadow_stack_restore(sp: ShadowSavepoint) { } s.frame_top = sp.frame_top; }); + super::temp_roots::temp_roots_restore(sp.temp_roots); } diff --git a/crates/perry-runtime/src/gc/roots/temp_roots.rs b/crates/perry-runtime/src/gc/roots/temp_roots.rs new file mode 100644 index 0000000000..577b76f82f --- /dev/null +++ b/crates/perry-runtime/src/gc/roots/temp_roots.rs @@ -0,0 +1,197 @@ +//! Precise GC roots for generated-code expression temporaries (#6951). +//! +//! # What this is for +//! +//! The shadow stack ([`super::shadow_stack`]) roots *named locals*: codegen +//! reserves one slot per pointer-typed local and binds it to that local's +//! alloca. It has no slots for the values that only ever exist between two +//! instructions — the accumulator array a variadic call builds while it is +//! still evaluating its arguments, an already-evaluated operand waiting for its +//! sibling, a freshly allocated receiver waiting for its consumer. Those live +//! in LLVM SSA registers, and an SSA register is not a root. +//! +//! Until #6951 that was covered — accidentally — by the conservative native +//! stack scan, which finds whatever LLVM happened to spill. `gc_check_trigger` +//! forces that scan on both automatic arms, so the gap was invisible in +//! practice while `gc::roots::conservative_stack_scan_mode` nominally resolves +//! `Auto -> SkipDisabled` in shipped builds. Run a compiled program with +//! `PERRY_CONSERVATIVE_STACK_SCAN=off` and the gap is a live use-after-free: +//! `console.log("label", allocatingCall())` loses its label, and the harder +//! shapes segfault. +//! +//! # The contract +//! +//! A temp root is a **stack** of words, per thread. Generated code pushes the +//! word it needs kept alive, evaluates whatever may collect, then reads the +//! word back out of the root and truncates. Reading back is not optional +//! bookkeeping: this is a *mutable* root, so an evacuating cycle rewrites the +//! slot, and the pre-collection SSA register is stale afterwards. Slots are +//! visited with [`RuntimeRootVisitor::visit_heap_word_u64_slot`], the same +//! decoder the shadow stack uses, so a slot may hold either form the +//! `gc::root_words` contract admits: +//! +//! - a NaN-boxed value (`POINTER_TAG` / `STRING_TAG` / `BIGINT_TAG`), or +//! - a bare heap address, which is what the raw `i64` array pointers threaded +//! through `js_array_alloc` / `js_array_push_f64` are. +//! +//! Immediates (numbers, `undefined`, small ints) decode to nothing and cost a +//! push slot and no more, so callers do not have to prove pointer-ness. +//! +//! # Balance +//! +//! `js_gc_temp_root_truncate(base)` drops `base` and everything above it, so a +//! missed truncate is bounded by the next one rather than leaking forever. +//! Non-local exits are covered too: [`super::shadow_stack::ShadowSavepoint`] +//! carries the temp-root depth, so the `longjmp` unwind that already restores +//! the shadow stack (`crate::exception`) restores this stack with it. + +use super::*; + +/// Initial capacity, in words. One `console.log` argument list needs one slot; +/// this is sized so ordinary nesting never reallocates. +const TEMP_ROOT_RESERVE: usize = 64; + +thread_local! { + /// Safety mirrors [`super::shadow_stack::SHADOW`]: these ops run only from + /// compiled code on this thread and from GC scanner/rewriter passes, and + /// the two never overlap (GC is stop-the-world relative to this TLS, and + /// the scanner allocates nothing). + pub(crate) static TEMP_ROOTS: std::cell::UnsafeCell> = + std::cell::UnsafeCell::new(Vec::with_capacity(TEMP_ROOT_RESERVE)); +} + +/// Push `value` and return the index generated code must pass to +/// `js_gc_temp_root_get` / `js_gc_temp_root_set` / `js_gc_temp_root_truncate`. +#[no_mangle] +pub extern "C" fn js_gc_temp_root_push(value: u64) -> u32 { + TEMP_ROOTS.with(|cell| unsafe { + let s = &mut *cell.get(); + let idx = s.len(); + // A depth this large means codegen dropped a truncate; refusing to grow + // keeps a runaway from turning into unbounded retention. The returned + // index still addresses a live slot, so get/set stay well-defined. + if idx >= u32::MAX as usize { + debug_assert!(false, "temp-root stack overflow (unbalanced truncate)"); + return (idx - 1) as u32; + } + s.push(value); + if value != 0 { + crate::gc::runtime_write_barrier_root_heap_word(value); + } + idx as u32 + }) +} + +/// Read slot `idx` back. Generated code must use this value, not the register +/// it pushed: an evacuating cycle rewrites the slot in place. +#[no_mangle] +pub extern "C" fn js_gc_temp_root_get(idx: u32) -> u64 { + TEMP_ROOTS.with(|cell| unsafe { + let s = &*cell.get(); + s.get(idx as usize).copied().unwrap_or(0) + }) +} + +/// Overwrite slot `idx`, for producers that hand back a possibly-reallocated +/// pointer (`js_array_push_f64`). +#[no_mangle] +pub extern "C" fn js_gc_temp_root_set(idx: u32, value: u64) { + TEMP_ROOTS.with(|cell| unsafe { + let s = &mut *cell.get(); + if let Some(slot) = s.get_mut(idx as usize) { + *slot = value; + if value != 0 { + crate::gc::runtime_write_barrier_root_heap_word(value); + } + } + }); +} + +/// Drop slot `base` and every slot above it. +#[no_mangle] +pub extern "C" fn js_gc_temp_root_truncate(base: u32) { + TEMP_ROOTS.with(|cell| unsafe { + let s = &mut *cell.get(); + let base = base as usize; + if base < s.len() { + s.truncate(base); + } + }); +} + +/// Push a rooted value onto the array in temp-root slot `idx`, writing the +/// (possibly reallocated) array pointer back into the slot. +/// +/// This is the fused form of get + `js_array_push_f64` + set: the argument +/// accumulator of a variadic call is pushed to once per argument, and the +/// three-call form would triple that traffic for no added safety. `value` is +/// rooted by `js_array_push_f64` itself (its grow path takes a +/// `RuntimeHandleScope`), so only the accumulator needs the slot. +#[no_mangle] +pub extern "C" fn js_array_push_f64_temp_rooted(idx: u32, value: f64) { + let arr = js_gc_temp_root_get(idx) as *mut crate::array::ArrayHeader; + let arr = crate::array::js_array_push_f64(arr, value); + js_gc_temp_root_set(idx, arr as u64); +} + +/// Current depth — the value a savepoint records. +pub(crate) fn temp_root_depth() -> usize { + TEMP_ROOTS.with(|cell| unsafe { (*cell.get()).len() }) +} + +/// Restore a previously-recorded depth. Used by the exception unwind path via +/// [`super::shadow_stack::ShadowSavepoint`]; a `longjmp` can only have added +/// slots relative to the savepoint, so the `<=` guard is defensive. +pub(crate) fn temp_roots_restore(depth: usize) { + TEMP_ROOTS.with(|cell| unsafe { + let s = &mut *cell.get(); + if depth <= s.len() { + s.truncate(depth); + } + }); +} + +/// Test-only: drop every temp root, so an isolated GC test starts from a known +/// empty stack. +#[cfg(test)] +pub(crate) fn reset_temp_roots() { + TEMP_ROOTS.with(|cell| unsafe { + (*cell.get()).clear(); + }); +} + +pub(crate) fn scan_temp_roots_mut(visitor: &mut RuntimeRootVisitor<'_>) { + TEMP_ROOTS.with(|cell| unsafe { + for slot in (*cell.get()).iter_mut() { + visitor.visit_heap_word_u64_slot(slot); + } + }); +} + +#[derive(Default)] +pub(crate) struct TempRootScanState { + cursor: usize, +} + +pub(crate) fn new_temp_root_scan_state() -> Box { + Box::::default() +} + +pub(crate) fn scan_temp_roots_mut_step( + visitor: &mut RuntimeRootVisitor<'_>, + state: &mut dyn Any, + remaining: &mut usize, +) -> bool { + let state = state + .downcast_mut::() + .expect("temp root scanner state type"); + TEMP_ROOTS.with(|cell| unsafe { + let s = &mut *cell.get(); + while *remaining > 0 && state.cursor < s.len() { + visitor.visit_heap_word_u64_slot(&mut s[state.cursor]); + state.cursor += 1; + *remaining -= 1; + } + state.cursor >= s.len() + }) +} diff --git a/crates/perry-runtime/src/gc/tests/mod.rs b/crates/perry-runtime/src/gc/tests/mod.rs index 3e544774c4..c425ed7b9d 100644 --- a/crates/perry-runtime/src/gc/tests/mod.rs +++ b/crates/perry-runtime/src/gc/tests/mod.rs @@ -23,4 +23,5 @@ mod smoke; pub(super) mod support; mod teardown; mod telemetry_verifier; +mod temp_roots; mod triggers; diff --git a/crates/perry-runtime/src/gc/tests/temp_roots.rs b/crates/perry-runtime/src/gc/tests/temp_roots.rs new file mode 100644 index 0000000000..d58f460c8f --- /dev/null +++ b/crates/perry-runtime/src/gc/tests/temp_roots.rs @@ -0,0 +1,193 @@ +//! #6951 — expression temporaries generated code pushes onto the temp-root +//! stack must be precise roots: marked by the mark phase and relocated by the +//! rewrite phase, with no help from the conservative native-stack scan. +//! +//! Every test here pins `ConservativeStackScanMode::Disabled`. The unit-test +//! build defaults to `Full`, whose native-stack scan would find the raw +//! pointer these tests hold in a Rust local and rescue the object — turning a +//! missing precise root into a green test and a production-only +//! use-after-free (production resolves `Auto -> SkipDisabled`). A test here +//! that does not pin the scan off proves nothing. + +use super::super::*; +use super::support::*; + +fn reset_old_reclaim_pressure() { + let old_in_use = crate::arena::old_gen_in_use_bytes(); + GC_LAST_OLD_RECLAIM_IN_USE_BYTES.with(|bytes| bytes.set(old_in_use)); + GC_OLD_RECLAIM_PENDING.with(|pending| pending.set(false)); +} + +/// Put the temp-root scanner back into a test-isolated thread's registry. +/// +/// `GcTestIsolationGuard` / `CopyingNurseryTestGuard` `mem::take` the thread's +/// `MUTABLE_ROOT_SCANNERS` so a collection sees exactly the roots the test +/// installs — and the temp-root scanner goes with it. Without this the temp +/// root under test is decorative and the test passes for the wrong reason. +fn register_temp_root_scanner_for_tests() { + gc_register_budgeted_mutable_root_scanner_with_source( + scan_temp_roots_mut, + scan_temp_roots_mut_step, + new_temp_root_scan_state, + MutableRootScannerSource::RuntimeMutableScanner, + ); +} + +/// The end-to-end regression: an object whose ONLY reference is a temp-root +/// slot must survive a real collection with precise roots only. +/// +/// This is the unit-level form of the #6951 repro. In a compiled program the +/// slot holds the argument accumulator of `console.log("label", churn())`, +/// which used to live in nothing but an LLVM SSA register — swept +/// mid-statement, with the next `js_array_push_f64` landing in recycled +/// memory, so the label silently vanished from the output. +#[test] +fn temp_rooted_value_survives_a_real_collection() { + 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 live = gc_malloc( + std::mem::size_of::(), + GC_TYPE_CLOSURE, + ); + unsafe { + init_test_closure(live); + } + assert!( + malloc_user_ptr_tracked(live), + "precondition: live is tracked" + ); + + // Both word forms the `gc::root_words` contract admits, because generated + // code pushes both: NaN-boxed values from `lower_expr`, and bare `i64` + // array pointers threaded through `js_array_alloc` / `js_array_push_f64`. + let bare = js_gc_temp_root_push(live as u64); + let tagged = js_gc_temp_root_push(POINTER_TAG | live 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" + ); + + // Read the survivor back out of the ROOT, never from the pre-collection + // register — that is the contract generated code follows, and it is the + // stronger assertion: it also proves the slot itself was maintained. + let survivor = js_gc_temp_root_get(bare); + assert_ne!( + survivor, 0, + "the bare temp-root slot must still hold a value" + ); + assert!( + malloc_user_ptr_tracked(survivor as *mut u8), + "an object reachable only through a temp root must be marked, not swept (#6951)" + ); + assert_eq!( + js_gc_temp_root_get(tagged) & POINTER_MASK, + survivor, + "the NaN-boxed slot must resolve to the same object as the bare slot" + ); + unsafe { + assert_eq!( + (*(survivor as *mut crate::closure::ClosureHeader)).type_tag, + crate::closure::CLOSURE_MAGIC, + "surviving object must still be intact" + ); + } + + js_gc_temp_root_truncate(bare); + assert_eq!(temp_root_depth(), 0); +} + +/// Truncation is a stack cut, not a pop: releasing a base slot must drop every +/// slot pushed above it, which is what lets one `js_gc_temp_root_truncate` at +/// the end of an argument list release the whole group. +#[test] +fn truncate_drops_every_slot_above_the_base() { + let _guard = GcTestIsolationGuard::new(); + reset_temp_roots(); + + let base = js_gc_temp_root_push(0x1000); + js_gc_temp_root_push(0x2000); + js_gc_temp_root_push(0x3000); + assert_eq!(temp_root_depth(), 3); + + js_gc_temp_root_truncate(base); + assert_eq!(temp_root_depth(), 0); + + // Out-of-range accesses are release-safe no-ops, the same discipline + // `js_shadow_slot_set` follows: a malformed index from generated code must + // not abort the host program. + assert_eq!(js_gc_temp_root_get(99), 0); + js_gc_temp_root_set(99, 0x4000); + js_gc_temp_root_truncate(99); + assert_eq!(temp_root_depth(), 0); +} + +/// `js_array_push_f64_temp_rooted` is the fused accumulator push: it must read +/// the array out of the slot, push, and write the possibly-reallocated pointer +/// back — otherwise a growth reallocation strands the root on the old header. +#[test] +fn fused_array_push_writes_the_reallocated_pointer_back() { + let _guard = GcTestIsolationGuard::new(); + reset_temp_roots(); + + let arr = crate::array::js_array_alloc(0); + let idx = js_gc_temp_root_push(arr as u64); + + // Push past the initial capacity so the array is forced to grow and hand + // back a different header. + let capacity = unsafe { (*arr).capacity }; + for i in 0..(capacity + 4) { + js_array_push_f64_temp_rooted(idx, i as f64); + } + + let grown = js_gc_temp_root_get(idx) as *const crate::array::ArrayHeader; + assert!(!grown.is_null(), "the slot must hold the grown array"); + unsafe { + assert_eq!( + (*grown).length, + capacity + 4, + "every fused push must have landed in the array the slot points at" + ); + } + + js_gc_temp_root_truncate(idx); +} + +/// A `longjmp` unwind past an argument list skips its +/// `js_gc_temp_root_truncate`. The shadow-stack savepoint the exception path +/// already takes carries the temp-root depth, so the orphaned slots are +/// dropped with the orphaned frames rather than retained forever. +#[test] +fn shadow_savepoint_restores_the_temp_root_depth() { + let _guard = GcTestIsolationGuard::new(); + reset_temp_roots(); + reset_shadow_stack(); + + let outer = js_gc_temp_root_push(0x1000); + let savepoint = shadow_stack_savepoint(); + js_gc_temp_root_push(0x2000); + js_gc_temp_root_push(0x3000); + assert_eq!(temp_root_depth(), 3); + + shadow_stack_restore(savepoint); + assert_eq!( + temp_root_depth(), + 1, + "the unwind must drop the slots pushed inside the protected region" + ); + assert_eq!(js_gc_temp_root_get(outer), 0x1000); + + js_gc_temp_root_truncate(outer); +} diff --git a/test-parity/gc_repsel_triage.txt b/test-parity/gc_repsel_triage.txt index 4399fddcd1..81ea8c0525 100644 --- a/test-parity/gc_repsel_triage.txt +++ b/test-parity/gc_repsel_triage.txt @@ -7,5 +7,11 @@ # representation defect. Do not add an entry to make a table green: an # untriaged red cell is the whole point of this gate. -test_gap_repsel_gc_stress | cons_scan_off | #6951 -- with the conservative stack scan disabled, console.log argument temporaries are not precise roots and string-literal labels are dropped. Reproduces byte-identically with ALL representation gates off, so it is a runtime rooting gap, not a representation defect. This arm becomes usable (and becomes the highest-value arm in the matrix) once #6951 is fixed. -test_gap_repsel_gc_stress | cons_scan_off_force | #6951 -- same root cause as the cons_scan_off arm; the evacuation flags add nothing because no automatic collection ever evacuates (#6950). +# (empty) +# +# #6951 is FIXED: the argument accumulator of a variadic call is now a precise +# root (`js_gc_temp_root_*`, `gc/roots/temp_roots.rs`), so the two +# `test_gap_repsel_gc_stress` cells that were triaged here -- `cons_scan_off` +# and `cons_scan_off_force` -- are green and are now hard gates. `cons_scan_off` +# is in the PR arm set, so it is the arm that will catch the next unrooted +# temporary. Do not re-triage it without a new issue number and a reason. From 92f2854d90d13439e70d30733704756facf67ebc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Wed, 29 Jul 2026 07:56:29 +0200 Subject: [PATCH 2/3] perf(gc): skip temp roots for values that cannot be heap references Two gates on top of "does anything after this collect": - A value `expr_is_known_non_pointer_shadow_value` proves is not a heap reference roots nothing, so a slot for it is pure TLS traffic. This is what keeps `total + s.length` and every other numeric operand pair on byte-identical IR. - A string literal loads from a module global that `__perry_init_strings_*` already registered with `js_gc_register_global_root`, so the sweep can never take it. (The loaded register is still stale after an *evacuating* cycle, but that is true of every `Expr::String` use in the compiler and is not the hazard #6951 is about.) Template literals are mostly literal parts, so this is the difference between 3 and 7 rooting calls per interpolation. Measured on a hot loop doing `"user_" + i`, an array literal, an object literal and a template literal per iteration: 32 -> 12 emitted rooting calls, and the remaining 12 are the template literal's one coerced heap operand plus the once-per-program console.log. --- crates/perry-codegen/src/expr/temp_root.rs | 16 +++++++++++++++- .../tests/temp_root_argument_temporaries.rs | 14 +++++++------- 2 files changed, 22 insertions(+), 8 deletions(-) diff --git a/crates/perry-codegen/src/expr/temp_root.rs b/crates/perry-codegen/src/expr/temp_root.rs index c9361bba6a..d571d72448 100644 --- a/crates/perry-codegen/src/expr/temp_root.rs +++ b/crates/perry-codegen/src/expr/temp_root.rs @@ -168,7 +168,21 @@ pub(crate) fn lower_exprs_rooted( let mut guard: Option = None; for (i, expr) in exprs.iter().enumerate() { let value = super::lower_expr(ctx, expr)?; - if any_later_ref_may_trigger_gc(exprs, i) { + // A value that provably cannot be a heap reference roots nothing, so a + // slot for it is pure TLS traffic. This is the gate that keeps + // `total + s.length` and other numeric operand pairs at their old IR. + // + // A string literal is skipped for the opposite reason: it is a load + // from a module global that `__perry_init_strings_*` registered with + // `js_gc_register_global_root`, so it already has a precise root and + // the sweep can never take it. (A register loaded from that global is + // still stale after an *evacuating* cycle — but that is true of every + // `Expr::String` use in the compiler, not something this site + // introduces, and it is not the hazard #6951 is about.) Template + // literals are mostly literal parts, so this matters. + let needs_root = !super::expr_is_known_non_pointer_shadow_value(ctx, expr) + && !matches!(expr, Expr::String(_)); + if needs_root && any_later_ref_may_trigger_gc(exprs, i) { 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. diff --git a/crates/perry-codegen/tests/temp_root_argument_temporaries.rs b/crates/perry-codegen/tests/temp_root_argument_temporaries.rs index 7a80a49399..14ea0f5c4b 100644 --- a/crates/perry-codegen/tests/temp_root_argument_temporaries.rs +++ b/crates/perry-codegen/tests/temp_root_argument_temporaries.rs @@ -207,21 +207,21 @@ fn non_allocating_element_list_emits_no_rooting_calls() { ); } -/// …and it IS emitted when a later element allocates: the earlier element's -/// value is in an SSA register across that allocation, which is not a root. +/// …and it IS emitted when an earlier element is a heap value and a later one +/// allocates: that value sits in an SSA register across the allocation, which +/// is not a root. #[test] fn array_literal_roots_elements_before_an_allocating_element() { let ir = ir_for( "array_literal_gc.ts", - vec![Stmt::Expr(Expr::Array(vec![ - Expr::String("a".to_string()), - allocating(), - ]))], + // Element 0 is itself a heap value (a literal would be skipped: it + // loads from a module global that is already a registered GC root). + vec![Stmt::Expr(Expr::Array(vec![allocating(), allocating()]))], ); assert!( ir.contains("call i32 @js_gc_temp_root_push"), "an array literal with an allocating later element must root the \ - elements already evaluated (#6951):\n{ir}" + heap elements already evaluated (#6951):\n{ir}" ); } From e46c5c8d7eddcdf154732c2062ee33acf8beb01c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Wed, 29 Jul 2026 08:00:30 +0200 Subject: [PATCH 3/3] docs: changelog fragment for #6972 --- .../6972-precise-root-argument-temporaries.md | 66 +++++++++++++++++++ 1 file changed, 66 insertions(+) create mode 100644 changelog.d/6972-precise-root-argument-temporaries.md diff --git a/changelog.d/6972-precise-root-argument-temporaries.md b/changelog.d/6972-precise-root-argument-temporaries.md new file mode 100644 index 0000000000..51aabb9699 --- /dev/null +++ b/changelog.d/6972-precise-root-argument-temporaries.md @@ -0,0 +1,66 @@ +**fix(gc): argument temporaries are precise roots (#6951)** + +With the conservative native-stack scan disabled — precise/shadow-stack roots +only — a collection landing during argument evaluation dropped `console.log`'s +string-literal argument, with no crash and no diagnostic. Harder shapes +(`fresh() + "/" + f()`, `new C(fresh(), f())`, `s.concat("|" + f())`) segfaulted. + +**Root cause.** The shadow stack roots *named locals*: one slot per pointer-typed +local, bound to that local's alloca. It has no slot for the values that exist +only between two instructions, and an LLVM SSA register is not a GC root. +`console.log("alpha", churn())` lowers to `js_array_alloc(2)` plus one +`js_array_push_f64` per argument, with the accumulator threaded through an SSA +register. That register held the ONLY reference to everything already pushed — +argument 0 included — across argument 1's evaluation. The sweep freed the +half-built array, `churn` recycled the block, and the next push wrote the number +into a header whose `length` had been reset to 0: a one-element array, and the +label gone. Conservative stack scanning hid it, because `gc_check_trigger` forces +a full conservative scan on both automatic arms while `gc/roots.rs`'s nominal +production default is `Auto -> SkipDisabled` — the scan was doing load-bearing +correctness work, not acting as a safety net. + +**Mechanism.** `crates/perry-runtime/src/gc/roots/temp_roots.rs` adds a +per-thread temp-root *stack* callable from generated code, registered in +`gc_init` as a budgeted mutable root scanner, so slots are marked AND rewritten +rather than pinned. Slots are visited through `visit_heap_word_u64_slot`, the +same decoder the shadow stack uses, so a slot may hold either word form the +`gc::root_words` contract admits: a NaN-boxed value or a bare heap address (the +raw `i64` array pointers threaded through `js_array_alloc`). Generated code +pushes before the collection point, **re-reads** after (mandatory — an +evacuating cycle rewrites the slot, so the pushed register is stale), and +truncates after the consuming call. Truncate is a stack cut, not a pop, so a +missed release is bounded by the next one. `ShadowSavepoint` now carries the +temp-root depth, so the `longjmp` unwind that already restores the shadow stack +restores this stack with it — no change to `crate::exception`. + +**Rooted sites.** The variadic argument accumulator (`console.log` / `info` / +`warn` / `error` / `debug` / `trace` / `assert` / `timeLog`); the string-concat +operand pair and the n-way concat chain (template literals, log lines), plus the +intermediate `js_jsvalue_to_string` handle in the both-non-string fallback; the +object-literal handle across its initializers (all three lowering paths); and +array-literal element values. + +**Cost.** Emission is gated three ways, any one of which suppresses it: nothing +after the value reaches a collection point; the value provably cannot be a heap +reference; or the value is a string literal (already a registered global root). +`"user_" + i`, `[1, 2, 3]`, `{a: i, b: total}` and all-local argument lists emit +byte-identical IR to before. On a hot loop doing a concat, an array literal, an +object literal and a template literal per iteration the gates take emitted +rooting calls from 32 to 12. + +**Verification.** `scripts/gc_repsel_matrix.sh --arms all` against pinned Node +26.5.0: 361/361 cells byte-exact, FAIL=0, XFAIL=0 — +`test_gap_repsel_gc_stress × cons_scan_off` and `× cons_scan_off_force` move from +XFAIL to PASS with the arm measurably live (17 completed cycles), so both entries +are removed from `test-parity/gc_repsel_triage.txt` and `cons_scan_off` (a PR +arm) becomes a hard gate on this shape. A 431-file gap-corpus A/B against +`origin/main` produced identical result sets. Four new unit tests in +`gc::tests::temp_roots`, every one pinning `ConservativeStackScanMode::Disabled` +(with the scan on the bug is invisible), plus three codegen IR tests pinning the +emission contract and the no-cost gate. + +**Still open**, filed with reproducers: #6968 (scalar-replaced object/array +locals), #6969 (`new C(a, b)` constructor arguments), #6970 (native-method-call +arguments), #6971 (string-method receiver + arguments). `moved_objects` remains 0 +in every arm and 333 matrix cells remain UNVERIFIED — that is #6950, which this +change unblocks rather than fixes.