timeline: MachineCore, galaxian: save the state at a mid-frame stop, not after the frame tail after a trap

extension/notes/PLAN.md

@@ -1701,7 +1701,9 @@ The shared core adopts the IDE's model:
   Registers, memory, the disassembly, and the debug views all read the
   snapshot, not the live machine. After `advanceFrame` returns, the core
   loads the snapshot back (as `postFrame` does) so memory reads and writes
-  go to the right state.
+  go to the right state. Done for `MachineCore` (`common/timeline.ts`):
+  it saves and reloads `saveState()`. An error thrown by the frame's tail
+  after a stop is dropped; resuming replays the frame and raises it then.
 - **Resuming or stepping always replays**: load the frame-start state and
   run to `step + n` with breakpoints suppressed until `step`. Step back
   replays to `step - 1` (a step count, not the IDE's `clock - 25`

src/common/timeline.ts

@@ -205,22 +205,35 @@ export class MachineCore<T extends CoreMachine> implements DeterministicCore {
     }
     let n = 0;
     let trapped = false;
+    // A trap stops the CPU loop, not the frame: advanceFrame() still runs its
+    // tail (drawing the line, the vblank interrupt, a watchdog) after the loop
+    // exits. So the state is saved inside the trap, at the exact stop, and
+    // loaded back once advanceFrame() returns.
+    let stopState: any = null;
     // The trap runs before each step, so when it returns true exactly n steps
     // have executed -- n is the position, not the step about to run. step is
     // published before calling the caller's trap so that now() is the current
     // position while the condition is being evaluated; a search relies on that
     // to record where a hit happened.
-    this.machine.advanceFrame(() => {
-      this.step = n;
-      if (n >= untilStep) return true;
-      if (n >= from && trap && trap()) {
-        trapped = true;
-        return true;
-      }
-      n++;
-      return false;
-    });
-    if (trapped || n >= untilStep) {
+    try {
+      this.machine.advanceFrame(() => {
+        this.step = n;
+        if (n >= untilStep || (n >= from && trap && (trapped = !!trap()))) {
+          stopState = this.machine.saveState();
+          return true;
+        }
+        n++;
+        return false;
+      });
+    } catch (e) {
+      // Once stopped, only the frame's tail runs, and it runs early: the frame
+      // hasn't really ended. An error from it (galaxian's watchdog) is not an
+      // error at the stop. Resuming replays the whole frame, so a real one is
+      // raised again then.
+      if (!stopState) throw e;
+    }
+    if (stopState) this.machine.loadState(stopState);
+    if (stopState) {
       this.step = n;
       return trapped;
     }

test/unit/testtimeline.ts

@@ -55,6 +55,34 @@ describe('Timestamp', function () {
 
 describe('MachineCore', function () {
 
+  it('ignores an error from the frame tail after a stop, but not otherwise', function () {
+    // like galaxian: code after the CPU loop mutates state and may throw
+    class TailMachine extends FakeMachine {
+      watchdog = 0;
+      advanceFrame(trap: TrapCondition): number {
+        const n = super.advanceFrame(trap);
+        this.acc += 1000;
+        if (++this.watchdog >= 3) throw new Error('watchdog');
+        return n;
+      }
+      saveState() { return { ...super.saveState(), watchdog: this.watchdog }; }
+      loadState(s) { super.loadState(s); this.watchdog = s.watchdog; }
+    }
+    const m = new TailMachine();
+    const c = new MachineCore(m);
+    c.runUntil(timestamp(2, 0));
+    // the tail throws on this frame, but the stop comes first
+    var steps = 0;
+    const r = c.runUntil(timestamp(3, 0), () => steps++ === 5);
+    assert.ok(r.trapped);
+    assert.ok(timestampsEqual(r.at, timestamp(2, 5)), formatTimestamp(r.at));
+    assert.strictEqual(m.acc, 2025);
+    assert.strictEqual(m.watchdog, 2);
+    // finishing the frame runs the tail for real
+    assert.throws(() => c.runUntil(timestamp(3, 0)), /watchdog/);
+  });
+
+
   it('counts frames monotonically and never rewinds the clock', function () {
     const m = new FakeMachine();
     const c = new MachineCore(m);

test/unit/testtimelinemachine.ts

@@ -9,6 +9,8 @@ mockAudio();
 // imported after the globals are mocked -- the machine pulls in the audio
 // classes that live on globalThis
 import { Midway8080 } from "../../src/machine/mw8080bw";
+import { GalaxianScrambleMachine } from "../../src/machine/galaxian";
+import { hashState as hashAnyState } from "../../src/common/statehash";
 import { NullProbe } from "../../src/common/devices";
 import { History } from "../../src/common/history";
 import {
@@ -191,3 +193,52 @@ describe('History on a real machine', function () {
     assert.strictEqual(m.cpu.getPC(), pcAt39);
   });
 });
+
+// A trap stops the CPU loop, not the frame: galaxian's advanceFrame() still
+// advances its graphics, decrements the watchdog and raises an NMI after the
+// loop exits. A core stopped mid-frame must show the state at the stop, not
+// the state after that tail ran.
+describe('MachineCore break state', function () {
+
+  function newGalaxian() {
+    const m = new GalaxianScrambleMachine();
+    m.connectVideo(new Uint32Array(m.canvasWidth * m.numVisibleScanlines));
+    m.loadROM(new Uint8Array(fs.readFileSync('./test/roms/galaxian-scramble/shoot2.c.rom')));
+    m.reset();
+    return { m, core: new MachineCore(m) };
+  }
+
+  it('shows the state at the trap, not after the frame tail', function () {
+    const { m, core } = newGalaxian();
+    core.runUntil(timestamp(5, 0));
+    var n = 0;
+    var atTrap: number = null;
+    const r = core.runUntil(timestamp(6, 0), () => {
+      if (n++ < 500) return false;
+      atTrap = hashAnyState(m.saveState());
+      return true;
+    });
+    assert.ok(r.trapped);
+    assert.strictEqual(hashAnyState(m.saveState()), atTrap);
+  });
+
+  it('shows the state at the target step when stopping at one', function () {
+    // the trap is not consulted at the target step itself, so capture the
+    // expected state from a second run that goes one step further
+    const ref = newGalaxian();
+    ref.core.runUntil(timestamp(5, 0));
+    var atStep: number = null;
+    var n = 0;
+    ref.core.runUntil(timestamp(5, 501), () => {
+      if (n++ === 500) atStep = hashAnyState(ref.m.saveState());
+      return false;
+    });
+    assert.notStrictEqual(atStep, null);
+
+    const { m, core } = newGalaxian();
+    core.runUntil(timestamp(5, 0));
+    core.runUntil(timestamp(5, 500));
+    assert.ok(timestampsEqual(core.now(), timestamp(5, 500)), formatTimestamp(core.now()));
+    assert.strictEqual(hashAnyState(m.saveState()), atStep);
+  });
+});