diff --git a/majit/majit-backend-dynasm/src/x86/assembler.rs b/majit/majit-backend-dynasm/src/x86/assembler.rs index 1fa72a501a5..6b9f604b150 100644 --- a/majit/majit-backend-dynasm/src/x86/assembler.rs +++ b/majit/majit-backend-dynasm/src/x86/assembler.rs @@ -101,6 +101,46 @@ mod tests { use crate::runner::DynasmBackend; + /// x86/regalloc.py `consider_call_malloc_nursery` binds the result with + /// `force_allocate_reg(op, selected_reg=ecx)`; only FrameManager may spill + /// it later. Storing it into a slot here as well grew every recursive + /// loop's JitFrame by one slot per allocation, which changes how much the + /// process allocates and so shifts the minor-collection schedule the + /// jitcounter decay is driven by. + #[test] + fn malloc_nursery_result_does_not_grow_frame_depth() { + let mut backend = DynasmBackend::new(); + backend.attach_default_test_descrs(); + + let malloc = Rc::new(Op::new( + OpCode::CallMallocNursery, + &[Operand::from_opref(OpRef::const_int(32))], + )); + malloc.pos.set(OpRef::ref_op(0)); + + let finish = Op::new(OpCode::Finish, &[Operand::from_bound_op(&malloc)]); + finish.pos.set(OpRef::void_op(1)); + finish.set_fail_arg_types(vec![Type::Ref]); + finish.setfailargs(vec![].into()); + + let token = JitCellToken::new(517); + backend + .compile_loop(&[], &[malloc, Rc::new(finish)], &token) + .expect("compile register-resident nursery result trace"); + + let compiled = token + .compiled + .get() + .expect("compiled code") + .downcast_ref::() + .expect("dynasm compiled code"); + assert_eq!( + compiled.frame_depth.load(Ordering::Acquire), + super::JITFRAME_FIXED_SIZE, + "a register-resident nursery result must not allocate a shadow frame slot", + ); + } + fn compile_eval_breaker_poll_trace( trace_id: u64, word_addr: usize, @@ -4190,7 +4230,7 @@ impl<'a> Assembler386<'a> { // store next GUARD_NOT_FORCED's descr ptr to jf_force_descr // BEFORE the call, so forcing code knows which guard to resume. self._store_force_index_if_next_guard(ops, op_index, fail_index); - self.genop_call_assembler(op, arglocs); + self.genop_call_assembler(op, arglocs, result_loc); } OpCode::CondCallN => self.genop_discard_cond_call(op, arglocs), OpCode::CondCallValueI | OpCode::CondCallValueR => { @@ -4315,20 +4355,10 @@ impl<'a> Assembler386<'a> { dynasm!(self.mc ; .arch x64 ; mov Rq(rv), rcx); } dynasm!(self.mc ; .arch x64 ; =>done); - // Spill the result to the regalloc-assigned jitframe slot - // directly from `result_reg`. The malloc-nursery clobber - // set is only ECX/EDX (SAVE_DEFAULT_REGS), so RAX may hold - // a live Box the regalloc bound to it across this op; - // routing the spill through RAX (`mov rax, result_reg`) - // silently destroyed that value — this mirrors the - // fixed-size `genop_call_malloc_nursery` final block, which - // was already corrected for the same reason. - let pos = op.pos.get(); - if !pos.is_none() { - let slot = self.allocate_slot(pos); - let offset = Self::slot_offset(slot); - dynasm!(self.mc ; .arch x64 ; mov [rbp + offset], Rq(rv)); - } + // The payload already sits in `result_reg` on both paths, and + // that register is the delivery contract (see + // `genop_call_malloc_nursery`); an extra store grew + // `frame_depth` by one slot per allocation. } // x86/assembler.py malloc_cond_varsize parity // arglocs = [lengthloc, imm(itemsize), imm(kind)] @@ -4392,8 +4422,20 @@ impl<'a> Assembler386<'a> { // result slot. self.emit_propagate_exception_if_zero(0); dynasm!(self.mc ; .arch x64 ; mov QWORD [rbp + gcmap_ofs], 0); - if !op.pos.get().is_none() { - self.store_rax_to_result(op.pos.get()); + // Unlike the other three nursery paths, this one leaves the + // helper's return in RAX rather than in the regalloc result + // register, so the move is emitted here instead of the store + // that used to grow `frame_depth` by a slot per allocation. + // `consider_call_malloc_nursery_varsize` forces the result to + // `MALLOC_NURSERY_RESULT`, so this is never a no-op. + let Some(Loc::Reg(r)) = result_loc else { + panic!( + "CallMallocNurseryVarsize result_loc must be a register; got {result_loc:?}" + ); + }; + if r.value != crate::regloc::EAX.value { + let rv = r.value; + dynasm!(self.mc ; .arch x64 ; mov Rq(rv), rax); } } // x86/assembler.py `genop_discard_check_memory_error` @@ -6999,7 +7041,7 @@ impl<'a> Assembler386<'a> { /// `reload_frame_if_necessary` because a minor GC during the callee /// may have moved the caller jitframe; the popped rbp is the /// pre-GC address while the shadow stack carries the updated one. - fn genop_call_assembler(&mut self, op: &Op, arglocs: &[Loc]) { + fn genop_call_assembler(&mut self, op: &Op, arglocs: &[Loc], result_loc: Option<&Loc>) { // handle_call_assembler (rewrite.py) always pre-builds the // callee jitframe — storing every inputarg, and for a virtualizable // passing the forced vable object as the second arg — so the backend @@ -7042,9 +7084,7 @@ impl<'a> Assembler386<'a> { } else { dynasm!(self.mc ; .arch x64 ; xor eax, eax); } - if !op.pos.get().is_none() { - self.store_rax_to_result(op.pos.get()); - } + self.move_call_assembler_result(result_type, result_loc); return; } @@ -7119,8 +7159,36 @@ impl<'a> Assembler386<'a> { ; =>merge ); } - if !op.pos.get().is_none() { - self.store_rax_to_result(op.pos.get()); + self.move_call_assembler_result(result_type, result_loc); + } + + /// Materialize a CALL_ASSEMBLER result from the raw bits both paths leave + /// in RAX into the regalloc-assigned location. + /// + /// The previous shape spilled RAX to a fresh JitFrame slot, which grew + /// `frame_depth` by one slot per call and left a `Float` result taking the + /// helper path in RAX while the regalloc expected XMM0. Only the fast path + /// happened to leave it in XMM0 as a side effect of its `movq rax, xmm0` + /// normalisation. + /// + /// x86/regalloc.py `_consider_call_assembler` binds the result through + /// `after_call`, so it is `eax` for an int or ref and `xmm0` for a float; + /// the move is elided when the value already sits there. + fn move_call_assembler_result(&mut self, result_type: Type, result_loc: Option<&Loc>) { + match (result_type, result_loc) { + (Type::Void, None) => {} + (Type::Float, Some(Loc::Reg(r))) if r.is_xmm => { + dynasm!(self.mc ; .arch x64 ; movq Rx(r.value), rax); + } + (_, Some(Loc::Reg(r))) if !r.is_xmm => { + if r.value != crate::regloc::EAX.value { + dynasm!(self.mc ; .arch x64 ; mov Rq(r.value), rax); + } + } + _ => panic!( + "CALL_ASSEMBLER result must use its regalloc result register: \ + type={result_type:?} loc={result_loc:?}" + ), } } @@ -7495,25 +7563,11 @@ impl<'a> Assembler386<'a> { let _ = gcmap_ofs; dynasm!(self.mc ; .arch x64 ; =>done); - // Spill the result to the regalloc-assigned jitframe slot. Stage - // it directly from `result_reg`; routing through RAX (the previous - // shape) silently clobbered any live Box the regalloc bound to - // RAX across this op, since the malloc-nursery clobber set is - // only ECX/EDX. - let pos = op.pos.get(); - if !pos.is_none() { - let slot = self.allocate_slot(pos); - let offset = Self::slot_offset(slot); - // malloc_cond / malloc_cond_varsize (assembler.py:2556,2604) keep - // the allocated pointer in ecx and route it through the regalloc- - // assigned `result_reg`. Anything else here would spill a stale - // RAX (now caller-live) instead of the object pointer. - let Some(Loc::Reg(r)) = result_loc else { - panic!("CallMallocNursery result_loc must be a register; got {result_loc:?}"); - }; - let rv = r.value; - dynasm!(self.mc ; .arch x64 ; mov [rbp + offset], Rq(rv)); - } + // x86/regalloc.py `consider_call_malloc_nursery` binds the result with + // `force_allocate_reg(op, selected_reg=ecx)`, so the register IS the + // delivery contract; FrameManager emits a spill only where a later + // lifetime boundary needs one. Storing it here as well grew + // `frame_depth` by one slot for every allocation. } /// Headerless fixed-size nursery allocation. `size` excludes any GC @@ -7599,18 +7653,9 @@ impl<'a> Assembler386<'a> { let _ = gcmap_ofs; dynasm!(self.mc ; .arch x64 ; =>done); - let pos = op.pos.get(); - if !pos.is_none() { - let slot = self.allocate_slot(pos); - let offset = Self::slot_offset(slot); - let Some(Loc::Reg(r)) = result_loc else { - panic!( - "CallMallocNurseryHeaderless result_loc must be a register; got {result_loc:?}" - ); - }; - let rv = r.value; - dynasm!(self.mc ; .arch x64 ; mov [rbp + offset], Rq(rv)); - } + // The result register is the delivery contract (see + // `genop_call_malloc_nursery`); an extra store here grew `frame_depth` + // by one slot per allocation. } /// NEW: allocate a fixed-size object. Requires GC runtime. diff --git a/majit/majit-translate/src/codewriter/jitcode.rs b/majit/majit-translate/src/codewriter/jitcode.rs index b266e7deeb4..3185c98a5ee 100644 --- a/majit/majit-translate/src/codewriter/jitcode.rs +++ b/majit/majit-translate/src/codewriter/jitcode.rs @@ -205,10 +205,11 @@ pub struct JitCode { /// so an answer already computed for the original holds for it too. #[derive(Debug, Default, Clone)] pub struct DerivedBodyFacts { - /// Whether descending into this body can reach a residual call whose - /// funcbox is an un-lowered helper's symbolic hash. Read through - /// [`JitCode::descent_reaches_unlowered_helper_call`]. - descent_reaches_unlowered_helper_call: OnceLock, + /// The un-lowered helper a descent into this body can reach, named by + /// the symbolic hash standing in for its funcbox, or `None` when the + /// descent reaches no such call. Read through + /// [`JitCode::descent_unlowered_helper_blocker`]. + descent_unlowered_helper_blocker: OnceLock>, } mod oncelock_usize_serde { @@ -312,16 +313,20 @@ impl JitCode { .expect("JitCode body not yet set — call set_body() before body_mut()") } - /// Whether descending into this body can reach a residual call whose - /// funcbox is an un-lowered helper's symbolic hash, computing the answer - /// with `compute` the first time it is asked. The property is fixed by the - /// assembled body, so the first answer is the only one this instance gives: - /// `body_mut` needs `&mut self`, which `runtime_fnaddr_patch` can only take - /// before the jitcode is published behind an `Arc`. - pub fn descent_reaches_unlowered_helper_call(&self, compute: impl FnOnce() -> bool) -> bool { + /// The un-lowered helper a descent into this body can reach, named by + /// the symbolic hash standing in for its funcbox, computing the answer + /// with `compute` the first time it is asked. `None` means the descent + /// reaches no such call. The property is fixed by the assembled body, so + /// the first answer is the only one this instance gives: `body_mut` needs + /// `&mut self`, which `runtime_fnaddr_patch` can only take before the + /// jitcode is published behind an `Arc`. + pub fn descent_unlowered_helper_blocker( + &self, + compute: impl FnOnce() -> Option, + ) -> Option { *self .derived - .descent_reaches_unlowered_helper_call + .descent_unlowered_helper_blocker .get_or_init(compute) } diff --git a/majit/majit-translate/src/front/mir.rs b/majit/majit-translate/src/front/mir.rs index 03e79f0e018..7cd7e458aa2 100644 --- a/majit/majit-translate/src/front/mir.rs +++ b/majit/majit-translate/src/front/mir.rs @@ -2213,6 +2213,10 @@ fn lower_unstructured_with_static_addrs_and_attrs( tail_forwarded_returns, ) .map_err(LowerError::Unsupported)?; + // Fold each raise site's `PyError` constructor into its + // materialisation call, so the transparent constructor — which has + // no host symbol and therefore no address — leaves this graph. + crate::front::result_exc::fuse_kind_ctor_raise(&mut lo.graph); if result_exc_ok_is_unit { // Stamp `FUNC.RESULT = void`. The exception-link lowering // already returns the unit `()` (the callee no longer diff --git a/majit/majit-translate/src/front/result_exc.rs b/majit/majit-translate/src/front/result_exc.rs index 886e516d17e..13d84e651b0 100644 --- a/majit/majit-translate/src/front/result_exc.rs +++ b/majit/majit-translate/src/front/result_exc.rs @@ -583,7 +583,18 @@ pub(crate) fn lower_result_exc_returns( .push_op_var( block_id, OpKind::Call { - target: CallTarget::method("to_exc_object", Some("PyError".to_string())), + // The free-function spelling, not the method: it is + // `dont_look_inside` and its address is published, so + // the raise site records one residual call instead of + // inlining the materialisation body — whose GC-root, + // `w_exception_new_empty_impl`, WTF-8 and allocation + // calls would otherwise sit in this JitCode and refuse + // every descent that reaches it. + target: CallTarget::FunctionPath { + segments: ["pyre_interpreter", "error", "pyerror_to_exc_object"] + .map(str::to_string) + .to_vec(), + }, args: vec![payload], result_ty: ValueType::Ref(None), }, @@ -2771,3 +2782,212 @@ pub(crate) fn collapse_pos0_read( block.exits = exits; Ok(Some(payload_ty)) } + +/// The one-argument `PyError` constructors this pass fuses, and the published +/// helper each fuses into. +/// +/// Measured over the 301 distinct gateway wrappers: `type_error` is the only +/// constructor that reaches a raise site, 681 occurrences, all of them in the +/// shape below. Extending the table is a one-line change plus its helper. +const FUSED_KIND_CTORS: &[(&str, &str)] = &[("type_error", "pyerror_type_error_to_exc_object")]; + +/// Fuse `PyError::(msg)` and the `pyerror_to_exc_object` that consumes +/// it into a single published call. +/// +/// [`lower_result_exc_returns`] leaves each raise site as a constructor in one +/// block feeding a materialisation in its successor. The constructor is +/// `PyError::new` once inlined — a transparent constructor with no host +/// symbol, so it can never be given an address, and the descent that reaches +/// it is refused. Rewriting the pair to one opaque call removes it from the +/// caller's JitCode altogether. +/// +/// The shape required, all of it verified before anything is mutated: +/// +/// ```text +/// pred: v_msg = (may be several blocks back) +/// v_err = PyError::(v_msg) +/// -> succ, args[pos] = v_err (pred's only exit) +/// succ: inputargs[pos] = v_payload (pred is succ's only predecessor) +/// v_exc = pyerror_to_exc_object(v_payload) (succ's only operation) +/// -> exceptblock, args = [v_exc, v_exc] +/// ``` +/// +/// `v_msg` must resolve to a string literal: the helper reads it as a +/// `W_UnicodeObject`, which is what a literal's one-word `r` constant +/// materialises to, and a runtime-built message need not be one. +pub(crate) fn fuse_kind_ctor_raise(graph: &mut FunctionGraph) { + // (pred, ctor op index, helper leaf, succ, payload position) + let mut fusions: Vec<(usize, usize, &'static str, usize, usize)> = Vec::new(); + for si in 0..graph.blocks.len() { + let succ = &graph.blocks[si]; + // `succ` holds nothing but the materialisation, and raises its result. + let [op] = succ.operations.as_slice() else { + continue; + }; + let OpKind::Call { + target: CallTarget::FunctionPath { segments }, + args, + .. + } = &op.kind + else { + continue; + }; + if segments.last().map(String::as_str) != Some("pyerror_to_exc_object") { + continue; + } + let ([v_payload], Some(v_exc)) = (args.as_slice(), op.result.as_ref()) else { + continue; + }; + let [exit] = succ.exits.as_slice() else { + continue; + }; + if exit.target != graph.exceptblock + || !exit + .args + .iter() + .all(|a| matches!(a, LinkArg::Value(v) if v == v_exc)) + { + continue; + } + let Some(pos) = succ.inputargs.iter().position(|v| v == v_payload) else { + continue; + }; + // Exactly one predecessor, reaching `succ` by exactly one exit: any + // other producer of the payload would keep its own constructor. + let [pred_id] = graph.predecessors(BlockId(si))[..] else { + continue; + }; + let pi = pred_id.0; + let pred = &graph.blocks[pi]; + let [pred_exit] = pred.exits.as_slice() else { + continue; + }; + let Some(LinkArg::Value(v_err)) = pred_exit.args.get(pos) else { + continue; + }; + // The constructor, and the guarantee that the forwarding exit is the + // only thing that reads it — a second reader wants a real `PyError`. + let Some(ctor_idx) = pred.operations.iter().position(|o| { + o.result.as_ref() == Some(v_err) && matches!(&o.kind, OpKind::Call { .. }) + }) else { + continue; + }; + let OpKind::Call { + target: CallTarget::FunctionPath { segments }, + args, + .. + } = &pred.operations[ctor_idx].kind + else { + continue; + }; + let n = segments.len(); + if n < 2 || segments[n - 2] != "PyError" { + continue; + } + let Some((_, helper)) = FUSED_KIND_CTORS.iter().find(|(c, _)| *c == segments[n - 1]) else { + continue; + }; + let [v_msg] = args.as_slice() else { + continue; + }; + let uses = count_var_uses(graph, v_err); + if uses.op_uses != 0 || uses.link_uses != 1 { + continue; + } + // The helper reads the message as a `W_UnicodeObject`. + if !message_is_str_literal(graph, pi, ctor_idx, v_msg) { + continue; + } + fusions.push((pi, ctor_idx, helper, si, pos)); + } + for &(pi, ctor_idx, helper, si, pos) in &fusions { + if let OpKind::Call { target, .. } = &mut graph.blocks[pi].operations[ctor_idx].kind { + *target = CallTarget::FunctionPath { + segments: ["pyre_interpreter", "error", helper] + .map(str::to_string) + .to_vec(), + }; + } + // The constructor's result variable now holds the exception object, so + // the value already forwarded into `succ` is what the raise link wants. + let payload = graph.blocks[si].inputargs[pos].clone(); + graph.blocks[si].operations.clear(); + let raise_arity = graph.blocks[si].exits[0].args.len(); + graph.blocks[si].exits[0].args = vec![LinkArg::Value(payload); raise_arity]; + } +} + +/// Whether `var`, read in `block` before operation `before`, is a string +/// literal on every path that reaches that read. +/// +/// [`box_str_const_fold::dominating_literal`](crate::translator::rtyper::box_str_const_fold) +/// answers the neighbouring question — *which* literal — and so accepts only +/// straight-line control flow. A raise site's message routinely arrives at a +/// merge block instead, where the paths carry different literals; the fusion +/// does not need to know which one, only that every one of them is a literal, +/// because that is what makes the word a `box_str_constant` object. +/// +/// A value that is neither produced nor an input in the block it is read from +/// is looked for in the predecessors unrenamed, mirroring the walk above; the +/// entry block has none, so a parameter is not a literal and stops the proof. +#[expect( + clippy::mutable_key_type, + reason = "Eq and Hash use immutable identity/value data; interior mutation is excluded, matching RPython identity-keyed dict semantics" +)] +fn message_is_str_literal( + graph: &FunctionGraph, + block: usize, + before: usize, + var: &Variable, +) -> bool { + let mut work = vec![(block, before, var.clone())]; + let mut seen: std::collections::HashSet<(usize, Variable)> = std::collections::HashSet::new(); + while let Some((bi, before, value)) = work.pop() { + if !seen.insert((bi, value.clone())) { + continue; + } + let block = &graph.blocks[bi]; + if let Some(producer) = block.operations[..before] + .iter() + .rev() + .find(|op| op.result.as_ref() == Some(&value)) + { + if crate::translator::rtyper::box_str_const_fold::str_literal_bytes(&producer.kind) + .is_none() + { + return false; + } + continue; + } + let slot = block.inputargs.iter().position(|a| *a == value); + let predecessors = graph.predecessors(BlockId(bi)); + if predecessors.is_empty() { + return false; + } + for pred in predecessors { + let pb = &graph.blocks[pred.0]; + let incoming = match slot { + None => vec![value.clone()], + Some(slot) => { + let mut vs = Vec::new(); + for link in pb.exits.iter().filter(|l| l.target == BlockId(bi)) { + let Some(LinkArg::Value(v)) = link.args.get(slot) else { + return false; + }; + vs.push(v.clone()); + } + if vs.is_empty() { + return false; + } + vs + } + }; + work.extend( + incoming + .into_iter() + .map(|v| (pred.0, pb.operations.len(), v)), + ); + } + } + true +} diff --git a/majit/majit-translate/src/translator/rtyper/box_str_const_fold.rs b/majit/majit-translate/src/translator/rtyper/box_str_const_fold.rs index 2e213ea9a57..7e41e836911 100644 --- a/majit/majit-translate/src/translator/rtyper/box_str_const_fold.rs +++ b/majit/majit-translate/src/translator/rtyper/box_str_const_fold.rs @@ -30,6 +30,25 @@ fn is_box_str_constant_call(kind: &OpKind) -> Option<&Variable> { .then_some(arg) } +/// The bytes of a string literal, in either spelling it can have. +/// +/// [`str_const_fold::fold_str_consts`](crate::translator::rtyper::str_const_fold::fold_str_consts) +/// rewrites the front's synthetic `__str_const` call to [`OpKind::ConstStr`], +/// but it runs in the codewriter — a front pass still sees the call. +pub(crate) fn str_literal_bytes(kind: &OpKind) -> Option> { + match kind { + OpKind::ConstStr(bytes) => Some(bytes.clone()), + OpKind::Call { + target: CallTarget::FunctionPath { segments }, + args, + .. + } if args.is_empty() && segments.len() == 2 && segments[0] == "__str_const" => { + Some(segments[1].as_bytes().to_vec()) + } + _ => None, + } +} + /// Resolve `value` to a string literal that dominates its use. /// /// The walk accepts only straight-line control flow. At a block input it also @@ -59,10 +78,7 @@ fn dominating_literal( .rev() .find(|op| op.result.as_ref() == Some(&value)) { - return match &producer.kind { - OpKind::ConstStr(bytes) => Some(bytes.clone()), - _ => None, - }; + return str_literal_bytes(&producer.kind); } if let Some(slot) = block.inputargs.iter().position(|arg| arg == &value) { @@ -167,6 +183,33 @@ mod tests { assert_eq!(folded.kind, OpKind::ConstStr(b"__instancecheck__".to_vec())); } + /// The other arm of [`str_literal_bytes`]. A front pass sees the literal + /// as an unlowered `__str_const` call, because `fold_str_consts` runs later + /// in the codewriter — so this fold must not depend on having run it, which + /// the test above cannot show because it runs it first. + #[test] + fn folds_an_unlowered_str_const_call() { + let mut graph = FunctionGraph::new("box_literal_front"); + let entry = graph.startblock; + let literal = graph + .push_op_var(entry, str_const_call("__instancecheck__"), true) + .expect("string literal must produce a value"); + let boxed = graph + .push_op_var(entry, box_str_constant_call(literal.clone()), true) + .expect("box call must produce a value"); + + fold_box_str_constants(&mut graph); + + // Only the box call folds; its producer is left for the codewriter. + assert_eq!( + graph.block(entry).operations[0].kind, + str_const_call("__instancecheck__") + ); + let folded = &graph.block(entry).operations[1]; + assert_eq!(folded.result.as_ref(), Some(&boxed)); + assert_eq!(folded.kind, OpKind::ConstStr(b"__instancecheck__".to_vec())); + } + #[test] fn leaves_dynamic_argument_call_unchanged() { let mut graph = FunctionGraph::new("box_dynamic"); diff --git a/majit/majit-translate/tests/test_result_exc_lowering.rs b/majit/majit-translate/tests/test_result_exc_lowering.rs index b64b3fbfca4..65496c2f128 100644 --- a/majit/majit-translate/tests/test_result_exc_lowering.rs +++ b/majit/majit-translate/tests/test_result_exc_lowering.rs @@ -84,10 +84,16 @@ fn pop_value_lowers_to_raise_links() { } if owner_path.last().map(String::as_str) == Some("Result") => { result_ctors += 1; } + // The raise site reaches the materialisation through the + // published free function, not the method: its body must stay + // opaque to the codewriter so the GC-root, exception-object + // and WTF-8 machinery under it does not land in this graph. OpKind::Call { - target: CallTarget::Method { name, .. }, + target: CallTarget::FunctionPath { segments }, .. - } if name == "to_exc_object" => to_exc_object_calls += 1, + } if segments.last().map(String::as_str) == Some("pyerror_to_exc_object") => { + to_exc_object_calls += 1 + } _ => {} } } @@ -322,3 +328,72 @@ fn eval_loop_custom_match_gets_catch_and_rewrap() { "rewrap exception arm binds PyError::from_exc_object(last_exc_value)" ); } + +/// Count the raise-path calls in `name`'s lowered graph: fused, unfused +/// materialisations, and surviving `PyError` constructors. +fn raise_path_calls(name: &str) -> (usize, usize, usize) { + let graph = lower_function(interp(), name).unwrap_or_else(|e| panic!("lower {name}: {e:?}")); + let (mut fused, mut materialise, mut ctors) = (0, 0, 0); + for block in &graph.blocks { + for op in &block.operations { + let OpKind::Call { + target: CallTarget::FunctionPath { segments }, + .. + } = &op.kind + else { + continue; + }; + match segments.last().map(String::as_str) { + Some("pyerror_type_error_to_exc_object") => fused += 1, + Some("pyerror_to_exc_object") => materialise += 1, + Some(_) if segments.len() >= 2 && segments[segments.len() - 2] == "PyError" => { + ctors += 1 + } + _ => {} + } + } + } + (fused, materialise, ctors) +} + +#[test] +fn constant_message_raise_sites_fuse_their_constructor() { + // A gateway wrapper's receiver and arity checks each raise a TypeError + // with a literal message. Every one of them must reach the published + // `pyerror_type_error_to_exc_object`, leaving no `PyError` constructor + // behind: the constructor is transparent, has no host symbol, and one of + // them anywhere in the body refuses the whole descent. + let (fused, materialise, ctors) = raise_path_calls("__pyre_wrap_random"); + assert!(fused > 0, "the fusion must fire on a gateway wrapper"); + assert_eq!(ctors, 0, "no PyError constructor may survive"); + assert_eq!(materialise, 0, "no unfused materialisation may survive"); +} + +#[test] +fn formatted_message_raise_sites_keep_the_two_call_form() { + // `__class_getitem__`'s checks build their message with `format!`, whose + // result is not the `box_str_constant` object the helper reads. Those + // sites must keep the constructor plus `pyerror_to_exc_object`: the + // fusion is additive and never replaces its own fallback. + let (fused, materialise, ctors) = raise_path_calls("__pyre_wrap___class_getitem__"); + assert_eq!(fused, 0, "a formatted message must not fuse"); + assert!( + ctors > 0, + "the fixture must still raise through a constructor" + ); + assert_eq!( + materialise, ctors, + "every declined site keeps both halves of the pair" + ); +} + +#[test] +fn pop_value_keeps_its_unfused_materialisation() { + // `pop_value` raises through `shared_opcode::stack_underflow_error`, not + // a `PyError` constructor, so there is nothing to fuse and the single + // materialisation call stands. + assert_eq!( + raise_path_calls("pyre_interpreter::eval::::pop_value"), + (0, 1, 0) + ); +} diff --git a/pyre/bench/synth/float_subclass_binop_dispatch.cranelift.jitstats b/pyre/bench/synth/float_subclass_binop_dispatch.cranelift.jitstats index 6b29b86656c..9cb62b5b198 100644 --- a/pyre/bench/synth/float_subclass_binop_dispatch.cranelift.jitstats +++ b/pyre/bench/synth/float_subclass_binop_dispatch.cranelift.jitstats @@ -8,7 +8,8 @@ fbw_rolled_back_with_effects=0 fbw_store_journal_rollback_failed=0 field_pos_attached_misplaced=0 field_pos_spec_misplaced=0 -guard_failures=9 +guard_failures=17 internal_compile_panics=0 loops_aborted=0 -loops_compiled=9 +loops_compiled=17 +retraces_compiled=0 diff --git a/pyre/bench/synth/float_subclass_binop_dispatch.dynasm.jitstats b/pyre/bench/synth/float_subclass_binop_dispatch.dynasm.jitstats index 6b29b86656c..9cb62b5b198 100644 --- a/pyre/bench/synth/float_subclass_binop_dispatch.dynasm.jitstats +++ b/pyre/bench/synth/float_subclass_binop_dispatch.dynasm.jitstats @@ -8,7 +8,8 @@ fbw_rolled_back_with_effects=0 fbw_store_journal_rollback_failed=0 field_pos_attached_misplaced=0 field_pos_spec_misplaced=0 -guard_failures=9 +guard_failures=17 internal_compile_panics=0 loops_aborted=0 -loops_compiled=9 +loops_compiled=17 +retraces_compiled=0 diff --git a/pyre/bench/synth/float_subclass_binop_dispatch.py b/pyre/bench/synth/float_subclass_binop_dispatch.py index 93b9107134e..eda73f5d90e 100644 --- a/pyre/bench/synth/float_subclass_binop_dispatch.py +++ b/pyre/bench/synth/float_subclass_binop_dispatch.py @@ -12,8 +12,18 @@ # Without it every line below silently loses the override and prints the raw # IEEE result. # -# The int subclass at the bottom is the control: the int specialization has -# carried that exactness test all along, so it must stay correct either way. +# The int subclass at the bottom is the control for the record-time gate: when +# the subclass is present from the first iteration, the gate sees it on the +# recorded operand and declines, and every fold below stayed correct on that +# shape alone. +# +# That shape is not sufficient. The `warm_then_swap_*` cases below compile the +# trace from EXACT builtins first and introduce the subclass afterwards, so the +# gate never sees it and only the emitted guard can reject it. `compare_op_int`, +# `compare_op_float`, `store_subscr`, `newlist` and the `store_attr` in-place +# arm each emitted the `ob_type` unbox guard without the matching `w_class` pin +# and answered these with the raw payload -- `a < 1` returning True where the +# override returns a string, and a stored subclass reading back as a plain int. N = 20000 @@ -126,3 +136,98 @@ def int_control_hot(n): print(eq_hot(N)) print(mixed_int_operand_hot(N)) print(int_control_hot(N)) + + +# --- warm on the exact builtin, then swap in the subclass ------------------- +# The list has no branch on the element, so the compiled trace can only reject +# the tail element through a type guard. Each function prints what the override +# says; a fold missing its `w_class` pin prints the raw builtin answer instead. +class LiarInt(int): + def __lt__(self, other): + return "LT" + + +class LiarFloat(float): + def __lt__(self, other): + return "FLT" + + +class Slotted: + __slots__ = ("x",) + + +class LiarBool(int): + def __bool__(self): + return True + + +def warm_then_swap_compare_int(n): + out = None + for a in [0] * n + [LiarInt(0)]: + out = a < 1 + return out + + +def warm_then_swap_compare_float(n): + out = None + for a in [0.0] * n + [LiarFloat(0.0)]: + out = a < 1.0 + return out + + +def warm_then_swap_store_subscr(n): + lst = [0] + for a in [0] * n + [LiarInt(7)]: + lst[0] = a + return type(lst[0]).__name__ + + +def warm_then_swap_newlist(n): + out = None + for a in [0] * n + [LiarInt(7)]: + out = [a] + return type(out[0]).__name__ + + +def warm_then_swap_store_attr(n): + holder = Slotted() + for a in [0] * n + [LiarInt(7)]: + holder.x = a + return type(holder.x).__name__ + + +# `truth_int` reaches the same hole from the branch side rather than the value +# side: `POP_JUMP_IF_*` and the short-circuit operators read the truth of a +# payload the `GUARD_CLASS INT` admits, so a `__bool__` override on a zero-payload +# subclass is skipped and the branch is taken the wrong way. `bool(a)` does not +# reach the fold and stays correct either way, so it is the control. +def warm_then_swap_truth_if(n): + hits = 0 + for a in [0] * n + [LiarBool(0)]: + if a: + hits += 1 + return hits + + +def warm_then_swap_truth_and(n): + out = None + for a in [0] * n + [LiarBool(0)]: + out = a and "yes" + return out + + +def truth_bool_call_control(n): + out = None + for a in [0] * n + [LiarBool(0)]: + out = bool(a) + return out + + +print(warm_then_swap_compare_int(N)) +print(warm_then_swap_compare_float(N)) +print(warm_then_swap_store_subscr(N)) +print(warm_then_swap_newlist(N)) +print(warm_then_swap_store_attr(N)) +print(warm_then_swap_truth_if(N)) +print(warm_then_swap_truth_and(N)) +print(truth_bool_call_control(N)) diff --git a/pyre/bench/synth/float_subclass_binop_dispatch.wasm.jitstats b/pyre/bench/synth/float_subclass_binop_dispatch.wasm.jitstats index 6b29b86656c..9cb62b5b198 100644 --- a/pyre/bench/synth/float_subclass_binop_dispatch.wasm.jitstats +++ b/pyre/bench/synth/float_subclass_binop_dispatch.wasm.jitstats @@ -8,7 +8,8 @@ fbw_rolled_back_with_effects=0 fbw_store_journal_rollback_failed=0 field_pos_attached_misplaced=0 field_pos_spec_misplaced=0 -guard_failures=9 +guard_failures=17 internal_compile_panics=0 loops_aborted=0 -loops_compiled=9 +loops_compiled=17 +retraces_compiled=0 diff --git a/pyre/bench/synth/generator_tree_recursion.py b/pyre/bench/synth/generator_tree_recursion.py index e14e5a2bf09..c3a19d886ac 100644 --- a/pyre/bench/synth/generator_tree_recursion.py +++ b/pyre/bench/synth/generator_tree_recursion.py @@ -3,11 +3,12 @@ # Jitcounter decay is 0.96 every 32 minor collections # (majit-trace/src/counter.rs), so guard_failures tracks collection count during # each guard's warm-up rather than a compile decision. One host measured -# 2951..2958 across nursery sizes; PYRE_JIT=decay=0 pinned 2999 everywhere, -# while loops_compiled=3 and bridges_compiled=26 stayed invariant and remain -# gated exactly. Width 8 adds one count of margin (0.27%); real regressions this -# gate caught moved by hundreds to thousands (828 -> 4923, 404 -> 812, -# 937 -> 7408). +# 3648..3661 across nursery sizes; PYRE_JIT=decay=0 pinned 3600 everywhere, +# while loops_compiled=3 and bridges_compiled=29 stayed invariant and remain +# gated exactly. The fixture sets decay=0 itself, so the band covers the pinned +# run, not that 13-wide unpinned spread; width 8 is margin (0.22%). Real +# regressions this gate caught moved by hundreds to thousands (828 -> 4923, +# 404 -> 812, 937 -> 7408). # Generator-driven accumulation over recursive tree/linear results. The # tree_sum recursion once silently miscompiled on cranelift (first checksum # already wrong) and recovered a regalloc panic on dynasm. Deterministic; @@ -17,12 +18,13 @@ # `decay` (rlib/jit.py:588, default 40) scales every JitCounter entry down, and # counter.py:104-121 applies that scaling once per 32 minor collections. How far # a guard's counter has advanced by the time the workload reaches it therefore -# depends on how much the process has allocated so far, and for this recursion -# that is a per-ISA quantity: x86 spills every CALL_ASSEMBLER result into a fresh -# JITFRAME slot (`x86/assembler.rs:7319`, `:7396` -> `allocate_slot`) where -# aarch64 keeps it in the regalloc result register (`aarch64/assembler.rs:6037`), -# so the two dynasm backends run a different minor-collection schedule over the -# same trace. Left at the default, `guard_failures` reads 2957/2955/2951 across a +# depends on how much the process has allocated so far, which is why the pin +# exists: anything that shifts allocation volume shifts every counter. Both +# dynasm backends now deliver a CALL_ASSEMBLER or nursery result into the +# regalloc result register rather than a JitFrame slot +# (`move_call_assembler_result`, and `consider_call_malloc_nursery`'s +# `force_allocate_reg`), so neither grows the frame per call the way x86 did. +# Left at the default, `guard_failures` reads 3661/3648/3648 across a # 1MB/4MB/16MB nursery sweep on one binary with `loops_compiled` and # `bridges_compiled` unchanged; at 0 the same sweep reads one number. # diff --git a/pyre/extra_tests/parity_tests/numeric_binary_subclass_specialization.py b/pyre/extra_tests/parity_tests/numeric_binary_subclass_specialization.py index ba7876c8ff4..20df3427605 100644 --- a/pyre/extra_tests/parity_tests/numeric_binary_subclass_specialization.py +++ b/pyre/extra_tests/parity_tests/numeric_binary_subclass_specialization.py @@ -109,4 +109,109 @@ def tail(hot_value, subclass_value): assert rhs_add(tail(True, IntOperand(3))) == ("radd", 7, 3) assert rhs_floordiv(tail(True, IntOperand(3))) == ("rfloordiv", 7, 3) + +# ── `__float__` coercion at a hot site ────────────────────────────────────── +# The float coercions are the same exact-class question one layer down. Reading +# an int's payload is the answer only for an exact builtin: `PyNumber_Float` +# (the `float()` constructor) and `PyFloat_AsDouble` (the `math` entry points) +# both dispatch `nb_float` for a non-float, so a subclass override decides. +# The layout predicates read `ob_type`, which the subclass shares, so both the +# interpreter fast path and the trace guard admitted it and read the payload. +import math + + +class ToFloat(int): + def __float__(self): + return 99.0 + + +class ToFloatFromFloat(float): + def __float__(self): + return 99.0 + + +def coerce_hot(operands, fn): + out = None + for operand in operands: + out = fn(operand) + return out + + +# `float()` honors the override for both bases: `PyNumber_Float` short-circuits +# only `PyFloat_CheckExact`. +assert coerce_hot(tail(4, ToFloat(4)), float) == 99.0 +assert coerce_hot(tail(4.0, ToFloatFromFloat(4.0)), float) == 99.0 + +# `math` uses `PyFloat_AsDouble`, whose short-circuit is `PyFloat_Check` — so a +# FLOAT subclass keeps its payload while an INT subclass takes the override. +assert coerce_hot(tail(4, ToFloat(4)), math.sqrt) == math.sqrt(99.0) +assert coerce_hot(tail(4.0, ToFloatFromFloat(4.0)), math.sqrt) == 2.0 +assert coerce_hot(tail(4, ToFloat(4)), math.cos) == math.cos(99.0) +assert coerce_hot(tail(4, ToFloat(4)), math.frexp) == math.frexp(99.0) +assert coerce_hot(tail(4, ToFloat(4)), lambda a: math.ldexp(a, 1)) == 198.0 + +# `complex()` reaches the same real-number ladder. +assert coerce_hot(tail(4, ToFloat(4)), complex) == complex(99.0, 0.0) + +# A float presentation code formats the `PyNumber_Float` conversion. +assert coerce_hot(tail(4, ToFloat(4)), lambda a: format(a, ".2f")) == "99.00" + +# `loghelper` is the exception that proves the rule: it converts EVERY +# `PyLong_Check` operand from its payload, subclass included, for both the +# argument and the base. Routing either through the general coercion would +# answer a different logarithm. +assert coerce_hot(tail(4, ToFloat(4)), math.log) == math.log(4) +assert math.log(100, ToFloat(10)) == 2.0 + +# `__index__`-based entry points are likewise payload-only. +assert coerce_hot(tail(16, ToFloat(16)), math.isqrt) == 4 +assert coerce_hot(tail(0, ToFloat(0)), lambda a: math.ldexp(1.0, a)) == 1.0 + + +# ── the subclass that does NOT override ───────────────────────────────────── +# Every gate above sends a strict subclass to a `__float__` lookup. When it +# overrides nothing, that lookup resolves to the INHERITED `int.__float__` / +# `float.__float__`, which convert the receiver's payload. Binding either name +# to its constructor instead makes this case re-enter the lookup that reached +# it, so these asserts are what separates a payload-only dunder from one. +class Plain(int): + pass + + +class PlainFloat(float): + pass + + +assert float(Plain(7)) == 7.0 +assert type(float(Plain(7))) is float +assert Plain(7).__float__() == 7.0 +assert int.__float__(Plain(7)) == 7.0 +assert float(PlainFloat(4.5)) == 4.5 +assert type(float(PlainFloat(4.5))) is float + +assert coerce_hot(tail(4, Plain(4)), float) == 4.0 +assert coerce_hot(tail(4, Plain(4)), math.sqrt) == 2.0 +assert coerce_hot(tail(4, Plain(4)), math.frexp) == math.frexp(4.0) +assert coerce_hot(tail(4, Plain(4)), complex) == complex(4.0, 0.0) +assert coerce_hot(tail(4, Plain(4)), lambda a: format(a, ".2f")) == "4.00" +assert coerce_hot(tail(4.0, PlainFloat(4.0)), math.sqrt) == 2.0 +assert math.log(100, Plain(10)) == 2.0 + +# `int.__float__` converts a payload rather than constructing, so a receiver of +# the wrong layout is a descriptor error, not a conversion. +try: + int.__float__("x") +except TypeError as exc: + assert "requires a 'int' object" in str(exc), exc +else: + raise AssertionError("int.__float__ accepted a str") + +# `rbigint.tofloat()` raises rather than answering inf. +try: + (1 << 2000).__float__() +except OverflowError as exc: + assert "too large" in str(exc), exc +else: + raise AssertionError("no OverflowError for an out-of-range int") + print("OK") diff --git a/pyre/pyre-interpreter/src/baseobjspace.rs b/pyre/pyre-interpreter/src/baseobjspace.rs index 9dbffa44dfd..97582666084 100644 --- a/pyre/pyre-interpreter/src/baseobjspace.rs +++ b/pyre/pyre-interpreter/src/baseobjspace.rs @@ -14045,29 +14045,39 @@ pub fn float_w(obj: PyObjectRef) -> Result { if pyre_object::is_float(obj) { return Ok(pyre_object::w_float_get_value(obj)); } - // `is_int` is true for a bool (`BOOL_TYPE`), so test `is_bool` first. - if pyre_object::pyobject::is_bool(obj) { - return Ok(if pyre_object::boolobject::w_bool_get_value(obj) { - 1.0 - } else { - 0.0 - }); - } - if pyre_object::pyobject::is_int(obj) { - return Ok(pyre_object::intobject::w_int_get_value(obj) as f64); - } - if pyre_object::pyobject::is_long(obj) { - use num_traits::ToPrimitive; - // longobject.py `tofloat` — `rbigint.tofloat()` raises - // OverflowError "int too large to convert to float" when the - // value does not fit a C double. - let f = pyre_object::longobject::jit_bigint_to_f64_or_inf( - pyre_object::longobject::w_long_get_value(obj), - ); - if !f.is_finite() { - return Err(PyError::overflow_error("int too large to convert to float")); + // The integer fast paths read the payload, which is the answer only + // when the operand's Python class is the builtin itself: a strict + // subclass may override `__float__`, and the layout predicates read + // `ob_type`, which the subclass shares. A subclass falls through to + // the lookup below, where an inherited `int.__float__` reproduces the + // same payload. The `float` arm above is deliberately not gated: the + // conversion short-circuits on the float layout and never consults an + // override. + if pyre_object::is_exact_builtin_instance(obj) { + // `is_int` is true for a bool (`BOOL_TYPE`), so test `is_bool` first. + if pyre_object::pyobject::is_bool(obj) { + return Ok(if pyre_object::boolobject::w_bool_get_value(obj) { + 1.0 + } else { + 0.0 + }); + } + if pyre_object::pyobject::is_int(obj) { + return Ok(pyre_object::intobject::w_int_get_value(obj) as f64); + } + if pyre_object::pyobject::is_long(obj) { + use num_traits::ToPrimitive; + // longobject.py `tofloat` — `rbigint.tofloat()` raises + // OverflowError "int too large to convert to float" when the + // value does not fit a C double. + let f = pyre_object::longobject::jit_bigint_to_f64_or_inf( + pyre_object::longobject::w_long_get_value(obj), + ); + if !f.is_finite() { + return Err(PyError::overflow_error("int too large to convert to float")); + } + return Ok(f); } - return Ok(f); } } let Some(method) = (unsafe { lookup(obj, "__float__") }) else { diff --git a/pyre/pyre-interpreter/src/builtins.rs b/pyre/pyre-interpreter/src/builtins.rs index cf4f9f83b77..560ef92eb9a 100644 --- a/pyre/pyre-interpreter/src/builtins.rs +++ b/pyre/pyre-interpreter/src/builtins.rs @@ -10430,39 +10430,54 @@ pub(crate) fn builtin_float_dunder(args: &[PyObjectRef]) -> Result Result { - let obj = args[0]; +/// The `int` payload as a double, or `None` when `obj` is not an int by layout. +/// +/// Reads the payload of a strict subclass too. `PyLong_AsDouble` is a layout +/// read, and an inherited `int.__float__` has to reproduce it: the exactness +/// gates on the coercion entry points send a subclass to a `__float__` lookup, +/// and this is what that lookup resolves to. +pub(crate) fn int_payload_as_f64(obj: PyObjectRef) -> Option> { unsafe { + // `is_int` is true for a bool (`BOOL_TYPE`), so test `is_bool` first. if is_bool(obj) { - return Ok(floatobject::w_float_new(if w_bool_get_value(obj) { - 1.0 - } else { - 0.0 - })); + return Some(Ok(if w_bool_get_value(obj) { 1.0 } else { 0.0 })); } if is_int(obj) { - return Ok(floatobject::w_float_new(w_int_get_value(obj) as f64)); + return Some(Ok(w_int_get_value(obj) as f64)); } if pyre_object::is_long(obj) { + // `rbigint.tofloat()` raises when the value does not fit a double. let v = pyre_object::jit_bigint_to_f64_or_nan(pyre_object::w_long_get_value(obj)); if !v.is_finite() { - return Err(crate::PyError::overflow_error( + return Some(Err(crate::PyError::overflow_error( "int too large to convert to float", - )); + ))); } - return Ok(floatobject::w_float_new(v)); + return Some(Ok(v)); } } - Err(crate::PyError::type_error(format!( - "descriptor '__float__' requires an 'int' object but received a '{}'", - crate::type_methods::arg_type_name(obj) - ))) + None +} + +/// `int.__float__(self)` — longobject.py `descr___float__`, which converts the +/// receiver's payload and never re-dispatches. `bool` inherits it. +/// +/// Kept separate from the `float()` constructor for the reason +/// [`builtin_float_dunder`] gives: the constructor looks `__float__` up on a +/// strict subclass so an override is honored, so binding this name to the +/// constructor makes a subclass that does *not* override it re-enter the very +/// lookup that reached here. +pub(crate) fn builtin_int_float_dunder( + args: &[PyObjectRef], +) -> Result { + let obj = args[0]; + match int_payload_as_f64(obj) { + Some(value) => Ok(floatobject::w_float_new(value?)), + None => Err(crate::PyError::type_error(format!( + "descriptor '__float__' requires a 'int' object but received a '{}'", + crate::type_methods::arg_type_name(obj) + ))), + } } /// `float(obj)` → convert to float @@ -18877,10 +18892,12 @@ pub(crate) fn complex_coerce(obj: PyObjectRef) -> Result<(f64, f64), crate::PyEr if is_bool(obj) { return Ok((w_bool_get_value(obj) as i64 as f64, 0.0)); } - if is_int(obj) { + if is_int(obj) && is_exact_builtin_instance(obj) { return Ok((w_int_get_value(obj) as f64, 0.0)); } if is_long(obj) { + // `float_w` applies the same exactness rule to the payload arms, + // so a strict subclass overriding `__float__` is honored there. return Ok((crate::baseobjspace::float_w(obj)?, 0.0)); } if is_float(obj) { diff --git a/pyre/pyre-interpreter/src/error.rs b/pyre/pyre-interpreter/src/error.rs index 30908f16095..171a5cbc276 100644 --- a/pyre/pyre-interpreter/src/error.rs +++ b/pyre/pyre-interpreter/src/error.rs @@ -425,6 +425,56 @@ pub struct PyError { /// traceback, or context state. pub type OperationError = PyError; +/// [`PyError::to_exc_object`] behind the residual-call ABI: +/// one pointer in, one pointer out, and a body the codewriter does not +/// look inside. +/// +/// The lowered raise path emits this spelling instead of the method +/// (`front/result_exc.rs`'s callee rule), so materialising the exception +/// keeps its shadow-stack roots, `w_exception_new_empty_impl`, and the +/// WTF-8 and allocation machinery under it out of the caller's JitCode. +/// A jitcode that merely *can* raise otherwise carries all of it, and one +/// un-lowered call anywhere in that body refuses the whole descent. +/// +/// The pointer spellings are load-bearing: the call-target trampoline +/// reads word-sized arguments and cannot see through a reference or the +/// `PyObjectRef` alias. +/// +/// # Safety +/// `err` must point to a live `PyError` no other borrow aliases. +#[majit_macros::dont_look_inside] +pub unsafe fn pyerror_to_exc_object(err: *mut PyError) -> *mut pyre_object::PyObject { + unsafe { (*err).to_exc_object() } +} + +/// [`PyError::type_error`] and [`PyError::to_exc_object`] fused into one +/// residual call, so a raise site carries neither body. +/// +/// The two-call form leaves `PyError::new` in the caller's JitCode as a +/// transparent constructor with no host symbol, and every descent that +/// reaches it is refused. Fusing them removes the constructor from the +/// caller entirely: the front rule +/// (`front/result_exc.rs::fuse_kind_ctor_raise`) rewrites the +/// `PyError::type_error` call to this one and drops the successor's +/// `pyerror_to_exc_object`. +/// +/// `w_msg` is the message *object*, not a `&str`: the JIT models a Rust +/// string constant as a single `W_UnicodeObject` word, while `&str` is a +/// two-word aggregate with no one-word residual-call ABI — the reason +/// `stack_underflow_error` stays unpublished (`jit_fnaddr.rs`). The front +/// rule only fires where the message resolves to a string literal, which is +/// what makes that word a `box_str_constant` object. +/// +/// # Safety +/// `w_msg` must be a live `W_UnicodeObject`. +#[majit_macros::dont_look_inside] +pub unsafe fn pyerror_type_error_to_exc_object( + w_msg: *mut pyre_object::PyObject, +) -> *mut pyre_object::PyObject { + let msg = unsafe { pyre_object::unicodeobject::w_str_get_wtf8(w_msg) }.to_owned(); + PyError::type_error(msg).to_exc_object() +} + impl PyError { /// Forward the up-to-three GC-managed references a `PyError` holds — the /// cached exception object and the lazy NameError/AttributeError name/obj diff --git a/pyre/pyre-interpreter/src/jit_fnaddr.rs b/pyre/pyre-interpreter/src/jit_fnaddr.rs index 0a4c0587b5b..6650d6d6925 100644 --- a/pyre/pyre-interpreter/src/jit_fnaddr.rs +++ b/pyre/pyre-interpreter/src/jit_fnaddr.rs @@ -1975,6 +1975,28 @@ pub fn jit_trace_fnaddrs() -> Vec<(&'static str, i64)> { "pyre_object::jit_range_iter_new", pyre_object::jit_range_iter_new as *const (), ); + // The lowered raise path's exception materialisation, opaque so that its + // body stays out of every JitCode that can raise. + let pyerror_to_exc_object: extern "C" fn(i64) -> i64 = + crate::error::__majit_call_target_pyerror_to_exc_object; + push_alias_pair( + &mut entries, + "pyre_interpreter::error::pyerror_to_exc_object", + "pyre_interpreter::pyerror_to_exc_object", + pyerror_to_exc_object as *const (), + ); + // The same materialisation with the `type_error` constructor folded in, so + // the raise site carries neither body. The typed local is the only + // compile-time check that the trampoline's signature matches the residual + // call — `push_alias_pair` performs none. + let pyerror_type_error_to_exc_object: extern "C" fn(i64) -> i64 = + crate::error::__majit_call_target_pyerror_type_error_to_exc_object; + push_alias_pair( + &mut entries, + "pyre_interpreter::error::pyerror_type_error_to_exc_object", + "pyre_interpreter::pyerror_type_error_to_exc_object", + pyerror_type_error_to_exc_object as *const (), + ); // `elidable_cannot_raise` subclass-range check; the trampoline widens its // one-word bool return by zero-extension. let ll_issubclass: extern "C" fn(i64, i64) -> i64 = @@ -3886,6 +3908,43 @@ mod tests { /// fallback (which SEGVs at trace time); a typo in either the /// module-qualified or root alias would silently regress to a /// symbolic hash, so pin both spellings against the live fnaddr. + /// The lowered raise path spells both of these as string literals in + /// another crate (`front::result_exc`, which takes the fused leaf from its + /// `FUSED_KIND_CTORS` table), and nothing links the two spellings at build + /// time: a typo on either side degrades the residual call to a + /// `symbolic_fnaddr_for_path` hash instead of failing to compile. Pinning + /// the registration against the live trampoline catches a drift on this + /// side; a drift in the consumer's literal still shows up only as a + /// declined descent. + #[test] + fn jit_trace_fnaddrs_covers_raise_path_exception_materialisation() { + let bindings: HashMap<&'static str, i64> = jit_trace_fnaddrs().into_iter().collect(); + + let materialise: extern "C" fn(i64) -> i64 = + crate::error::__majit_call_target_pyerror_to_exc_object; + let materialise = materialise as *const () as usize as i64; + assert_eq!( + bindings["pyre_interpreter::error::pyerror_to_exc_object"], + materialise + ); + assert_eq!( + bindings["pyre_interpreter::pyerror_to_exc_object"], + materialise + ); + + let fused: extern "C" fn(i64) -> i64 = + crate::error::__majit_call_target_pyerror_type_error_to_exc_object; + let fused = fused as *const () as usize as i64; + assert_eq!( + bindings["pyre_interpreter::error::pyerror_type_error_to_exc_object"], + fused + ); + assert_eq!( + bindings["pyre_interpreter::pyerror_type_error_to_exc_object"], + fused + ); + } + #[test] fn jit_trace_fnaddrs_covers_pop_value_and_exception_tls_helpers() { let bindings: HashMap<&'static str, i64> = jit_trace_fnaddrs().into_iter().collect(); diff --git a/pyre/pyre-interpreter/src/module/math/interp_math.rs b/pyre/pyre-interpreter/src/module/math/interp_math.rs index 38d912a5e9a..0d1e17bb81d 100644 --- a/pyre/pyre-interpreter/src/module/math/interp_math.rs +++ b/pyre/pyre-interpreter/src/module/math/interp_math.rs @@ -21,25 +21,33 @@ pub fn get_double(obj: PyObjectRef) -> f64 { /// to reject `math.exp("spam")` etc. pub fn try_get_double(obj: PyObjectRef) -> Result { unsafe { - if is_int(obj) { - return Ok(w_int_get_value(obj) as f64); - } if is_float(obj) { return Ok(floatobject::w_float_get_value(obj)); } - if is_long(obj) { - // A Python int is always finite, so a non-finite conversion means - // the magnitude exceeds f64 range — PyFloat_AsDouble raises here. - let v = jit_bigint_to_f64_or_nan(w_long_get_value(obj)); - if !v.is_finite() { - return Err(crate::PyError::overflow_error( - "int too large to convert to float", - )); + // Reading the payload answers only for an operand whose Python class + // is the builtin: `is_int` / `is_long` / `is_bool` read `ob_type`, + // which a strict subclass shares while overriding `__float__`. The + // subclass falls to the ladder below; an inherited `int.__float__` + // reproduces the payload. The `float` arm stays ungated because the + // conversion short-circuits on the layout and ignores an override. + if pyre_object::is_exact_builtin_instance(obj) { + if is_int(obj) { + return Ok(w_int_get_value(obj) as f64); + } + if is_long(obj) { + // A Python int is always finite, so a non-finite conversion means + // the magnitude exceeds f64 range — PyFloat_AsDouble raises here. + let v = jit_bigint_to_f64_or_nan(w_long_get_value(obj)); + if !v.is_finite() { + return Err(crate::PyError::overflow_error( + "int too large to convert to float", + )); + } + return Ok(v); + } + if is_bool(obj) { + return Ok(if w_bool_get_value(obj) { 1.0 } else { 0.0 }); } - return Ok(v); - } - if is_bool(obj) { - return Ok(if w_bool_get_value(obj) { 1.0 } else { 0.0 }); } } // `__float__` is a type-only special-method lookup (`space.lookup`); an @@ -808,6 +816,19 @@ fn bigint_log(n: &BigInt, base: f64) -> Result { n.log(base).map_err(map_rbigint_err) } +/// `loghelper` converts *any* integer operand from its payload — a subclass +/// included, since the check is `PyLong_Check` — and only a non-integer one +/// reaches the general float coercion. `log(x, base)` runs both operands +/// through it, so the base needs the same rule as the argument that +/// [`log_any`] handles inline; `try_get_double` would consult an overridden +/// `__float__` here and answer a different logarithm. +fn log_operand_double(obj: PyObjectRef) -> Result { + match crate::builtins::int_payload_as_f64(obj) { + Some(value) => value, + None => try_get_double(obj), + } +} + /// Special-case integer arguments to avoid overflow, and give the domain /// error the value-carrying message except for an int argument, whose error /// carries no value. @@ -885,7 +906,7 @@ pub fn log(args: &[PyObjectRef]) -> PyResult { let base = if args.len() >= 2 { // The base is validated before the argument, so log(x, base) with a // non-positive base reports the base rather than the argument. - let b = try_get_double(args[1])?; + let b = log_operand_double(args[1])?; if b <= 0.0 { return Err(crate::PyError::value_error(format!( "expected a positive input, got {}", diff --git a/pyre/pyre-interpreter/src/type_methods.rs b/pyre/pyre-interpreter/src/type_methods.rs index 2b8f98bc020..7ef5117adcc 100644 --- a/pyre/pyre-interpreter/src/type_methods.rs +++ b/pyre/pyre-interpreter/src/type_methods.rs @@ -3081,8 +3081,19 @@ fn format_with_spec(val: PyObjectRef, spec: &Wtf8) -> Result bool /// scan stack is a cycle and answers `false`: the occurrence that opened it /// decides. /// +/// The answer names the blocker rather than merely reporting one, because a +/// declined descent is a wall to be removed and the removal work is per +/// blocker, not per declining builtin. The hash resolves to a description +/// through the `symbolic_fnaddr_paths` registry the pipeline snapshots, which +/// `jit_metadata.json` carries. +/// /// The answer is memoized on the jitcode itself, so the scan runs once per /// body rather than once per call site. Only this entry point memoizes: the -/// `false` a cycle produces belongs to the occurrence that opened it, not to +/// `None` a cycle produces belongs to the occurrence that opened it, not to /// the body, so [`scan_body_for_unlowered_helper_call`] caches nothing. -fn descent_reaches_unlowered_helper_call(jitcode_index: usize) -> bool { - let Some(jitcode) = crate::jitcode_runtime::get_jitcode_ref_by_index(jitcode_index) else { - // Same condition the caller declines on; nothing to answer for. - return false; - }; - jitcode.descent_reaches_unlowered_helper_call(|| { +fn descent_unlowered_helper_blocker(jitcode_index: usize) -> Option { + let jitcode = crate::jitcode_runtime::get_jitcode_ref_by_index(jitcode_index)?; + jitcode.descent_unlowered_helper_blocker(|| { scan_body_for_unlowered_helper_call(jitcode_index, &mut Vec::new()) }) } -/// Recursive worker of [`descent_reaches_unlowered_helper_call`]. `seen` is +/// Recursive worker of [`descent_unlowered_helper_blocker`]. `seen` is /// the stack of jitcode indices currently being scanned. -fn scan_body_for_unlowered_helper_call(jitcode_index: usize, seen: &mut Vec) -> bool { +fn scan_body_for_unlowered_helper_call(jitcode_index: usize, seen: &mut Vec) -> Option { if seen.contains(&jitcode_index) { - return false; + return None; } let Some(body) = crate::jitcode_dispatch::sub_jitcode_body_by_index(jitcode_index) else { // No installed body means no descent, so there is nothing to answer // for; the caller declines on the same lookup. - return false; + return None; }; seen.push(jitcode_index); let descrs = crate::jitcode_runtime::descr_ref_table(); @@ -631,7 +634,7 @@ fn scan_body_for_unlowered_helper_call(jitcode_index: usize, seen: &mut Vec( if body.num_regs_r < 1 { return Ok(None); } - if descent_reaches_unlowered_helper_call(jitcode.index()) { - builtin_inline_decline!("un-lowered helper call in body", fnaddr); + if let Some(blocker) = descent_unlowered_helper_blocker(jitcode.index()) { + builtin_inline_decline!( + format_args!("un-lowered helper call in body blocker={blocker:#x}"), + fnaddr + ); return Ok(None); } let nested_helper = ctx.fbw_mode.inline_subwalk; diff --git a/pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs b/pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs index 96b99ef7e43..662e65ebc5a 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs @@ -159,9 +159,14 @@ fn walker_emit_recorded_builtin_raise( /// the emitted `GUARD_CLASS INT` would not match it), unbox it /// (`GUARD_CLASS INT` + `getfield intval`) and record `int_is_true`, stamping /// the folded concrete truth. Returns the raw truth `OpRef` on success; -/// `None` when the operand is not a concrete non-bool int — the caller then -/// falls through to the generic may-force residual, preserving `__bool__` / -/// `__len__` semantics. +/// `None` when the operand is not a concrete int — the caller then falls +/// through to the generic may-force residual, which runs `__bool__` / +/// `__len__`. +/// +/// Declining a subclass on the *recorded* operand is not enough: `is_int` +/// and the `GUARD_CLASS` below both read `ob_type`, so a trace compiled from +/// an exact int still admits a subclass that arrives later. The `w_class` +/// pin is what rejects it. /// /// Eliding the `CALL_MAY_FORCE` here also removes its `GUARD_NOT_FORCED` / /// `GUARD_NO_EXCEPTION`, whose kept-stack blackhole resume reads NULL peeled @@ -183,6 +188,7 @@ pub(crate) fn try_walker_specialize_truth_int( }; let int_type_addr = &pyre_object::pyobject::INT_TYPE as *const _ as i64; let raw = walker_unbox_int(ctx, op_pc, operand, int_type_addr)?; + walker_guard_exact_w_class(ctx, op_pc, operand, walker_numeric_builtin_class(obj))?; let truth = ctx.trace_ctx.record_op(OpCode::IntIsTrue, &[raw]); ctx.trace_ctx .set_opref_concrete(truth, majit_ir::Value::Int((val != 0) as i64)); @@ -3948,6 +3954,14 @@ pub(crate) fn try_walker_specialize_store_attr( pyre_interpreter::objspace::std::mapdict::UnboxType::Int => { let int_type_addr = &pyre_object::pyobject::INT_TYPE as *const _ as i64; let raw = walker_unbox_int(ctx, op_pc, value, int_type_addr)?; + // A subclass shares the builtin's `ob_type`, which is all the unbox + // guard proves; the operand gate read `w_class`, so pin that too. + walker_guard_exact_w_class( + ctx, + op_pc, + value, + walker_numeric_builtin_class(concrete_value), + )?; ( crate::helpers::jit_mapdict_unboxed_write_raw as *const (), raw, @@ -3957,6 +3971,14 @@ pub(crate) fn try_walker_specialize_store_attr( pyre_interpreter::objspace::std::mapdict::UnboxType::Float => { let float_type_addr = &pyre_object::pyobject::FLOAT_TYPE as *const _ as i64; let raw = walker_unbox_float(ctx, op_pc, value, float_type_addr)?; + // A subclass shares the builtin's `ob_type`, which is all the unbox + // guard proves; the operand gate read `w_class`, so pin that too. + walker_guard_exact_w_class( + ctx, + op_pc, + value, + walker_numeric_builtin_class(concrete_value), + )?; let live_f = unsafe { pyre_object::w_float_get_value(concrete_value) }; ctx.trace_ctx .set_opref_concrete(raw, majit_ir::Value::Float(live_f)); @@ -4454,6 +4476,11 @@ pub(crate) fn try_walker_specialize_newlist( } else { walker_unbox_int(ctx, op_pc, it, int_type_addr)? }; + // The unbox proves `ob_type`, which a subclass shares; without + // the `w_class` pin the element is rewrapped as a plain int. + if let Some(obj) = walker_concrete_ref_object(ctx, it) { + walker_guard_exact_w_class(ctx, op_pc, it, walker_numeric_builtin_class(obj))?; + } ctx.trace_ctx .set_opref_concrete(raw, majit_ir::Value::Int(v)); raws.push(raw); @@ -4472,6 +4499,9 @@ pub(crate) fn try_walker_specialize_newlist( let mut raws: Vec = Vec::with_capacity(len); for (&it, &v) in items.iter().zip(vals.iter()) { let raw = walker_unbox_float(ctx, op_pc, it, float_type_addr)?; + if let Some(obj) = walker_concrete_ref_object(ctx, it) { + walker_guard_exact_w_class(ctx, op_pc, it, walker_numeric_builtin_class(obj))?; + } ctx.trace_ctx .set_opref_concrete(raw, majit_ir::Value::Float(v)); // `walker_unbox_float` guards `ob_type` only, which a float @@ -4855,7 +4885,13 @@ pub(crate) fn try_walker_specialize_compare_op_int( let (lhs_type, lhs_descr) = crate::state::int_or_bool_unbox_type_descr(lhs_obj); let (rhs_type, rhs_descr) = crate::state::int_or_bool_unbox_type_descr(rhs_obj); let lhs_raw = walker_unbox_int_typed(ctx, op_pc, lhs, lhs_type, lhs_descr)?; + // `walker_unbox_int_typed` proves only `ob_type`, which an `int` subclass + // shares with `int`; the operand gate reads `w_class`, which it does not. + // Without these the compiled guard admits the subclass and the comparison + // is answered by `IntLt` instead of the overriding `__lt__`. + walker_guard_exact_w_class(ctx, op_pc, lhs, walker_numeric_builtin_class(lhs_obj))?; let rhs_raw = walker_unbox_int_typed(ctx, op_pc, rhs, rhs_type, rhs_descr)?; + walker_guard_exact_w_class(ctx, op_pc, rhs, walker_numeric_builtin_class(rhs_obj))?; let truth = ctx.trace_ctx.record_op(cmp, &[lhs_raw, rhs_raw]); let folded = majit_metainterp::eval_binop_i(cmp, la, rb); ctx.trace_ctx @@ -9777,6 +9813,14 @@ pub(crate) fn try_walker_specialize_math_sqrt( // Coerce the argument to a raw float (int → guard_class + unbox + // CastIntToFloat; float → guard_class + unbox). let x = walker_coerce_operand_to_float(ctx, op.pc, r_args[2], arg_obj, is_int, val, false)?; + if is_int { + // The interpreter's `try_get_double` reads an int payload only for an + // exact builtin: a subclass may override `__float__`, and the unbox + // guard proves only `ob_type`, which the subclass shares. The float + // arm needs no pin -- that coercion short-circuits on the float layout + // and ignores an override there too. + walker_guard_exact_w_class(ctx, op.pc, r_args[2], walker_numeric_builtin_class(arg_obj))?; + } // `ll_math_sqrt` domain guards: `if x < 0.0` (FloatLt pinned false) and // `if isfinite(x)` (FloatSub(x,x) == 0 pinned true — excludes NaN/±inf). let zero = ctx.trace_ctx.const_float(0.0f64.to_bits() as i64); @@ -9889,6 +9933,14 @@ pub(crate) fn try_walker_specialize_math_log_trig( .replace_box(callable_op, expected); } let x = walker_coerce_operand_to_float(ctx, op.pc, r_args[2], arg_obj, is_int, val, false)?; + if is_int { + // The interpreter's `try_get_double` reads an int payload only for an + // exact builtin: a subclass may override `__float__`, and the unbox + // guard proves only `ob_type`, which the subclass shares. The float + // arm needs no pin -- that coercion short-circuits on the float layout + // and ignores an override there too. + walker_guard_exact_w_class(ctx, op.pc, r_args[2], walker_numeric_builtin_class(arg_obj))?; + } let zero = ctx.trace_ctx.const_float(0.0f64.to_bits() as i64); if is_log { walker_float_cmp_guard(ctx, op.pc, OpCode::FloatLt, &[zero, x], true)?; @@ -9990,6 +10042,14 @@ pub(crate) fn try_walker_specialize_math_frexp( .replace_box(callable_op, expected); } let x = walker_coerce_operand_to_float(ctx, op.pc, r_args[2], arg_obj, is_int, x_value, false)?; + if is_int { + // The interpreter's `try_get_double` reads an int payload only for an + // exact builtin: a subclass may override `__float__`, and the unbox + // guard proves only `ob_type`, which the subclass shares. The float + // arm needs no pin -- that coercion short-circuits on the float layout + // and ignores an override there too. + walker_guard_exact_w_class(ctx, op.pc, r_args[2], walker_numeric_builtin_class(arg_obj))?; + } let mantissa = ctx.trace_ctx.call_float_typed_with_effect( pyre_interpreter::module::math::interp_math::jit_math_frexp_mantissa as *const (), &[x], @@ -10114,6 +10174,14 @@ pub(crate) fn try_walker_specialize_math_ldexp( .replace_box(callable_op, expected); } let x = walker_coerce_operand_to_float(ctx, op.pc, r_args[2], x_obj, x_is_int, x_value, false)?; + if x_is_int { + // The interpreter's `try_get_double` reads an int payload only for an + // exact builtin: a subclass may override `__float__`, and the unbox + // guard proves only `ob_type`, which the subclass shares. The float + // arm needs no pin -- that coercion short-circuits on the float layout + // and ignores an override there too. + walker_guard_exact_w_class(ctx, op.pc, r_args[2], walker_numeric_builtin_class(x_obj))?; + } let (exp_type_addr, exp_descr) = crate::state::int_or_bool_unbox_type_descr(exp_obj); let exp = walker_unbox_int_typed(ctx, op.pc, r_args[3], exp_type_addr, exp_descr)?; ctx.trace_ctx @@ -11251,6 +11319,11 @@ pub(crate) fn try_walker_specialize_float_call( if is_int { // int/bool → CastIntToFloat + inline wrapfloat (no residual call). let raw = walker_coerce_operand_to_float(ctx, op.pc, arg_op, arg_obj, true, val, false)?; + // `builtin_float` reads an int payload only for an exact builtin, so an + // `int` subclass overriding `__float__` must side-exit; the unbox guard + // proves only `ob_type`, which the subclass shares. The float arm below + // pins its own `w_class` for the same reason. + walker_guard_exact_w_class(ctx, op.pc, arg_op, walker_numeric_builtin_class(arg_obj))?; let boxed = crate::state::wrapfloat(ctx.trace_ctx, raw); ctx.trace_ctx.set_opref_concrete( boxed, @@ -14362,6 +14435,15 @@ pub(crate) fn try_walker_specialize_store_subscr( // storage), so it unboxes through the plain INT_TYPE guard. let int_type_addr = &pyre_object::pyobject::INT_TYPE as *const _ as i64; let raw = walker_unbox_int(ctx, op_pc, value_op, int_type_addr)?; + // The list gets an exact-`w_class` guard above; the VALUE needs its own, + // because the unbox proves only `ob_type` and a subclass shares it — + // storing its payload would drop the element's Python class. + walker_guard_exact_w_class( + ctx, + op_pc, + value_op, + walker_numeric_builtin_class(value_obj), + )?; let elem = unsafe { pyre_object::w_int_get_value(value_obj) }; ctx.trace_ctx .set_opref_concrete(raw, majit_ir::Value::Int(elem)); @@ -14374,6 +14456,12 @@ pub(crate) fn try_walker_specialize_store_subscr( ); let float_type_addr = &pyre_object::pyobject::FLOAT_TYPE as *const _ as i64; let raw = walker_unbox_float(ctx, op_pc, value_op, float_type_addr)?; + walker_guard_exact_w_class( + ctx, + op_pc, + value_op, + walker_numeric_builtin_class(value_obj), + )?; let elem = unsafe { pyre_object::w_float_get_value(value_obj) }; ctx.trace_ctx .set_opref_concrete(raw, majit_ir::Value::Float(elem)); @@ -15482,8 +15570,13 @@ pub(crate) fn try_walker_specialize_compare_op_float( // --- emit the specialized IR (walker-native) --- let lhs_raw = walker_coerce_operand_to_float(ctx, op_pc, lhs, lhs_obj, lhs_is_int, lhs_f64, true)?; + // The coercion proves `ob_type`, which an `int`/`float` subclass shares with + // its builtin; the operand gate read `w_class`, so pin that too or the + // compiled guard admits the subclass and answers with `FloatLt`. + walker_guard_exact_w_class(ctx, op_pc, lhs, walker_numeric_builtin_class(lhs_obj))?; let rhs_raw = walker_coerce_operand_to_float(ctx, op_pc, rhs, rhs_obj, rhs_is_int, rhs_f64, true)?; + walker_guard_exact_w_class(ctx, op_pc, rhs, walker_numeric_builtin_class(rhs_obj))?; let truth = ctx.trace_ctx.record_op(cmp, &[lhs_raw, rhs_raw]); let folded = majit_metainterp::eval_float_cmp(cmp, lhs_f64.to_bits() as i64, rhs_f64.to_bits() as i64);