Page MenuHomePhabricator

[lldb] [client] Support for multiprocess extension
ClosedPublic

Authored by mgorny on Mar 30 2021, 9:17 AM.

Details

Summary

Add a minimal support for the multiprocess extension in gdb-remote
client. It accepts PIDs as part of thread-ids, and rejects PIDs that
do not match the current inferior.

Diff Detail

Event Timeline

mgorny requested review of this revision.Mar 30 2021, 9:17 AM
mgorny created this revision.

@labath, split GetCurrentThreadIDs() as suggested. I've also modified GetCurrentProcessID() to prefer explicit PID over TID when available.

mgorny updated this revision to Diff 334404.Mar 31 2021, 4:22 AM

Added better PID mismatch handling in SetThreadStopInfo(). Not that most of the call sites actually check the return value...

I think this looks fine. Could you also create a gdb-client test case (you can ignore the default thread thingy)?

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
1130

It looks like this is only used in the (probably completely broken) non-stop mode. I should put that on my to-delete list...

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
370

lets make this an actual return value

mgorny updated this revision to Diff 334686.Apr 1 2021, 7:37 AM
mgorny marked an inline comment as done.

Changed GetCurrentProcessAndThreadIDs() to return the vector, and reformatted.

Tests next.

mgorny updated this revision to Diff 334725.Apr 1 2021, 9:15 AM

Add some tests.

@labath, I'd use some help now. When the stop reason includes wrong PID, the GDB exchange seems to enter a dead loop:

b-remote.async>  <   5> send packet: $c#63
b-remote.async>  <  25> read packet: $S02thread:p404.10210;#cc
intern-state     <  16> send packet: $qfThreadInfo#bb
intern-state     <  26> read packet: $mp400.10200,p400.10204#e7
intern-state     <  16> send packet: $qsThreadInfo#c8
intern-state     <   5> read packet: $l#6c

I think this is because most of the code doesn't really care about SetThreadStopInfo()'s return value, and I'm not really convinced that's the best way of reporting an error. Could you suggest how to handle 'wrong PID' error here?

mgorny updated this revision to Diff 334930.Apr 2 2021, 3:02 AM

Added a skip to the hanging new test.

labath added a comment.Apr 8 2021, 3:29 AM

Yeah, that's tricky. For a inconsistent/unsupported response like this, probably the only reasonable thing we can do print some error message, terminate the connection and put the inferior into some terminal state. However, I don't think we have anything like that (and I suspect there are plenty of other ways one could trigger a loop like this). I'd say we should just delete the test right now.

mgorny updated this revision to Diff 336052.Apr 8 2021, 3:55 AM

Removed the hanging test.

mgorny added a comment.Apr 8 2021, 3:55 AM

@labath, any further comments or is this good to go then?

labath accepted this revision.Apr 8 2021, 4:07 AM

Yea, I think it should be fine.

This revision is now accepted and ready to land.Apr 8 2021, 4:07 AM
This revision was landed with ongoing or failed builds.Apr 8 2021, 4:46 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 8 2021, 4:46 AM