[PATCH v2 08/13] fbsd-nat: Implement async target support.

[Andrew Burgess]

* John Baldwin <jhb@FreeBSD.org> [2021-08-03 11:49:55 -0700]:

> This is a fairly simple version of async target support.
> 
> Synchronous mode still uses blocking waitpid() calls in
> inf_ptrace::wait() unlike the Linux native target which always uses
> WNOHANG and uses sigsuspend() for synchronous operation.
> 
> Asynchronous mode registers an event pipe with the core as a file
> handle and writes to the pipe when SIGCHLD is raised.  TARGET_WNOHANG
> is handled by inf_ptrace::wait().
> ---
>  gdb/fbsd-nat.c | 155 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  gdb/fbsd-nat.h |  12 ++++
>  2 files changed, 165 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
> index 0ae1195791c..36ce00164dd 100644
> --- a/gdb/fbsd-nat.c
> +++ b/gdb/fbsd-nat.c
> @@ -19,14 +19,18 @@
>  
>  #include "defs.h"
>  #include "gdbsupport/byte-vector.h"
> +#include "gdbsupport/event-loop.h"
> +#include "gdbsupport/event-pipe.h"
>  #include "gdbcore.h"
>  #include "inferior.h"
>  #include "regcache.h"
>  #include "regset.h"
>  #include "gdbarch.h"
>  #include "gdbcmd.h"
> +#include "gdbsupport/gdb-sigmask.h"
>  #include "gdbthread.h"
>  #include "gdbsupport/gdb_wait.h"
> +#include "inf-loop.h"
>  #include "inf-ptrace.h"
>  #include <sys/types.h>
>  #ifdef HAVE_SYS_PROCCTL_H
> @@ -925,6 +929,118 @@ fbsd_nat_target::update_thread_list ()
>  #endif
>  }
>  
> +/* Async mode support.  */
> +
> +static event_pipe fbsd_nat_event_pipe;
> +
> +/* Implement the "can_async_p" target method.  */
> +
> +bool
> +fbsd_nat_target::can_async_p ()
> +{
> +  /* Use async unless the user explicitly prevented it via the "maint
> +     set target-async" command.  */
> +  return target_async_permitted;
> +}

With this patch:

  https://sourceware.org/pipermail/gdb-patches/2021-November/183764.html

which is not merged yet, this can be changed to:

    bool
    fbsd_nat_target::can_async_p ()
    {
      assert (target_async_permitted);
      return true;
    }

I'm happy to make this change if this work gets merged first - I'm
just noting this here.

> +
> +/* Implement the "is_async_p" target method.  */
> +
> +bool
> +fbsd_nat_target::is_async_p ()
> +{
> +  return fbsd_nat_event_pipe.is_open ();
> +}
> +
> +/* Implement the "async_wait_fd" target method.  */
> +
> +int
> +fbsd_nat_target::async_wait_fd ()
> +{
> +  return fbsd_nat_event_pipe.event_fd ();
> +}
> +
> +/* SIGCHLD handler notifies the event-loop in async mode.  */
> +
> +static void
> +sigchld_handler (int signo)
> +{
> +  int old_errno = errno;
> +
> +  if (fbsd_nat_event_pipe.is_open ())
> +    fbsd_nat_event_pipe.mark ();
> +
> +  errno = old_errno;
> +}
> +
> +/* Callback registered with the target events file descriptor.  */
> +
> +static void
> +handle_target_event (int error, gdb_client_data client_data)
> +{
> +  inferior_event_handler (INF_REG_EVENT);
> +}
> +
> +/* Implement the "async" target method.  */
> +
> +void
> +fbsd_nat_target::async (int enable)
> +{
> +  if ((enable != 0) == is_async_p ())
> +    return;
> +
> +  /* Block SIGCHILD while we create/destroy the pipe, as the handler
> +     writes to it.  */
> +
> +  sigset_t chld_mask, prev_mask;
> +  sigemptyset (&chld_mask);
> +  sigaddset (&chld_mask, SIGCHLD);
> +  gdb_sigmask (SIG_BLOCK, &chld_mask, &prev_mask);
> +
> +  if (enable)
> +    {
> +      if (!fbsd_nat_event_pipe.open ())
> +	internal_error (__FILE__, __LINE__, "failed to create event pipe.");
> +
> +      add_file_handler (fbsd_nat_event_pipe.event_fd (),
> +			handle_target_event, NULL, "fbsd-nat");
> +
> +      /* Trigger a poll in case there are pending events to
> +	 handle.  */
> +      fbsd_nat_event_pipe.mark ();
> +    }
> +  else
> +    {
> +      delete_file_handler (fbsd_nat_event_pipe.event_fd ());
> +      fbsd_nat_event_pipe.close ();
> +    }
> +
> +  gdb_sigmask (SIG_SETMASK, &prev_mask, NULL);
> +}
> +
> +/* Implement the "close" target method.  */
> +
> +void
> +fbsd_nat_target::close ()
> +{
> +  if (is_async_p ())
> +    async (0);
> +
> +  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
> @@ -1164,8 +1280,8 @@ fbsd_handle_debug_trap (fbsd_nat_target *target, ptid_t ptid,
>     the status in *OURSTATUS.  */
>  
>  ptid_t
> -fbsd_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
> -		       target_wait_flags target_options)
> +fbsd_nat_target::wait_1 (ptid_t ptid, struct target_waitstatus *ourstatus,
> +			 target_wait_flags target_options)
>  {
>    ptid_t wptid;
>  
> @@ -1377,6 +1493,33 @@ fbsd_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
>      }
>  }
>  
> +ptid_t
> +fbsd_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
> +		       target_wait_flags target_options)
> +{
> +  ptid_t wptid;
> +
> +  fbsd_nat_debug_printf ("[%s], [%s]", target_pid_to_str (ptid).c_str (),
> +			 target_options_to_string (target_options).c_str ());
> +
> +  /* Ensure any subsequent events trigger a new event in the loop.  */
> +  if (is_async_p ())
> +    fbsd_nat_event_pipe.flush ();
> +
> +  wptid = wait_1 (ptid, ourstatus, target_options);
> +
> +  /* If we are in async mode and found an event, there may still be
> +     another event pending.  Trigger the event pipe so that that the
> +     event loop keeps polling until no event is returned.  */
> +  if (is_async_p () && ourstatus->kind != TARGET_WAITKIND_IGNORE)
> +    fbsd_nat_event_pipe.mark ();

I notice you use a different condition here to the one present in
linux-nat.c, specifically, if you get an event of type
TARGET_WAITKIND_NO_RESUMED, you'll remark the pipe, while Linux
doesn't.

Is there a reason for this difference?

> +
> +  fbsd_nat_debug_printf ("returning [%s], [%s]",
> +			 target_pid_to_str (wptid).c_str (),
> +			 target_waitstatus_to_string (ourstatus).c_str ());
> +  return wptid;
> +}
> +
>  #ifdef USE_SIGTRAP_SIGINFO
>  /* Implement the "stopped_by_sw_breakpoint" target_ops method.  */
>  
> @@ -1510,6 +1653,11 @@ fbsd_nat_target::follow_fork (ptid_t child_ptid, target_waitkind fork_kind,
>  	  /* Schedule a fake VFORK_DONE event to report on the next
>  	     wait.  */
>  	  fbsd_add_vfork_done (inferior_ptid);
> +
> +	  /* If we're in async mode, need to tell the event loop
> +	     there's something here to process.  */
> +	  if (is_async_p ())
> +	    fbsd_nat_event_pipe.mark ();

If feels like this marking of the event pipe should be within
fbsd_add_vfork_done maybe?  The two actions, queuing the fake event,
and marking the async event pipe are tightly coupled, right?

I guess you might need to make fbsd_add_vfork_done a private member
function so you can easily call is_async_p(), or just call
'target_is_async_p()' instead.

Thanks,
Andrew

>  	}
>  #endif
>      }
> @@ -1667,4 +1815,7 @@ Enables printf debugging output."),
>  			   NULL,
>  			   &show_fbsd_nat_debug,
>  			   &setdebuglist, &showdebuglist);
> +
> +  /* Install a SIGCHLD handler.  */
> +  signal (SIGCHLD, sigchld_handler);
>  }
> diff --git a/gdb/fbsd-nat.h b/gdb/fbsd-nat.h
> index 3fb502ca8d0..9dd9017c1c3 100644
> --- a/gdb/fbsd-nat.h
> +++ b/gdb/fbsd-nat.h
> @@ -66,9 +66,19 @@ class fbsd_nat_target : public inf_ptrace_target
>  
>    void update_thread_list () override;
>  
> +  bool can_async_p () override;
> +  bool is_async_p () override;
> +
> +  int async_wait_fd () override;
> +  void async (int) override;
> +
> +  void close () override;
> +
>    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;
>  
> @@ -107,6 +117,8 @@ class fbsd_nat_target : public inf_ptrace_target
>    bool supports_disable_randomization () override;
>  
>  private:
> +  ptid_t wait_1 (ptid_t, struct target_waitstatus *, target_wait_flags);
> +
>    /* Helper routines for use in fetch_registers and store_registers in
>       subclasses.  These routines fetch and store a single set of
>       registers described by REGSET.  The REGSET's 'regmap' field must
> -- 
> 2.31.1
>