From c3d2942675e7920675f9bb00725b0afa72fb1536 Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Wed, 29 Jul 2020 12:20:52 +0200 Subject: [PATCH 1/2] panic: skip over the first invalid PC in case of InstrFetchProhibited InstrFetchProhibited usually occurs because of a jump to an invalid pointer. In this case, PC in the exception frame is the address of the jump destination. 'esp_ptr_executable' check in print_backtrace function recognizes the first frame as invalid, and the backtrace is interrupted. This prevents the user from finding the location where the invalid pointer is dereferenced. Bypass the 'esp_ptr_executable' check if the exception cause is InstrFetchProhibited. Update the test case to no longer ignore this issue. --- components/esp_system/port/panic_handler.c | 6 ++- tools/test_apps/system/panic/app_test.py | 51 +++++++++---------- .../panic/test_panic_util/test_panic_util.py | 4 ++ 3 files changed, 32 insertions(+), 29 deletions(-) diff --git a/components/esp_system/port/panic_handler.c b/components/esp_system/port/panic_handler.c index 4e28a92e00..dc71a9c0f9 100644 --- a/components/esp_system/port/panic_handler.c +++ b/components/esp_system/port/panic_handler.c @@ -158,7 +158,9 @@ static void print_backtrace(const void *f, int core) //Check if first frame is valid bool corrupted = !(esp_stack_ptr_is_sane(stk_frame.sp) && - esp_ptr_executable((void *)esp_cpu_process_stack_pc(stk_frame.pc))); + (esp_ptr_executable((void *)esp_cpu_process_stack_pc(stk_frame.pc)) || + /* Ignore the first corrupted PC in case of InstrFetchProhibited */ + frame->exccause == EXCCAUSE_INSTR_PROHIBITED)); uint32_t i = ((depth <= 0) ? INT32_MAX : depth) - 1; //Account for stack frame that's already printed while (i-- > 0 && stk_frame.next_pc != 0 && !corrupted) { @@ -457,7 +459,7 @@ static void frame_to_panic_info(XtExcFrame *frame, panic_info_t *info, bool pseu info->description = "Exception was unhandled."; - if (info->reason == reason[0]) { + if (frame->exccause == EXCCAUSE_ILLEGAL) { info->details = print_illegal_instruction_details; } } diff --git a/tools/test_apps/system/panic/app_test.py b/tools/test_apps/system/panic/app_test.py index e4c82e80d2..0b371ac957 100644 --- a/tools/test_apps/system/panic/app_test.py +++ b/tools/test_apps/system/panic/app_test.py @@ -10,7 +10,7 @@ def test_panic_task_wdt(env, extra_data): dut.expect("CPU 0: main") dut.expect(re.compile(r"abort\(\) was called at PC [0-9xa-f]+ on core 0")) dut.expect_none("register dump:") - dut.expect("Backtrace:") + dut.expect_backtrace() dut.expect_elf_sha256() dut.expect_none("CORRUPTED", "Guru Meditation") dut.expect("Rebooting...") @@ -21,12 +21,12 @@ def test_panic_int_wdt(env, extra_data): with get_dut(env, "panic", "test_int_wdt", qemu_wdt_enable=True) as dut: dut.expect_gme("Interrupt wdt timeout on CPU0") dut.expect_reg_dump(0) - dut.expect("Backtrace:") - dut.expect_none("CORRUPTED", "Guru Meditation") + dut.expect_backtrace() + dut.expect_none("Guru Meditation") dut.expect_reg_dump(1) - dut.expect("Backtrace:") + dut.expect_backtrace() dut.expect_elf_sha256() - dut.expect_none("CORRUPTED", "Guru Meditation") + dut.expect_none("Guru Meditation") dut.expect("Rebooting...") @@ -36,9 +36,9 @@ def test_cache_error(env, extra_data): dut.expect("Re-enable cpu cache.") dut.expect_gme("Cache disabled but cached memory region accessed") dut.expect_reg_dump(0) - dut.expect("Backtrace:") + dut.expect_backtrace() dut.expect_elf_sha256() - dut.expect_none("CORRUPTED", "Guru Meditation") + dut.expect_none("Guru Meditation") dut.expect("Rebooting...") @@ -48,12 +48,12 @@ def test_panic_int_wdt_cache_disabled(env, extra_data): dut.expect("Re-enable cpu cache.") dut.expect_gme("Interrupt wdt timeout on CPU0") dut.expect_reg_dump(0) - dut.expect("Backtrace:") - dut.expect_none("CORRUPTED", "Guru Meditation") + dut.expect_backtrace() + dut.expect_none("Guru Meditation") dut.expect_reg_dump(1) - dut.expect("Backtrace:") + dut.expect_backtrace() dut.expect_elf_sha256() - dut.expect_none("CORRUPTED", "Guru Meditation") + dut.expect_none("Guru Meditation") dut.expect("Rebooting...") @@ -62,8 +62,8 @@ def test_panic_abort(env, extra_data): with get_dut(env, "panic", "test_abort") as dut: dut.expect(re.compile(r"abort\(\) was called at PC [0-9xa-f]+ on core 0")) dut.expect_none("register dump:") - dut.expect("Backtrace:") - dut.expect_none("CORRUPTED", "Guru Meditation") + dut.expect_backtrace() + dut.expect_none("Guru Meditation") dut.expect("Rebooting...") @@ -72,9 +72,9 @@ def test_panic_storeprohibited(env, extra_data): with get_dut(env, "panic", "test_storeprohibited") as dut: dut.expect_gme("StoreProhibited") dut.expect_reg_dump(0) - dut.expect("Backtrace:") + dut.expect_backtrace() dut.expect_elf_sha256() - dut.expect_none("CORRUPTED", "Guru Meditation") + dut.expect_none("Guru Meditation") dut.expect("Rebooting...") @@ -84,9 +84,9 @@ def test_panic_stack_overflow(env, extra_data): dut.expect_gme("Unhandled debug exception") dut.expect("Stack canary watchpoint triggered (main)") dut.expect_reg_dump(0) - dut.expect("Backtrace:") + dut.expect_backtrace() dut.expect_elf_sha256() - dut.expect_none("CORRUPTED", "Guru Meditation") + dut.expect_none("Guru Meditation") dut.expect("Rebooting...") @@ -95,9 +95,9 @@ def test_panic_illegal_instruction(env, extra_data): with get_dut(env, "panic", "test_illegal_instruction") as dut: dut.expect_gme("IllegalInstruction") dut.expect_reg_dump(0) - dut.expect("Backtrace:") + dut.expect_backtrace() dut.expect_elf_sha256() - dut.expect_none("CORRUPTED", "Guru Meditation") + dut.expect_none("Guru Meditation") dut.expect("Rebooting...") @@ -106,10 +106,7 @@ def test_panic_instr_fetch_prohibited(env, extra_data): with get_dut(env, "panic", "test_instr_fetch_prohibited") as dut: dut.expect_gme("InstrFetchProhibited") dut.expect_reg_dump(0) - dut.expect("Backtrace:") - # At the moment the backtrace is corrupted, need to jump over the first PC in case of InstrFetchProhibited. - # Fix this and change expect to expect_none. - dut.expect("CORRUPTED") + dut.expect_backtrace() dut.expect_elf_sha256() dut.expect_none("Guru Meditation") dut.expect("Rebooting...") @@ -119,9 +116,9 @@ def test_panic_instr_fetch_prohibited(env, extra_data): def test_coredump_uart_abort(env, extra_data): with get_dut(env, "coredump_uart", "test_abort") as dut: dut.expect(re.compile(r"abort\(\) was called at PC [0-9xa-f]+ on core 0")) - dut.expect("Backtrace:") + dut.expect_backtrace() dut.expect_elf_sha256() - dut.expect_none("CORRUPTED", "Guru Meditation", "Re-entered core dump") + dut.expect_none("Guru Meditation", "Re-entered core dump") dut.expect(dut.COREDUMP_UART_END) dut.expect("Rebooting...") dut.process_coredump_uart() @@ -132,9 +129,9 @@ def test_coredump_uart_abort(env, extra_data): def test_coredump_flash_abort(env, extra_data): with get_dut(env, "coredump_flash", "test_abort") as dut: dut.expect(re.compile(r"abort\(\) was called at PC [0-9xa-f]+ on core 0")) - dut.expect("Backtrace:") + dut.expect_backtrace() dut.expect_elf_sha256() - dut.expect_none("CORRUPTED", "Guru Meditation", "Re-entered core dump") + dut.expect_none("Guru Meditation", "Re-entered core dump") dut.expect("Rebooting...") dut.process_coredump_flash() # TODO: check the contents of core dump output diff --git a/tools/test_apps/system/panic/test_panic_util/test_panic_util.py b/tools/test_apps/system/panic/test_panic_util/test_panic_util.py index 39f0cfda09..d50c100861 100644 --- a/tools/test_apps/system/panic/test_panic_util/test_panic_util.py +++ b/tools/test_apps/system/panic/test_panic_util/test_panic_util.py @@ -76,6 +76,10 @@ class PanicTestMixin(object): elf_sha256_len = int(sdkconfig.get("CONFIG_APP_RETRIEVE_LEN_ELF_SHA", "16")) self.expect("ELF file SHA256: " + elf_sha256[0:elf_sha256_len]) + def expect_backtrace(self): + self.expect("Backtrace:") + self.expect_none("CORRUPTED") + def __enter__(self): self._raw_data = None return self From 73813c6bacac29bd112c0f06277bcd3651cea61c Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Wed, 5 Aug 2020 12:50:34 +0200 Subject: [PATCH 2/2] freertos: ensure the interrupt stack is aligned CONFIG_FREERTOS_ISR_STACKSIZE was set to 2100 when ELF core dump was enabled, which resulted in a non-16-byte-aligned interrupt stack offset. This triggered "is SP corrupted" check in the backtrace, terminating the backtrace early. Fix the default value, and make sure that the stack is always aligned, regardless of the value of CONFIG_FREERTOS_ISR_STACKSIZE. --- components/freertos/Kconfig | 4 ++-- .../xtensa/include/freertos/FreeRTOSConfig.h | 12 +++++++++--- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/components/freertos/Kconfig b/components/freertos/Kconfig index de7a368ff7..fad37f3512 100644 --- a/components/freertos/Kconfig +++ b/components/freertos/Kconfig @@ -174,8 +174,8 @@ menu "FreeRTOS" config FREERTOS_ISR_STACKSIZE int "ISR stack size" - range 2100 32768 if ESP32_COREDUMP_DATA_FORMAT_ELF - default 2100 if ESP32_COREDUMP_DATA_FORMAT_ELF + range 2096 32768 if ESP32_COREDUMP_DATA_FORMAT_ELF + default 2096 if ESP32_COREDUMP_DATA_FORMAT_ELF range 1536 32768 default 1536 help diff --git a/components/freertos/xtensa/include/freertos/FreeRTOSConfig.h b/components/freertos/xtensa/include/freertos/FreeRTOSConfig.h index 51cbcbce2e..282ff0e547 100644 --- a/components/freertos/xtensa/include/freertos/FreeRTOSConfig.h +++ b/components/freertos/xtensa/include/freertos/FreeRTOSConfig.h @@ -192,10 +192,16 @@ int xt_clock_freq(void) __attribute__((deprecated)); #define configIDLE_TASK_STACK_SIZE CONFIG_FREERTOS_IDLE_TASK_STACKSIZE #endif -/* The Xtensa port uses a separate interrupt stack. Adjust the stack size */ -/* to suit the needs of your specific application. */ +/* Stack alignment, architecture specifc. Must be a power of two. */ +#define configSTACK_ALIGNMENT 16 + +/* The Xtensa port uses a separate interrupt stack. Adjust the stack size + * to suit the needs of your specific application. + * Size needs to be aligned to the stack increment, since the location of + * the stack for the 2nd CPU will be calculated using configISR_STACK_SIZE. + */ #ifndef configISR_STACK_SIZE -#define configISR_STACK_SIZE CONFIG_FREERTOS_ISR_STACKSIZE +#define configISR_STACK_SIZE ((CONFIG_FREERTOS_ISR_STACKSIZE + configSTACK_ALIGNMENT - 1) & (~(configSTACK_ALIGNMENT - 1))) #endif /* Minimal heap size to make sure examples can run on memory limited