gc: separate a failed managed allocation from an absent GC

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::<FrameBlock>(),
+        // `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::<FrameBlock>();
+        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;

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::<usize>())
+        .unwrap_or_else(|_| std::alloc::Layout::new::<usize>());
+    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();

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);