From 7398898a61afb5a668d1ae665bae93a980973cea Mon Sep 17 00:00:00 2001 From: Kayne Ruse Date: Sun, 17 Nov 2024 18:49:40 +1100 Subject: [PATCH] Added valgrind to the CI, fixed tests This exposed an issue with my dev environment, which I had to patch. Fixed #153 --- .../workflows/continuous-integration-v2.yml | 19 +++++++++++++++++++ .notes/raspberry-pi-issues.txt | 8 ++++++++ repl/main.c | 9 +++++---- source/toy_bucket.c | 4 +--- source/toy_bytecode.c | 2 ++ source/toy_parser.c | 3 ++- source/toy_scope.c | 12 +++++------- source/toy_string.c | 8 +++++++- source/toy_vm.c | 15 ++++++++++----- tests/cases/makefile | 2 +- tests/cases/test_string.c | 4 ++-- tests/integrations/makefile | 2 +- 12 files changed, 63 insertions(+), 25 deletions(-) create mode 100644 .notes/raspberry-pi-issues.txt diff --git a/.github/workflows/continuous-integration-v2.yml b/.github/workflows/continuous-integration-v2.yml index 86657db..7503a5d 100644 --- a/.github/workflows/continuous-integration-v2.yml +++ b/.github/workflows/continuous-integration-v2.yml @@ -60,3 +60,22 @@ jobs: - name: run the tests if: (matrix.commands.gdb == true && matrix.platforms.gdb_enabled == false) != true run: ${{ matrix.commands.exec }} + + run-test-valgrind: + needs: run-test-integrations + continue-on-error: true + strategy: + matrix: + platforms: + - { os: ubuntu-latest, preinstall: sudo apt-get install valgrind } + commands: + - { exec: make tests-valgrind } + + runs-on: ${{ matrix.platforms.os }} + steps: + - uses: actions/checkout@v4 + - name: Preinstall dependencies + run: ${{ matrix.platforms.preinstall }} + - name: run the tests + run: ${{ matrix.commands.exec }} + diff --git a/.notes/raspberry-pi-issues.txt b/.notes/raspberry-pi-issues.txt new file mode 100644 index 0000000..aea0bf1 --- /dev/null +++ b/.notes/raspberry-pi-issues.txt @@ -0,0 +1,8 @@ +The default version of GCC that ships on Raspian has an issue. The file '/usr/lib/arm-linux-gnueabihf/libarmmem-v7l.so' has a faulty implementation of 'memcpy()' and possibly 'memset()'. Changing to the newer versions doens't work, as they're just symlinks to v7. + +To resolve this, download and build this shared object: + +https://github.com/simonjhall/copies-and-fills + +Then, set the linker's preload value to point to that '.so' file (You may need to edit '/etc/ld.so.preload') + diff --git a/repl/main.c b/repl/main.c index cc600fe..940869a 100644 --- a/repl/main.c +++ b/repl/main.c @@ -9,9 +9,9 @@ strncpy((dest) + (strlen(dest)), (src), strlen((src)) + 1); #if defined(_WIN32) || defined(_WIN64) - #define FLIPSLASH(str) for (int i = 0; str[i]; i++) str[i] = str[i] == '/' ? '\\' : str[i]; + #define FLIPSLASH(str) for (int i = 0; str != NULL && str[i]; i++) str[i] = str[i] == '/' ? '\\' : str[i]; #else - #define FLIPSLASH(str) for (int i = 0; str[i]; i++) str[i] = str[i] == '\\' ? '/' : str[i]; + #define FLIPSLASH(str) for (int i = 0; str != NULL && str[i]; i++) str[i] = str[i] == '\\' ? '/' : str[i]; #endif unsigned char* readFile(char* path, int* size) { @@ -219,8 +219,9 @@ CmdLine parseCmdLine(int argc, const char* argv[]) { i++; //total space to reserve - it's actually longer than needed, due to the exe name being removed - cmd.infileLength = strlen(argv[0]) + strlen(argv[i]); - cmd.infile = malloc(cmd.infileLength + 1); + cmd.infileLength = strlen(argv[0]) + strlen(argv[i]) + 1; + cmd.infileLength = (cmd.infileLength + 3) & ~3; //BUGFIX: align to word size for malloc() + cmd.infile = malloc(cmd.infileLength); if (cmd.infile == NULL) { fprintf(stderr, TOY_CC_ERROR "ERROR: Failed to allocate space while parsing the command line, exiting\n" TOY_CC_RESET); diff --git a/source/toy_bucket.c b/source/toy_bucket.c index 9363481..22d1a7a 100644 --- a/source/toy_bucket.c +++ b/source/toy_bucket.c @@ -33,9 +33,7 @@ void* Toy_partitionBucket(Toy_Bucket** bucketHandle, unsigned int amount) { } //BUGFIX: the endpoint must be aligned to the word size, otherwise you'll get a bus error from moving pointers - if (amount % 4 != 0) { - amount += 4 - (amount % 4); //ceil - } + amount = (amount + 3) & ~3; //if you try to allocate too much space if ((*bucketHandle)->capacity < amount) { diff --git a/source/toy_bytecode.c b/source/toy_bytecode.c index 96fe889..fdb7ddc 100644 --- a/source/toy_bytecode.c +++ b/source/toy_bytecode.c @@ -68,6 +68,8 @@ static void writeBytecodeBody(Toy_Bytecode* bc, Toy_Ast* ast) { memcpy(bc->ptr + bc->count, module, len); bc->count += len; bc->moduleCount++; + + free(module); } //exposed functions diff --git a/source/toy_parser.c b/source/toy_parser.c index 1293117..23fbc35 100644 --- a/source/toy_parser.c +++ b/source/toy_parser.c @@ -379,7 +379,8 @@ static Toy_AstFlag literal(Toy_Bucket** bucketHandle, Toy_Parser* parser, Toy_As } while (parser->previous.lexeme[o++] && i < parser->previous.length); buffer[i] = '\0'; - Toy_private_emitAstValue(bucketHandle, rootHandle, TOY_VALUE_FROM_STRING(Toy_createStringLength(bucketHandle, buffer, i - escapeCounter))); + unsigned int len = ((i - escapeCounter) + 3) & ~3; + Toy_private_emitAstValue(bucketHandle, rootHandle, TOY_VALUE_FROM_STRING(Toy_createStringLength(bucketHandle, buffer, len))); return TOY_AST_FLAG_NONE; } diff --git a/source/toy_scope.c b/source/toy_scope.c index c3dd8de..af61695 100644 --- a/source/toy_scope.c +++ b/source/toy_scope.c @@ -17,6 +17,10 @@ static void incrementRefCount(Toy_Scope* scope) { static void decrementRefCount(Toy_Scope* scope) { for (Toy_Scope* iter = scope; iter; iter = iter->next) { iter->refCount--; + if (iter->refCount == 0 && iter->table != NULL) { + Toy_freeTable(iter->table); + iter->table = NULL; + } } } @@ -64,12 +68,6 @@ Toy_Scope* Toy_popScope(Toy_Scope* scope) { } decrementRefCount(scope); - - if (scope->refCount == 0) { - Toy_freeTable(scope->table); - scope->table = NULL; - } - return scope->next; } @@ -86,7 +84,7 @@ Toy_Scope* Toy_deepCopyScope(Toy_Bucket** bucketHandle, Toy_Scope* scope) { //forcibly copy the contents for (int i = 0; i < scope->table->capacity; i++) { if (!TOY_VALUE_IS_NULL(scope->table->data[i].key)) { - Toy_insertTable(&newScope->table, scope->table->data[i].key, scope->table->data[i].value); + Toy_insertTable(&newScope->table, Toy_copyValue(scope->table->data[i].key), Toy_copyValue(scope->table->data[i].value)); } } diff --git a/source/toy_string.c b/source/toy_string.c index 17f9aa5..394dd88 100644 --- a/source/toy_string.c +++ b/source/toy_string.c @@ -229,7 +229,13 @@ char* Toy_getStringRawBuffer(Toy_String* str) { exit(-1); } - char* buffer = malloc(str->length + 1); + //BUGFIX: Make sure it's aligned, and there's space for the null + int len = (str->length + 3) & ~3; + if (len == str->length) { + len += 4; + } + + char* buffer = malloc(len); deepCopyUtil(buffer, str); buffer[str->length] = '\0'; diff --git a/source/toy_vm.c b/source/toy_vm.c index b3bd54b..e19d781 100644 --- a/source/toy_vm.c +++ b/source/toy_vm.c @@ -651,12 +651,17 @@ void Toy_bindVMToModule(Toy_VM* vm, unsigned char* module) { vm->subsAddr = READ_UNSIGNED_INT(vm); } - //allocate the stack, scope, and memory - vm->stringBucket = Toy_allocateBucket(TOY_BUCKET_IDEAL); - vm->scopeBucket = Toy_allocateBucket(TOY_BUCKET_SMALL); - vm->stack = Toy_allocateStack(); + //allocate the stack, scope, and memory (skip if already in use) + if (vm->stringBucket == NULL) { + vm->stringBucket = Toy_allocateBucket(TOY_BUCKET_IDEAL); + } + if (vm->scopeBucket == NULL) { + vm->scopeBucket = Toy_allocateBucket(TOY_BUCKET_SMALL); + } + if (vm->stack == NULL) { + vm->stack = Toy_allocateStack(); + } if (vm->scope == NULL) { - //only allocate a new top-level scope when needed, otherwise REPL will break vm->scope = Toy_pushScope(&vm->scopeBucket, NULL); } } diff --git a/tests/cases/makefile b/tests/cases/makefile index edf68fe..22c8cf6 100644 --- a/tests/cases/makefile +++ b/tests/cases/makefile @@ -81,4 +81,4 @@ build-run-valgrind: $(addprefix $(TEST_OUTDIR)/,$(notdir $(TEST_CASESFILES:%.c=% .PRECIOUS: $(TEST_OUTDIR)/%.run-valgrind $(TEST_OUTDIR)/%.run-valgrind: $(TEST_OUTDIR)/%.exe - valgrind $< --leak-check=full --show-leak-kinds=all --track-origins=yes --verbose + valgrind --leak-check=full --show-leak-kinds=all --track-origins=yes $< diff --git a/tests/cases/test_string.c b/tests/cases/test_string.c index f455e3b..c09a458 100644 --- a/tests/cases/test_string.c +++ b/tests/cases/test_string.c @@ -210,7 +210,7 @@ int test_string_concatenation() { //check the refcounts if (strlen(buffer) != 11 || - strcmp(buffer, "Hello world") != 0) + strncmp(buffer, "Hello world", 11) != 0) { fprintf(stderr, TOY_CC_ERROR "ERROR: Failed to get the raw buffer from concatenated string\n" TOY_CC_RESET); free(buffer); @@ -267,7 +267,7 @@ int test_string_with_stressed_bucket() { //grab the buffer char* buffer = Toy_getStringRawBuffer(str); - if (strcmp(buffer, "thequickbrownfoxjumpedoverthelazydog") != 0 || + if (strncmp(buffer, "thequickbrownfoxjumpedoverthelazydog", 36) != 0 || strlen(buffer) != 36) { fprintf(stderr, TOY_CC_ERROR "ERROR: Unexpected state of the raw buffer after string stress test: '%s'\n" TOY_CC_RESET, buffer); diff --git a/tests/integrations/makefile b/tests/integrations/makefile index ef48e70..af71489 100644 --- a/tests/integrations/makefile +++ b/tests/integrations/makefile @@ -49,7 +49,7 @@ valgrind: source repl run-valgrind run-valgrind: $(TEST_SCRIPTFILES:.toy=.toy-valgrind) %.toy-valgrind: %.toy - valgrind --leak-check=full --show-leak-kinds=all --track-origins=yes --verbose $(TEST_OUTDIR)/$(TEST_REPLNAME) -f ../$< --verbose + valgrind --leak-check=full --show-leak-kinds=all --track-origins=yes $(TEST_OUTDIR)/$(TEST_REPLNAME) -f ../$< --verbose #compile the source and repl first source: $(TEST_OBJDIR) $(TEST_OUTDIR)