PyFrame drops execution_context and w_globals; the #1372 review follow-ups

majit/majit-backend-cranelift/src/compiler.rs

@@ -16103,19 +16103,23 @@ fn collect_guards(
         //     also contain Const (handled by regalloc.py:1192-1193 in the same
         //     loop). majit groups external JUMP with FINISH for fail_args
         //     bookkeeping; treat their gcmap the same way.
-        // `compute_gcmap` skips the hole a virtual leaves (`if arg is None:
-        // continue`), marks every remaining REF failarg, and narrows nothing
-        // else. No hole reaches this map: `spill_guard_fail_args` resolves
-        // every fail arg through `resolve_opref`, which refuses `OpRef::NONE`
-        // rather than substituting a zero, so a guard carrying one fails to
-        // compile before a gcmap exists. A force token is REF too
-        // (`FORCE_TOKEN/0/r` returns the jitframe, a moving GC object), so its
-        // slot is marked like any other.
+        // `compute_gcmap` opens with `if arg is None: continue`, then marks
+        // every remaining REF failarg and narrows nothing else. The skip is
+        // load-bearing here rather than vacuous: a virtual leaves a hole in
+        // fail_args, and both `infer_fail_arg_types` and
+        // `resolve_fail_arg_types` type that hole `Ref` (a virtual object is a
+        // GCREF), so the type test on its own would mark a slot that owns no
+        // root. A force token is REF too (`FORCE_TOKEN/0/r` returns the
+        // jitframe, a moving GC object), so its slot is marked like any other.
         let failarg_ref_slots = {
             let mut slots = Vec::new();
             for (i, tp) in fail_arg_types.iter().enumerate() {
                 if *tp == Type::Ref {
                     let arg_ref = fail_arg_refs.get(i).copied().unwrap_or(OpRef::NONE);
+                    // `assembler.py compute_gcmap` `if arg is None: continue`.
+                    if arg_ref.is_none() {
+                        continue;
+                    }
                     // regalloc.py:1206 — guard fail_args must never be Const.
                     // history.py/268/314 inline-Const carries the value on
                     // the OpRef itself; legacy idx-Const lives in `constants`.

majit/majit-backend/src/lib.rs

@@ -1648,6 +1648,17 @@ impl std::fmt::Debug for JitCellToken {
     }
 }
 
+impl majit_ir::QuasiImmutLoopToken for JitCellToken {
+    fn invalidate_for_quasi_immut(&self) {
+        // quasiimmut.py `QuasiImmut.invalidate`: `looptoken.invalidated = True`
+        // followed by `cpu.invalidate_loop(looptoken)`.  `invalidate` performs both
+        // projections in pyre: the root flag makes the warm cell stop
+        // returning this token, and every bridge-generation flag activates
+        // its still-unpatched GUARD_NOT_INVALIDATED sites.
+        self.invalidate();
+    }
+}
+
 // pyre is single-threaded (no-GIL → still one JIT thread in practice,
 // matching RPython's single-interpreter assumption).  `JitCellToken`
 // embeds `Rc<RdVirtualInfo>` and `Box<dyn Any + Send>` which are not

majit/majit-gc/src/collector.rs

@@ -1806,7 +1806,11 @@ impl MiniMarkGC {
             let type_id = unsafe { (*header_of(pinned_obj)).type_id() };
             let payload_size = self.size_for_typeid(pinned_obj, type_id, "pinned_barriers");
             let object_size = Self::nursery_allocation_size(GcHeader::SIZE + payload_size);
-            let next_free = pinned_header + object_size;
+            // `size_for_typeid` decodes the pinned object's extent from its
+            // header. A decode that overstates it would push free past the
+            // barrier we are about to publish, and `Nursery::alloc` would then
+            // hand out bytes beyond the gap. The barrier is the hard bound.
+            let next_free = (pinned_header + object_size).min(next_top);
             unsafe {
                 // Set the wider bound first so Nursery's pointer invariant is
                 // maintained while free crosses the old (pinned) top.
@@ -2535,10 +2539,9 @@ impl MiniMarkGC {
         self.nursery_surviving_size = 0;
         // `IncrementalMiniMarkGC._minor_collection`: pinning does not keep an
         // object alive. Rebuild the AddressStack and count from traced edges.
-        // pyre currently walks the complete root stacks on every minor, so the
-        // saved stopper decision is conservatively unused; keep the state with
-        // the collector, where upstream owns it.
-        let _any_pinned_object_from_earlier = self.any_pinned_object_kept;
+        // The flag sampled here is the previous minor's, which
+        // `collect_roots_in_nursery` turns into `use_jit_frame_stoppers`.
+        let any_pinned_object_from_earlier = self.any_pinned_object_kept;
         self.surviving_pinned_objects.clear();
         self.pinned_objects_in_nursery = 0;
         self.any_pinned_object_kept = false;
@@ -2678,9 +2681,18 @@ impl MiniMarkGC {
         // a minor collection (incminimark.py:339-344
         // `old_objects_pointing_to_young`); restored to the conservative
         // Major default right after.
-        crate::shadow_stack::set_extra_root_walk_kind(
-            crate::shadow_stack::ExtraRootWalkKind::Minor,
-        );
+        //
+        // `collect_roots_in_nursery` computes
+        // `use_jit_frame_stoppers = not any_pinned_object_from_earlier` and
+        // passes it as `is_minor`: a pinned object created before the previous
+        // minor is still in the nursery and was never promoted, so the skip
+        // would drop the only edge reaching it. Announce a full walk instead.
+        let extra_root_walk_kind = if any_pinned_object_from_earlier {
+            crate::shadow_stack::ExtraRootWalkKind::Major
+        } else {
+            crate::shadow_stack::ExtraRootWalkKind::Minor
+        };
+        crate::shadow_stack::set_extra_root_walk_kind(extra_root_walk_kind);
         let mut visit_extra_area = |gcref: &mut GcRef| {
             self.drag_out_root(gcref);
         };
@@ -11707,6 +11719,56 @@ cache size\t: 8192 kB\n";
         gc.roots.clear();
     }
 
+    /// `collect_roots_in_nursery` computes
+    /// `use_jit_frame_stoppers = not any_pinned_object_from_earlier` and passes
+    /// it to `walk_roots` as `is_minor`. A pin that survived the previous minor
+    /// was never promoted and still sits at its nursery address, so the walkers
+    /// that skip a clean area on a minor have to be told to walk everything.
+    #[test]
+    fn a_surviving_pin_makes_the_next_minor_announce_a_full_extra_root_walk() {
+        // The walker registry has no removal and every test binary thread
+        // shares it, so record on the collecting thread only.
+        thread_local! {
+            static SEEN: std::cell::RefCell<Vec<crate::shadow_stack::ExtraRootWalkKind>> =
+                const { std::cell::RefCell::new(Vec::new()) };
+        }
+        fn record(_visit: &mut dyn FnMut(&mut GcRef)) {
+            SEEN.with(|seen| {
+                seen.borrow_mut()
+                    .push(crate::shadow_stack::extra_root_walk_kind())
+            });
+        }
+
+        let _guard = SHADOW_STACK_TEST_LOCK.lock().unwrap();
+        crate::shadow_stack::clear();
+        crate::shadow_stack::register_extra_root_walker(record);
+        SEEN.with(|seen| seen.borrow_mut().clear());
+
+        let mut gc = test_gc(4096);
+        let tid = gc.register_type(TypeInfo::simple(16));
+        let mut obj = gc.alloc_with_type(tid, 16);
+        assert!(gc.pin(obj));
+        unsafe { gc.roots.add(&mut obj) };
+
+        // Nothing was pinned before this one, so the first minor still
+        // announces the skip; it discovers the pin and leaves it in place.
+        gc.do_collect_nursery();
+        assert!(gc.is_pinned(obj));
+
+        // The pin now predates the previous minor: it is "from earlier".
+        gc.do_collect_nursery();
+        assert!(gc.is_pinned(obj));
+
+        gc.roots.clear();
+        assert_eq!(
+            SEEN.with(|seen| seen.borrow().clone()),
+            vec![
+                crate::shadow_stack::ExtraRootWalkKind::Minor,
+                crate::shadow_stack::ExtraRootWalkKind::Major,
+            ]
+        );
+    }
+
     /// The sibling above only ever discovers the parent *after* the list is
     /// swapped out. Phase 1c traces an old-generation jitframe directly, with
     /// itself as the holder, and that runs earlier — so a parent found there

majit/majit-gc/src/nursery.rs

@@ -149,10 +149,17 @@ impl Nursery {
         debug_assert!(start >= self.start as usize);
         debug_assert!(start <= end);
         debug_assert!(end <= self.start as usize + self.size);
-        let len = end - start;
-        if len == 0 {
+        // This is a safe fn that writes raw bytes at caller-supplied
+        // addresses, so the bounds have to hold in release too: an
+        // out-of-range end would write outside the arena, and `start > end`
+        // would wrap the length into a near-`usize::MAX` fill. Intersect with
+        // the arena instead of trusting the caller.
+        let lo = start.max(self.start as usize);
+        let hi = end.min(self.start as usize + self.size);
+        if lo >= hi {
             return;
         }
+        let (start, len) = (lo, hi - lo);
         #[cfg(target_arch = "wasm32")]
         unsafe {
             ptr::write_bytes(start as *mut u8, 0, len);

majit/majit-ir/src/descr.rs

@@ -3684,6 +3684,20 @@ pub trait Descr: Send + Sync + std::fmt::Debug {
     }
 }
 
+/// `history.py JitCellToken` as seen by `quasiimmut.py QuasiImmut`.
+///
+/// A quasi-immutable dependency belongs to the owning loop token, never to
+/// one machine-code fragment.  `QuasiImmut.invalidate` marks that token
+/// invalid and asks the CPU to activate every still-unpatched
+/// `GUARD_NOT_INVALIDATED` in the loop and its bridges.  Keeping this as a
+/// trait avoids making the interpreter object model depend on a backend
+/// implementation while preserving the upstream ownership shape.
+pub trait QuasiImmutLoopToken: Send + Sync + std::fmt::Debug {
+    /// `quasiimmut.py QuasiImmut.invalidate` —
+    /// `looptoken.invalidated = True; cpu.invalidate_loop(looptoken)`.
+    fn invalidate_for_quasi_immut(&self);
+}
+
 /// `quasiimmut.py QuasiImmut` seen from the JIT side — one object
 /// gathering the loops that folded a single quasi-immutable field.
 ///
@@ -3698,7 +3712,7 @@ pub trait QuasiImmutHandle: Send + Sync + std::fmt::Debug {
 
     /// `quasiimmut.py register_loop_token`, reached from
     /// `compile.py:204-207`.
-    fn register_loop_token(&self, flag: &std::sync::Arc<std::sync::atomic::AtomicBool>);
+    fn register_loop_token(&self, token: &std::sync::Arc<dyn QuasiImmutLoopToken>);
 
     /// Identity of the instance behind the handle.
     ///

majit/majit-ir/src/lib.rs

@@ -30,13 +30,13 @@ pub mod value;
 pub use descr::{
     AccumInfo, ArrayDescr, ArrayFlag, CallDescr, DebugMergePointDescr, DebugMergePointInfo, Descr,
     DescrRef, FailDescr, FailDescrCell, FieldDescr, GcCache, InteriorFieldDescr, JitCodeDescr,
-    LLType, LoopTargetDescr, LoopTokenDescr, QuasiImmutDescr, QuasiImmutHandle, SimpleCallDescr,
-    SimpleFailDescr, SimpleFieldDescr, SizeDescr, SwitchDescr, TargetArgLoc, UnpackAtExitInfo,
-    descr_identity, make_array_descr, make_array_descr_signed, make_call_descr, make_field_descr,
-    make_field_descr_full, make_loop_target_descr, make_malloc_array_calldescr,
-    make_malloc_array_nonstandard_calldescr, make_malloc_big_fixedsize_calldescr,
-    make_malloc_str_calldescr, make_malloc_unicode_calldescr, make_memcpy_calldescr,
-    make_size_descr_full, make_size_descr_with_vtable, make_tid_field_descr,
+    LLType, LoopTargetDescr, LoopTokenDescr, QuasiImmutDescr, QuasiImmutHandle,
+    QuasiImmutLoopToken, SimpleCallDescr, SimpleFailDescr, SimpleFieldDescr, SizeDescr,
+    SwitchDescr, TargetArgLoc, UnpackAtExitInfo, descr_identity, make_array_descr,
+    make_array_descr_signed, make_call_descr, make_field_descr, make_field_descr_full,
+    make_loop_target_descr, make_malloc_array_calldescr, make_malloc_array_nonstandard_calldescr,
+    make_malloc_big_fixedsize_calldescr, make_malloc_str_calldescr, make_malloc_unicode_calldescr,
+    make_memcpy_calldescr, make_size_descr_full, make_size_descr_with_vtable, make_tid_field_descr,
     make_vtable_field_descr, memcpy_fn_addr, recover_fail_descr_cell, unpack_fielddescr,
 };
 pub use effectinfo::{

majit/majit-metainterp/src/compile.rs

@@ -1658,9 +1658,10 @@ pub(crate) fn infer_terminal_exit_layout<T: AsRef<majit_ir::Op>>(
         .iter()
         .map(|opref| {
             // `OpRef::NONE` represents a null-ref placeholder per
-            // `fail_arg_type`; preserve `Type::Ref` so the gcmap and
-            // `decode_values_with_layout` see the same null-Ref typing the
-            // rest of the resume path uses.
+            // `fail_arg_type`; preserve `Type::Ref` so every consumer of this
+            // layout sees the same null-Ref typing the rest of the resume
+            // path uses. The gcmap is not one of them: `compute_gcmap` drops
+            // a `None` failarg before it reads the type at all.
             if opref.is_none() {
                 return Type::Ref;
             }
@@ -1724,49 +1725,6 @@ pub(crate) fn build_terminal_exit_layouts<T: AsRef<majit_ir::Op>>(
     layouts
 }
 
-#[allow(dead_code)]
-pub(crate) fn terminal_exit_layout_for_trace(
-    trace: &CompiledTrace,
-    owning_key: u64,
-    trace_id: u64,
-    op_index: usize,
-) -> Option<CompiledExitLayout> {
-    if let Some(layout) = trace.terminal_exit_layouts.get(&op_index) {
-        return Some(layout.public(
-            owning_key,
-            trace_id,
-            find_fail_index_for_exit_op(&trace.ops, op_index).unwrap_or(u32::MAX),
-        ));
-    }
-    if let Some(fail_index) = find_fail_index_for_exit_op(&trace.ops, op_index)
-        && let Some(layout) = trace.exit_layouts.get(&fail_index)
-    {
-        return Some(layout.public(owning_key, trace_id, fail_index));
-    }
-    infer_terminal_exit_layout(&trace.inputargs, &trace.ops, owning_key, trace_id, op_index)
-}
-
-#[allow(dead_code)]
-pub(crate) fn decode_values_with_layout(
-    raw_values: &[i64],
-    layout: &CompiledExitLayout,
-) -> Vec<Value> {
-    layout
-        .exit_types
-        .iter()
-        .enumerate()
-        .map(|(index, tp)| {
-            let raw = raw_values.get(index).copied().unwrap_or(0);
-            match tp {
-                Type::Int => Value::Int(raw),
-                Type::Ref => Value::Ref(GcRef(raw as usize)),
-                Type::Float => Value::Float(f64::from_bits(raw as u64)),
-                Type::Void => Value::Void,
-            }
-        })
-        .collect()
-}
-
 pub(crate) fn normalize_closing_jump_args(
     ops: Vec<majit_ir::OpRc>,
     constants: &majit_ir::ConstMap<majit_ir::Value>,

majit/majit-metainterp/src/jitcode/assembler.rs

@@ -6550,8 +6550,6 @@ mod tests {
     /// — the offset is only a stand-in for the mint sites that carry no name.
     #[test]
     fn a_named_field_resolves_by_name_through_an_ambiguous_offset() {
-        #[allow(dead_code)]
-        const TID: u64 = 0x4E41_4D45_4B59;
         let fields = [
             (0, false, "head", 8, false),
             (8, false, "agg", 8, true),