Detect memory leaks in tests (#2698)

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);
+}

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);
+#define Alloc(size) Alloc_(size, __FILE__ ":" STR(__LINE__))
-void *AllocZeroed(u32 size);
+#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

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

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)
 

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 \

test/test_battle.h

@@ -614,6 +614,7 @@ struct BattleTestRunnerState
     bool8 runThen:1;
     bool8 runFinally:1;
     bool8 runningFinally:1;
+    bool8 tearDownBattle:1;
     struct BattleTestData data;
     u8 *results;
     u8 checkProgressParameter;

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_("<unknown>: %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;

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;
     }
 }