alex/esp-idf
alex/esp-idf
1
0
Fork 0
mirror of https://github.com/espressif/esp-idf.git synced 2024-10-05 20:47:46 -04:00
Code Issues Actions 2 Packages Projects Releases Wiki Activity

heap trace standalone: Fix initialization of the unused linked list, update tests

Browse Source 
- 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.
This commit is contained in:
Guillaume Souchere 2023-02-14 09:48:14 +01:00
parent a555563558
commit 2cf9236f6c
5 changed files with 96 additions and 73 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

123
components/heap/heap_trace_standalone.c
View File

@ -26,7 +26,7 @@ static bool tracing;
static heap_trace_mode_t mode; static heap_trace_mode_t mode;
/* Define struct: linked list of records */ /* 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; typedef struct heap_trace_record_list_struct_t heap_trace_record_list_t;
/* Linked List of Records */ /* Linked List of Records */
@ -58,11 +58,11 @@ typedef struct {
// Forward Defines // Forward Defines
static void heap_trace_dump_base(bool internal_ram, bool psram); 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 record_deep_copy(heap_trace_record_t *rDest, const heap_trace_record_t* r_src);
static void list_setup(); static void list_setup(void);
static void list_remove(heap_trace_record_t* rRemove); static void list_remove(heap_trace_record_t* r_remove);
static bool list_add(const heap_trace_record_t* rAppend); static bool list_add(const heap_trace_record_t* r_append);
static heap_trace_record_t* list_pop_unused(); static heap_trace_record_t* list_pop_unused(void);
static heap_trace_record_t* list_find_address_reverse(void* p); static heap_trace_record_t* list_find_address_reverse(void* p);
/* The actual records. */ /* The actual records. */
@ -75,8 +75,8 @@ static size_t total_allocations;
static size_t total_frees; static size_t total_frees;
/* Used to speed up heap_trace_get */ /* Used to speed up heap_trace_get */
static heap_trace_record_t* rGet; static heap_trace_record_t* r_get;
static size_t rGetIdx; static size_t r_get_idx;
esp_err_t heap_trace_init_standalone(heap_trace_record_t *record_buffer, size_t num_records) 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; return ESP_ERR_INVALID_STATE;
} }
if (num_records == 0) { if (record_buffer == NULL || num_records == 0) {
return ESP_ERR_INVALID_ARG;
}
if (record_buffer == NULL) {
return ESP_ERR_INVALID_ARG; 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.count = 0;
records.has_overflowed = false; records.has_overflowed = false;
list_setup(&records); list_setup();
total_allocations = 0; total_allocations = 0;
total_frees = 0; total_frees = 0;
@ -148,9 +144,9 @@ size_t heap_trace_get_count(void)
return records.count; 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; return ESP_ERR_INVALID_STATE;
} }
@ -165,37 +161,37 @@ esp_err_t heap_trace_get(size_t index, heap_trace_record_t *rOut)
} else { } else {
// Perf: speed up sequential access // Perf: speed up sequential access
if (rGet && rGetIdx == index - 1) { if (r_get && r_get_idx == index - 1) {
rGet = TAILQ_NEXT(rGet, tailq); r_get = TAILQ_NEXT(r_get, tailq);
rGetIdx = index; r_get_idx = index;
} else { } else {
// Iterate through through the linked list // Iterate through through the linked list
rGet = TAILQ_FIRST(&records.list); r_get = TAILQ_FIRST(&records.list);
for (int i = 0; i < index; i++) { for (int i = 0; i < index; i++) {
if (rGet == NULL) { if (r_get == NULL) {
break; break;
} }
rGet = TAILQ_NEXT(rGet, tailq); r_get = TAILQ_NEXT(r_get, tailq);
rGetIdx = i + 1; r_get_idx = i + 1;
} }
} }
// copy to destination // copy to destination
if (rGet) { if (r_get) {
memcpy(rOut, rGet, sizeof(heap_trace_record_t)); memcpy(r_out, r_get, sizeof(heap_trace_record_t));
} else { } else {
// this should not happen since we already // this should not happen since we already
// checked that index < records.count, // checked that index < records.count,
// but could be indicative of memory corruption // but could be indicative of memory corruption
result = ESP_ERR_INVALID_STATE; 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 delta_allocs = 0;
size_t start_count = records.count; 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); records.count, records.capacity);
// Iterate through through the linked list // Iterate through through the linked list
@ -249,7 +245,7 @@ static void heap_trace_dump_base(bool internal_ram, bool psram)
// check corruption // check corruption
if (rCur == NULL) { 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; break;
} }
@ -268,22 +264,22 @@ static void heap_trace_dump_base(bool internal_ram, bool psram)
label = ", 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); rCur->size, rCur->address, label, rCur->ccount & 1, rCur->ccount & ~3);
for (int j = 0; j < STACK_DEPTH && rCur->alloced_by[j] != 0; j++) { 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) ? ":" : ""); (j < STACK_DEPTH - 1) ? ":" : "");
} }
if (mode != HEAP_TRACE_ALL || STACK_DEPTH == 0 || rCur->freed_by[0] == NULL) { if (mode != HEAP_TRACE_ALL || STACK_DEPTH == 0 || rCur->freed_by[0] == NULL) {
delta_size += rCur->size; delta_size += rCur->size;
delta_allocs++; delta_allocs++;
printf("\n"); esp_rom_printf("\n");
} else { } else {
printf("\nfreed by "); esp_rom_printf("\nfreed by ");
for (int j = 0; j < STACK_DEPTH; j++) { 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"); (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); rCur = TAILQ_NEXT(rCur, tailq);
} }
printf("====== Heap Trace Summary ======\n"); esp_rom_printf("====== Heap Trace Summary ======\n");
if (mode == HEAP_TRACE_ALL) { if (mode == HEAP_TRACE_ALL) {
printf("Mode: Heap Trace All\n"); esp_rom_printf("Mode: Heap Trace All\n");
printf("%u bytes alive in trace (%u/%u allocations)\n", esp_rom_printf("%u bytes alive in trace (%u/%u allocations)\n",
delta_size, delta_allocs, heap_trace_get_count()); delta_size, delta_allocs, heap_trace_get_count());
} else { } else {
printf("Mode: Heap Trace Leaks\n"); esp_rom_printf("Mode: Heap Trace Leaks\n");
printf("%u bytes 'leaked' in trace (%u allocations)\n", delta_size, delta_allocs); 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); records.count, records.capacity, records.high_water_mark);
printf("total allocations: %u\n", total_allocations); esp_rom_printf("total allocations: %u\n", total_allocations);
printf("total frees: %u\n", total_frees); esp_rom_printf("total frees: %u\n", total_frees);
if (start_count != records.count) { // only a problem if trace isn't stopped before dumping 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) { 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); 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_ALL, this means filling in the freed_by pointer.
For HEAP_TRACE_LEAKS, this means removing the record from the log. 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) 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 // 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.list);
TAILQ_INIT(&records.unused); TAILQ_INIT(&records.unused);
@ -399,25 +398,25 @@ static void list_setup()
heap_trace_record_t* rCur = &records.buffer[i]; 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 */ 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); assert(records.count > 0);
// remove from records.list // remove from records.list
TAILQ_REMOVE(&records.list, rRemove, tailq); TAILQ_REMOVE(&records.list, r_remove, tailq);
// set as unused // set as unused
rRemove->address = 0; r_remove->address = 0;
rRemove->size = 0; r_remove->size = 0;
// add to records.unused // add to records.unused
TAILQ_INSERT_HEAD(&records.unused, rRemove, tailq); TAILQ_INSERT_HEAD(&records.unused, r_remove, tailq);
// decrement // decrement
records.count--; records.count--;
@ -425,7 +424,7 @@ static IRAM_ATTR void list_remove(heap_trace_record_t* rRemove)
// pop record from unused list // 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? // no records left?
if (records.count >= records.capacity) { if (records.count >= records.capacity) {
@ -445,18 +444,18 @@ static IRAM_ATTR heap_trace_record_t* list_pop_unused()
// deep copy a record. // deep copy a record.
// Note: only copies the *allocation data*, not the next & prev ptrs // 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->ccount = r_src->ccount;
rDest->address = rSrc->address; rDest->address = r_src->address;
rDest->size = rSrc->size; rDest->size = r_src->size;
memcpy(rDest->freed_by, rSrc->freed_by, sizeof(void *) * STACK_DEPTH); memcpy(rDest->freed_by, r_src->freed_by, sizeof(void *) * STACK_DEPTH);
memcpy(rDest->alloced_by, rSrc->alloced_by, sizeof(void *) * STACK_DEPTH); memcpy(rDest->alloced_by, r_src->alloced_by, sizeof(void *) * STACK_DEPTH);
} }
// Append a record to records.list // Append a record to records.list
// Note: This deep copies rAppend // Note: This deep copies r_append
static IRAM_ATTR bool list_add(const heap_trace_record_t *rAppend) static IRAM_ATTR bool list_add(const heap_trace_record_t *r_append)
{ {
if (records.count < records.capacity) { if (records.count < records.capacity) {
@ -468,7 +467,7 @@ static IRAM_ATTR bool list_add(const heap_trace_record_t *rAppend)
assert(rDest != NULL); assert(rDest != NULL);
// copy allocation data // copy allocation data
record_deep_copy(rDest, rAppend); record_deep_copy(rDest, r_append);
// append to records.list // append to records.list
TAILQ_INSERT_TAIL(&records.list, rDest, tailq); TAILQ_INSERT_TAIL(&records.list, rDest, tailq);

10
components/heap/include/esp_heap_trace.h
View File

@ -30,16 +30,16 @@ typedef enum {
/** /**
* @brief Trace record data type. Stores information about an allocated region of memory. * @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). 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. void *address; ///< Address which was allocated. If NULL, then this record is empty.
size_t size; ///< Size of the allocation 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 *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.) 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 #ifdef CONFIG_HEAP_TRACING_STANDALONE
}; TAILQ_ENTRY(heap_trace_record_t) tailq; ///< Linked list: prev & next records
#endif // CONFIG_HEAP_TRACING_STANDALONE
typedef struct heap_trace_record_struct_t heap_trace_record_t; } heap_trace_record_t;
/** /**
* @brief Stores information about the result of a heap trace. * @brief Stores information about the result of a heap trace.

4
components/heap/test_apps/main/test_heap_main.c
View File

@ -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 * SPDX-License-Identifier: Apache-2.0
*/ */
@ -8,7 +8,7 @@
#include "unity_test_runner.h" #include "unity_test_runner.h"
#include "esp_heap_caps.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; static int leak_threshold = TEST_MEMORY_LEAK_THRESHOLD_DEFAULT;
void set_leak_threshold(int threshold) void set_leak_threshold(int threshold)
{ {

8
components/heap/test_apps/main/test_heap_trace.c
View File

@ -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 * 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); heap_trace_get(0, &trace_b);
TEST_ASSERT_EQUAL_PTR(b, trace_b.address); TEST_ASSERT_EQUAL_PTR(b, trace_b.address);
/* buffer deletes trace_a when freed, /* trace_a freed and placed back to unused list,
so trace_b at head of buffer */ so recs[0].address is 0*/
TEST_ASSERT_EQUAL_PTR(recs[0].address, trace_b.address); TEST_ASSERT_EQUAL_PTR(recs[0].address, 0x00);
heap_trace_stop(); heap_trace_stop();
} }

24
docs/sphinx-known-warnings.txt
View File

@ -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'. 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. 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'. 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'.