* 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

@@ -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)
 	{

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, &reg_list, &reg_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, &reg_list, &reg_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, &reg_list, &reg_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, &reg_list, &reg_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;
 }

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 */

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;

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[];