At line 20 of main.c, check_rom() is called. Soon after, load_rom() is called at line 25. This causes several problems:

check_rom() fopen()s a file and doesn't close it before returning, causing the open FILE to leak. Additionally, it unnecessarily makes any further attempt at opening the file implementation-defined behavior, although I don't know of a C implementation where opening the same file multiple times doesn't just work.

The load_rom() call also fopen()s the same file, without any checking of return values. A non-existing file is not the only cause for an error return of fopen().

Even if the above issues are fixed, there is an inherent TOCTTOU error in doing this. There is no guarantee that the file will still exist by the time load_rom() is called.

The fix is to get rid of the check_rom() function and check the return value of fopen() in load_rom().

Hi again. Someone in the thread at https://www.reddit.com/r/C_Programming/comments/lcgwj3/a_simple_and_beginner_friendly_chip8_emulator/ provided an overhauled Makefile.

I made a few small changes to their suggestion:

• The .POSIX special target was added, now the makefile is explicitly POSIX-compliant.
• CFLAGS, LDFLAGS, and LDLIBS are appended to MYCFLAGS, MYLDFLAGS, and MYLDLIBS respectively, these variables are then used by the C compiler. This allows appending extra compiler flags in pure POSIX make, no GNU extensions.
• /usr/local/include is added to the list of paths to search for includes. /usr/local/lib is searched for libraries, so I thought this would match the original intent. This change also fixes the build on OpenBSD.

Now making debug or optimized builds is easy: $ make CFLAGS='-g' Would build this program with debug symbols without touching the source code for instance.

Hi, I'm a beginner C programmer myself and I'm sending a minor patch to test the waters.

It's a bit silly to use a for loop to copy memory when we have memcpy. This patch also gets rid of the magic number 80, the sizeof operator is the right tool for this job.

As suggested by a oh5nxo on Reddit, the function load_rom() could be improved in the following way:

void load_rom(char* filename) {
    FILE* fp = fopen(filename, "rb");

    // read program size into memory
    fread(memory + 0x200, 1, sizeof(memory) - 0x200, fp);

    fclose(fp);
}

From a quick first test, it seems good. Waiting to update to give it a better look.

I found a mistake in your implementation for the OP code 0xFX55

As it is stated in the Cogwood reference:

Fx55 - LD [I], Vx Store registers V0 through Vx in memory starting at location I.

The interpreter copies the values of registers V0 through Vx into memory, starting at the address in I.

http://devernay.free.fr/hacks/chip8/C8TECH10.HTM#0.1

` // FX55: Stores V0 through Vx (Vx included) in memory starting // at addr I. case 0x0055: debug_print("[OK] 0x%X: FX55\n", op);

                for (int i = 0; i <= x; i++) {
                    V[i] = memory[I + i];
                }

                pc += 2;
                break;

`

shouldn't this segment of code be like this:

`// FX55: Stores V0 through Vx (Vx included) in memory starting // at addr I. case 0x0055: debug_print("[OK] 0x%X: FX55\n", op);

                for (int i = 0; i <= x; i++) {
                    memory[I + i] = V[i];
                }

                pc += 2;
                break;`