From cf50ab74d8aa720875d2fe6da93f45d8c4b3c519 Mon Sep 17 00:00:00 2001 From: Daniel Sanders Date: Tue, 21 Oct 2025 01:38:24 -0700 Subject: [PATCH 1/2] [lldb] Add bidirectional packetLog to gdbclientutils.py (#162176) While debugging the tests for #155000 I found it helpful to have both sides of the simulated gdb-rsp traffic rather than just the responses so I've extended the packetLog in MockGDBServerResponder to record traffic in both directions. Tests have been updated accordingly (cherry picked from commit f188c97cc193773071b0b61ebf214705fb84189e) --- .../Python/lldbsuite/test/gdbclientutils.py | 65 ++++++++++++++++--- .../Python/lldbsuite/test/lldbgdbclient.py | 18 ++--- .../gdb_remote_client/TestContinue.py | 6 +- .../gdb_remote_client/TestGDBRemoteClient.py | 52 ++++++--------- .../gdb_remote_client/TestGDBRemoteLoad.py | 4 +- .../TestGDBRemotePlatformFile.py | 22 +++---- 6 files changed, 102 insertions(+), 65 deletions(-) diff --git a/lldb/packages/Python/lldbsuite/test/gdbclientutils.py b/lldb/packages/Python/lldbsuite/test/gdbclientutils.py index b603c35c8df09..343a2627846d7 100644 --- a/lldb/packages/Python/lldbsuite/test/gdbclientutils.py +++ b/lldb/packages/Python/lldbsuite/test/gdbclientutils.py @@ -4,7 +4,9 @@ import threading import socket import traceback +from enum import Enum from lldbsuite.support import seven +from typing import Optional, List, Tuple, Union, Sequence def checksum(message): @@ -74,6 +76,35 @@ def hex_decode_bytes(hex_bytes): return out +class PacketDirection(Enum): + RECV = "recv" + SEND = "send" + + +class PacketLog: + def __init__(self): + self._packets: list[tuple[PacketDirection, str]] = [] + + def add_sent(self, packet: str): + self._packets.append((PacketDirection.SEND, packet)) + + def add_received(self, packet: str): + self._packets.append((PacketDirection.RECV, packet)) + + def get_sent(self): + return [ + pkt for direction, pkt in self._packets if direction == PacketDirection.SEND + ] + + def get_received(self): + return [ + pkt for direction, pkt in self._packets if direction == PacketDirection.RECV + ] + + def __iter__(self): + return iter(self._packets) + + class MockGDBServerResponder: """ A base class for handling client packets and issuing server responses for @@ -89,21 +120,33 @@ class MockGDBServerResponder: registerCount = 40 packetLog = None - class RESPONSE_DISCONNECT: - pass + class SpecialResponse(Enum): + RESPONSE_DISCONNECT = 0 + RESPONSE_NONE = 1 - class RESPONSE_NONE: - pass + RESPONSE_DISCONNECT = SpecialResponse.RESPONSE_DISCONNECT + RESPONSE_NONE = SpecialResponse.RESPONSE_NONE + Response = Union[str, SpecialResponse] def __init__(self): - self.packetLog = [] + self.packetLog = PacketLog() - def respond(self, packet): + def respond(self, packet: str) -> Sequence[Response]: """ Return the unframed packet data that the server should issue in response to the given packet received from the client. """ - self.packetLog.append(packet) + self.packetLog.add_received(packet) + response = self._respond_impl(packet) + if not isinstance(response, list): + response = [response] + for part in response: + if isinstance(part, self.SpecialResponse): + continue + self.packetLog.add_sent(part) + return response + + def _respond_impl(self, packet) -> Union[Response, List[Response]]: if packet is MockGDBServer.PACKET_INTERRUPT: return self.interrupt() if packet == "c": @@ -649,17 +692,19 @@ def _handlePacket(self, packet): # adding validation code to make sure the client only sends ACKs # when it's supposed to. return - response = "" + response = [""] # We'll handle the ack stuff here since it's not something any of the # tests will be concerned about, and it'll get turned off quickly anyway. if self._shouldSendAck: self._socket.sendall(seven.bitcast_to_bytes("+")) if packet == "QStartNoAckMode": self._shouldSendAck = False - response = "OK" + response = ["OK"] elif self.responder is not None: # Delegate everything else to our responder response = self.responder.respond(packet) + # MockGDBServerResponder no longer returns non-lists but others like + # ReverseTestBase still do if not isinstance(response, list): response = [response] for part in response: @@ -667,6 +712,8 @@ def _handlePacket(self, packet): continue if part is MockGDBServerResponder.RESPONSE_DISCONNECT: raise self.TerminateConnectionException() + # Should have handled the non-str's above + assert isinstance(part, str) self._sendPacket(part) PACKET_ACK = object() diff --git a/lldb/packages/Python/lldbsuite/test/lldbgdbclient.py b/lldb/packages/Python/lldbsuite/test/lldbgdbclient.py index 599f7878e6edb..9b2a89e934132 100644 --- a/lldb/packages/Python/lldbsuite/test/lldbgdbclient.py +++ b/lldb/packages/Python/lldbsuite/test/lldbgdbclient.py @@ -10,7 +10,7 @@ class GDBRemoteTestBase(TestBase): Base class for GDB client tests. This class will setup and start a mock GDB server for the test to use. - It also provides assertPacketLogContains, which simplifies the checking + It also provides assertPacketLogReceived, which simplifies the checking of packets sent by the client. """ @@ -60,30 +60,32 @@ def connect(self, target, plugin="gdb-remote"): self.assertTrue(process, PROCESS_IS_VALID) return process - def assertPacketLogContains(self, packets, log=None): + def assertPacketLogReceived(self, packets, log: PacketLog = None): """ - Assert that the mock server's packet log contains the given packets. + Assert that the mock server's packet log received the given packets. The packet log includes all packets sent by the client and received - by the server. This fuction makes it easy to verify that the client + by the server. This function makes it easy to verify that the client sent the expected packets to the server. The check does not require that the packets be consecutive, but does require that they are ordered in the log as they ordered in the arg. """ if log is None: - log = self.server.responder.packetLog + received = self.server.responder.packetLog.get_received() + else: + received = log.get_received() i = 0 j = 0 - while i < len(packets) and j < len(log): - if log[j] == packets[i]: + while i < len(packets) and j < len(received): + if received[j] == packets[i]: i += 1 j += 1 if i < len(packets): self.fail( "Did not receive: %s\nLast 10 packets:\n\t%s" - % (packets[i], "\n\t".join(log)) + % (packets[i], "\n\t".join(received)) ) diff --git a/lldb/test/API/functionalities/gdb_remote_client/TestContinue.py b/lldb/test/API/functionalities/gdb_remote_client/TestContinue.py index 3af4ca859f86e..67f0783167a32 100644 --- a/lldb/test/API/functionalities/gdb_remote_client/TestContinue.py +++ b/lldb/test/API/functionalities/gdb_remote_client/TestContinue.py @@ -41,7 +41,7 @@ def qfThreadInfo(self): lldbutil.expect_state_changes( self, self.dbg.GetListener(), process, [lldb.eStateExited] ) - self.assertPacketLogContains(["vCont;C13:401"]) + self.assertPacketLogReceived(["vCont;C13:401"]) def test_continue_no_vCont(self): class MyResponder(self.BaseResponder): @@ -61,7 +61,7 @@ def other(self, packet): lldbutil.expect_state_changes( self, self.dbg.GetListener(), process, [lldb.eStateExited] ) - self.assertPacketLogContains(["Hc401", "C13"]) + self.assertPacketLogReceived(["Hc401", "C13"]) def test_continue_multiprocess(self): class MyResponder(self.BaseResponder): @@ -74,4 +74,4 @@ class MyResponder(self.BaseResponder): lldbutil.expect_state_changes( self, self.dbg.GetListener(), process, [lldb.eStateExited] ) - self.assertPacketLogContains(["vCont;C13:p400.401"]) + self.assertPacketLogReceived(["vCont;C13:p400.401"]) diff --git a/lldb/test/API/functionalities/gdb_remote_client/TestGDBRemoteClient.py b/lldb/test/API/functionalities/gdb_remote_client/TestGDBRemoteClient.py index 12b464d3397eb..e6ba673ab2869 100644 --- a/lldb/test/API/functionalities/gdb_remote_client/TestGDBRemoteClient.py +++ b/lldb/test/API/functionalities/gdb_remote_client/TestGDBRemoteClient.py @@ -36,7 +36,7 @@ def test_connect(self): """Test connecting to a remote gdb server""" target = self.createTarget("a.yaml") process = self.connect(target) - self.assertPacketLogContains(["qProcessInfo", "qfThreadInfo"]) + self.assertPacketLogReceived(["qProcessInfo", "qfThreadInfo"]) def test_attach_fail(self): error_msg = "mock-error-msg" @@ -142,15 +142,16 @@ def test_read_registers_using_g_packets(self): # But there certainly should be no p packets after the g packet. self.read_registers(process) - print(f"\nPACKET LOG:\n{self.server.responder.packetLog}\n") + received = self.server.responder.packetLog.get_received() + print(f"\nPACKET LOG:\n{received}\n") g_pos = 0 try: - g_pos = self.server.responder.packetLog.index("g") + g_pos = received.index("g") except err: self.fail("'g' packet not found after fetching registers") try: - second_g = self.server.responder.packetLog.index("g", g_pos) + second_g = received.index("g", g_pos + 1) self.fail("Found more than one 'g' packet") except: pass @@ -158,13 +159,7 @@ def test_read_registers_using_g_packets(self): # Make sure there aren't any `p` packets after the `g` packet: self.assertEqual( 0, - len( - [ - p - for p in self.server.responder.packetLog[g_pos:] - if p.startswith("p") - ] - ), + len([p for p in received[g_pos:] if p.startswith("p")]), ) def test_read_registers_using_p_packets(self): @@ -177,10 +172,9 @@ def test_read_registers_using_p_packets(self): process = self.connect(target) self.read_registers(process) - self.assertNotIn("g", self.server.responder.packetLog) - self.assertGreater( - len([p for p in self.server.responder.packetLog if p.startswith("p")]), 0 - ) + received = self.server.responder.packetLog.get_received() + self.assertNotIn("g", received) + self.assertGreater(len([p for p in received if p.startswith("p")]), 0) def test_write_registers_using_P_packets(self): """Test writing registers using 'P' packets (default behavior)""" @@ -189,12 +183,9 @@ def test_write_registers_using_P_packets(self): process = self.connect(target) self.write_registers(process) - self.assertEqual( - 0, len([p for p in self.server.responder.packetLog if p.startswith("G")]) - ) - self.assertGreater( - len([p for p in self.server.responder.packetLog if p.startswith("P")]), 0 - ) + received = self.server.responder.packetLog.get_received() + self.assertEqual(0, len([p for p in received if p.startswith("G")])) + self.assertGreater(len([p for p in received if p.startswith("P")]), 0) def test_write_registers_using_G_packets(self): """Test writing registers using 'G' packets""" @@ -209,12 +200,9 @@ def readRegister(self, register): process = self.connect(target) self.write_registers(process) - self.assertEqual( - 0, len([p for p in self.server.responder.packetLog if p.startswith("P")]) - ) - self.assertGreater( - len([p for p in self.server.responder.packetLog if p.startswith("G")]), 0 - ) + received = self.server.responder.packetLog.get_received() + self.assertEqual(0, len([p for p in received if p.startswith("P")])) + self.assertGreater(len([p for p in received if p.startswith("G")]), 0) def read_registers(self, process): self.for_each_gpr( @@ -291,7 +279,7 @@ def qLaunchSuccess(self): self.assertTrue(process, PROCESS_IS_VALID) self.assertEqual(process.GetProcessID(), 16) - self.assertPacketLogContains( + self.assertPacketLogReceived( [ "A%d,0,%s,8,1,61726731,8,2,61726732,8,3,61726733" % (len(exe_hex), exe_hex), @@ -352,7 +340,7 @@ def A(self, packet): self.assertTrue(process, PROCESS_IS_VALID) self.assertEqual(process.GetProcessID(), 16) - self.assertPacketLogContains( + self.assertPacketLogReceived( ["vRun;%s;61726731;61726732;61726733" % (exe_hex,)] ) @@ -424,7 +412,7 @@ def A(self, packet): self.assertTrue(process, PROCESS_IS_VALID) self.assertEqual(process.GetProcessID(), 16) - self.assertPacketLogContains( + self.assertPacketLogReceived( ["vRun;%s;61726731;61726732;61726733" % (exe_hex,)] ) @@ -468,7 +456,7 @@ def vRun(self, packet): lldb.SBError(), ) # error - self.assertPacketLogContains( + self.assertPacketLogReceived( [ "QEnvironment:EQUALS=foo=bar", "QEnvironmentHexEncoded:4e45454453454e433d66726f6224", @@ -522,7 +510,7 @@ def QEnvironment(self, packet): lldb.SBError(), ) # error - self.assertPacketLogContains( + self.assertPacketLogReceived( [ "QEnvironmentHexEncoded:455155414c533d666f6f3d626172", "QEnvironmentHexEncoded:4e45454453454e433d66726f6224", diff --git a/lldb/test/API/functionalities/gdb_remote_client/TestGDBRemoteLoad.py b/lldb/test/API/functionalities/gdb_remote_client/TestGDBRemoteLoad.py index f0a5429e6c1ce..d8214ae6b9a2d 100644 --- a/lldb/test/API/functionalities/gdb_remote_client/TestGDBRemoteLoad.py +++ b/lldb/test/API/functionalities/gdb_remote_client/TestGDBRemoteLoad.py @@ -22,7 +22,7 @@ def test_ram_load(self): target = self.createTarget("a.yaml") process = self.connect(target) self.dbg.HandleCommand("target modules load -l -s0") - self.assertPacketLogContains(["M1000,4:c3c3c3c3", "M1004,2:3232"]) + self.assertPacketLogReceived(["M1000,4:c3c3c3c3", "M1004,2:3232"]) @skipIfXmlSupportMissing def test_flash_load(self): @@ -63,7 +63,7 @@ def other(self, packet): target = self.createTarget("a.yaml") process = self.connect(target) self.dbg.HandleCommand("target modules load -l -s0") - self.assertPacketLogContains( + self.assertPacketLogReceived( [ "vFlashErase:1000,100", "vFlashWrite:1000:\xc3\xc3\xc3\xc3", diff --git a/lldb/test/API/functionalities/gdb_remote_client/TestGDBRemotePlatformFile.py b/lldb/test/API/functionalities/gdb_remote_client/TestGDBRemotePlatformFile.py index 69e04df81bc6e..a3def8165586a 100644 --- a/lldb/test/API/functionalities/gdb_remote_client/TestGDBRemotePlatformFile.py +++ b/lldb/test/API/functionalities/gdb_remote_client/TestGDBRemotePlatformFile.py @@ -30,7 +30,7 @@ def vFile(self, packet): ) self.match("platform file write 16 -o 11 -d teststring", [r"Return = 10"]) self.match("platform file close 16", [r"file 16 closed."]) - self.assertPacketLogContains( + self.assertPacketLogReceived( [ "vFile:open:2f736f6d652f66696c652e747874,00000202,000001ed", "vFile:pread:10,d,b", @@ -66,7 +66,7 @@ def vFile(self, packet): error=True, ) self.match("platform file close 16", [enosys_regex], error=True) - self.assertPacketLogContains( + self.assertPacketLogReceived( [ "vFile:open:2f736f6d652f66696c652e747874,00000202,000001ed", "vFile:pread:10,d,b", @@ -88,7 +88,7 @@ def vFile(self, packet): "platform get-size /some/file.txt", [r"File size of /some/file\.txt \(remote\): 4096"], ) - self.assertPacketLogContains( + self.assertPacketLogReceived( [ "vFile:size:2f736f6d652f66696c652e747874", ] @@ -113,7 +113,7 @@ def vFile(self, packet): "platform get-size /some/file.txt", [r"File size of /some/file\.txt \(remote\): 66051"], ) - self.assertPacketLogContains( + self.assertPacketLogReceived( [ "vFile:size:2f736f6d652f66696c652e747874", "vFile:open:2f736f6d652f66696c652e747874,00000000,00000000", @@ -135,7 +135,7 @@ def vFile(self, packet): [r"File size of /other/file\.txt \(remote\): 66051"], ) - self.assertPacketLogContains( + self.assertPacketLogReceived( [ "vFile:size:2f6f746865722f66696c652e747874", "vFile:open:2f6f746865722f66696c652e747874,00000000,00000000", @@ -161,7 +161,7 @@ def vFile(self, packet): "platform get-permissions /some/file.txt", [r"File permissions of /some/file\.txt \(remote\): 0o0644"], ) - self.assertPacketLogContains( + self.assertPacketLogReceived( [ "vFile:mode:2f736f6d652f66696c652e747874", ] @@ -190,7 +190,7 @@ def vFile(self, packet): "platform get-permissions /some/file.txt", [r"File permissions of /some/file\.txt \(remote\): 0o0644"], ) - self.assertPacketLogContains( + self.assertPacketLogReceived( [ "vFile:mode:2f736f6d652f66696c652e747874", "vFile:open:2f736f6d652f66696c652e747874,00000000,00000000", @@ -214,7 +214,7 @@ def vFile(self, packet): "platform file-exists /some/file.txt", [r"File /some/file\.txt \(remote\) exists"], ) - self.assertPacketLogContains( + self.assertPacketLogReceived( [ "vFile:exists:2f736f6d652f66696c652e747874", ] @@ -233,7 +233,7 @@ def vFile(self, packet): "platform file-exists /some/file.txt", [r"File /some/file\.txt \(remote\) does not exist"], ) - self.assertPacketLogContains( + self.assertPacketLogReceived( [ "vFile:exists:2f736f6d652f66696c652e747874", ] @@ -256,7 +256,7 @@ def vFile(self, packet): "platform file-exists /some/file.txt", [r"File /some/file\.txt \(remote\) exists"], ) - self.assertPacketLogContains( + self.assertPacketLogReceived( [ "vFile:exists:2f736f6d652f66696c652e747874", "vFile:open:2f736f6d652f66696c652e747874,00000000,00000000", @@ -279,7 +279,7 @@ def vFile(self, packet): "platform file-exists /some/file.txt", [r"File /some/file\.txt \(remote\) does not exist"], ) - self.assertPacketLogContains( + self.assertPacketLogReceived( [ "vFile:exists:2f736f6d652f66696c652e747874", "vFile:open:2f736f6d652f66696c652e747874,00000000,00000000", From 2612b022f21dcc1613359307069655f40369ec23 Mon Sep 17 00:00:00 2001 From: Jonas Devlieghere Date: Fri, 7 Nov 2025 10:07:38 -0800 Subject: [PATCH 2/2] [lldb] Correctly detach (rather than kill) when connecting with gdb-remote (#166869) We weren't setting `m_should_detach` when going through the `DoConnectRemote` code path. This meant that when you would attaches to a remote process with `gdb-remote ` and use Ctrl+D, it would kill the process instead of detach from it. rdar://156111423 (cherry picked from commit cce1055e4803ce67f908844681d745d6a87ad450) --- lldb/source/Target/Process.cpp | 1 + .../TestConnectRemoteDetach.py | 67 +++++++++++++++++++ 2 files changed, 68 insertions(+) create mode 100644 lldb/test/API/functionalities/gdb_remote_client/TestConnectRemoteDetach.py diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp index 5e2fc7e4a8ef9..af2fe13b1ddce 100644 --- a/lldb/source/Target/Process.cpp +++ b/lldb/source/Target/Process.cpp @@ -3443,6 +3443,7 @@ Status Process::ConnectRemote(llvm::StringRef remote_url) { if (state == eStateStopped || state == eStateCrashed) { // If we attached and actually have a process on the other end, then // this ended up being the equivalent of an attach. + SetShouldDetach(true); CompleteAttach(); // This delays passing the stopped event to listeners till diff --git a/lldb/test/API/functionalities/gdb_remote_client/TestConnectRemoteDetach.py b/lldb/test/API/functionalities/gdb_remote_client/TestConnectRemoteDetach.py new file mode 100644 index 0000000000000..4380455efc452 --- /dev/null +++ b/lldb/test/API/functionalities/gdb_remote_client/TestConnectRemoteDetach.py @@ -0,0 +1,67 @@ +""" +Test that ConnectRemote sets ShouldDetach flag correctly. + +When connecting to a remote process that stops after connection, +the process should be marked for detach (not kill) on destruction. +""" + +import lldb +from lldbsuite.test.lldbtest import * +from lldbsuite.test.decorators import * +from lldbsuite.test.gdbclientutils import * +from lldbsuite.test.lldbgdbclient import GDBRemoteTestBase +from lldbsuite.test import lldbutil + + +class TestConnectRemoteDetach(GDBRemoteTestBase): + """Test that ConnectRemote properly sets ShouldDetach flag.""" + + class StoppedResponder(MockGDBServerResponder): + """A responder that returns a stopped process.""" + + def qfThreadInfo(self): + return "m1" + + def qsThreadInfo(self): + return "l" + + def qC(self): + return "QC1" + + def haltReason(self): + # Return that we're stopped + return "T05thread:1;" + + def cont(self): + # Stay stopped + return "T05thread:1;" + + def D(self): + # Detach packet: this is what we want to verify gets called. + return "OK" + + def k(self): + # Kill packet: this is what we want to verify doesn't get called. + raise RuntimeError("should not receive k(ill) packet") + + def test_connect_remote_sets_detach(self): + """Test that ConnectRemote to a stopped process sets ShouldDetach.""" + self.server.responder = self.StoppedResponder() + + target = self.createTarget("a.yaml") + process = self.connect(target) + + # Wait for the process to be in stopped state after connecting. + # When ConnectRemote connects to a remote process that is stopped, + # it should call SetShouldDetach(true) before CompleteAttach(). + lldbutil.expect_state_changes( + self, self.dbg.GetListener(), process, [lldb.eStateStopped] + ) + + # Now destroy the process. Because ShouldDetach was set to true + # during ConnectRemote, this should send a 'D' (detach) packet + # rather than a 'k' (kill) packet when the process is destroyed. + process.Destroy() + + # Verify that the (D)etach packet was sent. + self.assertPacketLogReceived(["D"])