[PATCH v5 00/15] FreeBSD target async mode and related refactoring

[PATCH v5 00/15] FreeBSD target async mode and related refactoring

[John Baldwin]

Changes since V4:

- Patch 4 is new and enables async mode inside of target_resume.

- Patch 5 is adjusted slightly due to Patch 4.

- Patch 7 now returns an IGNORE status rather than throwing an error
  when waitpid() fails in inf_ptrace::wait

- Added a NEWS blurb.

John Baldwin (15):
  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.
  Enable async mode on supported targets in target_resume.
  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: Return an IGNORE event 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.
  NEWS: Note that the FreeBSD async target supports async mode.

 gdb/NEWS                 |   2 +
 gdb/fbsd-nat.c           | 150 ++++++++++++++++++++++++++++++-----
 gdb/fbsd-nat.h           |   6 ++
 gdb/inf-ptrace.c         |  47 +++++++++--
 gdb/inf-ptrace.h         |  30 +++++++
 gdb/infcmd.c             |   4 +
 gdb/infrun.c             |   5 +-
 gdb/linux-nat.c          | 164 +++++++--------------------------------
 gdb/linux-nat.h          |   4 -
 gdb/record-full.c        |  10 ---
 gdb/remote.c             |  14 ----
 gdb/target.c             |   3 +
 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 ++++++++++++++
 19 files changed, 445 insertions(+), 230 deletions(-)
 create mode 100644 gdbsupport/event-pipe.cc
 create mode 100644 gdbsupport/event-pipe.h

-- 
2.34.1

[PATCH v5 01/15] 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, 190 insertions(+), 3 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 6049fac20ef..1416027e1c9 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 \
@@ -74,6 +78,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 852e98c6bdf..bdceff3b56a 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) \
@@ -160,7 +161,7 @@ am_libgdbsupport_a_OBJECTS = agent.$(OBJEXT) btrace-common.$(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)
+	$(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@)
@@ -338,7 +339,6 @@ pdfdir = @pdfdir@
 prefix = @prefix@
 program_transform_name = @program_transform_name@
 psdir = @psdir@
-runstatedir = @runstatedir@
 sbindir = @sbindir@
 sharedstatedir = @sharedstatedir@
 srcdir = @srcdir@
@@ -361,6 +361,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 \
@@ -400,6 +401,7 @@ libgdbsupport_a_SOURCES = \
     tdesc.cc \
     thread-pool.cc \
     xml-utils.cc \
+    ${eventpipe} \
     $(selftest)
 
 all: config.h
@@ -482,6 +484,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 afaba88ee94..2070a39fbb7 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
@@ -10002,6 +10004,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.
 
 
@@ -10597,6 +10608,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 55477d55c82..1f794605f3c 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.34.1

[PATCH v5 02/15] 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 0cc6923ad02..db3585dfa23 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).
@@ -228,26 +229,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
@@ -257,21 +250,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);
@@ -4202,7 +4181,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.  */
 
@@ -4234,19 +4213,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);
@@ -4258,7 +4231,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.  */
@@ -4270,7 +4243,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
@@ -4280,7 +4253,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.34.1

[PATCH v5 03/15] 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 9e571a4d771..d0cc682f022 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);
 
@@ -3506,28 +3507,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
@@ -5823,21 +5810,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");
 
@@ -5846,12 +5828,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.34.1

[PATCH v5 04/15] Enable async mode on supported targets in target_resume.

[John Baldwin]

Enabling async mode above the target layer removes duplicate code in
::resume methods of async-capable targets.  Commit 5b6d1e4fa4f
("Multi-target support") enabled async mode in do_target_resume after
target_resume returns which is a step in this direction.  However,
other callers of target_resume such as target_continue do not enable
async mode.  Rather than enabling async mode in each of the callers
after target_resume returns, enable async mode at the end of
target_resume.
---
 gdb/infrun.c | 3 ---
 gdb/target.c | 3 +++
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 2e7ed15723f..5959c3e6658 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -2187,9 +2187,6 @@ do_target_resume (ptid_t resume_ptid, bool step, enum gdb_signal sig)
     target_pass_signals (signal_pass);
 
   target_resume (resume_ptid, step, sig);
-
-  if (target_can_async_p ())
-    target_async (1);
 }
 
 /* Resume the inferior.  SIG is the signal to give the inferior
diff --git a/gdb/target.c b/gdb/target.c
index 950f2f08e31..b7033eaa930 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -2673,6 +2673,9 @@ target_resume (ptid_t ptid, int step, enum gdb_signal signal)
      thread's stop_pc as side effect.  */
   set_executing (curr_target, ptid, true);
   clear_inline_frame_state (curr_target, ptid);
+
+  if (target_can_async_p ())
+    target_async (1);
 }
 
 /* See target.h.  */
-- 
2.34.1

[PATCH v5 05/15] Don't enable async mode at the end of target ::resume methods.

[John Baldwin]

Now that target_resume always enables async mode after target::resume
returns, these calls are redundant.

The other place that target resume methods are invoked outside of
target_resume are as the beneath target in record_full_wait_1.  In
this case, 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 db3585dfa23..621118ba862 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -1764,9 +1764,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 76b21523853..bd8c49c1abe 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;
@@ -2062,11 +2057,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 b093ad86675..2711e5a301c 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -6571,16 +6571,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.34.1

[PATCH v5 06/15] 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 5959c3e6658..f8ccafd287d 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -3629,7 +3629,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.34.1

[PATCH v5 07/15] inf-ptrace: Return an IGNORE event 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 | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
index 6e4706a3d20..0b94aad54d7 100644
--- a/gdb/inf-ptrace.c
+++ b/gdb/inf-ptrace.c
@@ -313,9 +313,8 @@ inf_ptrace_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
 			      _("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;
+	  ourstatus->set_ignore ();
+	  return minus_one_ptid;
 	}
 
       /* Ignore terminated detached child processes.  */
-- 
2.34.1

[PATCH v5 08/15] 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 0b94aad54d7..ebcc409b989 100644
--- a/gdb/inf-ptrace.c
+++ b/gdb/inf-ptrace.c
@@ -289,10 +289,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
     {
@@ -300,15 +304,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;
+	    }
+
 	  fprintf_unfiltered (gdb_stderr,
 			      _("Child process unexpectedly missing: %s.\n"),
 			      safe_strerror (save_errno));
-- 
2.34.1

[PATCH v5 09/15] 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 c485fff6ed4..1ed7092fece 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"
@@ -28,6 +31,7 @@
 #include "gdbthread.h"
 #include "gdbsupport/buildargv.h"
 #include "gdbsupport/gdb_wait.h"
+#include "inf-loop.h"
 #include "inf-ptrace.h"
 #include <sys/types.h>
 #ifdef HAVE_SYS_PROCCTL_H
@@ -927,6 +931,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
@@ -998,6 +1110,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.  */
@@ -1166,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;
 
@@ -1382,6 +1499,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.  */
 
@@ -1676,4 +1823,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 6f8b206dcd5..4ac8faad4df 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;
 
@@ -110,6 +120,8 @@ class fbsd_nat_target : public inf_ptrace_target
   void post_startup_inferior (ptid_t) 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.34.1

[PATCH v5 10/15] 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 1ed7092fece..89596f6f822 100644
--- a/gdb/fbsd-nat.c
+++ b/gdb/fbsd-nat.c
@@ -822,7 +822,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);
@@ -851,22 +851,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
 }
@@ -885,13 +885,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++)
     {
@@ -905,7 +905,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
@@ -1180,7 +1180,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
@@ -1189,7 +1191,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;
     }
 
@@ -1219,7 +1221,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 ());
@@ -1307,7 +1309,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);
 
@@ -1339,7 +1341,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
@@ -1402,7 +1404,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);
@@ -1491,7 +1493,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;
 	    }
 	}
@@ -1638,7 +1640,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.34.1

[PATCH v5 11/15] 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 89596f6f822..3f36ebaef52 100644
--- a/gdb/fbsd-nat.c
+++ b/gdb/fbsd-nat.c
@@ -1163,8 +1163,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.34.1

[PATCH v5 12/15] 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 3f36ebaef52..13b4386fd2e 100644
--- a/gdb/fbsd-nat.c
+++ b/gdb/fbsd-nat.c
@@ -820,9 +820,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.34.1

[PATCH v5 13/15] 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    |  4 ----
 5 files changed, 4 insertions(+), 22 deletions(-)

diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
index 13b4386fd2e..ec20a65bf3a 100644
--- a/gdb/fbsd-nat.c
+++ b/gdb/fbsd-nat.c
@@ -1026,19 +1026,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 4ac8faad4df..e479612ea7a 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 9f4ed8bff13..d6daaadb17d 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -2553,6 +2553,10 @@ attach_command (const char *args, int from_tty)
 			     thread_state_string (thread->state));
     }
 
+  /* 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 621118ba862..e8c1496dcb4 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -1220,9 +1220,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);
 }
 
 /* Ptrace-detach the thread with pid PID.  */
diff --git a/gdb/remote.c b/gdb/remote.c
index 2711e5a301c..e7bd59fb755 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -6183,16 +6183,12 @@ 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);
-
-      if (target_can_async_p ())
-	target_async (1);
     }
   else
     {
       gdb_assert (wait_status == NULL);
 
       gdb_assert (target_can_async_p ());
-      target_async (1);
     }
 }
 
-- 
2.34.1

[PATCH v5 14/15] 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 ec20a65bf3a..90800c86737 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"
@@ -933,8 +932,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
@@ -947,22 +944,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
@@ -970,8 +951,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;
 }
@@ -998,34 +978,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
@@ -1101,7 +1069,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.  */
@@ -1500,7 +1468,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);
 
@@ -1511,7 +1479,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 e479612ea7a..6e0abc22b0f 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 ebcc409b989..ea1fe4541f0 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 ()
 {}
 
@@ -527,3 +530,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 01b05f7d20e..62cc7778767 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,7 +60,31 @@ 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);
 
@@ -71,6 +98,9 @@ struct inf_ptrace_target : public inf_child_target
      Such targets will supply an appropriate definition for this
      function.  */
   virtual void post_startup_inferior (ptid_t ptid) = 0;
+
+private:
+  static event_pipe m_event_pipe;
 };
 
 #ifndef __NetBSD__
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index e8c1496dcb4..27d767478dd 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"
@@ -227,32 +227,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);
@@ -4115,14 +4089,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
@@ -4174,10 +4140,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;
 }
@@ -4190,67 +4157,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
@@ -4299,16 +4234,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 befe5b6311a..12a90eccb28 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.34.1

[PATCH v5 15/15] NEWS: Note that the FreeBSD async target supports async mode.

[John Baldwin]

---
 gdb/NEWS | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/gdb/NEWS b/gdb/NEWS
index 8c13cefb22f..7f14a9a374c 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -3,6 +3,8 @@
 
 *** Changes since GDB 11
 
+* The FreeBSD native target now supports async mode.
+
 * Configure changes
 
 --enable-threading
-- 
2.34.1

Re: [PATCH v5 15/15] NEWS: Note that the FreeBSD async target supports async mode.

[Eli Zaretskii]

> From: John Baldwin <jhb@FreeBSD.org>
> Date: Fri, 21 Jan 2022 12:16:31 -0800
> 
> ---
>  gdb/NEWS | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/gdb/NEWS b/gdb/NEWS
> index 8c13cefb22f..7f14a9a374c 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -3,6 +3,8 @@
>  
>  *** Changes since GDB 11
>  
> +* The FreeBSD native target now supports async mode.
> +
>  * Configure changes

This is OK, thanks.

Re: [PATCH v5 04/15] Enable async mode on supported targets in target_resume.

[Joel Brobecker]

Hi John,

> Enabling async mode above the target layer removes duplicate code in
> ::resume methods of async-capable targets.  Commit 5b6d1e4fa4f
> ("Multi-target support") enabled async mode in do_target_resume after
> target_resume returns which is a step in this direction.  However,
> other callers of target_resume such as target_continue do not enable
> async mode.  Rather than enabling async mode in each of the callers
> after target_resume returns, enable async mode at the end of
> target_resume.

Thanks for this. This is OK.

> ---
>  gdb/infrun.c | 3 ---
>  gdb/target.c | 3 +++
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 2e7ed15723f..5959c3e6658 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -2187,9 +2187,6 @@ do_target_resume (ptid_t resume_ptid, bool step, enum gdb_signal sig)
>      target_pass_signals (signal_pass);
>  
>    target_resume (resume_ptid, step, sig);
> -
> -  if (target_can_async_p ())
> -    target_async (1);
>  }
>  
>  /* Resume the inferior.  SIG is the signal to give the inferior
> diff --git a/gdb/target.c b/gdb/target.c
> index 950f2f08e31..b7033eaa930 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -2673,6 +2673,9 @@ target_resume (ptid_t ptid, int step, enum gdb_signal signal)
>       thread's stop_pc as side effect.  */
>    set_executing (curr_target, ptid, true);
>    clear_inline_frame_state (curr_target, ptid);
> +
> +  if (target_can_async_p ())
> +    target_async (1);
>  }
>  
>  /* See target.h.  */
> -- 
> 2.34.1
> 

-- 
Joel

Re: [PATCH v5 05/15] Don't enable async mode at the end of target ::resume methods.

[Joel Brobecker]

Hi John,

> Now that target_resume always enables async mode after target::resume
> returns, these calls are redundant.
> 
> The other place that target resume methods are invoked outside of
> target_resume are as the beneath target in record_full_wait_1.  In
> this case, 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.

Not sure if you expected a review of this patch or not, so I took
a look, and it looks OK to me.

> ---
>  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 db3585dfa23..621118ba862 100644
> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c
> @@ -1764,9 +1764,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 76b21523853..bd8c49c1abe 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;
> @@ -2062,11 +2057,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 b093ad86675..2711e5a301c 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -6571,16 +6571,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.34.1
> 

-- 
Joel

Re: [PATCH v5 07/15] inf-ptrace: Return an IGNORE event if waitpid() fails.

[Joel Brobecker]

Hi John,

On Fri, Jan 21, 2022 at 12:16:23PM -0800, John Baldwin wrote:
> 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 | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
> index 6e4706a3d20..0b94aad54d7 100644
> --- a/gdb/inf-ptrace.c
> +++ b/gdb/inf-ptrace.c
> @@ -313,9 +313,8 @@ inf_ptrace_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
>  			      _("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;
> +	  ourstatus->set_ignore ();
> +	  return minus_one_ptid;
>  	}
>  
>        /* Ignore terminated detached child processes.  */

This patch looks good to me.

-- 
Joel

Re: [PATCH v5 06/15] do_target_wait_1: Clear TARGET_WNOHANG if the target isn't async.

[Simon Marchi]

On 2022-01-21 15:16, John Baldwin wrote:
> 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 5959c3e6658..f8ccafd287d 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -3629,7 +3629,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)

Hi John,

Starting with this patch I see a bunch of multi-target tests start
to fail, for instance gdb.multi/multi-target-info-inferiors.exp.

This is on plain Ubuntu 20.04 amd64.

I don't have much time now, but I can provide more info later if
needed.

Simon

Re: [PATCH v5 06/15] do_target_wait_1: Clear TARGET_WNOHANG if the target isn't async.

[John Baldwin]

On 2/22/22 2:38 PM, Simon Marchi wrote:
> 
> 
> On 2022-01-21 15:16, John Baldwin wrote:
>> 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 5959c3e6658..f8ccafd287d 100644
>> --- a/gdb/infrun.c
>> +++ b/gdb/infrun.c
>> @@ -3629,7 +3629,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)
> 
> Hi John,
> 
> Starting with this patch I see a bunch of multi-target tests start
> to fail, for instance gdb.multi/multi-target-info-inferiors.exp.
> 
> This is on plain Ubuntu 20.04 amd64.
> 
> I don't have much time now, but I can provide more info later if
> needed.

Hmmm, I had tested this in a Ubuntu amd64 VM and hadn't seen any regressions.
However, this change can be easily reverted without reverting the rest of
the series I believe.

Looking at my logs from before, it seems that all of the gdb.multi/multi-target*
tests didn't report a status (either on stock master or in the branch with these
changes) which explains why I didn't see a regression earlier:

(gdb) PASS: gdb.multi/multi-re-run.exp: re_run_inf=2: iter=2: print re_run_var_2
testcase /home/john/work/git/gdb/obj/gdb/testsuite/../../../gdb/testsuite/gdb.multi/multi-re-run.exp completed in 0 seconds
Running /home/john/work/git/gdb/obj/gdb/testsuite/../../../gdb/testsuite/gdb.multi/multi-target-continue.exp ...
testcase /home/john/work/git/gdb/obj/gdb/testsuite/../../../gdb/testsuite/gdb.multi/multi-target-continue.exp completed in 0 seconds
Running /home/john/work/git/gdb/obj/gdb/testsuite/../../../gdb/testsuite/gdb.multi/multi-target-info-inferiors.exp ...
testcase /home/john/work/git/gdb/obj/gdb/testsuite/../../../gdb/testsuite/gdb.multi/multi-target-info-inferiors.exp completed in 0 seconds
Running /home/john/work/git/gdb/obj/gdb/testsuite/../../../gdb/testsuite/gdb.multi/multi-target-interrupt.exp ...
testcase /home/john/work/git/gdb/obj/gdb/testsuite/../../../gdb/testsuite/gdb.multi/multi-target-interrupt.exp completed in 0 seconds
Running /home/john/work/git/gdb/obj/gdb/testsuite/../../../gdb/testsuite/gdb.multi/multi-target-no-resumed.exp ...
testcase /home/john/work/git/gdb/obj/gdb/testsuite/../../../gdb/testsuite/gdb.multi/multi-target-no-resumed.exp completed in 0 seconds
Running /home/john/work/git/gdb/obj/gdb/testsuite/../../../gdb/testsuite/gdb.multi/multi-target-ping-pong-next.exp ...
testcase /home/john/work/git/gdb/obj/gdb/testsuite/../../../gdb/testsuite/gdb.multi/multi-target-ping-pong-next.exp completed in 0 seconds
Running /home/john/work/git/gdb/obj/gdb/testsuite/../../../gdb/testsuite/gdb.multi/multi-target-thread-find.exp ...
testcase /home/john/work/git/gdb/obj/gdb/testsuite/../../../gdb/testsuite/gdb.multi/multi-target-thread-find.exp completed in 0 seconds
Running /home/john/work/git/gdb/obj/gdb/testsuite/../../../gdb/testsuite/gdb.multi/multi-term-settings.exp ...

I have no idea why these tests didn't run in my VM.  If you revert just this
commit and keep the rest of the series do you see any other regressions?

-- 
John Baldwin

Re: [PATCH v5 06/15] do_target_wait_1: Clear TARGET_WNOHANG if the target isn't async.

[Simon Marchi]

On 2022-02-22 19:07, John Baldwin wrote:
> I have no idea why these tests didn't run in my VM.  If you revert just this
> commit and keep the rest of the series do you see any other regressions?

If I revert this commit, the testsuite goes back to "no regressions" for me.

Simon

Re: [PATCH v5 06/15] do_target_wait_1: Clear TARGET_WNOHANG if the target isn't async.

[Simon Marchi]

On 2022-02-22 20:40, Simon Marchi via Gdb-patches wrote:
> 
> 
> On 2022-02-22 19:07, John Baldwin wrote:
>> I have no idea why these tests didn't run in my VM.  If you revert just this
>> commit and keep the rest of the series do you see any other regressions?
> 
> If I revert this commit, the testsuite goes back to "no regressions" for me.

Here's what happens:

1. On remote inferior was started, is now stopped.  It is not "async",
   because when everything is stopped, the async fd is removed from the
   event loop, but it "can async".

2. We run a native inferior, the event loop gets woken up by the native
   target's fd.

3. In do_target_wait, we randomly choose an inferior to call target_wait
   on first, it happens to be the remote inferior.

4. Because the target is currently not "async", we clear
   TARGET_WNOHANG, resulting in synchronous wait.  We therefore block
   here:

  #0  0x00007fe9540dbb4d in select () from /usr/lib/libc.so.6
  #1  0x000055fc7e821da7 in gdb_select (n=15, readfds=0x7ffdb77c1fb0, writefds=0x0, exceptfds=0x7ffdb77c2050, timeout=0x7ffdb77c1f90) at /home/simark/src/binutils-gdb/gdb/posix-hdep.c:31
  #2  0x000055fc7ddef905 in interruptible_select (n=15, readfds=0x7ffdb77c1fb0, writefds=0x0, exceptfds=0x7ffdb77c2050, timeout=0x7ffdb77c1f90) at /home/simark/src/binutils-gdb/gdb/event-top.c:1134
  #3  0x000055fc7eda58e4 in ser_base_wait_for (scb=0x6250002e4100, timeout=1) at /home/simark/src/binutils-gdb/gdb/ser-base.c:240
  #4  0x000055fc7eda66ba in do_ser_base_readchar (scb=0x6250002e4100, timeout=-1) at /home/simark/src/binutils-gdb/gdb/ser-base.c:365
  #5  0x000055fc7eda6ff6 in generic_readchar (scb=0x6250002e4100, timeout=-1, do_readchar=0x55fc7eda663c <do_ser_base_readchar(serial*, int)>) at /home/simark/src/binutils-gdb/gdb/ser-base.c:444
  #6  0x000055fc7eda718a in ser_base_readchar (scb=0x6250002e4100, timeout=-1) at /home/simark/src/binutils-gdb/gdb/ser-base.c:471
  #7  0x000055fc7edb1ecd in serial_readchar (scb=0x6250002e4100, timeout=-1) at /home/simark/src/binutils-gdb/gdb/serial.c:393
  #8  0x000055fc7ec48b8f in remote_target::readchar (this=0x617000038780, timeout=-1) at /home/simark/src/binutils-gdb/gdb/remote.c:9446
  #9  0x000055fc7ec4da82 in remote_target::getpkt_or_notif_sane_1 (this=0x617000038780, buf=0x6170000387a8, forever=1, expecting_notif=1, is_notif=0x7ffdb77c24f0) at /home/simark/src/binutils-gdb/gdb/remote.c:9928
  #10 0x000055fc7ec4f045 in remote_target::getpkt_or_notif_sane (this=0x617000038780, buf=0x6170000387a8, forever=1, is_notif=0x7ffdb77c24f0) at /home/simark/src/binutils-gdb/gdb/remote.c:10037
  #11 0x000055fc7ec354d4 in remote_target::wait_ns (this=0x617000038780, ptid=..., status=0x7ffdb77c33c8, options=...) at /home/simark/src/binutils-gdb/gdb/remote.c:8147
  #12 0x000055fc7ec38aa1 in remote_target::wait (this=0x617000038780, ptid=..., status=0x7ffdb77c33c8, options=...) at /home/simark/src/binutils-gdb/gdb/remote.c:8337
  #13 0x000055fc7f1409ce in target_wait (ptid=..., status=0x7ffdb77c33c8, options=...) at /home/simark/src/binutils-gdb/gdb/target.c:2612
  #14 0x000055fc7e19da98 in do_target_wait_1 (inf=0x617000038080, ptid=..., status=0x7ffdb77c33c8, options=...) at /home/simark/src/binutils-gdb/gdb/infrun.c:3636
  #15 0x000055fc7e19e26b in operator() (__closure=0x7ffdb77c2f90, inf=0x617000038080) at /home/simark/src/binutils-gdb/gdb/infrun.c:3697
  #16 0x000055fc7e19f0c4 in do_target_wait (ecs=0x7ffdb77c33a0, options=...) at /home/simark/src/binutils-gdb/gdb/infrun.c:3716
  #17 0x000055fc7e1a31f7 in fetch_inferior_event () at /home/simark/src/binutils-gdb/gdb/infrun.c:4061

Normally, the wait on the remote target would return nothing, so we
would then wait on the native target and fetch its event.

Simon

Re: [PATCH v5 06/15] do_target_wait_1: Clear TARGET_WNOHANG if the target isn't async.

[John Baldwin]

On 2/23/22 7:51 AM, Simon Marchi wrote:
> On 2022-02-22 20:40, Simon Marchi via Gdb-patches wrote:
>>
>>
>> On 2022-02-22 19:07, John Baldwin wrote:
>>> I have no idea why these tests didn't run in my VM.  If you revert just this
>>> commit and keep the rest of the series do you see any other regressions?
>>
>> If I revert this commit, the testsuite goes back to "no regressions" for me.
> 
> Here's what happens:
> 
> 1. On remote inferior was started, is now stopped.  It is not "async",
>     because when everything is stopped, the async fd is removed from the
>     event loop, but it "can async".
> 
> 2. We run a native inferior, the event loop gets woken up by the native
>     target's fd.
> 
> 3. In do_target_wait, we randomly choose an inferior to call target_wait
>     on first, it happens to be the remote inferior.
> 
> 4. Because the target is currently not "async", we clear
>     TARGET_WNOHANG, resulting in synchronous wait.  We therefore block
>     here:
> 
>    #0  0x00007fe9540dbb4d in select () from /usr/lib/libc.so.6
>    #1  0x000055fc7e821da7 in gdb_select (n=15, readfds=0x7ffdb77c1fb0, writefds=0x0, exceptfds=0x7ffdb77c2050, timeout=0x7ffdb77c1f90) at /home/simark/src/binutils-gdb/gdb/posix-hdep.c:31
>    #2  0x000055fc7ddef905 in interruptible_select (n=15, readfds=0x7ffdb77c1fb0, writefds=0x0, exceptfds=0x7ffdb77c2050, timeout=0x7ffdb77c1f90) at /home/simark/src/binutils-gdb/gdb/event-top.c:1134
>    #3  0x000055fc7eda58e4 in ser_base_wait_for (scb=0x6250002e4100, timeout=1) at /home/simark/src/binutils-gdb/gdb/ser-base.c:240
>    #4  0x000055fc7eda66ba in do_ser_base_readchar (scb=0x6250002e4100, timeout=-1) at /home/simark/src/binutils-gdb/gdb/ser-base.c:365
>    #5  0x000055fc7eda6ff6 in generic_readchar (scb=0x6250002e4100, timeout=-1, do_readchar=0x55fc7eda663c <do_ser_base_readchar(serial*, int)>) at /home/simark/src/binutils-gdb/gdb/ser-base.c:444
>    #6  0x000055fc7eda718a in ser_base_readchar (scb=0x6250002e4100, timeout=-1) at /home/simark/src/binutils-gdb/gdb/ser-base.c:471
>    #7  0x000055fc7edb1ecd in serial_readchar (scb=0x6250002e4100, timeout=-1) at /home/simark/src/binutils-gdb/gdb/serial.c:393
>    #8  0x000055fc7ec48b8f in remote_target::readchar (this=0x617000038780, timeout=-1) at /home/simark/src/binutils-gdb/gdb/remote.c:9446
>    #9  0x000055fc7ec4da82 in remote_target::getpkt_or_notif_sane_1 (this=0x617000038780, buf=0x6170000387a8, forever=1, expecting_notif=1, is_notif=0x7ffdb77c24f0) at /home/simark/src/binutils-gdb/gdb/remote.c:9928
>    #10 0x000055fc7ec4f045 in remote_target::getpkt_or_notif_sane (this=0x617000038780, buf=0x6170000387a8, forever=1, is_notif=0x7ffdb77c24f0) at /home/simark/src/binutils-gdb/gdb/remote.c:10037
>    #11 0x000055fc7ec354d4 in remote_target::wait_ns (this=0x617000038780, ptid=..., status=0x7ffdb77c33c8, options=...) at /home/simark/src/binutils-gdb/gdb/remote.c:8147
>    #12 0x000055fc7ec38aa1 in remote_target::wait (this=0x617000038780, ptid=..., status=0x7ffdb77c33c8, options=...) at /home/simark/src/binutils-gdb/gdb/remote.c:8337
>    #13 0x000055fc7f1409ce in target_wait (ptid=..., status=0x7ffdb77c33c8, options=...) at /home/simark/src/binutils-gdb/gdb/target.c:2612
>    #14 0x000055fc7e19da98 in do_target_wait_1 (inf=0x617000038080, ptid=..., status=0x7ffdb77c33c8, options=...) at /home/simark/src/binutils-gdb/gdb/infrun.c:3636
>    #15 0x000055fc7e19e26b in operator() (__closure=0x7ffdb77c2f90, inf=0x617000038080) at /home/simark/src/binutils-gdb/gdb/infrun.c:3697
>    #16 0x000055fc7e19f0c4 in do_target_wait (ecs=0x7ffdb77c33a0, options=...) at /home/simark/src/binutils-gdb/gdb/infrun.c:3716
>    #17 0x000055fc7e1a31f7 in fetch_inferior_event () at /home/simark/src/binutils-gdb/gdb/infrun.c:4061
> 
> Normally, the wait on the remote target would return nothing, so we
> would then wait on the native target and fetch its event.

Thanks for digging the details up.  I assumed it had something to do with
an initial misunderstanding I had which was that a target was always in
async mode vs only being in async mode when there are non-stopped
inferiors.  Can you please include these details in your commit reverting
this change?  (Or did you want me to push the revert?  I'm happy for you
to do so since you did the testing and investigation.)

-- 
John Baldwin

Re: [PATCH v5 06/15] do_target_wait_1: Clear TARGET_WNOHANG if the target isn't async.

[Simon Marchi]

> Thanks for digging the details up.  I assumed it had something to do with
> an initial misunderstanding I had which was that a target was always in
> async mode vs only being in async mode when there are non-stopped
> inferiors.  Can you please include these details in your commit reverting
> this change?  (Or did you want me to push the revert?  I'm happy for you
> to do so since you did the testing and investigation.)

I'm not sure I understand what you want me to add.  Here's what I came
up with, if you have suggestions of anything to add, let me know.

Simon

From 6adb0a6fb006b32b0ff51dcc59a0a5e90f944a82 Mon Sep 17 00:00:00 2001
From: John Baldwin <jhb@FreeBSD.org>
Date: Tue, 22 Feb 2022 11:22:14 -0800
Subject: [PATCH] Revert "do_target_wait_1: Clear TARGET_WNOHANG if the target
 isn't async."

Commit 14b3360508b1 ("do_target_wait_1: Clear
TARGET_WNOHANG if the target isn't async.") broke some multi-target
tests, such as gdb.multi/multi-target-info-inferiors.exp.  The symptom
is that execution just hangs at some point.  What happens is:

1. One remote inferior is started, and now sits stopped at a breakpoint.
   It is not "async" at this point (but it "can async").

2. We run a native inferior, the event loop gets woken up by the native
   target's fd.

3. In do_target_wait, we randomly choose an inferior to call target_wait
   on first, it happens to be the remote inferior.

4. Because the target is currently not "async", we clear
   TARGET_WNOHANG, resulting in synchronous wait.  We therefore block
   here:

  #0  0x00007fe9540dbb4d in select () from /usr/lib/libc.so.6
  #1  0x000055fc7e821da7 in gdb_select (n=15, readfds=0x7ffdb77c1fb0, writefds=0x0, exceptfds=0x7ffdb77c2050, timeout=0x7ffdb77c1f90) at /home/simark/src/binutils-gdb/gdb/posix-hdep.c:31
  #2  0x000055fc7ddef905 in interruptible_select (n=15, readfds=0x7ffdb77c1fb0, writefds=0x0, exceptfds=0x7ffdb77c2050, timeout=0x7ffdb77c1f90) at /home/simark/src/binutils-gdb/gdb/event-top.c:1134
  #3  0x000055fc7eda58e4 in ser_base_wait_for (scb=0x6250002e4100, timeout=1) at /home/simark/src/binutils-gdb/gdb/ser-base.c:240
  #4  0x000055fc7eda66ba in do_ser_base_readchar (scb=0x6250002e4100, timeout=-1) at /home/simark/src/binutils-gdb/gdb/ser-base.c:365
  #5  0x000055fc7eda6ff6 in generic_readchar (scb=0x6250002e4100, timeout=-1, do_readchar=0x55fc7eda663c <do_ser_base_readchar(serial*, int)>) at /home/simark/src/binutils-gdb/gdb/ser-base.c:444
  #6  0x000055fc7eda718a in ser_base_readchar (scb=0x6250002e4100, timeout=-1) at /home/simark/src/binutils-gdb/gdb/ser-base.c:471
  #7  0x000055fc7edb1ecd in serial_readchar (scb=0x6250002e4100, timeout=-1) at /home/simark/src/binutils-gdb/gdb/serial.c:393
  #8  0x000055fc7ec48b8f in remote_target::readchar (this=0x617000038780, timeout=-1) at /home/simark/src/binutils-gdb/gdb/remote.c:9446
  #9  0x000055fc7ec4da82 in remote_target::getpkt_or_notif_sane_1 (this=0x617000038780, buf=0x6170000387a8, forever=1, expecting_notif=1, is_notif=0x7ffdb77c24f0) at /home/simark/src/binutils-gdb/gdb/remote.c:9928
  #10 0x000055fc7ec4f045 in remote_target::getpkt_or_notif_sane (this=0x617000038780, buf=0x6170000387a8, forever=1, is_notif=0x7ffdb77c24f0) at /home/simark/src/binutils-gdb/gdb/remote.c:10037
  #11 0x000055fc7ec354d4 in remote_target::wait_ns (this=0x617000038780, ptid=..., status=0x7ffdb77c33c8, options=...) at /home/simark/src/binutils-gdb/gdb/remote.c:8147
  #12 0x000055fc7ec38aa1 in remote_target::wait (this=0x617000038780, ptid=..., status=0x7ffdb77c33c8, options=...) at /home/simark/src/binutils-gdb/gdb/remote.c:8337
  #13 0x000055fc7f1409ce in target_wait (ptid=..., status=0x7ffdb77c33c8, options=...) at /home/simark/src/binutils-gdb/gdb/target.c:2612
  #14 0x000055fc7e19da98 in do_target_wait_1 (inf=0x617000038080, ptid=..., status=0x7ffdb77c33c8, options=...) at /home/simark/src/binutils-gdb/gdb/infrun.c:3636
  #15 0x000055fc7e19e26b in operator() (__closure=0x7ffdb77c2f90, inf=0x617000038080) at /home/simark/src/binutils-gdb/gdb/infrun.c:3697
  #16 0x000055fc7e19f0c4 in do_target_wait (ecs=0x7ffdb77c33a0, options=...) at /home/simark/src/binutils-gdb/gdb/infrun.c:3716
  #17 0x000055fc7e1a31f7 in fetch_inferior_event () at /home/simark/src/binutils-gdb/gdb/infrun.c:4061

Before the aforementioned commit, we would not have cleared
TARGET_WNOHANG, the remote target's wait would have returned nothing,
and we would have consumed the native target's event.

After applying this revert, the testsuite state looks as good as before
for me on Ubuntu 20.04 amd64.

Change-Id: Ic17a1642935cabcc16c25cb6899d52e12c2f5c3f
---
 gdb/infrun.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 17ec8b27fa4e..5311822fcb84 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -3627,7 +3627,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_is_async_p ())
+  if (!target_can_async_p ())
     options &= ~TARGET_WNOHANG;
 
   if (deprecated_target_wait_hook)

base-commit: 7190ae0d393d09c3c1f24add83d1e90727402749
-- 
2.35.1

Re: [PATCH v5 06/15] do_target_wait_1: Clear TARGET_WNOHANG if the target isn't async.

[John Baldwin]

On 2/23/22 6:46 PM, Simon Marchi wrote:
>> Thanks for digging the details up.  I assumed it had something to do with
>> an initial misunderstanding I had which was that a target was always in
>> async mode vs only being in async mode when there are non-stopped
>> inferiors.  Can you please include these details in your commit reverting
>> this change?  (Or did you want me to push the revert?  I'm happy for you
>> to do so since you did the testing and investigation.)
> 
> I'm not sure I understand what you want me to add.  Here's what I came
> up with, if you have suggestions of anything to add, let me know.

This looks good to me.  Thanks!

-- 
John Baldwin

Re: [PATCH v5 06/15] do_target_wait_1: Clear TARGET_WNOHANG if the target isn't async.

[Simon Marchi]

On 2022-02-24 14:02, John Baldwin wrote:
> On 2/23/22 6:46 PM, Simon Marchi wrote:
>>> Thanks for digging the details up.  I assumed it had something to do with
>>> an initial misunderstanding I had which was that a target was always in
>>> async mode vs only being in async mode when there are non-stopped
>>> inferiors.  Can you please include these details in your commit reverting
>>> this change?  (Or did you want me to push the revert?  I'm happy for you
>>> to do so since you did the testing and investigation.)
>>
>> I'm not sure I understand what you want me to add.  Here's what I came
>> up with, if you have suggestions of anything to add, let me know.
> 
> This looks good to me.  Thanks!
> 

Thanks, pushed.

Simon