diff --git a/changelog.d/6983-operand-temporaries-precise-roots.md b/changelog.d/6983-operand-temporaries-precise-roots.md new file mode 100644 index 0000000000..d1d3a17bdc --- /dev/null +++ b/changelog.d/6983-operand-temporaries-precise-roots.md @@ -0,0 +1,83 @@ +### Fixed + +- **GC: operand temporaries in three more lowering paths are precise roots (#6969, #6970, #6971).** + #6951 (via #6972) rooted variadic argument accumulators, concat operand pairs + and literal element lists; #6975 closed the coercion hole in the gate. Three sibling paths + still kept an evaluated operand in a bare LLVM SSA register across a + collection point, which under precise-roots-only + (`PERRY_CONSERVATIVE_STACK_SCAN=off`) is a live use-after-free: + + - **#6970 — collection-method operands.** `m.set(fresh(0), churn(N))` + **aborted** (exit 134, `grown Map must retain its side-allocation owner + record`): the key was finished and live only in a register across the + value's lowering, so `js_map_set` ran against a header the sweep had freed + and `churn` had reused. Fixed in the `Expr::MapSet` / `MapGet` / `MapHas` + lowering and in the `PropertyGet` dispatch that handles non-`Ident` + receivers (`this.field.set(…)`), including `Map`/`Set`/`URLSearchParams` + `forEach`, whose callback closure is itself an allocation the receiver has + to survive. + - **#6969 — constructor arguments.** `new Pair(fresh(0), churn(N))` held + argument 0 across argument 1's lowering *and* across the instance + allocation, which always collects. + - **#6971 — string-method receiver and the `concat` accumulator.** + `fresh(0).concat("|" + churn(N))` dropped its receiver. `concat` is the + dangerous form: its accumulator is a bare `StringHeader*`, the word form + only `gc::root_words`' bare case covers, and every `js_string_concat` + returns a *new* address — so the slot is written back with + `js_gc_temp_root_set`, not merely re-read. + + Two mechanism additions, both built from #6972's primitives: + `RootedOperands` (root already-lowered operands when the *caller* knows what + follows collects — a per-branch operand representation for `MapSet`, an + allocation for `new`), and `temp_root_scope_begin`/`_end`, an + expression-scope barrier. The barrier is what makes `lower_new` tractable: + truncation is a stack *cut*, so one marker slot releases the whole group on + whichever of that function's ~20 return paths ran, instead of a + `temp_root_release` at each that future edits must keep balanced. + + A new suppression, `operand_needs_root`, keeps this free where it was already + safe: literals, module globals, provable non-pointers, and locals that + **have a reserved shadow slot**. The shadow-slot check is load-bearing — a + blanket `LocalGet` suppression regressed #6970 straight back to an abort, + because a local can be pointer-valued with no shadow slot and therefore no + precise root at all (that is #6968). + + A temp root buys **three** things, and an operand needs all of them: liveness + (not swept), a location the collector rewrites (survives relocation), and the + value the call actually observed (not a later one). A registered root — a + local, a module global, a string literal — supplies the first for free, which + tempts you to skip the slot. Skipping it is only safe when the source is also + *immutable*: re-loading a local or global recovers the right address after + evacuation but reads its value **now**, after later arguments, field + initializers and possibly an inlined constructor body have run, any of which + may have reassigned it. `new C(g, bump())` where `bump()` sets `g` then + captured the post-`bump()` value — a miscompile, not a rooting bug, caught in + review and covered by `test_gap_ctor_arg_capture_order.ts`. So only string + literals are re-loaded; locals and globals get a real slot, which preserves + the call-time value and is rewritten on evacuation. + + Cost: on a probe of already-safe shapes (`"user_" + i`, `[1,2,3]`, + `{a:i,b:total}`, all-local argument lists, `m.set(k, 1)`, `m.get(k)`, + `label.slice(1,3)`, `new Pair(label, i)`) the emitted LLVM IR is + **byte-identical** to `main`, md5 included. The three protected shapes add 11 + runtime calls and 14 IR lines in total. Where a real allocation does intervene + over a registered-root operand, the cost is one extra `load` per operand and + no runtime call. + + Verification: each issue's reproducer is byte-exact over 4 runs under + `PERRY_CONSERVATIVE_STACK_SCAN=off PERRY_GC_HEAP_LIMIT=8` with the arm + measurably live (`PERRY_GC_TRACE=1`: 22 completed cycles), against exit-134 / + silent-DIFF before. Rooting a constructor's argument list *after* the lowering + loop rather than interleaved turned #6969's silent DIFF into a SIGSEGV — it + publishes an already-dangling pointer to the scanner — so the interleaving is + pinned by a test. + +### Notes + +- `gc::tests::temp_roots::rewriting_a_slot_roots_the_new_value_and_releases_the_replaced_one` + pins `ConservativeStackScanMode::Disabled`, as every test in that module must: + with the unit-test default (`Full`) the native-stack scan finds the raw + pointers in the test's own Rust locals and the test passes without proving + anything about precise roots. +- Six codegen IR tests pin the emission contract and, just as importantly, the + *absence* of rooting on the shapes that were never broken. diff --git a/crates/perry-codegen/src/expr/math_simple.rs b/crates/perry-codegen/src/expr/math_simple.rs index 9cc593a92d..2f6efb9908 100644 --- a/crates/perry-codegen/src/expr/math_simple.rs +++ b/crates/perry-codegen/src/expr/math_simple.rs @@ -8,6 +8,7 @@ use anyhow::Result; use perry_hir::types::Type as HirType; use perry_hir::{BinaryOp, Expr}; +use crate::expr::temp_root; use crate::type_analysis::{is_definitely_string_expr, is_numeric_expr, map_static_type_args}; use crate::types::{DOUBLE, F32, I1, I32, I64}; @@ -264,6 +265,34 @@ fn guarded_map_number_key_set( ) } +/// Re-read the `MapSet` receiver + key after `value` has been lowered (#6970). +/// +/// Every `Expr::MapSet` branch lowers `value` before it touches the receiver +/// handle or the key, and that lowering is the collection point. On the +/// protected path this hands back values read out of their temp-root slots — +/// mandatory, since an evacuating cycle rewrites the slot in place — and +/// derives the receiver handle from the re-read box. On the unprotected path +/// `RootedOperands::reread` returns the original registers and +/// `m_handle_unrooted` is the eagerly computed handle, so nothing is emitted. +fn reread_map_set_receiver_and_key( + ctx: &mut FnCtx<'_>, + roots: &temp_root::RootedOperands, + operands: &[&Expr; 2], + m_handle_unrooted: &Option, +) -> Result<(String, String)> { + let values = roots.reread(ctx, operands)?; + let k_box = values[1].clone(); + let m_handle = match m_handle_unrooted { + Some(handle) => handle.clone(), + None => { + let m_box = values[0].clone(); + let blk = ctx.block(); + unbox_to_i64(blk, &m_box) + } + }; + Ok((m_handle, k_box)) +} + fn guarded_map_number_key_get(ctx: &mut FnCtx<'_>, map_handle: &str, key_box: &str) -> String { let guard_raw = ctx .block() @@ -521,15 +550,41 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { let use_string_string_map = is_static_string_string_map(ctx, map) && is_definitely_string_expr(ctx, key) && is_definitely_string_expr(ctx, value); + // #6970: each operand is finished before the next is lowered, and + // both are live in nothing but SSA registers until the runtime + // call. `m.set(fresh(k), churn(N))` aborted inside `js_map_set` on + // a key whose header had been recycled. + // + // Root each one BEFORE lowering the next, not after the whole list: + // the receiver's exposure starts at `key`'s lowering, not `value`'s, + // and rooting a list that is already lowered can publish an + // already-dangling pointer into a scanned slot — strictly worse + // than not rooting at all. + let key_collects = temp_root::expr_may_trigger_gc(ctx, key); + let value_collects = temp_root::expr_may_trigger_gc(ctx, value); + let map_key_operands: [&Expr; 2] = [map, key]; + let mut roots = temp_root::root_operands_begin(2); let m_box = lower_expr(ctx, map)?; + roots.push(ctx, map, &m_box, key_collects || value_collects); let k_box = lower_expr(ctx, key)?; - let m_handle = { + roots.push(ctx, key, &k_box, value_collects); + // Unbox eagerly only on the unprotected path, so its IR — including + // register numbering — is exactly what it was before this change. + // On the protected path the handle has to come from the *re-read* + // box, so it is derived after `value` is lowered instead. + let m_handle_unrooted = (!roots.is_rooted()).then(|| { let blk = ctx.block(); unbox_to_i64(blk, &m_box) - }; + }); let new_handle = if use_string_i32_map { let value_i32 = lower_expr_native(ctx, value, crate::native_value::ExpectedNativeRep::I32)?; + let (m_handle, k_box) = reread_map_set_receiver_and_key( + ctx, + &roots, + &map_key_operands, + &m_handle_unrooted, + )?; let (k_handle, new_handle) = { let blk = ctx.block(); let k_handle = unbox_str_handle(blk, &k_box); @@ -561,6 +616,12 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { } else if use_string_u32_map { let value_u32 = lower_expr_native(ctx, value, crate::native_value::ExpectedNativeRep::U32)?; + let (m_handle, k_box) = reread_map_set_receiver_and_key( + ctx, + &roots, + &map_key_operands, + &m_handle_unrooted, + )?; let (k_handle, new_handle) = { let blk = ctx.block(); let k_handle = unbox_str_handle(blk, &k_box); @@ -592,6 +653,12 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { } else if use_string_f32_map { let value_f32 = lower_expr_native(ctx, value, crate::native_value::ExpectedNativeRep::F32)?; + let (m_handle, k_box) = reread_map_set_receiver_and_key( + ctx, + &roots, + &map_key_operands, + &m_handle_unrooted, + )?; let (k_handle, new_handle) = { let blk = ctx.block(); let k_handle = unbox_str_handle(blk, &k_box); @@ -622,6 +689,12 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { new_handle } else if use_string_number_map { let v_box = lower_expr(ctx, value)?; + let (m_handle, k_box) = reread_map_set_receiver_and_key( + ctx, + &roots, + &map_key_operands, + &m_handle_unrooted, + )?; let (k_handle, new_handle) = { let blk = ctx.block(); let k_handle = unbox_str_handle(blk, &k_box); @@ -644,6 +717,12 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { } else if use_string_boolean_map { let value_i1 = lower_expr_native(ctx, value, crate::native_value::ExpectedNativeRep::I1)?; + let (m_handle, k_box) = reread_map_set_receiver_and_key( + ctx, + &roots, + &map_key_operands, + &m_handle_unrooted, + )?; let (k_handle, new_handle) = { let blk = ctx.block(); let k_handle = unbox_str_handle(blk, &k_box); @@ -675,6 +754,12 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { new_handle } else if use_string_string_map { let v_box = lower_expr(ctx, value)?; + let (m_handle, k_box) = reread_map_set_receiver_and_key( + ctx, + &roots, + &map_key_operands, + &m_handle_unrooted, + )?; let (k_handle, v_handle, new_handle) = { let blk = ctx.block(); let k_handle = unbox_str_handle(blk, &k_box); @@ -707,6 +792,12 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { new_handle } else if has_string_key_map { let v_box = lower_expr(ctx, value)?; + let (m_handle, k_box) = reread_map_set_receiver_and_key( + ctx, + &roots, + &map_key_operands, + &m_handle_unrooted, + )?; let (k_handle, new_handle) = { let blk = ctx.block(); let k_handle = unbox_str_handle(blk, &k_box); @@ -740,6 +831,12 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { new_handle } else if use_number_string_map { let v_box = lower_expr(ctx, value)?; + let (m_handle, k_box) = reread_map_set_receiver_and_key( + ctx, + &roots, + &map_key_operands, + &m_handle_unrooted, + )?; let (v_handle, v_slot_box) = { let blk = ctx.block(); let v_handle = unbox_str_handle(blk, &v_box); @@ -760,6 +857,12 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { guarded_map_number_key_set(ctx, &m_handle, &k_box, &v_slot_box) } else if use_number_key_map { let v_box = lower_expr(ctx, value)?; + let (m_handle, k_box) = reread_map_set_receiver_and_key( + ctx, + &roots, + &map_key_operands, + &m_handle_unrooted, + )?; if static_number_string_map { record_collection_typed_value_fallback( ctx, @@ -775,6 +878,12 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { guarded_map_number_key_set(ctx, &m_handle, &k_box, &v_box) } else { let v_box = lower_expr(ctx, value)?; + let (m_handle, k_box) = reread_map_set_receiver_and_key( + ctx, + &roots, + &map_key_operands, + &m_handle_unrooted, + )?; let new_handle = { let blk = ctx.block(); blk.call( @@ -794,6 +903,9 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { ); new_handle }; + // Released only now: the runtime call above allocates while it + // reads the key, so the group has to stay rooted across it. + roots.release(ctx); // map.set returns the (possibly-realloc'd) map. Re-NaN-box // and return. The caller may need to write this back to a // local; that's the caller's problem if Map is held in a @@ -807,13 +919,14 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { let use_number_key_map = !use_string_key_map && is_static_number_key_map(ctx, map) && is_numeric_expr(ctx, key); - let m_box = lower_expr(ctx, map)?; - let k_box = lower_expr(ctx, key)?; + // #6970: `key` is lowered after the receiver and can collect, so the + // receiver would otherwise sit unrooted in an SSA register across it. + let (m_box, k_box, guard) = temp_root::lower_operand_pair_rooted(ctx, map, key)?; let m_handle = { let blk = ctx.block(); unbox_to_i64(blk, &m_box) }; - if use_string_key_map { + let value = if use_string_key_map { let (k_handle, value) = { let blk = ctx.block(); let k_handle = unbox_str_handle(blk, &k_box); @@ -832,9 +945,9 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { "map", "js_map_get_string_key", ); - Ok(value) + value } else if use_number_key_map { - Ok(guarded_map_number_key_get(ctx, &m_handle, &k_box)) + guarded_map_number_key_get(ctx, &m_handle, &k_box) } else { let value = { let blk = ctx.block(); @@ -849,8 +962,10 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { "js_map_get", "receiver_or_key_not_static_string", ); - Ok(value) - } + value + }; + temp_root::temp_root_release(ctx, guard); + Ok(value) } Expr::MapHas { map, key } => { let use_string_key_map = @@ -858,8 +973,9 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { let use_number_key_map = !use_string_key_map && is_static_number_key_map(ctx, map) && is_numeric_expr(ctx, key); - let m_box = lower_expr(ctx, map)?; - let k_box = lower_expr(ctx, key)?; + // #6970: same hazard as `MapGet` — the key's lowering can collect + // while the receiver is live only in an SSA register. + let (m_box, k_box, guard) = temp_root::lower_operand_pair_rooted(ctx, map, key)?; let m_handle = { let blk = ctx.block(); unbox_to_i64(blk, &m_box) @@ -902,6 +1018,7 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { ); i32_v }; + temp_root::temp_root_release(ctx, guard); // NaN-tagged boolean for "true"/"false" printing. let blk = ctx.block(); let bit = blk.icmp_ne(I32, &i32_v, "0"); diff --git a/crates/perry-codegen/src/expr/temp_root.rs b/crates/perry-codegen/src/expr/temp_root.rs index 85c32bfd88..b1a11e9deb 100644 --- a/crates/perry-codegen/src/expr/temp_root.rs +++ b/crates/perry-codegen/src/expr/temp_root.rs @@ -52,6 +52,16 @@ pub(crate) fn temp_root_get_double(ctx: &mut FnCtx<'_>, idx: &str) -> String { ctx.block().bitcast_i64_to_double(&bits) } +/// Overwrite slot `idx` with a new raw `i64`. +/// +/// For producers that hand back a *different* address each round — the +/// `concat` accumulator (#6971), where every `js_string_concat` yields a new +/// string and the old one stops being the value that must stay alive. +pub(crate) fn temp_root_set_i64(ctx: &mut FnCtx<'_>, idx: &str, value_i64: &str) { + ctx.block() + .call_void("js_gc_temp_root_set", &[(I32, idx), (I64, value_i64)]); +} + /// Drop slot `idx` and everything pushed above it. pub(crate) fn temp_root_truncate(ctx: &mut FnCtx<'_>, idx: &str) { ctx.block() @@ -261,6 +271,175 @@ pub(crate) fn lower_operand_pair_rooted( Ok((left_value, right_value, guard)) } +/// Already-lowered operand values kept alive across work whose shape the +/// caller controls — a later operand whose *representation* is chosen per +/// branch (`Expr::MapSet`, #6970) or an allocation that happens after the whole +/// list is lowered (`new C(a, b)`, #6969). +/// +/// [`lower_exprs_rooted`] cannot serve those: it decides what to protect from +/// the expressions it is handed and re-reads immediately, whereas these sites +/// need the re-read to happen *after* a step the helper never sees. So the +/// caller supplies the protection decision and picks the re-read point. +/// +/// When `protect` is false this emits nothing at all and [`RootedOperands::reread`] +/// hands the original registers straight back, so unprotected sites keep their +/// pre-#6951 IR byte for byte. +pub(crate) struct RootedOperands { + /// Slot index per operand, or `None` when the operand was not rooted. + slots: Vec>, + /// The registers as originally lowered — the answer when nothing is rooted + /// and the operand cannot be re-loaded. + values: Vec, + /// Whether an unrooted operand must be re-loaded from its own storage + /// rather than reused from its register. See [`RootedOperands::reread`]. + reloadable: Vec, + /// First slot pushed; truncating it drops the whole group. + guard: Option, +} + +/// Does this operand read a location the collector *rewrites in place*, so that +/// re-lowering it after a collection yields the corrected address? +/// +/// A local with a shadow slot, a module global and a string-literal handle are +/// all registered roots — they are marked, and on an evacuating cycle they are +/// **rewritten**. That keeps the object alive and the *storage* correct, but it +/// says nothing about a register loaded from that storage beforehand: after +/// relocation the register holds the pre-move address. Re-loading is the fix, +/// and it is free — no temp-root traffic, just the load that would have been +/// emitted anyway. +/// +/// This is the same staleness #6981 reports one layer in (a raw typed-array +/// pointer passed under the specialized ABI). +pub(crate) fn operand_is_reloadable(expr: &Expr) -> bool { + // ONLY provably immutable sources. A string literal always re-lowers to a + // load of the same `__perry_init_strings_*` handle, so re-reading it can + // never observe a different value. + // + // A local or a module global must NOT be here, even though both are + // registered roots whose storage evacuation rewrites. Re-lowering one reads + // its value *now*, and "now" is after the later arguments, the field + // initializers and possibly an inlined constructor body have run — any of + // which may have reassigned it. `new C(g, bump())` where `bump()` sets + // `g` must capture `g`'s value at call time; re-lowering produced the + // post-`bump()` value, a miscompile rather than a rooting bug. Those + // operands get a real temp root instead: the slot preserves the call-time + // value AND the collector rewrites it on evacuation. + matches!(expr, Expr::String(_)) +} + +/// Build the protection **incrementally**, one operand at a time, so each is +/// rooted before the next one is lowered. +/// +/// That ordering is the whole point. Lowering every operand first and rooting +/// the finished list afterwards is not merely late, it is *worse than doing +/// nothing*: by then an earlier operand may already have been swept, and the +/// push publishes a dangling pointer into a slot the collector scans. That is +/// what turned #6969 from a silent wrong answer into a SIGSEGV, and it is why +/// `m.set(k, v)` roots `map` before `key` is lowered rather than after. +/// +/// See [`RootedOperands::push`] for the per-operand contract. +pub(crate) fn root_operands_begin(capacity: usize) -> RootedOperands { + RootedOperands { + slots: Vec::with_capacity(capacity), + values: Vec::with_capacity(capacity), + reloadable: Vec::with_capacity(capacity), + guard: None, + } +} + +impl RootedOperands { + /// Record one already-lowered operand. + /// + /// `collects` says "something between this operand and the consuming call + /// can reach a collection point" — the caller supplies it because the + /// hazard is not visible in an expression list: for `m.set(k, v)` the + /// receiver's window covers both `key`'s lowering and `value`'s, while the + /// key's covers only `value`'s. + /// + /// From that flag two decisions follow, and an operand needs exactly one: + /// + /// - [`operand_needs_root`] → push a temp-root slot, because nothing else + /// keeps this value alive; + /// - otherwise [`operand_is_reloadable`] → emit no runtime call, but + /// re-load the value at the re-read point, because its storage is a + /// registered root that evacuation *rewrites* while the cached register + /// keeps the old address. + /// + /// When `collects` is false neither applies: nothing can be swept and + /// nothing can move, so the register is reused and the IR is unchanged. + pub(crate) fn push( + &mut self, + ctx: &mut FnCtx<'_>, + operand: &Expr, + value: &str, + collects: bool, + ) { + let needs_root = collects && operand_needs_root(ctx, operand); + if needs_root { + let idx = temp_root_push_double(ctx, value); + // The FIRST slot pushed is the guard: truncating it drops every + // slot above it too, so one call releases the whole group. + if self.guard.is_none() { + self.guard = Some(idx.clone()); + } + self.slots.push(Some(idx)); + } else { + self.slots.push(None); + } + self.reloadable + .push(!needs_root && collects && operand_is_reloadable(operand)); + self.values.push(value.to_string()); + } + + /// Re-read every operand after the collection point. + /// + /// Three cases, and the third is the subtle one: + /// + /// - **rooted** → read the slot. Mandatory, not defensive: the slot is a + /// *mutable* root, so an evacuating cycle rewrites it and the register + /// pushed beforehand is stale. + /// - **unrooted but re-loadable** → re-lower it. A local/global/literal is + /// already a registered root, so it was never at risk of being *swept* — + /// but an evacuating cycle rewrote its storage, so the register loaded + /// before the collection points at where the object *used to be*. Emitting + /// the load again is correct and costs no runtime call. + /// - **unrooted and not re-loadable** → keep the register. This is only + /// reached for values `expr_is_known_non_pointer_shadow_value` proved are + /// not heap references, which relocation cannot invalidate. + pub(crate) fn reread( + &self, + ctx: &mut FnCtx<'_>, + operands: &[&Expr], + ) -> anyhow::Result> { + let mut out = Vec::with_capacity(self.values.len()); + for (i, original) in self.values.iter().enumerate() { + let value = match &self.slots[i] { + Some(idx) => { + let idx = idx.clone(); + temp_root_get_double(ctx, &idx) + } + None if self.reloadable[i] => super::lower_expr(ctx, operands[i])?, + None => original.clone(), + }; + out.push(value); + } + Ok(out) + } + + /// True when this group actually pushed slots — the signal a caller uses to + /// keep an eager unbox (and therefore its exact register numbering) on the + /// unprotected path. + pub(crate) fn is_rooted(&self) -> bool { + self.guard.is_some() + } + + /// Drop the group. Call it *after* the consuming call: the consumer + /// allocates while reading these values. + pub(crate) fn release(self, ctx: &mut FnCtx<'_>) { + temp_root_release(ctx, self.guard); + } +} + /// Release a guard returned by [`lower_exprs_rooted`]. Call it *after* the /// consuming call, not before: the consumer allocates while reading these /// values. @@ -320,3 +499,79 @@ pub(crate) fn any_may_trigger_gc<'a>( ) -> bool { exprs.into_iter().any(|e| expr_may_trigger_gc(ctx, e)) } + +/// Would `expr`'s lowered value need a temp root, assuming everything after it +/// reaches a collection point? +/// +/// A temp root buys two distinct things, and the suppressions here only give +/// up the first: +/// +/// 1. **liveness** — the object is marked instead of swept; +/// 2. **a re-readable location** — a slot the collector rewrites, so the value +/// can be recovered after relocation. +/// +/// Suppressed operands already have (1) from somewhere else, and get (2) from +/// [`operand_is_reloadable`] instead, which re-emits the load rather than +/// reusing the pre-collection register. Both halves are required: dropping the +/// second is exactly the staleness #6981 reports one layer in. +/// +/// - provably not a heap reference — a slot for it is pure TLS traffic, and +/// relocation cannot invalidate it either; +/// - a string literal — a load from a module global `__perry_init_strings_*` +/// registered with `js_gc_register_global_root`; +/// - a module-global read — `@perry_global_*` are registered GC roots +/// (marked *and* rewritten on evacuation); +/// - a local that **has a reserved shadow slot**, which binds the collector to +/// the local's own alloca — so evacuation rewrites the alloca in place. +/// +/// Together these are why `new C(a, b)` on ordinary locals emits no runtime +/// rooting calls even though the instance allocation that follows always +/// collects. +/// +/// The shadow-slot check is load-bearing, not decoration. Suppressing every +/// `LocalGet` looks equivalent and is not: a local can be pointer-valued and +/// have *no* shadow slot, in which case it lives in a bare alloca that the root +/// walk never visits (that is the #6968 defect) — so it has neither (1) nor +/// (2). `m.set(fresh(), churn())` regressed straight back to an abort when this +/// was written as a blanket `LocalGet` suppression; the Map receiver was +/// exactly such a local. +pub(crate) fn operand_needs_root(ctx: &FnCtx<'_>, expr: &Expr) -> bool { + if super::expr_is_known_non_pointer_shadow_value(ctx, expr) { + return false; + } + // Only a string literal is suppressed: it is a registered root AND + // immutable, so `operand_is_reloadable` can recover it with a plain load. + // + // Locals and module globals are deliberately NOT suppressed. Being a + // registered root buys liveness, but the value has to survive relocation + // *and* stay the value the call actually observed — and a re-load gives up + // the second. Rooting is the only thing that gives both, so they pay for a + // slot. + !matches!(expr, Expr::String(_)) +} + +/// Open an expression-scope temp-root barrier for a call/constructor whose +/// operands are `args`. +/// +/// Pushes a null marker slot and returns its index. Because +/// [`temp_root_truncate`] is a stack *cut*, [`temp_root_scope_end`] drops the +/// marker and every slot pushed above it — no matter which of the callee's +/// return paths ran. That is what makes rooting tractable in +/// `lower_call/new.rs`, where `lowered_args` is consumed at a dozen sites +/// spread over ~20 return paths (#6969); the alternative is a `temp_root_release` +/// at each, which is exactly the bookkeeping that gets missed. +/// +/// A null word decodes to nothing, so the marker itself roots no object. +/// Emits nothing when no operand could ever need rooting. +pub(crate) fn temp_root_scope_begin(ctx: &mut FnCtx<'_>, args: &[Expr]) -> Option { + args.iter() + .any(|a| operand_needs_root(ctx, a)) + .then(|| temp_root_push_i64(ctx, "0")) +} + +/// Close a barrier opened by [`temp_root_scope_begin`]. +pub(crate) fn temp_root_scope_end(ctx: &mut FnCtx<'_>, scope: Option) { + if let Some(idx) = scope { + temp_root_truncate(ctx, &idx); + } +} diff --git a/crates/perry-codegen/src/lower_call/new.rs b/crates/perry-codegen/src/lower_call/new.rs index 0c7f321eea..e1fafd2bd4 100644 --- a/crates/perry-codegen/src/lower_call/new.rs +++ b/crates/perry-codegen/src/lower_call/new.rs @@ -20,7 +20,7 @@ use super::new_helpers::{ ctor_body_has_value_return, ctor_body_uses_this, ctor_chain_uses_new_target, emit_promise_subclass_init, local_constructor_symbol_exists, node_stream_parent_kind, }; -use crate::expr::{lower_expr, lower_js_args_array, nanbox_pointer_inline, FnCtx}; +use crate::expr::{lower_expr, lower_js_args_array, nanbox_pointer_inline, temp_root, FnCtx}; use crate::nanbox::{double_literal, POINTER_MASK_I64}; use crate::types::{DOUBLE, I32, I64, I8, PTR}; @@ -131,11 +131,66 @@ pub(crate) fn lower_new_member_captured( lower_new_impl(ctx, class_name, args, true) } +/// Refresh `lowered_args` after something that may have collected (#6969). +/// +/// Two cases, and both are mandatory rather than defensive: +/// +/// - a **rooted** argument is re-read from its slot, because the slot is a +/// *mutable* root that an evacuating cycle rewrites in place, leaving the +/// register pushed beforehand stale; +/// - an argument that was NOT rooted because it reads a registered root (a +/// shadow-slotted local, a module global, a string literal) is **re-loaded**. +/// Those are never swept, but evacuation rewrote their storage too, so the +/// cached register points at where the value used to be. Re-lowering emits +/// the load again and costs no runtime call. +/// +/// Called after the instance allocation and again before the late consumers +/// that sit behind further arbitrary lowering (field initializers, an inlined +/// constructor body) — each of those is another chance to relocate. +fn refresh_rooted_args( + ctx: &mut FnCtx<'_>, + args: &[Expr], + lowered_args: &mut [String], + arg_roots: &[Option], +) -> Result<()> { + for (i, (value, slot)) in lowered_args.iter_mut().zip(arg_roots.iter()).enumerate() { + match slot { + Some(idx) => { + let idx = idx.clone(); + *value = temp_root::temp_root_get_double(ctx, &idx); + } + None if temp_root::operand_is_reloadable(&args[i]) => { + *value = lower_constructor_arg(ctx, &args[i])?; + } + None => {} + } + } + Ok(()) +} + fn lower_new_impl( ctx: &mut FnCtx<'_>, class_name: &str, args: &[Expr], caps_absent_from_args: bool, +) -> Result { + // #6969: expression-scope temp-root barrier. The body below roots its + // constructor arguments across the instance allocation, and it has ~20 + // return paths with `lowered_args` consumed at a dozen of them — one cut + // here releases the group whichever path ran, instead of a + // `temp_root_release` at each that reviewers and future edits must keep + // balanced. + let scope = temp_root::temp_root_scope_begin(ctx, args); + let result = lower_new_impl_inner(ctx, class_name, args, caps_absent_from_args); + temp_root::temp_root_scope_end(ctx, scope); + result +} + +fn lower_new_impl_inner( + ctx: &mut FnCtx<'_>, + class_name: &str, + args: &[Expr], + caps_absent_from_args: bool, ) -> Result { // Built-in Web classes that the runtime provides constructors for. // These are checked BEFORE the ctx.classes lookup because the user @@ -324,9 +379,26 @@ fn lower_new_impl( // user args happened to equal its captured locals. // Lower the args first (constructor params). + // + // #6969: each argument is rooted as soon as it is lowered, NOT after the + // loop — `new Pair(fresh(0), churn(N))` collects inside `churn`, which is + // argument 1's lowering, and by then argument 0 exists only in an SSA + // register. (Rooting after the loop is worse than not rooting at all: it + // publishes an already-dangling pointer to the scanner.) The roots also + // carry the arguments across the instance allocation below, which always + // collects; the re-read is immediately after it (see `obj_box`), and the + // scope cut in `lower_new_impl` is the release. let mut lowered_args: Vec = Vec::with_capacity(args.len()); + let mut arg_roots: Vec> = Vec::with_capacity(args.len()); for a in args { - lowered_args.push(lower_constructor_arg(ctx, a)?); + let value = lower_constructor_arg(ctx, a)?; + let slot = if temp_root::operand_needs_root(ctx, a) { + Some(temp_root::temp_root_push_double(ctx, &value)) + } else { + None + }; + lowered_args.push(value); + arg_roots.push(slot); } // Compute total field count including inherited parent fields. @@ -768,6 +840,9 @@ fn lower_new_impl( ) }; let obj_box = nanbox_pointer_inline(ctx.block(), &obj_handle); + // #6969: the instance allocation has run, so refresh every argument before + // the constructor consumes them. + refresh_rooted_args(ctx, args, &mut lowered_args, &arg_roots)?; // Constructor bodies may contain terminating recursive construction // shapes such as `if (typeof opts === "function") return new C(...)`. @@ -1485,6 +1560,9 @@ fn lower_new_impl( // Walked to an ancestor — call its ctor with this and forwarded args. // `...rest` ctors get the trailing args packed into one array // for the final slot (mirrors method_has_rest, #672). + // Field initializers / an inlined constructor body were lowered + // between the instance allocation and here, so refresh again. + refresh_rooted_args(ctx, args, &mut lowered_args, &arg_roots)?; let marshalled = marshal_imported_ctor_args(ctx, &ctor, &lowered_args); let mut ctor_args: Vec<(crate::types::LlvmType, &str)> = Vec::with_capacity(1 + marshalled.len()); @@ -1522,6 +1600,9 @@ fn lower_new_impl( // Pad missing optional args with TAG_UNDEFINED so the constructor // doesn't read garbage from stale registers, and pack the rest // slot into an array when the ctor's last param is `...rest`. + // Field initializers / an inlined constructor body were lowered + // between the instance allocation and here, so refresh again. + refresh_rooted_args(ctx, args, &mut lowered_args, &arg_roots)?; let marshalled = marshal_imported_ctor_args(ctx, &ctor, &lowered_args); // Pass `this` as NaN-boxed double (same as compile_method's this_arg). let mut ctor_args: Vec<(crate::types::LlvmType, &str)> = @@ -1608,6 +1689,9 @@ fn lower_new_impl( "js_get_dynamic_parent_value", &[(I32, &cid.to_string())], ); + // Same here: the dynamic-parent `super(...)` buffer is filled long + // after the allocation, behind further lowering. + refresh_rooted_args(ctx, args, &mut lowered_args, &arg_roots)?; let (args_ptr, args_len) = if lowered_args.is_empty() { ("null".to_string(), "0".to_string()) } else { diff --git a/crates/perry-codegen/src/lower_call/property_get/map_set.rs b/crates/perry-codegen/src/lower_call/property_get/map_set.rs index a403503acc..10cd6530d9 100644 --- a/crates/perry-codegen/src/lower_call/property_get/map_set.rs +++ b/crates/perry-codegen/src/lower_call/property_get/map_set.rs @@ -4,7 +4,7 @@ use anyhow::Result; use perry_hir::Expr; -use crate::expr::{lower_expr, unbox_to_i64, FnCtx}; +use crate::expr::{lower_expr, temp_root, unbox_to_i64, FnCtx}; use crate::nanbox::double_literal; use crate::type_analysis::{is_map_expr, is_set_expr, is_url_search_params_expr}; use crate::types::{DOUBLE, I64}; @@ -22,51 +22,70 @@ pub(crate) fn try_lower_map_set_methods( if is_map_expr(ctx, object) { match property { "set" if args.len() == 2 => { - let m_box = lower_expr(ctx, object)?; - let k_box = lower_expr(ctx, &args[0])?; - let v_box = lower_expr(ctx, &args[1])?; - let blk = ctx.block(); - let m_handle = unbox_to_i64(blk, &m_box); - blk.call_void( - "js_map_set", - &[(I64, &m_handle), (DOUBLE, &k_box), (DOUBLE, &v_box)], - ); + // #6970: each finished operand is live in an SSA register + // across the ones that follow, and those can collect. + let (vals, guard) = + temp_root::lower_exprs_rooted(ctx, &[object, &args[0], &args[1]])?; + let (m_box, k_box, v_box) = (vals[0].clone(), vals[1].clone(), vals[2].clone()); + { + let blk = ctx.block(); + let m_handle = unbox_to_i64(blk, &m_box); + blk.call_void( + "js_map_set", + &[(I64, &m_handle), (DOUBLE, &k_box), (DOUBLE, &v_box)], + ); + } + temp_root::temp_root_release(ctx, guard); return Ok(Some(m_box)); } "get" if args.len() == 1 => { - let m_box = lower_expr(ctx, object)?; - let k_box = lower_expr(ctx, &args[0])?; - let blk = ctx.block(); - let m_handle = unbox_to_i64(blk, &m_box); - return Ok(Some(blk.call( - DOUBLE, - "js_map_get", - &[(I64, &m_handle), (DOUBLE, &k_box)], - ))); + // #6970: the argument's lowering can collect while the + // receiver is live only in an SSA register. + let (m_box, k_box, guard) = + temp_root::lower_operand_pair_rooted(ctx, object, &args[0])?; + let value = { + let blk = ctx.block(); + let m_handle = unbox_to_i64(blk, &m_box); + blk.call(DOUBLE, "js_map_get", &[(I64, &m_handle), (DOUBLE, &k_box)]) + }; + temp_root::temp_root_release(ctx, guard); + return Ok(Some(value)); } "has" if args.len() == 1 => { - let m_box = lower_expr(ctx, object)?; - let k_box = lower_expr(ctx, &args[0])?; - let blk = ctx.block(); - let m_handle = unbox_to_i64(blk, &m_box); - let i32_v = blk.call( - crate::types::I32, - "js_map_has", - &[(I64, &m_handle), (DOUBLE, &k_box)], - ); - return Ok(Some(crate::expr::i32_bool_to_nanbox(blk, &i32_v))); + // #6970: the argument's lowering can collect while the + // receiver is live only in an SSA register. + let (m_box, k_box, guard) = + temp_root::lower_operand_pair_rooted(ctx, object, &args[0])?; + let result = { + let blk = ctx.block(); + let m_handle = unbox_to_i64(blk, &m_box); + let i32_v = blk.call( + crate::types::I32, + "js_map_has", + &[(I64, &m_handle), (DOUBLE, &k_box)], + ); + crate::expr::i32_bool_to_nanbox(blk, &i32_v) + }; + temp_root::temp_root_release(ctx, guard); + return Ok(Some(result)); } "delete" if args.len() == 1 => { - let m_box = lower_expr(ctx, object)?; - let k_box = lower_expr(ctx, &args[0])?; - let blk = ctx.block(); - let m_handle = unbox_to_i64(blk, &m_box); - let i32_v = blk.call( - crate::types::I32, - "js_map_delete", - &[(I64, &m_handle), (DOUBLE, &k_box)], - ); - return Ok(Some(crate::expr::i32_bool_to_nanbox(blk, &i32_v))); + // #6970: the argument's lowering can collect while the + // receiver is live only in an SSA register. + let (m_box, k_box, guard) = + temp_root::lower_operand_pair_rooted(ctx, object, &args[0])?; + let result = { + let blk = ctx.block(); + let m_handle = unbox_to_i64(blk, &m_box); + let i32_v = blk.call( + crate::types::I32, + "js_map_delete", + &[(I64, &m_handle), (DOUBLE, &k_box)], + ); + crate::expr::i32_bool_to_nanbox(blk, &i32_v) + }; + temp_root::temp_root_release(ctx, guard); + return Ok(Some(result)); } "clear" if args.is_empty() => { let m_box = lower_expr(ctx, object)?; @@ -112,36 +131,53 @@ pub(crate) fn try_lower_map_set_methods( if is_set_expr(ctx, object) { match property { "add" if args.len() == 1 => { - let s_box = lower_expr(ctx, object)?; - let v_box = lower_expr(ctx, &args[0])?; - let blk = ctx.block(); - let s_handle = unbox_to_i64(blk, &s_box); - blk.call_void("js_set_add", &[(I64, &s_handle), (DOUBLE, &v_box)]); + // #6970: the argument's lowering can collect while the + // receiver is live only in an SSA register. + let (s_box, v_box, guard) = + temp_root::lower_operand_pair_rooted(ctx, object, &args[0])?; + { + let blk = ctx.block(); + let s_handle = unbox_to_i64(blk, &s_box); + blk.call_void("js_set_add", &[(I64, &s_handle), (DOUBLE, &v_box)]); + } + temp_root::temp_root_release(ctx, guard); return Ok(Some(s_box)); } "has" if args.len() == 1 => { - let s_box = lower_expr(ctx, object)?; - let v_box = lower_expr(ctx, &args[0])?; - let blk = ctx.block(); - let s_handle = unbox_to_i64(blk, &s_box); - let i32_v = blk.call( - crate::types::I32, - "js_set_has", - &[(I64, &s_handle), (DOUBLE, &v_box)], - ); - return Ok(Some(crate::expr::i32_bool_to_nanbox(blk, &i32_v))); + // #6970: the argument's lowering can collect while the + // receiver is live only in an SSA register. + let (s_box, v_box, guard) = + temp_root::lower_operand_pair_rooted(ctx, object, &args[0])?; + let result = { + let blk = ctx.block(); + let s_handle = unbox_to_i64(blk, &s_box); + let i32_v = blk.call( + crate::types::I32, + "js_set_has", + &[(I64, &s_handle), (DOUBLE, &v_box)], + ); + crate::expr::i32_bool_to_nanbox(blk, &i32_v) + }; + temp_root::temp_root_release(ctx, guard); + return Ok(Some(result)); } "delete" if args.len() == 1 => { - let s_box = lower_expr(ctx, object)?; - let v_box = lower_expr(ctx, &args[0])?; - let blk = ctx.block(); - let s_handle = unbox_to_i64(blk, &s_box); - let i32_v = blk.call( - crate::types::I32, - "js_set_delete", - &[(I64, &s_handle), (DOUBLE, &v_box)], - ); - return Ok(Some(crate::expr::i32_bool_to_nanbox(blk, &i32_v))); + // #6970: the argument's lowering can collect while the + // receiver is live only in an SSA register. + let (s_box, v_box, guard) = + temp_root::lower_operand_pair_rooted(ctx, object, &args[0])?; + let result = { + let blk = ctx.block(); + let s_handle = unbox_to_i64(blk, &s_box); + let i32_v = blk.call( + crate::types::I32, + "js_set_delete", + &[(I64, &s_handle), (DOUBLE, &v_box)], + ); + crate::expr::i32_bool_to_nanbox(blk, &i32_v) + }; + temp_root::temp_root_release(ctx, guard); + return Ok(Some(result)); } "clear" if args.is_empty() => { let s_box = lower_expr(ctx, object)?; @@ -184,37 +220,50 @@ pub(crate) fn try_lower_map_set_methods( // a boolean. The runtime fns receive the receiver as an I64 set // handle and `other` as a NaN-boxed f64. "union" | "intersection" | "difference" | "symmetricDifference" if args.len() == 1 => { - let s_box = lower_expr(ctx, object)?; - let other_box = lower_expr(ctx, &args[0])?; - let blk = ctx.block(); - let s_handle = unbox_to_i64(blk, &s_box); - let runtime_fn = match property { - "union" => "js_set_union", - "intersection" => "js_set_intersection", - "difference" => "js_set_difference", - "symmetricDifference" => "js_set_symmetric_difference", - _ => unreachable!(), + // #6970: the argument's lowering can collect while the + // receiver is live only in an SSA register. + let (s_box, other_box, guard) = + temp_root::lower_operand_pair_rooted(ctx, object, &args[0])?; + let boxed = { + let blk = ctx.block(); + let s_handle = unbox_to_i64(blk, &s_box); + let runtime_fn = match property { + "union" => "js_set_union", + "intersection" => "js_set_intersection", + "difference" => "js_set_difference", + "symmetricDifference" => "js_set_symmetric_difference", + _ => unreachable!(), + }; + let result = + blk.call(I64, runtime_fn, &[(I64, &s_handle), (DOUBLE, &other_box)]); + crate::expr::nanbox_pointer_inline_pub(blk, &result) }; - let result = blk.call(I64, runtime_fn, &[(I64, &s_handle), (DOUBLE, &other_box)]); - return Ok(Some(crate::expr::nanbox_pointer_inline_pub(blk, &result))); + temp_root::temp_root_release(ctx, guard); + return Ok(Some(boxed)); } "isSubsetOf" | "isSupersetOf" | "isDisjointFrom" if args.len() == 1 => { - let s_box = lower_expr(ctx, object)?; - let other_box = lower_expr(ctx, &args[0])?; - let blk = ctx.block(); - let s_handle = unbox_to_i64(blk, &s_box); - let runtime_fn = match property { - "isSubsetOf" => "js_set_is_subset_of", - "isSupersetOf" => "js_set_is_superset_of", - "isDisjointFrom" => "js_set_is_disjoint_from", - _ => unreachable!(), + // #6970: the argument's lowering can collect while the + // receiver is live only in an SSA register. + let (s_box, other_box, guard) = + temp_root::lower_operand_pair_rooted(ctx, object, &args[0])?; + let result = { + let blk = ctx.block(); + let s_handle = unbox_to_i64(blk, &s_box); + let runtime_fn = match property { + "isSubsetOf" => "js_set_is_subset_of", + "isSupersetOf" => "js_set_is_superset_of", + "isDisjointFrom" => "js_set_is_disjoint_from", + _ => unreachable!(), + }; + let i32_v = blk.call( + crate::types::I32, + runtime_fn, + &[(I64, &s_handle), (DOUBLE, &other_box)], + ); + crate::expr::i32_bool_to_nanbox(blk, &i32_v) }; - let i32_v = blk.call( - crate::types::I32, - runtime_fn, - &[(I64, &s_handle), (DOUBLE, &other_box)], - ); - return Ok(Some(crate::expr::i32_bool_to_nanbox(blk, &i32_v))); + temp_root::temp_root_release(ctx, guard); + return Ok(Some(result)); } _ => {} } @@ -238,37 +287,57 @@ pub(crate) fn try_lower_collection_foreach( // with the full `(value, key, collection)` triple. Map.forEach // returns `undefined`. if is_map_expr(ctx, object) { - let m_box = lower_expr(ctx, object)?; - let cb_box = lower_expr(ctx, &args[0])?; - let this_arg = if args.len() >= 2 { - lower_expr(ctx, &args[1])? - } else { - double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED)) - }; - let blk = ctx.block(); - let m_handle = unbox_to_i64(blk, &m_box); - blk.call_void( - "js_map_foreach", - &[(I64, &m_handle), (DOUBLE, &cb_box), (DOUBLE, &this_arg)], - ); + // #6970: the callback (a closure allocation) and the optional + // `thisArg` are lowered after the receiver, so the receiver would + // otherwise be live only in an SSA register across them. + let mut operands: Vec<&Expr> = vec![object, &args[0]]; + if args.len() >= 2 { + operands.push(&args[1]); + } + let (vals, guard) = temp_root::lower_exprs_rooted(ctx, &operands)?; + let m_box = vals[0].clone(); + let cb_box = vals[1].clone(); + let this_arg = vals + .get(2) + .cloned() + .unwrap_or_else(|| double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED))); + { + let blk = ctx.block(); + let m_handle = unbox_to_i64(blk, &m_box); + blk.call_void( + "js_map_foreach", + &[(I64, &m_handle), (DOUBLE, &cb_box), (DOUBLE, &this_arg)], + ); + } + temp_root::temp_root_release(ctx, guard); return Ok(Some(double_literal(f64::from_bits( crate::nanbox::TAG_UNDEFINED, )))); } if is_set_expr(ctx, object) { - let s_box = lower_expr(ctx, object)?; - let cb_box = lower_expr(ctx, &args[0])?; - let this_arg = if args.len() >= 2 { - lower_expr(ctx, &args[1])? - } else { - double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED)) - }; - let blk = ctx.block(); - let s_handle = unbox_to_i64(blk, &s_box); - blk.call_void( - "js_set_foreach", - &[(I64, &s_handle), (DOUBLE, &cb_box), (DOUBLE, &this_arg)], - ); + // #6970: the callback (a closure allocation) and the optional + // `thisArg` are lowered after the receiver, so the receiver would + // otherwise be live only in an SSA register across them. + let mut operands: Vec<&Expr> = vec![object, &args[0]]; + if args.len() >= 2 { + operands.push(&args[1]); + } + let (vals, guard) = temp_root::lower_exprs_rooted(ctx, &operands)?; + let s_box = vals[0].clone(); + let cb_box = vals[1].clone(); + let this_arg = vals + .get(2) + .cloned() + .unwrap_or_else(|| double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED))); + { + let blk = ctx.block(); + let s_handle = unbox_to_i64(blk, &s_box); + blk.call_void( + "js_set_foreach", + &[(I64, &s_handle), (DOUBLE, &cb_box), (DOUBLE, &this_arg)], + ); + } + temp_root::temp_root_release(ctx, guard); return Ok(Some(double_literal(f64::from_bits( crate::nanbox::TAG_UNDEFINED, )))); @@ -281,19 +350,29 @@ pub(crate) fn try_lower_collection_foreach( // runtime entry so the callback gets the string `(value, key)` // pair instead of `(NaN, 0)` from the Array.forEach fast path. if is_url_search_params_expr(ctx, object) { - let p_box = lower_expr(ctx, object)?; - let cb_box = lower_expr(ctx, &args[0])?; - let this_arg = if args.len() >= 2 { - lower_expr(ctx, &args[1])? - } else { - double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED)) - }; - let blk = ctx.block(); - let p_handle = unbox_to_i64(blk, &p_box); - blk.call_void( - "js_url_search_params_for_each", - &[(I64, &p_handle), (DOUBLE, &cb_box), (DOUBLE, &this_arg)], - ); + // #6970: the callback (a closure allocation) and the optional + // `thisArg` are lowered after the receiver, so the receiver would + // otherwise be live only in an SSA register across them. + let mut operands: Vec<&Expr> = vec![object, &args[0]]; + if args.len() >= 2 { + operands.push(&args[1]); + } + let (vals, guard) = temp_root::lower_exprs_rooted(ctx, &operands)?; + let p_box = vals[0].clone(); + let cb_box = vals[1].clone(); + let this_arg = vals + .get(2) + .cloned() + .unwrap_or_else(|| double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED))); + { + let blk = ctx.block(); + let p_handle = unbox_to_i64(blk, &p_box); + blk.call_void( + "js_url_search_params_for_each", + &[(I64, &p_handle), (DOUBLE, &cb_box), (DOUBLE, &this_arg)], + ); + } + temp_root::temp_root_release(ctx, guard); return Ok(Some(double_literal(0.0))); } } diff --git a/crates/perry-codegen/src/lower_string_method.rs b/crates/perry-codegen/src/lower_string_method.rs index b9f17f405d..9bceb71729 100644 --- a/crates/perry-codegen/src/lower_string_method.rs +++ b/crates/perry-codegen/src/lower_string_method.rs @@ -8,8 +8,9 @@ use perry_hir::types::Type as HirType; use perry_hir::Expr; use crate::expr::temp_root::{ - lower_exprs_rooted, lower_operand_pair_rooted, temp_root_get_double, temp_root_get_i64, - temp_root_push_double, temp_root_push_i64, temp_root_release, temp_root_truncate, + self, lower_exprs_rooted, lower_operand_pair_rooted, temp_root_get_double, temp_root_get_i64, + temp_root_push_double, temp_root_push_i64, temp_root_release, temp_root_set_i64, + temp_root_truncate, }; use crate::expr::{ i32_bool_to_nanbox, lower_expr, nanbox_pointer_inline, nanbox_string_inline, unbox_str_handle, @@ -156,6 +157,70 @@ pub(crate) fn lower_string_method( nanbox_string_inline(blk, &coerced) }; + // #6971: the receiver is lowered BEFORE the arguments, and an argument's + // lowering can collect — `fresh(k).concat("|" + churn(N))` dropped the + // receiver, whose unboxed form is a BARE string address that only the + // `gc::root_words` bare form covers. Root it across the whole dispatch; + // the truncate below is the single release point for every one of the + // match's ~60 return paths. + let args_can_collect = args.iter().any(|a| temp_root::expr_may_trigger_gc(ctx, a)); + let recv_root = args_can_collect.then(|| temp_root_push_double(ctx, &recv_box)); + let result = lower_string_method_dispatch(ctx, object, property, args, &recv_box, &recv_root); + // Released only after the dispatch's consuming runtime call has run: that + // call allocates while it reads the receiver. + // + // The `is_terminated` guard is load-bearing, not defensive. The + // unknown-property arm of the dispatch throws and emits `unreachable`, then + // still returns `Ok(placeholder)` so callers have a register to phi against + // — so control reaches here with the block already terminated. Appending the + // truncate there would emit an instruction after the terminator: invalid IR, + // reachable from `("a" + churn()).nope(obj)` (an unrecognized string method + // whose arguments can collect, which is what sets `recv_root` at all). + // Skipping the release is sound: `unreachable` means no path resumes, and + // the temp-root stack is cut by the enclosing scope regardless. + // + // NOT covered by a regression test, deliberately: an attempted HIR-level + // reproducer (`("a" + "b").nope({})`) never reached the throwing arm — the + // emitted IR contained no `unreachable` at all — so the test passed with + // and without this guard. A test that is green either way is worse than no + // test, so it was removed rather than shipped. The guard is kept as + // defense-in-depth: emitting after a terminator is never correct, and the + // check is free. Reachability of the arm from a TypeScript source remains + // unproven; see the PR body. + if let Some(idx) = &recv_root { + if !ctx.block().is_terminated() { + temp_root_truncate(ctx, idx); + } + } + result +} + +/// Re-read the string-method receiver out of its temp root (#6971). +/// +/// Mandatory rather than defensive: the slot is a *mutable* root, so an +/// evacuating cycle rewrites it and the register pushed beforehand is stale. +/// Returns the original register when nothing was rooted, emitting no IR — so a +/// method whose arguments cannot collect keeps its previous code byte for byte. +fn reread_recv(ctx: &mut FnCtx<'_>, recv_root: &Option, recv_box: &str) -> String { + match recv_root { + Some(idx) => { + let idx = idx.clone(); + temp_root_get_double(ctx, &idx) + } + None => recv_box.to_string(), + } +} + +#[allow(clippy::too_many_lines)] +fn lower_string_method_dispatch( + ctx: &mut FnCtx<'_>, + object: &Expr, + property: &str, + args: &[Expr], + recv_box: &str, + recv_root: &Option, +) -> Result { + let recv_box = recv_box.to_string(); match property { "indexOf" => { if args.len() > 2 { @@ -182,6 +247,7 @@ pub(crate) fn lower_string_method( } else { None }; + let recv_box = reread_recv(ctx, recv_root, &recv_box); let blk = ctx.block(); let recv_handle = unbox_str_handle(blk, &recv_box); let needle_handle = if needle_is_str { @@ -230,6 +296,7 @@ pub(crate) fn lower_string_method( } else { None }; + let recv_box = reread_recv(ctx, recv_root, &recv_box); let blk = ctx.block(); let recv_handle = unbox_str_handle(blk, &recv_box); // String length (i32 at header offset 0). Used as the default end @@ -282,6 +349,7 @@ pub(crate) fn lower_string_method( // its coercion/undefined/RegExp checks for this common hot path. if args.len() == 1 && matches!(&args[0], Expr::String(_) | Expr::WtfString(_)) { let delim_box = lower_expr(ctx, &args[0])?; + let recv_box = reread_recv(ctx, recv_root, &recv_box); let blk = ctx.block(); let recv_handle = unbox_str_handle(blk, &recv_box); let delim_handle = unbox_str_handle(blk, &delim_box); @@ -310,6 +378,7 @@ pub(crate) fn lower_string_method( } else { None }; + let recv_box = reread_recv(ctx, recv_root, &recv_box); let blk = ctx.block(); let recv_handle = unbox_str_handle(blk, &recv_box); // No separator → pass `undefined`, which `js_string_split_value` @@ -354,6 +423,7 @@ pub(crate) fn lower_string_method( } else { Some(lower_expr(ctx, &args[0])?) }; + let recv_box = reread_recv(ctx, recv_root, &recv_box); let blk = ctx.block(); let locales_box = match locales_box { Some(v) => v, @@ -380,6 +450,7 @@ pub(crate) fn lower_string_method( for extra in args.iter() { let _ = lower_expr(ctx, extra)?; } + let recv_box = reread_recv(ctx, recv_root, &recv_box); let blk = ctx.block(); let recv_handle = unbox_str_handle(blk, &recv_box); let runtime_fn = match property { @@ -399,6 +470,7 @@ pub(crate) fn lower_string_method( for extra in args.iter() { let _ = lower_expr(ctx, extra)?; } + let recv_box = reread_recv(ctx, recv_root, &recv_box); let blk = ctx.block(); let recv_handle = unbox_str_handle(blk, &recv_box); let runtime_fn = match property { @@ -427,6 +499,7 @@ pub(crate) fn lower_string_method( for extra in args.iter().skip(1) { let _ = lower_expr(ctx, extra)?; } + let recv_box = reread_recv(ctx, recv_root, &recv_box); let blk = ctx.block(); let recv_handle = unbox_str_handle(blk, &recv_box); let value_handle = blk.call(I64, "js_string_coerce", &[(DOUBLE, &value_d)]); @@ -459,6 +532,7 @@ pub(crate) fn lower_string_method( for extra in args.iter().skip(1) { let _ = lower_expr(ctx, extra)?; } + let recv_box = reread_recv(ctx, recv_root, &recv_box); let blk = ctx.block(); let recv_handle = unbox_str_handle(blk, &recv_box); let idx_i32 = blk.call(I32, "js_string_index_to_i32", &[(DOUBLE, &idx_d)]); @@ -477,6 +551,7 @@ pub(crate) fn lower_string_method( ); } let count_d = lower_expr(ctx, &args[0])?; + let recv_box = reread_recv(ctx, recv_root, &recv_box); let blk = ctx.block(); let recv_handle = unbox_str_handle(blk, &recv_box); let result = blk.call( @@ -518,6 +593,7 @@ pub(crate) fn lower_string_method( let repl_is_str = is_string_expr(ctx, &args[1]); let needle_box = lower_expr(ctx, &args[0])?; let repl_box = lower_expr(ctx, &args[1])?; + let recv_box = reread_recv(ctx, recv_root, &recv_box); let blk = ctx.block(); let recv_handle = unbox_str_handle(blk, &recv_box); // #4871: a `searchValue` codegen can't type (an object-property @@ -641,6 +717,7 @@ pub(crate) fn lower_string_method( for extra in args.iter().skip(1) { let _ = lower_expr(ctx, extra)?; } + let recv_box = reread_recv(ctx, recv_root, &recv_box); let recv_handle = str_operand_handle_tag_dispatched(ctx, object, &recv_box); let blk = ctx.block(); let idx_i32 = blk.call(I32, "js_string_index_to_i32", &[(DOUBLE, &idx_d)]); @@ -662,6 +739,7 @@ pub(crate) fn lower_string_method( for extra in args.iter().skip(1) { let _ = lower_expr(ctx, extra)?; } + let recv_box = reread_recv(ctx, recv_root, &recv_box); let recv_handle = str_operand_handle_tag_dispatched(ctx, object, &recv_box); let blk = ctx.block(); let idx_i32 = blk.call(I32, "js_string_index_to_i32", &[(DOUBLE, &idx_d)]); @@ -683,6 +761,7 @@ pub(crate) fn lower_string_method( for extra in args.iter().skip(1) { let _ = lower_expr(ctx, extra)?; } + let recv_box = reread_recv(ctx, recv_root, &recv_box); let recv_handle = str_operand_handle_tag_dispatched(ctx, object, &recv_box); let blk = ctx.block(); let idx_i32 = blk.call(I32, "js_string_index_to_i32", &[(DOUBLE, &idx_d)]); @@ -720,6 +799,7 @@ pub(crate) fn lower_string_method( } else { None }; + let recv_box = reread_recv(ctx, recv_root, &recv_box); let blk = ctx.block(); let recv_handle = unbox_str_handle(blk, &recv_box); let needle_handle = if needle_is_str { @@ -788,6 +868,7 @@ pub(crate) fn lower_string_method( let sp_box = blk.load(DOUBLE, &sp_global); unbox_str_handle(blk, &sp_box) }; + let recv_box = reread_recv(ctx, recv_root, &recv_box); let blk = ctx.block(); let recv_handle = unbox_str_handle(blk, &recv_box); // Pass `target_length` as raw DOUBLE — the runtime does the @@ -824,6 +905,7 @@ pub(crate) fn lower_string_method( } form }; + let recv_box = reread_recv(ctx, recv_root, &recv_box); let blk = ctx.block(); let recv_handle = unbox_str_handle(blk, &recv_box); let result = blk.call( @@ -884,6 +966,7 @@ pub(crate) fn lower_string_method( &[(DOUBLE, loc), (DOUBLE, opts_ref)], ); } + let recv_box = reread_recv(ctx, recv_root, &recv_box); let blk = ctx.block(); let recv_handle = unbox_str_handle(blk, &recv_box); // A non-string `that` (undefined/number/object) must be @@ -926,6 +1009,7 @@ pub(crate) fn lower_string_method( } else { crate::nanbox::double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED)) }; + let recv_box = reread_recv(ctx, recv_root, &recv_box); let blk = ctx.block(); let recv_handle = unbox_str_handle(blk, &recv_box); let i32_v = blk.call( @@ -950,6 +1034,7 @@ pub(crate) fn lower_string_method( } else { crate::nanbox::double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED)) }; + let recv_box = reread_recv(ctx, recv_root, &recv_box); let blk = ctx.block(); let recv_handle = unbox_str_handle(blk, &recv_box); let result = blk.call( @@ -979,6 +1064,7 @@ pub(crate) fn lower_string_method( } else { crate::nanbox::double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED)) }; + let recv_box = reread_recv(ctx, recv_root, &recv_box); let blk = ctx.block(); let recv_handle = unbox_str_handle(blk, &recv_box); let result = blk.call( @@ -994,6 +1080,7 @@ pub(crate) fn lower_string_method( for extra in args.iter() { let _ = lower_expr(ctx, extra)?; } + let recv_box = reread_recv(ctx, recv_root, &recv_box); let blk = ctx.block(); let recv_handle = unbox_str_handle(blk, &recv_box); // Returns a NaN-tagged boolean directly. @@ -1004,6 +1091,7 @@ pub(crate) fn lower_string_method( for extra in args.iter() { let _ = lower_expr(ctx, extra)?; } + let recv_box = reread_recv(ctx, recv_root, &recv_box); let blk = ctx.block(); let recv_handle = unbox_str_handle(blk, &recv_box); let result = blk.call(I64, "js_string_to_well_formed", &[(I64, &recv_handle)]); @@ -1015,24 +1103,52 @@ pub(crate) fn lower_string_method( // arg (`undefined`, a boolean, a `{ toString }` object) must render // as its string form, not be bit-cast as a string handle (which // dropped `undefined`/booleans). A static string arg skips coercion. - let blk = ctx.block(); - let mut acc_handle = unbox_str_handle(blk, &recv_box); + // #6971: unlike every other arm, `concat` unboxes the receiver + // BEFORE it lowers its arguments, then keeps threading the running + // accumulator through an SSA register across each one. That + // accumulator is a bare `StringHeader*`, so the generic + // `reread_recv` above cannot help: it has to be rooted in its own + // right and written back after every concat, because each iteration + // produces a NEW address. + let mut acc_handle = { + let blk = ctx.block(); + unbox_str_handle(blk, &recv_box) + }; + let args_can_collect = args.iter().any(|a| temp_root::expr_may_trigger_gc(ctx, a)); + let acc_root = args_can_collect.then(|| temp_root_push_i64(ctx, &acc_handle)); for a in args { let a_is_str = is_string_expr(ctx, a); let s_box = lower_expr(ctx, a)?; - let blk = ctx.block(); - let s_handle = if a_is_str { - unbox_str_handle(blk, &s_box) - } else { - blk.call(I64, "js_string_coerce", &[(DOUBLE, &s_box)]) + // The ToString coercion allocates too, so re-read only after it. + let s_handle = { + let blk = ctx.block(); + if a_is_str { + unbox_str_handle(blk, &s_box) + } else { + blk.call(I64, "js_string_coerce", &[(DOUBLE, &s_box)]) + } }; - acc_handle = blk.call( + if let Some(idx) = &acc_root { + let idx = idx.clone(); + acc_handle = temp_root_get_i64(ctx, &idx); + } + acc_handle = ctx.block().call( I64, "js_string_concat", &[(I64, &acc_handle), (I64, &s_handle)], ); + // Write the new accumulator back, so the NEXT argument's + // lowering keeps *this* string alive rather than its input. + if let Some(idx) = &acc_root { + let idx = idx.clone(); + temp_root_set_i64(ctx, &idx, &acc_handle); + } + } + let boxed = nanbox_string_inline(ctx.block(), &acc_handle); + if let Some(idx) = &acc_root { + temp_root_truncate(ctx, idx); } - Ok(nanbox_string_inline(ctx.block(), &acc_handle)) + Ok(boxed) } "substr" => { // Legacy substr(start, length) — distinct from substring/slice: @@ -1053,6 +1169,7 @@ pub(crate) fn lower_string_method( } else { None }; + let recv_box = reread_recv(ctx, recv_root, &recv_box); let blk = ctx.block(); let recv_handle = unbox_str_handle(blk, &recv_box); // Pass the raw NaN-boxed args straight through: `js_string_substr` @@ -1088,6 +1205,7 @@ pub(crate) fn lower_string_method( } else { None }; + let recv_box = reread_recv(ctx, recv_root, &recv_box); let blk = ctx.block(); let recv_handle = unbox_str_handle(blk, &recv_box); let method_id = regexp_search_method_id(property); @@ -1148,6 +1266,7 @@ pub(crate) fn lower_string_method( } else { None }; + let recv_box = reread_recv(ctx, recv_root, &recv_box); let blk = ctx.block(); let recv_handle = unbox_str_handle(blk, &recv_box); let method_id = regexp_search_method_id(property); @@ -1188,6 +1307,7 @@ pub(crate) fn lower_string_method( for a in args { let _ = lower_expr(ctx, a)?; } + let recv_box = reread_recv(ctx, recv_root, &recv_box); let blk = ctx.block(); let handle = blk.call(I64, "js_jsvalue_to_string", &[(DOUBLE, &recv_box)]); Ok(nanbox_string_inline(blk, &handle)) diff --git a/crates/perry-codegen/tests/temp_root_operand_temporaries.rs b/crates/perry-codegen/tests/temp_root_operand_temporaries.rs new file mode 100644 index 0000000000..695a36715b --- /dev/null +++ b/crates/perry-codegen/tests/temp_root_operand_temporaries.rs @@ -0,0 +1,433 @@ +//! #6969 / #6970 / #6971 — the operand temporaries that #6951 did not reach. +//! +//! #6951 rooted variadic argument accumulators, concat operand pairs and +//! literal element lists. Three sibling lowering paths kept their operands in +//! bare LLVM SSA registers across a collection point: +//! +//! - **#6970** native collection-method arguments (`m.set(fresh(), churn())`) — +//! this one *aborted*, inside `js_map_set`, on a key whose header had been +//! recycled; +//! - **#6969** constructor arguments, held across the instance allocation +//! (which always collects) as well as across each other; +//! - **#6971** the string-method receiver, and `concat`'s accumulator — a bare +//! `StringHeader*`, the form only `gc::root_words`' bare case covers. +//! +//! These tests pin the *codegen contract*. The end-to-end proof is the +//! `cons_scan_off` arm (`PERRY_CONSERVATIVE_STACK_SCAN=off`), the only +//! configuration where the bug is observable — every other automatic collection +//! forces a conservative native-stack scan that pins the temporary by accident. +//! Equally important is the negative half: the shapes that were always safe +//! must still emit no rooting calls at all. + +use perry_codegen::{compile_module, AppMetadata, CompileOptions}; +use perry_hir::{Class, Expr, Module, ModuleInitKind, Stmt}; + +fn entry_opts() -> CompileOptions { + CompileOptions { + target: None, + is_entry_module: true, + non_entry_module_prefixes: Vec::new(), + nextjs_path_init_modules: Vec::new(), + import_function_prefixes: std::collections::HashMap::new(), + import_function_ffi_aliases: std::collections::HashMap::new(), + import_function_origin_names: std::collections::HashMap::new(), + import_function_v8_specifiers: std::collections::HashMap::new(), + import_function_node_submodule: std::collections::HashMap::new(), + namespace_node_submodules: std::collections::HashMap::new(), + namespace_v8_specifiers: std::collections::HashMap::new(), + namespace_member_prefixes: std::collections::HashMap::new(), + namespace_member_origin_names: std::collections::HashMap::new(), + emit_ir_only: true, + verify_native_regions: false, + disable_buffer_fast_path: false, + namespace_imports: Vec::new(), + imported_classes: Vec::new(), + imported_enums: Vec::new(), + imported_async_funcs: std::collections::HashSet::new(), + type_aliases: std::collections::HashMap::new(), + imported_func_param_counts: std::collections::HashMap::new(), + imported_func_has_rest: std::collections::HashSet::new(), + imported_func_synthetic_arguments: std::collections::HashSet::new(), + imported_func_return_types: std::collections::HashMap::new(), + imported_vars: std::collections::HashSet::new(), + output_type: "executable".to_string(), + needs_stdlib: false, + needs_ui: false, + needs_geisterhand: false, + geisterhand_port: 7676, + enabled_features: Vec::new(), + native_module_init_names: Vec::new(), + js_module_specifiers: Vec::new(), + bundled_extensions: Vec::new(), + native_library_functions: Vec::new(), + i18n_table: None, + fast_math: false, + fp_contract_mode: perry_codegen::FpContractMode::Off, + app_metadata: AppMetadata::default(), + namespace_entries: Vec::new(), + dynamic_import_path_to_prefix: std::collections::HashMap::new(), + deferred_module_prefixes: std::collections::HashSet::new(), + module_init_deps: Vec::new(), + is_dynamic_import_target: false, + debug_locations: false, + module_source: None, + debug_source_line_offset: 0, + } +} + +fn module_with_init(name: &str, init: Vec) -> Module { + Module { + name: name.to_string(), + imports: Vec::new(), + exports: Vec::new(), + classes: Vec::new(), + interfaces: Vec::new(), + type_aliases: Vec::new(), + enums: Vec::new(), + globals: Vec::new(), + functions: Vec::new(), + script_global_functions: Vec::new(), + references_global_this: false, + annexb_global_undefined_names: Vec::new(), + init, + exported_native_instances: Vec::new(), + exported_func_return_native_instances: Vec::new(), + exported_objects: Vec::new(), + exported_functions: Vec::new(), + widgets: Vec::new(), + uses_fetch: false, + uses_webassembly: false, + extern_funcs: Vec::new(), + init_was_unrolled: false, + has_top_level_await: false, + init_kind: ModuleInitKind::Eager, + async_step_closures: std::collections::HashSet::new(), + closure_display_names: std::collections::HashMap::new(), + class_display_names: std::collections::HashMap::new(), + closure_source_text: std::collections::HashMap::new(), + async_generator_funcs: std::collections::HashSet::new(), + gen_param_prologue_len: std::collections::HashMap::new(), + } +} + +fn ir_for(name: &str, init: Vec) -> String { + String::from_utf8(compile_module(&module_with_init(name, init), entry_opts()).unwrap()) + .expect("LLVM IR should be UTF-8") +} + +/// An allocating operand: an object literal is a collection point, which is all +/// `expr_may_trigger_gc` needs to see. +fn allocating() -> Expr { + Expr::Object(Vec::new()) +} + +// ---------------------------------------------------------------- #6970 ---- + +/// `m.set(key, value)` where `value` allocates: `key` is finished but lives in +/// an SSA register across `value`'s lowering. +/// +/// This is the abort in #6970. `js_map_set` ran with a key pointer whose block +/// the sweep had already returned and `churn` had reused, and the Map's +/// side-allocation owner record no longer matched — `grown Map must retain its +/// side-allocation owner record`, exit 134. +#[test] +fn map_set_key_is_rooted_across_an_allocating_value() { + let ir = ir_for( + "map_set_rooted.ts", + vec![Stmt::Expr(Expr::MapSet { + map: Box::new(Expr::MapNew), + key: Box::new(allocating()), + value: Box::new(allocating()), + })], + ); + + assert!( + ir.contains("call i32 @js_gc_temp_root_push"), + "the receiver and key must be pushed onto the temp-root stack before \ + the value's lowering, which collects (#6970):\n{ir}" + ); + assert!( + ir.contains("call i64 @js_gc_temp_root_get"), + "they must be RE-READ after the value is lowered — the slot is a \ + mutable root and an evacuating cycle rewrites it:\n{ir}" + ); + + let push = ir.find("call i32 @js_gc_temp_root_push").unwrap(); + let get = ir.find("call i64 @js_gc_temp_root_get").unwrap(); + let consume = ir.find("call i64 @js_map_set(").unwrap(); + let truncate = ir.find("call void @js_gc_temp_root_truncate").unwrap(); + assert!( + push < get && get < consume && consume < truncate, + "order must be push → re-read → consuming call → release; the release \ + comes last because js_map_set allocates while it reads the key:\n{ir}" + ); +} + +/// The gate: a `map.set` whose value cannot collect must emit no rooting at all. +#[test] +fn map_set_with_a_non_allocating_value_emits_no_rooting_calls() { + let ir = ir_for( + "map_set_no_gc.ts", + vec![Stmt::Expr(Expr::MapSet { + map: Box::new(Expr::MapNew), + key: Box::new(Expr::String("k".to_string())), + value: Box::new(Expr::Number(1.0)), + })], + ); + + assert!( + !ir.contains("call i32 @js_gc_temp_root_push"), + "nothing after the key can collect, so this must cost exactly what it \ + cost before (the `declare` line is unconditional; only a CALL counts):\n{ir}" + ); +} + +// ---------------------------------------------------------------- #6971 ---- + +/// `s.concat(x)` threads a bare `StringHeader*` accumulator through an SSA +/// register across every argument, and each `js_string_concat` returns a NEW +/// address — so the slot has to be written back, not just re-read. +#[test] +fn concat_accumulator_is_rooted_and_written_back() { + let ir = ir_for( + "concat_rooted.ts", + vec![Stmt::Expr(Expr::Call { + callee: Box::new(Expr::PropertyGet { + object: Box::new(Expr::Binary { + op: perry_hir::BinaryOp::Add, + left: Box::new(Expr::String("a".to_string())), + right: Box::new(Expr::String("b".to_string())), + }), + property: "concat".to_string(), + byte_offset: 0, + }), + args: vec![allocating()], + type_args: Vec::new(), + byte_offset: 0, + })], + ); + + assert!( + ir.contains("call i32 @js_gc_temp_root_push"), + "the concat accumulator must be rooted across an allocating argument \ + (#6971):\n{ir}" + ); + assert!( + ir.contains("call void @js_gc_temp_root_set"), + "each js_string_concat yields a NEW address, so the accumulator must be \ + written back into its slot — otherwise the next argument's lowering \ + keeps the INPUT alive and sweeps the string under construction:\n{ir}" + ); + assert!( + ir.contains("call i64 @js_gc_temp_root_get"), + "the accumulator must be re-read after the argument (and its ToString \ + coercion, which also allocates):\n{ir}" + ); +} + +/// The gate for the whole string-method family: a receiver whose arguments +/// cannot collect must not pay for the dispatch-wide root. +#[test] +fn string_method_with_non_allocating_args_emits_no_rooting_calls() { + let ir = ir_for( + "string_method_no_gc.ts", + vec![Stmt::Expr(Expr::Call { + callee: Box::new(Expr::PropertyGet { + object: Box::new(Expr::Binary { + op: perry_hir::BinaryOp::Add, + left: Box::new(Expr::String("a".to_string())), + right: Box::new(Expr::String("b".to_string())), + }), + property: "slice".to_string(), + byte_offset: 0, + }), + args: vec![Expr::Number(1.0)], + type_args: Vec::new(), + byte_offset: 0, + })], + ); + + assert!( + !ir.contains("call i32 @js_gc_temp_root_push"), + "a numeric argument cannot collect, so the receiver needs no root:\n{ir}" + ); +} + +// ---------------------------------------------------------------- #6969 ---- + +/// A module declaring `class Pair {}` plus a top-level `new Pair(a, b)`. +fn module_with_new(name: &str, args: Vec) -> Module { + let mut module = module_with_init( + name, + vec![Stmt::Expr(Expr::New { + class_name: "Pair".to_string(), + args, + type_args: Vec::new(), + byte_offset: 0, + cap_args_appended: 0, + })], + ); + module.classes = vec![Class { + id: 1, + name: "Pair".to_string(), + type_params: Vec::new(), + extends: None, + extends_name: None, + native_extends: None, + extends_expr: None, + heritage_lexically_shadowed: false, + fields: Vec::new(), + constructor: None, + methods: Vec::new(), + getters: Vec::new(), + setters: Vec::new(), + static_accessor_names: Vec::new(), + static_accessor_fn_ids: Vec::new(), + computed_members: Vec::new(), + static_fields: Vec::new(), + static_methods: Vec::new(), + decorators: Vec::new(), + is_exported: false, + aliases: Vec::new(), + is_nested: false, + alloc_width_hint: 0, + }]; + module +} + +fn ir_for_new(name: &str, args: Vec) -> String { + String::from_utf8(compile_module(&module_with_new(name, args), entry_opts()).unwrap()) + .expect("LLVM IR should be UTF-8") +} + +/// Constructor arguments are all lowered before the instance is allocated, and +/// that allocation always collects — so every heap-valued argument must be +/// rooted, and re-read after the allocation. +/// +/// The rooting must also be interleaved with the lowering, not appended after +/// it: argument 0 is live across argument 1's evaluation. Pushing the whole +/// list afterwards is strictly worse than not rooting at all — it publishes an +/// already-dangling pointer to the scanner, which turned the #6969 silent DIFF +/// into a SIGSEGV while this fix was being written. +#[test] +fn constructor_arguments_are_rooted_across_the_instance_allocation() { + let ir = ir_for_new("ctor_args_rooted.ts", vec![allocating(), allocating()]); + + assert!( + ir.contains("call i32 @js_gc_temp_root_push"), + "constructor arguments must be rooted (#6969):\n{ir}" + ); + assert!( + ir.contains("call i64 @js_gc_temp_root_get"), + "they must be re-read after the instance allocation:\n{ir}" + ); + + // Push order: the scope marker, then one root per heap argument — and + // argument 0's push must precede argument 1's *lowering*, not merely + // precede the instance allocation. + let pushes: Vec = ir + .match_indices("call i32 @js_gc_temp_root_push") + .map(|(i, _)| i) + .collect(); + assert!( + pushes.len() >= 3, + "expected the scope marker plus a root for each of the two heap \ + arguments, got {} pushes:\n{ir}", + pushes.len() + ); + // Each argument is an object literal, so each lowers to its own + // `js_object_alloc`; the SECOND one is argument 1's, i.e. the collection + // point argument 0 has to survive. + let arg_allocs: Vec = ir + .match_indices("call i64 @js_object_alloc(") + .map(|(i, _)| i) + .collect(); + assert!(arg_allocs.len() >= 2, "both arguments allocate:\n{ir}"); + assert!( + pushes[1] < arg_allocs[1], + "argument 0 must be rooted BEFORE argument 1 is lowered — rooting the \ + whole list after the loop publishes an already-dangling pointer (#6969):\n{ir}" + ); + + let instance_alloc = ir + .find("call i64 @js_object_alloc_class_inline_keys") + .expect("the instance allocation"); + let get = ir.find("call i64 @js_gc_temp_root_get").unwrap(); + assert!( + pushes[pushes.len() - 1] < instance_alloc && instance_alloc < get, + "every argument must still be rooted across the instance allocation, \ + and re-read after it:\n{ir}" + ); + + let truncate = ir.find("call void @js_gc_temp_root_truncate").unwrap(); + assert!( + get < truncate, + "the scope cut must come after the arguments are consumed:\n{ir}" + ); +} + +/// The gate: `new Pair(a, b)` on immediates must emit no rooting. +/// +/// A number roots nothing at all, and a string literal is already a registered +/// root, so neither needs a temp-root slot. (The literal is still *re-loaded* +/// after the allocation — see +/// `registered_root_operands_are_reloaded_rather_than_rooted` — but that is a +/// plain load, not a runtime call.) +#[test] +fn constructor_arguments_on_plain_locals_emit_no_rooting_calls() { + let ir = ir_for_new( + "ctor_args_locals.ts", + vec![Expr::Number(1.0), Expr::String("s".to_string())], + ); + + assert!( + !ir.contains("call i32 @js_gc_temp_root_push"), + "a number and a string literal need no root — the literal is a load \ + from a module global already registered with js_gc_register_global_root:\n{ir}" + ); +} + +/// An operand that reads a *registered root* is not rooted again — but it must +/// be **re-loaded**, not reused from its pre-collection register. +/// +/// A string literal, a module global and a shadow-slotted local are all marked +/// by the collector, so they are never swept. But an evacuating cycle +/// **rewrites their storage**, and the register loaded before the collection +/// still holds the pre-move address. Emitting the load again is correct and +/// costs no runtime call — the same staleness #6981 reports one layer in, for a +/// raw typed-array pointer under the specialized ABI. +#[test] +fn registered_root_operands_are_reloaded_rather_than_rooted() { + let ir = ir_for_new( + "ctor_args_reload.ts", + vec![Expr::String("lit".to_string()), allocating()], + ); + + let handle_load = "load double, ptr @ctor_args_reload_ts_.str."; + let loads: Vec = ir.match_indices(handle_load).map(|(i, _)| i).collect(); + assert!( + loads.len() >= 2, + "the literal operand must be loaded a SECOND time after the instance \ + allocation — reusing the first register leaves it pointing at where \ + the string used to be once evacuation moves it:\n{ir}" + ); + + let alloc = ir + .find("call i64 @js_object_alloc_class_inline_keys") + .expect("the instance allocation"); + assert!( + loads[0] < alloc && loads.iter().any(|&l| l > alloc), + "one load before the allocation (the original lowering) and one after \ + it (the re-load):\n{ir}" + ); + + // And it must be a re-LOAD, not a temp root: a registered root needs no + // second liveness mechanism, so this must cost zero runtime calls for it. + // (The allocating operand still gets a real root — hence >= 1 push.) + let pushes = ir.matches("call i32 @js_gc_temp_root_push").count(); + assert!( + pushes <= 2, + "expected at most the scope marker plus the allocating operand's root; \ + the literal must not get a slot of its own, got {pushes}:\n{ir}" + ); +} diff --git a/crates/perry-runtime/src/gc/tests/temp_roots.rs b/crates/perry-runtime/src/gc/tests/temp_roots.rs index d58f460c8f..afb46e1aa0 100644 --- a/crates/perry-runtime/src/gc/tests/temp_roots.rs +++ b/crates/perry-runtime/src/gc/tests/temp_roots.rs @@ -191,3 +191,87 @@ fn shadow_savepoint_restores_the_temp_root_depth() { js_gc_temp_root_truncate(outer); } + +/// Rewriting a slot must re-aim the root: the NEW value becomes reachable and +/// the replaced one does not stay alive on the strength of having once been in +/// that slot. +/// +/// This is the semantics `String.prototype.concat` depends on (#6971). Its +/// accumulator is a bare `StringHeader*` that changes address on every +/// iteration — each `js_string_concat` returns a *new* string — so codegen +/// writes the result back with `js_gc_temp_root_set` before lowering the next +/// argument. If a write-back rooted the old address instead of the new one, +/// the accumulator under construction would be the thing swept. +/// +/// Pins `ConservativeStackScanMode::Disabled`: with the unit-test default +/// (`Full`) the native-stack scan finds both raw pointers in these Rust locals +/// and the test passes without proving anything about precise roots. +#[test] +fn rewriting_a_slot_roots_the_new_value_and_releases_the_replaced_one() { + let _guard = CopyingNurseryTestGuard::new(1); + let _scan = ConservativeScanDisabledGuard::new(); + let _trigger_guard = GcTriggerThresholdTestGuard::suppress_automatic_triggers(); + register_temp_root_scanner_for_tests(); + reset_temp_roots(); + reset_old_reclaim_pressure(); + + let dead_headers = allocate_dead_malloc_churn_headers(8); + + let replaced = gc_malloc( + std::mem::size_of::(), + GC_TYPE_CLOSURE, + ); + let successor = gc_malloc( + std::mem::size_of::(), + GC_TYPE_CLOSURE, + ); + unsafe { + init_test_closure(replaced); + init_test_closure(successor); + } + assert!( + malloc_user_ptr_tracked(replaced) && malloc_user_ptr_tracked(successor), + "precondition: both objects are tracked" + ); + + // The accumulator pattern: root the first value, then hand the slot the + // successor the way `js_string_concat`'s result is written back. + let slot = js_gc_temp_root_push(replaced as u64); + js_gc_temp_root_set(slot, successor as u64); + + GC_NEXT_MALLOC_TRIGGER.with(|trigger| trigger.set(malloc_object_count().saturating_sub(1))); + gc_check_trigger(); + let completed = complete_budgeted_gc_cycle(); + assert_eq!(completed.status, JS_GC_STEP_STATUS_COMPLETED); + + assert_eq!( + tracked_malloc_headers_matching(&dead_headers), + 0, + "the sweep must actually have run for this test to mean anything" + ); + + let survivor = js_gc_temp_root_get(slot); + assert_eq!( + survivor, successor as u64, + "the slot must still hold the value written back into it" + ); + assert!( + malloc_user_ptr_tracked(survivor as *mut u8), + "the value a slot was re-aimed at must be marked, not swept (#6971)" + ); + unsafe { + assert_eq!( + (*(survivor as *mut crate::closure::ClosureHeader)).type_tag, + crate::closure::CLOSURE_MAGIC, + "the surviving accumulator must still be intact" + ); + } + assert!( + !malloc_user_ptr_tracked(replaced), + "the replaced value must NOT be retained by a slot that no longer \ + points at it — otherwise every concat round leaks its input" + ); + + js_gc_temp_root_truncate(slot); + assert_eq!(temp_root_depth(), 0); +} diff --git a/test-files/test_gap_ctor_arg_capture_order.ts b/test-files/test_gap_ctor_arg_capture_order.ts new file mode 100644 index 0000000000..a11dd22a41 --- /dev/null +++ b/test-files/test_gap_ctor_arg_capture_order.ts @@ -0,0 +1,45 @@ +// Constructor arguments must capture the value they had at `new` time, even +// when a LATER argument, a field initializer or the constructor body reassigns +// the variable they came from. +// +// Regression guard for the GC-rooting work (#6969/#6983): an argument that +// reads a registered root (a local, a module global) must be SNAPSHOT into a +// temp-root slot, not re-lowered after the fact. Re-lowering recovers the right +// address after an evacuating cycle but reads the variable's *current* value — +// which is a miscompile, not a rooting fix. Only immutable sources (string +// literals) may be re-loaded. +let g = "before"; +function bump(): number { + g = "after"; + return 1; +} + +class C { + p: unknown; + q: unknown; + constructor(p: unknown, q: unknown) { + this.p = p; + this.q = q; + } +} + +const sink: unknown[] = []; + +// Argument 0 reads `g` before bump() reassigns it. +const c = new C(g, bump()); +sink.push(c); // force a real allocation (defeat scalar replacement) +console.log("global captured:", c.p, "now:", g); + +// Same, with a local reassigned by the constructor body itself. +let local = "L0"; +class D { + v: unknown; + constructor(v: unknown) { + local = "L1"; + this.v = v; + } +} +const d = new D(local); +sink.push(d); +console.log("local captured:", d.v, "now:", local); +console.log("allocated:", sink.length);