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

[Andrew Burgess <aburgess@redhat.com>]

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

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

Thanks, this looks like a great idea.

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

> 
> Co-Authored-By: Lancelot SIX <lsix@lancelotsix.com>
> ---
>  gdbsupport/Makefile.am   |   5 ++
>  gdbsupport/Makefile.in   |   9 +++-
>  gdbsupport/configure     |  15 ++++++
>  gdbsupport/configure.ac  |   3 ++
>  gdbsupport/event-pipe.cc | 100 +++++++++++++++++++++++++++++++++++++++
>  gdbsupport/event-pipe.h  |  56 ++++++++++++++++++++++
>  6 files changed, 186 insertions(+), 2 deletions(-)
>  create mode 100644 gdbsupport/event-pipe.cc
>  create mode 100644 gdbsupport/event-pipe.h
> 
> diff --git a/gdbsupport/Makefile.am b/gdbsupport/Makefile.am
> index 6d4678c8c9b..3426d813c16 100644
> --- a/gdbsupport/Makefile.am
> +++ b/gdbsupport/Makefile.am
> @@ -35,6 +35,10 @@ if SELFTEST
>  selftest = selftest.cc
>  endif
>  
> +if HAVE_PIPE_OR_PIPE2
> +eventpipe = event-pipe.cc
> +endif
> +
>  libgdbsupport_a_SOURCES = \
>      agent.cc \
>      btrace-common.cc \
> @@ -71,6 +75,7 @@ libgdbsupport_a_SOURCES = \
>      tdesc.cc \
>      thread-pool.cc \
>      xml-utils.cc \
> +    ${eventpipe} \
>      $(selftest)
>  
>  # Double-check that no defines are missing from our configury.
> diff --git a/gdbsupport/Makefile.in b/gdbsupport/Makefile.in
> index d7f2d4914b0..c9a8398ee44 100644
> --- a/gdbsupport/Makefile.in
> +++ b/gdbsupport/Makefile.in
> @@ -144,7 +144,8 @@ am__v_AR_0 = @echo "  AR      " $@;
>  am__v_AR_1 = 
>  libgdbsupport_a_AR = $(AR) $(ARFLAGS)
>  libgdbsupport_a_LIBADD =
> -@SELFTEST_TRUE@am__objects_1 = selftest.$(OBJEXT)
> +@HAVE_PIPE_OR_PIPE2_TRUE@am__objects_1 = event-pipe.$(OBJEXT)
> +@SELFTEST_TRUE@am__objects_2 = selftest.$(OBJEXT)
>  am_libgdbsupport_a_OBJECTS = agent.$(OBJEXT) btrace-common.$(OBJEXT) \
>  	buffer.$(OBJEXT) cleanups.$(OBJEXT) common-debug.$(OBJEXT) \
>  	common-exceptions.$(OBJEXT) common-inferior.$(OBJEXT) \
> @@ -158,7 +159,8 @@ am_libgdbsupport_a_OBJECTS = agent.$(OBJEXT) btrace-common.$(OBJEXT) \
>  	run-time-clock.$(OBJEXT) safe-strerror.$(OBJEXT) \
>  	scoped_mmap.$(OBJEXT) search.$(OBJEXT) signals.$(OBJEXT) \
>  	signals-state-save-restore.$(OBJEXT) tdesc.$(OBJEXT) \
> -	thread-pool.$(OBJEXT) xml-utils.$(OBJEXT) $(am__objects_1)
> +	thread-pool.$(OBJEXT) xml-utils.$(OBJEXT) $(am__objects_1) \
> +	$(am__objects_2)
>  libgdbsupport_a_OBJECTS = $(am_libgdbsupport_a_OBJECTS)
>  AM_V_P = $(am__v_P_@AM_V@)
>  am__v_P_ = $(am__v_P_@AM_DEFAULT_V@)
> @@ -358,6 +360,7 @@ AM_CPPFLAGS = -I$(srcdir)/../include -I$(srcdir)/../gdb \
>  AM_CXXFLAGS = $(WARN_CFLAGS) $(WERROR_CFLAGS)
>  noinst_LIBRARIES = libgdbsupport.a
>  @SELFTEST_TRUE@selftest = selftest.cc
> +@HAVE_PIPE_OR_PIPE2_TRUE@eventpipe = event-pipe.cc
>  libgdbsupport_a_SOURCES = \
>      agent.cc \
>      btrace-common.cc \
> @@ -394,6 +397,7 @@ libgdbsupport_a_SOURCES = \
>      tdesc.cc \
>      thread-pool.cc \
>      xml-utils.cc \
> +    ${eventpipe} \
>      $(selftest)
>  
>  all: config.h
> @@ -476,6 +480,7 @@ distclean-compile:
>  @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/environ.Po@am__quote@
>  @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/errors.Po@am__quote@
>  @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/event-loop.Po@am__quote@
> +@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/event-pipe.Po@am__quote@
>  @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/fileio.Po@am__quote@
>  @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/filestuff.Po@am__quote@
>  @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/format.Po@am__quote@
> diff --git a/gdbsupport/configure b/gdbsupport/configure
> index a9dd02c5b72..f557c45c3f7 100755
> --- a/gdbsupport/configure
> +++ b/gdbsupport/configure
> @@ -626,6 +626,8 @@ LTLIBOBJS
>  LIBOBJS
>  WERROR_CFLAGS
>  WARN_CFLAGS
> +HAVE_PIPE_OR_PIPE2_FALSE
> +HAVE_PIPE_OR_PIPE2_TRUE
>  SELFTEST_FALSE
>  SELFTEST_TRUE
>  LTLIBIPT
> @@ -9910,6 +9912,15 @@ else
>  fi
>  
>  
> + if test x$ac_cv_func_pipe = xyes -o x$ac_cv_func_pipe2 = xyes ; then
> +  HAVE_PIPE_OR_PIPE2_TRUE=
> +  HAVE_PIPE_OR_PIPE2_FALSE='#'
> +else
> +  HAVE_PIPE_OR_PIPE2_TRUE='#'
> +  HAVE_PIPE_OR_PIPE2_FALSE=
> +fi
> +
> +
>  # Check the return and argument types of ptrace.
>  
>  
> @@ -10448,6 +10459,10 @@ if test -z "${SELFTEST_TRUE}" && test -z "${SELFTEST_FALSE}"; then
>    as_fn_error $? "conditional \"SELFTEST\" was never defined.
>  Usually this means the macro was only invoked conditionally." "$LINENO" 5
>  fi
> +if test -z "${HAVE_PIPE_OR_PIPE2_TRUE}" && test -z "${HAVE_PIPE_OR_PIPE2_FALSE}"; then
> +  as_fn_error $? "conditional \"HAVE_PIPE_OR_PIPE2\" was never defined.
> +Usually this means the macro was only invoked conditionally." "$LINENO" 5
> +fi
>  
>  : "${CONFIG_STATUS=./config.status}"
>  ac_write_fail=0
> diff --git a/gdbsupport/configure.ac b/gdbsupport/configure.ac
> index a8fcfe24c32..c0700753839 100644
> --- a/gdbsupport/configure.ac
> +++ b/gdbsupport/configure.ac
> @@ -53,6 +53,9 @@ GDB_AC_COMMON
>  GDB_AC_SELFTEST
>  AM_CONDITIONAL(SELFTEST, $enable_unittests)
>  
> +AM_CONDITIONAL(HAVE_PIPE_OR_PIPE2,
> +   [test x$ac_cv_func_pipe = xyes -o x$ac_cv_func_pipe2 = xyes ])
> +
>  # Check the return and argument types of ptrace.
>  GDB_AC_PTRACE
>  
> diff --git a/gdbsupport/event-pipe.cc b/gdbsupport/event-pipe.cc
> new file mode 100644
> index 00000000000..db9f9983cdb
> --- /dev/null
> +++ b/gdbsupport/event-pipe.cc
> @@ -0,0 +1,100 @@
> +/* Event pipe for GDB, the GNU debugger.
> +
> +   Copyright (C) 2021 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#include "gdbsupport/common-defs.h"
> +#include "gdbsupport/event-pipe.h"
> +#include "gdbsupport/filestuff.h"
> +
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <unistd.h>
> +
> +event_pipe::~event_pipe ()
> +{
> +  if (is_open ())
> +    close ();
> +}
> +
> +/* Create a new pipe.  */

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

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

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

This might be clearer if written as:

  if (is_open ())
    return false;

> +
> +  if (gdb_pipe_cloexec (m_fds) == -1)
> +    return false;
> +
> +  if (fcntl (m_fds[0], F_SETFL, O_NONBLOCK) == -1
> +      || fcntl (m_fds[1], F_SETFL, O_NONBLOCK) == -1) {

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

> +    close ();
> +    return false;
> +  }
> +
> +  return true;
> +}
> +
> +void
> +event_pipe::close ()
> +{
> +  ::close (m_fds[0]);
> +  ::close (m_fds[1]);
> +  m_fds[0] = -1;
> +  m_fds[1] = -1;
> +}
> +
> +/* Get rid of any pending events in the pipe.  */
> +
> +void
> +event_pipe::flush ()
> +{
> +  int ret;
> +  char buf;
> +
> +  do
> +    {
> +      ret = read (m_fds[0], &buf, 1);
> +    }
> +  while (ret >= 0 || (ret == -1 && errno == EINTR));
> +}
> +
> +/* Put something (anything, doesn't matter what, or how much) in event
> +   pipe, so that the select/poll in the event-loop realizes we have
> +   something to process.  */
> +
> +void
> +event_pipe::mark ()
> +{
> +  int ret;
> +
> +  /* It doesn't really matter what the pipe contains, as long we end
> +     up with something in it.  Might as well flush the previous
> +     left-overs.  */
> +  flush ();
> +
> +  do
> +    {
> +      ret = write (m_fds[1], "+", 1);
> +    }
> +  while (ret == -1 && errno == EINTR);
> +
> +  /* Ignore EAGAIN.  If the pipe is full, the event loop will already
> +     be awakened anyway.  */
> +}
> diff --git a/gdbsupport/event-pipe.h b/gdbsupport/event-pipe.h
> new file mode 100644
> index 00000000000..25301ec85df
> --- /dev/null
> +++ b/gdbsupport/event-pipe.h
> @@ -0,0 +1,56 @@
> +/* Event pipe for GDB, the GNU debugger.
> +
> +   Copyright (C) 2021 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#ifndef COMMON_EVENT_PIPE_H
> +#define COMMON_EVENT_PIPE_H
> +
> +/* An event pipe used as a waitable file in the event loop in place of
> +   some other event associated with a signal.  The handler for the
> +   signal marks the event pipe to force a wakeup in the event loop.
> +   This uses the well-known self-pipe trick.  */
> +
> +class event_pipe
> +{
> +public:
> +  event_pipe() = default;
> +  ~event_pipe();

I agree with Lancelot about using DISABLE_COPY_AND_ASSIGN here.

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

The GDB style seems to be to place single line functions like this on
the next line, as in:

  bool is_open () const
  { return m_fds[0] != -1; }

> +
> +  /* The file descriptor of the waitable file to use in the event
> +     loop.  */
> +  int event_fd () const { return m_fds[0]; }

And again.

Thanks,
Andrew


> +
> +  /* Flush the event pipe.  */
> +  void flush ();
> +
> +  /* Put something in the pipe, so the event loop wakes up.  */
> +  void mark ();
> +private:
> +  int m_fds[2] = { -1, -1 };
> +};
> +
> +#endif /* COMMON_EVENT_PIPE_H */
> -- 
> 2.31.1
>