Z80 second processor might have bugs in CPU emulation

[ZornsLemma]

I've been experimenting with the Z80 instruction exerciser at http://www.mdfsnet.f9.co.uk/Software/Z80/Exerciser/ I've attached a zipped Acorn CP/M .dsd image for use with b-em containing the two .com files from the CPM.zip file on that site. testcpm.dsd.zip

When I run zexdoc under b-em commit af26c27bb1284bf716e897e2ba7f8fd31694baa0 (latest on current master), I get some failures:

I haven't got the facilities to confirm this executable does actually work with no failures on real hardware. I suspect it's correct, but I don't know for a fact.

I haven't tried the more stringent zexall version, which tests the undocumented flags as well as the documented ones, although the b-em code in z80.c looks like it is aiming to support those as well.

I appreciate this could be a pretty tedious bug to fix for relatively little real benefit. I may be able to submit some patches to fix some of the issues myself later, but I don't want to promise anything.

[ZornsLemma]

There's a slightly different executable at http://mdfs.net/Software/Z80/Exerciser/ but it appears to behave exactly the same.

[SteveFosdick]

These things can be interesting. I don't have a real Z80 machine either but perhaps we can compare with one of the mainstream CP/M emulators. A couple spring to mind - YAZE and z80sim. I'll see if I can do some digging later.

[ZornsLemma]

Thanks. A few observations based on what I've looked at so far:

• http://www.z80.info/zip/z80-documented.pdf might be helpful

• b-em's z80.c seems to use 6502-style naming for the Z80 S and N flags, so the names are swapped from a Z80 perspective. That is, the Z80 S flag which occupies b7 of the flags register and indicates whether the sign bit of a result is set or not and which corresponds to the 6502 N flag is called N_FLAG in z80.c. The Z80 N flag which occupies b1 of the flags register and indicates whether the most recent operation was an addition or subtraction is called S_FLAG in z80.c. (You may well already know this, but I thought I'd point it out just in case.)

• opcodes 0x7 (rlca) and 0xf (rrca) should clear the Z80 H and N flags according to z80-documented.pdf (and Zaks' "Programming the Z80") but b-em's implementation only seems to modify the carry flag

[ZornsLemma]

The binaries I've included on the disc images above are pre-built ones, but there's source code as well, modified by JGH to build with the zmac assembler. I have been rebuilding this myself (in order to disable some tests to speed things up) using http://48k.ca/zmac.html, but I have had some trouble getting bit-for-bit identical binary output. The binaries I build myself seem to work, but this makes me a little nervous.

For what it's worth, at least using the version of zmac from 48k.ca, it seems to be necessary to specify "--zmac" on the command line to make the macro substitution of the individual test names work correctly. My assembly command line is:

zmac --zmac -z -o zexdoc-sf.cim -l zexdoc.src > zexdoc-sf.lst

[SteveFosdick]

That's very interesting about not getting the same executable at the end. I did have this previously with an assembler in a similar situation. I went looking for bug in the b-em disassembler because it wasn't showing the same instructions as I had written in the source but then when I looked at the listing file I found the assembler had an optimiser and had changed the instructions I had written to something faster/more compact.

But at the same time I am thinking trying the pre-built executable on YAZE might tell us something so I will do that.

[SteveFosdick]

Ok, the executable from the CPM.zip file from JGH's website runs fine on YAZE.

[SteveFosdick]

I did try looking at some individual instructions but was not getting far. I have come up with a plan which is to trace the execution of the offending tests. B-Em can already do that, though I have added flag decoding to make it easier to see what is going on (in https://github.com/stardot/b-em/tree/sf/z80fix) and I have also patched the Z80 disassembler from b-em into YAZE and added similar tracing there so as to be able to compare the output.

This was how I found the B-Em 6502 overflow flag bug when I first started - I compared a trace of the BCPL ROM running in B-Em vs. BeebEm.

But at the moment it generates too much output so the next stage is going to be looking into the source to cut down the tests run to just the failing ones.

[ZornsLemma]

That sounds like a good idea - a bit of work up front but once it's done it should hopefully be relatively easy to find the discrepancies.

It's nothing you probably wouldn't find yourself via Google, but this page may help with looking at the source: http://www.jeffavery.ca/computers/macintosh_z80exerciser.html (The "macintosh" in the URL is a bit misleading, don't let it put you off)

[SteveFosdick]

I have fixed at least one of the errors - see https://github.com/stardot/b-em/commit/c3b2ec58ffd5f7893e9e1349482536bf18f98794 For some reason the actual logical operation for AND, OR and XOR was not being done but only for the IX+nn case. The IY+nn one was fine. Here's the latest run of the full suite.

B>ZEXDOC

Z80 instruction exerciser
<adc,sbc> hl,<bc,de,hl,sp>....  OK
add hl,<bc,de,hl,sp>..........  OK
add ix,<bc,de,ix,sp>..........  OK
add iy,<bc,de,iy,sp>..........  OK
aluop a,nn....................  OK
aluop a,<b,c,d,e,h,l,(hl),a>..  OK
aluop a,<ixh,ixl,iyh,iyl>.....  OK
aluop a,(<ix,iy>+1)...........  OK
bit n,(<ix,iy>+1).............  OK
bit n,<b,c,d,e,h,l,(hl),a>....  OK
cpd<r>........................  ERROR **** crc expected:a87e6cfa found:71568dde
cpi<r>........................  ERROR **** crc expected:06deb356 found:deb515a6
<daa,cpl,scf,ccf>.............  ERROR **** crc expected:9b4ba675 found:d6fc44de
<inc,dec> a...................  ERROR **** crc expected:d18815a4 found:80725c0f
<inc,dec> b...................  ERROR **** crc expected:5f682264 found:4b5817a3
<inc,dec> bc..................  OK
<inc,dec> c...................  ERROR **** crc expected:c284554c found:937e1ce7
<inc,dec> d...................  ERROR **** crc expected:4523de10 found:5113ebd7
<inc,dec> de..................  OK
<inc,dec> e...................  ERROR **** crc expected:e175afcc found:f5459a0b
<inc,dec> h...................  ERROR **** crc expected:1ced847d found:08ddb1ba
<inc,dec> hl..................  OK
<inc,dec> ix..................  OK
<inc,dec> iy..................  OK
<inc,dec> l...................  ERROR **** crc expected:56cd06f3 found:07374f58
<inc,dec> (hl)................  ERROR **** crc expected:b83adcef found:e9c09544
<inc,dec> sp..................  OK
<inc,dec> (<ix,iy>+1).........  ERROR **** crc expected:20581470 found:c7027d2a
<inc,dec> ixh.................  ERROR **** crc expected:6f463662 found:3ebc7fc9
<inc,dec> ixl.................  ERROR **** crc expected:027bef2c found:164bdaeb
<inc,dec> iyh.................  ERROR **** crc expected:2d966cf3 found:39a65934
<inc,dec> iyl.................  ERROR **** crc expected:fbcbba95 found:aa31f33e
ld <bc,de>,(nnnn).............  OK
ld hl,(nnnn)..................  OK
ld sp,(nnnn)..................  OK
ld <ix,iy>,(nnnn).............  OK
ld (nnnn),<bc,de>.............  OK
ld (nnnn),hl..................  OK
ld (nnnn),sp..................  OK
ld (nnnn),<ix,iy>.............  OK
ld <bc,de,hl,sp>,nnnn.........  OK
ld <ix,iy>,nnnn...............  OK
ld a,<(bc),(de)>..............  OK
ld <b,c,d,e,h,l,(hl),a>,nn....  OK
ld (<ix,iy>+1),nn.............  OK
ld <b,c,d,e>,(<ix,iy>+1)......  OK
ld <h,l>,(<ix,iy>+1)..........  OK
ld a,(<ix,iy>+1)..............  OK
ld <ixh,ixl,iyh,iyl>,nn.......  OK
ld <bcdehla>,<bcdehla>........  OK
ld <bcdexya>,<bcdexya>........  ERROR **** crc expected:478ba36b found:b483ae26
ld a,(nnnn) / ld (nnnn),a.....  OK
ldd<r> (1)....................  OK
ldd<r> (2)....................  OK
ldi<r> (1)....................  OK
ldi<r> (2)....................  OK
neg...........................  OK
<rrd,rld>.....................  ERROR **** crc expected:955ba326 found:86487439
<rlca,rrca,rla,rra>...........  ERROR **** crc expected:251330ae found:9ac609b5
shf/rot (<ix,iy>+1)...........  ERROR **** crc expected:713acd81 found:4cd11c4a
shf/rot <b,c,d,e,h,l,(hl),a>..  OK
<set,res> n,<bcdehl(hl)a>.....  OK
<set,res> n,(<ix,iy>+1).......  OK
ld (<ix,iy>+1),<b,c,d,e>......  OK
ld (<ix,iy>+1),<h,l>..........  OK
ld (<ix,iy>+1),a..............  OK
ld (<bc,de>),a................  OK
Tests complete
B>STAR SPOOL

[ZornsLemma]

Nice work, it's amazing how much has been able to work even with such nominally glaring errors.

What I find particularly odd is that the "<inc,dec> b" test fails - I haven't compared that code with other emulators, but I have compared it with my own emulator and the specs and it looks absolutely fine. Obviously it isn't, but it feels like it should be.

[SteveFosdick]

The YAZE version of the flag setting for 8bit INC/DEC looks strange, but passes:

    case 0x04:          /* INC B */
        BC += 0x100;
        temp = hreg(BC);
        AF = (AF & ~0xfe) | (temp & 0xa8) |
            (((temp & 0xff) == 0) << 6) |
            (((temp & 0xf) == 0) << 4) |
            ((temp == 0x80) << 2);
        break;
    case 0x05:          /* DEC B */
        BC -= 0x100;
        temp = hreg(BC);
        AF = (AF & ~0xfe) | (temp & 0xa8) |
            (((temp & 0xff) == 0) << 6) |
            (((temp & 0xf) == 0xf) << 4) |
            ((temp == 0x7f) << 2) | 2;
        break;

compare with B-Em's

static inline void setinc(uint8_t v)
{
    af.b.l &= ~(N_FLAG | Z_FLAG | V_FLAG | 0x28 | H_FLAG);
    af.b.l |= znptable[(v + 1) & 0xFF];
    if (v == 0x7F)
        af.b.l |= V_FLAG;
    else
        af.b.l &= ~V_FLAG;
    if (((v & 0xF) + 1) & 0x10)
        af.b.l |= H_FLAG;
}

static inline void setdec(uint8_t v)
{
    af.b.l &= ~(N_FLAG | Z_FLAG | V_FLAG | 0x28 | H_FLAG);
    af.b.l |= znptable[(v - 1) & 0xFF] | S_FLAG;
    if (v == 0x80)
        af.b.l |= V_FLAG;
    else
        af.b.l &= ~V_FLAG;
    if (!(v & 8) && ((v - 1) & 8))
        af.b.l |= H_FLAG;
}

[SteveFosdick]

So commenting the YAZE version:

    case 0x04:          /* INC B */
        BC += 0x100;
        temp = hreg(BC);    /* just B */
        AF = (AF & ~0xfe) | (temp & 0xa8) | /* copy S,Y,X from result */
            (((temp & 0xff) == 0) << 6) |   /* Z flag */
            (((temp & 0xf) == 0) << 4) |    /* H flag */
            ((temp == 0x80) << 2);          /* V flag */
        break;
    case 0x05:          /* DEC B */
        BC -= 0x100;
        temp = hreg(BC);    /* just B */
        AF = (AF & ~0xfe) | (temp & 0xa8) | /* copy S,Y,X from result */
            (((temp & 0xff) == 0) << 6) |   /* Z flag */
            (((temp & 0xf) == 0xf) << 4) |  /* H flag */
            ((temp == 0x7f) << 2) | 2;      /* V flag */
        break;

[SteveFosdick]

So I think I can see one bug in the b-em version - znptable should be znptablenv, i.e. we don't want the parity in the V/P flag as we're using it for overflow (V). But just changing that doesn't seem enough to make it pass.

[ZornsLemma]

It's too late/early for me to comment on the rest of your investigation, but I think the znptable vs znptablenv distinction doesn't matter because setinc()/setdec() override the V flag value from znptable anyway. That's not to say using znptablenv wouldn't be better/clearer in the first place...

[SteveFosdick]

Continuing on INC/DEC, you're right. I had that thought after I went to bed. Comparing a run of the test suite is proving to generate too much data - even a single test is well over 16Gb.

So I used some shell to write a little program - for each register and for each operation (inc and dec) it sets the register to zero then has 257 of the operation in a row (no loops to affect flags), so as to start at zero and wrap back to zero. I traced the output from B-Em and YAZE, removed the SP as this different (CCP in a different place) and compared the files (with meld).

There were no differences, i.e. no difference in flags, or in the register than gets incremented/decremented or any of the other common registers. I don't think I was displaying IX and IY but there is no obvious reason why those should matter.

So we may need to look more carefully at the test.

[hoglet67]

(Terminology: I'll use the Z80 flag names, so bit 7 is the S flag and bit 1 is the N flag)

DEC looks OK to me, but I believe INC should clear the N flag (which indicates whether the last operation was a subtract). It does in YAZE by ANDing with ~0xFE but this doesn't happen in B-Em.

If you need a reference, PiTubeDirect uses YAZE which passes ZEXDOC and ZEXALL. It includes a debugger. Alternatively, there is Visual Z80. I also find this summary useful and accurate.

Dave

[hoglet67]

I'm going for the low hanging fruit here...

SCF also clears the N and H flags.

CCF also clears the N flag, and sets the H flag to the old value of the C flag.

(the N flag is S_FLAG in B-Em; would it make sense to rename these???)

[hoglet67]

The ld bcdexya,bcdexya test is failing because of missing (arguably undocumented) instructions.

It tests the following ranges:

• 0xDD 0x40 .. 0xDD 7F
• 0xFD 0x40 ... 0xFD 7F

It looks like B-Em only implements a select subset of these instructions. The others should be implemented as if the prefix were not present.

[hoglet67]

The following instructions need to clear the H and N flags:

• RRD
• RLD
• RLCA
• RRCA
• RLA
• RRA

[hoglet67]

The shf/rot (<ix,iy>+1) test is failing because of missing instructions:

• 0xDD 0xCB 0x01 0x36 - SLL (IX+nn)
• 0xFD 0xCB 0x01 0x26 - SLA (IY+nn)
• 0xFD 0xCB 0x01 0x36 - SLL (IY+nn)
• 0xFD 0xCB 0x01 0x3E - SRL (IY+nn)

There are also bugs in a couple that are present, which I'm looking at now, then I'll do a pull request for you.

[SteveFosdick]

Thanks, David. SO my simple test case for INC/DEC presumably didn't find the 'N' flag issue because it was already clear. I have got as far as CCF/SCF, just working through your comments. Fixes are in https://github.com/stardot/b-em/compare/sf/z80fix

[hoglet67]

Steve, some of the later fixes are non trivial. I have all of them stacked up ready to do a pull request, but this will get confusing if you have started fixing them as well. What would you like me to do?

[SteveFosdick]

Dave, you didn't say you fixing them too. Which branch have you based your changes on?

I'd got as far as the DD/FD prefix and I just goto back to the main opcode switch if the instruction is not an IX/IY one. I don't generally like goto but I wasn't going to duplicate much of the original switch and, according to the documentation, you can have any number of DD/FD prefixes and only the last one counts.

[hoglet67]

Sorry, I should have been clearer.

I started with: sf/z80fix

FYI, I've pushed what I have so far to db/z80fix

You should be able to cherry pick: 18dec85a db5898ec

Looks like you have already made some of the changes in a47d55c7

There are just two failing tests now.

The LD DD/FD prefix one, which I was definitely going to leave to you.

And the cpd/cpi one which I can look at now (CPD 0xED 0xA9 is missing)

[SteveFosdick]

David, I have merged your changes into by sf/z80fix branch and pushed it to GitHub. I'll leave you to do cpd/cpi one.

[SteveFosdick]

David, the cherry-picking didn't address RRD etc. Do you have them as uncommitted change (or unpushed).

[hoglet67]

I think that's down to the N and S flags being renamed.

With the new names, RRD etc should be clearing the N flag I think.

[hoglet67]

There is a fix for the CPI/CPD tests in branch db/z80fix2 d5489655

BTW, Are you planning to stop with the documented behaviour.

The undocumented X and Z flags adds another level of complexity, because they require the internal WZ register to be emulated. This is a real pain, for almost no benefit that I'm aware of.

[SteveFosdick]

Ok, just testing that now. Personally I am not that bothered, perhaps the question is better aimed at ZornsLemma who raised the bug. On the other hand it looks as if efforts have already been made. In the B-Em version what you call WZ is, I think, called intreg and some of the flag setting routines do already set X and Y.

[ZornsLemma]

Thanks to both of you for taking a look at this. I started looking at this on b-em because I'm tinkering with another emulator and as I had the b-em code handy I started using it as a pseudo-reference. I don't have any personal need for support for the undocumented Z80 flags, although as Steve says it looks as though b-em's code is trying to support these already.

[SteveFosdick]

Ok, just as a snapshot, then, here's the test result for ZEXALL. Meanwhile I have merged the fixes into master so that now passes the ZEXDOC test.

Z80 instruction exerciser
<adc,sbc> hl,<bc,de,hl,sp>....  OK
add hl,<bc,de,hl,sp>..........  OK
add ix,<bc,de,ix,sp>..........  OK
add iy,<bc,de,iy,sp>..........  OK
aluop a,nn....................  OK
aluop a,<b,c,d,e,h,l,(hl),a>..  OK
aluop a,<ixh,ixl,iyh,iyl>.....  OK
aluop a,(<ix,iy>+1)...........  OK
bit n,(<ix,iy>+1).............  OK
bit n,<b,c,d,e,h,l,(hl),a>....  ERROR **** crc expected:5e020e98 found:9edc2fa8
cpd<r>........................  ERROR **** crc expected:134b622d found:d0ecd9bb
cpi<r>........................  ERROR **** crc expected:2da42d19 found:f7644b3b
<daa,cpl,scf,ccf>.............  ERROR **** crc expected:6d2dd213 found:3c5b3b49
<inc,dec> a...................  OK
<inc,dec> b...................  OK
<inc,dec> bc..................  OK
<inc,dec> c...................  OK
<inc,dec> d...................  OK
<inc,dec> de..................  OK
<inc,dec> e...................  OK
<inc,dec> h...................  OK
<inc,dec> hl..................  OK
<inc,dec> ix..................  OK
<inc,dec> iy..................  OK
<inc,dec> l...................  OK
<inc,dec> (hl)................  OK
<inc,dec> sp..................  OK
<inc,dec> (<ix,iy>+1).........  OK
<inc,dec> ixh.................  OK
<inc,dec> ixl.................  OK
<inc,dec> iyh.................  OK
<inc,dec> iyl.................  OK
ld <bc,de>,(nnnn).............  OK
ld hl,(nnnn)..................  OK
ld sp,(nnnn)..................  OK
ld <ix,iy>,(nnnn).............  OK
ld (nnnn),<bc,de>.............  OK
ld (nnnn),hl..................  OK
ld (nnnn),sp..................  OK
ld (nnnn),<ix,iy>.............  OK
ld <bc,de,hl,sp>,nnnn.........  OK
ld <ix,iy>,nnnn...............  OK
ld a,<(bc),(de)>..............  OK
ld <b,c,d,e,h,l,(hl),a>,nn....  OK
ld (<ix,iy>+1),nn.............  OK
ld <b,c,d,e>,(<ix,iy>+1)......  OK
ld <h,l>,(<ix,iy>+1)..........  OK
ld a,(<ix,iy>+1)..............  OK
ld <ixh,ixl,iyh,iyl>,nn.......  OK
ld <bcdehla>,<bcdehla>........  OK
ld <bcdexya>,<bcdexya>........  OK
ld a,(nnnn) / ld (nnnn),a.....  OK
ldd<r> (1)....................  OK
ldd<r> (2)....................  ERROR **** crc expected:39dd3de1 found:5a907ed4
ldi<r> (1)....................  ERROR **** crc expected:f782b0d1 found:9abdf6b5
ldi<r> (2)....................  ERROR **** crc expected:e9ead0ae found:eb59891b
neg...........................  OK
<rrd,rld>.....................  OK
<rlca,rrca,rla,rra>...........  ERROR **** crc expected:9ba3807c found:251330ae
shf/rot (<ix,iy>+1)...........  OK
shf/rot <b,c,d,e,h,l,(hl),a>..  OK
<set,res> n,<bcdehl(hl)a>.....  OK
<set,res> n,(<ix,iy>+1).......  OK
ld (<ix,iy>+1),<b,c,d,e>......  OK
ld (<ix,iy>+1),<h,l>..........  OK
ld (<ix,iy>+1),a..............  OK
ld (<bc,de>),a................  OK
Tests complete

[hoglet67]

In the B-Em version what you call WZ is, I think, called intreg and some of the flag setting routines do already set X and Y.

IntReg is currently only 8-bits though, where as WZ is 16 bits.

This document describes how WZ is updated: https://gist.github.com/drhelius/8497817

The only way to read the WZ register is using BIT n,(HL), which sets the X/Y flag bits to bits 13/11 of WZ.

CPI and CPD do a 16-bit increment/decrement of WZ, so all 16 bits do need to be emulated.

[hoglet67]

Implementing X/Y correctly, apart from BIT n,(HL) might be a good milestone if you do want to go further.

(BTW, I'm think the correct name for this internal register is MEMPTR, not WZ. It's a couple of years since I added this stuff to the Z80 decoder)