diff --git a/majit/majit-metainterp/src/virtualref.rs b/majit/majit-metainterp/src/virtualref.rs index 645b9c90396..934de8276f9 100644 --- a/majit/majit-metainterp/src/virtualref.rs +++ b/majit/majit-metainterp/src/virtualref.rs @@ -172,9 +172,12 @@ pub use crate::jit::InvalidVirtualRef; /// so the address is stable across a minor collection. The `forced` frame may /// be young, hence the creation write barrier. /// -/// The `Box` fallback covers the window before `set_vref_gc_type_id` has run -/// (the id still reads its unset sentinel) and an old-gen allocation failure; -/// it is leaked, and reclamation is what it gives up. +/// The `Box` fallback covers only the window before `set_vref_gc_type_id` has +/// run (the id still reads its unset sentinel), where there is no registered +/// type to allocate and nothing has handed a vref to the collector yet; it is +/// leaked, and reclamation is what it gives up. Once the id is set the +/// allocation stays collector-owned or fails loudly — a host box past that +/// point would silently drop the `forced` edge this object exists to hold. fn alloc_virtual_ref(real_object: *mut u8) -> *mut u8 { let vref = JitVirtualRef { super_: ObjectHeader { @@ -186,12 +189,14 @@ fn alloc_virtual_ref(real_object: *mut u8) -> *mut u8 { let type_id = vref_gc_type_id(); if type_id != VREF_GC_TYPE_ID_UNSET { let gcref = majit_gc::alloc_oldgen_typed(type_id, std::mem::size_of::()); - if gcref.0 != 0 { - unsafe { std::ptr::write(gcref.0 as *mut JitVirtualRef, vref) }; - // Creation write barrier: an old-gen vref may point at a young frame. - majit_gc::gc_write_barrier(gcref); - return gcref.0 as *mut u8; - } + assert!( + gcref.0 != 0, + "JitVirtualRef old-gen allocation failed after the type was registered", + ); + unsafe { std::ptr::write(gcref.0 as *mut JitVirtualRef, vref) }; + // Creation write barrier: an old-gen vref may point at a young frame. + majit_gc::gc_write_barrier(gcref); + return gcref.0 as *mut u8; } Box::into_raw(Box::new(vref)) as *mut u8 } diff --git a/pyre/bench/synth/type_metatype_method_call.py b/pyre/bench/synth/type_metatype_method_call.py new file mode 100644 index 00000000000..b94703759e2 --- /dev/null +++ b/pyre/bench/synth/type_metatype_method_call.py @@ -0,0 +1,50 @@ +# A metaclass resolves `Cls.name()` before the class's own MRO does: +# `type.__getattribute__` lets a metatype DATA descriptor win outright, and a +# metatype `__getattribute__` override produces the value itself. Either way the +# call must use what the metaclass returned, not rebind the class onto it. + + +class MetaProp(type): + @property + def where(cls): + return lambda: 'meta-prop' + + +class ByProp(metaclass=MetaProp): + @classmethod + def where(cls): + return 'own-classmethod' + + +class MetaGetattr(type): + def __getattribute__(cls, name): + if name == 'ping': + return lambda: 'meta-getattr' + return type.__getattribute__(cls, name) + + +class ByGetattr(metaclass=MetaGetattr): + @classmethod + def ping(cls): + return 'own-classmethod' + + +class Plain: + @classmethod + def tag(cls): + return cls.__name__ + + +def main(): + prop = getattr_ = plain = None + for _ in range(20000): + prop = ByProp.where() + getattr_ = ByGetattr.ping() + # an ordinary class still binds its classmethod's cls + plain = Plain.tag() + print('prop', prop) + print('getattr', getattr_) + print('plain', plain) + + +main() diff --git a/pyre/pyre-interpreter/src/baseobjspace.rs b/pyre/pyre-interpreter/src/baseobjspace.rs index e1dbc24c2b3..34d94cbb83f 100644 --- a/pyre/pyre-interpreter/src/baseobjspace.rs +++ b/pyre/pyre-interpreter/src/baseobjspace.rs @@ -8905,11 +8905,12 @@ pub unsafe fn load_method_fast_path( /// same shape a plain instance method takes, with the class in the receiver /// slot instead of an instance. /// -/// `is_type` is exact-metatype, so a custom metaclass declines (its -/// `__getattribute__` may not follow `type.__getattribute__`). A name the -/// metatype itself defines is declined too: a metatype attribute would shadow -/// the class attribute (data descriptor) or be returned in its place. An -/// uncacheable type and any non-`classmethod` descriptor also decline. +/// The metatype is read off the class (`getclass()`) and must be `type` +/// itself, so a custom metaclass declines (its `__getattribute__` may not +/// follow `type.__getattribute__`). A name the metatype itself defines is +/// declined too: a metatype attribute would shadow the class attribute (data +/// descriptor) or be returned in its place. An uncacheable type and any +/// non-`classmethod` descriptor also decline. /// /// # Safety /// `w_obj` must be a valid object pointer (null tolerated). @@ -8921,10 +8922,18 @@ pub unsafe fn classmethod_on_type_fast_path( return None; } let w_type = w_obj; - // `is_type` pins the metaclass to exactly `type`, so `type.__getattribute__` - // is the resolution path. A metatype attribute of the same name would win - // over the class attribute, so decline any name the metatype defines. - let metatype = &pyre_object::pyobject::TYPE_TYPE as *const _ as PyObjectRef; + // `is_type` answers for the object's physical layout — every type object + // carries the same `ob_type` — so it says nothing about the metaclass. + // `getclass()` (baseobjspace.py) reads the metaclass off `w_class`; only + // `type` itself resolves the name through `type.__getattribute__`, so any + // other metaclass declines rather than have its `__getattribute__` + // override bypassed. + let metatype = crate::typedef::r#type(w_obj)?.as_ptr(); + if !std::ptr::eq(metatype, crate::typedef::w_type()) { + return None; + } + // A metatype attribute of the same name would win over the class attribute, + // so decline any name the metatype defines. if lookup_in_type(metatype, name).is_some() { return None; } diff --git a/pyre/pyre-interpreter/src/eval.rs b/pyre/pyre-interpreter/src/eval.rs index 0cfb8fa8ad9..b84df72ca3e 100644 --- a/pyre/pyre-interpreter/src/eval.rs +++ b/pyre/pyre-interpreter/src/eval.rs @@ -2979,6 +2979,20 @@ pub fn compute_load_method_bound(obj: PyObjectRef, attr: PyObjectRef, name: &str // METAclass MRO (`space.type(w_obj)`), so a name found in the // type's own MRO reaches the call as a plain getattr value // with no binding. + // + // `is_type` reports the physical layout every type object shares, + // not the metaclass, so read the metaclass and require it to be + // `type`. The shape inferred below is what + // `type.__getattribute__` produces; a custom metaclass can + // override `__getattribute__` or define a data descriptor of the + // same name, and either one produced `attr` in place of the + // class's own MRO entry — binding `cls` onto that value would + // pass the class to something that never asked for it. + let metatype_is_type = crate::typedef::r#type(obj) + .is_some_and(|meta| std::ptr::eq(meta.as_ptr(), crate::typedef::w_type())); + if !metatype_is_type { + return PY_NULL; + } let raw = crate::baseobjspace::lookup_in_type(obj, name); match raw { Some(d) if pyre_object::is_classmethod(d) => obj, diff --git a/pyre/pyre-jit-trace/src/descr.rs b/pyre/pyre-jit-trace/src/descr.rs index c4b28cd9460..7d841451130 100644 --- a/pyre/pyre-jit-trace/src/descr.rs +++ b/pyre/pyre-jit-trace/src/descr.rs @@ -2886,6 +2886,18 @@ pub fn pytraceback_w_next_descr() -> DescrRef { field_descr_from_group(&PYTRACEBACK_DESCR_GROUP, index) } +/// Field descriptor for `PyTraceback.lineno`, the line `descr_get_tb_lineno` +/// reports. Located by offset for the same reason as +/// [`pytraceback_w_next_descr`]. +pub fn pytraceback_lineno_descr() -> DescrRef { + let index = PYTRACEBACK_DESCR_GROUP + .field_descrs + .iter() + .position(|d| d.offset() == pyre_interpreter::pytraceback::PYTRACEBACK_LINENO_OFFSET) + .expect("PyTraceback descr group has no lineno field"); + field_descr_from_group(&PYTRACEBACK_DESCR_GROUP, index) +} + /// Cached field descriptor for a raw reference slot selected by the /// exception attribute fold. Indices are those of `build_w_exception_group`; /// no parallel descriptor is constructed. diff --git a/pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs b/pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs index af72d5fb42a..86e1d6bf85d 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs @@ -2148,6 +2148,21 @@ pub(crate) fn fbw_callee_body_replay_safety( // [`fbw_store_journal_rollback`] replays the journal in reverse // on a non-committed exit — so a folded store is undone for the // replay, and an unfolded one never runs. + // `binary_op` / `compare_op` over operands this scan could not + // prove exact-numeric is the same shape one more time: which + // `__add__` / `__lt__` runs is a property of the operand's + // runtime class, not of this body. The proven-operand case was + // already accepted above; what is left here is exactly the + // operand whose provenance the scan lost — most commonly a + // `LOAD_ATTR` result, since that arm is itself deferred and + // clears numeric provenance. Deferring instead of declining is + // what lets `self.v + i` inline: at trace time the attribute + // read folds to a mapdict slot with a concrete int shadow, so + // the walker's numeric specialization erases the residual before + // the backstop is reached. An operand pair that stays opaque + // leaves the residual standing, and it reaches + // `fbw_abort_nested_unjournaled_residual` like any other — the + // helper never runs. if matches!( ei.pyre_helper, majit_ir::PyreHelperKind::CallFn @@ -2156,6 +2171,8 @@ pub(crate) fn fbw_callee_body_replay_safety( | majit_ir::PyreHelperKind::RaiseVarargs | majit_ir::PyreHelperKind::SetCurrentException | majit_ir::PyreHelperKind::LoadAttr + | majit_ir::PyreHelperKind::BinaryOp + | majit_ir::PyreHelperKind::CompareOp ) { deferred_call = true; // The callee this resolves to is a runtime value, so what it diff --git a/pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs b/pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs index 2a36d3c319d..3ed3751495a 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs @@ -2681,7 +2681,17 @@ pub(crate) fn try_walker_inline_resolved_user_call( let legacy_admit = match safety { CalleeReplaySafety::Clean => true, CalleeReplaySafety::DeferredCall => { - foriter_deferred_admit = !fbw_foriter_deferred_call_denied(callee_code_key); + // The deferred promise rests on the abort REWINDING to the + // enclosing CALL and re-executing it from scratch. A binop + // dunder dispatch (the only entry carrying an + // `arg_class_guard`) reaches this lever from a `BINARY_OP` + // instead, and that opcode is not a call boundary the rewind + // can name: the flush resumes one operand short and the whole + // iteration's contribution is dropped, silently. A `Clean` + // body is still admitted from there — it has nothing that can + // abort. + foriter_deferred_admit = + arg_class_guard.is_none() && !fbw_foriter_deferred_call_denied(callee_code_key); foriter_deferred_admit } CalleeReplaySafety::Dirty => { @@ -3196,18 +3206,19 @@ pub(crate) fn try_walker_inline_resolved_user_call( // Stored bound methods carry their explicit receiver and callee frame, // so their Ref operands remain available to the resume path. // - // This is the one precondition here that still aborts instead of - // returning `Ok(None)`, and deliberately so. Residualizing it does - // work — `bench/synth/_pending/gc_bug_bridge_flavor_traceback_names` - // goes from 98 aborts to 2 and - // `_pending/exception_nested_exc_info_restore` from 5 aborts to 0, - // both compiling loops they never compiled before — but the loops it - // newly compiles then print traceback tuples missing their outermost - // frame, diverging from the interpreter (that fixture pins its - // expected output in its header). The abort was masking a lost - // `PyTraceback` node on the compiled exception path, not preventing - // one. Restore `Ok(None)` here once that node is recorded; it is the - // largest single win left in this function. + // This precondition used to abort the enclosing trace rather than + // decline the inline, because residualizing it let loops compile that + // then printed traceback tuples missing their OUTERMOST frame. That + // node is now recorded — the two bridge handler-entry arms attach the + // catching frame's own node — so the decline joins every other + // precondition here and returns `Ok(None)`. + // + // The abort was expensive out of all proportion to the inline it was + // protecting: a callee that walks a traceback (`while tb is not None`) + // lowers to exactly this instruction, so any handler calling such a + // helper aborted every retrace of the enclosing loop. The guard whose + // bridge the retrace was building therefore never got one and deopted + // on every delivery. if bound_method.is_none() && (0..callee_code.instructions.len()).any(|pc| { matches!( @@ -3221,7 +3232,7 @@ pub(crate) fn try_walker_inline_resolved_user_call( }) { if try_multiframe { - return Err(DispatchError::callee_inline_unsupported(op.pc)); + return Ok(None); } break 'seed; } diff --git a/pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs b/pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs index ca513df99b8..342d2265b53 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs @@ -1887,12 +1887,14 @@ fn walker_guard_specialised_pair_class( /// One hop of a `while tb is not None: names.append(tb.tb_frame.f_code.co_name); /// tb = tb.tb_next` traceback walk. /// -/// Each of these is a `GetSetProperty` whose getter body is a single slot read -/// on a receiver [`walker_specialize_traceback_walk_field`] pins by class: -/// `pytraceback.py descr_get_next` / `descr_get_tb_frame` and `pyframe.py` -/// `fget_code`. None of them dispatches anywhere or can raise. Left residual, -/// every hop of the walk costs a forcing call, which is what makes each -/// traceback fixture dominated by the walk rather than by the raise. +/// Each of these is a `GetSetProperty` whose getter body is a slot read on a +/// receiver [`walker_specialize_traceback_walk_field`] pins by class: +/// `pytraceback.py descr_get_next` / `descr_get_tb_frame` / +/// `descr_get_tb_lineno` / `descr_get_tb_lasti` and `pyframe.py fget_code`. +/// None of them dispatches anywhere or can raise. Left residual, every hop of +/// the walk costs a forcing call — measured at 207 ns per `tb_lineno` read +/// against 0 for the folded `tb_next` — which is what makes each traceback +/// fixture dominated by the walk rather than by the raise. #[derive(Clone, Copy, PartialEq, Eq)] enum TracebackWalkField { /// `tb.tb_next` — the chain link; a null slot is the terminator and @@ -1903,6 +1905,14 @@ enum TracebackWalkField { TbFrame, /// `frame.f_code` — `fget_f_code` is `self.pycode as PyObjectRef`. FCode, + /// `tb.tb_lineno` — the line the node froze at. `get_lineno` resolves it + /// lazily upstream; pyre stamps it at `record_application_traceback` time, + /// so the getter is the slot read plus the `LINENO_NOT_COMPUTED` mapping. + /// + /// `tb_lasti` is deliberately absent: its getter reports `lasti * 2`, so + /// the fold would have to carry the doubling rather than hand back the + /// slot. + TbLineno, } /// Which walk hop, if any, this `(receiver, attribute)` pair is. @@ -1915,6 +1925,7 @@ fn traceback_walk_field( return match name { "tb_next" => Some(TracebackWalkField::TbNext), "tb_frame" => Some(TracebackWalkField::TbFrame), + "tb_lineno" => Some(TracebackWalkField::TbLineno), _ => None, }; } @@ -1942,30 +1953,16 @@ fn walker_specialize_traceback_walk_field( ) -> Result, DispatchError> { use pyre_interpreter::pyframe::PyFrame; - let (receiver_type, descr, stored) = match field { - TracebackWalkField::TbNext => ( - &pyre_interpreter::pytraceback::PYTRACEBACK_TYPE, - crate::descr::pytraceback_w_next_descr(), - unsafe { pyre_interpreter::pytraceback::w_pytraceback_get_w_next(concrete_obj) }, - ), - TracebackWalkField::TbFrame => ( - &pyre_interpreter::pytraceback::PYTRACEBACK_TYPE, - crate::descr::pytraceback_frame_descr(), - unsafe { pyre_interpreter::pytraceback::w_pytraceback_get_frame(concrete_obj) } - as pyre_object::PyObjectRef, - ), - TracebackWalkField::FCode => ( - &pyre_interpreter::pyframe::FRAME_TYPE, - crate::descr::pyframe_code_descr(), - unsafe { (*(concrete_obj as *const PyFrame)).pycode } as pyre_object::PyObjectRef, - ), + let receiver_type = match field { + TracebackWalkField::FCode => &pyre_interpreter::pyframe::FRAME_TYPE, + _ => &pyre_interpreter::pytraceback::PYTRACEBACK_TYPE, + }; + let descr = match field { + TracebackWalkField::TbNext => crate::descr::pytraceback_w_next_descr(), + TracebackWalkField::TbFrame => crate::descr::pytraceback_frame_descr(), + TracebackWalkField::FCode => crate::descr::pyframe_code_descr(), + TracebackWalkField::TbLineno => crate::descr::pytraceback_lineno_descr(), }; - // Only `tb_next` has a null with a defined meaning. A null frame is a - // torn-down traceback and a null `pycode` a half-built frame; both are - // answered by a `sys.namespace` stub or `None` the residual owns. - if stored.is_null() && field != TracebackWalkField::TbNext { - return Ok(None); - } let w_type = pyre_interpreter::typedef::gettypeobject(receiver_type); let version_tag = unsafe { pyre_object::typeobject::w_type_get_version_tag(w_type) }; if version_tag == 0 { @@ -1979,6 +1976,54 @@ fn walker_specialize_traceback_walk_field( return Ok(None); } + if field == TracebackWalkField::TbLineno { + let live = + unsafe { pyre_interpreter::pytraceback::w_pytraceback_get_lineno_raw(concrete_obj) }; + // `get_lineno` answers the sentinel with `-1`, so the slot value is the + // getter's value only once it is pinned against the sentinel. A node + // that already carries it — built from a frame with no `pycode` — has + // nothing to pin, so decline before recording anything. + if live == pyre_interpreter::pytraceback::LINENO_NOT_COMPUTED { + return Ok(None); + } + walker_guard_exception_attr_slot(ctx, op_pc, obj, concrete_obj, w_type, version_tag)?; + let raw_value = crate::state::opimpl_getfield_gc_i(ctx.trace_ctx, obj, descr); + ctx.trace_ctx + .set_opref_concrete(raw_value, majit_ir::Value::Int(live)); + let not_computed = ctx + .trace_ctx + .const_int(pyre_interpreter::pytraceback::LINENO_NOT_COMPUTED); + let is_not_computed = ctx + .trace_ctx + .record_op(OpCode::IntEq, &[raw_value, not_computed]); + walker_emit_fold_guard_with_snapshot(ctx, op_pc, OpCode::GuardFalse, &[is_not_computed])?; + // The getter returns a Python int, so the raw slot is reboxed the way + // the unboxed mapdict read does; the boxed op is a heap `NewWithVtable` + // so its concrete has to be a heap pointer too. + let boxed = walker_box_int(ctx, op_pc, raw_value, live)?; + let live_ptr = pyre_object::w_int_new(live) as i64; + ctx.trace_ctx + .set_opref_concrete(boxed, box_int_concrete(live, live_ptr)); + write_residual_call_result_to_dst(ctx, op_pc, dst, dst_bank, boxed)?; + return Ok(Some(())); + } + + let stored = match field { + TracebackWalkField::TbNext => unsafe { + pyre_interpreter::pytraceback::w_pytraceback_get_w_next(concrete_obj) + }, + TracebackWalkField::TbFrame => { + (unsafe { pyre_interpreter::pytraceback::w_pytraceback_get_frame(concrete_obj) }) + as pyre_object::PyObjectRef + } + _ => (unsafe { (*(concrete_obj as *const PyFrame)).pycode }) as pyre_object::PyObjectRef, + }; + // Only `tb_next` has a null with a defined meaning. A null frame is a + // torn-down traceback and a null `pycode` a half-built frame; both are + // answered by a `sys.namespace` stub or `None` the residual owns. + if stored.is_null() && field != TracebackWalkField::TbNext { + return Ok(None); + } walker_guard_exception_attr_slot(ctx, op_pc, obj, concrete_obj, w_type, version_tag)?; let raw_value = crate::state::opimpl_getfield_gc_r(ctx.trace_ctx, obj, descr); let value = if stored.is_null() {