fix(codegen): never let a shadow-frame slot index escape the pushed frame (#7154)

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.

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<u32, u32>, 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
 }
 

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(