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

["Saiapova, Natalia" <natalia.saiapova@intel.com>]

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?

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