Merge branch 'bugfix/heap_fragmentation_on_split_pr1761' into 'master'

heap: Try to merge tail with next block when splitting

See merge request idf/esp-idf!2147

.gitlab-ci.yml

@@ -273,7 +273,7 @@ test_multi_heap_on_host:
     - wl_host_test
   script:
     - cd components/heap/test_multi_heap_host
-    - make test
+    - ./test_all_configs.sh
 
 test_build_system:
   stage: test

components/heap/multi_heap.c

@@ -131,6 +131,12 @@ static inline bool is_free(const heap_block_t *block)
     return block->header & BLOCK_FREE_FLAG;
 }
 
+/* Return true if this block is the first in the heap */
+static inline bool is_first_block(const heap_t *heap, const heap_block_t *block)
+{
+    return (block == &heap->first_block);
+}
+
 /* Return true if this block is the last_block in the heap
    (the only block with no next pointer) */
 static inline bool is_last_block(const heap_block_t *block)
@@ -175,7 +181,7 @@ static void assert_valid_block(const heap_t *heap, const heap_block_t *block)
 */
 static heap_block_t *get_prev_free_block(heap_t *heap, const heap_block_t *block)
 {
-    assert(block != &heap->first_block); /* can't look for a block before first_block */
+    assert(!is_first_block(heap, block)); /* can't look for a block before first_block */
 
     for (heap_block_t *b = &heap->first_block; b != NULL && b < block; b = b->next_free) {
         MULTI_HEAP_ASSERT(is_free(b), b); // Block should be free
@@ -206,7 +212,7 @@ static heap_block_t *merge_adjacent(heap_t *heap, heap_block_t *a, heap_block_t
     if (is_last_block(b)) {
         return a;
     }
-    if (a == &heap->first_block) {
+    if (is_first_block(heap, a)) {
         return b;
     }
 
@@ -258,33 +264,52 @@ static heap_block_t *merge_adjacent(heap_t *heap, heap_block_t *a, heap_block_t
 */
 static void split_if_necessary(heap_t *heap, heap_block_t *block, size_t size, heap_block_t *prev_free_block)
 {
+    const size_t block_size = block_data_size(block);
     MULTI_HEAP_ASSERT(!is_free(block), block); // split block shouldn't be free
-    MULTI_HEAP_ASSERT(size <= block_data_size(block), block); // size should be valid
+    MULTI_HEAP_ASSERT(size <= block_size, block); // size should be valid
     size = ALIGN_UP(size);
 
     /* can't split the head or tail block */
-    assert(block != &heap->first_block);
+    assert(!is_first_block(heap, block));
     assert(!is_last_block(block));
 
-    if (block_data_size(block) < size + sizeof(heap_block_t)) {
-        /* Can't split 'block' if we're not going to get a usable free block afterwards */
-        return;
-    }
-
-    /* Block is larger than it needs to be, insert a new free block after it */
     heap_block_t *new_block = (heap_block_t *)(block->data + size);
-    new_block->header = block->header | BLOCK_FREE_FLAG;
-    block->header = (intptr_t)new_block;
+    heap_block_t *next_block = get_next_block(block);
 
-    if (prev_free_block == NULL) {
-        prev_free_block = get_prev_free_block(heap, block);
+    if (is_free(next_block) && !is_last_block(next_block)) {
+        /* The next block is free, just extend it upwards. */
+        new_block->header = next_block->header;
+        new_block->next_free = next_block->next_free;
+        if (prev_free_block == NULL) {
+            prev_free_block = get_prev_free_block(heap, block);
+        }
+        /* prev_free_block should point to the next block (which we found to be free). */
+        MULTI_HEAP_ASSERT(prev_free_block->next_free == next_block,
+                          &prev_free_block->next_free); // free blocks should be in order
+        /* Note: We have not introduced a new block header, hence the simple math. */
+        heap->free_bytes += block_size - size;
+#ifdef MULTI_HEAP_POISONING_SLOW
+        /* next_block header needs to be replaced with a fill pattern */
+        multi_heap_internal_poison_fill_region(next_block, sizeof(heap_block_t), true /* free */);
+#endif
+    } else {
+        /* Insert a free block between the current and the next one. */
+        if (block_data_size(block) < size + sizeof(heap_block_t)) {
+            /* Can't split 'block' if we're not going to get a usable free block afterwards */
+            return;
+        }
+        if (prev_free_block == NULL) {
+            prev_free_block = get_prev_free_block(heap, block);
+        }
+        new_block->header = block->header | BLOCK_FREE_FLAG;
+        new_block->next_free = prev_free_block->next_free;
+        /* prev_free_block should point to a free block after new_block */
+        MULTI_HEAP_ASSERT(prev_free_block->next_free > new_block,
+                          &prev_free_block->next_free); // free blocks should be in order
+        heap->free_bytes += block_data_size(new_block);
     }
-    /* prev_free_block should point to a free block after new_block */
-    MULTI_HEAP_ASSERT(prev_free_block->next_free > new_block,
-                      &prev_free_block->next_free); // free blocks should be in order
-    new_block->next_free = prev_free_block->next_free;
+    block->header = (intptr_t)new_block;
     prev_free_block->next_free = new_block;
-    heap->free_bytes += block_data_size(new_block);
 }
 
 void *multi_heap_get_block_address_impl(multi_heap_block_handle_t block)
@@ -448,7 +473,7 @@ void multi_heap_free_impl(multi_heap_handle_t heap, void *p)
     assert_valid_block(heap, pb);
     MULTI_HEAP_ASSERT(!is_free(pb), pb); // block should not be free
     MULTI_HEAP_ASSERT(!is_last_block(pb), pb); // block should not be last block
-    MULTI_HEAP_ASSERT(pb != &heap->first_block, pb); // block should not be first block
+    MULTI_HEAP_ASSERT(!is_first_block(heap, pb), pb); // block should not be first block
 
     heap_block_t *next = get_next_block(pb);
 
@@ -604,19 +629,21 @@ bool multi_heap_check(multi_heap_handle_t heap, bool print_errors)
             FAIL_PRINT("CORRUPT HEAP: Block %p is outside heap (last valid block %p)\n", b, prev);
             goto done;
         }
-        prev = b;
-
         if (is_free(b)) {
+            if (prev != NULL && is_free(prev) && !is_first_block(heap, prev) && !is_last_block(b)) {
+                FAIL_PRINT("CORRUPT HEAP: Two adjacent free blocks found, %p and %p\n", prev, b);
+            }
             if (expected_free != NULL && expected_free != b) {
                 FAIL_PRINT("CORRUPT HEAP: Prev free block %p pointed to next free %p but this free block is %p\n",
                        prev_free, expected_free, b);
             }
             prev_free = b;
             expected_free = b->next_free;
-            if (b != &heap->first_block) {
+            if (!is_first_block(heap, b)) {
                 total_free_bytes += block_data_size(b);
             }
         }
+        prev = b;
 
 #ifdef MULTI_HEAP_POISONING
         if (!is_last_block(b)) {

components/heap/multi_heap_config.h

@@ -14,8 +14,8 @@
 #pragma once
 
 #ifdef ESP_PLATFORM
-
 #include "sdkconfig.h"
+#endif
 
 /* Configuration macros for multi-heap */
 
@@ -27,11 +27,3 @@
 #define MULTI_HEAP_POISONING
 #define MULTI_HEAP_POISONING_SLOW
 #endif
-
-#else /* !ESP_PLATFORM */
-
-/* Host-side tests, enable full poisoning */
-#define MULTI_HEAP_POISONING
-#define MULTI_HEAP_POISONING_SLOW
-
-#endif

components/heap/multi_heap_poisoning.c

@@ -244,6 +244,8 @@ void *multi_heap_realloc(multi_heap_handle_t heap, void *p, size_t size)
        place.)
 
        For now we just malloc a new buffer, copy, and free. :|
+
+       Note: If this ever changes, multi_heap defrag realloc test should be enabled.
     */
     size_t orig_alloc_size = head->alloc_size;
 

components/heap/test_multi_heap_host/test_all_configs.sh

@@ -0,0 +1,20 @@
+#!/bin/bash
+#
+# Run the test suite with all configurations enabled
+#
+
+FAIL=0
+
+for FLAGS in "CONFIG_HEAP_POISONING_NONE" "CONFIG_HEAP_POISONING_LIGHT" "CONFIG_HEAP_POISONING_COMPREHENSIVE"; do
+    echo "==== Testing with config: ${FLAGS} ===="
+    CPPFLAGS="-D${FLAGS}" make clean test || FAIL=1
+done
+
+make clean
+
+if [ $FAIL == 0 ]; then
+    echo "All configurations passed"
+else
+    echo "Some configurations failed, see log."
+    exit 1
+fi

components/heap/test_multi_heap_host/test_multi_heap.cpp

@@ -76,7 +76,7 @@ TEST_CASE("multi_heap fragmentation", "[multi_heap]")
 
     printf("allocated %p %p %p %p\n", p[0], p[1], p[2], p[3]);
 
-    REQUIRE( multi_heap_malloc(heap, alloc_size * 3) == NULL ); /* no room to allocate 3*alloc_size now */
+    REQUIRE( multi_heap_malloc(heap, alloc_size * 5) == NULL ); /* no room to allocate 5*alloc_size now */
 
     printf("4 allocations:\n");
     multi_heap_dump(heap);
@@ -105,6 +105,103 @@ TEST_CASE("multi_heap fragmentation", "[multi_heap]")
     multi_heap_free(heap, big);
 }
 
+/* Test that malloc/free does not leave free space fragmented */
+TEST_CASE("multi_heap defrag", "[multi_heap]")
+{
+    void *p[4];
+    uint8_t small_heap[512];
+    multi_heap_info_t info, info2;
+    multi_heap_handle_t heap = multi_heap_register(small_heap, sizeof(small_heap));
+
+    printf("0 ---\n");
+    multi_heap_dump(heap);
+    REQUIRE( multi_heap_check(heap, true) );
+    multi_heap_get_info(heap, &info);
+    REQUIRE( 0 == info.allocated_blocks );
+    REQUIRE( 1 == info.free_blocks );
+
+    printf("1 ---\n");
+    p[0] = multi_heap_malloc(heap, 128);
+    p[1] = multi_heap_malloc(heap, 32);
+    multi_heap_dump(heap);
+    REQUIRE( multi_heap_check(heap, true) );
+
+    printf("2 ---\n");
+    multi_heap_free(heap, p[0]);
+    p[2] = multi_heap_malloc(heap, 64);
+    multi_heap_dump(heap);
+    REQUIRE( p[2] == p[0] );
+    REQUIRE( multi_heap_check(heap, true) );
+
+    printf("3 ---\n");
+    multi_heap_free(heap, p[2]);
+    p[3] = multi_heap_malloc(heap, 32);
+    multi_heap_dump(heap);
+    REQUIRE( p[3] == p[0] );
+    REQUIRE( multi_heap_check(heap, true) );
+
+    multi_heap_get_info(heap, &info2);
+    REQUIRE( 2 == info2.allocated_blocks );
+    REQUIRE( 2 == info2.free_blocks );
+
+    multi_heap_free(heap, p[0]);
+    multi_heap_free(heap, p[1]);
+    multi_heap_get_info(heap, &info2);
+    REQUIRE( 0 == info2.allocated_blocks );
+    REQUIRE( 1 == info2.free_blocks );
+    REQUIRE( info.total_free_bytes == info2.total_free_bytes );
+}
+
+/* Test that malloc/free does not leave free space fragmented
+   Note: With fancy poisoning, realloc is implemented as malloc-copy-free and this test does not apply.
+ */
+#ifndef MULTI_HEAP_POISONING_SLOW
+TEST_CASE("multi_heap defrag realloc", "[multi_heap]")
+{
+    void *p[4];
+    uint8_t small_heap[512];
+    multi_heap_info_t info, info2;
+    multi_heap_handle_t heap = multi_heap_register(small_heap, sizeof(small_heap));
+
+    printf("0 ---\n");
+    multi_heap_dump(heap);
+    REQUIRE( multi_heap_check(heap, true) );
+    multi_heap_get_info(heap, &info);
+    REQUIRE( 0 == info.allocated_blocks );
+    REQUIRE( 1 == info.free_blocks );
+
+    printf("1 ---\n");
+    p[0] = multi_heap_malloc(heap, 128);
+    p[1] = multi_heap_malloc(heap, 32);
+    multi_heap_dump(heap);
+    REQUIRE( multi_heap_check(heap, true) );
+
+    printf("2 ---\n");
+    p[2] = multi_heap_realloc(heap, p[0], 64);
+    multi_heap_dump(heap);
+    REQUIRE( p[2] == p[0] );
+    REQUIRE( multi_heap_check(heap, true) );
+
+    printf("3 ---\n");
+    p[3] = multi_heap_realloc(heap, p[2], 32);
+    multi_heap_dump(heap);
+    REQUIRE( p[3] == p[0] );
+    REQUIRE( multi_heap_check(heap, true) );
+
+    multi_heap_get_info(heap, &info2);
+    REQUIRE( 2 == info2.allocated_blocks );
+    REQUIRE( 2 == info2.free_blocks );
+
+    multi_heap_free(heap, p[0]);
+    multi_heap_free(heap, p[1]);
+    multi_heap_get_info(heap, &info2);
+    REQUIRE( 0 == info2.allocated_blocks );
+    REQUIRE( 1 == info2.free_blocks );
+    REQUIRE( info.total_free_bytes == info2.total_free_bytes );
+}
+#endif
+
+
 TEST_CASE("multi_heap many random allocations", "[multi_heap]")
 {
     uint8_t big_heap[1024];
@@ -329,7 +426,13 @@ TEST_CASE("multi_heap_realloc()", "[multi_heap]")
     REQUIRE( multi_heap_check(heap, true) );
     REQUIRE( f == b ); /* 'b' should be extended in-place, over space formerly occupied by 'd' */
 
-    uint32_t *g = (uint32_t *)multi_heap_realloc(heap, e, 128); /* not enough contiguous space left in the heap */
+#ifdef MULTI_HEAP_POISONING
+#define TOO_MUCH 92 + 1
+#else
+#define TOO_MUCH 128 + 1
+#endif
+    /* not enough contiguous space left in the heap */
+    uint32_t *g = (uint32_t *)multi_heap_realloc(heap, e, TOO_MUCH);
     REQUIRE( g == NULL );
 
     multi_heap_free(heap, f);