[RFC,4/7] fbsd-nat: Implement async target support.

Message ID 20210607170932.3954-5-jhb@FreeBSD.org
State New
Headers show
Series
  • FreeBSD target async mode and related refactoring
Related show

Commit Message

John Baldwin June 7, 2021, 5:09 p.m.
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.  WNOHANG is
passed to waitpid when TARGET_WNOHANG is passed to the
inf_ptrace::wait.

gdb/ChangeLog:

	* fbsd-nat.c: Include gdbsupport/event-loop.h,
	gdbsupport/event-pipe.h, gdbsupport/gdb-sigmask.h, and inf-loop.h.
	(fbsd_nat_event_pipe, fbsd_nat_target::can_async_p)
	(fbsd_nat_target::is_async_p, fbsd_nat_target::async_wait_fd)
	(sigchld_handler, handle_target_event, fbsd_nat_target::async)
	(fbsd_nat_target::close, fbsd_nat_target::attach): New.
	(fbsd_nat_target::wait): Rename to ...
	(fbsd_nat_target::wait_1): ... this.
	(fbsd_nat_target::wait): New.
	(fbsd_nat_target::follow_fork): Trigger the async event pipe when
	adding a vfork_done event.
	(_initialize_fbsd_nat): Install SIGCHLD handler.
	* fbsd-nat.h (fbsd_nat_target::can_async_p)
	(fbsd_nat_target::is_async_p, fbsd_nat_target::async_wait_fd)
	(fbsd_nat_target::async, fbsd_nat_target::close)
	(fbsd_nat_target::attach, fbsd_nat_target::wait_1): New.
	* inf-ptrace.c (inf_ptrace_target::wait): Pass WNOHANG when
	TARGET_WNOHANG is set.  Handle waitpid return of 0 and ECHILD
	errors.
---
 gdb/ChangeLog    |  22 +++++++
 gdb/fbsd-nat.c   | 161 ++++++++++++++++++++++++++++++++++++++++++++++-
 gdb/fbsd-nat.h   |  13 ++++
 gdb/inf-ptrace.c |  27 +++++++-
 4 files changed, 218 insertions(+), 5 deletions(-)

-- 
2.31.1

Comments

John Baldwin June 7, 2021, 10:49 p.m. | #1
A few more things I thought of today:

On 6/7/21 10:09 AM, John Baldwin wrote:
> 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.  WNOHANG is

> passed to waitpid when TARGET_WNOHANG is passed to the

> inf_ptrace::wait.

> 

> diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c

> index 581c04d5f8..e5128266b8 100644

> --- a/gdb/fbsd-nat.c

> +++ b/gdb/fbsd-nat.c

> @@ -1374,6 +1496,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 (target_is_async_p ())

> +    fbsd_nat_event_pipe.flush ();


In target class methods like this I should likely just use 'is_async_p ()' instead
to get the benefit of the "final" tag on the class for resolving virtual methods.
It also avoids the somewhat confusing flow since target_is_async_p invoking this on
a target class that we are assuming is the current class.

> diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c

> index b6fa71fd2c..ee0331db09 100644

> --- a/gdb/inf-ptrace.c

> +++ b/gdb/inf-ptrace.c

> @@ -300,10 +300,14 @@ inf_ptrace_target::resume (ptid_t ptid, int step, enum gdb_signal signal)

>   

>   ptid_t

>   inf_ptrace_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,

> -			 target_wait_flags options)

> +			 target_wait_flags target_options)

>   {

>     pid_t pid;

> -  int status, save_errno;

> +  int options, status, save_errno;

> +

> +  options = 0;

> +  if (target_options & TARGET_WNOHANG)

> +    options |= WNOHANG;

>   

>     do

>       {

> @@ -311,15 +315,32 @@ inf_ptrace_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,

>   

>         do

>   	{

> -	  pid = waitpid (ptid.pid (), &status, 0);

> +	  pid = waitpid (ptid.pid (), &status, options);

>   	  save_errno = errno;

>   	}

>         while (pid == -1 && errno == EINTR);

>   

>         clear_sigint_trap ();

>   

> +      if (pid == 0)

> +	{

> +	  gdb_assert (target_options & TARGET_WNOHANG);

> +	  ourstatus->kind = TARGET_WAITKIND_IGNORE;

> +	  return minus_one_ptid;

> +	}

> +

>         if (pid == -1)

>   	{

> +	  /* In async mode the SIGCHLD might have raced and triggered

> +	     a check for an event that had already been reported.  If

> +	     the event was the exit of the only remaining child,

> +	     waitpid() will fail with ECHILD.  */

> +	  if (ptid == minus_one_ptid && errno == ECHILD)

> +	    {

> +	      ourstatus->kind = TARGET_WAITKIND_IGNORE;

> +	      return minus_one_ptid;

> +	    }

> +

>   	  fprintf_unfiltered (gdb_stderr,

>   			      _("Child process unexpectedly missing: %s.\n"),

>   			      safe_strerror (save_errno));


Note that the code below this hunk returns a "made up" wait status that uses
inferior_ptid.  Since inferior_ptid is always 0 at this point, it always
triggers an assertion later on when the core sees an all-zero ptid.  I
think we should probably make this code just internal_error() when waitpid
returns an error rather than tripping an assertion later.

-- 
John Baldwin

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index cb301e1e81..fba798a63d 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,25 @@ 
+2021-06-04  John Baldwin  <jhb@FreeBSD.org>
+
+	* fbsd-nat.c: Include gdbsupport/event-loop.h,
+	gdbsupport/event-pipe.h, gdbsupport/gdb-sigmask.h, and inf-loop.h.
+	(fbsd_nat_event_pipe, fbsd_nat_target::can_async_p)
+	(fbsd_nat_target::is_async_p, fbsd_nat_target::async_wait_fd)
+	(sigchld_handler, handle_target_event, fbsd_nat_target::async)
+	(fbsd_nat_target::close, fbsd_nat_target::attach): New.
+	(fbsd_nat_target::wait): Rename to ...
+	(fbsd_nat_target::wait_1): ... this.
+	(fbsd_nat_target::wait): New.
+	(fbsd_nat_target::follow_fork): Trigger the async event pipe when
+	adding a vfork_done event.
+	(_initialize_fbsd_nat): Install SIGCHLD handler.
+	* fbsd-nat.h (fbsd_nat_target::can_async_p)
+	(fbsd_nat_target::is_async_p, fbsd_nat_target::async_wait_fd)
+	(fbsd_nat_target::async, fbsd_nat_target::close)
+	(fbsd_nat_target::attach, fbsd_nat_target::wait_1): New.
+	* inf-ptrace.c (inf_ptrace_target::wait): Pass WNOHANG when
+	TARGET_WNOHANG is set.  Handle waitpid return of 0 and ECHILD
+	errors.
+
 2021-06-04  John Baldwin  <jhb@FreeBSD.org>
 
 	* linux-nat.c: Include gdbsupport/event-pipe.h.
diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
index 581c04d5f8..e5128266b8 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>
 #include <sys/procfs.h>
@@ -922,6 +926,124 @@  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;
+}
+
+/* Implement the "is_async_p" target method.  */
+
+bool
+fbsd_nat_target::is_async_p ()
+{
+  return fbsd_nat_event_pipe.active ();
+}
+
+/* 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.active ())
+    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, instead
+   * do_target_wait_1 only strips TARGET_WNOHANG if target_can_async_p
+   * is false even if the target isn't actually async (target_async_p
+   * is false).  As a result, this must enable async mode here to
+   * avoid racing with the stop reported for attach.
+   */
+  if (target_can_async_p ())
+    target_async (1);
+}
+
+
 #ifdef TDP_RFPPWAIT
 /*
   To catch fork events, PT_FOLLOW_FORK is set on every traced process
@@ -1161,8 +1283,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;
 
@@ -1374,6 +1496,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 (target_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 (target_is_async_p () && ourstatus->kind != TARGET_WAITKIND_IGNORE)
+    fbsd_nat_event_pipe.mark ();
+
+  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.  */
 
@@ -1445,6 +1594,11 @@  fbsd_nat_target::follow_fork (bool follow_child, bool detach_fork)
 	  /* 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 (target_is_async_p ())
+	    fbsd_nat_event_pipe.mark ();
 	}
 #endif
     }
@@ -1546,4 +1700,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 772655d320..7a0157a5ab 100644
--- a/gdb/fbsd-nat.h
+++ b/gdb/fbsd-nat.h
@@ -64,9 +64,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 resume (ptid_t, int, enum gdb_signal) override;
 
   ptid_t wait (ptid_t, struct target_waitstatus *, target_wait_flags) override;
@@ -98,6 +108,9 @@  class fbsd_nat_target : public inf_ptrace_target
 #endif
 
   bool supports_multi_process () override;
+
+private:
+  ptid_t wait_1 (ptid_t, struct target_waitstatus *, target_wait_flags);
 };
 
 #endif /* fbsd-nat.h */
diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
index b6fa71fd2c..ee0331db09 100644
--- a/gdb/inf-ptrace.c
+++ b/gdb/inf-ptrace.c
@@ -300,10 +300,14 @@  inf_ptrace_target::resume (ptid_t ptid, int step, enum gdb_signal signal)
 
 ptid_t
 inf_ptrace_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
-			 target_wait_flags options)
+			 target_wait_flags target_options)
 {
   pid_t pid;
-  int status, save_errno;
+  int options, status, save_errno;
+
+  options = 0;
+  if (target_options & TARGET_WNOHANG)
+    options |= WNOHANG;
 
   do
     {
@@ -311,15 +315,32 @@  inf_ptrace_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
 
       do
 	{
-	  pid = waitpid (ptid.pid (), &status, 0);
+	  pid = waitpid (ptid.pid (), &status, options);
 	  save_errno = errno;
 	}
       while (pid == -1 && errno == EINTR);
 
       clear_sigint_trap ();
 
+      if (pid == 0)
+	{
+	  gdb_assert (target_options & TARGET_WNOHANG);
+	  ourstatus->kind = TARGET_WAITKIND_IGNORE;
+	  return minus_one_ptid;
+	}
+
       if (pid == -1)
 	{
+	  /* In async mode the SIGCHLD might have raced and triggered
+	     a check for an event that had already been reported.  If
+	     the event was the exit of the only remaining child,
+	     waitpid() will fail with ECHILD.  */
+	  if (ptid == minus_one_ptid && errno == ECHILD)
+	    {
+	      ourstatus->kind = TARGET_WAITKIND_IGNORE;
+	      return minus_one_ptid;
+	    }
+
 	  fprintf_unfiltered (gdb_stderr,
 			      _("Child process unexpectedly missing: %s.\n"),
 			      safe_strerror (save_errno));