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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
