From 8b2b430d68e5c5ab6e786df1c2731f0001b4f5be Mon Sep 17 00:00:00 2001
From: Chip Weinberger <weinberger.c@gmail.com>
Date: Tue, 10 Jan 2023 15:36:33 -0800
Subject: [PATCH 1/4] [Heap Trace Standalone] fix terrible Leaks perf on large
 records by using doubly linked list

---
 components/heap/heap_trace_standalone.c  | 275 +++++++++++++++++------
 components/heap/include/esp_heap_trace.h |  10 +-
 2 files changed, 219 insertions(+), 66 deletions(-)

diff --git a/components/heap/heap_trace_standalone.c b/components/heap/heap_trace_standalone.c
index c78f3b3c91..68c13fe39d 100644
--- a/components/heap/heap_trace_standalone.c
+++ b/components/heap/heap_trace_standalone.c
@@ -14,6 +14,7 @@
 #include "freertos/FreeRTOS.h"
 #include "freertos/task.h"
 #include "esp_memory_utils.h"
+#include "sys/queue.h"
 
 
 #define STACK_DEPTH CONFIG_HEAP_TRACING_STACK_DEPTH
@@ -24,15 +25,26 @@ static portMUX_TYPE trace_mux = portMUX_INITIALIZER_UNLOCKED;
 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);
+typedef struct heap_trace_record_list_struct_t heap_trace_record_list_t;
+
+/* Linked List of Records */
 typedef struct {
 
-    /* Buffer used for records, starting at offset 0 */
+    /* Buffer used for records. */
     heap_trace_record_t *buffer;
 
+    /* Linked list of recorded allocations */
+    heap_trace_record_list_t list;
+
+    /* Linked list of available record objects */
+    heap_trace_record_list_t unused;
+
     /* capacity of the buffer */
     size_t capacity;
 
-    /* Count of entries logged in the buffer.*/
+    /* Count of entries logged in the buffer. */
     size_t count;
 
     /* During execution, we remember the maximum
@@ -46,8 +58,13 @@ typedef struct {
 
 
 // Forward Defines
-static void remove_record(records_t *r, int index);
 static void heap_trace_dump_base(bool internal_ram, bool psram);
+static void linked_list_setup();
+static void linked_list_remove(heap_trace_record_t* rRemove);
+static void linked_list_deep_copy(heap_trace_record_t *rDest, const heap_trace_record_t* rSrc);
+static bool linked_list_add(const heap_trace_record_t* rAppend);
+static heap_trace_record_t* linked_list_pop_unused();
+static heap_trace_record_t* linked_list_find_address_reverse(void* p);
 
 /* The actual records. */
 static records_t records;
@@ -64,9 +81,16 @@ 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) {
+        return ESP_ERR_INVALID_ARG;
+    }
+
     records.buffer = record_buffer;
     records.capacity = num_records;
-    memset(records.buffer, 0, num_records * sizeof(heap_trace_record_t));
 
     return ESP_OK;
 }
@@ -77,14 +101,17 @@ esp_err_t heap_trace_start(heap_trace_mode_t mode_param)
         return ESP_ERR_INVALID_STATE;
     }
 
-
     portENTER_CRITICAL(&trace_mux);
 
     tracing = false;
     mode = mode_param;
 
+    // clear buffer
+    memset(records.buffer, 0, sizeof(heap_trace_record_t) * records.capacity);
+
     records.count = 0;
     records.has_overflowed = false;
+    linked_list_setup(&records);
 
     total_allocations = 0;
     total_frees = 0;
@@ -118,9 +145,9 @@ size_t heap_trace_get_count(void)
     return records.count;
 }
 
-esp_err_t heap_trace_get(size_t index, heap_trace_record_t *record)
+esp_err_t heap_trace_get(size_t index, heap_trace_record_t *rOut)
 {
-    if (record == NULL) {
+    if (rOut == NULL) {
         return ESP_ERR_INVALID_STATE;
     }
 
@@ -129,9 +156,29 @@ esp_err_t heap_trace_get(size_t index, heap_trace_record_t *record)
     portENTER_CRITICAL(&trace_mux);
 
     if (index >= records.count) {
+
         result = ESP_ERR_INVALID_ARG; /* out of range for 'count' */
+
     } else {
-        memcpy(record, &records.buffer[index], sizeof(heap_trace_record_t));
+
+        // Iterate through through the linked list
+
+        heap_trace_record_t* rCur = TAILQ_FIRST(&records.list);
+
+        for (int i = 0; i < index; i++) {
+
+            // this should not happen, since we already
+            // checked that index >= records.count == false
+            if (rCur == NULL) {
+                result = ESP_ERR_INVALID_STATE;
+                break;
+            }
+
+            rCur = TAILQ_NEXT(rCur, tailq);
+        }
+
+        // copy to destination
+        memcpy(rOut, rCur, sizeof(heap_trace_record_t));
     }
 
     portEXIT_CRITICAL(&trace_mux);
@@ -167,6 +214,8 @@ void heap_trace_dump_caps(const uint32_t caps) {
 
 static void heap_trace_dump_base(bool internal_ram, bool psram)
 {
+    portENTER_CRITICAL(&trace_mux);
+
     size_t delta_size = 0;
     size_t delta_allocs = 0;
     size_t start_count = records.count;
@@ -174,45 +223,55 @@ static void heap_trace_dump_base(bool internal_ram, bool psram)
     printf("====== Heap Trace: %u records (%u capacity) ======\n",
         records.count, records.capacity);
 
+    // Iterate through through the linked list
+
+    heap_trace_record_t *rCur = TAILQ_FIRST(&records.list);
+
     for (int i = 0; i < records.count; i++) {
 
-        heap_trace_record_t *rec = &records.buffer[i];
+        // check corruption
+        if (rCur == NULL) {
+            printf("\nError: heap trace linked list is corrupt. expected more records.\n");
+            break;
+        }
 
-        bool should_print = rec->address != NULL &&
+        bool should_print = rCur->address != NULL &&
             ((psram && internal_ram) ||
-             (internal_ram && esp_ptr_internal(rec->address)) ||
-             (psram && esp_ptr_external_ram(rec->address)));
+             (internal_ram && esp_ptr_internal(rCur->address)) ||
+             (psram && esp_ptr_external_ram(rCur->address)));
 
         if (should_print) {
 
             const char* label = "";
-            if (esp_ptr_internal(rec->address)) {
+            if (esp_ptr_internal(rCur->address)) {
                 label = ", Internal";
             }
-            if (esp_ptr_external_ram(rec->address)) {
+            if (esp_ptr_external_ram(rCur->address)) {
                 label = ",    PSRAM";
             }
 
             printf("%6d bytes (@ %p%s) allocated CPU %d ccount 0x%08x caller ",
-                   rec->size, rec->address, label, rec->ccount & 1, rec->ccount & ~3);
+                   rCur->size, rCur->address, label, rCur->ccount & 1, rCur->ccount & ~3);
 
-            for (int j = 0; j < STACK_DEPTH && rec->alloced_by[j] != 0; j++) {
-                printf("%p%s", rec->alloced_by[j],
+            for (int j = 0; j < STACK_DEPTH && rCur->alloced_by[j] != 0; j++) {
+                printf("%p%s", rCur->alloced_by[j],
                        (j < STACK_DEPTH - 1) ? ":" : "");
             }
 
-            if (mode != HEAP_TRACE_ALL || STACK_DEPTH == 0 || rec->freed_by[0] == NULL) {
-                delta_size += rec->size;
+            if (mode != HEAP_TRACE_ALL || STACK_DEPTH == 0 || rCur->freed_by[0] == NULL) {
+                delta_size += rCur->size;
                 delta_allocs++;
                 printf("\n");
             } else {
                 printf("\nfreed by ");
                 for (int j = 0; j < STACK_DEPTH; j++) {
-                    printf("%p%s", rec->freed_by[j],
+                    printf("%p%s", rCur->freed_by[j],
                            (j < STACK_DEPTH - 1) ? ":" : "\n");
                 }
             }
         }
+
+        rCur = TAILQ_NEXT(rCur, tailq);
     }
 
     printf("====== Heap Trace Summary ======\n");
@@ -239,12 +298,14 @@ static void heap_trace_dump_base(bool internal_ram, bool psram)
         printf("(NB: Internal Buffer has overflowed, so trace data is incomplete.)\n");
     }
     printf("================================\n");
+
+    portEXIT_CRITICAL(&trace_mux);
 }
 
 /* Add a new allocation to the heap trace records */
-static IRAM_ATTR void record_allocation(const heap_trace_record_t *record)
+static IRAM_ATTR void record_allocation(const heap_trace_record_t *rAllocation)
 {
-    if (!tracing || record->address == NULL) {
+    if (!tracing || rAllocation->address == NULL) {
         return;
     }
 
@@ -252,34 +313,19 @@ static IRAM_ATTR void record_allocation(const heap_trace_record_t *record)
 
     if (tracing) {
 
+        // If buffer is full, pop off the oldest
+        // record to make more space
         if (records.count == records.capacity) {
 
             records.has_overflowed = true;
 
-            /* Move the whole buffer back one slot.
+            heap_trace_record_t *rFirst = TAILQ_FIRST(&records.list);
 
-            This is a bit slow, compared to treating this buffer as a
-            ringbuffer and rotating a head pointer.
-
-            However, ringbuffer code gets tricky when we remove elements
-            in mid-buffer (for leak trace mode) while trying to keep
-            track of an item count that may overflow.
-            */
-            memmove(&records.buffer[0], &records.buffer[1],
-                sizeof(heap_trace_record_t) * (records.capacity -1));
-
-            records.count--;
+            linked_list_remove(rFirst);
         }
 
-        // Copy new record into place
-        memcpy(&records.buffer[records.count], record, sizeof(heap_trace_record_t));
-
-        records.count++;
-
-        // high water mark
-        if (records.count > records.high_water_mark) {
-            records.high_water_mark = records.count;
-        }
+        // push onto end of list
+        linked_list_add(rAllocation);
 
         total_allocations++;
     }
@@ -304,25 +350,20 @@ static IRAM_ATTR void record_free(void *p, void **callers)
 
         total_frees++;
 
-        /* search backwards for the allocation record matching this free */
-        int i = -1;
-        for (i = records.count - 1; i >= 0; i--) {
-            if (records.buffer[i].address == p) {
-                break;
-            }
-        }
+        // search backwards for the allocation record matching this free
+        heap_trace_record_t* rFound = linked_list_find_address_reverse(p);
 
-        if (i >= 0) {
+        if (rFound) {
             if (mode == HEAP_TRACE_ALL) {
 
                 // add 'freed_by' info to the record
-                memcpy(records.buffer[i].freed_by, callers, sizeof(void *) * STACK_DEPTH);
+                memcpy(rFound->freed_by, callers, sizeof(void *) * STACK_DEPTH);
 
             } else { // HEAP_TRACE_LEAKS
 
                 // Leak trace mode, once an allocation is freed
                 // we remove it from the list
-                remove_record(&records, i);
+                linked_list_remove(rFound);
             }
         }
     }
@@ -330,18 +371,126 @@ static IRAM_ATTR void record_free(void *p, void **callers)
     portEXIT_CRITICAL(&trace_mux);
 }
 
-/* remove the entry at 'index' from the ringbuffer of saved records */
-static IRAM_ATTR void remove_record(records_t *r, int index)
+// connect all records into a linked list of 'unused' records
+static void linked_list_setup()
 {
-    if (index < r->count - 1) {
-        // Remove the buffer entry from the list
-        memmove(&r->buffer[index], &r->buffer[index+1],
-                sizeof(heap_trace_record_t) * (r->capacity - index - 1));
-    } else {
-        // For last element, just zero it out to avoid ambiguity
-        memset(&r->buffer[index], 0, sizeof(heap_trace_record_t));
+    TAILQ_INIT(&records.list);
+    TAILQ_INIT(&records.unused);
+
+    for (int i = 0; i < records.capacity; i++) {
+
+        heap_trace_record_t* rCur = &records.buffer[i];
+
+        TAILQ_INSERT_HEAD(&records.unused, rCur, tailq);
     }
-    r->count--;
+}
+
+/* 1. removes record from records.list,
+   2. places it into records.unused */
+static IRAM_ATTR void linked_list_remove(heap_trace_record_t* rRemove)
+{
+    assert(records.count > 0);
+
+    // remove from records.list
+    TAILQ_REMOVE(&records.list, rRemove, tailq);
+
+    // set as unused
+    rRemove->address = 0;
+    rRemove->size = 0;
+
+    // add to records.unused
+    TAILQ_INSERT_HEAD(&records.unused, rRemove, tailq);
+
+    // decrement
+    records.count--;
+}
+
+
+// pop record from unused list
+static IRAM_ATTR heap_trace_record_t* linked_list_pop_unused()
+{
+    // no records left?
+    if (records.count >= records.capacity) {
+        return NULL;
+    }
+
+    // get from records.unused
+    heap_trace_record_t* rUnused = TAILQ_FIRST(&records.unused);
+    assert(rUnused->address == NULL);
+    assert(rUnused->size == 0);
+
+    // remove from records.unused
+    TAILQ_REMOVE(&records.unused, rUnused, tailq);
+
+    return rUnused;
+}
+
+// deep copy a record.
+// Note: only copies the *allocation data*, not the next & prev ptrs
+static IRAM_ATTR void linked_list_deep_copy(heap_trace_record_t *rDest, const heap_trace_record_t *rSrc)
+{
+    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);
+}
+
+// Append a record to records.list
+// Note: This deep copies rAppend
+static IRAM_ATTR bool linked_list_add(const heap_trace_record_t *rAppend)
+{
+    if (records.count < records.capacity) {
+
+        // get unused record
+        heap_trace_record_t* rDest = linked_list_pop_unused();
+
+        // we checked that there is capacity, so this
+        // should never be null.
+        assert(rDest != NULL);
+
+        // copy allocation data
+        linked_list_deep_copy(rDest, rAppend);
+
+        // append to records.list
+        TAILQ_INSERT_TAIL(&records.list, rDest, tailq);
+
+        // increment
+        records.count++;
+
+        // high water mark
+        if (records.count > records.high_water_mark) {
+            records.high_water_mark = records.count;
+        }
+
+        return true;
+
+    } else {
+        records.has_overflowed = true;
+        return false;
+    }
+}
+
+// search records.list backwards for the allocation record matching this address
+static IRAM_ATTR heap_trace_record_t* linked_list_find_address_reverse(void* p)
+{
+    if (records.count == 0) {
+        return NULL;
+    }
+
+    heap_trace_record_t* rFound = NULL;
+
+    // Perf: We search backwards because new allocations are appended
+    // to the end of the list and most allocations are short lived.
+    heap_trace_record_t *rCur = NULL;
+    TAILQ_FOREACH_REVERSE(rCur, &records.list, heap_trace_record_list_struct_t, tailq) {
+        if (rCur->address == p) {
+            rFound = rCur;
+            break;
+        }
+    }
+
+    return rFound;
 }
 
 #include "heap_trace.inc"
diff --git a/components/heap/include/esp_heap_trace.h b/components/heap/include/esp_heap_trace.h
index 81ac813303..acc6328995 100644
--- a/components/heap/include/esp_heap_trace.h
+++ b/components/heap/include/esp_heap_trace.h
@@ -6,6 +6,7 @@
 #pragma once
 
 #include "sdkconfig.h"
+#include "sys/queue.h"
 #include <stdint.h>
 #include <esp_err.h>
 
@@ -29,13 +30,16 @@ typedef enum {
 /**
  * @brief Trace record data type. Stores information about an allocated region of memory.
  */
-typedef struct {
+struct heap_trace_record_struct_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
+    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.)
-} heap_trace_record_t;
+    TAILQ_ENTRY(heap_trace_record_struct_t) tailq; ///< Linked list: prev & next records
+};
+
+typedef struct heap_trace_record_struct_t heap_trace_record_t;
 
 /**
  * @brief Stores information about the result of a heap trace.

From 26a5117870d751d12a308d5c72f4c9e3cfdde494 Mon Sep 17 00:00:00 2001
From: Chip Weinberger <weinberger.c@gmail.com>
Date: Tue, 10 Jan 2023 15:36:33 -0800
Subject: [PATCH 2/4] [Heap Trace Standalone] fix terrible Leaks perf on large
 records by using doubly linked list

---
 components/heap/heap_trace_standalone.c | 39 ++++++++++++-------------
 1 file changed, 19 insertions(+), 20 deletions(-)

diff --git a/components/heap/heap_trace_standalone.c b/components/heap/heap_trace_standalone.c
index 68c13fe39d..2c345ef361 100644
--- a/components/heap/heap_trace_standalone.c
+++ b/components/heap/heap_trace_standalone.c
@@ -56,15 +56,14 @@ typedef struct {
     bool has_overflowed;
 } records_t;
 
-
 // Forward Defines
 static void heap_trace_dump_base(bool internal_ram, bool psram);
-static void linked_list_setup();
-static void linked_list_remove(heap_trace_record_t* rRemove);
-static void linked_list_deep_copy(heap_trace_record_t *rDest, const heap_trace_record_t* rSrc);
-static bool linked_list_add(const heap_trace_record_t* rAppend);
-static heap_trace_record_t* linked_list_pop_unused();
-static heap_trace_record_t* linked_list_find_address_reverse(void* p);
+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 heap_trace_record_t* list_find_address_reverse(void* p);
 
 /* The actual records. */
 static records_t records;
@@ -111,7 +110,7 @@ esp_err_t heap_trace_start(heap_trace_mode_t mode_param)
 
     records.count = 0;
     records.has_overflowed = false;
-    linked_list_setup(&records);
+    list_setup(&records);
 
     total_allocations = 0;
     total_frees = 0;
@@ -321,11 +320,11 @@ static IRAM_ATTR void record_allocation(const heap_trace_record_t *rAllocation)
 
             heap_trace_record_t *rFirst = TAILQ_FIRST(&records.list);
 
-            linked_list_remove(rFirst);
+            list_remove(rFirst);
         }
 
         // push onto end of list
-        linked_list_add(rAllocation);
+        list_add(rAllocation);
 
         total_allocations++;
     }
@@ -351,7 +350,7 @@ static IRAM_ATTR void record_free(void *p, void **callers)
         total_frees++;
 
         // search backwards for the allocation record matching this free
-        heap_trace_record_t* rFound = linked_list_find_address_reverse(p);
+        heap_trace_record_t* rFound = list_find_address_reverse(p);
 
         if (rFound) {
             if (mode == HEAP_TRACE_ALL) {
@@ -363,7 +362,7 @@ static IRAM_ATTR void record_free(void *p, void **callers)
 
                 // Leak trace mode, once an allocation is freed
                 // we remove it from the list
-                linked_list_remove(rFound);
+                list_remove(rFound);
             }
         }
     }
@@ -372,7 +371,7 @@ static IRAM_ATTR void record_free(void *p, void **callers)
 }
 
 // connect all records into a linked list of 'unused' records
-static void linked_list_setup()
+static void list_setup()
 {
     TAILQ_INIT(&records.list);
     TAILQ_INIT(&records.unused);
@@ -387,7 +386,7 @@ static void linked_list_setup()
 
 /* 1. removes record from records.list,
    2. places it into records.unused */
-static IRAM_ATTR void linked_list_remove(heap_trace_record_t* rRemove)
+static IRAM_ATTR void list_remove(heap_trace_record_t* rRemove)
 {
     assert(records.count > 0);
 
@@ -407,7 +406,7 @@ static IRAM_ATTR void linked_list_remove(heap_trace_record_t* rRemove)
 
 
 // pop record from unused list
-static IRAM_ATTR heap_trace_record_t* linked_list_pop_unused()
+static IRAM_ATTR heap_trace_record_t* list_pop_unused()
 {
     // no records left?
     if (records.count >= records.capacity) {
@@ -427,7 +426,7 @@ static IRAM_ATTR heap_trace_record_t* linked_list_pop_unused()
 
 // deep copy a record.
 // Note: only copies the *allocation data*, not the next & prev ptrs
-static IRAM_ATTR void linked_list_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 *rSrc)
 {
     rDest->ccount  = rSrc->ccount;
     rDest->address = rSrc->address;
@@ -438,19 +437,19 @@ static IRAM_ATTR void linked_list_deep_copy(heap_trace_record_t *rDest, const he
 
 // Append a record to records.list
 // Note: This deep copies rAppend
-static IRAM_ATTR bool linked_list_add(const heap_trace_record_t *rAppend)
+static IRAM_ATTR bool list_add(const heap_trace_record_t *rAppend)
 {
     if (records.count < records.capacity) {
 
         // get unused record
-        heap_trace_record_t* rDest = linked_list_pop_unused();
+        heap_trace_record_t* rDest = list_pop_unused();
 
         // we checked that there is capacity, so this
         // should never be null.
         assert(rDest != NULL);
 
         // copy allocation data
-        linked_list_deep_copy(rDest, rAppend);
+        record_deep_copy(rDest, rAppend);
 
         // append to records.list
         TAILQ_INSERT_TAIL(&records.list, rDest, tailq);
@@ -472,7 +471,7 @@ static IRAM_ATTR bool linked_list_add(const heap_trace_record_t *rAppend)
 }
 
 // search records.list backwards for the allocation record matching this address
-static IRAM_ATTR heap_trace_record_t* linked_list_find_address_reverse(void* p)
+static IRAM_ATTR heap_trace_record_t* list_find_address_reverse(void* p)
 {
     if (records.count == 0) {
         return NULL;

From a555563558423b56807c95ffc6dc77183d94ef5a Mon Sep 17 00:00:00 2001
From: Chip Weinberger <weinberger.c@gmail.com>
Date: Fri, 13 Jan 2023 12:33:43 -0800
Subject: [PATCH 3/4] [Heap Trace Standalone] speed up sequential access

---
 components/heap/heap_trace_standalone.c | 45 ++++++++++++++++++-------
 1 file changed, 32 insertions(+), 13 deletions(-)

diff --git a/components/heap/heap_trace_standalone.c b/components/heap/heap_trace_standalone.c
index 2c345ef361..c7c5734f81 100644
--- a/components/heap/heap_trace_standalone.c
+++ b/components/heap/heap_trace_standalone.c
@@ -41,10 +41,10 @@ typedef struct {
     /* Linked list of available record objects */
     heap_trace_record_list_t unused;
 
-    /* capacity of the buffer */
+    /* capacity of 'buffer' */
     size_t capacity;
 
-    /* Count of entries logged in the buffer. */
+    /* Count of entries in 'list' */
     size_t count;
 
     /* During execution, we remember the maximum
@@ -74,6 +74,10 @@ static size_t total_allocations;
 /* Actual number of frees logged */
 static size_t total_frees;
 
+/* Used to speed up heap_trace_get */
+static heap_trace_record_t* rGet;
+static size_t rGetIdx;
+
 esp_err_t heap_trace_init_standalone(heap_trace_record_t *record_buffer, size_t num_records)
 {
     if (tracing) {
@@ -160,24 +164,39 @@ esp_err_t heap_trace_get(size_t index, heap_trace_record_t *rOut)
 
     } else {
 
-        // Iterate through through the linked list
+        // Perf: speed up sequential access
+        if (rGet && rGetIdx == index - 1) {
 
-        heap_trace_record_t* rCur = TAILQ_FIRST(&records.list);
+            rGet = TAILQ_NEXT(rGet, tailq);
+            rGetIdx = index;
 
-        for (int i = 0; i < index; i++) {
+        } else {
 
-            // this should not happen, since we already
-            // checked that index >= records.count == false
-            if (rCur == NULL) {
-                result = ESP_ERR_INVALID_STATE;
-                break;
+            // Iterate through through the linked list
+
+            rGet = TAILQ_FIRST(&records.list);
+
+            for (int i = 0; i < index; i++) {
+
+                if (rGet == NULL) {
+                    break;
+                }
+
+                rGet = TAILQ_NEXT(rGet, tailq);
+                rGetIdx = i + 1;
             }
-
-            rCur = TAILQ_NEXT(rCur, tailq);
         }
 
         // copy to destination
-        memcpy(rOut, rCur, sizeof(heap_trace_record_t));
+        if (rGet) {
+            memcpy(rOut, rGet, 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));
+        }
     }
 
     portEXIT_CRITICAL(&trace_mux);

From 2cf9236f6cd158fd0a1b4a35e98bc7dd5ab63d35 Mon Sep 17 00:00:00 2001
From: Guillaume Souchere <guillaume.souchere@espressif.com>
Date: Tue, 14 Feb 2023 09:48:14 +0100
Subject: [PATCH 4/4] 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'.