From 9407445b9a76b989b711bc1c590477ded51384c6 Mon Sep 17 00:00:00 2001 From: jdalton Date: Sat, 1 Aug 2026 08:23:44 -0400 Subject: [PATCH] fix(codegen): never let a shadow-frame slot index escape the pushed frame MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Duplicate `var` declarations share one HIR local id but keep a Stmt::Let per declaration site. collect_pointer_typed_locals burned a slot index for every Let while the map kept one entry per id, and every caller sizes the frame with map.len() — so functions with redeclarations pushed a frame smaller than the highest index handed out (lodash's runInContext: frame 598, binds up to 769). js_shadow_slot_bind and the #7088 inline store both bounds-check and silently no-op on an out-of-frame index, so those locals were live but invisible to the precise-root moving minor: an evacuating collection at a loop back-edge poll relocated the object and left the compiled local slot pointing into from-space — the mutator then re-injected the stale address through ordinary stores ("TypeError: value is not a function" once called). This is the sfw-registry --help crash under PERRY_GC_MOVING_LOOP_POLLS=1 traced in #7154 (lldb: lodash.js:10751, castRest read from an unrooted local). Assign at most one slot per id (entry().or_insert_with), restoring the invariant map.len() == slots handed out == max index + 1, and assert it. Regression test compiles a duplicate-var function and asserts every emitted slot index is inside the pushed frame. Refs #7154. --- .../7184-shadow-frame-slot-overflow.md | 3 + .../src/collectors/pointer_locals.rs | 47 +++++++-- .../tests/shadow_slot_hygiene.rs | 97 +++++++++++++++++++ 3 files changed, 141 insertions(+), 6 deletions(-) create mode 100644 changelog.d/7184-shadow-frame-slot-overflow.md diff --git a/changelog.d/7184-shadow-frame-slot-overflow.md b/changelog.d/7184-shadow-frame-slot-overflow.md new file mode 100644 index 0000000000..b81ac656bd --- /dev/null +++ b/changelog.d/7184-shadow-frame-slot-overflow.md @@ -0,0 +1,3 @@ +### Fixed + +- **codegen: shadow-frame slot indices could silently escape the pushed frame, unrooting live locals under the moving minor (#7154 partial)**. `collect_pointer_typed_locals` burned one slot index per `Stmt::Let`, but duplicate `var` declarations share a single HIR local id — the map kept one entry while the counter advanced, and every caller sizes the frame with `map.len()`. A function with redeclarations (lodash's `runInContext`: 170+ of them, frame 598, binds up to slot 769) therefore emitted root stores whose index failed `js_shadow_slot_bind`'s (and the #7088 inline store's) bounds check and silently no-opped. Those locals were live but invisible to the precise-root evacuating minor: a loop back-edge collection relocated the referent and left the compiled local slot pointing into from-space, which the mutator then re-injected through ordinary stores — `TypeError: value is not a function` when the stale value was called (the `sfw-registry --help` crash under `PERRY_GC_MOVING_LOOP_POLLS=1`; lldb-traced to `castRest` at lodash.js:10751). Slots are now assigned at most once per id, restoring `map.len() == slots handed out == max index + 1` (debug-asserted), with a regression test asserting every emitted slot index is inside the pushed frame. One additional polls-only offender remains in the registry workload and is tracked on #7154 — this does not close it, and stopgap #7161 stays. diff --git a/crates/perry-codegen/src/collectors/pointer_locals.rs b/crates/perry-codegen/src/collectors/pointer_locals.rs index e51a5bbe09..a7163bfd22 100644 --- a/crates/perry-codegen/src/collectors/pointer_locals.rs +++ b/crates/perry-codegen/src/collectors/pointer_locals.rs @@ -897,10 +897,37 @@ pub fn collect_pointer_typed_locals( let mut out = std::collections::HashMap::new(); let mut next_slot: u32 = 0; + /// Assign `id` a shadow-frame slot, exactly once. + /// + /// #7154 root cause: a local id can appear in MORE THAN ONE `Stmt::Let` — + /// duplicate `var` declarations share a single HIR binding, and lowering + /// keeps a `Let` at each declaration site (lodash's `runInContext` has + /// 170+ of them). The old `out.insert(id, slot); slot += 1;` replaced the + /// map entry but still burned a slot index, so `out.len()` (what every + /// caller passes to `enable_shadow_frame` / + /// `enable_post_init_shadow_frame`) undercounted the indices actually + /// handed out. Every local whose index landed at or beyond the frame + /// length failed `js_shadow_slot_bind` / the #7088 inline store's bounds + /// check SILENTLY — the local was live but invisible to the precise-root + /// moving minor, so an evacuation at a loop back-edge poll relocated the + /// object and left the compiled local slot pointing into from-space + /// ("TypeError: value is not a function" once it was called). + /// + /// Assigning through `entry` keeps one slot per id, restoring the + /// invariant the frame sizing depends on: `out.len() == next_slot == + /// max_index + 1`. Re-binding the same slot from each duplicate + /// declaration site is correct — the id names one alloca, and the bind + /// snapshots that alloca's current value either way. + fn assign_slot(out: &mut std::collections::HashMap, next_slot: &mut u32, id: u32) { + out.entry(id).or_insert_with(|| { + let s = *next_slot; + *next_slot += 1; + s + }); + } for p in params { if is_ptr_typed(&p.ty) && !non_pointer_locals.contains(&p.id) { - out.insert(p.id, next_slot); - next_slot += 1; + assign_slot(&mut out, &mut next_slot, p.id); } } fn walk( @@ -917,8 +944,7 @@ pub fn collect_pointer_typed_locals( && !non_pointer_locals.contains(id) && !flat_row_alias_ids.contains(id) => { - out.insert(*id, *next_slot); - *next_slot += 1; + assign_slot(out, next_slot, *id); } Stmt::If { then_branch, @@ -962,8 +988,7 @@ pub fn collect_pointer_typed_locals( // Catch parameter is implicitly bound; // treat as Any (pointer-possible). if !non_pointer_locals.contains(id) { - out.insert(*id, *next_slot); - *next_slot += 1; + assign_slot(out, next_slot, *id); } } walk( @@ -1007,6 +1032,16 @@ pub fn collect_pointer_typed_locals( &non_pointer_locals, &flat_row_alias_ids, ); + // The frame-sizing invariant every caller relies on: they pass + // `map.len()` to `enable_shadow_frame`, so the count MUST equal the + // number of indices handed out. If this ever breaks again, slots at or + // beyond the frame length are silently dropped by the runtime's bounds + // check and their locals become invisible to the moving GC (#7154). + debug_assert_eq!( + out.len() as u32, + next_slot, + "shadow-frame slot map cardinality must equal the slot counter" + ); out } diff --git a/crates/perry-codegen/tests/shadow_slot_hygiene.rs b/crates/perry-codegen/tests/shadow_slot_hygiene.rs index d60ea79c10..11a1b364ef 100644 --- a/crates/perry-codegen/tests/shadow_slot_hygiene.rs +++ b/crates/perry-codegen/tests/shadow_slot_hygiene.rs @@ -542,6 +542,55 @@ fn closure_captured_write_shadow_module() -> Module { } } +/// #7154: duplicate `var` declarations — one HIR local id, one `Stmt::Let` +/// per declaration site (the shape lowering emits for JS `var` redeclaration; +/// lodash's `runInContext` carries 170+ of them). +fn duplicate_var_decl_shadow_module() -> Module { + let mut module = shadow_hygiene_module(); + module.name = "dup_var_shadow.ts".to_string(); + module.functions = vec![Function { + id: 1, + name: "probe".to_string(), + type_params: Vec::new(), + params: Vec::new(), + return_type: Type::Any, + body: vec![ + Stmt::Let { + id: 1, + name: "dup".to_string(), + ty: Type::Any, + mutable: true, + init: Some(Expr::MapNew), + }, + // Same id, second declaration site. + Stmt::Let { + id: 1, + name: "dup".to_string(), + ty: Type::Any, + mutable: true, + init: Some(Expr::MapNew), + }, + Stmt::Let { + id: 2, + name: "later".to_string(), + ty: Type::Any, + mutable: false, + init: Some(Expr::Array(Vec::new())), + }, + Stmt::Return(Some(Expr::LocalGet(2))), + ], + is_async: false, + is_generator: false, + is_strict: false, + is_exported: false, + captures: Vec::new(), + decorators: Vec::new(), + was_plain_async: false, + was_unrolled: false, + }]; + module +} + fn function_slice<'a>(ir: &'a str, name: &str) -> &'a str { let define_marker = format!("@{}(", name); let define_start = ir @@ -608,6 +657,54 @@ fn function_shadow_slots_clear_dead_values_and_skip_numeric_roots() { ); } +/// #7154 regression: every emitted shadow-slot index must be inside the +/// pushed frame. Duplicate `var` declarations used to burn a slot index per +/// `Stmt::Let` while the frame was sized by map *cardinality*, so trailing +/// locals landed at indices `>= slot_count` — the runtime bounds check then +/// dropped their root stores SILENTLY and the moving minor never rewrote +/// them (the "value is not a function" mutator reinjection). +/// +/// The inline #7088 store keeps a `js_shadow_slot_bind` call on its +/// null-state fallback arm at the same index, so scanning the bind calls +/// covers the inline sites too. +#[test] +fn duplicate_var_declarations_keep_every_slot_inside_the_frame() { + let ir = String::from_utf8( + compile_module(&duplicate_var_decl_shadow_module(), empty_opts()).unwrap(), + ) + .expect("LLVM IR should be UTF-8"); + let fn_ir = function_slice(&ir, "perry_fn_dup_var_shadow_ts__probe"); + + let frame_slots: u32 = fn_ir + .split("call ptr @js_shadow_frame_enter(i32 ") + .nth(1) + .and_then(|rest| rest.split(')').next()) + .and_then(|n| n.parse().ok()) + .expect("probe must push a shadow frame"); + assert_eq!( + frame_slots, 2, + "one slot for the duplicate-decl local, one for the trailing local" + ); + + for chunk in fn_ir.split("@js_shadow_slot_bind(i32 ").skip(1) { + let idx: u32 = chunk + .split(',') + .next() + .and_then(|n| n.trim().parse().ok()) + .expect("bind index must be an integer literal"); + assert!( + idx < frame_slots, + "shadow slot index {idx} out of bounds for a {frame_slots}-slot \ + frame — the runtime bounds check drops this root silently and \ + the local is invisible to the moving GC; fn IR:\n{fn_ir}" + ); + } + assert!( + fn_ir.contains("@js_shadow_slot_bind(i32 1, ptr %"), + "trailing pointer local must be rooted at the deduped index; fn IR:\n{fn_ir}" + ); +} + #[test] fn entry_module_top_level_shadow_frame_starts_after_init_prelude() { let ir = String::from_utf8(