diff --git a/pyre/pyre-interpreter/src/pyframe.rs b/pyre/pyre-interpreter/src/pyframe.rs index f0248723f37..7b1d286c3b1 100644 --- a/pyre/pyre-interpreter/src/pyframe.rs +++ b/pyre/pyre-interpreter/src/pyframe.rs @@ -1080,11 +1080,15 @@ unsafe fn alloc_frame_block( allocation: FrameLocalsArrayAllocation, ) -> *mut FrameBlock { if allocation == FrameLocalsArrayAllocation::OldGenGc { - if let Some(raw) = pyre_object::gc_hook::try_gc_alloc( - FRAME_BLOCK_GC_TYPE_ID, - std::mem::size_of::(), + // `FRAME_BLOCK_GC_TYPE_ID` registers `previous` as a traced edge, so + // the walker forwards the rest of the chain unconditionally once it + // reaches a managed block. A `malloc_raw` block spliced in after a + // failed managed allocation would be forwarded without a header. + let payload_size = std::mem::size_of::(); + if let Some(raw) = pyre_object::gc_hook::GcAllocOutcome::from_hook( + pyre_object::gc_hook::try_gc_alloc(FRAME_BLOCK_GC_TYPE_ID, payload_size), ) - .filter(|raw| !raw.is_null()) + .allocated_or_abort(payload_size) { unsafe { std::ptr::write(raw as *mut FrameBlock, block) }; return raw as *mut FrameBlock; diff --git a/pyre/pyre-object/src/gc_hook.rs b/pyre/pyre-object/src/gc_hook.rs index 43f27650f3f..6643a227189 100644 --- a/pyre/pyre-object/src/gc_hook.rs +++ b/pyre/pyre-object/src/gc_hook.rs @@ -13,11 +13,18 @@ //! the same pointer to every thread, and a collector running on an arbitrary //! thread must see it even if that thread never ran the install path. //! -//! Callers use [`try_gc_alloc`] which returns `None` when no hook is -//! installed — they fall back to the `Box::into_raw` path in -//! that case. Incremental migration drops the `Box::into_raw` -//! fallback at each call site as the hook's reliability is verified -//! under the full bench suite. +//! Callers use [`try_gc_alloc`], whose two non-pointer answers mean +//! different things: `None` is *no GC owns the heap*, `Some(null)` is *a GC +//! owns the heap and this allocation failed*. Only the first licenses the +//! `Box::into_raw` path. [`GcAllocOutcome`] names the two so a call site +//! cannot collapse them: substituting a raw object for a failed managed one +//! puts a headerless payload into the traced graph, where an owner field +//! registered as a gc-pointer offset forwards it as though it had a header. +//! +//! Dropping the `Box::into_raw` fallback at a call site is therefore a +//! question of whether that site can still run before `init_gc_subsystem`, +//! not of how reliable the hook has proven under a bench suite: the state +//! that matters is `Some(null)`, which a green benchmark never exercises. //! //! Layering: this module defines the function-pointer slots only. Wire-up //! lives in `pyre-jit`. @@ -129,6 +136,70 @@ pub fn try_gc_alloc(type_id: u32, payload_size: usize) -> Option<*mut u8> { GC_ALLOC_HOOK.get().map(|f| f(type_id, payload_size)) } +/// What an allocation hook answered, with the two non-pointer states kept +/// apart. +/// +/// `malloc_fixedsize` (incminimark.py:640-693) has neither state. The GC is a +/// prebuilt constant (framework.py:254), so a route always exists, and a +/// nursery that cannot satisfy the request reaches `collect_and_reserve` +/// (incminimark.py:981-985), which raises MemoryError rather than handing a +/// null back. Both states are pyre's own, and they are not interchangeable: +/// +/// * [`NoRoute`](Self::NoRoute) — nothing owns the heap yet: a bare unit +/// test, the pre-`init_gc_subsystem` bootstrap, or a build with no backend. +/// The caller's `malloc_raw` path *is* the whole heap there, so taking it +/// keeps the object graph consistent. +/// * [`Failed`](Self::Failed) — a GC owns the heap and could not satisfy the +/// request. `malloc_raw` is the wrong answer now: the object would hold +/// managed references the collector never traces or forwards, and its +/// missing header lets a type-id witness misread the words before it. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum GcAllocOutcome { + Allocated(*mut u8), + Failed, + NoRoute, +} + +impl GcAllocOutcome { + /// Classify a hook result: `None` is [`NoRoute`](Self::NoRoute), + /// `Some(null)` is [`Failed`](Self::Failed). + #[inline] + pub fn from_hook(result: Option<*mut u8>) -> Self { + match result { + Some(raw) if !raw.is_null() => Self::Allocated(raw), + Some(_) => Self::Failed, + None => Self::NoRoute, + } + } + + /// The allocated pointer, or `None` for [`NoRoute`](Self::NoRoute) so the + /// caller takes its own non-GC path. A [`Failed`](Self::Failed) does not + /// return: see [`gc_alloc_failed`]. + #[inline] + pub fn allocated_or_abort(self, payload_size: usize) -> Option<*mut u8> { + match self { + Self::Allocated(raw) => Some(raw), + Self::Failed => gc_alloc_failed(payload_size), + Self::NoRoute => None, + } + } +} + +/// A GC that owns the heap could not satisfy an allocation. +/// +/// `collect_and_reserve` (incminimark.py:981-985) raises MemoryError at this +/// point, so no caller of `malloc_fixedsize` observes a null. The pyre callers +/// return a bare pointer and run under JIT frames that cannot unwind, so the +/// failure aborts instead — the answer `alloc_typed_items_block_nursery` +/// (`object_array.rs`) already gives for a digit array whose allocation fails. +#[cold] +#[inline(never)] +pub fn gc_alloc_failed(payload_size: usize) -> ! { + let layout = std::alloc::Layout::from_size_align(payload_size, std::mem::align_of::()) + .unwrap_or_else(|_| std::alloc::Layout::new::()); + std::alloc::handle_alloc_error(layout) +} + /// Install the `malloc_fast` allocation callback. pub fn register_gc_alloc_fast_hook(hook: GcAllocFastHookFn) { GC_ALLOC_FAST_HOOK.set(Some(hook)); @@ -244,6 +315,19 @@ pub fn try_gc_alloc_stable_raw(type_id: u32, payload_size: usize) -> *mut u8 { try_gc_alloc_stable(type_id, payload_size).unwrap_or(core::ptr::null_mut()) } +/// [`try_gc_alloc_stable_raw`] for a caller whose fallback is `malloc_raw`. +/// +/// Returns null only for [`GcAllocOutcome::NoRoute`]; a GC that owns the heap +/// and then fails aborts rather than letting the caller substitute an untraced +/// object. Keeps the raw return so the residualised call carries no +/// discriminant, for the reason [`try_gc_alloc_stable_raw`] documents. +#[majit_macros::dont_look_inside] +pub fn try_gc_alloc_stable_or_abort(type_id: u32, payload_size: usize) -> *mut u8 { + GcAllocOutcome::from_hook(try_gc_alloc_stable(type_id, payload_size)) + .allocated_or_abort(payload_size) + .unwrap_or(core::ptr::null_mut()) +} + majit_gc::global_hook!(static GC_ALLOC_COLLECTING_HOOK: GcAllocHookFn); /// Install the *collecting* nursery allocation callback. @@ -881,6 +965,41 @@ mod tests { clear_gc_alloc_hook(); } + #[test] + fn outcome_separates_no_route_from_failure() { + let mut probe = 0u8; + assert_eq!(GcAllocOutcome::from_hook(None), GcAllocOutcome::NoRoute); + assert_eq!( + GcAllocOutcome::from_hook(Some(std::ptr::null_mut())), + GcAllocOutcome::Failed + ); + assert_eq!( + GcAllocOutcome::from_hook(Some(&mut probe as *mut u8)), + GcAllocOutcome::Allocated(&mut probe as *mut u8) + ); + } + + #[test] + fn only_no_route_returns_the_caller_to_its_raw_path() { + // `Failed` does not return at all, so the surviving `None` is the sole + // licence for a `malloc_raw` fallback. + assert!(GcAllocOutcome::NoRoute.allocated_or_abort(24).is_none()); + let mut probe = 0u8; + assert_eq!( + GcAllocOutcome::Allocated(&mut probe as *mut u8).allocated_or_abort(24), + Some(&mut probe as *mut u8) + ); + } + + #[test] + fn installed_hook_returning_null_classifies_as_failure_not_no_route() { + let _hook_lock = hook_test_guard(); + register_gc_alloc_hook(null_hook); + let outcome = GcAllocOutcome::from_hook(unsafe { try_gc_alloc_fast(1, 8) }); + clear_gc_alloc_hook(); + assert_eq!(outcome, GcAllocOutcome::Failed); + } + #[test] fn managed_bigint_digits_do_not_fall_back_to_raw_after_hook_failure() { let _hook_lock = hook_test_guard(); diff --git a/pyre/pyre-object/src/rbigint.rs b/pyre/pyre-object/src/rbigint.rs index 9f8aea8fc01..8e1eab26807 100644 --- a/pyre/pyre-object/src/rbigint.rs +++ b/pyre/pyre-object/src/rbigint.rs @@ -586,15 +586,18 @@ pub(crate) fn alloc_rbigint_nursery_impl( } let tid = rbigint_gc_type_id(); let mut needs_write_barrier = true; + // A `Some(null)` here means the GC owns the heap and could not satisfy the + // request; `malloc_raw` below would then leave `_digits` — this payload's + // one traced edge — unreachable to the collector. if tid != 0 - && let Some(raw) = unsafe { + && let Some(raw) = crate::gc_hook::GcAllocOutcome::from_hook(unsafe { crate::gc_hook::try_gc_alloc_fast_with_placement( tid, RBIGINT_PAYLOAD_SIZE, &mut needs_write_barrier, ) - } - .filter(|pointer| !pointer.is_null()) + }) + .allocated_or_abort(RBIGINT_PAYLOAD_SIZE) { unsafe { std::ptr::write(raw as *mut RBigInt, value); @@ -638,15 +641,20 @@ fn alloc_rbigint_nursery_collecting_impl( // selects. let digit_slot = (&mut value._digits as *mut *mut TypedItemsBlock).cast::<*mut u8>(); let mut needs_write_barrier = true; - let raw = unsafe { + // `NoRoute` falls through to the no-collect path below, which has its + // own hook to try. A failure does not: this allocation already ran a + // minor collection, so retrying the no-collect path would only reach + // its `malloc_raw` fallback and hide the failure behind an untraced + // payload. + let raw = crate::gc_hook::GcAllocOutcome::from_hook(unsafe { crate::gc_hook::try_gc_alloc_fast_collecting_rooted( tid, RBIGINT_PAYLOAD_SIZE, digit_slot, &mut needs_write_barrier, ) - } - .filter(|pointer| !pointer.is_null()); + }) + .allocated_or_abort(RBIGINT_PAYLOAD_SIZE); if let Some(raw) = raw { unsafe { std::ptr::write(raw as *mut RBigInt, value); @@ -687,7 +695,7 @@ pub fn alloc_rbigint_stable(value: RBigInt) -> *mut RBigInt { } let tid = rbigint_gc_type_id(); if tid != 0 { - let raw = crate::gc_hook::try_gc_alloc_stable_raw(tid, RBIGINT_PAYLOAD_SIZE); + let raw = crate::gc_hook::try_gc_alloc_stable_or_abort(tid, RBIGINT_PAYLOAD_SIZE); if !raw.is_null() { unsafe { std::ptr::write(raw as *mut RBigInt, value);