Disassembler for intel 8080 v2

Question

As many suggestions were given for the code I posted here, I decided to show you the new version for review.

• I Removed the OpenCatalog class, Disassembler now has the responsibility for handling the catalog of OpCode that is now stored in an std::map.

• OpCode is now a struct and AssemblyLine handles the generation of the assembler code.

• FileReaderHelper is now file_parse_helpers, a namespace containing helper functions.
• I used string_view everywhere I though I could.
• I fixed all the things like initialization order, includes, spelling, consts, etc
• Reduced the amount of streams I was using.

file_parse_helpers.h

#ifndef FILE_PARSER_HELPER_H
#define FILE_PARSER_HELPER_H

#include <vector>
#include <string>

namespace file_parse_helpers
{
    std::vector<std::string_view> splitString(std::string_view toSplit, std::string_view delimiter);
    int hexStringToInt(std::string_view toSplit);
    std::vector<unsigned char> loadBinaryFile(std::string filepath);
}

#endif // FILE_PARSER_HELPER_H

file_parse_helpers.cpp

#include "FileReaderHelper.h"
#include <fstream>
#include <algorithm>

namespace file_parse_helpers
{
    std::vector<std::string_view> splitString(std::string_view toSplit, std::string_view delimiter)
    {
        std::vector<std::string_view> result;
        size_t lastPos = 0;

        while (lastPos < toSplit.size()) {
            const auto position = toSplit.find_first_of(delimiter, lastPos);

            if (lastPos != position) {
                result.emplace_back(toSplit.substr(lastPos, position - lastPos));
            }

            if (position == std::string_view::npos)
                break;

            lastPos = position + 1;
        }

        return result;
    }

    int hexStringToInt(std::string_view toSplit)
    {
        const auto pouet = std::strtol(toSplit.data(), 0, 16);
        return pouet;
    }

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

OpCode.h

#ifndef OPCODE_H
#define OPCODE_H

#include <string>

struct OpCode {
    uint8_t code;
    int byteCount;
    std::string resultTemplate;
};

#endif // OPCODE_H

AssemblyLine.h

#ifndef ASSEMBLYLINE_H
#define ASSEMBLYLINE_H

#include <string>

#include "OpCode.h"

class AssemblyLine
{
public:
    AssemblyLine(const OpCode& opCode);
    AssemblyLine(const OpCode& opCode, const uint8_t param1);
    AssemblyLine(const OpCode& opCode, const uint8_t param1, const uint8_t param2);

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

    std::string generateAssembler(const std::string& templateString) const;

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

#endif

AsemblyLine.cpp

#include "AssemblyLine.h"

#include <stdexcept>

AssemblyLine::AssemblyLine(const OpCode& opCode) :
    byteCount(opCode.byteCount),
    code(opCode.code),
    param1(0),
    param2(0)
{

}

AssemblyLine::AssemblyLine(const OpCode& opCode, const uint8_t param1) :
    byteCount(opCode.byteCount),
    code(opCode.code),
    param1(param1),
    param2(0)
{

}

AssemblyLine::AssemblyLine(const OpCode& opCode, const uint8_t param1, const uint8_t param2) :
    byteCount(opCode.byteCount),
    code(opCode.code),
    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;
}

std::string AssemblyLine::generateAssembler(const std::string& templateString) const
{
    const int maxStringLength{ 32 };

    if (templateString.length() > maxStringLength)
    {
        throw std::invalid_argument("This template string is too long: " + templateString);
    }

    char temp[maxStringLength];
    switch (byteCount) {
    case 1:
        return templateString;
    case 2:
        std::snprintf(temp, maxStringLength, templateString.c_str(), param1);
        break;
    case 3:
        std::snprintf(temp, maxStringLength, templateString.c_str(), param1, param2);
        break;
    }
    return temp;
}

Disassembler.h

#ifndef DISASSEMBLER_H
#define DISASSEMBLER_H


#include <map>
#include <vector>

#include "AssemblyLine.h"
#include "OpCode.h"

class Disassembler
{

public:
    Disassembler(const std::string& filename);
    std::vector<AssemblyLine> disassemble(const std::vector<unsigned char>& data);
    void printAssembly(const std::vector<unsigned char>& data);
    AssemblyLine disasembleOneCode(const std::vector<unsigned char>& data, size_t codeIndex);
    void loadCatalog(const std::string& filename);

private:
    std::map<uint8_t, OpCode> opCodes;
};

#endif

Disassembler.cpp

#include "Disassembler.h"
#include "File/FileReaderHelper.h"

#include <iostream>
#include <fstream>

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

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

    switch (byteCount) {
    default:
    case 1:
        return AssemblyLine(opCode);
        break;
    case 2:
        return AssemblyLine(opCode, data[codeIndex + 1]);
        break;
    case 3:
        return  AssemblyLine(opCode, data[codeIndex + 2], data[codeIndex + 1]);
        break;
    }
}

std::vector<AssemblyLine> Disassembler::disassemble(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;
}

void Disassembler::loadCatalog(const std::string& filename)
{
    opCodes.clear();
    std::ifstream input(filename);
    for (std::string line; std::getline(input, line);)
    {
        const auto tokens = file_parse_helpers::splitString(line, "\t");

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

        uint8_t code = file_parse_helpers::hexStringToInt(std::string(tokens[0]));
        opCodes.emplace(code, OpCode{ code,
                                    std::stoi(std::string(tokens[2]), nullptr, 10),
                                    std::move(std::string(tokens[1])) });
    }
}

void Disassembler::printAssembly(const std::vector<unsigned char>& data)
{
    size_t currentId = 0;
    for (; currentId < data.size() - 1;)
    {
        const auto asmLine = disasembleOneCode(data, currentId);
        currentId += asmLine.getByteCount();
        std::cout << asmLine.generateAssembler(opCodes[asmLine.getCode()].resultTemplate) << std::endl;
    }
}

Edit: After posting I realized that I made a mistake. I throw an exception about the size of the template string provided in the catalog, but I should do it when parsing the catalog, not when I use the string. Now, how do I replace maxStringLength so that I can use it when loading the catalog and as the size of the temp array in generateAssembler? Are C macros the only solution here?

Answer 1

1. loadBinaryFile should either take a std::string_view, or, at least, a std::string const& as argument. You don't want to take ownership of the string here.
2. Your filenames do not match. file_parse_helpers.cpp begins with #include "FileReaderHelper.h", but you don't show us the contents of that header. Presumably, you'd want to include file_parse_helpers.h.
3. You're missing the required includes for size_t, uint8_t and friends. Also, ::size_t, ::uint8_t etc. might not exist if you include the correct <cheader>, and should be replaced with std::size_t and std::uint8_t.
4. hexStringToInt is dangerous. std::string_view is not guaranteed to be null-terminated, and if it isn't, you could be reading out of bounds and invoking undefined behavior.
5. Don't pass 0 to pointer arguments, pass nullptr. The latter is typesafe and indicates your intent clearly.
6. You can merge the three AssemblyLine constructors into one by adding default arguments: AssemblyLine(OpCode const& opCode, uint8_t param1 = 0, uint8_t param2 = 0).
7. Why are the member variables of AssemblyLine protected? Doesn't look like you're planning on inheriting from it, so these should be private.
8. Beware of const member variables! While they might seem harmless on first sight, they're actually quite dangerous if you ever intend to put your objects into some sort of container. In fact, making a std::vector<AssemblyLine>, adding some elements, removing some elements, then adding some elements will invoke undefined behavior.
9. Don't use printf and friends; they're not type safe and don't always deal well with non-null-terminated strings. Either use the standard library facilities (i.e. iostreams), or make use of a modern formatting library, such as fmt.
10. disasembleOneCode [sic] can exhibit undefined behavior since you don't do any bounds checking, and if codeIndex + 2 >= data.size(), then you're accessing somewhere you shouldn't.
11. for (; currentId < data.size() - 1;) should be expressed as while (currentId < data.size() - 1).
12. Don't write printX functions. Instead, write either a function taking a std::ostream and writing to it, or overload operator<< for your class. The reason for this is that this makes printing a lot more flexible and enables you to print to many different sinks, without having to adjust the actual printing code.

---

To answer your question: No, you don't need a macro here at all. Just put constexpr inline unsigned const maxStringLength = 32; in a header and include it.

Answer 2

This version is improved, but there are still more things you can do to make it still better.

Remove paths from local includes

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

#include "File/FileReaderHelper.h"

Better is to omit that like this:

#include "FileReaderHelper.h"

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

Don't include redundant qualifications

In the same way that one shouldn't include the class name within class member function declarations, this line:

std::vector<unsigned char> file_parse_helpers::loadBinaryFile(std::string filepath)

because it already appears within the namespace declaration, should not have the namespace qualification and should instead be written like this:

std::vector<unsigned char> loadBinaryFile(std::string filepath)

Let the user specify an output stream

The printAssembly function of the current Disassembler class is useful, but somewhat limited for two reasons. The first reason is that it is hardcoded to only print to std::cout. It would be more flexible to have a std::ostream& paramater an argument. The second reason is mentioned in the next suggestion.

Rethink the interface

The Disassembler::printAssembly function requires a std::vector<unsigned char>& argument. I'd suggest that instead of specifically having such a structure, it might be useful to be rewrite it as a templated function to have it use any pair of iterators so that one could pass, say, code.begin(), code.end(). The same change would be beneficial to DisasembleOneCode (which still has a spelling error, by the way). Also, I'd suggest storing an Opcode & rather than just the opcode byte and bytecount within AssemblyLine. This would make it much more useful because it would contain everything it needs to generate its own assembly output. Also, instead of three separate constructors, just provide one with default parameters:

AssemblyLine(const OpCode& opCode, const uint8_t param1=0, const uint8_t param2=0);

Finally, I'd suggest that generateAssembler() might delegate to a member function of Opcode rather than needing to extract the templateString from the latter.

Avoid storing data

The current code reads the entire binary file into memory, but if all we're doing is disassembling, that's not really needed. Instead, one could make a single forward pass through the code and disassemble it on the fly. Here's what my implementation looks like:

#include "Disassembler.h"
#include <iostream>
#include <fstream>
#include <iterator>

int main(int argc, char *argv[]) {
    if (argc != 3) {
        std::cerr << "Usage: d8080 opcodefile binfile\n";
        return 1;
    };
    Disassembler d{argv[1]};
    std::ifstream bin{argv[2]};
    using iter = std::istream_iterator<unsigned char>;
    auto end{iter{}};
    for (auto curr{iter{bin}}; bin; ) {
        std::cout << d.disassembleOne(curr, end) << '\n';
    }
}

The disassembleOne function looks like this:

std::string Disassembler::disassembleOne(iter& begin, iter& end) const {
    if (begin == end)
        return "";
    auto op{opCodes.at(*begin++)};
    unsigned char param[2]{0,0};
    int i{0};
    switch (op.byteCount) {
        case 3:
            if (begin != end) 
                param[i++] = *begin++;
            // deliberately fall through
        case 2:
            if (begin != end) 
                param[i++] = *begin++;
            break;
        default:
            break;
    }
    return op.asmString(param[0], param[1]);
}

Here's the slightly revised OpCode.cpp:

#include "OpCode.h"
#include <stdexcept>

std::string OpCode::asmString(unsigned char param1, unsigned char param2) const {
    static constexpr int maxStringLength{ 32 };

    if (resultTemplate.length() > maxStringLength)
    {
        throw std::invalid_argument("This template string is too long: " + resultTemplate);
    }

    char temp[maxStringLength];
    switch (byteCount) {
    case 1:
        return resultTemplate;
    case 2:
        std::snprintf(temp, maxStringLength, resultTemplate.c_str(), param1);
        break;
    case 3:
        std::snprintf(temp, maxStringLength, resultTemplate.c_str(), param2, param1);
        break;
    }
    return temp;
}

Note that in this revised version, only two classes, Disassembler and OpCode are used.

Think of future expansion

Some things that might be useful for future features might be to keep track of the program counter, perhaps also optionally printing it and the disassembled byte values as part of the listing. It's also probably useful to think about how this might be used for other small 8-bit processors, such as the Z80 and 6502. I believe that all that would be required would be a different input opcode file.

Checking string lengths

As to your question about how to check string lengths, the simplest would be to simply check when reading the opcode file. No further checks are then necessary, since the opcode map does not change after construction.