diff --git a/pyre/extra_tests/parity_tests/pickle_dispatch_table_across_collection.py b/pyre/extra_tests/parity_tests/pickle_dispatch_table_across_collection.py new file mode 100644 index 00000000000..c879e0412ec --- /dev/null +++ b/pyre/extra_tests/parity_tests/pickle_dispatch_table_across_collection.py @@ -0,0 +1,71 @@ +# CPython-suite gap: pickle tests do not keep dump hooks live across collection. +# parity-tests reason: this guards pyre/PyPy moving-GC roots during pickling. + +"""Pickler hooks and the effective dispatch table survive collection.""" + +import copyreg +import gc +import io +import pickle + + +class Payload: + def __init__(self, value): + self.value = value + + +def rebuild_payload(value): + return Payload(value) + + +def reduce_payload(obj): + return rebuild_payload, (obj.value,) + + +class CollectingPickler(pickle.Pickler): + def __init__(self, file, protocol): + super().__init__(file, protocol=protocol) + self.churned = False + self.persistent_before_churn = 0 + self.persistent_after_churn = 0 + self.override_after_churn = 0 + + def persistent_id(self, obj): + if self.churned: + self.persistent_after_churn += 1 + else: + self.persistent_before_churn += 1 + junk = [[object() for _ in range(64)] for _ in range(128)] + assert len(junk) == 128 + gc.collect() + self.churned = True + return None + + def reducer_override(self, obj): + assert self.churned + self.override_after_churn += 1 + return NotImplemented + + +original_dispatch_table = copyreg.dispatch_table +try: + copyreg.dispatch_table = {Payload: reduce_payload} + expected = [Payload(i) for i in range(40)] + expected.extend([{"padding": [str(i) for i in range(80)]}, Payload(999)]) + + for protocol in (0, 2, pickle.HIGHEST_PROTOCOL): + stream = io.BytesIO() + pickler = CollectingPickler(stream, protocol) + pickler.dump(expected) + restored = pickle.loads(stream.getvalue()) + + assert [obj.value for obj in restored[:40]] == list(range(40)) + assert restored[-1].value == 999 + assert restored[-2] == expected[-2] + assert pickler.persistent_before_churn == 1 + assert pickler.persistent_after_churn > 0 + assert pickler.override_after_churn >= 41 +finally: + copyreg.dispatch_table = original_dispatch_table + +print("OK") diff --git a/pyre/extra_tests/parity_tests/weakref_ref_subclass_layout.py b/pyre/extra_tests/parity_tests/weakref_ref_subclass_layout.py new file mode 100644 index 00000000000..bfc5d10fc47 --- /dev/null +++ b/pyre/extra_tests/parity_tests/weakref_ref_subclass_layout.py @@ -0,0 +1,102 @@ +# CPython-suite gap: weakref subclass tests do not combine callbacks with all storage shapes. +# parity-tests reason: this guards the translated W_Weakref payload and subclass carrier layout. + +import gc +from weakref import WeakValueDictionary, ref + + +class Value: + def __init__(self, value): + self.value = value + + def __eq__(self, other): + return isinstance(other, Value) and self.value == other.value + + def __hash__(self): + return hash(self.value) + + +class PlainRef(ref): + pass + + +class EmptyRef(ref): + __slots__ = () + + +class KeyedRef(ref): + __slots__ = ("key",) + + +PRIVATE_FIELDS = ("w_obj_weak", "w_callable", "w_hash", "w_slots") + + +def collect(): + for _ in range(3): + gc.collect() + + +def check_ref_subclass(ref_type): + callbacks = [] + + def callback(w_ref): + callbacks.append(w_ref() is None) + + value = Value((ref_type.__name__, 42)) + equal_value = Value((ref_type.__name__, 42)) + w_ref = ref_type(value, callback) + equal_ref = ref(equal_value) + + assert w_ref() is value + assert w_ref.__callback__ is callback + assert hash(w_ref) == hash(value) + assert w_ref == equal_ref + + if ref_type is PlainRef: + w_ref.marker = "plain" + assert w_ref.__dict__ == {"marker": "plain"} + else: + assert not hasattr(w_ref, "__dict__") + + if ref_type is KeyedRef: + w_ref.key = ("module", "name") + assert w_ref.key == ("module", "name") + w_ref.key = ("module", "renamed") + assert w_ref.key == ("module", "renamed") + assert w_ref() is value + del w_ref.key + try: + w_ref.key + except AttributeError: + pass + else: + raise AssertionError("deleted slot remained readable") + + for name in PRIVATE_FIELDS: + assert not hasattr(w_ref, name) + if hasattr(w_ref, "__dict__"): + assert name not in w_ref.__dict__ + + del value + collect() + assert w_ref() is None + assert callbacks == [True] + + +for ref_type in (PlainRef, EmptyRef, KeyedRef): + check_ref_subclass(ref_type) + + +values = WeakValueDictionary() +key = ("namespace", "spam") +value = Value("live") +values[key] = value +assert values[key] is value +stored_ref = values.data[key] +assert stored_ref.key == key +assert stored_ref() is value +del value +collect() +assert key not in values + +print("OK") diff --git a/pyre/pyre-interpreter/src/baseobjspace.rs b/pyre/pyre-interpreter/src/baseobjspace.rs index 2ff461b5139..cf49b5c2b9e 100644 --- a/pyre/pyre-interpreter/src/baseobjspace.rs +++ b/pyre/pyre-interpreter/src/baseobjspace.rs @@ -4725,6 +4725,9 @@ pub(crate) fn native_slot_get( if unsafe { pyre_object::is_list(obj) } { return Ok(unsafe { pyre_object::listobject::w_list_slot_get(obj, index as usize) }); } + if unsafe { pyre_object::weakref::is_typed_weakref(obj) } { + return Ok(unsafe { pyre_object::weakref::w_weakref_object_slot_get(obj, index as usize) }); + } let w_dict = getdict(obj)?; if w_dict.is_null() { return Ok(None); @@ -4766,6 +4769,10 @@ pub(crate) fn native_slot_set( unsafe { pyre_object::listobject::w_list_slot_set(obj, index as usize, value) }; return Ok(true); } + if unsafe { pyre_object::weakref::is_typed_weakref(obj) } { + unsafe { pyre_object::weakref::w_weakref_object_slot_set(obj, index as usize, value) }; + return Ok(true); + } let w_dict = getdict(obj)?; if w_dict.is_null() { return Ok(false); @@ -4798,6 +4805,9 @@ pub(crate) fn native_slot_del(obj: PyObjectRef, name: &str, index: u32) -> Resul if unsafe { pyre_object::is_list(obj) } { return Ok(unsafe { pyre_object::listobject::w_list_slot_del(obj, index as usize) }); } + if unsafe { pyre_object::weakref::is_typed_weakref(obj) } { + return Ok(unsafe { pyre_object::weakref::w_weakref_object_slot_del(obj, index as usize) }); + } let w_dict = getdict(obj)?; if w_dict.is_null() { return Ok(false); diff --git a/pyre/pyre-interpreter/src/module/_pickle/pickler.rs b/pyre/pyre-interpreter/src/module/_pickle/pickler.rs index 80a8bb321f1..85116b331f4 100644 --- a/pyre/pyre-interpreter/src/module/_pickle/pickler.rs +++ b/pyre/pyre-interpreter/src/module/_pickle/pickler.rs @@ -30,6 +30,35 @@ fn cur_pickler(slot: usize) -> &'static mut W_Pickler { unsafe { &mut *(pyre_object::gc_roots::shadow_stack_get(slot) as *mut W_Pickler) } } +/// A reference kept for the duration of `pickle_core_impl`. Movable values +/// live in a shadow-stack slot and are re-read on every access so relocation +/// is observed; `PY_NULL` and immovable singletons can be held verbatim. +struct PinnedRef { + slot: Option, + immovable: PyObjectRef, +} + +impl PinnedRef { + fn new(value: PyObjectRef, movable: bool) -> Self { + let slot = if movable { + pyre_object::gc_roots::pin_root(value); + Some(pyre_object::gc_roots::shadow_stack_len() - 1) + } else { + None + }; + Self { + slot, + immovable: value, + } + } + + fn get(&self) -> PyObjectRef { + self.slot.map_or(self.immovable, |slot| { + pyre_object::gc_roots::shadow_stack_get(slot) + }) + } +} + /// Old-to-young / incremental-mark write barrier for a store into the /// pickler's own GC-pointer fields (`w_memo` / `w_dispatch_table` / /// `w_pers_func` / …). The GC transform emits `ll_writebarrier` after every @@ -97,9 +126,9 @@ struct PickleCtx { index: HashMap>, /// `persistent_id` callable resolved off the pickler (subclass override /// or set attribute), or `PY_NULL` when not defined. - pers_func: PyObjectRef, + pers_func: PinnedRef, /// `buffer_callback` for proto-5 out-of-band buffers, or `None`/`PY_NULL`. - buffer_callback: PyObjectRef, + buffer_callback: PinnedRef, /// `fast` mode — when set, memoization is skipped (no PUT/GET); a /// shallow cyclic-object guard (`fast_nesting` / `fast_memo`) still fires /// past `FAST_NESTING_LIMIT`. @@ -116,10 +145,10 @@ struct PickleCtx { /// Effective `dispatch_table` (the pickler's, else `copyreg.dispatch_table`) /// consulted by `type` for the reduce of an otherwise-unhandled object; /// `None`/`PY_NULL` when unavailable. - dispatch_table: PyObjectRef, + dispatch_table: PinnedRef, /// `reducer_override` callable (a subclass hook) consulted for every /// object, or `PY_NULL` when not defined. - reducer_override: PyObjectRef, + reducer_override: PinnedRef, } impl PickleCtx { @@ -1026,18 +1055,16 @@ fn pickle_core_impl( ) -> Result { let _roots = pyre_object::gc_roots::push_roots(); pyre_object::gc_roots::pin_root(w_obj); - if !pers_func.is_null() { - pyre_object::gc_roots::pin_root(pers_func); - } - if !buffer_callback.is_null() && !unsafe { pyre_object::is_none(buffer_callback) } { - pyre_object::gc_roots::pin_root(buffer_callback); - } - if !dispatch_table.is_null() && !unsafe { pyre_object::is_none(dispatch_table) } { - pyre_object::gc_roots::pin_root(dispatch_table); - } - if !reducer_override.is_null() { - pyre_object::gc_roots::pin_root(reducer_override); - } + let pers_func = PinnedRef::new(pers_func, !pers_func.is_null()); + let buffer_callback = PinnedRef::new( + buffer_callback, + !buffer_callback.is_null() && !unsafe { pyre_object::is_none(buffer_callback) }, + ); + let dispatch_table = PinnedRef::new( + dispatch_table, + !dispatch_table.is_null() && !unsafe { pyre_object::is_none(dispatch_table) }, + ); + let reducer_override = PinnedRef::new(reducer_override, !reducer_override.is_null()); // Pin the memo list and index its existing entries (a reused `Pickler` // carries memo state across `dump` calls until `clear_memo`). pyre_object::gc_roots::pin_root(w_memo); @@ -1179,11 +1206,9 @@ fn save(ctx: &mut PickleCtx, buf: &mut Framer, w_obj: PyObjectRef) -> Result<(), pyre_object::gc_roots::pin_root(w_obj); let slot = pyre_object::gc_roots::shadow_stack_len() - 1; buf.commit_frame(false)?; - if !ctx.pers_func.is_null() { - let w_pid = call_fn( - ctx.pers_func, - &[pyre_object::gc_roots::shadow_stack_get(slot)], - )?; + let pers_func = ctx.pers_func.get(); + if !pers_func.is_null() { + let w_pid = call_fn(pers_func, &[pyre_object::gc_roots::shadow_stack_get(slot)])?; if !unsafe { pyre_object::is_none(w_pid) } { save_pers(ctx, buf, w_pid) } else { @@ -1306,12 +1331,13 @@ fn dispatch_save(ctx: &mut PickleCtx, buf: &mut Framer, w_obj: PyObjectRef) -> R // default reduction. (interp_pickle.py:619-625 calls it earlier; CPython // 3.14 — the behaviour target — dispatches built-in types first, so the // hook never sees a list/dict/str and a repeated object hits the memo.) - if !ctx.reducer_override.is_null() { + let reducer_override = ctx.reducer_override.get(); + if !reducer_override.is_null() { let _roots = pyre_object::gc_roots::push_roots(); pyre_object::gc_roots::pin_root(w_obj); let slot = pyre_object::gc_roots::shadow_stack_len() - 1; let w_rv = call_fn( - ctx.reducer_override, + reducer_override, &[pyre_object::gc_roots::shadow_stack_get(slot)], )?; if !unsafe { pyre_object::is_not_implemented(w_rv) } { @@ -1482,7 +1508,7 @@ fn dispatch_table_reduce( ctx: &PickleCtx, w_obj: PyObjectRef, ) -> Result, PyError> { - let dt = ctx.dispatch_table; + let dt = ctx.dispatch_table.get(); if dt.is_null() { return Ok(None); } @@ -2056,8 +2082,9 @@ fn save_picklebuffer( } let (data, readonly) = crate::module::__pypy__::interp_buffer::buffer_view(wrapped)?; let mut in_band = true; - if !unsafe { pyre_object::is_none(ctx.buffer_callback) } { - let w_ret = call_fn(ctx.buffer_callback, &[w_obj])?; + let buffer_callback = ctx.buffer_callback.get(); + if !unsafe { pyre_object::is_none(buffer_callback) } { + let w_ret = call_fn(buffer_callback, &[w_obj])?; in_band = crate::baseobjspace::is_true(w_ret)?; } if in_band { diff --git a/pyre/pyre-interpreter/src/module/_weakref/interp__weakref.rs b/pyre/pyre-interpreter/src/module/_weakref/interp__weakref.rs index 8fe2403eb81..b4b6d204c78 100644 --- a/pyre/pyre-interpreter/src/module/_weakref/interp__weakref.rs +++ b/pyre/pyre-interpreter/src/module/_weakref/interp__weakref.rs @@ -78,10 +78,9 @@ fn write_attr(obj: PyObjectRef, name: &str, value: PyObjectRef) { } } -/// Field bridge for the exact typed `W_Weakref` and the still-generated -/// mapdict layout of a Python `weakref.ref` subclass. PyPy presents the same -/// `W_WeakrefBase` fields on both translated classes; only their physical -/// composition differs (`typedef.py:174-227`). +/// Field bridge for typed `W_Weakref` and mapdict `W_AbstractProxy` objects. +/// `W_WeakrefBase` (interp__weakref.py:160-202) supplies the same fields to +/// both; only their physical composition differs. #[inline] fn weakref_obj_weak(obj: PyObjectRef) -> PyObjectRef { if unsafe { pyre_object::weakref::is_typed_weakref(obj) } { @@ -128,12 +127,12 @@ fn weakref_set_hash(obj: PyObjectRef, value: PyObjectRef) { } /// Scoped GC root for a freshly-allocated instance still held only in a -/// Rust local. The weakref / proxy constructors allocate the instance, -/// then allocate a `GcWeakrefBox` (an `rweakref` `Weakref` via -/// `try_gc_alloc`) and grow the instance dict — each is a nursery -/// allocation that can drive a minor collection. Without a root the -/// not-yet-reachable instance is swept (its header zeroed), and a later -/// `write_attr` / `typedef::r#type` dereferences the dangling pointer. +/// Rust local. The proxy constructors allocate the instance, then allocate a +/// `GcWeakrefBox` (an `rweakref` `Weakref` via `try_gc_alloc`) and grow the +/// instance dict — each is a nursery allocation that can drive a minor +/// collection. Without a root the not-yet-reachable instance is swept (its +/// header zeroed), and a later `write_attr` / `typedef::r#type` dereferences +/// the dangling pointer. /// Registering the slot keeps the instance alive and relocates the local /// in place when the collector promotes it, mirroring `FrameLocalsRoot`. struct InstanceRoot { @@ -647,67 +646,41 @@ pub fn W_Weakref_new( w_obj: PyObjectRef, w_callable: PyObjectRef, ) -> PyObjectRef { - use pyre_object::objectobject::w_instance_new; // typedef.py:519 generic_new_descr → space.allocate_instance(W_Type, w_subtype) let actual_type = if w_subtype.is_null() { weakref_type() } else { w_subtype }; - // typeobject.py `allocate_instance`: a subclass whose Layout adds no - // storage keeps the builtin W_Weakref layout and changes only `w_class`. - // In particular `class R(ref): __slots__ = ()` has neither mapdict nor - // member slots in which the three interpreter-owned fields could live. + // `W_Weakref` (interp__weakref.py:195-257) / `descr__new__weakref` + // (interp__weakref.py:259-269): the three fields are interpreter-owned, so + // every subtype keeps the builtin payload. Its Python-level `__dict__` and + // slots use the same tagged carrier as other builtin subclasses. let exact_type = std::ptr::eq(actual_type, weakref_type()); - let shares_base_layout = !exact_type - && unsafe { - !pyre_object::w_type_get_hasdict(actual_type) - && pyre_object::w_type_get_nslots(actual_type) - == pyre_object::w_type_get_nslots(weakref_type()) - }; - if exact_type || shares_base_layout { - let _roots = pyre_object::gc_roots::push_roots(); - let root_base = pyre_object::gc_roots::shadow_stack_len(); - pyre_object::gc_roots::pin_root(w_obj); - pyre_object::gc_roots::pin_root(w_callable); - let w_obj_weak = pyre_object::weakref::w_gc_weakref_box_new_or_strong( - pyre_object::gc_roots::shadow_stack_get(root_base), - ); - pyre_object::gc_roots::pin_root(w_obj_weak); - let callable = pyre_object::gc_roots::shadow_stack_get(root_base + 1); - let callable = if !callable.is_null() && !unsafe { pyre_object::is_none(callable) } { - callable - } else { - pyre_object::PY_NULL - }; - let weakref = pyre_object::weakref::w_weakref_object_new( - pyre_object::gc_roots::shadow_stack_get(root_base + 2), - callable, - pyre_object::PY_NULL, - ); - return if exact_type { - weakref - } else { - crate::typedef::tag_subclass_instance(weakref, actual_type) - }; - } - - let mut obj = w_instance_new(actual_type); - let _root = InstanceRoot::new(&mut obj); - // W_WeakrefBase.__init__: self.w_obj_weak = weakref.ref(w_obj). - // Compute the box before `write_attr`: the allocation must not happen - // while a stale `obj` is captured as the call's receiver argument. - let w_obj_weak = pyre_object::weakref::w_gc_weakref_box_new_or_strong(w_obj); - write_attr(obj, ATTR_W_OBJ_WEAK, w_obj_weak); - let w_callable = if !w_callable.is_null() && !unsafe { pyre_object::is_none(w_callable) } { - w_callable + let _roots = pyre_object::gc_roots::push_roots(); + let root_base = pyre_object::gc_roots::shadow_stack_len(); + pyre_object::gc_roots::pin_root(w_obj); + pyre_object::gc_roots::pin_root(w_callable); + let w_obj_weak = pyre_object::weakref::w_gc_weakref_box_new_or_strong( + pyre_object::gc_roots::shadow_stack_get(root_base), + ); + pyre_object::gc_roots::pin_root(w_obj_weak); + let callable = pyre_object::gc_roots::shadow_stack_get(root_base + 1); + let callable = if !callable.is_null() && !unsafe { pyre_object::is_none(callable) } { + callable } else { - pyre_object::w_none() + pyre_object::PY_NULL }; - write_attr(obj, ATTR_W_CALLABLE, w_callable); - // W_Weakref.__init__: self.w_hash = None - write_attr(obj, ATTR_W_HASH, pyre_object::w_none()); - obj + let weakref = pyre_object::weakref::w_weakref_object_new( + pyre_object::gc_roots::shadow_stack_get(root_base + 2), + callable, + pyre_object::PY_NULL, + ); + if exact_type { + weakref + } else { + crate::typedef::tag_subclass_instance(weakref, actual_type) + } } #[allow(non_snake_case)] diff --git a/pyre/pyre-jit/src/eval.rs b/pyre/pyre-jit/src/eval.rs index 188841c16ef..27b7c883513 100644 --- a/pyre/pyre-jit/src/eval.rs +++ b/pyre/pyre-jit/src/eval.rs @@ -3906,8 +3906,9 @@ fn build_gc() -> Box { // `interp__weakref.py:193-205 W_Weakref` exact builtin payload. Like the // lifeline above, its allocation is selected by its translated GC layout; // Python class identity remains in the header's `w_class`. Append it after - // the lifeline so the already-published lifeline tid stays stable. The - // separate generated user-subclass layout remains mapdict-backed. + // the lifeline so the already-published lifeline tid stays stable. Every + // `weakref.ref` subclass instance carries this same payload, so its + // `w_slots` tail is traced here too. let weakref_descr = ::DESCRIPTOR; let weakref_object_tid = gc.register_type(TypeInfo::with_gc_ptrs( diff --git a/pyre/pyre-object/src/weakref.rs b/pyre/pyre-object/src/weakref.rs index fbe63992e2f..8fdb1e9ba82 100644 --- a/pyre/pyre-object/src/weakref.rs +++ b/pyre/pyre-object/src/weakref.rs @@ -107,6 +107,8 @@ pub struct W_Weakref { pub w_callable: PyObjectRef, /// `W_Weakref.__init__: self.w_hash = None`. pub w_hash: PyObjectRef, + /// Native-subclass `__slots__` storage indexed by `Member.index`. + pub w_slots: PyObjectRef, } /// Allocate the exact builtin W_Weakref payload. The caller has already @@ -122,6 +124,7 @@ pub fn w_weakref_object_new( w_obj_weak, w_callable, w_hash, + w_slots: PY_NULL, }) } @@ -157,6 +160,35 @@ pub unsafe fn w_weakref_object_set_hash(obj: PyObjectRef, value: PyObjectRef) { crate::gc_hook::try_gc_write_barrier(obj as *mut u8); } +/// Address of `W_Weakref::w_slots` for the shared slot helpers. +/// +/// # Safety +/// `obj` must point to a valid `W_Weakref`. +unsafe fn weakref_slots_field(obj: PyObjectRef) -> *mut PyObjectRef { + unsafe { &mut (*(obj as *mut W_Weakref)).w_slots } +} + +/// # Safety +/// The caller must uphold every validity, runtime-type, aliasing, and lifetime +/// invariant required by the object and pointer arguments for the entire call. +pub unsafe fn w_weakref_object_slot_get(obj: PyObjectRef, index: usize) -> Option { + unsafe { crate::slots::slot_get(obj, index, weakref_slots_field) } +} + +/// # Safety +/// The caller must uphold every validity, runtime-type, aliasing, and lifetime +/// invariant required by the object and pointer arguments for the entire call. +pub unsafe fn w_weakref_object_slot_set(obj: PyObjectRef, index: usize, value: PyObjectRef) { + unsafe { crate::slots::slot_set(obj, index, value, weakref_slots_field) } +} + +/// # Safety +/// The caller must uphold every validity, runtime-type, aliasing, and lifetime +/// invariant required by the object and pointer arguments for the entire call. +pub unsafe fn w_weakref_object_slot_del(obj: PyObjectRef, index: usize) -> bool { + unsafe { crate::slots::slot_del(obj, index, weakref_slots_field) } +} + /// GC type id for the WEAKREF GcStruct. Registered by /// `pyre-jit::eval::init` after `W_INT_MUTABLE_CELL` and before the /// per-exception kind loop. A `debug_assert_eq!` in the registration @@ -550,6 +582,16 @@ mod tests { } assert_eq!(unsafe { w_weakref_object_hash(weakref) }, hash); assert!(unsafe { w_weakref_object_callable(weakref) }.is_null()); + + let slot_value = 0x7000_usize as PyObjectRef; + unsafe { w_weakref_object_slot_set(weakref, 2, slot_value) }; + assert_eq!( + unsafe { w_weakref_object_slot_get(weakref, 2) }, + Some(slot_value) + ); + assert!(unsafe { w_weakref_object_slot_del(weakref, 2) }); + assert_eq!(unsafe { w_weakref_object_slot_get(weakref, 2) }, None); + assert_eq!( W_Weakref::DESCRIPTOR.ptr_offsets, &[ @@ -557,6 +599,7 @@ mod tests { std::mem::offset_of!(W_Weakref, w_obj_weak), std::mem::offset_of!(W_Weakref, w_callable), std::mem::offset_of!(W_Weakref, w_hash), + std::mem::offset_of!(W_Weakref, w_slots), ] ); }