massung / CHIP-8

Assembler and Emulator in Go
http://massung.github.io/CHIP-8
zlib License
254 stars 18 forks source link

Bugs in 8xy4, 8xy5, 8xy6, 8xy7, 8xyE and Fx1E #12

Open gulrak opened 1 month ago

gulrak commented 1 month ago

Hi, first of all I like that this is one of the few implementations that also has an assembler, and the UI is clean and nice!

I just found that the implementation suffers from some issues in the arithmetic opcodes:

8xy4: The way the overflow is detected breaks on use cases where x == y, so 8004 with v0 = 80 gives 0 and VF=0 which is wrong.

8xy5, 8xy6, 8xy7 and 8xyE all have the issue that VF is set first, breaking if VF is one of the operands, the typical solution is to set the flag result into a temp, set Vx to the result of the operation and set VF to that temp then. This also correctly handles the case that x is F as in CHIP-8 for arithmetic operations with VF as the result register the flag and not the operation result should be in VF.

This VF order issue is a quite common one as many documentations either lack the information or the description is kinda misleading. But as Octo uses this behavior with VF to implement additional pseudo SKIPs by using an operation on VF and skipping on the result to not overwrite other registers, any modern program that is using one of these skip variants is going to fail in this emulator. It is the behavior of the original CHIP-8/CHIP-48/SCHIP-1.x interpreters.

The excellent test suite from Timendus (https://github.com/Timendus/chip8-test-suite) has a flags test that can be used to check this.

Also the Fx1E handling is propagating a myth, while the discussion in Chromatophores HP-48 analysis repo talks about the possibility of an Amiga Interpreter that shows this behavior, none of the three currently known interpreters does this, so it is just a theory. The only game that might profit from it (the infamous Spacefight 2091 from that discussion) actually only "needs" VF to be reset and is broken even with this "fix", Chromatophore made a fixed version of the game fixing two bugs and not needing this. Also this behavior instead breaks other games that correctly do not expect VF to be erased by Fx1E, so it is doing more harm than good. If it would be configurable and off by default, okay, but this way it's more of a problem. (And yes, like in the scroll-up case, trapexit's documentation is incorrect in this regard.