From efce094b409179acbaa7726c112a10fcf8343503 Mon Sep 17 00:00:00 2001 From: Tim Newsome Date: Wed, 14 Aug 2019 11:56:44 -0700 Subject: [PATCH] Don't fake step for hwthread rtos. (#393) Fake step is a hack introduced to make things work with real RTOSs that have a concept of a current thread. The hwthread rtos always has access to all threads, so doesn't need it. This fixes a bug when running my MulticoreRegTest against HiFive Unleashed where OpenOCD would return the registers of the wrong thread after gdb stepped a hart. Change-Id: I64f538a133fb078c05a0c6b8121388b0b9d7f1b8 --- src/rtos/hwthread.c | 8 +++++++- src/rtos/rtos.c | 7 +++++++ src/rtos/rtos.h | 8 ++++++++ src/server/gdb_server.c | 14 ++++---------- 4 files changed, 26 insertions(+), 11 deletions(-) diff --git a/src/rtos/hwthread.c b/src/rtos/hwthread.c index 90ba95850..61dd3388f 100644 --- a/src/rtos/hwthread.c +++ b/src/rtos/hwthread.c @@ -38,6 +38,7 @@ static int hwthread_get_thread_reg_list(struct rtos *rtos, int64_t thread_id, static int hwthread_get_symbol_list_to_lookup(symbol_table_elem_t *symbol_list[]); static int hwthread_smp_init(struct target *target); int hwthread_set_reg(struct rtos *rtos, uint32_t reg_num, uint8_t *reg_value); +bool hwthread_needs_fake_step(struct target *target, int64_t thread_id); #define HW_THREAD_NAME_STR_SIZE (32) @@ -58,6 +59,7 @@ const struct rtos_type hwthread_rtos = { .get_symbol_list_to_lookup = hwthread_get_symbol_list_to_lookup, .smp_init = hwthread_smp_init, .set_reg = hwthread_set_reg, + .needs_fake_step = hwthread_needs_fake_step }; struct hwthread_params { @@ -353,7 +355,6 @@ static int hwthread_thread_packet(struct connection *connection, const char *pac int64_t current_threadid; if (packet[0] == 'H' && packet[1] == 'g') { - /* Never reached, because this case is handled by rtos_thread_packet(). */ sscanf(packet, "Hg%16" SCNx64, ¤t_threadid); if (current_threadid > 0) { @@ -387,3 +388,8 @@ static int hwthread_create(struct target *target) target->rtos->gdb_thread_packet = hwthread_thread_packet; return 0; } + +bool hwthread_needs_fake_step(struct target *target, int64_t thread_id) +{ + return false; +} diff --git a/src/rtos/rtos.c b/src/rtos/rtos.c index d4eff5b26..96dc99b55 100644 --- a/src/rtos/rtos.c +++ b/src/rtos/rtos.c @@ -673,3 +673,10 @@ void rtos_free_threadlist(struct rtos *rtos) rtos->current_thread = 0; } } + +bool rtos_needs_fake_step(struct target *target, int64_t thread_id) +{ + if (target->rtos->type->needs_fake_step) + return target->rtos->type->needs_fake_step(target, thread_id); + return target->rtos->current_thread != thread_id; +} diff --git a/src/rtos/rtos.h b/src/rtos/rtos.h index 81829b1c5..35420b1b2 100644 --- a/src/rtos/rtos.h +++ b/src/rtos/rtos.h @@ -83,6 +83,13 @@ struct rtos_type { int (*clean)(struct target *target); char * (*ps_command)(struct target *target); int (*set_reg)(struct rtos *rtos, uint32_t reg_num, uint8_t *reg_value); + /** + * Possibly work around an annoying gdb behaviour: when the current thread + * is changed in gdb, it assumes that the target can follow and also make + * the thread current. This is an assumption that cannot hold for a real + * target running a multi-threading OS. If an RTOS can do this, override + * needs_fake_step(). */ + bool (*needs_fake_step)(struct target *target, int64_t thread_id); }; struct stack_register_offset { @@ -128,5 +135,6 @@ void rtos_free_threadlist(struct rtos *rtos); int rtos_smp_init(struct target *target); /* function for handling symbol access */ int rtos_qsymbol(struct connection *connection, char const *packet, int packet_size); +bool rtos_needs_fake_step(struct target *target, int64_t thread_id); #endif /* OPENOCD_RTOS_RTOS_H */ diff --git a/src/server/gdb_server.c b/src/server/gdb_server.c index c9c8aaec8..56da6ed4c 100644 --- a/src/server/gdb_server.c +++ b/src/server/gdb_server.c @@ -2808,8 +2808,7 @@ static bool gdb_handle_vcont_packet(struct connection *connection, const char *p * check if the thread to be stepped is the current rtos thread * if not, we must fake the step */ - if (target->rtos->current_thread != thread_id) - fake_step = true; + fake_step = rtos_needs_fake_step(target, thread_id); } if (parse[0] == ';') { @@ -2849,15 +2848,10 @@ static bool gdb_handle_vcont_packet(struct connection *connection, const char *p log_add_callback(gdb_log_callback, connection); target_call_event_callbacks(ct, TARGET_EVENT_GDB_START); - /* - * work around an annoying gdb behaviour: when the current thread - * is changed in gdb, it assumes that the target can follow and also - * make the thread current. This is an assumption that cannot hold - * for a real target running a multi-threading OS. We just fake - * the step to not trigger an internal error in gdb. See - * https://sourceware.org/bugzilla/show_bug.cgi?id=22925 for details - */ if (fake_step) { + /* We just fake the step to not trigger an internal error in + * gdb. See https://sourceware.org/bugzilla/show_bug.cgi?id=22925 + * for details. */ int sig_reply_len; char sig_reply[128];