[PATCH 0/4] Introduce scoped_ignore_signal & make it thread safe

[PATCH 0/4] Introduce scoped_ignore_signal & make it thread safe

[Pedro Alves]

For the Ctrl-C rework series I posted recently, I stared at code using
scoped_ignore_sigttou a lot, and it annoyed me that it isn't thread
safe, because it changes the signal's disposition.

Very recently, we got a new scoped_ignore_sigpipe class modelled on
scoped_ignore_sigttou, which made me want to fix this before it ever
becomes a (hard to debug) problem.  I mentioned this here:

 https://sourceware.org/pipermail/gdb-patches/2021-June/179958.html

This series then:

  - Moves scoped_ignore_sigttou to gdbsupport/.  This patch is
    actually included in my Ctrl-C series too, I just borrowed it from
    there.

  - Introduces a scoped_ignore_signal template, to be used by both
    scoped_ignore_sigpipe and scoped_ignore_sigttou.

  - Makes scoped_ignore_signal thread-safe, by using sigprocmask +
    sigtimedwait.

  - Adds a scoped_ignore_sigpipe unit test.

You can also find this in the users/palves/scoped_ignore_signal
branch.

Pedro Alves (4):
  Move scoped_ignore_sigttou to gdbsupport/
  Introduce scoped_restore_signal
  scoped_ignore_signal: Use sigprocmask+sigtimedwait instead of signal
  Add a unit test for scoped_ignore_sigpipe

 gdb/Makefile.in                               |   2 +-
 gdb/compile/compile.c                         |  29 +---
 gdb/inf-ptrace.c                              |   1 -
 gdb/inflow.c                                  |   2 +-
 gdb/procfs.c                                  |   1 -
 gdb/ser-unix.c                                |   2 +-
 .../scoped_ignore_signal-selftests.c          | 125 ++++++++++++++++++
 gdbsupport/scoped_ignore_signal.h             |  97 ++++++++++++++
 .../scoped_ignore_sigttou.h                   |  56 +++++---
 9 files changed, 266 insertions(+), 49 deletions(-)
 create mode 100644 gdb/unittests/scoped_ignore_signal-selftests.c
 create mode 100644 gdbsupport/scoped_ignore_signal.h
 rename gdb/inflow.h => gdbsupport/scoped_ignore_sigttou.h (51%)


base-commit: c8795e1f2f4d8617f22c3bd40dad75c75482e164
-- 
2.26.2

[PATCH 1/4] Move scoped_ignore_sigttou to gdbsupport/

[Pedro Alves]

A following patch will want to use scoped_ignore_sigttou in code
shared between GDB and GDBserver.  Move it under gdbsupport/.

Note that despite what inflow.h/inflow.c's first line says, inflow.c
is no longer about ptrace, it is about terminal management.  Some
other files were unnecessarily including inflow.h, I guess a leftover
from the days when inflow.c really was about ptrace.  Those inclusions
are simply dropped.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <pedro@palves.net>

	* Makefile.in (HFILES_NO_SRCDIR): Remove inflow.h.
	* inf-ptrace.c, inflow.c, procfs.c: Don't include "inflow.h".
	* inflow.h: Delete, moved to gdbsupport/ under a different name.
	* ser-unix.c: Don't include "inflow.h".  Include
	"gdbsupport/scoped_ignore_sigttou.h".

gdbsupport/ChangeLog:
yyyy-mm-dd  Pedro Alves  <pedro@palves.net>

	* scoped_ignore_sigttou.h: New file, moved from gdb/ and renamed.

Change-Id: Ie390abf42c3a78bec6d282ad2a63edd3e623559a
---
 gdb/Makefile.in                                    | 1 -
 gdb/inf-ptrace.c                                   | 1 -
 gdb/inflow.c                                       | 2 +-
 gdb/procfs.c                                       | 1 -
 gdb/ser-unix.c                                     | 2 +-
 gdb/inflow.h => gdbsupport/scoped_ignore_sigttou.h | 8 ++++----
 6 files changed, 6 insertions(+), 9 deletions(-)
 rename gdb/inflow.h => gdbsupport/scoped_ignore_sigttou.h (90%)

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index b3d264f267c..881ebde8fb0 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -1336,7 +1336,6 @@ HFILES_NO_SRCDIR = \
 	inf-ptrace.h \
 	infcall.h \
 	inferior.h \
-	inflow.h \
 	inline-frame.h \
 	interps.h \
 	jit.h \
diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
index b6fa71fd2c0..afa38de6ef7 100644
--- a/gdb/inf-ptrace.c
+++ b/gdb/inf-ptrace.c
@@ -20,7 +20,6 @@
 #include "defs.h"
 #include "command.h"
 #include "inferior.h"
-#include "inflow.h"
 #include "terminal.h"
 #include "gdbcore.h"
 #include "regcache.h"
diff --git a/gdb/inflow.c b/gdb/inflow.c
index d241540c4cd..f9917d6a81c 100644
--- a/gdb/inflow.c
+++ b/gdb/inflow.c
@@ -29,12 +29,12 @@
 #include <fcntl.h>
 #include "gdbsupport/gdb_select.h"
 
-#include "inflow.h"
 #include "gdbcmd.h"
 #ifdef HAVE_TERMIOS_H
 #include <termios.h>
 #endif
 #include "gdbsupport/job-control.h"
+#include "gdbsupport/scoped_ignore_sigttou.h"
 
 #ifdef HAVE_SYS_IOCTL_H
 #include <sys/ioctl.h>
diff --git a/gdb/procfs.c b/gdb/procfs.c
index 23c0aa22a7a..529ee33df90 100644
--- a/gdb/procfs.c
+++ b/gdb/procfs.c
@@ -40,7 +40,6 @@
 #include <signal.h>
 #include <ctype.h>
 #include "gdb_bfd.h"
-#include "inflow.h"
 #include "auxv.h"
 #include "procfs.h"
 #include "observable.h"
diff --git a/gdb/ser-unix.c b/gdb/ser-unix.c
index e97dc2f925d..96d024eea3d 100644
--- a/gdb/ser-unix.c
+++ b/gdb/ser-unix.c
@@ -32,7 +32,7 @@
 #include "gdbcmd.h"
 #include "gdbsupport/filestuff.h"
 #include <termios.h>
-#include "inflow.h"
+#include "gdbsupport/scoped_ignore_sigttou.h"
 
 struct hardwire_ttystate
   {
diff --git a/gdb/inflow.h b/gdbsupport/scoped_ignore_sigttou.h
similarity index 90%
rename from gdb/inflow.h
rename to gdbsupport/scoped_ignore_sigttou.h
index 8a671c7c4c7..a31316460b4 100644
--- a/gdb/inflow.h
+++ b/gdbsupport/scoped_ignore_sigttou.h
@@ -1,4 +1,4 @@
-/* Low level interface to ptrace, for GDB when running under Unix.
+/* Support for signoring SIGTTOU.
 
    Copyright (C) 2003-2021 Free Software Foundation, Inc.
 
@@ -17,8 +17,8 @@
    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 INFLOW_H
-#define INFLOW_H
+#ifndef SCOPED_IGNORE_SIGTTOU_H
+#define SCOPED_IGNORE_SIGTTOU_H
 
 #include <unistd.h>
 #include <signal.h>
@@ -53,4 +53,4 @@ class scoped_ignore_sigttou
 #endif
 };
 
-#endif /* inflow.h */
+#endif /* SCOPED_IGNORE_SIGTTOU_H */
-- 
2.26.2

[PATCH 2/4] Introduce scoped_restore_signal

[Pedro Alves]

We currently have scoped_restore_sigttou and scoped_restore_sigpipe
doing basically the same thing -- temporarily ignoring a specific
signal.

This patch introduce a scoped_restore_signal type that can be used for
both.  This will become more important for the next patch which
changes how the signal-ignoring is implemented.

scoped_restore_sigpipe is a straight alias to
scoped_restore_signal<SIGPIPE> on systems that define SIGPIPE, and an
alias to scoped_restore_signal_nop (a no-op version of
scoped_restore_signal) otherwise.

scoped_restore_sigttou is not a straight alias because it wants to
check the job_control global.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <pedro@palves.net>

	* gdbsupport/scoped_ignore_signal.h: New.
	* compile/compile.c: Include gdbsupport/scoped_ignore_signal.h
	instead of <signal.h>.  Don't include <unistd.h>.
	(scoped_ignore_sigpipe): Remove.
	* gdbsupport/scoped_ignore_sigttou.h: Include gdbsupport/scoped_ignore_signal.h
	instead of <signal.h>.  Don't include <unistd.h>.
	(lazy_init): New.
	(scoped_ignore_sigttou): Reimplement using scoped_ignore_signal
	and lazy_init.

Change-Id: Ibb44d0bd705e96df03ef0787c77358a4a7b7086c
---
 gdb/compile/compile.c              | 29 +-------------
 gdbsupport/scoped_ignore_signal.h  | 64 ++++++++++++++++++++++++++++++
 gdbsupport/scoped_ignore_sigttou.h | 48 ++++++++++++++++------
 3 files changed, 101 insertions(+), 40 deletions(-)
 create mode 100644 gdbsupport/scoped_ignore_signal.h

diff --git a/gdb/compile/compile.c b/gdb/compile/compile.c
index abbb72a393c..e815348ff07 100644
--- a/gdb/compile/compile.c
+++ b/gdb/compile/compile.c
@@ -43,7 +43,7 @@
 #include "gdbsupport/gdb_optional.h"
 #include "gdbsupport/gdb_unlinker.h"
 #include "gdbsupport/pathstuff.h"
-#include <signal.h>
+#include "gdbsupport/scoped_ignore_signal.h"
 
 \f
 
@@ -634,33 +634,6 @@ print_callback (void *ignore, const char *message)
   fputs_filtered (message, gdb_stderr);
 }
 
-/* RAII class used to ignore SIGPIPE in a scope.  */
-
-class scoped_ignore_sigpipe
-{
-public:
-  scoped_ignore_sigpipe ()
-  {
-#ifdef SIGPIPE
-    m_osigpipe = signal (SIGPIPE, SIG_IGN);
-#endif
-  }
-
-  ~scoped_ignore_sigpipe ()
-  {
-#ifdef SIGPIPE
-    signal (SIGPIPE, m_osigpipe);
-#endif
-  }
-
-  DISABLE_COPY_AND_ASSIGN (scoped_ignore_sigpipe);
-
-private:
-#ifdef SIGPIPE
-  sighandler_t m_osigpipe = NULL;
-#endif
-};
-
 /* Process the compilation request.  On success it returns the object
    and source file names.  On an error condition, error () is
    called.  */
diff --git a/gdbsupport/scoped_ignore_signal.h b/gdbsupport/scoped_ignore_signal.h
new file mode 100644
index 00000000000..cccd390529b
--- /dev/null
+++ b/gdbsupport/scoped_ignore_signal.h
@@ -0,0 +1,64 @@
+/* Support for ignoring signals.
+
+   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 SCOPED_IGNORE_SIGNAL_H
+#define SCOPED_IGNORE_SIGNAL_H
+
+#include <signal.h>
+
+/* RAII class used to ignore a signal in a scope.  */
+
+template <int Sig>
+class scoped_ignore_signal
+{
+public:
+  scoped_ignore_signal ()
+  {
+    m_osig = signal (Sig, SIG_IGN);
+  }
+
+  ~scoped_ignore_signal ()
+  {
+    signal (Sig, m_osig);
+  }
+
+  DISABLE_COPY_AND_ASSIGN (scoped_ignore_signal);
+
+private:
+  sighandler_t m_osig = nullptr;
+};
+
+struct scoped_ignore_signal_nop
+{
+  /* Note, these can't both be "= default", because otherwise the
+     compiler warns that variables of this type are not used.  */
+  scoped_ignore_signal_nop ()
+  {}
+  ~scoped_ignore_signal_nop ()
+  {}
+  DISABLE_COPY_AND_ASSIGN (scoped_ignore_signal_nop);
+};
+
+#ifdef SIGPIPE
+using scoped_ignore_sigpipe = scoped_ignore_signal<SIGPIPE>;
+#else
+using scoped_ignore_sigpipe = scoped_ignore_signal_nop;
+#endif
+
+#endif /* SCOPED_IGNORE_SIGNAL_H */
diff --git a/gdbsupport/scoped_ignore_sigttou.h b/gdbsupport/scoped_ignore_sigttou.h
index a31316460b4..7b29f2b79a3 100644
--- a/gdbsupport/scoped_ignore_sigttou.h
+++ b/gdbsupport/scoped_ignore_sigttou.h
@@ -20,37 +20,61 @@
 #ifndef SCOPED_IGNORE_SIGTTOU_H
 #define SCOPED_IGNORE_SIGTTOU_H
 
-#include <unistd.h>
-#include <signal.h>
+#include "gdbsupport/scoped_ignore_signal.h"
 #include "gdbsupport/job-control.h"
 
-/* RAII class used to ignore SIGTTOU in a scope.  */
+#ifdef SIGTTOU
+
+/* Simple wrapper that allows lazy initialization / destruction of T.
+   Slightly more efficient than gdb::optional, because it doesn't
+   carry storage to track whether the object has been initialized.  */
+template<typename T>
+class lazy_init
+{
+public:
+  void emplace ()
+  {
+    new (m_buf) T ();
+  }
+
+  void reset ()
+  {
+    reinterpret_cast <T *> (m_buf)->~T ();
+  }
+
+private:
+  alignas (T) gdb_byte m_buf[sizeof (T)];
+};
+
+/* RAII class used to ignore SIGTTOU in a scope.  This isn't simply
+   scoped_ignore_signal<SIGTTOU> because we want to check the
+   `job_control' global.  */
 
 class scoped_ignore_sigttou
 {
 public:
   scoped_ignore_sigttou ()
   {
-#ifdef SIGTTOU
     if (job_control)
-      m_osigttou = signal (SIGTTOU, SIG_IGN);
-#endif
+      m_ignore_signal.emplace ();
   }
 
   ~scoped_ignore_sigttou ()
   {
-#ifdef SIGTTOU
     if (job_control)
-      signal (SIGTTOU, m_osigttou);
-#endif
+      m_ignore_signal.reset ();
   }
 
   DISABLE_COPY_AND_ASSIGN (scoped_ignore_sigttou);
 
 private:
-#ifdef SIGTTOU
-  sighandler_t m_osigttou = NULL;
-#endif
+  lazy_init<scoped_ignore_signal<SIGTTOU>> m_ignore_signal;
 };
 
+#else
+
+using scoped_ignore_sigttou = scoped_ignore_signal_nop;
+
+#endif
+
 #endif /* SCOPED_IGNORE_SIGTTOU_H */
-- 
2.26.2

[PATCH 3/4] scoped_ignore_signal: Use sigprocmask+sigtimedwait instead of signal

[Pedro Alves]

The problem with using signal(...) to temporarily ignore a signal, is
that that changes the the signal disposition for the whole process.
If multiple threads do it at the same time, you have a race.

Fix this by using sigprocmask + sigtimedwait to implement the ignoring
instead, if available, which I think probably means everywhere except
Windows nowadays.  This way, we only change the signal mask for the
current thread, so there's no race.

Change-Id: Idfe3fb08327ef8cae926f3de9ee81c56a83b1738

gdbsupport/ChangeLog:
yyyy-mm-dd  Pedro Alves  <pedro@palves.net>

	* scoped_ignore_signal.h
	(scoped_ignore_signal::scoped_ignore_signal)
	[HAVE_SIGPROCMASK]: Use sigprocmask to block the signal instead of
	changing the signal disposition for the whole process.
	(scoped_ignore_signal::~scoped_ignore_signal) [HAVE_SIGPROCMASK]:
	Use sigtimedwait and sigprocmask to flush and unblock the signal.
---
 gdbsupport/scoped_ignore_signal.h | 37 +++++++++++++++++++++++++++++--
 1 file changed, 35 insertions(+), 2 deletions(-)

diff --git a/gdbsupport/scoped_ignore_signal.h b/gdbsupport/scoped_ignore_signal.h
index cccd390529b..55a921cb332 100644
--- a/gdbsupport/scoped_ignore_signal.h
+++ b/gdbsupport/scoped_ignore_signal.h
@@ -22,7 +22,10 @@
 
 #include <signal.h>
 
-/* RAII class used to ignore a signal in a scope.  */
+/* RAII class used to ignore a signal in a scope.  If sigprocmask is
+   supported, then the signal is only ignored by the calling thread.
+   Otherwise, the signal disposition is set to SIG_IGN, which affects
+   the whole process.  */
 
 template <int Sig>
 class scoped_ignore_signal
@@ -30,18 +33,48 @@ class scoped_ignore_signal
 public:
   scoped_ignore_signal ()
   {
+#ifdef HAVE_SIGPROCMASK
+    sigset_t set, old_state;
+
+    sigemptyset (&set);
+    sigaddset (&set, Sig);
+    sigprocmask (SIG_BLOCK, &set, &old_state);
+    m_was_blocked = sigismember (&old_state, Sig);
+#else
     m_osig = signal (Sig, SIG_IGN);
+#endif
   }
 
   ~scoped_ignore_signal ()
   {
+#ifdef HAVE_SIGPROCMASK
+    if (!m_was_blocked)
+      {
+	sigset_t set;
+	const timespec zero_timeout = {};
+
+	sigemptyset (&set);
+	sigaddset (&set, Sig);
+
+	/* If we got a pending Sig signal, consume it before
+	   unblocking.  */
+	sigtimedwait (&set, nullptr, &zero_timeout);
+
+	sigprocmask (SIG_UNBLOCK, &set, nullptr);
+      }
+#else
     signal (Sig, m_osig);
+#endif
   }
 
   DISABLE_COPY_AND_ASSIGN (scoped_ignore_signal);
 
 private:
-  sighandler_t m_osig = nullptr;
+#ifdef HAVE_SIGPROCMASK
+  bool m_was_blocked;
+#else
+  sighandler_t m_osig;
+#endif
 };
 
 struct scoped_ignore_signal_nop
-- 
2.26.2

[PATCH 4/4] Add a unit test for scoped_ignore_sigpipe

[Pedro Alves]

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <pedro@palves.net>

	* Makefile.in (SELFTESTS_SRCS): Add
	unittests/scoped_ignore_signal-selftests.c.
	* unittests/scoped_ignore_signal-selftests.c: New.

Change-Id: Idce24aa9432a3f1eb7065bc9aa030b1d0d7dcad5
---
 gdb/Makefile.in                               |   1 +
 .../scoped_ignore_signal-selftests.c          | 125 ++++++++++++++++++
 2 files changed, 126 insertions(+)
 create mode 100644 gdb/unittests/scoped_ignore_signal-selftests.c

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 881ebde8fb0..1bc97885536 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -461,6 +461,7 @@ SELFTESTS_SRCS = \
 	unittests/mkdir-recursive-selftests.c \
 	unittests/rsp-low-selftests.c \
 	unittests/scoped_fd-selftests.c \
+	unittests/scoped_ignore_signal-selftests.c \
 	unittests/scoped_mmap-selftests.c \
 	unittests/scoped_restore-selftests.c \
 	unittests/search-memory-selftests.c \
diff --git a/gdb/unittests/scoped_ignore_signal-selftests.c b/gdb/unittests/scoped_ignore_signal-selftests.c
new file mode 100644
index 00000000000..f727b464567
--- /dev/null
+++ b/gdb/unittests/scoped_ignore_signal-selftests.c
@@ -0,0 +1,125 @@
+/* Self tests for scoped_ignored_signal 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 "defs.h"
+#include "gdbsupport/scoped_ignore_signal.h"
+#include "gdbsupport/selftest.h"
+#include "gdbsupport/scope-exit.h"
+#include <unistd.h>
+#include <signal.h>
+
+namespace selftests {
+namespace scoped_ignore_sig {
+
+#ifdef SIGPIPE
+
+/* True if the SIGPIPE handler ran.  */
+static sig_atomic_t got_sigpipe = 0;
+
+/* SIGPIPE handler for testing.  */
+
+static void
+handle_sigpipe (int)
+{
+  got_sigpipe = 1;
+}
+
+/* Test scoped_ignore_sigpipe.  */
+
+static void
+test_sigpipe ()
+{
+  auto *osig = signal (SIGPIPE, handle_sigpipe);
+  SCOPE_EXIT { signal (SIGPIPE, osig); };
+
+#ifdef HAVE_SIGPROCMASK
+  /* Make sure SIGPIPE isn't blocked.  */
+  sigset_t set, old_state;
+  sigemptyset (&set);
+  sigaddset (&set, SIGPIPE);
+  sigprocmask (SIG_UNBLOCK, &set, &old_state);
+  SCOPE_EXIT { sigprocmask (SIG_SETMASK, &old_state, nullptr); };
+#endif
+
+  /* Create pipe, and close read end so that writes to the pipe fail
+     with EPIPE.  */
+
+  int fd[2];
+  char c = 0xff;
+  int r;
+
+  r = pipe (fd);
+  SELF_CHECK (r == 0);
+
+  close (fd[0]); SCOPE_EXIT { close (fd[1]); };
+
+  /* Check that writing to the pipe results in EPIPE.  EXPECT_SIG
+     indicates whether a SIGPIPE signal is expected.  */
+  auto check_pipe_write = [&] (bool expect_sig)
+  {
+    got_sigpipe = 0;
+    errno = 0;
+
+    r = write (fd[1], &c, 1);
+    SELF_CHECK (r == -1 && errno == EPIPE
+		&& got_sigpipe == expect_sig);
+  };
+
+  /* Check that without a scoped_ignore_sigpipe in scope we indeed get
+     a SIGPIPE signal.  */
+  check_pipe_write (true);
+
+  /* Now check that with a scoped_ignore_sigpipe in scope, SIGPIPE is
+     ignored/blocked.  */
+  {
+    scoped_ignore_sigpipe ignore1;
+
+    check_pipe_write (false);
+
+    /* Check that scoped_ignore_sigpipe nests correctly.  */
+    {
+      scoped_ignore_sigpipe ignore2;
+
+      check_pipe_write (false);
+    }
+
+    /* If nesting works correctly, this write results in no
+       SIGPIPE.  */
+    check_pipe_write (false);
+  }
+
+  /* No scoped_ignore_sigpipe is is scope anymore, so this should
+     result in a SIGPIPE signal.  */
+  check_pipe_write (true);
+}
+
+#endif /* SIGPIPE */
+
+} /* namespace scoped_ignore_sig */
+} /* namespace selftests */
+
+void _initialize_scoped_ignore_signal_selftests ();
+void
+_initialize_scoped_ignore_signal_selftests ()
+{
+#ifdef SIGPIPE
+  selftests::register_test ("scoped_ignore_sigpipe",
+			    selftests::scoped_ignore_sig::test_sigpipe);
+#endif
+}
-- 
2.26.2

Re: [PATCH 0/4] Introduce scoped_ignore_signal & make it thread safe

[John Baldwin]

On 6/15/21 4:14 AM, Pedro Alves wrote:
> For the Ctrl-C rework series I posted recently, I stared at code using
> scoped_ignore_sigttou a lot, and it annoyed me that it isn't thread
> safe, because it changes the signal's disposition.
> 
> Very recently, we got a new scoped_ignore_sigpipe class modelled on
> scoped_ignore_sigttou, which made me want to fix this before it ever
> becomes a (hard to debug) problem.  I mentioned this here:
> 
>   https://sourceware.org/pipermail/gdb-patches/2021-June/179958.html

I think this looks ok to me.  The one difference I can think of when
using sigpromask() instead of SIG_IGN is if the signal might be
sent to another thread instead of masked.  However, both SIGPIPE
and SIGTTOU sent in the context of write() should be sent to the
specific thread calling write().  SIGTTOU is a bit funky in that
it gets raised to the entire process group, but in FreeBSD at least
we only raise it if it is not masked for the current thread that
has invoked write().  (If the signal is masked, it isn't raised at
all.)

-- 
John Baldwin

Re: [PATCH 0/4] Introduce scoped_ignore_signal & make it thread safe

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:

Pedro> This series then:

Pedro>   - Moves scoped_ignore_sigttou to gdbsupport/.  This patch is
Pedro>     actually included in my Ctrl-C series too, I just borrowed it from
Pedro>     there.

Pedro>   - Introduces a scoped_ignore_signal template, to be used by both
Pedro>     scoped_ignore_sigpipe and scoped_ignore_sigttou.

Pedro>   - Makes scoped_ignore_signal thread-safe, by using sigprocmask +
Pedro>     sigtimedwait.

Pedro>   - Adds a scoped_ignore_sigpipe unit test.

I read through these and it all looks reasonable to me.
Thanks for doing this.

Tom

Re: [PATCH 4/4] Add a unit test for scoped_ignore_sigpipe

[Lancelot SIX]

Hi,

There is a tiny typo (see above).

Lancelot.

On Tue, Jun 15, 2021 at 12:14:29PM +0100, Pedro Alves wrote:
> gdb/ChangeLog:
> yyyy-mm-dd  Pedro Alves  <pedro@palves.net>
> 
> 	* Makefile.in (SELFTESTS_SRCS): Add
> 	unittests/scoped_ignore_signal-selftests.c.
> 	* unittests/scoped_ignore_signal-selftests.c: New.
> 
> Change-Id: Idce24aa9432a3f1eb7065bc9aa030b1d0d7dcad5
> ---
>  gdb/Makefile.in                               |   1 +
>  .../scoped_ignore_signal-selftests.c          | 125 ++++++++++++++++++
>  2 files changed, 126 insertions(+)
>  create mode 100644 gdb/unittests/scoped_ignore_signal-selftests.c
> 
> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index 881ebde8fb0..1bc97885536 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -461,6 +461,7 @@ SELFTESTS_SRCS = \
>  	unittests/mkdir-recursive-selftests.c \
>  	unittests/rsp-low-selftests.c \
>  	unittests/scoped_fd-selftests.c \
> +	unittests/scoped_ignore_signal-selftests.c \
>  	unittests/scoped_mmap-selftests.c \
>  	unittests/scoped_restore-selftests.c \
>  	unittests/search-memory-selftests.c \
> diff --git a/gdb/unittests/scoped_ignore_signal-selftests.c b/gdb/unittests/scoped_ignore_signal-selftests.c
> new file mode 100644
> index 00000000000..f727b464567
> --- /dev/null
> +++ b/gdb/unittests/scoped_ignore_signal-selftests.c
> @@ -0,0 +1,125 @@
> +/* Self tests for scoped_ignored_signal 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 "defs.h"
> +#include "gdbsupport/scoped_ignore_signal.h"
> +#include "gdbsupport/selftest.h"
> +#include "gdbsupport/scope-exit.h"
> +#include <unistd.h>
> +#include <signal.h>
> +
> +namespace selftests {
> +namespace scoped_ignore_sig {
> +
> +#ifdef SIGPIPE
> +
> +/* True if the SIGPIPE handler ran.  */
> +static sig_atomic_t got_sigpipe = 0;
> +
> +/* SIGPIPE handler for testing.  */
> +
> +static void
> +handle_sigpipe (int)
> +{
> +  got_sigpipe = 1;
> +}
> +
> +/* Test scoped_ignore_sigpipe.  */
> +
> +static void
> +test_sigpipe ()
> +{
> +  auto *osig = signal (SIGPIPE, handle_sigpipe);
> +  SCOPE_EXIT { signal (SIGPIPE, osig); };
> +
> +#ifdef HAVE_SIGPROCMASK
> +  /* Make sure SIGPIPE isn't blocked.  */
> +  sigset_t set, old_state;
> +  sigemptyset (&set);
> +  sigaddset (&set, SIGPIPE);
> +  sigprocmask (SIG_UNBLOCK, &set, &old_state);
> +  SCOPE_EXIT { sigprocmask (SIG_SETMASK, &old_state, nullptr); };
> +#endif
> +
> +  /* Create pipe, and close read end so that writes to the pipe fail
> +     with EPIPE.  */
> +
> +  int fd[2];
> +  char c = 0xff;
> +  int r;
> +
> +  r = pipe (fd);
> +  SELF_CHECK (r == 0);
> +
> +  close (fd[0]); SCOPE_EXIT { close (fd[1]); };
> +
> +  /* Check that writing to the pipe results in EPIPE.  EXPECT_SIG
> +     indicates whether a SIGPIPE signal is expected.  */
> +  auto check_pipe_write = [&] (bool expect_sig)
> +  {
> +    got_sigpipe = 0;
> +    errno = 0;
> +
> +    r = write (fd[1], &c, 1);
> +    SELF_CHECK (r == -1 && errno == EPIPE
> +		&& got_sigpipe == expect_sig);
> +  };
> +
> +  /* Check that without a scoped_ignore_sigpipe in scope we indeed get
> +     a SIGPIPE signal.  */
> +  check_pipe_write (true);
> +
> +  /* Now check that with a scoped_ignore_sigpipe in scope, SIGPIPE is
> +     ignored/blocked.  */
> +  {
> +    scoped_ignore_sigpipe ignore1;
> +
> +    check_pipe_write (false);
> +
> +    /* Check that scoped_ignore_sigpipe nests correctly.  */
> +    {
> +      scoped_ignore_sigpipe ignore2;
> +
> +      check_pipe_write (false);
> +    }
> +
> +    /* If nesting works correctly, this write results in no
> +       SIGPIPE.  */
> +    check_pipe_write (false);
> +  }
> +
> +  /* No scoped_ignore_sigpipe is is scope anymore, so this should

s/is is/is in/

> +     result in a SIGPIPE signal.  */
> +  check_pipe_write (true);
> +}
> +
> +#endif /* SIGPIPE */
> +
> +} /* namespace scoped_ignore_sig */
> +} /* namespace selftests */
> +
> +void _initialize_scoped_ignore_signal_selftests ();
> +void
> +_initialize_scoped_ignore_signal_selftests ()
> +{
> +#ifdef SIGPIPE
> +  selftests::register_test ("scoped_ignore_sigpipe",
> +			    selftests::scoped_ignore_sig::test_sigpipe);
> +#endif
> +}
> -- 
> 2.26.2
> 

Re: [PATCH 4/4] Add a unit test for scoped_ignore_sigpipe

[Pedro Alves]

On 2021-06-15 11:26 p.m., Lancelot SIX wrote:
> Hi,

Hi!

> 
> There is a tiny typo (see above).

>> +  /* No scoped_ignore_sigpipe is is scope anymore, so this should
> 
> s/is is/is in/

Thanks, I fixed this.

Re: [PATCH 0/4] Introduce scoped_ignore_signal & make it thread safe

[Pedro Alves]

On 2021-06-15 6:04 p.m., John Baldwin wrote:
> On 6/15/21 4:14 AM, Pedro Alves wrote:
>> For the Ctrl-C rework series I posted recently, I stared at code using
>> scoped_ignore_sigttou a lot, and it annoyed me that it isn't thread
>> safe, because it changes the signal's disposition.
>>
>> Very recently, we got a new scoped_ignore_sigpipe class modelled on
>> scoped_ignore_sigttou, which made me want to fix this before it ever
>> becomes a (hard to debug) problem.  I mentioned this here:
>>
>>   https://sourceware.org/pipermail/gdb-patches/2021-June/179958.html
> 
> I think this looks ok to me.  The one difference I can think of when
> using sigpromask() instead of SIG_IGN is if the signal might be
> sent to another thread instead of masked.  However, both SIGPIPE
> and SIGTTOU sent in the context of write() should be sent to the
> specific thread calling write().  SIGTTOU is a bit funky in that
> it gets raised to the entire process group, but in FreeBSD at least
> we only raise it if it is not masked for the current thread that
> has invoked write().  (If the signal is masked, it isn't raised at
> all.)

Yeah, I was a bit worried about that too, but what I observe on Linux
is the same as what you're describing for FreeBSD.

 - SIGTTOU isn't sent at all if the thread touching the terminal has it masked.

 - SIGPIPE is sent to the thread that writes and remains pending (hence
   the need for sigtimedwait).

     (confirmed that it was sent to the thread and now the process by inspecting
      the Sig* entries in /proc/TID/status.)

To double check that the kernel wouldn't send the SIGTTOU signal if some other
thread had it unblocked, I hacked GDB to make sure all GDB threads start
with signals unblocked:

--- c/gdbsupport/thread-pool.cc
+++ w/gdbsupport/thread-pool.cc
@@ -100,7 +100,7 @@ thread_pool::set_thread_count (size_t num_threads)
     {
       /* Ensure that signals used by gdb are blocked in the new
         threads.  */
-      block_signals blocker;
+      // block_signals blocker;

(and then confirmed by inspecting the Sig* entries in /proc/TID/status.)

If SIGTTOU ends up raised by some other thread while the main thread has it blocked
in scoped_ignore_sigttou, and if it is sent to the whole process, there's a small time window
where the sigtimedwait in the main thread can eat the signal.  That still seems better than
the current status where the end result can be that we can end up with the signal enabled
instead of ignored and then gdb backgrounds itself.  This is of course highly theoretical since I
expect that gdb threads run with signals blocked from the get go.  But you never know what
Python code does.

Re: [PATCH 2/4] Introduce scoped_restore_signal

[Pedro Alves]

On 2021-06-15 12:14 p.m., Pedro Alves wrote:
> +/* Simple wrapper that allows lazy initialization / destruction of T.
> +   Slightly more efficient than gdb::optional, because it doesn't
> +   carry storage to track whether the object has been initialized.  */
> +template<typename T>
> +class lazy_init
> +{
> +public:
> +  void emplace ()
> +  {
> +    new (m_buf) T ();
> +  }
> +
> +  void reset ()
> +  {
> +    reinterpret_cast <T *> (m_buf)->~T ();
> +  }
> +
> +private:
> +  alignas (T) gdb_byte m_buf[sizeof (T)];
> +};
> +

I was looking at this again, and realized that a raw buffer like that isn't super
convenient when you're debugging gdb and want to inspect T (you'll need to cast).

So I made it a union instead (see below).  It has the added advantage that no cast
is necessary.

I'm going to push the series with this change.

diff --git c/gdbsupport/scoped_ignore_sigttou.h w/gdbsupport/scoped_ignore_sigttou.h
index 7b29f2b79a3..1fc8f80d7fd 100644
--- c/gdbsupport/scoped_ignore_sigttou.h
+++ w/gdbsupport/scoped_ignore_sigttou.h
@@ -34,16 +34,23 @@ class lazy_init
 public:
   void emplace ()
   {
-    new (m_buf) T ();
+    new (&m_u.obj) T ();
   }
 
   void reset ()
   {
-    reinterpret_cast <T *> (m_buf)->~T ();
+    m_u.obj.~T ();
   }
 
 private:
-  alignas (T) gdb_byte m_buf[sizeof (T)];
+  union u
+  {
+    /* Must define ctor/dtor if T has non-trivial ctor/dtor.  */
+    u () {}
+    ~u () {}
+
+    T obj;
+  } m_u;
 };

Re: [PATCH 0/4] Introduce scoped_ignore_signal & make it thread safe

[Pedro Alves]

On 2021-06-17 3:38 p.m., Pedro Alves wrote:
> On 2021-06-15 6:04 p.m., John Baldwin wrote:

> If SIGTTOU ends up raised by some other thread while the main thread has it blocked
> in scoped_ignore_sigttou, and if it is sent to the whole process, there's a small time window
> where the sigtimedwait in the main thread can eat the signal.  That still seems better than
> the current status where the end result can be that we can end up with the signal enabled
> instead of ignored and then gdb backgrounds itself.  This is of course highly theoretical since I
> expect that gdb threads run with signals blocked from the get go.  But you never know what
> Python code does.
> 

Hmm, I just thought of an easy fix for that.  Just don't consume the pending
signal in SIGTTOU's case.  I wonder how portable is this?  WDYT?

I'll see if I can try this on Solaris.

From 7f9288f1c3c4f184ebc08dd56406de0c13f5246a Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Thu, 17 Jun 2021 16:23:03 +0100
Subject: [PATCH] No sigtimedwait for SIGTTOU

Change-Id: I92f754dbc45c45819dce2ce68b8c067d8d5c61b1
---
 gdbsupport/scoped_ignore_signal.h  | 18 +++++++++++++-----
 gdbsupport/scoped_ignore_sigttou.h |  2 +-
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/gdbsupport/scoped_ignore_signal.h b/gdbsupport/scoped_ignore_signal.h
index 55a921cb332..a14c96779bf 100644
--- a/gdbsupport/scoped_ignore_signal.h
+++ b/gdbsupport/scoped_ignore_signal.h
@@ -25,9 +25,16 @@
 /* RAII class used to ignore a signal in a scope.  If sigprocmask is
    supported, then the signal is only ignored by the calling thread.
    Otherwise, the signal disposition is set to SIG_IGN, which affects
-   the whole process.  */
-
-template <int Sig>
+   the whole process.  If ConsumePending is true, the destructor
+   consumes a pending Sig.  SIGPIPE for example is queued on the
+   thread even if blocked at the time the pipe is written to.  SIGTTOU
+   OTOH is not raised at all if the thread writing to the terminal has
+   it blocked.  Because SIGTTOU is sent to the whole process instead
+   of to a specific thread, consuming a pending SIGTTOU in the
+   destructor could consume a signal raised due to actions done by
+   some other thread.  */
+
+template <int Sig, bool ConsumePending>
 class scoped_ignore_signal
 {
 public:
@@ -58,7 +65,8 @@ class scoped_ignore_signal
 
 	/* If we got a pending Sig signal, consume it before
 	   unblocking.  */
-	sigtimedwait (&set, nullptr, &zero_timeout);
+	if (ConsumePending)
+	  sigtimedwait (&set, nullptr, &zero_timeout);
 
 	sigprocmask (SIG_UNBLOCK, &set, nullptr);
       }
@@ -89,7 +97,7 @@ struct scoped_ignore_signal_nop
 };
 
 #ifdef SIGPIPE
-using scoped_ignore_sigpipe = scoped_ignore_signal<SIGPIPE>;
+using scoped_ignore_sigpipe = scoped_ignore_signal<SIGPIPE, true>;
 #else
 using scoped_ignore_sigpipe = scoped_ignore_signal_nop;
 #endif
diff --git a/gdbsupport/scoped_ignore_sigttou.h b/gdbsupport/scoped_ignore_sigttou.h
index 1fc8f80d7fd..5695c5db905 100644
--- a/gdbsupport/scoped_ignore_sigttou.h
+++ b/gdbsupport/scoped_ignore_sigttou.h
@@ -75,7 +75,7 @@ class scoped_ignore_sigttou
   DISABLE_COPY_AND_ASSIGN (scoped_ignore_sigttou);
 
 private:
-  lazy_init<scoped_ignore_signal<SIGTTOU>> m_ignore_signal;
+  lazy_init<scoped_ignore_signal<SIGTTOU, false>> m_ignore_signal;
 };
 
 #else

base-commit: 2af6d46fd331b8e632bb9245614bad0c974392a4
-- 
2.26.2

Re: [PATCH 0/4] Introduce scoped_ignore_signal & make it thread safe

[John Baldwin]

On 6/17/21 8:36 AM, Pedro Alves wrote:
> On 2021-06-17 3:38 p.m., Pedro Alves wrote:
>> On 2021-06-15 6:04 p.m., John Baldwin wrote:
> 
>> If SIGTTOU ends up raised by some other thread while the main thread has it blocked
>> in scoped_ignore_sigttou, and if it is sent to the whole process, there's a small time window
>> where the sigtimedwait in the main thread can eat the signal.  That still seems better than
>> the current status where the end result can be that we can end up with the signal enabled
>> instead of ignored and then gdb backgrounds itself.  This is of course highly theoretical since I
>> expect that gdb threads run with signals blocked from the get go.  But you never know what
>> Python code does.
>>
> 
> Hmm, I just thought of an easy fix for that.  Just don't consume the pending
> signal in SIGTTOU's case.  I wonder how portable is this?  WDYT?
> 
> I'll see if I can try this on Solaris.

I think this is ok.  I suspect that some of the rules around SIGTTOU are in POSIX as I
think job control is in POSIX, so (hopefully) the behavior there is fairly uniform.
I strongly suspect the other BSD's all follow FreeBSD at least, so if Solaris is the
same I'd feel pretty good about this behavior being consistent.

-- 
John Baldwin

[pushed] Don't call sigtimedwait for scoped_ignore_sigttou

[Pedro Alves]

On 2021-06-17 5:45 p.m., John Baldwin wrote:
> On 6/17/21 8:36 AM, Pedro Alves wrote:
>> On 2021-06-17 3:38 p.m., Pedro Alves wrote:
>>> On 2021-06-15 6:04 p.m., John Baldwin wrote:
>>
>>> If SIGTTOU ends up raised by some other thread while the main thread has it blocked
>>> in scoped_ignore_sigttou, and if it is sent to the whole process, there's a small time window
>>> where the sigtimedwait in the main thread can eat the signal.  That still seems better than
>>> the current status where the end result can be that we can end up with the signal enabled
>>> instead of ignored and then gdb backgrounds itself.  This is of course highly theoretical since I
>>> expect that gdb threads run with signals blocked from the get go.  But you never know what
>>> Python code does.
>>>
>>
>> Hmm, I just thought of an easy fix for that.  Just don't consume the pending
>> signal in SIGTTOU's case.  I wonder how portable is this?  WDYT?
>>
>> I'll see if I can try this on Solaris.
> 
> I think this is ok.  I suspect that some of the rules around SIGTTOU are in POSIX as I
> think job control is in POSIX, so (hopefully) the behavior there is fairly uniform.
> I strongly suspect the other BSD's all follow FreeBSD at least, so if Solaris is the
> same I'd feel pretty good about this behavior being consistent.
> 

I tested this on the Solaris 11.3 SPARC machine on the compile farm (gcc211).  I did
not get any spurious SIGTTOU so it must mean the signal wasn't sent at all there either.

I tried to test on AIX (gcc119 on the compile farm), but the build is broken there due
to PR 27123, and even with an evil hack to get past that, for some reason I couldn't
get GDB to build against the GMP on the system.  So I gave up...

I did notice that the "maint selftest scoped_ignore_sigpipe" test fails on Solaris,
bitten by BSD signal semantics, which I forgot to handle...  I'll fix that in a bit.

Here's the patch that I merged.  Thanks for the help John.

From 336b30e58ae98fe66862ab8480d3f7bb885fef23 Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Thu, 17 Jun 2021 16:23:03 +0100
Subject: [PATCH] Don't call sigtimedwait for scoped_ignore_sigttou

Because SIGTTOU is sent to the whole process instead of to a specific
thread, consuming a pending SIGTTOU in the destructor of
scoped_ignore_sigttou could consume a SIGTTOU signal raised due to
actions done by some other thread.  Simply avoid sigtimedwait in
scoped_ignore_sigttou, thus plugging the race.  This works because we
know that when the thread writes to the terminal and the signal is
blocked, the kernel does not raise the signal at all.

Tested on GNU/Linux, Solaris 11 and FreeBSD.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <pedro@palves.net>

	* scoped_ignore_signal.h (scoped_ignore_signal): Add
	ConsumePending template parameter.
	(scoped_ignore_signal::~scoped_ignore_signal): Skip calling
	sigtimedwait if ConsumePending is false.
	(scoped_ignore_sigpipe): Initialize with ConsumePending=true.
	* scoped_ignore_sigttou.h (scoped_ignore_sigttou)
	<m_ignore_signal>: Initialize with ConsumePending=false.

Change-Id: I92f754dbc45c45819dce2ce68b8c067d8d5c61b1
---
 gdb/ChangeLog                      | 10 ++++++++++
 gdbsupport/scoped_ignore_signal.h  | 18 +++++++++++++-----
 gdbsupport/scoped_ignore_sigttou.h |  2 +-
 3 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index e4e58173ee6..c70f6ef5329 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,13 @@
+2021-06-17  Pedro Alves  <pedro@palves.net>
+
+	* scoped_ignore_signal.h (scoped_ignore_signal): Add
+	ConsumePending template parameter.
+	(scoped_ignore_signal::~scoped_ignore_signal): Skip calling
+	sigtimedwait if ConsumePending is false.
+	(scoped_ignore_sigpipe): Initialize with ConsumePending=true.
+	* scoped_ignore_sigttou.h (scoped_ignore_sigttou)
+	<m_ignore_signal>: Initialize with ConsumePending=false.
+
 2021-06-17  Pedro Alves  <pedro@palves.net>
 
 	* Makefile.in (SELFTESTS_SRCS): Add
diff --git a/gdbsupport/scoped_ignore_signal.h b/gdbsupport/scoped_ignore_signal.h
index 55a921cb332..a14c96779bf 100644
--- a/gdbsupport/scoped_ignore_signal.h
+++ b/gdbsupport/scoped_ignore_signal.h
@@ -25,9 +25,16 @@
 /* RAII class used to ignore a signal in a scope.  If sigprocmask is
    supported, then the signal is only ignored by the calling thread.
    Otherwise, the signal disposition is set to SIG_IGN, which affects
-   the whole process.  */
-
-template <int Sig>
+   the whole process.  If ConsumePending is true, the destructor
+   consumes a pending Sig.  SIGPIPE for example is queued on the
+   thread even if blocked at the time the pipe is written to.  SIGTTOU
+   OTOH is not raised at all if the thread writing to the terminal has
+   it blocked.  Because SIGTTOU is sent to the whole process instead
+   of to a specific thread, consuming a pending SIGTTOU in the
+   destructor could consume a signal raised due to actions done by
+   some other thread.  */
+
+template <int Sig, bool ConsumePending>
 class scoped_ignore_signal
 {
 public:
@@ -58,7 +65,8 @@ class scoped_ignore_signal
 
 	/* If we got a pending Sig signal, consume it before
 	   unblocking.  */
-	sigtimedwait (&set, nullptr, &zero_timeout);
+	if (ConsumePending)
+	  sigtimedwait (&set, nullptr, &zero_timeout);
 
 	sigprocmask (SIG_UNBLOCK, &set, nullptr);
       }
@@ -89,7 +97,7 @@ struct scoped_ignore_signal_nop
 };
 
 #ifdef SIGPIPE
-using scoped_ignore_sigpipe = scoped_ignore_signal<SIGPIPE>;
+using scoped_ignore_sigpipe = scoped_ignore_signal<SIGPIPE, true>;
 #else
 using scoped_ignore_sigpipe = scoped_ignore_signal_nop;
 #endif
diff --git a/gdbsupport/scoped_ignore_sigttou.h b/gdbsupport/scoped_ignore_sigttou.h
index 1fc8f80d7fd..5695c5db905 100644
--- a/gdbsupport/scoped_ignore_sigttou.h
+++ b/gdbsupport/scoped_ignore_sigttou.h
@@ -75,7 +75,7 @@ class scoped_ignore_sigttou
   DISABLE_COPY_AND_ASSIGN (scoped_ignore_sigttou);
 
 private:
-  lazy_init<scoped_ignore_signal<SIGTTOU>> m_ignore_signal;
+  lazy_init<scoped_ignore_signal<SIGTTOU, false>> m_ignore_signal;
 };
 
 #else

base-commit: e013d20dc73b40d4b70c4a366c9adc619503e66b
-- 
2.26.2

Re: [PATCH 3/4] scoped_ignore_signal: Use sigprocmask+sigtimedwait instead of signal

[Simon Marchi]

On 2021-06-15 7:14 a.m., Pedro Alves wrote:
> The problem with using signal(...) to temporarily ignore a signal, is
> that that changes the the signal disposition for the whole process.
> If multiple threads do it at the same time, you have a race.
> 
> Fix this by using sigprocmask + sigtimedwait to implement the ignoring
> instead, if available, which I think probably means everywhere except
> Windows nowadays.  This way, we only change the signal mask for the
> current thread, so there's no race.

I tried to build on macOS today and got:


      CXX    compile/compile.o
    In file included from /Users/smarchi/src/binutils-gdb/gdb/compile/compile.c:46:
    /Users/smarchi/src/binutils-gdb/gdb/../gdbsupport/scoped_ignore_signal.h:69:4: error: use of undeclared identifier 'sigtimedwait'
              sigtimedwait (&set, nullptr, &zero_timeout);
              ^

I didn't have time to dig yet.

Simon

Re: [PATCH 3/4] scoped_ignore_signal: Use sigprocmask+sigtimedwait instead of signal

[John Baldwin]

On 6/26/21 5:35 AM, Simon Marchi via Gdb-patches wrote:
> On 2021-06-15 7:14 a.m., Pedro Alves wrote:
>> The problem with using signal(...) to temporarily ignore a signal, is
>> that that changes the the signal disposition for the whole process.
>> If multiple threads do it at the same time, you have a race.
>>
>> Fix this by using sigprocmask + sigtimedwait to implement the ignoring
>> instead, if available, which I think probably means everywhere except
>> Windows nowadays.  This way, we only change the signal mask for the
>> current thread, so there's no race.
> 
> I tried to build on macOS today and got:
> 
> 
>        CXX    compile/compile.o
>      In file included from /Users/smarchi/src/binutils-gdb/gdb/compile/compile.c:46:
>      /Users/smarchi/src/binutils-gdb/gdb/../gdbsupport/scoped_ignore_signal.h:69:4: error: use of undeclared identifier 'sigtimedwait'
>                sigtimedwait (&set, nullptr, &zero_timeout);
>                ^
> 
> I didn't have time to dig yet.

It looks like macOS doesn't implement either sigtimedwait() or sigwaitinfo()
which are part of POSIX from 1996 (*sigh*); however, macOS does provide
sigpending() and sigwait(), so perhaps we can do something like:

    if (ConsumePending)
      {
#ifdef HAVE_SIGTIMEDWAIT
        const timespec zero_timeout = {};

        sigtimedwait (&set, nullptr, &zero_timeout);
#else
        sigset_t pending;

        sigpending (&pending);
        if (sigismember (&pending, SIG))
          sigwait (&set, nullptr);
#endif
      }

(Ironically, sigwait() was added to POSIX in the same version of the spec as
sigtimedwait().)

-- 
John Baldwin

Re: [PATCH 3/4] scoped_ignore_signal: Use sigprocmask+sigtimedwait instead of signal

[Simon Marchi]

On 2021-06-26 9:41 a.m., John Baldwin wrote:
> On 6/26/21 5:35 AM, Simon Marchi via Gdb-patches wrote:
>> On 2021-06-15 7:14 a.m., Pedro Alves wrote:
>>> The problem with using signal(...) to temporarily ignore a signal, is
>>> that that changes the the signal disposition for the whole process.
>>> If multiple threads do it at the same time, you have a race.
>>>
>>> Fix this by using sigprocmask + sigtimedwait to implement the ignoring
>>> instead, if available, which I think probably means everywhere except
>>> Windows nowadays.  This way, we only change the signal mask for the
>>> current thread, so there's no race.
>>
>> I tried to build on macOS today and got:
>>
>>
>>        CXX    compile/compile.o
>>      In file included from /Users/smarchi/src/binutils-gdb/gdb/compile/compile.c:46:
>>      /Users/smarchi/src/binutils-gdb/gdb/../gdbsupport/scoped_ignore_signal.h:69:4: error: use of undeclared identifier 'sigtimedwait'
>>                sigtimedwait (&set, nullptr, &zero_timeout);
>>                ^
>>
>> I didn't have time to dig yet.
> 
> It looks like macOS doesn't implement either sigtimedwait() or sigwaitinfo()
> which are part of POSIX from 1996 (*sigh*); however, macOS does provide
> sigpending() and sigwait(), so perhaps we can do something like:
> 
>    if (ConsumePending)
>      {
> #ifdef HAVE_SIGTIMEDWAIT
>        const timespec zero_timeout = {};
> 
>        sigtimedwait (&set, nullptr, &zero_timeout);
> #else
>        sigset_t pending;
> 
>        sigpending (&pending);
>        if (sigismember (&pending, SIG))

SIG -> Sig

>          sigwait (&set, nullptr);
> #endif
>      }
> 
> (Ironically, sigwait() was added to POSIX in the same version of the spec as
> sigtimedwait().)
> 

Thanks.  At least that change fixes that particular build issue, but I
can't tell if it has the correct behavior though (I haven't looked at
what sigtimedwait / sigwait do yet...).

Simon

Re: [PATCH 3/4] scoped_ignore_signal: Use sigprocmask+sigtimedwait instead of signal

[Pedro Alves]

On 2021-06-26 2:41 p.m., John Baldwin wrote:
> On 6/26/21 5:35 AM, Simon Marchi via Gdb-patches wrote:
>> On 2021-06-15 7:14 a.m., Pedro Alves wrote:
>>> The problem with using signal(...) to temporarily ignore a signal, is
>>> that that changes the the signal disposition for the whole process.
>>> If multiple threads do it at the same time, you have a race.
>>>
>>> Fix this by using sigprocmask + sigtimedwait to implement the ignoring
>>> instead, if available, which I think probably means everywhere except
>>> Windows nowadays.  This way, we only change the signal mask for the
>>> current thread, so there's no race.
>>
>> I tried to build on macOS today and got:
>>
>>
>>        CXX    compile/compile.o
>>      In file included from /Users/smarchi/src/binutils-gdb/gdb/compile/compile.c:46:
>>      /Users/smarchi/src/binutils-gdb/gdb/../gdbsupport/scoped_ignore_signal.h:69:4: error: use of undeclared identifier 'sigtimedwait'
>>                sigtimedwait (&set, nullptr, &zero_timeout);
>>                ^
>>
>> I didn't have time to dig yet.
> 
> It looks like macOS doesn't implement either sigtimedwait() or sigwaitinfo()
> which are part of POSIX from 1996 (*sigh*); however, macOS does provide
> sigpending() and sigwait(), so perhaps we can do something like:
> 
>    if (ConsumePending)
>      {
> #ifdef HAVE_SIGTIMEDWAIT
>        const timespec zero_timeout = {};
> 
>        sigtimedwait (&set, nullptr, &zero_timeout);
> #else
>        sigset_t pending;
> 
>        sigpending (&pending);
>        if (sigismember (&pending, SIG))
>          sigwait (&set, nullptr);
> #endif
>      }
> 

Thanks, that looks right to me.  We're using sigtimewait with zero timeout to consume
a pending signal.  sigpending + sigwait effectively does the same thing.

According to gnulib, sigpending exists everywhere but mingw:

 https://www.gnu.org/software/gnulib/manual/html_node/sigpending.html

and sigtimedwait is missing on "Mac OS X 10.13, OpenBSD 6.7, Minix 3.1.8, Cygwin 2.9, mingw, MSVC 14, Android 5.1.":

  https://www.gnu.org/software/gnulib/manual/html_node/sigtimedwait.html

So the approach you propose should work well.

Given sigpending is everywhere Unix-like, if we'd like, we could instead use the sigpending + sigwait path
unconditionally everywhere, but that's two syscalls instead of one, and the configure check is
trivial (just add sigtimedwait to AC_CHECK_FUNCS in gdbsupport/common.m4), so why not indeed save one
syscall, seems easy enough.

Let me know if you'd rather me do the leg work of writing the actual patch.

Pedro Alves

Re: [PATCH 3/4] scoped_ignore_signal: Use sigprocmask+sigtimedwait instead of signal

[Simon Marchi]

> Thanks, that looks right to me.  We're using sigtimewait with zero timeout to consume
> a pending signal.  sigpending + sigwait effectively does the same thing.
> 
> According to gnulib, sigpending exists everywhere but mingw:
> 
>  https://www.gnu.org/software/gnulib/manual/html_node/sigpending.html
> 
> and sigtimedwait is missing on "Mac OS X 10.13, OpenBSD 6.7, Minix 3.1.8, Cygwin 2.9, mingw, MSVC 14, Android 5.1.":
> 
>   https://www.gnu.org/software/gnulib/manual/html_node/sigtimedwait.html
> 
> So the approach you propose should work well.
> 
> Given sigpending is everywhere Unix-like, if we'd like, we could instead use the sigpending + sigwait path
> unconditionally everywhere, but that's two syscalls instead of one, and the configure check is
> trivial (just add sigtimedwait to AC_CHECK_FUNCS in gdbsupport/common.m4), so why not indeed save one
> syscall, seems easy enough.
> 
> Let me know if you'd rather me do the leg work of writing the actual patch.

Thanks, I'll do it since I have access to a macOS system, and therefore
I can at least compile-test it.

Simon