10
\$\begingroup\$

I have written a disassembler for Intel 8080, able to translate binaries into opcodes and parameters. Sorry for posting so much code, but everything posted here is useful for the understanding of the code (and I hope that you will criticize all of it too). Thanks a lot for your time.

FileReaderHelper holds some functions that help with reading and parsing.

FileReaderHelper.h

#include <vector>
#include <string>

class FileReaderHelper
{
public:
    static const std::vector<std::string> splitString(const std::string& toSplit, char delimiter);
    static const int hexStringToInt(const std::string& toSplit);
    static const std::vector<unsigned char> loadBinaryFile(const std::string& filepath);
};

FileReaderHelper.cpp

#include "FileReaderHelper.h"
#include <sstream>
#include <fstream>
#include <iterator>

const std::vector<std::string> FileReaderHelper::splitString(const std::string& toSplit, char delimiter)
{
    std::istringstream iss(toSplit);
    std::vector<std::string> results;
    std::string s;
    while (std::getline(iss, s, delimiter)) {
        results.emplace_back(s);
    }
    return results;
}

const int FileReaderHelper::hexStringToInt(const std::string& input)
{
    std::stringstream  stringToHex;
    stringToHex << std::hex << input;
    int hexValue = 0;
    stringToHex >> hexValue;
    return hexValue;
}

const std::vector<unsigned char> FileReaderHelper::loadBinaryFile(const std::string& filepath)
{
    std::ifstream inputStream(filepath, std::ios::binary);
    std::vector<unsigned char> result(std::istreambuf_iterator<char>{inputStream}, {});
    return result;
}

The OpCodeCatalog class contains all the OpCodes, parsed from a text file that contains lines like these (it is much more readable with tabs indentation):

0xc0    RNZ 1               if NZ, RET 
0xc1    POP B   1           C <- (sp)    B <- (sp+1)     sp <- sp+2
0xc2    JNZ $%02x%02x   3   if NZ, PC <- adr 
0xc3    JMP $%02x%02x   3   PC <= adr
0xc4    CNZ $%02x%02x   3   if NZ, CALL adr 
0xc5    PUSH B  1           (sp-2)<-C (sp-1)<-B  sp <- sp - 2 
0xc6    ADI #%02x   2       Z, S, P, CY, AC A <- A + byte
0xc7    RST 0   1           CALL $0

OpCodeCatalog.h

#include <string>
#include <vector>

#include "OpCode.h"

class OpCodeCatalog
{
private:

public:
    OpCodeCatalog(const std::string& filename);

    void loadCatalog(const std::string& filename);
    const OpCode& getOpCode(int code) const;
    size_t getOpCodeCount() const noexcept;

private:
    std::vector<OpCode> opCodes;
};

OpCodeCatalog.cpp

#include "OpCodeCatalog.h"

#include <fstream>
#include <sstream>
#include <iterator>
#include <string>

#include "File\FileReaderHelper.h"

OpCodeCatalog::OpCodeCatalog(const std::string& filename)
{
    loadCatalog(filename);
}

void OpCodeCatalog::loadCatalog(const std::string& filename)    
{
    opCodes.clear();
    std::ifstream input(filename);
    for(std::string line; std::getline(input, line);)
    {
        const auto tokens = FileReaderHelper::splitString(line, '   ');

        if (tokens.size() < 3) { 
            throw "Malformed catalog line: " + line; 
        }

        opCodes.emplace_back(OpCode{    FileReaderHelper::hexStringToInt(tokens[0]),
                                        std::stoi(tokens[2], nullptr, 10),
                                        tokens[1] });   
    }
}

const OpCode& OpCodeCatalog::getOpCode(int code) const
{
    if (code < 0 || code >= opCodes.size())
    {
        throw std::invalid_argument("OpCode value outside the range of the catalog. Value: " + code);
    }
    return opCodes[code];
}

size_t OpCodeCatalog::getOpCodeCount() const noexcept
{
    return opCodes.size();
}

The OpCode class holds the informations for one OpCode: its code and byte count. It can also generate the associated assembly line using the template string.

OpCode.h

#include <string>

#define OPCODE_TEMPLATE_MAX_LENGTH 32

class OpCode
{
public:
    OpCode(int code, int byteCount, std::string resultTemplate);

    constexpr int getCode() const noexcept
    {
        return code;
    }

    constexpr int getByteCount() const noexcept
    {
        return byteCount;
    }

    const std::string getGeneratedAsm() const;
    const std::string getGeneratedAsm(int v1) const;
    const std::string getGeneratedAsm(int v1, int v2) const;

private:
    const int code;
    const int byteCount;
    const std::string resultTemplate;
};

OpCode.cpp

#include "OpCode.h"

OpCode::OpCode(int code, int byteCount, std::string resultTemplate) :
    code{ code },
    byteCount{ byteCount },
    resultTemplate{ resultTemplate }
{
    if (resultTemplate.length() > OPCODE_TEMPLATE_MAX_LENGTH)
    {
        throw std::invalid_argument("This template string is too long: " + resultTemplate);
    }
}

const std::string OpCode::getGeneratedAsm() const
{
    return resultTemplate;
}

const std::string OpCode::getGeneratedAsm(int v1) const
{
    char temp[OPCODE_TEMPLATE_MAX_LENGTH];
    std::snprintf(temp, OPCODE_TEMPLATE_MAX_LENGTH, resultTemplate.c_str(), v1);
    return std::string(temp);
}

const std::string OpCode::getGeneratedAsm(int v1, int v2) const
{
    char temp[OPCODE_TEMPLATE_MAX_LENGTH];
    std::snprintf(temp, OPCODE_TEMPLATE_MAX_LENGTH, resultTemplate.c_str(), v1, v2);
    return std::string(temp);
}

The AssemblyLine class holds all the information of a line of assembly language (code, parameters, bytecount).

AssemblyLine.h

#pragma once

#include "Source\OpCode.h"

class AssemblyLine
{
public:
    AssemblyLine(uint8_t code, uint8_t byteCount);
    AssemblyLine(uint8_t code, uint8_t byteCount, const uint8_t param1);
    AssemblyLine(uint8_t code, uint8_t byteCount, const uint8_t param1, const uint8_t param2);

    uint8_t getParam1() const;
    uint8_t getParam2() const;
    uint8_t getCode() const;
    uint8_t getByteCount() const;

protected:
    const uint8_t byteCount;
    const uint8_t code;
    const uint8_t param1;
    const uint8_t param2;
};

AssemblyLine.cpp

#include "AssemblyLine.h"

AssemblyLine::AssemblyLine(uint8_t code, uint8_t byteCount) :
    code(code),
    byteCount(byteCount),
    param1(0),
    param2(0)
{

}

AssemblyLine::AssemblyLine(uint8_t code, uint8_t byteCount, const uint8_t param1) :
    code(code),
    byteCount(byteCount),
    param1(param1),
    param2(0)
{

}

AssemblyLine::AssemblyLine(uint8_t code, uint8_t byteCount, const uint8_t param1, const uint8_t param2) :
    code(code),
    byteCount(byteCount),
    param1(param1),
    param2(param2)
{

}

uint8_t AssemblyLine::getParam1() const
{
    return param1;
}

uint8_t AssemblyLine::getParam2() const
{ 
    return param2;
}

uint8_t AssemblyLine::getCode() const
{
    return code;
}

uint8_t AssemblyLine::getByteCount() const
{
    return byteCount;
}

Last but not least, Disassembler is the interface that use all the previous classes to provide the user with meaningful information.

Disassembler.h

#pragma once

#include "Source\OpCodeCatalog.h"
#include "AssemblyLine.h"

class Disassembler
{
public:
    Disassembler(const OpCodeCatalog& catalog);
    const std::vector<AssemblyLine> disasemble(const std::vector<unsigned char>& data);
    const AssemblyLine disasembleOneCode(const std::vector<unsigned char>& data, size_t codeIndex);

private:
    const OpCodeCatalog& catalog;
};

Disassembler.cpp

#include "Disassembler.h"

Disassembler::Disassembler(const OpCodeCatalog& catalog) :
    catalog{ catalog }
{
}

const std::vector<AssemblyLine> Disassembler::disasemble(const std::vector<unsigned char>& data)
{
    size_t currentId = 0;
    std::vector<AssemblyLine> disasembledCode;
    for (; currentId < data.size() - 1;)
    {
        const auto asmLine = disasembleOneCode(data, currentId);
        currentId += asmLine.getByteCount();
        disasembledCode.emplace_back(asmLine);
    }
    return disasembledCode;
}

const AssemblyLine Disassembler::disasembleOneCode(const std::vector<unsigned char>& data, size_t codeIndex)
{
    int codeValue = data[codeIndex];
    const auto& opCode = catalog.getOpCode(codeValue);
    const auto byteCount = opCode.getByteCount();

    switch (byteCount) {
    default:
    case 1:
        return AssemblyLine(opCode.getCode(), opCode.getByteCount());
        break;
    case 2:
        return AssemblyLine(opCode.getCode(), opCode.getByteCount(), data[codeIndex + 1]);
        break;
    case 3:
        return  AssemblyLine(opCode.getCode(), opCode.getByteCount(), data[codeIndex + 2], data[codeIndex + 1]);
        break;
    }
}
\$\endgroup\$
6
\$\begingroup\$

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

Remove paths from local includes

It's generally better not to put the path explicitly in a local #include like this:

#include "Source\OpCodeCatalog.h"

Better is to omit that like this:

#include "OpCodeCatalog.h"

and then point the compiler to the appropriate path. This makes your source code cleaner, more portable and easier to maintain.

Use include guards in all .h files

Every .h file should have an include guard. For example, your OpCode.h file could be written like this:

#ifndef OPCODE_H
#define OPCODE_H
// existing code here
#endif // OPCODE_H

It's better to use this than #pragma once because, by definition, a #pragma is compiler-specific and non-portable, while the include guards are explicitly part of the C++ standard. See SF.8

Use the required #includes

The code uses std::invalid_argument which means that it should #include <stdexcept>. It was not difficult to infer, but it helps reviewers if the code is complete.

Rethink your object model

There doesn't seem to be much point to the AssemblyLine class. It seems to do little more than store the bytes retrieved. There is also no simple way to print the instructions as it is. I wrote this to print, but it's not very nice:

std::ostream& AssemblyLine::print(std::ostream& out, OpCodeCatalog& cat) const {
    auto op = cat.getOpCode(code);
    switch (byteCount) {
        case 1:
            out << op.getGeneratedAsm();
            break;
        case 2:
            out << op.getGeneratedAsm(param1);
            break;
        case 3:
            out << op.getGeneratedAsm(param1, param2);
            break;
        default:
            out << "too long!";
    }
    return out << '\n';
}

Part of the problem with it is the interaction between the AssemblyLine class and the OpCodeCatalog class and the OpCode class. We really shouldn't need all three for this.

Also, if the Disassembler class cannot function without the OpCodeCatalog class, is there really much point in having them as separate classes?

Check your spelling

The function in the disassembler is disasemble (missing an s). Those kinds of errors don't bother the compiler at all, of course, but they're really annoying to human users of the code.

Don't abuse const

The usual advice I give is to add more const to function declarations, parameters, etc. However in this case we have a little too much const. Specifically, const type qualifiers on return values such as this:

static const int hexStringToInt(const std::string& toSplit);

are simply ignored and should be discarded.

Write member initializers in declaration order

The AssemblyLine class has this constructor:

AssemblyLine::AssemblyLine(uint8_t code, uint8_t byteCount, const uint8_t param1, const uint8_t param2) :
    code(code),
    byteCount(byteCount),
    param1(param1),
    param2(param2)
{
}

That looks fine, but in fact, byteCount will be initialized before code because members are always initialized in declaration order and byteCount is declared before code in this class. To avoid misleading another programmer, you should swap the order of those such that it says instead:

AssemblyLine::AssemblyLine(uint8_t code, uint8_t byteCount, const uint8_t param1, const uint8_t param2) :
    byteCount(byteCount),
    code(code),
    param1(param1),
    param2(param2)
{
}

Make character constants visible

One line in OpCodeCatalog.cpp looks like this:

const auto tokens = FileReaderHelper::splitString(line, '   ');

That second argument is a tab character, but it's hard to tell because that's a non-printing character. Better would be to use '\t' instead to make it visible rather than actually using a tab character there.

Fix the bug

If we have a processor (such as the 8080) for which some byte values are not documented opcodes, there may be less than 256 opcodes in the OpCodeCatalog. However, the getOpCode function uses a direct lookup which makes the assumption that all of the opcodes are populated and that they were read in from the file in numerical order. Neither of those assumptions are checked or enforced by the code. I'd suggest using a std::map instead.

\$\endgroup\$
8
\$\begingroup\$
  1. C++ is not Java. While there is a valid use-case for classes treated as namespaces with a type-name, that's only for traits-classes used with templates, and not for general consumption.

  2. I don't know why, but you seem quite attached to "a class for everything" and "everything in a class". You know C++ is multi-paradigm, and OOP isn't always the best tool for the job?
    Wrapping everything leads to useless boilerplate, which not only has to be written, but read too!

  3. There are exactly two cases for prefering std::string const& over std::string_view in C++17:

    1. You need the zero-terminator. Unfortunately, there's no corresponding view offering that too.

    2. You have a pre-existing interface and need to maintain compatibility.

    Does either of those apply to you?

  4. splitString() might be better off returning a std::vector<std::string_view> referencing the input-string instead, avoiding most of the allocations. If necessary, decoupling from the passed string is easy, and only eats the savings from before.

  5. Using iostreams is quite heavy-weight. Try to avoid it unless performance really doesn't matter to you at all.

  6. Don't return a const T (const T& is fine). In ancient times before move-semantics, it was used to inhibit using the return-value directly, but we are beyond that silliness.

  7. There's an implicit conversion from const char* to std::string, no need to coerce it.

\$\endgroup\$
  • \$\begingroup\$ Thanks you for the answer. Can you please be more specific about 1. and 2.? Are they both about FileReaderHelper? \$\endgroup\$ – Stud Aug 4 '18 at 16:51
  • \$\begingroup\$ The first is about that, the second about you always making a new class. There's nothing wrong with using std::vector<Opcode> allOpcodes directly. \$\endgroup\$ – Deduplicator Aug 4 '18 at 16:53
  • \$\begingroup\$ I'm sorry but I do not understand in 4. how do you "reference the input-string"? \$\endgroup\$ – Stud Aug 4 '18 at 17:30
  • \$\begingroup\$ std::string_view is not a container owning the sequence it presents, but a view just having a non-owning reference to it. \$\endgroup\$ – Deduplicator Aug 4 '18 at 17:33
  • \$\begingroup\$ Yes, that's why I do not understand. Do you suggest manually splitting the string using std::string_view all the way? I've never used string_view, I don't know what's possible, hence I don't know what you mean here. \$\endgroup\$ – Stud Aug 4 '18 at 17:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.