[PATCH v2 12/13] Enable async mode in the target in attach_cmd.

[Andrew Burgess]

* John Baldwin <jhb@FreeBSD.org> [2021-11-24 14:42:06 -0800]:

> On 11/24/21 7:16 AM, Andrew Burgess wrote:
> > * John Baldwin <jhb@FreeBSD.org> [2021-08-03 11:49:59 -0700]:
> > 
> > > If the attach target supports async mode, enable it after the
> > > attach target's ::attach method returns.
> > 
> > This makes sense to me.  I did have one comment.
> > 
> > > ---
> > >   gdb/fbsd-nat.c  | 13 -------------
> > >   gdb/fbsd-nat.h  |  2 --
> > >   gdb/infcmd.c    |  4 ++++
> > >   gdb/linux-nat.c |  3 ---
> > >   gdb/remote.c    |  1 -
> > >   5 files changed, 4 insertions(+), 19 deletions(-)
> > > 
> > > diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
> > > index 1c27b698cce..34713151cbe 100644
> > > --- a/gdb/fbsd-nat.c
> > > +++ b/gdb/fbsd-nat.c
> > > @@ -1028,19 +1028,6 @@ fbsd_nat_target::close ()
> > >     inf_ptrace_target::close ();
> > >   }
> > > -/* Implement the "attach" target method.  */
> > > -
> > > -void
> > > -fbsd_nat_target::attach (const char *args, int from_tty)
> > > -{
> > > -  inf_ptrace_target::attach (args, from_tty);
> > > -
> > > -  /* Curiously, the core does not do this automatically.  */
> > > -  if (target_can_async_p ())
> > > -    target_async (1);
> > > -}
> > > -
> > > -
> > >   #ifdef TDP_RFPPWAIT
> > >   /*
> > >     To catch fork events, PT_FOLLOW_FORK is set on every traced process
> > > diff --git a/gdb/fbsd-nat.h b/gdb/fbsd-nat.h
> > > index 9dd9017c1c3..ad7f76bca40 100644
> > > --- a/gdb/fbsd-nat.h
> > > +++ b/gdb/fbsd-nat.h
> > > @@ -77,8 +77,6 @@ class fbsd_nat_target : public inf_ptrace_target
> > >     thread_control_capabilities get_thread_control_capabilities () override
> > >     { return tc_schedlock; }
> > > -  void attach (const char *, int) override;
> > > -
> > >     void create_inferior (const char *, const std::string &,
> > >   			char **, int) override;
> > > diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> > > index c183b60e81a..d4e678643a0 100644
> > > --- a/gdb/infcmd.c
> > > +++ b/gdb/infcmd.c
> > > @@ -2541,6 +2541,10 @@ attach_command (const char *args, int from_tty)
> > >        shouldn't refer to attach_target again.  */
> > >     attach_target = NULL;
> > > +  /* Enable async mode if it is supported by the target.  */
> > > +  if (target_can_async_p ())
> > > +    target_async (1);
> > > +
> > >     /* Set up the "saved terminal modes" of the inferior
> > >        based on what modes we are starting it with.  */
> > >     target_terminal::init ();
> > > diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
> > > index 3a6d48f08e6..b80fa25c902 100644
> > > --- a/gdb/linux-nat.c
> > > +++ b/gdb/linux-nat.c
> > > @@ -1214,9 +1214,6 @@ linux_nat_target::attach (const char *args, int from_tty)
> > >        threads and associate pthread info with each LWP.  */
> > >     linux_proc_attach_tgid_threads (lp->ptid.pid (),
> > >   				  attach_proc_task_lwp_callback);
> > > -
> > > -  if (target_can_async_p ())
> > > -    target_async (1);
> > >   }
> > >   /* Get pending signal of THREAD as a host signal number, for detaching
> > > diff --git a/gdb/remote.c b/gdb/remote.c
> > > index 552481fc551..2fb95c1e93b 100644
> > > --- a/gdb/remote.c
> > > +++ b/gdb/remote.c
> > > @@ -6095,7 +6095,6 @@ extended_remote_target::attach (const char *args, int from_tty)
> > >         gdb_assert (wait_status == NULL);
> > >         gdb_assert (target_can_async_p ());
> > > -      target_async (1);
> > >       }
> > 
> > I think there's a second call to target_async that can be deleted a
> > few lines earlier, when the condition  '!target_is_non_stop_p ()' and
> > then 'target_can_async_p ()' is true.  I'd be tempted to replace that
> > earlier occurrence with 'gdb_assert (target_can_async_p ());' though.
> 
> Hmm, yes, that call to target_async is effectively the last statement in that
> set of conditions so can be elided.  I'm not sure adding an assertion in its
> place makes sense though as it is already inside the target_can_async_p()
> block, so you would end up with:
> 
>    if (target_can_async_p())
>      {
>        ..
>        gdb_assert (target_can_async_p());
>      }

Good spot!  Just getting rid of the code will be enough then.

Thanks,
Andrew


> 
> Or do you mean that the first if there though can be replaced by the assertion
> instead?  That I'm not sure of (and seems like a behavior change that
> probably warrants its own patch).
> 
> -- 
> John Baldwin
>