diff --git a/crates/volamos-core/src/backend.rs b/crates/volamos-core/src/backend.rs index 990adac..98ae4e4 100644 --- a/crates/volamos-core/src/backend.rs +++ b/crates/volamos-core/src/backend.rs @@ -170,6 +170,21 @@ pub struct M68kCpu { /// (see the CLI's `--jit`/`--no-jit` flags); set with /// [`Self::set_jit`]. jit: bool, + /// The clock rate (in Hz) [`Self::run_via_cycles`] is deriving + /// emulated time from, if `--clock-mhz` was given -- `Some` makes + /// [`Cpu::run`] switch from `run_batch` to + /// [`m68k::CpuCore::run_for_cycles`] entirely, ignoring [`Self::jit`] + /// (see [`Self::set_clock_mhz`]'s doc for why the two are mutually + /// exclusive at the CLI layer). `None` (the default) is this + /// backend's behavior before issue #102: no cycle counting at all, + /// `run_batch` as always. + clock_hz: Option, + /// Real m68k cycles consumed so far by [`Self::run_via_cycles`] -- + /// see [`Cpu::emulated_cycles`]. Only advanced while + /// [`Self::clock_hz`] is `Some`; stays `0` otherwise, matching + /// [`Cpu::emulated_cycles`]'s documented "0 means never asked" + /// default. + cycles: u64, } impl M68kCpu { @@ -187,6 +202,64 @@ impl M68kCpu { self.jit = jit; } + /// Installs (`Some(mhz)`) or clears (`None`) an emulated clock rate + /// for `timer.device`'s `ReadEClock` -- the CLI's `--clock-mhz` + /// flag (issue #102). `mhz` is a clock rate in megahertz (fractional + /// values allowed, e.g. Copperline's own A600/Gayle configuration + /// models a 25 MHz 68000 as `clock_mhz = 25.0`); stored internally + /// as a plain Hz rate so [`Self::run_via_cycles`] and + /// [`crate::exectask::read_eclock_handler`] don't need to repeat the + /// `* 1_000_000.0` conversion. + /// + /// The point of this mode is reproducible, host-load-independent + /// compiler A/B benchmarks: [`Cpu::run`]'s normal + /// [`m68k::CpuCore::run_batch`] path (`--jit` or not) never surfaces + /// a cycle count at all (see this module's docs on why + /// `m68k::BatchResult` has no `cycles` field), so with `Some` here + /// [`Cpu::run`] switches to [`Self::run_via_cycles`] instead, which + /// calls [`m68k::CpuCore::run_for_cycles`] -- the crate's + /// transaction-exact, `precise_bus`-stepping path -- and accumulates + /// the real cycle counts it returns into [`Self::cycles`] + /// ([`Cpu::emulated_cycles`]). `read_eclock_handler` then reports + /// `cycles / hz` seconds of *emulated* time instead of host + /// wall-clock time. + /// + /// This is unconditional and mutually exclusive with the JIT at the + /// CLI layer (`main.rs` refuses `--clock-mhz` together with an + /// explicit `--jit`), not something this method itself arbitrates: + /// `run_batch`'s trace JIT has nothing a cycle count could be + /// derived from, so there is no sensible way to honor both at once. + /// Note this is *unlike* `--sanitize` forcing the JIT off on its own + /// (see [`AddressBus::fast_mem`]'s doc on `FlatMemory`) -- there, + /// forcing is safe because `--no-jit` is an exact, not merely + /// approximate, substitute for what `--sanitize` needs. Here there + /// is no such fallback (`run_batch` has literally no cycle count to + /// hand back under any configuration), so the CLI refuses the + /// combination outright rather than silently picking a winner. + /// + /// `--clock-mhz` is *also* refused together with `--sanitize`, for + /// a related but distinct reason: [`Self::run_via_cycles`] (the path + /// this switches [`Cpu::run`] to) never calls the sanitizer's + /// per-instruction hooks at all, so combining the two wouldn't just + /// be imprecise, it would leave the shadow call stack and below-`A7` + /// tracking silently stale while `--sanitize` still reports as if + /// everything had been checked -- see `main.rs`'s + /// `check_clock_mhz_sanitize` for the CLI-layer refusal and + /// [`Self::run_via_cycles`]'s own doc for the mechanical reason. + /// + /// Roughly 2.2x slower than `--no-jit` and 7x slower than `--jit` in + /// measurement (CoreMark 1.0, `~/src/external/coremark/ + /// coremark.amiga`, `--cpu 68020`, host wall-clock throughput: + /// ~217 iterations/sec for `--no-jit`, ~97 for `--clock-mhz 25`, + /// ~680 for `--jit` -- see [`Self::run_via_cycles`]'s doc for the + /// full measurement methodology), which is expected and not a bug + /// to chase -- `run_for_cycles` tracks cycle-accurate bus/prefetch + /// state that this otherwise non-cycle-accurate runtime never + /// needed before. Opt-in, for benchmarking only. + pub fn set_clock_mhz(&mut self, mhz: Option) { + self.clock_hz = mhz.map(|mhz| mhz * 1_000_000.0); + } + /// Creates a new core for `cpu_type`, with `fpu_present` controlling /// whether F-line (coprocessor ID 1) opcodes execute as real FPU /// instructions or trap out to [`Cpu::take_hardware_exception`] -- @@ -214,7 +287,12 @@ impl M68kCpu { core.set_cpu_type(cpu_type); core.fpu_present = fpu_present; core.reset_soft(); - Self { core, jit: false } + Self { + core, + jit: false, + clock_hz: None, + cycles: 0, + } } } @@ -340,6 +418,127 @@ impl M68kCpu { shadow.update_stack_pointer(sp); } } + + /// The `--clock-mhz` execution path: runs via + /// [`m68k::CpuCore::run_for_cycles`] instead of `run_batch`, + /// accumulating real emulated cycles into [`Self::cycles`] as it + /// goes -- see [`Self::set_clock_mhz`]'s doc for why this path + /// exists and what it costs. Only called from [`Cpu::run`], and only + /// once [`Self::clock_hz`] is `Some`. + /// + /// Every trap/halt exit this runtime cares about is surfaced + /// identically to the `run_batch` path in [`Cpu::run`] below -- + /// [`m68k::CycleBatchExit`] matches [`m68k::BatchExit`] one-for-one + /// plus one extra variant, `BoundaryRequested` (see below). + /// + /// `run_for_cycles`'s budget parameter is a plain `i32`, unlike + /// `run_batch`'s `u32::MAX`-as-"unbounded" convention, so there is + /// no single call that can express "run until something interesting + /// happens" the way the JIT path's batch size does. Each call is + /// simply capped at `i32::MAX` cycles instead (a hair over three and + /// a half minutes of emulated time even at a generous 10 MHz, far + /// longer than any single guest instruction sequence between two + /// library-call traps takes in practice) and re-issued on + /// `BudgetExhausted`, exactly mirroring how [`Cpu::run`]'s + /// `run_batch` loop already treats its own `BudgetExhausted` exit as + /// "nothing happened yet, keep going". + /// + /// This runtime never installs the sanitizer's per-instruction hooks + /// ([`Self::sanitize_before_instruction`]/ + /// [`Self::sanitize_after_instruction`]) here, which is why the CLI + /// (`main.rs`'s `check_clock_mhz_sanitize`) refuses `--clock-mhz` + /// together with `--sanitize` outright rather than letting a caller + /// combine them: the shadow map's own byte-level checks (routed + /// through ordinary [`AddressSpace`] reads/writes, which + /// `run_for_cycles` always uses -- it has no `fast_mem` raw-pointer + /// fast path to bypass them) would still run, but the return-address + /// and below-`A7` bookkeeping that needs strict one-instruction + /// granularity would see many instructions retire between + /// publications -- a stale shadow call stack and PC-attributed + /// violations pinned to whatever address happened to be current + /// several cycle-batches ago, while `--sanitize` still prints a + /// normal-looking summary as if every instruction had been checked. + /// A caller that reaches this method directly (bypassing the CLI) + /// with a shadow map installed gets exactly that silently degraded + /// behavior -- this method itself does not (and, short of adopting + /// `run_batch`'s own shadow-map-forces-batches-of-one treatment, + /// cannot cheaply) guard against it; only the CLI's explicit refusal + /// does. + /// + /// # Measurement methodology (for [`Self::set_clock_mhz`]'s + /// perf numbers) + /// + /// [CoreMark 1.0](https://github.com/eembc/coremark) + /// (`~/src/external/coremark/coremark.amiga`, `-O2 -m68020 + /// -msoft-float`), run under `--cpu 68020`. CoreMark self-calibrates + /// its own iteration count against `ReadEClock`, and self-reports + /// "iterations/sec" computed from the same clock -- both entirely + /// valid for `--no-jit`/`--jit` (host wall-clock `ReadEClock`), but + /// meaningless for `--clock-mhz` (which redefines what `ReadEClock` + /// measures). So every figure here is host wall-clock throughput + /// instead: completed iterations (as CoreMark reports them) divided + /// by real elapsed seconds around the whole process (`time`), three + /// runs per mode, averaged. + fn run_via_cycles(&mut self, mem: &mut FlatMemory) -> StopReason { + use m68k::CycleBatchExit; + + const CYCLE_BUDGET: i32 = i32::MAX; + + loop { + let pc = self.pc(); + if pc as usize >= AddressSpace::len(mem) { + return StopReason::PcOutOfBounds { pc }; + } + let result = self.core.run_for_cycles(mem, CYCLE_BUDGET); + // `cycles` is documented as "actual CPU cycles consumed" for + // a positive budget, so this is never negative in practice; + // `max(0)` is just cheap insurance against ever underflowing + // the u64 accumulator if that documented behavior changes. + self.cycles = self.cycles.saturating_add(result.cycles.max(0) as u64); + match result.exit { + CycleBatchExit::BudgetExhausted => continue, + // volamos's `FlatMemory` implements no `sync`/bus- + // boundary-request mechanism at all (see this module's + // `AddressBus` impl), so nothing volamos ever hands the + // `m68k` crate as a bus can actually produce this exit. + // If a future crate version ever raised one anyway, the + // only sane response is the same as `BudgetExhausted`: + // there is no trap/halt to report, so just keep running. + CycleBatchExit::BoundaryRequested => continue, + CycleBatchExit::Stopped => return StopReason::Halted, + CycleBatchExit::AlineTrap { opcode } => { + return StopReason::Trap(TrapInfo { + kind: TrapKind::ALine { opcode }, + pc: self.core.ppc, + }); + } + CycleBatchExit::FlineTrap { opcode } => { + return StopReason::Trap(TrapInfo { + kind: TrapKind::FLine { opcode }, + pc: self.core.ppc, + }); + } + CycleBatchExit::TrapInstruction { trap_num } => { + return StopReason::Trap(TrapInfo { + kind: TrapKind::Trap { trap_num }, + pc: self.core.ppc, + }); + } + CycleBatchExit::Breakpoint { bp_num } => { + return StopReason::Trap(TrapInfo { + kind: TrapKind::Breakpoint { bp_num }, + pc: self.core.ppc, + }); + } + CycleBatchExit::IllegalInstruction { opcode } => { + return StopReason::Trap(TrapInfo { + kind: TrapKind::Illegal { opcode }, + pc: self.core.ppc, + }); + } + } + } + } } impl Cpu for M68kCpu { @@ -408,6 +607,15 @@ impl Cpu for M68kCpu { /// always, in `--no-jit` mode's batch-of-1) just resumes the batch /// loop rather than returning early. fn run(&mut self, mem: &mut Self::Memory) -> StopReason { + // `--clock-mhz` (issue #102) takes over the whole run loop: see + // `Self::run_via_cycles`'s doc for why `run_batch` (`self.jit` + // either way) can't participate in cycle-derived `ReadEClock` + // timing at all, and why the CLI refuses to let both be + // requested at once rather than picking a winner here. + if self.clock_hz.is_some() { + return self.run_via_cycles(mem); + } + use m68k::BatchExit; // An installed shadow map forces batches of one instruction even @@ -562,6 +770,14 @@ impl Cpu for M68kCpu { } true } + + fn emulated_cycles(&self) -> u64 { + self.cycles + } + + fn clock_hz(&self) -> Option { + self.clock_hz + } } #[cfg(test)] @@ -826,4 +1042,103 @@ mod tests { ); assert_eq!(interp_cpu.pc(), jit_cpu.pc()); } + + #[test] + fn emulated_cycles_and_clock_hz_default_to_zero_and_none() { + // Every backend's default posture, per Cpu::emulated_cycles'/ + // Cpu::clock_hz's own doc comments -- these defaults are what + // makes adding both trait methods non-breaking for any other + // Cpu implementation that predates issue #102. + let cpu = M68kCpu::new(); + assert_eq!(Cpu::emulated_cycles(&cpu), 0); + assert_eq!(Cpu::clock_hz(&cpu), None); + } + + #[test] + fn set_clock_mhz_none_leaves_run_on_the_ordinary_run_batch_path() { + // With no clock-mhz mode installed, `Cpu::run` must take exactly + // the same run_batch path it always has -- this is the "strictly + // additive, zero behaviour change when the flag is absent" + // requirement from issue #102. set_clock_mhz(None) (the same + // thing M68kCpu::new()/with_config() already leave it at) must + // not, on its own, start counting cycles. + let (mut cpu, mut mem) = new_cpu_with_memory(0x3000); + cpu.set_clock_mhz(None); + let start = cpu.pc(); + // MOVEQ.L #5, D0, then an A-line trap so `run` (which executes + // until a trap/halt, unlike `step`) has somewhere to stop. + load_words(&mut mem, start, &[0x7005, 0xA000]); + + let reason = cpu.run(&mut mem); + + assert!(matches!( + reason, + StopReason::Trap(TrapInfo { + kind: TrapKind::ALine { opcode: 0xA000 }, + .. + }) + )); + assert_eq!(cpu.data_register(DataRegister(0)), 5); + assert_eq!(cpu.emulated_cycles(), 0); + assert_eq!(cpu.clock_hz(), None); + } + + #[test] + fn clock_mhz_mode_accumulates_real_cycles_and_still_reports_traps() { + // A DBRA-based loop, same shape as + // jit_batch_execution_matches_interpreter_for_a_backward_branch_loop + // above, run through the run_for_cycles path instead. Two + // things this pins down: (1) the trap the loop ends on is still + // reported exactly like every other execution mode, and (2) + // Cpu::emulated_cycles() actually goes up -- a MOVEQ/NOP/DBRA + // sequence executed 5 times plus the trapping opcode fetch is + // dozens of real 68000 cycles, so any plausible lower bound + // catches "never accumulated anything" without pinning this + // test to the m68k crate's exact per-instruction cycle counts + // (an internal detail of that crate, not this one). + let words: &[u16] = &[ + 0x7004, // MOVEQ #4, D0 + 0x4E71, // [loop] NOP + 0x51C8, 0xFFFC, // DBRA D0, loop (disp = -4) + 0xA000, // A-line trap: stop here + ]; + let mut mem = FlatMemory::new(0x3000); + load_words(&mut mem, TRAP_TABLE_END, words); + let mut cpu = M68kCpu::new(); + cpu.set_clock_mhz(Some(25.0)); + cpu.set_pc(TRAP_TABLE_END); + + let reason = cpu.run(&mut mem); + + assert!(matches!( + reason, + StopReason::Trap(TrapInfo { + kind: TrapKind::ALine { opcode: 0xA000 }, + .. + }) + )); + // DBRA is a word-sized decrement: D0 started at 4 (from MOVEQ, + // zero-extended in the high word), looped 5 times down to -1 as + // a 16-bit value (0xFFFF), leaving the high word untouched. + assert_eq!(cpu.data_register(DataRegister(0)), 0x0000_FFFF); + assert!( + cpu.emulated_cycles() >= 20, + "expected at least 20 real 68000 cycles for a 5-iteration NOP/DBRA loop, got {}", + cpu.emulated_cycles() + ); + assert_eq!(cpu.clock_hz(), Some(25_000_000.0)); + } + + #[test] + fn clock_mhz_mode_also_reports_pc_out_of_bounds() { + // Same guarantee as jit_mode_also_reports_pc_out_of_bounds above, + // for the third execution path Cpu::run can now take. + let (mut cpu, mut mem) = new_cpu_with_memory(0x3000); + cpu.set_clock_mhz(Some(7.0)); + cpu.set_pc(0xFFFF_FFD1); + + let reason = cpu.run(&mut mem); + + assert_eq!(reason, StopReason::PcOutOfBounds { pc: 0xFFFF_FFD1 }); + } } diff --git a/crates/volamos-core/src/cpu.rs b/crates/volamos-core/src/cpu.rs index 84e4bb5..bf78244 100644 --- a/crates/volamos-core/src/cpu.rs +++ b/crates/volamos-core/src/cpu.rs @@ -202,4 +202,54 @@ pub trait Cpu { /// real hardware semantics, faithfully reproduced, not a bug in /// this method. fn take_hardware_exception(&mut self, mem: &mut Self::Memory, kind: TrapKind) -> bool; + + /// Accumulated real m68k CPU cycles consumed so far, for a backend + /// that tracks them at all -- see + /// [`crate::backend::M68kCpu::set_clock_mhz`] (the CLI's + /// `--clock-mhz` flag, issue #102) for the one implementation that + /// does. + /// + /// **Only [`Cpu::run`] accumulates cycles, never [`Cpu::step`].** + /// `M68kCpu::run_via_cycles` (the method backing this) is reached + /// exclusively through `Cpu::run`; `M68kCpu::step` always goes + /// through the plain interpreter (`core.step`), which this crate's + /// backend never wires up to any cycle counter. This matters + /// because a handful of callers reach guest code through `step` + /// directly rather than `run`: `execfmt.rs`'s `RawDoFmt` calls a + /// guest-supplied `PutChProc` callback one instruction at a time + /// (see its step loop, `CALLOUT_STEP_BUDGET`), and its `Supervisor` + /// handler single-steps a guest routine the same way (see its own + /// step loop, `SUPERVISOR_STEP_BUDGET`). Guest instructions executed + /// through either of those cost *zero* emulated cycles under + /// `--clock-mhz`, exactly like time spent inside any other + /// native-Rust library handler -- consistent with, not a special + /// case of, the general "native handlers are free" behavior (see + /// `crate::exectask::read_eclock_handler`'s and the CLI's + /// `--clock-mhz` doc for the user-facing version of this caveat). + /// + /// Defaults to `0` so adding this method is non-breaking: every + /// caller that predates issue #102 already assumes execution carries + /// no cycle count at all (the normal `run_batch` path this runtime + /// has used since Phase 1 never surfaced one -- see `backend.rs`'s + /// module docs on why `m68k::BatchResult` has no `cycles` field), + /// and `0` is indistinguishable from "never asked". Only meaningful + /// together with [`Cpu::clock_hz`]; a caller that cares about actual + /// elapsed emulated time should check `clock_hz().is_some()` first + /// rather than trying to interpret a `0` cycle count on its own. + fn emulated_cycles(&self) -> u64 { + 0 + } + + /// The clock rate ([`Cpu::emulated_cycles`] is being counted at (in + /// Hz), if this backend is currently deriving guest-visible elapsed + /// time from emulated cycles rather than the host wall clock -- + /// `Some` exactly when [`crate::backend::M68kCpu::set_clock_mhz`] + /// installed a rate. `None` (the default for every backend, and for + /// [`crate::backend::M68kCpu`] itself unless `--clock-mhz` was + /// given) means "no such mode active"; callers like + /// [`crate::exectask::read_eclock_handler`] use that to decide + /// whether to fall back to the host clock. + fn clock_hz(&self) -> Option { + None + } } diff --git a/crates/volamos-core/src/exectask.rs b/crates/volamos-core/src/exectask.rs index 2762262..2f79c7a 100644 --- a/crates/volamos-core/src/exectask.rs +++ b/crates/volamos-core/src/exectask.rs @@ -1348,11 +1348,47 @@ fn get_sys_time_handler(ctx: &mut HandlerContext<'_, C>) -> Result<(), D /// meaningful -- so deriving the tick count from the same wall clock as /// [`host_time_secs_micro`] (seconds-since-1978 at 709379 ticks/sec) /// gives correct interval arithmetic, which is the only documented use. +/// +/// That's the default. With `--clock-mhz` (issue #102, see +/// [`crate::backend::M68kCpu::set_clock_mhz`]) the *tick count* instead +/// comes from `ctx.cpu`'s accumulated emulated cycle count +/// ([`Cpu::emulated_cycles`]) divided by the configured clock rate +/// ([`Cpu::clock_hz`]) -- real emulated seconds, converted to +/// E-Clock-rate ticks the same way the host-clock path converts real +/// wall-clock seconds. The point is reproducible, host-load-independent +/// A/B benchmarking: two `ReadEClock` calls bracketing some guest work +/// now measure *emulated* elapsed time (derived from how many m68k +/// cycles the guest actually executed) rather than however long the +/// host machine happened to take, which varies with scheduling noise, +/// thermal throttling, whatever else is running, and so on. +/// +/// **The rate reported in `D0` is always [`ECLOCK_PAL_HZ`], regardless +/// of `--clock-mhz`.** This is deliberate, not an oversight: on real +/// hardware the E-Clock is derived from the video/CIA timing chain, not +/// the CPU's own clock -- a PAL Amiga's E-Clock ticks at the same +/// 709,379 Hz whether it has a stock 68000 or an aftermarket +/// accelerator running many times faster, because the accelerator's +/// higher clock never touches the chipset's E-Clock source. So +/// `--clock-mhz` (which models a faster/slower *CPU*) has no business +/// changing the *rate value* a real program would read out of `D0` -- +/// only the numerator (how much emulated time actually elapsed) is +/// meant to move. Reporting anything else in `D0` here would be a +/// self-inconsistent EClockVal: a caller that (correctly, per the RKRM) +/// divides an interval's tick delta by the rate in `D0` needs that rate +/// to be the same one the ticks were generated at. fn read_eclock_handler(ctx: &mut HandlerContext<'_, C>) -> Result<(), DispatchError> { let dest = ctx.cpu.address_register(AddressRegister(0)); - let (secs, micro) = host_time_secs_micro(); - let ticks = u64::from(secs) * u64::from(ECLOCK_PAL_HZ) - + u64::from(micro) * u64::from(ECLOCK_PAL_HZ) / 1_000_000; + let ticks = match ctx.cpu.clock_hz() { + Some(clock_hz) => { + let emulated_secs = ctx.cpu.emulated_cycles() as f64 / clock_hz; + (emulated_secs * f64::from(ECLOCK_PAL_HZ)) as u64 + } + None => { + let (secs, micro) = host_time_secs_micro(); + u64::from(secs) * u64::from(ECLOCK_PAL_HZ) + + u64::from(micro) * u64::from(ECLOCK_PAL_HZ) / 1_000_000 + } + }; ctx.mem.write_u32(dest, (ticks >> 32) as u32); ctx.mem.write_u32(dest.wrapping_add(4), ticks as u32); ctx.cpu.set_data_register(DataRegister(0), ECLOCK_PAL_HZ); @@ -2933,6 +2969,145 @@ mod tests { ); } + // --- ReadEClock under --clock-mhz (issue #102) --- + + #[test] + fn read_eclock_reports_the_pal_rate_in_d0_even_under_clock_mhz_mode() { + // The single most load-bearing detail of the whole --clock-mhz + // feature: the *rate* ReadEClock reports in D0 must stay + // ECLOCK_PAL_HZ regardless of the configured clock -- see + // read_eclock_handler's doc comment for why (real hardware + // derives the E-Clock from the chipset's video/CIA timing, not + // the CPU's own clock, so a faster/slower emulated CPU has no + // business changing the reported rate, only the numerator). + let _guard = lock_host_break(); + let entry = TRAP_TABLE_END; + + let mut words = Vec::new(); + words.push(move_imm_to_a(6)); // A6 = TIMER_DEVICE_BASE + words.push((TIMER_DEVICE_BASE >> 16) as u16); + words.push(TIMER_DEVICE_BASE as u16); + words.push(move_imm_to_a(0)); // A0 = EClockVal dest (patched) + let ev_idx = words.len(); + words.push(0); + words.push(0); + words.extend_from_slice(&jsr_disp16_a6(-60)); // ReadEClock -> D0 = rate + words.push(RTS); + + let ev_addr = entry + (words.len() as u32) * 2 + 4; + words[ev_idx] = (ev_addr >> 16) as u16; + words[ev_idx + 1] = ev_addr as u16; + + let mut mem = FlatMemory::new(0x2_0000); + load_words(&mut mem, entry, &words); + + let mut cpu = M68kCpu::new(); + cpu.set_clock_mhz(Some(7.14)); // an arbitrary, non-round rate + let mut rt = Runtime::new( + cpu, + mem, + StartConfig { + entry, + load_end: ev_addr + 8, + args: Vec::new(), + ..StartConfig::default() + }, + ); + let mut out = Vec::new(); + let code = rt.run(&mut out, None).expect("run should succeed"); + assert_eq!( + code as u32, ECLOCK_PAL_HZ, + "ReadEClock's D0 rate must stay ECLOCK_PAL_HZ regardless of --clock-mhz" + ); + } + + #[test] + fn read_eclock_under_clock_mhz_mode_derives_ticks_from_emulated_cycles_not_wall_clock() { + // Two ReadEClock calls, bracketing a handful of ordinary guest + // instructions, at a deliberately very slow configured clock + // (100 Hz -- `set_clock_mhz`'s `mhz` argument is in MHz, so + // 0.0001). At that rate even the tiny number of real 68000 + // cycles a few NOPs and their surrounding MOVEA/JSR setup cost + // (tens of cycles) works out to hundreds of milliseconds of + // *emulated* elapsed time -- hundreds of thousands of E-Clock + // ticks. A host wall clock, by contrast, cannot produce + // anywhere near that many ticks for a handful of synchronous + // instructions inside a single host function call (a few + // microseconds at most, worth low single-digit ticks at + // ECLOCK_PAL_HZ's ~0.71 ticks/microsecond). So a large observed + // delta here can only come from the cycle-derived formula, not + // from `host_time_secs_micro`, which is exactly what this test + // pins down: read_eclock_handler must actually be branching on + // `Cpu::clock_hz` rather than falling through to the host-clock + // path by accident. + let _guard = lock_host_break(); + let entry = TRAP_TABLE_END; + + let mut words = Vec::new(); + words.push(move_imm_to_a(6)); // A6 = TIMER_DEVICE_BASE + words.push((TIMER_DEVICE_BASE >> 16) as u16); + words.push(TIMER_DEVICE_BASE as u16); + words.push(move_imm_to_a(0)); // A0 = ev1 dest (patched) + let ev1_idx = words.len(); + words.push(0); + words.push(0); + words.extend_from_slice(&jsr_disp16_a6(-60)); // ReadEClock #1 + words.push(0x4E71); // NOP + words.push(0x4E71); // NOP + words.push(0x4E71); // NOP -- "guest instructions between them" + words.push(move_imm_to_a(0)); // A0 = ev2 dest (patched) + let ev2_idx = words.len(); + words.push(0); + words.push(0); + words.extend_from_slice(&jsr_disp16_a6(-60)); // ReadEClock #2 + words.push(RTS); + + let ev1_addr = entry + (words.len() as u32) * 2 + 4; + let ev2_addr = ev1_addr + 8; + words[ev1_idx] = (ev1_addr >> 16) as u16; + words[ev1_idx + 1] = ev1_addr as u16; + words[ev2_idx] = (ev2_addr >> 16) as u16; + words[ev2_idx + 1] = ev2_addr as u16; + + let mut mem = FlatMemory::new(0x2_0000); + load_words(&mut mem, entry, &words); + + let mut cpu = M68kCpu::new(); + cpu.set_clock_mhz(Some(0.0001)); // 100 Hz + let mut rt = Runtime::new( + cpu, + mem, + StartConfig { + entry, + load_end: ev2_addr + 8, + args: Vec::new(), + ..StartConfig::default() + }, + ); + let mut out = Vec::new(); + let code = rt.run(&mut out, None).expect("run should succeed"); + assert_eq!( + code as u32, ECLOCK_PAL_HZ, + "ReadEClock's D0 rate must stay ECLOCK_PAL_HZ under --clock-mhz too" + ); + + let ev1 = (u64::from(rt.memory().read_u32(ev1_addr)) << 32) + | u64::from(rt.memory().read_u32(ev1_addr + 4)); + let ev2 = (u64::from(rt.memory().read_u32(ev2_addr)) << 32) + | u64::from(rt.memory().read_u32(ev2_addr + 4)); + assert!( + ev2 > ev1, + "the second ReadEClock should report a later tick count than the first" + ); + let delta = ev2 - ev1; + assert!( + delta > 50_000, + "a handful of guest instructions at 100 Hz should be hundreds of thousands of \ + E-Clock ticks apart if the delta is cycle-derived, not wall-clock-derived; got \ + {delta}" + ); + } + // --- CloseDevice --- #[test] diff --git a/crates/volamos/src/main.rs b/crates/volamos/src/main.rs index e390fff..ec845bf 100644 --- a/crates/volamos/src/main.rs +++ b/crates/volamos/src/main.rs @@ -167,6 +167,31 @@ impl InstrumentationOptions { } } +/// Where an `opts.jit == true` actually came from -- an explicit +/// `--jit`/`--no-jit` on this command line, or a `JIT=true`/`JIT=false` +/// line in a `~/.volamos`/`.volamos` config file (see +/// [`crate::config`]'s module doc for the full precedence chain). Exists +/// solely so [`check_clock_mhz_jit`]'s error message can name the flag +/// the user actually needs to change, rather than always saying "drop +/// --jit" to someone who never typed it -- a `JIT=true` sitting in a +/// config file they may not even remember exists is a much easier +/// mistake to make than a stray `--jit` on the command line right in +/// front of the `--clock-mhz` that conflicts with it. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum JitSource { + /// `jit` is `false` (the default, or an explicit `--no-jit`/ + /// `JIT=false`) -- never actually consulted by + /// [`check_clock_mhz_jit`], since there's nothing to complain about, + /// but kept as the [`Options::jit_source`] default so every + /// `Options` value has one. + Default, + /// `jit` is `true` because of `--jit` on this command line. + CommandLine, + /// `jit` is `true` because of `JIT=true` in a config file, with no + /// overriding `--jit`/`--no-jit` on the command line itself. + ConfigFile, +} + #[derive(Debug)] struct Options { verbose: bool, @@ -182,6 +207,10 @@ struct Options { cpu_type: CpuType, fpu: bool, jit: bool, + /// See [`JitSource`]. `Default` whenever `jit` is `false`; otherwise + /// names which source (`--jit` or a config file's `JIT=true`) is + /// responsible, for [`check_clock_mhz_jit`]'s error message. + jit_source: JitSource, /// `--sanitize`: installs a [`volamos_core::sanitize::ShadowMap`] on /// the guest [`FlatMemory`] and forces the JIT off (see [`run`]) -- /// see `volamos_core::sanitize`'s module doc for the detector @@ -192,6 +221,25 @@ struct Options { /// per-invocation choice rather than something a stale config file /// silently turns on. See [`InstrumentationOptions`] for the family. sanitize: InstrumentationOptions, + /// `--clock-mhz N` (issue #102): install an emulated clock rate of + /// `N` MHz and switch `timer.device`'s `ReadEClock` from host + /// wall-clock time to time derived from the CPU's own emulated + /// cycle count at that rate -- see + /// [`volamos_core::backend::M68kCpu::set_clock_mhz`]'s doc for the + /// full mechanism and [`volamos_core::exectask::read_eclock_handler`] + /// for why the *rate* `ReadEClock` reports in `D0` is unaffected by + /// this. `None` (the default) is unchanged host-clock behavior. + /// + /// CLI-only, like `sanitize`/`net` -- kept out of + /// `config::Overrides` deliberately: this changes what a guest + /// program's own timing measurements *mean* (emulated time instead + /// of real time), which is exactly the kind of thing a stale + /// `~/.volamos`/`.volamos` shouldn't be able to silently flip on for + /// every future run in that directory. + /// + /// Mutually exclusive with `jit` and with `sanitize.enabled`: see + /// [`check_clock_mhz_jit`]/[`check_clock_mhz_sanitize`]. + clock_mhz: Option, net: bool, /// The built-in defaults layer's own lazy-creation bookkeeping /// (issue #43), if that layer is active -- passed straight through @@ -232,7 +280,7 @@ fn print_usage(program_name: &str) { [-a NAME:target[+target...]]... [--cwd AMIGAPATH] \ [--auto-assign HOSTDIR] [--defaults|--no-defaults] [--volumes-dir HOSTDIR] \ [--stack SIZE] [--ram SIZE] [--cpu MODEL] \ - [--fpu|--no-fpu] [--jit|--no-jit] [--sanitize] [--sanitize-uninit] \ + [--fpu|--no-fpu] [--jit|--no-jit] [--clock-mhz N] [--sanitize] [--sanitize-uninit] \ [--sanitize-ignore-pc ADDR] [--dirty-heap] [--net] [args...]" ); eprintln!(); @@ -309,6 +357,51 @@ fn print_usage(program_name: &str) { " reference); every library-call trap boundary is identical" ); eprintln!(" either way"); + eprintln!( + " --clock-mhz N report timer.device's ReadEClock as emulated time derived" + ); + eprintln!( + " from the CPU's own emulated cycle count at N MHz (fractional" + ); + eprintln!( + " values allowed, e.g. 25 or 7.14), instead of host wall-clock" + ); + eprintln!(" time -- for reproducible, host-load-independent A/B"); + eprintln!( + " benchmarking. Off by default. Cannot be combined with an" + ); + eprintln!( + " explicit --jit (run_batch's trace JIT never tracks a cycle" + ); + eprintln!( + " count, so there'd be nothing to derive emulated time from)" + ); + eprintln!( + " or with --sanitize (the cycle-counted execution path skips" + ); + eprintln!(" the sanitizer's per-instruction hooks entirely, so its"); + eprintln!(" checks would be silently incomplete rather than merely"); + eprintln!( + " slow). This is the slowest execution mode: measured ~2.2x" + ); + eprintln!( + " slower than --no-jit and ~7x slower than --jit (CoreMark 1.0" + ); + eprintln!(" on --cpu 68020, host wall-clock throughput). volamos's"); + eprintln!( + " memory bus has no wait states at all, so memory-bound guest" + ); + eprintln!( + " code still won't match real hardware timing, and time spent" + ); + eprintln!( + " inside volamos's own native-Rust library handlers (e.g." + ); + eprintln!(" CopyMem) or single-stepped guest callbacks (RawDoFmt's"); + eprintln!( + " PutChProc, Supervisor's routine) costs zero emulated cycles" + ); + eprintln!(" and is invisible in the reported total"); eprintln!(" --sanitize enable shadow-memory checking of guest accesses (heap"); eprintln!( " redzones, freed blocks, below-stack-pointer reads/writes);" @@ -413,6 +506,70 @@ fn parse_byte_size(flag: &str, s: &str) -> Result { .ok_or_else(|| format!("{flag} value {s:?} overflows")) } +/// An upper bound `--clock-mhz` refuses outright (issue #102, see +/// [`parse_clock_mhz`]). Exists purely to turn a fat-fingered entry (an +/// extra digit, a misplaced decimal point) into a clear error instead of +/// a silently-nonsensical "the CPU runs at a terahertz" benchmark run -- +/// real classic-Amiga hardware, up to and including the fastest +/// Vampire-class FPGA accelerators on the market, never gets remotely +/// close to even a tenth of this. +const MAX_CLOCK_MHZ: f64 = 10_000.0; + +/// A lower bound `--clock-mhz` refuses outright, for the same reason +/// [`MAX_CLOCK_MHZ`] exists but at the other end of the scale: without +/// one, something like `--clock-mhz 1e-30` parses as a perfectly +/// ordinary positive, finite `f64` and sails straight through the +/// `is_finite() && > 0.0` check, but drives every `ReadEClock` tick +/// count (`cycles / clock_hz * ECLOCK_PAL_HZ`, see +/// `read_eclock_handler`) to `f64::INFINITY`, which then saturates on +/// the `as u64` cast to `u64::MAX` -- harmless (no panic, no UB), but a +/// benchmark silently reporting the largest possible tick count instead +/// of a clear rejection is exactly the kind of "looks fine, isn't" +/// result `--clock-mhz` exists to avoid. `1 Hz` is generously below any +/// clock rate a real or realistically-slowed-down classic Amiga could +/// plausibly model (even a 1980s calculator's clock beats it), while +/// staying far away from the zero/underflow edge this guards against. +const MIN_CLOCK_MHZ: f64 = 0.000_001; + +/// Parses a `--clock-mhz N` value: a positive clock rate in megahertz +/// for `ReadEClock`'s cycle-derived emulated-time mode (issue #102, see +/// [`volamos_core::backend::M68kCpu::set_clock_mhz`]). Fractional values +/// are accepted (`N.parse::()`) rather than requiring a whole +/// number -- Copperline, this project's own hardware-timing oracle, +/// models an A600's 68000 as `clock_mhz = 25.0`, and a real accelerator +/// board's rated speed is routinely a fraction (e.g. "14.28 MHz" for an +/// early NTSC-derived clock doubler), so an integer-only parser would +/// force a caller to round away the exact rate they're trying to model. +/// +/// Rejects empty/non-numeric input, `0` and negative values (there is no +/// such thing as a non-positive clock rate), `NaN`/`inf`/`-inf` (which +/// `f64::parse` otherwise happily accepts and which would poison every +/// downstream division in `Cpu::run_via_cycles`/`read_eclock_handler`), +/// and anything outside [`MIN_CLOCK_MHZ`]..=[`MAX_CLOCK_MHZ`]. +fn parse_clock_mhz(s: &str) -> Result { + let value: f64 = s + .parse() + .map_err(|_| format!("--clock-mhz expects a positive number of MHz, got {s:?}"))?; + if !value.is_finite() || value <= 0.0 { + return Err(format!( + "--clock-mhz expects a positive number of MHz, got {s:?}" + )); + } + if value < MIN_CLOCK_MHZ { + return Err(format!( + "--clock-mhz {value} is implausibly low (under {MIN_CLOCK_MHZ} MHz) -- check for a \ + typo" + )); + } + if value > MAX_CLOCK_MHZ { + return Err(format!( + "--clock-mhz {value} is implausibly high (over {MAX_CLOCK_MHZ} MHz) -- check for a \ + typo" + )); + } + Ok(value) +} + /// Parses a `--cpu MODEL` value (case-insensitive) into a [`CpuType`]. /// Covers every real model the `m68k` crate models -- see /// [`print_usage`] for the accepted spellings. @@ -562,6 +719,19 @@ fn split_name_value<'a>(flag: &str, arg: &'a str) -> Result<(&'a str, &'a str), } } +/// [`parse_args_raw`]'s return type, factored out purely to satisfy +/// clippy's `type_complexity` lint -- see that function's doc for what +/// each element means; `Option` is `--clock-mhz`'s CLI-only value +/// (issue #102), threaded alongside `InstrumentationOptions` for the +/// same reason (see `Options::clock_mhz`'s doc). +type RawParsedArgs = ( + config::Overrides, + InstrumentationOptions, + Option, + String, + Vec, +); + /// Hand-rolled argument parsing: this CLI's surface is small enough that /// pulling in an argument-parsing crate isn't worth the dependency. /// @@ -574,21 +744,17 @@ fn split_name_value<'a>(flag: &str, arg: &'a str) -> Result<(&'a str, &'a str), /// (see `crate::config`'s module doc). [`parse_args`] is the /// no-config-files convenience wrapper most callers (and every existing /// test) actually want. -fn parse_args_raw( - mut args: impl Iterator, -) -> Result< - ( - config::Overrides, - InstrumentationOptions, - String, - Vec, - ), - String, -> { +fn parse_args_raw(mut args: impl Iterator) -> Result { let mut overrides = config::Overrides::default(); // CLI-only, unlike every other flag here -- see `Options::sanitize`'s // doc for why this deliberately isn't part of `config::Overrides`. let mut sanitize = InstrumentationOptions::default(); + // Also CLI-only, for the same reason -- see `Options::clock_mhz`'s + // doc. Kept as its own local (rather than folded into + // `InstrumentationOptions`) because it isn't part of that family's + // shadow-map-instrumentation theme; it changes what `ReadEClock` + // reports, not how guest memory accesses are checked. + let mut clock_mhz: Option = None; let mut program = None; let mut guest_args = Vec::new(); @@ -652,6 +818,12 @@ fn parse_args_raw( "--no-fpu" => overrides.fpu = Some(false), "--jit" => overrides.jit = Some(true), "--no-jit" => overrides.jit = Some(false), + "--clock-mhz" => { + let value = args + .next() + .ok_or_else(|| "--clock-mhz requires an N argument".to_string())?; + clock_mhz = Some(parse_clock_mhz(&value)?); + } "--sanitize" => sanitize.enabled = true, "--dirty-heap" => sanitize.dirty_heap = true, // Implies --sanitize: asking for uninitialized-read @@ -688,7 +860,7 @@ fn parse_args_raw( } let program = program.ok_or_else(|| "missing argument".to_string())?; - Ok((overrides, sanitize, program, guest_args)) + Ok((overrides, sanitize, clock_mhz, program, guest_args)) } /// Fills every unset field of `overrides` with its built-in default, @@ -698,9 +870,27 @@ fn parse_args_raw( fn resolve( overrides: config::Overrides, sanitize: InstrumentationOptions, + clock_mhz: Option, + cli_jit: Option, program: String, guest_args: Vec, ) -> Options { + let jit = overrides.jit.unwrap_or(false); + // See JitSource's doc: only matters when `jit` is actually `true`, + // in which case it's either straight from this command line's own + // `--jit` (`cli_jit == Some(true)`), or `overrides.jit` only ended + // up `true` because a config file's `JIT=true` won out over an + // absent CLI flag (`config::merge`'s CLI-wins-when-present rule + // means `cli_jit` can only be `Some(false)` or `None` here, never + // `Some(true)` -- if it were, `overrides.jit` would already equal + // it). + let jit_source = if !jit { + JitSource::Default + } else if cli_jit == Some(true) { + JitSource::CommandLine + } else { + JitSource::ConfigFile + }; Options { verbose: overrides.verbose.unwrap_or(false), snoop: overrides.snoop.unwrap_or(false), @@ -714,8 +904,10 @@ fn resolve( ram_size: overrides.ram_size.unwrap_or(DEFAULT_RAM_SIZE), cpu_type: overrides.cpu_type.unwrap_or(CpuType::M68000), fpu: overrides.fpu.unwrap_or(false), - jit: overrides.jit.unwrap_or(false), + jit, + jit_source, sanitize, + clock_mhz, net: overrides.net.unwrap_or(false), // Only ever non-empty when `overrides` already includes // `config::built_in_defaults`'s own layer -- `main` merges that @@ -735,8 +927,11 @@ fn resolve( /// module wants instead. #[cfg(test)] fn parse_args(args: impl Iterator) -> Result { - let (overrides, sanitize, program, guest_args) = parse_args_raw(args)?; - Ok(resolve(overrides, sanitize, program, guest_args)) + let (overrides, sanitize, clock_mhz, program, guest_args) = parse_args_raw(args)?; + let cli_jit = overrides.jit; + Ok(resolve( + overrides, sanitize, clock_mhz, cli_jit, program, guest_args, + )) } /// Works out the initial guest current directory per the defaulting rule @@ -901,6 +1096,91 @@ fn check_ram_addressable(ram_size: u32, cpu_type: CpuType) -> Result<(), String> )) } +/// Refuses `--clock-mhz` together with `jit` set (issue #102): `jit` +/// only ever becomes `true` via an explicit override -- `--jit` on the +/// command line, or `JIT=true` in a config file -- since +/// [`resolve`]'s own default is `false` (see [`print_usage`]'s `--jit`/ +/// `--no-jit` doc). There is therefore no "default-on JIT" case this +/// could spuriously trip on: every `jit == true` this ever sees really +/// was asked for, by someone, somewhere. `jit_source` (see +/// [`JitSource`]) says which -- used only to phrase the error message +/// around whichever one it actually was, so a `JIT=true` sitting +/// forgotten in a config file doesn't get blamed on a `--jit` the user +/// never typed on this command line. +/// +/// The combination is refused rather than one flag silently winning +/// (the way `--sanitize` silently forces the JIT off) because there is +/// no approximate answer to fall back to here: `run_batch` (the JIT's +/// execution path) never surfaces a cycle count at all -- see +/// `volamos_core::backend::M68kCpu::set_clock_mhz`'s doc -- so +/// `--clock-mhz` would have literally nothing to derive `ReadEClock`'s +/// reported time from. Reporting `0` or falling back to the host clock +/// without saying so would silently give a benchmark run numbers that +/// don't mean what `--clock-mhz` promised; an explicit error is the +/// honest answer. +fn check_clock_mhz_jit( + clock_mhz: Option, + jit: bool, + jit_source: JitSource, +) -> Result<(), String> { + if !(clock_mhz.is_some() && jit) { + return Ok(()); + } + let how_to_fix = match jit_source { + JitSource::CommandLine => "drop --jit (or pass --no-jit, the default)", + JitSource::ConfigFile => { + "pass --no-jit on the command line to override it, or drop JIT=true from your \ + ~/.volamos/.volamos config file" + } + JitSource::Default => { + unreachable!("jit_source is Default whenever jit is false, but jit is true here") + } + }; + Err(format!( + "--clock-mhz cannot be combined with --jit: the trace JIT (run_batch) never tracks a \ + cycle count, so there is nothing for --clock-mhz to derive ReadEClock's emulated time \ + from -- {how_to_fix} to use --clock-mhz" + )) +} + +/// Refuses `--clock-mhz` together with `--sanitize` (issue #102 code +/// review): `M68kCpu::run_via_cycles` -- the execution path +/// `--clock-mhz` switches [`Cpu::run`](volamos_core::cpu::Cpu::run) to +/// -- never calls the per-instruction sanitizer hooks +/// (`sanitize_before_instruction`/`sanitize_after_instruction`) the +/// ordinary `run_batch` path calls on every single instruction when a +/// shadow map is installed: no `set_current_pc` publication, no +/// `check_return`/`record_call` shadow-call-stack bookkeeping, no +/// `update_stack_pointer` below-`A7` tracking. Running both flags +/// together wouldn't merely be slower or imprecise -- key parts of the +/// detector would be silently dead (a stale shadow call stack +/// comparing new returns against frames from far earlier in the run) +/// or attributing every violation in a whole cycle-budget's worth of +/// instructions to one stale PC, while `--sanitize` still prints its +/// normal-looking "no violations" or "N violations" summary as if +/// everything had been checked. +/// +/// [`check_clock_mhz_jit`]'s own doc makes the applicable argument: +/// "an explicit error is the honest answer" beats a silently +/// compromised result, and that applies at least as strongly to a +/// safety/correctness tool quietly running with its instrumentation +/// half-disabled as it does to `--clock-mhz` having no cycle count to +/// read. Wiring the sanitizer hooks into `run_via_cycles` properly +/// (matching `run_batch`'s shadow-map-forces-batches-of-one treatment) +/// is future work, not something to fake with a doc-comment caveat. +fn check_clock_mhz_sanitize(clock_mhz: Option, sanitize_enabled: bool) -> Result<(), String> { + if clock_mhz.is_some() && sanitize_enabled { + return Err( + "--clock-mhz cannot be combined with --sanitize: the cycle-counted execution path \ + --clock-mhz uses (M68kCpu::run_via_cycles) does not run the sanitizer's \ + per-instruction shadow-map hooks, so --sanitize's checks would be silently \ + incomplete rather than merely slow -- drop one of the two flags" + .to_string(), + ); + } + Ok(()) +} + /// Checks that `stack_size` plus [`MIN_HEAP_HEADROOM`] actually fits /// between `load_end` (the loaded program's own end address) and /// `ram_size` (the top of the guest address space) -- see @@ -936,6 +1216,7 @@ fn run_nested_program( fpu: bool, jit: bool, sanitize: InstrumentationOptions, + clock_mhz: Option, net: bool, ) -> i32 { let Ok(bytes) = std::fs::read(host_path) else { @@ -979,6 +1260,13 @@ fn run_nested_program( // shadow map is installed, but making it explicit here means a // nested run never even attempts the JIT path in the first place). cpu.set_jit(jit && !sanitize.enabled); + // --clock-mhz (issue #102): the top-level run already validated + // this against `jit` (see `check_clock_mhz_jit`) before this + // nested-run closure was ever installed, so `jit`/`clock_mhz` here + // are the same already-consistent pair -- a nested System()/ + // Execute() run gets its own fresh cycle counter, starting at `0` + // again, same as its own fresh CPU. + cpu.set_clock_mhz(clock_mhz); let mut runtime = Runtime::new(cpu, mem, config); if sanitize.dirty_heap { runtime.enable_dirty_heap(); @@ -1052,10 +1340,15 @@ fn build_runtime_with_vfs( } fn run(opts: &Options) -> Result { - // Before anything is read or loaded: this one depends only on the - // flags, and a run that violates it fails much later in a way that - // does not point at them. + // Before anything is read or loaded: these depend only on the + // flags, and a run that violates any of them fails much later in a + // way that does not point at them. --jit is checked before + // --sanitize (matching the doc comments' own ordering) purely so + // a run that somehow violates both reports the --jit conflict + // first -- there's no other significance to the order. check_ram_addressable(opts.ram_size, opts.cpu_type)?; + check_clock_mhz_jit(opts.clock_mhz, opts.jit, opts.jit_source)?; + check_clock_mhz_sanitize(opts.clock_mhz, opts.sanitize.enabled)?; let bytes = std::fs::read(&opts.program) .map_err(|e| format!("couldn't read '{}': {e}", opts.program))?; @@ -1069,6 +1362,11 @@ fn run(opts: &Options) -> Result { // over --jit, since the JIT's fast_mem path would otherwise bypass // every shadow-map check. cpu.set_jit(opts.jit && !opts.sanitize.enabled); + // --clock-mhz (issue #102): already validated against --jit above, + // so this is never fighting the JIT for control of Cpu::run -- see + // M68kCpu::set_clock_mhz's doc for how the two would conflict if it + // weren't. + cpu.set_clock_mhz(opts.clock_mhz); let program_name = program_name_from_path(std::path::Path::new(&opts.program)); let vfs_config = vfs_config_from_opts(opts); @@ -1148,6 +1446,7 @@ fn run(opts: &Options) -> Result { let nested_fpu = opts.fpu; let nested_jit = opts.jit; let nested_sanitize = opts.sanitize.clone(); + let nested_clock_mhz = opts.clock_mhz; let nested_net = opts.net; runtime.set_system_runner(move |req| { @@ -1162,6 +1461,7 @@ fn run(opts: &Options) -> Result { nested_fpu, nested_jit, nested_sanitize.clone(), + nested_clock_mhz, nested_net, ) }); @@ -1200,7 +1500,7 @@ fn main() -> ExitCode { // -h/--help and any CLI parse error short-circuit here, before // ~/.volamos/.volamos are even read -- neither is relevant to // those paths (see parse_args_raw's doc). - let (cli_overrides, sanitize, program, guest_args) = match parse_args_raw(args) { + let (cli_overrides, sanitize, clock_mhz, program, guest_args) = match parse_args_raw(args) { Ok(v) => v, Err(msg) => { if !msg.is_empty() { @@ -1231,6 +1531,12 @@ fn main() -> ExitCode { // `built_in_defaults` does no I/O itself (see its own doc) -- the // directories it names are created lazily, by `Vfs`, only if the // guest program actually uses them. + // + // Captured before `cli_overrides` is consumed by the merge below -- + // this is the CLI's own `--jit`/`--no-jit` value alone, needed by + // `resolve` to tell "explicit --jit" apart from "JIT=true only in a + // config file" for JitSource/check_clock_mhz_jit's error message. + let cli_jit = cli_overrides.jit; let base = config::merge(cli_overrides, file_overrides); let defaults_enabled = base.standard_volumes.unwrap_or(true); let volumes_dir = base.volumes_dir.clone(); @@ -1240,7 +1546,7 @@ fn main() -> ExitCode { base }; - let opts = resolve(merged, sanitize, program, guest_args); + let opts = resolve(merged, sanitize, clock_mhz, cli_jit, program, guest_args); // Cleanup happens here, once, after `run` (and every nested // System()/Execute() it spawned, all sharing this same @@ -1849,6 +2155,143 @@ mod tests { assert!(opts.sanitize.enabled); } + // --- --clock-mhz (issue #102) --- + + #[test] + fn default_clock_mhz_is_off() { + let opts = parse_args(args(&["prog"])).unwrap(); + assert_eq!(opts.clock_mhz, None); + } + + #[test] + fn clock_mhz_flag_accepts_an_integer_value() { + let opts = parse_args(args(&["--clock-mhz", "25", "prog"])).unwrap(); + assert_eq!(opts.clock_mhz, Some(25.0)); + } + + #[test] + fn clock_mhz_flag_accepts_a_fractional_value() { + // Copperline's own A600/Gayle configuration models a 25 MHz + // 68000, and a real accelerator's rated speed is routinely a + // fraction (e.g. an NTSC-derived clock-doubler board) -- an + // integer-only parser would force rounding away the exact rate + // a caller is trying to model, so fractional MHz must parse. + let opts = parse_args(args(&["--clock-mhz", "7.14", "prog"])).unwrap(); + assert_eq!(opts.clock_mhz, Some(7.14)); + } + + #[test] + fn clock_mhz_flag_rejects_zero_negative_and_non_numeric() { + assert!(parse_args(args(&["--clock-mhz", "0", "prog"])).is_err()); + assert!(parse_args(args(&["--clock-mhz", "-5", "prog"])).is_err()); + assert!(parse_args(args(&["--clock-mhz", "nonsense", "prog"])).is_err()); + assert!(parse_args(args(&["--clock-mhz"])).is_err()); + } + + #[test] + fn clock_mhz_flag_rejects_nan_and_infinity() { + // f64::parse happily accepts these spellings; --clock-mhz must + // not, or they'd poison every downstream division in + // M68kCpu::run_via_cycles/read_eclock_handler. + assert!(parse_args(args(&["--clock-mhz", "NaN", "prog"])).is_err()); + assert!(parse_args(args(&["--clock-mhz", "inf", "prog"])).is_err()); + assert!(parse_args(args(&["--clock-mhz", "-inf", "prog"])).is_err()); + } + + #[test] + fn clock_mhz_flag_rejects_an_absurdly_high_value() { + assert!(parse_args(args(&["--clock-mhz", "1000000", "prog"])).is_err()); + } + + #[test] + fn clock_mhz_flag_rejects_an_absurdly_low_value() { + // Without a floor, --clock-mhz 1e-30 parses as an ordinary + // positive finite f64 and drives every ReadEClock tick count to + // f64::INFINITY, silently saturating to u64::MAX on the cast -- + // no panic, but a benchmark reporting the largest possible tick + // count instead of a clear rejection is exactly the "looks + // fine, isn't" result this flag exists to avoid. + assert!(parse_args(args(&["--clock-mhz", "1e-30", "prog"])).is_err()); + } + + #[test] + fn clock_mhz_with_explicit_jit_is_rejected_by_check_clock_mhz_jit() { + // run_batch (the JIT path) never surfaces a cycle count at all, + // so there is nothing for --clock-mhz to derive ReadEClock's + // emulated time from -- this must be a clean error, not a + // silent fallback to the host clock or to --no-jit behavior. + let opts = parse_args(args(&["--clock-mhz", "25", "--jit", "prog"])).unwrap(); + assert_eq!(opts.jit_source, JitSource::CommandLine); + assert!(check_clock_mhz_jit(opts.clock_mhz, opts.jit, opts.jit_source).is_err()); + } + + #[test] + fn clock_mhz_with_default_jit_is_accepted_by_check_clock_mhz_jit() { + // --jit defaults to off (see default_jit_is_off), so an + // ordinary --clock-mhz run with no --jit/--no-jit mentioned at + // all must not spuriously trip the mutual-exclusion check -- + // only an explicit jit=true (which can only come from an + // explicit --jit or a config file's JIT=true) may. + let opts = parse_args(args(&["--clock-mhz", "25", "prog"])).unwrap(); + assert!(!opts.jit); + assert_eq!(opts.jit_source, JitSource::Default); + assert!(check_clock_mhz_jit(opts.clock_mhz, opts.jit, opts.jit_source).is_ok()); + } + + #[test] + fn clock_mhz_with_explicit_no_jit_is_accepted_by_check_clock_mhz_jit() { + let opts = parse_args(args(&["--clock-mhz", "25", "--no-jit", "prog"])).unwrap(); + assert!(check_clock_mhz_jit(opts.clock_mhz, opts.jit, opts.jit_source).is_ok()); + } + + #[test] + fn clock_mhz_jit_error_names_no_jit_when_jit_came_from_the_command_line() { + let err = check_clock_mhz_jit(Some(25.0), true, JitSource::CommandLine).unwrap_err(); + assert!( + err.contains("drop --jit"), + "error should point at the --jit the user actually typed: {err}" + ); + } + + #[test] + fn clock_mhz_jit_error_names_the_config_file_when_jit_came_from_one() { + // A JIT=true sitting in ~/.volamos/.volamos, with no --jit typed + // on this command line, must not be blamed on a --jit the user + // never wrote -- the message should point at --no-jit/the + // config file instead. + let err = check_clock_mhz_jit(Some(25.0), true, JitSource::ConfigFile).unwrap_err(); + assert!( + !err.contains("drop --jit"), + "error should not blame a --jit the user never typed: {err}" + ); + assert!( + err.contains("--no-jit") && err.contains("config"), + "error should mention overriding via --no-jit or editing the config file: {err}" + ); + } + + #[test] + fn clock_mhz_with_sanitize_is_rejected() { + // run_via_cycles (the --clock-mhz execution path) skips every + // per-instruction sanitizer hook -- see check_clock_mhz_sanitize's + // doc. Running both together must be a clean error, not a + // silently-incomplete "no violations" report. + let opts = parse_args(args(&["--clock-mhz", "25", "--sanitize", "prog"])).unwrap(); + assert!(check_clock_mhz_sanitize(opts.clock_mhz, opts.sanitize.enabled).is_err()); + } + + #[test] + fn clock_mhz_without_sanitize_is_accepted_by_check_clock_mhz_sanitize() { + let opts = parse_args(args(&["--clock-mhz", "25", "prog"])).unwrap(); + assert!(check_clock_mhz_sanitize(opts.clock_mhz, opts.sanitize.enabled).is_ok()); + } + + #[test] + fn sanitize_without_clock_mhz_is_accepted_by_check_clock_mhz_sanitize() { + let opts = parse_args(args(&["--sanitize", "prog"])).unwrap(); + assert!(check_clock_mhz_sanitize(opts.clock_mhz, opts.sanitize.enabled).is_ok()); + } + // --- --defaults/--no-defaults/--volumes-dir (issue #43) --- // // `standard_volumes`/`volumes_dir` are consumed by `main` before @@ -1859,13 +2302,13 @@ mod tests { #[test] fn defaults_flag_sets_standard_volumes_on() { - let (overrides, _, _, _) = parse_args_raw(args(&["--defaults", "prog"])).unwrap(); + let (overrides, _, _, _, _) = parse_args_raw(args(&["--defaults", "prog"])).unwrap(); assert_eq!(overrides.standard_volumes, Some(true)); } #[test] fn no_defaults_flag_sets_standard_volumes_off() { - let (overrides, _, _, _) = parse_args_raw(args(&["--no-defaults", "prog"])).unwrap(); + let (overrides, _, _, _, _) = parse_args_raw(args(&["--no-defaults", "prog"])).unwrap(); assert_eq!(overrides.standard_volumes, Some(false)); } @@ -1875,13 +2318,13 @@ mod tests { // CLI parse itself must report "not specified", so a config // file's own DEFAULTS= can still be told apart from an explicit // --defaults. - let (overrides, _, _, _) = parse_args_raw(args(&["prog"])).unwrap(); + let (overrides, _, _, _, _) = parse_args_raw(args(&["prog"])).unwrap(); assert_eq!(overrides.standard_volumes, None); } #[test] fn volumes_dir_flag_sets_the_override() { - let (overrides, _, _, _) = + let (overrides, _, _, _, _) = parse_args_raw(args(&["--volumes-dir", "/custom/vols", "prog"])).unwrap(); assert_eq!(overrides.volumes_dir, Some(PathBuf::from("/custom/vols"))); } diff --git a/userdocs/CLI-Reference.md b/userdocs/CLI-Reference.md index 0ff4b4c..ca364b5 100644 --- a/userdocs/CLI-Reference.md +++ b/userdocs/CLI-Reference.md @@ -3,7 +3,7 @@ ``` volamos [-v|--verbose] [-s|--snoop] [-V NAME:hostdir]... [-a NAME:target[+target...]]... [--cwd AMIGAPATH] [--auto-assign HOSTDIR] [--stack SIZE] [--ram SIZE] - [--cpu MODEL] [--fpu|--no-fpu] [--jit|--no-jit] [--sanitize] + [--cpu MODEL] [--fpu|--no-fpu] [--jit|--no-jit] [--clock-mhz N] [--sanitize] [--sanitize-uninit] [--sanitize-ignore-pc ADDR] [args...] ``` @@ -288,6 +288,88 @@ volamos --jit fixtures/hello | volamos 0.3, interpreter | 198.2 (~1.9x faster than 0.2) | | volamos 0.3, `--jit` | 537.1 (~1.2x faster than 0.2) | +## `--clock-mhz N` + +Makes `timer.device`'s `ReadEClock` report **emulated** time — derived +from the CPU's own emulated cycle count at `N` MHz — instead of host +wall-clock time. `N` may be fractional (e.g. `--clock-mhz 25` or +`--clock-mhz 7.14`). Default: off (host clock, as always). This exists +for reproducible, host-load-independent compiler A/B benchmarks: two +`ReadEClock` calls bracketing some guest work measure how many m68k +cycles actually ran, not however long the host machine happened to take +around them (which varies with scheduling noise, thermal throttling, +whatever else is running on the box at the time). + +```sh +volamos --clock-mhz 25 fixtures/hello +``` + +Cannot be combined with an explicit `--jit`: the trace JIT +(`CpuCore::run_batch`) never tracks a cycle count at all, so there would +be nothing for `--clock-mhz` to derive `ReadEClock`'s reported time +from. `--clock-mhz` implies the plain interpreter path either way +(equivalent to `--no-jit`, the default) — only an *explicit* `--jit` on +top of it is refused. + +Also cannot be combined with [`--sanitize`](#--sanitize): the +cycle-counted execution path `--clock-mhz` uses +(`M68kCpu::run_via_cycles`) doesn't run the sanitizer's per-instruction +shadow-map hooks at all, so `--sanitize` would silently check nothing +while still printing a normal-looking summary — refused for the same +"an explicit error beats a silently wrong answer" reason as the `--jit` +conflict above. + +Only `ReadEClock` changes. `GetSysTime`, `TR_GETSYSTIME`, `DateStamp`, +and `CurrentTime` all still read the real host clock — a benchmark run +under `--clock-mhz` doesn't think it's 1978. + +!!! warning "Read this before trusting the numbers" + - **This is the slowest execution mode volamos has.** Measured with + [CoreMark 1.0](https://github.com/eembc/coremark) + (`~/src/external/coremark/coremark.amiga`, `-O2 -m68020 + -msoft-float`, `--cpu 68020`, host wall-clock throughput — + completed iterations divided by real elapsed seconds, three runs + averaged per mode, since `--clock-mhz` redefines the `ReadEClock` + CoreMark itself times against, so its *self-reported* + iterations/sec isn't a valid host-speed comparison once + `--clock-mhz` is active): the default interpreter (`--no-jit`) + manages ~217 iterations/sec, `--clock-mhz 25` (`CpuCore:: + run_for_cycles`, the `m68k` crate's transaction-exact, + cycle-accurate path) manages ~97 — about **2.2x slower** than + `--no-jit`, and about **7x slower** than `--jit`'s ~680. It's + opt-in, for benchmarking, not for everyday use. + - **volamos's memory bus has no wait states.** `FlatMemory` is a + flat `Vec` with no `sync`/bus-timing model at all — unlike + real Chip/Fast RAM, which real hardware (and + [Copperline](https://github.com/sidick/copperline), volamos's own + hardware-timing oracle) bills real contention cycles for. + Memory-bound guest code will still report a different emulated + time than real hardware for this reason, even with a + correctly-configured `--clock-mhz`. + - **Time spent inside volamos's own native-Rust library handlers is + free.** Every `dos.library`/`exec.library`/... call this runtime + implements natively (see [Supported Libraries]( + Supported-Libraries.md)) costs *zero* emulated cycles — it's host + code, not guest instructions. A workload dominated by, say, + `CopyMem` will report an emulated time implying it spent no time + there at all, because from the CPU's point of view, it didn't. + - **So is time spent inside a guest callback volamos single-steps + rather than runs**, for the same reason: `RawDoFmt`'s + `PutChProc` callback and `Supervisor`'s routine are both executed + one instruction at a time through a separate, non-cycle-counting + path, so guest code reached that way is just as invisible to + `--clock-mhz` as a native library handler is. + - **A `System()`/`Execute()`-nested program gets its own, separate + cycle counter, starting back at zero.** A parent process's + `ReadEClock` calls bracketing a `System()` call will see that + child's entire run as contributing (close to) zero elapsed time, + since the child's cycles accumulate into its own nested runtime + instance, never the parent's. This is deliberate (nesting a + shared counter through a separately-constructed `Runtime` would + be significant extra plumbing for a benchmarking edge case), but + it means a benchmark that shells out mid-measurement will + under-report. + ## `--sanitize` Turns on shadow-memory checking of every guest memory access, to catch