Revert "Pins hold in the page: pinned spans kept from the arena, armed per thread"

This reverts commit 62b38fc2. hp30 and hp31 (shim-m6, which has it) hung
before Hollow Knight's first present. hp32 has the same placement-window
pins, units and ld.so unit on shim-m5, without it: every pin lands, the
first frame is at 34 s, and 100 presents at 66 s. Pins in the placement
window (046bec02) need neither the arena reservation nor per-thread arming.
Which of the two hangs the game is not known.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_1896c5b302a54696aaa92c5f4047f20b

crates/gex-kernel/src/mem.rs

@@ -122,10 +122,6 @@ pub struct MemoryMap {
     /// there. `None` for every M32 process, whose map behaves exactly as it
     /// always has.
     high: Option<(u64, u64)>,
-    /// Spans pinned objects own (`Kernel::pins`): the arena hands none of
-    /// their addresses to anything else, so a pinned library finds its base
-    /// free whenever it is loaded. Only an address hint lands there.
-    reserved: Vec<(u64, u64)>,
 }
 
 /// Bytes an M32 process asked for and could not be given for lack of address
@@ -175,7 +171,6 @@ impl MemoryMap {
             // Every process of a host with a 64-bit guest memory has the
             // high arena (docs/mem64.md §3).
             high: gex_abi::mem64().then_some((gex_abi::HIGH_ARENA_BASE, gex_abi::LIMIT64)),
-            reserved: Vec::new(),
         }
     }
 
@@ -653,20 +648,29 @@ impl MemoryMap {
         // address in use stays low: a `munmap` hole is reused before the arena
         // -- and the memory behind it -- gets bigger.
         if self.take.is_some() {
-            // A fresh chunk may run into a reserved span; it is arena all the
-            // same, so look again (each pass extends the arena past the last).
-            for _ in 0..8 {
-                if let Some(start) = self.first_fit(len) {
-                    self.insert_from(start, start + len, prot, name, origin);
-                    return Ok(start);
-                }
-                let start = self.extend_arena(len)?;
-                if self.reserved_overlap(start, start + len).is_none() {
-                    self.insert_from(start, start + len, prot, name, origin);
-                    return Ok(start);
+            let chunks = self.arena.clone();
+            for (lo, hi) in chunks {
+                let mut candidate = page_align_up(lo);
+                while candidate + len <= hi {
+                    if self.is_free(candidate, candidate + len) {
+                        self.insert_from(candidate, candidate + len, prot, name, origin);
+                        return Ok(candidate);
+                    }
+                    match self
+                        .vmas
+                        .iter()
+                        .filter(|v| v.start < candidate + len && v.end > candidate)
+                        .map(|v| v.end)
+                        .max()
+                    {
+                        Some(end) => candidate = page_align_up(end),
+                        None => break,
+                    }
                 }
             }
-            return Err(Errno::ENOMEM);
+            let start = self.extend_arena(len)?;
+            self.insert_from(start, start + len, prot, name, origin);
+            return Ok(start);
         }
 
         let mut candidate = self.mmap_next;
@@ -755,38 +759,6 @@ impl MemoryMap {
         Ok((dst, true))
     }
 
-    /// Keep `[lo, hi)` for the object pinned there (`reserved`).
-    pub fn reserve(&mut self, lo: u64, hi: u64) {
-        if lo < hi {
-            self.reserved.push((page_align_down(lo), page_align_up(hi)));
-        }
-    }
-
-    /// The end of the furthest reserved span overlapping `[start, end)`.
-    fn reserved_overlap(&self, start: u64, end: u64) -> Option<u64> {
-        self.reserved.iter().filter(|&&(s, e)| s < end && start < e).map(|&(_, e)| e).max()
-    }
-
-    /// The lowest free, unreserved `len` bytes inside the arena.
-    fn first_fit(&self, len: u64) -> Option<u64> {
-        for &(lo, hi) in &self.arena {
-            let mut candidate = page_align_up(lo);
-            while candidate + len <= hi {
-                let taken = self
-                    .vmas
-                    .iter()
-                    .filter(|v| v.start < candidate + len && v.end > candidate)
-                    .map(|v| v.end)
-                    .max();
-                match taken.max(self.reserved_overlap(candidate, candidate + len)) {
-                    None => return Some(candidate),
-                    Some(end) => candidate = page_align_up(end),
-                }
-            }
-        }
-        None
-    }
-
     fn is_free(&self, start: u64, end: u64) -> bool {
         !self.vmas.iter().any(|v| v.start < end && v.end > start)
     }
@@ -916,37 +888,6 @@ impl MemoryMap {
 mod tests {
     use super::*;
 
-    /// A pinned object's span (`reserve`) is never handed to an unhinted
-    /// mapping, inside the arena already claimed or in a chunk claimed later,
-    /// and the pinned object still gets its base by asking for it.
-    #[test]
-    fn reserved_spans_wait_for_their_object() {
-        static NEXT: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0x2000_0000);
-        fn take(len: u64) -> u64 {
-            NEXT.fetch_add(len, std::sync::atomic::Ordering::SeqCst)
-        }
-        fn back(end: u64) -> Option<u64> {
-            Some(NEXT.fetch_max(end, std::sync::atomic::Ordering::SeqCst))
-        }
-        let mut m = MemoryMap::new(0x40_1000);
-        m.grows_on_demand(take, back);
-        // One span inside the first chunk, one well past it.
-        m.reserve(0x2000_4000, 0x2001_0000);
-        m.reserve(0x2100_0000, 0x2140_0000);
-        let mut got = Vec::new();
-        for _ in 0..64 {
-            let a = m.mmap(0, 0x10_0000, prot::READ, flags::PRIVATE | flags::ANONYMOUS, "x").unwrap();
-            got.push(a);
-        }
-        for &a in &got {
-            let e = a + 0x10_0000;
-            assert!(!(a < 0x2001_0000 && 0x2000_4000 < e), "{a:#x} is inside the first reserved span");
-            assert!(!(a < 0x2140_0000 && 0x2100_0000 < e), "{a:#x} is inside the second reserved span");
-        }
-        assert_eq!(m.mmap(0x2000_4000, 0xc000, prot::READ, flags::PRIVATE | flags::ANONYMOUS, "pinned").unwrap(), 0x2000_4000);
-        assert_eq!(m.mmap(0x2100_0000, 0x40_0000, prot::READ, flags::PRIVATE | flags::ANONYMOUS, "pinned").unwrap(), 0x2100_0000);
-    }
-
     /// glibc's heap pattern must not leak the arena a claim at a time.
     ///
     /// `new_heap` frees the head and tail of each claim; the freed tail of one

crates/gex-kernel/src/syscalls.rs

@@ -612,11 +612,6 @@ pub struct Kernel {
     /// this one has to ask, and can only ask once the reservation says how
     /// long the object is.
     pub armed_place: Option<String>,
-    /// The thread that armed [`Self::armed_pin`] / [`Self::armed_place`]:
-    /// only its own next reservation takes them. Another thread's anonymous
-    /// `PROT_NONE` mapping between a `dlopen`'s open and its mmap (a thread
-    /// stack's guard, a GC reservation) took the pin otherwise.
-    pub armed_tid: i32,
     /// Every thread's own signal state but the running one's, which is in
     /// `signals` (`Signals::save_thread`/`load_thread`): masks, thread-directed
     /// pending signals and alternate stacks are per thread, as on Linux.
@@ -1340,7 +1335,6 @@ impl Kernel {
             pins: std::collections::HashMap::new(),
             place: None,
             armed_place: None,
-            armed_tid: 0,
             syscall_pc: 0,
             syscall_nr: 0,
             thread_signals: std::collections::HashMap::new(),
@@ -1589,7 +1583,6 @@ impl Kernel {
             // same library must be given the same address its parent was.
             place: self.place.clone(),
             armed_place: None,
-            armed_tid: 0,
             syscall_pc: 0,
             syscall_nr: 0,
             thread_signals: std::collections::HashMap::new(),
@@ -3757,7 +3750,6 @@ impl Kernel {
             // the path is what identifies the object.
             if let Some(base) = self.pin_for(&p) {
                 self.armed_pin = Some((p.clone(), base));
-                self.armed_tid = self.tid;
             }
             // O_CREAT of "name/" that is not there: a slash names a
             // directory, which open does not create (EISDIR, as Linux).
@@ -3788,7 +3780,6 @@ impl Kernel {
             // it loads -- leaves nothing armed.
             if self.place.is_some() && is_shared_object(&p) {
                 self.armed_place = Some(p.clone());
-                self.armed_tid = self.tid;
             }
             h
         };
@@ -5397,7 +5388,7 @@ impl Kernel {
             // pin armed when the object was opened, the same way.
             let mut addr = addr;
             let mut pinned = None;
-            if addr == 0 && flags & crate::mem::flags::FIXED == 0 && self.armed_tid == self.tid {
+            if addr == 0 && flags & crate::mem::flags::FIXED == 0 {
                 if let Some((path, base)) = self.armed_pin.take() {
                     if path.rsplit('/').next() == name.rsplit('/').next() {
                         self.armed_place = None;
@@ -5475,7 +5466,7 @@ impl Kernel {
         // a collision is ignored and recorded in `pin_landed`, not forced.
         let mut addr = addr;
         let mut pinned = None;
-        if addr == 0 && prot_bits == 0 && flags & mflags::ANONYMOUS != 0 && self.armed_tid == self.tid {
+        if addr == 0 && prot_bits == 0 && flags & mflags::ANONYMOUS != 0 {
             if let Some((path, base)) = self.armed_pin.take() {
                 self.armed_place = None;
                 addr = base;

shims/browser/src/lib.rs

@@ -3790,13 +3790,7 @@ pub extern "C" fn gex_load(ptr: u32, len: u32, argv: u32, argv_len: u32) -> u64
             kernel.new_code.extend(gex_kernel::CodeMap::of_placed(path, &bytes, program.interp_base()));
         }
     }
-    let pins = std::mem::take(&mut *PINS.0.borrow_mut());
-    // Each pinned object's span is kept from the arena, so whatever loads
-    // before it cannot take its base (`MemoryMap::reserve`).
-    for (_, base, span) in &pins {
-        kernel.mem.reserve(*base, base + span);
-    }
-    kernel.pins = pins.into_iter().map(|(n, b, _)| (n, b)).collect();
+    kernel.pins = std::mem::take(&mut *PINS.0.borrow_mut()).into_iter().collect();
     // This host's memory has to be allocated up to whatever the map hands
     // out, so the map asks before it hands anything out.
     kernel.mem.grows_on_demand(arena_grow, arena_back);
@@ -4057,7 +4051,7 @@ pub extern "C" fn gex_pin_object(path: u32, path_len: u32, base: u64, span: u64)
     let Ok(name) = std::str::from_utf8(shim_slice(path, path_len)) else {
         return;
     };
-    PINS.0.borrow_mut().push((name.to_string(), base, span));
+    PINS.0.borrow_mut().push((name.to_string(), base));
     // SAFETY: `k_present` takes no borrow, and `k_place_set` takes scalars.
     unsafe {
         if k_present() != 0 {
@@ -4402,7 +4396,7 @@ pub extern "C" fn gex_closure_owner_at(page: u64, k: u32) -> i32 {
 /// Where the closure build says each object goes, staged until there is a
 /// `Host`: `gex_pin_object` runs before `gex_load` (the layout must precede the
 /// loader), when no `Host` exists yet. The global table is told straight away.
-static PINS: SingleThreaded<Vec<(String, u64, u64)>> =
+static PINS: SingleThreaded<Vec<(String, u64)>> =
     SingleThreaded(const { RefCell::new(Vec::new()) });
 
 /// Files bundled by the page, in the order they arrived.