labath added a comment.
My main comment is about making sure the tearDown story is sufficiently robust.
I want to be sure we don't introduce flakyness here.
================
Comment at:
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteClient.py:12
+ target = self.createTarget("a.yaml")
+ process = self.connect(target)
----------------
Could you put some basic assertion here? I guess at this point we can expect to
see QStartNoAckMode and/or some basic query packets (?)
================
Comment at:
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py:25
+
+def yaml2elf(yaml_path, elf_path):
+ """
----------------
call this yaml2obj? We should be able to use this function to create a Mach-O
file as well...
================
Comment at:
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py:164-167
+ return "xxxxxxxx" * self.registerCount
+
+ def readRegister(self, register):
+ return "xxxxxxxx"
----------------
Maybe at least return valid hex digits here?
================
Comment at:
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py:246-248
+ if self._client is not None:
+ self._client.shutdown(socket.SHUT_RDWR)
+ self._client.close()
----------------
The concurrent access to self._client here makes me very uneasy. If this were
C, it would definitely be a race, but I don't think python can do much to avoid
a fd-reassignment race anyway. The exception handling code below only reaffirms
my suspicion. I think we should make this safer to avoid test flakyness down
the line.
I think a client-initiated shutdown should be sufficient. SBProcess.Kill()
should be enough to make sure the client socket is closed. After that, you just
need to join the server thread (which should exit after it detects a connection
drop).
What do you think?
================
Comment at:
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py:249-250
+ self._client.close()
+ # Would call self._socket.shutdown, but it blocks forever for some
+ # unknown reason. close() works just fine.
+ self._socket.close()
----------------
I don't think calling shutdown on a socket in listen mode is expected (i.e.,
just delete this comment).
================
Comment at:
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py:280
+ break
+ self._receive(data)
+
----------------
Could you try break the _receive function down into simpler chunks. The nested
while loops make this quite hard to follow. I suggest a design that would
involve functions like:
```
getFullPacket # checks whether accumulated input contains full packet and
returns it
getPayload # verifies checksum, unframes, and unescapes
```
and making sure all sending happens in _handlePacket only.
================
Comment at:
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py:431
+ if i < len(packets):
+ self.fail("Did not receive: %s\n\t%s" % (packets[i],
+ '\n\t'.join(log[-10:])))
----------------
Add a message that the lines below are the last 10 packets received.
https://reviews.llvm.org/D42195
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
