search

RIP-Comm / clementine

Gameboy Advance emulator.
MIT License
49 stars 13 forks source link

ARM: Add check before advancing `PC` #84

Closed AlessioC31 closed 1 year ago

AlessioC31 commented 2 years ago

After executing an instruction that modifies PC, it should not be advanced by 4.

I've added that functions that implement instructions should return a bool representing whether PC should be increased after its execution.

I don't know if this is the best solution, maybe we could use an Option<u32> containing the new PC (returned from the instruction function) if Some or execute advance_program_counter if None. We can discuss it here :)

Tests are broken since they were written taking into account the fact that PC is always increased, when we decide which way to take I can fix the tests

guerinoni commented 1 year ago

It's a little bit unclear why you are returning a bool, I prefer to return how much should the PC advance, but it's not a big deal...

AlessioC31 commented 1 year ago

It's a little bit unclear why you are returning a bool, I prefer to return how much should the PC advance, but it's not a big deal...

I see what you mean. So maybe we could do as I wrote in the first message? Instead of bool functions return an Option<u32> with the new PC or None if the instruction doesn't move the PC so it needs to be increased by 4?

I'm not sure about returning the amount we should advance the PC since if we do something like R15 = R0 + R1 it is an absolute move, so we would first calculate the difference between the previous PC and the new one. So we would treat every move as a relative one, it would work but it would be confusing in my opinion.

AlessioC31 commented 1 year ago

After a discussion on discord, we decided to keep this PR as it is now and if the bool turns out to be confusing we change it to something else.