NES Emulator in C

Question 1

I've been coding an NES emulator for a couple of weeks, and would like to have some feedback on what I have written so far. I don't have any experience in C other than a few very small programs, so I will probably have messed something up.

My attempt is to make a cycle accurate emulator, and I have decided to go with an array of instructions. Technically a JIT compiler would be more efficient but I don't feel confident enough to do that yet. Still, here is my code.

Memory implementation is pretty straight forward. I made several arrays for different memory regions and made some functions to access them.

mem.h:

#pragma once
#include <defines.h>
#define RAM_SIZE 0x800
#define PPU_SIZE 0x08
#define APU_IO_SIZE 0x18
#define CART_SIZE 0xBFBF
typedef struct {
 u8 ram[RAM_SIZE]; // 0x0000 - 0x1FFF
 u8 ppu[PPU_SIZE]; // 0x2000 - 0x3FFF
 u8 apu_io[APU_IO_SIZE]; // 0x4000 - 0x401F
 
 u8 cart[CART_SIZE]; // 0x4020 - 0xFFFF
 bool is16kb;
} Memory;
Memory* memory_new(void);
void memory_free(Memory* m);
u8 memory_read(Memory* m, u16 a);
void memory_write(Memory* m, u16 a, u8 v);
void memory_push(Memory* m, u8* sp, u8 v);
u8 memory_pop(Memory* m, u8* sp);
void load_rom(Memory* m, const char* path);

mem.c:

#include <emu/mem.h>
#include <stdlib.h>
#include <stdio.h>
#include <string.h>
#define HEADER_SIZE 16
#define PRG_ROM_SIZE 0x4000
#define CHR_ROM_SIZE 0x2000
#define ROM_START 0x8000
Memory* memory_new(void) {
 Memory* m = malloc(sizeof(Memory));
 memset(m, 0, sizeof(Memory));
 m->is16kb = false;
 return m;
}
void memory_free(Memory* m) { 
 if (m)
 free(m); 
 m = NULL;
}
u8 memory_read(Memory* m, u16 a) {
 switch(a) {
 case 0x0000 ... 0x1FFF:
 return m->ram[a % RAM_SIZE];
 case 0x2000 ... 0x3FFF:
 return m->ppu[a % PPU_SIZE];
 case 0x4000 ... 0x401F:
 return m->apu_io[a % APU_IO_SIZE];
 case 0x4020 ... 0xFFFF:
 if (m->is16kb && a >= 0xC000) {
 return m->cart[(a - 0x8000) % 0x4000]; // Mirror 0x8000 - 0xBFFF to 0xC000 - 0xFFFF
 }
 return m->cart[a - 0x4020];
 }
 return 0;
}
void memory_write(Memory* m, u16 a, u8 v) {
 switch(a) {
 case 0x0000 ... 0x1FFF:
 m->ram[a % RAM_SIZE] = v;
 break;
 case 0x2000 ... 0x3FFF:
 m->ppu[a % PPU_SIZE] = v;
 break;
 case 0x4000 ... 0x401F:
 m->apu_io[a % APU_IO_SIZE] = v;
 break;
 case 0x4020 ... 0xFFFF:
 if (m->is16kb && a >= 0xC000) {
 m->cart[(a - 0x8000) % 0x4000] = v; // Mirror 0x8000 - 0xBFFF to 0xC000 - 0xFFFF
 } else {
 m->cart[a - 0x4020] = v;
 }
 break;
 }
}
void memory_push(Memory* m, u8* sp, u8 v) {
 u16 addr = 0x0100 | *sp;
 memory_write(m, addr, v);
 *sp -= 1;
}
u8 memory_pop(Memory* m, u8* sp) {
 *sp += 1;
 u16 addr = 0x0100 | *sp;
 return memory_read(m, addr);
}
void load_rom(Memory* m, const char* path) {
 FILE* file = fopen(path, "rb");
 if (!file) {
 printf("Could not open ROM at path: %s", path);
 return;
 }
 uint8_t header[HEADER_SIZE];
 if (fread(header, 1, HEADER_SIZE, file) != HEADER_SIZE) {
 fprintf(stderr, "Error: Could not read iNES header from %s\n", path);
 fclose(file);
 return;
 }
 if (header[0] != 'N' || header[1] != 'E' || header[2] != 'S' || header[3] != 0x1A) {
 fprintf(stderr, "Error: Invalid iNES header in %s\n", path);
 fclose(file);
 return;
 }
 uint8_t prg_rom_size = header[4]; // Size of PRG ROM in 16KB units
 uint8_t chr_rom_size = header[5]; // Size of CHR ROM in 8KB units
 size_t prg_rom_bytes = prg_rom_size * PRG_ROM_SIZE;
 uint8_t* prg_rom_data = malloc(prg_rom_bytes);
 if (fread(prg_rom_data, 1, prg_rom_bytes, file) != prg_rom_bytes) {
 fprintf(stderr, "Error: Could not read PRG ROM data from %s\n", path);
 free(prg_rom_data);
 fclose(file);
 return;
 }
 memcpy(m->cart + (ROM_START - 0x4020), prg_rom_data, prg_rom_bytes);
 if (prg_rom_size == 1) {
 m->is16kb = true;
 memcpy(m->cart + (ROM_START - 0x4020 + PRG_ROM_SIZE), prg_rom_data, PRG_ROM_SIZE);
 }
 free(prg_rom_data);
 fclose(file);
}

The cpu itself is also pretty straight forward. The way that I tried to use the instructions is to put the array inside the cpu struct itself, but I don't know if that's the best way forward. Still its working for now.

cpu.h:

#pragma once
#include <defines.h>
#include "mem.h"
typedef enum {
 REG_A = 0,
 REG_X = 1,
 REG_Y = 2,
 REG_SP = 3,
 REG_P = 4
} CpuReg;
typedef enum {
 MODE_IMM,
 MODE_ZPG,
 MODE_ABS,
 MODE_IMP,
 MODE_REL
} AddrMode;
typedef enum {
 COND_PL = 0, COND_MI = 1,
 COND_CC = 2, COND_CS = 3,
 COND_NE = 4, COND_EQ = 5,
 COND_VC = 6, COND_VS = 7,
 // More to be added later (?)
} BranchCond;
typedef enum {
 FLAG_C = 0,
 FLAG_Z = 1,
 FLAG_I = 2,
 FLAG_D = 3,
 FLAG_B = 4,
 FLAG_1 = 5, // Always 1
 FLAG_V = 6,
 FLAG_N = 7
} CpuFlag;
typedef struct {
 u8 regs[5];
 u16 pc;
 u16 temp;
 u8 current_opcode;
 void** instructions; // Fill with Instruction instances
} Cpu;
Cpu* cpu_new(Memory* m);
void cpu_free(Cpu* c);
u8 cpu_get_reg(Cpu* c, CpuReg r); 
void cpu_set_reg(Cpu* c, CpuReg r, u8 v); 
bool cpu_get_flag(Cpu* c, CpuFlag f);
void cpu_set_flag(Cpu* c, CpuFlag f, bool v);
void cpu_test_flag(Cpu* c, CpuFlag f, u8 v);
void cpu_log_instr(Cpu* c, u8 opcode);
bool cpu_step(Cpu* c, Memory* m);

cpu.c:

#include <emu/cpu.h>
#include <emu/instructions.h>
#include <stdio.h>
#include <stdlib.h>
Instruction** instructions = NULL;
Instruction* current_instruction = NULL;
bool first = true;
const char* INSTR_STR[0x100] = {
 "BRK ", "ORA (IND,X)", "KIL ", "SLO (IND,X)", "NOP ZPG ", "ORA ZPG ", "ASL ZPG ", "SLO ZPG ", 
 "PHP ", "ORA IMM ", "ASL A ", "ANC IMM ", "NOP ABS ", "ORA ABS ", "ASL ABS ", "SLO ABS ", 
 "BPL REL ", "ORA (IND),Y", "KIL ", "SLO (IND),Y", "NOP ZPG,X ", "ORA ZPG,X ", "ASL ZPG,X ", "SLO ZPG,X ", 
 "CLC ", "ORA ABS,Y ", "NOP ", "SLO ABS,Y ", "NOP ABS,X ", "ORA ABS,X ", "ASL ABS,X ", "SLO ABS,X ", 
 "JSR ABS ", "AND (IND,X)", "KIL ", "RLA (IND,X)", "BIT ZPG ", "AND ZPG ", "ROL ZPG ", "RLA ZPG ", 
 "PLP ", "AND IMM ", "ROL A ", "ANC IMM ", "BIT ABS ", "AND ABS ", "ROL ABS ", "RLA ABS ", 
 "BMI REL ", "AND (IND),Y", "KIL ", "RLA (IND),Y", "NOP ZPG,X ", "AND ZPG,X ", "ROL ZPG,X ", "RLA ZPG,X ", 
 "SEC ", "AND ABS,Y ", "NOP ", "RLA ABS,Y ", "NOP ABS,X ", "AND ABS,X ", "ROL ABS,X ", "RLA ABS,X ", 
 "RTI ", "EOR (IND,X)", "KIL ", "SRE (IND,X)", "NOP ZPG ", "EOR ZPG ", "LSR ZPG ", "SRE ZPG ", 
 "PHA ", "EOR IMM ", "LSR A ", "ALR IMM ", "JMP ABS ", "EOR ABS ", "LSR ABS ", "SRE ABS ", 
 "BVC REL ", "EOR (IND),Y", "KIL ", "SRE (IND),Y", "NOP ZPG,X ", "EOR ZPG,X ", "LSR ZPG,X ", "SRE ZPG,X ", 
 "CLI ", "EOR ABS,Y ", "NOP ", "SRE ABS,Y ", "NOP ABS,X ", "EOR ABS,X ", "LSR ABS,X ", "SRE ABS,X ", 
 "RTS ", "ADC (IND,X)", "KIL ", "RRA (IND,X)", "NOP ZPG ", "ADC ZPG ", "ROR ZPG ", "RRA ZPG ", 
 "PLA ", "ADC IMM ", "ROR A ", "ARR IMM ", "JMP (IND) ", "ADC ABS ", "ROR ABS ", "RRA ABS ", 
 "BVS REL ", "ADC (IND),Y", "KIL ", "RRA (IND),Y", "NOP ZPG,X ", "ADC ZPG,X ", "ROR ZPG,X ", "RRA ZPG,X ", 
 "SEI ", "ADC ABS,Y ", "NOP ", "RRA ABS,Y ", "NOP ABS,X ", "ADC ABS,X ", "ROR ABS,X ", "RRA ABS,X ", 
 "NOP IMM ", "STA (IND,X)", "NOP IMM ", "SAX (IND,X)", "STY ZPG ", "STA ZPG ", "STX ZPG ", "SAX ZPG ", 
 "DEY ", "NOP IMM ", "TXA ", "XAA IMM ", "STY ABS ", "STA ABS ", "STX ABS ", "SAX ABS ", 
 "BCC REL ", "STA (IND),Y", "KIL ", "AHX (IND),Y", "STY ZPG,X ", "STA ZPG,X ", "STX ZPG,Y ", "SAX ZPG,Y ", 
 "TYA ", "STA ABS,Y ", "TXS ", "TAS ABS,Y ", "SHY ABS,X ", "STA ABS,X ", "SHX ABS,Y ", "AHX ABS,Y ", 
 "LDY IMM ", "LDA (IND,X)", "LDX IMM ", "LAX (IND,X)", "LDY ZPG ", "LDA ZPG ", "LDX ZPG ", "LAX ZPG ", 
 "TAY ", "LDA IMM ", "TAX ", "LAX IMM ", "LDY ABS ", "LDA ABS ", "LDX ABS ", "LAX ABS ", 
 "BCS REL ", "LDA (IND),Y", "KIL ", "LAX (IND),Y", "LDY ZPG,X ", "LDA ZPG,X ", "LDX ZPG,Y ", "LAX ZPG,Y ", 
 "CLV ", "LDA ABS,Y ", "TSX ", "LAS ABS,Y ", "LDY ABS,X ", "LDA ABS,X ", "LDX ABS,Y ", "LAX ABS,Y ", 
 "CPY IMM ", "CMP (IND,X)", "NOP IMM ", "DCP (IND,X)", "CPY ZPG ", "CMP ZPG ", "DEC ZPG ", "DCP ZPG ", 
 "INY ", "CMP IMM ", "DEX ", "AXS IMM ", "CPY ABS ", "CMP ABS ", "DEC ABS ", "DCP ABS ", 
 "BNE REL ", "CMP (IND),Y", "KIL ", "DCP (IND),Y", "NOP ZPG,X ", "CMP ZPG,X ", "DEC ZPG,X ", "DCP ZPG,X ", 
 "CLD ", "CMP ABS,Y ", "NOP ", "DCP ABS,Y ", "NOP ABS,X ", "CMP ABS,X ", "DEC ABS,X ", "DCP ABS,X ", 
 "CPX IMM ", "SBC (IND,X)", "NOP IMM ", "ISC (IND,X)", "CPX ZPG ", "SBC ZPG ", "INC ZPG ", "ISC ZPG ", 
 "INX ", "SBC IMM ", "NOP ", "SBC IMM ", "CPX ABS ", "SBC ABS ", "INC ABS ", "ISC ABS ", 
 "BEQ REL ", "SBC (IND),Y", "KIL ", "ISC (IND),Y", "NOP ZPG,X ", "SBC ZPG,X ", "INC ZPG,X ", "ISC ZPG,X ", 
 "SED ", "SBC ABS,Y ", "NOP ", "ISC ABS,Y ", "NOP ABS,X ", "SBC ABS,X ", "INC ABS,X ", "ISC ABS,X "
};
bool logged = false;
Cpu* cpu_new(Memory* m) {
 Cpu* c = malloc(sizeof(Cpu));
 c->regs[REG_A] = 0;
 c->regs[REG_X] = 0;
 c->regs[REG_Y] = 0;
 c->regs[REG_SP] = 0xFD;
 c->regs[REG_P] = 0;
 cpu_set_flag(c, FLAG_I, true);
 cpu_set_flag(c, FLAG_1, true);
 c->pc = 0;
 c->temp = 0;
 c->current_opcode = 0;
 c->instructions = (void**)initialize_instructions(c, m);
 instructions = (Instruction**)c->instructions;
 return c;
}
void cpu_free(Cpu* c) {
 if (c->instructions) {
 free(c->instructions);
 c->instructions = NULL;
 }
 free(c);
 c = NULL;
}
u8 cpu_get_reg(Cpu* c, CpuReg r) {
 return c->regs[r];
}
void cpu_set_reg(Cpu* c, CpuReg r, u8 v) {
 c->regs[r] = v;
}
bool cpu_get_flag(Cpu* c, CpuFlag f) {
 return (c->regs[REG_P] >> f) & 1;
}
void cpu_set_flag(Cpu* c, CpuFlag f, bool v) {
 if (v) c->regs[REG_P] |= (1 << f);
 else c->regs[REG_P] &= ~(1 << f);
}
void cpu_test_flag(Cpu* c, CpuFlag f, u8 v) {
 switch (f) {
 case FLAG_Z:
 cpu_set_flag(c, FLAG_Z, v == 0);
 break;
 case FLAG_N:
 cpu_set_flag(c, FLAG_N, (v >> 7) & 1);
 break;
 case FLAG_V:
 break;
 default:
 break;
 }
}
void cpu_log_instr(Cpu* c, u8 opcode) {
 printf("%s - PC: %04X - A: %02X - X: %02X - Y: %02X - P: %02X - SP: %02X\n", INSTR_STR[opcode], c->pc, cpu_get_reg(c, REG_A), cpu_get_reg(c, REG_X), cpu_get_reg(c, REG_Y), cpu_get_reg(c, REG_P), cpu_get_reg(c, REG_SP));
}
bool cpu_step(Cpu* c, Memory* m) {
 if (first) {
 c->current_opcode = memory_read(m, c->pc);
 current_instruction = instructions[c->current_opcode];
 first = false;
 return true;
 }
 if (!current_instruction) {
 printf("-- INSTRUCTION %02X UNINITIALIZED (%04X) --\n", c->current_opcode, c->pc);
 return false;
 }
 if (instr_step(current_instruction)) {
 c->current_opcode = memory_read(m, c->pc);
 current_instruction = instructions[c->current_opcode];
 // printf("OP: %02X\n", c->current_opcode);
 return true;
 }
 return true;
}

This is how I implemented instructions. Working with pointers has been tricky but it works. Still I feel like I may be using stuff incorrectly/inefficiently, so I'd appreciate any help with how to handle these.

instructions.h:

#pragma once
#include "cpu.h"
#include "mem.h"
// Instruction*, aux
typedef void (*CycleOp)(void*, void*);
typedef struct {
 CycleOp* operations; 
 Cpu* cpu_ref;
 Memory* mem_ref;
 void* aux;
 u8 current_cycle;
 u8 expected_cycles;
 AddrMode mode;
} Instruction;
Instruction** initialize_instructions(Cpu* c, Memory* m);
bool instr_step(Instruction* instr);
// COMMON
void instr_empty(Instruction* instr, void* aux); 
void instr_fetch(Instruction* instr, void* aux); 
void instr_ld_temp_inc(Instruction* instr, void* reg);
void instr_inc_pc(Instruction* instr, void* aux);
// LD IMM
void instr_ld_imm(Instruction* instr, void* reg);
// ST ZPG
void instr_st_zpg(Instruction* instr, void* reg);
// JMP ABS
void instr_jmp_abs(Instruction* instr, void* reg);
// SUBROUTINE
void instr_push_hi_pc(Instruction* instr, void* aux);
void instr_push_lo_pc(Instruction* instr, void* aux);
void instr_jump_subroutine(Instruction* instr, void* aux);
void instr_pop_lo_pc(Instruction* instr, void* aux);
void instr_pop_hi_pc(Instruction* instr, void* aux);
// BRN REL
void instr_brn_cond(Instruction* instr, void* cond);
// FLAGS
void instr_set_flag(Instruction* instr, void* flag);
void instr_clear_flag(Instruction* instr, void* flag);
// LOGIC
void instr_test_bit(Instruction* instr, void* aux);
void instr_and_acc(Instruction* instr, void* aux);
// STACK
void instr_push_r_stack(Instruction* instr, void* reg);
void instr_pop_r_stack(Instruction* instr, void* reg);

instructions.c:

#include <emu/instructions.h>
#include <stdio.h>
#include <stdlib.h>
#include <stdarg.h>
#define INSTRUCTION_SET_SIZE 0x100
static Instruction* instruction_new(CycleOp* ops, u8 e, AddrMode mode, Cpu* c, Memory* mem, void* aux) {
 Instruction* instr = (Instruction*)malloc(sizeof(Instruction));
 if (!instr) {
 printf("Instruction initialization failed\n");
 return NULL;
 }
 instr->operations = ops;
 instr->cpu_ref = c;
 instr->mem_ref = mem;
 instr->aux = aux;
 instr->current_cycle = 0;
 instr->expected_cycles = e;
 instr->mode = mode;
 return instr;
}
static CycleOp* create_cycle_ops(int count, ...) {
 CycleOp* ops = malloc(sizeof(CycleOp) * count);
 if (!ops) {
 printf("Failed to allocate memory for CycleOp array\n");
 return NULL;
 }
 va_list args;
 va_start(args, count);
 for (int i = 0; i < count; i++) {
 ops[i] = va_arg(args, CycleOp);
 }
 va_end(args);
 return ops;
}
Instruction** initialize_instructions(Cpu* c, Memory* m) {
 Instruction** instructions = (Instruction**)malloc(sizeof(Instruction*) * INSTRUCTION_SET_SIZE);
 if (!instructions) {
 printf("Failed to initialize instructions\n");
 return NULL;
 }
 for (int i = 0; i < INSTRUCTION_SET_SIZE; i++)
 instructions[i] = NULL;
 // Auxiliaries
 static CpuReg Tgt_A = REG_A;
 static CpuReg Tgt_X = REG_X;
 static CpuReg Tgt_P = REG_P;
 static BranchCond Tgt_PL = COND_PL;
 static BranchCond Tgt_NE = COND_NE;
 static BranchCond Tgt_EQ = COND_EQ;
 static BranchCond Tgt_CC = COND_CC;
 static BranchCond Tgt_CS = COND_CS;
 static BranchCond Tgt_VC = COND_VC;
 static BranchCond Tgt_VS = COND_VS;
 static CpuFlag Tgt_C = FLAG_C;
 static CpuFlag Tgt_I = FLAG_I;
 static CpuFlag Tgt_D = FLAG_D;
 // INSTRUCTION INITIALIZATION
 
 // NOP
 CycleOp* nop_ops = create_cycle_ops(2, instr_fetch, instr_empty); 
 instructions[0xEA] = instruction_new(nop_ops, 2, MODE_IMP, c, m, NULL);
 // LD 
 CycleOp* ld_imm_ops = create_cycle_ops(2, instr_fetch, instr_ld_imm);
 instructions[0xA9] = instruction_new(ld_imm_ops, 2, MODE_IMM, c, m, &Tgt_A);
 instructions[0xA2] = instruction_new(ld_imm_ops, 2, MODE_IMM, c, m, &Tgt_X);
 // STORE
 CycleOp* st_zpg_ops = create_cycle_ops(3, instr_fetch, instr_ld_temp_inc, instr_st_zpg);
 instructions[0x86] = instruction_new(st_zpg_ops, 3, MODE_ZPG, c, m, &Tgt_X);
 instructions[0x85] = instruction_new(st_zpg_ops, 3, MODE_ZPG, c, m, &Tgt_A);
 // JUMP
 CycleOp* jmp_abs_ops = create_cycle_ops(3, instr_fetch, instr_ld_temp_inc, instr_jmp_abs);
 instructions[0x4C] = instruction_new(jmp_abs_ops, 3, MODE_ABS, c, m, NULL);
 // SUBROUTINE
 CycleOp* jsr_abs_ops = create_cycle_ops(6, instr_fetch, instr_ld_temp_inc, instr_empty, instr_push_hi_pc, instr_push_lo_pc, instr_jump_subroutine);
 instructions[0x20] = instruction_new(jsr_abs_ops, 6, MODE_ABS, c, m, NULL);
 CycleOp* rts_imp_ops = create_cycle_ops(6, instr_fetch, instr_empty, instr_empty, instr_pop_lo_pc, instr_pop_hi_pc, instr_inc_pc);
 instructions[0x60] = instruction_new(rts_imp_ops, 6, MODE_IMP, c, m, NULL);
 // BRANCH
 CycleOp* brn_rel_ops = create_cycle_ops(4, instr_fetch, instr_brn_cond, instr_empty, instr_empty);
 instructions[0x10] = instruction_new(brn_rel_ops, 2, MODE_REL, c, m, &Tgt_PL);
 instructions[0xD0] = instruction_new(brn_rel_ops, 2, MODE_REL, c, m, &Tgt_NE);
 instructions[0xF0] = instruction_new(brn_rel_ops, 2, MODE_REL, c, m, &Tgt_EQ);
 instructions[0x90] = instruction_new(brn_rel_ops, 2, MODE_REL, c, m, &Tgt_CC);
 instructions[0xB0] = instruction_new(brn_rel_ops, 2, MODE_REL, c, m, &Tgt_CS);
 instructions[0x50] = instruction_new(brn_rel_ops, 2, MODE_REL, c, m, &Tgt_VC);
 instructions[0x70] = instruction_new(brn_rel_ops, 2, MODE_REL, c, m, &Tgt_VS);
 // FLAGS
 CycleOp* set_flag_ops = create_cycle_ops(2, instr_fetch, instr_set_flag); 
 instructions[0x38] = instruction_new(set_flag_ops, 2, MODE_IMP, c, m, &Tgt_C);
 instructions[0x78] = instruction_new(set_flag_ops, 2, MODE_IMP, c, m, &Tgt_I);
 instructions[0xF8] = instruction_new(set_flag_ops, 2, MODE_IMP, c, m, &Tgt_D);
 CycleOp* clear_flag_ops = create_cycle_ops(2, instr_fetch, instr_clear_flag); 
 instructions[0x18] = instruction_new(clear_flag_ops, 2, MODE_IMP, c, m, &Tgt_C);
 // LOGIC
 CycleOp* test_bits_ops = create_cycle_ops(3, instr_fetch, instr_ld_temp_inc, instr_test_bit);
 instructions[0x24] = instruction_new(test_bits_ops, 3, MODE_ZPG, c, m, NULL);
 CycleOp* and_imm_ops = create_cycle_ops(2, instr_fetch, instr_and_acc);
 instructions[0x29] = instruction_new(and_imm_ops, 2, MODE_IMM, c, m, NULL);
 // STACK
 CycleOp* push_r_stack = create_cycle_ops(3, instr_fetch, instr_empty, instr_push_r_stack);
 instructions[0x08] = instruction_new(push_r_stack, 3, MODE_IMP, c, m, &Tgt_P);
 CycleOp* pop_r_stack = create_cycle_ops(3, instr_fetch, instr_empty, instr_pop_r_stack);
 instructions[0x68] = instruction_new(pop_r_stack, 3, MODE_IMP, c, m, &Tgt_A);
 return instructions;
}
bool instr_step(Instruction* instr) {
 if (!instr) {
 printf("UNINITIALIZED INSTRUCTION\n");
 return false;
 }
 if (instr->current_cycle == instr->expected_cycles) {
 instr->current_cycle = 0;
 return true;
 }
 if (!instr->operations[instr->current_cycle]) {
 printf("Trying to execute empty cycle op\n");
 exit(1);
 }
 instr->operations[instr->current_cycle++](instr, instr->aux);
 return false;
}
// COMMON
void instr_empty(Instruction* instr, void* aux) {
 return;
}
void instr_fetch(Instruction* instr, void* aux) {
 instr->cpu_ref->current_opcode = memory_read(instr->mem_ref, instr->cpu_ref->pc);
 cpu_log_instr(instr->cpu_ref, instr->cpu_ref->current_opcode); 
 instr->cpu_ref->pc++;
}
void instr_ld_temp_inc(Instruction* instr, void* reg) {
 instr->cpu_ref->temp = memory_read(instr->mem_ref, instr->cpu_ref->pc++);
}
void instr_inc_pc(Instruction* instr, void* aux) {
 instr->cpu_ref->pc++;
}
// LOAD
void instr_ld_imm(Instruction* instr, void* reg) {
 CpuReg r = *(CpuReg*)reg;
 u8 val = memory_read(instr->mem_ref, instr->cpu_ref->pc++);
 cpu_set_reg(instr->cpu_ref, r, val);
 cpu_test_flag(instr->cpu_ref, FLAG_Z, val);
 cpu_test_flag(instr->cpu_ref, FLAG_N, val);
}
// STORE
void instr_st_zpg(Instruction* instr, void* reg) {
 CpuReg r = *(CpuReg*)reg;
 memory_write(instr->mem_ref, instr->cpu_ref->temp & 0x00FF, cpu_get_reg(instr->cpu_ref, r));
}
// JUMP
void instr_jmp_abs(Instruction* instr, void* reg) {
 instr->cpu_ref->temp |= (memory_read(instr->mem_ref, instr->cpu_ref->pc) << 8);
 instr->cpu_ref->pc = instr->cpu_ref->temp;
}
// JSR
void instr_push_hi_pc(Instruction* instr, void* aux) {
 memory_push(instr->mem_ref, &instr->cpu_ref->regs[REG_SP], instr->cpu_ref->pc >> 8);
}
void instr_push_lo_pc(Instruction* instr, void* aux) {
 memory_push(instr->mem_ref, &instr->cpu_ref->regs[REG_SP], instr->cpu_ref->pc & 0x00FF);
}
void instr_jump_subroutine(Instruction* instr, void* aux) {
 instr->cpu_ref->temp |= (u16)(memory_read(instr->mem_ref, instr->cpu_ref->pc) << 8);
 instr->cpu_ref->pc = instr->cpu_ref->temp;
}
void instr_pop_lo_pc(Instruction* instr, void* aux) {
 instr->cpu_ref->temp = memory_pop(instr->mem_ref, &instr->cpu_ref->regs[REG_SP]);
}
void instr_pop_hi_pc(Instruction* instr, void* aux) {
 instr->cpu_ref->temp |= (u16)(memory_pop(instr->mem_ref, &instr->cpu_ref->regs[REG_SP]) << 8);
 instr->cpu_ref->pc = instr->cpu_ref->temp;
}
// BRANCH
enum CondType {
 SET = 0, CLEAR = 1
};
CpuFlag flag_map[8] = {FLAG_N, FLAG_N, FLAG_C, FLAG_C, FLAG_Z, FLAG_Z, FLAG_V, FLAG_V};
void instr_brn_cond(Instruction* instr, void* cond) {
 BranchCond bc = *(BranchCond*)cond;
 enum CondType ct = bc % 2 == 0 ? CLEAR : SET;
 CpuFlag f = flag_map[bc];
 u8 curr_hi = instr->cpu_ref->pc >> 8;
 instr->cpu_ref->temp = memory_read(instr->mem_ref, instr->cpu_ref->pc++);
 if (ct == SET ? cpu_get_flag(instr->cpu_ref, f) : !cpu_get_flag(instr->cpu_ref, f)) {
 instr->cpu_ref->pc += instr->cpu_ref->temp;
 instr->expected_cycles++;
 if ((instr->cpu_ref->pc >> 8) != curr_hi) instr->expected_cycles++;
 }
}
// FLAGS
void instr_set_flag(Instruction* instr, void* flag) {
 CpuFlag f = *(CpuFlag*)flag;
 cpu_set_flag(instr->cpu_ref, f, true);
}
void instr_clear_flag(Instruction* instr, void* flag) {
 CpuFlag f = *(CpuFlag*)flag;
 cpu_set_flag(instr->cpu_ref, f, false);
}
// LOGIC
void instr_test_bit(Instruction* instr, void* aux) {
 u8 val = cpu_get_reg(instr->cpu_ref, REG_A) & memory_read(instr->mem_ref, instr->cpu_ref->temp);
 cpu_test_flag(instr->cpu_ref, FLAG_N, val);
 cpu_test_flag(instr->cpu_ref, FLAG_Z, val);
 cpu_set_flag(instr->cpu_ref, FLAG_V, (val >> 6) & 1);
}
void instr_and_acc(Instruction* instr, void* aux) {
 u8 val = cpu_get_reg(instr->cpu_ref, REG_A) & memory_read(instr->mem_ref, instr->cpu_ref->pc++);
 cpu_test_flag(instr->cpu_ref, FLAG_Z, val);
 cpu_test_flag(instr->cpu_ref, FLAG_N, val);
 cpu_set_reg(instr->cpu_ref, REG_A, val);
}
// STACK
void instr_push_r_stack(Instruction* instr, void* reg) {
 CpuReg r = *(CpuReg*)reg;
 u8 val = cpu_get_reg(instr->cpu_ref, r);
 if (instr->cpu_ref->current_opcode == 0x08) val |= (1 << FLAG_B);
 // !!! If an interrupt set to false
 memory_push(instr->mem_ref, &instr->cpu_ref->regs[REG_SP], val);
}
void instr_pop_r_stack(Instruction* instr, void* reg) {
 CpuReg r = *(CpuReg*)reg;
 u8 val = memory_pop(instr->mem_ref, &instr->cpu_ref->regs[REG_SP]);
 cpu_test_flag(instr->cpu_ref, FLAG_Z, val);
 cpu_test_flag(instr->cpu_ref, FLAG_N, val);
 cpu_set_reg(instr->cpu_ref, r, val);
}

I have also tried making a debugger which I can start by giving specific command line arguments, and which allows me to take a better look at the program during execution.I also plan on putting a breakpoint system in the debugger, but I'll leave that for when I have the CPU done at least.

debugger.h:

#pragma once
#include <emu/emu.h>
#include <SDL2/SDL.h>
#include <SDL_ttf.h>
#define FONT_SIZE 18
// TEXT
typedef struct {
 char* text;
 SDL_Texture* texture;
 SDL_Rect rect;
 SDL_Color bg_color;
 SDL_Color fg_color;
} Text;
Text* text_new(const char* text, SDL_Color bg, SDL_Color fg, int x, int y);
void text_display(Text* t, TTF_Font* f, SDL_Renderer* r);
void text_free(Text* t);
// DEBUGGER
typedef enum {
 MEMORY_TAB = 0,
 CPU_TAB = 1,
 DISASSEMBLY_TAB = 2,
} ViewTab;
typedef struct {
 Emu* emu_info;
 SDL_Window* window;
 SDL_Renderer* renderer;
 TTF_Font* font;
 ViewTab current_tab;
 u16 current_address;
 bool input_mode;
 char input_buffer[5]; 
 int input_length;
} Debugger;
Debugger* debugger_new(Emu* e, const char* font_path);
void debugger_free(Debugger* d);
bool debugger_handle_input(Debugger* d, SDL_Event* e); // false: quit, true: continue
void debugger_draw_labels(Debugger* d);
void debugger_memory_tab(Debugger* d);
void debugger_cpu_tab(Debugger* d);
void debugger_run(Debugger* d, char* rompath, bool test);
void debugger_emu_run(Emu* e, const char* rompath, bool test);

debugger.c:

#include <sys/debugger.h>
#include <stdlib.h>
#include <string.h>
SDL_Color black = {0, 0, 0, 255};
SDL_Color white = {255, 255, 255, 255};
// TEXT
Text* text_new(const char* text, SDL_Color bg, SDL_Color fg, int x, int y) {
 Text* t = malloc(sizeof(Text));
 t->text = malloc(strlen(text) + 1);
 strcpy(t->text, text);
 t->rect.x = x;
 t->rect.y = y;
 t->rect.w = strlen(t->text) * FONT_SIZE;
 t->rect.h = FONT_SIZE + 10;
 
 t->bg_color = bg;
 t->fg_color = fg;
 return t;
}
void text_display(Text* t, TTF_Font* f, SDL_Renderer* r) {
 SDL_SetRenderDrawColor(r, t->bg_color.r, t->bg_color.g, t->bg_color.b, 255);
 SDL_RenderFillRect(r, &t->rect);
 SDL_Surface* surface = TTF_RenderText_Blended(f, t->text, t->fg_color);
 if (surface == NULL) {
 printf("Failed to initialize text surface: %s\n", SDL_GetError());
 return;
 }
 t->texture = SDL_CreateTextureFromSurface(r, surface);
 if (t->texture == NULL) {
 printf("Failed to initialize text texture: %s\n", SDL_GetError());
 return;
 }
 SDL_FreeSurface(surface);
 if (SDL_RenderCopy(r, t->texture, NULL, &t->rect) != 0) {
 printf("Failed to copy text texture: %s\n", SDL_GetError());
 }
}
void text_free(Text* t) {
 if (t->text) {
 free(t->text);
 t->text = NULL;
 }
 if (t->texture) {
 SDL_DestroyTexture(t->texture);
 t->texture = NULL;
 }
 free(t);
 t = NULL;
}
// DEBUGGER
Debugger* debugger_new(Emu* e, const char* font_path) {
 Debugger* d = malloc(sizeof(Debugger));
 if (SDL_Init(SDL_INIT_VIDEO) != 0) {
 printf("Failed to initialize SDL: %s\n", SDL_GetError());
 return NULL;
 }
 if (TTF_Init() != 0) {
 printf("Failed to initialize font: %s\n", TTF_GetError());
 return NULL;
 }
 d->font = TTF_OpenFont(font_path, FONT_SIZE);
 if (d->font == NULL) {
 printf("Failed to read font: %s\n", TTF_GetError());
 return NULL;
 }
 d->window = SDL_CreateWindow("NES Debugger", SDL_WINDOWPOS_UNDEFINED, SDL_WINDOWPOS_UNDEFINED, 1920, 1080, 0);
 if (d->window == NULL) {
 printf("Failed to initialize window: %s\n", SDL_GetError());
 d->font = NULL;
 return NULL;
 }
 d->renderer = SDL_CreateRenderer(d->window, -1, 0);
 if (d->renderer == NULL) {
 printf("Failed to initialize renderer: %s\n", SDL_GetError());
 d->font = NULL;
 d->window = NULL;
 return NULL;
 }
 d->emu_info = e;
 d->current_tab = MEMORY_TAB;
 d->current_address = 0;
 d->input_mode = false;
 d->input_length = 0; 
 memset(d->input_buffer, 0, sizeof(d->input_buffer)); 
 return d;
}
void debugger_free(Debugger* d) {
 if (d->window) SDL_DestroyWindow(d->window);
 if (d->renderer) SDL_DestroyRenderer(d->renderer);
 if (d->font) TTF_CloseFont(d->font);
 free(d);
 d = NULL;
 SDL_Quit();
}
bool debugger_handle_input(Debugger* d, SDL_Event* e) {
 while (SDL_PollEvent(e)) {
 if (e->type == SDL_QUIT) {
 return false;
 } else if (e->type == SDL_KEYDOWN) {
 if (!d->input_mode) {
 switch (e->key.keysym.sym) {
 case SDLK_q:
 return false;
 break;
 case SDLK_k:
 if (d->current_tab == MEMORY_TAB) 
 if (d->current_address >= 16) d->current_address -= 16;
 break;
 case SDLK_j:
 if (d->current_tab == MEMORY_TAB) 
 if (d->current_address <= 0xFFF0) d->current_address += 16;
 break;
 case SDLK_TAB:
 if (d->current_tab == DISASSEMBLY_TAB) d->current_tab = MEMORY_TAB;
 else d->current_tab++;
 break;
 case SDLK_SEMICOLON:
 d->input_mode = true;
 break;
 case SDLK_SPACE:
 cpu_step(d->emu_info->cpu, d->emu_info->mem);
 }
 }
 else {
 if (e->key.keysym.sym >= SDLK_0 && e->key.keysym.sym <= SDLK_9 && d->input_length < 4) {
 d->input_buffer[d->input_length++] = e->key.keysym.sym;
 } else if (e->key.keysym.sym >= SDLK_a && e->key.keysym.sym <= SDLK_f && d->input_length < 4) {
 d->input_buffer[d->input_length++] = e->key.keysym.sym;
 } else if (e->key.keysym.sym == SDLK_RETURN) {
 d->input_mode = false;
 if (d->input_length > 0) {
 d->current_address = (u16)strtol(d->input_buffer, NULL, 16);
 d->input_buffer[0] = '0円';
 d->input_length = 0;
 }
 } else if (e->key.keysym.sym == SDLK_BACKSPACE && d->input_length > 0) {
 d->input_buffer[--d->input_length] = '0円';
 }
 }
 }
 }
 return true;
}
void debugger_draw_labels(Debugger* d) {
 // MEMORY
 SDL_Color mbg = d->current_tab == MEMORY_TAB ? white : black;
 SDL_Color mfg = d->current_tab == MEMORY_TAB ? black : white;
 Text* memory_label = text_new("MEMORY", mbg, mfg, 10, 10);
 text_display(memory_label, d->font, d->renderer);
 text_free(memory_label);
 // CPU
 SDL_Color cbg = d->current_tab == CPU_TAB ? white : black;
 SDL_Color cfg = d->current_tab == CPU_TAB ? black : white;
 Text* cpu_label = text_new("CPU", cbg, cfg, 7 * FONT_SIZE, 10);
 text_display(cpu_label, d->font, d->renderer);
 text_free(cpu_label);
 
 // DISASSEMBLY
 SDL_Color dbg = d->current_tab == DISASSEMBLY_TAB ? white : black;
 SDL_Color dfg = d->current_tab == DISASSEMBLY_TAB ? black : white;
 Text* disassembly_label = text_new("DISASSEMBLY", dbg, dfg, 11 * FONT_SIZE, 10);
 text_display(disassembly_label, d->font, d->renderer);
 text_free(disassembly_label);
}
void debugger_memory_tab(Debugger* d) {
 if (!d->emu_info) {
 printf("emu_info is NULL\n");
 return;
 }
 if (!d->emu_info->mem) {
 printf("emu_info->mem is NULL\n");
 return;
 }
 for (int i = 0; i < 0x10; i++) {
 u16 addr = d->current_address + i * 0x10;
 char line[64] = {0}; 
 sprintf(line, "0x%04X %02X %02X %02X %02X %02X %02X %02X %02X %02X %02X %02X %02X %02X %02X %02X %02X",
 addr,
 memory_read(d->emu_info->mem, addr + 0),
 memory_read(d->emu_info->mem, addr + 1),
 memory_read(d->emu_info->mem, addr + 2),
 memory_read(d->emu_info->mem, addr + 3),
 memory_read(d->emu_info->mem, addr + 4),
 memory_read(d->emu_info->mem, addr + 5),
 memory_read(d->emu_info->mem, addr + 6),
 memory_read(d->emu_info->mem, addr + 7),
 memory_read(d->emu_info->mem, addr + 8),
 memory_read(d->emu_info->mem, addr + 9),
 memory_read(d->emu_info->mem, addr + 10),
 memory_read(d->emu_info->mem, addr + 11),
 memory_read(d->emu_info->mem, addr + 12),
 memory_read(d->emu_info->mem, addr + 13),
 memory_read(d->emu_info->mem, addr + 14),
 memory_read(d->emu_info->mem, addr + 15));
 Text* mem_line = text_new(line, black, white, 10, 40 + (FONT_SIZE * i + 10));
 text_display(mem_line, d->font, d->renderer);
 text_free(mem_line);
 }
 SDL_Delay(10);
}
void debugger_cpu_tab(Debugger* d) {
 if (!d->emu_info) {
 printf("emu_info is NULL\n");
 return;
 }
 if (!d->emu_info->mem) {
 printf("emu_info->mem is NULL\n");
 return;
 }
 char a_reg[10] = {0};
 char x_reg[10] = {0};
 char y_reg[10] = {0};
 char p_reg[10] = {0};
 char sp_reg[10] = {0};
 char pc_reg[10] = {0};
 sprintf(a_reg, "A: %02X", cpu_get_reg(d->emu_info->cpu, REG_A));
 sprintf(x_reg, "X: %02X", cpu_get_reg(d->emu_info->cpu, REG_X));
 sprintf(y_reg, "Y: %02X", cpu_get_reg(d->emu_info->cpu, REG_Y));
 sprintf(p_reg, "P: %02X", cpu_get_reg(d->emu_info->cpu, REG_P));
 sprintf(sp_reg, "SP: %02X", cpu_get_reg(d->emu_info->cpu, REG_SP));
 sprintf(pc_reg, "PC: %04X", d->emu_info->cpu->pc);
 Text* a_text = text_new(a_reg, black, white, 10, 40);
 Text* x_text = text_new(x_reg, black, white, 11, 40 + (FONT_SIZE * 1) + 10);
 Text* y_text = text_new(y_reg, black, white, 11, 40 + (FONT_SIZE * 2) + 10);
 Text* p_text = text_new(p_reg, black, white, 11, 40 + (FONT_SIZE * 3) + 10);
 Text* sp_text = text_new(sp_reg, black, white, 11, 40 + (FONT_SIZE * 4) + 10);
 Text* pc_text = text_new(pc_reg, black, white, 11, 40 + (FONT_SIZE * 5) + 10);
 text_display(a_text, d->font, d->renderer);
 text_display(x_text, d->font, d->renderer);
 text_display(y_text, d->font, d->renderer);
 text_display(p_text, d->font, d->renderer);
 text_display(sp_text, d->font, d->renderer);
 text_display(pc_text, d->font, d->renderer);
 text_free(a_text);
 text_free(x_text);
 text_free(y_text);
 text_free(p_text);
 text_free(sp_text);
 text_free(pc_text);
}
void debugger_run(Debugger* d, char* rompath, bool test) {
 debug_emu_init(d->emu_info, rompath, test);
 bool running = true;
 SDL_Event e;
 while (running) {
 running = debugger_handle_input(d, &e);
 SDL_SetRenderDrawColor(d->renderer, 0, 0, 0, 255);
 SDL_RenderClear(d->renderer);
 debugger_draw_labels(d);
 switch (d->current_tab) {
 case MEMORY_TAB:
 debugger_memory_tab(d);
 break;
 case CPU_TAB:
 debugger_cpu_tab(d);
 break;
 default:
 break;
 }
 SDL_RenderPresent(d->renderer);
 SDL_Delay(10);
 }
}
void debugger_emu_run(Emu* e, const char* rompath, bool test) {
 debug_emu_init(e, rompath, test);
 debug_emu_run(e);
}

I also tried using valgrind to check for memory leaks but gave up trying to suppress SDL/SDL_ttf related warnings.

And thanks in advance for the help

Question 2

Kindly add what C Standard you are aiming to conform to. Moreover, would you open to receiving suggestions about new C23 features?

Question 3

@Harith As far as I understand C23 has added quite some features but is still not fully supported by compilers, so I would say the one before that. I do still have to look into C standards though

Question 4

Reserved keywords:

According to ISO C Standard:

Function names that begin with str, mem, or wcs and a lowercase letter are potentially reserved identifiers and may be added to the declarations in the <string.h> header.

Six function names in "memory.h" begin with mem, but I can not tell whether "defines.h", which you have not added here, includes <string.h> or not. Those identifiers are technically only reserved if that header is included.

`#pragma once` is not standard:

It is a widespread (but not universal) extension, but not in ISO C. It is simpler to do:

#ifndef MEM_H
#define MEM_H 1
#endif /* MEM_H */

Surely you would want your emulator to run on many systems.

Prefer `static constexpr` to `#define`:

In C23, you can do:

static constexpr unsigned int RAM_SIZE = 0x800;
typedef struct {
 u8 ram[RAM_SIZE]; // 0x0000 - 0x1FFF
 ..

Unlike #define, this is typed, and a bit safer to use. I have used unsigned int here, you may swap it for an unsigned short int instead. Prior to C23, stick with #defines for object-like macros for program-wide constants.

Missing documentation:

Memory* memory_new(void);
void memory_free(Memory* m);
u8 memory_read(Memory* m, u16 a);
void memory_write(Memory* m, u16 a, u8 v);
void memory_push(Memory* m, u8* sp, u8 v);
u8 memory_pop(Memory* m, u8* sp);
void load_rom(Memory* m, const char* path);

Well, nothing? That is disappointing. I can not even tell whether a null pointer would be a valid value for any of these, or what would happen on a write error, or a memory allocation error, of if the stack was full before calling memory_push(), or if the stack was empty before calling memory_pop().

An attempt to allocate memory can fail:

Memory* memory_new(void) {
 Memory* m = malloc(sizeof(Memory));
 memset(m, 0, sizeof(Memory));
 m->is16kb = false;
 return m;
}

You are blindly writing to m without checking the return value of malloc(). malloc() and family return a nullptr to indicate failure. Failure to check for it risks invoking undefined behavior by a subsequent null pointer dereference.

malloc() + memset() is better as calloc().

After you replace the two functions with it, the function would become:

Memory* memory_new(void) {
 return calloc(1, sizeof(Memory));
}

Quite small, eh? What happened to setting the is16kb member to false? calloc() did it for us.

Furthermore, some malloc() calls have casting, some don't. malloc() and family return a generic void * that is implicitly converted to any other pointer type. As such, a cast is redundant and only serves to clutter the codebase.

I see you have used a cast in "instructions.c", and have also checked the return value of malloc(), but elsewise in "mem.h". Do be consistent.

Additionally, do not return an error code if you are not going to check for it. instruction_new() returns NULL if malloc() failed, yet nowhere has the return value been checked.

`free(nullptr)` is a NOP:

void memory_free(Memory* m) { 
 if (m)
 free(m); 
 m = NULL;
}

You need not check whether m is a nullptr, as it is a valid argument for free(). Perhaps it is the case of "a single if" vs "a function call and an if", and profiling showed that it made a difference? I think not.

GNU C's coding style conventions have:

Avoid unnecessary test before free

Since SunOS 4 stopped being a reasonable portability target, (which happened around 2007) there has been no need to guard against "free (NULL)". Thus, any guard like the following constitutes a redundant test:
if (P) 
 free (P);
It is better to avoid the test.[*] Instead, simply free P, regardless of whether it is NULL.

[*] However, if your profiling exposes a test like this in a performance-critical loop, say where P is nearly always NULL, and the cost of calling free on a NULL pointer would be prohibitively high, consider using __builtin_expect, e.g., like this:
if (__builtin_expect (ptr != NULL, 0))
 free (ptr);

What is strange is that you are not using a similar check in "debugger.c". This is yet another inconsistency througout the codebase.

Case ranges are not standard:

u8 memory_read(Memory* m, u16 a) {
 switch(a) {
 case 0x0000 ... 0x1FFF:
 return m->ram[a % RAM_SIZE];
 case 0x2000 ... 0x3FFF:
 return m->ppu[a % PPU_SIZE];
 case 0x4000 ... 0x401F:
 return m->apu_io[a % APU_IO_SIZE];
 case 0x4020 ... 0xFFFF:
 if (m->is16kb && a >= 0xC000) {
 return m->cart[(a - 0x8000) % 0x4000]; // Mirror 0x8000 - 0xBFFF to 0xC000 - 0xFFFF
 }
 return m->cart[a - 0x4020];
 }
 return 0;
}

Yes, they are useful and look nice, but case ranges have not been added to ISO C Standard yet. For the curious, there are proposals for it, and it may very well be added in the next revision of the C Standard.

Nevertheless, what can we do instead?

Use an if-else ladder:

#define RANGE(VAL_, LO_, HI_) \
 (((VAL_) >= (LO_)) && ((VAL_) <= (_HI)))
u8 memory_read(Memory* m, u16 a) {
 if (RANGE(a, 0x0000, 0x1FFF)) {
 return m->ram[a % RAM_SIZE];
 } else if (RANGE(a, 0x2000, 0x3FFF)) {
 return m->ppu[a % PPU_SIZE];
 } else if (RANGE(a, 0x4000, 0x401F)) {
 return m->apu_io[a % APU_IO_SIZE];
 } else if (RANGE(a, 0x4020, 0xFFFF)) {
 return m->is16Kb && a >= 0xC000 ? m->cart[(a - 0x8000) % 0x4000] : m->cart[a - 0x4020];
 } 
 return 0;
}

But that is a hideous sight to my eyes, and not very readable. You can instead replace all else if with just if and a newline for readability.

There's a greater problem, half your code is indented with two spaces, and the other half with four spaces. What and why is this disrepancy?

You are using the same case ranges in memory_write(). I'd replace the switch in it with the ifs as well.

Though you have not considered the case where a - what's with the one-letter naming? - is not within the range 0x0000-0xFFFF. For such cases, I tend to have an UNEXPECTED_VALUE() macro:

/**
 * A special-case of INTERNAL_ERROR() that prints an unexpected integer value.
 * 
 * EXPR - The expression having the unexpected value. */
#define UNEXPECTED_INT_VALUE(EXPR) \
 INTERNAL_ERROR("%lld (0x%llX): unexpected value for " #EXPR "\n", \
 (long long)(EXPR), (unsigned long long)(EXPR))
/**
 * A special-case of fatal_error() that additionally prints the file, line, and
 * function name where an internal error occured.
 *
 * FMT - The printf() format string literal to use.
 * ... - The printf() arguments. */
#define INTERNAL_FORMAT(FMT, ...) \
 fatal_error("%s::%d::%s(): internal error: " FMT "", __FILE__, __LINE__, \
 __VA_OPT__(,) __VA_ARGS__)

where fatal_error() is your usual variadic function that flushes stdout, prints a the error message to stderr, and then calls exit(EXIT_FAILURE), _Exit(EXIR_FAILURE), or abort().

I found these whilst reading cdecl's source code, and adopted them for myself.

__VA_OPT__ is part of C23. Without __VA_OPT__, in the case that __VA_ARGS__ expanded to nothing (because no arguments were passed), we would have a trailing comma and the program would fail to compile.

If you can not use __VA_OPT__, you may do:

INTERNAL_FORMAT("%s", arg);

The strange empty string literal after FMT is present to provide rudimentary type-checking. Consecutive string literals are concatenated in C, so passing something other than a string literal to INTERNAL_FORMAT is likely to fail. One can make it better, but the added robustness may not be worth the added complexity, and this is not a tutorial on how to write robust macros.

Only declare as many variables as you require:

void memory_push(Memory* m, u8* sp, u8 v) {
 u16 addr = 0x0100 | *sp;
 memory_write(m, addr, v);
 *sp -= 1;
}
u8 memory_pop(Memory* m, u8* sp) {
 *sp += 1;
 u16 addr = 0x0100 | *sp;
 return memory_read(m, addr);
}

addr is not referenced at all in these functions. Just pass the value directly to memory_read().

Other than that, is there a reason you are avoiding assert? You could always turn them off with -DNDEBUG in release mode.

See also: What is the difference between #include <filename> and #include "filename"?.

Error messages go to `stderr`:

 FILE* file = fopen(path, "rb");
 if (!file) {
 printf("Could not open ROM at path: %s", path);
 return;
 }

In the case that stdout was redirected to a file or a pipe, the error message may go unseen. Use fprintf(stderr, ...) or fputs(..., stderr) instead.

In the same function, you are again ignoring the return value of malloc(). Good job on checking fread()'s return value albeit.

It might be nicer to have some helper functions for reading the header, validating it, and so on.

I am not very happy to see a load_rom() function print error messages to stdout. All it should do it load ROM. Consider leaving the error messages to the caller, and return a meaningful status code. Perhaps an enum?

Use array notation for pointer arguments:

The function prototypes aren't as self-descriptive as they could be. C (since C99, or about 25 years) has a method for declaring a pointer parameter that must not be null. This is done by the rarely used syntax of putting the static keyword inside the parameter declaration:

void func(char arg[static 1])
{
 ....
}

This says that arg must point to the first element of an array of at least one element, per C 2011 [N1570] 6.7.6.3 7, which means that it can not be null.

So you can do array[static 1] to specify that array should point to at least one item.

This has some benefits:

Compilers can detect that a null pointer was passed and can output some diagnostic information. GCC has the best diagnostic information, Clang and Intel's compiler, not so much. For example, Clang and ICX only issue a warning when NULL/nullptr is passed as an argument, but not when a variable which happens to be a null pointer is passed, unlike GCC.
It better documents the code, as having at least 1 element indicates that a null pointer is not a valid value for this argument. This way, a comment about the function invoking undefined behavior on being passed a null pointer is not necessary.
It can eliminate one or more branches (assuming the called function was checking the validity of the arguments).

According to @Pascal Cuoq in this StackOverflow answer: What is the meaning of "static" in parameters array types in C?,

One common use is f(int p[static 1]), which is more or less equivalent to the notion of reference (as opposed to pointer) in other languages.

After adding the static keyword to load_rom(), it becomes:

// void load_rom(Memory* m, const char* path);
void load_rom(Memory m[static 1], const char path[static 1]);

There are 3 ways this syntax can be utilized to distinguish different cases:

A pointer to single object of type char:

void func(char arg[static 1]);

A pointer to a collection of objects of known numbers of type char:

void func(char arg[static 256]);

A pointer to a collection of objects of unknown numbers of type char:

void func(size_t n, char arg[static n]);

And of course, without the array syntax:

A pointer to a single object of type char or a null pointer:

void func(char *arg);

Note that this notation can not be used for local variables, return values, or for functions like memcpy() because arrays of void are not valid. To specify that a function returns a value that is not a null pointer, use GCC's __attribute__((returns_nonnull)). It is also supported by Clang and Intel's compiler. For void *s (and others too), you may use GCC's __attribute__((nonnull)). For local variables, wait for the C committee to add an _Optional type, or for Clang, use _Nonnull. This extension is not supported by GCC. Also note that MSVC does not support this C99 feature.

See also: Is it valid to pass the address of a non-array variable to a function parameter declared as Type ptr[static 1]?.

The value of each subsequent enumerator is one greater than the previous enumerator:

So you can just do:

typedef enum {
 REG_A,
 REG_X,
 REG_Y,
 REG_SP,
 REG_P,
} CpuReg;

instead of:

typedef enum {
 REG_A = 0,
 REG_X = 1,
 REG_Y = 2,
 REG_SP = 3,
 REG_P = 4
} CpuReg;

Restrict the scope of variables as much as possible:

In "cpu.h", we have:

Instruction** instructions = NULL;
Instruction* current_instruction = NULL;
bool first = true;
const char* INSTR_STR[0x100] = {

Yet a quick search showed that these are not used outside of this translation unit. So they should all be prefixed with the static keyword, as there's no need for them to external linkage.

Custom `typedef`s for integer types:

<stdint.h> defines fixed-width integer types like uint8_t, uint16_t, et cetera. Your codebase is using u8, u16, et cetera, which seem to be typedefs for these standard fixed-width integer types , defined in the file <defines.h>, which you did not include here. Why not simply use the standard types? Don't make us keep track of additional aliases.

Missing definitions of `bool`, `true`, and `false`:

Prior to C23, <stdbool.h> should be included for the definitions of bool, true, and false. In C23, they are keywords, and this header is not required.

You can use the iwyu (include-what-you-use) program to check if each file is self-contained. Or gcc -fsyntax-only, assuming you can use gcc.

Replace `malloc()` + `strlen()` + `strcpy()` with `strdup()`:

strdup() is part of ISO POSIX Standard that has been added to C23. Prior to C23, it is a widely-available function. If a platform does not have it, it is trivial to roll out your own. Here's one implementation:

char *strdup(const char s[static 1])
{
 const size_t size = strlen(s) + 1;
 char *const p = malloc(size);
 
 return p != NULL ? memcpy(p, s, size) : p;
}

Now text_new() can be updated to use this:

#if 0
// TEXT
Text* text_new(const char* text, SDL_Color bg, SDL_Color fg, int x, int y) {
 Text* t = malloc(sizeof(Text));
 t->text = malloc(strlen(text) + 1);
 strcpy(t->text, text);
 ...
#else
// TEXT
Text* text_new(const char* text, SDL_Color bg, SDL_Color fg, int x, int y) {
 Text* t = malloc(sizeof(Text));
 t->text = strdup(text);
 ...
}

Note that it returns a null pointer to indicate failure.

But what is with this // TEXT comment? Not helpful, elide it.

Cache `strlen()` calls:

In text_new(), you have:

// TEXT
Text* text_new(const char* text, SDL_Color bg, SDL_Color fg, int x, int y) {
 Text* t = malloc(sizeof(Text));
 t->text = malloc(strlen(text) + 1);
 ...
 t->rect.w = strlen(t->text) * FONT_SIZE;
 ...

Yet you already calculated strlen(text) in the call to malloc(), so declare a temporary variable and use that instead of calling strlen() twice.

Use a function-like macro to avoid duplication:

In "debugger.c", we have:

Text* a_text = text_new(a_reg, black, white, 10, 40);
 Text* x_text = text_new(x_reg, black, white, 11, 40 + (FONT_SIZE * 1) + 10);
 Text* y_text = text_new(y_reg, black, white, 11, 40 + (FONT_SIZE * 2) + 10);
 Text* p_text = text_new(p_reg, black, white, 11, 40 + (FONT_SIZE * 3) + 10);
 Text* sp_text = text_new(sp_reg, black, white, 11, 40 + (FONT_SIZE * 4) + 10);
 Text* pc_text = text_new(pc_reg, black, white, 11, 40 + (FONT_SIZE * 5) + 10);
 text_display(a_text, d->font, d->renderer);
 text_display(x_text, d->font, d->renderer);
 text_display(y_text, d->font, d->renderer);
 text_display(p_text, d->font, d->renderer);
 text_display(sp_text, d->font, d->renderer);
 text_display(pc_text, d->font, d->renderer);
 text_free(a_text);
 text_free(x_text);
 text_free(y_text);
 text_free(p_text);
 text_free(sp_text);
 text_free(pc_text);

Perhaps we can make a struct and loop over it?

If not, then at least we can have TEXT_FREE_ALL():

/**
 * Repeatedly calls the function FN with each argument of type TYPE *. */
#define FN_APPLY(TYPE, FN, ..) \
 do { \
 TYPE **list = (TYPE*[]){ __VA_ARGS__, nullptr}; \
 for (size_t i = 0; list[i]; ++i) \
 FN(list[i]); \
 } while (false)
/**
 * Calls the free() function individually on all arguments --- useful for
 * replacing multiple individual calls to free() like these:
 *
 * free(a);
 * free(b);
 * free(c);
 * free(d);
 */
#define FREE_ALL(...) FN_APPLY(void, free, __VA_ARGS__)

You can adjust the above for text_free().

Modifying your copy of the caller's argument does not modify the value of caller's argument:

In debugger_free(), we have:

free(d);
d = NULL;

d is a copy of the caller's pointer. d = NULL only changes debugger_free()'s copy of d, not the caller's. @Fe2O3 suggests returning NULL instead. Alternatively, you can take a pointer to a pointer.

In debugger_handle_input() there's an if(cond) return; followed by an else. You could get rid of else and gain one level of indent.

Memory leaks:

There are approximately 22 calls to instruction_new() in the code. All of them return a pointer to dynamically-allocated memory. There is no corresponding call to instruction_free() in the code. Why, such a function is not even present. It seems as if running cleanly under valgrind was not one of the goals.

Question 5

Many good points, but I disagree about the array notation for pointer arguments. While there are indeed technical benefits, the notation suggests that there are arrays, but load_rom() only expects a single Memory object, not an array of them. Secondly, the idiomatic way to pass pointers to strings in C is using (const) char*. It's unfortunate C23 didn't come with a [[nonnull]]. But for readability, I would avoid the use of [static 1] here.

Question 6

Thanks a lot for the time you took into reviewing my code. I will have to look into error handling in general as I didn't know of stderr, and will probably make an enum for return codes. I will also change the ranges and the defines to be safer/more portable. I just have a question: in the memory_read/write functions a is a u16 (which I defined as a uint16_t), so wouldn't it be redundant to check if its not within range? Or can type casting create errors in cases like this?

Question 7

"In C23, you can do:" is premature. C23 is yet to be released. Even once it is, correct and common implementations will take some time.

Question 8

@G.Sliepen Re: "load_rom() only expects a single Memory object, not an array of them." That is correct, but we can think of it as array of a single object.

Question 9

@Harith if (RANGE(a, 0x0000, 0x1FFF)) or if( IS_BETWEEN( 0x0000, a, 0x1FFF ) ) to accentuate the in-between-i-ness of the test... :-)... Gotta stop this... :-)

Question 10

Just scrolling the OP's code, looking for obvious things.
^{(Will try to NOT duplicate commentary from the other current answer.)}

Not obvious is: Where's emu.h?
I'm not familiar with what is being emulated, so this may be a strange question.

The code is well presented, and the factoring of functionality seems well thought-out.

Header file `#include`:

#include <defines.h>
Ordinarily, project related headers (those the coder can manipulate for an app) use double-quotes, not angle brackets. Configuration and administration of the directory structure (esp. for a small-ish, standalone project) is a bit of a black art.
Recommend keeping things as simple as possible while gaining experience.

The first chapter of this recent long answer addresses my preferred objective regarding sequencing of #include lines, including (pardon the pun) avoiding (unless absolutely necessary) using #include inside header files. Please review the first few paragraphs and the comments below that answer.

DRY:

In debugger_memory_tab(), there is a large, verbose and tricky to get right block to form a string for display:

 for (int i = 0; i < 0x10; i++) {
 u16 addr = d->current_address + i * 0x10;
 char line[64] = {0}; 
 sprintf(line, "0x%04X %02X %02X" /* ... */ "%02X %02X",
 addr,
 memory_read(d->emu_info->mem, addr + 0),
 memory_read(d->emu_info->mem, addr + 1),
 ... // abridged
 memory_read(d->emu_info->mem, addr + 14),
 memory_read(d->emu_info->mem, addr + 15));
 Text* mem_line = text_new(line, black, white, 10, 40 + (FONT_SIZE * i + 10));
 text_display(mem_line, d->font, d->renderer);
 text_free(mem_line);
 }

This seems like it might be equivalent (without needing to be abridged):

 u16 start = d->emu_info->mem;
 for( int i = 0; i < 0x10 * 0x10; i += 0x10 ) {
 char line[64], *at = line;
 u16 addr = d->current_address + i;
 at += sprintf( at, "0x%04X", addr ); // init of line[] happens here
 for( u16 j = 0; j < 0x10; j++ )
 at += sprintf( at, " %02X", memory_read( start, addr + j ) );
 // 3 more unaltered lines not repeated here
 }

OP must check that this does what is intended. This is a free-hand revision to point in a DRY-er direction, but may contain human errors.

^{(If one is enamoured with replication (i.e. "copy/paste") then one should become a Rockstar musician who earns millions touring 48 cities in 112 days. A new audience, but the same-old set list night-after-night. Coders should strive to be concise and non-repetitive.)}

Re: 'Hex dump'. Take a 5 min break and enjoy watching this hex dump. While watching, notice the display of scrolling hex addresses on the left are NOT prefixed with "0x". Debug users don't need unnecessary clutter. (Consider, too, how that display also shows, on the right side, printable ASCII characters for the 16 bytes of each line. Consider if this is right for your project, too.)

Line length:

cpu_log_instr() has a VERY long line (not reproduced here) that makes reading and maintenance very difficult. (Requires horizontal scrolling here.)

Here is that function broken into chewable chunks:

void cpu_log_instr(Cpu* c, u8 opcode) {
 struct {
 char *n; u32 tok;
 } Rs[] = {
 { "A", REG_A, },
 { "X", REG_X, },
 { "Y", REG_Y, },
 { "P", REG_P, },
 { "SP", REG_SP,},
 };
 // 128? Big enough?? Don't scrimp on temp buffer sizes
 char buf[128], *at = buf;
 at += sprintf( at, "%s - PC: %04X", INSTR_STR[ opcode ], c->pc );
 for( int i = 0; i < sizeof Rs/sizeof Rs[0]; i++ )
 at += sprintf( at, " - %s: %02X", Rs[i].n, cpu_get_reg( Rs[i].tok ) );
 puts( buf );
}

(Now it looks like I am repeating myself.)
While it's unlikely more registers will be incorporated into the device being emulated, the reader can be confident labels and values are correctly paired when output.

Consider that you will be the reader when you come back to this code in many months. You will want it to be easy to read and understand.

Use more Standard Library functions:

In load_rom() there's this longish line,

 if (header[0] != 'N' || header[1] != 'E' || header[2] != 'S' || header[3] != 0x1A) {

that could be replaced with

 if( memcmp( header, "NES032円", 4 ) ) { // 0x1A == 032 (octal)

Suggest allocating 10-20% of your coding time to simply discovering, superficially, what is in the C Standard Library. Even just reading the function names and the one-to-two line description of its purpose. Eg: when you know strspn() is already written and tested, and what it and its cousin strcspn() can do for you, you will save literal hours of your time in the future. (And, those functions are tested and known to work!)

Perhaps review the entire code looking for opportunities to replace hand-rolled with tailor made.

Problematic function:

The code of this function is troubling.
This function is a multi-step process to achieve an objective.
Any single step could fail, but must fail robustly

Here it is, annotated:

// void return should instead signal "fail/success" back to caller
void load_rom(Memory* m, const char* path) {
// 'file' is a generic name; suggest 'fp' is conventional, better
 FILE* file = fopen(path, "rb");
 if (!file) {
// see "fail/success" above (Caller reports error to stderr)
 printf("Could not open ROM at path: %s", path);
 return;
 }
// why 'uint8_t' and not 'u8' for consistency
 uint8_t header[HEADER_SIZE];
 if (fread(header, 1, HEADER_SIZE, file) != HEADER_SIZE) {
 fprintf(stderr, "Error: Could not read iNES header from %s\n", path);
// close #1
 fclose(file);
 return;
 }
// earlier comment in this answer
 if (header[0] != 'N' || header[1] != 'E' || header[2] != 'S' || header[3] != 0x1A) {
 fprintf(stderr, "Error: Invalid iNES header in %s\n", path);
// close #2
 fclose(file);
 return;
 }
 uint8_t prg_rom_size = header[4]; // Size of PRG ROM in 16KB units
 uint8_t chr_rom_size = header[5]; // Size of CHR ROM in 8KB units
// chr_rom_size is not used in the function
 size_t prg_rom_bytes = prg_rom_size * PRG_ROM_SIZE;
 uint8_t* prg_rom_data = malloc(prg_rom_bytes);
// missing verification of success
 if (fread(prg_rom_data, 1, prg_rom_bytes, file) != prg_rom_bytes) {
 fprintf(stderr, "Error: Could not read PRG ROM data from %s\n", path);
 free(prg_rom_data);
// close #3
 fclose(file);
 return;
 }
// some peculiar arithmetic happening here
 memcpy(m->cart + (ROM_START - 0x4020), prg_rom_data, prg_rom_bytes);
 if (prg_rom_size == 1) {
 m->is16kb = true;
 memcpy(m->cart + (ROM_START - 0x4020 + PRG_ROM_SIZE), prg_rom_data, PRG_ROM_SIZE);
 }
 free(prg_rom_data);
// close #4
 fclose(file);
}

And with a few tweaks:

// error return codes from this function go into a header
// file that is seen by interested code files
enum {
 eERR_none,
 eERR_fopen,
 eERR_hdr,
 eERR_bad,
 eERR_heap,
 eERR_body
};
...
int load_rom( Memory *m, const char *path ) {
 int err = eERR_none; // optimism
 u8 *p = NULL;
 FILE *fp; // Open File
 if( ( fp = fopen( path, "rb" ) ) == NULL )
 err = eERR_fopen, goto bot;
 u8 hdr[ HEADER_SIZE ]; // Load Header
 if( fread( hdr, 1, HEADER_SIZE, fp ) != HEADER_SIZE )
 err = eERR_hdr, goto bot;
 if( memcmp( hdr, "NES031円", 4 ) ) // Check Header
 err = eERR_bad, goto bot;
 // hdr[4] contains the size of PRG ROM in 16KB units
 size_t alloc_sz = hdr[4] * PRG_ROM_SIZE;
 // hdr[5] contains the size of CHR ROM in 8KB units
 // hdr[5] is not used in this function
 if( ( p = malloc( alloc_sz ) ) == NULL ) // Allocate buffer
 err = eERR_heap, goto bot;
 if( fread( p, 1, alloc_sz, fp ) != alloc_sz ) // Load Data
 err = eERR_body, goto bot;
 u16 addr = m->cart + ( ROM_START - 0x4020 );// Store for use
 memcpy( addr, p, alloc_sz );
 m->is16kb = ( rom_sz == 1 );
 if( m->is16kb )
 memcpy( addr + PRG_ROM_SIZE, p, PRG_ROM_SIZE );
bot: // Clean up & return
 free( p ); // NULL is okay
 if(fp) fclose(fp); // Only close if not NULL
 return err;
}

Most of the comments above are not needed. (Trust that the readers will have a grasp of C and common library functions.) They're present only to illustrate sequence of operations that, with minor variations, appear in many programs.

The two unexpected calls to memcpy() should be explained with a comment (from the coder who knows why.)

Notice how one comment about pHdr[4] precludes defining an intermediary variable that is only used for one calculation?

There's nothing inherently wrong with using goto as long as its use is easy to comprehend. Keep an open mind regarding "rules to live by" told by others. There's no need to join a clique. Simply write the best code that you can.

Take a look at the whitespace in the code I've posted. This is "unconventional", but I find the extra whitespace just inside parentheses helps me see variable names better. Especially helpful when a formulation is intricate like:
while(!!(t=realloc(t,(n+1)*sz))&&!!(t[n]=strtok(p," .\n"))) p=NULL, n++;
(Something I tossed together for my own use. It chops lines into 'words', storing pointers into a growing array of pointers. Don't take it too seriously!)
Judicious whitespace is almost as important as the algorithm in the code. Human eyeballs need to verify that the code is semantically correct.

Good or bad, patterns or sequences such as those illustrated come with experience. Experience is acquired through patient practice. Keep at it, and don't get discouraged. Keep learning from all sources available to you.

The boss is a (w)rapper:

That the caller should correctly handle error conditions (that may involve a message to the user) is good. What if "the caller" appears at several places in the code? (Do not resort to copy/paste! Remember DRY.)

Below is a wrapper that does something close to all that is needed. (Primarily, notice the separation of the clerk functionality that tries to get the job done, and the executive functionality that checks/reports the success/failure of completion of that work.

static int load_rom_doer( Memory *m, const char *path ) {
 ... // code of the function formerly known as "load_rom()"
 // notice that this is 'static`; to be called by only one master
}
int load_rom( Memory *m, const char *path ) {
 char *str = NULL;
 u32 ret;
 switch( ret = load_rom_doer( m, path ) ) {
 case eERR_none: break;
 case eERR_fopen: str = "open ROM file"; break;
 case eERR_hdr: str = "read header bytes"; break;
 case eERR_bad: str = "not 'iNES' file"; break;
 case eERR_heap: str = "heap allocation"; break;
 case eERR_body: str = "read ROM bytes"; break;
 default: str = "Unknown return from load_rom_doer()"; break;
 }
 if( str )
 fprintf( stderr, "Fail: load_rom(): %s (%s)\n", str, path );
 return ret;
}

You can develop this to extremes, of course, using variadic facilities. It's hard to know when is "good enough/that's all that's required", sometimes.

OR, if it is simple enough, as this might be, this wrapping code could be blended back the load_rom_doer() function before it returns. It's a judgement call that the coder makes. Few functions is good, but so is clear delineation of tasks and responsibilities.

Alternative for contiguous values

memory_read() uses an unconventional syntax that is not supported by all implementations.

Because the ranges of interest are adjacent a standard, conventional syntax can be used.

u8 memory_read(Memory* m, u16 a) {
 if( a <= 0x1FFF ) return m->ram[a % RAM_SIZE];
 if( a <= 0x3FFF ) return m->ppu[a % PPU_SIZE];
 if( a <= 0x401F ) return m->apu_io[a % APU_IO_SIZE];
 // so the value of a is <= 0xFFFF...
 u16 addr = ( m->is16kb && a >= 0xC000 )
 ? (a - 0x8000) % 0x4000 // Mirror 0x8000-0xBFFF to 0xC000-0xFFFF
 : a - 0x4020;
 return m->cart[ addr ];
}

These hexadecimal constants are still magic numbers that should be represented in code with tokens that have meaningful names.
Recommend using enum, or const u16 name = value;, over #define so that those names won't evaporate going through the preprocessor.

^{{more to come? perhaps.}}

Question 11

1) What does bot mean? 2) I like this version of memory_read().

Question 12

@Harith Re: Point 2... Yes. Feels like stepping back to using Hollerith cards and the early days of "tabulating machines". Re: Point 1... I'd like to write "Bail Out Tactics" (or something other "mystical knowledge" garnered over the years...) But, for me, it merely means "bottom". I do like to think about bottoms, sometimes :-) Cheers!

Madagascar Madagascar 10.1k1 gold badge15 silver badges51 bronze badges · Accepted Answer · 2024-06-08 21:00:49Z

Reserved keywords:

According to ISO C Standard:

Function names that begin with str, mem, or wcs and a lowercase letter are potentially reserved identifiers and may be added to the declarations in the <string.h> header.

Six function names in "memory.h" begin with mem, but I can not tell whether "defines.h", which you have not added here, includes <string.h> or not. Those identifiers are technically only reserved if that header is included.

`#pragma once` is not standard:

It is a widespread (but not universal) extension, but not in ISO C. It is simpler to do:

#ifndef MEM_H
#define MEM_H 1
#endif /* MEM_H */

Surely you would want your emulator to run on many systems.

Prefer `static constexpr` to `#define`:

In C23, you can do:

static constexpr unsigned int RAM_SIZE = 0x800;
typedef struct {
 u8 ram[RAM_SIZE]; // 0x0000 - 0x1FFF
 ..

Unlike #define, this is typed, and a bit safer to use. I have used unsigned int here, you may swap it for an unsigned short int instead. Prior to C23, stick with #defines for object-like macros for program-wide constants.

Missing documentation:

Memory* memory_new(void);
void memory_free(Memory* m);
u8 memory_read(Memory* m, u16 a);
void memory_write(Memory* m, u16 a, u8 v);
void memory_push(Memory* m, u8* sp, u8 v);
u8 memory_pop(Memory* m, u8* sp);
void load_rom(Memory* m, const char* path);

Well, nothing? That is disappointing. I can not even tell whether a null pointer would be a valid value for any of these, or what would happen on a write error, or a memory allocation error, of if the stack was full before calling memory_push(), or if the stack was empty before calling memory_pop().

An attempt to allocate memory can fail:

Memory* memory_new(void) {
 Memory* m = malloc(sizeof(Memory));
 memset(m, 0, sizeof(Memory));
 m->is16kb = false;
 return m;
}

You are blindly writing to m without checking the return value of malloc(). malloc() and family return a nullptr to indicate failure. Failure to check for it risks invoking undefined behavior by a subsequent null pointer dereference.

malloc() + memset() is better as calloc().

After you replace the two functions with it, the function would become:

Memory* memory_new(void) {
 return calloc(1, sizeof(Memory));
}

Quite small, eh? What happened to setting the is16kb member to false? calloc() did it for us.

Furthermore, some malloc() calls have casting, some don't. malloc() and family return a generic void * that is implicitly converted to any other pointer type. As such, a cast is redundant and only serves to clutter the codebase.

I see you have used a cast in "instructions.c", and have also checked the return value of malloc(), but elsewise in "mem.h". Do be consistent.

Additionally, do not return an error code if you are not going to check for it. instruction_new() returns NULL if malloc() failed, yet nowhere has the return value been checked.

`free(nullptr)` is a NOP:

void memory_free(Memory* m) { 
 if (m)
 free(m); 
 m = NULL;
}

You need not check whether m is a nullptr, as it is a valid argument for free(). Perhaps it is the case of "a single if" vs "a function call and an if", and profiling showed that it made a difference? I think not.

GNU C's coding style conventions have:

Avoid unnecessary test before free

Since SunOS 4 stopped being a reasonable portability target, (which happened around 2007) there has been no need to guard against "free (NULL)". Thus, any guard like the following constitutes a redundant test:
if (P) 
 free (P);
It is better to avoid the test.[*] Instead, simply free P, regardless of whether it is NULL.

[*] However, if your profiling exposes a test like this in a performance-critical loop, say where P is nearly always NULL, and the cost of calling free on a NULL pointer would be prohibitively high, consider using __builtin_expect, e.g., like this:
if (__builtin_expect (ptr != NULL, 0))
 free (ptr);

What is strange is that you are not using a similar check in "debugger.c". This is yet another inconsistency througout the codebase.

Case ranges are not standard:

u8 memory_read(Memory* m, u16 a) {
 switch(a) {
 case 0x0000 ... 0x1FFF:
 return m->ram[a % RAM_SIZE];
 case 0x2000 ... 0x3FFF:
 return m->ppu[a % PPU_SIZE];
 case 0x4000 ... 0x401F:
 return m->apu_io[a % APU_IO_SIZE];
 case 0x4020 ... 0xFFFF:
 if (m->is16kb && a >= 0xC000) {
 return m->cart[(a - 0x8000) % 0x4000]; // Mirror 0x8000 - 0xBFFF to 0xC000 - 0xFFFF
 }
 return m->cart[a - 0x4020];
 }
 return 0;
}

Yes, they are useful and look nice, but case ranges have not been added to ISO C Standard yet. For the curious, there are proposals for it, and it may very well be added in the next revision of the C Standard.

Nevertheless, what can we do instead?

Use an if-else ladder:

#define RANGE(VAL_, LO_, HI_) \
 (((VAL_) >= (LO_)) && ((VAL_) <= (_HI)))
u8 memory_read(Memory* m, u16 a) {
 if (RANGE(a, 0x0000, 0x1FFF)) {
 return m->ram[a % RAM_SIZE];
 } else if (RANGE(a, 0x2000, 0x3FFF)) {
 return m->ppu[a % PPU_SIZE];
 } else if (RANGE(a, 0x4000, 0x401F)) {
 return m->apu_io[a % APU_IO_SIZE];
 } else if (RANGE(a, 0x4020, 0xFFFF)) {
 return m->is16Kb && a >= 0xC000 ? m->cart[(a - 0x8000) % 0x4000] : m->cart[a - 0x4020];
 } 
 return 0;
}

But that is a hideous sight to my eyes, and not very readable. You can instead replace all else if with just if and a newline for readability.

There's a greater problem, half your code is indented with two spaces, and the other half with four spaces. What and why is this disrepancy?

You are using the same case ranges in memory_write(). I'd replace the switch in it with the ifs as well.

Though you have not considered the case where a - what's with the one-letter naming? - is not within the range 0x0000-0xFFFF. For such cases, I tend to have an UNEXPECTED_VALUE() macro:

/**
 * A special-case of INTERNAL_ERROR() that prints an unexpected integer value.
 * 
 * EXPR - The expression having the unexpected value. */
#define UNEXPECTED_INT_VALUE(EXPR) \
 INTERNAL_ERROR("%lld (0x%llX): unexpected value for " #EXPR "\n", \
 (long long)(EXPR), (unsigned long long)(EXPR))
/**
 * A special-case of fatal_error() that additionally prints the file, line, and
 * function name where an internal error occured.
 *
 * FMT - The printf() format string literal to use.
 * ... - The printf() arguments. */
#define INTERNAL_FORMAT(FMT, ...) \
 fatal_error("%s::%d::%s(): internal error: " FMT "", __FILE__, __LINE__, \
 __VA_OPT__(,) __VA_ARGS__)

where fatal_error() is your usual variadic function that flushes stdout, prints a the error message to stderr, and then calls exit(EXIT_FAILURE), _Exit(EXIR_FAILURE), or abort().

I found these whilst reading cdecl's source code, and adopted them for myself.

__VA_OPT__ is part of C23. Without __VA_OPT__, in the case that __VA_ARGS__ expanded to nothing (because no arguments were passed), we would have a trailing comma and the program would fail to compile.

If you can not use __VA_OPT__, you may do:

INTERNAL_FORMAT("%s", arg);

The strange empty string literal after FMT is present to provide rudimentary type-checking. Consecutive string literals are concatenated in C, so passing something other than a string literal to INTERNAL_FORMAT is likely to fail. One can make it better, but the added robustness may not be worth the added complexity, and this is not a tutorial on how to write robust macros.

Only declare as many variables as you require:

void memory_push(Memory* m, u8* sp, u8 v) {
 u16 addr = 0x0100 | *sp;
 memory_write(m, addr, v);
 *sp -= 1;
}
u8 memory_pop(Memory* m, u8* sp) {
 *sp += 1;
 u16 addr = 0x0100 | *sp;
 return memory_read(m, addr);
}

addr is not referenced at all in these functions. Just pass the value directly to memory_read().

Other than that, is there a reason you are avoiding assert? You could always turn them off with -DNDEBUG in release mode.

See also: What is the difference between #include <filename> and #include "filename"?.

Error messages go to `stderr`:

 FILE* file = fopen(path, "rb");
 if (!file) {
 printf("Could not open ROM at path: %s", path);
 return;
 }

In the case that stdout was redirected to a file or a pipe, the error message may go unseen. Use fprintf(stderr, ...) or fputs(..., stderr) instead.

In the same function, you are again ignoring the return value of malloc(). Good job on checking fread()'s return value albeit.

It might be nicer to have some helper functions for reading the header, validating it, and so on.

I am not very happy to see a load_rom() function print error messages to stdout. All it should do it load ROM. Consider leaving the error messages to the caller, and return a meaningful status code. Perhaps an enum?

Use array notation for pointer arguments:

The function prototypes aren't as self-descriptive as they could be. C (since C99, or about 25 years) has a method for declaring a pointer parameter that must not be null. This is done by the rarely used syntax of putting the static keyword inside the parameter declaration:

void func(char arg[static 1])
{
 ....
}

This says that arg must point to the first element of an array of at least one element, per C 2011 [N1570] 6.7.6.3 7, which means that it can not be null.

So you can do array[static 1] to specify that array should point to at least one item.

This has some benefits:

Compilers can detect that a null pointer was passed and can output some diagnostic information. GCC has the best diagnostic information, Clang and Intel's compiler, not so much. For example, Clang and ICX only issue a warning when NULL/nullptr is passed as an argument, but not when a variable which happens to be a null pointer is passed, unlike GCC.
It better documents the code, as having at least 1 element indicates that a null pointer is not a valid value for this argument. This way, a comment about the function invoking undefined behavior on being passed a null pointer is not necessary.
It can eliminate one or more branches (assuming the called function was checking the validity of the arguments).

According to @Pascal Cuoq in this StackOverflow answer: What is the meaning of "static" in parameters array types in C?,

One common use is f(int p[static 1]), which is more or less equivalent to the notion of reference (as opposed to pointer) in other languages.

After adding the static keyword to load_rom(), it becomes:

// void load_rom(Memory* m, const char* path);
void load_rom(Memory m[static 1], const char path[static 1]);

There are 3 ways this syntax can be utilized to distinguish different cases:

A pointer to single object of type char:

void func(char arg[static 1]);

A pointer to a collection of objects of known numbers of type char:

void func(char arg[static 256]);

A pointer to a collection of objects of unknown numbers of type char:

void func(size_t n, char arg[static n]);

And of course, without the array syntax:

A pointer to a single object of type char or a null pointer:

void func(char *arg);

Note that this notation can not be used for local variables, return values, or for functions like memcpy() because arrays of void are not valid. To specify that a function returns a value that is not a null pointer, use GCC's __attribute__((returns_nonnull)). It is also supported by Clang and Intel's compiler. For void *s (and others too), you may use GCC's __attribute__((nonnull)). For local variables, wait for the C committee to add an _Optional type, or for Clang, use _Nonnull. This extension is not supported by GCC. Also note that MSVC does not support this C99 feature.

See also: Is it valid to pass the address of a non-array variable to a function parameter declared as Type ptr[static 1]?.

The value of each subsequent enumerator is one greater than the previous enumerator:

So you can just do:

typedef enum {
 REG_A,
 REG_X,
 REG_Y,
 REG_SP,
 REG_P,
} CpuReg;

instead of:

typedef enum {
 REG_A = 0,
 REG_X = 1,
 REG_Y = 2,
 REG_SP = 3,
 REG_P = 4
} CpuReg;

Restrict the scope of variables as much as possible:

In "cpu.h", we have:

Instruction** instructions = NULL;
Instruction* current_instruction = NULL;
bool first = true;
const char* INSTR_STR[0x100] = {

Yet a quick search showed that these are not used outside of this translation unit. So they should all be prefixed with the static keyword, as there's no need for them to external linkage.

Custom `typedef`s for integer types:

<stdint.h> defines fixed-width integer types like uint8_t, uint16_t, et cetera. Your codebase is using u8, u16, et cetera, which seem to be typedefs for these standard fixed-width integer types , defined in the file <defines.h>, which you did not include here. Why not simply use the standard types? Don't make us keep track of additional aliases.

Missing definitions of `bool`, `true`, and `false`:

Prior to C23, <stdbool.h> should be included for the definitions of bool, true, and false. In C23, they are keywords, and this header is not required.

You can use the iwyu (include-what-you-use) program to check if each file is self-contained. Or gcc -fsyntax-only, assuming you can use gcc.

Replace `malloc()` + `strlen()` + `strcpy()` with `strdup()`:

strdup() is part of ISO POSIX Standard that has been added to C23. Prior to C23, it is a widely-available function. If a platform does not have it, it is trivial to roll out your own. Here's one implementation:

char *strdup(const char s[static 1])
{
 const size_t size = strlen(s) + 1;
 char *const p = malloc(size);
 
 return p != NULL ? memcpy(p, s, size) : p;
}

Now text_new() can be updated to use this:

#if 0
// TEXT
Text* text_new(const char* text, SDL_Color bg, SDL_Color fg, int x, int y) {
 Text* t = malloc(sizeof(Text));
 t->text = malloc(strlen(text) + 1);
 strcpy(t->text, text);
 ...
#else
// TEXT
Text* text_new(const char* text, SDL_Color bg, SDL_Color fg, int x, int y) {
 Text* t = malloc(sizeof(Text));
 t->text = strdup(text);
 ...
}

Note that it returns a null pointer to indicate failure.

But what is with this // TEXT comment? Not helpful, elide it.

Cache `strlen()` calls:

In text_new(), you have:

// TEXT
Text* text_new(const char* text, SDL_Color bg, SDL_Color fg, int x, int y) {
 Text* t = malloc(sizeof(Text));
 t->text = malloc(strlen(text) + 1);
 ...
 t->rect.w = strlen(t->text) * FONT_SIZE;
 ...

Yet you already calculated strlen(text) in the call to malloc(), so declare a temporary variable and use that instead of calling strlen() twice.

Use a function-like macro to avoid duplication:

In "debugger.c", we have:

Text* a_text = text_new(a_reg, black, white, 10, 40);
 Text* x_text = text_new(x_reg, black, white, 11, 40 + (FONT_SIZE * 1) + 10);
 Text* y_text = text_new(y_reg, black, white, 11, 40 + (FONT_SIZE * 2) + 10);
 Text* p_text = text_new(p_reg, black, white, 11, 40 + (FONT_SIZE * 3) + 10);
 Text* sp_text = text_new(sp_reg, black, white, 11, 40 + (FONT_SIZE * 4) + 10);
 Text* pc_text = text_new(pc_reg, black, white, 11, 40 + (FONT_SIZE * 5) + 10);
 text_display(a_text, d->font, d->renderer);
 text_display(x_text, d->font, d->renderer);
 text_display(y_text, d->font, d->renderer);
 text_display(p_text, d->font, d->renderer);
 text_display(sp_text, d->font, d->renderer);
 text_display(pc_text, d->font, d->renderer);
 text_free(a_text);
 text_free(x_text);
 text_free(y_text);
 text_free(p_text);
 text_free(sp_text);
 text_free(pc_text);

Perhaps we can make a struct and loop over it?

If not, then at least we can have TEXT_FREE_ALL():

/**
 * Repeatedly calls the function FN with each argument of type TYPE *. */
#define FN_APPLY(TYPE, FN, ..) \
 do { \
 TYPE **list = (TYPE*[]){ __VA_ARGS__, nullptr}; \
 for (size_t i = 0; list[i]; ++i) \
 FN(list[i]); \
 } while (false)
/**
 * Calls the free() function individually on all arguments --- useful for
 * replacing multiple individual calls to free() like these:
 *
 * free(a);
 * free(b);
 * free(c);
 * free(d);
 */
#define FREE_ALL(...) FN_APPLY(void, free, __VA_ARGS__)

You can adjust the above for text_free().

Modifying your copy of the caller's argument does not modify the value of caller's argument:

In debugger_free(), we have:

free(d);
d = NULL;

d is a copy of the caller's pointer. d = NULL only changes debugger_free()'s copy of d, not the caller's. @Fe2O3 suggests returning NULL instead. Alternatively, you can take a pointer to a pointer.

In debugger_handle_input() there's an if(cond) return; followed by an else. You could get rid of else and gain one level of indent.

Memory leaks:

There are approximately 22 calls to instruction_new() in the code. All of them return a pointer to dynamically-allocated memory. There is no corresponding call to instruction_free() in the code. Why, such a function is not even present. It seems as if running cleanly under valgrind was not one of the goals.

Many good points, but I disagree about the array notation for pointer arguments. While there are indeed technical benefits, the notation suggests that there are arrays, but load_rom() only expects a single Memory object, not an array of them. Secondly, the idiomatic way to pass pointers to strings in C is using (const) char*. It's unfortunate C23 didn't come with a [[nonnull]]. But for readability, I would avoid the use of [static 1] here.
Thanks a lot for the time you took into reviewing my code. I will have to look into error handling in general as I didn't know of stderr, and will probably make an enum for return codes. I will also change the ranges and the defines to be safer/more portable. I just have a question: in the memory_read/write functions a is a u16 (which I defined as a uint16_t), so wouldn't it be redundant to check if its not within range? Or can type casting create errors in cases like this?
"In C23, you can do:" is premature. C23 is yet to be released. Even once it is, correct and common implementations will take some time.
@G.Sliepen Re: "load_rom() only expects a single Memory object, not an array of them." That is correct, but we can think of it as array of a single object.
@Harith if (RANGE(a, 0x0000, 0x1FFF)) or if( IS_BETWEEN( 0x0000, a, 0x1FFF ) ) to accentuate the in-between-i-ness of the test... :-)... Gotta stop this... :-)

Stack Exchange Network

NES Emulator in C

2 Answers 2

Reserved keywords:

`#pragma once` is not standard:

Prefer `static constexpr` to `#define`:

Missing documentation:

An attempt to allocate memory can fail:

`free(nullptr)` is a NOP:

Avoid unnecessary test before free

Case ranges are not standard:

Only declare as many variables as you require:

Error messages go to `stderr`:

Use array notation for pointer arguments:

The value of each subsequent enumerator is one greater than the previous enumerator:

Restrict the scope of variables as much as possible:

Custom `typedef`s for integer types:

Missing definitions of `bool`, `true`, and `false`:

Replace `malloc()` + `strlen()` + `strcpy()` with `strdup()`:

Cache `strlen()` calls:

Use a function-like macro to avoid duplication:

Modifying your copy of the caller's argument does not modify the value of caller's argument:

Memory leaks:

Header file `#include`:

DRY:

Line length:

Use more Standard Library functions:

Problematic function:

The boss is a (w)rapper:

Alternative for contiguous values

Your Answer

Sign up or log in

Post as a guest

Post as a guest

Linked

Hot Network Questions

NES Emulator in C

2 Answers 2

Reserved keywords:

#pragma once is not standard:

Prefer static constexpr to #define:

Missing documentation:

An attempt to allocate memory can fail:

free(nullptr) is a NOP:

Avoid unnecessary test before free

Case ranges are not standard:

Only declare as many variables as you require:

Error messages go to stderr:

Use array notation for pointer arguments:

The value of each subsequent enumerator is one greater than the previous enumerator:

Restrict the scope of variables as much as possible:

Custom typedefs for integer types:

Missing definitions of bool, true, and false:

Replace malloc() + strlen() + strcpy() with strdup():

Cache strlen() calls:

Use a function-like macro to avoid duplication:

Modifying your copy of the caller's argument does not modify the value of caller's argument:

Memory leaks:

Header file #include:

DRY:

Line length:

Use more Standard Library functions:

Problematic function:

The boss is a (w)rapper:

Alternative for contiguous values

Your Answer

Sign up or log in

Post as a guest

Post as a guest

Linked

Related

Hot Network Questions

`#pragma once` is not standard:

Prefer `static constexpr` to `#define`:

`free(nullptr)` is a NOP:

Error messages go to `stderr`:

Custom `typedef`s for integer types:

Missing definitions of `bool`, `true`, and `false`:

Replace `malloc()` + `strlen()` + `strcpy()` with `strdup()`:

Cache `strlen()` calls:

Header file `#include`: