https://github.com/dlgus8648 updated https://github.com/llvm/llvm-project/pull/202556
>From 3fc3e8f91128ad4f907e7f53ff41a50076c8c48e Mon Sep 17 00:00:00 2001 From: KIMRIHYEON <[email protected]> Date: Tue, 9 Jun 2026 18:07:43 +0900 Subject: [PATCH 1/5] [lldb] Ignore async notification packets while waiting for a response OpenOCD's gdbserver sends an async notification packet ("%oocd_keepalive:XX#cc") roughly every 500ms while a long memory read/write is in progress. WaitForPacketNoLock() treated any non-invalid packet returned by CheckForPacket() -- including a PacketType::Notify -- as the response to the pending request, so a keepalive arriving during a memory write produced: unexpected response to GDB server memory write packet 'M2ffff8,4:54430000': 'oocd_keepalive:00' GDB has silently dropped unknown notifications received while waiting for a packet since 7.0 (2009). Match that behaviour: drop notification packets and keep waiting for the actual response. The LLDB client does not otherwise consume '%' notifications, so this is safe. Adds a unit test covering single and multiple notifications preceding the response. Fixes #197944. Assisted-by: Claude Code (Anthropic) --- .../gdb-remote/GDBRemoteCommunication.cpp | 24 +++++++++++++++++-- .../gdb-remote/GDBRemoteCommunicationTest.cpp | 19 +++++++++++++++ 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp index 04f486882e2c2..56d1d8bc09fb1 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp @@ -242,7 +242,17 @@ GDBRemoteCommunication::WaitForPacketNoLock(StringExtractorGDBRemote &packet, Log *log = GetLog(GDBRLog::Packets); // Check for a packet from our cache first without trying any reading... - if (CheckForPacket(nullptr, 0, packet) != PacketType::Invalid) + // Async notification packets (e.g. OpenOCD's "oocd_keepalive", sent during + // long memory operations) are not responses to our request. GDB silently + // drops such notifications while waiting for a packet; do the same and keep + // looking for the actual response. + PacketType packet_type = CheckForPacket(nullptr, 0, packet); + while (packet_type == PacketType::Notify) { + LLDB_LOGF(log, "GDBRemoteCommunication::%s ignoring notification packet", + __FUNCTION__); + packet_type = CheckForPacket(nullptr, 0, packet); + } + if (packet_type != PacketType::Invalid) return PacketResult::Success; bool timed_out = false; @@ -258,7 +268,17 @@ GDBRemoteCommunication::WaitForPacketNoLock(StringExtractorGDBRemote &packet, error, bytes_read); if (bytes_read > 0) { - if (CheckForPacket(buffer, bytes_read, packet) != PacketType::Invalid) + // Drop any async notification packets (see above) and keep waiting for + // the actual response. Once the freshly-read bytes have been consumed, + // re-check the cache for any further buffered packets. + packet_type = CheckForPacket(buffer, bytes_read, packet); + while (packet_type == PacketType::Notify) { + LLDB_LOGF(log, + "GDBRemoteCommunication::%s ignoring notification packet", + __FUNCTION__); + packet_type = CheckForPacket(nullptr, 0, packet); + } + if (packet_type != PacketType::Invalid) return PacketResult::Success; } else { switch (status) { diff --git a/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp b/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp index e96d587b10e25..2491ace98565b 100644 --- a/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp +++ b/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp @@ -75,6 +75,25 @@ TEST_F(GDBRemoteCommunicationTest, ReadPacket) { } } +// Test that async notification packets received while waiting for a response +// are silently dropped and that we keep looking for the actual response. +// OpenOCD sends a "%oocd_keepalive:XX#cc" notification during long memory +// operations; like GDB (since 7.0), LLDB must ignore it rather than mistake it +// for the response. See https://github.com/llvm/llvm-project/issues/197944. +TEST_F(GDBRemoteCommunicationTest, ReadPacketIgnoresNotifications) { + StringExtractorGDBRemote response; + + // A single notification ahead of the response. + ASSERT_TRUE(Write("%oocd_keepalive:00#54$OK#9a")); + ASSERT_EQ(PacketResult::Success, client.ReadPacket(response)); + EXPECT_EQ("OK", response.GetStringRef()); + + // Several notifications ahead of the response. + ASSERT_TRUE(Write("%oocd_keepalive:01#55%oocd_keepalive:02#56$OK#9a")); + ASSERT_EQ(PacketResult::Success, client.ReadPacket(response)); + EXPECT_EQ("OK", response.GetStringRef()); +} + // Test that packets with incorrect RLE sequences do not cause a crash and // reported as invalid. TEST_F(GDBRemoteCommunicationTest, CheckForPacket) { >From 72b80166d1359ab60a49e4662946b36e2a0d723d Mon Sep 17 00:00:00 2001 From: KIMRIHYEON <[email protected]> Date: Tue, 9 Jun 2026 20:45:13 +0900 Subject: [PATCH 2/5] Address review comments - Add a unit test asserting ReadPacket fails (times out) when only a notification packet is received, rather than returning it. - Drop a redundant sentence from the notification-handling comment. --- .../Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp | 3 +-- .../Process/gdb-remote/GDBRemoteCommunicationTest.cpp | 5 +++++ 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp index 56d1d8bc09fb1..6228095c3763a 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp @@ -269,8 +269,7 @@ GDBRemoteCommunication::WaitForPacketNoLock(StringExtractorGDBRemote &packet, if (bytes_read > 0) { // Drop any async notification packets (see above) and keep waiting for - // the actual response. Once the freshly-read bytes have been consumed, - // re-check the cache for any further buffered packets. + // the actual response. packet_type = CheckForPacket(buffer, bytes_read, packet); while (packet_type == PacketType::Notify) { LLDB_LOGF(log, diff --git a/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp b/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp index 2491ace98565b..b7bf541fdb7f8 100644 --- a/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp +++ b/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp @@ -92,6 +92,11 @@ TEST_F(GDBRemoteCommunicationTest, ReadPacketIgnoresNotifications) { ASSERT_TRUE(Write("%oocd_keepalive:01#55%oocd_keepalive:02#56$OK#9a")); ASSERT_EQ(PacketResult::Success, client.ReadPacket(response)); EXPECT_EQ("OK", response.GetStringRef()); + + // A notification with no response following it is dropped, and the read + // fails (times out) rather than returning the notification. + ASSERT_TRUE(Write("%oocd_keepalive:03#57")); + EXPECT_EQ(PacketResult::ErrorReplyTimeout, client.ReadPacket(response)); } // Test that packets with incorrect RLE sequences do not cause a crash and >From 6a1840630acd7f2bd12ae658a0e97ec98ffd35f8 Mon Sep 17 00:00:00 2001 From: KIMRIHYEON <[email protected]> Date: Thu, 11 Jun 2026 13:35:41 +0900 Subject: [PATCH 3/5] Add a test for a notification arriving in a separate read Cover the case raised in review where the async notification and the actual response do not arrive in the same read: send the notification on its own, then send the response from another thread after a short delay so it lands in a later read once the client is already waiting again. Verifies that the client drops the notification and keeps reading for the real response instead of giving up once the receive buffer briefly drains. Assisted-by: Claude Code (Anthropic) --- .../gdb-remote/GDBRemoteCommunicationTest.cpp | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp b/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp index b7bf541fdb7f8..094ead3881709 100644 --- a/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp +++ b/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp @@ -10,6 +10,9 @@ #include "lldb/Host/ConnectionFileDescriptor.h" #include "llvm/Testing/Support/Error.h" +#include <chrono> +#include <thread> + using namespace lldb_private::process_gdb_remote; using namespace lldb_private; using namespace lldb; @@ -99,6 +102,32 @@ TEST_F(GDBRemoteCommunicationTest, ReadPacketIgnoresNotifications) { EXPECT_EQ(PacketResult::ErrorReplyTimeout, client.ReadPacket(response)); } +// Test the case where the notification and the actual response do NOT arrive +// together in a single read: the notification is sent first, and the response +// follows only after the client has already consumed the notification and gone +// back to waiting. The notification must still be dropped and the client must +// keep reading until the real response arrives, rather than giving up once the +// receive buffer briefly drains. +TEST_F(GDBRemoteCommunicationTest, ReadPacketIgnoresNotificationsAcrossReads) { + StringExtractorGDBRemote response; + + // Send the notification on its own. The client should read it, drop it, and + // block on the next read with the buffer empty. + ASSERT_TRUE(Write("%oocd_keepalive:00#54")); + + // Send the response from another thread after a short delay, so it lands in + // a separate read once the client is already waiting again. + std::thread responder([this] { + std::this_thread::sleep_for(std::chrono::milliseconds(200)); + Write("$OK#9a"); + }); + + EXPECT_EQ(PacketResult::Success, client.ReadPacket(response)); + EXPECT_EQ("OK", response.GetStringRef()); + + responder.join(); +} + // Test that packets with incorrect RLE sequences do not cause a crash and // reported as invalid. TEST_F(GDBRemoteCommunicationTest, CheckForPacket) { >From 07c2a22ac5dc5b925eacedcf52f6854f4cabc176 Mon Sep 17 00:00:00 2001 From: KIMRIHYEON <[email protected]> Date: Mon, 15 Jun 2026 14:43:36 +0900 Subject: [PATCH 4/5] Document the eConnectionStatusSuccess case in WaitForPacketNoLock Explain why a zero-byte read with a success status is not treated as a failure: it can happen on a non-socket connection when the read returns EAGAIN, and we must keep looping for the actual response (e.g. after dropping async notification packets). Requested in review. Assisted-by: Claude Code (Anthropic) --- .../Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp index 6228095c3763a..94506da0345a8 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp @@ -376,6 +376,11 @@ GDBRemoteCommunication::WaitForPacketNoLock(StringExtractorGDBRemote &packet, } break; case eConnectionStatusSuccess: + // Read() can return zero bytes with a success status (e.g. a spurious + // readable wakeup that yields EAGAIN on a non-socket connection). That + // is neither EOF nor an error, so keep looping -- we may still be + // waiting for the actual response, for instance after dropping the + // async notification packets handled above. // printf ("status = success but error = %s\n", // error.AsCString("<invalid>")); break; >From a619fc1f074e798475b4138d3e8d3ed1bb2a8623 Mon Sep 17 00:00:00 2001 From: KIMRIHYEON <[email protected]> Date: Mon, 15 Jun 2026 22:24:52 +0900 Subject: [PATCH 5/5] Extract notification-draining loop into GetNextNonNotifyPacket The loop that drops async notification packets while looking for the next real packet was duplicated at the two CheckForPacket() call sites in WaitForPacketNoLock(). Factor it into a GetNextNonNotifyPacket() helper so the behaviour -- and the comment explaining it -- lives in one place. No functional change. Requested in review. Assisted-by: Claude Code (Anthropic) --- .../gdb-remote/GDBRemoteCommunication.cpp | 42 +++++++++---------- .../gdb-remote/GDBRemoteCommunication.h | 5 +++ 2 files changed, 26 insertions(+), 21 deletions(-) diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp index 94506da0345a8..6a06de99c2b54 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp @@ -232,6 +232,24 @@ GDBRemoteCommunication::ReadPacket(StringExtractorGDBRemote &response, } } +GDBRemoteCommunication::PacketType +GDBRemoteCommunication::GetNextNonNotifyPacket( + const uint8_t *src, size_t src_len, StringExtractorGDBRemote &packet) { + // Async notification packets (e.g. OpenOCD's "oocd_keepalive", sent during + // long memory operations) are not responses to our request. GDB silently + // drops unknown notifications while waiting for a packet; do the same and + // keep looking for the actual response. After the freshly-read bytes are + // consumed, drain any further buffered packets from the cache. + Log *log = GetLog(GDBRLog::Packets); + PacketType packet_type = CheckForPacket(src, src_len, packet); + while (packet_type == PacketType::Notify) { + LLDB_LOGF(log, "GDBRemoteCommunication::%s ignoring notification packet", + __FUNCTION__); + packet_type = CheckForPacket(nullptr, 0, packet); + } + return packet_type; +} + GDBRemoteCommunication::PacketResult GDBRemoteCommunication::WaitForPacketNoLock(StringExtractorGDBRemote &packet, Timeout<std::micro> timeout, @@ -242,17 +260,7 @@ GDBRemoteCommunication::WaitForPacketNoLock(StringExtractorGDBRemote &packet, Log *log = GetLog(GDBRLog::Packets); // Check for a packet from our cache first without trying any reading... - // Async notification packets (e.g. OpenOCD's "oocd_keepalive", sent during - // long memory operations) are not responses to our request. GDB silently - // drops such notifications while waiting for a packet; do the same and keep - // looking for the actual response. - PacketType packet_type = CheckForPacket(nullptr, 0, packet); - while (packet_type == PacketType::Notify) { - LLDB_LOGF(log, "GDBRemoteCommunication::%s ignoring notification packet", - __FUNCTION__); - packet_type = CheckForPacket(nullptr, 0, packet); - } - if (packet_type != PacketType::Invalid) + if (GetNextNonNotifyPacket(nullptr, 0, packet) != PacketType::Invalid) return PacketResult::Success; bool timed_out = false; @@ -268,16 +276,8 @@ GDBRemoteCommunication::WaitForPacketNoLock(StringExtractorGDBRemote &packet, error, bytes_read); if (bytes_read > 0) { - // Drop any async notification packets (see above) and keep waiting for - // the actual response. - packet_type = CheckForPacket(buffer, bytes_read, packet); - while (packet_type == PacketType::Notify) { - LLDB_LOGF(log, - "GDBRemoteCommunication::%s ignoring notification packet", - __FUNCTION__); - packet_type = CheckForPacket(nullptr, 0, packet); - } - if (packet_type != PacketType::Invalid) + if (GetNextNonNotifyPacket(buffer, bytes_read, packet) != + PacketType::Invalid) return PacketResult::Success; } else { switch (status) { diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h index 35bf5eb2e3f0d..b1c235891f465 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h @@ -119,6 +119,11 @@ class GDBRemoteCommunication : public Communication { PacketType CheckForPacket(const uint8_t *src, size_t src_len, StringExtractorGDBRemote &packet); + // Like CheckForPacket, but silently drops any async notification packets and + // returns the type of the next non-notification packet. See the definition. + PacketType GetNextNonNotifyPacket(const uint8_t *src, size_t src_len, + StringExtractorGDBRemote &packet); + bool GetSendAcks() { return m_send_acks; } // Set the global packet timeout. _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
