gc: run the allocation-point GC trigger outside the &mut Arena borrow (#7022)

changelog.d/7050-gc-allocation-point-trigger-outside-arena-borrow.md

@@ -0,0 +1,89 @@
+### `fix(gc)`: run the allocation-point GC trigger outside the `&mut Arena` borrow (#7022)
+
+`test_gap_repsel_gc_stress` SIGSEGV'd deterministically in the 12
+`gc_repsel_matrix.sh` arms carrying `PERRY_CONSERVATIVE_STACK_SCAN=off` — the
+arms in which #7019's default-on evacuating young-gen scavenge is eligible at
+all. The fault landed inside mimalloc's `realloc`, reached from the collector's
+own `Vec` growth (`ValidPointerSetBuilder::step` → `RawVec::grow_one`): the
+malloc heap was already corrupt on arrival.
+
+**Root cause.** `Arena::alloc(&mut self, ..)` called `crate::gc::gc_check_trigger()`
+from inside its own borrow. A collection *allocates into the arenas* — promotion
+and C4b evacuation call `arena_alloc_gc_old`, an evacuating minor fills a
+survivor semispace — and either can reach `Arena::install_fresh_block` →
+`self.blocks.push(..)` on the **same** `Arena` the allocating frame is holding.
+The `Vec` growth frees the buffer the outer frame goes on to index
+(`self.blocks[idx]`, `for i in 0..self.blocks.len()`), and `&mut` carries
+`noalias`, so the outer frame is equally entitled to have cached
+`blocks.ptr`/`len` across the call.
+
+Instrumented on the reproducer this is direct and frequent: 204 re-entries in a
+single run, including `install_fresh_block gen=Old space=Old` while
+`Arena::alloc` on the old arena holds the borrow, with `self.blocks`'s length
+changing underneath it. The crashing stack is that shape exactly:
+`js_array_grow → arena_alloc_gc_old → Arena::alloc → gc_check_trigger →
+gc_collect_full_mark_sweep_with_trigger → … → _mi_theap_realloc_zero`.
+
+The hazard predates #7019 but was **latent**: before the moving minor, a
+collection triggered from an allocation point did not itself install arena blocks
+anywhere near this often. That is why this is a PASS → FAIL correctly attributed
+to #7019 without #7019 containing the defect, and it explains the discriminator —
+the copying minor is only eligible when the conservative stack scan is off.
+
+**Fix.** `arena_cell_alloc(*mut Arena, size, align)` is the new collecting entry
+point: try the current block under a borrow that ends with the statement, run
+`gc_check_trigger()` with **no arena borrow live**, then re-derive a fresh borrow
+for the slow path. `Arena::alloc(&mut self, ..)` is now collection-free, and the
+five thread-local entry points (`arena_alloc`, `arena_alloc_longlived`,
+`arena_alloc_old`, `arena_alloc_gc_survivor`, `js_inline_arena_slow_alloc`) route
+through it. The two that also touch `INLINE_STATE` keep those borrows short for
+the same reason — `Arena::resync_inline_to_current` mutates `INLINE_STATE` from
+inside the collection. When the GC runs is unchanged.
+
+**Measured.** Same commit, same compiler, only the four arena files differing;
+release build, auto-optimized binaries (with `PERRY_NO_AUTO_OPTIMIZE=1` the crash
+does not reproduce at all, which is why the harness deliberately does not set
+it); macOS arm64, node 26.5.0; `PERRY_GC_HEAP_LIMIT=8 PERRY_GC_INCREMENTAL=0
+PERRY_CONSERVATIVE_STACK_SCAN=off`.
+
+| `PERRY_GC_SCAVENGE_NURSERY_MB` | before | after |
+|---|---|---|
+| 1, 2, 3, 4, 5, 6, 8, 10, 12, 24, 32, 64 (× 3) | 0 | 0 |
+| **16** (the default), × 20 | **139 × 20** | **0 × 20**, byte-exact vs node |
+| default, no override, × 5 | **139 × 5** | **0 × 5** |
+
+`PERRY_GC_FORCE_EVACUATE=1`, `+ PERRY_GC_VERIFY_EVACUATION=1` and
+`PERRY_GC_FROMSPACE_SCAN=1` on top of the same base: 5/5 crashes before, 0/5
+after. `cargo test --release -p perry-runtime --lib -- --test-threads=1`: 1521
+passed, 0 failed.
+
+**Matrix.** `scripts/gc_repsel_matrix.sh --arms all --pressure 8`, 440 cells:
+`PASS=324 UNVER=91 XFAIL=1 FAIL=24`, from the `PASS=302 UNVER=91 XFAIL=1 FAIL=46`
+baseline — **+22 PASS / -22 FAIL, no cell regressed**. `repsel_gc_stress` is PASS
+in all 20 arms (was FAIL in 12), and so is `repsel_scalar_replaced_locals`
+(#7023, was intermittently FAIL in 11) — the same defect. The residual 24 FAILs
+are the two #6981 `p4a3` numarray rows, unchanged. Liveness is intact:
+`evac_minor`/`force_evac`/`force_verify` still report `copy-minor 21/22`.
+
+**Regression coverage** (`cargo-test`-visible, per #5960):
+`arena::tests::allocation_point_gc_trigger_runs_with_no_live_arena_borrow` pins
+that the trigger reached from `arena_cell_alloc` sees an arena-borrow depth of 0
+(and that it was reached at all, so the test cannot pass vacuously);
+`arena::tests::raw_arena_alloc_method_never_reaches_the_gc_trigger` pins that
+`Arena::alloc` stays collection-free, so a refactor cannot move the trigger back
+under the borrow. The borrow-depth probe is `cfg(test)`-only — production
+allocation pays nothing. Teeth verified by sabotage: reinstating either half of
+the old shape turns the matching test red.
+
+**Investigation note.** #7022's dossier classified this as a missing rewrite of
+an old→young remembered-set edge, from `PERRY_GC_FROMSPACE_SCAN`'s
+`lost_dirty`/`dirty_but_missed` split. That classification was measuring noise:
+extending the scan with two axes — is the offender's *owner* itself
+`GC_FLAG_FORWARDED`, and is the slot inside the collector's own rewrite
+enumeration — shows `enumerated=0` on every cycle of every run, with 99%+ of
+offenders being the dead payload of `js_array_grow`'s permanent growth stubs
+(#233), the rest uninitialized bytes inside an array's unused capacity (arena
+blocks are recycled, not zeroed, and `move_young` copies the whole payload) and
+unaligned words that `CopyingPointerSet::decode_bits` rejects by construction.
+The scan's offender counts are unchanged by this fix. See #7050 for the full
+split; a follow-up against #7041/#7035 will teach the instrument to report it.

crates/perry-runtime/src/arena/allocators.rs

@@ -14,21 +14,32 @@ use super::*;
 #[inline]
 pub fn arena_alloc(size: usize, align: usize) -> *mut u8 {
     INLINE_STATE.with(|inline_s| unsafe {
-        let inline = &mut *inline_s.get();
+        let inline_ptr = inline_s.get();
         ARENA.with(|a| {
-            let arena = &mut *(*a).get();
+            let arena_ptr = (*a).get();
             // Sync inline → block before allocating, if the inline
-            // state has been initialized.
-            if !inline.data.is_null() {
-                arena.blocks[arena.current].offset = inline.offset;
+            // state has been initialized. Borrows are deliberately
+            // short-lived: `arena_cell_alloc` runs the GC between two
+            // disjoint borrows, and the collector mutates BOTH the arena
+            // and `INLINE_STATE` (`Arena::resync_inline_to_current`). #7022.
+            if !(*inline_ptr).data.is_null() {
+                let offset = (*inline_ptr).offset;
+                let arena = &mut *arena_ptr;
+                let current = arena.current;
+                arena.blocks[current].offset = offset;
             }
-            let ptr = arena.alloc(size, align);
+            let ptr = crate::arena::arena_cell_alloc(arena_ptr, size, align);
             // Resync block → inline (may have advanced to a new block).
-            if !inline.data.is_null() {
-                let block = &arena.blocks[arena.current];
-                inline.data = block.data;
-                inline.offset = block.offset;
-                inline.size = block.size;
+            if !(*inline_ptr).data.is_null() {
+                let (data, offset, block_size) = {
+                    let arena = &*arena_ptr;
+                    let block = &arena.blocks[arena.current];
+                    (block.data, block.offset, block.size)
+                };
+                let inline = &mut *inline_ptr;
+                inline.data = data;
+                inline.offset = offset;
+                inline.size = block_size;
             }
             ptr
         })
@@ -41,10 +52,7 @@ pub fn arena_alloc(size: usize, align: usize) -> *mut u8 {
 /// (`js_string_from_bytes_longlived`, `js_array_alloc_with_length_longlived`),
 /// not hot-path `new ClassName()` bump allocations.
 pub fn arena_alloc_longlived(size: usize, align: usize) -> *mut u8 {
-    LONGLIVED_ARENA.with(|a| unsafe {
-        let arena = &mut *a.get();
-        arena.alloc(size, align)
-    })
+    LONGLIVED_ARENA.with(|a| unsafe { crate::arena::arena_cell_alloc(a.get(), size, align) })
 }
 
 /// Allocate a GcHeader-prefixed object from the longlived arena (issue #179).
@@ -90,10 +98,7 @@ pub fn arena_alloc_gc_longlived(size: usize, align: usize, obj_type: u8) -> *mut
 /// touch) so codegen's hot bump-pointer loop on `new ClassName()`
 /// stays exclusively pinned to the nursery.
 pub fn arena_alloc_old(size: usize, align: usize) -> *mut u8 {
-    OLD_ARENA.with(|a| unsafe {
-        let arena = &mut *a.get();
-        arena.alloc(size, align)
-    })
+    OLD_ARENA.with(|a| unsafe { crate::arena::arena_cell_alloc(a.get(), size, align) })
 }
 
 pub(crate) fn arena_alloc_old_excluding_pages(
@@ -174,6 +179,16 @@ pub(crate) fn with_survivor_arena_mut<R>(idx: usize, f: impl FnOnce(&mut Arena)
     }
 }
 
+/// Raw-pointer counterpart of [`with_survivor_arena_mut`] for callers that must
+/// not hold an `&mut Arena` across a GC trigger (#7022).
+pub(crate) fn with_survivor_arena_cell<R>(idx: usize, f: impl FnOnce(*mut Arena) -> R) -> R {
+    match idx {
+        0 => SURVIVOR_ARENA_0.with(|a| f(a.get())),
+        1 => SURVIVOR_ARENA_1.with(|a| f(a.get())),
+        _ => unreachable!("invalid survivor arena index"),
+    }
+}
+
 pub(crate) fn with_survivor_arena<R>(idx: usize, f: impl FnOnce(&Arena) -> R) -> R {
     match idx {
         0 => SURVIVOR_ARENA_0.with(|a| unsafe { f(&*a.get()) }),
@@ -189,7 +204,9 @@ pub(crate) fn arena_alloc_gc_survivor(size: usize, align: usize, obj_type: u8) -
 
     let total = gc_padded_total_size(size, align);
     let idx = inactive_survivor_index();
-    let raw = with_survivor_arena_mut(idx, |arena| arena.alloc(total, align));
+    let raw = with_survivor_arena_cell(idx, |cell| unsafe {
+        crate::arena::arena_cell_alloc(cell, total, align)
+    });
 
     unsafe {
         let header = raw as *mut GcHeader;

crates/perry-runtime/src/arena/block.rs

@@ -341,24 +341,18 @@ impl Arena {
         }
     }
 
+    /// Fast path only: bump within the current block. Never collects, never
+    /// pushes a block — so it is safe to call under a short `&mut` borrow.
     #[inline]
-    pub(crate) fn alloc(&mut self, size: usize, align: usize) -> *mut u8 {
-        // Try current block first
-        if let Some(ptr) = self.try_block_alloc(self.current, size, align) {
-            return ptr;
-        }
-
-        // Current block is full. Check GC trigger first — if it fires
-        // and reclaims at least one fully-empty block (via
-        // `arena_reset_empty_blocks`), we may be able to reuse that
-        // block instead of pushing a new one.
-        //
-        // Threshold pressure is paid through bounded mutator-assist work.
-        // A completed assist cycle may reset blocks before we retry; an
-        // incomplete cycle leaves the debt active for later host or allocator
-        // steps.
-        crate::gc::gc_check_trigger();
+    pub(crate) fn try_alloc_current(&mut self, size: usize, align: usize) -> Option<*mut u8> {
+        self.try_block_alloc(self.current, size, align)
+    }
 
+    /// Everything `alloc` does *after* the GC trigger: retry the (possibly
+    /// newly reset) current block, scan the other blocks, then install a fresh
+    /// one. Split out of `alloc` for #7022 — see [`arena_cell_alloc`].
+    #[inline]
+    pub(crate) fn alloc_after_gc(&mut self, size: usize, align: usize) -> *mut u8 {
         // Retry the (possibly newly-reset) current block. arena.current
         // may have been changed by arena_reset_empty_blocks to point
         // at the lowest reset block.
@@ -391,6 +385,17 @@ impl Arena {
         self.alloc_fresh_block(size, align)
     }
 
+    /// GC-free allocation. Used by paths that already run inside a collection
+    /// (`alloc_excluding_pages`) and by the tests; the collecting entry point
+    /// is [`arena_cell_alloc`].
+    #[inline]
+    pub(crate) fn alloc(&mut self, size: usize, align: usize) -> *mut u8 {
+        if let Some(ptr) = self.try_alloc_current(size, align) {
+            return ptr;
+        }
+        self.alloc_after_gc(size, align)
+    }
+
     pub(crate) fn alloc_excluding_pages(
         &mut self,
         size: usize,
@@ -420,7 +425,133 @@ impl Arena {
     }
 }
 
+/// Allocate from an arena that lives behind a thread-local `UnsafeCell`,
+/// running the allocation-point GC trigger **between** two disjoint borrows.
+///
+/// # Why the borrow has to be split (#7022)
+///
+/// `gc_check_trigger()` can run a full mark-sweep or an evacuating minor, and
+/// both of those *allocate into the arenas*: promotion and C4b evacuation call
+/// `arena_alloc_gc_old`, the copying minor fills a survivor semispace, and
+/// either may reach `install_fresh_block` → `self.blocks.push(..)`. When the
+/// trigger was called from inside `Arena::alloc(&mut self, ..)`, that push ran
+/// against the **same** `Arena` the caller was holding a live `&mut` to: a
+/// `Vec` growth then frees the buffer the outer frame goes on to index
+/// (`self.blocks[idx]`), and `&mut` carries `noalias`, so the outer frame is
+/// also entitled to have cached `blocks.ptr`/`len` across the call. Measured on
+/// the #7022 reproducer: `install_fresh_block gen=Old space=Old` fires while
+/// `Arena::alloc` on the old arena holds the borrow, and `self.blocks`'s length
+/// changes underneath it.
+///
+/// #7019 (default-on evacuating young-gen scavenge) is what made this latent
+/// hazard live: before it, a collection triggered from an allocation point did
+/// not itself allocate arena blocks anywhere near this often.
+///
+/// Taking a `*mut Arena` and re-deriving a short-lived `&mut` per statement
+/// keeps the two borrows disjoint, so the collector may freely mutate the arena
+/// while it runs.
+///
+/// # Safety
+/// `arena` must be the `UnsafeCell` payload of a live thread-local `Arena` for
+/// the current thread.
+#[inline]
+pub(crate) unsafe fn arena_cell_alloc(arena: *mut Arena, size: usize, align: usize) -> *mut u8 {
+    // Try current block first, under a borrow that ends with this statement.
+    {
+        let _borrow = ArenaBorrowGuard::new();
+        if let Some(ptr) = (*arena).try_alloc_current(size, align) {
+            return ptr;
+        }
+    }
+
+    // Current block is full. Check the GC trigger first — if it fires and
+    // reclaims at least one fully-empty block (via `arena_reset_empty_blocks`),
+    // we may be able to reuse that block instead of pushing a new one.
+    //
+    // Threshold pressure is paid through bounded mutator-assist work. A
+    // completed assist cycle may reset blocks before we retry; an incomplete
+    // cycle leaves the debt active for later host or allocator steps.
+    //
+    // NO ARENA BORROW IS LIVE HERE. See the function docs.
+    note_gc_trigger_arena_borrow_depth();
+    crate::gc::gc_check_trigger();
+
+    let _borrow = ArenaBorrowGuard::new();
+    (*arena).alloc_after_gc(size, align)
+}
+
+// ---------------------------------------------------------------------------
+// #7022 invariant instrumentation: "no `&mut Arena` borrow is live while the
+// allocation-point GC trigger runs".
+//
+// Compiled ONLY under `cfg(test)`, so production allocation pays nothing. The
+// unit tests in `arena::tests` read `gc_trigger_arena_borrow_depth()` after
+// forcing an arena allocation past its current block; a refactor that puts the
+// trigger back inside the borrow makes them red.
+// ---------------------------------------------------------------------------
+
+#[cfg(test)]
+thread_local! {
+    /// Number of `&mut Arena` borrows currently live inside `arena_cell_alloc`.
+    static ARENA_BORROW_DEPTH: Cell<u32> = const { Cell::new(0) };
+    /// `ARENA_BORROW_DEPTH` sampled immediately before the most recent
+    /// `gc_check_trigger()` call made from `arena_cell_alloc`, and how many
+    /// such calls have been made.
+    static GC_TRIGGER_BORROW_DEPTH: Cell<u32> = const { Cell::new(u32::MAX) };
+    static GC_TRIGGER_CALLS: Cell<u32> = const { Cell::new(0) };
+}
+
+/// RAII marker for a live `&mut Arena` borrow (test builds only).
+pub(crate) struct ArenaBorrowGuard;
+
+impl ArenaBorrowGuard {
+    #[inline(always)]
+    fn new() -> Self {
+        #[cfg(test)]
+        ARENA_BORROW_DEPTH.with(|d| d.set(d.get() + 1));
+        ArenaBorrowGuard
+    }
+}
+
+impl Drop for ArenaBorrowGuard {
+    #[inline(always)]
+    fn drop(&mut self) {
+        #[cfg(test)]
+        ARENA_BORROW_DEPTH.with(|d| d.set(d.get().saturating_sub(1)));
+    }
+}
+
+#[inline(always)]
+fn note_gc_trigger_arena_borrow_depth() {
+    #[cfg(test)]
+    {
+        let depth = ARENA_BORROW_DEPTH.with(Cell::get);
+        GC_TRIGGER_BORROW_DEPTH.with(|d| d.set(depth));
+        GC_TRIGGER_CALLS.with(|c| c.set(c.get() + 1));
+    }
+}
+
+/// Arena-borrow depth observed at the most recent allocation-point GC trigger,
+/// or `u32::MAX` if no trigger has been reached yet on this thread.
+#[cfg(test)]
+pub(crate) fn gc_trigger_arena_borrow_depth() -> u32 {
+    GC_TRIGGER_BORROW_DEPTH.with(Cell::get)
+}
+
+/// How many allocation-point GC triggers `arena_cell_alloc` has reached.
+#[cfg(test)]
+pub(crate) fn gc_trigger_arena_calls() -> u32 {
+    GC_TRIGGER_CALLS.with(Cell::get)
+}
+
+#[cfg(test)]
+pub(crate) fn reset_gc_trigger_arena_probe() {
+    GC_TRIGGER_BORROW_DEPTH.with(|d| d.set(u32::MAX));
+    GC_TRIGGER_CALLS.with(|c| c.set(0));
+}
+
 thread_local! {
+
     /// Cached running sum of `block.size` across every arena (general,
     /// longlived, old-gen). `arena_total_bytes()` previously walked
     /// every block of every arena summing this on every call — and