ToyVM - a small and simple virtual machine in C - follow-up

(See the previous and initial iteration)

Now I have refactored the code in order to conform to the suggestions made in the answers to the first iteration. I have this:

toyvm.h:

```
#ifndef TOYVM_H
#define TOYVM_H

#include
#include
#include
#include

enum {
/ Arithmetics /
ADD = 0x01,
NEG = 0x02,
MUL = 0x03,
DIV = 0x04,
MOD = 0x05,

/ Conditionals /
CMP = 0x10,
JA = 0x11,
JE = 0x12,
JB = 0x13,
JMP = 0x14,

/ Subroutines /
CALL = 0x20,
RET = 0x21,

/ Moving data /
LOAD = 0x30,
STORE = 0x31,
CONST = 0x32,

/ Auxiliary /
HALT = 0x40,
INT = 0x41,
NOP = 0x42,

/ Stack /
PUSH = 0x50,
PUSH_ALL = 0x51,
POP = 0x52,
POP_ALL = 0x53,
LSP = 0x54,

/ Registers /
REG1 = 0x00,
REG2 = 0x01,
REG3 = 0x02,
REG4 = 0x03,

/ Interupts /
INTERRUPT_PRINT_INTEGER = 0x01,
INTERRUPT_PRINT_STRING = 0x02,

/ Miscellaneous /
N_REGISTERS = 4,

OPCODE_MAP_SIZE = 256,
};

typedef struct VM_CPU {
int32_t registers[N_REGISTERS];
int32_t program_counter;
int32_t stack_pointer;

struct {
uint8_t BAD_INSTRUCTION : 1;
uint8_t STACK_UNDERFLOW : 1;
uint8_t STACK_OVERFLOW : 1;
uint8_t INVALID_REGISTER_INDEX : 1;
uint8_t BAD_ACCESS : 1;
uint8_t COMPARISON_BELOW : 1;
uint8_t COMPARISON_EQUAL : 1;
uint8_t COMPARISON_ABOVE : 1;
} status;
} VM_CPU;

typedef struct TOYVM {
uint8_t* memory;
int32_t memory_size;
int32_t stack_limit;
VM_CPU cpu;
size_t opcode_map[OPCODE_MAP_SIZE];
} TOYVM;

/*****
Initializes the virtual machine with RAM memory of length 'memory_size' and
* the stack fence at 'stack_limit'.

I see some things that may help you improve your code.

Don't leak memory

The code for InitializeVM allocate memory for the vm->memory member. Every allocation should have a corresponding free, so I would suggest adding this function to the interface and then calling it at the end of main:

void DestroyVM(TOYVM* vm) {
    free(vm->memory);
}

Check return values

The call to calloc can fail and when it does, the only indication is that the value returned is equal to NULL. For that reason, you must check the value for a robust program. The same is true of other functions such as fopen.

Think about signed versus unsigned

There are several values in the virtual machine, such as program_counter and stack_pointer and memory_size which are declared to be int32_t types. However, does it actually make sense for any of these to be negative numbers? I suspect not, so I would advise making them all uint32_t instead. Arguably, this would apply to the return values of GetAvailableStackSize(), GetOccupiedStackSize(), etc. as well.

Return something useful from functions

The RunVM() function is a void function right now, but it would probably be more useful to have it return true in the case of an error or false otherwise. This would allow for simple error checking after the VM has run.

Simplify your code

The RunVM() function currently has these line:

bool (*opcode_exec)(TOYVM*) = instructions[index].execute;

if (opcode_exec(vm))

There isn't really a need to create a separate variable here. This could instead be written as a single line:

if (instructions[index].execute(vm))

Consolidate data tables

The decoding of opcodes uses two structures; one is the instructions array which is ordered by opcode numerical value, and the other is opcode_map which is an array of indices into the instructions array. There's nothing inherently wrong with that, but since it does not appear that the instructions are intended to be dynamically created or assigned by users of the program, these structures could either be consolidated into a single static structure, or you could keep them separate but statically create the opcode_map structure rather that initializing it at runtime.

Don't Repeat Yourself

In almost every ExecuteXXX function, the code begins with a memory check and ends by updating the program counter. For example, the NOP instruction:

static bool ExecuteNop(TOYVM* vm) {
    if (!InstructionFitsInMemory(vm, NOP))
    {
        vm->cpu.status.BAD_ACCESS = 1;
        return true;
    }

    vm->cpu.program_counter += GetInstructionLength(vm, NOP);
    return false;
}

This repetition could be factored out:

static bool ExecuteWrapper(TOYVM* vm, uint8_t opcode, bool (*execute)(TOYVM*), bool isNotJump) {
    if (!InstructionFitsInMemory(vm, opcode))
    {
        vm->cpu.status.BAD_ACCESS = 1;
        return true;
    }
    if (execute(vm)) {
        return true;
    }
    if (isNotJump) {
        vm->cpu.program_counter += GetInstructionLength(vm, opcode);
    }
    return false;
}

Then the instruction struct could have an additional boolean member isNotJump which would, of course, say whether the instruction might advance the program counter to something other than the next instruction. Calling would then look like this:

bool retval = ExecuteWrapper(vm, opcode, 
        instructions[index].execute, instructions[index].isNotJump);

Separate enums

The enums in toyvm.h are not all mutually exclusive values for the same concept which is a little confusing. I would recommend having the instruction values as one enum and then only group related values in other enums. That makes it clear that each enum is a collection of something rather than just a random assortment of constants.

StackExchange Code Review Q#122663, answer score: 11