From 75fb98bc5506757abef408f58586ca219e134c59 Mon Sep 17 00:00:00 2001 From: 0xrsydn Date: Mon, 5 Oct 2026 20:00:16 +0700 Subject: [PATCH] fix(cpu): restore saved status for test and compare Rd=15 forms Restore CPSR from SPSR for ARM TST/TEQ/CMP/CMN with unused Rd=15 when the current mode has an SPSR. Preserve sequential PC and nominal timing; use normal ALU flags in User/System. Keep ordinary result-writing PC return validation unchanged. Reproduced independent jsmolka/gba-tests ARM failure 234 before this fix. Original regressions cover flags, masks, bank restoration, operand forms, conditions, timing, and atomic errors. The public ROM progresses to a separate unsupported load/writeback alias at test 360; no full public-suite pass is claimed. --- crates/gba-core/src/cpu.rs | 2 + crates/gba-core/src/cpu/arm.rs | 5 +- crates/gba-core/src/cpu/compare_psr_tests.rs | 145 +++++++++++++++++++ docs/hardware/cpu.md | 10 +- docs/references.md | 6 +- 5 files changed, 165 insertions(+), 3 deletions(-) create mode 100644 crates/gba-core/src/cpu/compare_psr_tests.rs diff --git a/crates/gba-core/src/cpu.rs b/crates/gba-core/src/cpu.rs index 1c0a9be..9e7192f 100644 --- a/crates/gba-core/src/cpu.rs +++ b/crates/gba-core/src/cpu.rs @@ -13,6 +13,8 @@ pub use exception::Exception; pub use status::Mode; mod transfer; +#[cfg(test)] +mod compare_psr_tests; #[cfg(test)] mod exception_tests; #[cfg(test)] diff --git a/crates/gba-core/src/cpu/arm.rs b/crates/gba-core/src/cpu/arm.rs index 579b4c9..07c5c5c 100644 --- a/crates/gba-core/src/cpu/arm.rs +++ b/crates/gba-core/src/cpu/arm.rs @@ -69,7 +69,10 @@ impl Cpu { if test && !set_flags { return Err(self.unsupported(instruction)); } - let restore = if !test && destination == 15 && set_flags { + // Test/compare encodings with unused Rd=15 restore SPSR when present, + // but do not write PC or refill the pipeline. Without an SPSR they use + // ordinary ALU flags. Result-writing PC returns still require an SPSR. + let restore = if destination == 15 && set_flags && (!test || self.spsr().is_some()) { Some(self.return_status(instruction)?) } else { None diff --git a/crates/gba-core/src/cpu/compare_psr_tests.rs b/crates/gba-core/src/cpu/compare_psr_tests.rs new file mode 100644 index 0000000..2128487 --- /dev/null +++ b/crates/gba-core/src/cpu/compare_psr_tests.rs @@ -0,0 +1,145 @@ +//! Original regressions for ARM test/compare encodings with the unused Rd=15. +use super::*; +use crate::{ + memory::ROM_START, + timing::{bus_cycles, AccessKind, AccessWidth}, +}; + +const EXCEPTIONS: [Mode; 5] = [ + Mode::Fiq, + Mode::Irq, + Mode::Supervisor, + Mode::Abort, + Mode::Undefined, +]; +fn instruction(opcode: u32, operand: u32) -> u32 { + 0xe010_f000 | (opcode << 21) | operand // AL, S=1, Rn=r0, unused Rd=15 +} +fn memory(word: u32) -> Memory { + Memory::new(word.to_le_bytes().to_vec()).unwrap() +} + +#[test] +fn compare_psr_restores_saved_flags_masks_and_banks_without_branching() { + for opcode in 8..=11 { + for source in EXCEPTIONS { + for flags in 0..16 { + for masks in [0, 0x40, 0x80, 0xc0] { + let mut cpu = Cpu::new(ROM_START); + cpu.registers[8] = 0x55; + cpu.registers[13] = 0x0300_1000; + cpu.switch_mode(source); + cpu.registers[13] = 0x0300_7000; + if source == Mode::Fiq { + cpu.registers[8] = 0xaa; + } + cpu.registers[0] = 0x8000_0000; + cpu.registers[1] = 1; + let saved = flags << 28 | masks | Mode::System as u32; + cpu.set_spsr(saved); + cpu.step(&mut memory(instruction(opcode, 1))).unwrap(); + assert_eq!(cpu.cpsr(), saved, "opcode={opcode}, source={source:?}"); + assert_eq!(cpu.pc(), ROM_START + 4); + assert_eq!(cpu.registers[0], 0x8000_0000); + assert_eq!(cpu.registers[1], 1); + assert_eq!(cpu.registers[8], 0x55); + assert_eq!(cpu.registers[13], 0x0300_1000); + cpu.switch_mode(source); + assert_eq!(cpu.spsr(), Some(saved)); + assert_eq!(cpu.registers[13], 0x0300_7000); + if source == Mode::Fiq { + assert_eq!(cpu.registers[8], 0xaa); + } + } + } + } + } +} + +#[test] +fn compare_psr_in_user_and_system_modes_uses_ordinary_alu_flags() { + for mode in [Mode::User, Mode::System] { + for opcode in 8..=11 { + for operand in [1, 0x0200_0081, 0x0000_0211] { + // Rm, immediate, register shift + let mut cpu = Cpu::new(ROM_START); + cpu.apply_status(0x9000_00c0 | mode as u32, mode); + cpu.registers[0] = 0x8000_0000; + cpu.registers[1] = 7; + cpu.registers[2] = 3; + let mut ordinary = cpu.clone(); + let word = instruction(opcode, operand); + cpu.step(&mut memory(word)).unwrap(); + ordinary.step(&mut memory(word & !0xf000)).unwrap(); + assert_eq!(cpu, ordinary); + assert_eq!(cpu.mode(), mode); + assert_eq!(cpu.spsr(), None); + assert_eq!(cpu.pc(), ROM_START + 4); + } + } + } +} + +#[test] +fn compare_psr_has_sequential_timing_for_each_operand_form() { + for opcode in 8..=11 { + for (operand, internal) in [(1, 0), (0x0200_0001, 0), (0x0000_0211, 1)] { + let mut cpu = Cpu::new(ROM_START); + cpu.switch_mode(Mode::Irq); + cpu.set_spsr(Mode::System as u32); + let timing = cpu + .step_timed(&mut memory(instruction(opcode, operand))) + .unwrap(); + assert_eq!( + timing.code_cycles, + bus_cycles(0, ROM_START, AccessWidth::Word, AccessKind::Sequential) + ); + assert_eq!(timing.internal_cycles, internal); + assert_eq!(timing.data_cycles, 0); + assert_eq!(cpu.pc(), ROM_START + 4); + } + } +} + +#[test] +fn compare_psr_invalid_saved_mode_and_bad_shift_preserve_all_cpu_state() { + for opcode in 8..=11 { + for source in EXCEPTIONS { + for (saved, operand) in [(0, 1), (0x1f, 0x0f11)] { + // invalid SPSR; forbidden Rs=pc + let mut cpu = Cpu::new(ROM_START); + cpu.switch_mode(source); + cpu.set_spsr(saved); + let before = cpu.clone(); + assert!(cpu + .step_timed(&mut memory(instruction(opcode, operand))) + .is_err()); + assert_eq!(cpu, before); + } + } + } +} + +#[test] +fn failed_conditions_do_not_restore_status_or_validate_spsr() { + for opcode in 8..=11 { + let mut cpu = Cpu::new(ROM_START); + cpu.switch_mode(Mode::Fiq); // Z clear; SPSR mode invalid + let mut expected = cpu.clone(); + expected.registers[15] += 4; + cpu.step(&mut memory(instruction(opcode, 1) & 0x0fff_ffff)) + .unwrap(); // EQ + assert_eq!(cpu, expected); + } +} + +#[test] +fn ordinary_pc_result_returns_still_require_an_spsr() { + for mode in [Mode::User, Mode::System] { + let mut cpu = Cpu::new(ROM_START); + cpu.switch_mode(mode); + let before = cpu.clone(); + assert!(cpu.step(&mut memory(0xe1b0_f00e)).is_err()); // MOVS pc,lr + assert_eq!(cpu, before); + } +} diff --git a/docs/hardware/cpu.md b/docs/hardware/cpu.md index b6b14d0..4f36e0b 100644 --- a/docs/hardware/cpu.md +++ b/docs/hardware/cpu.md @@ -70,9 +70,17 @@ Entry selects ARM state, masks interrupt requests (`IRQ`), and branches to the e FIQ entry also masks FIQ. Ordinary IRQ entry preserves the FIQ mask. `MOVS pc, lr`, `SUBS pc, lr, #offset`, and `LDM` with S and PC restore CPSR from SPSR. Return alignment follows the saved instruction state, not target bit zero. -User/System status returns fail because those modes have no SPSR. +Result-writing PC status returns fail in User/System modes because those modes have no SPSR. An SPSR can contain invalid mode bits, but a return using those bits fails without changing CPU state. +ARM `TST`, `TEQ`, `CMP`, and `CMN` encodings with unused destination bits set to R15 restore CPSR when an SPSR exists. +These compare/status forms do not write the arithmetic result to PC. Execution continues at the next sequential address. +In User/System modes, they perform ordinary test/compare flag updates without a mode change. +Invalid saved modes still produce an atomic diagnostic. Failed conditions do not restore or validate status. +Nominal timing remains one sequential code access, plus an internal cycle for register-specified shifts; there is no refill cost. +This fixes the mode-switch expectation in public `jsmolka/gba-tests` ARM test 234 and agrees with mGBA's shared ALU flag handling. +The interpreter has no fetch pipeline; instruction-state-changing forms and exact pipeline behavior remain hardware-unverified. + Other S-bit block transfers access User registers while using the current mode's base register. User-bank writeback, S-bit empty lists, and User-mode S-bit transfers return diagnostics. These restrictions avoid unpredictable or unverified forms. diff --git a/docs/references.md b/docs/references.md index 0569290..664dc93 100644 --- a/docs/references.md +++ b/docs/references.md @@ -60,7 +60,11 @@ Hardware references used for the core: - [GBATEK processor registers and modes](https://problemkaputt.de/gbatek-arm-cpu-register-set.htm). - [GBATEK status transfers](https://problemkaputt.de/gbatek-arm-opcodes-psr-transfer-mrs-msr.htm). - [GBATEK CPU exceptions](https://problemkaputt.de/gbatek-arm-cpu-exceptions.htm). -- [GBATEK ARM data processing](https://problemkaputt.de/gbatek-arm-opcodes-data-processing-alu.htm). +- [GBATEK ARM data processing](https://problemkaputt.de/gbatek-arm-opcodes-data-processing-alu.htm), including R15/status notes. Its legacy `{P}` description alone does not establish ARM7 behavior. +- [mGBA ARM instruction implementation](https://github.com/mgba-emu/mgba/blob/master/src/arm/isa-arm.c), reviewed for Rd=15 test/compare SPSR restoration, User/System fallback, and no result write/refill. +- [jsmolka/gba-tests](https://github.com/jsmolka/gba-tests/tree/a7113b67e63f83a9b321696ddd7042ccfad6c881), pinned for independent ARM testing. Reviewed its MIT license, startup, result macros, compare/status tests, and load/writeback alias tests. +- [armwrestler-gba-fixed](https://github.com/destoer/armwrestler-gba-fixed) and [arm7wrestler](https://github.com/Arisotura/arm7wrestler), considered as test sources; not integrated. +- [SingleStepTests ARM7TDMI](https://github.com/SingleStepTests/ARM7TDMI), considered as an experimental, emulator-generated test source; not integrated. - [GBATEK ARM single data transfers](https://problemkaputt.de/gbatek-arm-opcodes-memory-single-data-transfer-ldr-str-pld.htm). - [GBATEK memory alignment](https://problemkaputt.de/gbatek-arm-cpu-memory-alignments.htm). - [GBATEK multiply instructions](https://problemkaputt.de/gbatek-arm-opcodes-multiply-and-multiply-accumulate-mul-mla.htm).