From 5ff9cd495e8bf4c2bf669e97aa30b61278c874d5 Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Wed, 29 Jul 2020 12:20:52 +0200 Subject: [PATCH 1/4] 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/panic_tests.py | 7 ++----- .../system/panic/test_panic_util/test_panic_util.py | 4 ++++ 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/components/esp_system/port/panic_handler.c b/components/esp_system/port/panic_handler.c index de1dd3f26e..37d334a177 100644 --- a/components/esp_system/port/panic_handler.c +++ b/components/esp_system/port/panic_handler.c @@ -156,7 +156,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) { @@ -456,7 +458,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/panic_tests.py b/tools/test_apps/system/panic/panic_tests.py index e322c55583..c939f24ce5 100644 --- a/tools/test_apps/system/panic/panic_tests.py +++ b/tools/test_apps/system/panic/panic_tests.py @@ -107,10 +107,7 @@ def instr_fetch_prohibited_inner(env, test_name): with get_dut(env, test_name, "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_none("CORRUPTED", "Guru Meditation") test_common(dut, test_name) 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 709730317b39e323f1256c8b12c348890eb1feec Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Wed, 29 Jul 2020 12:23:29 +0200 Subject: [PATCH 2/4] panic: fix checks for corrupted backtrace in the test cases "CORRUPTED" needs to be checked before ELF SHA256. Use expect_backtrace in every test (which checks for it), remove extra check for CORRUPTED. --- tools/test_apps/system/panic/panic_tests.py | 40 ++++++++++----------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/tools/test_apps/system/panic/panic_tests.py b/tools/test_apps/system/panic/panic_tests.py index c939f24ce5..6fb93f13b8 100644 --- a/tools/test_apps/system/panic/panic_tests.py +++ b/tools/test_apps/system/panic/panic_tests.py @@ -19,9 +19,9 @@ def task_wdt_inner(env, test_name): 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_none("Guru Meditation") test_common(dut, test_name) @@ -29,12 +29,12 @@ def int_wdt_inner(env, test_name): with get_dut(env, test_name, "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") test_common(dut, test_name) @@ -44,11 +44,11 @@ def int_wdt_cache_disabled_inner(env, test_name): dut.expect_gme("Interrupt wdt timeout on CPU0") dut.expect_reg_dump(0) dut.expect("Backtrace:") - dut.expect_none("CORRUPTED", "Guru Meditation") + 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") test_common(dut, test_name) @@ -57,18 +57,18 @@ def cache_error_inner(env, test_name): 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") test_common(dut, test_name) def abort_inner(env, test_name): with get_dut(env, test_name, "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") test_common(dut, test_name) @@ -76,9 +76,9 @@ def storeprohibited_inner(env, test_name): with get_dut(env, test_name, "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") test_common(dut, test_name) @@ -87,9 +87,9 @@ def stack_overflow_inner(env, test_name): 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") test_common(dut, test_name) @@ -97,9 +97,9 @@ def illegal_instruction_inner(env, test_name): with get_dut(env, test_name, "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") test_common(dut, test_name) @@ -109,5 +109,5 @@ def instr_fetch_prohibited_inner(env, test_name): dut.expect_reg_dump(0) dut.expect_backtrace() dut.expect_elf_sha256() - dut.expect_none("CORRUPTED", "Guru Meditation") + dut.expect_none("Guru Meditation") test_common(dut, test_name) From 481409ec0540182c703e93b983d354c8e15f42f6 Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Wed, 29 Jul 2020 12:24:39 +0200 Subject: [PATCH 3/4] panic: allow running specific test cases from command line Small quality-of-life improvement, make it easier to run specific test cases, when debugging the tests locally. Take the optional list of test cases to run from the command line. --- tools/test_apps/system/panic/app_test.py | 3 ++- .../system/panic/test_panic_util/test_panic_util.py | 9 +++++++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/tools/test_apps/system/panic/app_test.py b/tools/test_apps/system/panic/app_test.py index 8dc316d60f..231f42daf8 100644 --- a/tools/test_apps/system/panic/app_test.py +++ b/tools/test_apps/system/panic/app_test.py @@ -1,4 +1,5 @@ #!/usr/bin/env python +import sys import panic_tests as test from test_panic_util.test_panic_util import panic_test, run_all @@ -247,4 +248,4 @@ def test_coredump_abort_flash_bin_crc(env, extra_data): if __name__ == '__main__': - run_all(__file__) + run_all(__file__, sys.argv[1:]) 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 d50c100861..6229f6c402 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 @@ -159,14 +159,19 @@ def get_dut(env, app_config_name, test_name, qemu_wdt_enable=False): return dut -def run_all(filename): - """ Helper function to run all test cases defined in a file; to be called from __main__. """ +def run_all(filename, case_filter=[]): + """ Helper function to run test cases defined in a file; to be called from __main__. + case_filter is an optional list of case names to run. + If not specified, all test cases are run. + """ TinyFW.set_default_config(env_config_file=None, test_suite_name=TEST_SUITE) test_methods = SearchCases.Search.search_test_cases(filename) test_methods = filter(lambda m: not m.case_info["ignore"], test_methods) test_cases = CaseConfig.Parser.apply_config(test_methods, None) tests_failed = [] for case in test_cases: + if case_filter and case.test_method.__name__ not in case_filter: + continue result = case.run() if not result: tests_failed.append(case) From 4e7e8598f3833eb9fe6798ad0c9b16c3b3aa3d3c Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Wed, 5 Aug 2020 12:50:34 +0200 Subject: [PATCH 4/4] 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 834067280c..c824ffda68 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 80b5e5e090..6e589c673e 100644 --- a/components/freertos/xtensa/include/freertos/FreeRTOSConfig.h +++ b/components/freertos/xtensa/include/freertos/FreeRTOSConfig.h @@ -196,10 +196,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