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

[Andrew Burgess <andrew.burgess@embecosm.com>]

Thanks for all the feedback.  I pushed the patches with the fixes you
suggested.

* Pedro Alves <palves@redhat.com> [2020-03-02 12:25:12 +0000]:

> 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.

I hadn't really considered this case, however, this raises a
follow on question:

Before entering the target wait code we call
switch_to_inferior_no_thread, partly, as I understand it because
having inferior_ptid pointing at a thread leads to invalid code
that relies on this thread being _the_ event thread, when really we
need to extract the inferior and thread-id from the stop event.

So, why do we allow the current_inferior to remain valid while we
perform the wait?  Instead, why don't we clear both current_inferior
and inferior_ptid, and then enter the wait code?

Thanks,
Andrew

> 
> > 
> >  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
>