diff --git a/majit/majit-backend-wasm/src/codegen.rs b/majit/majit-backend-wasm/src/codegen.rs index 4f190cd899c..4f2bf79ab3d 100644 --- a/majit/majit-backend-wasm/src/codegen.rs +++ b/majit/majit-backend-wasm/src/codegen.rs @@ -872,6 +872,12 @@ impl HomeLiveness { fn live_across_any(&self, raw: u32, positions: &[usize]) -> bool { positions.iter().any(|&at| self.live_across(raw, at)) } + + /// Index of the last op that reads `raw` (arg or fail arg), or `-1` when + /// nothing reads it. `regalloc.py` spells this `Lifetime.last_usage`. + fn last_use(&self, raw: u32) -> i32 { + self.last_use.get(raw as usize).copied().unwrap_or(-1) + } } /// Static collecting-call positions whose gcmap-visible homes may be forwarded. @@ -2114,6 +2120,7 @@ fn build_function( let mut in_loop_body = false; let mut labels_passed = 0usize; let mut ovf_flag_live = false; + let mut fused_guard_at: Option = None; for (op_idx, op) in ops.iter().enumerate() { if op.opcode == OpCode::Label && key_dispatch && labels_passed < num_labels { @@ -2198,6 +2205,48 @@ fn build_function( }), (true, true) => Some(1u32), }; + // The guard whose condition the previous op already pushed and tested. + // `block_exit_depth` is unchanged across the pair: only a LABEL moves + // `labels_passed` or opens the `loop`, and a guard is neither. + if fused_guard_at == Some(op_idx) { + fused_guard_at = None; + continue; + } + if let Some(kind) = cond_kind_of(op.opcode) { + match next_op_can_accept_cc( + ops, + op_idx, + op.pos.get(), + &liveness, + &label_resume, + &ref_homes, + ) { + Some(guard) => { + push_cond(&mut sink, constants, value_types, op, kind); + // GUARD_TRUE exits when the condition is false. `i32.eqz` + // rather than the inverted comparison: `!(a < b)` is + // `a >= b` for integers but not for floats, where an + // unordered operand makes both forms false. + if guard.opcode == OpCode::GuardTrue { + sink.i32_eqz(); + } + emit_guard_if_exit( + &mut sink, + constants, + value_types, + guard_idx, + guard, + block_exit_depth, + ); + guard_idx += 1; + fused_guard_at = Some(op_idx + 1); + } + None => emit_cond(&mut sink, constants, value_types, op, kind), + } + // A comparison result is never a Ref, so the store-on-def tail has + // nothing to do for it. + continue; + } match op.opcode { OpCode::Label => {} @@ -2249,14 +2298,41 @@ fn build_function( let label_args = find_label_args(ops, op); let jump_args = op.getarglist(); let n = jump_args.len().min(label_args.len()); - for (jump_arg, label_arg) in jump_args.iter().zip(label_args.iter()).take(n) { + // A pair whose jump arg IS its label arg rebinds the local to + // the value it already holds, so its read/write contributes + // nothing: every read precedes every write, and no other pair + // writes the same target (a LABEL's args are distinct boxes, + // asserted below), so dropping the pair leaves every remaining + // read and write unchanged. The home-refresh loop below already + // skips this case for the same reason. + let moved: Vec = (0..n) + .filter(|&i| { + let jarg = jump_args[i].to_opref(); + jarg.is_constant() || jarg.raw() != label_args[i].raw() + }) + .collect(); + debug_assert!( + { + let mut seen: Vec = label_args[..n].iter().map(|a| a.raw()).collect(); + seen.sort_unstable(); + seen.windows(2).all(|w| w[0] != w[1]) + }, + "LABEL args must be distinct for the identity-pair skip to be a no-op" + ); + for &i in &moved { + let label_arg = label_args[i]; if value_types[label_arg.raw() as usize] == ValType::F64 { - emit_resolve_f64(&mut sink, constants, value_types, jump_arg.to_opref()); + emit_resolve_f64( + &mut sink, + constants, + value_types, + jump_args[i].to_opref(), + ); } else { - emit_resolve(&mut sink, constants, value_types, jump_arg.to_opref()); + emit_resolve(&mut sink, constants, value_types, jump_args[i].to_opref()); } } - for i in (0..n).rev() { + for &i in moved.iter().rev() { sink.local_set(1 + label_args[i].raw()); } // The parallel move rebinds loop-carried locals without going @@ -2605,28 +2681,6 @@ fn build_function( ); } - // ── Integer comparisons (signed) ── - OpCode::IntLt => emit_cmp(&mut sink, constants, value_types, op, CmpOp::I64LtS), - OpCode::IntLe => emit_cmp(&mut sink, constants, value_types, op, CmpOp::I64LeS), - OpCode::IntEq => emit_cmp(&mut sink, constants, value_types, op, CmpOp::I64Eq), - OpCode::IntNe => emit_cmp(&mut sink, constants, value_types, op, CmpOp::I64Ne), - OpCode::IntGt => emit_cmp(&mut sink, constants, value_types, op, CmpOp::I64GtS), - OpCode::IntGe => emit_cmp(&mut sink, constants, value_types, op, CmpOp::I64GeS), - - // ── Integer comparisons (unsigned) ── - OpCode::UintLt => emit_cmp(&mut sink, constants, value_types, op, CmpOp::I64LtU), - OpCode::UintLe => emit_cmp(&mut sink, constants, value_types, op, CmpOp::I64LeU), - OpCode::UintGt => emit_cmp(&mut sink, constants, value_types, op, CmpOp::I64GtU), - OpCode::UintGe => emit_cmp(&mut sink, constants, value_types, op, CmpOp::I64GeU), - - // ── Pointer comparisons ── - OpCode::PtrEq | OpCode::InstancePtrEq => { - emit_cmp(&mut sink, constants, value_types, op, CmpOp::I64Eq); - } - OpCode::PtrNe | OpCode::InstancePtrNe => { - emit_cmp(&mut sink, constants, value_types, op, CmpOp::I64Ne); - } - // ── Unary ops ── OpCode::IntNeg => emit_unary_vi( &mut sink, @@ -2652,26 +2706,6 @@ fn build_function( s.i64_xor(); }, ), - OpCode::IntIsTrue => { - let vi = op.pos.get().raw(); - if !OpRef::raw_is_constant(vi) { - emit_resolve(&mut sink, constants, value_types, op.arg(0).to_opref()); - sink.i64_const(0); - sink.i64_ne(); - sink.i64_extend_i32_u(); - sink.local_set(1 + vi); - } - } - OpCode::IntIsZero => { - let vi = op.pos.get().raw(); - if !OpRef::raw_is_constant(vi) { - emit_resolve(&mut sink, constants, value_types, op.arg(0).to_opref()); - sink.i64_eqz(); - sink.i64_extend_i32_u(); - sink.local_set(1 + vi); - } - } - // ── Extended integer ops ── OpCode::IntSignext => { // int_signext(val, num_bytes): sign-extend from num_bytes width @@ -2719,14 +2753,6 @@ fn build_function( } } - // ── Float comparisons ── - OpCode::FloatLt => emit_float_cmp(&mut sink, constants, value_types, op, FloatCmp::Lt), - OpCode::FloatLe => emit_float_cmp(&mut sink, constants, value_types, op, FloatCmp::Le), - OpCode::FloatEq => emit_float_cmp(&mut sink, constants, value_types, op, FloatCmp::Eq), - OpCode::FloatNe => emit_float_cmp(&mut sink, constants, value_types, op, FloatCmp::Ne), - OpCode::FloatGt => emit_float_cmp(&mut sink, constants, value_types, op, FloatCmp::Gt), - OpCode::FloatGe => emit_float_cmp(&mut sink, constants, value_types, op, FloatCmp::Ge), - // ── Float floor/mod ── OpCode::FloatFloorDiv => { let vi = op.pos.get().raw(); @@ -5128,6 +5154,66 @@ fn emit_guard_false( ); } +/// `llsupport/regalloc.py:873 next_op_can_accept_cc` — the comparison at `i` +/// may hand its condition straight to the op at `i + 1` instead of +/// materialising a boolean, when that op is the condition's only reader. x86 +/// leaves the condition in the flags (`x86/regalloc.py:265 +/// force_allocate_reg_or_cc`, ported to the dynasm sibling at +/// `majit-backend-dynasm/src/regalloc.rs:3665 next_op_can_accept_cc`); wasm's +/// operand stack plays that role — [`push_cond`]'s i32 stays on the stack and +/// the guard's `if` tests it, so the `i64.extend_i32_u`/`local.set` and the +/// guard's own `local.get`/re-test disappear. +/// +/// Narrower than the dynasm port on purpose: only `GuardTrue`/`GuardFalse`, +/// whose wasm arms do nothing but re-test the boolean. +fn next_op_can_accept_cc<'a>( + ops: &'a [Op], + i: usize, + result: OpRef, + liveness: &HomeLiveness, + label_resume: &LabelResumeData, + ref_homes: &RefHomes, +) -> Option<&'a Op> { + if result == OpRef::NONE || result.is_constant() { + return None; + } + let next_op = ops.get(i + 1)?; + if !matches!(next_op.opcode, OpCode::GuardTrue | OpCode::GuardFalse) { + return None; + } + // history.py:213 `Const.is_constant()` — a Const operand is not an + // op-result identity, so comparing raw positions against it is invalid. + if next_op.num_args() == 0 || next_op.arg(0).is_constant() { + return None; + } + if next_op.arg(0).to_opref().raw() != result.raw() { + return None; + } + // Any later reader (including this guard's own fail args, which + // `HomeLiveness` records as uses at `i + 1`) needs the materialised local. + if liveness.last_use(result.raw()) > i as i32 + 1 { + return None; + } + if next_op + .getfailargs() + .is_some_and(|fa| fa.iter().any(|a| a.to_opref() == result)) + { + return None; + } + // A LABEL resume loader restores its capture set from the frame, so a + // captured value must have been bound; skipping the `local.set` would leave + // wasm's zero-init in its place. + if label_resume.storage(result).is_some() { + return None; + } + // The store-on-def tail reads the result local for a Ref-homed value. A + // comparison result is never a Ref, so this only pins the invariant. + if ref_homes.home(result).is_some() { + return None; + } + Some(next_op) +} + /// Common guard exit: condition is on stack (i32), emit if + exit. /// /// `block_exit_depth` is the statement-level depth of the enclosing exit @@ -5428,6 +5514,7 @@ fn emit_ovf_binop( // ── Comparison ops ── +#[derive(Clone, Copy)] enum CmpOp { I64LtS, I64LeS, @@ -5478,6 +5565,7 @@ fn apply_cmp(sink: &mut InstructionSink<'_>, op: CmpOp) { // ── Float comparison helper ── +#[derive(Clone, Copy)] enum FloatCmp { Lt, Le, @@ -5487,57 +5575,110 @@ enum FloatCmp { Ge, } -fn emit_float_cmp( +/// An op whose result is a 0/1 boolean produced by a single wasm comparison. +/// [`push_cond`] leaves that comparison's i32 on the operand stack; [`emit_cond`] +/// is the ordinary spelling that widens and binds it to the result local. +#[derive(Clone, Copy)] +enum CondKind { + Int(CmpOp), + Float(FloatCmp), + IsTrue, + IsZero, +} + +fn cond_kind_of(opcode: OpCode) -> Option { + Some(match opcode { + // ── Integer comparisons (signed) ── + OpCode::IntLt => CondKind::Int(CmpOp::I64LtS), + OpCode::IntLe => CondKind::Int(CmpOp::I64LeS), + OpCode::IntEq => CondKind::Int(CmpOp::I64Eq), + OpCode::IntNe => CondKind::Int(CmpOp::I64Ne), + OpCode::IntGt => CondKind::Int(CmpOp::I64GtS), + OpCode::IntGe => CondKind::Int(CmpOp::I64GeS), + // ── Integer comparisons (unsigned) ── + OpCode::UintLt => CondKind::Int(CmpOp::I64LtU), + OpCode::UintLe => CondKind::Int(CmpOp::I64LeU), + OpCode::UintGt => CondKind::Int(CmpOp::I64GtU), + OpCode::UintGe => CondKind::Int(CmpOp::I64GeU), + // ── Pointer comparisons ── + OpCode::PtrEq | OpCode::InstancePtrEq => CondKind::Int(CmpOp::I64Eq), + OpCode::PtrNe | OpCode::InstancePtrNe => CondKind::Int(CmpOp::I64Ne), + // ── Float comparisons ── + OpCode::FloatLt => CondKind::Float(FloatCmp::Lt), + OpCode::FloatLe => CondKind::Float(FloatCmp::Le), + OpCode::FloatEq => CondKind::Float(FloatCmp::Eq), + OpCode::FloatNe => CondKind::Float(FloatCmp::Ne), + OpCode::FloatGt => CondKind::Float(FloatCmp::Gt), + OpCode::FloatGe => CondKind::Float(FloatCmp::Ge), + // ── Truth tests ── + OpCode::IntIsTrue => CondKind::IsTrue, + OpCode::IntIsZero => CondKind::IsZero, + _ => return None, + }) +} + +/// Push the comparison's i32 result (0 or 1) onto the operand stack. +fn push_cond( sink: &mut InstructionSink<'_>, constants: &indexmap::IndexMap, value_types: &[ValType], op: &Op, - cmp: FloatCmp, + kind: CondKind, ) { - let vi = op.pos.get().raw(); - if OpRef::raw_is_constant(vi) { - return; - } - emit_resolve_f64(sink, constants, value_types, op.arg(0).to_opref()); - emit_resolve_f64(sink, constants, value_types, op.arg(1).to_opref()); - match cmp { - FloatCmp::Lt => { - sink.f64_lt(); - } - FloatCmp::Le => { - sink.f64_le(); - } - FloatCmp::Eq => { - sink.f64_eq(); + match kind { + CondKind::Int(cmpop) => { + emit_resolve(sink, constants, value_types, op.arg(0).to_opref()); + emit_resolve(sink, constants, value_types, op.arg(1).to_opref()); + apply_cmp(sink, cmpop); } - FloatCmp::Ne => { - sink.f64_ne(); + CondKind::Float(cmp) => { + emit_resolve_f64(sink, constants, value_types, op.arg(0).to_opref()); + emit_resolve_f64(sink, constants, value_types, op.arg(1).to_opref()); + match cmp { + FloatCmp::Lt => { + sink.f64_lt(); + } + FloatCmp::Le => { + sink.f64_le(); + } + FloatCmp::Eq => { + sink.f64_eq(); + } + FloatCmp::Ne => { + sink.f64_ne(); + } + FloatCmp::Gt => { + sink.f64_gt(); + } + FloatCmp::Ge => { + sink.f64_ge(); + } + } } - FloatCmp::Gt => { - sink.f64_gt(); + CondKind::IsTrue => { + emit_resolve(sink, constants, value_types, op.arg(0).to_opref()); + sink.i64_const(0); + sink.i64_ne(); } - FloatCmp::Ge => { - sink.f64_ge(); + CondKind::IsZero => { + emit_resolve(sink, constants, value_types, op.arg(0).to_opref()); + sink.i64_eqz(); } } - sink.i64_extend_i32_u(); - sink.local_set(1 + vi); } -fn emit_cmp( +fn emit_cond( sink: &mut InstructionSink<'_>, constants: &indexmap::IndexMap, value_types: &[ValType], op: &Op, - cmpop: CmpOp, + kind: CondKind, ) { let vi = op.pos.get().raw(); if OpRef::raw_is_constant(vi) { return; } - emit_resolve(sink, constants, value_types, op.arg(0).to_opref()); - emit_resolve(sink, constants, value_types, op.arg(1).to_opref()); - apply_cmp(sink, cmpop); + push_cond(sink, constants, value_types, op, kind); sink.i64_extend_i32_u(); sink.local_set(1 + vi); } diff --git a/pyre/pyre-interpreter/src/baseobjspace.rs b/pyre/pyre-interpreter/src/baseobjspace.rs index bf455c379ab..8abe2df29d5 100644 --- a/pyre/pyre-interpreter/src/baseobjspace.rs +++ b/pyre/pyre-interpreter/src/baseobjspace.rs @@ -4921,12 +4921,12 @@ fn getdictvalue(obj: PyObjectRef, name: &str) -> Result, PyE // `("dict", SPECIAL)` wrapper and change the instance's map — see // [`setdictvalue`]. if unsafe { crate::objspace::std::mapdict::has_mapdict_storage(obj) } { - return Ok(unsafe { - crate::objspace::std::mapdict::instance_node_getdictvalue( + return unsafe { + crate::objspace::std::mapdict::instance_node_getdictvalue_checked( obj, rustpython_wtf8::Wtf8::new(name), ) - }); + }; } let w_dict = getdict_backing(obj)?; if w_dict.is_null() { @@ -5692,8 +5692,11 @@ fn getattr_str_impl(obj: PyObjectRef, name: &str, call_getattr: bool, suppress: // same `instance_node_getdictvalue`, so the value is identical and the // `__dict__` wrapper is built only on explicit `__dict__` access. let value = unsafe { - crate::objspace::std::mapdict::instance_node_getdictvalue(obj, Wtf8::new(name)) - }; + crate::objspace::std::mapdict::instance_node_getdictvalue_checked( + obj, + Wtf8::new(name), + ) + }?; if let Some(value) = value { return Ok(value); } @@ -6369,7 +6372,10 @@ pub fn object_getattribute(obj: PyObjectRef, name: &str) -> PyResult { // mapdict.py:846-847); a type receiver uses only its canonical // dictionary, which is the corresponding `getdictvalue` result. let value = if instance { - crate::objspace::std::mapdict::instance_node_getdictvalue(obj, Wtf8::new(name)) + crate::objspace::std::mapdict::instance_node_getdictvalue_checked( + obj, + Wtf8::new(name), + )? } else { crate::type_dict_lookup(obj, name) }; diff --git a/pyre/pyre-interpreter/src/module/signal/interp_signal.rs b/pyre/pyre-interpreter/src/module/signal/interp_signal.rs index 7656ffe3a10..4092cd5ec10 100644 --- a/pyre/pyre-interpreter/src/module/signal/interp_signal.rs +++ b/pyre/pyre-interpreter/src/module/signal/interp_signal.rs @@ -623,10 +623,19 @@ pub fn register_module(ns: pyre_object::PyObjectRef) { #[cfg(feature = "host_env")] { let signum = if let Some(&a) = args.first() { - unsafe { pyre_object::w_int_get_value(a) as i32 } + unsafe { pyre_object::w_int_get_value(a) } } else { return Err(crate::PyError::type_error("strsignal() missing argument")); }; + // interp_signal.py:593-594 spells this bound inline rather + // than calling `check_signum_in_range`, and its `signalnum + // > NSIG` admits `NSIG` itself. 3.14 rejects it — its own + // `pthread_sigmask` reports the range as `[1; NSIG - 1]` — + // so take the half-open bound the other entry points use: + // `strsignal(NSIG)` is `ValueError` here and + // `'Unknown signal: 32'` on pypy. + check_signum_in_range(signum)?; + let signum = signum as i32; return Ok(rustpython_host_env::signal::strsignal(signum) .map(|s| pyre_object::w_str_new(&s)) .unwrap_or(pyre_object::w_none())); @@ -855,7 +864,12 @@ pub fn register_module(ns: pyre_object::PyObjectRef) { "siginterrupt() requires 2 arguments", )); } - let sig = (unsafe { pyre_object::w_int_get_value(args[0]) }) as i32; + // interp_signal.py:388 — `check_signum_in_range` runs + // before the argument reaches `c_siginterrupt`, so the + // narrowing below is exact. + let sig = unsafe { pyre_object::w_int_get_value(args[0]) }; + check_signum_in_range(sig)?; + let sig = sig as i32; let flag = (unsafe { pyre_object::w_int_get_value(args[1]) }) as i32; rustpython_host_env::signal::siginterrupt(sig, flag).map_err(|e| { crate::PyError::os_error_with_errno( @@ -1077,7 +1091,12 @@ pub fn register_module(ns: pyre_object::PyObjectRef) { ) })?; for it in items { - let signum = (unsafe { pyre_object::w_int_get_value(it) }) as i32; + // interp_signal.py:492 — `SignalMask.__enter__` is + // shared with `sigwait` and range-checks every + // element before `c_sigaddset`. + let signum = unsafe { pyre_object::w_int_get_value(it) }; + check_signum_in_range(signum)?; + let signum = signum as i32; rustpython_host_env::signal::sigaddset(&mut set, signum).map_err( |e| { crate::PyError::os_error_with_errno( diff --git a/pyre/pyre-interpreter/src/objspace/std/mapdict.rs b/pyre/pyre-interpreter/src/objspace/std/mapdict.rs index 6eb96a1d8a8..beec68ac06b 100644 --- a/pyre/pyre-interpreter/src/objspace/std/mapdict.rs +++ b/pyre/pyre-interpreter/src/objspace/std/mapdict.rs @@ -666,11 +666,31 @@ pub unsafe fn map_is_devolved(map: MapRef) -> bool { /// `obj` must be a live `W_ObjectObject` (caller guards with `is_instance`). #[majit_macros::dont_look_inside] pub unsafe fn instance_node_getdictvalue(obj: PyObjectRef, name: &Wtf8) -> Option { + unsafe { instance_node_getdictvalue_checked(obj, name) }.unwrap_or(None) +} + +/// Fallible [`instance_node_getdictvalue`], for the callers that have an error +/// channel to propagate a raising `__eq__` on. +/// +/// Only the devolved terminator's dict probe can raise; the swallowing +/// spelling above is written in terms of this one and its `unwrap_or` consumes +/// the pending error slot, so a dropped error cannot surface on a later +/// operation. +/// +/// `dont_look_inside` for the same reason as [`instance_node_getdictvalue`]. +/// +/// # Safety +/// `obj` must be a live `W_ObjectObject` (caller guards with `is_instance`). +#[majit_macros::dont_look_inside] +pub unsafe fn instance_node_getdictvalue_checked( + obj: PyObjectRef, + name: &Wtf8, +) -> Result, PyError> { let _instance_guard = instance_lock(obj); ensure_mapdict_initialized(obj); let inst = &mut *(obj as *mut pyre_object::W_ObjectObject); let map = inst._get_mapdict_map(); - let w_res = unsafe { node_read(map, inst, name, DICT) }; + let w_res = unsafe { node_read_checked(map, inst, name, DICT) }; // mapdict.py:846-847 getdictvalue → read → _direct_read (592-598): lazily // migrate to boxed storage when the read attribute is unboxed and its class // has frozen unboxing. @@ -3092,18 +3112,43 @@ unsafe fn terminator_read( name: &Wtf8, attrkind: u16, ) -> Option { + unsafe { terminator_read_checked(term, obj, name, attrkind) }.unwrap_or(None) +} + +/// Fallible [`terminator_read`]. The devolved arm is the only one that can +/// raise, and `space.finditem_str` propagates there upstream; the swallowing +/// spelling above exists for the callers that have no error channel and is +/// written in terms of this one. Its `unwrap_or` also consumes the pending +/// error slot, so a dropped error cannot surface on a later operation. +/// +/// # Safety +/// `term` must point to a live Terminator map node. +unsafe fn terminator_read_checked( + term: MapRef, + obj: &O, + name: &Wtf8, + attrkind: u16, +) -> Result, PyError> { let t = unsafe { (*term).as_terminator() }; match t.kind { TerminatorKind::Devolved if attrkind == DICT => { // mapdict.py:383-388: the devolved terminator reads DICT attributes // from the materialised instance dict (`space.finditem_str( - // obj.getdict(space), name)`). + // obj.getdict(space), name)`). `finditem_str` is fallible: the + // probe compares against whatever the bucket holds, so a stored + // non-string key whose hash collides can reach a user `__eq__` + // that raises, and that must not read back as a miss. let w_dict = obj.getdict(); let backing = crate::type_methods::resolve_dict_backing(w_dict); - unsafe { pyre_object::w_dict_getitem_wtf8(backing, name) } + unsafe { pyre_object::dictmultiobject::w_dict_getitem_wtf8_checked(backing, name) } + .map_err(|_| { + crate::baseobjspace::take_pending_dict_key_error(pyre_object::w_str_from_wtf8( + name.to_wtf8_buf(), + )) + }) } // Terminator / DictTerminator / NoDictTerminator read nothing. - _ => None, + _ => Ok(None), } } @@ -3117,14 +3162,28 @@ pub unsafe fn node_read( name: &Wtf8, attrkind: u16, ) -> Option { + unsafe { node_read_checked(self_node, obj, name, attrkind) }.unwrap_or(None) +} + +/// Fallible [`node_read`], for the callers that can propagate the raising +/// `__eq__` a devolved terminator's dict probe may reach. +/// +/// # Safety +/// `self_node` and its chain must point to live map nodes. +pub unsafe fn node_read_checked( + self_node: MapRef, + obj: &O, + name: &Wtf8, + attrkind: u16, +) -> Result, PyError> { match unsafe { find_map_attr(self_node, name, attrkind) } { // The `jit.isconstant(attr) and jit.isconstant(obj) and not // attr.ever_mutated` guard selects `_pure_direct_read` // (mapdict.py:60-65). The PlainAttribute variants have the same body; // UnboxedPlainAttribute._direct_read's conversion tail lives in // `maybe_migrate_to_boxed`. - Some(attr) => Some(unsafe { plain_direct_read(attr, obj) }), - None => unsafe { terminator_read((*self_node).terminator(), obj, name, attrkind) }, + Some(attr) => Ok(Some(unsafe { plain_direct_read(attr, obj) })), + None => unsafe { terminator_read_checked((*self_node).terminator(), obj, name, attrkind) }, } } diff --git a/pyre/pyre-interpreter/src/pyframe.rs b/pyre/pyre-interpreter/src/pyframe.rs index 7eccedebeda..65f021eebb6 100644 --- a/pyre/pyre-interpreter/src/pyframe.rs +++ b/pyre/pyre-interpreter/src/pyframe.rs @@ -1389,6 +1389,12 @@ impl Default for FrameDebugData { } } +/// Byte offset of `w_locals` in `FrameDebugData`. +pub const FRAME_DEBUG_DATA_W_LOCALS_OFFSET: usize = std::mem::offset_of!(FrameDebugData, w_locals); + +/// Allocated size of a `FrameDebugData`. +pub const FRAME_DEBUG_DATA_SIZE: usize = std::mem::size_of::(); + /// pyopcode.py:1875-1897 FrameBlock — linked list node for the block stack. /// `previous` forms a singly-linked list; `lastblock` in PyFrame is the head. /// It is assigned only during construction and targets a strictly older node, @@ -4413,19 +4419,18 @@ fn delitem_str_object(w_obj: PyObjectRef, name: &str) -> Result<(), crate::PyErr } } -/// `pyframe.py:557 self.space.newdict(instance=True)` — the mapping half of -/// `fast2locals`, for a trace that models the fastlocals reads instead of -/// residualizing `interp_inspect.py:7-11 locals`. +/// `pyframe.py:557 self.space.newdict(instance=True)` — the mapping +/// `fast2locals` materialises for a frame that has none yet, for a trace that +/// models the fastlocals reads instead of residualizing +/// `interp_inspect.py:7-11 locals`. /// -/// Takes no `PyFrame`: the modelled expansion feeds the slot values in as -/// ordinary Ref operands, so nothing reachable from here can call -/// [`crate::executioncontext::force_frame`]. That is the whole point of the -/// split — a helper that touched the frame would re-arm the escape this -/// modelling exists to remove. The frame's own `w_locals` cache is -/// deliberately NOT populated: an OPTIMIZED frame hands out an independent -/// copy per read (`frame_locals_snapshot`), so the cache is never the object -/// application code sees, and a later `f_locals` rebuilds it from the -/// fastlocals anyway. +/// Takes no `PyFrame`, so nothing reachable from here can call +/// [`crate::executioncontext::force_frame`] — a helper that touched the frame +/// would re-arm the escape this modelling exists to remove. The store back +/// into `debugdata.w_locals` is therefore NOT modelled either: the caller +/// pins the frame's absent mapping with a guard and hands this dict out +/// directly, which for an OPTIMIZED frame is already the independent copy +/// `frame_locals_snapshot` returns. pub extern "C" fn jit_locals_dict_new() -> i64 { unsafe { pyre_object::w_dict_new() as i64 } } @@ -4433,9 +4438,13 @@ pub extern "C" fn jit_locals_dict_new() -> i64 { /// `pyframe.py:566-568 fast2locals` for ONE visible fastlocal slot: bind /// `code.varnames[index]` to `value` in `dict`. /// -/// Unbound slots are not routed here at all — the modelled expansion emits a -/// `guard_isnull` for them and skips the store, which is `fast2locals`' -/// `delitem` arm applied to a mapping that never held the key. +/// `dict` is the frame's own locals mapping — `getorcreatedebug().w_locals`, +/// which the modelled expansion reads through the `debugdata` virtualizable +/// field and hands in as an ordinary Ref operand. No `PyFrame` reaches this +/// helper, so nothing under it can call +/// [`crate::executioncontext::force_frame`]; that is the whole point of the +/// split, since a helper that touched the frame would re-arm the escape the +/// modelling exists to remove. /// /// Returns `dict` so the unrolled slot chain threads the (possibly forwarded) /// mapping from one store to the next instead of holding a raw address across @@ -4467,6 +4476,75 @@ pub extern "C" fn jit_locals_dict_setitem_local( pyre_object::gc_roots::shadow_stack_get(dict_slot) as i64 } +/// `pyframe.py:569-574 fast2locals` for ONE visible fastlocal slot that is +/// unbound: remove `code.varnames[index]` from `dict`. +/// +/// The delete is fallible for the same reason +/// [`crate::baseobjspace::delitem`] is — a stored key whose hash collides +/// with the varname can reach a user `__eq__` that raises — so it routes +/// through the checked spelling. A missing key is not an error here +/// (`w_dict_delitem_checked` reports it as `Ok(false)`), which is the +/// `KeyError` arm `delitem_str_object` swallows. Anything else is reported +/// as `PY_NULL` and the pending slot is drained, so the guarded side exit +/// re-runs the residual and raises from the eval loop. +/// +/// Returns `dict` on success, for the same threading reason as +/// [`jit_locals_dict_setitem_local`]. +/// +/// # Safety +/// `dict` must be a live dict and `code` a live `CodeObject` with +/// `index < varnames.len()`. +pub extern "C" fn jit_locals_dict_delitem_local(dict: i64, code: i64, index: i64) -> i64 { + let _roots = pyre_object::gc_roots::push_roots(); + let dict_slot = pyre_object::gc_roots::shadow_stack_len(); + pyre_object::gc_roots::pin_root(dict as PyObjectRef); + let code = unsafe { &*(code as usize as *const CodeObject) }; + let name: &str = &code.varnames[index as usize]; + let key_slot = pyre_object::gc_roots::shadow_stack_len(); + pyre_object::gc_roots::pin_root(unsafe { pyre_object::w_str_new(name) }); + let deleted = unsafe { + pyre_object::dictmultiobject::w_dict_delitem_checked( + pyre_object::gc_roots::shadow_stack_get(dict_slot), + pyre_object::gc_roots::shadow_stack_get(key_slot), + ) + }; + match deleted { + Ok(_) => pyre_object::gc_roots::shadow_stack_get(dict_slot) as i64, + Err(_) => { + let _ = crate::baseobjspace::take_pending_dict_key_error( + pyre_object::gc_roots::shadow_stack_get(key_slot), + ); + pyre_object::PY_NULL as i64 + } + } +} + +/// The copy half of [`PyFrame::frame_locals_snapshot`]: an INDEPENDENT dict +/// holding what the frame's own locals mapping holds (PEP 667), which is what +/// `locals()` / `vars()` hand back for an OPTIMIZED frame. +/// +/// Reports a failing copy as `PY_NULL` rather than publishing it, so the +/// guarded side exit re-runs the residual and raises from the eval loop. +/// +/// # Safety +/// `w_locals` must be a live mapping. +pub extern "C" fn jit_locals_dict_snapshot(w_locals: i64) -> i64 { + let _roots = pyre_object::gc_roots::push_roots(); + let locals_slot = pyre_object::gc_roots::shadow_stack_len(); + pyre_object::gc_roots::pin_root(w_locals as PyObjectRef); + let snapshot_slot = pyre_object::gc_roots::shadow_stack_len(); + pyre_object::gc_roots::pin_root(unsafe { pyre_object::w_dict_new() }); + // `dict_update_value` walks a mapping's `keys()`, so both sides are + // reloaded across it as well as across the `w_dict_new` above. + match crate::opcode_ops::dict_update_value( + pyre_object::gc_roots::shadow_stack_get(snapshot_slot), + pyre_object::gc_roots::shadow_stack_get(locals_slot), + ) { + Ok(()) => pyre_object::gc_roots::shadow_stack_get(snapshot_slot) as i64, + Err(_) => pyre_object::PY_NULL as i64, + } +} + #[cfg(test)] mod tests { use super::load_const_from_code; diff --git a/pyre/pyre-jit-trace/src/descr.rs b/pyre/pyre-jit-trace/src/descr.rs index 3b8f03474a1..0a5d6faaf11 100644 --- a/pyre/pyre-jit-trace/src/descr.rs +++ b/pyre/pyre-jit-trace/src/descr.rs @@ -1773,6 +1773,32 @@ static RBIGINT_PAIR_DESCR_GROUP: LazyLock = LazyLock::new( ) }); +// `pyframe.py:44 FrameDebugData.w_locals` — the frame's own locals mapping, +// reached as `self.getorcreatedebug().w_locals` at the head of `fast2locals` +// (pyframe.py:555-557). Not a PyObject — no vtable and no allocation type id, +// because the trace never NEWs one: it arrives as the `debugdata` +// virtualizable field and is only read. The field is MUTABLE (`setdictscope` +// and `fast2locals`' lazy materialisation both rebind it), so the read must +// not be treated as always-pure. +static FRAME_DEBUG_DATA_DESCR_GROUP: LazyLock = LazyLock::new(|| { + build_object_descr_group_with_def_path( + pyre_interpreter::pyframe::FRAME_DEBUG_DATA_SIZE, + 0, + 0, + &[( + "w_locals", + pyre_interpreter::pyframe::FRAME_DEBUG_DATA_W_LOCALS_OFFSET, + std::mem::size_of::(), + Type::Ref, + false, + false, + false, + )], + "FrameDebugData", + "pyframe::FrameDebugData", + ) +}); + // `pypy/objspace/std/sliceobject.py:13` `W_SliceObject._immutable_fields_ = // ['w_start', 'w_stop', 'w_step']` — all three Ref fields are immutable // once `__init__` runs. The `space.newslice(w_start, w_end, w_step)` JIT @@ -3063,6 +3089,12 @@ pub fn rbigint_pair_item1_descr() -> DescrRef { field_descr_from_group(&RBIGINT_PAIR_DESCR_GROUP, 1) } +/// `FrameDebugData.w_locals` — the mapping `getorcreatedebug().w_locals` +/// reads at the head of `fast2locals` (pyframe.py:555-557). +pub fn frame_debug_data_w_locals_descr() -> DescrRef { + field_descr_from_group(&FRAME_DEBUG_DATA_DESCR_GROUP, 0) +} + pub fn str_len_descr() -> DescrRef { // Python len(str) returns codepoint count. // unicodeobject.py:165 W_UnicodeObject._len() → _length field. diff --git a/pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs b/pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs index d5b139ae91f..8f1beb6cee5 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs @@ -6606,18 +6606,34 @@ enum FrameLocalsBuiltin { /// unconditionally on a detected force — so this removes the boundary and /// leaves the barrier live for every shape it declines. /// -/// Emitted shape, mirroring `pyframe.py:551-568` slot by slot: -/// `guard_value(callable)`, one `getarrayitem_vable_r(frame, ConstInt(i))` per -/// fastlocal (the same lowering `emit_load_fast_ref!` already emits for -/// LOAD_FAST), a `guard_isnull` / `guard_nonnull` pinning the slot's bound-ness, -/// and a plain non-forcing `Call` chain `newdict` → `setitem_str` per bound -/// slot. None of those ops can reach `force_frame`, so nothing arms the vable -/// protocol. +/// Emitted shape, mirroring `pyframe.py:555-574` line by line: +/// `guard_value(callable)`; the frame's own mapping, read as +/// `getorcreatedebug()` (the `debugdata` virtualizable field, answered from +/// `virtualizable_boxes`) followed by `getfield_gc_r(w_locals)` under a +/// non-null and exact-dict guard; one `getarrayitem_vable_r(frame, +/// ConstInt(i))` per fastlocal (the same lowering `emit_load_fast_ref!` +/// already emits for LOAD_FAST); a `guard_isnull` / `guard_nonnull` pinning +/// the slot's bound-ness; and a plain non-forcing `Call` per slot — +/// `setitem_str` when bound, `delitem` when not. None of those ops can reach +/// `force_frame`, so nothing arms the vable protocol. /// -/// `dir()` appends one further non-forcing `Call` to -/// `jit_dir_names_from_locals`, the split-out tail of `builtin_dir`'s -/// no-argument path, which turns that mapping into its sorted key set. It -/// takes the mapping and not the frame, so it too cannot reach `force_frame`. +/// The mapping is the FRAME's whenever the frame already carries one: +/// `fast2locals` rewrites only the varname keys, so a foreign key — one an +/// `f_locals` write put there (PEP 667) — survives every call, and an +/// expansion that always started from an empty dict would drop it for as long +/// as the loop stayed compiled. A frame that carries none keeps the empty +/// `newdict` (pyframe.py:557) the residual would have materialised, under a +/// `guard_isnull` that side-exits if one appears mid-loop; nothing else +/// references that dict, so it is already the independent copy +/// `frame_locals_snapshot` hands back and its `delitem` arm is a no-op. +/// +/// One further non-forcing `Call` turns that mapping into the published +/// result: `jit_locals_dict_snapshot` (the independent PEP 667 copy +/// `frame_locals_snapshot` builds) for `locals()` / `vars()`, and +/// `jit_dir_names_from_locals` (the split-out tail of `builtin_dir`'s +/// no-argument path, which reads `getdictscope` rather than the copy) for +/// `dir()`. Both take the mapping and not the frame, so they too cannot +/// reach `force_frame`. /// /// Returns `None` (fall through to the generic residual, SAFE — exactly /// today's behaviour) for every other shape: a rebound `locals` / `vars` / @@ -6625,8 +6641,9 @@ enum FrameLocalsBuiltin { /// a bound receiver, any argument, an inline sub-walk, a frame that is not the /// standard virtualizable the boxes describe, a hidden top frame, a /// non-OPTIMIZED (module / class / exec) frame, cellvars / freevars / -/// `CO_FAST_HIDDEN` slots, a slot the shadow cannot answer with a Ref, and a -/// frame wider than [`MAX_MODELLED_FASTLOCALS`]. +/// `CO_FAST_HIDDEN` slots, a slot the shadow cannot answer with a Ref, a frame +/// wider than [`MAX_MODELLED_FASTLOCALS`], a shadow whose mapping is not the +/// frame's, and a frame-owned mapping that is not an exact dict. pub(crate) fn try_walker_specialize_builtin_locals( ctx: &mut WalkContext<'_, '_, Sym>, code: &[u8], @@ -6717,6 +6734,63 @@ pub(crate) fn try_walker_specialize_builtin_locals( ) else { return Ok(None); }; + // `fast2locals` opens on `self.getorcreatedebug()` (pyframe.py:555) and + // writes into ITS `w_locals`: the mapping is the FRAME's, carried across + // calls, so a key written through `f_locals` outlives every `fast2locals` + // that does not name it. `debugdata` is a virtualizable field, so the read + // answers from `virtualizable_boxes` and records no op — exactly like the + // slot reads below — and the frame never becomes an operand, so nothing + // here can reach `force_frame`. + let Some((debugdata_op, majit_ir::Value::Ref(debugdata_ref))) = ctx + .trace_ctx + .virtualizable_entry_at(crate::virtualizable_spec::DEBUGDATA_VABLE_FIELD_INDEX) + else { + return Ok(None); + }; + // Read the mapping through the SHADOW's payload, which is what the emitted + // `getfield_gc_r` reads, and require it to be the one the residual would + // have used. The two payloads are not the same object: a root portal seed + // bakes the vable identity against the live frame but expands the shadow + // from the `snapshot_for_tracing` copy, whose `clone_debugdata_ptr` hands + // out a fresh `FrameDebugData` around the same `w_locals`. Comparing the + // holders would decline every portal trace; comparing the mapping is the + // invariant that actually has to hold. + let shadow_debugdata = + debugdata_ref.as_usize() as *const pyre_interpreter::pyframe::FrameDebugData; + let w_locals = if shadow_debugdata.is_null() { + pyre_object::PY_NULL + } else { + unsafe { (*shadow_debugdata).w_locals } + }; + if !std::ptr::eq(w_locals, frame_ref.get_w_locals()) { + return Ok(None); + } + // Two shapes, each pinned by a guard so the compiled loop side-exits when + // the frame moves to the other one: + // + // * the frame already carries its mapping — rewrite THAT, so a foreign key + // an `f_locals` write left in it survives, as it does across the + // residual's `fast2locals`; + // * the frame carries none — `fast2locals` would materialise an empty dict + // (pyframe.py:556-557 `d.w_locals = space.newdict()`) and fill it from + // the fastlocals, and the expansion builds exactly that dict instead of + // modelling the store. Nothing else references it, so it is already the + // independent copy `frame_locals_snapshot` would hand back, and a + // `delitem` on a key it never held is a no-op. + // + // The slot helpers are dict-keyed, so a frame-owned mapping that is not an + // exact dict declines. + let canonical_dict = pyre_object::get_instantiate(&pyre_object::pyobject::DICT_TYPE); + let frame_owned = !w_locals.is_null(); + if frame_owned + && (canonical_dict.is_null() + || !unsafe { + std::ptr::eq((*w_locals).ob_type, &pyre_object::pyobject::DICT_TYPE) + && std::ptr::eq((*w_locals).w_class, canonical_dict) + }) + { + return Ok(None); + } // Resolve every slot's shadow entry BEFORE emitting anything, so a slot the // shadow cannot answer declines from a clean trace position. The read is // the standard-virtualizable arm of `_opimpl_getarrayitem_vable` @@ -6753,11 +6827,38 @@ pub(crate) fn try_walker_specialize_builtin_locals( slots.push(value); } + // Which helper turns the mapping into the published result. `dir()` reads + // `getdictscope` — the mapping itself — through `builtin_dir`'s split-out + // sorted-key-set tail. `locals()` / `vars()` hand back + // `frame_locals_snapshot`'s independent PEP 667 copy, which a mapping the + // expansion just built for itself already is. + let tail_fn: Option i64> = match fold { + FrameLocalsBuiltin::Mapping if frame_owned => { + Some(pyre_interpreter::pyframe::jit_locals_dict_snapshot) + } + FrameLocalsBuiltin::Mapping => None, + FrameLocalsBuiltin::SortedNames => { + Some(pyre_interpreter::builtins::jit_dir_names_from_locals) + } + }; // Authentic mapping, built on the plain eval loop exactly as the skipped // residual would — through the SAME helpers the emitted calls invoke, so // the recording-time value and the compiled loop's value cannot diverge. - let (concrete_dict, concrete_result) = { + // On the frame-owned arm this MUTATES the frame's own mapping, which is + // exactly what the residual `fast2locals` does; the rewrite is a pure + // function of the fastlocals, so a decline below — or a discarded walk — + // leaves the residual free to redo it with the same outcome. + let (concrete_locals, concrete_result) = { let _roots = pyre_object::gc_roots::push_roots(); + let locals_root = pyre_object::gc_roots::shadow_stack_len(); + // Re-read rather than reuse the gate's `w_locals`: the slot resolution + // above sits between the two, so the pin takes the address the frame + // holds NOW. + pyre_object::gc_roots::pin_root(if frame_owned { + frame_ref.get_w_locals() + } else { + unsafe { pyre_object::w_dict_new() } + }); let value_roots: Vec = slots .iter() .map(|&value| { @@ -6766,40 +6867,54 @@ pub(crate) fn try_walker_specialize_builtin_locals( slot }) .collect(); - let dict_root = pyre_object::gc_roots::shadow_stack_len(); - pyre_object::gc_roots::pin_root( - pyre_interpreter::pyframe::jit_locals_dict_new() as pyre_object::PyObjectRef - ); + let mut result = pyre_object::PY_NULL; + let mut slot_failed = false; for (i, &value_root) in value_roots.iter().enumerate() { let value = pyre_object::gc_roots::shadow_stack_get(value_root); - if value.is_null() { - continue; + let locals = pyre_object::gc_roots::shadow_stack_get(locals_root) as i64; + // pyframe.py:566-574 — a bound slot is stored, an unbound one + // deleted. Both allocate, so the mapping is re-read from its + // pinned slot on every pass. A fresh mapping never held the key, + // so its `delitem` arm is skipped rather than emitted. + let updated = if !value.is_null() { + pyre_interpreter::pyframe::jit_locals_dict_setitem_local( + locals, + code_ptr as i64, + i as i64, + value as i64, + ) + } else if frame_owned { + pyre_interpreter::pyframe::jit_locals_dict_delitem_local( + locals, + code_ptr as i64, + i as i64, + ) + } else { + locals + }; + if (updated as pyre_object::PyObjectRef).is_null() { + slot_failed = true; + break; } - pyre_interpreter::pyframe::jit_locals_dict_setitem_local( - pyre_object::gc_roots::shadow_stack_get(dict_root) as i64, - code_ptr as i64, - i as i64, - value as i64, - ); } - // `dir()`'s tail runs here too, so the recorded result is produced by - // the very helper the emitted call names. It allocates, so the - // mapping is re-read from its pinned slot afterwards. - let result = match fold { - FrameLocalsBuiltin::Mapping => pyre_object::gc_roots::shadow_stack_get(dict_root), - FrameLocalsBuiltin::SortedNames => { - pyre_interpreter::builtins::jit_dir_names_from_locals( - pyre_object::gc_roots::shadow_stack_get(dict_root) as i64, - ) as pyre_object::PyObjectRef - } - }; - (pyre_object::gc_roots::shadow_stack_get(dict_root), result) + if !slot_failed { + let locals = pyre_object::gc_roots::shadow_stack_get(locals_root); + // The tail runs here too, so the recorded result is produced by the + // very helper the emitted call names. + result = match tail_fn { + Some(tail) => tail(locals as i64) as pyre_object::PyObjectRef, + None => locals, + }; + } + (pyre_object::gc_roots::shadow_stack_get(locals_root), result) }; - // The tail reports a failure as PY_NULL instead of publishing it; nothing - // has been emitted yet, so decline and let the residual raise. + // A slot rewrite or the tail reports a failure as PY_NULL instead of + // publishing it; nothing has been emitted yet, so decline and let the + // residual raise. if concrete_result.is_null() { return Ok(None); } + let concrete_locals_value = majit_ir::Value::Ref(majit_ir::GcRef(concrete_locals as usize)); // --- emit the specialized IR (walker-native) --- // Pin the callable identity (LOAD_GLOBAL `locals` is usually already a @@ -6814,23 +6929,78 @@ pub(crate) fn try_walker_specialize_builtin_locals( .heap_cache_mut() .replace_box(callable_op, expected); } - let concrete_dict_value = majit_ir::Value::Ref(majit_ir::GcRef(concrete_dict as usize)); // The code object is the jitdriver green this trace is keyed on // (`interp_jit.py:23 greens = ['next_instr', 'is_being_profiled', // 'pycode']`), so its address is a constant for the compiled loop and // carries no guard of its own. let code_const = ctx.trace_ctx.const_int(code_ptr as i64); - let mut dict_op = ctx.trace_ctx.call_ref_typed_with_effect( - pyre_interpreter::pyframe::jit_locals_dict_new as *const (), - &[], - &[], - majit_ir::EffectInfo::new( - majit_ir::ExtraEffect::CannotRaise, - majit_ir::OopSpecIndex::None, + // `d = self.getorcreatedebug()` — pyframe.py:555. An absent payload has no + // `w_locals` to read, so the guard pins that direction and the fresh-dict + // arm below stands in for the materialisation. + let debugdata_present = debugdata_ref.as_usize() != 0; + if !debugdata_op.is_constant() { + let opcode = if debugdata_present { + OpCode::GuardNonnull + } else { + OpCode::GuardIsnull + }; + walker_emit_fold_guard_with_snapshot(ctx, op.pc, opcode, &[debugdata_op])?; + } + // `d.w_locals` — pyframe.py:556. Read whenever there is a payload to read + // it from, and guarded in the direction recorded, so a frame that + // materialises its mapping mid-loop side-exits instead of going on writing + // into the expansion's own dict. + let mut field_op = None; + if debugdata_present { + let op_ref = crate::state::opimpl_getfield_gc_r( + ctx.trace_ctx, + debugdata_op, + crate::descr::frame_debug_data_w_locals_descr(), + ); + if !op_ref.is_constant() { + let opcode = if frame_owned { + OpCode::GuardNonnull + } else { + OpCode::GuardIsnull + }; + walker_emit_fold_guard_with_snapshot(ctx, op.pc, opcode, &[op_ref])?; + } + field_op = Some(op_ref); + } + let mut dict_op = match field_op.filter(|_| frame_owned) { + Some(op_ref) => op_ref, + // pyframe.py:557 `self.space.newdict(instance=True)` — the mapping + // `fast2locals` would have materialised, built here instead of + // modelling the store back into the debug payload. + None => ctx.trace_ctx.call_ref_typed_with_effect( + pyre_interpreter::pyframe::jit_locals_dict_new as *const (), + &[], + &[], + majit_ir::EffectInfo::new( + majit_ir::ExtraEffect::CannotRaise, + majit_ir::OopSpecIndex::None, + ), ), - ); + }; ctx.trace_ctx - .set_opref_concrete(dict_op, concrete_dict_value); + .set_opref_concrete(dict_op, concrete_locals_value); + if frame_owned { + walker_guard_class( + ctx, + op.pc, + dict_op, + &pyre_object::pyobject::DICT_TYPE as *const _ as i64, + )?; + walker_guard_exact_w_class( + ctx, + op.pc, + dict_op, + // Re-derived rather than reusing the gate's binding: the + // record-time rewrite above allocates, so this takes the address + // `dict` has NOW. + pyre_object::get_instantiate(&pyre_object::pyobject::DICT_TYPE), + )?; + } for (i, &value) in slots.iter().enumerate() { // `self.locals_cells_stack_w[i]` — `jtransform.py:1877 // do_fixed_list_getitem`, the identical lowering `emit_load_fast_ref!` @@ -6858,18 +7028,39 @@ pub(crate) fn try_walker_specialize_builtin_locals( }; walker_emit_fold_guard_with_snapshot(ctx, op.pc, opcode, &[slot_op])?; } - if !bound { + // `pyframe.py:566-574` — a bound slot is stored, an unbound one + // deleted. The delete is what keeps a key from a since-unbound local + // out of a mapping the frame carries across calls; on the fresh arm + // the mapping never held the key, so it is skipped. + if !bound && !frame_owned { continue; } + let (helper, args, arg_types): (_, Vec, Vec) = if bound { + ( + pyre_interpreter::pyframe::jit_locals_dict_setitem_local as *const (), + vec![dict_op, code_const, index_const, slot_op], + vec![ + majit_ir::Type::Ref, + majit_ir::Type::Int, + majit_ir::Type::Int, + majit_ir::Type::Ref, + ], + ) + } else { + ( + pyre_interpreter::pyframe::jit_locals_dict_delitem_local as *const (), + vec![dict_op, code_const, index_const], + vec![ + majit_ir::Type::Ref, + majit_ir::Type::Int, + majit_ir::Type::Int, + ], + ) + }; dict_op = ctx.trace_ctx.call_ref_typed_with_effect( - pyre_interpreter::pyframe::jit_locals_dict_setitem_local as *const (), - &[dict_op, code_const, index_const, slot_op], - &[ - majit_ir::Type::Ref, - majit_ir::Type::Int, - majit_ir::Type::Int, - majit_ir::Type::Ref, - ], + helper, + &args, + &arg_types, majit_ir::EffectInfo::new( majit_ir::ExtraEffect::CannotRaise, majit_ir::OopSpecIndex::None, @@ -6878,15 +7069,22 @@ pub(crate) fn try_walker_specialize_builtin_locals( // Every link of the chain names the SAME mapping, so the post-build // address is the live one for all of them. ctx.trace_ctx - .set_opref_concrete(dict_op, concrete_dict_value); - } - let result_op = match fold { - FrameLocalsBuiltin::Mapping => dict_op, - // `builtin_dir`'s no-argument tail, split out so the trace and the - // eval loop enumerate and sort one key set through one implementation. - FrameLocalsBuiltin::SortedNames => { - let names_op = ctx.trace_ctx.call_ref_typed_with_effect( - pyre_interpreter::builtins::jit_dir_names_from_locals as *const (), + .set_opref_concrete(dict_op, concrete_locals_value); + if !bound { + // The delete reports a raising comparison as PY_NULL instead of + // publishing it; side-exit so the residual re-runs and raises. + walker_emit_fold_guard_with_snapshot(ctx, op.pc, OpCode::GuardNonnull, &[dict_op])?; + } + } + // `frame_locals_snapshot`'s PEP 667 copy for `locals()` / `vars()`, or + // `builtin_dir`'s no-argument tail for `dir()` — each split out so the + // trace and the eval loop run one implementation. Both report a failure + // as PY_NULL instead of publishing it, so the guarded side exit re-runs + // the residual and raises from the eval loop. + let result_op = match tail_fn { + Some(tail) => { + let op_ref = ctx.trace_ctx.call_ref_typed_with_effect( + tail as *const (), &[dict_op], &[majit_ir::Type::Ref], majit_ir::EffectInfo::new( @@ -6895,14 +7093,13 @@ pub(crate) fn try_walker_specialize_builtin_locals( ), ); ctx.trace_ctx.set_opref_concrete( - names_op, + op_ref, majit_ir::Value::Ref(majit_ir::GcRef(concrete_result as usize)), ); - // PY_NULL is the tail's unpublished-error report; side-exit on it - // so the residual `dir()` re-runs and raises from the eval loop. - walker_emit_fold_guard_with_snapshot(ctx, op.pc, OpCode::GuardNonnull, &[names_op])?; - names_op + walker_emit_fold_guard_with_snapshot(ctx, op.pc, OpCode::GuardNonnull, &[op_ref])?; + op_ref } + None => dict_op, }; write_residual_call_result_to_dst(ctx, op.pc, dst, 'r', result_op)?; Ok(Some(())) diff --git a/pyre/pyre-jit-trace/src/virtualizable_spec.rs b/pyre/pyre-jit-trace/src/virtualizable_spec.rs index 0a4537e722d..4bd9397889e 100644 --- a/pyre/pyre-jit-trace/src/virtualizable_spec.rs +++ b/pyre/pyre-jit-trace/src/virtualizable_spec.rs @@ -50,6 +50,14 @@ pub const PYFRAME_VABLE_ARRAYS: &[(&str, usize)] = &[("locals_cells_stack_w", 0) /// index 0 and named `"locals_cells_stack_w"`. pub const LOCALS_CELLS_STACK_W_VABLE_ARRAY_INDEX: usize = 0; +/// Canonical vable-field index for `debugdata`. +/// +/// A static field's flat shadow index IS its position in +/// [`PYFRAME_VABLE_FIELDS`] (`initialize_virtualizable` lays the statics out +/// first, then the arrays, then the identity slot), so this doubles as the +/// `virtualizable_entry_at` index a reader of `getorcreatedebug()` uses. +pub const DEBUGDATA_VABLE_FIELD_INDEX: usize = 3; + const _: () = { assert!( !PYFRAME_VABLE_ARRAYS.is_empty(), @@ -77,4 +85,25 @@ const _: () = { ); i += 1; } + + assert!( + PYFRAME_VABLE_FIELDS[DEBUGDATA_VABLE_FIELD_INDEX].1 == DEBUGDATA_VABLE_FIELD_INDEX, + "debugdata must be registered at the expected vable field index" + ); + let name = PYFRAME_VABLE_FIELDS[DEBUGDATA_VABLE_FIELD_INDEX] + .0 + .as_bytes(); + let expected = b"debugdata"; + assert!( + name.len() == expected.len(), + "PYFRAME_VABLE_FIELDS[3] name mismatch" + ); + let mut i = 0; + while i < expected.len() { + assert!( + name[i] == expected[i], + "PYFRAME_VABLE_FIELDS[3] name mismatch" + ); + i += 1; + } }; diff --git a/pyre/pyre-object/src/dictmultiobject.rs b/pyre/pyre-object/src/dictmultiobject.rs index 5d4da0acc0e..534600d1b55 100644 --- a/pyre/pyre-object/src/dictmultiobject.rs +++ b/pyre/pyre-object/src/dictmultiobject.rs @@ -3445,6 +3445,31 @@ pub unsafe fn w_dict_getitem_wtf8( } } +/// Fallible sibling of [`w_dict_getitem_wtf8`]. +/// +/// The probe compares against whatever the bucket holds, so a stored +/// non-string key whose hash collides with `key` can reach a user `__eq__` +/// that raises. The unchecked spelling reports that as a miss; this one +/// surfaces it, and the caller recovers the concrete exception from the +/// interpreter-side error slot. +/// +/// Routes through [`w_dict_lookup_checked`] rather than draining the flag +/// after [`w_dict_getitem_wtf8`]: the strategy leaf +/// (`w_dict_lookup_object_strategy`) is itself +/// `..._checked(..).unwrap_or(None)`, so it has already taken the flag by the +/// time the unchecked spelling returns and a post-hoc `take_dict_key_error` +/// always reads `false`. +/// +/// # Safety +/// `obj` must point to a valid `W_DictObject`. +pub unsafe fn w_dict_getitem_wtf8_checked( + obj: PyObjectRef, + key: &rustpython_wtf8::Wtf8, +) -> Result, DictKeyError> { + let w_key = crate::w_str_from_wtf8(key.to_wtf8_buf()); + w_dict_lookup_checked(obj, w_key) +} + /// WTF-8 keyed equivalent of `space.setitem_str` — `setitem_str` is itself /// a fast path of `space.setitem`, so a key that is valid UTF-8 takes the /// str fast path (keeping an ASCII/Unicode dict on its strategy) and a diff --git a/pyre/pyre-object/src/listobject.rs b/pyre/pyre-object/src/listobject.rs index 364992ec786..786ab2c252e 100644 --- a/pyre/pyre-object/src/listobject.rs +++ b/pyre/pyre-object/src/listobject.rs @@ -1708,6 +1708,21 @@ pub unsafe fn w_list_init_items(obj: PyObjectRef, items: Vec) { // the allocating `build_list_storage` so the acquire cannot deadlock behind // a collection, and reload `obj` behind it because a contended acquire // blocks through `before_external_block`. + // The Object items block needs the same bracket as `obj`, and for the same + // reason: `build_list_storage` hands it back young, with no shadow-stack + // slot (`alloc_list_items_block_gc`'s `push_roots` scope ends at its + // return) and no heap edge until the store below, so a collection that runs + // while this thread sits in `before_external_block` sees it as garbage. + // `reload_typed_blocks` covers only the typed blocks -- `ListStorage` keeps + // a root slot for those two alone. Same pin/reload + // `w_list_new_with_strategy` puts around its header allocation. + let block_root: Option = if storage.block.is_null() { + None + } else { + let s = crate::gc_roots::shadow_stack_len(); + crate::gc_roots::pin_root(storage.block as PyObjectRef); + Some(s) + }; let _list_guard = w_list_lock(obj); let obj = crate::gc_roots::shadow_stack_get(obj_slot); let list = &mut *(obj as *mut W_ListObject); @@ -1716,6 +1731,9 @@ pub unsafe fn w_list_init_items(obj: PyObjectRef, items: Vec) { // bracket only once it is behind them (`IntArray::install`). list.drop_object_items(); storage.reload_typed_blocks(); + if let Some(s) = block_root { + storage.block = crate::gc_roots::shadow_stack_get(s) as *mut ItemsBlock; + } list.length = storage.length; list.items = storage.block; list.strategy = strategy;