[Lldb-commits] [lldb] [lldb] Ignore async notification packets while waiting for a response (PR #202556)
via lldb-commits
lldb-commits at lists.llvm.org
Sun Jun 14 22:44:12 PDT 2026
https://github.com/dlgus8648 updated https://github.com/llvm/llvm-project/pull/202556
>From 3fc3e8f91128ad4f907e7f53ff41a50076c8c48e Mon Sep 17 00:00:00 2001
From: KIMRIHYEON <dlgus8648 at naver.com>
Date: Tue, 9 Jun 2026 18:07:43 +0900
Subject: [PATCH 1/4] [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 <dlgus8648 at naver.com>
Date: Tue, 9 Jun 2026 20:45:13 +0900
Subject: [PATCH 2/4] 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 <dlgus8648 at naver.com>
Date: Thu, 11 Jun 2026 13:35:41 +0900
Subject: [PATCH 3/4] 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 <dlgus8648 at naver.com>
Date: Mon, 15 Jun 2026 14:43:36 +0900
Subject: [PATCH 4/4] 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;
More information about the lldb-commits
mailing list