fix: take into account MR comments

components/esp_system/Kconfig

@@ -126,15 +126,15 @@ menu "ESP System Settings"
                 unwinding and generation of both .eh_frame and .eh_frame_hdr sections, resulting in a bigger binary
                 size (20% to 100% larger). The main purpose of this option is to be able to have a backtrace parsed
                 and printed by the program itself, regardless of the serial monitor used.
-                This option shall NOT be used for production.
+                This option is not recommended to be used for production.
 
         config ESP_SYSTEM_USE_FRAME_POINTER
             bool "Use CPU Frame Pointer register"
             help
                 This configuration allows the compiler to allocate CPU register s0 as the frame pointer. The main usage
                 of the frame pointer is to be able to generate a backtrace from the panic handler on exception.
-                Enabling this option results in bigger code and slower code since all functions will have to populate
-                this register and won't be able to use it as a general-purpose register anymore.
+                Enabling this option results in bigger and slightly slower code since all functions will have
+                to populate this register and won't be able to use it as a general-purpose register anymore.
 
     endchoice
 

components/esp_system/fp_unwind.c

@@ -1,5 +1,5 @@
 /*
- * SPDX-FileCopyrightText: 2024 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2024-2025 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Apache-2.0
  */
@@ -9,6 +9,7 @@
 #include "esp_private/panic_internal.h"
 #include "esp_memory_utils.h"
 #include "riscv/libunwind-riscv.h"
+#include "esp_private/fp_unwind.h"
 
 #define FP_MAX_CALLERS  64
 #define RA_INDEX_IN_FP  -1
@@ -21,7 +22,7 @@
  * @param pc Program counter of the backtrace step.
  * @param sp Stack pointer of the backtrace step.
  */
-void __attribute__((weak)) esp_eh_frame_generated_step(uint32_t pc, uint32_t sp)
+void __attribute__((weak)) esp_fp_generated_step(uint32_t pc, uint32_t sp)
 {
     panic_print_str(" 0x");
     panic_print_hex(pc);
@@ -46,7 +47,7 @@ static inline bool esp_fp_ptr_is_data(void* ptr)
  *
  * @param frame[in] Frame pointer to start unrolling from.
  * @param callers[out] Array of callers where 0 will store the most recent caller. Can be NULL.
- * @param stacks[out] Array of callers' stacks where 0 will store the most recent caller. Can be NULL.
+ * @param stacks[out] Array of callers' stacks where 0 will store the most recent caller's stack. Can be NULL.
  * @param depth[in] Number of maximum entries to fill in the callers array.
  *
  * @returns Number of entries filled in the array.
@@ -65,7 +66,7 @@ uint32_t IRAM_ATTR esp_fp_get_callers(uint32_t frame, void** callers, void** sta
     /**
      * We continue filling the callers array until we meet a function in ROM (not compiled
      * with frame pointer) or the pointer we retrieved is not in RAM (binary libraries
-     * not compiled with frame pointer configuration)
+     * not compiled with frame pointer configuration, which means what is stored in the register is not a valid pointer)
      */
     while (depth > 0 && esp_fp_ptr_is_data(fp) && !esp_ptr_in_rom((void *) pc)) {
         /* Dereference the RA register from the frame pointer and store it in PC */
@@ -104,12 +105,12 @@ void esp_fp_print_backtrace(const void *frame_or)
     void* stacks[FP_MAX_CALLERS];
 
     panic_print_str("Backtrace:");
-    esp_eh_frame_generated_step(pc, sp);
+    esp_fp_generated_step(pc, sp);
 
     uint32_t len = esp_fp_get_callers(fp, callers, stacks, FP_MAX_CALLERS);
 
     for (int i = 0; i < len; i++) {
-        esp_eh_frame_generated_step((uint32_t) callers[i], (uint32_t) stacks[i]);
+        esp_fp_generated_step((uint32_t) callers[i], (uint32_t) stacks[i]);
     }
 
     panic_print_str("\r\n");

components/esp_system/include/esp_private/fp_unwind.h

@@ -0,0 +1,27 @@
+/*
+ * SPDX-FileCopyrightText: 2025 Espressif Systems (Shanghai) CO LTD
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+#ifndef FP_UNWIND_H
+#define FP_UNWIND_H
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/**
+ * @brief Print backtrace for the given execution frame thanks to the frame pointers.
+ *
+ * @param frame_or Snapshot of the CPU registers when the program stopped its
+ *                 normal execution. This frame is usually generated on the
+ *                 stack when an exception or an interrupt occurs.
+ */
+void esp_fp_print_backtrace(const void *frame_or);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif // FP_UNWIND_H

components/esp_system/port/arch/riscv/debug_helpers.c

@@ -1,5 +1,5 @@
 /*
- * SPDX-FileCopyrightText: 2023-2024 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2023-2025 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Apache-2.0
  */
@@ -28,10 +28,9 @@ extern void panic_print_registers(const void *frame, int core);
 
 /* Targets based on a RISC-V CPU cannot perform backtracing that easily.
     * We have two options here:
-    *     - Perform backtracing at runtime.
+    *     - Perform backtracing at runtime thanks to the configuration options
+    *       CONFIG_ESP_SYSTEM_USE_EH_FRAME and CONFIG_ESP_SYSTEM_USE_FRAME_POINTER.
     *     - Let IDF monitor do the backtracing for us. Used during panic already.
-    * This could be configurable, choosing one or the other depending on
-    * CONFIG_ESP_SYSTEM_USE_EH_FRAME configuration option.
     *
     * In both cases, this takes time, and we might be in an ISR, we must
     * exit this handler as fast as possible, then we will simply print
@@ -64,6 +63,7 @@ esp_err_t IRAM_ATTR esp_backtrace_print(int depth)
 
     panic_print_registers(&backtrace_frame, current_core);
     esp_rom_printf("\r\n");
+    esp_rom_printf("Please enable CONFIG_ESP_SYSTEM_USE_FRAME_POINTER option to have a full backtrace.\r\n");
 #endif // CONFIG_ESP_SYSTEM_USE_EH_FRAME
 
     return ESP_OK;

components/esp_system/port/arch/riscv/panic_arch.c

@@ -1,5 +1,5 @@
 /*
- * SPDX-FileCopyrightText: 2020-2024 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2020-2025 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Apache-2.0
  */
@@ -24,6 +24,10 @@
 #include "esp_private/cache_utils.h"
 #endif
 
+#if CONFIG_ESP_SYSTEM_USE_FRAME_POINTER
+#include "esp_private/fp_unwind.h"
+#endif
+
 #if CONFIG_ESP_SYSTEM_HW_STACK_GUARD
 #include "freertos/FreeRTOS.h"
 #include "freertos/task.h"
@@ -336,7 +340,6 @@ void panic_print_backtrace(const void *frame, int core)
         esp_eh_frame_print_backtrace(frame);
     }
 #elif CONFIG_ESP_SYSTEM_USE_FRAME_POINTER
-    extern void esp_fp_print_backtrace(const void*);
     esp_fp_print_backtrace(frame);
 #else
     panic_print_basic_backtrace(frame, core);

components/esp_system/test_apps/esp_system_unity_tests/main/test_backtrace.c

@@ -1,5 +1,5 @@
 /*
- * SPDX-FileCopyrightText: 2021-2024 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2021-2025 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Apache-2.0
  */
@@ -11,16 +11,16 @@
 #include <stdlib.h>
 #include "unity.h"
 #include "test_utils.h"
+#include "esp_rom_sys.h"
+#include "esp_rom_uart.h"
 
-#if __XTENSA__
+#if CONFIG_IDF_TARGET_ARCH_XTENSA
 
 #include "freertos/FreeRTOS.h"
 #include "freertos/task.h"
 #include "xtensa_api.h" // Replace with interrupt allocator API (IDF-3891)
 #include "esp_debug_helpers.h"
 #include "esp_intr_alloc.h"
-#include "esp_rom_sys.h"
-#include "esp_rom_uart.h"
 #include "hal/misc.h"
 
 #define SW_ISR_LEVEL_1          7
@@ -145,4 +145,55 @@ TEST_CASE("Test esp_backtrace_print_all_tasks()", "[esp_system]")
     }
 }
 
+#endif // CONFIG_IDF_TARGET_ARCH_XTENSA
+
+#if CONFIG_ESP_SYSTEM_USE_FRAME_POINTER
+
+void my_putc(char c)
+{
+    static bool first_exec = 1;
+    esp_rom_output_putc(c);
+
+    if (first_exec) {
+        first_exec = false;
+        *((int*) 1) = 0;
+    }
+}
+
+// TEST_CASE("Test backtrace detects corrupted frames", "[esp_system]")
+TEST_CASE("Backtrace detects ROM functions", "[esp_system]")
+{
+    esp_rom_install_channel_putc(1, my_putc);
+    esp_rom_printf("foo");
+}
+
+static void __attribute__((naked)) non_framed_function(void)
+{
+    asm volatile(
+        "add sp, sp, -16\n"
+        /* Save anything on the top of the stack, not the frame pointer nor RA */
+        "sw zero, 12(sp)\n"
+        "sw s0, 8(sp)\n"
+        /* Set s0 to a constant that cannot be interpreted as an address */
+        "li s0, 0x42\n"
+        /* Fail to trigger an exception */
+        "sw s0, (s0)\n"
+        "ret\n"
+        ::
+    );
+}
+
+/**
+ * Test that backtracing detects when the frame is it unwinding encounters a function (or routine)
+ * that was not compiles with frame pointer option. This does NOT guarantee that all the call stack
+ * will be valid (some data pointers may be interpreted as frames), but it guarantees that no
+ * exception will be triggered.
+ */
+TEST_CASE("Backtrace detects corrupted frames", "[esp_system]")
+{
+    /* Add some prints to make sure the compiler doesn't optimize it with a tail call */
+    non_framed_function();
+    printf("ERROR: must not reach this point\n");
+}
+
 #endif

components/esp_system/test_apps/esp_system_unity_tests/pytest_esp_system_unity_tests.py

@@ -1,4 +1,4 @@
-# SPDX-FileCopyrightText: 2022-2024 Espressif Systems (Shanghai) CO LTD
+# SPDX-FileCopyrightText: 2022-2025 Espressif Systems (Shanghai) CO LTD
 # SPDX-License-Identifier: CC0-1.0
 import pytest
 from pytest_embedded import Dut
@@ -32,3 +32,27 @@ def test_stack_smash_protection(dut: Dut) -> None:
     dut.write('"stack smashing protection"')
     dut.expect_exact('Stack smashing protect failure!')
     dut.expect_exact('Rebooting...')
+
+
+@pytest.mark.generic
+@pytest.mark.parametrize(
+    'config',
+    [
+        # Testing this feature on a single RISC-V target is enough
+        pytest.param('framepointer', marks=[pytest.mark.esp32c3]),
+    ]
+)
+def test_frame_pointer_backtracing(dut: Dut) -> None:
+    dut.expect_exact('Press ENTER to see the list of tests')
+    dut.write('"Backtrace detects corrupted frames"')
+    dut.expect_exact('Guru Meditation Error')
+    # The backtrace should be composed of a single entry
+    dut.expect(r'Backtrace: 0x[0-9a-f]{8}:0x[0-9a-f]{8}\s*[\r]?\n')
+    dut.expect_exact('Rebooting...')
+
+    dut.expect_exact('Press ENTER to see the list of tests')
+    dut.write('"Backtrace detects ROM functions"')
+    dut.expect_exact('Guru Meditation Error')
+    # The backtrace should have two entries
+    dut.expect(r'Backtrace: 0x[0-9a-f]{8}:0x[0-9a-f]{8} 0x[0-9a-f]{8}:0x[0-9a-f]{8}\s*[\r]?\n')
+    dut.expect_exact('Rebooting...')

components/esp_system/test_apps/esp_system_unity_tests/sdkconfig.ci.framepointer

@@ -0,0 +1 @@
+CONFIG_ESP_SYSTEM_USE_FRAME_POINTER=y

components/heap/test_apps/heap_tests/pytest_heap.py

@@ -94,8 +94,6 @@ def test_heap_misc_options(dut: Dut) -> None:
 
 
 @pytest.mark.generic
-@pytest.mark.esp32
-@pytest.mark.esp32c3
 @pytest.mark.parametrize(
     'config',
     [

components/heap/test_apps/heap_tests/sdkconfig.ci.heap_trace_esp32

@@ -0,0 +1,3 @@
+CONFIG_IDF_TARGET="esp32"
+CONFIG_SPIRAM=y
+CONFIG_HEAP_TRACING_STANDALONE=y

components/heap/test_apps/heap_tests/sdkconfig.ci.heap_trace_esp32c3

@@ -0,0 +1,3 @@
+CONFIG_IDF_TARGET="esp32c3"
+CONFIG_ESP_SYSTEM_USE_FRAME_POINTER=y
+CONFIG_HEAP_TRACING_STANDALONE=y