From 8d0fddc6f22c749820c84769418c724b199b6b07 Mon Sep 17 00:00:00 2001 From: Rafael Silva Date: Tue, 10 Dec 2024 14:48:16 +0000 Subject: [PATCH 01/16] gdb_if: split flush into it's own function --- src/gdb_packet.c | 34 +++++++------- src/include/gdb_if.h | 3 +- src/platforms/common/stm32/gdb_if.c | 49 ++++++++++++--------- src/platforms/common/tm4c/gdb_if.c | 33 +++++++++----- src/platforms/ctxlink/WiFi_Server.c | 33 +++++++++----- src/platforms/ctxlink/WiFi_Server.h | 3 +- src/platforms/ctxlink/ctxlink_gdb_if.c | 61 ++++++++++++++++---------- src/platforms/hosted/gdb_if.c | 27 ++++++++++-- src/remote.c | 20 ++++----- 9 files changed, 164 insertions(+), 99 deletions(-) diff --git a/src/gdb_packet.c b/src/gdb_packet.c index ed4022757eb..0dedfd136be 100644 --- a/src/gdb_packet.c +++ b/src/gdb_packet.c @@ -53,7 +53,7 @@ void gdb_set_noackmode(bool enable) * If we were asked after the connection was terminated, sending the ack will have no effect. */ if (!enable && noackmode) - gdb_if_putchar(GDB_PACKET_ACK, 1U); + gdb_if_putchar(GDB_PACKET_ACK, true); /* Log only changes */ if (noackmode != enable) @@ -209,7 +209,7 @@ size_t gdb_getpacket(char *const packet, const size_t size) rx_checksum |= unhex_digit(rx_char); /* BITWISE OR lower nibble with upper nibble */ /* (N)Acknowledge packet */ - gdb_if_putchar(rx_checksum == checksum ? GDB_PACKET_ACK : GDB_PACKET_NACK, 1U); + gdb_if_putchar(rx_checksum == checksum ? GDB_PACKET_ACK : GDB_PACKET_NACK, true); } if (noackmode || rx_checksum == checksum) { @@ -255,11 +255,11 @@ static void gdb_next_char(const char value, uint8_t *const csum) DEBUG_GDB("\\x%02X", (uint8_t)value); if (value == GDB_PACKET_START || value == GDB_PACKET_END || value == GDB_PACKET_ESCAPE || value == GDB_PACKET_RUNLENGTH_START) { - gdb_if_putchar(GDB_PACKET_ESCAPE, 0); - gdb_if_putchar((char)((uint8_t)value ^ GDB_PACKET_ESCAPE_XOR), 0); + gdb_if_putchar(GDB_PACKET_ESCAPE, false); + gdb_if_putchar((char)((uint8_t)value ^ GDB_PACKET_ESCAPE_XOR), false); *csum += GDB_PACKET_ESCAPE + ((uint8_t)value ^ GDB_PACKET_ESCAPE_XOR); } else { - gdb_if_putchar(value, 0); + gdb_if_putchar(value, false); *csum += value; } } @@ -272,17 +272,17 @@ void gdb_putpacket2(const char *const packet1, const size_t size1, const char *c do { DEBUG_GDB("%s: ", __func__); uint8_t csum = 0; - gdb_if_putchar(GDB_PACKET_START, 0); + gdb_if_putchar(GDB_PACKET_START, false); for (size_t i = 0; i < size1; ++i) gdb_next_char(packet1[i], &csum); for (size_t i = 0; i < size2; ++i) gdb_next_char(packet2[i], &csum); - gdb_if_putchar(GDB_PACKET_END, 0); + gdb_if_putchar(GDB_PACKET_END, false); snprintf(xmit_csum, sizeof(xmit_csum), "%02X", csum); - gdb_if_putchar(xmit_csum[0], 0); - gdb_if_putchar(xmit_csum[1], 1); + gdb_if_putchar(xmit_csum[0], false); + gdb_if_putchar(xmit_csum[1], true); DEBUG_GDB("\n"); } while (!noackmode && gdb_if_getchar_to(2000) != GDB_PACKET_ACK && tries++ < 3U); } @@ -295,13 +295,13 @@ void gdb_putpacket(const char *const packet, const size_t size) do { DEBUG_GDB("%s: ", __func__); uint8_t csum = 0; - gdb_if_putchar(GDB_PACKET_START, 0); + gdb_if_putchar(GDB_PACKET_START, false); for (size_t i = 0; i < size; ++i) gdb_next_char(packet[i], &csum); - gdb_if_putchar(GDB_PACKET_END, 0); + gdb_if_putchar(GDB_PACKET_END, false); snprintf(xmit_csum, sizeof(xmit_csum), "%02X", csum); - gdb_if_putchar(xmit_csum[0], 0); - gdb_if_putchar(xmit_csum[1], 1); + gdb_if_putchar(xmit_csum[0], false); + gdb_if_putchar(xmit_csum[1], true); DEBUG_GDB("\n"); } while (!noackmode && gdb_if_getchar_to(2000) != GDB_PACKET_ACK && tries++ < 3U); } @@ -312,13 +312,13 @@ void gdb_put_notification(const char *const packet, const size_t size) DEBUG_GDB("%s: ", __func__); uint8_t csum = 0; - gdb_if_putchar(GDB_PACKET_NOTIFICATION_START, 0); + gdb_if_putchar(GDB_PACKET_NOTIFICATION_START, false); for (size_t i = 0; i < size; ++i) gdb_next_char(packet[i], &csum); - gdb_if_putchar(GDB_PACKET_END, 0); + gdb_if_putchar(GDB_PACKET_END, false); snprintf(xmit_csum, sizeof(xmit_csum), "%02X", csum); - gdb_if_putchar(xmit_csum[0], 0); - gdb_if_putchar(xmit_csum[1], 1); + gdb_if_putchar(xmit_csum[0], false); + gdb_if_putchar(xmit_csum[1], true); DEBUG_GDB("\n"); } diff --git a/src/include/gdb_if.h b/src/include/gdb_if.h index 529ea808061..bf9d0964365 100644 --- a/src/include/gdb_if.h +++ b/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 */ diff --git a/src/platforms/common/stm32/gdb_if.c b/src/platforms/common/stm32/gdb_if.c index 62db91c7165..516ffda36fa 100644 --- a/src/platforms/common/stm32/gdb_if.c +++ b/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) diff --git a/src/platforms/common/tm4c/gdb_if.c b/src/platforms/common/tm4c/gdb_if.c index 031e85c3f79..eba6722fba1 100644 --- a/src/platforms/common/tm4c/gdb_if.c +++ b/src/platforms/common/tm4c/gdb_if.c @@ -34,20 +34,31 @@ 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) - continue; - count_in = 0; + if (flush || count_in == CDCACM_PACKET_SIZE) + gdb_if_flush(flush); +} + +void gdb_if_flush(const bool force) +{ + (void)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; + + /* Reset the buffer */ + count_in = 0U; } void gdb_usb_out_cb(usbd_device *dev, uint8_t ep) diff --git a/src/platforms/ctxlink/WiFi_Server.c b/src/platforms/ctxlink/WiFi_Server.c index 4b2f9eea46e..19b9616d50c 100644 --- a/src/platforms/ctxlink/WiFi_Server.c +++ b/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)); } diff --git a/src/platforms/ctxlink/WiFi_Server.h b/src/platforms/ctxlink/WiFi_Server.h index 9810a83cbb8..fe34ba0d02d 100644 --- a/src/platforms/ctxlink/WiFi_Server.h +++ b/src/platforms/ctxlink/WiFi_Server.h @@ -39,7 +39,8 @@ void wifi_setup_swo_trace_server(void); bool is_swo_trace_client_connected(void); void send_swo_trace_data(uint8_t *buffer, uint8_t length); -void wifi_gdb_putchar(uint8_t ch, int flush); +void wifi_gdb_putchar(uint8_t ch, bool flush); +void wifi_gdb_flush(bool force); bool wifi_got_client(void); uint8_t wifi_get_next(void); uint8_t wifi_get_next_to(uint32_t timeout); diff --git a/src/platforms/ctxlink/ctxlink_gdb_if.c b/src/platforms/ctxlink/ctxlink_gdb_if.c index ca1cf24ff07..6288c1e0c82 100644 --- a/src/platforms/ctxlink/ctxlink_gdb_if.c +++ b/src/platforms/ctxlink/ctxlink_gdb_if.c @@ -42,32 +42,39 @@ static char buffer_in[CDCACM_PACKET_SIZE]; static volatile uint32_t count_new; static char double_buffer_out[CDCACM_PACKET_SIZE]; -void gdb_usb_putchar(const char ch, const int flush) +void gdb_usb_flush(const bool force) { - buffer_in[count_in++] = ch; - 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) + /* 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; + } - 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; - } + /* Reset the buffer */ + count_in = 0U; +} - count_in = 0; - } +void gdb_usb_putchar(const char ch, const bool flush) +{ + buffer_in[count_in++] = ch; + if (flush || count_in == CDCACM_PACKET_SIZE) + gdb_usb_flush(flush); } void gdb_usb_out_cb(usbd_device *dev, uint8_t ep) @@ -144,7 +151,7 @@ char gdb_usb_getchar_to(const uint32_t timeout) return -1; } -void gdb_if_putchar(char ch, int flush) +void gdb_if_putchar(const char ch, const bool flush) { if (is_gdb_client_connected()) wifi_gdb_putchar(ch, flush); @@ -152,6 +159,14 @@ void gdb_if_putchar(char ch, int flush) gdb_usb_putchar(ch, flush); } +void gdb_if_flush(const bool force) +{ + if (is_gdb_client_connected()) + wifi_gdb_flush(force); + else + gdb_usb_flush(force); +} + char gdb_if_getchar(void) { platform_tasks(); diff --git a/src/platforms/hosted/gdb_if.c b/src/platforms/hosted/gdb_if.c index f5bc8733b6b..16641de2269 100644 --- a/src/platforms/hosted/gdb_if.c +++ b/src/platforms/hosted/gdb_if.c @@ -350,13 +350,32 @@ char gdb_if_getchar_to(uint32_t timeout) return -1; } -void gdb_if_putchar(char c, int flush) +void gdb_if_putchar(const char c, const bool flush) { if (gdb_if_conn == INVALID_SOCKET) return; gdb_buffer[gdb_buffer_used++] = c; - if (flush || gdb_buffer_used == GDB_BUFFER_LEN) { - send(gdb_if_conn, gdb_buffer, gdb_buffer_used, 0); - gdb_buffer_used = 0; + if (flush || gdb_buffer_used == GDB_BUFFER_LEN) + gdb_if_flush(flush); +} + +void gdb_if_flush(const bool force) +{ + (void)force; + + /* Flush only if there is data to flush */ + if (gdb_buffer_used == 0U) + return; + + /* Don't bother if the connection is not valid */ + if (gdb_if_conn == INVALID_SOCKET) { + gdb_buffer_used = 0U; + return; } + + /* Send the data */ + send(gdb_if_conn, gdb_buffer, gdb_buffer_used, 0); + + /* Reset the buffer */ + gdb_buffer_used = 0; } diff --git a/src/remote.c b/src/remote.c index 911d01d68ff..ad1faa6af21 100644 --- a/src/remote.c +++ b/src/remote.c @@ -52,20 +52,20 @@ static void remote_send_buf(const void *const buffer, const size_t len) const uint8_t *const data = (const uint8_t *)buffer; for (size_t offset = 0; offset < len; ++offset) { hexify(hex, data + offset, 1U); - gdb_if_putchar(hex[0], 0); - gdb_if_putchar(hex[1], 0); + gdb_if_putchar(hex[0], false); + gdb_if_putchar(hex[1], false); } } /* Send a response with some data following */ static void remote_respond_buf(const char response_code, const void *const buffer, const size_t len) { - gdb_if_putchar(REMOTE_RESP, 0); - gdb_if_putchar(response_code, 0); + gdb_if_putchar(REMOTE_RESP, false); + gdb_if_putchar(response_code, false); remote_send_buf(buffer, len); - gdb_if_putchar(REMOTE_EOM, 1); + gdb_if_putchar(REMOTE_EOM, true); } /* Send a response with a simple result code parameter */ @@ -98,18 +98,18 @@ static void remote_respond(const char response_code, uint64_t param) /* Send a response with a string following */ static void remote_respond_string(const char response_code, const char *const str) { - gdb_if_putchar(REMOTE_RESP, 0); - gdb_if_putchar(response_code, 0); + gdb_if_putchar(REMOTE_RESP, false); + gdb_if_putchar(response_code, false); const size_t str_length = strlen(str); for (size_t idx = 0; idx < str_length; ++idx) { const char chr = str[idx]; /* Replace problematic/illegal characters with a space to not disturb the protocol */ if (chr == '$' || chr == REMOTE_SOM || chr == REMOTE_EOM) - gdb_if_putchar(' ', 0); + gdb_if_putchar(' ', false); else - gdb_if_putchar(chr, 0); + gdb_if_putchar(chr, false); } - gdb_if_putchar(REMOTE_EOM, 1); + gdb_if_putchar(REMOTE_EOM, true); } /* From b4cf762cdada57afbc6caa14fe0060793ac7274c Mon Sep 17 00:00:00 2001 From: Rafael Silva Date: Fri, 13 Dec 2024 15:18:22 +0000 Subject: [PATCH 02/16] platforms/tm4c: port flush ZLP packet workaround from stm32 The tm4c platform should need the same flush workaround logic as stm32 but it was not implemented --- src/platforms/common/tm4c/gdb_if.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/platforms/common/tm4c/gdb_if.c b/src/platforms/common/tm4c/gdb_if.c index eba6722fba1..d1ddca2ee5e 100644 --- a/src/platforms/common/tm4c/gdb_if.c +++ b/src/platforms/common/tm4c/gdb_if.c @@ -43,8 +43,6 @@ void gdb_if_putchar(const char c, const bool flush) void gdb_if_flush(const bool force) { - (void)force; - /* Flush only if there is data to flush */ if (count_in == 0U) return; @@ -57,6 +55,16 @@ void gdb_if_flush(const bool force) 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; } From d8acf271db3e1295a4fd7cde67ea05999382021e Mon Sep 17 00:00:00 2001 From: Rafael Silva Date: Tue, 10 Dec 2024 14:48:16 +0000 Subject: [PATCH 03/16] gdb_packet: streamline gdb_putpacket This cleans up and reduces code duplication of gdb_putpacket2 and gdb_putpacket This also integrates data hexifing to simplify the usage on common use cases --- src/gdb_main.c | 16 ++-- src/gdb_packet.c | 162 +++++++++++++++++++++++++-------------- src/include/gdb_main.h | 5 -- src/include/gdb_packet.h | 46 ++++++++--- 4 files changed, 150 insertions(+), 79 deletions(-) diff --git a/src/gdb_main.c b/src/gdb_main.c index 06c506599a9..493ecc2b730 100644 --- a/src/gdb_main.c +++ b/src/gdb_main.c @@ -128,7 +128,7 @@ int32_t gdb_main_loop(target_controller_s *tc, char *pbuf, size_t pbuf_size, siz if (reg_size) { uint8_t *gp_regs = alloca(reg_size); target_regs_read(cur_target, gp_regs); - gdb_putpacket(hexify(pbuf, gp_regs, reg_size), reg_size * 2U); + gdb_putpacketx(gp_regs, reg_size); } else { gdb_putpacketz("00"); } @@ -147,7 +147,7 @@ int32_t gdb_main_loop(target_controller_s *tc, char *pbuf, size_t pbuf_size, siz if (target_mem32_read(cur_target, mem, addr, len)) gdb_putpacketz("E01"); else - gdb_putpacket(hexify(pbuf, mem, len), len * 2U); + gdb_putpacketx(mem, len); } else gdb_putpacketz("EFF"); break; @@ -243,7 +243,7 @@ int32_t gdb_main_loop(target_controller_s *tc, char *pbuf, size_t pbuf_size, siz uint8_t val[8]; const size_t length = target_reg_read(cur_target, reg, val, sizeof(val)); if (length != 0) - gdb_putpacket(hexify(pbuf, val, length), length * 2U); + gdb_putpacketx(val, length); else gdb_putpacketz("EFF"); } @@ -413,9 +413,7 @@ static void exec_q_rcmd(const char *packet, const size_t length) gdb_putpacketz("OK"); else { const char *const response = "Failed\n"; - const size_t response_length = strlen(response); - char *pbuf = alloca(response_length * 2 + 1); - gdb_putpacket(hexify(pbuf, response, response_length), 2 * response_length); + gdb_putpacketx(response, strlen(response)); } } @@ -441,7 +439,7 @@ static void handle_q_string_reply(const char *reply, const char *param) size_t output_len = reply_length - addr; if (output_len > len) output_len = len; - gdb_putpacket2("m", 1U, reply + addr, output_len); + gdb_putpacket("m", 1U, reply + addr, output_len, false); } static void exec_q_supported(const char *packet, const size_t length) @@ -612,7 +610,7 @@ static void handle_q_packet(char *packet, const size_t length) if (exec_command(packet, length, q_commands)) return; DEBUG_GDB("*** Unsupported packet: %s\n", packet); - gdb_putpacket("", 0); + gdb_putpacketz(""); } static void exec_v_attach(const char *packet, const size_t length) @@ -640,7 +638,7 @@ static void exec_v_attach(const char *packet, const size_t length) } else { DEBUG_GDB("*** Unsupported packet: %s\n", packet); - gdb_putpacket("", 0); + gdb_putpacketz(""); } } diff --git a/src/gdb_packet.c b/src/gdb_packet.c index 0dedfd136be..1df59c83457 100644 --- a/src/gdb_packet.c +++ b/src/gdb_packet.c @@ -247,78 +247,125 @@ size_t gdb_getpacket(char *const packet, const size_t size) } } -static void gdb_next_char(const char value, uint8_t *const csum) +static inline bool gdb_get_ack(const uint32_t timeout) { + /* Return true early if NoAckMode is enabled */ + if (noackmode) + return true; + + /* Wait for ACK/NACK */ + return gdb_if_getchar_to(timeout) == GDB_PACKET_ACK; +} + +static inline void gdb_putchar(const char value, uint8_t *const csum) +{ + /* Print the character to the debug console */ if (value >= ' ' && value < '\x7f') DEBUG_GDB("%c", value); else DEBUG_GDB("\\x%02X", (uint8_t)value); + + /* Add to checksum */ + if (csum != NULL) + *csum += value; + + /* Send the character to the GDB interface */ + gdb_if_putchar(value, false); +} + +static inline void gdb_putchar_escaped(const char value, uint8_t *const csum) +{ + /* Escape reserved characters */ if (value == GDB_PACKET_START || value == GDB_PACKET_END || value == GDB_PACKET_ESCAPE || value == GDB_PACKET_RUNLENGTH_START) { - gdb_if_putchar(GDB_PACKET_ESCAPE, false); - gdb_if_putchar((char)((uint8_t)value ^ GDB_PACKET_ESCAPE_XOR), false); - *csum += GDB_PACKET_ESCAPE + ((uint8_t)value ^ GDB_PACKET_ESCAPE_XOR); + gdb_putchar(GDB_PACKET_ESCAPE, csum); + gdb_putchar((char)((uint8_t)value ^ GDB_PACKET_ESCAPE_XOR), csum); } else { - gdb_if_putchar(value, false); - *csum += value; + gdb_putchar(value, csum); } } -void gdb_putpacket2(const char *const packet1, const size_t size1, const char *const packet2, const size_t size2) +static inline void gdb_putchar_hex(const char value, uint8_t *const csum) { - char xmit_csum[3]; - size_t tries = 0; + /* Hex characters don't need to be escaped as no hex digit is a reserved character */ + gdb_putchar(hex_digit(value >> 4U), csum); + gdb_putchar(hex_digit(value & 0xffU), csum); +} - do { +void gdb_putpacket(const char *const preamble, const size_t preamble_size, const char *const data, + const size_t data_size, const bool hexify) +{ + for (size_t attempt = 0U; attempt < GDB_PACKET_RETRIES; attempt++) { + /* Start debug print packet */ DEBUG_GDB("%s: ", __func__); - uint8_t csum = 0; - gdb_if_putchar(GDB_PACKET_START, false); - for (size_t i = 0; i < size1; ++i) - gdb_next_char(packet1[i], &csum); - for (size_t i = 0; i < size2; ++i) - gdb_next_char(packet2[i], &csum); + uint8_t csum = 0; /* Checksum of packet data */ - gdb_if_putchar(GDB_PACKET_END, false); - snprintf(xmit_csum, sizeof(xmit_csum), "%02X", csum); - gdb_if_putchar(xmit_csum[0], false); - gdb_if_putchar(xmit_csum[1], true); - DEBUG_GDB("\n"); - } while (!noackmode && gdb_if_getchar_to(2000) != GDB_PACKET_ACK && tries++ < 3U); -} + /* Write start of packet */ + gdb_putchar(GDB_PACKET_START, NULL); -void gdb_putpacket(const char *const packet, const size_t size) -{ - char xmit_csum[3]; - size_t tries = 0; + /* + * Write packet preamble if present + * The preamble in the context of GDB packets is nothing more than regular data + * The distinction in this function is made for convenience as many packets have a constant + * value at the start of the packet data, followed by variable data which may need transformation + */ + if (preamble != NULL) { + for (size_t i = 0; i < preamble_size; ++i) + gdb_putchar_escaped(preamble[i], &csum); + } - do { - DEBUG_GDB("%s: ", __func__); - uint8_t csum = 0; - gdb_if_putchar(GDB_PACKET_START, false); - for (size_t i = 0; i < size; ++i) - gdb_next_char(packet[i], &csum); + /* Write packet data, transforming if needed */ + if (data != NULL) { + if (hexify) { + for (size_t i = 0; i < data_size; ++i) + gdb_putchar_hex(data[i], &csum); + } else { + for (size_t i = 0; i < data_size; ++i) + gdb_putchar_escaped(data[i], &csum); + } + } + + /* Write end of packet */ gdb_if_putchar(GDB_PACKET_END, false); - snprintf(xmit_csum, sizeof(xmit_csum), "%02X", csum); - gdb_if_putchar(xmit_csum[0], false); - gdb_if_putchar(xmit_csum[1], true); + + /* Write checksum and flush the buffer */ + gdb_putchar_hex(csum, NULL); + gdb_if_flush(); + + /* Terminate debug print packet */ DEBUG_GDB("\n"); - } while (!noackmode && gdb_if_getchar_to(2000) != GDB_PACKET_ACK && tries++ < 3U); + + /* Wait for ACK/NACK */ + if (gdb_get_ack(2000U)) + break; + } } -void gdb_put_notification(const char *const packet, const size_t size) +void gdb_put_notification(const char *const data, const size_t size) { - char xmit_csum[3]; - + /* Start debug print packet */ DEBUG_GDB("%s: ", __func__); - uint8_t csum = 0; + + uint8_t csum = 0; /* Checksum of notification data */ + + /* Write start of notification */ gdb_if_putchar(GDB_PACKET_NOTIFICATION_START, false); - for (size_t i = 0; i < size; ++i) - gdb_next_char(packet[i], &csum); + + /* Write notification data */ + if (data != NULL) { + for (size_t i = 0; i < size; i++) + gdb_putchar_escaped(data[i], &csum); + } + + /* Write end of notification */ gdb_if_putchar(GDB_PACKET_END, false); - snprintf(xmit_csum, sizeof(xmit_csum), "%02X", csum); - gdb_if_putchar(xmit_csum[0], false); - gdb_if_putchar(xmit_csum[1], true); + + /* Write checksum and flush the buffer */ + gdb_putchar_hex(csum, NULL); + gdb_if_flush(); + + /* Terminate debug print packet */ DEBUG_GDB("\n"); } @@ -333,22 +380,25 @@ void gdb_putpacket_f(const char *const fmt, ...) /* Heap exhaustion. Report with puts() elsewhere. */ DEBUG_ERROR("gdb_putpacket_f: vasprintf failed\n"); } else { - gdb_putpacket(buf, size); + gdb_putpacket(NULL, 0, buf, size, false); free(buf); } va_end(ap); } -void gdb_out(const char *const buf) +void gdb_out(const char *const str) { - const size_t buf_len = strlen(buf); - char *hexdata = calloc(1, 2U * buf_len + 1U); - if (!hexdata) - return; - - hexify(hexdata, buf, buf_len); - gdb_putpacket2("O", 1, hexdata, 2U * buf_len); - free(hexdata); + /** + * Program console output packet + * See https://sourceware.org/gdb/current/onlinedocs/gdb.html/Stop-Reply-Packets.html#Stop-Reply-Packets + * + * Format; ‘O XX…’ + * ‘XX…’ is hex encoding of ASCII data, to be written as the program’s console output. + * + * Can happen at any time while the program is running and the debugger should continue to wait for ‘W’, ‘T’, etc. + * This reply is not permitted in non-stop mode. + */ + gdb_putpacket("O", 1U, str, strnlen(str, GDB_OUT_PACKET_MAX_SIZE), true); } void gdb_voutf(const char *const fmt, va_list ap) diff --git a/src/include/gdb_main.h b/src/include/gdb_main.h index 066841e5909..2618d64d9c9 100644 --- a/src/include/gdb_main.h +++ b/src/include/gdb_main.h @@ -23,11 +23,6 @@ #include "target.h" -/* Allow override in other platforms if needed */ -#ifndef GDB_PACKET_BUFFER_SIZE -#define GDB_PACKET_BUFFER_SIZE 1024U -#endif - extern bool gdb_target_running; extern target_s *cur_target; diff --git a/src/include/gdb_packet.h b/src/include/gdb_packet.h index a35666a50c1..dc40ce62ec1 100644 --- a/src/include/gdb_packet.h +++ b/src/include/gdb_packet.h @@ -25,6 +25,14 @@ #include #include +/* 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 / 2U) - 1U) + #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,35 @@ #define GDB_FORMAT_ATTR #endif +/* GDB packet transmission configuration */ void gdb_set_noackmode(bool enable); + +/* Raw GDB packet transmission */ 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); +void gdb_putpacket(const char *preamble, size_t preamble_size, const char *data, size_t data_size, bool hexify); +void gdb_put_notification(const char *data, size_t size); + +/* Convenience wrappers */ +static inline void gdb_putpacketz(const char *const str) +{ + gdb_putpacket(NULL, 0, str, strlen(str), false); +} + +static inline void gdb_putpacketx(const void *const data, const size_t size) +{ + gdb_putpacket(NULL, 0, (const char *)data, size, true); +} + +static inline void gdb_put_notificationz(const char *const str) +{ + gdb_put_notification(str, strlen(str)); +} + +/* Formatted output */ +void gdb_putpacket_f(const char *fmt, ...) GDB_FORMAT_ATTR; + +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 */ From 8d2c3b21be93967c2af848b58064238aeec8bf7a Mon Sep 17 00:00:00 2001 From: Rafael Silva Date: Tue, 10 Dec 2024 14:48:16 +0000 Subject: [PATCH 04/16] gdb_main: make the GDB packet buffer constant After the streamlining of gdb_putpacket the packet is not and does not need to be modified --- src/gdb_main.c | 21 +++++++++++---------- src/include/gdb_main.h | 4 ++-- src/target/semihosting.c | 2 +- src/target/semihosting.h | 2 +- 4 files changed, 15 insertions(+), 14 deletions(-) diff --git a/src/gdb_main.c b/src/gdb_main.c index 493ecc2b730..0ac30d03546 100644 --- a/src/gdb_main.c +++ b/src/gdb_main.c @@ -83,9 +83,9 @@ target_s *last_target; bool gdb_target_running = false; static bool gdb_needs_detach_notify = false; -static void handle_q_packet(char *packet, size_t len); -static void handle_v_packet(char *packet, size_t len); -static void handle_z_packet(char *packet, size_t len); +static void handle_q_packet(const char *packet, size_t len); +static void handle_v_packet(const char *packet, size_t len); +static void handle_z_packet(const char *packet, size_t len); static void handle_kill_target(void); static void gdb_target_destroy_callback(target_controller_s *tc, target_s *t) @@ -114,7 +114,8 @@ target_controller_s gdb_controller = { }; /* execute gdb remote command stored in 'pbuf'. returns immediately, no busy waiting. */ -int32_t gdb_main_loop(target_controller_s *tc, char *pbuf, size_t pbuf_size, size_t size, bool in_syscall) +int32_t gdb_main_loop(target_controller_s *const tc, const char *const pbuf, const size_t pbuf_size, const size_t size, + const bool in_syscall) { bool single_step = false; const char *rest = NULL; @@ -382,7 +383,7 @@ int32_t gdb_main_loop(target_controller_s *tc, char *pbuf, size_t pbuf_size, siz return 0; } -static bool exec_command(char *packet, const size_t length, const cmd_executer_s *exec) +static bool exec_command(const char *const packet, const size_t length, const cmd_executer_s *exec) { while (exec->cmd_prefix) { const size_t prefix_length = strlen(exec->cmd_prefix); @@ -605,7 +606,7 @@ static void handle_kill_target(void) } } -static void handle_q_packet(char *packet, const size_t length) +static void handle_q_packet(const char *const packet, const size_t length) { if (exec_command(packet, length, q_commands)) return; @@ -613,7 +614,7 @@ static void handle_q_packet(char *packet, const size_t length) gdb_putpacketz(""); } -static void exec_v_attach(const char *packet, const size_t length) +static void exec_v_attach(const char *const packet, const size_t length) { (void)length; @@ -839,7 +840,7 @@ static const cmd_executer_s v_commands[] = { {NULL, NULL}, }; -static void handle_v_packet(char *packet, const size_t plen) +static void handle_v_packet(const char *const packet, const size_t plen) { if (exec_command(packet, plen, v_commands)) return; @@ -853,7 +854,7 @@ static void handle_v_packet(char *packet, const size_t plen) gdb_putpacketz(""); } -static void handle_z_packet(char *packet, const size_t plen) +static void handle_z_packet(const char *const packet, const size_t plen) { (void)plen; @@ -883,7 +884,7 @@ static void handle_z_packet(char *packet, const size_t plen) gdb_putpacketz("E01"); } -void gdb_main(char *pbuf, size_t pbuf_size, size_t size) +void gdb_main(const char *const pbuf, const size_t pbuf_size, const size_t size) { gdb_main_loop(&gdb_controller, pbuf, pbuf_size, size, false); } diff --git a/src/include/gdb_main.h b/src/include/gdb_main.h index 2618d64d9c9..68440d405a9 100644 --- a/src/include/gdb_main.h +++ b/src/include/gdb_main.h @@ -27,8 +27,8 @@ 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); +void gdb_main(const char *pbuf, size_t pbuf_size, size_t size); +int32_t gdb_main_loop(target_controller_s *tc, const char *pbuf, size_t pbuf_size, size_t size, bool in_syscall); char *gdb_packet_buffer(void); #endif /* INCLUDE_GDB_MAIN_H */ diff --git a/src/target/semihosting.c b/src/target/semihosting.c index 1575064cfb5..603511713d6 100644 --- a/src/target/semihosting.c +++ b/src/target/semihosting.c @@ -127,7 +127,7 @@ const char *const semihosting_names[] = { static semihosting_errno_e semihosting_errno(void); #endif -int32_t semihosting_reply(target_controller_s *const tc, char *const pbuf) +int32_t semihosting_reply(target_controller_s *const tc, const char *const pbuf) { /* * File-I/O Remote Protocol Extension diff --git a/src/target/semihosting.h b/src/target/semihosting.h index 2f37e6aaa9c..7e477627dfc 100644 --- a/src/target/semihosting.h +++ b/src/target/semihosting.h @@ -28,6 +28,6 @@ extern uint32_t semihosting_wallclock_epoch; int32_t semihosting_request(target_s *target, uint32_t syscall, uint32_t r1); -int32_t semihosting_reply(target_controller_s *tc, char *packet); +int32_t semihosting_reply(target_controller_s *tc, const char *packet); #endif /* TARGET_SEMIHOSTING_H */ From de645593328d21776a5759c133f7db19c96359e4 Mon Sep 17 00:00:00 2001 From: Rafael Silva Date: Wed, 11 Dec 2024 11:55:30 +0000 Subject: [PATCH 05/16] gdb_packet: remove dynamic allocations in packet construction In a effort to reduce memory fragmentation this removes heap allocations by reusing the existing packet buffer where possible There is a large stack allocation in a particular code path, this is hopefully better for memory fragmentation though there is a chance of stack overflows, in the event it is a problem it is possible to remove allocations completely This also introduces a struct to group packet buffer data and further streamlines packet transmission --- src/gdb_main.c | 43 ++++---- src/gdb_packet.c | 219 +++++++++++++++++++++++---------------- src/include/gdb_main.h | 6 +- src/include/gdb_packet.h | 33 ++++-- src/main.c | 14 +-- src/target/semihosting.c | 9 +- 6 files changed, 183 insertions(+), 141 deletions(-) diff --git a/src/gdb_main.c b/src/gdb_main.c index 0ac30d03546..bd025d82701 100644 --- a/src/gdb_main.c +++ b/src/gdb_main.c @@ -114,14 +114,13 @@ target_controller_s gdb_controller = { }; /* execute gdb remote command stored in 'pbuf'. returns immediately, no busy waiting. */ -int32_t gdb_main_loop(target_controller_s *const tc, const char *const pbuf, const size_t pbuf_size, const size_t size, - const bool in_syscall) +int32_t gdb_main_loop(target_controller_s *const tc, const gdb_packet_s *const packet, const bool in_syscall) { bool single_step = false; const char *rest = NULL; /* GDB protocol main loop */ - switch (pbuf[0]) { + switch (packet->data[0]) { /* Implementation of these is mandatory! */ case 'g': { /* 'g': Read general registers */ ERROR_IF_NO_TARGET(); @@ -138,8 +137,8 @@ int32_t gdb_main_loop(target_controller_s *const tc, const char *const pbuf, con case 'm': { /* 'm addr,len': Read len bytes from addr */ uint32_t addr, len; ERROR_IF_NO_TARGET(); - if (read_hex32(pbuf + 1, &rest, &addr, ',') && read_hex32(rest, NULL, &len, READ_HEX_NO_FOLLOW)) { - if (len > pbuf_size / 2U) { + if (read_hex32(packet->data + 1, &rest, &addr, ',') && read_hex32(rest, NULL, &len, READ_HEX_NO_FOLLOW)) { + if (len > packet->size / 2U) { gdb_putpacketz("E02"); break; } @@ -158,7 +157,7 @@ int32_t gdb_main_loop(target_controller_s *const tc, const char *const pbuf, con const size_t reg_size = target_regs_size(cur_target); if (reg_size) { uint8_t *gp_regs = alloca(reg_size); - unhexify(gp_regs, &pbuf[1], reg_size); + unhexify(gp_regs, packet->data + 1, reg_size); target_regs_write(cur_target, gp_regs); } gdb_putpacketz("OK"); @@ -168,8 +167,8 @@ int32_t gdb_main_loop(target_controller_s *const tc, const char *const pbuf, con uint32_t addr = 0; uint32_t len = 0; ERROR_IF_NO_TARGET(); - if (read_hex32(pbuf + 1, &rest, &addr, ',') && read_hex32(rest, &rest, &len, ':')) { - if (len > (size - (size_t)(rest - pbuf)) / 2U) { + if (read_hex32(packet->data + 1, &rest, &addr, ',') && read_hex32(rest, &rest, &len, ':')) { + if (len > (packet->size - (size_t)(rest - packet->data)) / 2U) { gdb_putpacketz("E02"); break; } @@ -194,7 +193,7 @@ int32_t gdb_main_loop(target_controller_s *const tc, const char *const pbuf, con * Since we don't care about the operation just skip it but check there is at least 3 characters * in the packet. */ - if (size >= 3 && read_hex32(pbuf + 2, NULL, &thread_id, READ_HEX_NO_FOLLOW) && thread_id <= 1) + if (packet->size >= 3 && read_hex32(packet->data + 2, NULL, &thread_id, READ_HEX_NO_FOLLOW) && thread_id <= 1) gdb_putpacketz("OK"); else gdb_putpacketz("E01"); @@ -238,7 +237,7 @@ int32_t gdb_main_loop(target_controller_s *const tc, const char *const pbuf, con ERROR_IF_NO_TARGET(); if (cur_target->reg_read) { uint32_t reg; - if (!read_hex32(pbuf + 1, NULL, ®, READ_HEX_NO_FOLLOW)) + if (!read_hex32(packet->data + 1, NULL, ®, READ_HEX_NO_FOLLOW)) gdb_putpacketz("EFF"); else { uint8_t val[8]; @@ -266,7 +265,7 @@ int32_t gdb_main_loop(target_controller_s *const tc, const char *const pbuf, con uint32_t reg; /* Extract the register number and check that '=' follows it */ - if (!read_hex32(pbuf + 1, &rest, ®, '=')) { + if (!read_hex32(packet->data + 1, &rest, ®, '=')) { gdb_putpacketz("EFF"); break; } @@ -292,9 +291,9 @@ int32_t gdb_main_loop(target_controller_s *const tc, const char *const pbuf, con case 'F': /* Semihosting call finished */ if (in_syscall) /* Trim off the 'F' before calling semihosting_reply so that it doesn't have to skip it */ - return semihosting_reply(tc, pbuf + 1); + return semihosting_reply(tc, packet->data + 1); else { - DEBUG_GDB("*** F packet when not in syscall! '%s'\n", pbuf); + DEBUG_GDB("*** F packet when not in syscall! '%s'\n", packet->data); gdb_putpacketz(""); } break; @@ -320,7 +319,7 @@ int32_t gdb_main_loop(target_controller_s *const tc, const char *const pbuf, con last_target = cur_target; cur_target = NULL; } - if (pbuf[0] == 'D') + if (packet->data[0] == 'D') gdb_putpacketz("OK"); gdb_set_noackmode(false); break; @@ -345,8 +344,8 @@ int32_t gdb_main_loop(target_controller_s *const tc, const char *const pbuf, con target_addr32_t addr; uint32_t len; ERROR_IF_NO_TARGET(); - if (read_hex32(pbuf + 1, &rest, &addr, ',') && read_hex32(rest, &rest, &len, ':')) { - if (len > (size - (size_t)(rest - pbuf))) { + if (read_hex32(packet->data + 1, &rest, &addr, ',') && read_hex32(rest, &rest, &len, ':')) { + if (len > (packet->size - (size_t)(rest - packet->data))) { gdb_putpacketz("E02"); break; } @@ -362,22 +361,22 @@ int32_t gdb_main_loop(target_controller_s *const tc, const char *const pbuf, con case 'Q': /* General set packet */ case 'q': /* General query packet */ - handle_q_packet(pbuf, size); + handle_q_packet(packet->data, packet->size); break; case 'v': /* Verbose command packet */ - handle_v_packet(pbuf, size); + handle_v_packet(packet->data, packet->size); break; /* These packet implement hardware break-/watchpoints */ case 'Z': /* Z type,addr,len: Set breakpoint packet */ case 'z': /* z type,addr,len: Clear breakpoint packet */ ERROR_IF_NO_TARGET(); - handle_z_packet(pbuf, size); + handle_z_packet(packet->data, packet->size); break; default: /* Packet not implemented */ - DEBUG_GDB("*** Unsupported packet: %s\n", pbuf); + DEBUG_GDB("*** Unsupported packet: %s\n", packet->data); gdb_putpacketz(""); } return 0; @@ -884,9 +883,9 @@ static void handle_z_packet(const char *const packet, const size_t plen) gdb_putpacketz("E01"); } -void gdb_main(const char *const pbuf, const size_t pbuf_size, const size_t size) +void gdb_main(const gdb_packet_s *const packet) { - gdb_main_loop(&gdb_controller, pbuf, pbuf_size, size, false); + gdb_main_loop(&gdb_controller, packet, false); } /* Request halt on the active target */ diff --git a/src/gdb_packet.c b/src/gdb_packet.c index 1df59c83457..b90eba8e4fd 100644 --- a/src/gdb_packet.c +++ b/src/gdb_packet.c @@ -41,6 +41,15 @@ typedef enum packet_state { static bool noackmode = false; +/* This has to be aligned so the remote protocol can re-use it without causing Problems */ +static gdb_packet_s BMD_ALIGN_DEF(8) packet_buffer; + +char *gdb_packet_buffer(void) +{ + /* Return the static packet data buffer */ + return packet_buffer.data; +} + /* https://sourceware.org/gdb/onlinedocs/gdb/Packet-Acknowledgment.html */ void gdb_set_noackmode(bool enable) { @@ -117,11 +126,10 @@ packet_state_e consume_remote_packet(char *const packet, const size_t size) #endif } -size_t gdb_getpacket(char *const packet, const size_t size) +gdb_packet_s *gdb_packet_receive(void) { packet_state_e state = PACKET_IDLE; /* State of the packet capture */ - size_t offset = 0; uint8_t checksum = 0; uint8_t rx_checksum = 0; @@ -130,12 +138,13 @@ size_t gdb_getpacket(char *const packet, const size_t size) switch (state) { case PACKET_IDLE: - packet[0U] = rx_char; + packet_buffer.data[0U] = rx_char; if (rx_char == GDB_PACKET_START) { /* Start of GDB packet */ state = PACKET_GDB_CAPTURE; - offset = 0; checksum = 0; + packet_buffer.size = 0; + packet_buffer.notification = false; } #if CONFIG_BMDA == 0 else if (rx_char == REMOTE_SOM) { @@ -144,22 +153,23 @@ size_t gdb_getpacket(char *const packet, const size_t size) * Let consume_remote_packet handle this * returns PACKET_IDLE or PACKET_GDB_CAPTURE if it detects the start of a GDB packet */ - state = consume_remote_packet(packet, size); - offset = 0; + state = consume_remote_packet(packet_buffer.data, GDB_PACKET_BUFFER_SIZE); checksum = 0; + packet_buffer.size = 0; } #endif /* EOT (end of transmission) - connection was closed */ - if (packet[0U] == '\x04') { - packet[1U] = 0; - return 1U; + else if (rx_char == '\x04') { + packet_buffer.data[1U] = '\0'; /* Null terminate */ + packet_buffer.size = 1U; + return &packet_buffer; } break; case PACKET_GDB_CAPTURE: if (rx_char == GDB_PACKET_START) { /* Restart GDB packet capture */ - offset = 0; + packet_buffer.size = 0; checksum = 0; break; } @@ -180,7 +190,7 @@ size_t gdb_getpacket(char *const packet, const size_t size) state = PACKET_GDB_ESCAPE; else /* Add to packet buffer */ - packet[offset++] = rx_char; + packet_buffer.data[packet_buffer.size++] = rx_char; break; case PACKET_GDB_ESCAPE: @@ -188,7 +198,7 @@ size_t gdb_getpacket(char *const packet, const size_t size) checksum += rx_char; /* Resolve escaped char */ - packet[offset++] = rx_char ^ GDB_PACKET_ESCAPE_XOR; + packet_buffer.data[packet_buffer.size++] = rx_char ^ GDB_PACKET_ESCAPE_XOR; /* Return to normal packet capture */ state = PACKET_GDB_CAPTURE; @@ -214,12 +224,12 @@ size_t gdb_getpacket(char *const packet, const size_t size) if (noackmode || rx_checksum == checksum) { /* Null terminate packet */ - packet[offset] = '\0'; + packet_buffer.data[packet_buffer.size] = '\0'; /* Log packet for debugging */ DEBUG_GDB("%s: ", __func__); - for (size_t j = 0; j < offset; j++) { - const char value = packet[j]; + for (size_t j = 0; j < packet_buffer.size; j++) { + const char value = packet_buffer.data[j]; if (value >= ' ' && value < '\x7f') DEBUG_GDB("%c", value); else @@ -228,7 +238,7 @@ size_t gdb_getpacket(char *const packet, const size_t size) DEBUG_GDB("\n"); /* Return packet captured size */ - return offset; + return &packet_buffer; } /* Restart packet capture */ @@ -241,7 +251,7 @@ size_t gdb_getpacket(char *const packet, const size_t size) break; } - if (offset >= size) + if (packet_buffer.size >= GDB_PACKET_BUFFER_SIZE) /* Buffer overflow, restart packet capture */ state = PACKET_IDLE; } @@ -285,16 +295,9 @@ static inline void gdb_putchar_escaped(const char value, uint8_t *const csum) } } -static inline void gdb_putchar_hex(const char value, uint8_t *const csum) -{ - /* Hex characters don't need to be escaped as no hex digit is a reserved character */ - gdb_putchar(hex_digit(value >> 4U), csum); - gdb_putchar(hex_digit(value & 0xffU), csum); -} - -void gdb_putpacket(const char *const preamble, const size_t preamble_size, const char *const data, - const size_t data_size, const bool hexify) +void gdb_packet_send(const gdb_packet_s *const packet) { + /* Attempt packet transmission up to retries */ for (size_t attempt = 0U; attempt < GDB_PACKET_RETRIES; attempt++) { /* Start debug print packet */ DEBUG_GDB("%s: ", __func__); @@ -302,88 +305,117 @@ void gdb_putpacket(const char *const preamble, const size_t preamble_size, const uint8_t csum = 0; /* Checksum of packet data */ /* Write start of packet */ - gdb_putchar(GDB_PACKET_START, NULL); - - /* - * Write packet preamble if present - * The preamble in the context of GDB packets is nothing more than regular data - * The distinction in this function is made for convenience as many packets have a constant - * value at the start of the packet data, followed by variable data which may need transformation - */ - if (preamble != NULL) { - for (size_t i = 0; i < preamble_size; ++i) - gdb_putchar_escaped(preamble[i], &csum); - } + gdb_putchar(packet->notification ? GDB_PACKET_NOTIFICATION_START : GDB_PACKET_START, NULL); - /* Write packet data, transforming if needed */ - if (data != NULL) { - if (hexify) { - for (size_t i = 0; i < data_size; ++i) - gdb_putchar_hex(data[i], &csum); - } else { - for (size_t i = 0; i < data_size; ++i) - gdb_putchar_escaped(data[i], &csum); - } - } + /* Write packet data */ + for (size_t i = 0; i < packet->size; i++) + gdb_putchar_escaped(packet->data[i], &csum); /* Write end of packet */ gdb_if_putchar(GDB_PACKET_END, false); /* Write checksum and flush the buffer */ - gdb_putchar_hex(csum, NULL); + gdb_putchar(hex_digit(csum >> 4U), NULL); + gdb_putchar(hex_digit(csum & 0xfU), NULL); gdb_if_flush(); /* Terminate debug print packet */ DEBUG_GDB("\n"); - /* Wait for ACK/NACK */ - if (gdb_get_ack(2000U)) + /* Wait for ACK/NACK on standard packets */ + if (packet->notification || gdb_get_ack(2000U)) break; } } -void gdb_put_notification(const char *const data, const size_t size) +void gdb_putpacket(const char *preamble, size_t preamble_size, const char *data, size_t data_size, bool hex_data) { - /* Start debug print packet */ - DEBUG_GDB("%s: ", __func__); - - uint8_t csum = 0; /* Checksum of notification data */ - - /* Write start of notification */ - gdb_if_putchar(GDB_PACKET_NOTIFICATION_START, false); + /* + * Create a packet using the internal packet buffer + * This destroys the previous packet in the buffer + * any packets obtained from gdb_packet_receive() will be invalidated + */ + packet_buffer.notification = false; + packet_buffer.size = 0; - /* Write notification data */ - if (data != NULL) { - for (size_t i = 0; i < size; i++) - gdb_putchar_escaped(data[i], &csum); + /* + * Copy the preamble and data into the packet buffer, limited by the buffer size + * + * This considers GDB_PACKET_BUFFER_SIZE to be the maximum size of the packet + * But it does not take into consideration the extra space needed for escaping + * This is safe because the escaping is done during the actual packet transmission + * but it will result in a packet larger than what we told GDB we could handle + */ + if (preamble != NULL && preamble_size > 0) { + preamble_size = MIN(preamble_size, GDB_PACKET_BUFFER_SIZE); + memcpy(packet_buffer.data, preamble, preamble_size); + packet_buffer.size = preamble_size; } - /* Write end of notification */ - gdb_if_putchar(GDB_PACKET_END, false); - - /* Write checksum and flush the buffer */ - gdb_putchar_hex(csum, NULL); - gdb_if_flush(); + /* Add the data to the packet buffer and transform it if needed */ + if (data != NULL && data_size > 0) { + /* Hex data doubles in size */ + if (hex_data) + data_size *= 2U; + + /* Limit the data size to the remaining space in the packet buffer */ + const size_t remaining_size = GDB_PACKET_BUFFER_SIZE - packet_buffer.size; + data_size = MIN(data_size, remaining_size); + + /* Copy the data into the packet buffer */ + if (hex_data) + hexify(packet_buffer.data + packet_buffer.size, data, data_size / 2U); + else + memcpy(packet_buffer.data + packet_buffer.size, data, data_size); + packet_buffer.size += data_size; + } - /* Terminate debug print packet */ - DEBUG_GDB("\n"); + /* Transmit the packet */ + gdb_packet_send(&packet_buffer); } void gdb_putpacket_f(const char *const fmt, ...) { - va_list ap; - char *buf = NULL; + /* + * Create a packet using the internal packet buffer + * This destroys the previous packet in the buffer + * any packets obtained from gdb_packet_receive() will be invalidated + */ + packet_buffer.notification = false; + /* + * Format the string directly into the packet buffer + * This considers GDB_PACKET_BUFFER_SIZE to be the maximum size of the string + * But it does not take into consideration the extra space needed for escaping + * This is safe because the escaping is done during the actual packet transmission + * but it will result in a packet larger than what we told GDB we could handle + */ + va_list ap; va_start(ap, fmt); - const int size = vasprintf(&buf, fmt, ap); - if (size < 0) { - /* Heap exhaustion. Report with puts() elsewhere. */ - DEBUG_ERROR("gdb_putpacket_f: vasprintf failed\n"); - } else { - gdb_putpacket(NULL, 0, buf, size, false); - free(buf); - } + vsnprintf(packet_buffer.data, sizeof(packet_buffer.data), fmt, ap); va_end(ap); + + /* Get the size of the formatted string */ + packet_buffer.size = strnlen(packet_buffer.data, GDB_PACKET_BUFFER_SIZE); + + /* Transmit the packet */ + gdb_packet_send(&packet_buffer); +} + +void gdb_put_notificationz(const char *const str) +{ + /* + * Create a packet using the internal packet buffer + * This destroys the previous packet in the buffer + * any packets obtained from gdb_packet_receive() will be invalidated + */ + packet_buffer.notification = true; + + packet_buffer.size = strnlen(str, GDB_PACKET_BUFFER_SIZE); + memcpy(packet_buffer.data, str, packet_buffer.size); + + /* Transmit the packet */ + gdb_packet_send(&packet_buffer); } void gdb_out(const char *const str) @@ -392,7 +424,7 @@ void gdb_out(const char *const str) * Program console output packet * See https://sourceware.org/gdb/current/onlinedocs/gdb.html/Stop-Reply-Packets.html#Stop-Reply-Packets * - * Format; ‘O XX…’ + * Format: ‘O XX…’ * ‘XX…’ is hex encoding of ASCII data, to be written as the program’s console output. * * Can happen at any time while the program is running and the debugger should continue to wait for ‘W’, ‘T’, etc. @@ -403,21 +435,26 @@ void gdb_out(const char *const str) void gdb_voutf(const char *const fmt, va_list ap) { - char *buf = NULL; - if (vasprintf(&buf, fmt, ap) < 0) { - /* Heap exhaustion. Report with puts() elsewhere. */ - DEBUG_ERROR("gdb_voutf: vasprintf failed\n"); - return; - } + /* + * We could technically do the formatting and transformation in a single buffer reducing stack usage + * But it is a bit more complex and likely slower, we would need to spread the characters out such + * that each occupies two bytes, and then we could hex them in place + * + * If this stack usage proves to be a problem, we can revisit this + */ + char str_scratch[GDB_OUT_PACKET_MAX_SIZE + 1U]; - gdb_out(buf); - free(buf); + /* Format the string into the scratch buffer */ + vsnprintf(str_scratch, sizeof(str_scratch), fmt, ap); + + /* Delegate the rest of the work to gdb_out */ + gdb_out(str_scratch); } void gdb_outf(const char *const fmt, ...) { + /* Wrap the va_list version of gdb_voutf */ va_list ap; - va_start(ap, fmt); gdb_voutf(fmt, ap); va_end(ap); diff --git a/src/include/gdb_main.h b/src/include/gdb_main.h index 68440d405a9..29982b94e02 100644 --- a/src/include/gdb_main.h +++ b/src/include/gdb_main.h @@ -22,13 +22,13 @@ #define INCLUDE_GDB_MAIN_H #include "target.h" +#include "gdb_packet.h" extern bool gdb_target_running; extern target_s *cur_target; void gdb_poll_target(void); -void gdb_main(const char *pbuf, size_t pbuf_size, size_t size); -int32_t gdb_main_loop(target_controller_s *tc, const 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 */ diff --git a/src/include/gdb_packet.h b/src/include/gdb_packet.h index dc40ce62ec1..8ef25e4d8a9 100644 --- a/src/include/gdb_packet.h +++ b/src/include/gdb_packet.h @@ -31,7 +31,7 @@ #endif /* Limit out packet string size to the maximum packet size before hexifying */ -#define GDB_OUT_PACKET_MAX_SIZE ((GDB_PACKET_BUFFER_SIZE / 2U) - 1U) +#define GDB_OUT_PACKET_MAX_SIZE ((GDB_PACKET_BUFFER_SIZE - 1U) / 2U) #define GDB_PACKET_START '$' #define GDB_PACKET_END '#' @@ -52,18 +52,36 @@ #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); /* Raw GDB packet transmission */ -size_t gdb_getpacket(char *packet, size_t size); -void gdb_putpacket(const char *preamble, size_t preamble_size, const char *data, size_t data_size, bool hexify); -void gdb_put_notification(const char *data, size_t size); +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_putpacket(const char *preamble, size_t preamble_size, const char *data, size_t data_size, bool hex_data); + static inline void gdb_putpacketz(const char *const str) { - gdb_putpacket(NULL, 0, str, strlen(str), false); + gdb_putpacket(str, strlen(str), NULL, 0, false); } static inline void gdb_putpacketx(const void *const data, const size_t size) @@ -71,10 +89,7 @@ static inline void gdb_putpacketx(const void *const data, const size_t size) gdb_putpacket(NULL, 0, (const char *)data, size, true); } -static inline void gdb_put_notificationz(const char *const str) -{ - gdb_put_notification(str, strlen(str)); -} +void gdb_put_notificationz(const char *const str); /* Formatted output */ void gdb_putpacket_f(const char *fmt, ...) GDB_FORMAT_ATTR; diff --git a/src/main.c b/src/main.c index ffa9a7ee333..0b529a2a952 100644 --- a/src/main.c +++ b/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 diff --git a/src/target/semihosting.c b/src/target/semihosting.c index 603511713d6..fbff2f06c28 100644 --- a/src/target/semihosting.c +++ b/src/target/semihosting.c @@ -175,20 +175,19 @@ int32_t semihosting_reply(target_controller_s *const tc, const char *const pbuf) static int32_t semihosting_get_gdb_response(target_controller_s *const tc) { - char *const packet_buffer = gdb_packet_buffer(); /* Still have to service normal 'X'/'m'-packets */ while (true) { /* Get back the next packet to process and have the main loop handle it */ - const size_t size = gdb_getpacket(packet_buffer, GDB_PACKET_BUFFER_SIZE); + const gdb_packet_s *const packet = gdb_packet_receive(); /* If this was an escape packet (or gdb_if reports link closed), fail the call */ - if (size == 1U && packet_buffer[0] == '\x04') + if (packet->size == 1U && packet->data[0] == '\x04') return -1; /* * If this was an F-packet, we are done waiting. * Check before gdb_main_loop as it may clobber the packet buffer. */ - const bool done = packet_buffer[0] == 'F'; - const int32_t result = gdb_main_loop(tc, packet_buffer, GDB_PACKET_BUFFER_SIZE, size, true); + const bool done = packet->data[0] == 'F'; + const int32_t result = gdb_main_loop(tc, packet, true); if (done) return result; } From 8e32d76613ecc6fffd99510188c4e0722351418f Mon Sep 17 00:00:00 2001 From: Rafael Silva Date: Wed, 11 Dec 2024 11:55:30 +0000 Subject: [PATCH 06/16] gdb_packet: implement gdb_packet_debug --- src/gdb_packet.c | 47 +++++++++++++++++++++++++---------------------- 1 file changed, 25 insertions(+), 22 deletions(-) diff --git a/src/gdb_packet.c b/src/gdb_packet.c index b90eba8e4fd..b051dace889 100644 --- a/src/gdb_packet.c +++ b/src/gdb_packet.c @@ -71,6 +71,22 @@ void gdb_set_noackmode(bool enable) noackmode = enable; } +#ifndef DEBUG_GDB_IS_NOOP +static void gdb_packet_debug(const char *const func, const gdb_packet_s *const packet) +{ + /* Log packet for debugging */ + DEBUG_GDB("%s: ", func); + for (size_t i = 0; i < packet->size; i++) { + const char value = packet->data[i]; + if (value >= ' ' && value < '\x7f') + DEBUG_GDB("%c", value); + else + DEBUG_GDB("\\x%02X", (uint8_t)value); + } + DEBUG_GDB("\n"); +} +#endif + packet_state_e consume_remote_packet(char *const packet, const size_t size) { #if CONFIG_BMDA == 0 @@ -226,16 +242,10 @@ gdb_packet_s *gdb_packet_receive(void) /* Null terminate packet */ packet_buffer.data[packet_buffer.size] = '\0'; +#ifndef DEBUG_GDB_IS_NOOP /* Log packet for debugging */ - DEBUG_GDB("%s: ", __func__); - for (size_t j = 0; j < packet_buffer.size; j++) { - const char value = packet_buffer.data[j]; - if (value >= ' ' && value < '\x7f') - DEBUG_GDB("%c", value); - else - DEBUG_GDB("\\x%02X", (uint8_t)value); - } - DEBUG_GDB("\n"); + gdb_packet_debug(__func__, &packet_buffer); +#endif /* Return packet captured size */ return &packet_buffer; @@ -269,18 +279,12 @@ static inline bool gdb_get_ack(const uint32_t timeout) static inline void gdb_putchar(const char value, uint8_t *const csum) { - /* Print the character to the debug console */ - if (value >= ' ' && value < '\x7f') - DEBUG_GDB("%c", value); - else - DEBUG_GDB("\\x%02X", (uint8_t)value); + /* Send the character to the GDB interface */ + gdb_if_putchar(value, false); /* Add to checksum */ if (csum != NULL) *csum += value; - - /* Send the character to the GDB interface */ - gdb_if_putchar(value, false); } static inline void gdb_putchar_escaped(const char value, uint8_t *const csum) @@ -299,9 +303,6 @@ void gdb_packet_send(const gdb_packet_s *const packet) { /* Attempt packet transmission up to retries */ for (size_t attempt = 0U; attempt < GDB_PACKET_RETRIES; attempt++) { - /* Start debug print packet */ - DEBUG_GDB("%s: ", __func__); - uint8_t csum = 0; /* Checksum of packet data */ /* Write start of packet */ @@ -319,8 +320,10 @@ void gdb_packet_send(const gdb_packet_s *const packet) gdb_putchar(hex_digit(csum & 0xfU), NULL); gdb_if_flush(); - /* Terminate debug print packet */ - DEBUG_GDB("\n"); +#ifndef DEBUG_GDB_IS_NOOP + /* Log packet for debugging */ + gdb_packet_debug(__func__, packet); +#endif /* Wait for ACK/NACK on standard packets */ if (packet->notification || gdb_get_ack(2000U)) From 6c40340cccb410c6208b9ef828fe18fa694f27cd Mon Sep 17 00:00:00 2001 From: Rafael Silva Date: Wed, 11 Dec 2024 11:55:30 +0000 Subject: [PATCH 07/16] gdb_packet: implement gdb_packet_checksum Simplify packet transmission and reception with dedicated checksum calculation function --- src/gdb_packet.c | 93 ++++++++++++++++++++++++------------------------ 1 file changed, 46 insertions(+), 47 deletions(-) diff --git a/src/gdb_packet.c b/src/gdb_packet.c index b051dace889..260f980f21c 100644 --- a/src/gdb_packet.c +++ b/src/gdb_packet.c @@ -87,6 +87,27 @@ static void gdb_packet_debug(const char *const func, const gdb_packet_s *const p } #endif +static inline bool gdb_packet_is_reserved(const char character) +{ + /* Check if the character is a reserved GDB packet character */ + return character == GDB_PACKET_START || character == GDB_PACKET_END || character == GDB_PACKET_ESCAPE || + character == GDB_PACKET_RUNLENGTH_START; +} + +static uint8_t gdb_packet_checksum(const gdb_packet_s *const packet) +{ + /* Calculate the checksum of the packet */ + uint8_t checksum = 0; + for (size_t i = 0; i < packet->size; i++) { + const char character = packet->data[i]; + if (gdb_packet_is_reserved(character)) + checksum += GDB_PACKET_ESCAPE + (character ^ GDB_PACKET_ESCAPE_XOR); + else + checksum += character; + } + return checksum; +} + packet_state_e consume_remote_packet(char *const packet, const size_t size) { #if CONFIG_BMDA == 0 @@ -145,8 +166,6 @@ packet_state_e consume_remote_packet(char *const packet, const size_t size) gdb_packet_s *gdb_packet_receive(void) { packet_state_e state = PACKET_IDLE; /* State of the packet capture */ - - uint8_t checksum = 0; uint8_t rx_checksum = 0; while (true) { @@ -158,7 +177,6 @@ gdb_packet_s *gdb_packet_receive(void) if (rx_char == GDB_PACKET_START) { /* Start of GDB packet */ state = PACKET_GDB_CAPTURE; - checksum = 0; packet_buffer.size = 0; packet_buffer.notification = false; } @@ -170,7 +188,6 @@ gdb_packet_s *gdb_packet_receive(void) * returns PACKET_IDLE or PACKET_GDB_CAPTURE if it detects the start of a GDB packet */ state = consume_remote_packet(packet_buffer.data, GDB_PACKET_BUFFER_SIZE); - checksum = 0; packet_buffer.size = 0; } #endif @@ -186,7 +203,6 @@ gdb_packet_s *gdb_packet_receive(void) if (rx_char == GDB_PACKET_START) { /* Restart GDB packet capture */ packet_buffer.size = 0; - checksum = 0; break; } if (rx_char == GDB_PACKET_END) { @@ -197,9 +213,6 @@ gdb_packet_s *gdb_packet_receive(void) break; } - /* Not start or end of packet, add to checksum */ - checksum += rx_char; - /* Add to packet buffer, unless it is an escape char */ if (rx_char == GDB_PACKET_ESCAPE) /* GDB Escaped char */ @@ -210,9 +223,6 @@ gdb_packet_s *gdb_packet_receive(void) break; case PACKET_GDB_ESCAPE: - /* Add to checksum */ - checksum += rx_char; - /* Resolve escaped char */ packet_buffer.data[packet_buffer.size++] = rx_char ^ GDB_PACKET_ESCAPE_XOR; @@ -235,25 +245,25 @@ gdb_packet_s *gdb_packet_receive(void) rx_checksum |= unhex_digit(rx_char); /* BITWISE OR lower nibble with upper nibble */ /* (N)Acknowledge packet */ - gdb_if_putchar(rx_checksum == checksum ? GDB_PACKET_ACK : GDB_PACKET_NACK, true); + const bool checksum_ok = gdb_packet_checksum(&packet_buffer) == rx_checksum; + gdb_if_putchar(checksum_ok ? GDB_PACKET_ACK : GDB_PACKET_NACK, true); + if (!checksum_ok) { + /* Checksum error, restart packet capture */ + state = PACKET_IDLE; + break; + } } - if (noackmode || rx_checksum == checksum) { - /* Null terminate packet */ - packet_buffer.data[packet_buffer.size] = '\0'; + /* Null terminate packet */ + packet_buffer.data[packet_buffer.size] = '\0'; #ifndef DEBUG_GDB_IS_NOOP - /* Log packet for debugging */ - gdb_packet_debug(__func__, &packet_buffer); + /* Log packet for debugging */ + gdb_packet_debug(__func__, &packet_buffer); #endif - /* Return packet captured size */ - return &packet_buffer; - } - - /* Restart packet capture */ - state = PACKET_IDLE; - break; + /* Return captured packet */ + return &packet_buffer; default: /* Something is not right, restart packet capture */ @@ -277,48 +287,37 @@ static inline bool gdb_get_ack(const uint32_t timeout) return gdb_if_getchar_to(timeout) == GDB_PACKET_ACK; } -static inline void gdb_putchar(const char value, uint8_t *const csum) -{ - /* Send the character to the GDB interface */ - gdb_if_putchar(value, false); - - /* Add to checksum */ - if (csum != NULL) - *csum += value; -} - -static inline void gdb_putchar_escaped(const char value, uint8_t *const csum) +static inline void gdb_if_putchar_escaped(const char value) { /* Escape reserved characters */ - if (value == GDB_PACKET_START || value == GDB_PACKET_END || value == GDB_PACKET_ESCAPE || - value == GDB_PACKET_RUNLENGTH_START) { - gdb_putchar(GDB_PACKET_ESCAPE, csum); - gdb_putchar((char)((uint8_t)value ^ GDB_PACKET_ESCAPE_XOR), csum); + if (gdb_packet_is_reserved(value)) { + gdb_if_putchar(GDB_PACKET_ESCAPE, false); + gdb_if_putchar((char)((uint8_t)value ^ GDB_PACKET_ESCAPE_XOR), false); } else { - gdb_putchar(value, csum); + gdb_if_putchar(value, false); } } void gdb_packet_send(const gdb_packet_s *const packet) { + /* Calculate checksum first to avoid re-calculation */ + const uint8_t checksum = gdb_packet_checksum(packet); + /* Attempt packet transmission up to retries */ for (size_t attempt = 0U; attempt < GDB_PACKET_RETRIES; attempt++) { - uint8_t csum = 0; /* Checksum of packet data */ - /* Write start of packet */ - gdb_putchar(packet->notification ? GDB_PACKET_NOTIFICATION_START : GDB_PACKET_START, NULL); + gdb_if_putchar(packet->notification ? GDB_PACKET_NOTIFICATION_START : GDB_PACKET_START, false); /* Write packet data */ for (size_t i = 0; i < packet->size; i++) - gdb_putchar_escaped(packet->data[i], &csum); + gdb_if_putchar_escaped(packet->data[i]); /* Write end of packet */ gdb_if_putchar(GDB_PACKET_END, false); /* Write checksum and flush the buffer */ - gdb_putchar(hex_digit(csum >> 4U), NULL); - gdb_putchar(hex_digit(csum & 0xfU), NULL); - gdb_if_flush(); + gdb_if_putchar(hex_digit(checksum >> 4U), false); + gdb_if_putchar(hex_digit(checksum & 0xfU), true); #ifndef DEBUG_GDB_IS_NOOP /* Log packet for debugging */ From 20d2769ad34aedd8905b01665cea6cb80ca5c8c1 Mon Sep 17 00:00:00 2001 From: Rafael Silva Date: Wed, 11 Dec 2024 11:55:31 +0000 Subject: [PATCH 08/16] gdb_main: use gdb_packet_s in specialized packet handlers --- src/gdb_main.c | 37 +++++++++++++++++++------------------ 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/src/gdb_main.c b/src/gdb_main.c index bd025d82701..8c653243665 100644 --- a/src/gdb_main.c +++ b/src/gdb_main.c @@ -83,9 +83,9 @@ target_s *last_target; bool gdb_target_running = false; static bool gdb_needs_detach_notify = false; -static void handle_q_packet(const char *packet, size_t len); -static void handle_v_packet(const char *packet, size_t len); -static void handle_z_packet(const char *packet, size_t len); +static void handle_q_packet(const gdb_packet_s *packet); +static void handle_v_packet(const gdb_packet_s *packet); +static void handle_z_packet(const gdb_packet_s *packet); static void handle_kill_target(void); static void gdb_target_destroy_callback(target_controller_s *tc, target_s *t) @@ -361,18 +361,18 @@ int32_t gdb_main_loop(target_controller_s *const tc, const gdb_packet_s *const p case 'Q': /* General set packet */ case 'q': /* General query packet */ - handle_q_packet(packet->data, packet->size); + handle_q_packet(packet); break; case 'v': /* Verbose command packet */ - handle_v_packet(packet->data, packet->size); + handle_v_packet(packet); break; /* These packet implement hardware break-/watchpoints */ case 'Z': /* Z type,addr,len: Set breakpoint packet */ case 'z': /* z type,addr,len: Clear breakpoint packet */ ERROR_IF_NO_TARGET(); - handle_z_packet(packet->data, packet->size); + handle_z_packet(packet); break; default: /* Packet not implemented */ @@ -605,11 +605,11 @@ static void handle_kill_target(void) } } -static void handle_q_packet(const char *const packet, const size_t length) +static void handle_q_packet(const gdb_packet_s *const packet) { - if (exec_command(packet, length, q_commands)) + if (exec_command(packet->data, packet->size, q_commands)) return; - DEBUG_GDB("*** Unsupported packet: %s\n", packet); + DEBUG_GDB("*** Unsupported packet: %s\n", packet->data); gdb_putpacketz(""); } @@ -839,33 +839,34 @@ static const cmd_executer_s v_commands[] = { {NULL, NULL}, }; -static void handle_v_packet(const char *const packet, const size_t plen) +static void handle_v_packet(const gdb_packet_s *const packet) { - if (exec_command(packet, plen, v_commands)) + if (exec_command(packet->data, packet->size, v_commands)) return; +#ifndef DEBUG_GDB_IS_NOOP /* * The vMustReplyEmpty is used as a feature test to check how gdbserver handles * unknown packets, don't print an error message for it. */ - if (strcmp(packet, "vMustReplyEmpty") != 0) - DEBUG_GDB("*** Unsupported packet: %s\n", packet); + static const char *const must_reply_empty = "vMustReplyEmpty"; + if (strncmp(packet->data, must_reply_empty, strlen(must_reply_empty)) != 0) + DEBUG_GDB("*** Unsupported packet: %s\n", packet->data); +#endif gdb_putpacketz(""); } -static void handle_z_packet(const char *const packet, const size_t plen) +static void handle_z_packet(const gdb_packet_s *const packet) { - (void)plen; - uint32_t type; uint32_t len; uint32_t addr; const char *rest = NULL; - if (read_dec32(packet + 1, &rest, &type, ',') && read_hex32(rest, &rest, &addr, ',') && + if (read_dec32(packet->data + 1U, &rest, &type, ',') && read_hex32(rest, &rest, &addr, ',') && read_dec32(rest, NULL, &len, READ_HEX_NO_FOLLOW)) { int ret = 0; - if (packet[0] == 'Z') + if (packet->data[0] == 'Z') ret = target_breakwatch_set(cur_target, type, addr, len); else ret = target_breakwatch_clear(cur_target, type, addr, len); From cf30a4b53b50b2ad8d0decfa9800e541a905923f Mon Sep 17 00:00:00 2001 From: Rafael Silva Date: Wed, 11 Dec 2024 11:55:31 +0000 Subject: [PATCH 09/16] gdb_packet: rename put packet functions so their intent is clearer This also adds a gdb_put_packet_ok and gdb_put_packet_error for common standard packet responses --- src/gdb_main.c | 186 +++++++++++++++++++-------------------- src/gdb_packet.c | 8 +- src/include/gdb_packet.h | 58 ++++++++++-- src/main.c | 2 +- src/target/semihosting.c | 22 ++--- 5 files changed, 160 insertions(+), 116 deletions(-) diff --git a/src/gdb_main.c b/src/gdb_main.c index 8c653243665..4dbc81534a5 100644 --- a/src/gdb_main.c +++ b/src/gdb_main.c @@ -67,10 +67,10 @@ typedef enum gdb_signal { GDB_SIGLOST = 29, } gdb_signal_e; -#define ERROR_IF_NO_TARGET() \ - if (!cur_target) { \ - gdb_putpacketz("EFF"); \ - break; \ +#define ERROR_IF_NO_TARGET() \ + if (!cur_target) { \ + gdb_put_packet_error(0xffU); \ + break; \ } typedef struct cmd_executer { @@ -92,7 +92,7 @@ static void gdb_target_destroy_callback(target_controller_s *tc, target_s *t) { (void)tc; if (cur_target == t) { - gdb_put_notificationz("%Stop:W00"); + gdb_put_notification_str("%Stop:W00"); gdb_out("You are now detached from the previous target.\n"); cur_target = NULL; gdb_needs_detach_notify = true; @@ -128,9 +128,9 @@ int32_t gdb_main_loop(target_controller_s *const tc, const gdb_packet_s *const p if (reg_size) { uint8_t *gp_regs = alloca(reg_size); target_regs_read(cur_target, gp_regs); - gdb_putpacketx(gp_regs, reg_size); + gdb_put_packet_hex(gp_regs, reg_size); } else { - gdb_putpacketz("00"); + gdb_put_packet_str("00"); } break; } @@ -139,17 +139,17 @@ int32_t gdb_main_loop(target_controller_s *const tc, const gdb_packet_s *const p ERROR_IF_NO_TARGET(); if (read_hex32(packet->data + 1, &rest, &addr, ',') && read_hex32(rest, NULL, &len, READ_HEX_NO_FOLLOW)) { if (len > packet->size / 2U) { - gdb_putpacketz("E02"); + gdb_put_packet_error(2U); break; } DEBUG_GDB("m packet: addr = %" PRIx32 ", len = %" PRIx32 "\n", addr, len); uint8_t *mem = alloca(len); if (target_mem32_read(cur_target, mem, addr, len)) - gdb_putpacketz("E01"); + gdb_put_packet_error(1U); else - gdb_putpacketx(mem, len); + gdb_put_packet_hex(mem, len); } else - gdb_putpacketz("EFF"); + gdb_put_packet_error(0xffU); break; } case 'G': { /* 'G XX': Write general registers */ @@ -160,7 +160,7 @@ int32_t gdb_main_loop(target_controller_s *const tc, const gdb_packet_s *const p unhexify(gp_regs, packet->data + 1, reg_size); target_regs_write(cur_target, gp_regs); } - gdb_putpacketz("OK"); + gdb_put_packet_ok(); break; } case 'M': { /* 'M addr,len:XX': Write len bytes to addr */ @@ -169,18 +169,18 @@ int32_t gdb_main_loop(target_controller_s *const tc, const gdb_packet_s *const p ERROR_IF_NO_TARGET(); if (read_hex32(packet->data + 1, &rest, &addr, ',') && read_hex32(rest, &rest, &len, ':')) { if (len > (packet->size - (size_t)(rest - packet->data)) / 2U) { - gdb_putpacketz("E02"); + gdb_put_packet_error(2U); break; } DEBUG_GDB("M packet: addr = %" PRIx32 ", len = %" PRIx32 "\n", addr, len); uint8_t *mem = alloca(len); unhexify(mem, rest, len); if (target_mem32_write(cur_target, addr, mem, len)) - gdb_putpacketz("E01"); + gdb_put_packet_error(1U); else - gdb_putpacketz("OK"); + gdb_put_packet_ok(); } else - gdb_putpacketz("EFF"); + gdb_put_packet_error(0xffU); break; } /* @@ -194,9 +194,9 @@ int32_t gdb_main_loop(target_controller_s *const tc, const gdb_packet_s *const p * in the packet. */ if (packet->size >= 3 && read_hex32(packet->data + 2, NULL, &thread_id, READ_HEX_NO_FOLLOW) && thread_id <= 1) - gdb_putpacketz("OK"); + gdb_put_packet_ok(); else - gdb_putpacketz("E01"); + gdb_put_packet_error(1U); break; } case 's': /* 's [addr]': Single step [start at addr] */ @@ -205,7 +205,7 @@ int32_t gdb_main_loop(target_controller_s *const tc, const gdb_packet_s *const p case 'c': /* 'c [addr]': Continue [at addr] */ case 'C': /* 'C sig[;addr]': Continue with signal [at addr] */ if (!cur_target) { - gdb_putpacketz("X1D"); + gdb_put_packet_str("X1D"); break; } @@ -218,7 +218,7 @@ int32_t gdb_main_loop(target_controller_s *const tc, const gdb_packet_s *const p * but GDB doesn't work without it. */ if (!cur_target) { - gdb_putpacketz("W00"); /* Report "target exited" if no target */ + gdb_put_packet_str("W00"); /* Report "target exited" if no target */ break; } @@ -238,17 +238,17 @@ int32_t gdb_main_loop(target_controller_s *const tc, const gdb_packet_s *const p if (cur_target->reg_read) { uint32_t reg; if (!read_hex32(packet->data + 1, NULL, ®, READ_HEX_NO_FOLLOW)) - gdb_putpacketz("EFF"); + gdb_put_packet_error(0xffU); else { uint8_t val[8]; const size_t length = target_reg_read(cur_target, reg, val, sizeof(val)); if (length != 0) - gdb_putpacketx(val, length); + gdb_put_packet_hex(val, length); else - gdb_putpacketz("EFF"); + gdb_put_packet_error(0xffU); } } else { - gdb_putpacketz("00"); + gdb_put_packet_str("00"); } break; } @@ -266,24 +266,24 @@ int32_t gdb_main_loop(target_controller_s *const tc, const gdb_packet_s *const p /* Extract the register number and check that '=' follows it */ if (!read_hex32(packet->data + 1, &rest, ®, '=')) { - gdb_putpacketz("EFF"); + gdb_put_packet_error(0xffU); break; } const size_t value_length = strlen(rest) / 2U; /* If the value is bigger than 4 bytes report error */ if (value_length > 4U) { - gdb_putpacketz("EFF"); + gdb_put_packet_error(0xffU); break; } uint8_t value[4] = {0}; unhexify(value, rest, value_length); /* Finally, write the converted value to the target */ if (target_reg_write(cur_target, reg, value, sizeof(value)) != 0) - gdb_putpacketz("OK"); + gdb_put_packet_ok(); else - gdb_putpacketz("EFF"); + gdb_put_packet_error(0xffU); } else { - gdb_putpacketz("OK"); + gdb_put_packet_ok(); } break; } @@ -294,7 +294,7 @@ int32_t gdb_main_loop(target_controller_s *const tc, const gdb_packet_s *const p return semihosting_reply(tc, packet->data + 1); else { DEBUG_GDB("*** F packet when not in syscall! '%s'\n", packet->data); - gdb_putpacketz(""); + gdb_put_packet_empty(); } break; @@ -304,7 +304,7 @@ int32_t gdb_main_loop(target_controller_s *const tc, const gdb_packet_s *const p * protocol anyway, but GDB will never send us a 'R' * packet unless we answer 'OK' here. */ - gdb_putpacketz("OK"); + gdb_put_packet_ok(); break; case '\x04': @@ -320,7 +320,7 @@ int32_t gdb_main_loop(target_controller_s *const tc, const gdb_packet_s *const p cur_target = NULL; } if (packet->data[0] == 'D') - gdb_putpacketz("OK"); + gdb_put_packet_ok(); gdb_set_noackmode(false); break; @@ -346,16 +346,16 @@ int32_t gdb_main_loop(target_controller_s *const tc, const gdb_packet_s *const p ERROR_IF_NO_TARGET(); if (read_hex32(packet->data + 1, &rest, &addr, ',') && read_hex32(rest, &rest, &len, ':')) { if (len > (packet->size - (size_t)(rest - packet->data))) { - gdb_putpacketz("E02"); + gdb_put_packet_error(2U); break; } DEBUG_GDB("X packet: addr = %" PRIx32 ", len = %" PRIx32 "\n", addr, len); if (target_mem32_write(cur_target, addr, rest, len)) - gdb_putpacketz("E01"); + gdb_put_packet_error(1U); else - gdb_putpacketz("OK"); + gdb_put_packet_ok(); } else - gdb_putpacketz("EFF"); + gdb_put_packet_error(0xffU); break; } @@ -377,7 +377,7 @@ int32_t gdb_main_loop(target_controller_s *const tc, const gdb_packet_s *const p default: /* Packet not implemented */ DEBUG_GDB("*** Unsupported packet: %s\n", packet->data); - gdb_putpacketz(""); + gdb_put_packet_empty(); } return 0; } @@ -408,12 +408,12 @@ static void exec_q_rcmd(const char *packet, const size_t length) const int result = command_process(cur_target, data); if (result < 0) - gdb_putpacketz(""); + gdb_put_packet_empty(); else if (result == 0) - gdb_putpacketz("OK"); + gdb_put_packet_ok(); else { const char *const response = "Failed\n"; - gdb_putpacketx(response, strlen(response)); + gdb_put_packet_hex(response, strlen(response)); } } @@ -425,21 +425,21 @@ static void handle_q_string_reply(const char *reply, const char *param) const char *rest = NULL; if (!read_hex32(param, &rest, &addr, ',') || !read_hex32(rest, NULL, &len, READ_HEX_NO_FOLLOW)) { - gdb_putpacketz("E01"); + gdb_put_packet_error(1U); return; } if (addr > reply_length) { - gdb_putpacketz("E01"); + gdb_put_packet_error(1U); return; } if (addr == reply_length) { - gdb_putpacketz("l"); + gdb_put_packet_str("l"); return; } size_t output_len = reply_length - addr; if (output_len > len) output_len = len; - gdb_putpacket("m", 1U, reply + addr, output_len, false); + gdb_put_packet("m", 1U, reply + addr, output_len, false); } static void exec_q_supported(const char *packet, const size_t length) @@ -453,8 +453,8 @@ static void exec_q_supported(const char *packet, const size_t length) */ gdb_set_noackmode(false); - gdb_putpacket_f("PacketSize=%X;qXfer:memory-map:read+;qXfer:features:read+;" - "vContSupported+" GDB_QSUPPORTED_NOACKMODE, + gdb_putpacket_str_f("PacketSize=%X;qXfer:memory-map:read+;qXfer:features:read+;" + "vContSupported+" GDB_QSUPPORTED_NOACKMODE, GDB_PACKET_BUFFER_SIZE); } @@ -467,7 +467,7 @@ static void exec_q_memory_map(const char *packet, const size_t length) if (!target) target = last_target; if (!target) { - gdb_putpacketz("E01"); + gdb_put_packet_error(1U); return; } char buf[1024]; @@ -484,7 +484,7 @@ static void exec_q_feature_read(const char *packet, const size_t length) if (!target) target = last_target; if (!target) { - gdb_putpacketz("E01"); + gdb_put_packet_error(1U); return; } const char *const description = target_regs_description(target); @@ -503,14 +503,14 @@ static void exec_q_crc(const char *packet, const size_t length) const char *rest = NULL; if (read_hex32(packet, &rest, &addr, ',') && read_hex32(rest, NULL, &addr_length, READ_HEX_NO_FOLLOW)) { if (!cur_target) { - gdb_putpacketz("E01"); + gdb_put_packet_error(1U); return; } uint32_t crc; if (!bmd_crc32(cur_target, &crc, addr, addr_length)) - gdb_putpacketz("E03"); + gdb_put_packet_error(3U); else - gdb_putpacket_f("C%" PRIx32, crc); + gdb_putpacket_str_f("C%" PRIx32, crc); } } @@ -522,7 +522,7 @@ static void exec_q_c(const char *packet, const size_t length) { (void)packet; (void)length; - gdb_putpacketz("QC1"); + gdb_put_packet_str("QC1"); } /* @@ -536,9 +536,9 @@ static void exec_q_thread_info(const char *packet, const size_t length) { (void)length; if (packet[-11] == 'f' && cur_target) - gdb_putpacketz("m1"); + gdb_put_packet_str("m1"); else - gdb_putpacketz("l"); + gdb_put_packet_str("l"); } /* @@ -552,7 +552,7 @@ static void exec_q_noackmode(const char *packet, const size_t length) (void)packet; (void)length; gdb_set_noackmode(true); - gdb_putpacketz("OK"); + gdb_put_packet_ok(); } /* @@ -573,12 +573,12 @@ static void exec_q_attached(const char *packet, const size_t length) { /* If the packet has a trailing PID, or we're not attached to anything, error response */ if ((length && packet[0] == ':') || !cur_target) - gdb_putpacketz("E01"); + gdb_put_packet_error(1U); /* Check if the target tollerates being reset */ else if (cur_target->target_options & TOPT_INHIBIT_NRST) - gdb_putpacketz("1"); /* It does not. */ + gdb_put_packet_str("1"); /* It does not. */ else - gdb_putpacketz("0"); /* It does tolelrate reset */ + gdb_put_packet_str("0"); /* It does tolelrate reset */ } static const cmd_executer_s q_commands[] = { @@ -610,7 +610,7 @@ static void handle_q_packet(const gdb_packet_s *const packet) if (exec_command(packet->data, packet->size, q_commands)) return; DEBUG_GDB("*** Unsupported packet: %s\n", packet->data); - gdb_putpacketz(""); + gdb_put_packet_empty(); } static void exec_v_attach(const char *const packet, const size_t length) @@ -632,13 +632,13 @@ static void exec_v_attach(const char *const packet, const size_t length) * https://sourceware.org/pipermail/gdb-patches/2022-April/188058.html * https://sourceware.org/pipermail/gdb-patches/2022-July/190869.html */ - gdb_putpacket_f("T%02Xthread:1;", GDB_SIGTRAP); + gdb_putpacket_str_f("T%02Xthread:1;", GDB_SIGTRAP); } else - gdb_putpacketz("E01"); + gdb_put_packet_error(1U); } else { DEBUG_GDB("*** Unsupported packet: %s\n", packet); - gdb_putpacketz(""); + gdb_put_packet_empty(); } } @@ -648,7 +648,7 @@ static void exec_v_kill(const char *packet, const size_t length) (void)length; /* Kill the target - we don't actually care about the PID that follows "vKill;" */ handle_kill_target(); - gdb_putpacketz("OK"); + gdb_put_packet_ok(); } static void exec_v_run(const char *packet, const size_t length) @@ -698,7 +698,7 @@ static void exec_v_run(const char *packet, const size_t length) if (cur_target) { target_set_cmdline(cur_target, cmdline, offset); target_reset(cur_target); - gdb_putpacketz("T05"); + gdb_put_packet_str("T05"); } else if (last_target) { cur_target = target_attach(last_target, &gdb_controller); @@ -707,12 +707,12 @@ static void exec_v_run(const char *packet, const size_t length) target_set_cmdline(cur_target, cmdline, offset); target_reset(cur_target); morse(NULL, false); - gdb_putpacketz("T05"); + gdb_put_packet_str("T05"); } else - gdb_putpacketz("E01"); + gdb_put_packet_error(1U); } else - gdb_putpacketz("E01"); + gdb_put_packet_error(1U); } static void exec_v_cont(const char *packet, const size_t length) @@ -731,13 +731,13 @@ static void exec_v_cont(const char *packet, const size_t length) * * TODO: Support the 't' (stop) action needed for non-stop debug so GDB can request a halt. */ - gdb_putpacketz("vCont;c;C;s;t"); + gdb_put_packet_str("vCont;c;C;s;t"); return; } /* Otherwise it's a standard `vCont` packet, check if we're presently attached to a target */ if (!cur_target) { - gdb_putpacketz("E01"); + gdb_put_packet_error(1U); return; } @@ -749,7 +749,7 @@ static void exec_v_cont(const char *packet, const size_t length) case 'c': /* 'c': Continue */ case 'C': /* 'C sig': Continue with signal */ if (!cur_target) { - gdb_putpacketz("X1D"); + gdb_put_packet_str("X1D"); break; } @@ -773,18 +773,18 @@ static void exec_v_flash_erase(const char *packet, const size_t length) /* Erase Flash Memory */ DEBUG_GDB("Flash Erase %08" PRIX32 " %08" PRIX32 "\n", addr, len); if (!cur_target) { - gdb_putpacketz("EFF"); + gdb_put_packet_error(0xffU); return; } if (target_flash_erase(cur_target, addr, len)) - gdb_putpacketz("OK"); + gdb_put_packet_ok(); else { target_flash_complete(cur_target); - gdb_putpacketz("EFF"); + gdb_put_packet_error(0xffU); } } else - gdb_putpacketz("EFF"); + gdb_put_packet_error(0xffU); } static void exec_v_flash_write(const char *packet, const size_t length) @@ -796,13 +796,13 @@ static void exec_v_flash_write(const char *packet, const size_t length) const uint32_t count = length - (size_t)(rest - packet); DEBUG_GDB("Flash Write %08" PRIX32 " %08" PRIX32 "\n", addr, count); if (cur_target && target_flash_write(cur_target, addr, (const uint8_t *)rest, count)) - gdb_putpacketz("OK"); + gdb_put_packet_ok(); else { target_flash_complete(cur_target); - gdb_putpacketz("EFF"); + gdb_put_packet_error(0xffU); } } else - gdb_putpacketz("EFF"); + gdb_put_packet_error(0xffU); } static void exec_v_flash_done(const char *packet, const size_t length) @@ -811,9 +811,9 @@ static void exec_v_flash_done(const char *packet, const size_t length) (void)length; /* Commit flash operations. */ if (target_flash_complete(cur_target)) - gdb_putpacketz("OK"); + gdb_put_packet_ok(); else - gdb_putpacketz("EFF"); + gdb_put_packet_error(0xffU); } static void exec_v_stopped(const char *packet, const size_t length) @@ -821,10 +821,10 @@ static void exec_v_stopped(const char *packet, const size_t length) (void)packet; (void)length; if (gdb_needs_detach_notify) { - gdb_putpacketz("W00"); + gdb_put_packet_str("W00"); gdb_needs_detach_notify = false; } else - gdb_putpacketz("OK"); + gdb_put_packet_ok(); } static const cmd_executer_s v_commands[] = { @@ -853,7 +853,7 @@ static void handle_v_packet(const gdb_packet_s *const packet) if (strncmp(packet->data, must_reply_empty, strlen(must_reply_empty)) != 0) DEBUG_GDB("*** Unsupported packet: %s\n", packet->data); #endif - gdb_putpacketz(""); + gdb_put_packet_empty(); } static void handle_z_packet(const gdb_packet_s *const packet) @@ -873,15 +873,15 @@ static void handle_z_packet(const gdb_packet_s *const packet) /* If the target handler was unable to set/clear the break/watch-point, return an error */ if (ret < 0) - gdb_putpacketz("E01"); + gdb_put_packet_error(1U); /* If the handler does not support the kind requested, return empty string */ else if (ret > 0) - gdb_putpacketz(""); + gdb_put_packet_empty(); /* Otherwise let GDB know that everything went well */ else - gdb_putpacketz("OK"); + gdb_put_packet_ok(); } else - gdb_putpacketz("E01"); + gdb_put_packet_error(1U); } void gdb_main(const gdb_packet_s *const packet) @@ -896,7 +896,7 @@ void gdb_halt_target(void) target_halt_request(cur_target); else /* Report "target exited" if no target */ - gdb_putpacketz("W00"); + gdb_put_packet_str("W00"); } /* Poll the running target to see if it halted yet */ @@ -904,7 +904,7 @@ void gdb_poll_target(void) { if (!cur_target) { /* Report "target exited" if no target */ - gdb_putpacketz("W00"); + gdb_put_packet_str("W00"); return; } @@ -921,19 +921,19 @@ void gdb_poll_target(void) /* Translate reason to GDB signal */ switch (reason) { case TARGET_HALT_ERROR: - gdb_putpacket_f("X%02X", GDB_SIGLOST); + gdb_putpacket_str_f("X%02X", GDB_SIGLOST); morse("TARGET LOST.", true); break; case TARGET_HALT_REQUEST: - gdb_putpacket_f("T%02Xthread:1;", GDB_SIGINT); + gdb_putpacket_str_f("T%02Xthread:1;", GDB_SIGINT); break; case TARGET_HALT_WATCHPOINT: - gdb_putpacket_f("T%02Xwatch:%08" PRIX32 ";", GDB_SIGTRAP, watch); + gdb_putpacket_str_f("T%02Xwatch:%08" PRIX32 ";", GDB_SIGTRAP, watch); break; case TARGET_HALT_FAULT: - gdb_putpacket_f("T%02Xthread:1;", GDB_SIGSEGV); + gdb_putpacket_str_f("T%02Xthread:1;", GDB_SIGSEGV); break; default: - gdb_putpacket_f("T%02Xthread:1;", GDB_SIGTRAP); + gdb_putpacket_str_f("T%02Xthread:1;", GDB_SIGTRAP); } } diff --git a/src/gdb_packet.c b/src/gdb_packet.c index 260f980f21c..5295ccacfe7 100644 --- a/src/gdb_packet.c +++ b/src/gdb_packet.c @@ -330,7 +330,7 @@ void gdb_packet_send(const gdb_packet_s *const packet) } } -void gdb_putpacket(const char *preamble, size_t preamble_size, const char *data, size_t data_size, bool hex_data) +void gdb_put_packet(const char *preamble, size_t preamble_size, const char *data, size_t data_size, bool hex_data) { /* * Create a packet using the internal packet buffer @@ -376,7 +376,7 @@ void gdb_putpacket(const char *preamble, size_t preamble_size, const char *data, gdb_packet_send(&packet_buffer); } -void gdb_putpacket_f(const char *const fmt, ...) +void gdb_putpacket_str_f(const char *const fmt, ...) { /* * Create a packet using the internal packet buffer @@ -404,7 +404,7 @@ void gdb_putpacket_f(const char *const fmt, ...) gdb_packet_send(&packet_buffer); } -void gdb_put_notificationz(const char *const str) +void gdb_put_notification_str(const char *const str) { /* * Create a packet using the internal packet buffer @@ -432,7 +432,7 @@ void gdb_out(const char *const str) * Can happen at any time while the program is running and the debugger should continue to wait for ‘W’, ‘T’, etc. * This reply is not permitted in non-stop mode. */ - gdb_putpacket("O", 1U, str, strnlen(str, GDB_OUT_PACKET_MAX_SIZE), true); + gdb_put_packet("O", 1U, str, strnlen(str, GDB_OUT_PACKET_MAX_SIZE), true); } void gdb_voutf(const char *const fmt, va_list ap) diff --git a/src/include/gdb_packet.h b/src/include/gdb_packet.h index 8ef25e4d8a9..69ff65396f4 100644 --- a/src/include/gdb_packet.h +++ b/src/include/gdb_packet.h @@ -77,22 +77,66 @@ void gdb_packet_send(const gdb_packet_s *packet); char *gdb_packet_buffer(void); /* Convenience wrappers */ -void gdb_putpacket(const char *preamble, size_t preamble_size, const char *data, size_t data_size, bool hex_data); +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_putpacketz(const char *const str) +static inline void gdb_put_packet_empty(void) { - gdb_putpacket(str, strlen(str), NULL, 0, false); + /** + * 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_putpacketx(const void *const data, const size_t size) +static inline void gdb_put_packet_str(const char *const str) { - gdb_putpacket(NULL, 0, (const char *)data, size, true); + gdb_put_packet(str, strlen(str), NULL, 0, false); } -void gdb_put_notificationz(const char *const str); +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_f(const char *fmt, ...) GDB_FORMAT_ATTR; +void gdb_putpacket_str_f(const char *fmt, ...) GDB_FORMAT_ATTR; void gdb_out(const char *str); void gdb_voutf(const char *fmt, va_list ap); diff --git a/src/main.c b/src/main.c index 0b529a2a952..00f7dd9e153 100644 --- a/src/main.c +++ b/src/main.c @@ -77,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); diff --git a/src/target/semihosting.c b/src/target/semihosting.c index fbff2f06c28..b469dfb48a9 100644 --- a/src/target/semihosting.c +++ b/src/target/semihosting.c @@ -211,7 +211,7 @@ static int32_t semihosting_remote_read( return result; } #endif - gdb_putpacket_f("Fread,%08X,%08" PRIX32 ",%08" PRIX32, (unsigned)fd, buf_taddr, count); + gdb_putpacket_str_f("Fread,%08X,%08" PRIX32 ",%08" PRIX32, (unsigned)fd, buf_taddr, count); return semihosting_get_gdb_response(target->tc); } @@ -254,7 +254,7 @@ static int32_t semihosting_remote_write( return (int32_t)count; } - gdb_putpacket_f("Fwrite,%08X,%08" PRIX32 ",%08" PRIX32, (unsigned)fd, buf_taddr, count); + gdb_putpacket_str_f("Fwrite,%08X,%08" PRIX32 ",%08" PRIX32, (unsigned)fd, buf_taddr, count); return semihosting_get_gdb_response(target->tc); } @@ -407,7 +407,7 @@ int32_t semihosting_open(target_s *const target, const semihosting_s *const requ free((void *)file_name); #pragma GCC diagnostic pop #else - gdb_putpacket_f("Fopen,%08" PRIX32 "/%08" PRIX32 ",%08" PRIX32 ",%08X", file_name_taddr, file_name_length + 1U, + gdb_putpacket_str_f("Fopen,%08" PRIX32 "/%08" PRIX32 ",%08" PRIX32 ",%08X", file_name_taddr, file_name_length + 1U, open_mode, 0644U); const int32_t result = semihosting_get_gdb_response(target->tc); #endif @@ -431,7 +431,7 @@ int32_t semihosting_close(target_s *const target, const semihosting_s *const req target->tc->gdb_errno = semihosting_errno(); return result; #else - gdb_putpacket_f("Fclose,%08X", (unsigned)fd); + gdb_putpacket_str_f("Fclose,%08X", (unsigned)fd); return semihosting_get_gdb_response(target->tc); #endif } @@ -517,7 +517,7 @@ int32_t semihosting_isatty(target_s *const target, const semihosting_s *const re return result; } #endif - gdb_putpacket_f("Fisatty,%08X", (unsigned)fd); + gdb_putpacket_str_f("Fisatty,%08X", (unsigned)fd); return semihosting_get_gdb_response(target->tc); } @@ -540,7 +540,7 @@ int32_t semihosting_seek(target_s *const target, const semihosting_s *const requ return result; } #endif - gdb_putpacket_f("Flseek,%08X,%08lX,%08X", (unsigned)fd, (unsigned long)offset, SEEK_MODE_SET); + gdb_putpacket_str_f("Flseek,%08X,%08lX,%08X", (unsigned)fd, (unsigned long)offset, SEEK_MODE_SET); return semihosting_get_gdb_response(target->tc) == offset ? 0 : -1; } @@ -567,7 +567,7 @@ int32_t semihosting_rename(target_s *const target, const semihosting_s *const re #pragma GCC diagnostic pop return result; #else - gdb_putpacket_f("Frename,%08" PRIX32 "/%08" PRIX32 ",%08" PRIX32 "/%08" PRIX32, request->params[0], + gdb_putpacket_str_f("Frename,%08" PRIX32 "/%08" PRIX32 ",%08" PRIX32 "/%08" PRIX32, request->params[0], request->params[1] + 1U, request->params[2], request->params[3] + 1U); return semihosting_get_gdb_response(target->tc); #endif @@ -587,7 +587,7 @@ int32_t semihosting_remove(target_s *const target, const semihosting_s *const re #pragma GCC diagnostic pop return result; #else - gdb_putpacket_f("Funlink,%08" PRIX32 "/%08" PRIX32, request->params[0], request->params[1] + 1U); + gdb_putpacket_str_f("Funlink,%08" PRIX32 "/%08" PRIX32, request->params[0], request->params[1] + 1U); return semihosting_get_gdb_response(target->tc); #endif } @@ -595,7 +595,7 @@ int32_t semihosting_remove(target_s *const target, const semihosting_s *const re int32_t semihosting_system(target_s *const target, const semihosting_s *const request) { /* NB: Before use first enable system calls with the following gdb command: 'set remote system-call-allowed 1' */ - gdb_putpacket_f("Fsystem,%08" PRIX32 "/%08" PRIX32, request->params[0], request->params[1] + 1U); + gdb_putpacket_str_f("Fsystem,%08" PRIX32 "/%08" PRIX32, request->params[0], request->params[1] + 1U); return semihosting_get_gdb_response(target->tc); } @@ -629,7 +629,7 @@ int32_t semihosting_file_length(target_s *const target, const semihosting_s *con target->tc->semihosting_buffer_ptr = file_stat; target->tc->semihosting_buffer_len = sizeof(file_stat); /* Call GDB and ask for the file descriptor's stat info */ - gdb_putpacket_f("Ffstat,%X,%08" PRIX32, (unsigned)fd, target->ram->start); + gdb_putpacket_str_f("Ffstat,%X,%08" PRIX32, (unsigned)fd, target->ram->start); const int32_t stat_result = semihosting_get_gdb_response(target->tc); target->target_options &= ~TOPT_IN_SEMIHOSTING_SYSCALL; /* Extract the lower half of the file size from the buffer */ @@ -650,7 +650,7 @@ semihosting_time_s semihosting_get_time(target_s *const target) target->tc->semihosting_buffer_ptr = time_value; target->tc->semihosting_buffer_len = sizeof(time_value); /* Call GDB and ask for the current time using gettimeofday() */ - gdb_putpacket_f("Fgettimeofday,%08" PRIX32 ",%08" PRIX32, target->ram->start, (target_addr_t)NULL); + gdb_putpacket_str_f("Fgettimeofday,%08" PRIX32 ",%08" PRIX32, target->ram->start, (target_addr_t)NULL); const int32_t result = semihosting_get_gdb_response(target->tc); target->target_options &= ~TOPT_IN_SEMIHOSTING_SYSCALL; /* Check if the GDB remote gettimeofday() failed */ From 057d2522901e22c7d18b56f02353f9a044a4bfc3 Mon Sep 17 00:00:00 2001 From: Rafael Silva Date: Wed, 11 Dec 2024 12:10:51 +0000 Subject: [PATCH 10/16] gdb_main: correct read register responses when the register is not available MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When reading registers, the stub may also return a string of literal ‘x’’s in place of the register data digits, to indicate that the corresponding register’s value is unavailable. This indicates that the stub has no means to access the register contents, even though the corresponding register is known to exist. If a register truly does not exist on the target, then it is better to not include it in the target description in the first place. --- src/gdb_main.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/src/gdb_main.c b/src/gdb_main.c index 4dbc81534a5..a661d18f98a 100644 --- a/src/gdb_main.c +++ b/src/gdb_main.c @@ -130,7 +130,14 @@ int32_t gdb_main_loop(target_controller_s *const tc, const gdb_packet_s *const p target_regs_read(cur_target, gp_regs); gdb_put_packet_hex(gp_regs, reg_size); } else { - gdb_put_packet_str("00"); + /** + * Register data is unavailable + * See: https://sourceware.org/gdb/current/onlinedocs/gdb.html/Packets.html#read-registers-packet + * + * ... the stub may also return a string of literal ‘x’ in place of the register data digits, + * to indicate that the corresponding register’s value is unavailable. + */ + gdb_put_packet_str("xx"); } break; } @@ -248,7 +255,14 @@ int32_t gdb_main_loop(target_controller_s *const tc, const gdb_packet_s *const p gdb_put_packet_error(0xffU); } } else { - gdb_put_packet_str("00"); + /** + * Register data is unavailable + * See: https://sourceware.org/gdb/current/onlinedocs/gdb.html/Packets.html#read-registers-packet + * + * ... the stub may also return a string of literal ‘x’ in place of the register data digits, + * to indicate that the corresponding register’s value is unavailable. + */ + gdb_put_packet_str("xx"); } break; } From 9ce38606314830ef18aac157520137408872da81 Mon Sep 17 00:00:00 2001 From: Rafael Silva Date: Wed, 11 Dec 2024 23:09:15 +0000 Subject: [PATCH 11/16] gdb_main: use static char arrays in tandem with ARRAY_LENGTH This removes the need for strlen --- src/gdb_main.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/gdb_main.c b/src/gdb_main.c index a661d18f98a..ea1ed26711a 100644 --- a/src/gdb_main.c +++ b/src/gdb_main.c @@ -426,8 +426,8 @@ static void exec_q_rcmd(const char *packet, const size_t length) else if (result == 0) gdb_put_packet_ok(); else { - const char *const response = "Failed\n"; - gdb_put_packet_hex(response, strlen(response)); + static const char response[] = "Failed\n"; + gdb_put_packet_hex(response, ARRAY_LENGTH(response)); } } @@ -863,8 +863,8 @@ static void handle_v_packet(const gdb_packet_s *const packet) * The vMustReplyEmpty is used as a feature test to check how gdbserver handles * unknown packets, don't print an error message for it. */ - static const char *const must_reply_empty = "vMustReplyEmpty"; - if (strncmp(packet->data, must_reply_empty, strlen(must_reply_empty)) != 0) + static const char must_reply_empty[] = "vMustReplyEmpty"; + if (strncmp(packet->data, must_reply_empty, ARRAY_LENGTH(must_reply_empty) - 1U) != 0) DEBUG_GDB("*** Unsupported packet: %s\n", packet->data); #endif gdb_put_packet_empty(); From dc6b37bbd920c2659d7a375dd8ab4be9d83826a3 Mon Sep 17 00:00:00 2001 From: Rafael Silva Date: Fri, 13 Dec 2024 15:13:46 +0000 Subject: [PATCH 12/16] hex_utils: remote automatic NUL termination of hexify This assumed the buffer was large enough to account for the NUL byte, this was an unsafe assumption, we shall limit writes to the size explicitly given to us Note, none of the current usages of this function required it to handle NUL termination --- src/hex_utils.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/hex_utils.c b/src/hex_utils.c index f482a916588..fe60c42155b 100644 --- a/src/hex_utils.c +++ b/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; } From bb553fedf21953b384d1584762bdf743881e9338 Mon Sep 17 00:00:00 2001 From: Rafael Silva Date: Fri, 13 Dec 2024 15:15:21 +0000 Subject: [PATCH 13/16] general: remove vasprintf declaration The last usage of vasprintf as been removed thus this is no longer required --- src/include/general.h | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/src/include/general.h b/src/include/general.h index 5a2f2d421da..34278e949c5 100644 --- a/src/include/general.h +++ b/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 From 6a90ad4cb225bc4b274015616ff7a7044c25bb4f Mon Sep 17 00:00:00 2001 From: Rafael Silva Date: Fri, 13 Dec 2024 15:15:50 +0000 Subject: [PATCH 14/16] stdio_newlib: map vsnprintf to the integer only implementation --- src/include/stdio_newlib.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/include/stdio_newlib.h b/src/include/stdio_newlib.h index 095d154e1f0..36eb963aec0 100644 --- a/src/include/stdio_newlib.h +++ b/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 */ From 568df9a7dd5e3f1aeed195ebe35e88aec82680f3 Mon Sep 17 00:00:00 2001 From: Rafael Silva Date: Wed, 11 Dec 2024 23:18:03 +0000 Subject: [PATCH 15/16] gdb_packet: add warning about string truncation on gdb_out --- src/include/gdb_packet.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/include/gdb_packet.h b/src/include/gdb_packet.h index 69ff65396f4..7046cfe86c7 100644 --- a/src/include/gdb_packet.h +++ b/src/include/gdb_packet.h @@ -138,6 +138,11 @@ 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; From 8bcdc4892ed9581e8eccfb34ee3ef6d898dfb69a Mon Sep 17 00:00:00 2001 From: Rafael Silva Date: Fri, 13 Dec 2024 15:18:54 +0000 Subject: [PATCH 16/16] gdb_main: document PacketSize GDB remote feature format --- src/gdb_main.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/gdb_main.c b/src/gdb_main.c index ea1ed26711a..dc145247cc9 100644 --- a/src/gdb_main.c +++ b/src/gdb_main.c @@ -467,7 +467,12 @@ static void exec_q_supported(const char *packet, const size_t length) */ gdb_set_noackmode(false); - gdb_putpacket_str_f("PacketSize=%X;qXfer:memory-map:read+;qXfer:features:read+;" + /* + * The Remote Protocol documentation is not clear on what format the PacketSize feature should be in, + * according to the GDB source code (as of version 15.2) it should be a hexadecimal encoded number + * to be parsed by strtoul() with a base of 16. + */ + gdb_putpacket_str_f("PacketSize=%x;qXfer:memory-map:read+;qXfer:features:read+;" "vContSupported+" GDB_QSUPPORTED_NOACKMODE, GDB_PACKET_BUFFER_SIZE); }