From 4cf01f16698d59466ecedc5ab408efb2c738209e Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Thu, 30 Jul 2026 19:33:46 +0900 Subject: [PATCH 1/6] jit: root the forced-virtual caches and store them inside handle_async_forcing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review findings on `MetaInterp::forced_virtuals`, the stand-in for the `AllVirtuals` upstream hides in the deadframe's `jf_savedata` word. Rooting. The ptr half holds what `force_all_virtuals` (resume.py:969-981) materialized until the following GUARD_NOT_FORCED consumes it, and it was in no root set: a major collection inside that window frees any old-generation object without `flags::VISITED` (`OldGen::sweep_arenas_step`), and a virtual named only by a resume frame's ref registers has this Vec as its only referent. Upstream gets the edge from `jf_savedata` being traced as a GCREF field (`majit-backend/src/jitframe.rs:354`; the previous citation, :278, is `JitFrame::init`). Enroll it as the fifth `register_mutator_extra_area` member alongside rd_consts / partial_trace / active_trace / compile_snapshot, walking only the ptr half — the int half is unboxed field values. Register an ephemeron pruner, like `mapdict::prune_dead_owner_entries`, so an entry the guard never consumes goes when its owner frame is swept instead of pinning its objects and leaving a key a recycled PyFrame address could match. `EphemeronPrunerFn` carries no data pointer, so the pruner reaches only the collecting thread's `JIT_DRIVER`. Store placement. `compile.py:996-1000` calls `set_savedata_ref` inside `handle_async_forcing`; pyre did it at the `force_pyframe` hook, so `force_pyframe_vref` — the materializing arm of `virtualref.py:134 force_virtual_if_necessary`, which calls `force_virtualizable_token` as a statement — dropped the cache. `force_from_resumedata` now also returns the virtualizable its vable section named (resume.py:1404) and `handle_async_forcing` keys the store on it, so both entry points are covered and the key comes from the resume data rather than the caller. `force_virtualizable_token` returns nothing, like `force_now`. Adds a `[jit][take_forced_virtuals] hit/miss` counterpart to the existing `handle_async_forcing` log, and two tests for the `all_virtuals = Some(..)` resume: `consume_vref_and_vable` jumps the vable and vref sections (resume.py:1433-1435), and `_prepare_virtuals` zeroes a preloaded cache (resume.py:990-991), which is why the caller passes rd_virtuals as None. Assisted-by: Claude --- majit/majit-metainterp/src/jitdriver.rs | 16 ++- majit/majit-metainterp/src/pyjitpl.rs | 154 ++++++++++++++++++------ majit/majit-metainterp/src/resume.rs | 120 +++++++++++++++++- pyre/pyre-jit/src/eval.rs | 57 +++++++-- 4 files changed, 296 insertions(+), 51 deletions(-) diff --git a/majit/majit-metainterp/src/jitdriver.rs b/majit/majit-metainterp/src/jitdriver.rs index 2d602885f69..9622a8fdd3a 100644 --- a/majit/majit-metainterp/src/jitdriver.rs +++ b/majit/majit-metainterp/src/jitdriver.rs @@ -4590,14 +4590,14 @@ impl JitDriver { /// virtuals through the same resume allocator used by ordinary guard /// failure. In particular, a `jit.virtual_ref` frame must not be decoded /// through `NullAllocator`, or its `forced` writeback remains null. - pub fn force_virtualizable_token(&mut self, token: u64) -> Option<(Vec, Vec)> { + pub fn force_virtualizable_token(&mut self, token: u64) { let fallback_alloc = crate::resume::NullAllocator; let allocator: &dyn crate::resume::BlackholeAllocator = self .blackhole_allocator .as_deref() .unwrap_or(&fallback_alloc); self.meta - .force_virtualizable_token_with_allocator(token, allocator) + .force_virtualizable_token_with_allocator(token, allocator); } fn prepare_exit_resume_heap_with_blackhole_allocator( @@ -4643,6 +4643,18 @@ impl JitDriver { self.meta.walk_compile_snapshot_refs(visitor); } + /// GC walker for the forced-virtual caches awaiting a `GUARD_NOT_FORCED`. + /// See `MetaInterp::walk_forced_virtuals_refs`. + pub fn walk_forced_virtuals_refs(&mut self, visitor: impl FnMut(&mut majit_ir::GcRef)) { + self.meta.walk_forced_virtuals_refs(visitor); + } + + /// Drop forced-virtual caches whose owner frame died. + /// See `MetaInterp::prune_forced_virtuals`. + pub fn prune_forced_virtuals(&mut self, classify: &mut dyn FnMut(usize) -> Option) { + self.meta.prune_forced_virtuals(classify); + } + pub fn run_compiled_detailed_keyed( &mut self, green_key: u64, diff --git a/majit/majit-metainterp/src/pyjitpl.rs b/majit/majit-metainterp/src/pyjitpl.rs index 0e85a4cb032..7e74c2e0020 100644 --- a/majit/majit-metainterp/src/pyjitpl.rs +++ b/majit/majit-metainterp/src/pyjitpl.rs @@ -1162,11 +1162,23 @@ pub struct MetaInterp { /// /// Upstream hides an `AllVirtuals` instance in the deadframe's /// `jf_savedata` GCREF slot and `ResumeGuardForcedDescr.handle_fail` - /// fishes it back out. pyre cannot put a Rust value there — the GC - /// traces `jf_savedata` as a real object reference - /// (`jitframe.rs:278`) — so the cache is held here, keyed by the - /// virtualizable that was forced. One entry per frame, overwritten on - /// re-force, exactly like the single `jf_savedata` word. + /// fishes it back out. pyre cannot put a Rust value in that slot — the GC + /// traces it as a real object reference (`majit-backend/src/jitframe.rs:354`) + /// — so the cache is held here, keyed by the virtualizable that was forced. + /// One entry per frame, overwritten on re-force, exactly like the single + /// `jf_savedata` word. + /// + /// The ptr half is a GC root for as long as the entry lives, walked by + /// [`Self::walk_forced_virtuals_refs`]: `force_all_virtuals` + /// (`resume.py:969-981`) materializes every `rd_virtuals` entry, including + /// ones named only by a resume frame's ref registers, and those are written + /// nowhere else at force time — this `Vec` is their only referent until the + /// `GUARD_NOT_FORCED` failure consumes it. Upstream gets that edge from + /// `jf_savedata` being traced; here it comes from the root walker. + /// + /// An entry the guard never consumes is dropped by + /// [`Self::prune_forced_virtuals`] when its owner frame dies, which is what + /// `jf_savedata` gets for free by living on the deadframe. pub(crate) forced_virtuals: Vec<(u64, Vec, Vec)>, /// Virtualizable array lengths for trace-entry box layout. pub(crate) vable_array_lengths: Vec, @@ -1854,6 +1866,48 @@ impl MetaInterp { } } + /// GC walker for the forced-virtual caches held in + /// [`Self::forced_virtuals`], standing in for the trace `jf_savedata` gets + /// as a real GCREF field (`majit-backend/src/jitframe.rs:354`). + /// + /// Only the ptr half is walked. The int half is `virtuals_int_cache` — + /// unboxed integer field values — and handing those to the visitor would + /// test integers as heap addresses. `prepare_resume_heap_with_roots` roots + /// the same one half for the same reason. + /// + /// Unmaterialized `0` cache slots pass through unchanged, as they do in + /// `shadow_stack::walk_resume_ref_roots`. + pub fn walk_forced_virtuals_refs(&mut self, mut visitor: impl FnMut(&mut GcRef)) { + for (_owner, ptrs, _ints) in self.forced_virtuals.iter_mut() { + for slot in ptrs.iter_mut() { + // SAFETY: `GcRef` is a pointer-sized newtype over the same + // representation these slots hold, the same reinterpret + // `walk_resume_ref_roots` performs on `virtuals_ptr_cache` + // (`majit-gc/src/shadow_stack.rs:931`). Walked unconditionally + // for both collection kinds: the minor forwards a + // nursery-resident virtual in place, the major seeds it as a + // mark root so the sweep does not free it. + let gcref = unsafe { &mut *(slot as *mut i64 as *mut GcRef) }; + visitor(gcref); + } + } + } + + /// Drop the forced-virtual caches whose owner frame died. + /// + /// `classify` answers with the owner's current address, or `None` if it did + /// not survive. A major collection moves nothing, so a surviving owner + /// answers with the key it was asked about. + /// + /// Without this, rooting the ptr half would keep an unconsumed entry's + /// objects alive forever and leave a stale `PyFrame` key that a later frame + /// at the same address could fish. Upstream is immune because `jf_savedata` + /// dies with its deadframe; this reproduces that lifetime. + pub fn prune_forced_virtuals(&mut self, classify: &mut dyn FnMut(usize) -> Option) { + self.forced_virtuals + .retain(|(owner, _, _)| classify(*owner as usize) == Some(*owner as usize)); + } + #[inline] fn prepare_compiled_run_io() { io_buffer::io_buffer_discard(); @@ -11607,9 +11661,10 @@ impl MetaInterp { /// RPython flow: force_now() → cpu.force(token) → handle_async_forcing() /// → force_from_resumedata() → materialize all virtuals → save on deadframe. /// - /// Returns the forced virtual caches (ptr, int) for later blackhole - /// resumption from the GUARD_NOT_FORCED. RPython stores these as - /// AllVirtuals via cpu.set_savedata_ref(). + /// The forced virtual caches (ptr, int) are stored on + /// [`Self::forced_virtuals`] for the blackhole resumption from the + /// GUARD_NOT_FORCED — RPython's `AllVirtuals` via `cpu.set_savedata_ref()`. + /// They are also returned, which only the unit tests below read. pub fn handle_async_forcing( &mut self, green_key: u64, @@ -11696,24 +11751,22 @@ impl MetaInterp { // compile.py:990-991: vinfo = self.jitdriver_sd.virtualizable_info let vinfo = self.virtualizable_info(); let all_liveness = self.staticdata.liveness_info.as_slice(); - let (all_virtuals_ptr, all_virtuals_int) = crate::resume::force_from_resumedata( - &self.staticdata.profiler, - rd_numb, - rd_consts, - all_liveness, - fail_values, - deadframe_types.as_deref(), - rd_virtuals.as_deref(), - storage.map(|s| s.rd_pendingfields.as_slice()), - Some(&self.staticdata.virtualref_info as &dyn crate::resume::VRefInfo), - vinfo.map(|v| v.as_ref() as &dyn crate::resume::VirtualizableInfo), - None, // ginfo — pyre has no greenfield mechanism - allocator, - ); + let (all_virtuals_ptr, all_virtuals_int, virtualizable_ptr) = + crate::resume::force_from_resumedata( + &self.staticdata.profiler, + rd_numb, + rd_consts, + all_liveness, + fail_values, + deadframe_types.as_deref(), + rd_virtuals.as_deref(), + storage.map(|s| s.rd_pendingfields.as_slice()), + Some(&self.staticdata.virtualref_info as &dyn crate::resume::VRefInfo), + vinfo.map(|v| v.as_ref() as &dyn crate::resume::VirtualizableInfo), + None, // ginfo — pyre has no greenfield mechanism + allocator, + ); drop(_cc_guard); - // compile.py:999-1000: obj = AllVirtuals(all_virtuals) - // metainterp_sd.cpu.set_savedata_ref(deadframe, obj.hide()) - // Return the virtual caches so the caller can store them. if crate::majit_log_enabled() { eprintln!( "[jit][handle_async_forcing] forced {} ptr + {} int virtuals", @@ -11721,6 +11774,20 @@ impl MetaInterp { all_virtuals_int.len(), ); } + // compile.py:999-1000: obj = AllVirtuals(all_virtuals) + // metainterp_sd.cpu.set_savedata_ref(deadframe, obj.hide()) + // + // The store lives here, inside `handle_async_forcing`, exactly as + // upstream — every force entry point reaching this function is covered + // by it, including `force_virtual_if_necessary`'s (virtualref.py:134) + // which never sees the returned caches. + if virtualizable_ptr != 0 { + self.save_forced_virtuals( + virtualizable_ptr as u64, + all_virtuals_ptr.clone(), + all_virtuals_int.clone(), + ); + } Some((all_virtuals_ptr, all_virtuals_int)) } @@ -11732,11 +11799,14 @@ impl MetaInterp { /// `NullAllocator` convenience wrapper here — callers go through /// `JitDriver::force_virtualizable_token`, which supplies the registered /// blackhole allocator. + /// + /// Like `force_now`, this returns nothing: `handle_async_forcing` attaches + /// the materialized virtuals itself (compile.py:999-1000). pub fn force_virtualizable_token_with_allocator( &mut self, token: u64, allocator: &dyn crate::resume::BlackholeAllocator, - ) -> Option<(Vec, Vec)> { + ) { let deadframe = self .backend .force(GcRef(token as usize)) @@ -11761,25 +11831,25 @@ impl MetaInterp { Type::Void => 0, }) .collect::>(); - // compile.py:995-1000: the forced cache is returned so the caller can - // attach it to the frame this force belongs to. + // compile.py:995: faildescr.handle_async_forcing(deadframe) self.handle_async_forcing_with_allocator( green_key, trace_id, fail_index, &fail_values, allocator, - ) + ); } /// `compile.py:1000 cpu.set_savedata_ref(deadframe, obj.hide())`. /// - /// `owner` is the forced virtualizable. Upstream can key on the deadframe - /// because `handle_fail` receives it; pyre's guard-failure path surfaces - /// the frame rather than the jitframe, and the two ends agree on the - /// frame: the force runs against a named virtualizable and the - /// GUARD_NOT_FORCED that follows deopts that same frame's loop. - pub fn save_forced_virtuals(&mut self, owner: u64, ptrs: Vec, ints: Vec) { + /// `owner` is the forced virtualizable, as the guard's own resume data + /// named it (`resume.py:1404`). Upstream can key on the deadframe because + /// `handle_fail` receives it; pyre's guard-failure path surfaces the frame + /// rather than the jitframe, and the two ends agree on the frame: the force + /// runs against a named virtualizable and the GUARD_NOT_FORCED that follows + /// deopts that same frame's loop. + fn save_forced_virtuals(&mut self, owner: u64, ptrs: Vec, ints: Vec) { match self.forced_virtuals.iter_mut().find(|e| e.0 == owner) { // A second force of the same frame overwrites, the way a second // `set_savedata_ref` overwrites the one `jf_savedata` word. @@ -11795,8 +11865,18 @@ impl MetaInterp { /// second set (`resume.py:1373-1374`, and the `vable_size` skip in /// `consume_vref_and_vable`). pub fn take_forced_virtuals(&mut self, owner: u64) -> Option<(Vec, Vec)> { - let index = self.forced_virtuals.iter().position(|e| e.0 == owner)?; - let (_, ptrs, ints) = self.forced_virtuals.swap_remove(index); + let index = self.forced_virtuals.iter().position(|e| e.0 == owner); + // Only a GUARD_NOT_FORCED reaches here (`is_guard_forced()` gates the + // callers), so hit/miss is the force→resume handoff itself: the + // counterpart of the `handle_async_forcing` line above. + if crate::majit_log_enabled() { + eprintln!( + "[jit][take_forced_virtuals] owner=0x{:x} {}", + owner, + if index.is_some() { "hit" } else { "miss" }, + ); + } + let (_, ptrs, ints) = self.forced_virtuals.swap_remove(index?); Some((ptrs, ints)) } diff --git a/majit/majit-metainterp/src/resume.rs b/majit/majit-metainterp/src/resume.rs index 69bad7d94ac..9e382f2abda 100644 --- a/majit/majit-metainterp/src/resume.rs +++ b/majit/majit-metainterp/src/resume.rs @@ -5232,6 +5232,112 @@ mod tests { reader.consume_vref_and_vable(None, Some(&TestVirtualizableInfo), None, None); } + /// resume.py:1368-1375 / compile.py:956-963 — the GUARD_NOT_FORCED resume + /// path. `handle_async_forcing` already materialized the virtuals and + /// already rewrote the virtualizable, so this resume must reuse that cache + /// and leave the virtualizable alone: `consume_vref_and_vable` jumps the + /// vable and vref sections (resume.py:1433-1435) instead of consuming them. + #[test] + fn blackhole_from_resumedata_with_all_virtuals_skips_the_vable_section() { + use crate::blackhole::BlackholeInterpBuilder; + use crate::jitcode::JitCodeBuilder; + use crate::jitcode::insns::{BC_ABORT, BC_CATCH_EXCEPTION, BC_LIVE, BC_RVMPROF_CODE}; + + let mut writer = crate::resumecode::Writer::new(8); + writer.append_int(0); // items_resume_section (patched below) + writer.append_int(1); // count + writer.append_int(1); // vable_size: the identity only (get_total_size == 0) + writer.append_int(tag(0, TAGBOX).unwrap() as i64); // virtualizable identity + writer.append_int(0); // vref_array length + writer.append_int(0); // jitcode_pos + writer.append_int(0); // pc + writer.append_int(0); // py_pc + writer.patch_current_size(0); + let rd_numb = writer.create_numbering(); + + let mut runtime = JitCodeBuilder::default().finish(); + runtime.body_mut().code = vec![BC_LIVE, 0, 0, BC_ABORT]; + runtime.body_mut().c_num_regs_i = 1; + runtime.body_mut().constants_i = vec![321]; + runtime.body_mut().startpoints = Some([0_usize, 3].into_iter().collect()); + let runtime = std::sync::Arc::new(runtime); + let all_liveness: Vec = vec![0, 0, 0]; + let deadframe = [0x4000_i64]; + let deadframe_types = [majit_ir::Type::Ref]; + + let resume = |all_virtuals: Option<(Vec, Vec)>| { + let mut builder = BlackholeInterpBuilder::new(); + builder.setup_cached_control_opcodes( + BC_LIVE as i32, + BC_CATCH_EXCEPTION as i32, + BC_RVMPROF_CODE as i32, + ); + let resolve_jitcode = |_jitcode_pos: i32, _pc: i32| -> Option { + Some(ResolvedJitCode::new(runtime.clone(), 0)) + }; + blackhole_from_resumedata( + &mut builder, + &resolve_jitcode, + &rd_numb, + &[], + &all_liveness, + &deadframe, + Some(&deadframe_types), + None, // rd_virtuals + None, // rd_guard_pendingfields + None, + Some(&TestVirtualizableInfo), + None, + None, + all_virtuals, + &NullAllocator, + ) + .expect("resume should produce a blackhole") + }; + + // resume.py:1427-1428: the ordinary path consumes the vable section, so + // the reader surfaces the virtualizable the identity item named. + let (bh, virtualizable_ptr) = resume(None); + assert_eq!(virtualizable_ptr, 0x4000); + assert_eq!(bh.position, 0); + + // resume.py:1433-1435: with a GUARD_NOT_FORCED cache the same items are + // jumped — no virtualizable is surfaced, and the frame section behind + // them still decodes, which is what proves the jump lengths line up. + let (bh, virtualizable_ptr) = resume(Some((vec![0x1234], vec![7]))); + assert_eq!(virtualizable_ptr, 0); + assert_eq!(bh.position, 0); + } + + /// resume.py:990-991 `_prepare_virtuals` resets `virtuals_cache` to zeros. + /// That is why `blackhole_from_resumedata` must not run `_prepare` on the + /// GUARD_NOT_FORCED path (resume.py:1368-1375): there the preloaded cache is + /// the resume's only source of virtuals, and `rd_virtuals` stays None. + #[test] + fn prepare_virtuals_resets_a_preloaded_guard_not_forced_cache() { + let rd = majit_ir::RdVirtualInfo::VRawSliceInfo { + offset: 0, + fieldnums: vec![], + }; + let virtuals = [rd_virtual_to_virtual_info(&rd, &[], 0, 1)]; + let mut reader = ResumeDataDirectReader::new( + &[0, 0], + &[], + &[], + &[], + None, + Some((vec![0x1234], vec![7])), + &NullAllocator, + ); + assert_eq!(reader.resume_after_guard_not_forced, 2); + assert_eq!(reader.virtuals_cache.get_ptr(0), 0x1234); + assert_eq!(reader.virtuals_cache.get_int(0), 7); + + reader.prepare(Some(&virtuals), None); + assert_eq!(reader.virtuals_cache.get_ptr(0), 0); + assert_eq!(reader.virtuals_cache.get_int(0), 0); + } + #[test] #[should_panic(expected = "load_next_value_of_type: unexpected type Void")] fn test_next_value_of_type_rejects_void() { @@ -7512,7 +7618,10 @@ pub fn blackhole_from_resumedata<'a>( /// /// Force all virtuals from resume data without running a blackhole. /// Used for GUARD_NOT_FORCED handling. -/// Returns (virtuals_cache_ptr, virtuals_cache_int) — RPython VirtualCache parity. +/// +/// Returns (virtuals_cache_ptr, virtuals_cache_int) — RPython VirtualCache +/// parity — plus the virtualizable the vable section named, which the caller +/// needs as the cache key (see `MetaInterp::save_forced_virtuals`). pub fn force_from_resumedata<'a>( profiler: &crate::jitprof::JitProfiler, rd_numb: &'a [u8], @@ -7526,7 +7635,7 @@ pub fn force_from_resumedata<'a>( vinfo: Option<&dyn VirtualizableInfo>, ginfo: Option<&dyn GreenfieldInfo>, allocator: &'a dyn BlackholeAllocator, -) -> (Vec, Vec) { +) -> (Vec, Vec, i64) { // resume.py:1346 profiler.count(crate::pyjitpl::counters::FORCE_VIRTUALIZABLES, 1); // resume.py:1347-1348 @@ -7545,7 +7654,12 @@ pub fn force_from_resumedata<'a>( resumereader.handling_async_forcing(); // resume.py:1350 resumereader.consume_vref_and_vable(vrefinfo, vinfo, ginfo, None); + // resume.py:1404 the virtualizable the vable section just named. Read it + // before `force_all_virtuals` allocates: unlike `blackhole_from_resumedata`, + // the bare `prepare` above opened no resume-root scope, so there is nothing + // to forward the reader's slot in place. + let virtualizable_ptr = resumereader.virtualizable_ptr; // resume.py:1351: return resumereader.force_all_virtuals() let (ptrs, ints) = resumereader.force_all_virtuals(); - (ptrs.to_vec(), ints.to_vec()) + (ptrs.to_vec(), ints.to_vec(), virtualizable_ptr) } diff --git a/pyre/pyre-jit/src/eval.rs b/pyre/pyre-jit/src/eval.rs index 9a072739718..c0c659f5c83 100644 --- a/pyre/pyre-jit/src/eval.rs +++ b/pyre/pyre-jit/src/eval.rs @@ -3522,6 +3522,10 @@ fn install_gc_root_walkers() { majit_gc::shadow_stack::register_ephemeron_pruner( pyre_interpreter::objspace::std::mapdict::prune_dead_owner_entries, ); + // `MetaInterp::forced_virtuals` is the same shape: keyed by the forced + // frame's address and rooting its values, so an entry the following + // `GUARD_NOT_FORCED` never consumes has to go when its owner is swept. + majit_gc::shadow_stack::register_ephemeron_pruner(prune_forced_virtuals_for_dead_frames); } fn register_thread_root_areas() { @@ -3580,6 +3584,7 @@ fn register_thread_root_areas() { register(partial_trace_root_walker_area, jit_driver); register(active_trace_root_walker_area, jit_driver); register(compile_snapshot_root_walker_area, jit_driver); + register(forced_virtuals_root_walker_area, jit_driver); } } @@ -3975,15 +3980,11 @@ unsafe extern "C" fn force_pyframe(frame: *mut pyre_interpreter::PyFrame) { // Decoding it through `NullAllocator` instead wrote a null over // that slot, and `fast2locals` renders a null slot as an absent // name — a live local vanished from `f_locals`. - let all_virtuals = driver.force_virtualizable_token(token); - // compile.py:999-1000 set_savedata_ref: hold what was - // materialized for the GUARD_NOT_FORCED that follows, keyed by - // the frame this force ran against. - if let Some((ptrs, ints)) = all_virtuals { - driver - .meta_interp_mut() - .save_forced_virtuals(ptr as u64, ptrs, ints); - } + // + // `handle_async_forcing` keeps what it materialized for the + // GUARD_NOT_FORCED that follows (compile.py:999-1000), so there + // is nothing to attach here. + driver.force_virtualizable_token(token); }); }; // Force the traced frame only when the frame handed to Python belongs @@ -4080,6 +4081,44 @@ unsafe fn compile_snapshot_root_walker_area( } } +/// GC walker for the virtual caches `handle_async_forcing` produced and left +/// for the `GUARD_NOT_FORCED` that follows. Upstream traces them through the +/// deadframe's `jf_savedata` GCREF field; pyre holds them on `MetaInterp` and +/// needs the edge drawn explicitly. +/// See `MetaInterp::walk_forced_virtuals_refs`. +unsafe fn forced_virtuals_root_walker_area( + data: *const (), + visitor: &mut dyn FnMut(&mut majit_ir::GcRef), +) { + if let Some(pair) = unsafe { jit_driver_pair_from_root_area(data) } { + pair.0.walk_forced_virtuals_refs(visitor); + } +} + +/// Drop forced-virtual caches whose owner frame the major collection is about +/// to sweep — the ephemeron half of rooting them at all. +/// +/// The force runs inside a residual `CALL_MAY_FORCE`, and two paths leave the +/// entry unconsumed: an escaped virtualizable raises instead of failing a +/// guard, and `handle_fail`'s bridge-compiled arm returns without resuming. +/// Both would otherwise pin the materialized virtuals for the process lifetime +/// and leave a key a recycled `PyFrame` address could match. +/// +/// Reads the `JIT_DRIVER` cell directly rather than through `driver_pair()`: +/// that initializes the GC subsystem and can allocate, which is not allowed +/// mid-collection. +fn prune_forced_virtuals_for_dead_frames(classify: &mut dyn FnMut(usize) -> Option) { + JIT_DRIVER.with(|cell| { + let data = cell as *const _ as *const (); + // SAFETY: same re-derivation and same aliasing caveat as the root + // walkers above (`jit_driver_pair_from_root_area`). The pruner runs on + // the collecting thread, so it reaches only that thread's driver. + if let Some(pair) = unsafe { jit_driver_pair_from_root_area(data) } { + pair.0.prune_forced_virtuals(classify); + } + }); +} + /// Re-derives the thread-local `JitDriverPair` for a GC root walk from the /// registered `JIT_DRIVER` cell pointer. /// From 102d9e251b4c72e069adc1f70a027115bdab45ec Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Thu, 30 Jul 2026 22:07:24 +0900 Subject: [PATCH 2/6] gc: give owner-keyed mutator tables a pruner with the root walk's reach `MetaInterp::forced_virtuals` was pruned through `register_ephemeron_pruner`, which hands the classifier no way to name a thread, so the pyre side had to read `JIT_DRIVER` from caller TLS and saw only the collecting thread's driver. That contradicts the contract stated on `MutatorExtraWalkFn` -- "must derive every thread-specific address from `data`, never from caller TLS" -- and left another mutator's dead-owner entries pinned. Add `register_mutator_pruner` / `prune_all_mutator_areas` / `prune_my_mutator_areas` next to the extra-area equivalents, stored in the same `MutatorEntry`. The collector calls them from the pre-sweep point it already prunes at, with the same classifier, and picks between all-mutator and own-mutator on `gc_sync::mutators_quiesced()` -- the same predicate `do_collect_nursery` and `enumerate_root_walker_values` use, so a collection's prune reach always equals its own root-walk reach. That branch is load-bearing: the unconditional form tripped the quiescence assertion, because the pre-sweep point does not own STW. pyre registers the pruner on the same `jit_driver` data as `forced_virtuals_root_walker_area`. The mapdict tables stay on the global registration -- they are a process-global `Mutex` map, reachable from any thread. Also assert in `save_forced_virtuals` that the owner is not nursery-resident. The entry is keyed by a bare address, which is only sound because the virtualizable comes from `FrameBox::new` -> `try_gc_alloc_stable_raw` ("stable across minor and major collections"), never from the frames a trace builds virtually. A debug run of the getframe force fixture completes 11 majors and 44 minors with five forces and five cache hits, and the assertion does not fire. Assisted-by: Claude --- majit/majit-gc/src/collector.rs | 16 +++++- majit/majit-gc/src/shadow_stack.rs | 80 +++++++++++++++++++++++++++ majit/majit-metainterp/src/pyjitpl.rs | 13 +++++ pyre/pyre-jit/src/eval.rs | 39 +++++++------ 4 files changed, 129 insertions(+), 19 deletions(-) diff --git a/majit/majit-gc/src/collector.rs b/majit/majit-gc/src/collector.rs index 072ca14665c..a4356a5adcc 100644 --- a/majit/majit-gc/src/collector.rs +++ b/majit/majit-gc/src/collector.rs @@ -2857,7 +2857,7 @@ impl MiniMarkGC { // An owner outside old-gen is either immortal (`malloc_typed`, no // header to read) or, under a non-moving major, still in the live // nursery; neither can be proven dead here, so both are kept. - crate::shadow_stack::prune_ephemeron_tables(&mut |owner| { + let mut classify_owner = |owner: usize| -> Option { if owner == 0 || !self.oldgen.contains(owner) { return Some(owner); } @@ -2867,7 +2867,19 @@ impl MiniMarkGC { } else { None } - }); + }; + crate::shadow_stack::prune_ephemeron_tables(&mut classify_owner); + // The same question for tables a single mutator owns in its own TLS. + // Those cannot go through the global registration: it names no thread, + // so a major driven here would leave every other mutator's dead-owner + // entries pinned. Reach exactly as far as this collection's own root + // walk did (`enumerate_root_walker_values`) — foreign TLS only while + // this thread owns STW. + if crate::gc_sync::mutators_quiesced() { + crate::shadow_stack::prune_all_mutator_areas(&mut classify_owner); + } else { + crate::shadow_stack::prune_my_mutator_areas(&mut classify_owner); + } // incminimark.py:2510-2511 — run destructors of dying old objects // before the sweep frees them (VISITED still distinguishes // survivors from the dying at this point). diff --git a/majit/majit-gc/src/shadow_stack.rs b/majit/majit-gc/src/shadow_stack.rs index 6794c161628..7b2b59cbd15 100644 --- a/majit/majit-gc/src/shadow_stack.rs +++ b/majit/majit-gc/src/shadow_stack.rs @@ -263,6 +263,7 @@ struct MutatorEntry { bh_regs_stack: *const RefCell>, resume_ref_roots_stack: *const RefCell>, extra_areas: Vec, + pruners: Vec, } /// Walker for one opaque root area owned by a registered mutator. @@ -277,6 +278,18 @@ struct MutatorExtraArea { data: *const (), } +/// Pruner for one owner-keyed side table owned by a registered mutator. +/// +/// Same contract as [`MutatorExtraWalkFn`]: it runs on the collecting thread +/// and must derive everything from `data`, never from caller TLS. +pub type MutatorPrunerFn = unsafe fn(*const (), &mut dyn FnMut(usize) -> Option); + +#[derive(Clone, Copy)] +struct MutatorPruner { + prune: MutatorPrunerFn, + data: *const (), +} + // The raw pointers refer to TLS owned by `thread_id`. The registry only moves // pointer values between threads; dereferencing them requires the STW // quiescence established by gc_sync. @@ -307,6 +320,7 @@ pub fn register_mutator() { bh_regs_stack, resume_ref_roots_stack, extra_areas: Vec::new(), + pruners: Vec::new(), }); } @@ -328,6 +342,72 @@ pub unsafe fn register_mutator_extra_area(walk: MutatorExtraWalkFn, data: *const entry.extra_areas.push(MutatorExtraArea { walk, data }); } +/// Append an owner-keyed-table pruner to the current registered mutator. +/// +/// The ephemeron half of [`register_mutator_extra_area`]: a table whose keys are +/// owner addresses and whose values a walker roots needs its dead-owner entries +/// dropped, and needs it with the same per-mutator reach the root walk has. +/// [`register_ephemeron_pruner`] cannot serve TLS-owned state — it hands the +/// classifier no way to name a thread, so a major driven by one thread would +/// leave every other thread's dead-owner entries pinned. +/// +/// # Safety +/// +/// Same as [`register_mutator_extra_area`]: `data` must stay valid until +/// [`unregister_mutator`] runs on this thread, and `prune` must derive every +/// address it dereferences from `data`, never from caller TLS. +pub unsafe fn register_mutator_pruner(prune: MutatorPrunerFn, data: *const ()) { + let thread_id = std::thread::current().id(); + let mut registry = MUTATOR_REGISTRY.lock().unwrap(); + let entry = registry + .iter_mut() + .find(|entry| entry.thread_id == thread_id) + .expect("register_mutator_pruner called before register_mutator"); + entry.pruners.push(MutatorPruner { prune, data }); +} + +/// Prune every registered mutator's owner-keyed tables during STW. +/// +/// Called from the same pre-sweep point as [`prune_ephemeron_tables`] and with +/// the same classifier; see that function for why only a major prunes. Callers +/// pick between this and [`prune_my_mutator_areas`] on +/// `gc_sync::mutators_quiesced()`, exactly as the root walk picks between +/// [`walk_all_extra_areas`] and [`walk_my_extra_areas`] — so a collection's +/// prune reach always equals its own root-walk reach. +pub fn prune_all_mutator_areas(classify: &mut dyn FnMut(usize) -> Option) { + debug_assert!( + crate::gc_sync::mutators_quiesced(), + "prune_all_mutator_areas reaches foreign mutator TLS; caller must own collector-side STW", + ); + let registry = MUTATOR_REGISTRY.lock().unwrap(); + for mutator in registry.iter() { + for pruner in mutator.pruners.iter() { + // SAFETY: gc_sync has quiesced every registered owner, and each + // pruner's data remains valid until its MutatorEntry is removed. + unsafe { (pruner.prune)(pruner.data, classify) }; + } + } +} + +/// Prune the current mutator's owner-keyed tables. +/// +/// The single-thread collection path, mirroring [`walk_my_extra_areas`]: +/// callers without a registered mutator have no per-thread tables and are a +/// no-op. A collection that only walked its own roots must only prune its own +/// tables — another mutator's owner was never marked here, so its entries +/// cannot be classified. +pub fn prune_my_mutator_areas(classify: &mut dyn FnMut(usize) -> Option) { + let thread_id = std::thread::current().id(); + let registry = MUTATOR_REGISTRY.lock().unwrap(); + let Some(mutator) = registry.iter().find(|entry| entry.thread_id == thread_id) else { + return; + }; + for pruner in mutator.pruners.iter() { + // SAFETY: this is the owning thread's synchronous collection path. + unsafe { (pruner.prune)(pruner.data, classify) }; + } +} + /// Walk every registered mutator's opaque extra root areas during STW. pub fn walk_all_extra_areas(mut visitor: impl FnMut(&mut GcRef)) { debug_assert!( diff --git a/majit/majit-metainterp/src/pyjitpl.rs b/majit/majit-metainterp/src/pyjitpl.rs index 7e74c2e0020..96cae9fb45b 100644 --- a/majit/majit-metainterp/src/pyjitpl.rs +++ b/majit/majit-metainterp/src/pyjitpl.rs @@ -11850,6 +11850,19 @@ impl MetaInterp { /// runs against a named virtualizable and the GUARD_NOT_FORCED that follows /// deopts that same frame's loop. fn save_forced_virtuals(&mut self, owner: u64, ptrs: Vec, ints: Vec) { + // The key is a bare address, so the mechanism holds only while that + // address is stable. It is: the virtualizable is an interpreter-created + // frame (`FrameBox::new` → `try_gc_alloc_stable_raw`, "stable across + // minor and major collections (MiniMark mark-sweep does not move + // old-gen objects)"), never one of the frames a trace builds virtually. + // A nursery owner would break both ends — a minor would forward the + // frame out from under this key, and the pruner's classifier keeps every + // non-old-gen owner, so the entry could never be dropped either. + debug_assert!( + !majit_gc::gc_is_nursery_object(owner as usize), + "forced-virtual cache keyed on a nursery-resident virtualizable \ + (0x{owner:x}): the key must be a move-stable address", + ); match self.forced_virtuals.iter_mut().find(|e| e.0 == owner) { // A second force of the same frame overwrites, the way a second // `set_savedata_ref` overwrites the one `jf_savedata` word. diff --git a/pyre/pyre-jit/src/eval.rs b/pyre/pyre-jit/src/eval.rs index c0c659f5c83..75dda5d5344 100644 --- a/pyre/pyre-jit/src/eval.rs +++ b/pyre/pyre-jit/src/eval.rs @@ -3522,10 +3522,9 @@ fn install_gc_root_walkers() { majit_gc::shadow_stack::register_ephemeron_pruner( pyre_interpreter::objspace::std::mapdict::prune_dead_owner_entries, ); - // `MetaInterp::forced_virtuals` is the same shape: keyed by the forced - // frame's address and rooting its values, so an entry the following - // `GUARD_NOT_FORCED` never consumes has to go when its owner is swept. - majit_gc::shadow_stack::register_ephemeron_pruner(prune_forced_virtuals_for_dead_frames); + // `MetaInterp::forced_virtuals` is the same shape but lives in one mutator's + // `JIT_DRIVER` rather than a global table, so it registers per mutator + // instead — see `forced_virtuals_pruner_area`. } fn register_thread_root_areas() { @@ -3585,6 +3584,14 @@ fn register_thread_root_areas() { register(active_trace_root_walker_area, jit_driver); register(compile_snapshot_root_walker_area, jit_driver); register(forced_virtuals_root_walker_area, jit_driver); + // The ephemeron half of the walker above, on the same `data` so the + // prune reaches exactly the drivers the root walk reaches. + unsafe { + majit_gc::shadow_stack::register_mutator_pruner( + forced_virtuals_pruner_area, + jit_driver, + ); + } } } @@ -4104,19 +4111,17 @@ unsafe fn forced_virtuals_root_walker_area( /// Both would otherwise pin the materialized virtuals for the process lifetime /// and leave a key a recycled `PyFrame` address could match. /// -/// Reads the `JIT_DRIVER` cell directly rather than through `driver_pair()`: -/// that initializes the GC subsystem and can allocate, which is not allowed -/// mid-collection. -fn prune_forced_virtuals_for_dead_frames(classify: &mut dyn FnMut(usize) -> Option) { - JIT_DRIVER.with(|cell| { - let data = cell as *const _ as *const (); - // SAFETY: same re-derivation and same aliasing caveat as the root - // walkers above (`jit_driver_pair_from_root_area`). The pruner runs on - // the collecting thread, so it reaches only that thread's driver. - if let Some(pair) = unsafe { jit_driver_pair_from_root_area(data) } { - pair.0.prune_forced_virtuals(classify); - } - }); +/// Registered per mutator, next to `forced_virtuals_root_walker_area` and with +/// the same `data`, so the prune reaches every driver the root walk reaches. The +/// global `register_ephemeron_pruner` cannot: the table lives in this thread's +/// `JIT_DRIVER`, and a major driven by another thread would leave it pinned. +unsafe fn forced_virtuals_pruner_area( + data: *const (), + classify: &mut dyn FnMut(usize) -> Option, +) { + if let Some(pair) = unsafe { jit_driver_pair_from_root_area(data) } { + pair.0.prune_forced_virtuals(classify); + } } /// Re-derives the thread-local `JitDriverPair` for a GC root walk from the From aa6e5ccd0b560883cf8efa34feca8565c3733127 Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Fri, 31 Jul 2026 00:14:12 +0900 Subject: [PATCH 3/6] jit: name the force entry point in the MAJIT_LOG stream `force_pyframe` and `force_pyframe_vref` reach the same `handle_async_forcing`, and nothing downstream distinguished them, so a census of async forcing could not say which hook produced an event. That is how the vref hook silently kept the store the frame hook had. Three lines, all behind `majit_log_enabled`: hook entry for the vref (distinct from the token arm, since a vref built during tracing carries `forced` already set and `virtual_token = TOKEN_NONE` and returns without running the closure -- counting only the closure conflates "never reached" with "reached and short-circuited"), the token arm itself, and the frame hook. Census over the 330 runnable synth fixtures with these: 5 async-force events, all from getframe_caller_locals_nested_compiled_callee, all consumed by their GUARD_NOT_FORCED; 4 of the 5 materialize an empty cache. The vref hook is entered 195 times across 6 fixtures and takes the token arm 0 times. Assisted-by: Claude --- pyre/pyre-jit/src/eval.rs | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/pyre/pyre-jit/src/eval.rs b/pyre/pyre-jit/src/eval.rs index 75dda5d5344..73a24539750 100644 --- a/pyre/pyre-jit/src/eval.rs +++ b/pyre/pyre-jit/src/eval.rs @@ -3909,12 +3909,27 @@ unsafe extern "C" fn force_pyframe_vref( ) -> *mut pyre_interpreter::PyFrame { let (driver, _info) = driver_pair(); let vrefinfo = majit_metainterp::virtualref::VirtualRefInfo::new(); + // Entry, distinct from the token arm logged below: a vref built during + // tracing carries `forced` already set and `virtual_token = TOKEN_NONE` + // (`virtualref.py:85-92`), so `force_virtual` returns without running the + // closure. Counting only the closure conflates "never reached" with + // "reached and short-circuited". + if majit_metainterp::majit_log_enabled() { + eprintln!("[jit][force-hook] vref-entry vref={vref:p}"); + } let forced = unsafe { vrefinfo.force_virtual(vref as *mut u8, |v| { // `compile.py:967-971 force_now(cpu, token)` — force the JIT frame // the vref names, then run the guard's async forcing, which is // what writes `virtual_token = TOKEN_NONE` and `forced` back. let token = (*v).virtual_token as usize as u64; + // Name the entry point. The two hooks reach the same + // `handle_async_forcing`, and nothing downstream distinguishes + // them — which is how this one silently diverged from + // `force_pyframe` in the first place. + if majit_metainterp::majit_log_enabled() { + eprintln!("[jit][force-hook] vref token=0x{token:x}"); + } driver.force_virtualizable_token(token); }) }; @@ -3991,6 +4006,12 @@ unsafe extern "C" fn force_pyframe(frame: *mut pyre_interpreter::PyFrame) { // `handle_async_forcing` keeps what it materialized for the // GUARD_NOT_FORCED that follows (compile.py:999-1000), so there // is nothing to attach here. + // + // See the counterpart in `force_pyframe_vref` for why the entry + // point is named. + if majit_metainterp::majit_log_enabled() { + eprintln!("[jit][force-hook] frame token=0x{token:x} frame={ptr:p}"); + } driver.force_virtualizable_token(token); }); }; From 17c703e660b2a22da17e44a69afe89c93707c188 Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Fri, 31 Jul 2026 02:11:15 +0900 Subject: [PATCH 4/6] jit: name PyFrame.vable_token as a GC field of the frame size descr MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `rvirtualizable.py:29` declares `('vable_token', llmemory.GCREF)`, so upstream's `gc_fielddescrs` names it and `rewrite.py:498-504 clear_gc_fields` zeroes the slot behind every `new`. pyre types the field `usize` and `PYFRAME_DESCR_GROUP`'s positional census did not list it, so a JIT-inlined `NewWithVtable(pyframe_size_descr())` left it holding recycled nursery bytes — `Nursery::reset` does not zero-fill and the `CallMallocNursery` fast path clears only the header word. `emit_force_virtualizable` (`pyjitpl.py:1148-1158`) then read those bytes with `GETFIELD_GC_R`; the backend spilled the result to a jitframe ref-root slot and marked it live in the call site's gcmap, so the next minor collection dereferenced them and aborted with `GC BUG: invalid type_id` (`synth/mutate_then_raise_caught`, cranelift). Carry the edge through a new `build_object_descr_group_with_extra_gc_edges` lane, the one the inherited `PyObject.w_class` edge already uses, which adds to `gc_fielddescrs` without disturbing the positional `all_fielddescrs` that `field_descr_from_group` indexes. Assisted-by: Claude --- pyre/pyre-jit-trace/src/descr.rs | 70 +++++++++++++++++++++++++++++++- 1 file changed, 68 insertions(+), 2 deletions(-) diff --git a/pyre/pyre-jit-trace/src/descr.rs b/pyre/pyre-jit-trace/src/descr.rs index ef6e871845a..797beb59307 100644 --- a/pyre/pyre-jit-trace/src/descr.rs +++ b/pyre/pyre-jit-trace/src/descr.rs @@ -661,6 +661,33 @@ fn build_object_descr_group_with_def_path( fields: &[(&'static str, usize, usize, Type, bool, bool, bool)], simple_name: &str, def_path: &str, +) -> PyreObjectDescrGroup { + build_object_descr_group_with_extra_gc_edges( + obj_size, + type_id, + vtable, + fields, + simple_name, + def_path, + &[], + ) +} + +/// `build_object_descr_group_with_def_path` plus GC edges that the +/// positional `fields` census does not name. `extra_gc_edges` join the +/// `PyObject.w_class` edge every group already carries: they land in +/// `gc_fielddescrs` — which is what `rewrite.py:498-504 clear_gc_fields` +/// walks to zero a fresh object's GC-pointer slots — while staying out of +/// the positional `all_fielddescrs` list that `field_descr_from_group` +/// indexes. +fn build_object_descr_group_with_extra_gc_edges( + obj_size: usize, + type_id: u32, + vtable: usize, + fields: &[(&'static str, usize, usize, Type, bool, bool, bool)], + simple_name: &str, + def_path: &str, + extra_gc_edges: &[Arc], ) -> PyreObjectDescrGroup { let cache_key = if !def_path.is_empty() { majit_ir::descr::path_hash(def_path) @@ -695,6 +722,8 @@ fn build_object_descr_group_with_def_path( }, ) .collect(); + let mut gc_edges: Vec> = vec![W_CLASS_FIELD_DESCR.clone()]; + gc_edges.extend(extra_gc_edges.iter().cloned()); let group = majit_ir::descr::make_simple_descr_group_keyed_with_headerless( SIZE_DESCR_TAG | (obj_size as u32 & 0x0FFF_FFFF), obj_size, @@ -704,7 +733,7 @@ fn build_object_descr_group_with_def_path( true, false, &specs, - &[W_CLASS_FIELD_DESCR.clone()], + &gc_edges, ); let field_descrs = group.field_descrs; let size_descr = group.size_descr; @@ -1420,8 +1449,31 @@ static W_SLICE_DESCR_GROUP: LazyLock = LazyLock::new(|| { ) }); +/// `rvirtualizable.py:29` appends `('vable_token', llmemory.GCREF)` to the +/// virtualizable's own fields, so upstream's `gc_fielddescrs` names it and +/// `clear_gc_fields` zeroes the slot on every `new`. pyre declares +/// `PyFrame.vable_token` as a plain `usize` and the positional census below +/// does not list it, so without this edge a JIT-inlined +/// `NewWithVtable(pyframe_size_descr())` leaves the slot holding recycled +/// nursery bytes — which `emit_force_virtualizable`'s `GETFIELD_GC_R` then +/// reads as a live GC reference (`pyjitpl.py:1148-1158`). +static PYFRAME_VABLE_TOKEN_FIELD_DESCR: LazyLock> = LazyLock::new(|| { + Arc::new(PyreFieldDescr { + offset: crate::frame_layout::PYFRAME_VABLE_TOKEN_OFFSET, + field_size: std::mem::size_of::(), + field_type: Type::Ref, + signed: false, + immutable: false, + quasi_immutable: false, + name: "vable_token", + index_in_parent: 0, + parent_descr: None, + ei_index: AtomicU32::new(u32::MAX), + }) +}); + static PYFRAME_DESCR_GROUP: LazyLock = LazyLock::new(|| { - build_object_descr_group_with_def_path( + build_object_descr_group_with_extra_gc_edges( std::mem::size_of::(), PYFRAME_GC_TYPE_ID, // `NewWithVtable` writes this typeptr at `cpu.vtable_offset` @@ -1567,6 +1619,7 @@ static PYFRAME_DESCR_GROUP: LazyLock = LazyLock::new(|| { ], "PyFrame", "pyframe::PyFrame", + &[PYFRAME_VABLE_TOKEN_FIELD_DESCR.clone()], ) }); @@ -2980,6 +3033,19 @@ mod tests { ); } + #[test] + fn pyframe_size_descr_clears_the_vable_token_slot() { + let descr = pyframe_size_descr(); + let size = descr.as_size_descr().expect("PyFrame SizeDescr"); + assert!( + size.gc_fielddescrs() + .iter() + .any(|fd| fd.offset() == crate::frame_layout::PYFRAME_VABLE_TOKEN_OFFSET), + "emit_force_virtualizable reads vable_token with GETFIELD_GC_R, so \ + clear_gc_fields must zero it on a JIT-inlined frame allocation" + ); + } + #[test] fn exception_size_descr_clears_every_runtime_traced_gc_field() { let (descr, _, _, _) = w_exception_descrs(ExcKind::ValueError); From 32c797bc919a12d283ef755667de22ff73608e98 Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Fri, 31 Jul 2026 02:11:24 +0900 Subject: [PATCH 5/6] jit: store the inline empty list's length instead of assuming a memzero `emit_empty_list_inline` and `w_list_size_descr`'s doc both stated that `NewWithVtable` leaves the untouched slots zero-filled. It does not: the nursery is not zero-filled (`incminimark.py:211 malloc_zero_filled = False`) and the only zeroes an inline allocation gets are the pending stores `clear_gc_fields` derives from `gc_fielddescrs`. `items`, `int_items.block` and `float_items.block` are GC-pointer fields of the descr and so are covered; `length` is a plain int and was not. Store `length = 0` explicitly, as `rlist.py ll_newlist` does, and restate both comments in terms of the mechanism that actually clears the slots. Assisted-by: Claude --- pyre/pyre-jit-trace/src/descr.rs | 8 ++++++-- pyre/pyre-jit-trace/src/helpers.rs | 22 ++++++++++++++-------- 2 files changed, 20 insertions(+), 10 deletions(-) diff --git a/pyre/pyre-jit-trace/src/descr.rs b/pyre/pyre-jit-trace/src/descr.rs index 797beb59307..fcc16280692 100644 --- a/pyre/pyre-jit-trace/src/descr.rs +++ b/pyre/pyre-jit-trace/src/descr.rs @@ -2078,8 +2078,12 @@ pub fn int_mutable_cell_value_descr() -> DescrRef { /// Size descriptor for `W_ListObject` allocation via NewWithVtable. /// vtable = &LIST_TYPE; the Object-strategy fields `length` / `items` / -/// `strategy` are SetField'd after; `int_items` / `float_items` stay at the -/// NewWithVtable memzero (== empty, never read under the Object strategy). +/// `strategy` are SetField'd after. `int_items.block` / `float_items.block` +/// are GC-pointer fields of this descr, so `rewrite.py:498-504 +/// clear_gc_fields` zeroes them behind the allocation (== empty, never read +/// under the Object strategy); their `len` halves are plain ints and stay at +/// whatever the recycled nursery bytes held, which no strategy reads while the +/// block slot is null. pub fn w_list_size_descr() -> DescrRef { W_LIST_DESCR_GROUP.size_descr.clone() } diff --git a/pyre/pyre-jit-trace/src/helpers.rs b/pyre/pyre-jit-trace/src/helpers.rs index 21f897ea91f..050662b0fa3 100644 --- a/pyre/pyre-jit-trace/src/helpers.rs +++ b/pyre/pyre-jit-trace/src/helpers.rs @@ -650,19 +650,25 @@ pub fn emit_object_list_inline(ctx: &mut TraceCtx, items: &[OpRef]) -> OpRef { /// wrapper plus the `strategy` store, mirroring `w_list_new(vec![])` / /// `w_list_new_with_strategy(vec![], Empty)`. /// -/// `length` (0) and `items` (null) stay zero-filled by `NewWithVtable`, as the -/// non-Object strategies leave them. The typed `int_items` / `float_items` -/// blocks also stay null: the Empty strategy reads neither (its first append -/// installs fresh typed storage via `switch_to_correct_strategy`), and -/// `list_object_custom_trace` forwards a typed block only when the GC owns it, -/// so a null slot is inert. OptVirtualize folds the whole wrapper when the -/// list never escapes. +/// `items` and the typed `int_items` / `float_items` blocks stay null because +/// they are GC-pointer fields of the size descr, so `rewrite.py:498-504 +/// clear_gc_fields` zeroes them behind the `NewWithVtable`. `length` gets no +/// such pending zero — the recycled nursery bytes a `CALL_MALLOC_NURSERY` +/// hands back are not zero-filled (`incminimark.py:211 malloc_zero_filled = +/// False`) — so it is stored explicitly here, as `rlist.py ll_newlist` does. +/// OptVirtualize folds the whole wrapper when the list never escapes. pub fn emit_empty_list_inline(ctx: &mut TraceCtx) -> OpRef { - use crate::descr::{list_strategy_descr, w_list_size_descr}; + use crate::descr::{list_length_descr, list_strategy_descr, w_list_size_descr}; let list = ctx.record_op_with_descr(OpCode::NewWithVtable, &[], w_list_size_descr()); ctx.heap_cache_mut().new_object(list); + let zero = ctx.const_int(0); + let length_descr = list_length_descr(); + let length_idx = length_descr.index(); + ctx.record_op_with_descr(OpCode::SetfieldGc, &[list, zero], length_descr); + ctx.heapcache_setfield_cached(list, length_idx, zero); + let strategy_const = ctx.const_int(pyre_object::listobject::ListStrategy::Empty as i64); let strategy_descr = list_strategy_descr(); let strategy_idx = strategy_descr.index(); From 248e9a7cacb160f9213e23d206581509ad54915a Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Fri, 31 Jul 2026 02:23:32 +0900 Subject: [PATCH 6/6] fix: drop the duplicate interp_return_log_enabled definition #907 added a second `#[cfg(not(feature = "sandbox"))] fn interp_return_log_enabled` with the same body next to the existing one, so `pyre-interpreter` fails to compile with E0428 on main. Keep the first definition and its doc comment; delete the duplicate. Assisted-by: Claude --- pyre/pyre-interpreter/src/eval.rs | 9 --------- 1 file changed, 9 deletions(-) diff --git a/pyre/pyre-interpreter/src/eval.rs b/pyre/pyre-interpreter/src/eval.rs index cee028879cd..61a6d64659b 100644 --- a/pyre/pyre-interpreter/src/eval.rs +++ b/pyre/pyre-interpreter/src/eval.rs @@ -610,15 +610,6 @@ fn gc_prebuilt_remember_enabled() -> bool { }) } -/// Whether the per-return diagnostic dump is enabled -/// (`PYRE_INTERP_RETURN_LOG`). The probe sits on the RETURN_VALUE path, so an -/// uncached read would pay a `getenv` on every Python return. -#[cfg(not(feature = "sandbox"))] -fn interp_return_log_enabled() -> bool { - static ENABLED: std::sync::OnceLock = std::sync::OnceLock::new(); - *ENABLED.get_or_init(|| std::env::var_os("PYRE_INTERP_RETURN_LOG").is_some()) -} - pub fn capture_pyframe_root_area() -> *const () { PYFRAME_ROOT_AREA.with(|area| area as *const _ as *const ()) }