Less incomplete Z80 emulator written in C++

Question

Follow-up to Partial Zilog Z80 emulator written in C++

I would say that I'm still new to the language, so I'm going to keep the beginner tag this time.

Changes:

I've implemented the suggestions from @1201ProgramAlarm and have implemented about ¼ of the instructions in the main set.

I have in general reduced code duplication. I don't think that at this point I can do anything further in that direction.

I've refrained from creating variables until they are needed, and added some extra checks.

I've moved most of the code from tools.cpp back to z80emu.hpp.

I've started implementing the flag changes, though some are still incomplete.

emulate.cpp:

#include <stdexcept>
#include "z80emu.hpp"
#include "opcodes.h"

#ifndef NDEBUG
# include <iostream>
using std::cout;
using std::endl;
#endif

namespace z80emu
{

// return value: number of instructions executed
uint16_t z80::emulate(size_t file_size)
{
    reg *rp[] =
    {
        &regs.bc,
        &regs.de,
        &regs.hl,
        &regs.sp
    };
/*
    reg *rp2[] =
    {
        &regs.bc,
        &regs.de,
        &regs.hl,
        &regs.af
    };
*/
    uint16_t inst = 0;
    uint8_t op;

    (void)file_size;

    for(;;)
    {
        switch((op = mem[regs.pc]))
        {
            case NOP:
                break;
            case LD_BC_IMM:
            case LD_DE_IMM:
            case LD_HL_IMM:
            case LD_SP_IMM:
                ld16imm(op >> 4, rp);
                break;
            case LD_DBC_A:
            case LD_DDE_A:
                deref16_u8(op >> 4, rp) = regs.af.geth();
                break;
            case INC_BC:
            case INC_DE:
            case INC_HL:
            case INC_SP:
            case DEC_BC:
            case DEC_DE:
            case DEC_HL:
            case DEC_SP:
                incdec16(op >> 4, op & 8, rp);
                break;
            case INC_B:
            case INC_C:
            case INC_D:
            case INC_E:
            case INC_H:
            case INC_L:
            case INC_DHL:
            case INC_A:
            case DEC_B:
            case DEC_C:
            case DEC_D:
            case DEC_E:
            case DEC_H:
            case DEC_L:
            case DEC_DHL:
            case DEC_A:
                incdec8(op >> 4, op & 8, op & 1, rp);
                break;
            case LD_B_IMM:
            case LD_C_IMM:
            case LD_D_IMM:
            case LD_E_IMM:
            case LD_H_IMM:
            case LD_L_IMM:
            case LD_DHL_IMM:
            case LD_A_IMM:
                ld8imm(op, rp);
                break;
            case RLCA:
            case RRCA:
            case RLA:
            case RRA:
                bitshifta(op);
                break;
            case EX_AF_AF:
                regs.af.exchange();
                break;
            case ADD_HL_BC:
            case ADD_HL_DE:
            case ADD_HL_HL:
            case ADD_HL_SP:
                {
                    uint8_t f = regs.af.getl();

                    rp[RP_HL]->add16(rp[op>>4]->get16());
                    f &= ~(1 << F_N);
                    /* TODO: set C on carry */
                }
                break;
            case LD_A_DBC:
            case LD_A_DDE:
                regs.af.seth(deref16_u8(op >> 4, rp));
                break;
            case DJNZ_IMM:
                {
                    uint8_t off = mem[++regs.pc];
                    uint8_t b_adj = regs.bc.geth() - 1;

                    regs.bc.seth(b_adj);
                    if(b_adj)
                        reljmp(off);
                }
                break;
            case JR_IMM:
                reljmp(mem[++regs.pc]);
                break;
            case JR_NZ_IMM:
            case JR_Z_IMM:
            case JR_NC_IMM:
            case JR_C_IMM:
                ccreljmp(mem[++regs.pc]);
                break;
            case DAA:
                {
                    uint8_t f = regs.af.getl(),
                            a = regs.af.geth();

                    if((a & 0x0f) > 0x09 || (f & (1 << F_H)))
                        a += 0x06;

                    if(a & 0x10 && !(regs.af.geth() & 0x10))
                        f |= 1 << F_H;

                    if((a & 0xf0) > 0x90 || (f & (1 << F_C)))
                    {
                        a += 0x60;
                        f |= 1 << F_C;
                    }

                    f |= parity(a) << F_PV;
                }
                break;
            case CPL:
                {
                    uint8_t f = regs.af.getl(),
                            a = regs.af.geth();

                    a = ~a;

                    f |= 1 << F_H;
                    f |= 1 << F_N;

                    regs.af.seth(a);
                    regs.af.setl(f);
                }
                break;
            default:
#ifndef NDEBUG
                cout << std::hex << std::showbase
                     << "af: " << regs.af.get16() << endl
                     << "af': " << regs.af.getexx() << endl
                     << "bc: " << regs.bc.get16() << endl
                     << "bc': " << regs.bc.getexx() << endl
                     << "de: " << regs.de.get16() << endl
                     << "de': " << regs.de.getexx() << endl
                     << "hl: " << regs.hl.get16() << endl
                     << "hl': " << regs.hl.getexx() << endl
                     << "sp: " << regs.sp.get16() << endl
                     << "a: " << +regs.af.geth() << endl
                     << "f: " << +regs.af.getl() << endl
                     << "b: " << +regs.bc.geth() << endl
                     << "c: " << +regs.bc.getl() << endl
                     << "d: " << +regs.de.geth() << endl
                     << "e: " << +regs.de.getl() << endl
                     << "h: " << +regs.hl.geth() << endl
                     << "l: " << +regs.hl.getl() << endl;
#endif
                throw std::logic_error("Unimplemented opcode!");
        }
        regs.pc++;
        inst++;
    }
} // z80::emulate

} // namespace z80emu

main.cpp:

#include <cerrno>
#include <limits>
#include <cstdlib>
#include <cstring>
#include <fstream>
#include <iostream>
#include <exception>
#include "z80emu.hpp"

void usage(const char *progname);

int main(int argc, char **argv)
{
    if((unsigned)argc - 2 > 0)
    {
         usage(argv[0]);
         return EXIT_FAILURE;
    }

    std::ifstream infile;

    infile.open(argv[1], std::ifstream::in | std::ifstream::binary);
    if(!infile.good())
    {
        std::cerr << "Opening " << argv[1] << " failed: "
                  << std::strerror(errno) << std::endl;
        return EXIT_FAILURE;
    }

    size_t file_size;

    file_size = infile.seekg(0, infile.end).tellg();
    infile.seekg(0, infile.beg);

    if(file_size > UINT16_MAX)
    {
        std::cerr << "Error: File too large." << std::endl;
        return EXIT_FAILURE;
    }

    z80emu::z80 z80;

    infile.read((char *)z80.mem, file_size);

    try
    {
        z80.emulate(file_size);
    }
    catch(std::exception &e)
    {
        std::cerr << "Emulation failed: " << e.what() << std::endl;
        return EXIT_FAILURE;
    }

    return 0;
}

void usage(const char *progname)
{
    std::cout << "  Usage: " << progname << " z80-prog" << std::endl;
}

opcodes.hpp:

#ifndef Z80EMU_OPCODES_HPP
#define Z80EMU_OPCODES_HPP 1

namespace z80emu
{

enum opcodes
{
    NOP     = 0x00,
    LD_BC_IMM   = 0x01,
    LD_DBC_A    = 0x02,
    INC_BC  = 0x03,
    INC_B   = 0x04,
    DEC_B   = 0x05,
    LD_B_IMM    = 0x06,
    RLCA    = 0x07,
    EX_AF_AF    = 0x08,
    ADD_HL_BC   = 0x09,
    LD_A_DBC    = 0x0a,
    DEC_BC  = 0x0b,
    INC_C   = 0x0c,
    DEC_C   = 0x0d,
    LD_C_IMM    = 0x0e,
    RRCA    = 0x0f,
    DJNZ_IMM    = 0x10,
    LD_DE_IMM   = 0x11,
    LD_DDE_A    = 0x12,
    INC_DE  = 0x13,
    INC_D   = 0x14,
    DEC_D   = 0x15,
    LD_D_IMM    = 0x16,
    RLA     = 0x17,
    JR_IMM  = 0x18,
    ADD_HL_DE   = 0x19,
    LD_A_DDE    = 0x1a,
    DEC_DE  = 0x1b,
    INC_E   = 0x1c,
    DEC_E   = 0x1d,
    LD_E_IMM    = 0x1e,
    RRA     = 0x1f,
    JR_NZ_IMM   = 0x20,
    LD_HL_IMM   = 0x21,
    LD_DIMM_HL  = 0x22,
    INC_HL  = 0x23,
    INC_H   = 0x24,
    DEC_H   = 0x25,
    LD_H_IMM    = 0x26,
    DAA     = 0x27,
    JR_Z_IMM    = 0x28,
    ADD_HL_HL   = 0x29,
    LD_HL_DIMM  = 0x2a,
    DEC_HL  = 0x2b,
    INC_L   = 0x2c,
    DEC_L   = 0x2d,
    LD_L_IMM    = 0x2e,
    CPL     = 0x2f,
    JR_NC_IMM   = 0x30,
    LD_SP_IMM   = 0x31,
    LD_DIMM_A   = 0x32,
    INC_SP  = 0x33,
    INC_DHL = 0x34,
    DEC_DHL = 0x35,
    LD_DHL_IMM  = 0x36,
    SCF     = 0x37,
    JR_C_IMM    = 0x38,
    ADD_HL_SP   = 0x39,
    LD_A_DIMM   = 0x3a,
    DEC_SP  = 0x3b,
    INC_A   = 0x3c,
    DEC_A   = 0x3d,
    LD_A_IMM    = 0x3e,
    CCF     = 0x3f
}; // enum opcodes

} // namespace z80emu

#endif

z80emu.hpp:

#ifndef Z80EMU_HPP
#define Z80EMU_HPP 1

#if __cplusplus >= 201103L
# include <cstdint>
# include <utility>
using std::uint16_t;
using std::uint8_t;
#else
# include <algorithm>
# include <stdint.h>
#endif

#include <cassert>
#include <cstring>
#include <vector>

namespace z80emu
{

enum cc
{
    CC_NZ = 0,
    CC_Z  = 1,
    CC_NC = 2,
    CC_C  = 3,
    CC_PO = 4,
    CC_PE = 5,
    CC_P  = 6,
    CC_M  = 7
};
enum flags
{
    F_C  = 0,
    F_N  = 1,
    F_PV = 2,
    F_F3 = 3,
    F_H  = 4,
    F_F5 = 5,
    F_Z  = 6,
    F_S  = 7
};
enum regpair
{
    RP_BC = 0,
    RP_DE = 1,
    RP_HL = 2,
    RP_SP = 3
};
enum bytemask
{
    HIGH_BYTE = 0xff00,
    LOW_BYTE  = 0x00ff
};
enum bitmask
{
    BIT0 = 0x01,
    BIT1 = 0x02,
    BIT2 = 0x04,
    BIT3 = 0x08,
    BIT4 = 0x10,
    BIT5 = 0x20,
    BIT6 = 0x40,
    BIT7 = 0x80,
    BIT0MASK = 0x00,
    BIT1MASK = 0x01,
    BIT2MASK = 0x03,
    BIT3MASK = 0x07,
    BIT4MASK = 0x0f,
    BIT5MASK = 0x1f,
    BIT6MASK = 0x3f,
    BIT7MASK = 0x7f,
    FULLMASK = 0xff
};

inline bool parity(uint16_t n)
{
    uint8_t ctr, bits = sizeof(n) << 3;
    for( ctr = 0; bits; ctr++ )
    {
        bits >>= 1;
        n = (n >> bits) ^ (n & ((1u << bits) - 1));
    }
    return n;
}

// calculate the two's complement of an 8-bit integer
template<typename T>
inline T twoscomp(T val)
{
    return ~val + 1;
}

struct reg
{
    inline uint16_t get16() const
    {
        return val;
    }
    // Allow to get shadow register for debugging purposes
    inline uint16_t getexx() const
    {
        return exx;
    }
    inline uint8_t get8(bool low) const
    {
        return low ? getl() : geth();
    }
    inline uint8_t geth() const
    {
        return val >> 8;
    }
    inline uint8_t getl() const
    {
        return val;
    }

    inline void set16(uint16_t v)
    {
        val = v;
    }
    inline void set8(bool low, uint8_t v)
    {
        if(low)
            setl(v);
        else
            seth(v);
    }
    inline void seth(uint8_t h)
    {
        val = (val & LOW_BYTE) | h << 8;
    }
    inline void setl(uint8_t l)
    {
        val = (val & HIGH_BYTE) | l;
    }

    inline void add16(uint16_t a)
    {
        val += a;
    }

    inline void exchange()
    {
        std::swap(val, exx);
    }

    reg()
    {
        val = exx = 0;
    }

private:
    uint16_t val, exx;
}; // struct reg

#if __cplusplus >= 201103L
static_assert(sizeof(reg) == 4, "sizeof(reg) != 4");
#endif

struct registers
{
    reg af;
    reg bc;
    reg de;
    reg hl;
    reg ix;
    reg iy;
    reg sp;
    reg wz;
    uint16_t pc;
    registers()
    {
        pc = 0;
    }
};

struct z80
{
    uint8_t *mem;
    registers regs;

    uint16_t emulate(size_t file_size);

    /* return reference to a byte in memory
       specified by a 16-bit pointer */
    inline uint8_t &deref16_u8(uint8_t idx, reg **tab)
    {
        return mem[tab[idx]->get16()];
    }

    // set 8-bit register or memory location
    inline void set8(uint8_t idx, uint8_t val, bool low, reg **tab)
    {
        /* idx is the index for the 16-bit register

           if low is true, return the low part of the register,
           otherwise return the high part */

        switch(idx & 3)
        {
            case 3:
                if(low)
                    regs.af.seth(val);
                else
                    mem[regs.hl.get16()] = val;
                break;
            default:
                tab[idx]->set8(low, val);
                break;
        }
    }

    // get 8-bit register or memory location
    inline uint8_t get8(uint8_t idx, bool low, reg **tab)
    {
        // relatively the same usage as above

        switch(idx & 3)
        {
            case 3:
                if(low)
                {
                    return regs.af.geth();
                }
                else
                {
                    return mem[regs.hl.get16()];
                }
            default:
                return tab[idx]->get8(low);
        }
    }

    // load 16-bit register with immediate
    inline void ld16imm(uint8_t idx, reg **tab)
    {
        /* Do these individually because
           of endianness and memory wrapping */
        tab[idx]->setl(mem[++regs.pc]);
        tab[idx]->seth(mem[++regs.pc]);
    }

    // load 8-bit register with immediate
    inline void ld8imm(uint8_t op, reg **tab)
    {
        set8(op >> 4, mem[++regs.pc], op & 8, tab);
    }

    // increment or decrement 16-bit register
    inline void incdec16(uint8_t idx, bool dec, reg **tab)
    {
        tab[idx]->add16(dec ? -1 : 1);
    }

    // increment or decrement 8-bit register
    inline void incdec8(uint8_t idx, bool low, bool dec, reg **tab)
    {
        uint8_t val = get8(idx, low, tab);
        uint8_t f = regs.af.getl() & ~(1 << F_N | 1 << F_PV | 1 << F_Z | 1 << F_H);

        dec ? val-- : val++;
        f |= dec << F_N;
        f |= (val == (0x80 - dec) || !(val + dec)) << F_PV;
        f |= !(val + dec) << F_Z;
        f |= ((val & (0x10 - dec)) == (0x10 - dec)) << F_H;

        set8(idx, val, low, tab);
        regs.af.setl(f);
    }

    // main bitshift operations on a
    inline void bitshifta(uint8_t op)
    {
        uint8_t val = regs.af.geth();
        uint8_t f = regs.af.getl();

        f &= ~(1 << F_H | 1 << F_N | 1 << F_C);

        if(op >> 3 & 1) // rlca, rla
            f |= (val & 1) << F_C;
        else // rrca, rra
            f |= (val >> 7) << F_C;

        switch(op >> 3)
        {
            case 0: // rlca
                val = val << 1 | val >> 7;
                break;
            case 1: // rrca
                val = val >> 1 | val << 7;
                break;
            case 2: // rla
                val = val << 1 | !!(f & (1 << F_C));
                break;
            case 3: // rra
                val = val >> 1 | !!(f & (1 << F_C)) << 7;
                break;
        }

        f |= parity(val) << F_PV;

        regs.af.seth(val);
        regs.af.setl(f);
    }

    inline bool cond(cc condition_code)
    {
        uint8_t f = regs.af.getl();
        bool z  = f & 1 << F_Z,
             c  = f & 1 << F_C,
             pv = f & 1 << F_PV,
             s  = f & 1 << F_S;
        switch(condition_code)
        {
            case CC_NZ:
                return !z;
            case CC_Z:
                return z;
            case CC_NC:
                return !c;
            case CC_C:
                return c;
            case CC_PO:
                return !pv;
            case CC_PE:
                return pv;
            case CC_P:
                return !s;
            case CC_M:
                return s;
        }
        assert(!"This should never happen!");
    }

    inline void reljmp(uint8_t off)
    {
        if(off & BIT7)
            regs.pc -= twoscomp(off);
        else
            regs.pc += off;
    }

    inline void ccreljmp(uint8_t off)
    {
        if(cond(static_cast<cc>((off - 0x20) >> 3)))
            reljmp(off);
    }

    const z80 &operator=(const z80 &rhs)
    {
        memcpy(mem, rhs.mem, 1 << 16);
        regs = rhs.regs;
        return *this;
    }

    z80()
    {
        mem = new uint8_t[1 << 16]();
    }

    z80(const z80 &old)
    {
        mem = new uint8_t[1 << 16];
        memcpy(mem, old.mem, 1 << 16);
        regs = old.regs;
    }

    ~z80()
    {
        delete[] mem;
    }
}; // struct z80

} // namespace z80emu

#endif

I'm still looking for some of the same things as last time, but I'll go ahead and go over them again:

• Are there any "more C++" things (that work from C++03 to C++2a) that I can do? Have I started using the language's features adequately?

• Are there any C++ "best practices" that I'm missing here?

• If there are any other miscellaneous things that could be improved, please let me know.

Answer 1

Here are some things that may help you improve your program.

Decide on a header naming scheme

The emulate.cpp file has these two lines:

#include "z80emu.hpp"
#include "opcodes.h"

However the files are labeled in the question as both having .hpp extensions. It's better to pick one or the other and stick with it rather than mixing both. I use .h always.

Don't use std::endl if you don't really need it

The difference betweeen std::endl and '\n' is that '\n' just emits a newline character, while std::endl actually flushes the stream. This can be time-consuming in a program with a lot of I/O and is rarely actually needed. It's best to only use std::endl when you have some good reason to flush the stream and it's not very often needed for simple programs such as this one. Avoiding the habit of using std::endl when '\n' will do will pay dividends in the future as you write more complex programs with more I/O and where performance needs to be maximized.

Prefer class to struct

The only real difference, of course, is that by default, the members of a struct are public, while the members of a class are private. Still, it's best to keep the internals of a class private to reduce linkage among objects to only what they need. This simplifies the interface and therefore the maintenance. In this case, the z80 struct has a great many things, including registers and memory, that would likely be better hidden as internal private data structures.

Only use inline for small, timing-critical functions

The overuse of inline is a problem in two respects. First, it makes it harder for readers to read and understand the interface to your class. Second, it has the potential to ruin portability if you intend to maintain a stable ABI. See F.5 for details.

Prefer in-class initializers to member initializers

The code currently includes this:

struct registers
{
    reg af;
    reg bc;
    reg de;
    reg hl;
    reg ix;
    reg iy;
    reg sp;
    reg wz;
    uint16_t pc;
    registers()
    {
        pc = 0;
    }
};

This would be better written with no explicit constructor at all:

struct registers
{
    reg af;
    reg bc;
    reg de;
    reg hl;
    reg ix;
    reg iy;
    reg sp;
    reg wz;
    uint16_t pc = 0;
};

The same applies to the underlying reg. See C.48 for details.

Separate interface from implementation

The interface is the part in the .h file and the implementation is in the .cpp file. Users of this code should be able to read and understand everything they need from the interface file. That means, among other things, that only #includes essential to being able to understand the interface should be in the .h file. In this case, much of what is currently in the z80emu.h file should actually be moved into a z80emu.cpp file and the implementation of z80::emulate should be moved from emulate.cpp to z80emu.cpp.

Strive for a minimal sufficient interface

The code currently contains these lines:

// calculate the two's complement of an 8-bit integer
template<typename T>
inline T twoscomp(T val)
{
    return ~val + 1;
}

There are a couple of problems with this. First, it's only used internally in a single location. Second, there's not really a need for it to be a template, since both the comment and the usage indicate that it's only intended for use with a uint8_t type.

Use only necessary #includes

The #include <vector> line in z80emu.h is not necessary and can be safely removed. It would be good to review all includes to make sure that only the required ones are present.

Initialize variables with declaration

The code currently has these lines:

std::ifstream infile;

infile.open(argv[1], std::ifstream::in | std::ifstream::binary);
if(!infile.good())
{
    std::cerr << "Opening " << argv[1] << " failed: "
              << std::strerror(errno) << std::endl;
    return EXIT_FAILURE;
}

That's not wrong, per se, but there are more idiomatic ways to write that. First, in C++, it's generally good practice to initialize variables as they are declared so that they're immediately useful. In this case, that means combining lines:

std::ifstream infile{argv[1], std::ifstream::in | std::ifstream::binary};

Here I am using the C++11 style of initialization, which I highly recommend, but the same can be done with older C++ compilers with slightly different syntax.

The second thing is that instead of this:

if(!infile.good())

We can use the more idiomatic:

if (!infile) 

The result is the same, but the latter style is less verbose and more typical of modern style.

Avoid C-style casts

The code has these two lines:

z80emu::z80 z80;
infile.read((char *)z80.mem, file_size);

One problem with this is the C-style cast of z80.mem to a char * but the the more fundamental problem is that we're reaching into the innards of the z80 object. Better would be to create a constructor that accepts a memory chunk and size.

Check return values for errors

In the lines quoted above, infile.read() simply throws away the return value. That's not a good idea because that function returns the number of bytes actually read. It's better to always check that you're actually getting what you expected from I/O functions or memory allocation functions and take the appropriate error handling actions otherwise.

Rethink the class design

The opcodes enum is currently just a list of numbers with associated names. Better, in my view, would be to have an instruction class that would encapsulate both the opcode and the behavior. This is the very definition of object-oriented programming, and would help a lot here. To give you some ideas about how this might look consider MAC1 simulator/debugger and ToyVM - a small and simple virtual machine in C + FizzBuzz demonstration

Answer 2

This is a supplement to the excellent answer by Edward.

Care with namespaces

Currently, the header has

using std::uint16_t;
using std::uint8_t;

I recommend not bringing these into the global namespace in a header - that affects every translation unit that uses the header, which can be a nuisance in larger programs (particularly when not all written by the same author). Instead, if you really feel that std:: is too much to type and to read, bring them into a smaller scope (e.g. within a function, or at worst into global scope in individual, non-header, files).

There are a few uses of unqualified names from the std namespace - these should be portably written std::size_t, std::memcpy, etc. You've probably only compiled on systems that use their freedom to put copies of Standard Library identifiers into the global namespace, but that's not required, and you can't depend on it.

Includes

I recommend re-ordering these includes:

#include <cerrno>
#include <limits>
#include <cstdlib>
#include <cstring>
#include <fstream>
#include <iostream>
#include <exception>
#include "z80emu.hpp"

If we put our own includes before the standard library headers, we stand a better chance of identifying accidental dependencies:

#include "z80emu.hpp"

#include <cerrno>
#include <cstdlib>
#include <cstring>
#include <exception>
#include <fstream>
#include <iostream>
#include <limits>

I'm pretty sure we don't use <limits>, and really ought to have <cstdint> instead (for UINT16_MAX and the like).

Error reporting

Most error messages are correctly sent to std::cerr. But when we call usage() to indicate invocation errors, that's sent to std::cout. We should pass the stream to this function, too, so we can make it print to the error stream when it's shown as an error (rather than specifically requested, when we add support for --help argument).

static void usage(std::ostream& os, const char *progname)
{
    os << "  Usage: " << progname << " z80-prog\n";
}

I also recommend static linkage here, as this function shouldn't need to be accessible from other translation units.