From 51d86081b884795d1c953daadf85ec995703cf2e Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Wed, 30 Sep 2020 22:39:59 +0200 Subject: [PATCH 1/3] gdbstub: fix thread list generation This commit fixes an issue with gdbstub, where it would list threads with TIDs 1 to N in qfThreadInfo/qsThreadInfo responses, and then would tell GDB that the current TID is 0 in the qC response. This caused an assertion failure in GDB, because it couldn't find the thread structure corresponding to TID 0: src/gdb/gdb/thread.c:93: internal-error: thread_info* inferior_thread(): Assertion `tp' failed. The issue was caused by the logic of qfThreadInfo/qsThreadInfo. If the "paniced" task index was 1, the code would report it in the response to qfThreadInfo, and then mistakenly skip task with index 0 in qsThreadInfo, due to the use of pre-increment instead of a post-increment. With that issue fixed, GDB assertion doesn't happen anymore. However the code contained a deeper problem, which manifested itself in the fact that GDB would incorrectly show task index 0 as the current task, after the above fix. Previous version of the code assumed that when GDB requests the thread list, it uses the first thread returned by the target as the "default" thread, and subsequently shows the user that the program is stopped in that thread. This assumption was incorrect. In fact, after connecting to a remote target, GDB obtains information about the "default" or "current" thread from two sources: 1. the 'thread' special register indicated in the status response ($T00thread;00000001#ee) 2. if the target has only sent the plain stop response ($T00#ee), GDB would ask for the current thread using a qC packet. With that in mind, it is not necessary to report the paniced task as the first task in qfThreadInfo response. We can simply returns the tasks in their natural order, and then indicate the current task in the qS packet response. However even that change does not fully resolve the issues with task list. The previous version of this code also incorrectly interpreted the meaning of GDB TIDs -1 and 0. When GDB sends an "Hg0" command early in the connection process, it doesn't expect the server to set task 0 as the current task, as the code assumed. Rather, it tells the server to "set any (arbitrary) task as the current one", and the most logical thing to do for the server that is already in "stopped" state is to keep the current task selection. Since TID 0 has a special meaning in GDB remote protocol, gdbstub code is now modified to map task indices (which start from 0) to GDB TIDs. GDB TIDs are arbitrary, and for simplicity we keep the same order and start counting them from 1. The summary of all the above changes is: 1. Use "task index + 1" as the TID reported to GDB 2. Report the tasks in natural order; don't complicate the code to make the paniced task first in the list. 3. Centralize modification of 'current_task_index' and 'regfile' in the new 'set_active_task' function, to improve encapsulation. --- components/esp_gdbstub/src/gdbstub.c | 129 ++++++++++++++++++--------- 1 file changed, 85 insertions(+), 44 deletions(-) diff --git a/components/esp_gdbstub/src/gdbstub.c b/components/esp_gdbstub/src/gdbstub.c index f15be617f2..2debe577cc 100644 --- a/components/esp_gdbstub/src/gdbstub.c +++ b/components/esp_gdbstub/src/gdbstub.c @@ -19,8 +19,11 @@ #ifdef CONFIG_ESP_GDBSTUB_SUPPORT_TASKS +static inline int gdb_tid_to_task_index(int tid); +static inline int task_index_to_gdb_tid(int tid); static void init_task_info(void); static void find_paniced_task_index(void); +static void set_active_task(size_t index); static int handle_task_commands(unsigned char *cmd, int len); #endif @@ -44,12 +47,13 @@ void esp_gdbstub_panic_handler(esp_gdbstub_frame_t *frame) s_scratch.state = GDBSTUB_STARTED; /* Save the paniced frame and get the list of tasks */ memcpy(&s_scratch.paniced_frame, frame, sizeof(*frame)); - esp_gdbstub_frame_to_regfile(frame, &s_scratch.regfile); init_task_info(); find_paniced_task_index(); /* Current task is the paniced task */ if (s_scratch.paniced_task_index == GDBSTUB_CUR_TASK_INDEX_UNKNOWN) { - s_scratch.current_task_index = 0; + set_active_task(0); + } else { + set_active_task(s_scratch.paniced_task_index); } } #endif // CONFIG_ESP_GDBSTUB_SUPPORT_TASKS @@ -162,6 +166,34 @@ int esp_gdbstub_handle_command(unsigned char *cmd, int len) #ifdef CONFIG_ESP_GDBSTUB_SUPPORT_TASKS +/* Some terminology related to task/thread indices: + * + * "task index" — Index used here in gdbstub.c to enumberate all the tasks. + * Range is from 0 to - 1. + * The order is defined by uxTaskGetSnapshotAll. + * + * "GDB TID" — Thread ID used by GDB internally and in the remote protocol. + * IDs of real threads can be any positive values other than 0. + * For example, OpenOCD uses the TCB pointer (cast to a 32-bit int) as GDB TID. + * Values 0 and -1, when used in place of TID in the remote protocol + * have special meanings: + * -1: indicates all threads, may be used in 'Hc' command + * 0: indicates an arbitrary process or thread (GDB client doesn't care, server decides) + * + * Since GDB TIDs are arbitrary, except that 0 is reserved, we set them using a simple rule: + * GDB TID = task index + 1. + * The two functions below perform the conversions between the kinds of IDs. + */ +static inline int gdb_tid_to_task_index(int tid) +{ + return tid - 1; +} + +static inline int task_index_to_gdb_tid(int tid) +{ + return tid + 1; +} + static void init_task_info(void) { unsigned tcb_size; @@ -191,30 +223,47 @@ static void find_paniced_task_index(void) s_scratch.paniced_task_index = GDBSTUB_CUR_TASK_INDEX_UNKNOWN; } +/** Select the current task and update the regfile */ +static void set_active_task(size_t index) +{ + if (index == s_scratch.paniced_task_index) { + /* Have to get the registers from exception frame */ + esp_gdbstub_frame_to_regfile(&s_scratch.paniced_frame, &s_scratch.regfile); + } else { + /* Get the registers from TCB. + * FIXME: for the task currently running on the other CPU, extracting the registers from TCB + * isn't valid. Need to use some IPC mechanism to obtain the registers of the other CPU. + */ + TaskHandle_t handle = NULL; + get_task_handle(index, &handle); + if (handle != NULL) { + esp_gdbstub_tcb_to_regfile(handle, &s_scratch.regfile); + } + } + s_scratch.current_task_index = index; +} + /** H command sets the "current task" for the purpose of further commands */ static void handle_H_command(const unsigned char* cmd, int len) { - if (cmd[1] == 'g' || cmd[1] == 'c') { - const char *ret = "OK"; + const char *ret = "OK"; + if (cmd[1] == 'g') { cmd += 2; - int requested_task_index = esp_gdbstub_gethex(&cmd, -1); - if (requested_task_index == s_scratch.paniced_task_index || - (requested_task_index == 0 && s_scratch.current_task_index == GDBSTUB_CUR_TASK_INDEX_UNKNOWN)) { - /* Get the registers of the paniced task */ - esp_gdbstub_frame_to_regfile(&s_scratch.paniced_frame, &s_scratch.regfile); - } else if (requested_task_index > s_scratch.task_count) { + int requested_tid = esp_gdbstub_gethex(&cmd, -1); + int requested_task_index = gdb_tid_to_task_index(requested_tid); + if (requested_tid == 0) { + /* 0 means "arbitrary process or thread", keep the current thread */ + } else if (requested_task_index >= s_scratch.task_count || + requested_tid == -1) { /* -1 means "all threads", which doesn't make sense for "Hg" */ ret = "E00"; } else { - TaskHandle_t handle = NULL; - get_task_handle(requested_task_index, &handle); - /* FIXME: for the task currently running on the other CPU, extracting the registers from TCB - * isn't valid. Need to use some IPC mechanism to obtain the registers of the other CPU - */ - if (handle != NULL) { - esp_gdbstub_tcb_to_regfile(handle, &s_scratch.regfile); - } + set_active_task(requested_task_index); } esp_gdbstub_send_str_packet(ret); + } else if (cmd[1] == 'c') { + /* Select the task for "continue" and "step" operations. No-op in post-mortem mode. */ + /* Note that the argument may be -1 to indicate "all threads" or 0 to indicate "any thread" */ + esp_gdbstub_send_str_packet(ret); } else { esp_gdbstub_send_str_packet(NULL); } @@ -225,7 +274,7 @@ static void handle_qC_command(const unsigned char* cmd, int len) { esp_gdbstub_send_start(); esp_gdbstub_send_str("QC"); - esp_gdbstub_send_hex(s_scratch.current_task_index, 32); + esp_gdbstub_send_hex(task_index_to_gdb_tid(s_scratch.current_task_index), 32); esp_gdbstub_send_end(); } @@ -238,50 +287,42 @@ static void handle_T_command(const unsigned char* cmd, int len) esp_gdbstub_send_str_packet("OK"); } +/** Called by qfThreadInfo and qsThreadInfo handlers */ +static void send_single_thread_info(int task_index) +{ + esp_gdbstub_send_start(); + esp_gdbstub_send_str("m"); + esp_gdbstub_send_hex(task_index_to_gdb_tid(task_index), 32); + esp_gdbstub_send_end(); +} + /** qfThreadInfo requests the start of the thread list, qsThreadInfo (below) is repeated to * get the subsequent threads. */ static void handle_qfThreadInfo_command(const unsigned char* cmd, int len) { - /* The first task in qfThreadInfo reply is going to be the one which GDB will request to stop. - * Therefore it has to be the paniced task. - * Reply with the paniced task index, and later skip over this index while handling qsThreadInfo - */ - esp_gdbstub_send_start(); - esp_gdbstub_send_str("m"); - esp_gdbstub_send_hex(s_scratch.paniced_task_index, 32); - esp_gdbstub_send_end(); - - s_scratch.thread_info_index = 0; + assert(s_scratch.task_count > 0); /* There should be at least one task */ + send_single_thread_info(0); + s_scratch.thread_info_index = 1; } static void handle_qsThreadInfo_command(const unsigned char* cmd, int len) { - int next_task_index = ++s_scratch.thread_info_index; - - if (next_task_index == s_scratch.task_count) { + int task_index = s_scratch.thread_info_index; + if (task_index == s_scratch.task_count) { /* No more tasks */ esp_gdbstub_send_str_packet("l"); return; } - - if (next_task_index == s_scratch.paniced_task_index) { - /* Have already sent this one in the reply to qfThreadInfo, skip over it */ - handle_qsThreadInfo_command(cmd, len); - return; - } - - esp_gdbstub_send_start(); - esp_gdbstub_send_str("m"); - esp_gdbstub_send_hex(next_task_index, 32); - esp_gdbstub_send_end(); + send_single_thread_info(s_scratch.thread_info_index); + s_scratch.thread_info_index++; } /** qThreadExtraInfo requests the thread name */ static void handle_qThreadExtraInfo_command(const unsigned char* cmd, int len) { cmd += sizeof("qThreadExtraInfo,") - 1; - int task_index = esp_gdbstub_gethex(&cmd, -1); + int task_index = gdb_tid_to_task_index(esp_gdbstub_gethex(&cmd, -1)); TaskHandle_t handle; if (!get_task_handle(task_index, &handle)) { esp_gdbstub_send_str_packet("E01"); From b1d64d1a617b60db620dcc2d6197da3c371cd5d6 Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Wed, 30 Sep 2020 22:41:14 +0200 Subject: [PATCH 2/3] test/panic: add gdbstub test configuration --- tools/test_apps/system/panic/app_test.py | 135 ++++++++++++------ tools/test_apps/system/panic/panic_tests.py | 50 ++++++- .../system/panic/sdkconfig.ci.gdbstub | 1 + .../panic/test_panic_util/test_panic_util.py | 102 ++++++++++++- 4 files changed, 232 insertions(+), 56 deletions(-) create mode 100644 tools/test_apps/system/panic/sdkconfig.ci.gdbstub diff --git a/tools/test_apps/system/panic/app_test.py b/tools/test_apps/system/panic/app_test.py index 231f42daf8..15ce730a3d 100644 --- a/tools/test_apps/system/panic/app_test.py +++ b/tools/test_apps/system/panic/app_test.py @@ -7,245 +7,290 @@ from test_panic_util.test_panic_util import panic_test, run_all # test_task_wdt @panic_test() -def test_panic_task_wdt(env, extra_data): +def test_panic_task_wdt(env, _extra_data): test.task_wdt_inner(env, "panic") @panic_test() -def test_coredump_task_wdt_uart_elf_crc(env, extra_data): +def test_coredump_task_wdt_uart_elf_crc(env, _extra_data): test.task_wdt_inner(env, "coredump_uart_elf_crc") @panic_test() -def test_coredump_task_wdt_uart_bin_crc(env, extra_data): +def test_coredump_task_wdt_uart_bin_crc(env, _extra_data): test.task_wdt_inner(env, "coredump_uart_bin_crc") @panic_test() -def test_coredump_task_wdt_flash_elf_sha(env, extra_data): +def test_coredump_task_wdt_flash_elf_sha(env, _extra_data): test.task_wdt_inner(env, "coredump_flash_elf_sha") @panic_test() -def test_coredump_task_wdt_flash_bin_crc(env, extra_data): +def test_coredump_task_wdt_flash_bin_crc(env, _extra_data): test.task_wdt_inner(env, "coredump_flash_bin_crc") +@panic_test() +def test_gdbstub_task_wdt(env, _extra_data): + test.task_wdt_inner(env, "gdbstub") + + # test_int_wdt @panic_test() -def test_panic_int_wdt(env, extra_data): +def test_panic_int_wdt(env, _extra_data): test.int_wdt_inner(env, "panic") @panic_test() -def test_coredump_int_wdt_uart_elf_crc(env, extra_data): +def test_coredump_int_wdt_uart_elf_crc(env, _extra_data): test.int_wdt_inner(env, "coredump_uart_elf_crc") @panic_test() -def test_coredump_int_wdt_uart_bin_crc(env, extra_data): +def test_coredump_int_wdt_uart_bin_crc(env, _extra_data): test.int_wdt_inner(env, "coredump_uart_bin_crc") @panic_test() -def test_coredump_int_wdt_flash_elf_sha(env, extra_data): +def test_coredump_int_wdt_flash_elf_sha(env, _extra_data): test.int_wdt_inner(env, "coredump_flash_elf_sha") @panic_test() -def test_coredump_int_wdt_flash_bin_crc(env, extra_data): +def test_coredump_int_wdt_flash_bin_crc(env, _extra_data): test.int_wdt_inner(env, "coredump_flash_bin_crc") +@panic_test() +def test_gdbstub_int_wdt(env, _extra_data): + test.int_wdt_inner(env, "gdbstub") + + # test_int_wdt_cache_disabled @panic_test() -def test_panic_int_wdt_cache_disabled(env, extra_data): +def test_panic_int_wdt_cache_disabled(env, _extra_data): test.int_wdt_cache_disabled_inner(env, "panic") @panic_test() -def test_coredump_int_wdt_cache_disabled_uart_elf_crc(env, extra_data): +def test_coredump_int_wdt_cache_disabled_uart_elf_crc(env, _extra_data): test.int_wdt_cache_disabled_inner(env, "coredump_uart_elf_crc") @panic_test() -def test_coredump_int_wdt_cache_disabled_uart_bin_crc(env, extra_data): +def test_coredump_int_wdt_cache_disabled_uart_bin_crc(env, _extra_data): test.int_wdt_cache_disabled_inner(env, "coredump_uart_bin_crc") @panic_test() -def test_coredump_int_wdt_cache_disabled_flash_elf_sha(env, extra_data): +def test_coredump_int_wdt_cache_disabled_flash_elf_sha(env, _extra_data): test.int_wdt_cache_disabled_inner(env, "coredump_flash_elf_sha") @panic_test() -def test_coredump_int_wdt_cache_disabled_flash_bin_crc(env, extra_data): +def test_coredump_int_wdt_cache_disabled_flash_bin_crc(env, _extra_data): test.int_wdt_cache_disabled_inner(env, "coredump_flash_bin_crc") +@panic_test() +def test_gdbstub_int_wdt_cache_disabled(env, _extra_data): + test.int_wdt_cache_disabled_inner(env, "gdbstub") + + # test_cache_error @panic_test() -def test_panic_cache_error(env, extra_data): +def test_panic_cache_error(env, _extra_data): test.cache_error_inner(env, "panic") @panic_test() -def test_coredump_cache_error_uart_elf_crc(env, extra_data): +def test_coredump_cache_error_uart_elf_crc(env, _extra_data): test.cache_error_inner(env, "coredump_uart_elf_crc") @panic_test() -def test_coredump_cache_error_uart_bin_crc(env, extra_data): +def test_coredump_cache_error_uart_bin_crc(env, _extra_data): test.cache_error_inner(env, "coredump_uart_bin_crc") @panic_test() -def test_coredump_cache_error_flash_elf_sha(env, extra_data): +def test_coredump_cache_error_flash_elf_sha(env, _extra_data): test.cache_error_inner(env, "coredump_flash_elf_sha") @panic_test() -def test_coredump_cache_error_flash_bin_crc(env, extra_data): +def test_coredump_cache_error_flash_bin_crc(env, _extra_data): test.cache_error_inner(env, "coredump_flash_bin_crc") +@panic_test() +def test_gdbstub_cache_error(env, _extra_data): + test.cache_error_inner(env, "gdbstub") + + # test_stack_overflow @panic_test() -def test_panic_stack_overflow(env, extra_data): +def test_panic_stack_overflow(env, _extra_data): test.stack_overflow_inner(env, "panic") @panic_test() -def test_coredump_stack_overflow_uart_elf_crc(env, extra_data): +def test_coredump_stack_overflow_uart_elf_crc(env, _extra_data): test.stack_overflow_inner(env, "coredump_uart_elf_crc") @panic_test() -def test_coredump_stack_overflow_uart_bin_crc(env, extra_data): +def test_coredump_stack_overflow_uart_bin_crc(env, _extra_data): test.stack_overflow_inner(env, "coredump_uart_bin_crc") @panic_test() -def test_coredump_stack_overflow_flash_elf_sha(env, extra_data): +def test_coredump_stack_overflow_flash_elf_sha(env, _extra_data): test.stack_overflow_inner(env, "coredump_flash_elf_sha") @panic_test() -def test_coredump_stack_overflow_flash_bin_crc(env, extra_data): +def test_coredump_stack_overflow_flash_bin_crc(env, _extra_data): test.stack_overflow_inner(env, "coredump_flash_bin_crc") +@panic_test() +def test_gdbstub_stack_overflow(env, _extra_data): + test.stack_overflow_inner(env, "gdbstub") + + # test_instr_fetch_prohibited @panic_test() -def test_panic_instr_fetch_prohibited(env, extra_data): +def test_panic_instr_fetch_prohibited(env, _extra_data): test.instr_fetch_prohibited_inner(env, "panic") @panic_test() -def test_coredump_instr_fetch_prohibited_uart_elf_crc(env, extra_data): +def test_coredump_instr_fetch_prohibited_uart_elf_crc(env, _extra_data): test.instr_fetch_prohibited_inner(env, "coredump_uart_elf_crc") @panic_test() -def test_coredump_instr_fetch_prohibited_uart_bin_crc(env, extra_data): +def test_coredump_instr_fetch_prohibited_uart_bin_crc(env, _extra_data): test.instr_fetch_prohibited_inner(env, "coredump_uart_bin_crc") @panic_test() -def test_coredump_instr_fetch_prohibited_flash_elf_sha(env, extra_data): +def test_coredump_instr_fetch_prohibited_flash_elf_sha(env, _extra_data): test.instr_fetch_prohibited_inner(env, "coredump_flash_elf_sha") @panic_test() -def test_coredump_instr_fetch_prohibited_flash_bin_crc(env, extra_data): +def test_coredump_instr_fetch_prohibited_flash_bin_crc(env, _extra_data): test.instr_fetch_prohibited_inner(env, "coredump_flash_bin_crc") +@panic_test() +def test_gdbstub_instr_fetch_prohibited(env, _extra_data): + test.instr_fetch_prohibited_inner(env, "gdbstub") + + # test_illegal_instruction @panic_test() -def test_panic_illegal_instruction(env, extra_data): +def test_panic_illegal_instruction(env, _extra_data): test.illegal_instruction_inner(env, "panic") @panic_test() -def test_coredump_illegal_instruction_uart_elf_crc(env, extra_data): +def test_coredump_illegal_instruction_uart_elf_crc(env, _extra_data): test.illegal_instruction_inner(env, "coredump_uart_elf_crc") @panic_test() -def test_coredump_illegal_instruction_uart_bin_crc(env, extra_data): +def test_coredump_illegal_instruction_uart_bin_crc(env, _extra_data): test.illegal_instruction_inner(env, "coredump_uart_bin_crc") @panic_test() -def test_coredump_illegal_instruction_flash_elf_sha(env, extra_data): +def test_coredump_illegal_instruction_flash_elf_sha(env, _extra_data): test.illegal_instruction_inner(env, "coredump_flash_elf_sha") @panic_test() -def test_coredump_illegal_instruction_flash_bin_crc(env, extra_data): +def test_coredump_illegal_instruction_flash_bin_crc(env, _extra_data): test.illegal_instruction_inner(env, "coredump_flash_bin_crc") +@panic_test() +def test_gdbstub_illegal_instruction(env, _extra_data): + test.illegal_instruction_inner(env, "gdbstub") + + # test_storeprohibited @panic_test() -def test_panic_storeprohibited(env, extra_data): +def test_panic_storeprohibited(env, _extra_data): test.storeprohibited_inner(env, "panic") @panic_test() -def test_coredump_storeprohibited_uart_elf_crc(env, extra_data): +def test_coredump_storeprohibited_uart_elf_crc(env, _extra_data): test.storeprohibited_inner(env, "coredump_uart_elf_crc") @panic_test() -def test_coredump_storeprohibited_uart_bin_crc(env, extra_data): +def test_coredump_storeprohibited_uart_bin_crc(env, _extra_data): test.storeprohibited_inner(env, "coredump_uart_bin_crc") @panic_test() -def test_coredump_storeprohibited_flash_elf_sha(env, extra_data): +def test_coredump_storeprohibited_flash_elf_sha(env, _extra_data): test.storeprohibited_inner(env, "coredump_flash_elf_sha") @panic_test() -def test_coredump_storeprohibited_flash_bin_crc(env, extra_data): +def test_coredump_storeprohibited_flash_bin_crc(env, _extra_data): test.storeprohibited_inner(env, "coredump_flash_bin_crc") +@panic_test() +def test_gdbstub_storeprohibited(env, _extra_data): + test.storeprohibited_inner(env, "gdbstub") + + # test_abort @panic_test() -def test_panic_abort(env, extra_data): +def test_panic_abort(env, _extra_data): test.abort_inner(env, "panic") @panic_test() -def test_coredump_abort_uart_elf_crc(env, extra_data): +def test_coredump_abort_uart_elf_crc(env, _extra_data): test.abort_inner(env, "coredump_uart_elf_crc") @panic_test() -def test_coredump_abort_uart_bin_crc(env, extra_data): +def test_coredump_abort_uart_bin_crc(env, _extra_data): test.abort_inner(env, "coredump_uart_bin_crc") @panic_test() -def test_coredump_abort_flash_elf_sha(env, extra_data): +def test_coredump_abort_flash_elf_sha(env, _extra_data): test.abort_inner(env, "coredump_flash_elf_sha") @panic_test() -def test_coredump_abort_flash_bin_crc(env, extra_data): +def test_coredump_abort_flash_bin_crc(env, _extra_data): test.abort_inner(env, "coredump_flash_bin_crc") +@panic_test() +def test_gdbstub_abort(env, _extra_data): + test.abort_inner(env, "gdbstub") + + if __name__ == '__main__': run_all(__file__, sys.argv[1:]) diff --git a/tools/test_apps/system/panic/panic_tests.py b/tools/test_apps/system/panic/panic_tests.py index 6fb93f13b8..efb629391f 100644 --- a/tools/test_apps/system/panic/panic_tests.py +++ b/tools/test_apps/system/panic/panic_tests.py @@ -1,16 +1,43 @@ #!/usr/bin/env python +from pprint import pformat import re from test_panic_util.test_panic_util import get_dut -def test_common(dut, test_name): +def get_default_backtrace(test_name): + return [ + test_name, + "app_main", + "main_task", + "vPortTaskWrapper" + ] + + +def test_common(dut, test_name, expected_backtrace=None): + if expected_backtrace is None: + expected_backtrace = get_default_backtrace(dut.test_name) + + if "gdbstub" in test_name: + dut.start_gdb() + frames = dut.gdb_backtrace() + if not dut.match_backtrace(frames, expected_backtrace): + raise AssertionError("Unexpected backtrace in test {}:\n{}".format(test_name, pformat(frames))) + return + if "uart" in test_name: dut.expect(dut.COREDUMP_UART_END) + dut.expect("Rebooting...") + if "uart" in test_name: - dut.process_coredump_uart + dut.process_coredump_uart() + # TODO: check backtrace elif "flash" in test_name: - dut.process_coredump_flash + dut.process_coredump_flash() + # TODO: check backtrace + elif "panic" in test_name: + # TODO: check backtrace + pass def task_wdt_inner(env, test_name): @@ -22,7 +49,11 @@ def task_wdt_inner(env, test_name): dut.expect_backtrace() dut.expect_elf_sha256() dut.expect_none("Guru Meditation") - test_common(dut, test_name) + test_common(dut, test_name, expected_backtrace=[ + # Backtrace interrupted when abort is called, IDF-842. + # Task WDT calls abort internally. + "panic_abort", "esp_system_abort" + ]) def int_wdt_inner(env, test_name): @@ -60,7 +91,8 @@ def cache_error_inner(env, test_name): dut.expect_backtrace() dut.expect_elf_sha256() dut.expect_none("Guru Meditation") - test_common(dut, test_name) + test_common(dut, test_name, + expected_backtrace=["die"] + get_default_backtrace(dut.test_name)) def abort_inner(env, test_name): @@ -69,7 +101,10 @@ def abort_inner(env, test_name): dut.expect_backtrace() dut.expect_elf_sha256() dut.expect_none("Guru Meditation", "Re-entered core dump") - test_common(dut, test_name) + test_common(dut, test_name, expected_backtrace=[ + # Backtrace interrupted when abort is called, IDF-842 + "panic_abort", "esp_system_abort" + ]) def storeprohibited_inner(env, test_name): @@ -110,4 +145,5 @@ def instr_fetch_prohibited_inner(env, test_name): dut.expect_backtrace() dut.expect_elf_sha256() dut.expect_none("Guru Meditation") - test_common(dut, test_name) + test_common(dut, test_name, + expected_backtrace=["_init"] + get_default_backtrace(dut.test_name)) diff --git a/tools/test_apps/system/panic/sdkconfig.ci.gdbstub b/tools/test_apps/system/panic/sdkconfig.ci.gdbstub new file mode 100644 index 0000000000..38830f8dd6 --- /dev/null +++ b/tools/test_apps/system/panic/sdkconfig.ci.gdbstub @@ -0,0 +1 @@ +CONFIG_ESP_SYSTEM_PANIC_GDBSTUB=y 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 6229f6c402..f2de09a327 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 @@ -1,7 +1,9 @@ +import logging import os -import sys +from pygdbmi.gdbcontroller import GdbController import re import subprocess +import sys import ttfw_idf from tiny_test_fw import Utility, TinyFW, DUT from tiny_test_fw.Utility import SearchCases, CaseConfig @@ -82,6 +84,7 @@ class PanicTestMixin(object): def __enter__(self): self._raw_data = None + self.gdb = None return self def __exit__(self, type, value, traceback): @@ -89,7 +92,8 @@ class PanicTestMixin(object): with open(os.path.join(log_folder, "log_" + self.test_name + ".txt"), "w") as log_file: Utility.console_log("Writing output of {} to {}".format(self.test_name, log_file.name)) log_file.write(self.get_raw_data()) - + if self.gdb: + self.gdb.exit() self.close() def get_raw_data(self): @@ -140,6 +144,90 @@ class PanicTestMixin(object): output_file_name = os.path.join(log_folder, "coredump_flash_result_" + self.test_name + ".txt") self._call_espcoredump(["--core-format", "raw"], coredump_file_name, output_file_name) + def start_gdb(self): + """ + Runs GDB and connects it to the "serial" port of the DUT. + After this, the DUT expect methods can no longer be used to capture output. + """ + self.stop_receive() + self._port_close() + + Utility.console_log("Starting GDB...", "orange") + self.gdb = GdbController(gdb_path=self.TOOLCHAIN_PREFIX + "gdb") + + # pygdbmi logs to console by default, make it log to a file instead + log_folder = self.app.get_log_folder(TEST_SUITE) + pygdbmi_log_file_name = os.path.join(log_folder, "pygdbmi_log_" + self.test_name + ".txt") + pygdbmi_logger = self.gdb.logger + pygdbmi_logger.setLevel(logging.DEBUG) + while pygdbmi_logger.hasHandlers(): + pygdbmi_logger.removeHandler(pygdbmi_logger.handlers[0]) + log_handler = logging.FileHandler(pygdbmi_log_file_name) + log_handler.setFormatter(logging.Formatter("%(asctime)s %(levelname)s: %(message)s")) + pygdbmi_logger.addHandler(log_handler) + + # Set up logging for GDB remote protocol + gdb_remotelog_file_name = os.path.join(log_folder, "gdb_remote_log_" + self.test_name + ".txt") + self.gdb.write("-gdb-set remotelogfile " + gdb_remotelog_file_name) + + # Load the ELF file + self.gdb.write("-file-exec-and-symbols {}".format(self.app.elf_file)) + + # Connect GDB to UART + Utility.console_log("Connecting to GDB Stub...", "orange") + self.gdb.write("-gdb-set serial baud 115200") + responses = self.gdb.write("-target-select remote " + self.get_gdb_remote(), timeout_sec=3) + + # Make sure we get the 'stopped' notification + stop_response = self.find_gdb_response('stopped', 'notify', responses) + if not stop_response: + responses = self.gdb.write("-exec-interrupt", timeout_sec=3) + stop_response = self.find_gdb_response('stopped', 'notify', responses) + assert stop_response + frame = stop_response["payload"]["frame"] + if "file" not in frame: + frame["file"] = "?" + if "line" not in frame: + frame["line"] = "?" + Utility.console_log("Stopped in {func} at {addr} ({file}:{line})".format(**frame), "orange") + + # Drain remaining responses + self.gdb.get_gdb_response(raise_error_on_timeout=False) + + def gdb_backtrace(self): + """ + Returns the list of stack frames for the current thread. + Each frame is a dictionary, refer to pygdbmi docs for the format. + """ + assert self.gdb + + responses = self.gdb.write("-stack-list-frames", timeout_sec=3) + return self.find_gdb_response("done", "result", responses)["payload"]["stack"] + + @staticmethod + def match_backtrace(gdb_backtrace, expected_functions_list): + """ + Returns True if the function names listed in expected_functions_list match the backtrace + given by gdb_backtrace argument. The latter is in the same format as returned by gdb_backtrace() + function. + """ + return all([frame["func"] == expected_functions_list[i] for i, frame in enumerate(gdb_backtrace)]) + + @staticmethod + def find_gdb_response(message, response_type, responses): + """ + Helper function which extracts one response from an array of GDB responses, filtering + by message and type. Returned message is a dictionary, refer to pygdbmi docs for the format. + """ + def match_response(response): + return (response["message"] == message and + response["type"] == response_type) + + filtered_responses = [r for r in responses if match_response(r)] + if not filtered_responses: + return None + return filtered_responses[0] + class ESP32PanicTestDUT(ttfw_idf.ESP32DUT, PanicTestMixin): def get_gdb_remote(self): @@ -170,8 +258,14 @@ def run_all(filename, case_filter=[]): 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 + test_name = case.test_method.__name__ + if case_filter: + if case_filter[0].endswith("*"): + if not test_name.startswith(case_filter[0][:-1]): + continue + else: + if test_name not in case_filter: + continue result = case.run() if not result: tests_failed.append(case) From 2218ac91c27bd9ed01161b1336b33b862c3e0585 Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Wed, 30 Sep 2020 22:41:53 +0200 Subject: [PATCH 3/3] tools/ci: fix import error when running ttfw tests locally --- tools/ci/python_packages/ttfw_idf/IDFAssignTest.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tools/ci/python_packages/ttfw_idf/IDFAssignTest.py b/tools/ci/python_packages/ttfw_idf/IDFAssignTest.py index 75c581c1e8..04f33ae0cd 100644 --- a/tools/ci/python_packages/ttfw_idf/IDFAssignTest.py +++ b/tools/ci/python_packages/ttfw_idf/IDFAssignTest.py @@ -17,7 +17,11 @@ except ImportError: import gitlab_api from tiny_test_fw.Utility import CIAssignTest -from idf_py_actions.constants import SUPPORTED_TARGETS, PREVIEW_TARGETS +try: + from idf_py_actions.constants import SUPPORTED_TARGETS, PREVIEW_TARGETS +except ImportError: + SUPPORTED_TARGETS = [] + PREVIEW_TARGETS = [] IDF_PATH_FROM_ENV = os.getenv('IDF_PATH')