Re: [PATCHv3 2/2] gdb/remote: Restore support for 'S' stop reply packet

[Pedro Alves <palves@redhat.com>]

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