fix(xtensa): fix confusing backtrace when PC is invalid

Lapshin · Lapshin · commit 244c369cd800 · 2024-12-02T21:36:00.000+07:00
Before this change _invalid_pc_placeholder pointed to address of _init function from crti.o This made GDB input a bit confusing: 0x40080400 in _init () (gdb) bt #0 0x40080400 in _init () #1 0x400e519a in test_instr_fetch_prohibited () at /home/alex/git/esp-idf/tools/test_apps/system/panic/main/test_panic.c:271 #2 0x400d89a7 in app_main () at /home/alex/git/esp-idf/tools/test_apps/system/panic/main/test_app_main.c:116 #3 0x400e5f22 in main_task (args=0x0) at /home/alex/git/esp-idf/components/freertos/app_startup.c:208 #4 0x400895a8 in vPortTaskWrapper (pxCode=0x400e5eb0 <main_task>, pvParameters=0x0) at /home/alex/git/esp-idf/components/freertos/FreeRTOS-Kernel/portable/xtensa/port.c:139 After the change GDB prints output that contains a hint: _invalid_pc_placeholder () at /home/alex/git/esp-idf/components/xtensa/xtensa_vectors.S:2235 2235 UNREACHABLE_INSTRUCTION_CHECK_PREVIOUS_FRAMES (gdb) bt #0 _invalid_pc_placeholder () at /home/alex/git/esp-idf/components/xtensa/xtensa_vectors.S:2235 #1 0x400e519e in test_instr_fetch_prohibited () at /home/alex/git/esp-idf/tools/test_apps/system/panic/main/test_panic.c:271 #2 0x400d89ab in app_main () at /home/alex/git/esp-idf/tools/test_apps/system/panic/main/test_app_main.c:116 #3 0x400e5f26 in main_task (args=0x0) at /home/alex/git/esp-idf/components/freertos/app_startup.c:208 #4 0x400895a8 in vPortTaskWrapper (pxCode=0x400e5eb4 <main_task>, pvParameters=0x0) at /home/alex/git/esp-idf/components/freertos/FreeRTOS-Kernel/portable/xtensa/port.c:139
diff --git a/components/esp_system/ld/esp32/sections.ld.in b/components/esp_system/ld/esp32/sections.ld.in
@@ -212,18 +212,9 @@ SECTIONS
     . = 0x3C0;
     KEEP(*(.DoubleExceptionVector.text));
     . = 0x400;
-    _invalid_pc_placeholder = ABSOLUTE(.);
-    *(.*Vector.literal)
-
-    *(.UserEnter.literal);
-    *(.UserEnter.text);
-    . = ALIGN (16);
-    *(.entry.literal)
-    *(.entry.text)
-    *(.init.literal)
-    *(.init)
+    KEEP(*(._invalid_pc_placeholder.text));
 
-    _init_end = ABSOLUTE(.);
+    *(.*Vector.literal)
   } > iram0_0_seg
 
   .iram0.text :
diff --git a/components/esp_system/ld/esp32c2/sections.ld.in b/components/esp_system/ld/esp32c2/sections.ld.in
@@ -19,8 +19,6 @@ SECTIONS
     KEEP(*(.exception_vectors_table.text));
     KEEP(*(.exception_vectors.text));
 
-    ALIGNED_SYMBOL(4, _invalid_pc_placeholder)
-
     /* Code marked as running out of IRAM */
     _iram_text_start = ABSOLUTE(.);
 
diff --git a/components/esp_system/ld/esp32c3/sections.ld.in b/components/esp_system/ld/esp32c3/sections.ld.in
@@ -155,8 +155,6 @@ SECTIONS
     KEEP(*(.exception_vectors_table.text));
     KEEP(*(.exception_vectors.text));
 
-    ALIGNED_SYMBOL(4, _invalid_pc_placeholder)
-
     /* Code marked as running out of IRAM */
     _iram_text_start = ABSOLUTE(.);
 
diff --git a/components/esp_system/ld/esp32c5/sections.ld.in b/components/esp_system/ld/esp32c5/sections.ld.in
@@ -161,8 +161,6 @@ SECTIONS
     KEEP(*(.exception_vectors_table.text));
     KEEP(*(.exception_vectors.text));
 
-    ALIGNED_SYMBOL(4, _invalid_pc_placeholder)
-
     /* Code marked as running out of IRAM */
     _iram_text_start = ABSOLUTE(.);
 
diff --git a/components/esp_system/ld/esp32c6/sections.ld.in b/components/esp_system/ld/esp32c6/sections.ld.in
@@ -177,8 +177,6 @@ SECTIONS
     KEEP(*(.exception_vectors_table.text));
     KEEP(*(.exception_vectors.text));
 
-    ALIGNED_SYMBOL(4, _invalid_pc_placeholder)
-
     /* esp_tee_config_t structure: used to share information between the TEE and REE
      * (e.g. interrupt handler addresses, REE flash text-rodata boundaries, etc.)
      * This symbol is expected by the TEE at an offset of 0x300 from the vector table start.
diff --git a/components/esp_system/ld/esp32c61/sections.ld.in b/components/esp_system/ld/esp32c61/sections.ld.in
@@ -19,8 +19,6 @@ SECTIONS
     KEEP(*(.exception_vectors_table.text));
     KEEP(*(.exception_vectors.text));
 
-    ALIGNED_SYMBOL(4, _invalid_pc_placeholder)
-
     /* Code marked as running out of IRAM */
     _iram_text_start = ABSOLUTE(.);
 
diff --git a/components/esp_system/ld/esp32h2/sections.ld.in b/components/esp_system/ld/esp32h2/sections.ld.in
@@ -176,8 +176,6 @@ SECTIONS
     KEEP(*(.exception_vectors_table.text));
     KEEP(*(.exception_vectors.text));
 
-    ALIGNED_SYMBOL(4, _invalid_pc_placeholder)
-
     /* Code marked as running out of IRAM */
     _iram_text_start = ABSOLUTE(.);
 
diff --git a/components/esp_system/ld/esp32p4/sections.ld.in b/components/esp_system/ld/esp32p4/sections.ld.in
@@ -186,8 +186,6 @@ SECTIONS
     KEEP(*(.exception_vectors_table.text));
     KEEP(*(.exception_vectors.text));
 
-    ALIGNED_SYMBOL(4, _invalid_pc_placeholder)
-
     /* Code marked as running out of IRAM */
     _iram_text_start = ABSOLUTE(.);
 
diff --git a/components/esp_system/ld/esp32s2/sections.ld.in b/components/esp_system/ld/esp32s2/sections.ld.in
@@ -198,18 +198,9 @@ SECTIONS
     . = 0x3C0;
     KEEP(*(.DoubleExceptionVector.text));
     . = 0x400;
-    _invalid_pc_placeholder = ABSOLUTE(.);
-    *(.*Vector.literal)
-
-    *(.UserEnter.literal);
-    *(.UserEnter.text);
-    . = ALIGN (16);
-    *(.entry.literal)
-    *(.entry.text)
-    *(.init.literal)
-    *(.init)
+    KEEP(*(._invalid_pc_placeholder.text));
 
-    _init_end = ABSOLUTE(.);
+    *(.*Vector.literal)
   } > iram0_0_seg
 
   .iram0.text :
diff --git a/components/esp_system/ld/esp32s3/sections.ld.in b/components/esp_system/ld/esp32s3/sections.ld.in
@@ -181,18 +181,9 @@ SECTIONS
     . = 0x3C0;
     KEEP(*(.DoubleExceptionVector.text));
     . = 0x400;
-    _invalid_pc_placeholder = ABSOLUTE(.);
-    *(.*Vector.literal)
-
-    *(.UserEnter.literal);
-    *(.UserEnter.text);
-    . = ALIGN (16);
-    *(.entry.literal)
-    *(.entry.text)
-    *(.init.literal)
-    *(.init)
+    KEEP(*(._invalid_pc_placeholder.text));
 
-    _init_end = ABSOLUTE(.);
+    *(.*Vector.literal)
   } > iram0_0_seg
 
   .iram0.text :
diff --git a/components/esp_system/port/panic_handler.c b/components/esp_system/port/panic_handler.c
@@ -42,8 +42,6 @@
 #include "esp_private/hw_stack_guard.h"
 #endif
 
-extern int _invalid_pc_placeholder;
-
 extern void esp_panic_handler_reconfigure_wdts(uint32_t timeout_ms);
 
 extern void esp_panic_handler(panic_info_t *);
@@ -199,6 +197,7 @@ static void panic_handler(void *frame, bool pseudo_excause)
              * In case the PC is invalid, GDB will fail to translate addresses to function names
              * Hence replacing the PC to a placeholder address in case of invalid PC
              */
+            extern int _invalid_pc_placeholder;
             panic_set_address(frame, (uint32_t)&_invalid_pc_placeholder);
         }
 #endif
diff --git a/components/xtensa/xtensa_vectors.S b/components/xtensa/xtensa_vectors.S
@@ -3,7 +3,7 @@
  *
  * SPDX-License-Identifier: MIT
  *
- * SPDX-FileContributor: 2016-2023 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileContributor: 2016-2024 Espressif Systems (Shanghai) CO LTD
  */
 /*
  * Copyright (c) 2015-2019 Cadence Design Systems, Inc.
@@ -120,7 +120,7 @@
     frame just in case the exception dispatcher's SP does not point to the exception
     frame (which is the case when switching from task to interrupt stack).
 
-    Clearing the pseudo base-save area is uncessary as the interrupt dispatcher
+    Clearing the pseudo base-save area is unnecessary as the interrupt dispatcher
     will restore the current SP to that of the pre-exception SP.
 --------------------------------------------------------------------------------
 */
@@ -627,7 +627,7 @@ _UserExceptionVector:
 /*
 --------------------------------------------------------------------------------
   Insert some waypoints for jumping beyond the signed 8-bit range of
-  conditional branch instructions, so the conditional branchces to specific
+  conditional branch instructions, so the conditional branches to specific
   exception handlers are not taken in the mainline. Saves some cycles in the
   mainline.
 --------------------------------------------------------------------------------
@@ -741,7 +741,7 @@ _xt_handle_exc:
     rsr     a0, EXCVADDR
     s32i    a0, sp, XT_STK_EXCVADDR
 
-    /* Set up PS for C, reenable debug and NMI interrupts, and clear EXCM. */
+    /* Set up PS for C, re-enable debug and NMI interrupts, and clear EXCM. */
     #ifdef __XTENSA_CALL0_ABI__
     movi    a0, PS_INTLEVEL(XCHAL_DEBUGLEVEL - 2) | PS_UM
     #else
@@ -2223,8 +2223,16 @@ _WindowUnderflow12:
 
 #endif /* XCHAL_HAVE_WINDOWED */
 
-    .section    .UserEnter.text, "ax"
-    .global     call_user_start
-    .type       call_user_start,@function
+    .section    ._invalid_pc_placeholder.text, "ax"
+    .global     _invalid_pc_placeholder
+    .type       _invalid_pc_placeholder,@function
     .align      4
+_invalid_pc_placeholder:
+    /* This should be an entry instruction for correct stack unwinding.
+     * There could be just a line ".cfi_startproc", but unfortunately,
+     * CFI is not supported for the Xtensa architecture. */
+#define UNREACHABLE_INSTRUCTION_CHECK_PREVIOUS_FRAMES ENTRY0
+    UNREACHABLE_INSTRUCTION_CHECK_PREVIOUS_FRAMES
+    .size   _invalid_pc_placeholder, . - _invalid_pc_placeholder
+
     .literal_position
diff --git a/tools/test_apps/system/panic/pytest_panic.py b/tools/test_apps/system/panic/pytest_panic.py
@@ -362,7 +362,7 @@ def test_instr_fetch_prohibited(
         dut.expect_gme('InstrFetchProhibited')
         dut.expect_reg_dump(0)
         dut.expect_backtrace()
-        expected_backtrace = ['_init'] + get_default_backtrace(test_func_name)
+        expected_backtrace = ['_invalid_pc_placeholder'] + get_default_backtrace(test_func_name)
     else:
         dut.expect_gme('Instruction access fault')
         dut.expect_reg_dump(0)
