[Lldb-commits] [PATCH] D83728: [lldb] Make `process connect` blocking in synchronous mode.

Pavel Labath via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Tue Jul 14 00:51:04 PDT 2020


labath accepted this revision.
labath added a comment.

The code looks fine. The test could be cleaned up a bit...



================
Comment at: lldb/source/Target/Platform.cpp:1834
   if (error.Fail())
     return nullptr;
 
----------------
JDevlieghere wrote:
> jingham wrote:
> > If you fail here you leave the process hijacked.  That doesn't matter because if "ConnectRemote" fails, you aren't going to have much to listen to anyway.  But it still looks odd.  I'm surprised we don't have some RAII-dingus for process hijacking, but anyway, it's good practice to undo this in the error branch.
> Yeah, that's exactly my reasoning. You can't use a RAII object here, because the order of destruction is undefined, so you might end up calling `RestoreProcessEvents` after the shared pointer has been destructed. Anyway, I've added the call just for consistency. 
I'm not sure what you mean by the undefined destruction order. In c++ the destruction order is always the reverse of the construction order. The only "exception" to that that I am aware of is the destruction order for function call arguments. But even there the destruction order is still reverse construction order -- just the construction order itself is not (fully) defined.


================
Comment at: lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py:19-43
+    def qfProcessInfo(self, packet):
+        if "all_users:1" in packet:
+            self.all_users = True
+            name = hexlify("/a/test_process")
+            args = "-".join(
+                map(hexlify,
+                    ["/system/bin/sh", "-c", "/data/local/tmp/lldb-server"]))
----------------
It looks like this was copied from the "platform process list" test. I'd be very surprised if this is really needed. I guess the default responder should work just fine here...


================
Comment at: lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py:46
+
+class TestProcesConnect(GDBRemoteTestBase):
+    def test_gdb_remote_sync(self):
----------------
`s/s/ss` :P


================
Comment at: lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py:63
+            self.expect("gdb-remote %d" % self.server.port,
+                        matching=False,
+                        substrs=['Process', 'stopped'])
----------------
Besides the negative output check, it would be also good to test that the event actually gets delivered. I think something like `lldbutil.expect_state_changes(self, self.dbg.GetListener(), self.process(), [lldb.eStateStopped])` ought to do the trick.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83728/new/

https://reviews.llvm.org/D83728





More information about the lldb-commits mailing list