[Lldb-commits] [PATCH] D42195: [lldb] Generic base for testing gdb-remote behavior

[Pavel Labath via Phabricator via lldb-commits]

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