From d06f5c39f08435488cc1d39d9b78d8a70d8811c0 Mon Sep 17 00:00:00 2001 From: James Braid Date: Sat, 31 Oct 2026 13:01:16 -0700 Subject: [PATCH 1/2] gfx_display: stop display threads without waiting on the renderer from main Closing the window on macOS hung iris. main stops the machine, which joins each graphics board's display thread, and that thread ends by releasing its renderer's GL context. glutin runs that release on the main thread, which is the thread waiting in the join, so neither can go on. iris never exited and the disk overlay's sector list was never saved. Every board ended its display thread with its own call to the renderer's stop: REX3 (Indy, Indigo2), GR2, the IMPACT board (Indigo2 IP28 and the Octane) and the O2's GBE. DisplayStop is now the one place that decides it. A stop asked for by a thread named "main" means the process is ending and the window is gone, so the display thread leaves the renderer alone; that stop is final. A stop from any other thread (a reset) still releases it on the thread that owns the context, so the next start sets it up afresh. GfxDisplay has an accessor for the helper, so a new board is pointed at it; nothing makes a board call it. The O2 requests it from its drop, since its stop leaves the display thread running. GR2 now records the request before it clears its running flag, so the display thread cannot end between the two. The test builds one machine per display family (Newport on an Indy, GR2 on an Indigo2, IMPACT on an Octane, the O2's GBE) and puts a renderer in its display whose stop waits for the main thread. A machine is stopped from a thread named reset, where the renderer must still be released, started again, and stopped from a thread named main, which never answers: that stop must return. The O2's display thread ends only when the machine is dropped, so it gets a machine for each. Each test in this file works in a directory of its own, and the REX3 shape corpus it saves goes to a temp file rather than the home directory. Newport shows nothing until a video mode is programmed, so its configuration selects one rather than wait for a guest to do it. Without the change all four main-thread stops hang. --- src/dev/gr2/gr2disp.rs | 4 +- src/dev/gr2/mod.rs | 4 + src/dev/mgras/disp.rs | 5 +- src/dev/mgras/mod.rs | 3 + src/dev/ng1/rex3.rs | 8 +- src/display_stop_tests.rs | 174 ++++++++++++++++++++++++++++++++++++++ src/gfx_display.rs | 36 ++++++++ src/ip32_machine.rs | 10 ++- src/lib.rs | 2 + 9 files changed, 234 insertions(+), 12 deletions(-) create mode 100644 src/display_stop_tests.rs diff --git a/src/dev/gr2/gr2disp.rs b/src/dev/gr2/gr2disp.rs index 99dc8ac86..e46e7bb0c 100644 --- a/src/dev/gr2/gr2disp.rs +++ b/src/dev/gr2/gr2disp.rs @@ -116,8 +116,6 @@ impl Gr2 { } } - if let Some(r) = self.renderer.lock().as_mut() { - r.stop(); - } + self.display_stop.finish(&self.renderer); } } diff --git a/src/dev/gr2/mod.rs b/src/dev/gr2/mod.rs index 22f48ae26..bd9f70d01 100644 --- a/src/dev/gr2/mod.rs +++ b/src/dev/gr2/mod.rs @@ -234,6 +234,7 @@ pub struct Gr2 { pub screen: Mutex, pub renderer: Mutex>>, + display_stop: crate::gfx_display::DisplayStop, pub screenshot_pending: AtomicBool, screenshot_counter: AtomicU32, retrace_cb: Mutex>>, @@ -273,6 +274,7 @@ impl Gr2 { addr_of_mut!((*p).re3_thread).write(Mutex::new(None)); addr_of_mut!((*p).screen).write(Mutex::new(Rex3Screen::new())); addr_of_mut!((*p).renderer).write(Mutex::new(None)); + addr_of_mut!((*p).display_stop).write(Default::default()); addr_of_mut!((*p).screenshot_pending).write(AtomicBool::new(false)); addr_of_mut!((*p).screenshot_counter).write(AtomicU32::new(0)); addr_of_mut!((*p).retrace_cb).write(Mutex::new(None)); @@ -989,6 +991,7 @@ impl Gr2 { impl crate::gfx_display::GfxDisplay for Gr2 { fn renderer_slot(&self) -> &Mutex>> { &self.renderer } + fn display_stop(&self) -> &crate::gfx_display::DisplayStop { &self.display_stop } fn screen(&self) -> &Mutex { &self.screen } fn request_screenshot(&self) { self.screenshot_pending.store(true, Ordering::Relaxed); } fn cycles(&self) -> CyclesPtr { self.cycles.get() } @@ -1122,6 +1125,7 @@ impl Device for Gr2 { } fn stop(&self) { + self.display_stop.request(); if !self.running.swap(false, Ordering::SeqCst) { return; } diff --git a/src/dev/mgras/disp.rs b/src/dev/mgras/disp.rs index 2b73d5194..6457f3cd4 100644 --- a/src/dev/mgras/disp.rs +++ b/src/dev/mgras/disp.rs @@ -342,9 +342,7 @@ impl Mgras { } // The renderer's GL state belongs to this thread (its context is // current here), so it is torn down here and nowhere else. - if let Some(r) = self.renderer.lock().as_mut() { - r.stop(); - } + self.display_stop.finish(&self.renderer); } /// Start the engines and the display refresh thread. @@ -363,6 +361,7 @@ impl Mgras { } pub fn stop_display(&self) { + self.display_stop.request(); self.running.store(false, Ordering::Release); if let Some(h) = self.refresh.lock().take() { let _ = h.join(); diff --git a/src/dev/mgras/mod.rs b/src/dev/mgras/mod.rs index afbbbf248..b17b8cf45 100644 --- a/src/dev/mgras/mod.rs +++ b/src/dev/mgras/mod.rs @@ -367,6 +367,7 @@ pub struct Mgras { running: AtomicBool, refresh: SpinLock>>, renderer: Mutex>>, + display_stop: crate::gfx_display::DisplayStop, /// The finished frame, handed to the renderer as a prebuilt picture /// (`Rex3Screen::prebuilt`), as GR2 does. screen: Mutex, @@ -537,6 +538,7 @@ impl Mgras { addr_of_mut!((*p).dirty).write(AtomicBool::new(true)); addr_of_mut!((*p).refresh).write(SpinLock::new(None)); addr_of_mut!((*p).renderer).write(Mutex::new(None)); + addr_of_mut!((*p).display_stop).write(Default::default()); addr_of_mut!((*p).screen).write(Mutex::new(crate::disp::Rex3Screen::new())); addr_of_mut!((*p).heartbeat).write(heartbeat); addr_of_mut!((*p).fasttick).write(fasttick); @@ -1598,6 +1600,7 @@ impl Mgras { impl crate::gfx_display::GfxDisplay for Mgras { fn renderer_slot(&self) -> &Mutex>> { &self.renderer } + fn display_stop(&self) -> &crate::gfx_display::DisplayStop { &self.display_stop } fn screen(&self) -> &Mutex { &self.screen } fn request_screenshot(&self) { self.screenshot_pending.store(true, Ordering::Relaxed); } fn cycles(&self) -> crate::cpu::mips_core::CyclesPtr { *self.cycles.lock() } diff --git a/src/dev/ng1/rex3.rs b/src/dev/ng1/rex3.rs index 047ab0dc4..d7380c271 100644 --- a/src/dev/ng1/rex3.rs +++ b/src/dev/ng1/rex3.rs @@ -1388,6 +1388,7 @@ pub struct Rex3 { pub diag: AtomicU64, debug_state: Mutex, pub renderer: Mutex>>, + display_stop: crate::gfx_display::DisplayStop, /// Compiled shaders, keyed by `(dm0, dm1_normalized, clipmode_key)`. /// /// Always present, not gated on `rex-jit`: the LLVM-compiled shaders in @@ -1582,6 +1583,7 @@ impl Rex3 { debug_state: Mutex::new(DebugState::default()), diag: AtomicU64::new(0), renderer: Mutex::new(None), + display_stop: Default::default(), // IRIS_NO_JIT (set by the sandboxed Mac App Store GUI build) forces // the interpreter: Cranelift's mmap+mprotect executable pages aren't // MAP_JIT, so the App Sandbox kills the process with SIGKILL/ @@ -3241,9 +3243,7 @@ impl Rex3 { } } - if let Some(renderer) = self.renderer.lock().as_mut() { - renderer.stop(); - } + self.display_stop.finish(&self.renderer); } /// Returns `true` if the register offset was recognized and updated state. @@ -3623,6 +3623,7 @@ impl Device for Rex3 { if let Some(s) = self.stop_trace() { eprintln!("REX3: trace closed: {s}"); } + self.display_stop.request(); self.running.store(false, Ordering::SeqCst); // Send exit command to ensure processor thread wakes up and terminates. @@ -4920,6 +4921,7 @@ impl BusDevice for Rex3 { impl crate::gfx_display::GfxDisplay for Rex3 { fn renderer_slot(&self) -> &Mutex>> { &self.renderer } + fn display_stop(&self) -> &crate::gfx_display::DisplayStop { &self.display_stop } fn screen(&self) -> &Mutex { &self.screen } fn request_screenshot(&self) { self.screenshot_pending.store(true, Ordering::Relaxed); } fn screenshot_pending(&self) -> bool { self.screenshot_pending.load(Ordering::Relaxed) } diff --git a/src/display_stop_tests.rs b/src/display_stop_tests.rs new file mode 100644 index 000000000..4e3942c75 --- /dev/null +++ b/src/display_stop_tests.rs @@ -0,0 +1,174 @@ +//! Stopping a machine from the main thread must not wait on its renderer. +//! +//! On macOS a renderer's `stop` needs the main thread, which is the thread +//! stopping the machine (see `gfx_display::DisplayStop`). The renderer here +//! does the same: its `stop` asks for an answer from the test thread. One +//! machine per display family, stopped from a reset and then from main: +//! Newport (REX3), GR2, IMPACT (MGRAS) and the O2. + +use std::path::{Path, PathBuf}; +use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering::SeqCst}; +use std::sync::{mpsc, Arc}; +use std::time::{Duration, Instant}; + +use crate::config::{CpuModel, GraphicsBoard, MachineConfig, MachineProfile}; +use crate::dev::ng1::rex3::Renderer; +use crate::workstation::Workstation; + +/// A stop that waits on the main thread never returns. +const BOUND: Duration = Duration::from_secs(8); + +/// A renderer that counts presents and runs `on_stop` when torn down. +#[derive(Clone)] +struct Probe { + presents: Arc, + stopped: Arc, + on_stop: Arc, +} + +impl Probe { + fn new(on_stop: impl Fn() + Send + Sync + 'static) -> Self { + Self { presents: Arc::default(), stopped: Arc::default(), on_stop: Arc::new(on_stop) } + } +} + +impl Renderer for Probe { + fn present( + &mut self, _: &mut crate::disp::Rex3Screen, _: &mut crate::debug_overlay::DebugOverlay, + _: &mut crate::disp::StatusBar, _: &mut crate::disp::StatusBarTexture, _: &crate::disp::BarStats, + _: bool, _: Option<&[u32]>, _: Option<&[u32]>, + ) { + self.presents.fetch_add(1, SeqCst); + } + + fn stop(&mut self) { + (self.on_stop)(); + self.stopped.store(true, SeqCst); + } +} + +/// A scratch directory for one test's files (the Octane and O2 read a PROM +/// from a file; these tests run no guest code, so zeros will do). Tests run +/// side by side in one process and each removes its own directory, so each +/// takes a name of its own. +fn scratch(test: &str) -> PathBuf { + let dir = std::env::temp_dir().join(format!("iris-display-stop-{test}-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + // A REX3 saves the shapes it drew when it stops, to $HOME/.iris unless + // told otherwise. The path is the same for every test, as in + // `exit_persistence_tests::serial`. + std::env::set_var("IRIS_REX_JIT_PROFILE", std::env::temp_dir().join("iris-display-stop-rex-jit-profile.bin")); + dir +} + +fn config(dir: &Path, profile: MachineProfile, board: GraphicsBoard) -> MachineConfig { + let (mb, cpu) = match profile { + MachineProfile::OctaneIp30 => (64, CpuModel::R10000), + MachineProfile::O2Ip32 => (32, CpuModel::R5000), + _ => (16, CpuModel::default()), + }; + let mut cfg = crate::bench_runner::bench_config([mb, 0, 0, 0], cpu); + cfg.headless = false; + cfg.machine.profile = profile; + cfg.monitor_port = Some(0); + cfg.nvram = dir.join("nvram").to_string_lossy().into_owned(); + cfg.nveeprom = dir.join("nveeprom").to_string_lossy().into_owned(); + if matches!(profile, MachineProfile::OctaneIp30 | MachineProfile::O2Ip32) { + let prom = dir.join("prom.bin"); + std::fs::write(&prom, vec![0u8; 1 << 20]).unwrap(); + cfg.prom = prom.to_string_lossy().into_owned(); + } + // The O2's GBE is its own display; it has no `[graphics] board`. + cfg.graphics.board = (profile != MachineProfile::O2Ip32).then_some(board); + if board == GraphicsBoard::Newport { + // A Newport shows nothing until a video mode is programmed. Set it + // here rather than wait for a guest to get that far. + cfg.graphics.resolution = crate::dev::ng1::vc2_timings::NewportResolution::Res1280x1024; + } + cfg +} + +fn big_stack(f: impl FnOnce() -> T + Send + 'static) -> T { + std::thread::Builder::new().stack_size(64 * 1024 * 1024).spawn(f).unwrap().join().unwrap() +} + +/// Stop `machine` on a thread called `name`, and drop it there too if `last` +/// (the O2 stops its display in its drop). The machine comes back unless it +/// was dropped, or `None` if that did not finish within [`BOUND`]. +fn stop_as(name: &str, mut machine: Box, last: bool) -> Option>> { + let (done, finished) = mpsc::channel(); + std::thread::Builder::new().name(name.into()).spawn(move || { + machine.stop(); + if last { + drop(machine); + let _ = done.send(None); + } else { + let _ = done.send(Some(machine)); + } + }).unwrap(); + finished.recv_timeout(BOUND).ok() +} + +#[test] +fn stopping_from_the_main_thread_never_waits_on_the_renderer() { + let dir = scratch("main-thread"); + let families = [ + (MachineProfile::IndyIp24, GraphicsBoard::Newport), + (MachineProfile::Indigo2Ip22, GraphicsBoard::Xz), + (MachineProfile::OctaneIp30, GraphicsBoard::SolidImpact), + (MachineProfile::O2Ip32, GraphicsBoard::Newport), + ]; + let mut failures = Vec::new(); + for (profile, board) in families { + // Stopped from the main thread, the renderer's stop waits for an + // answer that never comes. Stopped from another thread (a reset) the + // main thread answers, and the renderer must still be released. + let answered = Arc::new(AtomicBool::new(true)); + let gate = answered.clone(); + let probe = Probe::new(move || while !gate.load(SeqCst) { std::thread::sleep(Duration::from_millis(5)) }); + // The reset comes first and the machine starts again for the stop + // from main. The O2's display thread ends only in its drop, so it + // needs a machine for each. + let reuse = profile != MachineProfile::O2Ip32; + let mut machine: Option> = None; + for (name, from_main) in [("reset", false), ("main", true)] { + let label = format!("{profile:?} {board:?} stopped from {name}"); + let before = probe.presents.load(SeqCst); + let m = match machine.take() { + Some(mut m) => { + m.restart_peripherals(); + m + } + None => { + let cfg = config(&dir, profile, board); + let mut m = big_stack(move || crate::workstation::build(cfg).expect("machine")); + *m.get_display().expect("a display").renderer_slot().lock() = Some(Box::new(probe.clone())); + m.start(); + m + } + }; + let up = Instant::now() + Duration::from_secs(10); + while probe.presents.load(SeqCst) == before && Instant::now() < up { + std::thread::sleep(Duration::from_millis(10)); + } + if probe.presents.load(SeqCst) == before { + failures.push(format!("{label}: the display never presented")); + break; + } + probe.stopped.store(false, SeqCst); + answered.store(!from_main, SeqCst); + match stop_as(name, m, from_main || !reuse) { + None => failures.push(format!("{label}: stop did not return within {BOUND:?}")), + Some(back) => { + machine = back; + if !from_main && !probe.stopped.load(SeqCst) { + failures.push(format!("{label}: the renderer was not released")); + } + } + } + answered.store(true, SeqCst); // let a hung stop finish + } + } + let _ = std::fs::remove_dir_all(&dir); + assert!(failures.is_empty(), "\n{}", failures.join("\n")); +} diff --git a/src/gfx_display.rs b/src/gfx_display.rs index 3163f9dce..105e543af 100644 --- a/src/gfx_display.rs +++ b/src/gfx_display.rs @@ -6,6 +6,8 @@ //! last-presented screen (screenshots, VNC), the hardware cursor, and the CPU //! cycle counter. +use std::sync::atomic::{AtomicBool, Ordering}; + use parking_lot::Mutex; use crate::disp::Rex3Screen; @@ -13,6 +15,9 @@ use crate::cpu::mips_core::CyclesPtr; use crate::dev::ng1::rex3::Renderer; pub trait GfxDisplay: Send + Sync { + /// The board's [`DisplayStop`], which its stop and its display thread use. + fn display_stop(&self) -> &DisplayStop; + /// Slot the front-end installs its renderer into. The board's display /// thread presents every frame through whatever is installed here. fn renderer_slot(&self) -> &Mutex>>; @@ -53,6 +58,37 @@ pub trait GfxDisplay: Send + Sync { fn hw_cursor(&self) -> Option { None } } +/// Whether the display thread tears its renderer down when it ends. +/// +/// Releasing a renderer's GL context has to happen on the thread that made +/// it current, so the display thread does it as its last act. On macOS +/// glutin runs that release on the main thread, and the main thread is the +/// one stopping the machine when the window closes, waiting in the join. +/// Neither goes on, and the disks are never flushed. So a board calls +/// [`request`](Self::request) first thing in its stop, and its display +/// thread ends with [`finish`](Self::finish), which leaves the renderer +/// alone when the stop came from the main thread. That stop is final: the +/// process is ending, and the renderer cannot be set up again. +#[derive(Default)] +pub struct DisplayStop(AtomicBool); + +impl DisplayStop { + pub fn request(&self) { + // std has no is-main-thread test. The platform ones exist where the + // hang does (pthread_main_np on macOS), but Rust names the main + // thread, and the name lets a test on Linux stand in for it. + self.0.store(std::thread::current().name() == Some("main"), Ordering::Release); + } + + pub fn finish(&self, renderer: &Mutex>>) { + if !self.0.load(Ordering::Acquire) { + if let Some(r) = renderer.lock().as_mut() { + r.stop(); + } + } + } +} + /// The hardware cursor: where the board draws its glyph and which of the /// glyph's pixels are opaque. The glyph's top-left is the pointer position /// minus the hotspot, which the hardware does not know. diff --git a/src/ip32_machine.rs b/src/ip32_machine.rs index a47378ea2..9f4bc6d2d 100644 --- a/src/ip32_machine.rs +++ b/src/ip32_machine.rs @@ -782,6 +782,7 @@ impl Drop for O2Machine { if let Some(slot) = self.controller_slot.take() { slot.lock().0 = MachineRef::null(); } + self.display.display_stop.request(); self.running.store(false, Ordering::SeqCst); for t in self.threads.drain(..) { let _ = t.join(); @@ -871,6 +872,7 @@ impl Workstation for O2Machine { pub struct O2Display { bus: Arc, renderer: Mutex>>, + display_stop: crate::gfx_display::DisplayStop, screen: Mutex, screenshot_pending: AtomicBool, cycles: CyclesPtr, @@ -885,6 +887,7 @@ impl O2Display { Self { bus, renderer: Mutex::new(None), + display_stop: Default::default(), screen: Mutex::new(Rex3Screen::new()), screenshot_pending: AtomicBool::new(false), cycles, @@ -962,9 +965,7 @@ impl O2Display { std::thread::sleep(frame_time - spent); } } - if let Some(r) = self.renderer.lock().as_mut() { - r.stop(); - } + self.display_stop.finish(&self.renderer); } } @@ -972,6 +973,9 @@ impl crate::gfx_display::GfxDisplay for O2Display { fn renderer_slot(&self) -> &Mutex>> { &self.renderer } + fn display_stop(&self) -> &crate::gfx_display::DisplayStop { + &self.display_stop + } fn screen(&self) -> &Mutex { &self.screen } diff --git a/src/lib.rs b/src/lib.rs index 7ca3576a3..2410aa7b9 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -237,6 +237,8 @@ pub use perfetto_macros::{PfBits, PfLevel, PfValue}; #[cfg(feature = "perfetto")] pub mod perfetto; +#[cfg(test)] +mod display_stop_tests; #[cfg(test)] mod platform_profile_tests; #[cfg(test)] From c528316e934cf6c3a99429b3f0b54cc67d6ed8c7 Mon Sep 17 00:00:00 2001 From: James Braid Date: Sat, 31 Oct 2026 13:01:16 -0700 Subject: [PATCH 2/2] machine: stop the disks before the graphics board Stopping a machine joined the graphics board first (the Octane stopped its devices in reverse start order, and the board starts last; the Indy and Indigo2 stopped REX3, the IMPACT board and GR2 before HPC3). A graphics stop waits on a display thread, and that wait is the one that can fail to end. When it did, everything stopped after it never ran. That is not the disk flush. The disk controllers' stops only join their workers, and the flush that follows the stop in the main thread's stop-and-flush is skipped whenever the stop does not return, whatever the order. What the reorder keeps is the work the stops themselves do: HPC3's stop saves the RTC NVRAM and the EEPROM, IOC3 writes its NVRAM out, and disk commands still in flight complete. The board now stops last, on the Octane (stop and drop) and on the Indy and Indigo2. The tests tear down a started machine's display with a renderer that notes whether a disk controller is still running at that moment: HPC3 on the Indy, the ISP1040s and IOC3 on the Octane. Each first checks that those controllers are running before the stop, so a machine that stopped starting them would fail rather than pass without testing anything. Both fail with the old order. The Octane's Ethernet controller is not checked; it has no public running state. The Indy test keeps its REX3 shape corpus out of the home directory. --- src/display_stop_tests.rs | 39 +++++++++++++++++++++++++++++++++------ src/ip30_machine.rs | 20 ++++++++++++++------ src/machine.rs | 37 ++++++++++++++++++++++++++++++++++--- 3 files changed, 81 insertions(+), 15 deletions(-) diff --git a/src/display_stop_tests.rs b/src/display_stop_tests.rs index 4e3942c75..fa1b319d5 100644 --- a/src/display_stop_tests.rs +++ b/src/display_stop_tests.rs @@ -20,16 +20,23 @@ const BOUND: Duration = Duration::from_secs(8); /// A renderer that counts presents and runs `on_stop` when torn down. #[derive(Clone)] -struct Probe { +pub(crate) struct Probe { presents: Arc, - stopped: Arc, + pub(crate) stopped: Arc, on_stop: Arc, } impl Probe { - fn new(on_stop: impl Fn() + Send + Sync + 'static) -> Self { + pub(crate) fn new(on_stop: impl Fn() + Send + Sync + 'static) -> Self { Self { presents: Arc::default(), stopped: Arc::default(), on_stop: Arc::new(on_stop) } } + + /// A probe that notes whether `disks_running` was true when torn down. + pub(crate) fn watching(disks_running: impl Fn() -> bool + Send + Sync + 'static) -> (Self, Arc) { + let seen = Arc::new(AtomicBool::default()); + let flag = seen.clone(); + (Self::new(move || if disks_running() { flag.store(true, SeqCst) }), seen) + } } impl Renderer for Probe { @@ -51,7 +58,7 @@ impl Renderer for Probe { /// from a file; these tests run no guest code, so zeros will do). Tests run /// side by side in one process and each removes its own directory, so each /// takes a name of its own. -fn scratch(test: &str) -> PathBuf { +pub(crate) fn scratch(test: &str) -> PathBuf { let dir = std::env::temp_dir().join(format!("iris-display-stop-{test}-{}", std::process::id())); std::fs::create_dir_all(&dir).unwrap(); // A REX3 saves the shapes it drew when it stops, to $HOME/.iris unless @@ -61,7 +68,7 @@ fn scratch(test: &str) -> PathBuf { dir } -fn config(dir: &Path, profile: MachineProfile, board: GraphicsBoard) -> MachineConfig { +pub(crate) fn config(dir: &Path, profile: MachineProfile, board: GraphicsBoard) -> MachineConfig { let (mb, cpu) = match profile { MachineProfile::OctaneIp30 => (64, CpuModel::R10000), MachineProfile::O2Ip32 => (32, CpuModel::R5000), @@ -88,7 +95,7 @@ fn config(dir: &Path, profile: MachineProfile, board: GraphicsBoard) -> MachineC cfg } -fn big_stack(f: impl FnOnce() -> T + Send + 'static) -> T { +pub(crate) fn big_stack(f: impl FnOnce() -> T + Send + 'static) -> T { std::thread::Builder::new().stack_size(64 * 1024 * 1024).spawn(f).unwrap().join().unwrap() } @@ -172,3 +179,23 @@ fn stopping_from_the_main_thread_never_waits_on_the_renderer() { let _ = std::fs::remove_dir_all(&dir); assert!(failures.is_empty(), "\n{}", failures.join("\n")); } + +/// The graphics board can hang in its stop, so the SCSI controllers and +/// IOC3 must already be stopped when its renderer is torn down. +#[test] +fn the_octane_stops_its_disks_before_its_graphics() { + use crate::traits::Device; + let dir = scratch("octane-order"); + let cfg = config(&dir, MachineProfile::OctaneIp30, GraphicsBoard::SolidImpact); + let mut machine = big_stack(move || crate::ip30_machine::OctaneMachine::new(&cfg).expect("Octane")); + let (scsi, ioc3) = (machine.scsi.clone(), machine.ioc3.clone()); + let (probe, overlap) = Probe::watching(move || ioc3.is_running() || scsi.iter().any(|s| s.is_running())); + *machine.gfx.as_ref().expect("graphics").display().renderer_slot().lock() = Some(Box::new(probe.clone())); + machine.restart_peripherals(); + assert!(!machine.scsi.is_empty() && machine.scsi.iter().all(|s| s.is_running()), "the SCSI controllers are running"); + assert!(machine.ioc3.is_running(), "IOC3 is running"); + machine.stop(); + assert!(probe.stopped.load(SeqCst), "the display thread released its renderer"); + assert!(!overlap.load(SeqCst), "a disk controller was still running when the graphics stopped"); + let _ = std::fs::remove_dir_all(&dir); +} diff --git a/src/ip30_machine.rs b/src/ip30_machine.rs index ac8e86024..2ab9722c9 100644 --- a/src/ip30_machine.rs +++ b/src/ip30_machine.rs @@ -528,6 +528,18 @@ impl OctaneMachine { d } + /// Reverse of start order, but graphics last: its stop joins a display + /// thread, which can fail to end, and the disks must be stopped by then. + fn stop_devices(&self) { + // `devices` ends with the graphics board, if there is one. + for d in self.devices().iter().rev().skip(self.gfx.iter().count()) { + d.stop(); + } + if let Some(g) = &self.gfx { + g.stop(); + } + } + pub fn start(&mut self) { if let Ok(starting) = self.lifecycle.transition(crate::machine_state::GuestState::Starting) { self.lifecycle = starting; @@ -546,9 +558,7 @@ impl OctaneMachine { /// and IOC3 writes its NVRAM out. pub fn stop(&mut self) { self.stop_cpus(); - for d in self.devices().iter().rev() { - d.stop(); - } + self.stop_devices(); // The guest clock points into this machine's CPU core. #[cfg(feature = "perfetto")] crate::perfetto::set_guest_cycles(None); @@ -622,9 +632,7 @@ impl Drop for OctaneMachine { slot.lock().0 = MachineRef::null(); } self.stop_cpus(); - for d in self.devices().iter().rev() { - d.stop(); - } + self.stop_devices(); crate::workstation::flush_on_drop(self); self.monitor.shutdown(); } diff --git a/src/machine.rs b/src/machine.rs index d47926dd4..3b4132605 100644 --- a/src/machine.rs +++ b/src/machine.rs @@ -1022,13 +1022,15 @@ impl Machine { pub fn stop(&mut self) { self.cpu.stop(); + // Graphics last: a board's stop joins its display thread, which can + // fail to end, and the disks must be stopped by then. + self.hpc3.stop(); + self.mc.stop(); + if let Some(u64) = &self.board.ultra64 { u64.stop(); } if let Some(rex3) = &self.board.rex3 { rex3.stop(); } if let Some(mgras) = &self.board.mgras { mgras.stop_display(); } if let Some(rex3) = &self.board.rex3_head1 { rex3.stop(); } if let Some(gr2) = &self.board.gr2 { gr2.stop(); } - self.hpc3.stop(); - self.mc.stop(); - if let Some(u64) = &self.board.ultra64 { u64.stop(); } // Idle -> Halted is not a lifecycle transition: a machine that was // never started has nothing to halt, and the monitor is not up to ask. if self.lifecycle != GuestState::Idle { @@ -2246,3 +2248,32 @@ mod controller_lifetime_tests { } } } + +#[cfg(test)] +mod stop_order_tests { + use super::*; + use crate::display_stop_tests::{big_stack, Probe}; + use std::sync::atomic::Ordering::SeqCst; + + /// The graphics board can hang in its stop, so HPC3 must already be + /// stopped when its renderer is torn down. + #[test] + fn stopping_the_disks_comes_before_stopping_the_graphics() { + // A REX3 saves its shape corpus when it stops; keep it out of $HOME. + let dir = crate::display_stop_tests::scratch("indy-order"); + let mut machine = big_stack(|| { + let mut cfg = crate::bench_runner::bench_config([16, 0, 0, 0], crate::config::CpuModel::default()); + cfg.headless = false; + Box::new(Machine::new(cfg)) + }); + let hpc3 = machine.hpc3.clone(); + let (probe, overlap) = Probe::watching(move || hpc3.is_running()); + *machine.board.rex3.clone().expect("a Newport").renderer.lock() = Some(Box::new(probe.clone())); + machine.restart_peripherals(); + assert!(machine.hpc3.is_running(), "HPC3 is running"); + machine.stop(); + assert!(probe.stopped.load(SeqCst), "the display thread released its renderer"); + assert!(!overlap.load(SeqCst), "HPC3 was still running when the graphics stopped"); + let _ = std::fs::remove_dir_all(&dir); + } +}