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. 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..d571d72448 --- /dev/null +++ b/crates/perry-codegen/src/expr/temp_root.rs @@ -0,0 +1,274 @@ +//! 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)?; + // 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. + 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..14ea0f5c4b --- /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 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", + // 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 \ + heap 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.