[lldb] Add bidirectional packetLog to gdbclientutils.py (llvm#162176)

While debugging the tests for llvm#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 f188c97)

Author: dsandersllvm

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,39 @@ 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):
+<<<<<<< HEAD
         self.packetLog = []
+||||||| parent of f188c97cc193 ([lldb] Add bidirectional packetLog to gdbclientutils.py (#162176))
+        self.packetLog: List[str] = []
+=======
+        self.packetLog = PacketLog()
+>>>>>>> f188c97cc193 ([lldb] Add bidirectional packetLog to gdbclientutils.py (#162176))
 
-    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,24 +698,28 @@ 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:
             if part is MockGDBServerResponder.RESPONSE_NONE:
                 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()

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))
             )
 
 

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"])

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,29 +142,24 @@ 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
 
         # 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",

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",