[RFC PATCH 0/7] FreeBSD target async mode and related refactoring

[RFC PATCH 0/7] FreeBSD target async mode and related refactoring

[John Baldwin]

I hacked on this over the weekend.  For the testsuite on FreeBSD/amd64
it doesn't seem to add any new regressions and fixes various tests.
Patch 7 fixes a few more failures I noticed when comparing the
before/after logs.

This is an RFC as I have a few questions / thoughts I'd like some
feedback on before refining this further:

1) I haven't yet tested this on Linux and will need to do so to make
   sure the initial event_pipe refactoring doesn't cause regressions.

2) gdbsupport/event-pipe.cc probably doesn't build on Windows
   (gdb/ser-event.c doesn't use pipes on Windows which I didn't
   find until after I'd written this patch).  I think I'd like to only
   build event-pipe.cc for systems with HAVE_PIPE or HAVE_PIPE2.
   I just don't know how to express this in Makefile.am.

   I could potentially add a follow-up patch to change gdb/ser-events.c
   to use event_pipe rather than duplicating very similar logic for
   the non-Windows case.  The one difference I noticed so far is
   that the existing Linux event pipe drains any existing input
   in the "mark" method before writing the new character, but the
   ser-events version does not do the drain.

   An alternative perhaps is that instead of adding event_pipe to
   gdbsupport, I could add a slightly more abstract interface that is
   more like ser_events.c but at the lower level API where the
   event_fd / mark / post methods used an Event on Windows.  I'm not
   sure this is really needed though as if Windows wanted to use an
   event in the future for async mode it would be in Windows-specific
   code and could just use an Event directly.

3) I was surprised by the need to enable async mode explicitly in
   target::attach.  I had assumed that the policy of async vs
   non-async would have belonged in the core and that the core would
   have enabled async if it wanted during attach.  Similarly, I would
   expect do_target_wait_1 in infrun.c to check target_is_async_p
   rather than target_can_async_p for deciding whether to clear
   TARGET_WNOHANG.

4) It may be that some of this can be pulled into inf-ptrace to
   simplify adding async support on other platforms.  For example,
   inf-ptrace.c could export a 'ptrace_event_pipe' and provide
   implementations of is_async_p and async_waitfd that used that along
   with possibly some class helper methods to wrap the flush/mark
   methods to mostly hide ptrace_event_pipe (though the sigchld
   handlers probably still want to access it directly).  This would
   mean that the linux_nat_target::close and fbsd_nat_target::close
   methods could collapse back up into inf_ptrace_target along with
   ::is_async_p and ::async_waitfd.  Targets on other platforms
   would opt-in by providing ::can_async_p.

5) I don't understand why linux_nat_target::resume enables async
   mode explicitly similar to ::attach.  I don't think I need it
   in the FreeBSD target, but I'd feel better about that if I knew
   why the Linux target needed it.

John Baldwin (7):
  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.
  fbsd-nat: Implement async target support.
  fbsd nat: Include ptrace operation in error messages.
  fbsd nat: Switch function entry debug message from lwp to nat.
  fbsd nat: Return NULL rather than failing thread_alive.

 gdb/ChangeLog            |  49 ++++++++++
 gdb/fbsd-nat.c           | 202 +++++++++++++++++++++++++++++++++++----
 gdb/fbsd-nat.h           |  13 +++
 gdb/inf-ptrace.c         |  27 +++++-
 gdb/linux-nat.c          |  48 +++-------
 gdbserver/ChangeLog      |   8 ++
 gdbserver/linux-low.cc   |  39 ++------
 gdbsupport/ChangeLog     |   7 ++
 gdbsupport/Makefile.am   |   1 +
 gdbsupport/Makefile.in   |  21 ++--
 gdbsupport/event-pipe.cc | 100 +++++++++++++++++++
 gdbsupport/event-pipe.h  |  55 +++++++++++
 12 files changed, 470 insertions(+), 100 deletions(-)
 create mode 100644 gdbsupport/event-pipe.cc
 create mode 100644 gdbsupport/event-pipe.h

-- 
2.31.1

[RFC PATCH 1/7] 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).

gdbsupport/ChangeLog:

	* Makefile.in: Rebuild.
	* Makefile.am (libgdbsupport_a_SOURCES): Add event-pipe.cc.
	* event-pipe.h: New file.
	* event-pipe.cc: New file.
---
 gdbsupport/ChangeLog     |   7 +++
 gdbsupport/Makefile.am   |   1 +
 gdbsupport/Makefile.in   |  21 ++++----
 gdbsupport/event-pipe.cc | 100 +++++++++++++++++++++++++++++++++++++++
 gdbsupport/event-pipe.h  |  55 +++++++++++++++++++++
 5 files changed, 175 insertions(+), 9 deletions(-)
 create mode 100644 gdbsupport/event-pipe.cc
 create mode 100644 gdbsupport/event-pipe.h

diff --git a/gdbsupport/ChangeLog b/gdbsupport/ChangeLog
index b790a97230..c953d87fbf 100644
--- a/gdbsupport/ChangeLog
+++ b/gdbsupport/ChangeLog
@@ -1,3 +1,10 @@
+2021-06-04  John Baldwin  <jhb@FreeBSD.org>
+
+	* Makefile.in: Rebuild.
+	* Makefile.am (libgdbsupport_a_SOURCES): Add event-pipe.cc.
+	* event-pipe.h: New file.
+	* event-pipe.cc: New file.
+
 2021-05-17  Andrew Burgess  <andrew.burgess@embecosm.com>
 
 	* .dir-locals.el: Set sentence-end-double-space for all modes, and
diff --git a/gdbsupport/Makefile.am b/gdbsupport/Makefile.am
index 6d4678c8c9..a7d78f8c26 100644
--- a/gdbsupport/Makefile.am
+++ b/gdbsupport/Makefile.am
@@ -48,6 +48,7 @@ libgdbsupport_a_SOURCES = \
     environ.cc \
     errors.cc \
     event-loop.cc \
+    event-pipe.cc \
     fileio.cc \
     filestuff.cc \
     format.cc \
diff --git a/gdbsupport/Makefile.in b/gdbsupport/Makefile.in
index d7f2d4914b..7ff5531998 100644
--- a/gdbsupport/Makefile.in
+++ b/gdbsupport/Makefile.in
@@ -150,15 +150,16 @@ am_libgdbsupport_a_OBJECTS = agent.$(OBJEXT) btrace-common.$(OBJEXT) \
 	common-exceptions.$(OBJEXT) common-inferior.$(OBJEXT) \
 	common-regcache.$(OBJEXT) common-utils.$(OBJEXT) \
 	environ.$(OBJEXT) errors.$(OBJEXT) event-loop.$(OBJEXT) \
-	fileio.$(OBJEXT) filestuff.$(OBJEXT) format.$(OBJEXT) \
-	gdb-dlfcn.$(OBJEXT) gdb_tilde_expand.$(OBJEXT) \
-	gdb_wait.$(OBJEXT) gdb_vecs.$(OBJEXT) job-control.$(OBJEXT) \
-	netstuff.$(OBJEXT) new-op.$(OBJEXT) pathstuff.$(OBJEXT) \
-	print-utils.$(OBJEXT) ptid.$(OBJEXT) rsp-low.$(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)
+	event-pipe.$(OBJEXT) fileio.$(OBJEXT) filestuff.$(OBJEXT) \
+	format.$(OBJEXT) gdb-dlfcn.$(OBJEXT) \
+	gdb_tilde_expand.$(OBJEXT) gdb_wait.$(OBJEXT) \
+	gdb_vecs.$(OBJEXT) job-control.$(OBJEXT) netstuff.$(OBJEXT) \
+	new-op.$(OBJEXT) pathstuff.$(OBJEXT) print-utils.$(OBJEXT) \
+	ptid.$(OBJEXT) rsp-low.$(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)
 libgdbsupport_a_OBJECTS = $(am_libgdbsupport_a_OBJECTS)
 AM_V_P = $(am__v_P_@AM_V@)
 am__v_P_ = $(am__v_P_@AM_DEFAULT_V@)
@@ -371,6 +372,7 @@ libgdbsupport_a_SOURCES = \
     environ.cc \
     errors.cc \
     event-loop.cc \
+    event-pipe.cc \
     fileio.cc \
     filestuff.cc \
     format.cc \
@@ -476,6 +478,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/event-pipe.cc b/gdbsupport/event-pipe.cc
new file mode 100644
index 0000000000..b701be0b5a
--- /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 (active ())
+    close ();
+}
+
+/* Create a new pipe.  */
+
+bool
+event_pipe::open ()
+{
+  if (fds[0] != -1)
+    return false;
+
+  if (gdb_pipe_cloexec (fds) == -1)
+    return false;
+
+  if (fcntl (fds[0], F_SETFL, O_NONBLOCK) == -1 ||
+      fcntl (fds[1], F_SETFL, O_NONBLOCK) == -1) {
+    close ();
+    return false;
+  }
+
+  return true;
+}
+
+void
+event_pipe::close ()
+{
+  ::close (fds[0]);
+  ::close (fds[1]);
+  fds[0] = -1;
+  fds[1] = -1;
+}
+
+/* Get rid of any pending events in the pipe.  */
+
+void
+event_pipe::flush ()
+{
+  int ret;
+  char buf;
+
+  do
+    {
+      ret = read (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 (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 0000000000..1717ea6790
--- /dev/null
+++ b/gdbsupport/event-pipe.h
@@ -0,0 +1,55 @@
+/* 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 not associated with a file descriptor.  */
+
+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 initialized.  */
+  bool active () const { return fds[0] != -1; }
+
+  /* The file descriptor of the waitable file to use in the event
+     loop.  */
+  int event_fd () const { return fds[0]; }
+
+  /* Flush the event pipe.  */
+  void flush ();
+
+  /* Put something in the pipe, so the event loop wakes up.  */
+  void mark ();
+private:
+  int fds[2] = { -1, -1 };
+};
+
+#endif /* COMMON_EVENT_PIPE_H */
+
-- 
2.31.1

Re: [RFC PATCH 1/7] gdbsupport: Add an event-pipe class.

[Lancelot SIX]

Hi,

I have just few style related remarks.

On Mon, Jun 07, 2021 at 10:09:26AM -0700, John Baldwin wrote:
> 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).
> 
> gdbsupport/ChangeLog:
> 
> 	* Makefile.in: Rebuild.
> 	* Makefile.am (libgdbsupport_a_SOURCES): Add event-pipe.cc.
> 	* event-pipe.h: New file.
> 	* event-pipe.cc: New file.
> ---
>  gdbsupport/ChangeLog     |   7 +++
>  gdbsupport/Makefile.am   |   1 +
>  gdbsupport/Makefile.in   |  21 ++++----
>  gdbsupport/event-pipe.cc | 100 +++++++++++++++++++++++++++++++++++++++
>  gdbsupport/event-pipe.h  |  55 +++++++++++++++++++++
>  5 files changed, 175 insertions(+), 9 deletions(-)
>  create mode 100644 gdbsupport/event-pipe.cc
>  create mode 100644 gdbsupport/event-pipe.h
> 
> diff --git a/gdbsupport/ChangeLog b/gdbsupport/ChangeLog
> index b790a97230..c953d87fbf 100644
> --- a/gdbsupport/ChangeLog
> +++ b/gdbsupport/ChangeLog
> @@ -1,3 +1,10 @@
> +2021-06-04  John Baldwin  <jhb@FreeBSD.org>
> +
> +	* Makefile.in: Rebuild.
> +	* Makefile.am (libgdbsupport_a_SOURCES): Add event-pipe.cc.
> +	* event-pipe.h: New file.
> +	* event-pipe.cc: New file.
> +
>  2021-05-17  Andrew Burgess  <andrew.burgess@embecosm.com>
>  
>  	* .dir-locals.el: Set sentence-end-double-space for all modes, and
> diff --git a/gdbsupport/Makefile.am b/gdbsupport/Makefile.am
> index 6d4678c8c9..a7d78f8c26 100644
> --- a/gdbsupport/Makefile.am
> +++ b/gdbsupport/Makefile.am
> @@ -48,6 +48,7 @@ libgdbsupport_a_SOURCES = \
>      environ.cc \
>      errors.cc \
>      event-loop.cc \
> +    event-pipe.cc \
>      fileio.cc \
>      filestuff.cc \
>      format.cc \
> diff --git a/gdbsupport/Makefile.in b/gdbsupport/Makefile.in
> index d7f2d4914b..7ff5531998 100644
> --- a/gdbsupport/Makefile.in
> +++ b/gdbsupport/Makefile.in
> @@ -150,15 +150,16 @@ am_libgdbsupport_a_OBJECTS = agent.$(OBJEXT) btrace-common.$(OBJEXT) \
>  	common-exceptions.$(OBJEXT) common-inferior.$(OBJEXT) \
>  	common-regcache.$(OBJEXT) common-utils.$(OBJEXT) \
>  	environ.$(OBJEXT) errors.$(OBJEXT) event-loop.$(OBJEXT) \
> -	fileio.$(OBJEXT) filestuff.$(OBJEXT) format.$(OBJEXT) \
> -	gdb-dlfcn.$(OBJEXT) gdb_tilde_expand.$(OBJEXT) \
> -	gdb_wait.$(OBJEXT) gdb_vecs.$(OBJEXT) job-control.$(OBJEXT) \
> -	netstuff.$(OBJEXT) new-op.$(OBJEXT) pathstuff.$(OBJEXT) \
> -	print-utils.$(OBJEXT) ptid.$(OBJEXT) rsp-low.$(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)
> +	event-pipe.$(OBJEXT) fileio.$(OBJEXT) filestuff.$(OBJEXT) \
> +	format.$(OBJEXT) gdb-dlfcn.$(OBJEXT) \
> +	gdb_tilde_expand.$(OBJEXT) gdb_wait.$(OBJEXT) \
> +	gdb_vecs.$(OBJEXT) job-control.$(OBJEXT) netstuff.$(OBJEXT) \
> +	new-op.$(OBJEXT) pathstuff.$(OBJEXT) print-utils.$(OBJEXT) \
> +	ptid.$(OBJEXT) rsp-low.$(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)
>  libgdbsupport_a_OBJECTS = $(am_libgdbsupport_a_OBJECTS)
>  AM_V_P = $(am__v_P_@AM_V@)
>  am__v_P_ = $(am__v_P_@AM_DEFAULT_V@)
> @@ -371,6 +372,7 @@ libgdbsupport_a_SOURCES = \
>      environ.cc \
>      errors.cc \
>      event-loop.cc \
> +    event-pipe.cc \
>      fileio.cc \
>      filestuff.cc \
>      format.cc \
> @@ -476,6 +478,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/event-pipe.cc b/gdbsupport/event-pipe.cc
> new file mode 100644
> index 0000000000..b701be0b5a
> --- /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 (active ())
> +    close ();
> +}
> +
> +/* Create a new pipe.  */
> +
> +bool
> +event_pipe::open ()
> +{
> +  if (fds[0] != -1)
> +    return false;
> +
> +  if (gdb_pipe_cloexec (fds) == -1)
> +    return false;
> +
> +  if (fcntl (fds[0], F_SETFL, O_NONBLOCK) == -1 ||
> +      fcntl (fds[1], F_SETFL, O_NONBLOCK) == -1) {
> +    close ();
> +    return false;
> +  }
> +

According to coding standards, this should be written more like:

    if (fcntl (fds[0], F_SETFL, O_NONBLOCK) == -1
        || fcntl (fds[1], F_SETFL, O_NONBLOCK) == -1)
      {
        close ();
        return false;
      }

> +  return true;
> +}
> +
> +void
> +event_pipe::close ()
> +{
> +  ::close (fds[0]);
> +  ::close (fds[1]);
> +  fds[0] = -1;
> +  fds[1] = -1;
> +}
> +
> +/* Get rid of any pending events in the pipe.  */
> +
> +void
> +event_pipe::flush ()
> +{
> +  int ret;
> +  char buf;
> +
> +  do
> +    {
> +      ret = read (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 (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 0000000000..1717ea6790
> --- /dev/null
> +++ b/gdbsupport/event-pipe.h
> @@ -0,0 +1,55 @@
> +/* 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 not associated with a file descriptor.  */
> +
> +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 initialized.  */
> +  bool active () const { return fds[0] != -1; }
> +
> +  /* The file descriptor of the waitable file to use in the event
> +     loop.  */
> +  int event_fd () const { return fds[0]; }
> +
> +  /* Flush the event pipe.  */
> +  void flush ();
> +
> +  /* Put something in the pipe, so the event loop wakes up.  */
> +  void mark ();
> +private:
> +  int fds[2] = { -1, -1 };

Member names are usually prefixed with `m_`, so I guess this could be
renamed `m_fds`.

Best,
Lancelot.

> +};
> +
> +#endif /* COMMON_EVENT_PIPE_H */
> +
> -- 
> 2.31.1
> 

Re: [RFC PATCH 1/7] gdbsupport: Add an event-pipe class.

[Pedro Alves]

On 2021-06-07 6:09 p.m., John Baldwin wrote:
> 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).
> 

Seems like a good idea, thanks.

> +  /* Create a new pipe.  */
> +  bool open ();
> +
> +  /* Close the pipe.  */
> +  void close ();
> +
> +  /* True if the event pipe has been initialized.  */
> +  bool active () const { return fds[0] != -1; }
> +

Did you consider "is_open" instead of "active"?  I think reading "active()" at the call
sites can make one wonder whether "active" is a different state from being
open, like e.g., the pipe is open but not registered in the event loop.

> diff --git a/gdbsupport/event-pipe.h b/gdbsupport/event-pipe.h
> new file mode 100644
> index 0000000000..1717ea6790
> --- /dev/null
> +++ b/gdbsupport/event-pipe.h
> @@ -0,0 +1,55 @@
> +/* 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 not associated with a file descriptor.  */

Note this comment can be a little bit unclear given we also have
create_async_event_handler / mark_async_event_handler, for events not associated
with a file descriptor.

We need to use a pipe iff we need to wake up the select/poll in the event loop
from a signal handler, otherwise we can use the cheaper mark_async_event_handler
from mainline code.

Maybe mention in the comment that this is used for implementing the well-known
self-pipe trick?

Re: [RFC PATCH 1/7] gdbsupport: Add an event-pipe class.

[John Baldwin]

On 6/27/21 9:12 AM, Pedro Alves wrote:
> On 2021-06-07 6:09 p.m., John Baldwin wrote:
>> 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).
>>
> 
> Seems like a good idea, thanks.
> 
>> +  /* Create a new pipe.  */
>> +  bool open ();
>> +
>> +  /* Close the pipe.  */
>> +  void close ();
>> +
>> +  /* True if the event pipe has been initialized.  */
>> +  bool active () const { return fds[0] != -1; }
>> +
> 
> Did you consider "is_open" instead of "active"?  I think reading "active()" at the call
> sites can make one wonder whether "active" is a different state from being
> open, like e.g., the pipe is open but not registered in the event loop.

Hmmm, I hadn't considered is_open.  I think was just going off the names of the existing
macros in the Linux targets, but I prefer is_open and will switch to that.

>> diff --git a/gdbsupport/event-pipe.h b/gdbsupport/event-pipe.h
>> new file mode 100644
>> index 0000000000..1717ea6790
>> --- /dev/null
>> +++ b/gdbsupport/event-pipe.h
>> @@ -0,0 +1,55 @@
>> +/* 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 not associated with a file descriptor.  */
> 
> Note this comment can be a little bit unclear given we also have
> create_async_event_handler / mark_async_event_handler, for events not associated
> with a file descriptor.
> 
> We need to use a pipe iff we need to wake up the select/poll in the event loop
> from a signal handler, otherwise we can use the cheaper mark_async_event_handler
> from mainline code.
> 
> Maybe mention in the comment that this is used for implementing the well-known
> self-pipe trick?

Yeah, I wanted to find a way to mention that anyway, and these are good points.
Here is what I came up with:

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

-- 
John Baldwin

Re: [RFC PATCH 1/7] gdbsupport: Add an event-pipe class.

[Pedro Alves]

On 2021-07-12 5:07 p.m., John Baldwin wrote:
> On 6/27/21 9:12 AM, Pedro Alves wrote:

>>> +/* An event pipe used as a waitable file in the event loop in place of
>>> +   some other event not associated with a file descriptor.  */
>>
>> Note this comment can be a little bit unclear given we also have
>> create_async_event_handler / mark_async_event_handler, for events not associated
>> with a file descriptor.
>>
>> We need to use a pipe iff we need to wake up the select/poll in the event loop
>> from a signal handler, otherwise we can use the cheaper mark_async_event_handler
>> from mainline code.
>>
>> Maybe mention in the comment that this is used for implementing the well-known
>> self-pipe trick?
> 
> Yeah, I wanted to find a way to mention that anyway, and these are good points.
> Here is what I came up with:
> 
> /* 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.  */
> 

LGTM.

[RFC PATCH 2/7] gdb linux-nat: Convert linux_nat_event_pipe to the event_pipe class.

[John Baldwin]

gdb/ChangeLog:

	* linux-nat.c: Include gdbsupport/event-pipe.h.
	(linux_nat_event_pipe): Convert to an instance of event_pipe.
	(linux_is_async_p, async_file_flush, async_file_mark): Implement
	as methods of event_pipe.
	(sigchld_handler, linux_async_pipe)
	(linux_nat_target::async_wait_fd, linux_nat_target::async): Update
	to use event_pipe methods.
---
 gdb/ChangeLog   | 10 ++++++++++
 gdb/linux-nat.c | 48 +++++++++++-------------------------------------
 2 files changed, 21 insertions(+), 37 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index e646fd53e6..cb301e1e81 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,13 @@
+2021-06-04  John Baldwin  <jhb@FreeBSD.org>
+
+	* linux-nat.c: Include gdbsupport/event-pipe.h.
+	(linux_nat_event_pipe): Convert to an instance of event_pipe.
+	(linux_is_async_p, async_file_flush, async_file_mark): Implement
+	as methods of event_pipe.
+	(sigchld_handler, linux_async_pipe)
+	(linux_nat_target::async_wait_fd, linux_nat_target::async): Update
+	to use event_pipe methods.
+
 2021-06-03  John Baldwin  <jhb@FreeBSD.org>
 
 	* fbsd-tdep.c (FBSD_SI_USER, FBSD_SI_QUEUE, FBSD_SI_TIMER)
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 34a2aee41d..150fdd43f8 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>
@@ -219,24 +220,17 @@ static int report_thread_events;
 
 /* The read/write ends of the pipe registered as waitable file in the
    event loop.  */
-static int linux_nat_event_pipe[2] = { -1, -1 };
+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.active ())
 
 /* 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 +240,7 @@ async_file_flush (void)
 static void
 async_file_mark (void)
 {
-  int ret;
-
-  /* It doesn't really matter what the pipe contains, as long we end
-     up with something in it.  Might as well flush the previous
-     left-overs.  */
-  async_file_flush ();
-
-  do
-    {
-      ret = write (linux_nat_event_pipe[1], "+", 1);
-    }
-  while (ret == -1 && errno == EINTR);
-
-  /* Ignore EAGAIN.  If the pipe is full, the event loop will already
-     be awakened anyway.  */
+  linux_nat_event_pipe.mark ();
 }
 
 static int kill_lwp (int lwpid, int signo);
@@ -4041,7 +4021,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.active ())
     async_file_mark (); /* Let the event loop know that there are
 			   events to handle.  */
 
@@ -4073,19 +4053,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);
@@ -4097,7 +4071,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.  */
@@ -4109,7 +4083,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
@@ -4119,7 +4093,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: [RFC PATCH 2/7] gdb linux-nat: Convert linux_nat_event_pipe to the event_pipe class.

[Pedro Alves]

On 2021-06-07 6:09 p.m., John Baldwin wrote:

> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
> index 34a2aee41d..150fdd43f8 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>
> @@ -219,24 +220,17 @@ static int report_thread_events;
>  
>  /* The read/write ends of the pipe registered as waitable file in the
>     event loop.  */

Should we update the comment above?  (Same in gdbserver.)

> -static int linux_nat_event_pipe[2] = { -1, -1 };
> +static event_pipe linux_nat_event_pipe;


>  /* Put something (anything, doesn't matter what, or how much) in event
> @@ -246,21 +240,7 @@ async_file_flush (void)
>  static void
>  async_file_mark (void)

I guess the intro comment could use a little update to talk about marking
the event pipe instead of talking about writing bytes?

Re: [RFC PATCH 2/7] gdb linux-nat: Convert linux_nat_event_pipe to the event_pipe class.

[John Baldwin]

On 6/27/21 9:12 AM, Pedro Alves wrote:
> On 2021-06-07 6:09 p.m., John Baldwin wrote:
> 
>> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
>> index 34a2aee41d..150fdd43f8 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>
>> @@ -219,24 +220,17 @@ static int report_thread_events;
>>   
>>   /* The read/write ends of the pipe registered as waitable file in the
>>      event loop.  */
> 
> Should we update the comment above?  (Same in gdbserver.)

Yes, fixed.

>> -static int linux_nat_event_pipe[2] = { -1, -1 };
>> +static event_pipe linux_nat_event_pipe;
> 
> 
>>   /* Put something (anything, doesn't matter what, or how much) in event
>> @@ -246,21 +240,7 @@ async_file_flush (void)
>>   static void
>>   async_file_mark (void)
> 
> I guess the intro comment could use a little update to talk about marking
> the event pipe instead of talking about writing bytes?

Yes, I've fixed that too, thanks.

-- 
John Baldwin

[RFC PATCH 3/7] gdbserver linux-low: Convert linux_event_pipe to the event-pipe class.

[John Baldwin]

gdbserver/ChangeLog:

	* 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.
---
 gdbserver/ChangeLog    |  8 ++++++++
 gdbserver/linux-low.cc | 39 +++++++++------------------------------
 2 files changed, 17 insertions(+), 30 deletions(-)

diff --git a/gdbserver/ChangeLog b/gdbserver/ChangeLog
index f8d7fd65fb..843402b26f 100644
--- a/gdbserver/ChangeLog
+++ b/gdbserver/ChangeLog
@@ -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-05-27  Simon Marchi  <simon.marchi@polymtl.ca>
 
 	* Fix some indentation mistakes throughout.
diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
index 5c6191d941..aeb7b9287f 100644
--- a/gdbserver/linux-low.cc
+++ b/gdbserver/linux-low.cc
@@ -21,6 +21,7 @@
 #include "nat/linux-osdata.h"
 #include "gdbsupport/agent.h"
 #include "tdesc.h"
+#include "gdbsupport/event-loop.h"
 #include "gdbsupport/rsp-low.h"
 #include "gdbsupport/signals-state-save-restore.h"
 #include "nat/linux-nat.h"
@@ -310,10 +311,10 @@ lwp_in_step_range (struct lwp_info *lwp)
 
 /* The read/write ends of the pipe registered as waitable file in the
    event loop.  */
-static int linux_event_pipe[2] = { -1, -1 };
+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.active ())
 
 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

[RFC PATCH 4/7] 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.  WNOHANG is
passed to waitpid when TARGET_WNOHANG is passed to the
inf_ptrace::wait.

gdb/ChangeLog:

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

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index cb301e1e81..fba798a63d 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,25 @@
+2021-06-04  John Baldwin  <jhb@FreeBSD.org>
+
+	* fbsd-nat.c: Include gdbsupport/event-loop.h,
+	gdbsupport/event-pipe.h, gdbsupport/gdb-sigmask.h, and inf-loop.h.
+	(fbsd_nat_event_pipe, fbsd_nat_target::can_async_p)
+	(fbsd_nat_target::is_async_p, fbsd_nat_target::async_wait_fd)
+	(sigchld_handler, handle_target_event, fbsd_nat_target::async)
+	(fbsd_nat_target::close, fbsd_nat_target::attach): New.
+	(fbsd_nat_target::wait): Rename to ...
+	(fbsd_nat_target::wait_1): ... this.
+	(fbsd_nat_target::wait): New.
+	(fbsd_nat_target::follow_fork): Trigger the async event pipe when
+	adding a vfork_done event.
+	(_initialize_fbsd_nat): Install SIGCHLD handler.
+	* fbsd-nat.h (fbsd_nat_target::can_async_p)
+	(fbsd_nat_target::is_async_p, fbsd_nat_target::async_wait_fd)
+	(fbsd_nat_target::async, fbsd_nat_target::close)
+	(fbsd_nat_target::attach, fbsd_nat_target::wait_1): New.
+	* inf-ptrace.c (inf_ptrace_target::wait): Pass WNOHANG when
+	TARGET_WNOHANG is set.  Handle waitpid return of 0 and ECHILD
+	errors.
+
 2021-06-04  John Baldwin  <jhb@FreeBSD.org>
 
 	* linux-nat.c: Include gdbsupport/event-pipe.h.
diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
index 581c04d5f8..e5128266b8 100644
--- a/gdb/fbsd-nat.c
+++ b/gdb/fbsd-nat.c
@@ -19,14 +19,18 @@
 
 #include "defs.h"
 #include "gdbsupport/byte-vector.h"
+#include "gdbsupport/event-loop.h"
+#include "gdbsupport/event-pipe.h"
 #include "gdbcore.h"
 #include "inferior.h"
 #include "regcache.h"
 #include "regset.h"
 #include "gdbarch.h"
 #include "gdbcmd.h"
+#include "gdbsupport/gdb-sigmask.h"
 #include "gdbthread.h"
 #include "gdbsupport/gdb_wait.h"
+#include "inf-loop.h"
 #include "inf-ptrace.h"
 #include <sys/types.h>
 #include <sys/procfs.h>
@@ -922,6 +926,124 @@ fbsd_nat_target::update_thread_list ()
 #endif
 }
 
+/* Async mode support.  */
+
+static event_pipe fbsd_nat_event_pipe;
+
+/* Implement the "can_async_p" target method.  */
+
+bool
+fbsd_nat_target::can_async_p ()
+{
+  /* Use async unless the user explicitly prevented it via the "maint
+     set target-async" command.  */
+  return target_async_permitted;
+}
+
+/* Implement the "is_async_p" target method.  */
+
+bool
+fbsd_nat_target::is_async_p ()
+{
+  return fbsd_nat_event_pipe.active ();
+}
+
+/* Implement the "async_wait_fd" target method.  */
+
+int
+fbsd_nat_target::async_wait_fd ()
+{
+  return fbsd_nat_event_pipe.event_fd ();
+}
+
+/* SIGCHLD handler notifies the event-loop in async mode.  */
+
+static void
+sigchld_handler (int signo)
+{
+  int old_errno = errno;
+
+  if (fbsd_nat_event_pipe.active ())
+    fbsd_nat_event_pipe.mark ();
+
+  errno = old_errno;
+}
+
+/* Callback registered with the target events file descriptor.  */
+
+static void
+handle_target_event (int error, gdb_client_data client_data)
+{
+  inferior_event_handler (INF_REG_EVENT);
+}
+
+/* Implement the "async" target method.  */
+
+void
+fbsd_nat_target::async (int enable)
+{
+  if (enable != 0 == is_async_p ())
+    return;
+
+  /* Block SIGCHILD while we create/destroy the pipe, as the handler
+     writes to it.  */
+
+  sigset_t chld_mask, prev_mask;
+  sigemptyset (&chld_mask);
+  sigaddset (&chld_mask, SIGCHLD);
+  gdb_sigmask (SIG_BLOCK, &chld_mask, &prev_mask);
+
+  if (enable)
+    {
+      if (!fbsd_nat_event_pipe.open ())
+	internal_error (__FILE__, __LINE__, "failed to create event pipe.");
+
+      add_file_handler (fbsd_nat_event_pipe.event_fd (),
+			handle_target_event, NULL, "fbsd-nat");
+
+      /* Trigger a poll in case there are pending events to
+	 handle.  */
+      fbsd_nat_event_pipe.mark ();
+    }
+  else
+    {
+      delete_file_handler (fbsd_nat_event_pipe.event_fd ());
+      fbsd_nat_event_pipe.close ();
+    }
+
+  gdb_sigmask (SIG_SETMASK, &prev_mask, NULL);
+}
+
+/* Implement the "close" target method.  */
+
+void
+fbsd_nat_target::close ()
+{
+  if (is_async_p ())
+    async (0);
+
+  inf_ptrace_target::close ();
+}
+
+/* Implement the "attach" target method.  */
+
+void
+fbsd_nat_target::attach (const char *args, int from_tty)
+{
+  inf_ptrace_target::attach (args, from_tty);
+
+  /*
+   * Curiously, the core does not do this automatically, instead
+   * do_target_wait_1 only strips TARGET_WNOHANG if target_can_async_p
+   * is false even if the target isn't actually async (target_async_p
+   * is false).  As a result, this must enable async mode here to
+   * avoid racing with the stop reported for attach.
+   */
+  if (target_can_async_p ())
+    target_async (1);
+}
+
+
 #ifdef TDP_RFPPWAIT
 /*
   To catch fork events, PT_FOLLOW_FORK is set on every traced process
@@ -1161,8 +1283,8 @@ fbsd_handle_debug_trap (fbsd_nat_target *target, ptid_t ptid,
    the status in *OURSTATUS.  */
 
 ptid_t
-fbsd_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
-		       target_wait_flags target_options)
+fbsd_nat_target::wait_1 (ptid_t ptid, struct target_waitstatus *ourstatus,
+			 target_wait_flags target_options)
 {
   ptid_t wptid;
 
@@ -1374,6 +1496,33 @@ fbsd_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
     }
 }
 
+ptid_t
+fbsd_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
+		       target_wait_flags target_options)
+{
+  ptid_t wptid;
+
+  fbsd_nat_debug_printf ("[%s], [%s]", target_pid_to_str (ptid).c_str (),
+			 target_options_to_string (target_options).c_str ());
+
+  /* Ensure any subsequent events trigger a new event in the loop.  */
+  if (target_is_async_p ())
+    fbsd_nat_event_pipe.flush ();
+
+  wptid = wait_1 (ptid, ourstatus, target_options);
+
+  /* If we are in async mode and found an event, there may still be
+     another event pending.  Trigger the event pipe so that that the
+     event loop keeps polling until no event is returned.  */
+  if (target_is_async_p () && ourstatus->kind != TARGET_WAITKIND_IGNORE)
+    fbsd_nat_event_pipe.mark ();
+
+  fbsd_nat_debug_printf ("returning [%s], [%s]",
+			 target_pid_to_str (wptid).c_str (),
+			 target_waitstatus_to_string (ourstatus).c_str ());
+  return wptid;
+}
+
 #ifdef USE_SIGTRAP_SIGINFO
 /* Implement the "stopped_by_sw_breakpoint" target_ops method.  */
 
@@ -1445,6 +1594,11 @@ fbsd_nat_target::follow_fork (bool follow_child, bool detach_fork)
 	  /* Schedule a fake VFORK_DONE event to report on the next
 	     wait.  */
 	  fbsd_add_vfork_done (inferior_ptid);
+
+	  /* If we're in async mode, need to tell the event loop
+	     there's something here to process.  */
+	  if (target_is_async_p ())
+	    fbsd_nat_event_pipe.mark ();
 	}
 #endif
     }
@@ -1546,4 +1700,7 @@ Enables printf debugging output."),
 			   NULL,
 			   &show_fbsd_nat_debug,
 			   &setdebuglist, &showdebuglist);
+
+  /* Install a SIGCHLD handler.  */
+  signal (SIGCHLD, sigchld_handler);
 }
diff --git a/gdb/fbsd-nat.h b/gdb/fbsd-nat.h
index 772655d320..7a0157a5ab 100644
--- a/gdb/fbsd-nat.h
+++ b/gdb/fbsd-nat.h
@@ -64,9 +64,19 @@ class fbsd_nat_target : public inf_ptrace_target
 
   void update_thread_list () override;
 
+  bool can_async_p () override;
+  bool is_async_p () override;
+
+  int async_wait_fd () override;
+  void async (int) override;
+
+  void close () override;
+
   thread_control_capabilities get_thread_control_capabilities () override
   { return tc_schedlock; }
 
+  void attach (const char *, int) override;
+
   void resume (ptid_t, int, enum gdb_signal) override;
 
   ptid_t wait (ptid_t, struct target_waitstatus *, target_wait_flags) override;
@@ -98,6 +108,9 @@ class fbsd_nat_target : public inf_ptrace_target
 #endif
 
   bool supports_multi_process () override;
+
+private:
+  ptid_t wait_1 (ptid_t, struct target_waitstatus *, target_wait_flags);
 };
 
 #endif /* fbsd-nat.h */
diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
index b6fa71fd2c..ee0331db09 100644
--- a/gdb/inf-ptrace.c
+++ b/gdb/inf-ptrace.c
@@ -300,10 +300,14 @@ inf_ptrace_target::resume (ptid_t ptid, int step, enum gdb_signal signal)
 
 ptid_t
 inf_ptrace_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
-			 target_wait_flags options)
+			 target_wait_flags target_options)
 {
   pid_t pid;
-  int status, save_errno;
+  int options, status, save_errno;
+
+  options = 0;
+  if (target_options & TARGET_WNOHANG)
+    options |= WNOHANG;
 
   do
     {
@@ -311,15 +315,32 @@ inf_ptrace_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
 
       do
 	{
-	  pid = waitpid (ptid.pid (), &status, 0);
+	  pid = waitpid (ptid.pid (), &status, options);
 	  save_errno = errno;
 	}
       while (pid == -1 && errno == EINTR);
 
       clear_sigint_trap ();
 
+      if (pid == 0)
+	{
+	  gdb_assert (target_options & TARGET_WNOHANG);
+	  ourstatus->kind = TARGET_WAITKIND_IGNORE;
+	  return minus_one_ptid;
+	}
+
       if (pid == -1)
 	{
+	  /* In async mode the SIGCHLD might have raced and triggered
+	     a check for an event that had already been reported.  If
+	     the event was the exit of the only remaining child,
+	     waitpid() will fail with ECHILD.  */
+	  if (ptid == minus_one_ptid && errno == ECHILD)
+	    {
+	      ourstatus->kind = TARGET_WAITKIND_IGNORE;
+	      return minus_one_ptid;
+	    }
+
 	  fprintf_unfiltered (gdb_stderr,
 			      _("Child process unexpectedly missing: %s.\n"),
 			      safe_strerror (save_errno));
-- 
2.31.1

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

[John Baldwin]

A few more things I thought of today:

On 6/7/21 10:09 AM, John Baldwin wrote:
> This is a fairly simple version of async target support.
> 
> Synchronous mode still uses blocking waitpid() calls in
> inf_ptrace::wait() unlike the Linux native target which always uses
> WNOHANG and uses sigsuspend() for synchronous operation.
> 
> Asynchronous mode registers an event pipe with the core as a file
> handle and writes to the pipe when SIGCHLD is raised.  WNOHANG is
> passed to waitpid when TARGET_WNOHANG is passed to the
> inf_ptrace::wait.
> 
> diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
> index 581c04d5f8..e5128266b8 100644
> --- a/gdb/fbsd-nat.c
> +++ b/gdb/fbsd-nat.c
> @@ -1374,6 +1496,33 @@ fbsd_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
>       }
>   }
>   
> +ptid_t
> +fbsd_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
> +		       target_wait_flags target_options)
> +{
> +  ptid_t wptid;
> +
> +  fbsd_nat_debug_printf ("[%s], [%s]", target_pid_to_str (ptid).c_str (),
> +			 target_options_to_string (target_options).c_str ());
> +
> +  /* Ensure any subsequent events trigger a new event in the loop.  */
> +  if (target_is_async_p ())
> +    fbsd_nat_event_pipe.flush ();

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

> diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
> index b6fa71fd2c..ee0331db09 100644
> --- a/gdb/inf-ptrace.c
> +++ b/gdb/inf-ptrace.c
> @@ -300,10 +300,14 @@ inf_ptrace_target::resume (ptid_t ptid, int step, enum gdb_signal signal)
>   
>   ptid_t
>   inf_ptrace_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
> -			 target_wait_flags options)
> +			 target_wait_flags target_options)
>   {
>     pid_t pid;
> -  int status, save_errno;
> +  int options, status, save_errno;
> +
> +  options = 0;
> +  if (target_options & TARGET_WNOHANG)
> +    options |= WNOHANG;
>   
>     do
>       {
> @@ -311,15 +315,32 @@ inf_ptrace_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
>   
>         do
>   	{
> -	  pid = waitpid (ptid.pid (), &status, 0);
> +	  pid = waitpid (ptid.pid (), &status, options);
>   	  save_errno = errno;
>   	}
>         while (pid == -1 && errno == EINTR);
>   
>         clear_sigint_trap ();
>   
> +      if (pid == 0)
> +	{
> +	  gdb_assert (target_options & TARGET_WNOHANG);
> +	  ourstatus->kind = TARGET_WAITKIND_IGNORE;
> +	  return minus_one_ptid;
> +	}
> +
>         if (pid == -1)
>   	{
> +	  /* In async mode the SIGCHLD might have raced and triggered
> +	     a check for an event that had already been reported.  If
> +	     the event was the exit of the only remaining child,
> +	     waitpid() will fail with ECHILD.  */
> +	  if (ptid == minus_one_ptid && errno == ECHILD)
> +	    {
> +	      ourstatus->kind = TARGET_WAITKIND_IGNORE;
> +	      return minus_one_ptid;
> +	    }
> +
>   	  fprintf_unfiltered (gdb_stderr,
>   			      _("Child process unexpectedly missing: %s.\n"),
>   			      safe_strerror (save_errno));

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

-- 
John Baldwin

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

[Pedro Alves]

On 2021-06-07 11:49 p.m., John Baldwin wrote:

>> @@ -311,15 +315,32 @@ inf_ptrace_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
>>           do
>>       {
>> -      pid = waitpid (ptid.pid (), &status, 0);
>> +      pid = waitpid (ptid.pid (), &status, options);
>>         save_errno = errno;
>>       }
>>         while (pid == -1 && errno == EINTR);
>>           clear_sigint_trap ();
>>   +      if (pid == 0)
>> +    {
>> +      gdb_assert (target_options & TARGET_WNOHANG);
>> +      ourstatus->kind = TARGET_WAITKIND_IGNORE;
>> +      return minus_one_ptid;
>> +    }
>> +
>>         if (pid == -1)
>>       {
>> +      /* In async mode the SIGCHLD might have raced and triggered
>> +         a check for an event that had already been reported.  If
>> +         the event was the exit of the only remaining child,
>> +         waitpid() will fail with ECHILD.  */
>> +      if (ptid == minus_one_ptid && errno == ECHILD)

I'm curious on the ptid == minus_one_ptid check.
Does this race&error only happen with waitpid(-1, ...) ?

>> +        {
>> +          ourstatus->kind = TARGET_WAITKIND_IGNORE;
>> +          return minus_one_ptid;
>> +        }
>> +
>>         fprintf_unfiltered (gdb_stderr,
>>                     _("Child process unexpectedly missing: %s.\n"),
>>                     safe_strerror (save_errno));
> 
> Note that the code below this hunk returns a "made up" wait status that uses
> inferior_ptid.  Since inferior_ptid is always 0 at this point, it always
> triggers an assertion later on when the core sees an all-zero ptid.  I
> think we should probably make this code just internal_error() when waitpid
> returns an error rather than tripping an assertion later.
> 

I agree.

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

[John Baldwin]

On 6/27/21 9:12 AM, Pedro Alves wrote:
> On 2021-06-07 11:49 p.m., John Baldwin wrote:
> 
>>> @@ -311,15 +315,32 @@ inf_ptrace_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
>>>            do
>>>        {
>>> -      pid = waitpid (ptid.pid (), &status, 0);
>>> +      pid = waitpid (ptid.pid (), &status, options);
>>>          save_errno = errno;
>>>        }
>>>          while (pid == -1 && errno == EINTR);
>>>            clear_sigint_trap ();
>>>    +      if (pid == 0)
>>> +    {
>>> +      gdb_assert (target_options & TARGET_WNOHANG);
>>> +      ourstatus->kind = TARGET_WAITKIND_IGNORE;
>>> +      return minus_one_ptid;
>>> +    }
>>> +
>>>          if (pid == -1)
>>>        {
>>> +      /* In async mode the SIGCHLD might have raced and triggered
>>> +         a check for an event that had already been reported.  If
>>> +         the event was the exit of the only remaining child,
>>> +         waitpid() will fail with ECHILD.  */
>>> +      if (ptid == minus_one_ptid && errno == ECHILD)
> 
> I'm curious on the ptid == minus_one_ptid check.
> Does this race&error only happen with waitpid(-1, ...) ?

I've only observed it with minus_one_ptid.  I believe that the the
SIGCHLD triggered waits (handle_target_event -> inferior_event_hander ->
fetch_inferior_event -> do_target_wait) will always use minus_one_ptid.

-- 
John Baldwin

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

[Pedro Alves]

On 2021-07-12 5:32 p.m., John Baldwin wrote:
> On 6/27/21 9:12 AM, Pedro Alves wrote:
>> On 2021-06-07 11:49 p.m., John Baldwin wrote:
>>
>>>> @@ -311,15 +315,32 @@ inf_ptrace_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
>>>>            do
>>>>        {
>>>> -      pid = waitpid (ptid.pid (), &status, 0);
>>>> +      pid = waitpid (ptid.pid (), &status, options);
>>>>          save_errno = errno;
>>>>        }
>>>>          while (pid == -1 && errno == EINTR);
>>>>            clear_sigint_trap ();
>>>>    +      if (pid == 0)
>>>> +    {
>>>> +      gdb_assert (target_options & TARGET_WNOHANG);
>>>> +      ourstatus->kind = TARGET_WAITKIND_IGNORE;
>>>> +      return minus_one_ptid;
>>>> +    }
>>>> +
>>>>          if (pid == -1)
>>>>        {
>>>> +      /* In async mode the SIGCHLD might have raced and triggered
>>>> +         a check for an event that had already been reported.  If
>>>> +         the event was the exit of the only remaining child,
>>>> +         waitpid() will fail with ECHILD.  */
>>>> +      if (ptid == minus_one_ptid && errno == ECHILD)
>>
>> I'm curious on the ptid == minus_one_ptid check.
>> Does this race&error only happen with waitpid(-1, ...) ?
> 
> I've only observed it with minus_one_ptid.  I believe that the the
> SIGCHLD triggered waits (handle_target_event -> inferior_event_hander ->
> fetch_inferior_event -> do_target_wait) will always use minus_one_ptid.
> 

OK.  

You may consider checking wonder whether this should return TARGET_WAITKIND_NO_RESUMED
instead of TARGET_WAITKIND_IGNORE.  It may end up helping make e.g.,
gdb.threads/no-unwaited-for-left.exp work against FreeBSD.  I assume that it is
failing today.

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

[John Baldwin]

On 7/13/21 5:38 AM, Pedro Alves wrote:
> On 2021-07-12 5:32 p.m., John Baldwin wrote:
>> On 6/27/21 9:12 AM, Pedro Alves wrote:
>>> On 2021-06-07 11:49 p.m., John Baldwin wrote:
>>>
>>>>> @@ -311,15 +315,32 @@ inf_ptrace_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
>>>>>             do
>>>>>         {
>>>>> -      pid = waitpid (ptid.pid (), &status, 0);
>>>>> +      pid = waitpid (ptid.pid (), &status, options);
>>>>>           save_errno = errno;
>>>>>         }
>>>>>           while (pid == -1 && errno == EINTR);
>>>>>             clear_sigint_trap ();
>>>>>     +      if (pid == 0)
>>>>> +    {
>>>>> +      gdb_assert (target_options & TARGET_WNOHANG);
>>>>> +      ourstatus->kind = TARGET_WAITKIND_IGNORE;
>>>>> +      return minus_one_ptid;
>>>>> +    }
>>>>> +
>>>>>           if (pid == -1)
>>>>>         {
>>>>> +      /* In async mode the SIGCHLD might have raced and triggered
>>>>> +         a check for an event that had already been reported.  If
>>>>> +         the event was the exit of the only remaining child,
>>>>> +         waitpid() will fail with ECHILD.  */
>>>>> +      if (ptid == minus_one_ptid && errno == ECHILD)
>>>
>>> I'm curious on the ptid == minus_one_ptid check.
>>> Does this race&error only happen with waitpid(-1, ...) ?
>>
>> I've only observed it with minus_one_ptid.  I believe that the the
>> SIGCHLD triggered waits (handle_target_event -> inferior_event_hander ->
>> fetch_inferior_event -> do_target_wait) will always use minus_one_ptid.
>>
> 
> OK.
> 
> You may consider checking wonder whether this should return TARGET_WAITKIND_NO_RESUMED
> instead of TARGET_WAITKIND_IGNORE.  It may end up helping make e.g.,
> gdb.threads/no-unwaited-for-left.exp work against FreeBSD.  I assume that it is
> failing today.

So this test is failing, yes, but it is failing before getting to this point.
The problem appears to be that when thread 2 exits, gdb never prints another
prompt to accept more commands.  I originally thought that perhaps the problem
was that fbsd-nat.c swallows thread exit events instead of reporting them as
TARGET_WAITKIND_THREAD_EXITED, but even with that fix I still get a hang.

Here's the output with fbsd-nat.c patched to report THREAD_EXITED:

Reading symbols from testsuite/outputs/gdb.threads/no-unwaited-for-left/no-unwaited-for-left...
(gdb) directory ~/work/git/gdb/gdb/testsuite/gdb.threads/
Source directories searched: /home/john/work/git/gdb/gdb/testsuite/gdb.threads:$cdir:$cwd
(gdb) break -qualified main
Breakpoint 1 at 0x400a8e: file /home/john/work/git/gdb/gdb/testsuite/gdb.threads/no-unwaited-for-left.c, line 51.
(gdb) r
Starting program: /usr/home/john/work/git/gdb/obj/gdb/testsuite/outputs/gdb.threads/no-unwaited-for-left/no-unwaited-for-left

Breakpoint 1, main () at /home/john/work/git/gdb/gdb/testsuite/gdb.threads/no-unwaited-for-left.c:51
51        i = pthread_create (&thread, NULL, thread_a, NULL);
(gdb) break no-unwaited-for-left.c:28
Breakpoint 2 at 0x400a2d: file /home/john/work/git/gdb/gdb/testsuite/gdb.threads/no-unwaited-for-left.c, line 28.
(gdb) c
Continuing.
[New LWP 500596 of process 51230]
[Switching to LWP 500596 of process 51230]

Thread 2 hit Breakpoint 2, thread_a (arg=0x0) at /home/john/work/git/gdb/gdb/testsuite/gdb.threads/no-unwaited-for-left.c:28
28        return 0; /* break-here */
(gdb) set scheduler-locking on
(gdb) set debug infrun on
(gdb) set debug fbsd-nat on
(gdb) set debug fbsd-lwp on
(gdb) c
Continuing.
[infrun] clear_proceed_status_thread: LWP 500596 of process 51230
[infrun] proceed: enter
   [infrun] proceed: addr=0xffffffffffffffff, signal=GDB_SIGNAL_DEFAULT
   [infrun] global_thread_step_over_chain_enqueue: enqueueing thread LWP 500596 of process 51230 in global step over chain
   [infrun] scoped_disable_commit_resumed: reason=proceeding
   [infrun] start_step_over: enter
     [infrun] start_step_over: stealing global queue of threads to step, length = 2
     [infrun] start_step_over: resuming [LWP 500596 of process 51230] for step-over
     [infrun] should_be_inserted: skipping breakpoint: stepping past insn at: 0x400a2d
     [infrun] should_be_inserted: skipping breakpoint: stepping past insn at: 0x400a2d
     [infrun] should_be_inserted: skipping breakpoint: stepping past insn at: 0x400a2d
     [infrun] resume_1: step=1, signal=GDB_SIGNAL_0, trap_expected=1, current thread [LWP 500596 of process 51230] at 0x400a2d
     [fbsd-nat] resume: [LWP 500596 of process 51230], step 1, signo 0 (0)
     [infrun] infrun_async: enable=1
     [infrun] prepare_to_wait: prepare_to_wait
     [infrun] start_step_over: [LWP 500596 of process 51230] was resumed.
     [infrun] operator(): step-over queue now empty
   [infrun] start_step_over: exit
   [infrun] reset: reason=proceeding
   [infrun] maybe_set_commit_resumed_all_targets: enabling commit-resumed for target native
   [infrun] maybe_call_commit_resumed_all_targets: calling commit_resumed for target native
[infrun] proceed: exit
[infrun] fetch_inferior_event: enter
   [infrun] scoped_disable_commit_resumed: reason=handling event
   [infrun] random_pending_event_thread: None found.
   [fbsd-nat] wait: [process -1], [TARGET_WNOHANG]
   [fbsd-nat] wait_1: stop for LWP 500596 event 1 flags 0x20
   [fbsd-nat] wait_1: si_signo 5 si_code 2
   [fbsd-nat] fbsd_handle_debug_trap: trace trap for LWP 500596
   [fbsd-nat] wait: returning [LWP 500596 of process 51230], [status->kind = stopped, signal = GDB_SIGNAL_TRAP]
   [infrun] print_target_wait_results: target_wait (-1.0.0 [process -1], status) =
   [infrun] print_target_wait_results:   51230.500596.0 [LWP 500596 of process 51230],
   [infrun] print_target_wait_results:   status->kind = stopped, signal = GDB_SIGNAL_TRAP
   [infrun] handle_inferior_event: status->kind = stopped, signal = GDB_SIGNAL_TRAP
   [infrun] clear_step_over_info: clearing step over info
   [infrun] context_switch: Switching context from process 0 to LWP 500596 of process 51230
   [infrun] handle_signal_stop: stop_pc=0x400a32
   [infrun] process_event_stop_test: no stepping, continue
   [infrun] resume_1: step=0, signal=GDB_SIGNAL_0, trap_expected=0, current thread [LWP 500596 of process 51230] at 0x400a32
   [fbsd-nat] resume: [LWP 500596 of process 51230], step 0, signo 0 (0)
   [infrun] prepare_to_wait: prepare_to_wait
   [infrun] reset: reason=handling event
   [infrun] maybe_set_commit_resumed_all_targets: enabling commit-resumed for target native
   [infrun] maybe_call_commit_resumed_all_targets: calling commit_resumed for target native
[infrun] fetch_inferior_event: exit
[infrun] fetch_inferior_event: enter
   [infrun] scoped_disable_commit_resumed: reason=handling event
   [infrun] random_pending_event_thread: None found.
   [fbsd-nat] wait: [process -1], [TARGET_WNOHANG]
   [fbsd-nat] wait_1: stop for LWP 500596 event 1 flags 0x204
   [fbsd-lwp] wait_1: deleting thread for LWP 500596
[LWP 500596 of process 51230 exited]
   [fbsd-nat] wait: returning [LWP 500596 of process 51230], [status->kind = thread exited, status = 0]
   [infrun] print_target_wait_results: target_wait (-1.0.0 [process -1], status) =
   [infrun] print_target_wait_results:   51230.500596.0 [LWP 500596 of process 51230],
   [infrun] print_target_wait_results:   status->kind = thread exited, status = 0
   [infrun] handle_inferior_event: status->kind = thread exited, status = 0
   [infrun] prepare_to_wait: prepare_to_wait
   [infrun] reset: reason=handling event
   [infrun] maybe_set_commit_resumed_all_targets: enabling commit-resumed for target native
   [infrun] maybe_call_commit_resumed_all_targets: calling commit_resumed for target native
[infrun] fetch_inferior_event: exit
[infrun] fetch_inferior_event: enter
   [infrun] scoped_disable_commit_resumed: reason=handling event
   [infrun] random_pending_event_thread: None found.
   [fbsd-nat] wait: [process -1], [TARGET_WNOHANG]
   [fbsd-nat] wait: returning [process -1], [status->kind = ignore]
   [infrun] fetch_inferior_event: do_target_wait returned no event
   [infrun] reset: reason=handling event
   [infrun] maybe_set_commit_resumed_all_targets: enabling commit-resumed for target native
   [infrun] maybe_call_commit_resumed_all_targets: calling commit_resumed for target native
[infrun] fetch_inferior_event: exit

The TARGET_WAITKIND_IGNORE comes from the earlier new block this patch
adds in inf_ptrace::wait() where it returns IGNORE when waitpid() returns
0 (meaning there are still child processes, but there are no events to
report).  I tried changing that earlier block to return NO_RESUMED, but
that caused other breakage (and I think it's also wrong, you really only
want NO_RESUMED I think when there are no viable processes left at all
which is the ECHILD error).  It did though provide a (gdb) prompt, so I
think something about returning IGNORE causes the core to not print a
prompt and sit waiting for an inferior event.  I can't really see what
else to return in this case though, and I believe linux-nat.c::wait()
returns IGNORE in this case as well.

-- 
John Baldwin

[RFC PATCH 5/7] fbsd nat: Include ptrace operation in error messages.

[John Baldwin]

	* fbsd-nat.c (fbsd_nat_target::thread_name)
	(fbsd_enable_proc_events, fbsd_add_threads)
	(fbsd_nat_target::resume, fbsd_nat_target::wait_1)
	(fbsd_nat_target::follow_fork): Include ptrace operation in error
	message if ptrace fails.
---
 gdb/ChangeLog  |  8 ++++++++
 gdb/fbsd-nat.c | 34 ++++++++++++++++++----------------
 2 files changed, 26 insertions(+), 16 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index fba798a63d..1369444e89 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,11 @@
+2021-06-04  John Baldwin  <jhb@FreeBSD.org>
+
+	* fbsd-nat.c (fbsd_nat_target::thread_name)
+	(fbsd_enable_proc_events, fbsd_add_threads)
+	(fbsd_nat_target::resume, fbsd_nat_target::wait_1)
+	(fbsd_nat_target::follow_fork): Include ptrace operation in error
+	message if ptrace fails.
+
 2021-06-04  John Baldwin  <jhb@FreeBSD.org>
 
 	* fbsd-nat.c: Include gdbsupport/event-loop.h,
diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
index e5128266b8..84bf23a36b 100644
--- a/gdb/fbsd-nat.c
+++ b/gdb/fbsd-nat.c
@@ -817,7 +817,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);
@@ -846,22 +846,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
 }
@@ -880,13 +880,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++)
     {
@@ -900,7 +900,7 @@ fbsd_add_threads (fbsd_nat_target *target, pid_t pid)
 	  /* Don't add exited threads.  Note that this is only called
 	     when attaching to a multi-threaded process.  */
 	  if (ptrace (PT_LWPINFO, lwps[i], (caddr_t) &pl, sizeof pl) == -1)
-	    perror_with_name (("ptrace"));
+	    perror_with_name (_("ptrace (PT_LWPINFO)"));
 	  if (pl.pl_flags & PL_FLAG_EXITED)
 	    continue;
 #endif
@@ -1180,7 +1180,9 @@ fbsd_nat_target::resume (ptid_t ptid, int step, enum gdb_signal signo)
 	    request = PT_SUSPEND;
 
 	  if (ptrace (request, tp->ptid.lwp (), NULL, 0) == -1)
-	    perror_with_name (("ptrace"));
+	    perror_with_name (request == PT_RESUME ?
+			      _("ptrace (PT_RESUME)") :
+			      _("ptrace (PT_SUSPEND)"));
 	}
     }
   else
@@ -1189,7 +1191,7 @@ fbsd_nat_target::resume (ptid_t ptid, int step, enum gdb_signal signo)
 	 until the process is continued however).  */
       for (thread_info *tp : all_non_exited_threads (this, ptid))
 	if (ptrace (PT_RESUME, tp->ptid.lwp (), NULL, 0) == -1)
-	  perror_with_name (("ptrace"));
+	  perror_with_name (_("ptrace (PT_RESUME)"));
       ptid = inferior_ptid;
     }
 
@@ -1219,7 +1221,7 @@ fbsd_nat_target::resume (ptid_t ptid, int step, enum gdb_signal signo)
   if (step)
     {
       if (ptrace (PT_SETSTEP, get_ptrace_pid (ptid), NULL, 0) == -1)
-	perror_with_name (("ptrace"));
+	perror_with_name (_("ptrace (PT_SETSTEP)"));
       step = 0;
     }
   ptid = ptid_t (ptid.pid ());
@@ -1307,7 +1309,7 @@ fbsd_nat_target::wait_1 (ptid_t ptid, struct target_waitstatus *ourstatus,
 
 	  pid = wptid.pid ();
 	  if (ptrace (PT_LWPINFO, pid, (caddr_t) &pl, sizeof pl) == -1)
-	    perror_with_name (("ptrace"));
+	    perror_with_name (_("ptrace (PT_LWPINFO)"));
 
 	  wptid = ptid_t (pid, pl.pl_lwpid, 0);
 
@@ -1339,7 +1341,7 @@ fbsd_nat_target::wait_1 (ptid_t ptid, struct target_waitstatus *ourstatus,
 		  delete_thread (thr);
 		}
 	      if (ptrace (PT_CONTINUE, pid, (caddr_t) 1, 0) == -1)
-		perror_with_name (("ptrace"));
+		perror_with_name (_("ptrace (PT_CONTINUE)"));
 	      continue;
 	    }
 #endif
@@ -1402,7 +1404,7 @@ fbsd_nat_target::wait_1 (ptid_t ptid, struct target_waitstatus *ourstatus,
 		  gdb_assert (pid == child);
 
 		  if (ptrace (PT_LWPINFO, child, (caddr_t)&pl, sizeof pl) == -1)
-		    perror_with_name (("ptrace"));
+		    perror_with_name (_("ptrace (PT_LWPINFO)"));
 
 		  gdb_assert (pl.pl_flags & PL_FLAG_CHILD);
 		  child_ptid = ptid_t (child, pl.pl_lwpid, 0);
@@ -1488,7 +1490,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;
 	    }
 	}
@@ -1566,7 +1568,7 @@ fbsd_nat_target::follow_fork (bool follow_child, bool detach_fork)
 	 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 (tp->pending_follow.kind == TARGET_WAITKIND_VFORKED)
-- 
2.31.1

Re: [RFC PATCH 5/7] fbsd nat: Include ptrace operation in error messages.

[Pedro Alves]

On 2021-06-07 6:09 p.m., John Baldwin wrote:
> -    perror_with_name (("ptrace"));
> +    perror_with_name (_("ptrace (PT_GET_EVENT_MASK)"));

Note that the missing _ was not a typo -- it's there to silence ARI warnings
about missing i18n when the string has nothing to translate.

Re: [RFC PATCH 5/7] fbsd nat: Include ptrace operation in error messages.

[Pedro Alves]

On 2021-06-27 5:13 p.m., Pedro Alves wrote:
> On 2021-06-07 6:09 p.m., John Baldwin wrote:
>> -    perror_with_name (("ptrace"));
>> +    perror_with_name (_("ptrace (PT_GET_EVENT_MASK)"));
> 
> Note that the missing _ was not a typo -- it's there to silence ARI warnings
> about missing i18n when the string has nothing to translate.
> 

TBC, by "it's there", I meant to say "the double parens are there".

Re: [RFC PATCH 5/7] fbsd nat: Include ptrace operation in error messages.

[John Baldwin]

On 6/27/21 9:13 AM, Pedro Alves wrote:
> On 2021-06-07 6:09 p.m., John Baldwin wrote:
>> -    perror_with_name (("ptrace"));
>> +    perror_with_name (_("ptrace (PT_GET_EVENT_MASK)"));
> 
> Note that the missing _ was not a typo -- it's there to silence ARI warnings
> about missing i18n when the string has nothing to translate.

Hmm, the new strings probably don't have anything to translate either, so
I should probably add the _ back?

-- 
John Baldwin

Re: [RFC PATCH 5/7] fbsd nat: Include ptrace operation in error messages.

[Pedro Alves]

On 2021-07-12 4:11 p.m., John Baldwin wrote:
> On 6/27/21 9:13 AM, Pedro Alves wrote:
>> On 2021-06-07 6:09 p.m., John Baldwin wrote:
>>> -    perror_with_name (("ptrace"));
>>> +    perror_with_name (_("ptrace (PT_GET_EVENT_MASK)"));
>>
>> Note that the missing _ was not a typo -- it's there to silence ARI warnings
>> about missing i18n when the string has nothing to translate.
> 
> Hmm, the new strings probably don't have anything to translate either, so
> I should probably add the _ back?
> 

I guess you meant "remove the _ back".  It's not a big deal, especially since
all these years and we never managed to start working with a translation
group...  :-/   But I think so.

[RFC PATCH 6/7] fbsd nat: Switch function 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/ChangeLog:

	* fbsd-nat.c (fbsd_nat_target::resume): Adjust debug message.
---
 gdb/ChangeLog  | 4 ++++
 gdb/fbsd-nat.c | 5 +++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 1369444e89..675cc0aaf6 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,7 @@
+2021-06-04  John Baldwin  <jhb@FreeBSD.org>
+
+	* fbsd-nat.c (fbsd_nat_target::resume): Adjust debug message.
+
 2021-06-04  John Baldwin  <jhb@FreeBSD.org>
 
 	* fbsd-nat.c (fbsd_nat_target::thread_name)
diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
index 84bf23a36b..92bd81ea33 100644
--- a/gdb/fbsd-nat.c
+++ b/gdb/fbsd-nat.c
@@ -1163,8 +1163,9 @@ fbsd_nat_target::resume (ptid_t ptid, int step, enum gdb_signal signo)
     return;
 #endif
 
-  fbsd_lwp_debug_printf ("ptid (%d, %ld, %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

[RFC PATCH 7/7] 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/ChangeLog:

	* fbsd-nat.c (fbsd_nat_target::thread_alive): Return NULL if
	fbsd_fetch_kinfo_proc or ptrace fail.
---
 gdb/ChangeLog  | 5 +++++
 gdb/fbsd-nat.c | 4 ++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 675cc0aaf6..1f5c939a4d 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2021-06-04  John Baldwin  <jhb@FreeBSD.org>
+
+	* fbsd-nat.c (fbsd_nat_target::thread_alive): Return NULL if
+	fbsd_fetch_kinfo_proc or ptrace fail.
+
 2021-06-04  John Baldwin  <jhb@FreeBSD.org>
 
 	* fbsd-nat.c (fbsd_nat_target::resume): Adjust debug message.
diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
index 92bd81ea33..d88add7b56 100644
--- a/gdb/fbsd-nat.c
+++ b/gdb/fbsd-nat.c
@@ -815,9 +815,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

Re: [RFC PATCH 0/7] FreeBSD target async mode and related refactoring

[Lancelot SIX]

On Mon, Jun 07, 2021 at 10:09:25AM -0700, John Baldwin wrote:
> I hacked on this over the weekend.  For the testsuite on FreeBSD/amd64
> it doesn't seem to add any new regressions and fixes various tests.
> Patch 7 fixes a few more failures I noticed when comparing the
> before/after logs.
> 
> This is an RFC as I have a few questions / thoughts I'd like some
> feedback on before refining this further:
> 
> 1) I haven't yet tested this on Linux and will need to do so to make
>    sure the initial event_pipe refactoring doesn't cause regressions.
> 
> 2) gdbsupport/event-pipe.cc probably doesn't build on Windows
>    (gdb/ser-event.c doesn't use pipes on Windows which I didn't
>    find until after I'd written this patch).  I think I'd like to only
>    build event-pipe.cc for systems with HAVE_PIPE or HAVE_PIPE2.
>    I just don't know how to express this in Makefile.am.

Hi,

The following could do the trick, but I am not sure if it is completely
bullet-proof.

	diff --git a/gdbsupport/Makefile.am b/gdbsupport/Makefile.am
	index 6d4678c8c9b..5c94fc8a8d3 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,7 +75,8 @@ libgdbsupport_a_SOURCES = \
	     tdesc.cc \
	     thread-pool.cc \
	     xml-utils.cc \
	-    $(selftest)
	+    $(selftest) \
	+    $(eventpipe)
	 
	 # Double-check that no defines are missing from our configury.
	 check-defines:
	diff --git a/gdbsupport/configure.ac b/gdbsupport/configure.ac
	index a8fcfe24c32..323ff52da37 100644
	--- a/gdbsupport/configure.ac
	+++ b/gdbsupport/configure.ac
	@@ -53,6 +53,8 @@ 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

From what I can see, ac_ct_func_pipe is available, otherwise
`AC_CHECK_FUNC([pipe])` (or derivative) can be used (same for pipe2).

Lancelot.

> 
>    I could potentially add a follow-up patch to change gdb/ser-events.c
>    to use event_pipe rather than duplicating very similar logic for
>    the non-Windows case.  The one difference I noticed so far is
>    that the existing Linux event pipe drains any existing input
>    in the "mark" method before writing the new character, but the
>    ser-events version does not do the drain.
> 
>    An alternative perhaps is that instead of adding event_pipe to
>    gdbsupport, I could add a slightly more abstract interface that is
>    more like ser_events.c but at the lower level API where the
>    event_fd / mark / post methods used an Event on Windows.  I'm not
>    sure this is really needed though as if Windows wanted to use an
>    event in the future for async mode it would be in Windows-specific
>    code and could just use an Event directly.
> 
> 3) I was surprised by the need to enable async mode explicitly in
>    target::attach.  I had assumed that the policy of async vs
>    non-async would have belonged in the core and that the core would
>    have enabled async if it wanted during attach.  Similarly, I would
>    expect do_target_wait_1 in infrun.c to check target_is_async_p
>    rather than target_can_async_p for deciding whether to clear
>    TARGET_WNOHANG.
> 
> 4) It may be that some of this can be pulled into inf-ptrace to
>    simplify adding async support on other platforms.  For example,
>    inf-ptrace.c could export a 'ptrace_event_pipe' and provide
>    implementations of is_async_p and async_waitfd that used that along
>    with possibly some class helper methods to wrap the flush/mark
>    methods to mostly hide ptrace_event_pipe (though the sigchld
>    handlers probably still want to access it directly).  This would
>    mean that the linux_nat_target::close and fbsd_nat_target::close
>    methods could collapse back up into inf_ptrace_target along with
>    ::is_async_p and ::async_waitfd.  Targets on other platforms
>    would opt-in by providing ::can_async_p.
> 
> 5) I don't understand why linux_nat_target::resume enables async
>    mode explicitly similar to ::attach.  I don't think I need it
>    in the FreeBSD target, but I'd feel better about that if I knew
>    why the Linux target needed it.
> 
> John Baldwin (7):
>   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.
>   fbsd-nat: Implement async target support.
>   fbsd nat: Include ptrace operation in error messages.
>   fbsd nat: Switch function entry debug message from lwp to nat.
>   fbsd nat: Return NULL rather than failing thread_alive.
> 
>  gdb/ChangeLog            |  49 ++++++++++
>  gdb/fbsd-nat.c           | 202 +++++++++++++++++++++++++++++++++++----
>  gdb/fbsd-nat.h           |  13 +++
>  gdb/inf-ptrace.c         |  27 +++++-
>  gdb/linux-nat.c          |  48 +++-------
>  gdbserver/ChangeLog      |   8 ++
>  gdbserver/linux-low.cc   |  39 ++------
>  gdbsupport/ChangeLog     |   7 ++
>  gdbsupport/Makefile.am   |   1 +
>  gdbsupport/Makefile.in   |  21 ++--
>  gdbsupport/event-pipe.cc | 100 +++++++++++++++++++
>  gdbsupport/event-pipe.h  |  55 +++++++++++
>  12 files changed, 470 insertions(+), 100 deletions(-)
>  create mode 100644 gdbsupport/event-pipe.cc
>  create mode 100644 gdbsupport/event-pipe.h
> 
> -- 
> 2.31.1
> 

Re: [RFC PATCH 0/7] FreeBSD target async mode and related refactoring

[Pedro Alves]

On 2021-06-07 6:09 p.m., John Baldwin wrote:
> I hacked on this over the weekend.  For the testsuite on FreeBSD/amd64
> it doesn't seem to add any new regressions and fixes various tests.
> Patch 7 fixes a few more failures I noticed when comparing the
> before/after logs.

Cool!

> 
> This is an RFC as I have a few questions / thoughts I'd like some
> feedback on before refining this further:
> 
> 1) I haven't yet tested this on Linux and will need to do so to make
>    sure the initial event_pipe refactoring doesn't cause regressions.
> 
> 2) gdbsupport/event-pipe.cc probably doesn't build on Windows
>    (gdb/ser-event.c doesn't use pipes on Windows which I didn't
>    find until after I'd written this patch).  I think I'd like to only
>    build event-pipe.cc for systems with HAVE_PIPE or HAVE_PIPE2.
>    I just don't know how to express this in Makefile.am.

A simple way is to just always build everywhere, and then wrap
the whole file with some #ifdef, like e.g., #ifndef __MINGW32__.

> 
>    I could potentially add a follow-up patch to change gdb/ser-events.c
>    to use event_pipe rather than duplicating very similar logic for
>    the non-Windows case.  The one difference I noticed so far is
>    that the existing Linux event pipe drains any existing input
>    in the "mark" method before writing the new character, but the
>    ser-events version does not do the drain.

I don't recall why I didn't factor out the self-pipe bits to its own
class like you are doing now, back when I added ser-events.h|c.
It's possible I thought we'd go the other way around, and use
serial_event in linux-nat.c, but hmmm, no, that can't be because gdbserver
doesn't use the struct serial stuff.  Your approach is better.

> 
>    An alternative perhaps is that instead of adding event_pipe to
>    gdbsupport, I could add a slightly more abstract interface that is
>    more like ser_events.c but at the lower level API where the
>    event_fd / mark / post methods used an Event on Windows.  I'm not
>    sure this is really needed though as if Windows wanted to use an
>    event in the future for async mode it would be in Windows-specific
>    code and could just use an Event directly.

Yeah, let's not come up with abstractions we don't know will be useful.

> 
> 3) I was surprised by the need to enable async mode explicitly in
>    target::attach.  I had assumed that the policy of async vs
>    non-async would have belonged in the core and that the core would
>    have enabled async if it wanted during attach.  

Yeah, the async enable-/disablement has been done in the targets
since really early stages, before my time.  Note comments like this
in remote_target::resume():

  /* 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);

I've occasionally pulled some of this to the core (e.g.,
infrun.c:do_target_resume enables async) but there's more to do as
you noticed.

>    Similarly, I would
>    expect do_target_wait_1 in infrun.c to check target_is_async_p
>    rather than target_can_async_p for deciding whether to clear
>    TARGET_WNOHANG.

Hmm, I think you're right.

> 
> 4) It may be that some of this can be pulled into inf-ptrace to
>    simplify adding async support on other platforms.  For example,
>    inf-ptrace.c could export a 'ptrace_event_pipe' and provide
>    implementations of is_async_p and async_waitfd that used that along
>    with possibly some class helper methods to wrap the flush/mark
>    methods to mostly hide ptrace_event_pipe (though the sigchld
>    handlers probably still want to access it directly).  This would
>    mean that the linux_nat_target::close and fbsd_nat_target::close
>    methods could collapse back up into inf_ptrace_target along with
>    ::is_async_p and ::async_waitfd.  Targets on other platforms
>    would opt-in by providing ::can_async_p.

That sounds good too.

> 
> 5) I don't understand why linux_nat_target::resume enables async
>    mode explicitly similar to ::attach.  I don't think I need it
>    in the FreeBSD target, but I'd feel better about that if I knew
>    why the Linux target needed it.

That simply predates the target_async call in do_target_resume that I
mentioned above.

Pedro Alves