[PATCH v4 00/13] FreeBSD target async mode and related refactoring

[PATCH v4 00/13] FreeBSD target async mode and related refactoring

[John Baldwin]

Changes since V2:

- Rebased for recent changes (waitkind setters from Simon
  and can_async_p changes from Andrew)

- Fixes based on feedback from V2.

- Patch 5 has an extended commit message and now includes changes to
  the remote and record targets in addition to the Linux native target.

Changes since V3:

- Patch 6 was merged into patch 5, commit log expanded and added
  remote target.

- Patch 8 includes an updated can_async_p implementation.

John Baldwin (13):
  gdbsupport: Add an event-pipe class.
  gdb linux-nat: Convert linux_nat_event_pipe to the event_pipe class.
  gdbserver linux-low: Convert linux_event_pipe to the event_pipe class.
  Don't enable async mode at the end of target ::resume methods.
  do_target_wait_1: Clear TARGET_WNOHANG if the target isn't async.
  inf-ptrace: Raise an error if waitpid() fails.
  inf-ptrace: Support async targets in inf_ptrace_target::wait.
  fbsd-nat: Implement async target support.
  fbsd-nat: Include ptrace operation in error messages.
  fbsd-nat: Various cleanups to the ::resume entry debug message.
  fbsd-nat: Return nullptr rather than failing ::thread_name.
  Enable async mode in the target in attach_cmd.
  inf-ptrace: Add an event_pipe to be used for async mode in subclasses.

 gdb/fbsd-nat.c           | 150 ++++++++++++++++++++++++++++++-----
 gdb/fbsd-nat.h           |   6 ++
 gdb/inf-ptrace.c         |  49 +++++++++---
 gdb/inf-ptrace.h         |  30 +++++++
 gdb/infcmd.c             |   4 +
 gdb/infrun.c             |   2 +-
 gdb/linux-nat.c          | 164 +++++++--------------------------------
 gdb/linux-nat.h          |   4 -
 gdb/record-full.c        |  10 ---
 gdb/remote.c             |  13 ----
 gdbserver/linux-low.cc   |  43 +++-------
 gdbsupport/Makefile.am   |   5 ++
 gdbsupport/Makefile.in   |   9 ++-
 gdbsupport/configure     |  15 ++++
 gdbsupport/configure.ac  |   3 +
 gdbsupport/event-pipe.cc | 101 ++++++++++++++++++++++++
 gdbsupport/event-pipe.h  |  60 ++++++++++++++
 17 files changed, 440 insertions(+), 228 deletions(-)
 create mode 100644 gdbsupport/event-pipe.cc
 create mode 100644 gdbsupport/event-pipe.h

-- 
2.32.0

[PATCH v4 01/13] gdbsupport: Add an event-pipe class.

[John Baldwin]

This pulls out the implementation of an event pipe used to implement
target async support in both linux-low.cc (gdbserver) and linux-nat.c
(gdb).

This will be used to replace the existing event pipe in linux-low.cc
and linux-nat.c in future commits.

Co-Authored-By: Lancelot SIX <lsix@lancelotsix.com>
---
 gdbsupport/Makefile.am   |   5 ++
 gdbsupport/Makefile.in   |   9 +++-
 gdbsupport/configure     |  15 ++++++
 gdbsupport/configure.ac  |   3 ++
 gdbsupport/event-pipe.cc | 101 +++++++++++++++++++++++++++++++++++++++
 gdbsupport/event-pipe.h  |  60 +++++++++++++++++++++++
 6 files changed, 191 insertions(+), 2 deletions(-)
 create mode 100644 gdbsupport/event-pipe.cc
 create mode 100644 gdbsupport/event-pipe.h

diff --git a/gdbsupport/Makefile.am b/gdbsupport/Makefile.am
index 6d4678c8c9b..3426d813c16 100644
--- a/gdbsupport/Makefile.am
+++ b/gdbsupport/Makefile.am
@@ -35,6 +35,10 @@ if SELFTEST
 selftest = selftest.cc
 endif
 
+if HAVE_PIPE_OR_PIPE2
+eventpipe = event-pipe.cc
+endif
+
 libgdbsupport_a_SOURCES = \
     agent.cc \
     btrace-common.cc \
@@ -71,6 +75,7 @@ libgdbsupport_a_SOURCES = \
     tdesc.cc \
     thread-pool.cc \
     xml-utils.cc \
+    ${eventpipe} \
     $(selftest)
 
 # Double-check that no defines are missing from our configury.
diff --git a/gdbsupport/Makefile.in b/gdbsupport/Makefile.in
index d7f2d4914b0..c9a8398ee44 100644
--- a/gdbsupport/Makefile.in
+++ b/gdbsupport/Makefile.in
@@ -144,7 +144,8 @@ am__v_AR_0 = @echo "  AR      " $@;
 am__v_AR_1 = 
 libgdbsupport_a_AR = $(AR) $(ARFLAGS)
 libgdbsupport_a_LIBADD =
-@SELFTEST_TRUE@am__objects_1 = selftest.$(OBJEXT)
+@HAVE_PIPE_OR_PIPE2_TRUE@am__objects_1 = event-pipe.$(OBJEXT)
+@SELFTEST_TRUE@am__objects_2 = selftest.$(OBJEXT)
 am_libgdbsupport_a_OBJECTS = agent.$(OBJEXT) btrace-common.$(OBJEXT) \
 	buffer.$(OBJEXT) cleanups.$(OBJEXT) common-debug.$(OBJEXT) \
 	common-exceptions.$(OBJEXT) common-inferior.$(OBJEXT) \
@@ -158,7 +159,8 @@ am_libgdbsupport_a_OBJECTS = agent.$(OBJEXT) btrace-common.$(OBJEXT) \
 	run-time-clock.$(OBJEXT) safe-strerror.$(OBJEXT) \
 	scoped_mmap.$(OBJEXT) search.$(OBJEXT) signals.$(OBJEXT) \
 	signals-state-save-restore.$(OBJEXT) tdesc.$(OBJEXT) \
-	thread-pool.$(OBJEXT) xml-utils.$(OBJEXT) $(am__objects_1)
+	thread-pool.$(OBJEXT) xml-utils.$(OBJEXT) $(am__objects_1) \
+	$(am__objects_2)
 libgdbsupport_a_OBJECTS = $(am_libgdbsupport_a_OBJECTS)
 AM_V_P = $(am__v_P_@AM_V@)
 am__v_P_ = $(am__v_P_@AM_DEFAULT_V@)
@@ -358,6 +360,7 @@ AM_CPPFLAGS = -I$(srcdir)/../include -I$(srcdir)/../gdb \
 AM_CXXFLAGS = $(WARN_CFLAGS) $(WERROR_CFLAGS)
 noinst_LIBRARIES = libgdbsupport.a
 @SELFTEST_TRUE@selftest = selftest.cc
+@HAVE_PIPE_OR_PIPE2_TRUE@eventpipe = event-pipe.cc
 libgdbsupport_a_SOURCES = \
     agent.cc \
     btrace-common.cc \
@@ -394,6 +397,7 @@ libgdbsupport_a_SOURCES = \
     tdesc.cc \
     thread-pool.cc \
     xml-utils.cc \
+    ${eventpipe} \
     $(selftest)
 
 all: config.h
@@ -476,6 +480,7 @@ distclean-compile:
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/environ.Po@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/errors.Po@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/event-loop.Po@am__quote@
+@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/event-pipe.Po@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/fileio.Po@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/filestuff.Po@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/format.Po@am__quote@
diff --git a/gdbsupport/configure b/gdbsupport/configure
index 0b4e81a9c16..36bb7090cf6 100755
--- a/gdbsupport/configure
+++ b/gdbsupport/configure
@@ -626,6 +626,8 @@ LTLIBOBJS
 LIBOBJS
 WERROR_CFLAGS
 WARN_CFLAGS
+HAVE_PIPE_OR_PIPE2_FALSE
+HAVE_PIPE_OR_PIPE2_TRUE
 SELFTEST_FALSE
 SELFTEST_TRUE
 LTLIBIPT
@@ -9980,6 +9982,15 @@ else
 fi
 
 
+ if test x$ac_cv_func_pipe = xyes -o x$ac_cv_func_pipe2 = xyes ; then
+  HAVE_PIPE_OR_PIPE2_TRUE=
+  HAVE_PIPE_OR_PIPE2_FALSE='#'
+else
+  HAVE_PIPE_OR_PIPE2_TRUE='#'
+  HAVE_PIPE_OR_PIPE2_FALSE=
+fi
+
+
 # Check the return and argument types of ptrace.
 
 
@@ -10511,6 +10522,10 @@ if test -z "${SELFTEST_TRUE}" && test -z "${SELFTEST_FALSE}"; then
   as_fn_error $? "conditional \"SELFTEST\" was never defined.
 Usually this means the macro was only invoked conditionally." "$LINENO" 5
 fi
+if test -z "${HAVE_PIPE_OR_PIPE2_TRUE}" && test -z "${HAVE_PIPE_OR_PIPE2_FALSE}"; then
+  as_fn_error $? "conditional \"HAVE_PIPE_OR_PIPE2\" was never defined.
+Usually this means the macro was only invoked conditionally." "$LINENO" 5
+fi
 
 : "${CONFIG_STATUS=./config.status}"
 ac_write_fail=0
diff --git a/gdbsupport/configure.ac b/gdbsupport/configure.ac
index f10a856fe24..1f596369931 100644
--- a/gdbsupport/configure.ac
+++ b/gdbsupport/configure.ac
@@ -53,6 +53,9 @@ GDB_AC_COMMON
 GDB_AC_SELFTEST
 AM_CONDITIONAL(SELFTEST, $enable_unittests)
 
+AM_CONDITIONAL(HAVE_PIPE_OR_PIPE2,
+   [test x$ac_cv_func_pipe = xyes -o x$ac_cv_func_pipe2 = xyes ])
+
 # Check the return and argument types of ptrace.
 GDB_AC_PTRACE
 
diff --git a/gdbsupport/event-pipe.cc b/gdbsupport/event-pipe.cc
new file mode 100644
index 00000000000..2b56b2fac8e
--- /dev/null
+++ b/gdbsupport/event-pipe.cc
@@ -0,0 +1,101 @@
+/* Event pipe for GDB, the GNU debugger.
+
+   Copyright (C) 2021 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include "gdbsupport/common-defs.h"
+#include "gdbsupport/event-pipe.h"
+#include "gdbsupport/filestuff.h"
+
+#include <errno.h>
+#include <fcntl.h>
+#include <unistd.h>
+
+event_pipe::~event_pipe ()
+{
+  if (is_open ())
+    close ();
+}
+
+/* See event-pipe.h.  */
+
+bool
+event_pipe::open ()
+{
+  if (is_open ())
+    return false;
+
+  if (gdb_pipe_cloexec (m_fds) == -1)
+    return false;
+
+  if (fcntl (m_fds[0], F_SETFL, O_NONBLOCK) == -1
+      || fcntl (m_fds[1], F_SETFL, O_NONBLOCK) == -1)
+    {
+      close ();
+      return false;
+    }
+
+  return true;
+}
+
+/* See event-pipe.h.  */
+
+void
+event_pipe::close ()
+{
+  ::close (m_fds[0]);
+  ::close (m_fds[1]);
+  m_fds[0] = -1;
+  m_fds[1] = -1;
+}
+
+/* See event-pipe.h.  */
+
+void
+event_pipe::flush ()
+{
+  int ret;
+  char buf;
+
+  do
+    {
+      ret = read (m_fds[0], &buf, 1);
+    }
+  while (ret >= 0 || (ret == -1 && errno == EINTR));
+}
+
+/* See event-pipe.h.  */
+
+void
+event_pipe::mark ()
+{
+  int ret;
+
+  /* It doesn't really matter what the pipe contains, as long we end
+     up with something in it.  Might as well flush the previous
+     left-overs.  */
+  flush ();
+
+  do
+    {
+      ret = write (m_fds[1], "+", 1);
+    }
+  while (ret == -1 && errno == EINTR);
+
+  /* Ignore EAGAIN.  If the pipe is full, the event loop will already
+     be awakened anyway.  */
+}
diff --git a/gdbsupport/event-pipe.h b/gdbsupport/event-pipe.h
new file mode 100644
index 00000000000..50679e470e4
--- /dev/null
+++ b/gdbsupport/event-pipe.h
@@ -0,0 +1,60 @@
+/* Event pipe for GDB, the GNU debugger.
+
+   Copyright (C) 2021 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#ifndef COMMON_EVENT_PIPE_H
+#define COMMON_EVENT_PIPE_H
+
+/* An event pipe used as a waitable file in the event loop in place of
+   some other event associated with a signal.  The handler for the
+   signal marks the event pipe to force a wakeup in the event loop.
+   This uses the well-known self-pipe trick.  */
+
+class event_pipe
+{
+public:
+  event_pipe() = default;
+  ~event_pipe();
+
+  DISABLE_COPY_AND_ASSIGN (event_pipe);
+
+  /* Create a new pipe.  */
+  bool open ();
+
+  /* Close the pipe.  */
+  void close ();
+
+  /* True if the event pipe has been opened.  */
+  bool is_open () const
+  { return m_fds[0] != -1; }
+
+  /* The file descriptor of the waitable file to use in the event
+     loop.  */
+  int event_fd () const
+  { return m_fds[0]; }
+
+  /* Flush the event pipe.  */
+  void flush ();
+
+  /* Put something in the pipe, so the event loop wakes up.  */
+  void mark ();
+private:
+  int m_fds[2] = { -1, -1 };
+};
+
+#endif /* COMMON_EVENT_PIPE_H */
-- 
2.32.0

[PATCH v4 02/13] gdb linux-nat: Convert linux_nat_event_pipe to the event_pipe class.

[John Baldwin]

Use event_pipe from gdbsupport in place of the existing file
descriptor array.
---
 gdb/linux-nat.c | 59 ++++++++++++++-----------------------------------
 1 file changed, 16 insertions(+), 43 deletions(-)

diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 656a0975ddd..b2c8ce48634 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -48,6 +48,7 @@
 #include <fcntl.h>		/* for O_RDONLY */
 #include "inf-loop.h"
 #include "gdbsupport/event-loop.h"
+#include "gdbsupport/event-pipe.h"
 #include "event-top.h"
 #include <pwd.h>
 #include <sys/types.h>
@@ -111,11 +112,11 @@ and target events, so neither blocking waitpid nor sigsuspend are
 viable options.  Instead, we should asynchronously notify the GDB main
 event loop whenever there's an unprocessed event from the target.  We
 detect asynchronous target events by handling SIGCHLD signals.  To
-notify the event loop about target events, the self-pipe trick is used
---- a pipe is registered as waitable event source in the event loop,
+notify the event loop about target events, an event pipe is used
+--- the pipe is registered as waitable event source in the event loop,
 the event loop select/poll's on the read end of this pipe (as well on
-other event sources, e.g., stdin), and the SIGCHLD handler writes a
-byte to this pipe.  This is more portable than relying on
+other event sources, e.g., stdin), and the SIGCHLD handler marks the
+event pipe to raise an event.  This is more portable than relying on
 pselect/ppoll, since on kernels that lack those syscalls, libc
 emulates them with select/poll+sigprocmask, and that is racy
 (a.k.a. plain broken).
@@ -218,26 +219,18 @@ static int report_thread_events;
 
 /* Async mode support.  */
 
-/* The read/write ends of the pipe registered as waitable file in the
-   event loop.  */
-static int linux_nat_event_pipe[2] = { -1, -1 };
+/* The event pipe registered as a waitable file in the event loop.  */
+static event_pipe linux_nat_event_pipe;
 
 /* True if we're currently in async mode.  */
-#define linux_is_async_p() (linux_nat_event_pipe[0] != -1)
+#define linux_is_async_p() (linux_nat_event_pipe.is_open ())
 
 /* Flush the event pipe.  */
 
 static void
 async_file_flush (void)
 {
-  int ret;
-  char buf;
-
-  do
-    {
-      ret = read (linux_nat_event_pipe[0], &buf, 1);
-    }
-  while (ret >= 0 || (ret == -1 && errno == EINTR));
+  linux_nat_event_pipe.flush ();
 }
 
 /* Put something (anything, doesn't matter what, or how much) in event
@@ -247,21 +240,7 @@ async_file_flush (void)
 static void
 async_file_mark (void)
 {
-  int ret;
-
-  /* It doesn't really matter what the pipe contains, as long we end
-     up with something in it.  Might as well flush the previous
-     left-overs.  */
-  async_file_flush ();
-
-  do
-    {
-      ret = write (linux_nat_event_pipe[1], "+", 1);
-    }
-  while (ret == -1 && errno == EINTR);
-
-  /* Ignore EAGAIN.  If the pipe is full, the event loop will already
-     be awakened anyway.  */
+  linux_nat_event_pipe.mark ();
 }
 
 static int kill_lwp (int lwpid, int signo);
@@ -4135,7 +4114,7 @@ sigchld_handler (int signo)
     gdb_stdlog->write_async_safe ("sigchld\n", sizeof ("sigchld\n") - 1);
 
   if (signo == SIGCHLD
-      && linux_nat_event_pipe[0] != -1)
+      && linux_nat_event_pipe.is_open ())
     async_file_mark (); /* Let the event loop know that there are
 			   events to handle.  */
 
@@ -4167,19 +4146,13 @@ linux_async_pipe (int enable)
 
       if (enable)
 	{
-	  if (gdb_pipe_cloexec (linux_nat_event_pipe) == -1)
+	  if (!linux_nat_event_pipe.open ())
 	    internal_error (__FILE__, __LINE__,
 			    "creating event pipe failed.");
-
-	  fcntl (linux_nat_event_pipe[0], F_SETFL, O_NONBLOCK);
-	  fcntl (linux_nat_event_pipe[1], F_SETFL, O_NONBLOCK);
 	}
       else
 	{
-	  close (linux_nat_event_pipe[0]);
-	  close (linux_nat_event_pipe[1]);
-	  linux_nat_event_pipe[0] = -1;
-	  linux_nat_event_pipe[1] = -1;
+	  linux_nat_event_pipe.close ();
 	}
 
       restore_child_signals_mask (&prev_mask);
@@ -4191,7 +4164,7 @@ linux_async_pipe (int enable)
 int
 linux_nat_target::async_wait_fd ()
 {
-  return linux_nat_event_pipe[0];
+  return linux_nat_event_pipe.event_fd ();
 }
 
 /* target_async implementation.  */
@@ -4203,7 +4176,7 @@ linux_nat_target::async (int enable)
     {
       if (!linux_async_pipe (1))
 	{
-	  add_file_handler (linux_nat_event_pipe[0],
+	  add_file_handler (linux_nat_event_pipe.event_fd (),
 			    handle_target_event, NULL,
 			    "linux-nat");
 	  /* There may be pending events to handle.  Tell the event loop
@@ -4213,7 +4186,7 @@ linux_nat_target::async (int enable)
     }
   else
     {
-      delete_file_handler (linux_nat_event_pipe[0]);
+      delete_file_handler (linux_nat_event_pipe.event_fd ());
       linux_async_pipe (0);
     }
   return;
-- 
2.32.0

[PATCH v4 03/13] gdbserver linux-low: Convert linux_event_pipe to the event_pipe class.

[John Baldwin]

Use event_pipe from gdbsupport in place of the existing file
descriptor array.
---
 gdbserver/linux-low.cc | 43 +++++++++++-------------------------------
 1 file changed, 11 insertions(+), 32 deletions(-)

diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
index d214aff7051..e2a9d505a54 100644
--- a/gdbserver/linux-low.cc
+++ b/gdbserver/linux-low.cc
@@ -21,6 +21,8 @@
 #include "nat/linux-osdata.h"
 #include "gdbsupport/agent.h"
 #include "tdesc.h"
+#include "gdbsupport/event-loop.h"
+#include "gdbsupport/event-pipe.h"
 #include "gdbsupport/rsp-low.h"
 #include "gdbsupport/signals-state-save-restore.h"
 #include "nat/linux-nat.h"
@@ -308,12 +310,11 @@ lwp_in_step_range (struct lwp_info *lwp)
   return (pc >= lwp->step_range_start && pc < lwp->step_range_end);
 }
 
-/* The read/write ends of the pipe registered as waitable file in the
-   event loop.  */
-static int linux_event_pipe[2] = { -1, -1 };
+/* The event pipe registered as a waitable file in the event loop.  */
+static event_pipe linux_event_pipe;
 
 /* True if we're currently in async mode.  */
-#define target_is_async_p() (linux_event_pipe[0] != -1)
+#define target_is_async_p() (linux_event_pipe.is_open ())
 
 static void send_sigstop (struct lwp_info *lwp);
 
@@ -3670,28 +3671,14 @@ linux_process_target::wait_1 (ptid_t ptid, target_waitstatus *ourstatus,
 static void
 async_file_flush (void)
 {
-  int ret;
-  char buf;
-
-  do
-    ret = read (linux_event_pipe[0], &buf, 1);
-  while (ret >= 0 || (ret == -1 && errno == EINTR));
+  linux_event_pipe.flush ();
 }
 
 /* Put something in the pipe, so the event loop wakes up.  */
 static void
 async_file_mark (void)
 {
-  int ret;
-
-  async_file_flush ();
-
-  do
-    ret = write (linux_event_pipe[1], "+", 1);
-  while (ret == 0 || (ret == -1 && errno == EINTR));
-
-  /* Ignore EAGAIN.  If the pipe is full, the event loop will already
-     be awakened anyway.  */
+  linux_event_pipe.mark ();
 }
 
 ptid_t
@@ -6086,21 +6073,16 @@ linux_process_target::async (bool enable)
 
       if (enable)
 	{
-	  if (pipe (linux_event_pipe) == -1)
+	  if (!linux_event_pipe.open ())
 	    {
-	      linux_event_pipe[0] = -1;
-	      linux_event_pipe[1] = -1;
 	      gdb_sigmask (SIG_UNBLOCK, &mask, NULL);
 
 	      warning ("creating event pipe failed.");
 	      return previous;
 	    }
 
-	  fcntl (linux_event_pipe[0], F_SETFL, O_NONBLOCK);
-	  fcntl (linux_event_pipe[1], F_SETFL, O_NONBLOCK);
-
 	  /* Register the event loop handler.  */
-	  add_file_handler (linux_event_pipe[0],
+	  add_file_handler (linux_event_pipe.event_fd (),
 			    handle_target_event, NULL,
 			    "linux-low");
 
@@ -6109,12 +6091,9 @@ linux_process_target::async (bool enable)
 	}
       else
 	{
-	  delete_file_handler (linux_event_pipe[0]);
+	  delete_file_handler (linux_event_pipe.event_fd ());
 
-	  close (linux_event_pipe[0]);
-	  close (linux_event_pipe[1]);
-	  linux_event_pipe[0] = -1;
-	  linux_event_pipe[1] = -1;
+	  linux_event_pipe.close ();
 	}
 
       gdb_sigmask (SIG_UNBLOCK, &mask, NULL);
-- 
2.32.0

[PATCH v4 04/13] Don't enable async mode at the end of target ::resume methods.

[John Baldwin]

Commit 5b6d1e4fa4f ("Multi-target support") enabled async mode in
do_target_resume after target::resume returns making this call
redundant in that case.

The other place that target resume methods are invoked are as the
beneath target in record_full_wait_1.  In this case, the async mode
should already be enabled when supported by the target before the
resume method is invoked due to the following:

  In general, targets which support async mode run as async until
  ::wait returns TARGET_WAITKIND_NO_RESUMED to indicate that there are
  no unwaited for children (either they have exited or are stopped).
  When that occurs, the loop in wait_one disables async mode.  Later
  if a stopped child is resumed, async mode is re-enabled in
  do_target_resume before waiting for the next event.

  In the case of record_full_wait_1, this function is invoked from the
  ::wait target method when fetching an event.  If the underlying
  target supports async mode, then an earlier call to do_target_resume
  to resume the child reporting an event in the loop in
  record_full_wait_1 would have already enabled async mode before
  ::wait was invoked.  In addition, nothing in the code executed in
  the loop in record_full_wait_1 disables async mode.  Async mode is
  only disabled higher in the call stack in wait_one after ::wait
  returns.

  It is also true that async mode can be disabled by an
  INF_EXEC_COMPLETE event passed to inferior_event_handle, but all of
  the places that invoke that are in the gdb core which is "above" a
  target ::wait method.

Note that there is an earlier call to enable async mode in
linux_nat_target::resume.  That call also marks the async event pipe
to report an existing event after enabling async mode, so it needs to
stay.
---
 gdb/linux-nat.c   |  3 ---
 gdb/record-full.c | 10 ----------
 gdb/remote.c      | 10 ----------
 3 files changed, 23 deletions(-)

diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index b2c8ce48634..00846b2d369 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -1697,9 +1697,6 @@ linux_nat_target::resume (ptid_t ptid, int step, enum gdb_signal signo)
 			   ? strsignal (gdb_signal_to_host (signo)) : "0"));
 
   linux_resume_one_lwp (lp, step, signo);
-
-  if (target_can_async_p ())
-    target_async (1);
 }
 
 /* Send a signal to an LWP.  */
diff --git a/gdb/record-full.c b/gdb/record-full.c
index 971c0f568b4..86dd1af50fe 100644
--- a/gdb/record-full.c
+++ b/gdb/record-full.c
@@ -1095,11 +1095,6 @@ record_full_target::resume (ptid_t ptid, int step, enum gdb_signal signal)
 
       this->beneath ()->resume (ptid, step, signal);
     }
-
-  /* We are about to start executing the inferior (or simulate it),
-     let's register it with the event loop.  */
-  if (target_can_async_p ())
-    target_async (1);
 }
 
 static int record_full_get_sig = 0;
@@ -2060,11 +2055,6 @@ record_full_core_target::resume (ptid_t ptid, int step,
   record_full_resume_step = step;
   record_full_resumed = 1;
   record_full_execution_dir = ::execution_direction;
-
-  /* We are about to start executing the inferior (or simulate it),
-     let's register it with the event loop.  */
-  if (target_can_async_p ())
-    target_async (1);
 }
 
 /* "kill" method for prec over corefile.  */
diff --git a/gdb/remote.c b/gdb/remote.c
index ebbc138b405..5d4e100efb4 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -6517,16 +6517,6 @@ remote_target::resume (ptid_t ptid, int step, enum gdb_signal siggnal)
   for (thread_info *tp : all_non_exited_threads (this, ptid))
     get_remote_thread_info (tp)->set_resumed ();
 
-  /* We are about to start executing the inferior, let's register it
-     with the event loop.  NOTE: this is the one place where all the
-     execution commands end up.  We could alternatively do this in each
-     of the execution commands in infcmd.c.  */
-  /* FIXME: ezannoni 1999-09-28: We may need to move this out of here
-     into infcmd.c in order to allow inferior function calls to work
-     NOT asynchronously.  */
-  if (target_can_async_p ())
-    target_async (1);
-
   /* We've just told the target to resume.  The remote server will
      wait for the inferior to stop, and then send a stop reply.  In
      the mean time, we can't start another command/query ourselves
-- 
2.32.0

[PATCH v4 05/13] do_target_wait_1: Clear TARGET_WNOHANG if the target isn't async.

[John Baldwin]

Previously, TARGET_WNOHANG was cleared if a target supported async
mode even if async mode wasn't currently enabled.  This change only
permits TARGET_WNOHANG if async mode is enabled.
---
 gdb/infrun.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index e4739ed14f6..fb838d8e74a 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -3625,7 +3625,7 @@ do_target_wait_1 (inferior *inf, ptid_t ptid,
 
   /* We can't ask a non-async target to do a non-blocking wait, so this will be
      a blocking wait.  */
-  if (!target_can_async_p ())
+  if (!target_is_async_p ())
     options &= ~TARGET_WNOHANG;
 
   if (deprecated_target_wait_hook)
-- 
2.32.0

[PATCH v4 06/13] inf-ptrace: Raise an error if waitpid() fails.

[John Baldwin]

Previously this returned a TARGET_WAITKIND_SIGNALLED event for
inferior_ptid.  However, inferior_ptid is invalid during ::wait()
methods after the multi-target changes, so this was triggering an
assertion further up the stack.
---
 gdb/inf-ptrace.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
index 2e7a03c63f5..1b009247251 100644
--- a/gdb/inf-ptrace.c
+++ b/gdb/inf-ptrace.c
@@ -319,13 +319,8 @@ inf_ptrace_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
 
       if (pid == -1)
 	{
-	  fprintf_unfiltered (gdb_stderr,
-			      _("Child process unexpectedly missing: %s.\n"),
-			      safe_strerror (save_errno));
-
-	  /* Claim it exited with unknown signal.  */
-	  ourstatus->set_signalled (GDB_SIGNAL_UNKNOWN);
-	  return inferior_ptid;
+	  errno = save_errno;
+	  perror_with_name (_("Child process unexpectedly missing"));
 	}
 
       /* Ignore terminated detached child processes.  */
-- 
2.32.0

[PATCH v4 07/13] inf-ptrace: Support async targets in inf_ptrace_target::wait.

[John Baldwin]

- Handle TARGET_WNOHANG by passing WNOHANG to waitpid and returning
  TARGET_WAITKIND_IGNORE if there are no events to report.

- Handle a race in async mode where SIGCHLD might signal the event
  pipe for an event that has already been reported.  If the event was
  the exit of the last child process, waitpid() will fail with ECHILD
  rather than returning a pid of 0.  For this case, return
  TARGET_WAITKIND_NO_RESUMED.
---
 gdb/inf-ptrace.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
index 1b009247251..822e27d7e00 100644
--- a/gdb/inf-ptrace.c
+++ b/gdb/inf-ptrace.c
@@ -299,10 +299,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
     {
@@ -310,15 +314,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->set_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 && save_errno == ECHILD)
+	    {
+	      ourstatus->set_no_resumed ();
+	      return minus_one_ptid;
+	    }
+
 	  errno = save_errno;
 	  perror_with_name (_("Child process unexpectedly missing"));
 	}
-- 
2.32.0

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

[John Baldwin]

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 | 154 ++++++++++++++++++++++++++++++++++++++++++++++++-
 gdb/fbsd-nat.h |  12 ++++
 2 files changed, 164 insertions(+), 2 deletions(-)

diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
index e90aa12e442..d6075d2e59b 100644
--- a/gdb/fbsd-nat.c
+++ b/gdb/fbsd-nat.c
@@ -18,7 +18,10 @@
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
 #include "defs.h"
+#include "gdbsupport/block-signals.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"
@@ -27,6 +30,7 @@
 #include "gdbcmd.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
@@ -926,6 +930,114 @@ 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 ()
+{
+  /* This flag should be checked in the common target.c code.  */
+  gdb_assert (target_async_permitted);
+
+  /* Otherwise, this targets is always able to support async mode.  */
+  return true;
+}
+
+/* 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.  */
+  gdb::block_signals blocker;
+
+  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 ();
+    }
+}
+
+/* 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
@@ -997,6 +1109,11 @@ static void
 fbsd_add_vfork_done (ptid_t pid)
 {
   fbsd_pending_vfork_done.push_front (pid);
+
+  /* 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 ();
 }
 
 /* Check for a pending vfork done event for a specific PID.  */
@@ -1165,8 +1282,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;
 
@@ -1381,6 +1498,36 @@ 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
+	  && ourstatus->kind() != TARGET_WAITKIND_NO_RESUMED)
+	  || ptid != minus_one_ptid))
+    fbsd_nat_event_pipe.mark ();
+
+  fbsd_nat_debug_printf ("returning [%s], [%s]",
+			 target_pid_to_str (wptid).c_str (),
+			 ourstatus->to_string ().c_str ());
+  return wptid;
+}
+
 #ifdef USE_SIGTRAP_SIGINFO
 /* Implement the "stopped_by_sw_breakpoint" target_ops method.  */
 
@@ -1675,4 +1822,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 34d7e6d25ba..84c0c5115a6 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.32.0

[PATCH v4 09/13] fbsd-nat: Include ptrace operation in error messages.

[John Baldwin]

---
 gdb/fbsd-nat.c | 34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
index d6075d2e59b..5c3aafc461c 100644
--- a/gdb/fbsd-nat.c
+++ b/gdb/fbsd-nat.c
@@ -821,7 +821,7 @@ fbsd_nat_target::thread_name (struct thread_info *thr)
   if (!fbsd_fetch_kinfo_proc (pid, &kp))
     perror_with_name (_("Failed to fetch process information"));
   if (ptrace (PT_LWPINFO, lwp, (caddr_t) &pl, sizeof pl) == -1)
-    perror_with_name (("ptrace"));
+    perror_with_name (("ptrace (PT_LWPINFO)"));
   if (strcmp (kp.ki_comm, pl.pl_tdname) == 0)
     return NULL;
   xsnprintf (buf, sizeof buf, "%s", pl.pl_tdname);
@@ -850,22 +850,22 @@ fbsd_enable_proc_events (pid_t pid)
 
   if (ptrace (PT_GET_EVENT_MASK, pid, (PTRACE_TYPE_ARG3)&events,
 	      sizeof (events)) == -1)
-    perror_with_name (("ptrace"));
+    perror_with_name (("ptrace (PT_GET_EVENT_MASK)"));
   events |= PTRACE_FORK | PTRACE_LWP;
 #ifdef PTRACE_VFORK
   events |= PTRACE_VFORK;
 #endif
   if (ptrace (PT_SET_EVENT_MASK, pid, (PTRACE_TYPE_ARG3)&events,
 	      sizeof (events)) == -1)
-    perror_with_name (("ptrace"));
+    perror_with_name (("ptrace (PT_SET_EVENT_MASK)"));
 #else
 #ifdef TDP_RFPPWAIT
   if (ptrace (PT_FOLLOW_FORK, pid, (PTRACE_TYPE_ARG3)0, 1) == -1)
-    perror_with_name (("ptrace"));
+    perror_with_name (("ptrace (PT_FOLLOW_FORK)"));
 #endif
 #ifdef PT_LWP_EVENTS
   if (ptrace (PT_LWP_EVENTS, pid, (PTRACE_TYPE_ARG3)0, 1) == -1)
-    perror_with_name (("ptrace"));
+    perror_with_name (("ptrace (PT_LWP_EVENTS)"));
 #endif
 #endif
 }
@@ -884,13 +884,13 @@ fbsd_add_threads (fbsd_nat_target *target, pid_t pid)
   gdb_assert (!in_thread_list (target, ptid_t (pid)));
   nlwps = ptrace (PT_GETNUMLWPS, pid, NULL, 0);
   if (nlwps == -1)
-    perror_with_name (("ptrace"));
+    perror_with_name (("ptrace (PT_GETNUMLWPS)"));
 
   gdb::unique_xmalloc_ptr<lwpid_t[]> lwps (XCNEWVEC (lwpid_t, nlwps));
 
   nlwps = ptrace (PT_GETLWPLIST, pid, (caddr_t) lwps.get (), nlwps);
   if (nlwps == -1)
-    perror_with_name (("ptrace"));
+    perror_with_name (("ptrace (PT_GETLWPLIST)"));
 
   for (i = 0; i < nlwps; i++)
     {
@@ -904,7 +904,7 @@ fbsd_add_threads (fbsd_nat_target *target, pid_t pid)
 	  /* Don't add exited threads.  Note that this is only called
 	     when attaching to a multi-threaded process.  */
 	  if (ptrace (PT_LWPINFO, lwps[i], (caddr_t) &pl, sizeof pl) == -1)
-	    perror_with_name (("ptrace"));
+	    perror_with_name (("ptrace (PT_LWPINFO)"));
 	  if (pl.pl_flags & PL_FLAG_EXITED)
 	    continue;
 #endif
@@ -1179,7 +1179,9 @@ fbsd_nat_target::resume (ptid_t ptid, int step, enum gdb_signal signo)
 	    request = PT_SUSPEND;
 
 	  if (ptrace (request, tp->ptid.lwp (), NULL, 0) == -1)
-	    perror_with_name (("ptrace"));
+	    perror_with_name (request == PT_RESUME ?
+			      ("ptrace (PT_RESUME)") :
+			      ("ptrace (PT_SUSPEND)"));
 	}
     }
   else
@@ -1188,7 +1190,7 @@ fbsd_nat_target::resume (ptid_t ptid, int step, enum gdb_signal signo)
 	 until the process is continued however).  */
       for (thread_info *tp : all_non_exited_threads (this, ptid))
 	if (ptrace (PT_RESUME, tp->ptid.lwp (), NULL, 0) == -1)
-	  perror_with_name (("ptrace"));
+	  perror_with_name (("ptrace (PT_RESUME)"));
       ptid = inferior_ptid;
     }
 
@@ -1218,7 +1220,7 @@ fbsd_nat_target::resume (ptid_t ptid, int step, enum gdb_signal signo)
   if (step)
     {
       if (ptrace (PT_SETSTEP, get_ptrace_pid (ptid), NULL, 0) == -1)
-	perror_with_name (("ptrace"));
+	perror_with_name (("ptrace (PT_SETSTEP)"));
       step = 0;
     }
   ptid = ptid_t (ptid.pid ());
@@ -1306,7 +1308,7 @@ fbsd_nat_target::wait_1 (ptid_t ptid, struct target_waitstatus *ourstatus,
 
 	  pid = wptid.pid ();
 	  if (ptrace (PT_LWPINFO, pid, (caddr_t) &pl, sizeof pl) == -1)
-	    perror_with_name (("ptrace"));
+	    perror_with_name (("ptrace (PT_LWPINFO)"));
 
 	  wptid = ptid_t (pid, pl.pl_lwpid);
 
@@ -1338,7 +1340,7 @@ fbsd_nat_target::wait_1 (ptid_t ptid, struct target_waitstatus *ourstatus,
 		  delete_thread (thr);
 		}
 	      if (ptrace (PT_CONTINUE, pid, (caddr_t) 1, 0) == -1)
-		perror_with_name (("ptrace"));
+		perror_with_name (("ptrace (PT_CONTINUE)"));
 	      continue;
 	    }
 #endif
@@ -1401,7 +1403,7 @@ fbsd_nat_target::wait_1 (ptid_t ptid, struct target_waitstatus *ourstatus,
 		  gdb_assert (pid == child);
 
 		  if (ptrace (PT_LWPINFO, child, (caddr_t)&pl, sizeof pl) == -1)
-		    perror_with_name (("ptrace"));
+		    perror_with_name (("ptrace (PT_LWPINFO)"));
 
 		  gdb_assert (pl.pl_flags & PL_FLAG_CHILD);
 		  child_ptid = ptid_t (child, pl.pl_lwpid);
@@ -1490,7 +1492,7 @@ fbsd_nat_target::wait_1 (ptid_t ptid, struct target_waitstatus *ourstatus,
 		 and once system call stops are enabled on a process
 		 it stops for all system call entries and exits.  */
 	      if (ptrace (PT_CONTINUE, pid, (caddr_t) 1, 0) == -1)
-		perror_with_name (("ptrace"));
+		perror_with_name (("ptrace (PT_CONTINUE)"));
 	      continue;
 	    }
 	}
@@ -1637,7 +1639,7 @@ fbsd_nat_target::follow_fork (inferior *child_inf, ptid_t child_ptid,
 	 infrun.c.  */
 
       if (ptrace (PT_DETACH, child_pid, (PTRACE_TYPE_ARG3)1, 0) == -1)
-	perror_with_name (("ptrace"));
+	perror_with_name (("ptrace (PT_DETACH)"));
 
 #ifndef PTRACE_VFORK
       if (fork_kind () == TARGET_WAITKIND_VFORKED)
-- 
2.32.0

[PATCH v4 10/13] fbsd-nat: Various cleanups to the ::resume entry debug message.

[John Baldwin]

Move the message from 'show debug fbsd-lwp' to 'show debug fbsd-nat'
since it is helpful for debugging async target support and not just
LWP support.

Use target_pid_to_str to format the ptid and log the step and signo
arguments.
---
 gdb/fbsd-nat.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
index 5c3aafc461c..563e1b75ed0 100644
--- a/gdb/fbsd-nat.c
+++ b/gdb/fbsd-nat.c
@@ -1162,8 +1162,9 @@ fbsd_nat_target::resume (ptid_t ptid, int step, enum gdb_signal signo)
     return;
 #endif
 
-  fbsd_lwp_debug_printf ("ptid (%d, %ld, %s)", ptid.pid (), ptid.lwp (),
-			 pulongest (ptid.tid ()));
+  fbsd_nat_debug_printf ("[%s], step %d, signo %d (%s)",
+			 target_pid_to_str (ptid).c_str (), step, signo,
+			 gdb_signal_to_name (signo));
   if (ptid.lwp_p ())
     {
       /* If ptid is a specific LWP, suspend all other LWPs in the process.  */
-- 
2.32.0

[PATCH v4 11/13] fbsd-nat: Return nullptr rather than failing ::thread_name.

[John Baldwin]

ptrace on FreeBSD cannot be used against running processes and instead
fails with EBUSY.  This meant that 'info threads' would fail if any of
the threads were running (for example when using schedule-multiple=on
in gdb.base/fork-running-state.exp).  Instead of throwing errors, just
return nullptr as no thread name is better than causing info threads to
fail completely.
---
 gdb/fbsd-nat.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
index 563e1b75ed0..77d66a82428 100644
--- a/gdb/fbsd-nat.c
+++ b/gdb/fbsd-nat.c
@@ -819,9 +819,9 @@ fbsd_nat_target::thread_name (struct thread_info *thr)
      if a name has not been set explicitly.  Return a NULL name in
      that case.  */
   if (!fbsd_fetch_kinfo_proc (pid, &kp))
-    perror_with_name (_("Failed to fetch process information"));
+    return nullptr;
   if (ptrace (PT_LWPINFO, lwp, (caddr_t) &pl, sizeof pl) == -1)
-    perror_with_name (("ptrace (PT_LWPINFO)"));
+    return nullptr;
   if (strcmp (kp.ki_comm, pl.pl_tdname) == 0)
     return NULL;
   xsnprintf (buf, sizeof buf, "%s", pl.pl_tdname);
-- 
2.32.0

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

[John Baldwin]

If the attach target supports async mode, enable it after the
attach target's ::attach method returns.
---
 gdb/fbsd-nat.c  | 13 -------------
 gdb/fbsd-nat.h  |  2 --
 gdb/infcmd.c    |  4 ++++
 gdb/linux-nat.c |  3 ---
 gdb/remote.c    |  3 ---
 5 files changed, 4 insertions(+), 21 deletions(-)

diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
index 77d66a82428..063525b1aa2 100644
--- a/gdb/fbsd-nat.c
+++ b/gdb/fbsd-nat.c
@@ -1025,19 +1025,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 84c0c5115a6..7dd47cf82c0 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 984ce4e042b..a13bab21389 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -2542,6 +2542,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 00846b2d369..a824b934866 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -1208,9 +1208,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 5d4e100efb4..8173befba3e 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -6123,8 +6123,6 @@ extended_remote_target::attach (const char *args, int from_tty)
 	    =  remote_notif_parse (this, &notif_client_stop, wait_status);
 
 	  push_stop_reply ((struct stop_reply *) reply);
-
-	  target_async (1);
 	}
       else
 	{
@@ -6138,7 +6136,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);
     }
 }
 
-- 
2.32.0

[PATCH v4 13/13] inf-ptrace: Add an event_pipe to be used for async mode in subclasses.

[John Baldwin]

Subclasses of inf_ptrace_target have to opt-in to using the event_pipe
by implementing the can_async_p and async methods.  For subclasses
which do this, inf_ptrace_target provides is_async_p, async_wait_fd
and closes the pipe in the close target method.

inf_ptrace_target also provides wrapper routines around the event pipe
(async_file_open, async_file_close, async_file_flush, and
async_file_mark) for use in target methods such as async.
inf_ptrace_target also exports a static async_file_mark_if_open
function which can be used in SIGCHLD signal handlers.
---
 gdb/fbsd-nat.c   |  50 ++++---------------
 gdb/fbsd-nat.h   |   4 --
 gdb/inf-ptrace.c |  15 ++++++
 gdb/inf-ptrace.h |  30 ++++++++++++
 gdb/linux-nat.c  | 123 +++++++++--------------------------------------
 gdb/linux-nat.h  |   4 --
 6 files changed, 78 insertions(+), 148 deletions(-)

diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
index 063525b1aa2..71985fb4160 100644
--- a/gdb/fbsd-nat.c
+++ b/gdb/fbsd-nat.c
@@ -21,7 +21,6 @@
 #include "gdbsupport/block-signals.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"
@@ -932,8 +931,6 @@ fbsd_nat_target::update_thread_list ()
 
 /* Async mode support.  */
 
-static event_pipe fbsd_nat_event_pipe;
-
 /* Implement the "can_async_p" target method.  */
 
 bool
@@ -946,22 +943,6 @@ fbsd_nat_target::can_async_p ()
   return true;
 }
 
-/* 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
@@ -969,8 +950,7 @@ sigchld_handler (int signo)
 {
   int old_errno = errno;
 
-  if (fbsd_nat_event_pipe.is_open ())
-    fbsd_nat_event_pipe.mark ();
+  fbsd_nat_target::async_file_mark_if_open ();
 
   errno = old_errno;
 }
@@ -997,34 +977,22 @@ fbsd_nat_target::async (int enable)
 
   if (enable)
     {
-      if (!fbsd_nat_event_pipe.open ())
+      if (!async_file_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");
+      add_file_handler (async_wait_fd (), handle_target_event, NULL, "fbsd-nat");
 
       /* Trigger a poll in case there are pending events to
 	 handle.  */
-      fbsd_nat_event_pipe.mark ();
+      async_file_mark ();
     }
   else
     {
-      delete_file_handler (fbsd_nat_event_pipe.event_fd ());
-      fbsd_nat_event_pipe.close ();
+      delete_file_handler (async_wait_fd ());
+      async_file_close ();
     }
 }
 
-/* Implement the "close" target method.  */
-
-void
-fbsd_nat_target::close ()
-{
-  if (is_async_p ())
-    async (0);
-
-  inf_ptrace_target::close ();
-}
-
 #ifdef TDP_RFPPWAIT
 /*
   To catch fork events, PT_FOLLOW_FORK is set on every traced process
@@ -1100,7 +1068,7 @@ fbsd_add_vfork_done (ptid_t pid)
   /* 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 ();
+    async_file_mark ();
 }
 
 /* Check for a pending vfork done event for a specific PID.  */
@@ -1499,7 +1467,7 @@ fbsd_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
 
   /* Ensure any subsequent events trigger a new event in the loop.  */
   if (is_async_p ())
-    fbsd_nat_event_pipe.flush ();
+    async_file_flush ();
 
   wptid = wait_1 (ptid, ourstatus, target_options);
 
@@ -1510,7 +1478,7 @@ fbsd_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
       && ((ourstatus->kind () != TARGET_WAITKIND_IGNORE
 	  && ourstatus->kind() != TARGET_WAITKIND_NO_RESUMED)
 	  || ptid != minus_one_ptid))
-    fbsd_nat_event_pipe.mark ();
+    async_file_mark ();
 
   fbsd_nat_debug_printf ("returning [%s], [%s]",
 			 target_pid_to_str (wptid).c_str (),
diff --git a/gdb/fbsd-nat.h b/gdb/fbsd-nat.h
index 7dd47cf82c0..9c76b187555 100644
--- a/gdb/fbsd-nat.h
+++ b/gdb/fbsd-nat.h
@@ -67,13 +67,9 @@ 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; }
 
diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
index 822e27d7e00..b38f8ede7da 100644
--- a/gdb/inf-ptrace.c
+++ b/gdb/inf-ptrace.c
@@ -48,6 +48,9 @@ gdb_ptrace (PTRACE_TYPE_ARG1 request, ptid_t ptid, PTRACE_TYPE_ARG3 addr,
 #endif
 }
 
+/* The event pipe registered as a waitable file in the event loop.  */
+event_pipe inf_ptrace_target::m_event_pipe;
+
 inf_ptrace_target::~inf_ptrace_target ()
 {}
 
@@ -533,3 +536,15 @@ inf_ptrace_target::pid_to_str (ptid_t ptid)
 {
   return normal_pid_to_str (ptid);
 }
+
+/* Implement the "close" target method.  */
+
+void
+inf_ptrace_target::close ()
+{
+  /* Unregister from the event loop.  */
+  if (is_async_p ())
+    async (0);
+
+  inf_child_target::close ();
+}
diff --git a/gdb/inf-ptrace.h b/gdb/inf-ptrace.h
index 8aded9b60db..997b8014440 100644
--- a/gdb/inf-ptrace.h
+++ b/gdb/inf-ptrace.h
@@ -20,6 +20,7 @@
 #ifndef INF_PTRACE_H
 #define INF_PTRACE_H
 
+#include "gdbsupport/event-pipe.h"
 #include "inf-child.h"
 
 /* An abstract prototype ptrace target.  The client can override it
@@ -33,6 +34,8 @@ struct inf_ptrace_target : public inf_child_target
 
   void detach (inferior *inf, int) override;
 
+  void close () override;
+
   void resume (ptid_t, int, enum gdb_signal) override;
 
   ptid_t wait (ptid_t, struct target_waitstatus *, target_wait_flags) override;
@@ -57,9 +60,36 @@ struct inf_ptrace_target : public inf_child_target
 					ULONGEST offset, ULONGEST len,
 					ULONGEST *xfered_len) override;
 
+  bool is_async_p () override
+  { return m_event_pipe.is_open (); }
+
+  int async_wait_fd () override
+  { return m_event_pipe.event_fd (); }
+
+  /* Helper routine used from SIGCHLD handlers to signal the async
+     event pipe.  */
+  static void async_file_mark_if_open ()
+  {
+    if (m_event_pipe.is_open ())
+      m_event_pipe.mark ();
+  }
+
 protected:
+  /* Helper routines for interacting with the async event pipe.  */
+  bool async_file_open ()
+  { return m_event_pipe.open (); }
+  void async_file_close ()
+  { m_event_pipe.close (); }
+  void async_file_flush ()
+  { m_event_pipe.flush (); }
+  void async_file_mark ()
+  { m_event_pipe.mark (); }
+
   /* Cleanup the inferior after a successful ptrace detach.  */
   void detach_success (inferior *inf);
+
+private:
+  static event_pipe m_event_pipe;
 };
 
 #ifndef __NetBSD__
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index a824b934866..fb5e2e0ba87 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -48,7 +48,6 @@
 #include <fcntl.h>		/* for O_RDONLY */
 #include "inf-loop.h"
 #include "gdbsupport/event-loop.h"
-#include "gdbsupport/event-pipe.h"
 #include "event-top.h"
 #include <pwd.h>
 #include <sys/types.h>
@@ -66,6 +65,7 @@
 #include "gdbsupport/filestuff.h"
 #include "objfiles.h"
 #include "nat/linux-namespaces.h"
+#include "gdbsupport/block-signals.h"
 #include "gdbsupport/fileio.h"
 #include "gdbsupport/scope-exit.h"
 #include "gdbsupport/gdb-sigmask.h"
@@ -217,32 +217,6 @@ static struct simple_pid_list *stopped_pids;
 /* Whether target_thread_events is in effect.  */
 static int report_thread_events;
 
-/* Async mode support.  */
-
-/* The event pipe registered as a waitable file in the event loop.  */
-static event_pipe linux_nat_event_pipe;
-
-/* True if we're currently in async mode.  */
-#define linux_is_async_p() (linux_nat_event_pipe.is_open ())
-
-/* Flush the event pipe.  */
-
-static void
-async_file_flush (void)
-{
-  linux_nat_event_pipe.flush ();
-}
-
-/* Put something (anything, doesn't matter what, or how much) in event
-   pipe, so that the select/poll in the event-loop realizes we have
-   something to process.  */
-
-static void
-async_file_mark (void)
-{
-  linux_nat_event_pipe.mark ();
-}
-
 static int kill_lwp (int lwpid, int signo);
 
 static int stop_callback (struct lwp_info *lp);
@@ -4048,14 +4022,6 @@ linux_nat_target::static_tracepoint_markers_by_strid (const char *strid)
   return markers;
 }
 
-/* target_is_async_p implementation.  */
-
-bool
-linux_nat_target::is_async_p ()
-{
-  return linux_is_async_p ();
-}
-
 /* target_can_async_p implementation.  */
 
 bool
@@ -4107,10 +4073,11 @@ sigchld_handler (int signo)
   if (debug_linux_nat)
     gdb_stdlog->write_async_safe ("sigchld\n", sizeof ("sigchld\n") - 1);
 
-  if (signo == SIGCHLD
-      && linux_nat_event_pipe.is_open ())
-    async_file_mark (); /* Let the event loop know that there are
-			   events to handle.  */
+  if (signo == SIGCHLD)
+    {
+      /* Let the event loop know that there are events to handle.  */
+      linux_nat_target::async_file_mark_if_open ();
+    }
 
   errno = old_errno;
 }
@@ -4123,67 +4090,35 @@ handle_target_event (int error, gdb_client_data client_data)
   inferior_event_handler (INF_REG_EVENT);
 }
 
-/* Create/destroy the target events pipe.  Returns previous state.  */
-
-static int
-linux_async_pipe (int enable)
-{
-  int previous = linux_is_async_p ();
-
-  if (previous != enable)
-    {
-      sigset_t prev_mask;
-
-      /* Block child signals while we create/destroy the pipe, as
-	 their handler writes to it.  */
-      block_child_signals (&prev_mask);
-
-      if (enable)
-	{
-	  if (!linux_nat_event_pipe.open ())
-	    internal_error (__FILE__, __LINE__,
-			    "creating event pipe failed.");
-	}
-      else
-	{
-	  linux_nat_event_pipe.close ();
-	}
-
-      restore_child_signals_mask (&prev_mask);
-    }
-
-  return previous;
-}
-
-int
-linux_nat_target::async_wait_fd ()
-{
-  return linux_nat_event_pipe.event_fd ();
-}
-
 /* target_async implementation.  */
 
 void
 linux_nat_target::async (int enable)
 {
+  if ((enable != 0) == is_async_p ())
+    return;
+
+  /* Block child signals while we create/destroy the pipe, as their
+     handler writes to it.  */
+  gdb::block_signals blocker;
+
   if (enable)
     {
-      if (!linux_async_pipe (1))
-	{
-	  add_file_handler (linux_nat_event_pipe.event_fd (),
-			    handle_target_event, NULL,
-			    "linux-nat");
-	  /* There may be pending events to handle.  Tell the event loop
-	     to poll them.  */
-	  async_file_mark ();
-	}
+      if (!async_file_open ())
+	internal_error (__FILE__, __LINE__, "creating event pipe failed.");
+
+      add_file_handler (async_wait_fd (), handle_target_event, NULL,
+			"linux-nat");
+
+      /* There may be pending events to handle.  Tell the event loop
+	 to poll them.  */
+      async_file_mark ();
     }
   else
     {
-      delete_file_handler (linux_nat_event_pipe.event_fd ());
-      linux_async_pipe (0);
+      delete_file_handler (async_wait_fd ());
+      async_file_close ();
     }
-  return;
 }
 
 /* Stop an LWP, and push a GDB_SIGNAL_0 stop status if no other
@@ -4231,16 +4166,6 @@ linux_nat_target::stop (ptid_t ptid)
   iterate_over_lwps (ptid, linux_nat_stop_lwp);
 }
 
-void
-linux_nat_target::close ()
-{
-  /* Unregister from the event loop.  */
-  if (is_async_p ())
-    async (0);
-
-  inf_ptrace_target::close ();
-}
-
 /* When requests are passed down from the linux-nat layer to the
    single threaded inf-ptrace layer, ptids of (lwpid,0,0) form are
    used.  The address space pointer is stored in the inferior object,
diff --git a/gdb/linux-nat.h b/gdb/linux-nat.h
index 95e26b7ee46..243d9a3f636 100644
--- a/gdb/linux-nat.h
+++ b/gdb/linux-nat.h
@@ -83,16 +83,12 @@ class linux_nat_target : public inf_ptrace_target
   void thread_events (int) override;
 
   bool can_async_p () override;
-  bool is_async_p () override;
 
   bool supports_non_stop () override;
   bool always_non_stop_p () override;
 
-  int async_wait_fd () override;
   void async (int) override;
 
-  void close () override;
-
   void stop (ptid_t) override;
 
   bool supports_multi_process () override;
-- 
2.32.0

Re: [PATCH v4 00/13] FreeBSD target async mode and related refactoring

[John Baldwin]

On 12/6/21 11:31 AM, John Baldwin wrote:
> Changes since V2:
> 
> - Rebased for recent changes (waitkind setters from Simon
>    and can_async_p changes from Andrew)
> 
> - Fixes based on feedback from V2.
> 
> - Patch 5 has an extended commit message and now includes changes to
>    the remote and record targets in addition to the Linux native target.

This should read 'patch 4'

> Changes since V3:
> 
> - Patch 6 was merged into patch 5, commit log expanded and added
>    remote target.

Similarly, 'patch 5 merged into patch 4'.

> - Patch 8 includes an updated can_async_p implementation.
> 
> John Baldwin (13):
>    gdbsupport: Add an event-pipe class.
>    gdb linux-nat: Convert linux_nat_event_pipe to the event_pipe class.
>    gdbserver linux-low: Convert linux_event_pipe to the event_pipe class.
>    Don't enable async mode at the end of target ::resume methods.
>    do_target_wait_1: Clear TARGET_WNOHANG if the target isn't async.
>    inf-ptrace: Raise an error if waitpid() fails.
>    inf-ptrace: Support async targets in inf_ptrace_target::wait.
>    fbsd-nat: Implement async target support.
>    fbsd-nat: Include ptrace operation in error messages.
>    fbsd-nat: Various cleanups to the ::resume entry debug message.
>    fbsd-nat: Return nullptr rather than failing ::thread_name.
>    Enable async mode in the target in attach_cmd.
>    inf-ptrace: Add an event_pipe to be used for async mode in subclasses.
> 
>   gdb/fbsd-nat.c           | 150 ++++++++++++++++++++++++++++++-----
>   gdb/fbsd-nat.h           |   6 ++
>   gdb/inf-ptrace.c         |  49 +++++++++---
>   gdb/inf-ptrace.h         |  30 +++++++
>   gdb/infcmd.c             |   4 +
>   gdb/infrun.c             |   2 +-
>   gdb/linux-nat.c          | 164 +++++++--------------------------------
>   gdb/linux-nat.h          |   4 -
>   gdb/record-full.c        |  10 ---
>   gdb/remote.c             |  13 ----
>   gdbserver/linux-low.cc   |  43 +++-------
>   gdbsupport/Makefile.am   |   5 ++
>   gdbsupport/Makefile.in   |   9 ++-
>   gdbsupport/configure     |  15 ++++
>   gdbsupport/configure.ac  |   3 +
>   gdbsupport/event-pipe.cc | 101 ++++++++++++++++++++++++
>   gdbsupport/event-pipe.h  |  60 ++++++++++++++
>   17 files changed, 440 insertions(+), 228 deletions(-)
>   create mode 100644 gdbsupport/event-pipe.cc
>   create mode 100644 gdbsupport/event-pipe.h
> 

Ping in case anyone has idle cycles for review at the end of December.

Patches 2, 3, 5, 7, 9, and 11 have already been approved by Andrew.

-- 
John Baldwin

Re: [PATCH v4 01/13] gdbsupport: Add an event-pipe class.

[Tom Tromey]

>>>>> ">" == John Baldwin <jhb@FreeBSD.org> writes:

>> +if HAVE_PIPE_OR_PIPE2
>> +eventpipe = event-pipe.cc
>> +endif

I forgot to mention it earlier, but there's similar code in ser-event.c
as well.  This code doesn't bother checking for 'pipe', it just assumes
it is fine unless USE_WIN32API.

To be clear there's no requirement to change ser-event.c to use this new
code or to change the configury in this patch.

thanks,
Tom

Re: [PATCH v4 06/13] inf-ptrace: Raise an error if waitpid() fails.

[Tom Tromey]

>>>>> "John" == John Baldwin <jhb@FreeBSD.org> writes:

John> Previously this returned a TARGET_WAITKIND_SIGNALLED event for
John> inferior_ptid.  However, inferior_ptid is invalid during ::wait()
John> methods after the multi-target changes, so this was triggering an
John> assertion further up the stack.

John> -	  fprintf_unfiltered (gdb_stderr,
John> -			      _("Child process unexpectedly missing: %s.\n"),
John> -			      safe_strerror (save_errno));
John> -
John> -	  /* Claim it exited with unknown signal.  */
John> -	  ourstatus->set_signalled (GDB_SIGNAL_UNKNOWN);
John> -	  return inferior_ptid;
John> +	  errno = save_errno;
John> +	  perror_with_name (_("Child process unexpectedly missing"));
John>  	}

I am not sure about this.  target_wait is documented as:

   pid to do something.  Return pid of child, or -1 in case of error;
   store status through argument pointer STATUS.  Note that it is
   _NOT_ OK to throw_exception() out of target_wait() without popping
   the debugging target from the stack; GDB isn't prepared to get back

and in gdb, target_wait is just a wrapper for target->wait(...).

I see other targets returning minus_one_ptid on error here, and that's
what default_target_wait does as well.  (It also uses
status->set_ignore() which may be useful here as well.)

So I suspect throwing here may be a problem and instead this should
return minus_one_ptid.

To be clear I don't know if that comment is really accurate.

Tom

Re: [PATCH v4 10/13] fbsd-nat: Various cleanups to the ::resume entry debug message.

[Tom Tromey]

>>>>> "John" == John Baldwin <jhb@FreeBSD.org> writes:

John> Move the message from 'show debug fbsd-lwp' to 'show debug fbsd-nat'
John> since it is helpful for debugging async target support and not just
John> LWP support.

John> Use target_pid_to_str to format the ptid and log the step and signo
John> arguments.

Looks good to me, and for my part I think this kind of change is up to
you anyhow.

Tom

Re: [PATCH v4 11/13] fbsd-nat: Return nullptr rather than failing ::thread_name.

[Tom Tromey]

>>>>> "John" == John Baldwin <jhb@FreeBSD.org> writes:

John> ptrace on FreeBSD cannot be used against running processes and instead
John> fails with EBUSY.  This meant that 'info threads' would fail if any of
John> the threads were running (for example when using schedule-multiple=on
John> in gdb.base/fork-running-state.exp).  Instead of throwing errors, just
John> return nullptr as no thread name is better than causing info threads to
John> fail completely.

Seems reasonable to me.

Tom

Re: [PATCH v4 12/13] Enable async mode in the target in attach_cmd.

[Tom Tromey]

>>>>> "John" == John Baldwin <jhb@FreeBSD.org> writes:

John> If the attach target supports async mode, enable it after the
John> attach target's ::attach method returns.

This looks good to me.

Tom

Re: [PATCH v4 13/13] inf-ptrace: Add an event_pipe to be used for async mode in subclasses.

[Tom Tromey]

>>>>> "John" == John Baldwin <jhb@FreeBSD.org> writes:

John> Subclasses of inf_ptrace_target have to opt-in to using the event_pipe
John> by implementing the can_async_p and async methods.  For subclasses
John> which do this, inf_ptrace_target provides is_async_p, async_wait_fd
John> and closes the pipe in the close target method.

John> inf_ptrace_target also provides wrapper routines around the event pipe
John> (async_file_open, async_file_close, async_file_flush, and
John> async_file_mark) for use in target methods such as async.
John> inf_ptrace_target also exports a static async_file_mark_if_open
John> function which can be used in SIGCHLD signal handlers.

Thank you.  This looks good to me.

Tom

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

[Tom Tromey]

>>>>> "John" == John Baldwin <jhb@FreeBSD.org> writes:

John> This is a fairly simple version of async target support.
John> Synchronous mode still uses blocking waitpid() calls in
John> inf_ptrace::wait() unlike the Linux native target which always uses
John> WNOHANG and uses sigsuspend() for synchronous operation.

John> Asynchronous mode registers an event pipe with the core as a file
John> handle and writes to the pipe when SIGCHLD is raised.  TARGET_WNOHANG
John> is handled by inf_ptrace::wait().

John> +  /* Install a SIGCHLD handler.  */
John> +  signal (SIGCHLD, sigchld_handler);

Does this need sigaction + SA_RESTART?

Otherwise this looks good to me.

Tom

Re: [PATCH v4 04/13] Don't enable async mode at the end of target ::resume methods.

[Tom Tromey]

>>>>> "John" == John Baldwin <jhb@FreeBSD.org> writes:

John> Commit 5b6d1e4fa4f ("Multi-target support") enabled async mode in
John> do_target_resume after target::resume returns making this call
John> redundant in that case.

I like this patch, mainly because I think historically there's been too
much stuff that has to be duplicated in each target, rather than having
the core do things in one place.

But I wonder if this is really equivalent to the status quo, because
target_resume is also called from target_continue_no_signal and
target_continue.  In these spots, the targets will end up setting
target-async, but in the new code, they won't.  It wasn't completely
clear to me that calls to these functions were covered by your analysis.

Also I wonder if the call to target_async should be pushed into
target_resume itself.

thanks,
Tom

Re: [PATCH v4 00/13] FreeBSD target async mode and related refactoring

[Tom Tromey]

John> Ping in case anyone has idle cycles for review at the end of December.

John> Patches 2, 3, 5, 7, 9, and 11 have already been approved by Andrew.

I went through and made a couple of comments.  Mostly it was stuff I am
unsure of, so best case, I'm just confused about some things and this is
all ready to go :)

thank you,
Tom

Re: [PATCH v4 01/13] gdbsupport: Add an event-pipe class.

[John Baldwin]

On 1/14/22 8:22 AM, Tom Tromey wrote:
>>>>>> ">" == John Baldwin <jhb@FreeBSD.org> writes:
> 
>>> +if HAVE_PIPE_OR_PIPE2
>>> +eventpipe = event-pipe.cc
>>> +endif
> 
> I forgot to mention it earlier, but there's similar code in ser-event.c
> as well.  This code doesn't bother checking for 'pipe', it just assumes
> it is fine unless USE_WIN32API.
> 
> To be clear there's no requirement to change ser-event.c to use this new
> code or to change the configury in this patch.

Yeah, I saw that and at one point thought about trying to re-use that
for this instead, but the semantics of the two aren't quite the same.

One question though that I thought of the other day is that it might be
interesting on Linux to try to replace the SIGCHLD handler with a
signalfd and using that directly instead of the pipe?

On FreeBSD I might someday look at replacing the event loop implementation
with kevent() instead of poll().  kevent() can wait for signals in addition
to file descriptors and would similarly be able to wait directly for the
signal in the event loop without needing the pipe.

-- 
John Baldwin

Re: [PATCH v4 04/13] Don't enable async mode at the end of target ::resume methods.

[John Baldwin]

On 1/14/22 9:05 AM, Tom Tromey wrote:
>>>>>> "John" == John Baldwin <jhb@FreeBSD.org> writes:
> 
> John> Commit 5b6d1e4fa4f ("Multi-target support") enabled async mode in
> John> do_target_resume after target::resume returns making this call
> John> redundant in that case.
> 
> I like this patch, mainly because I think historically there's been too
> much stuff that has to be duplicated in each target, rather than having
> the core do things in one place.
> 
> But I wonder if this is really equivalent to the status quo, because
> target_resume is also called from target_continue_no_signal and
> target_continue.  In these spots, the targets will end up setting
> target-async, but in the new code, they won't.  It wasn't completely
> clear to me that calls to these functions were covered by your analysis.
> 
> Also I wonder if the call to target_async should be pushed into
> target_resume itself.

So I think you are correct and that target_continue* would no longer be
enabling async mode.  I think moving this into target_resume instead
makes sense, though I would probably do that as a new commit in this
series before this one.  I hadn't considered moving the code previously
and had missed the other calls to target_resume previously.

-- 
John Baldwin

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

[John Baldwin]

On 1/14/22 8:58 AM, Tom Tromey wrote:
>>>>>> "John" == John Baldwin <jhb@FreeBSD.org> writes:
> 
> John> This is a fairly simple version of async target support.
> John> Synchronous mode still uses blocking waitpid() calls in
> John> inf_ptrace::wait() unlike the Linux native target which always uses
> John> WNOHANG and uses sigsuspend() for synchronous operation.
> 
> John> Asynchronous mode registers an event pipe with the core as a file
> John> handle and writes to the pipe when SIGCHLD is raised.  TARGET_WNOHANG
> John> is handled by inf_ptrace::wait().
> 
> John> +  /* Install a SIGCHLD handler.  */
> John> +  signal (SIGCHLD, sigchld_handler);
> 
> Does this need sigaction + SA_RESTART?
> 
> Otherwise this looks good to me.

On FreeBSD signal(3) always sets SA_RESTART when installing a handler,
so this doesn't need an explicit SA_RESTART.

-- 
John Baldwin

Re: [PATCH v4 06/13] inf-ptrace: Raise an error if waitpid() fails.

[John Baldwin]

On 1/14/22 8:35 AM, Tom Tromey wrote:
>>>>>> "John" == John Baldwin <jhb@FreeBSD.org> writes:
> 
> John> Previously this returned a TARGET_WAITKIND_SIGNALLED event for
> John> inferior_ptid.  However, inferior_ptid is invalid during ::wait()
> John> methods after the multi-target changes, so this was triggering an
> John> assertion further up the stack.
> 
> John> -	  fprintf_unfiltered (gdb_stderr,
> John> -			      _("Child process unexpectedly missing: %s.\n"),
> John> -			      safe_strerror (save_errno));
> John> -
> John> -	  /* Claim it exited with unknown signal.  */
> John> -	  ourstatus->set_signalled (GDB_SIGNAL_UNKNOWN);
> John> -	  return inferior_ptid;
> John> +	  errno = save_errno;
> John> +	  perror_with_name (_("Child process unexpectedly missing"));
> John>  	}
> 
> I am not sure about this.  target_wait is documented as:
> 
>     pid to do something.  Return pid of child, or -1 in case of error;
>     store status through argument pointer STATUS.  Note that it is
>     _NOT_ OK to throw_exception() out of target_wait() without popping
>     the debugging target from the stack; GDB isn't prepared to get back
> 
> and in gdb, target_wait is just a wrapper for target->wait(...).
> 
> I see other targets returning minus_one_ptid on error here, and that's
> what default_target_wait does as well.  (It also uses
> status->set_ignore() which may be useful here as well.)
> 
> So I suspect throwing here may be a problem and instead this should
> return minus_one_ptid.
> 
> To be clear I don't know if that comment is really accurate.

I agree and have changed it to use return an IGNORE event instead.

-- 
John Baldwin

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

[Tom Tromey]

John> +  /* Install a SIGCHLD handler.  */
John> +  signal (SIGCHLD, sigchld_handler);

>> Does this need sigaction + SA_RESTART?
>> Otherwise this looks good to me.

John> On FreeBSD signal(3) always sets SA_RESTART when installing a handler,
John> so this doesn't need an explicit SA_RESTART.

Aha, great.  Thank you.

Tom

Re: [PATCH v4 01/13] gdbsupport: Add an event-pipe class.

[Tom Tromey]

>>>>> "John" == John Baldwin <jhb@FreeBSD.org> writes:

John> One question though that I thought of the other day is that it might be
John> interesting on Linux to try to replace the SIGCHLD handler with a
John> signalfd and using that directly instead of the pipe?

Yeah, I think this would be interesting, with the usual caveat that we
might have to continue to support the old code as well, depending on
when signalfd was introduced and what distros we're supporting.

John> On FreeBSD I might someday look at replacing the event loop implementation
John> with kevent() instead of poll().  kevent() can wait for signals in addition
John> to file descriptors and would similarly be able to wait directly for the
John> signal in the event loop without needing the pipe.

Sounds nice.  I've occasionally wondered about using libevent instead of
rolling our own.  It's a pain to add a dependency, but OTOH this might
make integrating with Python's async feature a bit simpler.

Related to all this, I've often wished that ptrace just had a PTRACE_FD
request and then gdb could get the wait results directly from an fd
rather than dealing with the SIGCHLD business at all.

Tom