https://github.com/Timmmm updated https://github.com/llvm/llvm-project/pull/204788
>From d5f05d91caf14b146276fdaff0022c8ba0e2189e Mon Sep 17 00:00:00 2001 From: Tim Hutt <[email protected]> Date: Mon, 18 May 2026 10:22:54 +0100 Subject: [PATCH] [lldb] Ignore notification packets The gdb-protocol spec says > Recipients should silently ignore corrupted notifications and notifications > they do not understand. This changes `WaitForPacketNoLock` so that it ignores all notifications. An example of a notification that causes issues without this change is that OpenOCD sends `%ookeepalive:00` notifications during memory accesses. --- .../Python/lldbsuite/test/gdbclientutils.py | 15 ++-- .../gdb-remote/GDBRemoteCommunication.cpp | 42 +++++++++++- .../gdb-remote/GDBRemoteCommunication.h | 3 + .../TestIgnoringNotifications.py | 68 +++++++++++++++++++ 4 files changed, 120 insertions(+), 8 deletions(-) create mode 100644 lldb/test/API/functionalities/gdb_remote_client/TestIgnoringNotifications.py diff --git a/lldb/packages/Python/lldbsuite/test/gdbclientutils.py b/lldb/packages/Python/lldbsuite/test/gdbclientutils.py index 4c40299f3256d..b789452bac174 100644 --- a/lldb/packages/Python/lldbsuite/test/gdbclientutils.py +++ b/lldb/packages/Python/lldbsuite/test/gdbclientutils.py @@ -22,15 +22,18 @@ def checksum(message): return check % 256 -def frame_packet(message): +def frame_packet(message, prefix): """ Create a framed packet that's ready to send over the GDB connection channel. - Framing includes surrounding the message between $ and #, and appending - a two character hex checksum. + Framing means: + * Attaching the prefix. Which is usually '$' but for notifications will be + '%'. + * Appending a '#'. + * Adding a two character hex checksum. """ - return "$%s#%02x" % (message, checksum(message)) + return "%s%s#%02x" % (prefix, message, checksum(message)) def escape_binary(message): @@ -694,9 +697,9 @@ def _parsePacket(self): self._receivedDataOffset = 0 return packet - def _sendPacket(self, packet: str): + def _sendPacket(self, packet: str, prefix="$"): assert self._socket is not None - framed_packet = seven.bitcast_to_bytes(frame_packet(packet)) + framed_packet = seven.bitcast_to_bytes(frame_packet(packet, prefix)) self._socket.sendall(framed_packet) def _handlePacket(self, packet): diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp index 04f486882e2c2..38233f2eed3d6 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp @@ -242,8 +242,15 @@ 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) + switch (CheckForPacketIgnoreNotifications(nullptr, 0, packet)) { + case PacketType::Standard: return PacketResult::Success; + case PacketType::Invalid: + break; + case PacketType::Notify: + // These are ignored by CheckForPacketIgnoreNotifications. + llvm_unreachable("unreachable"); + } bool timed_out = false; bool disconnected = false; @@ -258,8 +265,15 @@ GDBRemoteCommunication::WaitForPacketNoLock(StringExtractorGDBRemote &packet, error, bytes_read); if (bytes_read > 0) { - if (CheckForPacket(buffer, bytes_read, packet) != PacketType::Invalid) + switch (CheckForPacketIgnoreNotifications(buffer, bytes_read, packet)) { + case PacketType::Standard: return PacketResult::Success; + case PacketType::Invalid: + break; + case PacketType::Notify: + // These are ignored by CheckForPacketIgnoreNotifications. + llvm_unreachable("unreachable"); + } } else { switch (status) { case eConnectionStatusTimedOut: @@ -839,6 +853,30 @@ GDBRemoteCommunication::CheckForPacket(const uint8_t *src, size_t src_len, return GDBRemoteCommunication::PacketType::Invalid; } +GDBRemoteCommunication::PacketType +GDBRemoteCommunication::CheckForPacketIgnoreNotifications( + const uint8_t *src, size_t src_len, StringExtractorGDBRemote &packet) { + for (;;) { + switch (CheckForPacket(src, src_len, packet)) { + case PacketType::Standard: + return PacketType::Standard; + case PacketType::Invalid: + // Either an incomplete packet or an invalid one that was removed. + // There may be more data in the buffer that contains valid + // packets but CheckForPacket does not distinguish these cases. + return PacketType::Invalid; + case PacketType::Notify: + // No notifications are currently supported so they should be ignored. + // There may be more packets in the buffer so go back to check. + + // Set src_len=0 so that it doesn't add the + // data we read to the internal buffer again. + src_len = 0; + break; + } + } +} + Status GDBRemoteCommunication::StartDebugserverProcess( std::variant<llvm::StringRef, shared_fd_t> comm, ProcessLaunchInfo &launch_info, const Args *inferior_args) { diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h index 35bf5eb2e3f0d..4b51cb3900e94 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h @@ -119,6 +119,9 @@ class GDBRemoteCommunication : public Communication { PacketType CheckForPacket(const uint8_t *src, size_t src_len, StringExtractorGDBRemote &packet); + PacketType CheckForPacketIgnoreNotifications(const uint8_t *src, size_t src_len, + StringExtractorGDBRemote &packet); + bool GetSendAcks() { return m_send_acks; } // Set the global packet timeout. diff --git a/lldb/test/API/functionalities/gdb_remote_client/TestIgnoringNotifications.py b/lldb/test/API/functionalities/gdb_remote_client/TestIgnoringNotifications.py new file mode 100644 index 0000000000000..acf44f5aa967e --- /dev/null +++ b/lldb/test/API/functionalities/gdb_remote_client/TestIgnoringNotifications.py @@ -0,0 +1,68 @@ +""" +Test that LLDB ignores notification packets (those beginning with '%') +when waiting for a response to a '$' packet. + +OpenOCD is one debug server that uses notification packets to reset +the connection timeout if a memory read takes too long. So that's what +we test here, but it should apply to any exchange. +""" + +import lldb +from lldbsuite.test.lldbtest import * +from lldbsuite.test.decorators import * +from lldbsuite.test.gdbclientutils import * +from lldbsuite.test.lldbgdbclient import GDBRemoteTestBase + + +class NotifyingServerResponder(MockGDBServerResponder): + def readMemory(self, addr, length): + return "01" * length + + +class NotifyingServer(MockGDBServer): + def __init__(self, socket): + self._socket = socket + self.responder = NotifyingServerResponder() + self.sent_notification = False + + def _sendPacket(self, packet: str, prefix="$"): + # In theory we could add notifies before all packets, but it always + # goes through the same code and just makes the test take longer. + if packet == "01010101": + # Send more than one to make sure we clear them all to find + # the real response. + super()._sendPacket("this_is_a_notification", prefix="%") + super()._sendPacket("this_is_a_2nd_notification", prefix="%") + self.sent_notification = True + + super()._sendPacket(packet) + + +class TestIgnoringNotifications(GDBRemoteTestBase): + def setUp(self): + TestBase.setUp(self) + self.server = NotifyingServer(self.server_socket_class()) + self.server.start() + + @skipIfLLVMTargetMissing("AArch64") + def test(self): + target = self.createTarget("basic_eh_frame-aarch64.yaml") + + if self.TraceOn(): + self.runCmd("log enable gdb-remote packets") + self.runCmd("log enable gdb-remote comm") + self.addTearDownHook(lambda: self.runCmd("log disable gdb-remote packets")) + + process = self.connect(target) + lldbutil.expect_state_changes( + self, self.dbg.GetListener(), process, [lldb.eStateStopped] + ) + + # Disabling cache is not required but makes debugging this test easier. + self.runCmd("settings set target.process.disable-memory-cache true") + + # Should succeed despite getting a notification before the real result. + self.expect("memory read --format hex 0x1234 0x1238", substrs=["0x01010101"]) + + # Check we didn't succeed because lldb got memory in some other way. + self.assertTrue(self.server.sent_notification) _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
