From 0adf0f85dd335c69c40c225a279e269bcf9942e0 Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Wed, 30 Sep 2020 22:39:59 +0200 Subject: [PATCH] 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");