Skip to content

Commit

Permalink
sapemu: Fix a few subtle addressing bugs
Browse files Browse the repository at this point in the history 
All pointer reads (for indirect addressing modes) wrap around at page boundaries.

79/256
  • Loading branch information
@kleinesfilmroellchen
kleinesfilmroellchen committed Sep 8, 2024
1 parent 63feb2c commit 39070fe
Show file tree
Hide file tree
Showing 2 changed files with 54 additions and 5 deletions.
1 change: 1 addition & 0 deletions sapemu/snes-tests
Submodule snes-tests added at 962c87
58 changes: 53 additions & 5 deletions sapemu/src/smp/ops.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1280,7 +1280,7 @@ fn movw_ya_dp(cpu: &mut Smp, memory: &mut Memory, cycle: usize, state: Instructi
},
3 => {
// wraparound at direct page boundary
let y_address = (state.address + 1) % 0xff;
let y_address = increment_wrap_within_page(state.address);
MicroArchAction::Continue(state.with_address(y_address))
},
4 => {
Expand Down Expand Up @@ -1509,7 +1509,7 @@ fn movw_dp_ya(cpu: &mut Smp, memory: &mut Memory, cycle: usize, state: Instructi
},
3 => {
// wraparound at direct page boundary
let y_address = (state.address + 1) % 0xff;
let y_address = increment_wrap_within_page(state.address);
MicroArchAction::Continue(state.with_address(y_address))
},
4 => {
Expand Down Expand Up @@ -1681,6 +1681,13 @@ fn stop(cpu: &mut Smp, memory: &mut Memory, cycle: usize, state: InstructionInte
}
}

/// Wrap an address increment within a 256-byte page.
#[inline]
fn increment_wrap_within_page(address: u16) -> u16 {
let low_byte_increment = (address & 0xff).wrapping_add(1) & 0xff;
address & 0xff00 | low_byte_increment
}

Check failure on line 1689 in sapemu/src/smp/ops.rs

View workflow job for this annotation

GitHub Actions / clippy

this could be a `const fn` 

error: this could be a `const fn`
    --> sapemu/src/smp/ops.rs:1686:1
     |
1686 | / fn increment_wrap_within_page(address: u16) -> u16 {
1687 | |     let low_byte_increment = (address & 0xff).wrapping_add(1) & 0xff;
1688 | |     address & 0xff00 | low_byte_increment
1689 | | }
     | |_^
     |
     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#missing_const_for_fn
     = note: `-D clippy::missing-const-for-fn` implied by `-D clippy::nursery`
     = help: to override `-D clippy::nursery` add `#[allow(clippy::missing_const_for_fn)]`
help: make the function `const`
     |
1686 | const fn increment_wrap_within_page(address: u16) -> u16 {
     | +++++

/// Common implementation for all conditional branches. The flag to branch on and its state when to branch are constant
/// parameters since they are determined at compile time, hopefully improving optimizations around flag checks.
#[inline]
Expand Down Expand Up @@ -1869,7 +1876,7 @@ where
MicroArchAction::Continue(state.with_address(u16::from(lower_target_address)))
},
6 => {
let upper_target_address = u16::from(cpu.read(call_address + 1, memory));
let upper_target_address = u16::from(cpu.read(increment_wrap_within_page(call_address), memory));
let target_address = state.address | upper_target_address << 8;
trace!("tcall {INDEX} got call target {target_address:04x} (from {call_address:04x})");
MicroArchAction::Continue(state.with_address(target_address))
Expand Down Expand Up @@ -2031,9 +2038,12 @@ fn logic_op_a_x_indirect(
match cycle {
0 => MicroArchAction::Continue(InstructionInternalState::default()),
// TODO: not sure what the CPU does in this cycle.
1 => MicroArchAction::Continue(state),
1 => {
let address = cpu.x as u16 + cpu.direct_page_offset();
MicroArchAction::Continue(state.with_address(address))
},
2 => {
let value = cpu.read(cpu.x as u16, memory);
let value = cpu.read(state.address, memory);
let result = op(cpu.a, value);
cpu.a = result;
cpu.set_negative_zero(result);
Expand All @@ -2050,6 +2060,44 @@ fn logic_op_a_dp_x_indirect(
cycle: usize,
state: InstructionInternalState,
op: impl Fn(u8, u8) -> u8,
) -> MicroArchAction {
match cycle {
0 => MicroArchAction::Continue(InstructionInternalState::default()),
1 => {
let address = cpu.read_next_pc(memory) as u16;
MicroArchAction::Continue(state.with_address(address))
},
2 => {
let pointer_address = ((state.address + cpu.x as u16) & 0xff) + cpu.direct_page_offset();
MicroArchAction::Continue(state.with_address(pointer_address))
},
3 => {
let address_low = cpu.read(state.address, memory);
MicroArchAction::Continue(state.with_address2(address_low as u16))
},
4 => {
let address_high = cpu.read(increment_wrap_within_page(state.address), memory) as u16;
let address = address_high << 8 | state.address2;
MicroArchAction::Continue(state.with_address2(address as u16))

Check failure on line 2081 in sapemu/src/smp/ops.rs

View workflow job for this annotation

GitHub Actions / clippy

casting to the same type is unnecessary (`u16` -> `u16`) 

error: casting to the same type is unnecessary (`u16` -> `u16`)
    --> sapemu/src/smp/ops.rs:2081:50
     |
2081 |             MicroArchAction::Continue(state.with_address2(address as u16))
     |                                                           ^^^^^^^^^^^^^^ help: try: `address`
     |
     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_cast
     = note: `-D clippy::unnecessary-cast` implied by `-D clippy::all`
     = help: to override `-D clippy::all` add `#[allow(clippy::unnecessary_cast)]`
},
5 => {
let value = cpu.read(state.address2, memory);
let result = op(cpu.a, value);
cpu.a = result;
cpu.set_negative_zero(result);
MicroArchAction::Next
},
_ => unreachable!(),
}
}

#[inline]
fn logic_op_a_dp_x(
cpu: &mut Smp,
memory: &Memory,
cycle: usize,
state: InstructionInternalState,
op: impl Fn(u8, u8) -> u8,
) -> MicroArchAction {
match cycle {
0 => MicroArchAction::Continue(InstructionInternalState::default()),
Expand Down

0 comments on commit 39070fe

Please sign in to comment.