Toy virtual machine

Question

I am a web developer, trying to have fun with C++11, so any positive/negative feedback is very welcome.

This is a toy virtual machine with 3 registers and 3 commands:

vm/vm.h

#ifndef VM_H
#define VM_H

#include <iostream>
#include <vector>

#include "vtypes.h"

namespace violet
{

    class Vm
    {
        bool running;
        std::vector<u16> program;
        u16  regs[REGS_N];
        u16  pc;

    public:
        Vm(const std::vector<u16>);
        ~Vm();

        u16 fetch();
        int run();
        std::vector<u16> dump_regs() const;
        instruction decode(const unsigned int) const;
    };

}

#endif // VM_H

vm/vm.cpp

#include "vm.h"

namespace violet
{

    Vm::Vm(const std::vector<u16> program):
        running(false),
        program(program),
        pc(0)
    {

    }

    Vm::~Vm()
    {

    }


    u16 Vm::fetch()
    {
        return program.at(pc++);
    }

    int Vm::run()
    {
        running = true;
        instruction instr;

        if (program.empty()) return 1;
        while(running && pc < program.size())
        {
            instr = decode(fetch());
            switch(instr.code)
            {
                case i_halt:
                    running = false;
                    break;

                case i_loadi:
                    regs[instr.r0] = instr.imm;
                    break;

                case i_add:
                    regs[instr.r0] = regs[instr.r1] + regs[instr.r2];
                    break;
            }
        }

        return 0;
    }

    std::vector<u16> Vm::dump_regs() const
    {
        return std::vector<u16>(std::begin(regs), std::end(regs));
    }

    instruction Vm::decode(const unsigned int opcode) const
    {
        instruction i;
        // Bits 15-12  (4bits) Instruction number
        // Bits 11-8   (4bits) Register number
        // Bits 7-0    (8bits) Immediate value

        i.code = (opcode & 0xF000) >> 12;
        i.r0   = (opcode & 0xF00)  >>  8;
        i.r1   = (opcode & 0xF0)   >>  4;
        i.r2   = (opcode & 0xF);
        i.imm  = (opcode & 0xFF);

        return i;
    }

}

vm/vtypes.h

#ifndef VTYPES_H
#define VTYPES_H

using u8  = unsigned char;    // At least 8-bits
using u16 = unsigned short;   // At least 16-bits   -  (Usually)

namespace violet
{

    const unsigned int REGS_N  {3};

    // Instructions
    const unsigned int i_halt  {0};
    const unsigned int i_loadi {0x1};
    const unsigned int i_add   {0x2};

    struct instruction
    {
        unsigned int code;
        unsigned int r0;
        unsigned int r1;
        unsigned int r2;
        int imm;
    };

}
#endif // VTYPES_H

vm/main.cpp

#include <vector>

#include "vm/vm.h"
#include "etc/utils.h"

using namespace violet;

int main(int argc, char *argv[])
{
    int vm_res;
    const std::vector<u16> program = utils::load_program_file(argv[1]);
    Vm vm(program);
    vm_res = vm.run();

    for (auto r : vm.dump_regs())
    {
        std::cout << r << " ";
    }   std::cout << std::endl;

    return vm_res;
}

asm/vlasm.h

#ifndef VLASM_H
#define VLASM_H

#include <vector>
#include <string>
#include <unordered_map>

#include "vm/vtypes.h"

namespace violet
{
    namespace vlasm
    {
        const char comment(';');
        const std::string i_halt("halt");
        const std::string i_loadi("loadi");
        const std::string i_add("add");

        const std::unordered_map<std::string, unsigned int>
        registers {
                {"r0", 0},
                {"r1", 1},
                {"r2", 2}
        };

        std::vector<unsigned int> parse_source(const std::vector<std::string>);
        std::vector<std::string> parse_line(const std::string);
        unsigned int parse_tokens(const std::vector<std::string>);
        unsigned int parse_reg(const std::string);
        void die(const char *);
    };

}

#endif // VLASM_H

asm/vlasm.cpp

#include <sstream>
#include <iostream>
#include <iterator>

#include "vlasm.h"

namespace violet
{
    namespace vlasm
    {
        std::vector<unsigned int>
        parse_source(const std::vector<std::string> source)
        {
            std::vector<unsigned int> program;

            for (auto line : source)
            {
                if (line[0] == comment) continue;
                std::vector<std::string> tokens = parse_line(line);
                program.push_back(parse_tokens(tokens));

            }

            return program;
        }

        std::vector<std::string>
        parse_line(const std::string line)
        {
            std::istringstream iss(line);
            std::vector<std::string> tokens {
                    std::istream_iterator<std::string>{iss},
                    std::istream_iterator<std::string>{}
            };

            return tokens;
        }

        unsigned int parse_tokens(const std::vector<std::string> tokens)
        {
            unsigned int opcode;

            if (tokens[0] == vlasm::i_halt)
            {
               opcode = violet::i_halt;
            }

            else if (tokens[0] == vlasm::i_loadi)
            {
                if (tokens.size() != 3) die("Invalid load operation");
                opcode = (violet::i_loadi      << 12) |
                         (parse_reg(tokens[1]) <<  8) |
                          std::stoi(tokens[2]) ;
            }

            else if (tokens[0] == vlasm::i_add)
            {
                if (tokens.size() != 4) die("Invalid addition operation");
                opcode = (violet::i_add << 12) |
                         (parse_reg(tokens[1]) << 8) |
                         (parse_reg(tokens[2]) << 4) |
                          parse_reg(tokens[3]);
            }

            else
            {
                die("Invalid instruction found");
            }


            return opcode;
        }

        unsigned int parse_reg(std::string reg_str)
        {
            std::unordered_map<std::string, unsigned int>::const_iterator
            r = registers.find(reg_str);

            if (r == registers.end()) die("Invalid register");
            return r->second;
        }

        void die(const char *message)
        {
            std::cerr << message << std::endl;
            exit(1);
        }
    }
}

asm/main.cpp

#include <vector>

#include <etc/utils.h>
#include "vlasm.h"

using namespace violet;

int main(int argc, char *argv[])
{
    const std::vector<std::string> source = utils::load_source_file(argv[1]);
    const std::vector<unsigned int> program = vlasm::parse_source(source);

    return utils::write_program_file(argv[2], program);
}

etc/utils.h

#ifndef UTILS_H
#define UTILS_H

#include <iostream>
#include <vector>
#include <string>
#include <fstream>

#include "vm/vtypes.h"

    namespace violet
    {

        namespace utils
        {
            std::vector<u16> load_program_file(const std::string);
            std::vector<std::string> load_source_file(const std::string);
            int write_program_file(const std::string, const std::vector<unsigned int>);
        }

    };

    #endif // UTILS_H

etc/utils.cpp

#include "utils.h"

namespace violet
{

    namespace utils
    {
        std::vector<u16>
        load_program_file(const std::string filename)
        {
            u16 instr;
            u8  bytes[2];
            std::vector<u16> result;

            std::ifstream file(filename, std::ios::binary);
            if (file.fail())
            {
                std::cerr << "Could not load program" << std::endl;
                return result;
            }

            while(file.read(reinterpret_cast<char*>(bytes), 2))
            {
                // Little endian
                instr = bytes[0] | bytes[1] << 8;
                result.push_back(instr);
            }

            return result;
        }

        std::vector<std::string>
        load_source_file(const std::string filename)
        {
            std::string line;
            std::vector<std::string> result;

            std::ifstream file(filename);
            if (file.fail())
            {
                std::cerr << "Could not load source" << std::endl;
                return result;
            }

            while(std::getline(file, line))
            {
                result.push_back(line);
            }

            return result;
        }

        int write_program_file(
                const std::string filename,
                const std::vector<unsigned int> program)
        {
            std::ofstream out(filename, std::ios::trunc | std::ios::binary);
            if (out.fail())
            {
                std::cerr << "Could not write program file" << std::endl;
                return 1;
            }

            for (auto i : program)
            {
                out.write(reinterpret_cast<const char *>(&i), sizeof(u16));
            }

            return 0;
        }
    }

}

Usage:

$ violet /path/to/bin.s

$ vlasm /path/to/source.vl /path/to/bin.s

Asm syntax:

• loadi [reg][val] - load immediate value([val]) to register([reg])
• add [reg1][reg2][reg3] - place the sum of [reg2] and [reg3] into [reg1]
• halt - stop the execution

Example program

; comments start with semicolon
loadi r0 100
loadi r1 200
add r2 r0 r1
; now r2 holds the value 300
halt

This program compiled to vm bytecode:

6410 c811 0122 0000  

Answer 1

A few notes to complete what @vnp already said:

• When you open several namespaces at once and close them at once, you better indent only once since the double indentation won't add to clarity but will produce "longer" lines. A future revision of the standard may introduce nested namespaces definition, which makes clear that two namespaces opened and closed at once are generally considered a a single code unit.

• if (out.fail()) is fine, but the idiomatic way to check whether errors occured while opening your file would be if (not out). It's true for input fstreams as well as for output fstreams.

• If you're not planning to modify the parameter filename in write_program_file then you might as well take it by const reference instead of taking it by value. That also applies to program and most of the function parameters in your program (and in the wild).

• Using (const) references may also improve this loop:

  for (auto line : source) { /* ... */ }
  

  Here, every line is a copy of its equivalent in source. It doesn't seem that you alter line afterwise, therefore, you could make it a const reference instead:

  for (const auto& line : source) { /* ... */ }
  

• Try to declare your variables only when you need them. In load_source_file, you better declare line and result only after the error check, otherwise you will end up default-initializing them even when there is an error and your program will waste some time for construction and destruction. That may not really impact the speed of your program specifically, but on a more general note, try not to perform useless operations.

• Your destructor isn't doing anything in Vm, so you better follow the rule of zero and not implement it.

• This is something we shouldn't see anymore:

  using u8  = unsigned char;    // At least 8-bits
  using u16 = unsigned short;   // At least 16-bits   -  (Usually)
  

  C++11 includes the C99 header <stdint.h> (under the name <cstdint>) where typedefs such as uint8_t and uint16_t plus many more variants are defined. If you want a fixed-type integer, use <cstdint>, do not define it yourself.

• You could use typed enum to represent your instructions instead of several const unsigned int variables:

  // Instructions
  enum instructions: unsigned int
  {
      i_halt  = 0x0,
      i_loadi = 0x1,
      i_add   = 0x2
  };
  

  You can even use a scoped enumeration (enum class) if you want more type safety.

• Instead of u16 regs[REGS_N];, you could use an std::array<u16, REGS_N>. std::array is a mere wrapper around a C array and its simplicity makes it generally zero-overhead. Moreover, it offers container-like goodies such as iterators. It's a more secure class for free! :)

• Why do you take a const char* parameter in the function die while you use std::string everywhere else in the project? You might as well be consistent and use an std::string here too.

Answer 2

• I seriously doubt that fetch and decode have reason to be public. They constitute an intimate core of the machine; no sane processor exposes the innards of its pipeline. I understand that they could be instrumental in debugging the machine, which is a good rationale for a debugger class (friendly to Vm). A public step() method would be nice.

• dump_regs is also a good candidate for debugger. In any case, pc must be part of the dump.

• Speaking of debugging, I would think of adding debug information (source file name and line numbers) to the program file.

• load_program_file expects little endiannes. However write_program_file uses a host endianness.