diff --git a/changelog.d/6975-temp-root-coercion-gate.md b/changelog.d/6975-temp-root-coercion-gate.md new file mode 100644 index 0000000000..a94096e8ef --- /dev/null +++ b/changelog.d/6975-temp-root-coercion-gate.md @@ -0,0 +1,43 @@ +**fix(gc): coercion-capable operators can collect — four rooting gaps from the #6972 review** + +Follow-up to #6972 (#6951). One soundness hole in the gate predicate plus three +additional sites of the same bug class, all found by review after #6972 merged. + +**The gate was unsound.** `expr/temp_root.rs` documents that +`expr_may_trigger_gc` is deliberately one-sided — `false` must mean "provably +allocates nothing" — and then answered `false` for `Compare` / `Unary` / +non-`Add` `Binary` whenever recursing into the operands found no allocation. +But `o < x`, `-o` and `o * 2` run ToPrimitive / ToNumber on their operands, and +a user-defined `Symbol.toPrimitive` / `valueOf` / `toString` is arbitrary JS: it +allocates and it collects. `a < b` over two plain `LocalGet`s recursed straight +to `false`, so `f(freshString(), a < b)` skipped rooting its first argument — +the #6951 use-after-free in a narrower case. These operators are now GC-capable +unless **every** operand is a proven inert primitive (`expr_is_inert_primitive`: +literals, plus locals the type analysis proved Number / Int32 / Boolean / Null / +Void / Never with no reserved shadow slot — a reserved slot means +pointer-possible whatever the refined type says). `Add` is never inert, since +concatenation allocates even over two literals. The predicate now takes +`&FnCtx`; `any_later_arg_may_trigger_gc` had no callers and is deleted rather +than shipped dead. Cost is unchanged: `i < n` and `x * 2` on proven-numeric +locals stay inert, and the hot-loop benchmark from #6972 still emits 12 rooting +calls. + +**Three more sites.** (1) `expr/binary.rs`'s BigInt dynamic helper had a second +copy of the two-`lower_expr` shape in its `!inline_bitwise` branch that #6972's +pass missed. (2) `lower_canonical_str_self_append`: `s += rhs` must load `s` +*before* evaluating `rhs` (a `rhs` that reassigns `s` must not be observed), so +the pre-rhs value crosses both `rhs` and `js_jsvalue_to_string` — re-reading the +slot would take the wrong value, so it goes into a temp root; the coerced rhs +handle is rooted too, because the cold arm's `unbox_str_handle` materializes an +SSO destination onto the heap with that bare handle live. (3) +`lower_object_literal`'s `this_patches` queue holds method-closure values across +every remaining property's initializer and then passes them to +`js_closure_set_capture_bits` as raw pointers; they are now rooted and refreshed +before the patch loop. + +Re-verified against pinned Node 26.5.0: the #6951 repro stays fixed under +`PERRY_CONSERVATIVE_STACK_SCAN=off`; the 431-file gap corpus is byte-identical +to the `origin/main` baseline; `scripts/gc_repsel_matrix.sh --arms all` is +361/361 byte-exact with FAIL=0 and XFAIL=0 and both `cons_scan_off` cells still +PASS; and a throw through a protected argument list is byte-exact under both +arms. diff --git a/crates/perry-codegen/src/expr/binary.rs b/crates/perry-codegen/src/expr/binary.rs index 47f0f5eac3..a377ff782e 100644 --- a/crates/perry-codegen/src/expr/binary.rs +++ b/crates/perry-codegen/src/expr/binary.rs @@ -541,12 +541,16 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { && crate::type_analysis::is_provably_not_bigint(ctx, left) && crate::type_analysis::is_provably_not_bigint(ctx, right); if !inline_bitwise { + // #6951: the dynamic helper runs ToNumeric on both + // operands, so a pointer-bearing left operand must + // survive the right operand's evaluation. let fname = bigint_dynamic_helper(*op); - 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); } } } diff --git a/crates/perry-codegen/src/expr/object_literal.rs b/crates/perry-codegen/src/expr/object_literal.rs index 98509316df..29ea88afb4 100644 --- a/crates/perry-codegen/src/expr/object_literal.rs +++ b/crates/perry-codegen/src/expr/object_literal.rs @@ -7,6 +7,7 @@ use perry_hir::Expr; use super::temp_root::{ any_may_trigger_gc, rooted_handle_begin, rooted_handle_get, rooted_handle_release, + temp_root_get_double, temp_root_push_double, }; use super::{lower_expr, nanbox_pointer_inline, FnCtx}; use crate::nanbox::POINTER_MASK_I64; @@ -316,7 +317,7 @@ pub(crate) fn lower_object_literal( // 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 protect_handle = any_may_trigger_gc(ctx, 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(); @@ -469,10 +470,15 @@ pub(crate) fn lower_object_literal( .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 + // Track `(temp_root_slot, closure_value_double, reserved_this_slot_idx)` + // for each method closure that needs `this` patched after the object is // fully built. Enables `calc.add(n) { this.value = ... }`. - let mut this_patches: Vec<(String, u32)> = Vec::new(); + // + // #6951: the closure value is *deferred* — it is reused after every + // remaining property has been lowered, so it sits in an SSA register + // across all of their allocations. Root it whenever any initializer can + // collect, and re-read it before the patch loop. + let mut this_patches: Vec<(Option, String, u32)> = Vec::new(); for (key, value_expr) in props { let key_idx = ctx.strings.intern(key); @@ -490,7 +496,8 @@ pub(crate) fn lower_object_literal( let this_idx = auto_caps.len() as u32; let v = lower_expr(ctx, value_expr)?; - this_patches.push((v.clone(), this_idx)); + let closure_root = protect_handle.then(|| temp_root_push_double(ctx, &v)); + this_patches.push((closure_root, v.clone(), this_idx)); let obj_handle = rooted_handle_get(ctx, &rooted); let blk = ctx.block(); @@ -519,6 +526,16 @@ 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. + // Refresh every deferred closure value from its root BEFORE taking the + // block builder — an evacuating cycle during a later property's + // initializer rewrote the slot, and the register queued above is stale. + let this_patches: Vec<(String, u32)> = this_patches + .into_iter() + .map(|(root, value, this_idx)| match root { + Some(idx) => (temp_root_get_double(ctx, &idx), this_idx), + None => (value, this_idx), + }) + .collect(); let obj_handle = rooted_handle_get(ctx, &rooted); if !this_patches.is_empty() { let blk = ctx.block(); diff --git a/crates/perry-codegen/src/expr/temp_root.rs b/crates/perry-codegen/src/expr/temp_root.rs index d571d72448..85c32bfd88 100644 --- a/crates/perry-codegen/src/expr/temp_root.rs +++ b/crates/perry-codegen/src/expr/temp_root.rs @@ -21,6 +21,7 @@ //! That is also why this is preferable to widening conservative scanning — //! conservative roots have to pin, precise ones can move. +use perry_hir::types::Type as HirType; use perry_hir::Expr; use crate::types::{DOUBLE, I32, I64}; @@ -96,10 +97,12 @@ pub(crate) fn rooted_array_read(ctx: &mut FnCtx<'_>, idx: &str) -> String { /// 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 { +pub(crate) fn expr_may_trigger_gc(ctx: &FnCtx<'_>, expr: &Expr) -> bool { match expr { // Immediates and plain slot reads. `LocalGet` reads an alloca, - // `GlobalGet` a module global — neither allocates. + // `GlobalGet` a module global — neither allocates. (Reading an + // object-typed local is still just a load; it is the *operators* below + // that can coerce it and run user code.) Expr::Undefined | Expr::Null | Expr::Bool(_) @@ -111,45 +114,87 @@ pub(crate) fn expr_may_trigger_gc(expr: &Expr) -> bool { // `__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) + // Coercing operators. `-o`, `o < x`, `o == x`, `o * 2` all run + // ToPrimitive / ToNumber on their operands, and a user-defined + // `Symbol.toPrimitive` / `valueOf` / `toString` is arbitrary JS: it + // allocates, and it collects. Recursing into the operands is NOT + // enough — `a < b` over two plain `LocalGet`s recurses to `false` + // while the comparison itself can call into user code. So these are + // GC-capable unless every operand is a proven inert primitive. + Expr::Unary { .. } | Expr::Compare { .. } | Expr::Binary { .. } => { + !expr_is_inert_primitive(ctx, expr) } 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_may_trigger_gc(ctx, condition) + || expr_may_trigger_gc(ctx, then_expr) + || expr_may_trigger_gc(ctx, else_expr) } - Expr::Sequence(exprs) => exprs.iter().any(expr_may_trigger_gc), + Expr::Sequence(exprs) => exprs.iter().any(|e| expr_may_trigger_gc(ctx, e)), _ => true, } } -/// Does any expression after index `i` reach a collection point? +/// Is `expr` a value whose evaluation *and coercion* provably cannot run user +/// code or allocate? /// -/// 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) +/// This is the inner half of [`expr_may_trigger_gc`]'s one-sidedness: only +/// literals and locals the type analysis proved to be numbers / booleans / +/// null / undefined qualify, plus operator trees built entirely out of those. +/// A local carrying an object — or one with a reserved shadow slot, which +/// means it is pointer-possible regardless of its refined type — is not inert, +/// because `ToPrimitive` on it dispatches to whatever the object defines. +fn expr_is_inert_primitive(ctx: &FnCtx<'_>, expr: &Expr) -> bool { + match expr { + Expr::Undefined | Expr::Null | Expr::Bool(_) | Expr::Number(_) | Expr::Integer(_) => true, + // A heap value, but ToPrimitive on a string is the identity: no user + // code, no allocation. (`+` is excluded below, since concatenation + // does allocate.) + Expr::String(_) => true, + Expr::LocalGet(id) => { + !ctx.shadow_slot_map.contains_key(id) + && matches!( + ctx.local_types.get(id), + Some( + HirType::Number + | HirType::Int32 + | HirType::Boolean + | HirType::Null + | HirType::Void + | HirType::Never + ) + ) + } + Expr::Unary { operand, .. } => expr_is_inert_primitive(ctx, operand), + Expr::Compare { left, right, .. } => { + expr_is_inert_primitive(ctx, left) && expr_is_inert_primitive(ctx, right) + } + // `+` allocates whenever it is a concatenation, so it is never inert + // even over two string literals. + Expr::Binary { op, left, right } => { + !matches!(op, perry_hir::BinaryOp::Add) + && expr_is_inert_primitive(ctx, left) + && expr_is_inert_primitive(ctx, right) + } + _ => false, + } } -fn any_later_ref_may_trigger_gc(exprs: &[&Expr], i: usize) -> bool { - exprs.iter().skip(i + 1).any(|e| expr_may_trigger_gc(e)) +/// Does any expression after index `i` reach a collection point? +/// +/// This is the gate for protecting value `i`: a value that nothing allocating +/// follows cannot be collected before it is consumed, so the rooting calls +/// would be pure overhead. `i < n`, `x * 2` on proven-numeric locals, +/// `f(x, y)` on plain locals and `[1, 2, 3]` therefore emit exactly the IR +/// they emitted before #6951. +fn any_later_ref_may_trigger_gc(ctx: &FnCtx<'_>, exprs: &[&Expr], i: usize) -> bool { + exprs + .iter() + .skip(i + 1) + .any(|e| expr_may_trigger_gc(ctx, e)) } /// Lower `exprs` left to right, keeping each already-evaluated value precisely @@ -182,7 +227,7 @@ pub(crate) fn lower_exprs_rooted( // 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) { + if needs_root && any_later_ref_may_trigger_gc(ctx, 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. @@ -269,6 +314,9 @@ pub(crate) fn rooted_handle_release(ctx: &mut FnCtx<'_>, handle: RootedHandle) { } /// 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) +pub(crate) fn any_may_trigger_gc<'a>( + ctx: &FnCtx<'_>, + exprs: impl IntoIterator, +) -> bool { + exprs.into_iter().any(|e| expr_may_trigger_gc(ctx, e)) } diff --git a/crates/perry-codegen/src/lower_string_method.rs b/crates/perry-codegen/src/lower_string_method.rs index 87440406d8..b9f17f405d 100644 --- a/crates/perry-codegen/src/lower_string_method.rs +++ b/crates/perry-codegen/src/lower_string_method.rs @@ -8,8 +8,8 @@ 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, + lower_exprs_rooted, lower_operand_pair_rooted, temp_root_get_double, temp_root_get_i64, + temp_root_push_double, temp_root_push_i64, temp_root_release, temp_root_truncate, }; use crate::expr::{ i32_bool_to_nanbox, lower_expr, nanbox_pointer_inline, nanbox_string_inline, unbox_str_handle, @@ -1421,10 +1421,21 @@ fn lower_canonical_str_self_append( // (lhs slot load, then rhs), coerce the rhs once (heap handle // guaranteed), then 2-arm on the destination tag only. let lhs_box = ctx.block().load(DOUBLE, slot); + // #6951: the load must happen before `rhs` per `s += rhs` evaluation + // order (a `rhs` that reassigns `s` must not be observed here), so the + // pre-rhs value has to be carried across `rhs`'s evaluation and the + // `js_jsvalue_to_string` coercion — both of which allocate. Re-reading + // the slot would take the wrong value; re-read the temp root instead. + let lhs_root = temp_root_push_double(ctx, &lhs_box); let rhs_val = lower_expr(ctx, rhs)?; let r_handle = ctx .block() .call(I64, "js_jsvalue_to_string", &[(DOUBLE, &rhs_val)]); + // The coerced rhs is a bare string handle that has to survive the cold + // arm's `unbox_str_handle`, which materializes an SSO destination onto + // the heap — another allocation. Root it too and re-read it per arm. + let r_root = temp_root_push_i64(ctx, &r_handle); + let lhs_box = temp_root_get_double(ctx, &lhs_root); let bits_d = ctx.block().bitcast_double_to_i64(&lhs_box); let tag_d = ctx.block().lshr(I64, &bits_d, "48"); let is_heap = ctx.block().icmp_eq(I64, &tag_d, TAG_HEAP_STR); @@ -1439,17 +1450,19 @@ fn lower_canonical_str_self_append( ctx.current_block = heap_idx; let h_d = ctx.block().and(I64, &bits_d, POINTER_MASK_I64); + let r_heap = temp_root_get_i64(ctx, &r_root); let h_heap = ctx .block() - .call(I64, "js_string_append", &[(I64, &h_d), (I64, &r_handle)]); + .call(I64, "js_string_append", &[(I64, &h_d), (I64, &r_heap)]); let heap_pred = ctx.block().label.clone(); ctx.block().br(&merge_label); ctx.current_block = cold_idx; let h_d2 = unbox_str_handle(ctx.block(), &lhs_box); + let r_cold = temp_root_get_i64(ctx, &r_root); let h_cold = ctx .block() - .call(I64, "js_string_append", &[(I64, &h_d2), (I64, &r_handle)]); + .call(I64, "js_string_append", &[(I64, &h_d2), (I64, &r_cold)]); let cold_pred = ctx.block().label.clone(); ctx.block().br(&merge_label); @@ -1459,6 +1472,9 @@ fn lower_canonical_str_self_append( .phi(I64, &[(&h_heap, &heap_pred), (&h_cold, &cold_pred)]); let new_box = nanbox_string_inline(ctx.block(), &handle); ctx.block().store(DOUBLE, &new_box, slot); + // `lhs_root` is the base of the pair, so one truncate drops both. The + // index register is defined in the entry block and dominates the merge. + temp_root_truncate(ctx, &lhs_root); return Ok(new_box); } diff --git a/test-parity/gc_repsel_corpus.txt b/test-parity/gc_repsel_corpus.txt index cce8252d95..6d144a6e5c 100644 --- a/test-parity/gc_repsel_corpus.txt +++ b/test-parity/gc_repsel_corpus.txt @@ -41,6 +41,13 @@ test_gap_repsel_p4a3_ptr_numarray # --- Phase 4b: class-field store note/addref elision (#6919) ---------------- test_gap_repsel_p4b_field_store_elision +# --- Phase 5a: Ptr proven `this` in methods (#6925) ------------------ +# Registered here after the fact: #6925 added the file without registering it, +# which is exactly the omission this manifest exists to catch — the matrix +# script exits 3 on an unregistered `test_gap_repsel_*` file, so `gc-stress` +# was failing on main for every PR until this line landed. +test_gap_repsel_proven_this_frozen + # --- The GC-live member ------------------------------------------------------ # Every file above performs ZERO collections (measured, #6950), which makes the # GC arms inert against them. This one holds each representation's local live diff --git a/test-parity/gc_repsel_triage.txt b/test-parity/gc_repsel_triage.txt index 81ea8c0525..a7c1437ab8 100644 --- a/test-parity/gc_repsel_triage.txt +++ b/test-parity/gc_repsel_triage.txt @@ -7,11 +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. -# (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. + +test_gap_repsel_ptr_shape_locals | rep_ptr_shape_off | #6976 -- REGRESSION IN THE REPRESENTATION'S OWN OFF-SWITCH, not a defect in this PR. Bisected: passes at 8327ced52, fails at 1a533a3a8 (#6925, repsel Phase 5a proven `this`). With PERRY_PTR_SHAPE_LOCALS=0 the program dies partway with `TypeError: Cannot read properties of undefined (reading 'area')`, losing its last five output lines. It was invisible until now because #6925 also left test_gap_repsel_proven_this_frozen.ts unregistered, which makes this script exit 3 before it runs anything -- the gate was dark, not green. REMOVE THIS ENTRY when #6976 is fixed; an OFF arm is supposed to be the safest cell in the matrix.