From 081954136681b26ad30db9b4cc40cb360f47602c Mon Sep 17 00:00:00 2001 From: Tim Newsome Date: Mon, 8 Apr 2019 16:42:48 -0700 Subject: [PATCH] gdb_server, rtos: Fine-grained RTOS register access 1. Add get_thread_reg() to rtos. It's used in rtos_get_gdb_reg() to read the value of a single register, instead of reading all register values by calling get_thread_reg_list(). 2. Add set_reg() to rtos. gdb_server uses this to change a single register value for a specific thread. 3. Add target_get_gdb_reg_list_noread() so it's possible for gdb to get a list of registers without attempting to read their contents. The clang static checker doesn't find any new problems with this change. Change-Id: I77f792d1238cb015b91527ca8cb99593ccc8870e Signed-off-by: Tim Newsome Reviewed-on: http://openocd.zylin.com/5114 Tested-by: jenkins Reviewed-by: Matthias Welwarsky --- src/rtos/rtos.c | 46 ++++++++++++++++++----- src/rtos/rtos.h | 9 +++++ src/server/gdb_server.c | 81 +++++++++++++++++++++++----------------- src/target/register.c | 23 ++++++++++++ src/target/register.h | 2 + src/target/target.c | 24 ++++++++++-- src/target/target.h | 10 +++++ src/target/target_type.h | 7 ++++ 8 files changed, 155 insertions(+), 47 deletions(-) diff --git a/src/rtos/rtos.c b/src/rtos/rtos.c index 20e875d8b8..002d7b543c 100644 --- a/src/rtos/rtos.c +++ b/src/rtos/rtos.c @@ -462,6 +462,7 @@ static int rtos_put_gdb_reg_list(struct connection *connection, return ERROR_OK; } +/** Look through all registers to find this register. */ int rtos_get_gdb_reg(struct connection *connection, int reg_num) { struct target *target = get_target_from_connection(connection); @@ -473,19 +474,31 @@ int rtos_get_gdb_reg(struct connection *connection, int reg_num) struct rtos_reg *reg_list; int num_regs; - LOG_DEBUG("RTOS: getting register %d for thread 0x%" PRIx64 - ", target->rtos->current_thread=0x%" PRIx64 "\r\n", + LOG_DEBUG("getting register %d for thread 0x%" PRIx64 + ", target->rtos->current_thread=0x%" PRIx64, reg_num, current_threadid, target->rtos->current_thread); - int retval = target->rtos->type->get_thread_reg_list(target->rtos, - current_threadid, - ®_list, - &num_regs); - if (retval != ERROR_OK) { - LOG_ERROR("RTOS: failed to get register list"); - return retval; + int retval; + if (target->rtos->type->get_thread_reg) { + reg_list = calloc(1, sizeof(*reg_list)); + num_regs = 1; + retval = target->rtos->type->get_thread_reg(target->rtos, + current_threadid, reg_num, ®_list[0]); + if (retval != ERROR_OK) { + LOG_ERROR("RTOS: failed to get register %d", reg_num); + return retval; + } + } else { + retval = target->rtos->type->get_thread_reg_list(target->rtos, + current_threadid, + ®_list, + &num_regs); + if (retval != ERROR_OK) { + LOG_ERROR("RTOS: failed to get register list"); + return retval; + } } for (int i = 0; i < num_regs; ++i) { @@ -501,6 +514,7 @@ int rtos_get_gdb_reg(struct connection *connection, int reg_num) return ERROR_FAIL; } +/** Return a list of general registers. */ int rtos_get_gdb_reg_list(struct connection *connection) { struct target *target = get_target_from_connection(connection); @@ -534,6 +548,20 @@ int rtos_get_gdb_reg_list(struct connection *connection) return ERROR_FAIL; } +int rtos_set_reg(struct connection *connection, int reg_num, + uint8_t *reg_value) +{ + struct target *target = get_target_from_connection(connection); + int64_t current_threadid = target->rtos->current_threadid; + if ((target->rtos != NULL) && + (target->rtos->type->set_reg != NULL) && + (current_threadid != -1) && + (current_threadid != 0)) { + return target->rtos->type->set_reg(target->rtos, reg_num, reg_value); + } + return ERROR_FAIL; +} + int rtos_generic_stack_read(struct target *target, const struct rtos_register_stacking *stacking, int64_t stack_ptr, diff --git a/src/rtos/rtos.h b/src/rtos/rtos.h index 93b1731aad..a649e2449d 100644 --- a/src/rtos/rtos.h +++ b/src/rtos/rtos.h @@ -20,6 +20,7 @@ #define OPENOCD_RTOS_RTOS_H #include "server/server.h" +#include "target/target.h" #include typedef int64_t threadid_t; @@ -49,7 +50,9 @@ struct rtos { symbol_table_elem_t *symbols; struct target *target; /* add a context variable instead of global variable */ + /* The thread currently selected by gdb. */ int64_t current_threadid; + /* The currently selected thread according to the target. */ threadid_t current_thread; struct thread_detail *thread_details; int thread_count; @@ -70,11 +73,15 @@ struct rtos_type { int (*create)(struct target *target); int (*smp_init)(struct target *target); int (*update_threads)(struct rtos *rtos); + /** Return a list of general registers, with their values filled out. */ int (*get_thread_reg_list)(struct rtos *rtos, int64_t thread_id, struct rtos_reg **reg_list, int *num_regs); + int (*get_thread_reg)(struct rtos *rtos, int64_t thread_id, + uint32_t reg_num, struct rtos_reg *reg); int (*get_symbol_list_to_lookup)(symbol_table_elem_t *symbol_list[]); 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); }; struct stack_register_offset { @@ -104,6 +111,8 @@ struct rtos_register_stacking { #define GDB_THREAD_PACKET_NOT_CONSUMED (-40) int rtos_create(Jim_GetOptInfo *goi, struct target *target); +int rtos_set_reg(struct connection *connection, int reg_num, + uint8_t *reg_value); int rtos_generic_stack_read(struct target *target, const struct rtos_register_stacking *stacking, int64_t stack_ptr, diff --git a/src/server/gdb_server.c b/src/server/gdb_server.c index ab3e6bef8d..02020425e4 100644 --- a/src/server/gdb_server.c +++ b/src/server/gdb_server.c @@ -724,7 +724,7 @@ static int gdb_output(struct command_context *context, const char *line) static void gdb_signal_reply(struct target *target, struct connection *connection) { struct gdb_connection *gdb_connection = connection->priv; - char sig_reply[45]; + char sig_reply[65]; char stop_reason[20]; char current_thread[25]; int sig_reply_len; @@ -735,17 +735,25 @@ static void gdb_signal_reply(struct target *target, struct connection *connectio if (target->debug_reason == DBG_REASON_EXIT) { sig_reply_len = snprintf(sig_reply, sizeof(sig_reply), "W00"); } else { + struct target *ct; + if (target->rtos != NULL) { + target->rtos->current_threadid = target->rtos->current_thread; + target->rtos->gdb_target_for_threadid(connection, target->rtos->current_threadid, &ct); + } else { + ct = target; + } + if (gdb_connection->ctrl_c) { signal_var = 0x2; } else - signal_var = gdb_last_signal(target); + signal_var = gdb_last_signal(ct); stop_reason[0] = '\0'; - if (target->debug_reason == DBG_REASON_WATCHPOINT) { + if (ct->debug_reason == DBG_REASON_WATCHPOINT) { enum watchpoint_rw hit_wp_type; target_addr_t hit_wp_address; - if (watchpoint_hit(target, &hit_wp_type, &hit_wp_address) == ERROR_OK) { + if (watchpoint_hit(ct, &hit_wp_type, &hit_wp_address) == ERROR_OK) { switch (hit_wp_type) { case WPT_WRITE: @@ -767,15 +775,9 @@ static void gdb_signal_reply(struct target *target, struct connection *connectio } current_thread[0] = '\0'; - if (target->rtos != NULL) { - struct target *ct; - snprintf(current_thread, sizeof(current_thread), "thread:%016" PRIx64 ";", + if (target->rtos != NULL) + snprintf(current_thread, sizeof(current_thread), "thread:%" PRIx64 ";", target->rtos->current_thread); - target->rtos->current_threadid = target->rtos->current_thread; - target->rtos->gdb_target_for_threadid(connection, target->rtos->current_threadid, &ct); - if (!gdb_connection->ctrl_c) - signal_var = gdb_last_signal(ct); - } sig_reply_len = snprintf(sig_reply, sizeof(sig_reply), "T%2.2x%s%s", signal_var, stop_reason, current_thread); @@ -1300,7 +1302,7 @@ static int gdb_get_register_packet(struct connection *connection, if ((target->rtos != NULL) && (ERROR_OK == rtos_get_gdb_reg(connection, reg_num))) return ERROR_OK; - retval = target_get_gdb_reg_list(target, ®_list, ®_list_size, + retval = target_get_gdb_reg_list_noread(target, ®_list, ®_list_size, REG_CLASS_ALL); if (retval != ERROR_OK) return gdb_error(connection, retval); @@ -1336,37 +1338,49 @@ static int gdb_set_register_packet(struct connection *connection, { struct target *target = get_target_from_connection(connection); char *separator; - uint8_t *bin_buf; int reg_num = strtoul(packet + 1, &separator, 16); struct reg **reg_list; int reg_list_size; int retval; +#ifdef _DEBUG_GDB_IO_ LOG_DEBUG("-"); +#endif - retval = target_get_gdb_reg_list(target, ®_list, ®_list_size, + if (*separator != '=') { + LOG_ERROR("GDB 'set register packet', but no '=' following the register number"); + return ERROR_SERVER_REMOTE_CLOSED; + } + size_t chars = strlen(separator + 1); + uint8_t *bin_buf = malloc(chars / 2); + gdb_target_to_reg(target, separator + 1, chars, bin_buf); + + if ((target->rtos != NULL) && + (ERROR_OK == rtos_set_reg(connection, reg_num, bin_buf))) { + free(bin_buf); + gdb_put_packet(connection, "OK", 2); + return ERROR_OK; + } + + retval = target_get_gdb_reg_list_noread(target, ®_list, ®_list_size, REG_CLASS_ALL); - if (retval != ERROR_OK) + if (retval != ERROR_OK) { + free(bin_buf); return gdb_error(connection, retval); + } if (reg_list_size <= reg_num) { LOG_ERROR("gdb requested a non-existing register"); + free(bin_buf); + free(reg_list); return ERROR_SERVER_REMOTE_CLOSED; } - if (*separator != '=') { - LOG_ERROR("GDB 'set register packet', but no '=' following the register number"); - return ERROR_SERVER_REMOTE_CLOSED; - } - - /* convert from GDB-string (target-endian) to hex-string (big-endian) */ - bin_buf = malloc(DIV_ROUND_UP(reg_list[reg_num]->size, 8)); - int chars = (DIV_ROUND_UP(reg_list[reg_num]->size, 8) * 2); - - if ((unsigned int)chars != strlen(separator + 1)) { - LOG_ERROR("gdb sent %zu bits for a %d-bit register (%s)", - strlen(separator + 1) * 4, chars * 4, reg_list[reg_num]->name); + if (chars != (DIV_ROUND_UP(reg_list[reg_num]->size, 8) * 2)) { + LOG_ERROR("gdb sent %d bits for a %d-bit register (%s)", + (int) chars * 4, reg_list[reg_num]->size, reg_list[reg_num]->name); free(bin_buf); + free(reg_list); return ERROR_SERVER_REMOTE_CLOSED; } @@ -1634,7 +1648,7 @@ static int gdb_breakpoint_watchpoint_packet(struct connection *connection, char *separator; int retval; - LOG_DEBUG("-"); + LOG_DEBUG("[%s]", target_name(target)); type = strtoul(packet + 1, &separator, 16); @@ -2200,7 +2214,7 @@ static int gdb_generate_target_description(struct target *target, char **tdesc_o arch_defined_types = calloc(1, sizeof(char *)); - retval = target_get_gdb_reg_list(target, ®_list, + retval = target_get_gdb_reg_list_noread(target, ®_list, ®_list_size, REG_CLASS_ALL); if (retval != ERROR_OK) { @@ -2388,7 +2402,7 @@ static int gdb_target_description_supported(struct target *target, int *supporte char const *architecture = target_get_gdb_arch(target); - retval = target_get_gdb_reg_list(target, ®_list, + retval = target_get_gdb_reg_list_noread(target, ®_list, ®_list_size, REG_CLASS_ALL); if (retval != ERROR_OK) { LOG_ERROR("get register list failed"); @@ -2783,13 +2797,11 @@ static bool gdb_handle_vcont_packet(struct connection *connection, const char *p if (parse[0] == 'c') { parse += 1; - packet_size -= 1; /* check if thread-id follows */ if (parse[0] == ':') { int64_t tid; parse += 1; - packet_size -= 1; tid = strtoll(parse, &endp, 16); if (tid == thread_id) { @@ -2879,10 +2891,9 @@ static int gdb_v_packet(struct connection *connection, char const *packet, int packet_size) { struct gdb_connection *gdb_connection = connection->priv; - struct target *target; int result; - target = get_target_from_connection(connection); + struct target *target = get_target_from_connection(connection); if (strncmp(packet, "vCont", 5) == 0) { bool handled; diff --git a/src/target/register.c b/src/target/register.c index 5352d2f21d..4ddda6e6b3 100644 --- a/src/target/register.c +++ b/src/target/register.c @@ -36,6 +36,29 @@ * may be separate registers associated with debug or trace modules. */ +struct reg *register_get_by_number(struct reg_cache *first, + uint32_t reg_num, bool search_all) +{ + unsigned i; + struct reg_cache *cache = first; + + while (cache) { + for (i = 0; i < cache->num_regs; i++) { + if (cache->reg_list[i].exist == false) + continue; + if (cache->reg_list[i].number == reg_num) + return &(cache->reg_list[i]); + } + + if (search_all) + cache = cache->next; + else + break; + } + + return NULL; +} + struct reg *register_get_by_name(struct reg_cache *first, const char *name, bool search_all) { diff --git a/src/target/register.h b/src/target/register.h index 32c1f39ace..7c53d6e16d 100644 --- a/src/target/register.h +++ b/src/target/register.h @@ -159,6 +159,8 @@ struct reg_arch_type { int (*set)(struct reg *reg, uint8_t *buf); }; +struct reg *register_get_by_number(struct reg_cache *first, + uint32_t reg_num, bool search_all); struct reg *register_get_by_name(struct reg_cache *first, const char *name, bool search_all); struct reg_cache **register_get_last_cache_p(struct reg_cache **first); diff --git a/src/target/target.c b/src/target/target.c index e6c434362a..51fdff3425 100644 --- a/src/target/target.c +++ b/src/target/target.c @@ -1215,7 +1215,24 @@ int target_get_gdb_reg_list(struct target *target, struct reg **reg_list[], int *reg_list_size, enum target_register_class reg_class) { - return target->type->get_gdb_reg_list(target, reg_list, reg_list_size, reg_class); + int result = target->type->get_gdb_reg_list(target, reg_list, + reg_list_size, reg_class); + if (result != ERROR_OK) { + *reg_list = NULL; + *reg_list_size = 0; + } + return result; +} + +int target_get_gdb_reg_list_noread(struct target *target, + struct reg **reg_list[], int *reg_list_size, + enum target_register_class reg_class) +{ + if (target->type->get_gdb_reg_list_noread && + target->type->get_gdb_reg_list_noread(target, reg_list, + reg_list_size, reg_class) == ERROR_OK) + return ERROR_OK; + return target_get_gdb_reg_list(target, reg_list, reg_list_size, reg_class); } bool target_supports_gdb_connection(struct target *target) @@ -1587,8 +1604,9 @@ int target_call_event_callbacks(struct target *target, enum target_event event) target_call_event_callbacks(target, TARGET_EVENT_GDB_HALT); } - LOG_DEBUG("target event %i (%s)", event, - Jim_Nvp_value2name_simple(nvp_target_event, event)->name); + LOG_DEBUG("target event %i (%s) for core %s", event, + Jim_Nvp_value2name_simple(nvp_target_event, event)->name, + target_name(target)); target_handle_event(target, event); diff --git a/src/target/target.h b/src/target/target.h index e944838ace..81fd9d23dc 100644 --- a/src/target/target.h +++ b/src/target/target.h @@ -502,6 +502,16 @@ int target_get_gdb_reg_list(struct target *target, struct reg **reg_list[], int *reg_list_size, enum target_register_class reg_class); +/** + * Obtain the registers for GDB, but don't read register values from the + * target. + * + * This routine is a wrapper for target->type->get_gdb_reg_list_noread. + */ +int target_get_gdb_reg_list_noread(struct target *target, + struct reg **reg_list[], int *reg_list_size, + enum target_register_class reg_class); + /** * Check if @a target allows GDB connections. * diff --git a/src/target/target_type.h b/src/target/target_type.h index 95745c9ebd..4bdea721e8 100644 --- a/src/target/target_type.h +++ b/src/target/target_type.h @@ -111,6 +111,13 @@ struct target_type { int (*get_gdb_reg_list)(struct target *target, struct reg **reg_list[], int *reg_list_size, enum target_register_class reg_class); + /** + * Same as get_gdb_reg_list, but doesn't read the register values. + * */ + int (*get_gdb_reg_list_noread)(struct target *target, + struct reg **reg_list[], int *reg_list_size, + enum target_register_class reg_class); + /* target memory access * size: 1 = byte (8bit), 2 = half-word (16bit), 4 = word (32bit) * count: number of items of -- 2.30.2