[PATCHv3 2/2] gdb/remote: Restore support for 'S' stop reply packet
Pedro Alves
palves@redhat.com
Mon Mar 2 12:25:00 GMT 2020
On 3/2/20 11:54 AM, Andrew Burgess wrote:
> My proposal for a fix then is:
>
> 1. Move the call to switch_to_inferior_no_thread into
> do_target_wait_1, this means that in call cases where we are waiting
"in all cases"
> for an inferior the inferior_ptid will be set to null_ptid. This is
> good as no wait code should rely on inferior_ptid.
>
> 2. Remove the use of general_thread from the 'T' packet processing.
> The general_thread read here was only ever correct by chance, and we
> shouldn't be using it this way.
>
> 3. Remove use of inferior_ptid from ::process_stop_event as this is
> wrong, and will always be null_ptid now anyway.
>
> 4. When a stop_even has null_ptid due to a lack of thread-id (either
"stop_even" -> "stop_event"
> from a T packet or an S packet) then pick the first non exited thread
> in the inferior and use that. This will be fine for single threaded
"in the inferior" -> "in the target".
> inferiors. A multi-threaded inferior really should be using T
Instead of
"A multi-threaded inferior",
we should say
"A multi-thread or multi-inferior aware remote server/stub"
or something around that.
> packets with a thread-id, so we give a warning if the inferior is
> multi-threaded, and we are still missing a thread-id.
"inferior" -> "target".
The "inferior" -> "target" distinction I'm making in these
small remarks above matters, because say the remote server
is debugging two single-threaded inferiors. We still want to
(and do) warn.
>
> 5. Extend the existing test that covered the T packet with missing
> thread-id to also cover the S packet.
Excellent.
>
> gdb/ChangeLog:
>
> * remote.c (remote_target::remote_parse_stop_reply): Don't use the
> general_thread if the stop reply is missing a thread-id.
> (remote_target::process_stop_reply): Use the first non-exited
> thread if the target didn't pass a thread-id.
> * infrun.c (do_target_wait): Move call to
> switch_to_inferior_no_thread to ....
> (do_target_wait_1): ... here.
>
> gdb/testsuite/ChangeLog:
>
> * gdb.server/stop-reply-no-thread.exp: Add test where T packet is
> disabled.
> ---
> gdb/ChangeLog | 10 +++
> gdb/infrun.c | 8 ++-
> gdb/remote.c | 43 ++++++++----
> gdb/testsuite/ChangeLog | 5 ++
> gdb/testsuite/gdb.server/stop-reply-no-thread.exp | 80 ++++++++++++++---------
> 5 files changed, 101 insertions(+), 45 deletions(-)
>
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index d9a6f733519..43199b17b05 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -3456,6 +3456,12 @@ do_target_wait_1 (inferior *inf, ptid_t ptid,
> ptid_t event_ptid;
> struct thread_info *tp;
>
> + /* We know that we are looking for an event in inferior INF, but we don't
"in the inferior INF" -> "in the target of inferior INF".
The distinction is important -- target_wait may well return an event
for an inferior different from INF.
> + know which thread the event might come from. As such we want to make
> + sure that INFERIOR_PTID is reset so that non of the wait code relies
"that non of" -> "that none of" ?
> + on it - doing so is always a mistake. */
> + switch_to_inferior_no_thread (inf);
> +
OK with the nits above fixed. Thanks much for doing this!
Pedro Alves
More information about the Gdb-patches
mailing list