An attempt at a toy Vm

Question

I usually use c++ so it may not be best practice for c.
This is a stack based toy vm and as a result it is very primitive, and it has no bitwise instructions
64k might be a bit overkill for a toy vm.
coding is hard

#include <stdio.h>
#include "vm.h"

int main() {
    Vm *vm = new_vm();
    i32 buffer[] = {
        0x00000A01, /* push 0x0A(\n) */
        0x00004301, /* push 0x43(C)  */
        0x00004201, /* push 0x42(B)  */
        0x00004101, /* push 0x41(A)  */
        0x00000009, /* output */
        0x00000002, /* pop */
        0x00000009,
        0x00000002,
        0x00000009,
        0x00000002,
        0x00000009,
        0x00000000  /* halt */
    };
    for (int i = 0; i < sizeof(buffer); i++) {
        vm->mem[vm->pc+i] = buffer[i];
    }
    run_vm(vm);
    free_vm(vm);
    return 0;
}

vm.h

#ifndef VM_H_
#define VM_H_

#include <stdlib.h>
#include <stdint.h>
#include <stdbool.h>

typedef uint32_t i32; /* other numbers */
typedef int32_t si32; /* stack pointer */
typedef unsigned char byte;

typedef struct {
    i32 mem[0xffff]; /* approx. 64k */
    si32 sp;
    i32 pc;
    
    i32 raw;
    byte opc;
    i32 param;
    
    bool running;
} Vm;

Vm *new_vm();
void reset_vm(Vm *vm);
void free_vm(Vm *vm);
void run_vm(Vm *vm);

#endif

vm.c

#include <stdio.h>
#include "vm.h"

Vm *new_vm() {
    Vm *ret = (Vm*)malloc(sizeof(Vm));
    ret->pc = 1024; /* add space for the stack */
    ret->sp = -1;
    ret->running = true;
    return ret;
}

void reset_vm(Vm *vm) {
    vm->running = true;
    for (int i = 0; i < 0xffff; i++) {
        vm->mem[i] = 0;
    }
    vm->sp = -1;
    vm->pc = 1024;
}
void free_vm(Vm *vm) {
    free(vm);
    vm = NULL;
}

static void fetch(Vm *vm) {
    vm->raw = vm->mem[vm->pc++];
}

static void decode(Vm *vm) {
    /* style of opcode
     * 24 bits for parameter
     * a byte for the opcode 
    */
    vm->opc = vm->raw & 0xff;
    vm->param = (vm->raw & 0xffffff00) >> 8;
}

static void execute(Vm *vm) {
    switch(vm->opc) {
        case 0x00: /* halt */
            vm->running = false;
            printf("Halt\n");
        break;
        case 0x01: /* push */
            vm->mem[++vm->sp] = vm->param;
        break;
        case 0x02: /* pop */
            vm->mem[vm->sp--] = 0;
        break;
        case 0x03: /* store */
            vm->mem[ vm->mem[vm->sp - 1] ] = vm->mem[vm->sp];
        break;
        case 0x04: /* load */
            vm->mem[vm->sp + 1] = vm->mem[ vm->mem[vm->sp] ];
            ++vm->sp;
        break;
        case 0x05: /* add */
            vm->mem[vm->sp + 1] = vm->mem[vm->sp] + vm->mem[vm->sp - 1];
            ++vm->sp;
        break;
        case 0x06: /* sub */
            vm->mem[vm->sp + 1] = vm->mem[vm->sp - 1] - vm->mem[vm->sp];
            ++vm->sp;
        break;
        case 0x07: /* mul */
            vm->mem[vm->sp + 1] = vm->mem[vm->sp] * vm->mem[vm->sp - 1];
            ++vm->sp;
        break;
        case 0x08: /* div */
            vm->mem[vm->sp + 1] = vm->mem[vm->sp - 1] / vm->mem[vm->sp];
            ++vm->sp;
        break;
        case 0x09: /* outc */
            printf("%c", vm->mem[vm->sp]);
        break;
        case 0x0A: /* inpc */
            vm->mem[++vm->sp] = getchar();
        break;
    }
}

void run_vm(Vm *vm) {
    while(vm->running) {
        fetch(vm);
        decode(vm);
        execute(vm);
    }
}

Answer 1

Missing Error Checking

I usually use c++ so it may not be best practice for c.

In C++ when memory allocation fails in new an exception is thrown, this is not the case in the C programming language when using malloc(), calloc() or realloc(). An additional check is required after any memory allocation call. The check is to see if the memory returned is NULL or not, if the allocation fails references through the pointer are Unknown Behavior.

Vm *new_vm() {
    Vm *ret = (Vm*)malloc(sizeof(Vm));
    if (!ret)
    {
        fprintf(stderr, "Allocation of the Virtual Machine failed.\n");
        return ret;
    }
    
    ret->pc = 1024; /* add space for the stack */
    ret->sp = -1;
    ret->running = true;
    return ret;
}

In main():

#include <stdio.h>
#include <stdlib.h>
#include "vm.h"

int main() {
    Vm *vm = new_vm();
    if (vm == NULL)
    {
        fprintf(stderr, "Exiting Toy Virtual Machine.\n");
        return EXIT_FAILURE;
    }
    i32 buffer[] = {
            0x00000A01, /* push 0x0A(\n) */
            0x00004301, /* push 0x43(C)  */
            0x00004201, /* push 0x42(B)  */
            0x00004101, /* push 0x41(A)  */
            0x00000009, /* output */
            0x00000002, /* pop */
            0x00000009,
            0x00000002,
            0x00000009,
            0x00000002,
            0x00000009,
            0x00000000  /* halt */
    };
    for (int i = 0; i < sizeof(buffer); i++) {
        vm->mem[vm->pc+i] = buffer[i];
    }
    run_vm(vm);
    free_vm(vm);
    return EXIT_SUCCESS;
}

Include Only Necessary Headers
In the code as posted, stdlib.h is included in vm.h, stdlib.h is not necessary in main(), only in vm.c. To reduce the scope of the header files and source files only include what is needed. Amoung other things, this will reduce compile times, and may also reduce linking problems.

Missing Default Case in Switch Statement
It is generally a good programming practice to have a default : case statement in a switch statement to handle cases that haven't been specified yet:

static void execute(Vm *vm) {
    switch(vm->opc) {
        default:
            fprintf(stderr, "Unknown Opcode in execute(). 0x%x\n", vm->opc);
            return;
        case 0x00: /* halt */
            vm->running = false;
            printf("Halt\n");
            break;
        ...
    }

Then all possible paths through the function have been implemented. This is true in either C or C++ and most other programming languages that have a switch statement.

Use an ENUM for the Opcodes
The code would be much more readable if less numeric constants and more symbolic constants were used. In C there are 2 ways to do this, to create single symbolic constants use macro definition

#define SYMBOL  VALUE

or to use enums

typedef enum {
    HALT = 0x00,
    PUSH = 0x01,
    POP = 0x02,
    ...
    INPUTCHAR = 0x0A
} OPCODE;


typedef struct {
    i32 mem[0xffff]; /* approx. 64k */
    si32 sp;
    i32 pc;

    i32 raw;
    OPCODE opc;
    i32 param;

    bool running;
} Vm;

Use Unsigned Types as Indexes
The stack pointer index is currently a signed integer and is initialized to -1, this is what I consider to be a bad practice since stack[-1] will cause Unknown Behavior. It would be better to use size_t or unsigned as the stack pointer index. This will force a change in a number of areas, but here is what I would recommend:

1. Initialize running to false rather than true.
2. Initialize sp to zero
3. Only index the stack and increment the stack pointer if running is true
4. At the beginning of run_vm(Vm *vm) before the loop set running to true
5. Change the implementation of reset_vm(Vm *vm) to match all of the above

Type Names and Variable Names
Initially I was confused about Vm, whether it was a Virtual Memory or a Virtual Machine, this was true of other variables and types as well. Well written code is self documenting and doesn't need a lot of comments, type names and variable names play a big part in this. In my opinion Vm should be renamed VirtualMachine, sp should be renamed StackPointer, pc should be renamed ProgramCounter etc.

Use Library Functions Where Available
C++ contains std::memset(), and memset() in C predates C++. The function reset_vm() should use memset() rather than the loop it is using to reset the memory. The function memset() should be faster than the current loop.

void reset_vm(Vm *vm) {
    vm->running = true;
    memset(&vm->mem[0], 0, sizeof(*vm->mem[0]) * 0xffff);
    vm->sp = -1;
    vm->pc = 1024;
}

Answer 2

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

Be careful with array lengths

The memory allocation for the virtual machine is currently this:

i32 mem[0xffff]; /* approx. 64k */

While there is no intrinsic problem with this declaration or the comment, it means that memory locations 0 through 0xfffe are valid, but memory location 0xffff is not. That's not inconsistent with the code, but it's an odd choice. Also, see the next suggestion.

Avoid magic numbers

As mentioned above, the number 0xffff is used in both the header and in the .c file. Because it's quite important, I'd suggest that it would be better if it were a named value. Similarly, such numbers as 1024 could be named constants. So if, for example, you wanted to change the memory size to be exactly 64K, it would be easier if you only had to change the value in one place rather than hunting for the constant in multiple places in the code and then having to decide whether this particular 0xffff referred to the memory size or something else.

Provide a default case

What happens if the VM encounters an unknown instruction? At the moment it's silently ignored. It might be better to flag it as an error and one way to accomplish that nicely would be to add a default case to the switch statement.

Pass the buffer to the VM directly

Instead of having main reach into the VM and manipulate its internal data directly, I'd suggest a better approach might be to provide a version of new_vm() that takes a pointer and length so that it can do the copying instead.

Consider adding flags

Real processors typically have a set of flags, such as a Zero or Negative flag, as well as Overflow and Carry. As you expand your virtual machine, you will find those additions important as you start adding things such as conditional jumps or looping instructions.

Consider a data-centric approach

The code is generally clear and easy to read and understand as it is written. That's great! I would suggest that it might be easier to keep that readability as the code is enhanced and expanded if the opcodes and operations are structured into an array of data. This is likely to make it easier to add or modify instructions and to write assembler and disassembler enhancements if you're interested in doing that. The current approach, however, has the advantage of lookup speed for instructions since typical compilers generate very efficient code for switch statements.

Let the compiler generate code

It's not wrong to put return 0; at the end of main and some people prefer it for stylistic reasons. I prefer to omit it since it's guaranteed that the compiler will generate the equivalent code by itself.