From 68b0f7bdff8a20f3280b47e28b5215a5ae2522b6 Mon Sep 17 00:00:00 2001 From: Tim Newsome Date: Wed, 14 Aug 2019 11:56:44 -0700 Subject: [PATCH] server: rtos: don't fake step for hwthread rtos. This is a cherry-pick of: Link: https://github.com/riscv-collab/riscv-openocd/commit/efce094b409179acbaa7726c112a10fcf8343503 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 Signed-off-by: Tim Newsome Reviewed-on: https://review.openocd.org/c/openocd/+/9177 Reviewed-by: Tomas Vanek Tested-by: jenkins --- src/rtos/hwthread.c | 7 +++++++ src/rtos/rtos.c | 7 +++++++ src/rtos/rtos.h | 8 ++++++++ src/server/gdb_server.c | 21 +++++++++++++++++---- 4 files changed, 39 insertions(+), 4 deletions(-) diff --git a/src/rtos/hwthread.c b/src/rtos/hwthread.c index 3f0041e0b..d4fc880f6 100644 --- a/src/rtos/hwthread.c +++ b/src/rtos/hwthread.c @@ -28,6 +28,7 @@ static int hwthread_read_buffer(struct rtos *rtos, target_addr_t address, uint32_t size, uint8_t *buffer); static int hwthread_write_buffer(struct rtos *rtos, target_addr_t address, uint32_t size, const uint8_t *buffer); +static bool hwthread_needs_fake_step(struct target *target, int64_t thread_id); #define HW_THREAD_NAME_STR_SIZE (32) @@ -59,6 +60,7 @@ const struct rtos_type hwthread_rtos = { .set_reg = hwthread_set_reg, .read_buffer = hwthread_read_buffer, .write_buffer = hwthread_write_buffer, + .needs_fake_step = hwthread_needs_fake_step }; struct hwthread_params { @@ -449,3 +451,8 @@ static int hwthread_write_buffer(struct rtos *rtos, target_addr_t address, return target_write_buffer(curr, address, size, buffer); } + +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 163d882a4..7957a5b6b 100644 --- a/src/rtos/rtos.c +++ b/src/rtos/rtos.c @@ -755,3 +755,10 @@ int rtos_write_buffer(struct target *target, target_addr_t address, return target->rtos->type->write_buffer(target->rtos, address, size, buffer); return ERROR_NOT_IMPLEMENTED; } + +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 ca6d7518d..bbd1d1a1d 100644 --- a/src/rtos/rtos.h +++ b/src/rtos/rtos.h @@ -79,6 +79,13 @@ struct rtos_type { uint8_t *buffer); int (*write_buffer)(struct rtos *rtos, target_addr_t address, uint32_t size, const uint8_t *buffer); + /** + * 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 { @@ -137,6 +144,7 @@ int rtos_read_buffer(struct target *target, target_addr_t address, uint32_t size, uint8_t *buffer); int rtos_write_buffer(struct target *target, target_addr_t address, uint32_t size, const uint8_t *buffer); +bool rtos_needs_fake_step(struct target *target, int64_t thread_id); // Keep in alphabetic order this list of rtos extern const struct rtos_type chibios_rtos; diff --git a/src/server/gdb_server.c b/src/server/gdb_server.c index d18d48a3a..486080bbd 100644 --- a/src/server/gdb_server.c +++ b/src/server/gdb_server.c @@ -3080,8 +3080,22 @@ static bool gdb_handle_vcont_packet(struct connection *connection, const char *p } if (target->rtos) { - /* FIXME: why is this necessary? rtos state should be up-to-date here already! */ - rtos_update_threads(target); + /* Sometimes this results in picking a different thread than + * gdb just requested to step. Then we fake it, and now there's + * a different thread selected than gdb expects, so register + * accesses go to the wrong one! + * E.g.: + * Hg1$ + * P8=72101ce197869329$ # write r8 on thread 1 + * g$ + * vCont?$ + * vCont;s:1;c$ # rtos_update_threads changes to other thread + * g$ + * qXfer:threads:read::0,fff$ + * P8=cc060607eb89ca7f$ # write r8 on other thread + * g$ + */ + /* rtos_update_threads(target); */ target->rtos->gdb_target_for_threadid(connection, thread_id, &ct); @@ -3089,8 +3103,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] == ';') {