Feature/gdb packet optimization

src/hex_utils.c

@@ -52,8 +52,7 @@ char *hexify(char *const dst, const void *const buf, const size_t size)
 		dst[dst_idx++] = hex_digit(src[src_idx] & 0xfU);
 	}
 
-	/* Make sure the result is NUL terminated */
-	dst[dst_idx] = '\0';
+	/* The hexifued string is *NOT* NUL terminated */
 	return dst;
 }
 

src/include/gdb_if.h

@@ -31,6 +31,7 @@ char gdb_if_getchar(void);
 char gdb_if_getchar_to(uint32_t timeout);
 
 /* sending gdb_if_putchar(0, true) seems to work as keep alive */
-void gdb_if_putchar(char c, int flush);
+void gdb_if_putchar(char c, bool flush);
+void gdb_if_flush(bool force);
 
 #endif /* INCLUDE_GDB_IF_H */

src/include/gdb_main.h

@@ -22,18 +22,13 @@
 #define INCLUDE_GDB_MAIN_H
 
 #include "target.h"
-
-/* Allow override in other platforms if needed */
-#ifndef GDB_PACKET_BUFFER_SIZE
-#define GDB_PACKET_BUFFER_SIZE 1024U
-#endif
+#include "gdb_packet.h"
 
 extern bool gdb_target_running;
 extern target_s *cur_target;
 
 void gdb_poll_target(void);
-void gdb_main(char *pbuf, size_t pbuf_size, size_t size);
-int32_t gdb_main_loop(target_controller_s *tc, char *pbuf, size_t pbuf_size, size_t size, bool in_syscall);
-char *gdb_packet_buffer(void);
+void gdb_main(const gdb_packet_s *packet);
+int32_t gdb_main_loop(target_controller_s *tc, const gdb_packet_s *packet, bool in_syscall);
 
 #endif /* INCLUDE_GDB_MAIN_H */

src/include/gdb_packet.h

@@ -25,6 +25,14 @@
 #include <stdarg.h>
 #include <stdbool.h>
 
+/* Allow override in other platforms if needed */
+#ifndef GDB_PACKET_BUFFER_SIZE
+#define GDB_PACKET_BUFFER_SIZE 1024U
+#endif
+
+/* Limit out packet string size to the maximum packet size before hexifying */
+#define GDB_OUT_PACKET_MAX_SIZE ((GDB_PACKET_BUFFER_SIZE - 1U) / 2U)
+
 #define GDB_PACKET_START              '$'
 #define GDB_PACKET_END                '#'
 #define GDB_PACKET_ACK                '+'
@@ -34,6 +42,8 @@
 #define GDB_PACKET_NOTIFICATION_START '%'
 #define GDB_PACKET_ESCAPE_XOR         (0x20U)
 
+#define GDB_PACKET_RETRIES 3U /* Number of times to retry sending a packet */
+
 #if defined(__MINGW32__) || defined(__MINGW64__) || defined(__CYGWIN__)
 #define GDB_FORMAT_ATTR __attribute__((format(__MINGW_PRINTF_FORMAT, 1, 2)))
 #elif defined(__GNUC__) || defined(__clang__)
@@ -42,17 +52,99 @@
 #define GDB_FORMAT_ATTR
 #endif
 
+/*
+ * GDB packet structure
+ * This is used to store the packet data during transmission and reception
+ * This will be statically allocated and aligned to 8 bytes to allow the remote protocol to re-use it
+ * A single packet instance exists in the system and is re-used for all packet operations
+ * This means transmiting a packet will invalidate any previously obtained packets
+ * Do not use this structure directly or you might risk runing out of memory
+ */
+typedef struct gdb_packet {
+	/* Data must be first to ensure alignment */
+	char data[GDB_PACKET_BUFFER_SIZE + 1U]; /* Packet data */
+	size_t size;                            /* Packet data size */
+	bool notification;                      /* Notification packet */
+} gdb_packet_s;
+
+/* GDB packet transmission configuration */
 void gdb_set_noackmode(bool enable);
-size_t gdb_getpacket(char *packet, size_t size);
-void gdb_putpacket(const char *packet, size_t size);
-void gdb_putpacket2(const char *packet1, size_t size1, const char *packet2, size_t size2);
-#define gdb_putpacketz(packet) gdb_putpacket((packet), strlen(packet))
-void gdb_putpacket_f(const char *packet, ...) GDB_FORMAT_ATTR;
-void gdb_put_notification(const char *packet, size_t size);
-#define gdb_put_notificationz(packet) gdb_put_notification((packet), strlen(packet))
-
-void gdb_out(const char *buf);
-void gdb_voutf(const char *fmt, va_list);
+
+/* Raw GDB packet transmission */
+gdb_packet_s *gdb_packet_receive(void);
+void gdb_packet_send(const gdb_packet_s *packet);
+
+char *gdb_packet_buffer(void);
+
+/* Convenience wrappers */
+void gdb_put_packet(const char *preamble, size_t preamble_size, const char *data, size_t data_size, bool hex_data);
+
+static inline void gdb_put_packet_empty(void)
+{
+	/**
+	 * Empty response packet
+	 * See https://sourceware.org/gdb/current/onlinedocs/gdb.html/Standard-Replies.html#Standard-Replies
+	 * 
+	 * An empty response (raw character sequence ‘$#00’) means the command is not supported by the stub.
+	 */
+	gdb_put_packet(NULL, 0, NULL, 0, false);
+}
+
+static inline void gdb_put_packet_str(const char *const str)
+{
+	gdb_put_packet(str, strlen(str), NULL, 0, false);
+}
+
+static inline void gdb_put_packet_hex(const void *const data, const size_t size)
+{
+	gdb_put_packet(NULL, 0, (const char *)data, size, true);
+}
+
+static inline void gdb_put_packet_ok(void)
+{
+	/**
+	 * OK response packet
+	 * 
+	 * This is a common response to acknowledge a command was successful.
+	 */
+	gdb_put_packet_str("OK");
+}
+
+static inline void gdb_put_packet_error(const uint8_t error)
+{
+	/*
+	 * Error response packet
+	 * See https://sourceware.org/gdb/current/onlinedocs/gdb.html/Standard-Replies.html#Standard-Replies
+	 *  
+	 * Format: ‘E xx’
+	 * xx is a two-digit hexadecimal error number. 
+	 * In almost all cases, the protocol does not specify the meaning of the error numbers
+	 * GDB usually ignores the numbers, or displays them to the user without further interpretation.
+	 * 
+	 * Textual error messages send the error text instead of the error number, but this response
+	 * is not guaranteed to be understood by GDB for all requests, the GDB feature error-message
+	 * lets us know if it is supported.
+	 * 
+	 * TODO: implement the error-message GDB feature, so we can send textual error messages.
+	 * 
+	 * Format: ‘E.errtext’
+     * errtext is the textual error message, encoded in ASCII.
+	 */
+	gdb_put_packet("E", 1U, (const char *)&error, 1U, true);
+}
+
+void gdb_put_notification_str(const char *const str);
+
+/* Formatted output */
+void gdb_putpacket_str_f(const char *fmt, ...) GDB_FORMAT_ATTR;
+
+/**
+ * Warning: gdb_(v)out(f) functions may truncate the output string if it is too long
+ * The output string is limited by the constant GDB_OUT_PACKET_MAX_SIZE derived from
+ * GDB_PACKET_BUFFER_SIZE. By default this is 511 characters.
+ */
+void gdb_out(const char *str);
+void gdb_voutf(const char *fmt, va_list ap);
 void gdb_outf(const char *fmt, ...) GDB_FORMAT_ATTR;
 
 #endif /* INCLUDE_GDB_PACKET_H */

src/include/general.h

@@ -166,22 +166,6 @@ void debug_serial_send_stdout(const uint8_t *data, size_t len);
 #ifdef _MSC_VER
 #define strcasecmp  _stricmp
 #define strncasecmp _strnicmp
-
-// FIXME: BMDA still uses this function in gdb_packet.c
-// It's defined here as an export from utils.c would pollute the ABI of libbmd
-static inline int vasprintf(char **strp, const char *const fmt, va_list ap)
-{
-	const int actual_size = vsnprintf(NULL, 0, fmt, ap);
-	if (actual_size < 0)
-		return -1;
-
-	*strp = malloc(actual_size + 1);
-	if (!*strp)
-		return -1;
-
-	return vsnprintf(*strp, actual_size + 1, fmt, ap);
-}
-
 #endif /* _MSC_VER */
 
 #ifndef PLATFORM_IDENT_DYNAMIC

src/include/stdio_newlib.h

@@ -43,4 +43,9 @@
 #endif
 #define snprintf sniprintf
 
+#ifdef vsnprintf
+#undef vsnprintf
+#endif
+#define vsnprintf vsniprintf
+
 #endif /* STDIO_NEWLIB_H */

src/main.c

@@ -33,14 +33,6 @@
 #include "rtt.h"
 #endif
 
-/* This has to be aligned so the remote protocol can re-use it without causing Problems */
-static char BMD_ALIGN_DEF(8) pbuf[GDB_PACKET_BUFFER_SIZE + 1U];
-
-char *gdb_packet_buffer()
-{
-	return pbuf;
-}
-
 static void bmp_poll_loop(void)
 {
 	SET_IDLE_STATE(false);
@@ -62,11 +54,11 @@ static void bmp_poll_loop(void)
 	}
 
 	SET_IDLE_STATE(true);
-	size_t size = gdb_getpacket(pbuf, GDB_PACKET_BUFFER_SIZE);
+	const gdb_packet_s *const packet = gdb_packet_receive();
 	// If port closed and target detached, stay idle
-	if (pbuf[0] != '\x04' || cur_target)
+	if (packet->data[0] != '\x04' || cur_target)
 		SET_IDLE_STATE(false);
-	gdb_main(pbuf, GDB_PACKET_BUFFER_SIZE, size);
+	gdb_main(packet);
 }
 
 #if CONFIG_BMDA == 1
@@ -85,7 +77,7 @@ int main(void)
 		}
 		CATCH () {
 		default:
-			gdb_putpacketz("EFF");
+			gdb_put_packet_error(0xffU);
 			target_list_free();
 			gdb_outf("Uncaught exception: %s\n", exception_frame.msg);
 			morse("TARGET LOST.", true);

src/platforms/common/stm32/gdb_if.c

@@ -41,32 +41,39 @@ static volatile uint32_t count_new;
 static char double_buffer_out[CDCACM_PACKET_SIZE];
 #endif
 
-void gdb_if_putchar(const char c, const int flush)
+void gdb_if_putchar(const char c, const bool flush)
 {
 	buffer_in[count_in++] = c;
-	if (flush || count_in == CDCACM_PACKET_SIZE) {
-		/* Refuse to send if USB isn't configured, and
-		 * don't bother if nobody's listening */
-		if (usb_get_config() != 1 || !gdb_serial_get_dtr()) {
-			count_in = 0;
-			return;
-		}
-		while (usbd_ep_write_packet(usbdev, CDCACM_GDB_ENDPOINT, buffer_in, count_in) <= 0)
-			continue;
+	if (flush || count_in == CDCACM_PACKET_SIZE)
+		gdb_if_flush(flush);
+}
 
-		if (flush && count_in == CDCACM_PACKET_SIZE) {
-			/* We need to send an empty packet for some hosts
-			 * to accept this as a complete transfer. */
-			/* libopencm3 needs a change for us to confirm when
-			 * that transfer is complete, so we just send a packet
-			 * containing a null byte for now.
-			 */
-			while (usbd_ep_write_packet(usbdev, CDCACM_GDB_ENDPOINT, "\0", 1) <= 0)
-				continue;
-		}
+void gdb_if_flush(const bool force)
+{
+	/* Flush only if there is data to flush */
+	if (count_in == 0U)
+		return;
 
-		count_in = 0;
+	/* Refuse to send if USB isn't configured, and don't bother if nobody's listening */
+	if (usb_get_config() != 1U || !gdb_serial_get_dtr()) {
+		count_in = 0U;
+		return;
 	}
+	while (usbd_ep_write_packet(usbdev, CDCACM_GDB_ENDPOINT, buffer_in, count_in) <= 0U)
+		continue;
+
+	/* We need to send an empty packet for some hosts to accept this as a complete transfer. */
+	if (force && count_in == CDCACM_PACKET_SIZE) {
+		/* 
+		 * libopencm3 needs a change for us to confirm when that transfer is complete,
+		 * so we just send a packet containing a null character for now.
+		 */
+		while (usbd_ep_write_packet(usbdev, CDCACM_GDB_ENDPOINT, "\0", 1U) <= 0U)
+			continue;
+	}
+
+	/* Reset the buffer */
+	count_in = 0U;
 }
 
 #if defined(STM32F4) || defined(STM32F7)

src/platforms/common/tm4c/gdb_if.c

@@ -34,20 +34,39 @@ static uint32_t count_in = 0;
 static volatile char buffer_out[16 * CDCACM_PACKET_SIZE];
 static char buffer_in[CDCACM_PACKET_SIZE];
 
-void gdb_if_putchar(char c, int flush)
+void gdb_if_putchar(const char c, const bool flush)
 {
 	buffer_in[count_in++] = c;
-	if (flush || count_in == CDCACM_PACKET_SIZE) {
-		/* Refuse to send if USB isn't configured, and
-		 * don't bother if nobody's listening */
-		if (usb_get_config() != 1 || !gdb_serial_get_dtr()) {
-			count_in = 0;
-			return;
-		}
-		while (usbd_ep_write_packet(usbdev, CDCACM_GDB_ENDPOINT, buffer_in, count_in) <= 0)
+	if (flush || count_in == CDCACM_PACKET_SIZE)
+		gdb_if_flush(flush);
+}
+
+void gdb_if_flush(const bool force)
+{
+	/* Flush only if there is data to flush */
+	if (count_in == 0U)
+		return;
+
+	/* Refuse to send if USB isn't configured, and don't bother if nobody's listening */
+	if (usb_get_config() != 1U || !gdb_serial_get_dtr()) {
+		count_in = 0U;
+		return;
+	}
+	while (usbd_ep_write_packet(usbdev, CDCACM_GDB_ENDPOINT, buffer_in, count_in) <= 0U)
+		continue;
+
+	/* We need to send an empty packet for some hosts to accept this as a complete transfer. */
+	if (force && count_in == CDCACM_PACKET_SIZE) {
+		/* 
+		 * libopencm3 needs a change for us to confirm when that transfer is complete,
+		 * so we just send a packet containing a null character for now.
+		 */
+		while (usbd_ep_write_packet(usbdev, CDCACM_GDB_ENDPOINT, "\0", 1U) <= 0U)
 			continue;
-		count_in = 0;
 	}
+
+	/* Reset the buffer */
+	count_in = 0U;
 }
 
 void gdb_usb_out_cb(usbd_device *dev, uint8_t ep)

src/platforms/ctxlink/WiFi_Server.c

@@ -1475,17 +1475,28 @@ void send_swo_trace_data(uint8_t *buffer, uint8_t length)
 		do_awo_trace_send();
 }
 
-void wifi_gdb_putchar(uint8_t ch, int flush)
+void wifi_gdb_putchar(const uint8_t ch, const bool flush)
 {
 	send_buffer[send_count++] = ch;
-	if (flush != 0 || send_count >= sizeof(send_buffer)) {
-		// TODO is this check required now, looks like a debug test left in place?
-		int len = (int)send_count;
-		if (len <= 0)
-			DEBUG_WARN("WiFi_putchar bad count\r\n");
-		send_count = 0;
-		DEBUG_WARN("Wifi_putchar %c\r\n", send_buffer[0]);
-		send(gdb_client_socket, &send_buffer[0], len, 0);
-		memset(&send_buffer[0], 0x00, sizeof(send_buffer));
-	}
+	if (flush || send_count >= sizeof(send_buffer))
+		wifi_gdb_flush(flush);
+}
+
+void wifi_gdb_flush(const bool force)
+{
+	(void)force;
+
+	/* Flush only if there is data to flush */
+	if (send_count == 0U)
+		return;
+
+	// TODO is this check required now, looks like a debug test left in place?
+	if (send_count <= 0U)
+		DEBUG_WARN("WiFi_putchar bad count\r\n");
+	DEBUG_WARN("Wifi_putchar %c\r\n", send_buffer[0]);
+	send(gdb_client_socket, &send_buffer[0], send_count, 0);
+
+	/* Reset the buffer */
+	send_count = 0U;
+	memset(&send_buffer[0], 0x00, sizeof(send_buffer));
 }