Re: [PATCH v2 4/6] gdb/infrun: in condition evaluation wait only for the current inferior.

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

* Saiapova, Natalia via Gdb-patches <gdb-patches@sourceware.org> [2020-10-26 12:11:06 +0000]:

> Hi Kevin,
> 
> Thank you for looking into this! See my answer below.
> 
> Regards,
> Natalia
> > -----Original Message-----
> > From: Kevin Buettner <kevinb@redhat.com>
> > Sent: Monday, October 19, 2020 4:44 AM
> > To: Natalia Saiapova via Gdb-patches <gdb-patches@sourceware.org>
> > Cc: Saiapova, Natalia <natalia.saiapova@intel.com>
> > Subject: Re: [PATCH v2 4/6] gdb/infrun: in condition evaluation wait only for the
> > current inferior.
> > 
> > Hi Natalia,
> > 
> > I have a question about this commit.  See below.
> > 
> > On Fri,  9 Oct 2020 13:27:16 +0200
> > Natalia Saiapova via Gdb-patches <gdb-patches@sourceware.org> wrote:
> > 
> > > In the middle of a condition evaluation, to successfully finish the
> > > inferior call we need to wait for events from the current thread.
> > > Otherwise, a pending event from another thread might be taken and
> > > the inferior call is abandoned.
> > >
> > > gdb/ChangeLog:
> > > 2020-08-26  Natalia Saiapova  <natalia.saiapova@intel.com>
> > > 	    Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
> > >
> > > 	* infrun.c (do_target_wait): Match an inferior PID with the
> > > 	expected PID.
> > > 	(fetch_inferior_event): In condition evaluation, wait for the
> > > 	event from the current inferior.
> > >
> > > Co-authored-by: Natalia Saiapova <natalia.saiapova@intel.com>
> > > Co-authored-by: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
> > >
> > > Signed-off-by: Natalia Saiapova <natalia.saiapova@intel.com>
> > > ---
> > >  gdb/infrun.c | 17 +++++++++++++----
> > >  1 file changed, 13 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/gdb/infrun.c b/gdb/infrun.c
> > > index 91271c2ddbe..d260eb6e3a7 100644
> > > --- a/gdb/infrun.c
> > > +++ b/gdb/infrun.c
> > > @@ -3565,10 +3565,11 @@ do_target_wait (ptid_t wait_ptid,
> > execution_control_state *ecs,
> > >       polling the rest of the inferior list starting from that one in a
> > >       circular fashion until the whole list is polled once.  */
> > >
> > > -  auto inferior_matches = [&wait_ptid] (inferior *inf)
> > > +  ptid_t wait_pid_ptid {wait_ptid.pid ()};
> > > +  auto inferior_matches = [&wait_pid_ptid] (inferior *inf)
> > >      {
> > > -      return (inf->process_target () != NULL
> > > -	      && ptid_t (inf->pid).matches (wait_ptid));
> > > +      return (inf->process_target () != nullptr
> > > +	      && ptid_t (inf->pid).matches (wait_pid_ptid));
> > >      };
> > 
> > I'm puzzled by the changes above.  Can you explain what this is
> > about?  (And perhaps add a comment...)
> 
> When we come here from the infcall inside the condition, the
> wait_ptid has a form {non-zero, non-zero, zero}, which corresponds
> to the thread started the infcall. So, when we construct a ptid via
> ptid_t (inf->pid) in inferior_matches function, it does not match
> the wait_ptid. That leads to ecs->ws.kind = TARGET_WAITKIND_IGNORE
> and we are not getting the event. But we wait for an event from the
> thread which has started the infcall, so until we get one we are in
> an infinite loop.
> 
> This code looks like the intention here is to find a matching
> inferior, so we are interested only in the PID part of the
> wait_ptid. Thus, I made the change. Does this make sense to you?

This describes what, but not why.  You earlier change in
user_visible_resume_ptid, which I'm guessing relates to this change
returned the inferior_ptid.  But that's not the only path by which we
might return inferior_ptid.  So its not clear why this change runs
into problems, but other cases don't.

Also I agree with Kevin that this probably needs explaining with
comments in the code.

Thanks,
Andrew


> 
> > 
> > >
> > >    /* First see how many matching inferiors we have.  */
> > > @@ -3906,7 +3907,15 @@ fetch_inferior_event ()
> > >        = make_scoped_restore (&execution_direction,
> > >  			     target_execution_direction ());
> > >
> > > -    if (!do_target_wait (minus_one_ptid, ecs, TARGET_WNOHANG))
> > > +    ptid_t waiton_ptid = minus_one_ptid;
> > > +
> > > +    /* If the thread is in the middle of the condition evaluation,
> > > +       wait for the event from the current inferior.  */
> > > +    if (inferior_ptid != null_ptid
> > > +	&& inferior_thread ()->control.in_cond_eval)
> > > +      waiton_ptid = inferior_ptid;
> > > +
> > > +    if (!do_target_wait (waiton_ptid, ecs, TARGET_WNOHANG))
> > >        return;
> > >
> > >      gdb_assert (ecs->ws.kind != TARGET_WAITKIND_IGNORE);
> > > --
> > > 2.17.1
> > >
> > > Intel Deutschland GmbH
> > > Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
> > > Tel: +49 89 99 8853-0, www.intel.de
> > > Managing Directors: Christin Eisenschmid, Gary Kershaw
> > > Chairperson of the Supervisory Board: Nicole Lau
> > > Registered Office: Munich
> > > Commercial Register: Amtsgericht Muenchen HRB 186928
> > >
> 
> Intel Deutschland GmbH
> Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
> Tel: +49 89 99 8853-0, www.intel.de
> Managing Directors: Christin Eisenschmid, Gary Kershaw
> Chairperson of the Supervisory Board: Nicole Lau
> Registered Office: Munich
> Commercial Register: Amtsgericht Muenchen HRB 186928
>