From 2cf9236f6cd158fd0a1b4a35e98bc7dd5ab63d35 Mon Sep 17 00:00:00 2001 From: Guillaume Souchere Date: Tue, 14 Feb 2023 09:48:14 +0100 Subject: [PATCH] heap trace standalone: Fix initialization of the unused linked list, update tests - Call TAILQ_INSERT_TAIL in linked_list_setup to add unused records from the tail of the list - Fix test "heap trace leak check" to expect that after a free, the record is zeroed instead of checking that the whole list of records is moved by one index in the array. - Use esp_rom_printf() under lock instead of printf() since it does not rely on interrupts. --- components/heap/heap_trace_standalone.c | 123 +++++++++--------- components/heap/include/esp_heap_trace.h | 10 +- .../heap/test_apps/main/test_heap_main.c | 4 +- .../heap/test_apps/main/test_heap_trace.c | 8 +- docs/sphinx-known-warnings.txt | 24 ++++ 5 files changed, 96 insertions(+), 73 deletions(-) diff --git a/components/heap/heap_trace_standalone.c b/components/heap/heap_trace_standalone.c index c7c5734f81..c422fa8432 100644 --- a/components/heap/heap_trace_standalone.c +++ b/components/heap/heap_trace_standalone.c @@ -26,7 +26,7 @@ static bool tracing; static heap_trace_mode_t mode; /* Define struct: linked list of records */ -TAILQ_HEAD(heap_trace_record_list_struct_t, heap_trace_record_struct_t); +TAILQ_HEAD(heap_trace_record_list_struct_t, heap_trace_record_t); typedef struct heap_trace_record_list_struct_t heap_trace_record_list_t; /* Linked List of Records */ @@ -58,11 +58,11 @@ typedef struct { // Forward Defines static void heap_trace_dump_base(bool internal_ram, bool psram); -static void record_deep_copy(heap_trace_record_t *rDest, const heap_trace_record_t* rSrc); -static void list_setup(); -static void list_remove(heap_trace_record_t* rRemove); -static bool list_add(const heap_trace_record_t* rAppend); -static heap_trace_record_t* list_pop_unused(); +static void record_deep_copy(heap_trace_record_t *rDest, const heap_trace_record_t* r_src); +static void list_setup(void); +static void list_remove(heap_trace_record_t* r_remove); +static bool list_add(const heap_trace_record_t* r_append); +static heap_trace_record_t* list_pop_unused(void); static heap_trace_record_t* list_find_address_reverse(void* p); /* The actual records. */ @@ -75,8 +75,8 @@ static size_t total_allocations; static size_t total_frees; /* Used to speed up heap_trace_get */ -static heap_trace_record_t* rGet; -static size_t rGetIdx; +static heap_trace_record_t* r_get; +static size_t r_get_idx; esp_err_t heap_trace_init_standalone(heap_trace_record_t *record_buffer, size_t num_records) { @@ -84,11 +84,7 @@ esp_err_t heap_trace_init_standalone(heap_trace_record_t *record_buffer, size_t return ESP_ERR_INVALID_STATE; } - if (num_records == 0) { - return ESP_ERR_INVALID_ARG; - } - - if (record_buffer == NULL) { + if (record_buffer == NULL || num_records == 0) { return ESP_ERR_INVALID_ARG; } @@ -114,7 +110,7 @@ esp_err_t heap_trace_start(heap_trace_mode_t mode_param) records.count = 0; records.has_overflowed = false; - list_setup(&records); + list_setup(); total_allocations = 0; total_frees = 0; @@ -148,9 +144,9 @@ size_t heap_trace_get_count(void) return records.count; } -esp_err_t heap_trace_get(size_t index, heap_trace_record_t *rOut) +esp_err_t heap_trace_get(size_t index, heap_trace_record_t *r_out) { - if (rOut == NULL) { + if (r_out == NULL) { return ESP_ERR_INVALID_STATE; } @@ -165,37 +161,37 @@ esp_err_t heap_trace_get(size_t index, heap_trace_record_t *rOut) } else { // Perf: speed up sequential access - if (rGet && rGetIdx == index - 1) { + if (r_get && r_get_idx == index - 1) { - rGet = TAILQ_NEXT(rGet, tailq); - rGetIdx = index; + r_get = TAILQ_NEXT(r_get, tailq); + r_get_idx = index; } else { // Iterate through through the linked list - rGet = TAILQ_FIRST(&records.list); + r_get = TAILQ_FIRST(&records.list); for (int i = 0; i < index; i++) { - if (rGet == NULL) { + if (r_get == NULL) { break; } - rGet = TAILQ_NEXT(rGet, tailq); - rGetIdx = i + 1; + r_get = TAILQ_NEXT(r_get, tailq); + r_get_idx = i + 1; } } // copy to destination - if (rGet) { - memcpy(rOut, rGet, sizeof(heap_trace_record_t)); + if (r_get) { + memcpy(r_out, r_get, sizeof(heap_trace_record_t)); } else { // this should not happen since we already // checked that index < records.count, // but could be indicative of memory corruption result = ESP_ERR_INVALID_STATE; - memset(rOut, 0, sizeof(heap_trace_record_t)); + memset(r_out, 0, sizeof(heap_trace_record_t)); } } @@ -238,7 +234,7 @@ static void heap_trace_dump_base(bool internal_ram, bool psram) size_t delta_allocs = 0; size_t start_count = records.count; - printf("====== Heap Trace: %u records (%u capacity) ======\n", + esp_rom_printf("====== Heap Trace: %u records (%u capacity) ======\n", records.count, records.capacity); // Iterate through through the linked list @@ -249,7 +245,7 @@ static void heap_trace_dump_base(bool internal_ram, bool psram) // check corruption if (rCur == NULL) { - printf("\nError: heap trace linked list is corrupt. expected more records.\n"); + esp_rom_printf("\nError: heap trace linked list is corrupt. expected more records.\n"); break; } @@ -268,22 +264,22 @@ static void heap_trace_dump_base(bool internal_ram, bool psram) label = ", PSRAM"; } - printf("%6d bytes (@ %p%s) allocated CPU %d ccount 0x%08x caller ", + esp_rom_printf("%6d bytes (@ %p%s) allocated CPU %d ccount 0x%08x caller ", rCur->size, rCur->address, label, rCur->ccount & 1, rCur->ccount & ~3); for (int j = 0; j < STACK_DEPTH && rCur->alloced_by[j] != 0; j++) { - printf("%p%s", rCur->alloced_by[j], + esp_rom_printf("%p%s", rCur->alloced_by[j], (j < STACK_DEPTH - 1) ? ":" : ""); } if (mode != HEAP_TRACE_ALL || STACK_DEPTH == 0 || rCur->freed_by[0] == NULL) { delta_size += rCur->size; delta_allocs++; - printf("\n"); + esp_rom_printf("\n"); } else { - printf("\nfreed by "); + esp_rom_printf("\nfreed by "); for (int j = 0; j < STACK_DEPTH; j++) { - printf("%p%s", rCur->freed_by[j], + esp_rom_printf("%p%s", rCur->freed_by[j], (j < STACK_DEPTH - 1) ? ":" : "\n"); } } @@ -292,30 +288,30 @@ static void heap_trace_dump_base(bool internal_ram, bool psram) rCur = TAILQ_NEXT(rCur, tailq); } - printf("====== Heap Trace Summary ======\n"); + esp_rom_printf("====== Heap Trace Summary ======\n"); if (mode == HEAP_TRACE_ALL) { - printf("Mode: Heap Trace All\n"); - printf("%u bytes alive in trace (%u/%u allocations)\n", + esp_rom_printf("Mode: Heap Trace All\n"); + esp_rom_printf("%u bytes alive in trace (%u/%u allocations)\n", delta_size, delta_allocs, heap_trace_get_count()); } else { - printf("Mode: Heap Trace Leaks\n"); - printf("%u bytes 'leaked' in trace (%u allocations)\n", delta_size, delta_allocs); + esp_rom_printf("Mode: Heap Trace Leaks\n"); + esp_rom_printf("%u bytes 'leaked' in trace (%u allocations)\n", delta_size, delta_allocs); } - printf("records: %u (%u capacity, %u high water mark)\n", + esp_rom_printf("records: %u (%u capacity, %u high water mark)\n", records.count, records.capacity, records.high_water_mark); - printf("total allocations: %u\n", total_allocations); - printf("total frees: %u\n", total_frees); + esp_rom_printf("total allocations: %u\n", total_allocations); + esp_rom_printf("total frees: %u\n", total_frees); if (start_count != records.count) { // only a problem if trace isn't stopped before dumping - printf("(NB: New entries were traced while dumping, so trace dump may have duplicate entries.)\n"); + esp_rom_printf("(NB: New entries were traced while dumping, so trace dump may have duplicate entries.)\n"); } if (records.has_overflowed) { - printf("(NB: Internal Buffer has overflowed, so trace data is incomplete.)\n"); + esp_rom_printf("(NB: Internal Buffer has overflowed, so trace data is incomplete.)\n"); } - printf("================================\n"); + esp_rom_printf("================================\n"); portEXIT_CRITICAL(&trace_mux); } @@ -355,6 +351,9 @@ static IRAM_ATTR void record_allocation(const heap_trace_record_t *rAllocation) For HEAP_TRACE_ALL, this means filling in the freed_by pointer. For HEAP_TRACE_LEAKS, this means removing the record from the log. + + callers is an array of STACK_DEPTH function pointer from the call stack + leading to the call of record_free. */ static IRAM_ATTR void record_free(void *p, void **callers) { @@ -390,7 +389,7 @@ static IRAM_ATTR void record_free(void *p, void **callers) } // connect all records into a linked list of 'unused' records -static void list_setup() +static void list_setup(void) { TAILQ_INIT(&records.list); TAILQ_INIT(&records.unused); @@ -399,25 +398,25 @@ static void list_setup() heap_trace_record_t* rCur = &records.buffer[i]; - TAILQ_INSERT_HEAD(&records.unused, rCur, tailq); + TAILQ_INSERT_TAIL(&records.unused, rCur, tailq); } } -/* 1. removes record from records.list, +/* 1. removes record r_remove from records.list, 2. places it into records.unused */ -static IRAM_ATTR void list_remove(heap_trace_record_t* rRemove) +static IRAM_ATTR void list_remove(heap_trace_record_t* r_remove) { assert(records.count > 0); // remove from records.list - TAILQ_REMOVE(&records.list, rRemove, tailq); + TAILQ_REMOVE(&records.list, r_remove, tailq); // set as unused - rRemove->address = 0; - rRemove->size = 0; + r_remove->address = 0; + r_remove->size = 0; // add to records.unused - TAILQ_INSERT_HEAD(&records.unused, rRemove, tailq); + TAILQ_INSERT_HEAD(&records.unused, r_remove, tailq); // decrement records.count--; @@ -425,7 +424,7 @@ static IRAM_ATTR void list_remove(heap_trace_record_t* rRemove) // pop record from unused list -static IRAM_ATTR heap_trace_record_t* list_pop_unused() +static IRAM_ATTR heap_trace_record_t* list_pop_unused(void) { // no records left? if (records.count >= records.capacity) { @@ -445,18 +444,18 @@ static IRAM_ATTR heap_trace_record_t* list_pop_unused() // deep copy a record. // Note: only copies the *allocation data*, not the next & prev ptrs -static IRAM_ATTR void record_deep_copy(heap_trace_record_t *rDest, const heap_trace_record_t *rSrc) +static IRAM_ATTR void record_deep_copy(heap_trace_record_t *rDest, const heap_trace_record_t *r_src) { - rDest->ccount = rSrc->ccount; - rDest->address = rSrc->address; - rDest->size = rSrc->size; - memcpy(rDest->freed_by, rSrc->freed_by, sizeof(void *) * STACK_DEPTH); - memcpy(rDest->alloced_by, rSrc->alloced_by, sizeof(void *) * STACK_DEPTH); + rDest->ccount = r_src->ccount; + rDest->address = r_src->address; + rDest->size = r_src->size; + memcpy(rDest->freed_by, r_src->freed_by, sizeof(void *) * STACK_DEPTH); + memcpy(rDest->alloced_by, r_src->alloced_by, sizeof(void *) * STACK_DEPTH); } // Append a record to records.list -// Note: This deep copies rAppend -static IRAM_ATTR bool list_add(const heap_trace_record_t *rAppend) +// Note: This deep copies r_append +static IRAM_ATTR bool list_add(const heap_trace_record_t *r_append) { if (records.count < records.capacity) { @@ -468,7 +467,7 @@ static IRAM_ATTR bool list_add(const heap_trace_record_t *rAppend) assert(rDest != NULL); // copy allocation data - record_deep_copy(rDest, rAppend); + record_deep_copy(rDest, r_append); // append to records.list TAILQ_INSERT_TAIL(&records.list, rDest, tailq); diff --git a/components/heap/include/esp_heap_trace.h b/components/heap/include/esp_heap_trace.h index acc6328995..b1c5d476e4 100644 --- a/components/heap/include/esp_heap_trace.h +++ b/components/heap/include/esp_heap_trace.h @@ -30,16 +30,16 @@ typedef enum { /** * @brief Trace record data type. Stores information about an allocated region of memory. */ -struct heap_trace_record_struct_t { +typedef struct heap_trace_record_t { uint32_t ccount; ///< CCOUNT of the CPU when the allocation was made. LSB (bit value 1) is the CPU number (0 or 1). void *address; ///< Address which was allocated. If NULL, then this record is empty. size_t size; ///< Size of the allocation void *alloced_by[CONFIG_HEAP_TRACING_STACK_DEPTH]; ///< Call stack of the caller which allocated the memory. void *freed_by[CONFIG_HEAP_TRACING_STACK_DEPTH]; ///< Call stack of the caller which freed the memory (all zero if not freed.) - TAILQ_ENTRY(heap_trace_record_struct_t) tailq; ///< Linked list: prev & next records -}; - -typedef struct heap_trace_record_struct_t heap_trace_record_t; +#ifdef CONFIG_HEAP_TRACING_STANDALONE + TAILQ_ENTRY(heap_trace_record_t) tailq; ///< Linked list: prev & next records +#endif // CONFIG_HEAP_TRACING_STANDALONE +} heap_trace_record_t; /** * @brief Stores information about the result of a heap trace. diff --git a/components/heap/test_apps/main/test_heap_main.c b/components/heap/test_apps/main/test_heap_main.c index c271df754f..1a77d749ba 100644 --- a/components/heap/test_apps/main/test_heap_main.c +++ b/components/heap/test_apps/main/test_heap_main.c @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2022 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2022-2023 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -8,7 +8,7 @@ #include "unity_test_runner.h" #include "esp_heap_caps.h" -#define TEST_MEMORY_LEAK_THRESHOLD_DEFAULT -100 +#define TEST_MEMORY_LEAK_THRESHOLD_DEFAULT -300 static int leak_threshold = TEST_MEMORY_LEAK_THRESHOLD_DEFAULT; void set_leak_threshold(int threshold) { diff --git a/components/heap/test_apps/main/test_heap_trace.c b/components/heap/test_apps/main/test_heap_trace.c index 4778103b99..8369b1ee13 100644 --- a/components/heap/test_apps/main/test_heap_trace.c +++ b/components/heap/test_apps/main/test_heap_trace.c @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2022 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2022-2023 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Unlicense OR CC0-1.0 */ @@ -66,9 +66,9 @@ TEST_CASE("heap trace leak check", "[heap-trace]") heap_trace_get(0, &trace_b); TEST_ASSERT_EQUAL_PTR(b, trace_b.address); - /* buffer deletes trace_a when freed, - so trace_b at head of buffer */ - TEST_ASSERT_EQUAL_PTR(recs[0].address, trace_b.address); + /* trace_a freed and placed back to unused list, + so recs[0].address is 0*/ + TEST_ASSERT_EQUAL_PTR(recs[0].address, 0x00); heap_trace_stop(); } diff --git a/docs/sphinx-known-warnings.txt b/docs/sphinx-known-warnings.txt index 7ae9c66e6c..ba372d4e30 100644 --- a/docs/sphinx-known-warnings.txt +++ b/docs/sphinx-known-warnings.txt @@ -33,3 +33,27 @@ wear-levelling.rst:line: WARNING: Duplicate C++ declaration, also defined at api Declaration is '.. cpp:member:: size_t allocation_unit_size'. wear-levelling.rst:line: WARNING: Duplicate C++ declaration, also defined at api-reference/storage/fatfs:line. Declaration is '.. cpp:member:: bool disk_status_check_enable'. +esp_heap_trace.inc:line: WARNING: Error when parsing function declaration. +If the function has no return type: + Invalid C++ declaration: Expected end of definition or ;. [error at 34] + TAILQ_ENTRY (heap_trace_record_t) tailq + ----------------------------------^ +If the function has a return type: + Error in declarator + If declarator-id with parameters-and-qualifiers: + Invalid C++ declaration: Expected identifier in nested name. [error at 12] + TAILQ_ENTRY (heap_trace_record_t) tailq + ------------^ + If parenthesis in noptr-declarator: + Error in declarator or parameters-and-qualifiers + If pointer to member declarator: + Invalid C++ declaration: Expected '::' in pointer to member (function). [error at 32] + TAILQ_ENTRY (heap_trace_record_t) tailq + --------------------------------^ + If declarator-id: + Invalid C++ declaration: Expecting "(" in parameters-and-qualifiers. [error at 32] + TAILQ_ENTRY (heap_trace_record_t) tailq + --------------------------------^ + +esp_heap_trace.inc:line: WARNING: Duplicate C++ declaration, also defined at api-reference/system/heap_debug:line. +Declaration is '.. cpp:type:: struct heap_trace_record_t heap_trace_record_t'.