Skip to content

Commit d2dcafe

Browse files
authored
fix(codegen): root the function across Func.prototype.x = <call> (#11635) (#11639)
* fix(codegen): root the function across Func.prototype.x = value lowering (#11635) * test: #11635 gap witness, codegen and runtime regression tests; rank prototype registration as a fatal sink * changelog: #11639 function prototype registration rooting * lint: lower the addr-class ratchet baseline (stale on main: 4 handle-floor sites already fixed)
1 parent 8d3b8ef commit d2dcafe

8 files changed

Lines changed: 250 additions & 10 deletions

File tree

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
- **fix(codegen): `F.prototype.x = <call>` 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`).

‎crates/perry-codegen/src/expr/computed_store_rooting_tests.rs‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -992,3 +992,69 @@ fn growing_array_store_uses_the_reallocated_head_for_its_barrier() {
992992
"the realloc-path barrier must use {new_head}, returned by the grow helper; got `{barrier}`"
993993
);
994994
}
995+
996+
/// #11635 — `F.prototype.x = f()` for a function declaration `F`
997+
/// (`Expr::RegisterFunctionPrototypeMethod`). `F` is evaluated first and was
998+
/// held in a register across the value's lowering, so an evacuating minor
999+
/// inside the value's call left `js_register_function_prototype_method`
1000+
/// reading the retired closure (moment 2.31.0's `proto.toIsoString =
1001+
/// deprecate(...)`). The function operand here is a call result — a value no
1002+
/// local slot can re-derive — so only a temp root can carry it across the
1003+
/// window. Its reload must sit below the allocating value and its store above.
1004+
#[test]
1005+
fn function_prototype_registration_roots_the_function_across_an_allocating_value() {
1006+
let make_func = Expr::Call {
1007+
callee: Box::new(Expr::LocalGet(1)),
1008+
args: Vec::new(),
1009+
type_args: Vec::new(),
1010+
byte_offset: 0,
1011+
};
1012+
let ir = compile_body_with_params(
1013+
"func_proto_register",
1014+
vec![param(1, "mk", Type::Any)],
1015+
vec![Stmt::Expr(Expr::RegisterFunctionPrototypeMethod {
1016+
func: Box::new(make_func),
1017+
method_name: "toIsoString".to_string(),
1018+
value: Box::new(allocating_value()),
1019+
})],
1020+
);
1021+
assert!(
1022+
calls(&ir, "js_register_function_prototype_method"),
1023+
"the registration arm was not reached:\n{ir}"
1024+
);
1025+
// The value operand is itself rooted across the registration call, so it
1026+
// reaches the call as a reload; the window is the value's ALLOCATION.
1027+
let alloc = ir
1028+
.lines()
1029+
.position(|l| l.contains("@js_object_alloc") && !l.trim_start().starts_with("declare"))
1030+
.unwrap_or_else(|| panic!("the allocating value was not emitted:\n{ir}"));
1031+
let func = call_operand_of(&ir, "js_register_function_prototype_method", 0);
1032+
let reload = producer_line(&ir, &func);
1033+
let reload_line = ir.lines().nth(reload).expect("producer line exists");
1034+
assert!(
1035+
reload_line.contains("load ptr addrspace(1), ptr "),
1036+
"the function operand ({func}) is not reloaded from a root slot:\n{ir}"
1037+
);
1038+
assert!(
1039+
reload > alloc,
1040+
"the function operand is reloaded at line {reload}, above the value's allocation at \
1041+
line {alloc}, so the register it names can be from-space:\n{ir}"
1042+
);
1043+
let slot = reload_line
1044+
.rsplit_once(", ptr ")
1045+
.map(|(_, tail)| tail.split(',').next().unwrap_or(tail).trim())
1046+
.expect("a root reload names its slot");
1047+
assert!(
1048+
ir.lines()
1049+
.take(alloc)
1050+
.any(|l| l.contains("store ptr addrspace(1)")
1051+
&& !l.contains(" null,")
1052+
&& l.rsplit_once(", ptr ").is_some_and(|(_, tail)| tail
1053+
.split(',')
1054+
.next()
1055+
.unwrap_or(tail)
1056+
.trim()
1057+
== slot)),
1058+
"root slot {slot} has no store above the value's allocation:\n{ir}"
1059+
);
1060+
}

‎crates/perry-codegen/src/expr/static_field_meta.rs‎

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -981,13 +981,26 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result<String> {
981981
// `new <FuncRef>(args)` lowering below stamps the same id on
982982
// the instance so dispatch finds the method via the regular
983983
// `(*obj).class_id` walk.
984+
//
985+
// #11635: `func` is evaluated FIRST and is live across the lowering
986+
// of `value`, which is routinely a call (`proto.toIsoString =
987+
// deprecate(msg, fn)` in moment). Holding it in a register let an
988+
// evacuating minor inside that call move the closure while the
989+
// register kept its from-space address, and the runtime then read
990+
// the retired closure header in `synthetic_class_id_for_function`.
991+
// Root it across the window and re-read it below. `value` is
992+
// rooted across the registration call too, because that call is a
993+
// `Reenters` runtime entry and the value is the expression result.
984994
Expr::RegisterFunctionPrototypeMethod {
985995
func,
986996
method_name,
987997
value,
988-
} => {
989-
let func_double = lower_expr(ctx, func)?;
990-
let val_double = lower_expr(ctx, value)?;
998+
} => with_rooted_group(ctx, 2, |ctx, group| {
999+
let protect_func = any_operand_may_collect(ctx, [value.as_ref()]);
1000+
let func_i = group.lower(ctx, func, protect_func)?;
1001+
let val_i = group.lower(ctx, value, true)?;
1002+
let func_double = group.reread(ctx, func_i)?;
1003+
let val_double = group.reread(ctx, val_i)?;
9911004
let key_idx = ctx.strings.intern(method_name);
9921005
let key_bytes_global = format!("@{}", ctx.strings.entry(key_idx).bytes_global);
9931006
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<String> {
10011014
(DOUBLE, &val_double),
10021015
],
10031016
);
1004-
Ok(val_double)
1005-
}
1017+
group.reread(ctx, val_i)
1018+
}),
10061019
// Read side of #838 followup (b): `<funcDecl>.prototype.<name>`
10071020
// (Ident or computed-string-literal form) lowered into a direct
10081021
// lookup of the prototype-method side-table. Returns the closure

‎crates/perry-runtime/src/gc/tests/copying_side_tables.rs‎

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,80 @@ fn test_copying_minor_rewrites_class_side_table_values_and_function_keys() {
8080
);
8181
}
8282

83+
/// #11635: the synthetic class id a `Func.prototype.x = fn` registration
84+
/// allocates is keyed by the function's NaN-boxed bits in
85+
/// `FUNCTION_CLASS_IDS`. When the function moves in a copying minor, the key
86+
/// must follow it: a second registration and a `new Func()` made through the
87+
/// POST-move address must land on the SAME id, or `F.prototype` methods
88+
/// registered before the move are split from instances created after it.
89+
///
90+
/// Driven through the real entry point (`js_register_function_prototype_method`)
91+
/// rather than a seeded key, and asserts the subject moved, so a green run
92+
/// cannot be one where nothing was relocated.
93+
#[test]
94+
fn test_function_prototype_registration_class_id_survives_a_move() {
95+
let _guard = CopyingNurseryTestGuard::new(1);
96+
crate::object::test_clear_class_side_table_roots();
97+
gc_register_mutable_root_scanner(crate::object::scan_class_side_table_roots_mut);
98+
99+
let func = crate::arena::arena_alloc_gc(
100+
std::mem::size_of::<crate::closure::ClosureHeader>(),
101+
std::mem::align_of::<crate::closure::ClosureHeader>(),
102+
GC_TYPE_CLOSURE,
103+
) as usize;
104+
unsafe { init_test_closure(func as *mut u8) };
105+
js_shadow_slot_set(0, ptr_bits(func));
106+
let before = young_leaf();
107+
let before_cid = unsafe {
108+
crate::object::js_register_function_prototype_method(
109+
f64::from_bits(ptr_bits(func)),
110+
b"before".as_ptr(),
111+
6,
112+
f64::from_bits(string_bits(before)),
113+
)
114+
};
115+
assert_ne!(
116+
before_cid, 0,
117+
"a verified closure must get a synthetic class id"
118+
);
119+
120+
let _ = gc_collect_minor();
121+
122+
let func_after_bits = js_shadow_slot_get(0);
123+
assert_ne!(
124+
func_after_bits,
125+
ptr_bits(func),
126+
"the function must MOVE, or this test proves nothing"
127+
);
128+
assert!(crate::arena::pointer_in_nursery(
129+
(func_after_bits & POINTER_MASK) as usize
130+
));
131+
132+
let after = young_leaf();
133+
let after_cid = unsafe {
134+
crate::object::js_register_function_prototype_method(
135+
f64::from_bits(func_after_bits),
136+
b"after".as_ptr(),
137+
5,
138+
f64::from_bits(string_bits(after)),
139+
)
140+
};
141+
assert_eq!(
142+
after_cid, before_cid,
143+
"a registration through the moved function must reuse its class id"
144+
);
145+
assert_eq!(
146+
crate::object::synthetic_class_id_for_function(f64::from_bits(func_after_bits)),
147+
before_cid,
148+
"`new F()` through the moved function must stamp the same class id"
149+
);
150+
assert_eq!(
151+
crate::object::test_class_prototype_method_root_bits(before_cid, "before") & TAG_MASK,
152+
STRING_TAG,
153+
"the method registered before the move must stay on the same class"
154+
);
155+
}
156+
83157
#[test]
84158
fn test_copying_minor_rewrites_symbol_side_table_roots_and_lookups() {
85159
let _guard = CopyingNurseryTestGuard::new(1);

‎scripts/addr_class_ratchet_baseline.txt‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ handle-floor | crates/perry-ext-http/src/lib.rs | 2
2828
handle-floor | crates/perry-runtime/src/array/alloc.rs | 2
2929
handle-floor | crates/perry-runtime/src/array/concat_reverse.rs | 1
3030
handle-floor | crates/perry-runtime/src/array/flat_clone.rs | 3
31-
handle-floor | crates/perry-runtime/src/array/generic.rs | 4
31+
handle-floor | crates/perry-runtime/src/array/generic.rs | 3
3232
handle-floor | crates/perry-runtime/src/array/header.rs | 3
3333
handle-floor | crates/perry-runtime/src/array/indexing.rs | 2
3434
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
6060
handle-floor | crates/perry-runtime/src/builtins/globals.rs | 9
6161
handle-floor | crates/perry-runtime/src/builtins/numbers.rs | 2
6262
handle-floor | crates/perry-runtime/src/builtins/table.rs | 1
63-
handle-floor | crates/perry-runtime/src/child_process/registry.rs | 1
6463
handle-floor | crates/perry-runtime/src/child_process/v8_serde.rs | 1
6564
handle-floor | crates/perry-runtime/src/closure/dispatch/validate.rs | 1
6665
handle-floor | crates/perry-runtime/src/cluster.rs | 1
67-
handle-floor | crates/perry-runtime/src/date.rs | 3
66+
handle-floor | crates/perry-runtime/src/date.rs | 1
6867
handle-floor | crates/perry-runtime/src/dgram.rs | 1
6968
handle-floor | crates/perry-runtime/src/dns.rs | 4
7069
handle-floor | crates/perry-runtime/src/exception.rs | 2
@@ -178,7 +177,7 @@ handle-floor | crates/perry-runtime/src/typed_feedback.rs | 3
178177
handle-floor | crates/perry-runtime/src/typed_feedback/guards.rs | 1
179178
handle-floor | crates/perry-runtime/src/typedarray/access.rs | 5
180179
handle-floor | crates/perry-runtime/src/typedarray/construct.rs | 4
181-
handle-floor | crates/perry-runtime/src/typedarray/mod.rs | 5
180+
handle-floor | crates/perry-runtime/src/typedarray/mod.rs | 4
182181
handle-floor | crates/perry-runtime/src/typedarray_props.rs | 2
183182
handle-floor | crates/perry-runtime/src/url/abort.rs | 1
184183
handle-floor | crates/perry-runtime/src/url/search_params.rs | 1

‎scripts/gc_root_dominance_check.py‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2833,7 +2833,11 @@ def rewritten_load_kind(text):
28332833
# A stale RegExpHeader* is dereferenced immediately by both of these —
28342834
# this is #7154's residual, and it faulted rather than merely answering
28352835
# wrong, so it belongs in the fatal ranking and not just the raw count.
2836-
r"regexp_test|regexp_exec|regexp_match\w*|regexp_replace\w*"
2836+
r"regexp_test|regexp_exec|regexp_match\w*|regexp_replace\w*|"
2837+
# Both read the function's closure header (`is_callable_function_value`)
2838+
# to key its synthetic class id. A stale function register here faulted
2839+
# at moment's module init (#11635), so it ranks as fatal too.
2840+
r"register_function_prototype_method|get_function_prototype_method"
28372841
r")$"
28382842
)
28392843

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,78 @@
1+
// #11635: `F.prototype.x = <call>` must not hold F in a register across the
2+
// call.
3+
//
4+
// HIR lowers `F.prototype.x = v` (and the aliased `var proto = F.prototype;
5+
// proto.x = v`) for a function declaration F into a runtime registration keyed
6+
// by F's closure. Codegen evaluated F first, then the value, then called the
7+
// registration with F's register. When the value is a call that collects, an
8+
// evacuating minor moves F while the register keeps its old address, and the
9+
// registration reads the retired closure. moment 2.31.0 does exactly this at
10+
// module init: `proto.toIsoString = deprecate(msg, toISOString$1)` with
11+
// `proto = Duration.prototype`, where Duration is a function declaration
12+
// inside the UMD factory. Under a seeded GC schedule the process faulted in
13+
// `synthetic_class_id_for_function`; without the from-space quarantine the
14+
// method could be registered under the stale address, so instances never saw
15+
// it.
16+
//
17+
// The shape is kept: F is declared inside a factory and captured by nested
18+
// functions (so it is a boxed heap closure that can move), the prototype is
19+
// aliased, and each value comes from a
20+
// helper that allocates enough garbage for a collection to land inside it.
21+
//
22+
// Output must be byte-identical to node.
23+
24+
function churn(rounds: number): number {
25+
let n = 0;
26+
for (let r = 0; r < rounds; r++) {
27+
const a: any[] = new Array(16);
28+
for (let j = 0; j < 16; j++) a[j] = { j, r, s: "v" + j };
29+
n += a.length;
30+
}
31+
return n;
32+
}
33+
34+
let churned = 0;
35+
36+
function factory(): any {
37+
const tag = "D";
38+
function Duration(this: any, v: number) {
39+
this.v = v;
40+
}
41+
function deprecate(msg: string, fn: (this: any) => string): (this: any) => string {
42+
churned += churn(20000);
43+
return function (this: any) {
44+
return msg + ":" + fn.call(this);
45+
};
46+
}
47+
function show(this: any) {
48+
return tag + this.v;
49+
}
50+
// Nested functions that capture Duration, as moment's do: that is what
51+
// puts Duration in a box whose read is not re-derivable from a root.
52+
function isDuration(o: any): boolean {
53+
return o instanceof Duration;
54+
}
55+
function make(v: number): any {
56+
return new (Duration as any)(v);
57+
}
58+
const proto = Duration.prototype;
59+
proto.a = deprecate("a", show);
60+
proto.b = deprecate("b", show);
61+
Duration.prototype.c = deprecate("c", show);
62+
proto.d = deprecate("d", show);
63+
(Duration as any).isDuration = isDuration;
64+
(Duration as any).make = make;
65+
return Duration;
66+
}
67+
68+
const D = factory();
69+
churn(20000);
70+
const out: string[] = [];
71+
for (let i = 0; i < 3; i++) {
72+
const d = new D(i);
73+
out.push(d.a(), d.b(), d.c(), d.d());
74+
out.push(String(d instanceof D), typeof D.prototype.a, typeof d.d);
75+
out.push(String(D.isDuration(d)), D.make(i + 10).c());
76+
}
77+
console.log(out.join(" "));
78+
console.log("churned", churned > 0);

‎test-parity/gc_repsel_corpus.txt‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -900,3 +900,8 @@ test_gap_gc_template_coerce_join
900900
# so the collector never traced the slot. claude-code `-p` read a tool
901901
# record's `prompt` method back as a plain object.
902902
test_gap_gc_11559_spill_headroom_store
903+
# `F.prototype.x = <call>` held F (a boxed function declaration) in a register
904+
# across the value's call, so an evacuating minor inside it left the
905+
# registration reading the retired closure. moment 2.31.0 module init under a
906+
# seeded schedule (#11635).
907+
test_gap_gc_11635_func_proto_register_across_call

0 commit comments

Comments
 (0)