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