Enable more compiler warnings:
gcc -std=c99 -fPIC -gdwarf-4 -Wall -Wextra -Wwrite-strings -Wno-parentheses -Wpedantic -Warray-bounds -Wconversion -Wstrict-prototypes -fanalyzer brainfuck.c -o brainfuck
brainfuck.c:31:1: warning: type qualifiers ignored on function return type [-Wignored-qualifiers]
31 | const SourceFile readSourceFile(const char* filePath) {
| ^~~~~
brainfuck.c: In function ‘readSourceFile’:
brainfuck.c:44:13: warning: conversion to ‘size_t’ {aka ‘long unsigned int’} from ‘long int’ may change the sign of the result [-Wsign-conversion]
44 | lSize = ftell(fp);
| ^~~~~
brainfuck.c: In function ‘checkSourceFileValidity’:
brainfuck.c:81:26: warning: conversion to ‘uint8_t’ {aka ‘unsigned char’} from ‘char’ may change the sign of the result [-Wsign-conversion]
81 | uint8_t symbol = sourceFile->contents[i];
| ^~~~~~~~~~
brainfuck.c: In function ‘main’:
brainfuck.c:153:26: warning: conversion to ‘uint8_t’ {aka ‘unsigned char’} from ‘char’ may change the sign of the result [-Wsign-conversion]
153 | uint8_t symbol = sourceFile.contents[i];
| ^~~~~~~~~~
brainfuck.c:172:38: warning: conversion from ‘int’ to ‘uint8_t’ {aka ‘unsigned char’} may change value [-Wconversion]
172 | tape[tapePosition] = getchar();
| ^~~~~~~
brainfuck.c:134:5: warning: use of possibly-NULL ‘tape’ where non-null expected [CWE-690] [-Wanalyzer-possible-null-argument]
134 | memset(tape, 0, tapeSize * sizeof(uint8_t));
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
‘main’: events 1-4
|
| 108 | if (argc < 2) {
| | ^
| | |
| | (1) following ‘false’ branch (when ‘argc > 1’)...
|......
| 118 | if (argc == 3) {
| | ~
| | |
| | (2) ...to here
|......
| 133 | uint8_t* tape = (uint8_t*)malloc(tapeSize * sizeof(uint8_t));
| | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| | |
| | (3) this call could return NULL
| 134 | memset(tape, 0, tapeSize * sizeof(uint8_t));
| | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| | |
| | (4) argument 1 (‘tape’) from (3) could be NULL where non-null expected
|
In file included from brainfuck.c:7:
/usr/include/string.h:61:14: note: argument 1 of ‘memset’ must be non-null
61 | extern void *memset (void *__s, int __c, size_t __n) __THROW __nonnull ((1));
| ^~~~~~
These are all avoidable.
Undefined behavior:
From the C Standard: (footnote 234 on p. 267 of the linked Standard)
Setting the file position indicator to end-of-file, as with fseek(file, 0, SEEK_END),has undefined behavior for a binary stream (because of possible trailing null characters) or for anystream with state-dependent encoding that does not assuredly end in the initial shift state.
The call to fseek() in readSourceFile() invokes undefined behavior.
Incorrect instructions:
$ ./brainfuck in.txt acksnca
Tape size out of range. Tape size must be greater than 0 and less than or equal to 18446744073709551615
But when the third argument is 18446744073709551615, it is rejected as invalid input:
./brainfuck in.txt 18446744073709551615
Tape size out of range. Tape size must be greater than 0 and less than or equal to 18446744073709551615
And when it is one less than the max size, a segmentation fault occurs.
/brainfuck in.txt 18446744073709551614
Segmentation fault (core dumped)
If the converted value falls out of range of corresponding return type, a range error occurs (setting errno to ERANGE) and INTMAX_MAX, INTMAX_MIN, UINTMAX_MAX or 0 is returned, as appropriate.
We are not handling the return value of strtoumax() (and malloc()) correctly.
The interpreter doesn't properly detect if the contents of the input file are valid Brainfuck code. For instance, this:
/* The contents of the file being "HELLO": */
$ ./brainfuck in.txt
outputs nothing.
Declare variables where they are needed:
Starting from C99, variables do not have to be declared at the beginning of a function (or block); instead, you can declare them at the point where they are needed within the function's body.
fopen() is not required to set errno:
The error-handling is inconsistent. At times, we are using perror(), and at times we're calling fputs() with an error message. Consider:
#if 0
fp = fopen (filePath , "rb");
if(!fp) {
perror(filePath);
exit(IO_ERR);
}
#else
errno = 0;
fp = fopen(filePath, "rb");
/* The cast void is required as ISO C forbids conditional expr with only one
* void side.
*/
if (!fp) {
errno ? perror(filePath) : (void) fputs("Error - failed to open file.", stderr);
exit(IO_ERR);
}
#endif
Noise:
const serves no purpose with const SourceFile readSourceFile in the declaration const SourceFile readSourceFile(const char* filePath). It is ignored.
Send error messages to stderr:
if (argc < 2) {
// puts("Error! Expected path to source file as command line argument.");
fputs("Error! Expected path to source file as command line argument.\n", stderr);
return USAGE_ERR;
}
Note that fputs() doesn't automatically append a newline like puts().
Minor:
/*
if we have 3 arguments, 3rd argument will be the tape size in bytes
we will try and parse it as a size_t
*/
Does it matter if there are more than 3 arguments?
Use more const:
In main():
// char* str = argv[2];
const char *str = argv[2];
Ignoring the return of malloc() and family risks invoking undefined behavior:
// Initial tape of memory and zero it all
uint8_t* tape = (uint8_t*)malloc(tapeSize * sizeof(uint8_t));
memset(tape, 0, tapeSize * sizeof(uint8_t));
malloc() and family returns NULL to indicate failure. The returned void * need not be casted as there is an implicit conversion to and from a void * to any other pointer type in C.
/* Allocate to the referenced object, not the type.
* It is easier to maintain.
*/
uint8_t tape = malloc(tapesize * sizeof *tape);
if (!tape) {
complain();
..
}
As suggested in the comments, malloc() + memset() can be replaced with calloc().
The return values of fseek() and ftell() are also ignored.
getchar() returns an int, not a uint8_t:
// ??
case ',': {
tape[tapePosition] = getchar();
break;
}
We are missing a \n in all the calls to fprintf() and fputs().