diff --git a/changelog.d/7230-timer-arg-staging-rooting.md b/changelog.d/7230-timer-arg-staging-rooting.md new file mode 100644 index 0000000000..58a8894ab4 --- /dev/null +++ b/changelog.d/7230-timer-arg-staging-rooting.md @@ -0,0 +1,78 @@ +### Fixed + +- **GC: the timer family's callback and trailing arguments are rooted across their own argument list (#7210).** + `setTimeout(cb, 0, {…}, churn())` had two unrooted windows, and the callback's + was a live deterministic crash on `main`, not the staleness the issue predicted. + + `cb_box` was lowered first and read at `js_timer_validate_callback` — after the + delay's `lower_expr` and after every trailing argument's. A freshly-allocated + closure therefore sat in a bare SSA register across a user call carrying loop + back-edge polls, and the moving minor inside it left the register naming + from-space: `TypeError [ERR_INVALID_ARG_TYPE]: The "callback" argument must be + of type function. Received an instance of Object`, 3/3 under + `PERRY_GC_MOVING_LOOP_POLLS=1 PERRY_GC_HEAP_LIMIT=8` and identically on the + `evac_minor` arm. + + The trailing-argument staging buffer is the second window and is worse in kind: + argument *i* sits in a bare `alloca_entry_array`, storage the precise root walk + never visits, while argument *i+1* is lowered. Nothing anywhere refers to the + object at that moment, so it is a premature **sweep**, not a stale address. + + The `timers`-namespace forms carried a third: `cb_handle` was `unbox_to_i64`'d + — a raw heap address, not even NaN-boxed — above the trailing arguments. + + All three close with one change: lower the whole argument list through + `expr::temp_root::lower_exprs_rooted`, then fill the buffer in a second, + lowering-free pass, and release the guard only after the consuming call (which + reads the buffer). Cost is zero when nothing in the list can collect + (`OperandProtection::Reuse`) — `setTimeout(fn, 0, someLocal)` emits + byte-identical IR. Covers global `setTimeout`/`setInterval`/`setImmediate`, + their `timers`-namespace siblings, and `process.nextTick`. + + `crates/perry-codegen/src/lower_call/extern_func.rs` crossed the 2000-line cap, + so the timer arms moved to a new `lower_call/extern_timers.rs` — a pure + mechanical move, dispatched immediately above the match so arm order is + unchanged. Timer routing after the split was re-verified against node 26.5.1 on + a program exercising every moved arm. + + Witness `test-files/test_gap_gc_staging_args_rooting.ts`, registered in + `test-parity/gc_repsel_corpus.txt`: red 3/3 at base, `bad 0` 5/5 after, clean + 3/3 on the shipped default, byte-exact against node. + +### Notes + +- The `#7210` triage that motivated this change found that the **66** + moving-reachable `gc_root_dominance_check.py --unrooted-allocas` reports are + **all false positives**, and says so rather than adding roots: 64 are the + `@perry_class_keys_*` pointer cache, whose array is allocated by + `js_array_alloc_with_length_longlived` into the old arena and is therefore + moved only by old-page defrag — which is **off by default** since #6206 + (`PERRY_GC_OLD_DEFRAG=1` opt-in); the other 2 are `js_box_alloc_bits` results, + which come from `std::alloc::alloc`, are never freed and are never relocated + (`scan_box_roots_mut` rewrites the JSValue *inside* the box, not its address). + The checker's `_is_heap_source` conflates "a location the collector rewrites" + with "an object the collector can move"; the sites it names are the former and + not the latter. + +- **GC: `setInterval`'s trailing arguments are now scanned (#7210, runtime half).** + `scan_timer_roots_mut` walked `timer.args` for `CALLBACK_TIMERS` and for both + `MOCK_TIMERS` lists, and never for `INTERVAL_TIMERS`; the incremental twin + `scan_interval_timers_step` had the same hole, and cycle-based collections run + **only** the step scanner. So `setInterval(fn, delay, { … })` left the object + in a table nothing scanned — swept at the first collection, then handed to the + callback as a dangling pointer on the next tick. + + This is a different failure class from a stale register: it does not need a + collection to land in a narrow window, it goes wrong at collection #0 and + stays wrong, and no static IR checker can see it (the table is not in the + emitted IR). A partially-correct scanner is worse than an absent one, because + it reads as covered. + + It is also the other half of the codegen fix above — rooting an argument + across its own lowering buys nothing if the table it then lands in is not a + root. Witness `test-files/test_gap_gc_interval_args_rooting.ts`, registered in + the corpus: `BAD interval.a`/`BAD interval.b` from tick 1, 3/3 at base under + `PERRY_GC_MOVING_LOOP_POLLS=1 PERRY_GC_HEAP_LIMIT=8`; `bad 0` 5/5 after; clean + 3/3 on the shipped default; byte-exact against node 26.5.1. + + The wider population this came from is enumerated in #7231. diff --git a/crates/perry-codegen/src/expr/instance_misc1.rs b/crates/perry-codegen/src/expr/instance_misc1.rs index c68ca6b52d..7c6d179058 100644 --- a/crates/perry-codegen/src/expr/instance_misc1.rs +++ b/crates/perry-codegen/src/expr/instance_misc1.rs @@ -1015,8 +1015,8 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { // the varargs runtime entry; the no-args form goes through the // simpler `js_queue_next_tick` to avoid the alloca cost. Expr::ProcessNextTick { callback, args } => { - let cb_box = lower_expr(ctx, callback)?; if args.is_empty() { + let cb_box = lower_expr(ctx, callback)?; let blk = ctx.block(); // #3046: validate the callback (non-callable → Node's // `ERR_INVALID_ARG_TYPE` "callback" message) before queueing. @@ -1031,13 +1031,23 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { blk.call_void("js_queue_next_tick", &[(I64, &cb_handle)]); return Ok(double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED))); } + // #7210: the callback and every trailing argument go through + // `lower_exprs_rooted` together. Two windows closed at once — the + // callback register, held across every argument's `lower_expr`, and + // the staging buffer, in which argument *i* has no root at all + // while argument *i+1* is lowered. See `lower_call/extern_func.rs`'s + // `setTimeout` arm for the full argument. let n = args.len(); + let mut arg_refs: Vec<&Expr> = Vec::with_capacity(n + 1); + arg_refs.push(callback); + arg_refs.extend(args.iter()); + let (vals, guard) = super::temp_root::lower_exprs_rooted(ctx, &arg_refs)?; + let cb_box = vals[0].clone(); let buf = ctx.func.alloca_entry_array(DOUBLE, n); - for (i, a) in args.iter().enumerate() { - let v = lower_expr(ctx, a)?; + for (i, v) in vals.iter().skip(1).enumerate() { let blk = ctx.block(); let slot = blk.gep(DOUBLE, &buf, &[(I64, &format!("{}", i))]); - blk.store(DOUBLE, &v, &slot); + blk.store(DOUBLE, v, &slot); } let ptr_reg = ctx.block().next_reg(); ctx.block().emit_raw(format!( @@ -1055,6 +1065,7 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { "js_queue_next_tick_args", &[(I64, &cb_handle), (PTR, &ptr_reg), (I32, &n.to_string())], ); + super::temp_root::temp_root_release(ctx, guard); Ok(double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED))) } diff --git a/crates/perry-codegen/src/lower_call/extern_func.rs b/crates/perry-codegen/src/lower_call/extern_func.rs index f38173d875..7a0edd2d1b 100644 --- a/crates/perry-codegen/src/lower_call/extern_func.rs +++ b/crates/perry-codegen/src/lower_call/extern_func.rs @@ -1095,214 +1095,19 @@ pub fn try_lower_extern_func_call( if ctx.import_function_node_submodule.contains_key(name) { return Ok(None); } + // Timers (`setTimeout`/`setInterval`/`setImmediate` and their `clear*` + // siblings) live in `extern_timers.rs`. Split out under #7210, when the GC + // rooting fix for their trailing-argument staging buffers pushed this file + // over the 2000-line cap (`scripts/check_file_size.sh`). Dispatched HERE, + // immediately above the match, because they were its first arms: arm order + // is observable (`"setTimeout" if args.len() == 1` must still beat the + // generic consumer-prefix path at the bottom), and every guard they carry + // is on `args.len()`, so a non-matching timer name or arity falls through + // to exactly the arm it fell through to before. + if let Some(v) = super::extern_timers::try_lower_extern_timer_call(ctx, name.as_str(), args)? { + return Ok(Some(v)); + } match name.as_str() { - // #1671: `setTimeout(fn)` with no explicit delay. Node treats a - // missing/undefined delay as 0 (fires on the next timer tick). - // Without this arm a 1-arg `setTimeout` falls through to the - // catch-all below, which emits a bare LLVM call to `@setTimeout` - // and the linker fails with `Undefined symbols: _setTimeout` - // (hit by hono/jsx's `hooks/index.js`, which schedules a re-render - // via `setTimeout(() => { … })`). Route it to the same runtime - // entry as the 2-arg form with a zero delay. - "setTimeout" if args.len() == 1 => { - let cb_box = lower_expr(ctx, &args[0])?; - let blk = ctx.block(); - // #2013 — validate the callback type before unboxing the - // pointer. `js_timer_validate_callback` throws - // ERR_INVALID_ARG_TYPE for any non-callable value and - // returns the raw closure pointer otherwise; the second - // arg `0` is the type-name index for "setTimeout". - let zero_idx = "0"; - let cb_handle = blk.call( - I64, - "js_timer_validate_callback", - &[(DOUBLE, &cb_box), (I32, zero_idx)], - ); - let zero = double_literal(0.0); - let id = blk.call( - I64, - "js_set_timeout_callback", - &[(I64, &cb_handle), (DOUBLE, &zero)], - ); - return Ok(Some(nanbox_pointer_inline(blk, &id))); - } - "setTimeout" if args.len() == 2 => { - let cb_box = lower_expr(ctx, &args[0])?; - let delay_box = lower_expr(ctx, &args[1])?; - let blk = ctx.block(); - let zero_idx = "0"; - let cb_handle = blk.call( - I64, - "js_timer_validate_callback", - &[(DOUBLE, &cb_box), (I32, zero_idx)], - ); - let id = blk.call( - I64, - "js_set_timeout_callback", - &[(I64, &cb_handle), (DOUBLE, &delay_box)], - ); - return Ok(Some(nanbox_pointer_inline(blk, &id))); - } - "setImmediate" if !args.is_empty() => { - let cb_box = lower_expr(ctx, &args[0])?; - if args.len() == 1 { - let blk = ctx.block(); - let two_idx = "2"; - let cb_handle = blk.call( - I64, - "js_timer_validate_callback", - &[(DOUBLE, &cb_box), (I32, two_idx)], - ); - let id = blk.call(I64, "js_set_immediate_callback", &[(I64, &cb_handle)]); - return Ok(Some(nanbox_pointer_inline(blk, &id))); - } - - let n = args.len() - 1; - let buf = ctx.func.alloca_entry_array(DOUBLE, n); - for (i, a) in args.iter().skip(1).enumerate() { - let v = lower_expr(ctx, a)?; - let blk = ctx.block(); - let slot = blk.gep(DOUBLE, &buf, &[(I64, &format!("{}", i))]); - blk.store(DOUBLE, &v, &slot); - } - let ptr_reg = ctx.block().next_reg(); - ctx.block().emit_raw(format!( - "{} = getelementptr [{} x double], ptr {}, i64 0, i64 0", - ptr_reg, n, buf - )); - let blk = ctx.block(); - let two_idx = "2"; - let cb_handle = blk.call( - I64, - "js_timer_validate_callback", - &[(DOUBLE, &cb_box), (I32, two_idx)], - ); - let id = blk.call( - I64, - "js_set_immediate_callback_args", - &[(I64, &cb_handle), (PTR, &ptr_reg), (I32, &n.to_string())], - ); - return Ok(Some(nanbox_pointer_inline(blk, &id))); - } - // Refs #665: `setTimeout(fn, delay, ...args)` — JS spec forwards - // the trailing args to `fn` when the timer fires. Pack them into - // a stack buffer of doubles and hand off to the varargs runtime - // entry. Used by Promise-executor patterns like - // `setTimeout(resolve, delay, res)` (rate-limiter-flexible's - // `RateLimiterMemory.consume` is the discovering call site). - "setTimeout" if args.len() >= 3 => { - let cb_box = lower_expr(ctx, &args[0])?; - let delay_box = lower_expr(ctx, &args[1])?; - let n = args.len() - 2; - let buf = ctx.func.alloca_entry_array(DOUBLE, n); - for (i, a) in args.iter().skip(2).enumerate() { - let v = lower_expr(ctx, a)?; - let blk = ctx.block(); - let slot = blk.gep(DOUBLE, &buf, &[(I64, &format!("{}", i))]); - blk.store(DOUBLE, &v, &slot); - } - let ptr_reg = ctx.block().next_reg(); - ctx.block().emit_raw(format!( - "{} = getelementptr [{} x double], ptr {}, i64 0, i64 0", - ptr_reg, n, buf - )); - let blk = ctx.block(); - let zero_idx = "0"; - let cb_handle = blk.call( - I64, - "js_timer_validate_callback", - &[(DOUBLE, &cb_box), (I32, zero_idx)], - ); - let id = blk.call( - I64, - "js_set_timeout_callback_args", - &[ - (I64, &cb_handle), - (DOUBLE, &delay_box), - (crate::types::PTR, &ptr_reg), - (I32, &n.to_string()), - ], - ); - return Ok(Some(nanbox_pointer_inline(blk, &id))); - } - "setInterval" if args.len() == 2 => { - let cb_box = lower_expr(ctx, &args[0])?; - let delay_box = lower_expr(ctx, &args[1])?; - let blk = ctx.block(); - let one_idx = "1"; - let cb_handle = blk.call( - I64, - "js_timer_validate_callback", - &[(DOUBLE, &cb_box), (I32, one_idx)], - ); - let id = blk.call( - I64, - "setInterval", - &[(I64, &cb_handle), (DOUBLE, &delay_box)], - ); - return Ok(Some(nanbox_pointer_inline(blk, &id))); - } - "setInterval" if args.len() >= 3 => { - let cb_box = lower_expr(ctx, &args[0])?; - let delay_box = lower_expr(ctx, &args[1])?; - let n = args.len() - 2; - let buf = ctx.func.alloca_entry_array(DOUBLE, n); - for (i, a) in args.iter().skip(2).enumerate() { - let v = lower_expr(ctx, a)?; - let blk = ctx.block(); - let slot = blk.gep(DOUBLE, &buf, &[(I64, &format!("{}", i))]); - blk.store(DOUBLE, &v, &slot); - } - let ptr_reg = ctx.block().next_reg(); - ctx.block().emit_raw(format!( - "{} = getelementptr [{} x double], ptr {}, i64 0, i64 0", - ptr_reg, n, buf - )); - let blk = ctx.block(); - let one_idx = "1"; - let cb_handle = blk.call( - I64, - "js_timer_validate_callback", - &[(DOUBLE, &cb_box), (I32, one_idx)], - ); - let id = blk.call( - I64, - "js_set_interval_callback_args", - &[ - (I64, &cb_handle), - (DOUBLE, &delay_box), - (crate::types::PTR, &ptr_reg), - (I32, &n.to_string()), - ], - ); - return Ok(Some(nanbox_pointer_inline(blk, &id))); - } - "clearTimeout" if args.len() == 1 => { - // Pass the raw NaN-boxed arg so the runtime accepts both the - // handle and its primitive numeric id (`clearTimeout(+t)`, #1213). - let id_box = lower_expr(ctx, &args[0])?; - ctx.block() - .call_void("js_clear_timeout_value", &[(DOUBLE, &id_box)]); - return Ok(Some(double_literal(f64::from_bits( - crate::nanbox::TAG_UNDEFINED, - )))); - } - "clearInterval" if args.len() == 1 => { - let id_box = lower_expr(ctx, &args[0])?; - ctx.block() - .call_void("js_clear_interval_value", &[(DOUBLE, &id_box)]); - return Ok(Some(double_literal(f64::from_bits( - crate::nanbox::TAG_UNDEFINED, - )))); - } - "clearImmediate" if args.len() == 1 => { - let id_box = lower_expr(ctx, &args[0])?; - ctx.block() - .call_void("js_clear_immediate_value", &[(DOUBLE, &id_box)]); - return Ok(Some(double_literal(f64::from_bits( - crate::nanbox::TAG_UNDEFINED, - )))); - } "gc" => { ctx.block().call_void("js_gc_collect", &[]); return Ok(Some(double_literal(f64::from_bits( diff --git a/crates/perry-codegen/src/lower_call/extern_timers.rs b/crates/perry-codegen/src/lower_call/extern_timers.rs new file mode 100644 index 0000000000..a157a31034 --- /dev/null +++ b/crates/perry-codegen/src/lower_call/extern_timers.rs @@ -0,0 +1,275 @@ +//! Timer lowerings for `Expr::ExternFuncRef` — `setTimeout`, `setInterval`, +//! `setImmediate` and their `clear*` siblings. +//! +//! Split out of `extern_func.rs` (#7210) when that file crossed the 2000-line +//! cap. Pure mechanical move: every arm body below is a verbatim copy of the +//! arm it replaced, reached from `try_lower_extern_func_call`'s dispatch. +//! +//! The three trailing-argument forms share one GC contract, documented on the +//! `setTimeout` arm: the whole argument list is lowered through +//! `lower_exprs_rooted`, and only then stored into the stack buffer. + +use anyhow::Result; +use perry_hir::Expr; + +use crate::expr::{lower_expr, nanbox_pointer_inline, FnCtx}; +use crate::nanbox::double_literal; +use crate::types::{DOUBLE, I32, I64, PTR}; + +/// Lower a timer builtin, or `Ok(None)` if `name` is not one. +pub fn try_lower_extern_timer_call( + ctx: &mut FnCtx<'_>, + name: &str, + args: &[Expr], +) -> Result> { + match name { + // #1671: `setTimeout(fn)` with no explicit delay. Node treats a + // missing/undefined delay as 0 (fires on the next timer tick). + // Without this arm a 1-arg `setTimeout` falls through to the + // catch-all below, which emits a bare LLVM call to `@setTimeout` + // and the linker fails with `Undefined symbols: _setTimeout` + // (hit by hono/jsx's `hooks/index.js`, which schedules a re-render + // via `setTimeout(() => { … })`). Route it to the same runtime + // entry as the 2-arg form with a zero delay. + "setTimeout" if args.len() == 1 => { + let cb_box = lower_expr(ctx, &args[0])?; + let blk = ctx.block(); + // #2013 — validate the callback type before unboxing the + // pointer. `js_timer_validate_callback` throws + // ERR_INVALID_ARG_TYPE for any non-callable value and + // returns the raw closure pointer otherwise; the second + // arg `0` is the type-name index for "setTimeout". + let zero_idx = "0"; + let cb_handle = blk.call( + I64, + "js_timer_validate_callback", + &[(DOUBLE, &cb_box), (I32, zero_idx)], + ); + let zero = double_literal(0.0); + let id = blk.call( + I64, + "js_set_timeout_callback", + &[(I64, &cb_handle), (DOUBLE, &zero)], + ); + return Ok(Some(nanbox_pointer_inline(blk, &id))); + } + "setTimeout" if args.len() == 2 => { + let cb_box = lower_expr(ctx, &args[0])?; + let delay_box = lower_expr(ctx, &args[1])?; + let blk = ctx.block(); + let zero_idx = "0"; + let cb_handle = blk.call( + I64, + "js_timer_validate_callback", + &[(DOUBLE, &cb_box), (I32, zero_idx)], + ); + let id = blk.call( + I64, + "js_set_timeout_callback", + &[(I64, &cb_handle), (DOUBLE, &delay_box)], + ); + return Ok(Some(nanbox_pointer_inline(blk, &id))); + } + "setImmediate" if !args.is_empty() => { + if args.len() == 1 { + let cb_box = lower_expr(ctx, &args[0])?; + let blk = ctx.block(); + let two_idx = "2"; + let cb_handle = blk.call( + I64, + "js_timer_validate_callback", + &[(DOUBLE, &cb_box), (I32, two_idx)], + ); + let id = blk.call(I64, "js_set_immediate_callback", &[(I64, &cb_handle)]); + return Ok(Some(nanbox_pointer_inline(blk, &id))); + } + + // #7210: same treatment as `setTimeout` below — see the comment + // there for why the callback register and the staging buffer are + // one fix, not two. + let arg_refs: Vec<&Expr> = args.iter().collect(); + let (vals, guard) = crate::expr::temp_root::lower_exprs_rooted(ctx, &arg_refs)?; + let cb_box = vals[0].clone(); + let n = args.len() - 1; + let buf = ctx.func.alloca_entry_array(DOUBLE, n); + for (i, v) in vals.iter().skip(1).enumerate() { + let blk = ctx.block(); + let slot = blk.gep(DOUBLE, &buf, &[(I64, &format!("{}", i))]); + blk.store(DOUBLE, v, &slot); + } + let ptr_reg = ctx.block().next_reg(); + ctx.block().emit_raw(format!( + "{} = getelementptr [{} x double], ptr {}, i64 0, i64 0", + ptr_reg, n, buf + )); + let blk = ctx.block(); + let two_idx = "2"; + let cb_handle = blk.call( + I64, + "js_timer_validate_callback", + &[(DOUBLE, &cb_box), (I32, two_idx)], + ); + let id = blk.call( + I64, + "js_set_immediate_callback_args", + &[(I64, &cb_handle), (PTR, &ptr_reg), (I32, &n.to_string())], + ); + let boxed = nanbox_pointer_inline(ctx.block(), &id); + crate::expr::temp_root::temp_root_release(ctx, guard); + return Ok(Some(boxed)); + } + // Refs #665: `setTimeout(fn, delay, ...args)` — JS spec forwards + // the trailing args to `fn` when the timer fires. Pack them into + // a stack buffer of doubles and hand off to the varargs runtime + // entry. Used by Promise-executor patterns like + // `setTimeout(resolve, delay, res)` (rate-limiter-flexible's + // `RateLimiterMemory.consume` is the discovering call site). + "setTimeout" if args.len() >= 3 => { + // #7210: lower the WHOLE argument list through `lower_exprs_rooted`, + // then fill the buffer in a second, lowering-free pass. + // + // The previous shape had two unrooted windows, and the callback's + // was the one that crashed. `cb_box` was lowered first and read at + // `js_timer_validate_callback` — after `lower_expr(delay)` and after + // every trailing argument's `lower_expr`. `setTimeout(cb, 0, {…}, + // churn())` therefore held a freshly-allocated closure in an SSA + // register across a user call with loop back-edge polls, and the + // moving minor inside `churn` left the register naming from-space: + // `TypeError: The "callback" argument must be of type function. + // Received an instance of Object`, deterministically, at base. + // + // The staging buffer is the second window and is the worse of the + // two in kind: argument *i* sits in a bare `alloca_entry_array`, + // which the precise root walk never visits, while argument *i+1* is + // lowered. That is not staleness — nothing anywhere refers to the + // object, so it is a premature SWEEP. + // + // `lower_exprs_rooted` closes both at once: it protects each value + // as soon as it is produced and re-reads them all below the last + // one, so the stores below observe post-collection addresses. Cost + // is zero when nothing in the list can collect (`OperandProtection:: + // Reuse`), which is the `setTimeout(fn, 0, someLocal)` case. + let arg_refs: Vec<&Expr> = args.iter().collect(); + let (vals, guard) = crate::expr::temp_root::lower_exprs_rooted(ctx, &arg_refs)?; + let cb_box = vals[0].clone(); + let delay_box = vals[1].clone(); + let n = args.len() - 2; + let buf = ctx.func.alloca_entry_array(DOUBLE, n); + for (i, v) in vals.iter().skip(2).enumerate() { + let blk = ctx.block(); + let slot = blk.gep(DOUBLE, &buf, &[(I64, &format!("{}", i))]); + blk.store(DOUBLE, v, &slot); + } + let ptr_reg = ctx.block().next_reg(); + ctx.block().emit_raw(format!( + "{} = getelementptr [{} x double], ptr {}, i64 0, i64 0", + ptr_reg, n, buf + )); + let blk = ctx.block(); + let zero_idx = "0"; + let cb_handle = blk.call( + I64, + "js_timer_validate_callback", + &[(DOUBLE, &cb_box), (I32, zero_idx)], + ); + let id = blk.call( + I64, + "js_set_timeout_callback_args", + &[ + (I64, &cb_handle), + (DOUBLE, &delay_box), + (crate::types::PTR, &ptr_reg), + (I32, &n.to_string()), + ], + ); + let boxed = nanbox_pointer_inline(ctx.block(), &id); + // Released only after the consuming call: it reads the buffer. + crate::expr::temp_root::temp_root_release(ctx, guard); + return Ok(Some(boxed)); + } + "setInterval" if args.len() == 2 => { + let cb_box = lower_expr(ctx, &args[0])?; + let delay_box = lower_expr(ctx, &args[1])?; + let blk = ctx.block(); + let one_idx = "1"; + let cb_handle = blk.call( + I64, + "js_timer_validate_callback", + &[(DOUBLE, &cb_box), (I32, one_idx)], + ); + let id = blk.call( + I64, + "setInterval", + &[(I64, &cb_handle), (DOUBLE, &delay_box)], + ); + return Ok(Some(nanbox_pointer_inline(blk, &id))); + } + "setInterval" if args.len() >= 3 => { + // #7210: same treatment as `setTimeout` above. + let arg_refs: Vec<&Expr> = args.iter().collect(); + let (vals, guard) = crate::expr::temp_root::lower_exprs_rooted(ctx, &arg_refs)?; + let cb_box = vals[0].clone(); + let delay_box = vals[1].clone(); + let n = args.len() - 2; + let buf = ctx.func.alloca_entry_array(DOUBLE, n); + for (i, v) in vals.iter().skip(2).enumerate() { + let blk = ctx.block(); + let slot = blk.gep(DOUBLE, &buf, &[(I64, &format!("{}", i))]); + blk.store(DOUBLE, v, &slot); + } + let ptr_reg = ctx.block().next_reg(); + ctx.block().emit_raw(format!( + "{} = getelementptr [{} x double], ptr {}, i64 0, i64 0", + ptr_reg, n, buf + )); + let blk = ctx.block(); + let one_idx = "1"; + let cb_handle = blk.call( + I64, + "js_timer_validate_callback", + &[(DOUBLE, &cb_box), (I32, one_idx)], + ); + let id = blk.call( + I64, + "js_set_interval_callback_args", + &[ + (I64, &cb_handle), + (DOUBLE, &delay_box), + (crate::types::PTR, &ptr_reg), + (I32, &n.to_string()), + ], + ); + let boxed = nanbox_pointer_inline(ctx.block(), &id); + crate::expr::temp_root::temp_root_release(ctx, guard); + return Ok(Some(boxed)); + } + "clearTimeout" if args.len() == 1 => { + // Pass the raw NaN-boxed arg so the runtime accepts both the + // handle and its primitive numeric id (`clearTimeout(+t)`, #1213). + let id_box = lower_expr(ctx, &args[0])?; + ctx.block() + .call_void("js_clear_timeout_value", &[(DOUBLE, &id_box)]); + return Ok(Some(double_literal(f64::from_bits( + crate::nanbox::TAG_UNDEFINED, + )))); + } + "clearInterval" if args.len() == 1 => { + let id_box = lower_expr(ctx, &args[0])?; + ctx.block() + .call_void("js_clear_interval_value", &[(DOUBLE, &id_box)]); + return Ok(Some(double_literal(f64::from_bits( + crate::nanbox::TAG_UNDEFINED, + )))); + } + "clearImmediate" if args.len() == 1 => { + let id_box = lower_expr(ctx, &args[0])?; + ctx.block() + .call_void("js_clear_immediate_value", &[(DOUBLE, &id_box)]); + return Ok(Some(double_literal(f64::from_bits( + crate::nanbox::TAG_UNDEFINED, + )))); + } + _ => {} + } + Ok(None) +} diff --git a/crates/perry-codegen/src/lower_call/mod.rs b/crates/perry-codegen/src/lower_call/mod.rs index f542a13e6b..7913ca5e04 100644 --- a/crates/perry-codegen/src/lower_call/mod.rs +++ b/crates/perry-codegen/src/lower_call/mod.rs @@ -42,6 +42,7 @@ mod dataview_intrinsic; mod early_branches; mod event_target; mod extern_func; +mod extern_timers; mod field_init; mod func_ref; mod jsx; diff --git a/crates/perry-codegen/src/lower_call/namespace_call.rs b/crates/perry-codegen/src/lower_call/namespace_call.rs index f462daeec3..173e9c899b 100644 --- a/crates/perry-codegen/src/lower_call/namespace_call.rs +++ b/crates/perry-codegen/src/lower_call/namespace_call.rs @@ -47,30 +47,41 @@ pub fn try_lower_namespace_member_call( .is_some_and(|submod| submod == "timers") { match property.as_str() { + // #7210: the `timers` namespace forms carry the same two unrooted + // windows as the global `setTimeout`/`setInterval`/`setImmediate` + // lowerings in `extern_func.rs` (see the comment there), plus one + // of their own: `cb_handle` is `unbox_to_i64`'d — a RAW heap + // address, not even NaN-boxed — before the trailing arguments are + // lowered. Lower the whole list through `lower_exprs_rooted` and + // unbox below it, so the handle is derived from a post-collection + // value. "setTimeout" if !args.is_empty() => { - let cb_box = lower_expr(ctx, &args[0])?; + let arg_refs: Vec<&Expr> = args.iter().collect(); + let (vals, guard) = crate::expr::temp_root::lower_exprs_rooted(ctx, &arg_refs)?; + let cb_box = vals[0].clone(); let delay_box = if args.len() >= 2 { - lower_expr(ctx, &args[1])? + vals[1].clone() } else { double_literal(0.0) }; - let blk = ctx.block(); - let cb_handle = unbox_to_i64(blk, &cb_box); if args.len() <= 2 { + let blk = ctx.block(); + let cb_handle = unbox_to_i64(blk, &cb_box); let id = blk.call( I64, "js_set_timeout_callback", &[(I64, &cb_handle), (DOUBLE, &delay_box)], ); - return Ok(Some(nanbox_pointer_inline(blk, &id))); + let boxed = nanbox_pointer_inline(ctx.block(), &id); + crate::expr::temp_root::temp_root_release(ctx, guard); + return Ok(Some(boxed)); } let n = args.len() - 2; let buf = ctx.func.alloca_entry_array(DOUBLE, n); - for (i, a) in args.iter().skip(2).enumerate() { - let v = lower_expr(ctx, a)?; + for (i, v) in vals.iter().skip(2).enumerate() { let blk = ctx.block(); let slot = blk.gep(DOUBLE, &buf, &[(I64, &format!("{}", i))]); - blk.store(DOUBLE, &v, &slot); + blk.store(DOUBLE, v, &slot); } let ptr_reg = ctx.block().next_reg(); ctx.block().emit_raw(format!( @@ -78,6 +89,7 @@ pub fn try_lower_namespace_member_call( ptr_reg, n, buf )); let blk = ctx.block(); + let cb_handle = unbox_to_i64(blk, &cb_box); let id = blk.call( I64, "js_set_timeout_callback_args", @@ -88,28 +100,33 @@ pub fn try_lower_namespace_member_call( (I32, &n.to_string()), ], ); - return Ok(Some(nanbox_pointer_inline(blk, &id))); + let boxed = nanbox_pointer_inline(ctx.block(), &id); + crate::expr::temp_root::temp_root_release(ctx, guard); + return Ok(Some(boxed)); } "setInterval" if args.len() >= 2 => { - let cb_box = lower_expr(ctx, &args[0])?; - let delay_box = lower_expr(ctx, &args[1])?; - let blk = ctx.block(); - let cb_handle = unbox_to_i64(blk, &cb_box); + let arg_refs: Vec<&Expr> = args.iter().collect(); + let (vals, guard) = crate::expr::temp_root::lower_exprs_rooted(ctx, &arg_refs)?; + let cb_box = vals[0].clone(); + let delay_box = vals[1].clone(); if args.len() == 2 { + let blk = ctx.block(); + let cb_handle = unbox_to_i64(blk, &cb_box); let id = blk.call( I64, "setInterval", &[(I64, &cb_handle), (DOUBLE, &delay_box)], ); - return Ok(Some(nanbox_pointer_inline(blk, &id))); + let boxed = nanbox_pointer_inline(ctx.block(), &id); + crate::expr::temp_root::temp_root_release(ctx, guard); + return Ok(Some(boxed)); } let n = args.len() - 2; let buf = ctx.func.alloca_entry_array(DOUBLE, n); - for (i, a) in args.iter().skip(2).enumerate() { - let v = lower_expr(ctx, a)?; + for (i, v) in vals.iter().skip(2).enumerate() { let blk = ctx.block(); let slot = blk.gep(DOUBLE, &buf, &[(I64, &format!("{}", i))]); - blk.store(DOUBLE, &v, &slot); + blk.store(DOUBLE, v, &slot); } let ptr_reg = ctx.block().next_reg(); ctx.block().emit_raw(format!( @@ -117,6 +134,7 @@ pub fn try_lower_namespace_member_call( ptr_reg, n, buf )); let blk = ctx.block(); + let cb_handle = unbox_to_i64(blk, &cb_box); let id = blk.call( I64, "js_set_interval_callback_args", @@ -127,23 +145,27 @@ pub fn try_lower_namespace_member_call( (I32, &n.to_string()), ], ); - return Ok(Some(nanbox_pointer_inline(blk, &id))); + let boxed = nanbox_pointer_inline(ctx.block(), &id); + crate::expr::temp_root::temp_root_release(ctx, guard); + return Ok(Some(boxed)); } "setImmediate" if !args.is_empty() => { - let cb_box = lower_expr(ctx, &args[0])?; - let blk = ctx.block(); - let cb_handle = unbox_to_i64(blk, &cb_box); if args.len() == 1 { + let cb_box = lower_expr(ctx, &args[0])?; + let blk = ctx.block(); + let cb_handle = unbox_to_i64(blk, &cb_box); let id = blk.call(I64, "js_set_immediate_callback", &[(I64, &cb_handle)]); return Ok(Some(nanbox_pointer_inline(blk, &id))); } + let arg_refs: Vec<&Expr> = args.iter().collect(); + let (vals, guard) = crate::expr::temp_root::lower_exprs_rooted(ctx, &arg_refs)?; + let cb_box = vals[0].clone(); let n = args.len() - 1; let buf = ctx.func.alloca_entry_array(DOUBLE, n); - for (i, a) in args.iter().skip(1).enumerate() { - let v = lower_expr(ctx, a)?; + for (i, v) in vals.iter().skip(1).enumerate() { let blk = ctx.block(); let slot = blk.gep(DOUBLE, &buf, &[(I64, &format!("{}", i))]); - blk.store(DOUBLE, &v, &slot); + blk.store(DOUBLE, v, &slot); } let ptr_reg = ctx.block().next_reg(); ctx.block().emit_raw(format!( @@ -151,12 +173,15 @@ pub fn try_lower_namespace_member_call( ptr_reg, n, buf )); let blk = ctx.block(); + let cb_handle = unbox_to_i64(blk, &cb_box); let id = blk.call( I64, "js_set_immediate_callback_args", &[(I64, &cb_handle), (PTR, &ptr_reg), (I32, &n.to_string())], ); - return Ok(Some(nanbox_pointer_inline(blk, &id))); + let boxed = nanbox_pointer_inline(ctx.block(), &id); + crate::expr::temp_root::temp_root_release(ctx, guard); + return Ok(Some(boxed)); } "clearTimeout" | "clearInterval" | "clearImmediate" if !args.is_empty() => { let id_box = lower_expr(ctx, &args[0])?; diff --git a/crates/perry-runtime/src/timer.rs b/crates/perry-runtime/src/timer.rs index 9540c68cd2..7c84324fcd 100644 --- a/crates/perry-runtime/src/timer.rs +++ b/crates/perry-runtime/src/timer.rs @@ -1652,6 +1652,21 @@ pub fn scan_timer_roots_mut(visitor: &mut crate::gc::RuntimeRootVisitor<'_>) { if !timer.cleared && timer.callback != 0 { visitor.visit_i64_slot(&mut timer.callback); } + // #7210: `setInterval(fn, delay, ...args)` stores the trailing + // arguments in `IntervalTimer.args`, and this was the only one of + // the four blocks in this function that never walked them — the + // `CALLBACK_TIMERS` block above and both `MOCK_TIMERS` blocks below + // do. So `setInterval(fn, d, { … })` left the object in a table + // nothing scanned: swept at the first collection, then handed to the + // callback as a dangling pointer on the next tick. + // + // A partially-correct scanner is worse than an absent one — it reads + // as covered. This is also the runtime half of the codegen fix in + // the same change: rooting an argument across its own lowering buys + // nothing if the table it then lands in is not a root. + for arg in &mut timer.args { + visitor.visit_nanbox_f64_slot(arg); + } crate::async_context::scan_snapshot_roots_mut(&mut timer.context, visitor); } } diff --git a/crates/perry-runtime/src/timer/gc_scan.rs b/crates/perry-runtime/src/timer/gc_scan.rs index 6e6dc3bb27..1166ada873 100644 --- a/crates/perry-runtime/src/timer/gc_scan.rs +++ b/crates/perry-runtime/src/timer/gc_scan.rs @@ -174,6 +174,23 @@ fn scan_interval_timers_step( state.slot = 1; } if state.slot == 1 { + // #7210: the step twin of the `INTERVAL_TIMERS` args pass added to + // `scan_timer_roots_mut`. Cycle-based collections run ONLY the step + // scanner, so updating the stop-the-world function alone would leave + // `setInterval(fn, d, { … })` unrooted on exactly the incremental + // path — the same "only one of the two twins was updated" gap this + // file's `scan_mock_timers_step` comment already records. + while state.arg_index < timer.args.len() { + if !consume_timer_root_work(remaining) { + return false; + } + visitor.visit_nanbox_f64_slot(&mut timer.args[state.arg_index]); + state.arg_index += 1; + } + state.slot = 2; + state.arg_index = 0; + } + if state.slot == 2 { if !crate::async_context::scan_snapshot_roots_mut_step( &mut timer.context, visitor, @@ -183,7 +200,9 @@ fn scan_interval_timers_step( ) { return false; } - state.slot = 2; + state.slot = 3; + state.context_entry = 0; + state.context_store = 0; } state.index += 1; state.finish_timer(); diff --git a/test-files/test_gap_gc_interval_args_rooting.ts b/test-files/test_gap_gc_interval_args_rooting.ts new file mode 100644 index 0000000000..521d934e19 --- /dev/null +++ b/test-files/test_gap_gc_interval_args_rooting.ts @@ -0,0 +1,55 @@ +// #7210, runtime half: `setInterval(fn, delay, ...args)` stores its trailing +// arguments in `IntervalTimer.args`, and `scan_timer_roots_mut` was the only +// one of its four blocks that never walked them — `CALLBACK_TIMERS` and both +// `MOCK_TIMERS` lists do. The incremental twin `scan_interval_timers_step` had +// the same hole, and cycle-based collections run ONLY the step scanner. +// +// So the object below lived in a table nothing scanned: swept at the first +// collection, then handed to the callback as a dangling pointer on the next +// tick. This is a different failure class from a stale register — it does not +// need a collection to land in a narrow window, it goes wrong at collection #0 +// and stays wrong. +// +// It is also the other half of the codegen fix that ships with it: rooting an +// argument across its own lowering buys nothing if the table it lands in is not +// a root. + +function churn(n: number): number { + let acc = 0; + for (let i = 0; i < n; i++) { + const o = { i, pad: "y".repeat(12) }; + acc += o.i; + } + return acc; +} + +let bad = 0; +let ticks = 0; + +const payload = { tag: "interval", n: 3 }; + +const handle = setInterval( + (a: { tag: string; n: number }, b: string) => { + ticks++; + // Read the staged arguments back on every tick. The first tick can precede + // the first collection; the later ones cannot. + if (a === null || typeof a !== "object" || a.tag !== "interval" || a.n !== 3) { + bad++; + console.log("BAD interval.a tick " + String(ticks)); + } + if (b !== "second") { + bad++; + console.log("BAD interval.b tick " + String(ticks)); + } + churn(200); + if (ticks >= 4) { + clearInterval(handle); + console.log("bad " + String(bad)); + } + }, + 1, + payload, + "second", +); + +churn(400); diff --git a/test-files/test_gap_gc_staging_args_rooting.ts b/test-files/test_gap_gc_staging_args_rooting.ts new file mode 100644 index 0000000000..db30f37730 --- /dev/null +++ b/test-files/test_gap_gc_staging_args_rooting.ts @@ -0,0 +1,79 @@ +// #7210 §2: an argument staging buffer filled INTERLEAVED with lowering. +// +// `setTimeout(cb, 0, {…}, churn())` lowers to +// +// %buf = alloca [2 x double] ; a bare entry alloca +// %v0 = ; a fresh heap object +// store double %v0, ptr %buf[0] ; ← now the ONLY reference +// %v1 = call double @churn() ; loop back-edge poll -> moving minor +// store double %v1, ptr %buf[1] +// call void @js_set_timeout_callback_args(…, ptr %buf, …) +// +// `%buf` is neither a shadow slot nor a temp root, so the precise root walk +// never sees `%v0`. The window is not staleness — it is a premature SWEEP: at +// the moment `churn()` collects, nothing anywhere refers to the object. +// +// The same shape covers setImmediate / setInterval / process.nextTick / the +// `timers` namespace forms / a spread call's regular-argument buffer. + +function churn(n: number): number { + let acc = 0; + for (let i = 0; i < n; i++) { + const o = { i, pad: "x".repeat(8) }; + acc += o.i; + } + return acc; +} + +let bad = 0; + +function expect(label: string, got: unknown, want: unknown): void { + if (got !== want) { + bad++; + console.log("BAD " + label + ": got " + String(got) + " want " + String(want)); + } +} + +// --- setTimeout with trailing args ----------------------------------------- +setTimeout( + (a: { tag: string; n: number }, b: number) => { + expect("setTimeout.a.tag", a.tag, "alpha"); + expect("setTimeout.a.n", a.n, 11); + expect("setTimeout.b", b, 4950); + }, + 0, + { tag: "alpha", n: 11 }, + churn(100), +); + +// --- setImmediate with trailing args --------------------------------------- +setImmediate( + (a: { tag: string }, b: { tag: string }, c: number) => { + expect("setImmediate.a", a.tag, "beta"); + expect("setImmediate.b", b.tag, "gamma"); + expect("setImmediate.c", c, 4950); + }, + { tag: "beta" }, + { tag: "gamma" }, + churn(100), +); + +// --- process.nextTick with trailing args ----------------------------------- +process.nextTick( + (a: { tag: string }, b: number) => { + expect("nextTick.a", a.tag, "delta"); + expect("nextTick.b", b, 4950); + }, + { tag: "delta" }, + churn(100), +); + +// --- a spread call's regular-argument buffer ------------------------------- +const rest = [7, 8]; +const sink = (a: { tag: string }, b: number, ...more: number[]): string => + a.tag + ":" + String(b) + ":" + more.join(","); +expect("spread", sink({ tag: "eps" }, churn(100), ...rest), "eps:4950:7,8"); + +setTimeout(() => { + console.log("bad " + String(bad)); +}, 1); diff --git a/test-parity/gc_repsel_corpus.txt b/test-parity/gc_repsel_corpus.txt index ff1164da4f..8c681db114 100644 --- a/test-parity/gc_repsel_corpus.txt +++ b/test-parity/gc_repsel_corpus.txt @@ -339,3 +339,22 @@ test_gap_gc_closure_call_prev_this_rooting # fix in this PR is verified on the safepoint route and is NOT claimed on the # allocation-point route, which is #7217's open defect class at a second site. test_gap_gc_regexp_receiver_rooting +# --- #7210: argument staging buffers filled interleaved with lowering ------- +# `setTimeout(cb, 0, {…}, churn())` kept the CALLBACK closure in a bare SSA +# register across every argument's lowering, and each trailing argument in a +# bare `alloca_entry_array` the precise root walk never visits, while the next +# one was lowered. Measured at base (`7d1dc9ca2`), compiled and run under +# `PERRY_GC_MOVING_LOOP_POLLS=1 PERRY_GC_HEAP_LIMIT=8`: 5/5 +# `TypeError: The "callback" argument must be of type function. Received an +# instance of Object` — deterministic, and identical on the `evac_minor` arm. +# Clean 5/5 on the shipped default (no polls, no pressure) both before and +# after, so this is a `requires=move`-only witness like the #7207 trio. +test_gap_gc_staging_args_rooting + +# --- #7210 runtime half: setInterval trailing args were in no root ---------- +# `scan_timer_roots_mut` walked `args` for CALLBACK_TIMERS and for both +# MOCK_TIMERS lists, and NOT for INTERVAL_TIMERS; the incremental twin +# `scan_interval_timers_step` had the same hole, and cycle-based collections run +# only the step scanner. A partially-correct scanner is worse than an absent one +# — it reads as covered. Measured at base: `BAD interval.a` from tick 2 onward. +test_gap_gc_interval_args_rooting