From 588816fe6db1bb1bd2e1ba33929be2a7ae40a7e6 Mon Sep 17 00:00:00 2001 From: Rafael Silva Date: Mon, 20 Jan 2025 12:39:39 +0000 Subject: [PATCH 1/4] gdb_packet: add note on useful GDB debug command --- src/gdb_packet.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/gdb_packet.c b/src/gdb_packet.c index acc0051be3a..379be5091fa 100644 --- a/src/gdb_packet.c +++ b/src/gdb_packet.c @@ -81,6 +81,14 @@ void gdb_set_noackmode(bool enable) } #ifndef DEBUG_GDB_IS_NOOP +/* + * To debug packets from the perspective of GDB we can use the following command: + * set debug remote on + * This will print packets sent and received by the GDB client + * + * This is not directly related to BMD, but as it's hard to find this information + * and it is extremely useful for debugging, we included it here for reference. + */ static void gdb_packet_debug(const char *const func, const gdb_packet_s *const packet) { /* Log packet for debugging */ From abfe002eaae3ca63091f0658c01ce0e79780d8bf Mon Sep 17 00:00:00 2001 From: Rafael Silva Date: Mon, 20 Jan 2025 12:40:44 +0000 Subject: [PATCH 2/4] gdb_packet: expose packet ack send/receive functionality --- src/gdb_packet.c | 19 ++++++++++++------- src/include/gdb_packet.h | 3 +++ 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/src/gdb_packet.c b/src/gdb_packet.c index 379be5091fa..f018b5a3236 100644 --- a/src/gdb_packet.c +++ b/src/gdb_packet.c @@ -264,7 +264,7 @@ gdb_packet_s *gdb_packet_receive(void) /* (N)Acknowledge packet */ const bool checksum_ok = gdb_packet_checksum(packet) == rx_checksum; - gdb_if_putchar(checksum_ok ? GDB_PACKET_ACK : GDB_PACKET_NACK, true); + gdb_packet_ack(checksum_ok); if (!checksum_ok) { /* Checksum error, restart packet capture */ state = PACKET_IDLE; @@ -295,14 +295,19 @@ gdb_packet_s *gdb_packet_receive(void) } } -static inline bool gdb_get_ack(const uint32_t timeout) +void gdb_packet_ack(const bool ack) { - /* Return true early if NoAckMode is enabled */ - if (noackmode) - return true; + /* Send ACK/NACK */ + DEBUG_GDB("%s: %s\n", __func__, ack ? "ACK" : "NACK"); + gdb_if_putchar(ack ? GDB_PACKET_ACK : GDB_PACKET_NACK, true); +} +bool gdb_packet_get_ack(const uint32_t timeout) +{ /* Wait for ACK/NACK */ - return gdb_if_getchar_to(timeout) == GDB_PACKET_ACK; + const bool ack = gdb_if_getchar_to(timeout) == GDB_PACKET_ACK; + DEBUG_GDB("%s: %s\n", __func__, ack ? "ACK" : "NACK"); + return ack; } static inline void gdb_if_putchar_escaped(const char value) @@ -343,7 +348,7 @@ void gdb_packet_send(const gdb_packet_s *const packet) #endif /* Wait for ACK/NACK on standard packets */ - if (packet->notification || gdb_get_ack(2000U)) + if (packet->notification || noackmode || gdb_packet_get_ack(2000U)) break; } } diff --git a/src/include/gdb_packet.h b/src/include/gdb_packet.h index 5d38d2655b7..7c8d74ef084 100644 --- a/src/include/gdb_packet.h +++ b/src/include/gdb_packet.h @@ -75,6 +75,9 @@ void gdb_set_noackmode(bool enable); gdb_packet_s *gdb_packet_receive(void); void gdb_packet_send(const gdb_packet_s *packet); +void gdb_packet_ack(bool ack); +bool gdb_packet_get_ack(uint32_t timeout); + char *gdb_packet_buffer(void); /* Convenience wrappers */ From ca2584e681439019acc9fa5ee7f1ec809b6311c7 Mon Sep 17 00:00:00 2001 From: Rafael Silva Date: Mon, 20 Jan 2025 12:41:06 +0000 Subject: [PATCH 3/4] gdb_packet: expose noackmode internal state with getter --- src/gdb_packet.c | 5 +++++ src/include/gdb_packet.h | 1 + 2 files changed, 6 insertions(+) diff --git a/src/gdb_packet.c b/src/gdb_packet.c index f018b5a3236..55f2920cfbe 100644 --- a/src/gdb_packet.c +++ b/src/gdb_packet.c @@ -80,6 +80,11 @@ void gdb_set_noackmode(bool enable) noackmode = enable; } +bool gdb_noackmode(void) +{ + return noackmode; +} + #ifndef DEBUG_GDB_IS_NOOP /* * To debug packets from the perspective of GDB we can use the following command: diff --git a/src/include/gdb_packet.h b/src/include/gdb_packet.h index 7c8d74ef084..72f25898944 100644 --- a/src/include/gdb_packet.h +++ b/src/include/gdb_packet.h @@ -70,6 +70,7 @@ typedef struct gdb_packet { /* GDB packet transmission configuration */ void gdb_set_noackmode(bool enable); +bool gdb_noackmode(void); /* Raw GDB packet transmission */ gdb_packet_s *gdb_packet_receive(void); From f4e79b65d4dc8bee6df74348150da36d8e112794 Mon Sep 17 00:00:00 2001 From: Rafael Silva Date: Mon, 20 Jan 2025 12:42:42 +0000 Subject: [PATCH 4/4] gdb_main: refactor noackmode zombie state handling When a connection is terminated abruptly the noackmode state may be left in a "zombie" state, enabled, because we are unable to detect the connection was terminated. We previously used the qSupported packet to signal a new connection and reset the state, but this breaks compatibility with LLDb which does not send qSupported as the first packet. This attempts a new approach, by checking if a acknowledgement was sent in response to qSupported reply, we can detect when noackmode is enabled when it shouldn't. This also needs some special cases to acknowledge the first packets in a connection when noackmode is in a zombie state. --- src/gdb_main.c | 31 +++++++++++++++++++++++++++---- src/gdb_packet.c | 13 +------------ 2 files changed, 28 insertions(+), 16 deletions(-) diff --git a/src/gdb_main.c b/src/gdb_main.c index 0b3591fd7f8..9ad36976bde 100644 --- a/src/gdb_main.c +++ b/src/gdb_main.c @@ -335,7 +335,8 @@ int32_t gdb_main_loop(target_controller_s *const tc, const gdb_packet_s *const p } if (packet->data[0] == 'D') gdb_put_packet_ok(); - gdb_set_noackmode(false); + else /* packet->data[0] == '\x04' */ + gdb_set_noackmode(false); break; case 'k': /* Kill the target */ @@ -462,10 +463,15 @@ static void exec_q_supported(const char *packet, const size_t length) (void)length; /* - * This is the first packet sent by GDB, so we can reset the NoAckMode flag here in case - * the previous session was terminated abruptly with NoAckMode enabled + * This might be the first packet of a GDB connection, If NoAckMode is enabled it might + * be because the previous session was terminated abruptly, and we need to acknowledge + * the first packet of the new session. + * + * If NoAckMode was intentionally enabled before (e.g. LLDB enables NoAckMode first), + * the acknowledgment should be safely ignored. */ - gdb_set_noackmode(false); + if (gdb_noackmode()) + gdb_packet_ack(true); /* * The Remote Protocol documentation is not clear on what format the PacketSize feature should be in, @@ -475,6 +481,16 @@ static void exec_q_supported(const char *packet, const size_t length) gdb_putpacket_str_f("PacketSize=%x;qXfer:memory-map:read+;qXfer:features:read+;" "vContSupported+" GDB_QSUPPORTED_NOACKMODE, GDB_PACKET_BUFFER_SIZE); + + /* + * If an acknowledgement was received in response while in NoAckMode, then NoAckMode is probably + * not meant to be enabled and is likely a result of the previous session being terminated + * abruptly. Disable NoAckMode to prevent any further issues. + */ + if (gdb_noackmode() && gdb_packet_get_ack(100U)) { + DEBUG_GDB("Received acknowledgment in NoAckMode, likely result of a session being terminated abruptly\n"); + gdb_set_noackmode(false); + } } static void exec_q_memory_map(const char *packet, const size_t length) @@ -570,6 +586,13 @@ static void exec_q_noackmode(const char *packet, const size_t length) { (void)packet; (void)length; + /* + * This might be the first packet of a LLDB connection, If NoAckMode is enabled it might + * be because the previous session was terminated abruptly, and we need to acknowledge + * the first packet of the new session. + */ + if (gdb_noackmode()) + gdb_packet_ack(true); gdb_set_noackmode(true); gdb_put_packet_ok(); } diff --git a/src/gdb_packet.c b/src/gdb_packet.c index 55f2920cfbe..420ed3f3227 100644 --- a/src/gdb_packet.c +++ b/src/gdb_packet.c @@ -60,19 +60,8 @@ char *gdb_packet_buffer(void) #endif /* EXTERNAL_PACKET_BUFFER */ /* https://sourceware.org/gdb/onlinedocs/gdb/Packet-Acknowledgment.html */ -void gdb_set_noackmode(bool enable) +void gdb_set_noackmode(const bool enable) { - /* - * If we were asked to disable NoAckMode, and it was previously enabled, - * it might mean we got a packet we determined to be the first of a new - * GDB session, and as such it was not acknowledged (before GDB enabled NoAckMode), - * better late than never. - * - * 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, true); - /* Log only changes */ if (noackmode != enable) DEBUG_GDB("%s NoAckMode\n", enable ? "Enabling" : "Disabling");