From 76f02774ea645eb285731b49f9c829bc17341b8b Mon Sep 17 00:00:00 2001 From: Martin Griffin Date: Thu, 20 Apr 2023 20:35:22 +0100 Subject: [PATCH] Detect memory leaks in tests Can use KNOWN_LEAKING; to specify that a test is known to leak memory. The location information is available in regular game builds. Thus it is available for use in debugging leaks in-game too. In the future we should consider replacing it with NULL if NDEBUG is defined. This is not currently possible because the tests do not force NDEBUG to be undefined. --- gflib/malloc.c | 69 +++++++++++++++++++-------------------- gflib/malloc.h | 41 +++++++++++++++++++++-- include/gba/defines.h | 3 ++ include/global.h | 3 ++ test/test.h | 5 +++ test/test_battle.h | 1 + test/test_runner.c | 26 +++++++++++++++ test/test_runner_battle.c | 10 ++++++ 8 files changed, 120 insertions(+), 38 deletions(-) diff --git a/gflib/malloc.c b/gflib/malloc.c index d0b949763..dcfac6ee8 100644 --- a/gflib/malloc.c +++ b/gflib/malloc.c @@ -1,37 +1,18 @@ #include "global.h" +#include "malloc.h" static void *sHeapStart; static u32 sHeapSize; -#define MALLOC_SYSTEM_ID 0xA3A3 - -struct MemBlock { - // Whether this block is currently allocated. - bool16 flag; - - // Magic number used for error checking. Should equal MALLOC_SYSTEM_ID. - u16 magic; - - // Size of the block (not including this header struct). - u32 size; - - // Previous block pointer. Equals sHeapStart if this is the first block. - struct MemBlock *prev; - - // Next block pointer. Equals sHeapStart if this is the last block. - struct MemBlock *next; - - // Data in the memory block. (Arrays of length 0 are a GNU extension.) - u8 data[0]; -}; - void PutMemBlockHeader(void *block, struct MemBlock *prev, struct MemBlock *next, u32 size) { struct MemBlock *header = (struct MemBlock *)block; - header->flag = FALSE; + header->allocated = FALSE; + header->locationHi = 0; header->magic = MALLOC_SYSTEM_ID; header->size = size; + header->locationLo = 0; header->prev = prev; header->next = next; } @@ -41,7 +22,7 @@ void PutFirstMemBlockHeader(void *block, u32 size) PutMemBlockHeader(block, (struct MemBlock *)block, (struct MemBlock *)block, size - sizeof(struct MemBlock)); } -void *AllocInternal(void *heapStart, u32 size) +void *AllocInternal(void *heapStart, u32 size, const char *location) { struct MemBlock *pos = (struct MemBlock *)heapStart; struct MemBlock *head = pos; @@ -55,14 +36,14 @@ void *AllocInternal(void *heapStart, u32 size) for (;;) { // Loop through the blocks looking for unused block that's big enough. - if (!pos->flag) { + if (!pos->allocated) { foundBlockSize = pos->size; if (foundBlockSize >= size) { if (foundBlockSize - size < 2 * sizeof(struct MemBlock)) { // The block isn't much bigger than the requested size, // so just use it. - pos->flag = TRUE; + pos->allocated = TRUE; } else { // The block is significantly bigger than the requested // size, so split the rest into a separate block. @@ -71,7 +52,7 @@ void *AllocInternal(void *heapStart, u32 size) splitBlock = (struct MemBlock *)(pos->data + size); - pos->flag = TRUE; + pos->allocated = TRUE; pos->size = size; PutMemBlockHeader(splitBlock, pos, pos->next, foundBlockSize); @@ -82,6 +63,9 @@ void *AllocInternal(void *heapStart, u32 size) splitBlock->next->prev = splitBlock; } + pos->locationHi = ((uintptr_t)location) >> 14; + pos->locationLo = (uintptr_t)location; + return pos->data; } } @@ -98,12 +82,12 @@ void FreeInternal(void *heapStart, void *pointer) if (pointer) { struct MemBlock *head = (struct MemBlock *)heapStart; struct MemBlock *block = (struct MemBlock *)((u8 *)pointer - sizeof(struct MemBlock)); - block->flag = FALSE; + block->allocated = FALSE; // If the freed block isn't the last one, merge with the next block // if it's not in use. if (block->next != head) { - if (!block->next->flag) { + if (!block->next->allocated) { block->size += sizeof(struct MemBlock) + block->next->size; block->next->magic = 0; block->next = block->next->next; @@ -115,7 +99,7 @@ void FreeInternal(void *heapStart, void *pointer) // If the freed block isn't the first one, merge with the previous block // if it's not in use. if (block != head) { - if (!block->prev->flag) { + if (!block->prev->allocated) { block->prev->next = block->next; if (block->next != head) @@ -128,9 +112,9 @@ void FreeInternal(void *heapStart, void *pointer) } } -void *AllocZeroedInternal(void *heapStart, u32 size) +void *AllocZeroedInternal(void *heapStart, u32 size, const char *location) { - void *mem = AllocInternal(heapStart, size); + void *mem = AllocInternal(heapStart, size, location); if (mem != NULL) { if (size & 3) @@ -175,14 +159,14 @@ void InitHeap(void *heapStart, u32 heapSize) PutFirstMemBlockHeader(heapStart, heapSize); } -void *Alloc(u32 size) +void *Alloc_(u32 size, const char *location) { - return AllocInternal(sHeapStart, size); + return AllocInternal(sHeapStart, size, location); } -void *AllocZeroed(u32 size) +void *AllocZeroed_(u32 size, const char *location) { - return AllocZeroedInternal(sHeapStart, size); + return AllocZeroedInternal(sHeapStart, size, location); } void Free(void *pointer) @@ -207,3 +191,16 @@ bool32 CheckHeap() return TRUE; } + +const struct MemBlock *HeapHead(void) +{ + return (const struct MemBlock *)sHeapStart; +} + +const char *MemBlockLocation(const struct MemBlock *block) +{ + if (!block->allocated) + return NULL; + + return (const char *)(ROM_START | (block->locationHi << 14) | block->locationLo); +} diff --git a/gflib/malloc.h b/gflib/malloc.h index 851db83a6..f484b1580 100644 --- a/gflib/malloc.h +++ b/gflib/malloc.h @@ -11,11 +11,48 @@ #define TRY_FREE_AND_SET_NULL(ptr) if (ptr != NULL) FREE_AND_SET_NULL(ptr) +#define MALLOC_SYSTEM_ID 0xA3A3 + +struct MemBlock +{ + // Whether this block is currently allocated. + u16 allocated:1; + + u16 unused_00:4; + + // High 11 bits of location pointer. + u16 locationHi:11; + + // Magic number used for error checking. Should equal MALLOC_SYSTEM_ID. + u16 magic; + + // Size of the block (not including this header struct). + u32 size:18; + + // Low 14 bits of location pointer. + u32 locationLo:14; + + // Previous block pointer. Equals sHeapStart if this is the first block. + struct MemBlock *prev; + + // Next block pointer. Equals sHeapStart if this is the last block. + struct MemBlock *next; + + // Data in the memory block. (Arrays of length 0 are a GNU extension.) + u8 data[0]; +}; + extern u8 gHeap[]; -void *Alloc(u32 size); -void *AllocZeroed(u32 size); +#define Alloc(size) Alloc_(size, __FILE__ ":" STR(__LINE__)) +#define AllocZeroed(size) AllocZeroed_(size, __FILE__ ":" STR(__LINE__)) + +void *Alloc_(u32 size, const char *location); +void *AllocZeroed_(u32 size, const char *location); void Free(void *pointer); void InitHeap(void *pointer, u32 size); +const struct MemBlock *HeapHead(void); +const char *MemBlockLocation(const struct MemBlock *block); + #endif // GUARD_ALLOC_H diff --git a/include/gba/defines.h b/include/gba/defines.h index 82caf56e6..9959a17ef 100644 --- a/include/gba/defines.h +++ b/include/gba/defines.h @@ -22,6 +22,9 @@ #define INTR_CHECK (*(u16 *)0x3007FF8) #define INTR_VECTOR (*(void **)0x3007FFC) +#define ROM_START 0x8000000 +#define ROM_END 0xA000000 + #define EWRAM_START 0x02000000 #define EWRAM_END (EWRAM_START + 0x40000) #define IWRAM_START 0x03000000 diff --git a/include/global.h b/include/global.h index c20f2ed42..eca45bdb6 100644 --- a/include/global.h +++ b/include/global.h @@ -147,6 +147,9 @@ #define CAT(a, b) CAT_(a, b) #define CAT_(a, b) a ## b +#define STR(a) STR_(a) +#define STR_(a) #a + // Converts a string to a compound literal, essentially making it a pointer to const u8 #define COMPOUND_STRING(str) (const u8[]) _(str) diff --git a/test/test.h b/test/test.h index dfa1f12b1..bf3e0b004 100644 --- a/test/test.h +++ b/test/test.h @@ -46,6 +46,7 @@ struct TestRunnerState u8 result; u8 expectedResult; + bool8 expectLeaks:1; u32 timeoutSeconds; }; @@ -69,6 +70,7 @@ extern struct TestRunnerState gTestRunnerState; void CB2_TestRunner(void); void Test_ExpectedResult(enum TestResult); +void Test_ExpectLeaks(bool32); void Test_ExitWithResult(enum TestResult, const char *fmt, ...); s32 MgbaPrintf_(const char *fmt, ...); @@ -160,6 +162,9 @@ s32 MgbaPrintf_(const char *fmt, ...); #define KNOWN_FAILING \ Test_ExpectedResult(TEST_RESULT_FAIL) +#define KNOWN_LEAKING \ + Test_ExpectLeaks(TRUE) + #define PARAMETRIZE if (gFunctionTestRunnerState->parameters++ == gFunctionTestRunnerState->runParameter) #define TO_DO \ diff --git a/test/test_battle.h b/test/test_battle.h index 967bb661d..3597e9d16 100644 --- a/test/test_battle.h +++ b/test/test_battle.h @@ -615,6 +615,7 @@ struct BattleTestRunnerState bool8 runThen:1; bool8 runFinally:1; bool8 runningFinally:1; + bool8 tearDownBattle:1; struct BattleTestData data; u8 *results; u8 checkProgressParameter; diff --git a/test/test_runner.c b/test/test_runner.c index ef9f2da73..64cb20e26 100644 --- a/test/test_runner.c +++ b/test/test_runner.c @@ -111,6 +111,7 @@ void CB2_TestRunner(void) gTestRunnerState.state = STATE_REPORT_RESULT; gTestRunnerState.result = TEST_RESULT_PASS; gTestRunnerState.expectedResult = TEST_RESULT_PASS; + gTestRunnerState.expectLeaks = FALSE; if (gTestRunnerHeadless) gTestRunnerState.timeoutSeconds = TIMEOUT_SECONDS; else @@ -141,6 +142,26 @@ void CB2_TestRunner(void) if (gTestRunnerState.test->runner->tearDown) gTestRunnerState.test->runner->tearDown(gTestRunnerState.test->data); + if (!gTestRunnerState.expectLeaks) + { + const struct MemBlock *head = HeapHead(); + const struct MemBlock *block = head; + do + { + if (block->allocated) + { + const char *location = MemBlockLocation(block); + if (location) + MgbaPrintf_("%s: %d bytes not freed", location, block->size); + else + MgbaPrintf_(": %d bytes not freed", block->size); + gTestRunnerState.result = TEST_RESULT_FAIL; + } + block = block->next; + } + while (block != head); + } + if (gTestRunnerState.test->runner == &gAssumptionsRunner) { if (gTestRunnerState.result != TEST_RESULT_PASS) @@ -238,6 +259,11 @@ void Test_ExpectedResult(enum TestResult result) gTestRunnerState.expectedResult = result; } +void Test_ExpectLeaks(bool32 expectLeaks) +{ + gTestRunnerState.expectLeaks = expectLeaks; +} + static void FunctionTest_SetUp(void *data) { (void)data; diff --git a/test/test_runner_battle.c b/test/test_runner_battle.c index 365120f13..8654f78cd 100644 --- a/test/test_runner_battle.c +++ b/test/test_runner_battle.c @@ -893,6 +893,15 @@ static void BattleTest_TearDown(void *data) { if (STATE) { + // Free resources that aren't cleaned up when the battle was + // aborted unexpectedly. + if (STATE->tearDownBattle) + { + FreeMonSpritesGfx(); + FreeBattleSpritesData(); + FreeBattleResources(); + FreeAllWindowBuffers(); + } FREE_AND_SET_NULL(STATE->results); FREE_AND_SET_NULL(STATE); } @@ -923,6 +932,7 @@ static bool32 BattleTest_HandleExitWithResult(void *data, enum TestResult result } else { + STATE->tearDownBattle = TRUE; return FALSE; } }