binji / binjgb

Gameboy emulator implemented in C, that also runs in the browser
https://binji.github.io/binjgb/
MIT License
534 stars 61 forks source link

Missing Z flag after add operation #20

Closed gitendo closed 5 years ago

gitendo commented 5 years ago

I think I spotted a bug. :) Please take a look at A value and flags after add a,a at 0x01f7:

A:80 F:---- BC:0000 DE:c205 HL:0260 SP:fffc PC:01ec (cy: 548) ppu:+3 |[00]0x01ec: 18 09     jr +9          
A:80 F:---- BC:0000 DE:c205 HL:0260 SP:fffc PC:01f7 (cy: 560) ppu:+3 |[00]0x01f7: 87        add a,a        
A:00 F:---C BC:0000 DE:c205 HL:0260 SP:fffc PC:01f8 (cy: 564) ppu:+3 |[00]0x01f8: 38 08     jr c,+8        
A:00 F:---C BC:0000 DE:c205 HL:0260 SP:fffc PC:0202 (cy: 576) ppu:+3 |[00]0x0202: 28 ea     jr z,-22       
A:00 F:---C BC:0000 DE:c205 HL:0260 SP:fffc PC:0204 (cy: 584) ppu:+3 |[00]0x0204: 01 02 00  ld bc,2        

Carry is set but Zero is missing so instead making conditional jump at 0x0202 it goes straight to 0x0204 which is wrong. I checked it in BGB before opening an issue and it sets Z--C so the result is as expected. Above output is from binjgb-tester.exe win64. This is really handy util BTW!

binji commented 5 years ago

This is really interesting! I looked at the code, and it seems like it should do the right thing... this seems like it would be a pretty common operation, so I found a game that does it and traced it on my system:

A:80 F:Z-HC BC:A016 DE:0080 HL:c286 SP:fff6 PC:4bee (cy: 8632816) ppu:+0 |[01]0x4bee: 87        add a,a        
A:00 F:Z--C BC:A016 DE:0080 HL:c286 SP:fff6 PC:4bef (cy: 8632820) ppu:+0 |[01]0x4bef: 32        ld [hl-],a     

It produces the expected result, Z--C. So I'm not sure how you got the wrong result there. What rom did you use to repro this error, and what version of the emulator?

I just pushed v0.1.4, try that and see if it has the same issue (you need to use binjgb-tester-debug now if you want tracing).

gitendo commented 5 years ago

I've attached the rom in question. It's a simple test using RNC mode 2. Basically just a call to unpack function and dead loop to count cycles. I've just tried v0.1.4 but it also never returns from call - reason seems to be the same. Thanks for looking into this!

binji commented 5 years ago

The cycles and instruction addresses don't line up for me, but I still get Z--C in the equivalent case:

A:80 F:---- BC:0000 DE:c219 HL:0259 SP:fffc PC:01e8 (cy: 1432) ppu:+2 |[00]0x01e8: 18 09     jr +9          
A:80 F:---- BC:0000 DE:c219 HL:0259 SP:fffc PC:01f3 (cy: 1444) ppu:+2 |[00]0x01f3: 87        add a,a        
A:00 F:Z--C BC:0000 DE:c219 HL:0259 SP:fffc PC:01f4 (cy: 1448) ppu:+2 |[00]0x01f4: 38 08     jr c,+8        
A:00 F:Z--C BC:0000 DE:c219 HL:0259 SP:fffc PC:01fe (cy: 1460) ppu:+3 |[00]0x01fe: 28 ea     jr z,-22       
A:00 F:Z--C BC:0000 DE:c219 HL:0259 SP:fffc PC:01ea (cy: 1472) ppu:+3 |[00]0x01ea: 2a        ld a,[hl+]     

I also see the loop end after ~300k cycles:

A:02 F:ZN-C BC:0000 DE:d1a0 HL:0e9b SP:fffc PC:023b (cy: 303240) ppu:+0 |[00]0x023b: 87        add a,a        
A:04 F:---- BC:0000 DE:d1a0 HL:0e9b SP:fffc PC:023c (cy: 303244) ppu:+2 |[00]0x023c: 20 02     jr nz,+2       
A:04 F:---- BC:0000 DE:d1a0 HL:0e9b SP:fffc PC:0240 (cy: 303256) ppu:+2 |[00]0x0240: 38 b1     jr c,-79       
A:04 F:---- BC:0000 DE:d1a0 HL:0e9b SP:fffc PC:0242 (cy: 303264) ppu:+2 |[00]0x0242: c9        ret            
A:04 F:---- BC:0000 DE:d1a0 HL:0e9b SP:fffe PC:015a (cy: 303280) ppu:+2 |[00]0x015a: 76        halt 

Now I'm wondering if this is specific to the win64 version... I'll check.

gitendo commented 5 years ago

Hm, that's odd. I get exactly the same results from 32 and 64 bit versions. It doesn't set the flag and is stuck in infinite loop repeating the same part of code. I've some thin client here which has Win XP & 32 bit cpu, so I'll try to see if it makes any difference.

Edit: Looks like TC is too old: binjgb-tester.exe is not a valid Win32 application but linux version works flawlessly, too bad I haven't tried it earlier. :) Still, it's quite strange why Windows ver. acts that way.

binji commented 5 years ago

Yep, just found the same thing as you in the windows version:

A:80 F:---- BC:0000 DE:c219 HL:0259 SP:fffc PC:01e8 (cy: 1432) ppu:+2 |[00]0x01e8: 18 09     jr +9          
A:80 F:---- BC:0000 DE:c219 HL:0259 SP:fffc PC:01f3 (cy: 1444) ppu:+2 |[00]0x01f3: 87        add a,a        
A:00 F:---C BC:0000 DE:c219 HL:0259 SP:fffc PC:01f4 (cy: 1448) ppu:+2 |[00]0x01f4: 38 08     jr c,+8        
A:00 F:---C BC:0000 DE:c219 HL:0259 SP:fffc PC:01fe (cy: 1460) ppu:+3 |[00]0x01fe: 28 ea     jr z,-22       
A:00 F:---C BC:0000 DE:c219 HL:0259 SP:fffc PC:0200 (cy: 1468) ppu:+3 |[00]0x0200: 01 02 00  ld bc,2        

I'll keep looking.

binji commented 5 years ago

I tried building it myself with VS 2019, and it worked. Maybe this is a VS2015 bug? I ran the same test with ubsan on linux and didn't find any issues.

gitendo commented 5 years ago

Nice! Meanwhile I've finished those tests with linux build but I'll be using it in near future for sure, so working windows version is more than welcomed. :) Thanks!