From 532aac48167ef705eaca35bf978009fcdc9fb304 Mon Sep 17 00:00:00 2001 From: "Jeong, YunWon" Date: Sun, 16 Aug 2026 12:29:35 +0900 Subject: [PATCH 1/3] majit: add three env-gated blackhole diagnostics MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `MAJIT_BH_ROOT_CHECK` reports a register bank resized while a root registration still names its buffer. `push_resume_ref_roots` and `push_bh_regs` root a bank by the raw `(pointer, length)` of its `Vec` buffer and document the same precondition — the bank is sized once and only indexed afterwards. `ref_bank_registration_len` answers whether that still holds for a given buffer, across both stacks. `MAJIT_BH_CALL_ARGS` reports a residual call's ref arguments by the register index each came out of, plus `num_regs_r` and the bank length, so an argument can be told apart as resume-seeded, written by an opcode of this run, or read out of the jitcode's constant pool. `MAJIT_BH_VABLE` reports a virtualizable array read by its index register rather than only the index value, plus the resolved array and its length. `handler_getarrayitem_vable_r` takes the index from `registers_i`, and an index register the resume section never named reads whatever `setposition` left there; zero is in bounds, so the existing bounds assert stays silent. All three follow the `MAJIT_GC_BH_PROBE` / `MAJIT_BH_NULL_ARG` shape: a `OnceLock`-cached env read and an `eprintln!`, off by default. Assisted-by: Claude --- majit/majit-gc/src/shadow_stack.rs | 28 ++++++ majit/majit-metainterp/src/blackhole.rs | 115 +++++++++++++++++++++++- majit/majit-metainterp/src/lib.rs | 7 ++ 3 files changed, 149 insertions(+), 1 deletion(-) diff --git a/majit/majit-gc/src/shadow_stack.rs b/majit/majit-gc/src/shadow_stack.rs index ebd645e6aa7..6cd1cb5c566 100644 --- a/majit/majit-gc/src/shadow_stack.rs +++ b/majit/majit-gc/src/shadow_stack.rs @@ -1148,6 +1148,34 @@ pub fn resume_ref_slice_registered(ptr: *const i64) -> bool { RESUME_REF_ROOTS_STACK.with(|ss| ss.borrow().iter().any(|&(p, _)| std::ptr::eq(p, ptr))) } +/// The longest registration naming a buffer that starts at `ptr`, across both +/// the resume-construction stack and the blackhole register-bank stack. +/// +/// Both stacks root a bank by the raw `(pointer, length)` of its `Vec` buffer +/// and share one precondition: the bank is sized once and only indexed +/// afterwards. A caller about to resize a bank consults this to find out +/// whether that precondition still holds for the buffer it is holding. +pub fn ref_bank_registration_len(ptr: *const i64) -> Option { + let resume = RESUME_REF_ROOTS_STACK.with(|ss| { + ss.borrow() + .iter() + .filter(|&&(p, _)| std::ptr::eq(p, ptr)) + .map(|&(_, len)| len) + .max() + }); + let bh = BH_REGS_STACK.with(|ss| { + ss.borrow() + .iter() + .filter(|entry| std::ptr::eq(entry.regs_ptr.cast_const(), ptr)) + .map(|entry| entry.regs_len) + .max() + }); + match (resume, bh) { + (Some(a), Some(b)) => Some(a.max(b)), + (only, None) | (None, only) => only, + } +} + /// Register a ref slice as a GC root for the blackhole resume /// construction window (`resume.py:1312 blackhole_from_resumedata`). /// diff --git a/majit/majit-metainterp/src/blackhole.rs b/majit/majit-metainterp/src/blackhole.rs index 80e8bf3f332..271b785bc30 100644 --- a/majit/majit-metainterp/src/blackhole.rs +++ b/majit/majit-metainterp/src/blackhole.rs @@ -444,7 +444,38 @@ impl BlackholeInterpreter { } } + /// TEMPORARY DIAGNOSTIC (`MAJIT_BH_ROOT_CHECK`). Remove with the + /// investigation. + /// + /// `push_resume_ref_roots` and `push_bh_regs` root a ref bank by the raw + /// `(pointer, length)` of its `Vec` buffer, and both document one + /// precondition: the bank is sized once and only indexed afterwards. An + /// interpreter that comes back out of the pool while an earlier window + /// still names its buffer breaks that — `init_register_file_from_i64s` + /// resizes, and a growth past the capacity moves the buffer, so the + /// registration names freed memory while the live bank is rooted by + /// nothing. Report the collision here instead of leaving it to the + /// wrong-type dereference it becomes several opcodes later. + fn report_resize_under_registration(&self, new_len: usize) { + let regs = &self.registers_r; + if !crate::bh_root_check_enabled() || regs.capacity() == 0 { + return; + } + let ptr = regs.as_ptr(); + let Some(rooted_len) = majit_gc::shadow_stack::ref_bank_registration_len(ptr) else { + return; + }; + eprintln!( + "[bh-root] resize under registration: buf={ptr:?} rooted_len={rooted_len} \ + live_len={} capacity={} new_len={new_len} reallocates={}", + regs.len(), + regs.capacity(), + new_len > regs.capacity(), + ); + } + fn init_register_files_from_runtime_jitcode(&mut self, jitcode: &JitCode) { + self.report_resize_under_registration(jitcode.num_regs_and_consts_r()); Self::init_register_file_from_i64s( &mut self.registers_i, jitcode.num_regs_and_consts_i(), @@ -7700,6 +7731,38 @@ fn bh_null_arg_report(bh: &BlackholeInterpreter, ar: &[i64], position: usize) { } } +/// TEMPORARY DIAGNOSTIC (`MAJIT_BH_CALL_ARGS`). Remove with the investigation. +/// +/// Report the ref arguments of a residual call by the register index each one +/// came out of, alongside the frame's own register geometry. `[bh-seed]` names +/// the indices the resume filled, so an argument index that appears there is +/// carrying what the guard's resume data said, one that does not is carrying +/// whatever an opcode of this run wrote, and one at or above `num_regs_r` is +/// reading the jitcode's constant pool rather than a register at all. Those +/// three provenances need different fixes and the value alone cannot tell them +/// apart. +fn bh_call_arg_report(bh: &BlackholeInterpreter, code: &[u8], list_pos: usize, func: i64) { + static ARMED: std::sync::OnceLock = std::sync::OnceLock::new(); + if !*ARMED.get_or_init(|| std::env::var_os("MAJIT_BH_CALL_ARGS").is_some()) { + return; + } + let count = code[list_pos] as usize; + let pairs: Vec = (0..count) + .map(|i| { + let reg = code[list_pos + 1 + i]; + format!("r{reg}={:#x}", bh.registers_r[reg as usize]) + }) + .collect(); + eprintln!( + "[bh-call] jitcode={} pos={} func={func:#x} num_regs_r={} bank_len={} args=[{}]", + bh.jitcode.name(), + bh.last_opcode_position, + bh.jitcode.num_regs_r(), + bh.registers_r.len(), + pairs.join(" "), + ); +} + // Call operations (`blackhole.py:1224-1276`) fn handler_residual_call_irf_i( bh: &mut BlackholeInterpreter, @@ -7931,6 +7994,7 @@ fn handler_residual_call_r_v( let (calldescr, p) = read_descr(bh, code, p); let calldescr = calldescr.as_calldescr().clone(); bh_null_arg_report(bh, &ar, position); + bh_call_arg_report(bh, code, position + 1, func); // blackhole.py:1230-1232 → bhimpl_residual_call_r_v. BH_LAST_EXC_VALUE.with(|c| c.set(0)); bh.bhimpl_residual_call_r_v(func, &ar, &calldescr); @@ -10088,7 +10152,8 @@ fn handler_getarrayitem_vable_r( ) -> Result { let nbody_debug = crate::nbody_debug_enabled(); let vable = bh.registers_r[code[p] as usize]; - let index = bh.registers_i[code[p + 1] as usize] as usize; + let p_index_reg = p + 1; + let index = bh.registers_i[code[p_index_reg] as usize] as usize; let vinfo = vable_clear_token_and_get_vinfo(bh, vable); let (field_descr, p) = read_descr(bh, code, p + 2); let array_idx = field_descr.as_vable_array_index(); @@ -10102,9 +10167,57 @@ fn handler_getarrayitem_vable_r( bh.position, bh.last_opcode_position, index, value as usize ); } + bh_vable_index_report( + bh, + "get-r", + code[p_index_reg] as usize, + index, + vable, + ainfo, + code[p] as usize, + value, + ); bh.registers_r[code[p] as usize] = value; Ok(p + 1) } + +/// TEMPORARY DIAGNOSTIC (`MAJIT_BH_VABLE`). Remove with the investigation. +/// +/// Report a virtualizable array access by the *index register* it read, not +/// only by the index value. The resume section seeds one register file entry +/// per live variable; an index register that the section never named reads +/// whatever `setposition` left there, which is zero for a plain register and +/// the jitcode's own constant for a slot at or above `num_regs_i`. Both are +/// in-bounds indices, so the existing bounds assert stays silent and the wrong +/// stack slot travels to whatever consumes it. +#[expect( + clippy::too_many_arguments, + reason = "a diagnostic that names both operands, their registers, and the array it resolved to" +)] +fn bh_vable_index_report( + bh: &BlackholeInterpreter, + kind: &str, + index_reg: usize, + index: usize, + vable: i64, + ainfo: &crate::virtualizable::VableArrayInfo, + dst_reg: usize, + value: i64, +) { + static ARMED: std::sync::OnceLock = std::sync::OnceLock::new(); + if !*ARMED.get_or_init(|| std::env::var_os("MAJIT_BH_VABLE").is_some()) { + return; + } + let len = unsafe { crate::virtualizable::bhimpl_arraylen_vable(vable as *const u8, ainfo) }; + eprintln!( + "[bh-vable-{kind}] jitcode={} pos={} vable={vable:#x} array={:?} len={len} \ + index_reg=i{index_reg} index={index} num_regs_i={} dst=r{dst_reg} value={value:#x}", + bh.jitcode.name(), + bh.last_opcode_position, + ainfo.name, + bh.jitcode.num_regs_i(), + ); +} fn handler_setarrayitem_vable_i( bh: &mut BlackholeInterpreter, code: &[u8], diff --git a/majit/majit-metainterp/src/lib.rs b/majit/majit-metainterp/src/lib.rs index 4202b8095ee..454541808a8 100644 --- a/majit/majit-metainterp/src/lib.rs +++ b/majit/majit-metainterp/src/lib.rs @@ -221,6 +221,13 @@ pub fn bh_debug_enabled() -> bool { *FLAG.get_or_init(|| std::env::var_os("MAJIT_BH_DEBUG").is_some()) } +/// TEMPORARY DIAGNOSTIC. Reports a blackhole register bank resized while a +/// root registration still names its buffer. Remove with the investigation. +pub fn bh_root_check_enabled() -> bool { + static FLAG: std::sync::OnceLock = std::sync::OnceLock::new(); + *FLAG.get_or_init(|| std::env::var_os("MAJIT_BH_ROOT_CHECK").is_some()) +} + pub fn callee_rca_enabled() -> bool { static FLAG: std::sync::OnceLock = std::sync::OnceLock::new(); *FLAG.get_or_init(|| std::env::var_os("PYRE_CALLEE_RCA").is_some()) From 260448e9c869f0834884a6c68c4e5d14078ef575 Mon Sep 17 00:00:00 2001 From: "Jeong, YunWon" Date: Sun, 16 Aug 2026 18:43:07 +0900 Subject: [PATCH 2/3] jit: stop replaying an opcode's stack effect when the after-residual reconcile names the coordinate the mirror already holds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `capture_resumedata(after_residual_call=True)` advances the walk-level operand-stack mirror to the post-call boundary before the guard supplies the virtualizable snapshot. The advance ran unconditionally, including when the resume `py_pc` equals `vstack_cur_pypc` — the coordinate the mirror is already at. `step_vstack_mirror` returns on that same equality, so there is no boundary there and `reconcile_vstack_at_boundary` replays the current opcode's stack effect a second time. `VstackOpClass::Swap` and `Copy` are permutations: applying `SWAP i` twice is the identity. In `dataclasses._process_class` the comprehension `[f for f in fields.values() if f._field_type in (_FIELD, _FIELD_INITVAR)]` lowers to `LOAD_FAST_AND_CLEAR; SWAP 2; BUILD_LIST 0; SWAP 2; FOR_ITER`, and the `BUILD_LIST` residual's guard reconciles at the second `SWAP`'s own py_pc. The mirror then held operand slots 1 and 2 in their pre-swap order for the rest of the walk, and every later guard snapshot published the accumulator list and the iterator crossed in the virtualizable array. On guard failure `LIST_APPEND` read the iterator out of the accumulator slot and `w_list_append` segfaulted. Gate the advance on `py_pc != ctx.vstack_cur_pypc`, which is what "the same transition the following walk step would make" already meant. Assisted-by: Claude --- .../src/jitcode_dispatch/resume_snapshot.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/pyre/pyre-jit-trace/src/jitcode_dispatch/resume_snapshot.rs b/pyre/pyre-jit-trace/src/jitcode_dispatch/resume_snapshot.rs index 0a3065140ea..4bb42c59a6b 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/resume_snapshot.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/resume_snapshot.rs @@ -591,7 +591,16 @@ pub(crate) fn walker_capture_snapshot_for_last_guard_impl( // This is the same transition the following walk step would make; // it merely makes the guard's resume image observe it at the // required point. - if after_residual_call && ctx.vstack_valid { + // + // `py_pc == vstack_cur_pypc` is not that transition: the walk has + // not left the opcode the mirror already holds, so the following + // step makes no boundary at all — `step_vstack_mirror` returns on + // the same equality. Reconciling anyway replays that opcode's + // stack effect a second time, and the permutation classes are not + // idempotent: a `SWAP` applied twice is the identity, so the two + // slots it exchanged keep their pre-swap order for the rest of the + // walk and every later guard snapshot publishes them crossed. + if after_residual_call && ctx.vstack_valid && py_pc != ctx.vstack_cur_pypc { let jc = unsafe { &*sym.jitcode() }; let code_ptr = jc.payload.code_ptr; if !code_ptr.is_null() { From 766471a4d6ffa61e531e744db86ab0d3e0794a9b Mon Sep 17 00:00:00 2001 From: "Jeong, YunWon" Date: Sun, 16 Aug 2026 18:43:11 +0900 Subject: [PATCH 3/3] Revert "majit: add three env-gated blackhole diagnostics" This reverts commit fb8972dfb4cf3ea7412ffb3785fea270e75aea3e. --- majit/majit-gc/src/shadow_stack.rs | 28 ------ majit/majit-metainterp/src/blackhole.rs | 115 +----------------------- majit/majit-metainterp/src/lib.rs | 7 -- 3 files changed, 1 insertion(+), 149 deletions(-) diff --git a/majit/majit-gc/src/shadow_stack.rs b/majit/majit-gc/src/shadow_stack.rs index 6cd1cb5c566..ebd645e6aa7 100644 --- a/majit/majit-gc/src/shadow_stack.rs +++ b/majit/majit-gc/src/shadow_stack.rs @@ -1148,34 +1148,6 @@ pub fn resume_ref_slice_registered(ptr: *const i64) -> bool { RESUME_REF_ROOTS_STACK.with(|ss| ss.borrow().iter().any(|&(p, _)| std::ptr::eq(p, ptr))) } -/// The longest registration naming a buffer that starts at `ptr`, across both -/// the resume-construction stack and the blackhole register-bank stack. -/// -/// Both stacks root a bank by the raw `(pointer, length)` of its `Vec` buffer -/// and share one precondition: the bank is sized once and only indexed -/// afterwards. A caller about to resize a bank consults this to find out -/// whether that precondition still holds for the buffer it is holding. -pub fn ref_bank_registration_len(ptr: *const i64) -> Option { - let resume = RESUME_REF_ROOTS_STACK.with(|ss| { - ss.borrow() - .iter() - .filter(|&&(p, _)| std::ptr::eq(p, ptr)) - .map(|&(_, len)| len) - .max() - }); - let bh = BH_REGS_STACK.with(|ss| { - ss.borrow() - .iter() - .filter(|entry| std::ptr::eq(entry.regs_ptr.cast_const(), ptr)) - .map(|entry| entry.regs_len) - .max() - }); - match (resume, bh) { - (Some(a), Some(b)) => Some(a.max(b)), - (only, None) | (None, only) => only, - } -} - /// Register a ref slice as a GC root for the blackhole resume /// construction window (`resume.py:1312 blackhole_from_resumedata`). /// diff --git a/majit/majit-metainterp/src/blackhole.rs b/majit/majit-metainterp/src/blackhole.rs index 271b785bc30..80e8bf3f332 100644 --- a/majit/majit-metainterp/src/blackhole.rs +++ b/majit/majit-metainterp/src/blackhole.rs @@ -444,38 +444,7 @@ impl BlackholeInterpreter { } } - /// TEMPORARY DIAGNOSTIC (`MAJIT_BH_ROOT_CHECK`). Remove with the - /// investigation. - /// - /// `push_resume_ref_roots` and `push_bh_regs` root a ref bank by the raw - /// `(pointer, length)` of its `Vec` buffer, and both document one - /// precondition: the bank is sized once and only indexed afterwards. An - /// interpreter that comes back out of the pool while an earlier window - /// still names its buffer breaks that — `init_register_file_from_i64s` - /// resizes, and a growth past the capacity moves the buffer, so the - /// registration names freed memory while the live bank is rooted by - /// nothing. Report the collision here instead of leaving it to the - /// wrong-type dereference it becomes several opcodes later. - fn report_resize_under_registration(&self, new_len: usize) { - let regs = &self.registers_r; - if !crate::bh_root_check_enabled() || regs.capacity() == 0 { - return; - } - let ptr = regs.as_ptr(); - let Some(rooted_len) = majit_gc::shadow_stack::ref_bank_registration_len(ptr) else { - return; - }; - eprintln!( - "[bh-root] resize under registration: buf={ptr:?} rooted_len={rooted_len} \ - live_len={} capacity={} new_len={new_len} reallocates={}", - regs.len(), - regs.capacity(), - new_len > regs.capacity(), - ); - } - fn init_register_files_from_runtime_jitcode(&mut self, jitcode: &JitCode) { - self.report_resize_under_registration(jitcode.num_regs_and_consts_r()); Self::init_register_file_from_i64s( &mut self.registers_i, jitcode.num_regs_and_consts_i(), @@ -7731,38 +7700,6 @@ fn bh_null_arg_report(bh: &BlackholeInterpreter, ar: &[i64], position: usize) { } } -/// TEMPORARY DIAGNOSTIC (`MAJIT_BH_CALL_ARGS`). Remove with the investigation. -/// -/// Report the ref arguments of a residual call by the register index each one -/// came out of, alongside the frame's own register geometry. `[bh-seed]` names -/// the indices the resume filled, so an argument index that appears there is -/// carrying what the guard's resume data said, one that does not is carrying -/// whatever an opcode of this run wrote, and one at or above `num_regs_r` is -/// reading the jitcode's constant pool rather than a register at all. Those -/// three provenances need different fixes and the value alone cannot tell them -/// apart. -fn bh_call_arg_report(bh: &BlackholeInterpreter, code: &[u8], list_pos: usize, func: i64) { - static ARMED: std::sync::OnceLock = std::sync::OnceLock::new(); - if !*ARMED.get_or_init(|| std::env::var_os("MAJIT_BH_CALL_ARGS").is_some()) { - return; - } - let count = code[list_pos] as usize; - let pairs: Vec = (0..count) - .map(|i| { - let reg = code[list_pos + 1 + i]; - format!("r{reg}={:#x}", bh.registers_r[reg as usize]) - }) - .collect(); - eprintln!( - "[bh-call] jitcode={} pos={} func={func:#x} num_regs_r={} bank_len={} args=[{}]", - bh.jitcode.name(), - bh.last_opcode_position, - bh.jitcode.num_regs_r(), - bh.registers_r.len(), - pairs.join(" "), - ); -} - // Call operations (`blackhole.py:1224-1276`) fn handler_residual_call_irf_i( bh: &mut BlackholeInterpreter, @@ -7994,7 +7931,6 @@ fn handler_residual_call_r_v( let (calldescr, p) = read_descr(bh, code, p); let calldescr = calldescr.as_calldescr().clone(); bh_null_arg_report(bh, &ar, position); - bh_call_arg_report(bh, code, position + 1, func); // blackhole.py:1230-1232 → bhimpl_residual_call_r_v. BH_LAST_EXC_VALUE.with(|c| c.set(0)); bh.bhimpl_residual_call_r_v(func, &ar, &calldescr); @@ -10152,8 +10088,7 @@ fn handler_getarrayitem_vable_r( ) -> Result { let nbody_debug = crate::nbody_debug_enabled(); let vable = bh.registers_r[code[p] as usize]; - let p_index_reg = p + 1; - let index = bh.registers_i[code[p_index_reg] as usize] as usize; + let index = bh.registers_i[code[p + 1] as usize] as usize; let vinfo = vable_clear_token_and_get_vinfo(bh, vable); let (field_descr, p) = read_descr(bh, code, p + 2); let array_idx = field_descr.as_vable_array_index(); @@ -10167,57 +10102,9 @@ fn handler_getarrayitem_vable_r( bh.position, bh.last_opcode_position, index, value as usize ); } - bh_vable_index_report( - bh, - "get-r", - code[p_index_reg] as usize, - index, - vable, - ainfo, - code[p] as usize, - value, - ); bh.registers_r[code[p] as usize] = value; Ok(p + 1) } - -/// TEMPORARY DIAGNOSTIC (`MAJIT_BH_VABLE`). Remove with the investigation. -/// -/// Report a virtualizable array access by the *index register* it read, not -/// only by the index value. The resume section seeds one register file entry -/// per live variable; an index register that the section never named reads -/// whatever `setposition` left there, which is zero for a plain register and -/// the jitcode's own constant for a slot at or above `num_regs_i`. Both are -/// in-bounds indices, so the existing bounds assert stays silent and the wrong -/// stack slot travels to whatever consumes it. -#[expect( - clippy::too_many_arguments, - reason = "a diagnostic that names both operands, their registers, and the array it resolved to" -)] -fn bh_vable_index_report( - bh: &BlackholeInterpreter, - kind: &str, - index_reg: usize, - index: usize, - vable: i64, - ainfo: &crate::virtualizable::VableArrayInfo, - dst_reg: usize, - value: i64, -) { - static ARMED: std::sync::OnceLock = std::sync::OnceLock::new(); - if !*ARMED.get_or_init(|| std::env::var_os("MAJIT_BH_VABLE").is_some()) { - return; - } - let len = unsafe { crate::virtualizable::bhimpl_arraylen_vable(vable as *const u8, ainfo) }; - eprintln!( - "[bh-vable-{kind}] jitcode={} pos={} vable={vable:#x} array={:?} len={len} \ - index_reg=i{index_reg} index={index} num_regs_i={} dst=r{dst_reg} value={value:#x}", - bh.jitcode.name(), - bh.last_opcode_position, - ainfo.name, - bh.jitcode.num_regs_i(), - ); -} fn handler_setarrayitem_vable_i( bh: &mut BlackholeInterpreter, code: &[u8], diff --git a/majit/majit-metainterp/src/lib.rs b/majit/majit-metainterp/src/lib.rs index 454541808a8..4202b8095ee 100644 --- a/majit/majit-metainterp/src/lib.rs +++ b/majit/majit-metainterp/src/lib.rs @@ -221,13 +221,6 @@ pub fn bh_debug_enabled() -> bool { *FLAG.get_or_init(|| std::env::var_os("MAJIT_BH_DEBUG").is_some()) } -/// TEMPORARY DIAGNOSTIC. Reports a blackhole register bank resized while a -/// root registration still names its buffer. Remove with the investigation. -pub fn bh_root_check_enabled() -> bool { - static FLAG: std::sync::OnceLock = std::sync::OnceLock::new(); - *FLAG.get_or_init(|| std::env::var_os("MAJIT_BH_ROOT_CHECK").is_some()) -} - pub fn callee_rca_enabled() -> bool { static FLAG: std::sync::OnceLock = std::sync::OnceLock::new(); *FLAG.get_or_init(|| std::env::var_os("PYRE_CALLEE_RCA").is_some())