diff --git a/changelog.d/11639-func-proto-register-rooting.md b/changelog.d/11639-func-proto-register-rooting.md new file mode 100644 index 0000000000..6f039ba78a --- /dev/null +++ b/changelog.d/11639-func-proto-register-rooting.md @@ -0,0 +1 @@ +- **fix(codegen): `F.prototype.x = ` no longer hands the runtime a from-space function (#11635).** `Expr::RegisterFunctionPrototypeMethod` lowered the function operand first and kept it in a register across the value's lowering. When the value was a call that collected (moment 2.31.0's `proto.toIsoString = deprecate(msg, fn)` at module init, with `Duration` a boxed function declaration), an evacuating minor moved the closure and `js_register_function_prototype_method` read the retired header in `synthetic_class_id_for_function`. Under a seeded schedule with the from-space quarantine that faulted (moment/parse_format: 22/200 seeds on main, 0/200 fixed); without it the method was registered against a stale address and instances never saw it (`TypeError: a is not a function` in the new witness, on every GC arm including the default one). The function and the value are now held in a `RootedGroup` and re-read below the window. The second hazard named in the issue, `FUNCTION_CLASS_IDS` being keyed by the closure's bits, was already handled by the class side-table scanner's rekey on evacuation; a new runtime test pins that through the real registration entry point (sabotaged: disabling the rekey turns it red). `gc_root_dominance_check.py --stale-registers` already reported the six moment uses; `--fatal-sinks` now ranks `js_register_function_prototype_method` / `js_get_function_prototype_method` as dereferencing sinks, so they no longer drop out of the fatal slice. New witness: `test-files/test_gap_gc_11635_func_proto_register_across_call.ts` (registered in `test-parity/gc_repsel_corpus.txt`). diff --git a/crates/perry-codegen/src/expr/computed_store_rooting_tests.rs b/crates/perry-codegen/src/expr/computed_store_rooting_tests.rs index f8a2ae1bfe..31b53ad115 100644 --- a/crates/perry-codegen/src/expr/computed_store_rooting_tests.rs +++ b/crates/perry-codegen/src/expr/computed_store_rooting_tests.rs @@ -992,3 +992,69 @@ fn growing_array_store_uses_the_reallocated_head_for_its_barrier() { "the realloc-path barrier must use {new_head}, returned by the grow helper; got `{barrier}`" ); } + +/// #11635 — `F.prototype.x = f()` for a function declaration `F` +/// (`Expr::RegisterFunctionPrototypeMethod`). `F` is evaluated first and was +/// held in a register across the value's lowering, so an evacuating minor +/// inside the value's call left `js_register_function_prototype_method` +/// reading the retired closure (moment 2.31.0's `proto.toIsoString = +/// deprecate(...)`). The function operand here is a call result — a value no +/// local slot can re-derive — so only a temp root can carry it across the +/// window. Its reload must sit below the allocating value and its store above. +#[test] +fn function_prototype_registration_roots_the_function_across_an_allocating_value() { + let make_func = Expr::Call { + callee: Box::new(Expr::LocalGet(1)), + args: Vec::new(), + type_args: Vec::new(), + byte_offset: 0, + }; + let ir = compile_body_with_params( + "func_proto_register", + vec![param(1, "mk", Type::Any)], + vec![Stmt::Expr(Expr::RegisterFunctionPrototypeMethod { + func: Box::new(make_func), + method_name: "toIsoString".to_string(), + value: Box::new(allocating_value()), + })], + ); + assert!( + calls(&ir, "js_register_function_prototype_method"), + "the registration arm was not reached:\n{ir}" + ); + // The value operand is itself rooted across the registration call, so it + // reaches the call as a reload; the window is the value's ALLOCATION. + let alloc = ir + .lines() + .position(|l| l.contains("@js_object_alloc") && !l.trim_start().starts_with("declare")) + .unwrap_or_else(|| panic!("the allocating value was not emitted:\n{ir}")); + let func = call_operand_of(&ir, "js_register_function_prototype_method", 0); + let reload = producer_line(&ir, &func); + let reload_line = ir.lines().nth(reload).expect("producer line exists"); + assert!( + reload_line.contains("load ptr addrspace(1), ptr "), + "the function operand ({func}) is not reloaded from a root slot:\n{ir}" + ); + assert!( + reload > alloc, + "the function operand is reloaded at line {reload}, above the value's allocation at \ + line {alloc}, so the register it names can be from-space:\n{ir}" + ); + let slot = reload_line + .rsplit_once(", ptr ") + .map(|(_, tail)| tail.split(',').next().unwrap_or(tail).trim()) + .expect("a root reload names its slot"); + assert!( + ir.lines() + .take(alloc) + .any(|l| l.contains("store ptr addrspace(1)") + && !l.contains(" null,") + && l.rsplit_once(", ptr ").is_some_and(|(_, tail)| tail + .split(',') + .next() + .unwrap_or(tail) + .trim() + == slot)), + "root slot {slot} has no store above the value's allocation:\n{ir}" + ); +} diff --git a/crates/perry-codegen/src/expr/static_field_meta.rs b/crates/perry-codegen/src/expr/static_field_meta.rs index 1a2c5bacc4..779454c010 100644 --- a/crates/perry-codegen/src/expr/static_field_meta.rs +++ b/crates/perry-codegen/src/expr/static_field_meta.rs @@ -981,13 +981,26 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { // `new (args)` lowering below stamps the same id on // the instance so dispatch finds the method via the regular // `(*obj).class_id` walk. + // + // #11635: `func` is evaluated FIRST and is live across the lowering + // of `value`, which is routinely a call (`proto.toIsoString = + // deprecate(msg, fn)` in moment). Holding it in a register let an + // evacuating minor inside that call move the closure while the + // register kept its from-space address, and the runtime then read + // the retired closure header in `synthetic_class_id_for_function`. + // Root it across the window and re-read it below. `value` is + // rooted across the registration call too, because that call is a + // `Reenters` runtime entry and the value is the expression result. Expr::RegisterFunctionPrototypeMethod { func, method_name, value, - } => { - let func_double = lower_expr(ctx, func)?; - let val_double = lower_expr(ctx, value)?; + } => with_rooted_group(ctx, 2, |ctx, group| { + let protect_func = any_operand_may_collect(ctx, [value.as_ref()]); + let func_i = group.lower(ctx, func, protect_func)?; + let val_i = group.lower(ctx, value, true)?; + let func_double = group.reread(ctx, func_i)?; + let val_double = group.reread(ctx, val_i)?; let key_idx = ctx.strings.intern(method_name); let key_bytes_global = format!("@{}", ctx.strings.entry(key_idx).bytes_global); let key_len = ctx.strings.entry(key_idx).byte_len.to_string(); @@ -1001,8 +1014,8 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { (DOUBLE, &val_double), ], ); - Ok(val_double) - } + group.reread(ctx, val_i) + }), // Read side of #838 followup (b): `.prototype.` // (Ident or computed-string-literal form) lowered into a direct // lookup of the prototype-method side-table. Returns the closure diff --git a/crates/perry-runtime/src/gc/tests/copying_side_tables.rs b/crates/perry-runtime/src/gc/tests/copying_side_tables.rs index ee0f409c9d..855c0ff24d 100644 --- a/crates/perry-runtime/src/gc/tests/copying_side_tables.rs +++ b/crates/perry-runtime/src/gc/tests/copying_side_tables.rs @@ -80,6 +80,80 @@ fn test_copying_minor_rewrites_class_side_table_values_and_function_keys() { ); } +/// #11635: the synthetic class id a `Func.prototype.x = fn` registration +/// allocates is keyed by the function's NaN-boxed bits in +/// `FUNCTION_CLASS_IDS`. When the function moves in a copying minor, the key +/// must follow it: a second registration and a `new Func()` made through the +/// POST-move address must land on the SAME id, or `F.prototype` methods +/// registered before the move are split from instances created after it. +/// +/// Driven through the real entry point (`js_register_function_prototype_method`) +/// rather than a seeded key, and asserts the subject moved, so a green run +/// cannot be one where nothing was relocated. +#[test] +fn test_function_prototype_registration_class_id_survives_a_move() { + let _guard = CopyingNurseryTestGuard::new(1); + crate::object::test_clear_class_side_table_roots(); + gc_register_mutable_root_scanner(crate::object::scan_class_side_table_roots_mut); + + let func = crate::arena::arena_alloc_gc( + std::mem::size_of::(), + std::mem::align_of::(), + GC_TYPE_CLOSURE, + ) as usize; + unsafe { init_test_closure(func as *mut u8) }; + js_shadow_slot_set(0, ptr_bits(func)); + let before = young_leaf(); + let before_cid = unsafe { + crate::object::js_register_function_prototype_method( + f64::from_bits(ptr_bits(func)), + b"before".as_ptr(), + 6, + f64::from_bits(string_bits(before)), + ) + }; + assert_ne!( + before_cid, 0, + "a verified closure must get a synthetic class id" + ); + + let _ = gc_collect_minor(); + + let func_after_bits = js_shadow_slot_get(0); + assert_ne!( + func_after_bits, + ptr_bits(func), + "the function must MOVE, or this test proves nothing" + ); + assert!(crate::arena::pointer_in_nursery( + (func_after_bits & POINTER_MASK) as usize + )); + + let after = young_leaf(); + let after_cid = unsafe { + crate::object::js_register_function_prototype_method( + f64::from_bits(func_after_bits), + b"after".as_ptr(), + 5, + f64::from_bits(string_bits(after)), + ) + }; + assert_eq!( + after_cid, before_cid, + "a registration through the moved function must reuse its class id" + ); + assert_eq!( + crate::object::synthetic_class_id_for_function(f64::from_bits(func_after_bits)), + before_cid, + "`new F()` through the moved function must stamp the same class id" + ); + assert_eq!( + crate::object::test_class_prototype_method_root_bits(before_cid, "before") & TAG_MASK, + STRING_TAG, + "the method registered before the move must stay on the same class" + ); +} + #[test] fn test_copying_minor_rewrites_symbol_side_table_roots_and_lookups() { let _guard = CopyingNurseryTestGuard::new(1); diff --git a/scripts/addr_class_ratchet_baseline.txt b/scripts/addr_class_ratchet_baseline.txt index b75f97452c..7cb202b8cc 100644 --- a/scripts/addr_class_ratchet_baseline.txt +++ b/scripts/addr_class_ratchet_baseline.txt @@ -28,7 +28,7 @@ handle-floor | crates/perry-ext-http/src/lib.rs | 2 handle-floor | crates/perry-runtime/src/array/alloc.rs | 2 handle-floor | crates/perry-runtime/src/array/concat_reverse.rs | 1 handle-floor | crates/perry-runtime/src/array/flat_clone.rs | 3 -handle-floor | crates/perry-runtime/src/array/generic.rs | 4 +handle-floor | crates/perry-runtime/src/array/generic.rs | 3 handle-floor | crates/perry-runtime/src/array/header.rs | 3 handle-floor | crates/perry-runtime/src/array/indexing.rs | 2 handle-floor | crates/perry-runtime/src/array/indexing_keyed.rs | 1 @@ -60,11 +60,10 @@ handle-floor | crates/perry-runtime/src/builtins/formatting/typed_array_equality handle-floor | crates/perry-runtime/src/builtins/globals.rs | 9 handle-floor | crates/perry-runtime/src/builtins/numbers.rs | 2 handle-floor | crates/perry-runtime/src/builtins/table.rs | 1 -handle-floor | crates/perry-runtime/src/child_process/registry.rs | 1 handle-floor | crates/perry-runtime/src/child_process/v8_serde.rs | 1 handle-floor | crates/perry-runtime/src/closure/dispatch/validate.rs | 1 handle-floor | crates/perry-runtime/src/cluster.rs | 1 -handle-floor | crates/perry-runtime/src/date.rs | 3 +handle-floor | crates/perry-runtime/src/date.rs | 1 handle-floor | crates/perry-runtime/src/dgram.rs | 1 handle-floor | crates/perry-runtime/src/dns.rs | 4 handle-floor | crates/perry-runtime/src/exception.rs | 2 @@ -178,7 +177,7 @@ handle-floor | crates/perry-runtime/src/typed_feedback.rs | 3 handle-floor | crates/perry-runtime/src/typed_feedback/guards.rs | 1 handle-floor | crates/perry-runtime/src/typedarray/access.rs | 5 handle-floor | crates/perry-runtime/src/typedarray/construct.rs | 4 -handle-floor | crates/perry-runtime/src/typedarray/mod.rs | 5 +handle-floor | crates/perry-runtime/src/typedarray/mod.rs | 4 handle-floor | crates/perry-runtime/src/typedarray_props.rs | 2 handle-floor | crates/perry-runtime/src/url/abort.rs | 1 handle-floor | crates/perry-runtime/src/url/search_params.rs | 1 diff --git a/scripts/gc_root_dominance_check.py b/scripts/gc_root_dominance_check.py index 3cae04b988..d3d46c94ff 100755 --- a/scripts/gc_root_dominance_check.py +++ b/scripts/gc_root_dominance_check.py @@ -2833,7 +2833,11 @@ def rewritten_load_kind(text): # A stale RegExpHeader* is dereferenced immediately by both of these — # this is #7154's residual, and it faulted rather than merely answering # wrong, so it belongs in the fatal ranking and not just the raw count. - r"regexp_test|regexp_exec|regexp_match\w*|regexp_replace\w*" + r"regexp_test|regexp_exec|regexp_match\w*|regexp_replace\w*|" + # Both read the function's closure header (`is_callable_function_value`) + # to key its synthetic class id. A stale function register here faulted + # at moment's module init (#11635), so it ranks as fatal too. + r"register_function_prototype_method|get_function_prototype_method" r")$" ) diff --git a/test-files/test_gap_gc_11635_func_proto_register_across_call.ts b/test-files/test_gap_gc_11635_func_proto_register_across_call.ts new file mode 100644 index 0000000000..32b39e8ef9 --- /dev/null +++ b/test-files/test_gap_gc_11635_func_proto_register_across_call.ts @@ -0,0 +1,78 @@ +// #11635: `F.prototype.x = ` must not hold F in a register across the +// call. +// +// HIR lowers `F.prototype.x = v` (and the aliased `var proto = F.prototype; +// proto.x = v`) for a function declaration F into a runtime registration keyed +// by F's closure. Codegen evaluated F first, then the value, then called the +// registration with F's register. When the value is a call that collects, an +// evacuating minor moves F while the register keeps its old address, and the +// registration reads the retired closure. moment 2.31.0 does exactly this at +// module init: `proto.toIsoString = deprecate(msg, toISOString$1)` with +// `proto = Duration.prototype`, where Duration is a function declaration +// inside the UMD factory. Under a seeded GC schedule the process faulted in +// `synthetic_class_id_for_function`; without the from-space quarantine the +// method could be registered under the stale address, so instances never saw +// it. +// +// The shape is kept: F is declared inside a factory and captured by nested +// functions (so it is a boxed heap closure that can move), the prototype is +// aliased, and each value comes from a +// helper that allocates enough garbage for a collection to land inside it. +// +// Output must be byte-identical to node. + +function churn(rounds: number): number { + let n = 0; + for (let r = 0; r < rounds; r++) { + const a: any[] = new Array(16); + for (let j = 0; j < 16; j++) a[j] = { j, r, s: "v" + j }; + n += a.length; + } + return n; +} + +let churned = 0; + +function factory(): any { + const tag = "D"; + function Duration(this: any, v: number) { + this.v = v; + } + function deprecate(msg: string, fn: (this: any) => string): (this: any) => string { + churned += churn(20000); + return function (this: any) { + return msg + ":" + fn.call(this); + }; + } + function show(this: any) { + return tag + this.v; + } + // Nested functions that capture Duration, as moment's do: that is what + // puts Duration in a box whose read is not re-derivable from a root. + function isDuration(o: any): boolean { + return o instanceof Duration; + } + function make(v: number): any { + return new (Duration as any)(v); + } + const proto = Duration.prototype; + proto.a = deprecate("a", show); + proto.b = deprecate("b", show); + Duration.prototype.c = deprecate("c", show); + proto.d = deprecate("d", show); + (Duration as any).isDuration = isDuration; + (Duration as any).make = make; + return Duration; +} + +const D = factory(); +churn(20000); +const out: string[] = []; +for (let i = 0; i < 3; i++) { + const d = new D(i); + out.push(d.a(), d.b(), d.c(), d.d()); + out.push(String(d instanceof D), typeof D.prototype.a, typeof d.d); + out.push(String(D.isDuration(d)), D.make(i + 10).c()); +} +console.log(out.join(" ")); +console.log("churned", churned > 0); diff --git a/test-parity/gc_repsel_corpus.txt b/test-parity/gc_repsel_corpus.txt index 628c113f3c..7ecbefbc4a 100644 --- a/test-parity/gc_repsel_corpus.txt +++ b/test-parity/gc_repsel_corpus.txt @@ -900,3 +900,8 @@ test_gap_gc_template_coerce_join # so the collector never traced the slot. claude-code `-p` read a tool # record's `prompt` method back as a plain object. test_gap_gc_11559_spill_headroom_store +# `F.prototype.x = ` held F (a boxed function declaration) in a register +# across the value's call, so an evacuating minor inside it left the +# registration reading the retired closure. moment 2.31.0 module init under a +# seeded schedule (#11635). +test_gap_gc_11635_func_proto_register_across_call