From 244c369cd800ae604a43daae051241677195b01f Mon Sep 17 00:00:00 2001 From: Alexey Lapshin Date: Thu, 17 Oct 2024 13:18:25 +0700 Subject: [PATCH] fix(xtensa): fix confusing backtrace when PC is invalid 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 , 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 , pvParameters=0x0) at /home/alex/git/esp-idf/components/freertos/FreeRTOS-Kernel/portable/xtensa/port.c:139 --- components/esp_system/ld/esp32/sections.ld.in | 13 ++--------- .../esp_system/ld/esp32c2/sections.ld.in | 2 -- .../esp_system/ld/esp32c3/sections.ld.in | 2 -- .../esp_system/ld/esp32c5/sections.ld.in | 2 -- .../esp_system/ld/esp32c6/sections.ld.in | 2 -- .../esp_system/ld/esp32c61/sections.ld.in | 2 -- .../esp_system/ld/esp32h2/sections.ld.in | 2 -- .../esp_system/ld/esp32p4/sections.ld.in | 2 -- .../esp_system/ld/esp32s2/sections.ld.in | 13 ++--------- .../esp_system/ld/esp32s3/sections.ld.in | 13 ++--------- components/esp_system/port/panic_handler.c | 3 +-- components/xtensa/xtensa_vectors.S | 22 +++++++++++++------ tools/test_apps/system/panic/pytest_panic.py | 2 +- 13 files changed, 23 insertions(+), 57 deletions(-) diff --git a/components/esp_system/ld/esp32/sections.ld.in b/components/esp_system/ld/esp32/sections.ld.in index 17b6fc276e..930e1b4cda 100644 --- 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(.); + KEEP(*(._invalid_pc_placeholder.text)); + *(.*Vector.literal) - - *(.UserEnter.literal); - *(.UserEnter.text); - . = ALIGN (16); - *(.entry.literal) - *(.entry.text) - *(.init.literal) - *(.init) - - _init_end = ABSOLUTE(.); } > 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 index 529e5ee5e2..8986e49940 100644 --- 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 index e4bdd72eef..9ce7a6e8ea 100644 --- 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 index 5b2ce27ed2..1bac043fb7 100644 --- 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 index e89336d044..f2b2df7926 100644 --- 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 index 91023d3855..1403fcd041 100644 --- 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 index 6282646923..34fa0ea8cf 100644 --- 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 index e1de398dd1..8457cecbee 100644 --- 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 index fee3ceedeb..ed99aa0f96 100644 --- 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(.); + KEEP(*(._invalid_pc_placeholder.text)); + *(.*Vector.literal) - - *(.UserEnter.literal); - *(.UserEnter.text); - . = ALIGN (16); - *(.entry.literal) - *(.entry.text) - *(.init.literal) - *(.init) - - _init_end = ABSOLUTE(.); } > 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 index 38fb25911f..15432b9a36 100644 --- 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(.); + KEEP(*(._invalid_pc_placeholder.text)); + *(.*Vector.literal) - - *(.UserEnter.literal); - *(.UserEnter.text); - . = ALIGN (16); - *(.entry.literal) - *(.entry.text) - *(.init.literal) - *(.init) - - _init_end = ABSOLUTE(.); } > iram0_0_seg .iram0.text : diff --git a/components/esp_system/port/panic_handler.c b/components/esp_system/port/panic_handler.c index 14d0f39711..a815e1cbe1 100644 --- 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 index bcaa6b97a9..c677f29b31 100644 --- 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 index 4f522488e0..c4b45cb31e 100644 --- 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)