This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCHv3 2/2] gdb/remote: Restore support for 'S' stop reply packet
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
>