[lldb] Add bidirectional packetLog to gdbclientutils.py #162176
Conversation
Everything in this should be python 3.9. The docs say the minimum is 3.8 but there's existing code in this suite that needs 3.9 so I think 3.9 is ok. Issues: qEcho() is passed an argument by the callers that the function didn't have Several functions in the base class would silently do nothing if not overriden. These now use @AbstractMethod to require overrides sendall() had inconsistent return types between overrides
While debugging the tests for PR155000 I found it helpful to have both sides of the simulated gdb-rsp traffic rather than just the responses so I've added a packetLog to MockGDBServer. The existing response-only one in MockGDBServerResponder is used by tests so I chose not to change it
|
@llvm/pr-subscribers-lldb Author: Daniel Sanders (dsandersllvm) ChangesWhile debugging the tests for #155000 I found it helpful to have both sides Full diff: https://github.com/llvm/llvm-project/pull/162176.diff 1 Files Affected:
diff --git a/lldb/packages/Python/lldbsuite/test/gdbclientutils.py b/lldb/packages/Python/lldbsuite/test/gdbclientutils.py
index b603c35c8df09..0395dcc5246f1 100644
--- a/lldb/packages/Python/lldbsuite/test/gdbclientutils.py
+++ b/lldb/packages/Python/lldbsuite/test/gdbclientutils.py
@@ -1,3 +1,4 @@
+from abc import ABC, abstractmethod
import ctypes
import errno
import io
@@ -5,6 +6,7 @@
import socket
import traceback
from lldbsuite.support import seven
+from typing import Optional, Literal
def checksum(message):
@@ -86,8 +88,8 @@ class MockGDBServerResponder:
handles any packet not recognized in the common packet handling code.
"""
- registerCount = 40
- packetLog = None
+ registerCount: int = 40
+ packetLog: Optional[list[str]] = None
class RESPONSE_DISCONNECT:
pass
@@ -103,6 +105,7 @@ def respond(self, packet):
Return the unframed packet data that the server should issue in response
to the given packet received from the client.
"""
+ assert self.packetLog is not None
self.packetLog.append(packet)
if packet is MockGDBServer.PACKET_INTERRUPT:
return self.interrupt()
@@ -242,7 +245,7 @@ def qProcessInfo(self):
def qHostInfo(self):
return "ptrsize:8;endian:little;"
- def qEcho(self):
+ def qEcho(self, _: int):
return "E04"
def qQueryGDBServer(self):
@@ -263,10 +266,10 @@ def A(self, packet):
def D(self, packet):
return "OK"
- def readRegisters(self):
+ def readRegisters(self) -> str:
return "00000000" * self.registerCount
- def readRegister(self, register):
+ def readRegister(self, register: int) -> str:
return "00000000"
def writeRegisters(self, registers_hex):
@@ -306,7 +309,8 @@ def haltReason(self):
# SIGINT is 2, return type is 2 digit hex string
return "S02"
- def qXferRead(self, obj, annex, offset, length):
+ def qXferRead(self, obj: str, annex: str, offset: int,
+ length: int) -> tuple[str | None, bool]:
return None, False
def _qXferResponse(self, data, has_more):
@@ -374,15 +378,17 @@ class UnexpectedPacketException(Exception):
pass
-class ServerChannel:
+class ServerChannel(ABC):
"""
A wrapper class for TCP or pty-based server.
"""
- def get_connect_address(self):
+ @abstractmethod
+ def get_connect_address(self) -> str:
"""Get address for the client to connect to."""
- def get_connect_url(self):
+ @abstractmethod
+ def get_connect_url(self) -> str:
"""Get URL suitable for process connect command."""
def close_server(self):
@@ -394,10 +400,12 @@ def accept(self):
def close_connection(self):
"""Close all resources used by the accepted connection."""
- def recv(self):
+ @abstractmethod
+ def recv(self) -> bytes:
"""Receive a data packet from the connected client."""
- def sendall(self, data):
+ @abstractmethod
+ def sendall(self, data: bytes) -> None:
"""Send the data to the connected client."""
@@ -428,11 +436,11 @@ def close_connection(self):
self._connection.close()
self._connection = None
- def recv(self):
+ def recv(self) -> bytes:
assert self._connection is not None
return self._connection.recv(4096)
- def sendall(self, data):
+ def sendall(self, data: bytes) -> None:
assert self._connection is not None
return self._connection.sendall(data)
@@ -444,10 +452,10 @@ def __init__(self):
)[0]
super().__init__(family, type, proto, addr)
- def get_connect_address(self):
+ def get_connect_address(self) -> str:
return "[{}]:{}".format(*self._server_socket.getsockname())
- def get_connect_url(self):
+ def get_connect_url(self) -> str:
return "connect://" + self.get_connect_address()
@@ -455,10 +463,10 @@ class UnixServerSocket(ServerSocket):
def __init__(self, addr):
super().__init__(socket.AF_UNIX, socket.SOCK_STREAM, 0, addr)
- def get_connect_address(self):
+ def get_connect_address(self) -> str:
return self._server_socket.getsockname()
- def get_connect_url(self):
+ def get_connect_url(self) -> str:
return "unix-connect://" + self.get_connect_address()
@@ -472,7 +480,7 @@ def __init__(self):
self._primary = io.FileIO(primary, "r+b")
self._secondary = io.FileIO(secondary, "r+b")
- def get_connect_address(self):
+ def get_connect_address(self) -> str:
libc = ctypes.CDLL(None)
libc.ptsname.argtypes = (ctypes.c_int,)
libc.ptsname.restype = ctypes.c_char_p
@@ -485,7 +493,7 @@ def close_server(self):
self._secondary.close()
self._primary.close()
- def recv(self):
+ def recv(self) -> bytes:
try:
return self._primary.read(4096)
except OSError as e:
@@ -494,8 +502,8 @@ def recv(self):
return b""
raise
- def sendall(self, data):
- return self._primary.write(data)
+ def sendall(self, data: bytes) -> None:
+ self._primary.write(data)
class MockGDBServer:
@@ -513,10 +521,12 @@ class MockGDBServer:
_receivedData = None
_receivedDataOffset = None
_shouldSendAck = True
+ packetLog: list[tuple[Literal["recv"] | Literal["send"], str | bytes]]
def __init__(self, socket):
self._socket = socket
self.responder = MockGDBServerResponder()
+ self.packetLog = []
def start(self):
# Start a thread that waits for a client connection.
@@ -528,18 +538,21 @@ def stop(self):
self._thread.join()
self._thread = None
- def get_connect_address(self):
+ def get_connect_address(self) -> str:
+ assert self._socket is not None
return self._socket.get_connect_address()
- def get_connect_url(self):
+ def get_connect_url(self) -> str:
+ assert self._socket is not None
return self._socket.get_connect_url()
def run(self):
+ assert self._socket is not None
# For testing purposes, we only need to worry about one client
# connecting just one time.
try:
self._socket.accept()
- except:
+ except Exception:
traceback.print_exc()
return
self._shouldSendAck = True
@@ -554,7 +567,7 @@ def run(self):
self._receive(data)
except self.TerminateConnectionException:
pass
- except Exception as e:
+ except Exception:
print(
"An exception happened when receiving the response from the gdb server. Closing the client..."
)
@@ -587,7 +600,9 @@ def _parsePacket(self):
Once a complete packet is found at the front of self._receivedData,
its data is removed form self._receivedData.
"""
+ assert self._receivedData is not None
data = self._receivedData
+ assert self._receivedDataOffset is not None
i = self._receivedDataOffset
data_len = len(data)
if data_len == 0:
@@ -638,12 +653,17 @@ def _parsePacket(self):
# can start on the next packet the next time around
self._receivedData = data[i:]
self._receivedDataOffset = 0
+ self.packetLog.append(("recv", packet))
return packet
- def _sendPacket(self, packet):
- self._socket.sendall(seven.bitcast_to_bytes(frame_packet(packet)))
+ def _sendPacket(self, packet: str):
+ assert self._socket is not None
+ framed_packet = seven.bitcast_to_bytes(frame_packet(packet))
+ self.packetLog.append(("send", framed_packet))
+ self._socket.sendall(framed_packet)
def _handlePacket(self, packet):
+ assert self._socket is not None
if packet is self.PACKET_ACK:
# Ignore ACKs from the client. For the future, we can consider
# adding validation code to make sure the client only sends ACKs
|
You can test this locally with the following command:darker --check --diff -r origin/main...HEAD lldb/packages/Python/lldbsuite/test/gdbclientutils.py
View the diff from darker here.--- gdbclientutils.py 2025-10-06 22:12:20.000000 +0000
+++ gdbclientutils.py 2025-10-06 22:16:24.820751 +0000
@@ -307,12 +307,13 @@
def haltReason(self):
# SIGINT is 2, return type is 2 digit hex string
return "S02"
- def qXferRead(self, obj: str, annex: str, offset: int,
- length: int) -> tuple[str | None, bool]:
+ def qXferRead(
+ self, obj: str, annex: str, offset: int, length: int
+ ) -> tuple[str | None, bool]:
return None, False
def _qXferResponse(self, data, has_more):
return "%s%s" % ("m" if has_more else "l", escape_binary(data))
|
|
I assume this is stacked on the other PR, please leave a comment saying so in future or configure whatever tool to do that for you.
There's at least 4 sides here, so please clarify :) From the client's point of view, it sends stuff and receives stuff. Then the server also has it's perspective. So this change adds logging from the server's perspective? Because you are adding send and receive, not just one. Which is what I first assumed when I saw "bidirectional" like it was only the sent packets and you're adding the received packets. |
Ok so you could have added the sent packets to this but chose not to. That's why it looks a bit strange to me. How does MockGDBServerResponder use the existing log and why did that push you to making a new one? I guess it's just blindly indexing it, making assumptions about ordering? (because it knows they're all one direction) Seems like we could fix that but idk how much work it would be. |
Yes it is, sorry about that. I used to use phabricator for this back when that was the way to do reviews, I don't currently know of a tool that works with github PR's
I meant that the mock server is logging both the packets it receives from the lldb client and the packets it sends back to it.
I need to correct 'response-only' in my description there, it's 'receive only'. The function is called It's all things that could be adapted to a log that keeps both sides, I just didn't want that part to be a prerequisite of the existing patches. It's mostly calling packetLog.index(...) with specific packets to confirm their presence and testing ranges based on that It's all stuff that can be adapted. It makes a bit more sense to me for the MockGDBServer to be keeping the packet log as that's the component that does the send/receive to/from the client but it'd be fine in either |
I miss this feature too. We mention a couple of tools on https://llvm.org/docs/GitHub.html#using-graphite-for-stacked-pull-requests but I've not tried any of them myself.
Ok so we're only talking about the mock server here.
Reading the whole PR there are several confusing things (which predate your changes but still) in here. They only get worse when we add another log. I would prefer you do this by extending the current log to include all packets. So that we have 1 log with 3 views. Sent, received and all. The existing code can all be changed to use the received view. Logging prints them all. Could leave out sent given that nothing will use it right now. As a starting point (feel free to take this): Two lists so we keep the indexing, slicing, iterating we do now, and a third list for insertion order so the log prints them in the correct sequence. Wrapping the lists prevents us modifying them and making the order go out of sync. There's some super fancy ways to do this but we're not too concerned about being Pythonic here. |
| """ | ||
|
|
||
| registerCount = 40 | ||
| packetLog = None |
There was a problem hiding this comment.
This predates you, but I wonder why we even have a class level attribute for this? Maybe I am misunderstanding some aspect of Python because it seems to me that the packet log should never be None. Empty list, sure, but not None.
There was a problem hiding this comment.
Assuming it passes testing, #162453 will remove this line.
|
|
||
| def qXferRead(self, obj, annex, offset, length): | ||
| def qXferRead(self, obj: str, annex: str, offset: int, | ||
| length: int) -> tuple[str | None, bool]: |
There was a problem hiding this comment.
In future please separate annotations of existing types into their own PR. If you're changing the type in the process of doing something then it's fine to include in the same PR.
| """ | ||
|
|
||
| def get_connect_address(self): | ||
| @abstractmethod |
There was a problem hiding this comment.
Good idea, but separate PR please.
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 added
a packetLog to MockGDBServer. The existing response-only one in
MockGDBServerResponder is used by tests so I chose not to change it