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

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

[John Baldwin]

Changes since the RFC:

- The series has been teste on Linux x86-64 and I did not see any
  regressions.

- The first patch includes some changes from Lancelot Six to only
  compile event-pipe.cc for platforms supporting pipe() or pipe2().  I
  haven't tested this on a Windows build however (and don't have an
  easy way to do so).

- Patches 4 through 6 are new based on Pedro's replies to some of my
  earlier questions.
  
- Patch 7 was originally part of the main "fbsd async" patch but I
  split it out so it is a bit clearer to other platforms which
  inherit from inf-ptrace which parts are shared vs platform-specific.
  It also now uses TARGET_WAITKIND_NO_RESUMED for the ECHILD case.

- Patches 12 and 13 are new and are further refactorings.  I could
  perhaps move them earlier before the main "fbsd async" patch
  if they are ok.  It would make the fbsd async patch itself a
  bit smaller if so, but for the last patch in particular it might
  also be useful to see two targets being updated to understand
  which parts were pulled up into inf-ptrace?

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.
  linux-nat: Don't enable async mode at the end of
    linux_nat_target::resume.
  do_target_wait_1: Clear TARGET_WNOHANG if the target isn't async.
  inf-ptrace: Raise an internal_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: Switch fbsd_nat_target::resume entry debug message from lwp
    to nat.
  fbsd-nat: Return NULL rather than failing thread_alive.
  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                | 151 +++++++++++++++++++++++++-----
 gdb/fbsd-nat.h                |   6 ++
 gdb/inf-ptrace.c              |  51 +++++++++--
 gdb/inf-ptrace.h              |  24 +++++
 gdb/infcmd.c                  |   4 +
 gdb/infrun.c                  |   2 +-
 gdb/linux-nat.c               | 167 +++++++---------------------------
 gdb/linux-nat.h               |   4 -
 gdb/remote.c                  |   1 -
 gdbserver/ChangeLog-2002-2021 |   8 ++
 gdbserver/linux-low.cc        |  43 +++------
 gdbsupport/Makefile.am        |   5 +
 gdbsupport/Makefile.in        |   9 +-
 gdbsupport/configure          |  15 +++
 gdbsupport/configure.ac       |   3 +
 gdbsupport/event-pipe.cc      | 100 ++++++++++++++++++++
 gdbsupport/event-pipe.h       |  56 ++++++++++++
 17 files changed, 442 insertions(+), 207 deletions(-)
 create mode 100644 gdbsupport/event-pipe.cc
 create mode 100644 gdbsupport/event-pipe.h

-- 
2.31.1

[PATCH v2 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).

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 | 100 +++++++++++++++++++++++++++++++++++++++
 gdbsupport/event-pipe.h  |  56 ++++++++++++++++++++++
 6 files changed, 186 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 a9dd02c5b72..f557c45c3f7 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
@@ -9910,6 +9912,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.
 
 
@@ -10448,6 +10459,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 a8fcfe24c32..c0700753839 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..db9f9983cdb
--- /dev/null
+++ b/gdbsupport/event-pipe.cc
@@ -0,0 +1,100 @@
+/* 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 ();
+}
+
+/* Create a new pipe.  */
+
+bool
+event_pipe::open ()
+{
+  if (m_fds[0] != -1)
+    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;
+}
+
+void
+event_pipe::close ()
+{
+  ::close (m_fds[0]);
+  ::close (m_fds[1]);
+  m_fds[0] = -1;
+  m_fds[1] = -1;
+}
+
+/* Get rid of any pending events in the pipe.  */
+
+void
+event_pipe::flush ()
+{
+  int ret;
+  char buf;
+
+  do
+    {
+      ret = read (m_fds[0], &buf, 1);
+    }
+  while (ret >= 0 || (ret == -1 && errno == EINTR));
+}
+
+/* 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.  */
+
+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..25301ec85df
--- /dev/null
+++ b/gdbsupport/event-pipe.h
@@ -0,0 +1,56 @@
+/* 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();
+
+  /* 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.31.1

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

[John Baldwin]

---
 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 e4d0206eaac..5a176ac9208 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>
@@ -110,11 +111,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).
@@ -217,26 +218,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
@@ -246,21 +239,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);
@@ -4192,7 +4171,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.  */
 
@@ -4224,19 +4203,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);
@@ -4248,7 +4221,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.  */
@@ -4260,7 +4233,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
@@ -4270,7 +4243,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.31.1

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

[John Baldwin]

---
 gdbserver/ChangeLog-2002-2021 |  8 +++++++
 gdbserver/linux-low.cc        | 43 +++++++++--------------------------
 2 files changed, 19 insertions(+), 32 deletions(-)

diff --git a/gdbserver/ChangeLog-2002-2021 b/gdbserver/ChangeLog-2002-2021
index c2ff85ed553..8b84750ae68 100644
--- a/gdbserver/ChangeLog-2002-2021
+++ b/gdbserver/ChangeLog-2002-2021
@@ -1,3 +1,11 @@
+2021-06-04  John Baldwin  <jhb@FreeBSD.org>
+
+	* linux-low.cc: Include gdbsupport/event-pipe.h.
+	(linux_event_pipe): Convert to an instance of event_pipe.
+	(target_is_async_p, async_file_flush, async_file_mark): Implement
+	as methods of event_pipe.
+	(linux_process_target::async): Update to use event_pipe methods.
+
 2021-06-19  Mike Frysinger  <vapier@gentoo.org>
 
 	* acinclude.m4: Delete most m4_include's of ../config files.
diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
index 5c6191d941c..27cc72c1b2d 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);
 
@@ -3675,28 +3676,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
@@ -6096,21 +6083,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");
 
@@ -6119,12 +6101,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.31.1

[PATCH v2 04/13] linux-nat: Don't enable async mode at the end of linux_nat_target::resume.

[John Baldwin]

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

Note that there is an earlier call in linux_nat_target::resume.  That
call also marks the async event pipe to report an existing event, so
it needs to stay.
---
 gdb/linux-nat.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 5a176ac9208..3a6d48f08e6 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -1703,9 +1703,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.  */
-- 
2.31.1

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

[John Baldwin]

Previously, TARGET_WNOHANG was cleared it 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 815ebf45c1f..e49b3471e3e 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -3622,7 +3622,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.31.1

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

[John Baldwin]

Previously this returned a TARGET_WAITKIND_SIGNALLED event for
inferior_ptid.  Since the multi-target changes, inferior_ptid is now
invalid during ::wait() methods, so this triggered an assertion
further up the stack.
---
 gdb/inf-ptrace.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
index afa38de6ef7..1f8e72d1aca 100644
--- a/gdb/inf-ptrace.c
+++ b/gdb/inf-ptrace.c
@@ -319,14 +319,9 @@ 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->kind = TARGET_WAITKIND_SIGNALLED;
-	  ourstatus->value.sig = GDB_SIGNAL_UNKNOWN;
-	  return inferior_ptid;
+	  internal_error (__FILE__, __LINE__,
+			  _("Child process unexpectedly missing: %s.\n"),
+			  safe_strerror (save_errno));
 	}
 
       /* Ignore terminated detached child processes.  */
-- 
2.31.1

[PATCH v2 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 1f8e72d1aca..fb734234d8e 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->kind = TARGET_WAITKIND_IGNORE;
+	  return minus_one_ptid;
+	}
+
       if (pid == -1)
 	{
+	  /* In async mode the SIGCHLD might have raced and triggered
+	     a check for an event that had already been reported.  If
+	     the event was the exit of the only remaining child,
+	     waitpid() will fail with ECHILD.  */
+	  if (ptid == minus_one_ptid && save_errno == ECHILD)
+	    {
+	      ourstatus->kind = TARGET_WAITKIND_NO_RESUMED;
+	      return minus_one_ptid;
+	    }
+
 	  internal_error (__FILE__, __LINE__,
 			  _("Child process unexpectedly missing: %s.\n"),
 			  safe_strerror (save_errno));
-- 
2.31.1

[PATCH v2 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 | 155 ++++++++++++++++++++++++++++++++++++++++++++++++-
 gdb/fbsd-nat.h |  12 ++++
 2 files changed, 165 insertions(+), 2 deletions(-)

diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
index 0ae1195791c..36ce00164dd 100644
--- a/gdb/fbsd-nat.c
+++ b/gdb/fbsd-nat.c
@@ -19,14 +19,18 @@
 
 #include "defs.h"
 #include "gdbsupport/byte-vector.h"
+#include "gdbsupport/event-loop.h"
+#include "gdbsupport/event-pipe.h"
 #include "gdbcore.h"
 #include "inferior.h"
 #include "regcache.h"
 #include "regset.h"
 #include "gdbarch.h"
 #include "gdbcmd.h"
+#include "gdbsupport/gdb-sigmask.h"
 #include "gdbthread.h"
 #include "gdbsupport/gdb_wait.h"
+#include "inf-loop.h"
 #include "inf-ptrace.h"
 #include <sys/types.h>
 #ifdef HAVE_SYS_PROCCTL_H
@@ -925,6 +929,118 @@ fbsd_nat_target::update_thread_list ()
 #endif
 }
 
+/* Async mode support.  */
+
+static event_pipe fbsd_nat_event_pipe;
+
+/* Implement the "can_async_p" target method.  */
+
+bool
+fbsd_nat_target::can_async_p ()
+{
+  /* Use async unless the user explicitly prevented it via the "maint
+     set target-async" command.  */
+  return target_async_permitted;
+}
+
+/* Implement the "is_async_p" target method.  */
+
+bool
+fbsd_nat_target::is_async_p ()
+{
+  return fbsd_nat_event_pipe.is_open ();
+}
+
+/* Implement the "async_wait_fd" target method.  */
+
+int
+fbsd_nat_target::async_wait_fd ()
+{
+  return fbsd_nat_event_pipe.event_fd ();
+}
+
+/* SIGCHLD handler notifies the event-loop in async mode.  */
+
+static void
+sigchld_handler (int signo)
+{
+  int old_errno = errno;
+
+  if (fbsd_nat_event_pipe.is_open ())
+    fbsd_nat_event_pipe.mark ();
+
+  errno = old_errno;
+}
+
+/* Callback registered with the target events file descriptor.  */
+
+static void
+handle_target_event (int error, gdb_client_data client_data)
+{
+  inferior_event_handler (INF_REG_EVENT);
+}
+
+/* Implement the "async" target method.  */
+
+void
+fbsd_nat_target::async (int enable)
+{
+  if ((enable != 0) == is_async_p ())
+    return;
+
+  /* Block SIGCHILD while we create/destroy the pipe, as the handler
+     writes to it.  */
+
+  sigset_t chld_mask, prev_mask;
+  sigemptyset (&chld_mask);
+  sigaddset (&chld_mask, SIGCHLD);
+  gdb_sigmask (SIG_BLOCK, &chld_mask, &prev_mask);
+
+  if (enable)
+    {
+      if (!fbsd_nat_event_pipe.open ())
+	internal_error (__FILE__, __LINE__, "failed to create event pipe.");
+
+      add_file_handler (fbsd_nat_event_pipe.event_fd (),
+			handle_target_event, NULL, "fbsd-nat");
+
+      /* Trigger a poll in case there are pending events to
+	 handle.  */
+      fbsd_nat_event_pipe.mark ();
+    }
+  else
+    {
+      delete_file_handler (fbsd_nat_event_pipe.event_fd ());
+      fbsd_nat_event_pipe.close ();
+    }
+
+  gdb_sigmask (SIG_SETMASK, &prev_mask, NULL);
+}
+
+/* Implement the "close" target method.  */
+
+void
+fbsd_nat_target::close ()
+{
+  if (is_async_p ())
+    async (0);
+
+  inf_ptrace_target::close ();
+}
+
+/* Implement the "attach" target method.  */
+
+void
+fbsd_nat_target::attach (const char *args, int from_tty)
+{
+  inf_ptrace_target::attach (args, from_tty);
+
+  /* Curiously, the core does not do this automatically.  */
+  if (target_can_async_p ())
+    target_async (1);
+}
+
+
 #ifdef TDP_RFPPWAIT
 /*
   To catch fork events, PT_FOLLOW_FORK is set on every traced process
@@ -1164,8 +1280,8 @@ fbsd_handle_debug_trap (fbsd_nat_target *target, ptid_t ptid,
    the status in *OURSTATUS.  */
 
 ptid_t
-fbsd_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
-		       target_wait_flags target_options)
+fbsd_nat_target::wait_1 (ptid_t ptid, struct target_waitstatus *ourstatus,
+			 target_wait_flags target_options)
 {
   ptid_t wptid;
 
@@ -1377,6 +1493,33 @@ fbsd_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
     }
 }
 
+ptid_t
+fbsd_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
+		       target_wait_flags target_options)
+{
+  ptid_t wptid;
+
+  fbsd_nat_debug_printf ("[%s], [%s]", target_pid_to_str (ptid).c_str (),
+			 target_options_to_string (target_options).c_str ());
+
+  /* Ensure any subsequent events trigger a new event in the loop.  */
+  if (is_async_p ())
+    fbsd_nat_event_pipe.flush ();
+
+  wptid = wait_1 (ptid, ourstatus, target_options);
+
+  /* If we are in async mode and found an event, there may still be
+     another event pending.  Trigger the event pipe so that that the
+     event loop keeps polling until no event is returned.  */
+  if (is_async_p () && ourstatus->kind != TARGET_WAITKIND_IGNORE)
+    fbsd_nat_event_pipe.mark ();
+
+  fbsd_nat_debug_printf ("returning [%s], [%s]",
+			 target_pid_to_str (wptid).c_str (),
+			 target_waitstatus_to_string (ourstatus).c_str ());
+  return wptid;
+}
+
 #ifdef USE_SIGTRAP_SIGINFO
 /* Implement the "stopped_by_sw_breakpoint" target_ops method.  */
 
@@ -1510,6 +1653,11 @@ fbsd_nat_target::follow_fork (ptid_t child_ptid, target_waitkind fork_kind,
 	  /* Schedule a fake VFORK_DONE event to report on the next
 	     wait.  */
 	  fbsd_add_vfork_done (inferior_ptid);
+
+	  /* If we're in async mode, need to tell the event loop
+	     there's something here to process.  */
+	  if (is_async_p ())
+	    fbsd_nat_event_pipe.mark ();
 	}
 #endif
     }
@@ -1667,4 +1815,7 @@ Enables printf debugging output."),
 			   NULL,
 			   &show_fbsd_nat_debug,
 			   &setdebuglist, &showdebuglist);
+
+  /* Install a SIGCHLD handler.  */
+  signal (SIGCHLD, sigchld_handler);
 }
diff --git a/gdb/fbsd-nat.h b/gdb/fbsd-nat.h
index 3fb502ca8d0..9dd9017c1c3 100644
--- a/gdb/fbsd-nat.h
+++ b/gdb/fbsd-nat.h
@@ -66,9 +66,19 @@ class fbsd_nat_target : public inf_ptrace_target
 
   void update_thread_list () override;
 
+  bool can_async_p () override;
+  bool is_async_p () override;
+
+  int async_wait_fd () override;
+  void async (int) override;
+
+  void close () override;
+
   thread_control_capabilities get_thread_control_capabilities () override
   { return tc_schedlock; }
 
+  void attach (const char *, int) override;
+
   void create_inferior (const char *, const std::string &,
 			char **, int) override;
 
@@ -107,6 +117,8 @@ class fbsd_nat_target : public inf_ptrace_target
   bool supports_disable_randomization () override;
 
 private:
+  ptid_t wait_1 (ptid_t, struct target_waitstatus *, target_wait_flags);
+
   /* Helper routines for use in fetch_registers and store_registers in
      subclasses.  These routines fetch and store a single set of
      registers described by REGSET.  The REGSET's 'regmap' field must
-- 
2.31.1

[PATCH v2 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 36ce00164dd..49dadb0a1e7 100644
--- a/gdb/fbsd-nat.c
+++ b/gdb/fbsd-nat.c
@@ -820,7 +820,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);
@@ -849,22 +849,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
 }
@@ -883,13 +883,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++)
     {
@@ -903,7 +903,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
@@ -1177,7 +1177,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
@@ -1186,7 +1188,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;
     }
 
@@ -1216,7 +1218,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 ());
@@ -1304,7 +1306,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, 0);
 
@@ -1336,7 +1338,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
@@ -1399,7 +1401,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, 0);
@@ -1485,7 +1487,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;
 	    }
 	}
@@ -1625,7 +1627,7 @@ fbsd_nat_target::follow_fork (ptid_t child_ptid, target_waitkind fork_kind,
 	 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.31.1

[PATCH v2 10/13] fbsd-nat: Switch fbsd_nat_target::resume entry debug message from lwp to nat.

[John Baldwin]

While here, 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 49dadb0a1e7..ec57bfe3d8a 100644
--- a/gdb/fbsd-nat.c
+++ b/gdb/fbsd-nat.c
@@ -1160,8 +1160,9 @@ fbsd_nat_target::resume (ptid_t ptid, int step, enum gdb_signal signo)
     return;
 #endif
 
-  fbsd_lwp_debug_printf ("ptid (%d, %ld, %ld)", ptid.pid (), ptid.lwp (),
-			 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.31.1

[PATCH v2 11/13] fbsd-nat: Return NULL rather than failing thread_alive.

[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 NULL 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 ec57bfe3d8a..1c27b698cce 100644
--- a/gdb/fbsd-nat.c
+++ b/gdb/fbsd-nat.c
@@ -818,9 +818,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 NULL;
   if (ptrace (PT_LWPINFO, lwp, (caddr_t) &pl, sizeof pl) == -1)
-    perror_with_name (("ptrace (PT_LWPINFO)"));
+    return NULL;
   if (strcmp (kp.ki_comm, pl.pl_tdname) == 0)
     return NULL;
   xsnprintf (buf, sizeof buf, "%s", pl.pl_tdname);
-- 
2.31.1

[PATCH v2 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    |  1 -
 5 files changed, 4 insertions(+), 19 deletions(-)

diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
index 1c27b698cce..34713151cbe 100644
--- a/gdb/fbsd-nat.c
+++ b/gdb/fbsd-nat.c
@@ -1028,19 +1028,6 @@ fbsd_nat_target::close ()
   inf_ptrace_target::close ();
 }
 
-/* Implement the "attach" target method.  */
-
-void
-fbsd_nat_target::attach (const char *args, int from_tty)
-{
-  inf_ptrace_target::attach (args, from_tty);
-
-  /* Curiously, the core does not do this automatically.  */
-  if (target_can_async_p ())
-    target_async (1);
-}
-
-
 #ifdef TDP_RFPPWAIT
 /*
   To catch fork events, PT_FOLLOW_FORK is set on every traced process
diff --git a/gdb/fbsd-nat.h b/gdb/fbsd-nat.h
index 9dd9017c1c3..ad7f76bca40 100644
--- a/gdb/fbsd-nat.h
+++ b/gdb/fbsd-nat.h
@@ -77,8 +77,6 @@ class fbsd_nat_target : public inf_ptrace_target
   thread_control_capabilities get_thread_control_capabilities () override
   { return tc_schedlock; }
 
-  void attach (const char *, int) override;
-
   void create_inferior (const char *, const std::string &,
 			char **, int) override;
 
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index c183b60e81a..d4e678643a0 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -2541,6 +2541,10 @@ attach_command (const char *args, int from_tty)
      shouldn't refer to attach_target again.  */
   attach_target = NULL;
 
+  /* Enable async mode if it is supported by the target.  */
+  if (target_can_async_p ())
+    target_async (1);
+
   /* Set up the "saved terminal modes" of the inferior
      based on what modes we are starting it with.  */
   target_terminal::init ();
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 3a6d48f08e6..b80fa25c902 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -1214,9 +1214,6 @@ linux_nat_target::attach (const char *args, int from_tty)
      threads and associate pthread info with each LWP.  */
   linux_proc_attach_tgid_threads (lp->ptid.pid (),
 				  attach_proc_task_lwp_callback);
-
-  if (target_can_async_p ())
-    target_async (1);
 }
 
 /* Get pending signal of THREAD as a host signal number, for detaching
diff --git a/gdb/remote.c b/gdb/remote.c
index 552481fc551..2fb95c1e93b 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -6095,7 +6095,6 @@ extended_remote_target::attach (const char *args, int from_tty)
       gdb_assert (wait_status == NULL);
 
       gdb_assert (target_can_async_p ());
-      target_async (1);
     }
 }
 
-- 
2.31.1

[PATCH v2 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 |  24 +++++++++
 gdb/linux-nat.c  | 126 ++++++++++-------------------------------------
 gdb/linux-nat.h  |   4 --
 6 files changed, 75 insertions(+), 148 deletions(-)

diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
index 34713151cbe..7c6632cf258 100644
--- a/gdb/fbsd-nat.c
+++ b/gdb/fbsd-nat.c
@@ -20,7 +20,6 @@
 #include "defs.h"
 #include "gdbsupport/byte-vector.h"
 #include "gdbsupport/event-loop.h"
-#include "gdbsupport/event-pipe.h"
 #include "gdbcore.h"
 #include "inferior.h"
 #include "regcache.h"
@@ -931,8 +930,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
@@ -943,22 +940,6 @@ fbsd_nat_target::can_async_p ()
   return target_async_permitted;
 }
 
-/* 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
@@ -966,8 +947,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,36 +978,24 @@ 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 ();
     }
 
   gdb_sigmask (SIG_SETMASK, &prev_mask, NULL);
 }
 
-/* Implement the "close" target method.  */
-
-void
-fbsd_nat_target::close ()
-{
-  if (is_async_p ())
-    async (0);
-
-  inf_ptrace_target::close ();
-}
-
 #ifdef TDP_RFPPWAIT
 /*
   To catch fork events, PT_FOLLOW_FORK is set on every traced process
@@ -1494,7 +1462,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);
 
@@ -1502,7 +1470,7 @@ fbsd_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
      another event pending.  Trigger the event pipe so that that the
      event loop keeps polling until no event is returned.  */
   if (is_async_p () && ourstatus->kind != TARGET_WAITKIND_IGNORE)
-    fbsd_nat_event_pipe.mark ();
+    async_file_mark ();
 
   fbsd_nat_debug_printf ("returning [%s], [%s]",
 			 target_pid_to_str (wptid).c_str (),
@@ -1647,7 +1615,7 @@ fbsd_nat_target::follow_fork (ptid_t child_ptid, target_waitkind fork_kind,
 	  /* If we're in async mode, need to tell the event loop
 	     there's something here to process.  */
 	  if (is_async_p ())
-	    fbsd_nat_event_pipe.mark ();
+	    async_file_mark ();
 	}
 #endif
     }
diff --git a/gdb/fbsd-nat.h b/gdb/fbsd-nat.h
index ad7f76bca40..4620750fc20 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 fb734234d8e..a63bebfb2b4 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::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..bdea7a9ac71 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,30 @@ struct inf_ptrace_target : public inf_child_target
 					ULONGEST offset, ULONGEST len,
 					ULONGEST *xfered_len) override;
 
+  bool is_async_p () override { return event_pipe.is_open (); }
+
+  int async_wait_fd () override { return event_pipe.event_fd (); }
+
+  /* Helper routine used from SIGCHLD handlers to signal the async
+     event pipe.  */
+  static void async_file_mark_if_open ()
+  {
+    if (event_pipe.is_open ())
+      event_pipe.mark ();
+  }
+
 protected:
+  /* Helper routines for interacting with the async event pipe.  */
+  bool async_file_open () { return event_pipe.open (); }
+  void async_file_close () { event_pipe.close (); }
+  void async_file_flush () { event_pipe.flush (); }
+  void async_file_mark () { event_pipe.mark (); }
+
   /* Cleanup the inferior after a successful ptrace detach.  */
   void detach_success (inferior *inf);
+
+private:
+  static class event_pipe event_pipe;
 };
 
 #ifndef __NetBSD__
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index b80fa25c902..831153929be 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>
@@ -216,32 +215,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);
@@ -4107,14 +4080,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
@@ -4164,10 +4129,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;
 }
@@ -4180,67 +4146,39 @@ 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;
+
+  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_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;
+
+  restore_child_signals_mask (&prev_mask);
 }
 
 /* Stop an LWP, and push a GDB_SIGNAL_0 stop status if no other
@@ -4288,16 +4226,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 ee36c56519b..f61790f732a 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.31.1

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

[John Baldwin]

On 8/3/21 11:49 AM, John Baldwin wrote:
> Changes since the RFC:
> 
> - The series has been teste on Linux x86-64 and I did not see any
>    regressions.
> 
> - The first patch includes some changes from Lancelot Six to only
>    compile event-pipe.cc for platforms supporting pipe() or pipe2().  I
>    haven't tested this on a Windows build however (and don't have an
>    easy way to do so).
> 
> - Patches 4 through 6 are new based on Pedro's replies to some of my
>    earlier questions.
>    
> - Patch 7 was originally part of the main "fbsd async" patch but I
>    split it out so it is a bit clearer to other platforms which
>    inherit from inf-ptrace which parts are shared vs platform-specific.
>    It also now uses TARGET_WAITKIND_NO_RESUMED for the ECHILD case.
> 
> - Patches 12 and 13 are new and are further refactorings.  I could
>    perhaps move them earlier before the main "fbsd async" patch
>    if they are ok.  It would make the fbsd async patch itself a
>    bit smaller if so, but for the last patch in particular it might
>    also be useful to see two targets being updated to understand
>    which parts were pulled up into inf-ptrace?

Ping.

-- 
John Baldwin

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

[Lancelot SIX]

> +
> +class event_pipe
> +{
> +public:
> +  event_pipe() = default;
> +  ~event_pipe();

Hi,

Sorry I am a bit late to react.

I think it would make sense to remove copy and assign operators from
this class with:

  DISABLE_COPY_AND_ASSIGN (event_pipe);

This will also disable move.  None of those operations are required and
the default implementations are unsuited for this use-case.  Removing
them is safer in my opinion.

Best,
Lancelot.

> +
> +  /* 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.31.1
> 

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

[Andrew Burgess]

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

> 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).

Thanks, this looks like a great idea.

It might be worth mentioning that later commits will do the work of
replacing the existing pipes with this code, otherwise it looks like
you forgot half the commit!

> 
> 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 | 100 +++++++++++++++++++++++++++++++++++++++
>  gdbsupport/event-pipe.h  |  56 ++++++++++++++++++++++
>  6 files changed, 186 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 a9dd02c5b72..f557c45c3f7 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
> @@ -9910,6 +9912,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.
>  
>  
> @@ -10448,6 +10459,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 a8fcfe24c32..c0700753839 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..db9f9983cdb
> --- /dev/null
> +++ b/gdbsupport/event-pipe.cc
> @@ -0,0 +1,100 @@
> +/* 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 ();
> +}
> +
> +/* Create a new pipe.  */

The GDB style is to document functions once, in the header file.  In
the source file we should just have: '/* See event-pipe.h.  */'.  The
aim is to try and avoid comments diverging between the two locations.

It certainly used to be the case that every function was expected to
have a comment of some form, but things have slipped a little in the
C++ world for things like constructors and destructors, so I guess
your choice for them.

> +
> +bool
> +event_pipe::open ()
> +{
> +  if (m_fds[0] != -1)
> +    return false;

This might be clearer if written as:

  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) {

The opening '{' should be on the next line.

> +    close ();
> +    return false;
> +  }
> +
> +  return true;
> +}
> +
> +void
> +event_pipe::close ()
> +{
> +  ::close (m_fds[0]);
> +  ::close (m_fds[1]);
> +  m_fds[0] = -1;
> +  m_fds[1] = -1;
> +}
> +
> +/* Get rid of any pending events in the pipe.  */
> +
> +void
> +event_pipe::flush ()
> +{
> +  int ret;
> +  char buf;
> +
> +  do
> +    {
> +      ret = read (m_fds[0], &buf, 1);
> +    }
> +  while (ret >= 0 || (ret == -1 && errno == EINTR));
> +}
> +
> +/* 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.  */
> +
> +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..25301ec85df
> --- /dev/null
> +++ b/gdbsupport/event-pipe.h
> @@ -0,0 +1,56 @@
> +/* 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();

I agree with Lancelot about using DISABLE_COPY_AND_ASSIGN here.

> +
> +  /* 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 GDB style seems to be to place single line functions like this on
the next line, as in:

  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]; }

And again.

Thanks,
Andrew


> +
> +  /* 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.31.1
> 

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

[Andrew Burgess]

<--- It's nice to say _something_ in the commit message, even if it's
     just a single sentence.

Otherwise, LGTM.

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

> ---
>  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 e4d0206eaac..5a176ac9208 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>
> @@ -110,11 +111,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).
> @@ -217,26 +218,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
> @@ -246,21 +239,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);
> @@ -4192,7 +4171,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.  */
>  
> @@ -4224,19 +4203,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);
> @@ -4248,7 +4221,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.  */
> @@ -4260,7 +4233,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
> @@ -4270,7 +4243,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.31.1
> 

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

[Andrew Burgess]

This LGTM, except I don't believe the ChangeLog entry is required.

Thanks,
Andrew


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

> ---
>  gdbserver/ChangeLog-2002-2021 |  8 +++++++
>  gdbserver/linux-low.cc        | 43 +++++++++--------------------------
>  2 files changed, 19 insertions(+), 32 deletions(-)
> 
> diff --git a/gdbserver/ChangeLog-2002-2021 b/gdbserver/ChangeLog-2002-2021
> index c2ff85ed553..8b84750ae68 100644
> --- a/gdbserver/ChangeLog-2002-2021
> +++ b/gdbserver/ChangeLog-2002-2021
> @@ -1,3 +1,11 @@
> +2021-06-04  John Baldwin  <jhb@FreeBSD.org>
> +
> +	* linux-low.cc: Include gdbsupport/event-pipe.h.
> +	(linux_event_pipe): Convert to an instance of event_pipe.
> +	(target_is_async_p, async_file_flush, async_file_mark): Implement
> +	as methods of event_pipe.
> +	(linux_process_target::async): Update to use event_pipe methods.
> +
>  2021-06-19  Mike Frysinger  <vapier@gentoo.org>
>  
>  	* acinclude.m4: Delete most m4_include's of ../config files.
> diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
> index 5c6191d941c..27cc72c1b2d 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);
>  
> @@ -3675,28 +3676,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
> @@ -6096,21 +6083,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");
>  
> @@ -6119,12 +6101,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.31.1
> 

Re: [PATCH v2 04/13] linux-nat: Don't enable async mode at the end of linux_nat_target::resume.

[Andrew Burgess]

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

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

The only thing I spotted where this might result in a change of
behaviour is in record-full.c in record_full_wait_1, there's this
line:

  ops->beneath ()->resume (ptid, step, GDB_SIGNAL_0);

Doesn't this mean that if the record target is sitting on top of a
Linux target, then me could potentially see a change of behaviour
here?

I know almost nothing about the record target, so I can't really offer
any insights into whether the situation about could actually happen or
not.

Maybe somebody else will have some thoughts, but at a minimum, it is
probably worth mentioning that there might be a change for that
case.... Unless you know for sure that there isn't.

Thanks,
Andrew





> 
> Note that there is an earlier call in linux_nat_target::resume.  That
> call also marks the async event pipe to report an existing event, so
> it needs to stay.
> ---
>  gdb/linux-nat.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
> index 5a176ac9208..3a6d48f08e6 100644
> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c
> @@ -1703,9 +1703,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.  */
> -- 
> 2.31.1
> 

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

[Andrew Burgess]

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

> Previously, TARGET_WNOHANG was cleared it a target supported async

typo: "it" -> "if".

> mode even if async mode wasn't currently enabled.  This change only
> permits TARGET_WNOHANG if async mode is enabled.

Otherwise, LGTM.

Thanks,
Andrew

> ---
>  gdb/infrun.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 815ebf45c1f..e49b3471e3e 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -3622,7 +3622,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.31.1
> 

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

[Andrew Burgess]

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

> Previously this returned a TARGET_WAITKIND_SIGNALLED event for
> inferior_ptid.  Since the multi-target changes, inferior_ptid is now
> invalid during ::wait() methods, so this triggered an assertion
> further up the stack.
> ---
>  gdb/inf-ptrace.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
> index afa38de6ef7..1f8e72d1aca 100644
> --- a/gdb/inf-ptrace.c
> +++ b/gdb/inf-ptrace.c
> @@ -319,14 +319,9 @@ 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->kind = TARGET_WAITKIND_SIGNALLED;
> -	  ourstatus->value.sig = GDB_SIGNAL_UNKNOWN;
> -	  return inferior_ptid;
> +	  internal_error (__FILE__, __LINE__,
> +			  _("Child process unexpectedly missing: %s.\n"),
> +			  safe_strerror (save_errno));
>  	}

Single statement if blocks should not have '{ ... }' around them.

I wonder if we could use perror_with_name here instead of
internal_error, there's at least one place this is done in
linux-nat.c.

Thanks,
Andrew


>  
>        /* Ignore terminated detached child processes.  */
> -- 
> 2.31.1
> 

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

[Andrew Burgess]

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

> - 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 1f8e72d1aca..fb734234d8e 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->kind = TARGET_WAITKIND_IGNORE;

Should we not do:

  outstatus->set_ignore ();

here?

> +	  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->kind = TARGET_WAITKIND_NO_RESUMED;

Similarly:

  outstatus->set_no_resumed ();

maybe?

Otherwise, LGTM.

Thanks,
Andrew

> +	      return minus_one_ptid;
> +	    }
> +
>  	  internal_error (__FILE__, __LINE__,
>  			  _("Child process unexpectedly missing: %s.\n"),
>  			  safe_strerror (save_errno));
> -- 
> 2.31.1
> 

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

[Andrew Burgess]

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

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

With this patch:

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

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

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

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

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

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

Is there a reason for this difference?

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

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

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

Thanks,
Andrew

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

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

[Andrew Burgess]

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

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

LGTM.

Thanks,
Andrew


> 
> diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
> index 36ce00164dd..49dadb0a1e7 100644
> --- a/gdb/fbsd-nat.c
> +++ b/gdb/fbsd-nat.c
> @@ -820,7 +820,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);
> @@ -849,22 +849,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
>  }
> @@ -883,13 +883,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++)
>      {
> @@ -903,7 +903,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
> @@ -1177,7 +1177,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
> @@ -1186,7 +1188,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;
>      }
>  
> @@ -1216,7 +1218,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 ());
> @@ -1304,7 +1306,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, 0);
>  
> @@ -1336,7 +1338,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
> @@ -1399,7 +1401,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, 0);
> @@ -1485,7 +1487,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;
>  	    }
>  	}
> @@ -1625,7 +1627,7 @@ fbsd_nat_target::follow_fork (ptid_t child_ptid, target_waitkind fork_kind,
>  	 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.31.1
> 

Re: [PATCH v2 10/13] fbsd-nat: Switch fbsd_nat_target::resume entry debug message from lwp to nat.

[Andrew Burgess]

I didn't understand what the subject line had to do with this change,
maybe update it based on the current patch?

But the patch itself looks great.

Thanks,
Andrew

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

> While here, 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 49dadb0a1e7..ec57bfe3d8a 100644
> --- a/gdb/fbsd-nat.c
> +++ b/gdb/fbsd-nat.c
> @@ -1160,8 +1160,9 @@ fbsd_nat_target::resume (ptid_t ptid, int step, enum gdb_signal signo)
>      return;
>  #endif
>  
> -  fbsd_lwp_debug_printf ("ptid (%d, %ld, %ld)", ptid.pid (), ptid.lwp (),
> -			 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.31.1
> 

Re: [PATCH v2 11/13] fbsd-nat: Return NULL rather than failing thread_alive.

[Andrew Burgess]

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

> 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 NULL 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 ec57bfe3d8a..1c27b698cce 100644
> --- a/gdb/fbsd-nat.c
> +++ b/gdb/fbsd-nat.c
> @@ -818,9 +818,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 NULL;
>    if (ptrace (PT_LWPINFO, lwp, (caddr_t) &pl, sizeof pl) == -1)
> -    perror_with_name (("ptrace (PT_LWPINFO)"));
> +    return NULL;

GDB has switched to using nullptr now instead of NULL for new code.

In your description you say that the code EBUSY indicates that the
call failed due to the thread running.

Should we check for that error code, and then return nullptr - are
there other error conditions that we don't want to hide?

Thanks,
Andrew


>    if (strcmp (kp.ki_comm, pl.pl_tdname) == 0)
>      return NULL;
>    xsnprintf (buf, sizeof buf, "%s", pl.pl_tdname);
> -- 
> 2.31.1
> 

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

[Andrew Burgess]

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

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

This makes sense to me.  I did have one comment.

> ---
>  gdb/fbsd-nat.c  | 13 -------------
>  gdb/fbsd-nat.h  |  2 --
>  gdb/infcmd.c    |  4 ++++
>  gdb/linux-nat.c |  3 ---
>  gdb/remote.c    |  1 -
>  5 files changed, 4 insertions(+), 19 deletions(-)
> 
> diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
> index 1c27b698cce..34713151cbe 100644
> --- a/gdb/fbsd-nat.c
> +++ b/gdb/fbsd-nat.c
> @@ -1028,19 +1028,6 @@ fbsd_nat_target::close ()
>    inf_ptrace_target::close ();
>  }
>  
> -/* Implement the "attach" target method.  */
> -
> -void
> -fbsd_nat_target::attach (const char *args, int from_tty)
> -{
> -  inf_ptrace_target::attach (args, from_tty);
> -
> -  /* Curiously, the core does not do this automatically.  */
> -  if (target_can_async_p ())
> -    target_async (1);
> -}
> -
> -
>  #ifdef TDP_RFPPWAIT
>  /*
>    To catch fork events, PT_FOLLOW_FORK is set on every traced process
> diff --git a/gdb/fbsd-nat.h b/gdb/fbsd-nat.h
> index 9dd9017c1c3..ad7f76bca40 100644
> --- a/gdb/fbsd-nat.h
> +++ b/gdb/fbsd-nat.h
> @@ -77,8 +77,6 @@ class fbsd_nat_target : public inf_ptrace_target
>    thread_control_capabilities get_thread_control_capabilities () override
>    { return tc_schedlock; }
>  
> -  void attach (const char *, int) override;
> -
>    void create_inferior (const char *, const std::string &,
>  			char **, int) override;
>  
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index c183b60e81a..d4e678643a0 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -2541,6 +2541,10 @@ attach_command (const char *args, int from_tty)
>       shouldn't refer to attach_target again.  */
>    attach_target = NULL;
>  
> +  /* Enable async mode if it is supported by the target.  */
> +  if (target_can_async_p ())
> +    target_async (1);
> +
>    /* Set up the "saved terminal modes" of the inferior
>       based on what modes we are starting it with.  */
>    target_terminal::init ();
> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
> index 3a6d48f08e6..b80fa25c902 100644
> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c
> @@ -1214,9 +1214,6 @@ linux_nat_target::attach (const char *args, int from_tty)
>       threads and associate pthread info with each LWP.  */
>    linux_proc_attach_tgid_threads (lp->ptid.pid (),
>  				  attach_proc_task_lwp_callback);
> -
> -  if (target_can_async_p ())
> -    target_async (1);
>  }
>  
>  /* Get pending signal of THREAD as a host signal number, for detaching
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 552481fc551..2fb95c1e93b 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -6095,7 +6095,6 @@ extended_remote_target::attach (const char *args, int from_tty)
>        gdb_assert (wait_status == NULL);
>  
>        gdb_assert (target_can_async_p ());
> -      target_async (1);
>      }

I think there's a second call to target_async that can be deleted a
few lines earlier, when the condition  '!target_is_non_stop_p ()' and
then 'target_can_async_p ()' is true.  I'd be tempted to replace that
earlier occurrence with 'gdb_assert (target_can_async_p ());' though.

Thanks,
Andrew

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

[Andrew Burgess]

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

> 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.

This feels like a nice clean up.

> ---
>  gdb/fbsd-nat.c   |  50 ++++---------------
>  gdb/fbsd-nat.h   |   4 --
>  gdb/inf-ptrace.c |  15 ++++++
>  gdb/inf-ptrace.h |  24 +++++++++
>  gdb/linux-nat.c  | 126 ++++++++++-------------------------------------
>  gdb/linux-nat.h  |   4 --
>  6 files changed, 75 insertions(+), 148 deletions(-)
> 
> diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
> index 34713151cbe..7c6632cf258 100644
> --- a/gdb/fbsd-nat.c
> +++ b/gdb/fbsd-nat.c
> @@ -20,7 +20,6 @@
>  #include "defs.h"
>  #include "gdbsupport/byte-vector.h"
>  #include "gdbsupport/event-loop.h"
> -#include "gdbsupport/event-pipe.h"
>  #include "gdbcore.h"
>  #include "inferior.h"
>  #include "regcache.h"
> @@ -931,8 +930,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
> @@ -943,22 +940,6 @@ fbsd_nat_target::can_async_p ()
>    return target_async_permitted;
>  }
>  
> -/* 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
> @@ -966,8 +947,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,36 +978,24 @@ 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 ();
>      }
>  
>    gdb_sigmask (SIG_SETMASK, &prev_mask, NULL);
>  }
>  
> -/* Implement the "close" target method.  */
> -
> -void
> -fbsd_nat_target::close ()
> -{
> -  if (is_async_p ())
> -    async (0);
> -
> -  inf_ptrace_target::close ();
> -}
> -
>  #ifdef TDP_RFPPWAIT
>  /*
>    To catch fork events, PT_FOLLOW_FORK is set on every traced process
> @@ -1494,7 +1462,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);
>  
> @@ -1502,7 +1470,7 @@ fbsd_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
>       another event pending.  Trigger the event pipe so that that the
>       event loop keeps polling until no event is returned.  */
>    if (is_async_p () && ourstatus->kind != TARGET_WAITKIND_IGNORE)
> -    fbsd_nat_event_pipe.mark ();
> +    async_file_mark ();
>  
>    fbsd_nat_debug_printf ("returning [%s], [%s]",
>  			 target_pid_to_str (wptid).c_str (),
> @@ -1647,7 +1615,7 @@ fbsd_nat_target::follow_fork (ptid_t child_ptid, target_waitkind fork_kind,
>  	  /* If we're in async mode, need to tell the event loop
>  	     there's something here to process.  */
>  	  if (is_async_p ())
> -	    fbsd_nat_event_pipe.mark ();
> +	    async_file_mark ();
>  	}
>  #endif
>      }
> diff --git a/gdb/fbsd-nat.h b/gdb/fbsd-nat.h
> index ad7f76bca40..4620750fc20 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 fb734234d8e..a63bebfb2b4 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::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..bdea7a9ac71 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,30 @@ struct inf_ptrace_target : public inf_child_target
>  					ULONGEST offset, ULONGEST len,
>  					ULONGEST *xfered_len) override;
>  
> +  bool is_async_p () override { return event_pipe.is_open (); }

Single line implementations should be on the next line, i.e.:

  bool is_async_p () override
  { return event_pipe.is_open (); }

there's a bunch of these below.

> +
> +  int async_wait_fd () override { return event_pipe.event_fd (); }
> +
> +  /* Helper routine used from SIGCHLD handlers to signal the async
> +     event pipe.  */
> +  static void async_file_mark_if_open ()
> +  {
> +    if (event_pipe.is_open ())
> +      event_pipe.mark ();
> +  }
> +
>  protected:
> +  /* Helper routines for interacting with the async event pipe.  */
> +  bool async_file_open () { return event_pipe.open (); }
> +  void async_file_close () { event_pipe.close (); }
> +  void async_file_flush () { event_pipe.flush (); }
> +  void async_file_mark () { event_pipe.mark (); }
> +
>    /* Cleanup the inferior after a successful ptrace detach.  */
>    void detach_success (inferior *inf);
> +
> +private:
> +  static class event_pipe event_pipe;

The member variable should be renamed m_event_pipe, in which case you
should be able to drop the "class" I think.

>  };
>  
>  #ifndef __NetBSD__
> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
> index b80fa25c902..831153929be 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>
> @@ -216,32 +215,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);
> @@ -4107,14 +4080,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
> @@ -4164,10 +4129,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;
>  }
> @@ -4180,67 +4146,39 @@ 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;
> +
> +  sigset_t prev_mask;
> +
> +  /* Block child signals while we create/destroy the pipe, as their
> +     handler writes to it.  */
> +  block_child_signals (&prev_mask);

Not your problem as it was a bug that existed before, but if we ever
throw an error below then the signal mask would not be restored.  We
should really switch to using a new scoped_block_child_signals class.

Thanks,
Andrew

> +
>    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;
> +
> +  restore_child_signals_mask (&prev_mask);
>  }
>  
>  /* Stop an LWP, and push a GDB_SIGNAL_0 stop status if no other
> @@ -4288,16 +4226,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 ee36c56519b..f61790f732a 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.31.1
> 

Re: [PATCH v2 04/13] linux-nat: Don't enable async mode at the end of linux_nat_target::resume.

[John Baldwin]

On 11/24/21 5:24 AM, Andrew Burgess wrote:
> * John Baldwin <jhb@FreeBSD.org> [2021-08-03 11:49:51 -0700]:
> 
>> Commit 5b6d1e4fa4f ("Multi-target support") enabled async mode in the caller
>> (do_target_resume) after target::resume returns making this call
>> redundant.
> 
> The only thing I spotted where this might result in a change of
> behaviour is in record-full.c in record_full_wait_1, there's this
> line:
> 
>    ops->beneath ()->resume (ptid, step, GDB_SIGNAL_0);
> 
> Doesn't this mean that if the record target is sitting on top of a
> Linux target, then me could potentially see a change of behaviour
> here?
> 
> I know almost nothing about the record target, so I can't really offer
> any insights into whether the situation about could actually happen or
> not.
> 
> Maybe somebody else will have some thoughts, but at a minimum, it is
> probably worth mentioning that there might be a change for that
> case.... Unless you know for sure that there isn't.

Hmm, I was not aware of that case.  This is a change Pedro had suggested
and it didn't show any regressions in the test suite on a Linux/x86-64
VM, but it very well might be that the test suite doesn't cover record?

Hmm, looking at record_full_target::resume, it enables async right after
calling the beneath method and it also does so before returning, so I
suspect it's explicit call is safe to elide for the same reason as this
patch.  That is, in the code today, linux_nat_target::resume enables async
mode, then would return to remote_full_target::resume which would enable
async mode (without any other statements in between), and then return
to do_target_resume which would enable async mode a third time (again without
any other code in between aside from any RAII destructors when returning
from the target methods).  Given, that I think this should be safe, and
I can add a patch to this series to remove the call from
remote_full_target::resume.

> Thanks,
> Andrew
> 
> 
> 
> 
> 
>>
>> Note that there is an earlier call in linux_nat_target::resume.  That
>> call also marks the async event pipe to report an existing event, so
>> it needs to stay.
>> ---
>>   gdb/linux-nat.c | 3 ---
>>   1 file changed, 3 deletions(-)
>>
>> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
>> index 5a176ac9208..3a6d48f08e6 100644
>> --- a/gdb/linux-nat.c
>> +++ b/gdb/linux-nat.c
>> @@ -1703,9 +1703,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.  */
>> -- 
>> 2.31.1
>>
> 


-- 
John Baldwin

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

[John Baldwin]

On 11/24/21 6:31 AM, Andrew Burgess wrote:
> * John Baldwin <jhb@FreeBSD.org> [2021-08-03 11:49:54 -0700]:
>> @@ -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->kind = TARGET_WAITKIND_IGNORE;
> 
> Should we not do:
> 
>    outstatus->set_ignore ();
> 
> here?
> 
>> +	  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->kind = TARGET_WAITKIND_NO_RESUMED;
> 
> Similarly:
> 
>    outstatus->set_no_resumed ();
> 
> maybe?
> 
> Otherwise, LGTM.

Yes, this series predates Simon's cleanups so I'll have to fix that when
rebasing.

-- 
John Baldwin

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

[Simon Marchi]

On 2021-11-24 9:16 a.m., Andrew Burgess via Gdb-patches wrote:
> * John Baldwin <jhb@FreeBSD.org> [2021-08-03 11:49:53 -0700]:
>
>> Previously this returned a TARGET_WAITKIND_SIGNALLED event for
>> inferior_ptid.  Since the multi-target changes, inferior_ptid is now
>> invalid during ::wait() methods, so this triggered an assertion
>> further up the stack.
>> ---
>>  gdb/inf-ptrace.c | 11 +++--------
>>  1 file changed, 3 insertions(+), 8 deletions(-)
>>
>> diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
>> index afa38de6ef7..1f8e72d1aca 100644
>> --- a/gdb/inf-ptrace.c
>> +++ b/gdb/inf-ptrace.c
>> @@ -319,14 +319,9 @@ 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->kind = TARGET_WAITKIND_SIGNALLED;
>> -	  ourstatus->value.sig = GDB_SIGNAL_UNKNOWN;
>> -	  return inferior_ptid;
>> +	  internal_error (__FILE__, __LINE__,
>> +			  _("Child process unexpectedly missing: %s.\n"),
>> +			  safe_strerror (save_errno));
>>  	}
>
> Single statement if blocks should not have '{ ... }' around them.
>
> I wonder if we could use perror_with_name here instead of
> internal_error, there's at least one place this is done in
> linux-nat.c.

If we are being pedantic about the uses of internal_error vs throwing an
error: factors external to GDB should not be able to cause an internal
error.  The kernel is an external factor: a bug in the kernel could
cause this branch to be taken, so it should probably not make GDB
internal error.  In that regard, perror_with_name would be better.
Although I would be fine with assuming that the kernel does not have
bugs (except when actively working around known bugs).

Otherwise, I would suggest using gdb_assert_not_reached instead of
internal_error, to achieve the same thing.

Simon

Re: [PATCH v2 11/13] fbsd-nat: Return NULL rather than failing thread_alive.

[John Baldwin]

On 11/24/21 7:12 AM, Andrew Burgess wrote:
> * John Baldwin <jhb@FreeBSD.org> [2021-08-03 11:49:58 -0700]:
> 
>> 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 NULL 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 ec57bfe3d8a..1c27b698cce 100644
>> --- a/gdb/fbsd-nat.c
>> +++ b/gdb/fbsd-nat.c
>> @@ -818,9 +818,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 NULL;
>>     if (ptrace (PT_LWPINFO, lwp, (caddr_t) &pl, sizeof pl) == -1)
>> -    perror_with_name (("ptrace (PT_LWPINFO)"));
>> +    return NULL;
> 
> GDB has switched to using nullptr now instead of NULL for new code.

In this case there is an existing return NULL a few lines down so I was
being consistent within the function, but I can change these to nullptr.

> In your description you say that the code EBUSY indicates that the
> call failed due to the thread running.
> 
> Should we check for that error code, and then return nullptr - are
> there other error conditions that we don't want to hide?

It seemed to me in seeing the uses of this function that it would in
general be better to return an empty name than to throw errors since
thread names aren't needed for correctness.  linux_proc_tid_get_name
similarly returns a NULL pointer when encountering errors (missing
file in /proc or the file in /proc is empty).

-- 
John Baldwin

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

[John Baldwin]

On 11/24/21 12:16 PM, Simon Marchi wrote:
> On 2021-11-24 9:16 a.m., Andrew Burgess via Gdb-patches wrote:
>> * John Baldwin <jhb@FreeBSD.org> [2021-08-03 11:49:53 -0700]:
>>
>>> Previously this returned a TARGET_WAITKIND_SIGNALLED event for
>>> inferior_ptid.  Since the multi-target changes, inferior_ptid is now
>>> invalid during ::wait() methods, so this triggered an assertion
>>> further up the stack.
>>> ---
>>>   gdb/inf-ptrace.c | 11 +++--------
>>>   1 file changed, 3 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
>>> index afa38de6ef7..1f8e72d1aca 100644
>>> --- a/gdb/inf-ptrace.c
>>> +++ b/gdb/inf-ptrace.c
>>> @@ -319,14 +319,9 @@ 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->kind = TARGET_WAITKIND_SIGNALLED;
>>> -	  ourstatus->value.sig = GDB_SIGNAL_UNKNOWN;
>>> -	  return inferior_ptid;
>>> +	  internal_error (__FILE__, __LINE__,
>>> +			  _("Child process unexpectedly missing: %s.\n"),
>>> +			  safe_strerror (save_errno));
>>>   	}
>>
>> Single statement if blocks should not have '{ ... }' around them.
>>
>> I wonder if we could use perror_with_name here instead of
>> internal_error, there's at least one place this is done in
>> linux-nat.c.
> 
> If we are being pedantic about the uses of internal_error vs throwing an
> error: factors external to GDB should not be able to cause an internal
> error.  The kernel is an external factor: a bug in the kernel could
> cause this branch to be taken, so it should probably not make GDB
> internal error.  In that regard, perror_with_name would be better.
> Although I would be fine with assuming that the kernel does not have
> bugs (except when actively working around known bugs).
> 
> Otherwise, I would suggest using gdb_assert_not_reached instead of
> internal_error, to achieve the same thing.

I think treating this as a kernel bug (and thus perror_with_name) is fine.
The only wrinkle is having to set errno back to save_errno explicitly before
calling perror_with_name, but that is minor.

-- 
John Baldwin

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

[John Baldwin]

On 11/24/21 7:00 AM, Andrew Burgess wrote:
> * John Baldwin <jhb@FreeBSD.org> [2021-08-03 11:49:55 -0700]:
>> @@ -1377,6 +1493,33 @@ fbsd_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
>>       }
>>   }
>>   
>> +ptid_t
>> +fbsd_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
>> +		       target_wait_flags target_options)
>> +{
>> +  ptid_t wptid;
>> +
>> +  fbsd_nat_debug_printf ("[%s], [%s]", target_pid_to_str (ptid).c_str (),
>> +			 target_options_to_string (target_options).c_str ());
>> +
>> +  /* Ensure any subsequent events trigger a new event in the loop.  */
>> +  if (is_async_p ())
>> +    fbsd_nat_event_pipe.flush ();
>> +
>> +  wptid = wait_1 (ptid, ourstatus, target_options);
>> +
>> +  /* If we are in async mode and found an event, there may still be
>> +     another event pending.  Trigger the event pipe so that that the
>> +     event loop keeps polling until no event is returned.  */
>> +  if (is_async_p () && ourstatus->kind != TARGET_WAITKIND_IGNORE)
>> +    fbsd_nat_event_pipe.mark ();
> 
> I notice you use a different condition here to the one present in
> linux-nat.c, specifically, if you get an event of type
> TARGET_WAITKIND_NO_RESUMED, you'll remark the pipe, while Linux
> doesn't.
> 
> Is there a reason for this difference?

Hmm.  Initially I had been returning IGNORE instead of NO_RESUMED for the ECHILD
error from waitpid() and Pedro had suggested fixing the waitkind for ECHILD during
the review of version 1 of the series.  I think when I made that change to
inf-ptrace.c I failed to update this hunk.  We probably do not want to keep
calling waitpid() after an ECHILD.

In terms of the check in linux-nat.c for the ptid being minus_one_ptid, here I'm
less certain.  I suppose the edge case there is if the core is waiting for an
event for a specific ptid (such as a step) and another process raises SIGCLD,
we might miss the other process' event.  Presumably the event raised by
inferior_event_handler is going to always do a wait() with minus_one_ptid so
it shouldn't spin endlessly to always mark after a wait() for a specific ptid.

>> @@ -1510,6 +1653,11 @@ fbsd_nat_target::follow_fork (ptid_t child_ptid, target_waitkind fork_kind,
>>   	  /* Schedule a fake VFORK_DONE event to report on the next
>>   	     wait.  */
>>   	  fbsd_add_vfork_done (inferior_ptid);
>> +
>> +	  /* If we're in async mode, need to tell the event loop
>> +	     there's something here to process.  */
>> +	  if (is_async_p ())
>> +	    fbsd_nat_event_pipe.mark ();
> 
> If feels like this marking of the event pipe should be within
> fbsd_add_vfork_done maybe?  The two actions, queuing the fake event,
> and marking the async event pipe are tightly coupled, right?
> 
> I guess you might need to make fbsd_add_vfork_done a private member
> function so you can easily call is_async_p(), or just call
> 'target_is_async_p()' instead.

Done.  I went with just using target_is_async_p().

-- 
John Baldwin

Re: [PATCH v2 10/13] fbsd-nat: Switch fbsd_nat_target::resume entry debug message from lwp to nat.

[John Baldwin]

On 11/24/21 7:05 AM, Andrew Burgess wrote:
> I didn't understand what the subject line had to do with this change,
> maybe update it based on the current patch?

It's a bit obscure, but it moves the printf from being conditional on
'set debug fbsd-lwp' to being conditional on 'set debug fbsd-nat' instead.

I will rework the subject line to be more generic and add that detail to
the log.

> But the patch itself looks great.
> 
> Thanks,
> Andrew
> 
> * John Baldwin <jhb@FreeBSD.org> [2021-08-03 11:49:57 -0700]:
> 
>> While here, 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 49dadb0a1e7..ec57bfe3d8a 100644
>> --- a/gdb/fbsd-nat.c
>> +++ b/gdb/fbsd-nat.c
>> @@ -1160,8 +1160,9 @@ fbsd_nat_target::resume (ptid_t ptid, int step, enum gdb_signal signo)
>>       return;
>>   #endif
>>   
>> -  fbsd_lwp_debug_printf ("ptid (%d, %ld, %ld)", ptid.pid (), ptid.lwp (),
>> -			 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.31.1
>>
> 


-- 
John Baldwin

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

[John Baldwin]

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

Hmm, yes, that call to target_async is effectively the last statement in that
set of conditions so can be elided.  I'm not sure adding an assertion in its
place makes sense though as it is already inside the target_can_async_p()
block, so you would end up with:

    if (target_can_async_p())
      {
        ..
        gdb_assert (target_can_async_p());
      }

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

-- 
John Baldwin

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

[John Baldwin]

On 11/24/21 7:30 AM, Andrew Burgess wrote:
> * John Baldwin <jhb@FreeBSD.org> [2021-08-03 11:50:00 -0700]:
>>   /* target_async implementation.  */
>>   
>>   void
>>   linux_nat_target::async (int enable)
>>   {
>> +  if ((enable != 0) == is_async_p ())
>> +    return;
>> +
>> +  sigset_t prev_mask;
>> +
>> +  /* Block child signals while we create/destroy the pipe, as their
>> +     handler writes to it.  */
>> +  block_child_signals (&prev_mask);
> 
> Not your problem as it was a bug that existed before, but if we ever
> throw an error below then the signal mask would not be restored.  We
> should really switch to using a new scoped_block_child_signals class.

This one might be a bit subtle as block_child_signals has the side effect
of updating the global variable blocked_mask.  However, this code seem to
have rotted a bit.  blocked_mask is set to empty in _initialize_linux_nat,
then the first call to block_child_signals sets SIGCLD (which is then
never un-set as far as I can see) before using sigprocmask to block
SIGCLD.  I don't know if at some point in time it used to be removed
from blocked_mask?  Looks like the LinuxThreads bits might have messed
with blocked_mask in the past (but that code has been removed).

I do think using gdb::block_signals() might be ok (even if a bit
heavyweight as it blocks multiple signals), and can probably be used to
replace all the uses of block_child_signals.  That's probably best as a
separate cleanup though.

-- 
John Baldwin

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

[John Baldwin]

On 11/24/21 4:14 PM, John Baldwin wrote:
> On 11/24/21 7:30 AM, Andrew Burgess wrote:
>> * John Baldwin <jhb@FreeBSD.org> [2021-08-03 11:50:00 -0700]:
>>>    /* target_async implementation.  */
>>>    
>>>    void
>>>    linux_nat_target::async (int enable)
>>>    {
>>> +  if ((enable != 0) == is_async_p ())
>>> +    return;
>>> +
>>> +  sigset_t prev_mask;
>>> +
>>> +  /* Block child signals while we create/destroy the pipe, as their
>>> +     handler writes to it.  */
>>> +  block_child_signals (&prev_mask);
>>
>> Not your problem as it was a bug that existed before, but if we ever
>> throw an error below then the signal mask would not be restored.  We
>> should really switch to using a new scoped_block_child_signals class.
> 
> This one might be a bit subtle as block_child_signals has the side effect
> of updating the global variable blocked_mask.  However, this code seem to
> have rotted a bit.  blocked_mask is set to empty in _initialize_linux_nat,
> then the first call to block_child_signals sets SIGCLD (which is then
> never un-set as far as I can see) before using sigprocmask to block
> SIGCLD.  I don't know if at some point in time it used to be removed
> from blocked_mask?  Looks like the LinuxThreads bits might have messed
> with blocked_mask in the past (but that code has been removed).
> 
> I do think using gdb::block_signals() might be ok (even if a bit
> heavyweight as it blocks multiple signals), and can probably be used to
> replace all the uses of block_child_signals.  That's probably best as a
> separate cleanup though.

Actually, while looking at this further, I think adapting this specific
place in ::async is ok, but I'm inclined to leave the other places
in linux-nat.c alone.  I'll roll that into this change.

-- 
John Baldwin

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

[Andrew Burgess]

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

> On 11/24/21 12:16 PM, Simon Marchi wrote:
> > On 2021-11-24 9:16 a.m., Andrew Burgess via Gdb-patches wrote:
> > > * John Baldwin <jhb@FreeBSD.org> [2021-08-03 11:49:53 -0700]:
> > > 
> > > > Previously this returned a TARGET_WAITKIND_SIGNALLED event for
> > > > inferior_ptid.  Since the multi-target changes, inferior_ptid is now
> > > > invalid during ::wait() methods, so this triggered an assertion
> > > > further up the stack.
> > > > ---
> > > >   gdb/inf-ptrace.c | 11 +++--------
> > > >   1 file changed, 3 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
> > > > index afa38de6ef7..1f8e72d1aca 100644
> > > > --- a/gdb/inf-ptrace.c
> > > > +++ b/gdb/inf-ptrace.c
> > > > @@ -319,14 +319,9 @@ 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->kind = TARGET_WAITKIND_SIGNALLED;
> > > > -	  ourstatus->value.sig = GDB_SIGNAL_UNKNOWN;
> > > > -	  return inferior_ptid;
> > > > +	  internal_error (__FILE__, __LINE__,
> > > > +			  _("Child process unexpectedly missing: %s.\n"),
> > > > +			  safe_strerror (save_errno));
> > > >   	}
> > > 
> > > Single statement if blocks should not have '{ ... }' around them.
> > > 
> > > I wonder if we could use perror_with_name here instead of
> > > internal_error, there's at least one place this is done in
> > > linux-nat.c.
> > 
> > If we are being pedantic about the uses of internal_error vs throwing an
> > error: factors external to GDB should not be able to cause an internal
> > error.  The kernel is an external factor: a bug in the kernel could
> > cause this branch to be taken, so it should probably not make GDB
> > internal error.  In that regard, perror_with_name would be better.
> > Although I would be fine with assuming that the kernel does not have
> > bugs (except when actively working around known bugs).
> > 
> > Otherwise, I would suggest using gdb_assert_not_reached instead of
> > internal_error, to achieve the same thing.
> 
> I think treating this as a kernel bug (and thus perror_with_name) is fine.

I don't think that perror_with_name implies this is a kernel bug.  The
perror function is just a wrapper around 'error' and safe_strerror -
so basically what you have already except using error, not
internal_error.

Though I guess if we get -1 back then this implies something has gone
wrong somewhere, right?  We expect to see some event for the child no
matter how the kernel kills it, so, if we get -1, that implies the
child disappeared without a trace, right?  But possibly the bug could
be that we already collected the event, and lost it somehow, so it
could be a GDB bug...

OOI did you manage to hit this case during your work on this series?
I guess from your comments you did, is that something that's still
repeatable, or was this only when you had GDB in a broken state?

Thanks,
Andrew

> The only wrinkle is having to set errno back to save_errno explicitly before
> calling perror_with_name, but that is minor.
> 
> -- 
> John Baldwin
> 

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

[John Baldwin]

On 11/25/21 2:30 AM, Andrew Burgess wrote:
> * John Baldwin <jhb@FreeBSD.org> [2021-11-24 14:00:16 -0800]:
> 
>> On 11/24/21 12:16 PM, Simon Marchi wrote:
>>> On 2021-11-24 9:16 a.m., Andrew Burgess via Gdb-patches wrote:
>>>> * John Baldwin <jhb@FreeBSD.org> [2021-08-03 11:49:53 -0700]:
>>>>
>>>>> Previously this returned a TARGET_WAITKIND_SIGNALLED event for
>>>>> inferior_ptid.  Since the multi-target changes, inferior_ptid is now
>>>>> invalid during ::wait() methods, so this triggered an assertion
>>>>> further up the stack.
>>>>> ---
>>>>>    gdb/inf-ptrace.c | 11 +++--------
>>>>>    1 file changed, 3 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
>>>>> index afa38de6ef7..1f8e72d1aca 100644
>>>>> --- a/gdb/inf-ptrace.c
>>>>> +++ b/gdb/inf-ptrace.c
>>>>> @@ -319,14 +319,9 @@ 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->kind = TARGET_WAITKIND_SIGNALLED;
>>>>> -	  ourstatus->value.sig = GDB_SIGNAL_UNKNOWN;
>>>>> -	  return inferior_ptid;
>>>>> +	  internal_error (__FILE__, __LINE__,
>>>>> +			  _("Child process unexpectedly missing: %s.\n"),
>>>>> +			  safe_strerror (save_errno));
>>>>>    	}
>>>>
>>>> Single statement if blocks should not have '{ ... }' around them.
>>>>
>>>> I wonder if we could use perror_with_name here instead of
>>>> internal_error, there's at least one place this is done in
>>>> linux-nat.c.
>>>
>>> If we are being pedantic about the uses of internal_error vs throwing an
>>> error: factors external to GDB should not be able to cause an internal
>>> error.  The kernel is an external factor: a bug in the kernel could
>>> cause this branch to be taken, so it should probably not make GDB
>>> internal error.  In that regard, perror_with_name would be better.
>>> Although I would be fine with assuming that the kernel does not have
>>> bugs (except when actively working around known bugs).
>>>
>>> Otherwise, I would suggest using gdb_assert_not_reached instead of
>>> internal_error, to achieve the same thing.
>>
>> I think treating this as a kernel bug (and thus perror_with_name) is fine.
> 
> I don't think that perror_with_name implies this is a kernel bug.  The
> perror function is just a wrapper around 'error' and safe_strerror -
> so basically what you have already except using error, not
> internal_error.
> 
> Though I guess if we get -1 back then this implies something has gone
> wrong somewhere, right?  We expect to see some event for the child no
> matter how the kernel kills it, so, if we get -1, that implies the
> child disappeared without a trace, right?  But possibly the bug could
> be that we already collected the event, and lost it somehow, so it
> could be a GDB bug...

Yes, it could be either one.  I had chosen internal_error to replace the
existing fprintf somewhat arbitrarily.  I don't have a strong opinion
on what type of error it should be, only that it is best to throw the
error closer to the real problem than to get a later assertion failure.

> OOI did you manage to hit this case during your work on this series?
> I guess from your comments you did, is that something that's still
> repeatable, or was this only when you had GDB in a broken state?

I think I only hit this when I had a bug earlier in this series and found
that the code was not only sketchy (in my mind, if you go back in annotate
it claims to be a workaround for a kernel bug added 20+ years ago), it no
longer worked after multi target as it always trips an assertion since
it effectively returns an event for null_ptid.

-- 
John Baldwin

Re: [PATCH v2 04/13] linux-nat: Don't enable async mode at the end of linux_nat_target::resume.

[Andrew Burgess]

* John Baldwin <jhb@FreeBSD.org> [2021-11-24 08:03:36 -0800]:

> On 11/24/21 5:24 AM, Andrew Burgess wrote:
> > * John Baldwin <jhb@FreeBSD.org> [2021-08-03 11:49:51 -0700]:
> > 
> > > Commit 5b6d1e4fa4f ("Multi-target support") enabled async mode in the caller
> > > (do_target_resume) after target::resume returns making this call
> > > redundant.
> > 
> > The only thing I spotted where this might result in a change of
> > behaviour is in record-full.c in record_full_wait_1, there's this
> > line:
> > 
> >    ops->beneath ()->resume (ptid, step, GDB_SIGNAL_0);
> > 
> > Doesn't this mean that if the record target is sitting on top of a
> > Linux target, then me could potentially see a change of behaviour
> > here?
> > 
> > I know almost nothing about the record target, so I can't really offer
> > any insights into whether the situation about could actually happen or
> > not.
> > 
> > Maybe somebody else will have some thoughts, but at a minimum, it is
> > probably worth mentioning that there might be a change for that
> > case.... Unless you know for sure that there isn't.
> 
> Hmm, I was not aware of that case.  This is a change Pedro had suggested
> and it didn't show any regressions in the test suite on a Linux/x86-64
> VM, but it very well might be that the test suite doesn't cover
> record?

I suspect it's not well tested.

> 
> Hmm, looking at record_full_target::resume, it enables async right after
> calling the beneath method and it also does so before returning, so I
> suspect it's explicit call is safe to elide for the same reason as this
> patch.  That is, in the code today, linux_nat_target::resume enables async
> mode, then would return to remote_full_target::resume which would enable
> async mode (without any other statements in between), and then return
> to do_target_resume which would enable async mode a third time (again without
> any other code in between aside from any RAII destructors when returning
> from the target methods).  Given, that I think this should be safe, and
> I can add a patch to this series to remove the call from
> remote_full_target::resume.

That's all true, but I wasn't commenting on
record_full_target::resume, rather record_full_wait_1, which is called
from record_full_base_target::wait.

I'd certainly be happy for your change to go in.  Having looked again,
I don't think there's going to be any problems, looking at other
targets, it doesn't seem like having to (re-)enable async mode is
something we'd normally expect to do on the ::wait path...

I only pointed this out as it seems to be a case that doesn't align
with the claims in your commit message...

Thanks,
Andrew

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

[Andrew Burgess]

* John Baldwin <jhb@FreeBSD.org> [2021-11-25 08:38:16 -0800]:

> On 11/25/21 2:30 AM, Andrew Burgess wrote:
> > * John Baldwin <jhb@FreeBSD.org> [2021-11-24 14:00:16 -0800]:
> > 
> > > On 11/24/21 12:16 PM, Simon Marchi wrote:
> > > > On 2021-11-24 9:16 a.m., Andrew Burgess via Gdb-patches wrote:
> > > > > * John Baldwin <jhb@FreeBSD.org> [2021-08-03 11:49:53 -0700]:
> > > > > 
> > > > > > Previously this returned a TARGET_WAITKIND_SIGNALLED event for
> > > > > > inferior_ptid.  Since the multi-target changes, inferior_ptid is now
> > > > > > invalid during ::wait() methods, so this triggered an assertion
> > > > > > further up the stack.
> > > > > > ---
> > > > > >    gdb/inf-ptrace.c | 11 +++--------
> > > > > >    1 file changed, 3 insertions(+), 8 deletions(-)
> > > > > > 
> > > > > > diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
> > > > > > index afa38de6ef7..1f8e72d1aca 100644
> > > > > > --- a/gdb/inf-ptrace.c
> > > > > > +++ b/gdb/inf-ptrace.c
> > > > > > @@ -319,14 +319,9 @@ 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->kind = TARGET_WAITKIND_SIGNALLED;
> > > > > > -	  ourstatus->value.sig = GDB_SIGNAL_UNKNOWN;
> > > > > > -	  return inferior_ptid;
> > > > > > +	  internal_error (__FILE__, __LINE__,
> > > > > > +			  _("Child process unexpectedly missing: %s.\n"),
> > > > > > +			  safe_strerror (save_errno));
> > > > > >    	}
> > > > > 
> > > > > Single statement if blocks should not have '{ ... }' around them.
> > > > > 
> > > > > I wonder if we could use perror_with_name here instead of
> > > > > internal_error, there's at least one place this is done in
> > > > > linux-nat.c.
> > > > 
> > > > If we are being pedantic about the uses of internal_error vs throwing an
> > > > error: factors external to GDB should not be able to cause an internal
> > > > error.  The kernel is an external factor: a bug in the kernel could
> > > > cause this branch to be taken, so it should probably not make GDB
> > > > internal error.  In that regard, perror_with_name would be better.
> > > > Although I would be fine with assuming that the kernel does not have
> > > > bugs (except when actively working around known bugs).
> > > > 
> > > > Otherwise, I would suggest using gdb_assert_not_reached instead of
> > > > internal_error, to achieve the same thing.
> > > 
> > > I think treating this as a kernel bug (and thus perror_with_name) is fine.
> > 
> > I don't think that perror_with_name implies this is a kernel bug.  The
> > perror function is just a wrapper around 'error' and safe_strerror -
> > so basically what you have already except using error, not
> > internal_error.
> > 
> > Though I guess if we get -1 back then this implies something has gone
> > wrong somewhere, right?  We expect to see some event for the child no
> > matter how the kernel kills it, so, if we get -1, that implies the
> > child disappeared without a trace, right?  But possibly the bug could
> > be that we already collected the event, and lost it somehow, so it
> > could be a GDB bug...
> 
> Yes, it could be either one.  I had chosen internal_error to replace the
> existing fprintf somewhat arbitrarily.  I don't have a strong opinion
> on what type of error it should be, only that it is best to throw the
> error closer to the real problem than to get a later assertion
> failure.

For this sort of situation we should assume GDB doesn't have bugs, and
that this situation must be caused by an external problem, so
`error`.  If we later find there are bugs in GDB, we'd then fix them.

Thanks,
Andrew


> 
> > OOI did you manage to hit this case during your work on this series?
> > I guess from your comments you did, is that something that's still
> > repeatable, or was this only when you had GDB in a broken state?
> 
> I think I only hit this when I had a bug earlier in this series and found
> that the code was not only sketchy (in my mind, if you go back in annotate
> it claims to be a workaround for a kernel bug added 20+ years ago), it no
> longer worked after multi target as it always trips an assertion since
> it effectively returns an event for null_ptid.
> 
> -- 
> John Baldwin
> 

Re: [PATCH v2 11/13] fbsd-nat: Return NULL rather than failing thread_alive.

[Andrew Burgess]

* John Baldwin <jhb@FreeBSD.org> [2021-11-24 13:29:34 -0800]:

> On 11/24/21 7:12 AM, Andrew Burgess wrote:
> > * John Baldwin <jhb@FreeBSD.org> [2021-08-03 11:49:58 -0700]:
> > 
> > > 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 NULL 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 ec57bfe3d8a..1c27b698cce 100644
> > > --- a/gdb/fbsd-nat.c
> > > +++ b/gdb/fbsd-nat.c
> > > @@ -818,9 +818,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 NULL;
> > >     if (ptrace (PT_LWPINFO, lwp, (caddr_t) &pl, sizeof pl) == -1)
> > > -    perror_with_name (("ptrace (PT_LWPINFO)"));
> > > +    return NULL;
> > 
> > GDB has switched to using nullptr now instead of NULL for new code.
> 
> In this case there is an existing return NULL a few lines down so I was
> being consistent within the function, but I can change these to nullptr.
> 
> > In your description you say that the code EBUSY indicates that the
> > call failed due to the thread running.
> > 
> > Should we check for that error code, and then return nullptr - are
> > there other error conditions that we don't want to hide?
> 
> It seemed to me in seeing the uses of this function that it would in
> general be better to return an empty name than to throw errors since
> thread names aren't needed for correctness.  linux_proc_tid_get_name
> similarly returns a NULL pointer when encountering errors (missing
> file in /proc or the file in /proc is empty).

Good point.  This is all fine with me then.

Thanks,
Andrew

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

[Andrew Burgess]

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

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

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

Thanks,
Andrew


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

Re: [PATCH v2 04/13] linux-nat: Don't enable async mode at the end of linux_nat_target::resume.

[John Baldwin]

On 12/1/21 4:02 AM, Andrew Burgess wrote:
> * John Baldwin <jhb@FreeBSD.org> [2021-11-24 08:03:36 -0800]:
> 
>> On 11/24/21 5:24 AM, Andrew Burgess wrote:
>>> * John Baldwin <jhb@FreeBSD.org> [2021-08-03 11:49:51 -0700]:
>>>
>>>> Commit 5b6d1e4fa4f ("Multi-target support") enabled async mode in the caller
>>>> (do_target_resume) after target::resume returns making this call
>>>> redundant.
>>>
>>> The only thing I spotted where this might result in a change of
>>> behaviour is in record-full.c in record_full_wait_1, there's this
>>> line:
>>>
>>>     ops->beneath ()->resume (ptid, step, GDB_SIGNAL_0);
>>>
>>> Doesn't this mean that if the record target is sitting on top of a
>>> Linux target, then me could potentially see a change of behaviour
>>> here?
>>>
>>> I know almost nothing about the record target, so I can't really offer
>>> any insights into whether the situation about could actually happen or
>>> not.
>>>
>>> Maybe somebody else will have some thoughts, but at a minimum, it is
>>> probably worth mentioning that there might be a change for that
>>> case.... Unless you know for sure that there isn't.
>>
>> Hmm, I was not aware of that case.  This is a change Pedro had suggested
>> and it didn't show any regressions in the test suite on a Linux/x86-64
>> VM, but it very well might be that the test suite doesn't cover
>> record?
> 
> I suspect it's not well tested.
> 
>>
>> Hmm, looking at record_full_target::resume, it enables async right after
>> calling the beneath method and it also does so before returning, so I
>> suspect it's explicit call is safe to elide for the same reason as this
>> patch.  That is, in the code today, linux_nat_target::resume enables async
>> mode, then would return to remote_full_target::resume which would enable
>> async mode (without any other statements in between), and then return
>> to do_target_resume which would enable async mode a third time (again without
>> any other code in between aside from any RAII destructors when returning
>> from the target methods).  Given, that I think this should be safe, and
>> I can add a patch to this series to remove the call from
>> remote_full_target::resume.
> 
> That's all true, but I wasn't commenting on
> record_full_target::resume, rather record_full_wait_1, which is called
> from record_full_base_target::wait.
> 
> I'd certainly be happy for your change to go in.  Having looked again,
> I don't think there's going to be any problems, looking at other
> targets, it doesn't seem like having to (re-)enable async mode is
> something we'd normally expect to do on the ::wait path...
> 
> I only pointed this out as it seems to be a case that doesn't align
> with the claims in your commit message...

Oh, humm, I see (and indeed, I had misread your earlier message).  It is
true that the commit log isn't accurate (and at the least that would
need to be amended).  One thing I don't really understand are the cases
when async is disabled such that resume needs to re-enable it vs are
the cases in resume just trying to catch an edge case with attach or
create_inferior or the like where async mode isn't enabled by some other
hook.

Ah, it seems that in wait_one if a target returns WAITKIND_NO_RESUMED,
we disable async on that target, and then the next time we want to
resume the target we re-enable async after target_resume returns.  The
other places we disable async are either temporary places around
readline, etc. or when when an inferior exits (INF_EXEC_COMPLETE)
passed to inferior_event_handle).

Given that, I do think the code in record_full_wait_1 is ok:

1) Any time ::wait() is invoked, it looks like target_async(1) should
    have already been called by the preceding do_target_resume().

2) The loop in record_full_wait_1() doesn't return to the wait_one()
    loop where target_async(0) is invoked, so async for the underlying
    target should stay enabled for the duration of the ::wait() call.

3) All the places that can invoke INF_EXEC_COMPLETE look to be in the
    gdb core itself which is "above" a target ::wait() method on the
    stack.

-- 
John Baldwin

Re: [PATCH v2 04/13] linux-nat: Don't enable async mode at the end of linux_nat_target::resume.

[Andrew Burgess]

* John Baldwin <jhb@FreeBSD.org> [2021-12-01 12:03:41 -0800]:

> On 12/1/21 4:02 AM, Andrew Burgess wrote:
> > * John Baldwin <jhb@FreeBSD.org> [2021-11-24 08:03:36 -0800]:
> > 
> > > On 11/24/21 5:24 AM, Andrew Burgess wrote:
> > > > * John Baldwin <jhb@FreeBSD.org> [2021-08-03 11:49:51 -0700]:
> > > > 
> > > > > Commit 5b6d1e4fa4f ("Multi-target support") enabled async mode in the caller
> > > > > (do_target_resume) after target::resume returns making this call
> > > > > redundant.
> > > > 
> > > > The only thing I spotted where this might result in a change of
> > > > behaviour is in record-full.c in record_full_wait_1, there's this
> > > > line:
> > > > 
> > > >     ops->beneath ()->resume (ptid, step, GDB_SIGNAL_0);
> > > > 
> > > > Doesn't this mean that if the record target is sitting on top of a
> > > > Linux target, then me could potentially see a change of behaviour
> > > > here?
> > > > 
> > > > I know almost nothing about the record target, so I can't really offer
> > > > any insights into whether the situation about could actually happen or
> > > > not.
> > > > 
> > > > Maybe somebody else will have some thoughts, but at a minimum, it is
> > > > probably worth mentioning that there might be a change for that
> > > > case.... Unless you know for sure that there isn't.
> > > 
> > > Hmm, I was not aware of that case.  This is a change Pedro had suggested
> > > and it didn't show any regressions in the test suite on a Linux/x86-64
> > > VM, but it very well might be that the test suite doesn't cover
> > > record?
> > 
> > I suspect it's not well tested.
> > 
> > > 
> > > Hmm, looking at record_full_target::resume, it enables async right after
> > > calling the beneath method and it also does so before returning, so I
> > > suspect it's explicit call is safe to elide for the same reason as this
> > > patch.  That is, in the code today, linux_nat_target::resume enables async
> > > mode, then would return to remote_full_target::resume which would enable
> > > async mode (without any other statements in between), and then return
> > > to do_target_resume which would enable async mode a third time (again without
> > > any other code in between aside from any RAII destructors when returning
> > > from the target methods).  Given, that I think this should be safe, and
> > > I can add a patch to this series to remove the call from
> > > remote_full_target::resume.
> > 
> > That's all true, but I wasn't commenting on
> > record_full_target::resume, rather record_full_wait_1, which is called
> > from record_full_base_target::wait.
> > 
> > I'd certainly be happy for your change to go in.  Having looked again,
> > I don't think there's going to be any problems, looking at other
> > targets, it doesn't seem like having to (re-)enable async mode is
> > something we'd normally expect to do on the ::wait path...
> > 
> > I only pointed this out as it seems to be a case that doesn't align
> > with the claims in your commit message...
> 
> Oh, humm, I see (and indeed, I had misread your earlier message).  It is
> true that the commit log isn't accurate (and at the least that would
> need to be amended).  One thing I don't really understand are the cases
> when async is disabled such that resume needs to re-enable it vs are
> the cases in resume just trying to catch an edge case with attach or
> create_inferior or the like where async mode isn't enabled by some other
> hook.
> 
> Ah, it seems that in wait_one if a target returns WAITKIND_NO_RESUMED,
> we disable async on that target, and then the next time we want to
> resume the target we re-enable async after target_resume returns.  The
> other places we disable async are either temporary places around
> readline, etc. or when when an inferior exits (INF_EXEC_COMPLETE)
> passed to inferior_event_handle).
> 
> Given that, I do think the code in record_full_wait_1 is ok:
> 
> 1) Any time ::wait() is invoked, it looks like target_async(1) should
>    have already been called by the preceding do_target_resume().
> 
> 2) The loop in record_full_wait_1() doesn't return to the wait_one()
>    loop where target_async(0) is invoked, so async for the underlying
>    target should stay enabled for the duration of the ::wait() call.
> 
> 3) All the places that can invoke INF_EXEC_COMPLETE look to be in the
>    gdb core itself which is "above" a target ::wait() method on the
>    stack.

Thanks for the detailed analysis.  I agree that, to the best of my
understanding, your change is fine.  I think it would be worth adding
this analysis to the commit message, then if there ever is a bug in
the future, we can understand what we thought was going on :)

Thanks again!
Andrew

Re: [PATCH v2 04/13] linux-nat: Don't enable async mode at the end of linux_nat_target::resume.

[John Baldwin]

On 12/2/21 3:19 AM, Andrew Burgess wrote:
> * John Baldwin <jhb@FreeBSD.org> [2021-12-01 12:03:41 -0800]:
> 
>> On 12/1/21 4:02 AM, Andrew Burgess wrote:
>>> * John Baldwin <jhb@FreeBSD.org> [2021-11-24 08:03:36 -0800]:
>>>
>>>> On 11/24/21 5:24 AM, Andrew Burgess wrote:
>>>>> * John Baldwin <jhb@FreeBSD.org> [2021-08-03 11:49:51 -0700]:
>>>>>
>>>>>> Commit 5b6d1e4fa4f ("Multi-target support") enabled async mode in the caller
>>>>>> (do_target_resume) after target::resume returns making this call
>>>>>> redundant.
>>>>>
>>>>> The only thing I spotted where this might result in a change of
>>>>> behaviour is in record-full.c in record_full_wait_1, there's this
>>>>> line:
>>>>>
>>>>>      ops->beneath ()->resume (ptid, step, GDB_SIGNAL_0);
>>>>>
>>>>> Doesn't this mean that if the record target is sitting on top of a
>>>>> Linux target, then me could potentially see a change of behaviour
>>>>> here?
>>>>>
>>>>> I know almost nothing about the record target, so I can't really offer
>>>>> any insights into whether the situation about could actually happen or
>>>>> not.
>>>>>
>>>>> Maybe somebody else will have some thoughts, but at a minimum, it is
>>>>> probably worth mentioning that there might be a change for that
>>>>> case.... Unless you know for sure that there isn't.
>>>>
>>>> Hmm, I was not aware of that case.  This is a change Pedro had suggested
>>>> and it didn't show any regressions in the test suite on a Linux/x86-64
>>>> VM, but it very well might be that the test suite doesn't cover
>>>> record?
>>>
>>> I suspect it's not well tested.
>>>
>>>>
>>>> Hmm, looking at record_full_target::resume, it enables async right after
>>>> calling the beneath method and it also does so before returning, so I
>>>> suspect it's explicit call is safe to elide for the same reason as this
>>>> patch.  That is, in the code today, linux_nat_target::resume enables async
>>>> mode, then would return to remote_full_target::resume which would enable
>>>> async mode (without any other statements in between), and then return
>>>> to do_target_resume which would enable async mode a third time (again without
>>>> any other code in between aside from any RAII destructors when returning
>>>> from the target methods).  Given, that I think this should be safe, and
>>>> I can add a patch to this series to remove the call from
>>>> remote_full_target::resume.
>>>
>>> That's all true, but I wasn't commenting on
>>> record_full_target::resume, rather record_full_wait_1, which is called
>>> from record_full_base_target::wait.
>>>
>>> I'd certainly be happy for your change to go in.  Having looked again,
>>> I don't think there's going to be any problems, looking at other
>>> targets, it doesn't seem like having to (re-)enable async mode is
>>> something we'd normally expect to do on the ::wait path...
>>>
>>> I only pointed this out as it seems to be a case that doesn't align
>>> with the claims in your commit message...
>>
>> Oh, humm, I see (and indeed, I had misread your earlier message).  It is
>> true that the commit log isn't accurate (and at the least that would
>> need to be amended).  One thing I don't really understand are the cases
>> when async is disabled such that resume needs to re-enable it vs are
>> the cases in resume just trying to catch an edge case with attach or
>> create_inferior or the like where async mode isn't enabled by some other
>> hook.
>>
>> Ah, it seems that in wait_one if a target returns WAITKIND_NO_RESUMED,
>> we disable async on that target, and then the next time we want to
>> resume the target we re-enable async after target_resume returns.  The
>> other places we disable async are either temporary places around
>> readline, etc. or when when an inferior exits (INF_EXEC_COMPLETE)
>> passed to inferior_event_handle).
>>
>> Given that, I do think the code in record_full_wait_1 is ok:
>>
>> 1) Any time ::wait() is invoked, it looks like target_async(1) should
>>     have already been called by the preceding do_target_resume().
>>
>> 2) The loop in record_full_wait_1() doesn't return to the wait_one()
>>     loop where target_async(0) is invoked, so async for the underlying
>>     target should stay enabled for the duration of the ::wait() call.
>>
>> 3) All the places that can invoke INF_EXEC_COMPLETE look to be in the
>>     gdb core itself which is "above" a target ::wait() method on the
>>     stack.
> 
> Thanks for the detailed analysis.  I agree that, to the best of my
> understanding, your change is fine.  I think it would be worth adding
> this analysis to the commit message, then if there ever is a bug in
> the future, we can understand what we thought was going on :)

I will include it, and I will merge the commit I had made to remove the
explicit target_async(1) calls in the record targets into this commit
so that the detailed commit log covers all of them.

I noticed one additional call that could perhaps be elided which is the
call to target_async(1) near the end of remote::resume in the remote
target.  Unlike the Linux native and record targets, this call is not
at the very end of the function, but I don't think it will break anything
to run the relevant code before enabling async mode rather than after:

   /* 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
      because the stub wouldn't be ready to process it.  This applies
      only to the base all-stop protocol, however.  In non-stop (which
      only supports vCont), the stub replies with an "OK", and is
      immediate able to process further serial input.  */
   if (!target_is_non_stop_p ())
     rs->waiting_for_stop_reply = 1;

This would also in theory remove the FIXME comment, though I don't know
that this change is fully realizing the goal of that FIXME (it's also
over 20 years old though).  If you think it makes sense to elide the
call in the remote target I can fold this into this commit as well.
I can also just leave the remote target as-is.

-- 
John Baldwin

Re: [PATCH v2 04/13] linux-nat: Don't enable async mode at the end of linux_nat_target::resume.

[Andrew Burgess]

* John Baldwin <jhb@FreeBSD.org> [2021-12-02 17:21:27 -0800]:

> On 12/2/21 3:19 AM, Andrew Burgess wrote:
> > * John Baldwin <jhb@FreeBSD.org> [2021-12-01 12:03:41 -0800]:
> > 
> > > On 12/1/21 4:02 AM, Andrew Burgess wrote:
> > > > * John Baldwin <jhb@FreeBSD.org> [2021-11-24 08:03:36 -0800]:
> > > > 
> > > > > On 11/24/21 5:24 AM, Andrew Burgess wrote:
> > > > > > * John Baldwin <jhb@FreeBSD.org> [2021-08-03 11:49:51 -0700]:
> > > > > > 
> > > > > > > Commit 5b6d1e4fa4f ("Multi-target support") enabled async mode in the caller
> > > > > > > (do_target_resume) after target::resume returns making this call
> > > > > > > redundant.
> > > > > > 
> > > > > > The only thing I spotted where this might result in a change of
> > > > > > behaviour is in record-full.c in record_full_wait_1, there's this
> > > > > > line:
> > > > > > 
> > > > > >      ops->beneath ()->resume (ptid, step, GDB_SIGNAL_0);
> > > > > > 
> > > > > > Doesn't this mean that if the record target is sitting on top of a
> > > > > > Linux target, then me could potentially see a change of behaviour
> > > > > > here?
> > > > > > 
> > > > > > I know almost nothing about the record target, so I can't really offer
> > > > > > any insights into whether the situation about could actually happen or
> > > > > > not.
> > > > > > 
> > > > > > Maybe somebody else will have some thoughts, but at a minimum, it is
> > > > > > probably worth mentioning that there might be a change for that
> > > > > > case.... Unless you know for sure that there isn't.
> > > > > 
> > > > > Hmm, I was not aware of that case.  This is a change Pedro had suggested
> > > > > and it didn't show any regressions in the test suite on a Linux/x86-64
> > > > > VM, but it very well might be that the test suite doesn't cover
> > > > > record?
> > > > 
> > > > I suspect it's not well tested.
> > > > 
> > > > > 
> > > > > Hmm, looking at record_full_target::resume, it enables async right after
> > > > > calling the beneath method and it also does so before returning, so I
> > > > > suspect it's explicit call is safe to elide for the same reason as this
> > > > > patch.  That is, in the code today, linux_nat_target::resume enables async
> > > > > mode, then would return to remote_full_target::resume which would enable
> > > > > async mode (without any other statements in between), and then return
> > > > > to do_target_resume which would enable async mode a third time (again without
> > > > > any other code in between aside from any RAII destructors when returning
> > > > > from the target methods).  Given, that I think this should be safe, and
> > > > > I can add a patch to this series to remove the call from
> > > > > remote_full_target::resume.
> > > > 
> > > > That's all true, but I wasn't commenting on
> > > > record_full_target::resume, rather record_full_wait_1, which is called
> > > > from record_full_base_target::wait.
> > > > 
> > > > I'd certainly be happy for your change to go in.  Having looked again,
> > > > I don't think there's going to be any problems, looking at other
> > > > targets, it doesn't seem like having to (re-)enable async mode is
> > > > something we'd normally expect to do on the ::wait path...
> > > > 
> > > > I only pointed this out as it seems to be a case that doesn't align
> > > > with the claims in your commit message...
> > > 
> > > Oh, humm, I see (and indeed, I had misread your earlier message).  It is
> > > true that the commit log isn't accurate (and at the least that would
> > > need to be amended).  One thing I don't really understand are the cases
> > > when async is disabled such that resume needs to re-enable it vs are
> > > the cases in resume just trying to catch an edge case with attach or
> > > create_inferior or the like where async mode isn't enabled by some other
> > > hook.
> > > 
> > > Ah, it seems that in wait_one if a target returns WAITKIND_NO_RESUMED,
> > > we disable async on that target, and then the next time we want to
> > > resume the target we re-enable async after target_resume returns.  The
> > > other places we disable async are either temporary places around
> > > readline, etc. or when when an inferior exits (INF_EXEC_COMPLETE)
> > > passed to inferior_event_handle).
> > > 
> > > Given that, I do think the code in record_full_wait_1 is ok:
> > > 
> > > 1) Any time ::wait() is invoked, it looks like target_async(1) should
> > >     have already been called by the preceding do_target_resume().
> > > 
> > > 2) The loop in record_full_wait_1() doesn't return to the wait_one()
> > >     loop where target_async(0) is invoked, so async for the underlying
> > >     target should stay enabled for the duration of the ::wait() call.
> > > 
> > > 3) All the places that can invoke INF_EXEC_COMPLETE look to be in the
> > >     gdb core itself which is "above" a target ::wait() method on the
> > >     stack.
> > 
> > Thanks for the detailed analysis.  I agree that, to the best of my
> > understanding, your change is fine.  I think it would be worth adding
> > this analysis to the commit message, then if there ever is a bug in
> > the future, we can understand what we thought was going on :)
> 
> I will include it, and I will merge the commit I had made to remove the
> explicit target_async(1) calls in the record targets into this commit
> so that the detailed commit log covers all of them.
> 
> I noticed one additional call that could perhaps be elided which is the
> call to target_async(1) near the end of remote::resume in the remote
> target.  Unlike the Linux native and record targets, this call is not
> at the very end of the function, but I don't think it will break anything
> to run the relevant code before enabling async mode rather than after:
> 
>   /* 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
>      because the stub wouldn't be ready to process it.  This applies
>      only to the base all-stop protocol, however.  In non-stop (which
>      only supports vCont), the stub replies with an "OK", and is
>      immediate able to process further serial input.  */
>   if (!target_is_non_stop_p ())
>     rs->waiting_for_stop_reply = 1;
> 
> This would also in theory remove the FIXME comment, though I don't know
> that this change is fully realizing the goal of that FIXME (it's also
> over 20 years old though).  If you think it makes sense to elide the
> call in the remote target I can fold this into this commit as well.
> I can also just leave the remote target as-is.

I agree that this call can be safely removed.

Thanks,
Andrew

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

[John Baldwin]

On 12/1/21 4:04 AM, Andrew Burgess wrote:
> * John Baldwin <jhb@FreeBSD.org> [2021-11-25 08:38:16 -0800]:
> 
>> On 11/25/21 2:30 AM, Andrew Burgess wrote:
>>> * John Baldwin <jhb@FreeBSD.org> [2021-11-24 14:00:16 -0800]:
>>>
>>>> On 11/24/21 12:16 PM, Simon Marchi wrote:
>>>>> On 2021-11-24 9:16 a.m., Andrew Burgess via Gdb-patches wrote:
>>>>>> * John Baldwin <jhb@FreeBSD.org> [2021-08-03 11:49:53 -0700]:
>>>>>>
>>>>>>> Previously this returned a TARGET_WAITKIND_SIGNALLED event for
>>>>>>> inferior_ptid.  Since the multi-target changes, inferior_ptid is now
>>>>>>> invalid during ::wait() methods, so this triggered an assertion
>>>>>>> further up the stack.
>>>>>>> ---
>>>>>>>     gdb/inf-ptrace.c | 11 +++--------
>>>>>>>     1 file changed, 3 insertions(+), 8 deletions(-)
>>>>>>>
>>>>>>> diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
>>>>>>> index afa38de6ef7..1f8e72d1aca 100644
>>>>>>> --- a/gdb/inf-ptrace.c
>>>>>>> +++ b/gdb/inf-ptrace.c
>>>>>>> @@ -319,14 +319,9 @@ 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->kind = TARGET_WAITKIND_SIGNALLED;
>>>>>>> -	  ourstatus->value.sig = GDB_SIGNAL_UNKNOWN;
>>>>>>> -	  return inferior_ptid;
>>>>>>> +	  internal_error (__FILE__, __LINE__,
>>>>>>> +			  _("Child process unexpectedly missing: %s.\n"),
>>>>>>> +			  safe_strerror (save_errno));
>>>>>>>     	}
>>>>>>
>>>>>> Single statement if blocks should not have '{ ... }' around them.
>>>>>>
>>>>>> I wonder if we could use perror_with_name here instead of
>>>>>> internal_error, there's at least one place this is done in
>>>>>> linux-nat.c.
>>>>>
>>>>> If we are being pedantic about the uses of internal_error vs throwing an
>>>>> error: factors external to GDB should not be able to cause an internal
>>>>> error.  The kernel is an external factor: a bug in the kernel could
>>>>> cause this branch to be taken, so it should probably not make GDB
>>>>> internal error.  In that regard, perror_with_name would be better.
>>>>> Although I would be fine with assuming that the kernel does not have
>>>>> bugs (except when actively working around known bugs).
>>>>>
>>>>> Otherwise, I would suggest using gdb_assert_not_reached instead of
>>>>> internal_error, to achieve the same thing.
>>>>
>>>> I think treating this as a kernel bug (and thus perror_with_name) is fine.
>>>
>>> I don't think that perror_with_name implies this is a kernel bug.  The
>>> perror function is just a wrapper around 'error' and safe_strerror -
>>> so basically what you have already except using error, not
>>> internal_error.
>>>
>>> Though I guess if we get -1 back then this implies something has gone
>>> wrong somewhere, right?  We expect to see some event for the child no
>>> matter how the kernel kills it, so, if we get -1, that implies the
>>> child disappeared without a trace, right?  But possibly the bug could
>>> be that we already collected the event, and lost it somehow, so it
>>> could be a GDB bug...
>>
>> Yes, it could be either one.  I had chosen internal_error to replace the
>> existing fprintf somewhat arbitrarily.  I don't have a strong opinion
>> on what type of error it should be, only that it is best to throw the
>> error closer to the real problem than to get a later assertion
>> failure.
> 
> For this sort of situation we should assume GDB doesn't have bugs, and
> that this situation must be caused by an external problem, so
> `error`.  If we later find there are bugs in GDB, we'd then fix them.

To be clear, is perror_with_name what you'd prefer here or explicitly
using error()?

-- 
John Baldwin

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

[Andrew Burgess]

* John Baldwin <jhb@FreeBSD.org> [2021-12-03 10:12:15 -0800]:

> On 12/1/21 4:04 AM, Andrew Burgess wrote:
> > * John Baldwin <jhb@FreeBSD.org> [2021-11-25 08:38:16 -0800]:
> > 
> > > On 11/25/21 2:30 AM, Andrew Burgess wrote:
> > > > * John Baldwin <jhb@FreeBSD.org> [2021-11-24 14:00:16 -0800]:
> > > > 
> > > > > On 11/24/21 12:16 PM, Simon Marchi wrote:
> > > > > > On 2021-11-24 9:16 a.m., Andrew Burgess via Gdb-patches wrote:
> > > > > > > * John Baldwin <jhb@FreeBSD.org> [2021-08-03 11:49:53 -0700]:
> > > > > > > 
> > > > > > > > Previously this returned a TARGET_WAITKIND_SIGNALLED event for
> > > > > > > > inferior_ptid.  Since the multi-target changes, inferior_ptid is now
> > > > > > > > invalid during ::wait() methods, so this triggered an assertion
> > > > > > > > further up the stack.
> > > > > > > > ---
> > > > > > > >     gdb/inf-ptrace.c | 11 +++--------
> > > > > > > >     1 file changed, 3 insertions(+), 8 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
> > > > > > > > index afa38de6ef7..1f8e72d1aca 100644
> > > > > > > > --- a/gdb/inf-ptrace.c
> > > > > > > > +++ b/gdb/inf-ptrace.c
> > > > > > > > @@ -319,14 +319,9 @@ 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->kind = TARGET_WAITKIND_SIGNALLED;
> > > > > > > > -	  ourstatus->value.sig = GDB_SIGNAL_UNKNOWN;
> > > > > > > > -	  return inferior_ptid;
> > > > > > > > +	  internal_error (__FILE__, __LINE__,
> > > > > > > > +			  _("Child process unexpectedly missing: %s.\n"),
> > > > > > > > +			  safe_strerror (save_errno));
> > > > > > > >     	}
> > > > > > > 
> > > > > > > Single statement if blocks should not have '{ ... }' around them.
> > > > > > > 
> > > > > > > I wonder if we could use perror_with_name here instead of
> > > > > > > internal_error, there's at least one place this is done in
> > > > > > > linux-nat.c.
> > > > > > 
> > > > > > If we are being pedantic about the uses of internal_error vs throwing an
> > > > > > error: factors external to GDB should not be able to cause an internal
> > > > > > error.  The kernel is an external factor: a bug in the kernel could
> > > > > > cause this branch to be taken, so it should probably not make GDB
> > > > > > internal error.  In that regard, perror_with_name would be better.
> > > > > > Although I would be fine with assuming that the kernel does not have
> > > > > > bugs (except when actively working around known bugs).
> > > > > > 
> > > > > > Otherwise, I would suggest using gdb_assert_not_reached instead of
> > > > > > internal_error, to achieve the same thing.
> > > > > 
> > > > > I think treating this as a kernel bug (and thus perror_with_name) is fine.
> > > > 
> > > > I don't think that perror_with_name implies this is a kernel bug.  The
> > > > perror function is just a wrapper around 'error' and safe_strerror -
> > > > so basically what you have already except using error, not
> > > > internal_error.
> > > > 
> > > > Though I guess if we get -1 back then this implies something has gone
> > > > wrong somewhere, right?  We expect to see some event for the child no
> > > > matter how the kernel kills it, so, if we get -1, that implies the
> > > > child disappeared without a trace, right?  But possibly the bug could
> > > > be that we already collected the event, and lost it somehow, so it
> > > > could be a GDB bug...
> > > 
> > > Yes, it could be either one.  I had chosen internal_error to replace the
> > > existing fprintf somewhat arbitrarily.  I don't have a strong opinion
> > > on what type of error it should be, only that it is best to throw the
> > > error closer to the real problem than to get a later assertion
> > > failure.
> > 
> > For this sort of situation we should assume GDB doesn't have bugs, and
> > that this situation must be caused by an external problem, so
> > `error`.  If we later find there are bugs in GDB, we'd then fix them.
> 
> To be clear, is perror_with_name what you'd prefer here or explicitly
> using error()?

perror_with_name is my preference.

Sorry for not being clear.

Thanks,
Andrew

Re: [PATCH v2 04/13] linux-nat: Don't enable async mode at the end of linux_nat_target::resume.

[Tom Tromey]

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

John>   /* FIXME: ezannoni 1999-09-28: We may need to move this out of here
John>      into infcmd.c in order to allow inferior function calls to work
John>      NOT asynchronously.  */
John>   if (target_can_async_p ())
John>     target_async (1);

John> This would also in theory remove the FIXME comment, though I don't know
John> that this change is fully realizing the goal of that FIXME (it's also
John> over 20 years old though).

I don't really understand this comment.  My mental model is that target
async is an implementation detail of gdb, so it seems like if this is
needed, it must be to compensate for some shortcoming somewhere else.
If that's true, then I'd rather the long term direction be to only have
target async, and eventually remove sync entirely; in which case we
could drop this comment.

Tom