From 6d95014674415e3b9ea9d46d5148d3410f96bbfd Mon Sep 17 00:00:00 2001 From: oharboe Date: Wed, 5 Mar 2008 10:28:32 +0000 Subject: [PATCH] * fixed malloc corruption in target->debug_reason * GDB remote server will now remain online even if the target is in a funny state, e.g. if it requires a reset, it is running while GDB is not in the continue or step packet, e.g. via monitor resume/halt commands in GDB script. * Added some _DEBUG_GDB_IO_ debug tools * Fixed a couple of GDB server lockups, e.g. when O packets detect a severed connection * added ACK upon connection (send +). * added keep-alive messages to reset so GDB protocol remains happy. * fixed crash when timing out connection to GDB git-svn-id: svn://svn.berlios.de/openocd/trunk@445 b42882b7-edfa-0310-969c-e2dbd0fdcd60 --- src/helper/command.c | 2 - src/server/gdb_server.c | 231 +++++++++++++++++++++------------------- src/server/server.c | 1 + src/target/target.c | 9 +- src/target/target.h | 3 +- 5 files changed, 130 insertions(+), 116 deletions(-) diff --git a/src/helper/command.c b/src/helper/command.c index aa71f0ba..5df12901 100644 --- a/src/helper/command.c +++ b/src/helper/command.c @@ -447,7 +447,6 @@ void command_print_help_line(command_context_t* context, struct command_s *comma char indent_text[indent + 2]; char *help = "no help available"; char name_buf[64]; - int i; if (indent) { @@ -478,7 +477,6 @@ void command_print_help_line(command_context_t* context, struct command_s *comma int command_print_help_match(command_context_t* context, command_t* c_first, char* name, char** args, int argc) { command_t * c; - int i; for (c = c_first; c; c = c->next) { diff --git a/src/server/gdb_server.c b/src/server/gdb_server.c index a8b60231..c7200d20 100644 --- a/src/server/gdb_server.c +++ b/src/server/gdb_server.c @@ -43,6 +43,7 @@ #define _DEBUG_GDB_IO_ #endif +extern int gdb_error(connection_t *connection, int retval); static unsigned short gdb_port; static const char *DIGITS = "0123456789abcdef"; @@ -84,14 +85,58 @@ int gdb_last_signal(target_t *target) case DBG_REASON_NOTHALTED: return 0x0; /* no signal... shouldn't happen */ default: - ERROR("BUG: undefined debug reason"); - exit(-1); + USER("undefined debug reason %d - target needs reset", target->debug_reason); + return 0x0; + } +} + +#ifndef _WIN32 +int check_pending(connection_t *connection, int timeout_s, int *got_data) +{ + /* a non-blocking socket will block if there is 0 bytes available on the socket, + * but return with as many bytes as are available immediately + */ + struct timeval tv; + fd_set read_fds; + gdb_connection_t *gdb_con = connection->priv; + int t; + if (got_data==NULL) + got_data=&t; + *got_data=0; + + if (gdb_con->buf_cnt>0) + { + *got_data = 1; + return ERROR_OK; + } + + FD_ZERO(&read_fds); + FD_SET(connection->fd, &read_fds); + + tv.tv_sec = timeout_s; + tv.tv_usec = 0; + if (select(connection->fd + 1, &read_fds, NULL, NULL, &tv) == 0) + { + /* This can typically be because a "monitor" command took too long + * before printing any progress messages + */ + if (timeout_s>0) + { + return ERROR_GDB_TIMEOUT; + } else + { + return ERROR_OK; + } } + *got_data=FD_ISSET(connection->fd, &read_fds)!=0; + return ERROR_OK; } +#endif int gdb_get_char(connection_t *connection, int* next_char) { gdb_connection_t *gdb_con = connection->priv; + int retval=ERROR_OK; #ifdef _DEBUG_GDB_IO_ char *debug_buffer; @@ -115,24 +160,9 @@ int gdb_get_char(connection_t *connection, int* next_char) for (;;) { #ifndef _WIN32 - /* a non-blocking socket will block if there is 0 bytes available on the socket, - * but return with as many bytes as are available immediately - */ - struct timeval tv; - fd_set read_fds; - - FD_ZERO(&read_fds); - FD_SET(connection->fd, &read_fds); - - tv.tv_sec = 1; - tv.tv_usec = 0; - if (select(connection->fd + 1, &read_fds, NULL, NULL, &tv) == 0) - { - /* This can typically be because a "monitor" command took too long - * before printing any progress messages - */ - return ERROR_GDB_TIMEOUT; - } + retval=check_pending(connection, 1, NULL); + if (retval!=ERROR_OK) + return retval; #endif gdb_con->buf_cnt = read_socket(connection->fd, gdb_con->buffer, GDB_BUFFER_SIZE); if (gdb_con->buf_cnt > 0) @@ -154,8 +184,10 @@ int gdb_get_char(connection_t *connection, int* next_char) usleep(1000); break; case WSAECONNABORTED: + gdb_con->closed = 1; return ERROR_SERVER_REMOTE_CLOSED; case WSAECONNRESET: + gdb_con->closed = 1; return ERROR_SERVER_REMOTE_CLOSED; default: ERROR("read: %d", errno); @@ -168,11 +200,14 @@ int gdb_get_char(connection_t *connection, int* next_char) usleep(1000); break; case ECONNABORTED: + gdb_con->closed = 1; return ERROR_SERVER_REMOTE_CLOSED; case ECONNRESET: + gdb_con->closed = 1; return ERROR_SERVER_REMOTE_CLOSED; default: ERROR("read: %s", strerror(errno)); + gdb_con->closed = 1; return ERROR_SERVER_REMOTE_CLOSED; } #endif @@ -197,7 +232,7 @@ int gdb_get_char(connection_t *connection, int* next_char) DEBUG("returned char '%c' (0x%2.2x)", *next_char, *next_char); #endif - return ERROR_OK; + return retval; } int gdb_putback_char(connection_t *connection, int last_char) @@ -248,6 +283,27 @@ int gdb_put_packet_inner(connection_t *connection, char *buffer, int len) for (i = 0; i < len; i++) my_checksum += buffer[i]; +#ifdef _DEBUG_GDB_IO_ + /* + * At this point we should have nothing in the input queue from GDB, + * however sometimes '-' is sent even though we've already received + * an ACK (+) for everything we've sent off. + */ +#ifndef _WIN32 + int gotdata; + for (;;) + { + if ((retval=check_pending(connection, 0, &gotdata))!=ERROR_OK) + return retval; + if (!gotdata) + break; + if ((retval = gdb_get_char(connection, &reply)) != ERROR_OK) + return retval; + WARNING("Discard unexpected char %c", reply); + } +#endif +#endif + while (1) { #ifdef _DEBUG_GDB_IO_ @@ -257,17 +313,7 @@ int gdb_put_packet_inner(connection_t *connection, char *buffer, int len) DEBUG("sending packet '$%s#%2.2x'", debug_buffer, my_checksum); free(debug_buffer); #endif -#if 0 - char checksum[3]; - gdb_write(connection, "$", 1); - if (len > 0) - gdb_write(connection, buffer, len); - gdb_write(connection, "#", 1); - - snprintf(checksum, 3, "%2.2x", my_checksum); - gdb_write(connection, checksum, 2); -#else void *allocated = NULL; char stackAlloc[1024]; char *t = stackAlloc; @@ -294,7 +340,7 @@ int gdb_put_packet_inner(connection_t *connection, char *buffer, int len) { free(allocated); } -#endif + if ((retval = gdb_get_char(connection, &reply)) != ERROR_OK) return retval; @@ -322,12 +368,14 @@ int gdb_put_packet_inner(connection_t *connection, char *buffer, int len) else { ERROR("unknown character 0x%2.2x in reply, dropping connection", reply); + gdb_con->closed=1; return ERROR_SERVER_REMOTE_CLOSED; } } else { ERROR("unknown character 0x%2.2x in reply, dropping connection", reply); + gdb_con->closed=1; return ERROR_SERVER_REMOTE_CLOSED; } } @@ -624,6 +672,9 @@ int gdb_new_connection(connection_t *connection) gdb_connection->vflash_image = NULL; gdb_connection->closed = 0; gdb_connection->busy = 0; + + /* send ACK to GDB for debug request */ + gdb_write(connection, "+", 1); /* output goes through gdb connection */ command_set_output_handler(connection->cmd_ctx, gdb_output, connection); @@ -631,21 +682,27 @@ int gdb_new_connection(connection_t *connection) /* register callback to be informed about target events */ target_register_event_callback(gdb_target_callback_event_handler, connection); - /* a gdb session just attached, put the target in halt mode */ + /* a gdb session just attached, try to put the target in halt mode + * or alterantively try to issue a reset. + * + * GDB connection will fail if e.g. register read packets fail, + * otherwise resetting/halting the target could have been left to GDB init + * scripts + */ if (((retval = gdb_service->target->type->halt(gdb_service->target)) != ERROR_OK) && (retval != ERROR_TARGET_ALREADY_HALTED)) { - ERROR("error(%d) when trying to halt target, falling back to \"reset halt\"", retval); - command_run_line(connection->cmd_ctx, "reset halt"); + ERROR("error(%d) when trying to halt target, falling back to \"reset\"", retval); + command_run_line(connection->cmd_ctx, "reset"); } - - /* This will time out after 1 second */ - command_run_line(connection->cmd_ctx, "wait_halt 1"); - + /* remove the initial ACK from the incoming buffer */ if ((retval = gdb_get_char(connection, &initial_ack)) != ERROR_OK) return retval; + /* FIX!!!??? would we actually ever receive a + here??? + * Not observed. + */ if (initial_ack != '+') gdb_putback_char(connection, initial_ack); @@ -778,16 +835,7 @@ int gdb_get_registers_packet(connection_t *connection, target_t *target, char* p if ((retval = target->type->get_gdb_reg_list(target, ®_list, ®_list_size)) != ERROR_OK) { - switch (retval) - { - case ERROR_TARGET_NOT_HALTED: - ERROR("gdb requested registers but we're not halted, dropping connection"); - return ERROR_SERVER_REMOTE_CLOSED; - default: - /* this is a bug condition - get_gdb_reg_list() may not return any other error */ - ERROR("BUG: unexpected error returned by get_gdb_reg_list()"); - exit(-1); - } + return gdb_error(connection, retval); } for (i = 0; i < reg_list_size; i++) @@ -845,16 +893,7 @@ int gdb_set_registers_packet(connection_t *connection, target_t *target, char *p if ((retval = target->type->get_gdb_reg_list(target, ®_list, ®_list_size)) != ERROR_OK) { - switch (retval) - { - case ERROR_TARGET_NOT_HALTED: - ERROR("gdb tried to registers but we're not halted, dropping connection"); - return ERROR_SERVER_REMOTE_CLOSED; - default: - /* this is a bug condition - get_gdb_reg_list() may not return any other error */ - ERROR("BUG: unexpected error returned by get_gdb_reg_list()"); - exit(-1); - } + return gdb_error(connection, retval); } packet_p = packet; @@ -910,16 +949,7 @@ int gdb_get_register_packet(connection_t *connection, target_t *target, char *pa if ((retval = target->type->get_gdb_reg_list(target, ®_list, ®_list_size)) != ERROR_OK) { - switch (retval) - { - case ERROR_TARGET_NOT_HALTED: - ERROR("gdb requested registers but we're not halted, dropping connection"); - return ERROR_SERVER_REMOTE_CLOSED; - default: - /* this is a bug condition - get_gdb_reg_list() may not return any other error */ - ERROR("BUG: unexpected error returned by get_gdb_reg_list()"); - exit(-1); - } + return gdb_error(connection, retval); } if (reg_list_size <= reg_num) @@ -955,22 +985,13 @@ int gdb_set_register_packet(connection_t *connection, target_t *target, char *pa if ((retval = target->type->get_gdb_reg_list(target, ®_list, ®_list_size)) != ERROR_OK) { - switch (retval) - { - case ERROR_TARGET_NOT_HALTED: - ERROR("gdb tried to set a register but we're not halted, dropping connection"); - return ERROR_SERVER_REMOTE_CLOSED; - default: - /* this is a bug condition - get_gdb_reg_list() may not return any other error */ - ERROR("BUG: unexpected error returned by get_gdb_reg_list()"); - exit(-1); - } + return gdb_error(connection, retval); } if (reg_list_size < reg_num) { ERROR("gdb requested a non-existing register"); - return ERROR_SERVER_REMOTE_CLOSED; + return ERROR_SERVER_REMOTE_CLOSED; } if (*separator != '=') @@ -1005,13 +1026,10 @@ int gdb_set_register_packet(connection_t *connection, target_t *target, char *pa return ERROR_OK; } -int gdb_memory_packet_error(connection_t *connection, int retval) +int gdb_error(connection_t *connection, int retval) { switch (retval) { - case ERROR_TARGET_NOT_HALTED: - ERROR("gdb tried to read memory but we're not halted, dropping connection"); - return ERROR_SERVER_REMOTE_CLOSED; case ERROR_TARGET_DATA_ABORT: gdb_send_error(connection, EIO); break; @@ -1021,10 +1039,14 @@ int gdb_memory_packet_error(connection_t *connection, int retval) case ERROR_TARGET_UNALIGNED_ACCESS: gdb_send_error(connection, EFAULT); break; + case ERROR_TARGET_NOT_HALTED: + gdb_send_error(connection, EFAULT); + break; default: /* This could be that the target reset itself. */ - ERROR("unexpected error %i. Dropping connection.", retval); - return ERROR_SERVER_REMOTE_CLOSED; + ERROR("unexpected error %i", retval); + gdb_send_error(connection, EFAULT); + break; } return ERROR_OK; @@ -1101,7 +1123,7 @@ int gdb_read_memory_packet(connection_t *connection, target_t *target, char *pac } else { - retval = gdb_memory_packet_error(connection, retval); + retval = gdb_error(connection, retval); } free(buffer); @@ -1158,7 +1180,7 @@ int gdb_write_memory_packet(connection_t *connection, target_t *target, char *pa } else { - if ((retval = gdb_memory_packet_error(connection, retval)) != ERROR_OK) + if ((retval = gdb_error(connection, retval)) != ERROR_OK) return retval; } @@ -1208,7 +1230,7 @@ int gdb_write_memory_binary_packet(connection_t *connection, target_t *target, c } else { - if ((retval = gdb_memory_packet_error(connection, retval)) != ERROR_OK) + if ((retval = gdb_error(connection, retval)) != ERROR_OK) return retval; } @@ -1244,25 +1266,6 @@ void gdb_step_continue_packet(connection_t *connection, target_t *target, char * } } -int gdb_bp_wp_packet_error(connection_t *connection, int retval) -{ - switch (retval) - { - case ERROR_TARGET_NOT_HALTED: - ERROR("gdb tried to set a breakpoint but we're not halted, dropping connection"); - return ERROR_SERVER_REMOTE_CLOSED; - break; - case ERROR_TARGET_RESOURCE_NOT_AVAILABLE: - gdb_send_error(connection, EBUSY); - break; - default: - ERROR("BUG: unexpected error %i", retval); - exit(-1); - } - - return ERROR_OK; -} - int gdb_breakpoint_watchpoint_packet(connection_t *connection, target_t *target, char *packet, int packet_size) { int type; @@ -1312,7 +1315,7 @@ int gdb_breakpoint_watchpoint_packet(connection_t *connection, target_t *target, { if ((retval = breakpoint_add(target, address, size, bp_type)) != ERROR_OK) { - if ((retval = gdb_bp_wp_packet_error(connection, retval)) != ERROR_OK) + if ((retval = gdb_error(connection, retval)) != ERROR_OK) return retval; } else @@ -1334,7 +1337,7 @@ int gdb_breakpoint_watchpoint_packet(connection_t *connection, target_t *target, { if ((retval = watchpoint_add(target, address, size, type-2, 0, 0xffffffffu)) != ERROR_OK) { - if ((retval = gdb_bp_wp_packet_error(connection, retval)) != ERROR_OK) + if ((retval = gdb_error(connection, retval)) != ERROR_OK) return retval; } else @@ -1502,7 +1505,7 @@ int gdb_query_packet(connection_t *connection, target_t *target, char *packet, i } else { - if ((retval = gdb_memory_packet_error(connection, retval)) != ERROR_OK) + if ((retval = gdb_error(connection, retval)) != ERROR_OK) return retval; } @@ -1939,8 +1942,14 @@ int gdb_input_inner(connection_t *connection) int gdb_input(connection_t *connection) { int retval = gdb_input_inner(connection); + gdb_connection_t *gdb_con = connection->priv; if (retval == ERROR_SERVER_REMOTE_CLOSED) return retval; + + /* logging does not propagate the error, yet can set th gdb_con->closed flag */ + if (gdb_con->closed) + return ERROR_SERVER_REMOTE_CLOSED; + /* we'll recover from any other errors(e.g. temporary timeouts, etc.) */ return ERROR_OK; } diff --git a/src/server/server.c b/src/server/server.c index 4e8ddc69..ae13b129 100644 --- a/src/server/server.c +++ b/src/server/server.c @@ -71,6 +71,7 @@ int add_connection(service_t *service, command_context_t *cmd_ctx) close_socket(c->fd); INFO("attempted '%s' connection rejected", service->name); free(c); + return retval; } /* add to the end of linked list */ diff --git a/src/target/target.c b/src/target/target.c index 76f7449b..06abb37c 100644 --- a/src/target/target.c +++ b/src/target/target.c @@ -122,7 +122,7 @@ char *target_debug_reason_strings[] = { "debug request", "breakpoint", "watchpoint", "watchpoint and breakpoint", "single step", - "target not halted" + "target not halted", "undefined" }; char *target_endianess_strings[] = @@ -362,7 +362,11 @@ int target_process_reset(struct command_context_s *cmd_ctx) command_print(cmd_ctx, "Timed out waiting for reset"); goto done; } - usleep(100*1000); /* Do not eat all cpu */ + /* this will send alive messages on e.g. GDB remote protocol. + * GDB warns me that I'm sending a zero length formatting message, + * which is strange, but in fact what is intended here. */ + usleep(500*1000); + USER_N(""); goto again; } } @@ -1178,6 +1182,7 @@ int handle_target_command(struct command_context_s *cmd_ctx, char *cmd, char **a (*last_target_p)->backup_working_area = 0; (*last_target_p)->state = TARGET_UNKNOWN; + (*last_target_p)->debug_reason = DBG_REASON_UNDEFINED; (*last_target_p)->reg_cache = NULL; (*last_target_p)->breakpoints = NULL; (*last_target_p)->watchpoints = NULL; diff --git a/src/target/target.h b/src/target/target.h index e6d6f294..dae5f19e 100644 --- a/src/target/target.h +++ b/src/target/target.h @@ -78,7 +78,8 @@ enum target_debug_reason DBG_REASON_WATCHPOINT = 2, DBG_REASON_WPTANDBKPT = 3, DBG_REASON_SINGLESTEP = 4, - DBG_REASON_NOTHALTED = 5 + DBG_REASON_NOTHALTED = 5, + DBG_REASON_UNDEFINED = 6 }; extern char *target_debug_reason_strings[]; -- 2.39.5