[PATCH 0/5] GDB Synchronous Signal Handling

[PATCH 0/5] GDB Synchronous Signal Handling

[Andrew Burgess]

This series is all about how GDB handles fatal, synchronous, signls
generated from within GDB itself, e.g. SIGFPE, SIGSEGV, etc This is
series has nothing to do with signal from or to the inferior.

Patches #1, #2, and #3 all fix existing issues and make sense to merge
even without the later patches in this series.

Patches #4 and #5 add some new functionality to GDB relating to how
fatal signals are handled.

All feedback welcome,
Thanks,
Andrew

---

Andrew Burgess (5):
  gdb: terminate upon receipt of SIGFPE
  gdb: register signal handler after setting up event token
  gdb: rewrite header comment on async_init_signals
  gdb: print backtrace on fatal SIGSEGV
  gdb: register SIGBUS, SIGFPE, and SIGABRT handlers

 gdb/ChangeLog                                 |  42 ++++
 gdb/NEWS                                      |   6 +
 gdb/config.in                                 |   6 +
 gdb/configure                                 |  51 +++++
 gdb/configure.ac                              |  22 ++
 gdb/doc/ChangeLog                             |   4 +
 gdb/doc/gdb.texinfo                           |  12 ++
 gdb/event-top.c                               | 202 +++++++++++++-----
 gdb/testsuite/ChangeLog                       |   9 +
 gdb/testsuite/gdb.base/bt-on-fatal-signal.c   |  22 ++
 gdb/testsuite/gdb.base/bt-on-fatal-signal.exp | 111 ++++++++++
 gdb/ui-file.h                                 |   9 +
 12 files changed, 447 insertions(+), 49 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/bt-on-fatal-signal.c
 create mode 100644 gdb/testsuite/gdb.base/bt-on-fatal-signal.exp

-- 
2.25.4

[PATCH 1/5] gdb: terminate upon receipt of SIGFPE

[Andrew Burgess]

GDB's SIGFPE handling is broken.

We currently try to use an async event token to process SIGFPE.  So,
when a SIGFPE arrives the signal handler calls
mark_async_signal_handler then returns, effectively ignoring the
signal (for now).

The intention is that later the event loop will see that the async
token associated with SIGFPE has been marked and will call the async
handler, which just throws an error.

The problem is that SIGFPE is not safe to ignore.  Ignoring a
SIGFPE (unless it is generated artificially, e.g. by raise()) is
undefined behaviour, after ignoring the signal on many targets we
return to the instruction that caused the SIGFPE to be raised, which
immediately causes another SIGFPE to be raised, we get stuck in an
infinite loop.  The behaviour is certainly true on x86-64.

To view this behaviour I simply added some dummy code to GDB that
performed an integer divide by zero, compiled this on x86-64
GNU/Linux, ran GDB and saw GDB hang.

In this commit, I propose to remove all special handling of SIGFPE and
instead just let GDB make use of the default SIGFPE action, that is,
to terminate the process.

The only user visible change here should be:

  - If a user sends a SIGFPE to GDB using something like kill,
    previously GDB would just print an error and remain alive, now GDB
    will terminate.  This is inline with what happens if the user
    sends GDB a SIGSEGV from kill though, so I don't see this as an
    issue.

  - If a bug in GDB causes a real SIGFPE, previously the users GDB
    session would hang.  Now the GDB session will terminate.  Again,
    this is inline with what happens if GDB receives a SIGSEGV due to
    an internal bug.

gdb/ChangeLog:

	* event-top.c (handle_sigfpe): Delete.
	(async_float_handler): Delete.
	(sigfpe_token): Delete.
	(handle_fatal_signal): New function.
	(async_init_signals): Update header comment and remove use of
	handle_sigfpe and sigfpe_token.
---
 gdb/ChangeLog   |  9 +++++++++
 gdb/event-top.c | 25 +------------------------
 2 files changed, 10 insertions(+), 24 deletions(-)

diff --git a/gdb/event-top.c b/gdb/event-top.c
index 002a7dc95e0..ab5179b7d32 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -58,7 +58,6 @@ static void handle_sigquit (int sig);
 #ifdef SIGHUP
 static void handle_sighup (int sig);
 #endif
-static void handle_sigfpe (int sig);
 
 /* Functions to be invoked by the event loop in response to
    signals.  */
@@ -68,7 +67,6 @@ static void async_do_nothing (gdb_client_data);
 #ifdef SIGHUP
 static void async_disconnect (gdb_client_data);
 #endif
-static void async_float_handler (gdb_client_data);
 #ifdef SIGTSTP
 static void async_sigtstp_handler (gdb_client_data);
 #endif
@@ -111,7 +109,6 @@ static struct async_signal_handler *sighup_token;
 #ifdef SIGQUIT
 static struct async_signal_handler *sigquit_token;
 #endif
-static struct async_signal_handler *sigfpe_token;
 #ifdef SIGTSTP
 static struct async_signal_handler *sigtstp_token;
 #endif
@@ -904,7 +901,7 @@ static struct serial_event *quit_serial_event;
 
 /* Initialization of signal handlers and tokens.  There is a function
    handle_sig* for each of the signals GDB cares about.  Specifically:
-   SIGINT, SIGFPE, SIGQUIT, SIGTSTP, SIGHUP, SIGWINCH.  These
+   SIGINT, SIGQUIT, SIGTSTP, SIGHUP, SIGWINCH.  These
    functions are the actual signal handlers associated to the signals
    via calls to signal().  The only job for these functions is to
    enqueue the appropriate event/procedure with the event loop.  Such
@@ -955,9 +952,6 @@ async_init_signals (void)
     sighup_token =
       create_async_signal_handler (async_do_nothing, NULL, "sighup");
 #endif
-  signal (SIGFPE, handle_sigfpe);
-  sigfpe_token =
-    create_async_signal_handler (async_float_handler, NULL, "sigfpe");
 
 #ifdef SIGTSTP
   sigtstp_token =
@@ -1198,23 +1192,6 @@ async_sigtstp_handler (gdb_client_data arg)
 }
 #endif /* SIGTSTP */
 
-/* Tell the event loop what to do if SIGFPE is received.
-   See event-signal.c.  */
-static void
-handle_sigfpe (int sig)
-{
-  mark_async_signal_handler (sigfpe_token);
-  signal (sig, handle_sigfpe);
-}
-
-/* Event loop will call this function to process a SIGFPE.  */
-static void
-async_float_handler (gdb_client_data arg)
-{
-  /* This message is based on ANSI C, section 4.7.  Note that integer
-     divide by zero causes this, so "float" is a misnomer.  */
-  error (_("Erroneous arithmetic operation."));
-}
 \f
 
 /* Set things up for readline to be invoked via the alternate
-- 
2.25.4

[PATCH 2/5] gdb: register signal handler after setting up event token

[Andrew Burgess]

This commit fixes the smallest of small possible bug related to signal
handling.  If we look in async_init_signals we see code like this:

  signal (SIGQUIT, handle_sigquit);
  sigquit_token =
    create_async_signal_handler (async_do_nothing, NULL, "sigquit");

Then if we look in handle_sigquit we see code like this:

  mark_async_signal_handler (sigquit_token);
  signal (sig, handle_sigquit);

Finally, in mark_async_signal_handler we have:

  async_handler_ptr->ready = 1;

Where async_handler_ptr will be sigquit_token.

What this means is that if a SIGQUIT arrive in async_init_signals
after handle_sigquit has been registered, but before sigquit_token has
been initialised, then GDB will most likely crash.

The chance of this happening is tiny, but fixing this is trivial, just
ensure we call create_async_signal_handler before calling signal, so
lets do that.

There are no tests for this.  Trying to land a signal in the right
spot is pretty hit and miss.  I did try changing the current HEAD GDB
like this:

  signal (SIGQUIT, handle_sigquit);
  raise (SIGQUIT);
  sigquit_token =
    create_async_signal_handler (async_do_nothing, NULL, "sigquit");

And confirmed that this did result in a crash, after my change I tried
this:

  sigquit_token =
    create_async_signal_handler (async_do_nothing, NULL, "sigquit");
  signal (SIGQUIT, handle_sigquit);
  raise (SIGQUIT);

And GDB now starts up just fine.

gdb/ChangeLog:

	* event-top.c (async_init_signals): For each signal, call signal
	only after calling create_async_signal_handler.
---
 gdb/ChangeLog   | 5 +++++
 gdb/event-top.c | 8 +++++---
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/gdb/event-top.c b/gdb/event-top.c
index ab5179b7d32..2d3bfa6a9c9 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -918,12 +918,13 @@ async_init_signals (void)
 
   quit_serial_event = make_serial_event ();
 
-  signal (SIGINT, handle_sigint);
   sigint_token =
     create_async_signal_handler (async_request_quit, NULL, "sigint");
-  signal (SIGTERM, handle_sigterm);
+  signal (SIGINT, handle_sigint);
+
   async_sigterm_token
     = create_async_signal_handler (async_sigterm_handler, NULL, "sigterm");
+  signal (SIGTERM, handle_sigterm);
 
   /* If SIGTRAP was set to SIG_IGN, then the SIG_IGN will get passed
      to the inferior and breakpoints will be ignored.  */
@@ -940,10 +941,11 @@ async_init_signals (void)
      might be in memory, shared between the two).  Since we establish
      a handler for SIGQUIT, when we call exec it will set the signal
      to SIG_DFL for us.  */
-  signal (SIGQUIT, handle_sigquit);
   sigquit_token =
     create_async_signal_handler (async_do_nothing, NULL, "sigquit");
+  signal (SIGQUIT, handle_sigquit);
 #endif
+
 #ifdef SIGHUP
   if (signal (SIGHUP, handle_sighup) != SIG_IGN)
     sighup_token =
-- 
2.25.4

[PATCH 3/5] gdb: rewrite header comment on async_init_signals

[Andrew Burgess]

The header comment on the function async_init_signals was getting
quite out of date, this commit just rewrites to (hopefully) reflect
the current reality.

There should be no user visible change after this commit.

gdb/ChangeLog:

	* event-top.c (async_init_signals): Rewrite the header comment.
---
 gdb/ChangeLog   |  4 ++++
 gdb/event-top.c | 27 +++++++++++++++------------
 2 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/gdb/event-top.c b/gdb/event-top.c
index 2d3bfa6a9c9..c9158fbdc2d 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -899,18 +899,21 @@ handle_sigsegv (int sig)
    handler.  */
 static struct serial_event *quit_serial_event;
 
-/* Initialization of signal handlers and tokens.  There is a function
-   handle_sig* for each of the signals GDB cares about.  Specifically:
-   SIGINT, SIGQUIT, SIGTSTP, SIGHUP, SIGWINCH.  These
-   functions are the actual signal handlers associated to the signals
-   via calls to signal().  The only job for these functions is to
-   enqueue the appropriate event/procedure with the event loop.  Such
-   procedures are the old signal handlers.  The event loop will take
-   care of invoking the queued procedures to perform the usual tasks
-   associated with the reception of the signal.  */
-/* NOTE: 1999-04-30 This is the asynchronous version of init_signals.
-   init_signals will become obsolete as we move to have to event loop
-   as the default for gdb.  */
+/* Initialization of signal handlers and tokens.  There are a number of
+   different strategies for handling different signals here.
+
+   For SIGINT, SIGTERM, SIGQUIT, SIGHUP, SIGTSTP, there is a function
+   handle_sig* for each of these signals.  These functions are the actual
+   signal handlers associated to the signals via calls to signal().  The
+   only job for these functions is to enqueue the appropriate
+   event/procedure with the event loop.  The event loop will take care of
+   invoking the queued procedures to perform the usual tasks associated
+   with the reception of the signal.
+
+   For SIGTRAP we just ensure its disposition is set to SIG_DFL.
+
+   For SIGSEGV the handle_sig* function does all the work for handling this
+   signal.  */
 void
 async_init_signals (void)
 {
-- 
2.25.4

[PATCH 4/5] gdb: print backtrace on fatal SIGSEGV

[Andrew Burgess]

This commit adds a new maintenance feature, the ability to print
a (limited) backtrace if GDB dies due to a fatal signal.

The backtrace is produced using the backtrace and backtrace_symbols_fd
functions which are declared in the execinfo.h header.  A configure
check has been added to check for these features, if they are not
available then the new code is not compiled into GDB.

The motivation for this new feature is to aid in debugging GDB in
situations where GDB has crashed at a users site, but the user is
reluctant to share core files, possibly due to concerns about what
might be in the memory image within the core file.  Such a user might
be happy to share a simple backtrace that was written to stderr.

The production of the backtrace is off by default, and can switched on
using the new commands:

  maintenance set backtrace-on-fatal-signal on|off
  maintenance show backtrace-on-fatal-signal

Right now, I have hooked this feature in to GDB's existing handling of
SIGSEGV only, but this could be extended to more signals in a later
commit.

One additional change I have made in this commit is that, when we
decide that GDB should terminate due to the fatal signal we now
set the signal disposition of the signal in question back to SIG_DFL,
unblock the signal, and then re-raise the signal.  This ensures that
GDB should terminate due to the specific bad signal that arrived.

So, currently, this is only effecting our handling of SIGSEGV.
Previously, if GDB hit a SEGV then we would terminate GDB with a
SIGABRT.  After this commit we will terminate GDB with a SIGSEGV.
This feels like an improvement to me, we should still get a core dump,
but in many shells, the user will see a more specific message once GDB
exits, in bash for example "Segmentation fault" rather than "Aborted".

gdb/ChangeLog:

	* NEWS: Mention new commands.
	* config.in: Regenerate.
	* configure: Regenerate.
	* configure.ac: Add header check for execinfo.h.  Add a check for
	the backtrace and backtrace_symbols_fd functions.
	* event-top.c: Add execinfo.h include.
	(bt_on_fatal_signal): New static global.
	(show_bt_on_fatal_signal): New function.
	(set_bt_on_fatal_signal): New function.
	(unblock_signal): New function.
	(handle_fatal_signal): New function.
	(handle_sigsegv): Call handle_fatal_signal instead of abort.
	(async_sigtstp_handler): Make use of unblock_signal.
	(_initialize_event_top): Register new maintenance command.
	* ui-file.h (ui_file) <fd>: New virtual member function.
	(stdio_file) <fd>: New member function.

gdb/doc/ChangeLog:

	* gdb.texinfo (Maintenance Commands): Document new commands.

gdb/testsuite/ChangeLog:

	* gdb.base/bt-on-fatal-signal.c: New file.
	* gdb.base/bt-on-fatal-signal.exp: New file.
---
 gdb/ChangeLog                                 |  19 +++
 gdb/NEWS                                      |   6 +
 gdb/config.in                                 |   6 +
 gdb/configure                                 |  51 +++++++
 gdb/configure.ac                              |  22 +++
 gdb/doc/ChangeLog                             |   4 +
 gdb/doc/gdb.texinfo                           |  12 ++
 gdb/event-top.c                               | 129 ++++++++++++++++--
 gdb/testsuite/ChangeLog                       |   5 +
 gdb/testsuite/gdb.base/bt-on-fatal-signal.c   |  22 +++
 gdb/testsuite/gdb.base/bt-on-fatal-signal.exp | 108 +++++++++++++++
 gdb/ui-file.h                                 |   9 ++
 12 files changed, 382 insertions(+), 11 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/bt-on-fatal-signal.c
 create mode 100644 gdb/testsuite/gdb.base/bt-on-fatal-signal.exp

diff --git a/gdb/NEWS b/gdb/NEWS
index 7f3ed4f02f0..52f00bb7093 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -170,6 +170,12 @@ show python dont-write-bytecode
   When set to 'auto' (the default) Python will check the
   PYTHONDONTWRITEBYTECODE environment variable.
 
+maint set backtrace-on-fatal-signal on|off
+maint show backtrace-on-fatal-signal
+  This setting is 'off' by default.  When 'on' GDB will print a
+  limited backtrace to stderr in the situation where GDB terminates
+  with a fatal signal.
+
 * Changed commands
 
 break [PROBE_MODIFIER] [LOCATION] [thread THREADNUM]
diff --git a/gdb/config.in b/gdb/config.in
index 9342604ac4c..ecbdfed4f2d 100644
--- a/gdb/config.in
+++ b/gdb/config.in
@@ -162,6 +162,12 @@
 /* Define to 1 if your system has the etext variable. */
 #undef HAVE_ETEXT
 
+/* Define to 1 if execinfo.h backtrace functions are available. */
+#undef HAVE_EXECINFO_BACKTRACE
+
+/* Define to 1 if you have the <execinfo.h> header file. */
+#undef HAVE_EXECINFO_H
+
 /* Define to 1 if you have the `fdwalk' function. */
 #undef HAVE_FDWALK
 
diff --git a/gdb/configure b/gdb/configure
index a5c6fab118c..81f28c5be75 100755
--- a/gdb/configure
+++ b/gdb/configure
@@ -12347,6 +12347,18 @@ fi
 
 done
 
+for ac_header in execinfo.h
+do :
+  ac_fn_c_check_header_mongrel "$LINENO" "execinfo.h" "ac_cv_header_execinfo_h" "$ac_includes_default"
+if test "x$ac_cv_header_execinfo_h" = xyes; then :
+  cat >>confdefs.h <<_ACEOF
+#define HAVE_EXECINFO_H 1
+_ACEOF
+
+fi
+
+done
+
 
 # ------------------------- #
 # Checks for declarations.  #
@@ -16516,6 +16528,45 @@ fi
 { $as_echo "$as_me:${as_lineno-$LINENO}: result: $found" >&5
 $as_echo "$found" >&6; }
 
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking whether execinfo.h backtrace is available" >&5
+$as_echo_n "checking whether execinfo.h backtrace is available... " >&6; }
+if ${gdb_cv_execinfo_backtrace+:} false; then :
+  $as_echo_n "(cached) " >&6
+else
+  cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h.  */
+
+         #include <execinfo.h>
+
+int
+main ()
+{
+
+         int f;
+         void *b[2];
+         f = backtrace (b, 2);
+         backtrace_symbols_fd (b, f, 2);
+
+  ;
+  return 0;
+}
+_ACEOF
+if ac_fn_c_try_link "$LINENO"; then :
+  gdb_cv_execinfo_backtrace=yes
+else
+  gdb_cv_execinfo_backtrace=no
+fi
+rm -f core conftest.err conftest.$ac_objext \
+    conftest$ac_exeext conftest.$ac_ext
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $gdb_cv_execinfo_backtrace" >&5
+$as_echo "$gdb_cv_execinfo_backtrace" >&6; }
+if test "$gdb_cv_execinfo_backtrace" = yes; then
+
+$as_echo "#define HAVE_EXECINFO_BACKTRACE 1" >>confdefs.h
+
+fi
+
 
 if test "${build}" = "${host}" -a "${host}" = "${target}" ; then
    case ${host_os} in
diff --git a/gdb/configure.ac b/gdb/configure.ac
index 67aa628c47d..aa2b5eeeae9 100644
--- a/gdb/configure.ac
+++ b/gdb/configure.ac
@@ -1315,6 +1315,7 @@ AC_CHECK_HEADERS(term.h, [], [],
 
 AC_CHECK_HEADERS([sys/socket.h])
 AC_CHECK_HEADERS([ws2tcpip.h])
+AC_CHECK_HEADERS([execinfo.h])
 
 # ------------------------- #
 # Checks for declarations.  #
@@ -1706,6 +1707,27 @@ fi
 AC_SUBST(RDYNAMIC)
 AC_MSG_RESULT($found)
 
+AC_CACHE_CHECK(
+  [whether execinfo.h backtrace is available],
+  gdb_cv_execinfo_backtrace,
+  [AC_LINK_IFELSE(
+     [AC_LANG_PROGRAM(
+        [
+         #include <execinfo.h>
+        ],
+        [
+         int f;
+         void *b[[2]];
+         f = backtrace (b, 2);
+         backtrace_symbols_fd (b, f, 2);
+        ])],
+   [gdb_cv_execinfo_backtrace=yes],
+   [gdb_cv_execinfo_backtrace=no])])
+if test "$gdb_cv_execinfo_backtrace" = yes; then
+  AC_DEFINE(HAVE_EXECINFO_BACKTRACE, 1,
+            [Define to 1 if execinfo.h backtrace functions are available.])
+fi
+
 dnl For certain native configurations, we need to check whether thread
 dnl support can be built in or not.
 dnl
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index f1c3e7ba847..dc3852d26f7 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -39693,6 +39693,18 @@
 @value{GDBN} supports.  They are used by the testsuite for exercising
 the settings infrastructure.
 
+@kindex maint set backtrace-on-fatal-signal
+@kindex maint show backtrace-on-fatal-signal
+@item maint set backtrace-on-fatal-signal [on|off]
+@itemx maint show backtrace-on-fatal-signal
+When this setting is @code{on}, if @value{GDBN} itself terminates with
+a fatal signal (e.g.@: SIGSEGV), then a limited backtrace will be
+printed to stderr.  This backtrace can be used to help diagnose
+crashes within @value{GDBN} in situations where a user is unable to
+share a corefile with the @value{GDBN} developers.
+
+This setting is @code{off} by default.
+
 @kindex maint with
 @item maint with @var{setting} [@var{value}] [-- @var{command}]
 Like the @code{with} command, but works with @code{maintenance set}
diff --git a/gdb/event-top.c b/gdb/event-top.c
index c9158fbdc2d..47dd6301a0e 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -46,6 +46,10 @@
 #include "readline/readline.h"
 #include "readline/history.h"
 
+#ifdef HAVE_EXECINFO_H
+#include <execinfo.h>
+#endif /* HAVE_EXECINFO_H */
+
 /* readline defines this.  */
 #undef savestring
 
@@ -96,6 +100,33 @@ bool exec_done_display_p = false;
    run again.  */
 int call_stdin_event_handler_again_p;
 
+/* When true GDB will produce a minimal backtrace when a fatal signal is
+   reached (within GDB code).  */
+static bool bt_on_fatal_signal = false;
+
+/* Implement 'maintenance show backtrace-on-fatal-signal'.  */
+
+static void
+show_bt_on_fatal_signal (struct ui_file *file, int from_tty,
+			 struct cmd_list_element *cmd, const char *value)
+{
+  fprintf_filtered (file, _("Backtrace on a fatal signal is %s.\n"), value);
+}
+
+/* Implement 'maintenance set backtrace-on-fatal-signal'.  */
+
+static void
+set_bt_on_fatal_signal (const char *args, int from_tty, cmd_list_element *c)
+{
+#ifndef HAVE_EXECINFO_BACKTRACE
+  if (bt_on_fatal_signal)
+    {
+      bt_on_fatal_signal = false;
+      error (_("support for this feature is not compiled into GDB"));
+    }
+#endif
+}
+
 /* Signal handling variables.  */
 /* Each of these is a pointer to a function that the event loop will
    invoke if the corresponding signal has received.  The real signal
@@ -846,6 +877,78 @@ gdb_readline_no_editing_callback (gdb_client_data client_data)
 }
 \f
 
+/* Attempt to unblock signal SIG, return true if the signal was unblocked,
+   otherwise, return false.  */
+
+static bool
+unblock_signal (int sig)
+{
+#if HAVE_SIGPROCMASK
+  sigset_t sigset;
+  sigemptyset (&sigset);
+  sigaddset (&sigset, sig);
+  gdb_sigmask (SIG_UNBLOCK, &sigset, 0);
+  return true;
+#elif HAVE_SIGSETMASK
+  int mask = siggetmask ();
+  mask &= ~sigmask (sig);
+  sigsetmask (mask);
+  return true;
+#endif
+
+  return false;
+}
+
+/* Called to handle fatal signals.  SIG is the signal number.  */
+
+static void __attribute__ ((__noreturn__))
+handle_fatal_signal (int sig)
+{
+#ifdef HAVE_EXECINFO_BACKTRACE
+  const auto sig_write = [] (const char *msg) -> void {
+    gdb_stderr->write_async_safe (msg, strlen (msg));
+  };
+
+  if (bt_on_fatal_signal)
+    {
+      /* Ensure we don't get stuck trying to handle signals recursively.  */
+      signal (sig, SIG_DFL);
+
+      sig_write ("\n\nFatal signal: ");
+      sig_write (strsignal (sig));
+      sig_write ("\n");
+
+      /* Allow up to 25 frames of backtrace.  */
+      void *buffer[25];
+      int frames = backtrace (buffer, sizeof(buffer) / sizeof(buffer[0]));
+      sig_write ("----- Backtrace -----\n");
+      if (gdb_stderr->fd () > -1)
+	{
+	  backtrace_symbols_fd (buffer, frames, gdb_stderr->fd ());
+	  if (frames == (sizeof(buffer) / sizeof(buffer[0])))
+	    sig_write ("Backtrace might be incomplete.\n");
+	}
+      else
+	sig_write ("Backtrace unavailable\n");
+      sig_write ("---------------------\n");
+      gdb_stderr->flush ();
+    }
+#endif /* HAVE_EXECINF_BACKTRACE */
+
+  /* If possible arrange for SIG to have its default behaviour (which
+     should be to terminate the current process), unblock SIG, and reraise
+     the signal.  This ensures GDB terminates with the expected signal.  */
+  if (signal (sig, SIG_DFL) != SIG_ERR
+      && unblock_signal (sig))
+    raise (sig);
+
+  /* The above failed, so try to use SIGABRT to terminate GDB.  */
+#ifdef SIGABRT
+  signal (SIGABRT, SIG_DFL);
+#endif
+  abort ();		/* ARI: abort */
+}
+
 /* The SIGSEGV handler for this thread, or NULL if there is none.  GDB
    always installs a global SIGSEGV handler, and then lets threads
    indicate their interest in handling the signal by setting this
@@ -887,7 +990,7 @@ handle_sigsegv (int sig)
   install_handle_sigsegv ();
 
   if (thread_local_segv_handler == nullptr)
-    abort ();			/* ARI: abort */
+    handle_fatal_signal (sig);
   thread_local_segv_handler (sig);
 }
 
@@ -1176,16 +1279,7 @@ async_sigtstp_handler (gdb_client_data arg)
   char *prompt = get_prompt ();
 
   signal (SIGTSTP, SIG_DFL);
-#if HAVE_SIGPROCMASK
-  {
-    sigset_t zero;
-
-    sigemptyset (&zero);
-    gdb_sigmask (SIG_SETMASK, &zero, 0);
-  }
-#elif HAVE_SIGSETMASK
-  sigsetmask (0);
-#endif
+  unblock_signal (SIGTSTP);
   raise (SIGTSTP);
   signal (SIGTSTP, handle_sigtstp);
   printf_unfiltered ("%s", prompt);
@@ -1336,4 +1430,17 @@ Control whether to show event loop-related debug messages."),
 			set_debug_event_loop_command,
 			show_debug_event_loop_command,
 			&setdebuglist, &showdebuglist);
+
+  add_setshow_boolean_cmd ("backtrace-on-fatal-signal", class_maintenance,
+			   &bt_on_fatal_signal, _("\
+Set whether to produce a backtrace if GDB receives a fatal signal."), _("\
+Show whether GDB will produce a backtrace if it receives a fatal signal."), _("\
+Use \"on\" to enable, \"off\" to disable.\n\
+If enabled, GDB will produce a minimal backtrace if it encounters a fatal\n\
+signal from within GDB itself.  This is a mechanism to help diagnose\n\
+crashes within GDB, not a mechanism for debugging inferiors."),
+			   set_bt_on_fatal_signal,
+			   show_bt_on_fatal_signal,
+			   &maintenance_set_cmdlist,
+			   &maintenance_show_cmdlist);
 }
diff --git a/gdb/testsuite/gdb.base/bt-on-fatal-signal.c b/gdb/testsuite/gdb.base/bt-on-fatal-signal.c
new file mode 100644
index 00000000000..d9d56c3700f
--- /dev/null
+++ b/gdb/testsuite/gdb.base/bt-on-fatal-signal.c
@@ -0,0 +1,22 @@
+/* Copyright 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/>.  */
+
+int
+main (void)
+{
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/bt-on-fatal-signal.exp b/gdb/testsuite/gdb.base/bt-on-fatal-signal.exp
new file mode 100644
index 00000000000..a822b324065
--- /dev/null
+++ b/gdb/testsuite/gdb.base/bt-on-fatal-signal.exp
@@ -0,0 +1,108 @@
+# Copyright 2021 Free Software Foundation, Inc.
+
+# 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/>.
+
+# Test the 'maint set backtrace-on-fatal-signal' behaviour.  Start up
+# GDB, turn on backtrace-on-fatal-signal, then send fatal signals to
+# GDB and ensure we see the backtrace.
+
+standard_testfile
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile]} {
+    return -1
+}
+
+# Check we can run to main.  If this works this time then we just
+# assume that it will work later on (when we repeatedly restart GDB).
+if ![runto_main] then {
+    untested $testfile
+    return -1
+}
+
+# Check that the backtrace-on-fatal-signal feature is supported.  If
+# this target doesn't have the backtrace function available then
+# trying to turn this on will give an error, in which case we just
+# skip this test.
+gdb_test_multiple "maint set backtrace-on-fatal-signal on" "" {
+    -re "support for this feature is not compiled into GDB" {
+	untested $testfile
+	return -1
+    }
+    -re "$gdb_prompt $" {
+	pass $gdb_test_name
+    }
+}
+
+# Now the actual test loop.
+foreach test_data {{SEGV "Segmentation fault"}} {
+    set sig [lindex ${test_data} 0]
+    set msg [lindex ${test_data} 1]
+    with_test_prefix ${sig} {
+
+	# Restart GDB.
+	clean_restart $binfile
+
+	# Capture the pid of GDB.
+	set testpid [spawn_id_get_pid $inferior_spawn_id]
+
+	# Start the inferior.
+	runto_main
+
+	# Turn on the backtrace-on-fatal-signal feature.
+	gdb_test_no_output "maint set backtrace-on-fatal-signal on"
+
+	# Flags for various bits of the output we expect to see, we
+	# check for these in the gdb_test_multiple below.
+	set saw_fatal_msg False
+	set saw_bt_start False
+	set saw_bt_end False
+
+	# Send the fatal signal to GDB.
+	eval exec kill -${sig} ${testpid}
+
+	# Scan GDB's output for the backtrace.
+	gdb_test_multiple "" "scan for backtrace" {
+	    -re "^\r\n" {
+		exp_continue
+	    }
+	    -re "^Fatal signal: ${msg}\r\n" {
+		set saw_fatal_msg True
+		exp_continue
+	    }
+	    -re "^----- Backtrace -----\r\n" {
+		set saw_bt_start True
+		exp_continue
+	    }
+	    -re ".+\r\n---------------------\r\n" {
+		set saw_bt_end True
+		exp_continue
+	    }
+	    eof {
+		# Catch the eof case as this indicates that GDB has
+		# gone away, which in this case, is what we expect to
+		# happen.
+		gdb_assert { $saw_fatal_msg }
+		gdb_assert { $saw_bt_start }
+		gdb_assert { $saw_bt_end }
+	    }
+	    -re "$gdb_prompt $" {
+		# GDB should terminate, we should never get back to
+		# the prompt.
+		fail $gdb_test_name
+	    }
+	}
+
+	# GDB is now dead and gone.
+    }
+}
diff --git a/gdb/ui-file.h b/gdb/ui-file.h
index 61509735c68..9593c94e673 100644
--- a/gdb/ui-file.h
+++ b/gdb/ui-file.h
@@ -83,6 +83,11 @@ class ui_file
 
   virtual void flush ()
   {}
+
+  /* If this object has an underlying file descriptor, then return it.
+     Otherwise, return -1.  */
+  virtual int fd () const
+  { return -1; }
 };
 
 typedef std::unique_ptr<ui_file> ui_file_up;
@@ -195,6 +200,10 @@ class stdio_file : public ui_file
 
   bool can_emit_style_escape () override;
 
+  /* Return the underlying file descriptor.  */
+  int fd () const override
+  { return m_fd; }
+
 private:
   /* Sets the internal stream to FILE, and saves the FILE's file
      descriptor in M_FD.  */
-- 
2.25.4

[PATCH 5/5] gdb: register SIGBUS, SIGFPE, and SIGABRT handlers

[Andrew Burgess]

Register handlers for SIGBUS, SIGFPE, and SIGABRT.  All of these
signals are setup as fatal signals that will cause GDB to terminate.
However, by passing these signals through the handle_fatal_signal
function, a user can arrange to see a backtrace when GDB
terminates (see maint set backtrace-on-fatal-signal).

In normal use of GDB there should be no user visible changes after
this commit.  Only if GDB terminates with one of the above signals
will GDB change slightly, potentially printing a backtrace before
aborting.

gdb/ChangeLog:

	* event-top.c (async_init_signals): Handle SIGFPE, SIGBUS, and
	SIGABRT.  Update header comment.

gdb/testsuite/ChangeLog:

	* gdb.base/bt-on-fatal-signal.exp: Add additional tests.
---
 gdb/ChangeLog                                 |  5 +++++
 gdb/event-top.c                               | 17 ++++++++++++++++-
 gdb/testsuite/ChangeLog                       |  4 ++++
 gdb/testsuite/gdb.base/bt-on-fatal-signal.exp |  5 ++++-
 4 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/gdb/event-top.c b/gdb/event-top.c
index 47dd6301a0e..293d90ae7a6 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -1016,7 +1016,10 @@ static struct serial_event *quit_serial_event;
    For SIGTRAP we just ensure its disposition is set to SIG_DFL.
 
    For SIGSEGV the handle_sig* function does all the work for handling this
-   signal.  */
+   signal.
+
+   For SIGFPE, SIGBUS, and SIGABRT, these signals will all cause GDB to
+   terminate immediately.  */
 void
 async_init_signals (void)
 {
@@ -1066,6 +1069,18 @@ async_init_signals (void)
     create_async_signal_handler (async_sigtstp_handler, NULL, "sigtstp");
 #endif
 
+#ifdef SIGFPE
+  signal (SIGFPE, handle_fatal_signal);
+#endif
+
+#ifdef SIGBUS
+  signal (SIGBUS, handle_fatal_signal);
+#endif
+
+#ifdef SIGABRT
+  signal (SIGABRT, handle_fatal_signal);
+#endif
+
   install_handle_sigsegv ();
 }
 
diff --git a/gdb/testsuite/gdb.base/bt-on-fatal-signal.exp b/gdb/testsuite/gdb.base/bt-on-fatal-signal.exp
index a822b324065..3efb3218005 100644
--- a/gdb/testsuite/gdb.base/bt-on-fatal-signal.exp
+++ b/gdb/testsuite/gdb.base/bt-on-fatal-signal.exp
@@ -45,7 +45,10 @@ gdb_test_multiple "maint set backtrace-on-fatal-signal on" "" {
 }
 
 # Now the actual test loop.
-foreach test_data {{SEGV "Segmentation fault"}} {
+foreach test_data {{SEGV "Segmentation fault"} \
+		       {FPE "Floating point exception"} \
+		       {BUS "Bus error"} \
+		       {ABRT "Aborted"}} {
     set sig [lindex ${test_data} 0]
     set msg [lindex ${test_data} 1]
     with_test_prefix ${sig} {
-- 
2.25.4

Re: [PATCH 4/5] gdb: print backtrace on fatal SIGSEGV

[Eli Zaretskii]

> From: Andrew Burgess <andrew.burgess@embecosm.com>
> Date: Fri,  2 Jul 2021 12:06:07 +0100
> So, currently, this is only effecting our handling of SIGSEGV.
> Previously, if GDB hit a SEGV then we would terminate GDB with a
> SIGABRT.  After this commit we will terminate GDB with a SIGSEGV.
> This feels like an improvement to me, we should still get a core dump,
> but in many shells, the user will see a more specific message once GDB
> exits, in bash for example "Segmentation fault" rather than "Aborted".

But will GDB still present the "internal error, submit a bug report"
message when a fatal signal such as SIGSEGV is encountered?

> gdb/ChangeLog:
> 
> 	* NEWS: Mention new commands.
> 	* config.in: Regenerate.
> 	* configure: Regenerate.
> 	* configure.ac: Add header check for execinfo.h.  Add a check for
> 	the backtrace and backtrace_symbols_fd functions.
> 	* event-top.c: Add execinfo.h include.
> 	(bt_on_fatal_signal): New static global.
> 	(show_bt_on_fatal_signal): New function.
> 	(set_bt_on_fatal_signal): New function.
> 	(unblock_signal): New function.
> 	(handle_fatal_signal): New function.
> 	(handle_sigsegv): Call handle_fatal_signal instead of abort.
> 	(async_sigtstp_handler): Make use of unblock_signal.
> 	(_initialize_event_top): Register new maintenance command.
> 	* ui-file.h (ui_file) <fd>: New virtual member function.
> 	(stdio_file) <fd>: New member function.
> 
> gdb/doc/ChangeLog:
> 
> 	* gdb.texinfo (Maintenance Commands): Document new commands.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.base/bt-on-fatal-signal.c: New file.
> 	* gdb.base/bt-on-fatal-signal.exp: New file.

The documentation parts are okay, but please make a point of saying
there that the backtrace will be printed only on capable systems,
because otherwise the text implies that this is supported on all
platforms.

Thanks.

Re: [PATCH 1/5] gdb: terminate upon receipt of SIGFPE

[Eli Zaretskii]

> From: Andrew Burgess <andrew.burgess@embecosm.com>
> Date: Fri,  2 Jul 2021 12:06:04 +0100
> 
> GDB's SIGFPE handling is broken.
> 
> We currently try to use an async event token to process SIGFPE.  So,
> when a SIGFPE arrives the signal handler calls
> mark_async_signal_handler then returns, effectively ignoring the
> signal (for now).
> 
> The intention is that later the event loop will see that the async
> token associated with SIGFPE has been marked and will call the async
> handler, which just throws an error.
> 
> The problem is that SIGFPE is not safe to ignore.  Ignoring a
> SIGFPE (unless it is generated artificially, e.g. by raise()) is
> undefined behaviour, after ignoring the signal on many targets we
> return to the instruction that caused the SIGFPE to be raised, which
> immediately causes another SIGFPE to be raised, we get stuck in an
> infinite loop.  The behaviour is certainly true on x86-64.
> 
> To view this behaviour I simply added some dummy code to GDB that
> performed an integer divide by zero, compiled this on x86-64
> GNU/Linux, ran GDB and saw GDB hang.
> 
> In this commit, I propose to remove all special handling of SIGFPE and
> instead just let GDB make use of the default SIGFPE action, that is,
> to terminate the process.

It is indeed a bad idea to return from a fatal signal handler, but
terminating the GDB process is not the only safe alternative.  Another
alternative is to longjmp from the signal handler to the top-level
command loop.  Wouldn't that be better?  Terminating the entire
debugging session is quite radical, and doesn't leave the user any
chance to save enough of the session in order not to lose valuable
data, let alone continue the session (and avoid the problematic
operation so as not to hit the same fatal signal again).

WDYT?

Re: [PATCH 1/5] gdb: terminate upon receipt of SIGFPE

[Tom Tromey]

>>>>> "Eli" == Eli Zaretskii via Gdb-patches <gdb-patches@sourceware.org> writes:

>> In this commit, I propose to remove all special handling of SIGFPE and
>> instead just let GDB make use of the default SIGFPE action, that is,
>> to terminate the process.

Eli> It is indeed a bad idea to return from a fatal signal handler, but
Eli> terminating the GDB process is not the only safe alternative.  Another
Eli> alternative is to longjmp from the signal handler to the top-level
Eli> command loop.  Wouldn't that be better?

I was also wondering if this would be possible -- treat it the way that
gdb treats assertion failures and try to carry on.

Tom

Re: [PATCH 1/5] gdb: terminate upon receipt of SIGFPE

[Pedro Alves]

On 2021-07-02 7:11 p.m., Tom Tromey wrote:
>>>>>> "Eli" == Eli Zaretskii via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
>>> In this commit, I propose to remove all special handling of SIGFPE and
>>> instead just let GDB make use of the default SIGFPE action, that is,
>>> to terminate the process.
> 
> Eli> It is indeed a bad idea to return from a fatal signal handler, but
> Eli> terminating the GDB process is not the only safe alternative.  Another
> Eli> alternative is to longjmp from the signal handler to the top-level
> Eli> command loop.  Wouldn't that be better?
> 
> I was also wondering if this would be possible -- treat it the way that
> gdb treats assertion failures and try to carry on.

Unless restricted to some small defined area of code like what we do
with the demangler and SIGSEGV, I don't see that recovering very well, because
it will skip running C++ destructors, and we tend to have RAII objects
on the stack all the time.

Thanks,
Pedro Alves

Re: [PATCH 1/5] gdb: terminate upon receipt of SIGFPE

[Eli Zaretskii]

> From: Pedro Alves <pedro@palves.net>
> Date: Fri, 2 Jul 2021 23:51:47 +0100
> 
> > Eli> It is indeed a bad idea to return from a fatal signal handler, but
> > Eli> terminating the GDB process is not the only safe alternative.  Another
> > Eli> alternative is to longjmp from the signal handler to the top-level
> > Eli> command loop.  Wouldn't that be better?
> > 
> > I was also wondering if this would be possible -- treat it the way that
> > gdb treats assertion failures and try to carry on.
> 
> Unless restricted to some small defined area of code like what we do
> with the demangler and SIGSEGV, I don't see that recovering very well, because
> it will skip running C++ destructors, and we tend to have RAII objects
> on the stack all the time.

Does this mean C++ doesn't allow recovering from a fatal signal?
Isn't there some way of converting a signal into a C++ exception, and
then unwinding "the C++ way"?

Re: [PATCH 1/5] gdb: terminate upon receipt of SIGFPE

[Pedro Alves]

On 2021-07-03 7:14 a.m., Eli Zaretskii wrote:
>> From: Pedro Alves <pedro@palves.net>
>> Date: Fri, 2 Jul 2021 23:51:47 +0100
>>
>>> Eli> It is indeed a bad idea to return from a fatal signal handler, but
>>> Eli> terminating the GDB process is not the only safe alternative.  Another
>>> Eli> alternative is to longjmp from the signal handler to the top-level
>>> Eli> command loop.  Wouldn't that be better?
>>>
>>> I was also wondering if this would be possible -- treat it the way that
>>> gdb treats assertion failures and try to carry on.
>>
>> Unless restricted to some small defined area of code like what we do
>> with the demangler and SIGSEGV, I don't see that recovering very well, because
>> it will skip running C++ destructors, and we tend to have RAII objects
>> on the stack all the time.
> 
> Does this mean C++ doesn't allow recovering from a fatal signal?

(Note that even in C -- if you siglongjmp and skip a bunch of manual
unwinding / restoring state in mainline code, then the program will be broken too.
If we were still using C and old C-base implementation of exceptions/cleanups, then
we'd siglongjmp up to the closest sigsetjmp, not to the top level.
Longjmping to the top level directly would likely result in a broken
GDB in a similar way.)

> Isn't there some way of converting a signal into a C++ exception, and
> then unwinding "the C++ way"?
> 

Not with standard C++.  Though as an extension, with GCC, if we build GDB
with -fnon-call-exceptions, then we can throw from trapping instructions:

 -fnon-call-exceptions
 Generate code that allows trapping instructions to throw exceptions. Note that this requires platform-specific runtime support
 that does not exist everywhere. Moreover, it only allows trapping instructions to throw exceptions, i.e. memory references
 or floating  point instructions. It does not allow exceptions to be thrown from arbitrary signal handlers such as SIGALRM.

I don't know how portable that is, but I think the Ada and Go frontends rely
on it, so it's probably solid.

I compared '-O2' vs '-O2 -fnon-call-exceptions' builds for code size, and
the binary grew about 3%:

 $ size gdb.O2
    text    data     bss     dec     hex filename
 9332146  918258  151232 10401636         9eb764 gdb.O2
 
 $ size gdb.O2-fnon-call-exceptions
    text    data     bss     dec     hex filename
 9628267  918882  151232 10698381         a33e8d gdb.O2-fnon-call-exceptions

For Windows I think we'd need to use SEH instead and register a translator 
with _set_se_translator, but I don't know the status of SEH support in
mingw nowadays.

Re: [PATCH 1/5] gdb: terminate upon receipt of SIGFPE

[Eli Zaretskii]

> From: Pedro Alves <pedro@palves.net>
> Cc: tom@tromey.com, gdb-patches@sourceware.org
> Date: Sat, 3 Jul 2021 19:02:50 +0100
> 
> (Note that even in C -- if you siglongjmp and skip a bunch of manual
> unwinding / restoring state in mainline code, then the program will be broken too.
> If we were still using C and old C-base implementation of exceptions/cleanups, then
> we'd siglongjmp up to the closest sigsetjmp, not to the top level.
> Longjmping to the top level directly would likely result in a broken
> GDB in a similar way.)

Yes, of course.  But Emacs does that, so I'm surprised that GDB
didn't.

> > Isn't there some way of converting a signal into a C++ exception, and
> > then unwinding "the C++ way"?
> > 
> 
> Not with standard C++.  Though as an extension, with GCC, if we build GDB
> with -fnon-call-exceptions, then we can throw from trapping instructions:
> 
>  -fnon-call-exceptions
>  Generate code that allows trapping instructions to throw exceptions. Note that this requires platform-specific runtime support
>  that does not exist everywhere. Moreover, it only allows trapping instructions to throw exceptions, i.e. memory references
>  or floating  point instructions. It does not allow exceptions to be thrown from arbitrary signal handlers such as SIGALRM.
> 
> I don't know how portable that is, but I think the Ada and Go frontends rely
> on it, so it's probably solid.

This is better than crashing, I think.

Re: [PATCH 1/5] gdb: terminate upon receipt of SIGFPE

[Pedro Alves]

On 2021-07-03 7:23 p.m., Eli Zaretskii wrote:
>> Not with standard C++.  Though as an extension, with GCC, if we build GDB
>> with -fnon-call-exceptions, then we can throw from trapping instructions:
>>
>>  -fnon-call-exceptions
>>  Generate code that allows trapping instructions to throw exceptions. Note that this requires platform-specific runtime support
>>  that does not exist everywhere. Moreover, it only allows trapping instructions to throw exceptions, i.e. memory references
>>  or floating  point instructions. It does not allow exceptions to be thrown from arbitrary signal handlers such as SIGALRM.
>>
>> I don't know how portable that is, but I think the Ada and Go frontends rely
>> on it, so it's probably solid.

> This is better than crashing, I think.
> 

We can't do the usual internal_error query in the signal handler, though,
as that's non-async-signal safe, recurses into readline, can deadlock, etc.  All we can
do is throw.  Which means that at the catch site / top level back in mainline code,
we lost the context of the bug that led to the signal.  So I think we'll still want to print
a backtrace like in Andrew's patch, and then if GDB is built with -fnon-call-exceptions, throw
an exception from the signal handler, otherwise, let the default signal action
run after printing the backtrace (i.e., terminate).  So it seems that "throw from signal
handler" work could be done in a separate patch.  Andrew's patch is already an improvement,
because today you just get GDB stuck in an infinite loop.  Do you agree?

Re: [PATCH 1/5] gdb: terminate upon receipt of SIGFPE

[Pedro Alves]

On 2021-07-02 12:06 p.m., Andrew Burgess wrote:
> GDB's SIGFPE handling is broken.

Yup.  This is PRs gdb/16505 and gdb/17891 at least.  Please mention those PRs
in the commit log so the commit is filed in bugzilla.

> 
> We currently try to use an async event token to process SIGFPE.  So,
> when a SIGFPE arrives the signal handler calls
> mark_async_signal_handler then returns, effectively ignoring the
> signal (for now).
> 
> The intention is that later the event loop will see that the async
> token associated with SIGFPE has been marked and will call the async
> handler, which just throws an error.
> 
> The problem is that SIGFPE is not safe to ignore.  Ignoring a
> SIGFPE (unless it is generated artificially, e.g. by raise()) is
> undefined behaviour, after ignoring the signal on many targets we
> return to the instruction that caused the SIGFPE to be raised, which
> immediately causes another SIGFPE to be raised, we get stuck in an
> infinite loop.  The behaviour is certainly true on x86-64.
> 
> To view this behaviour I simply added some dummy code to GDB that
> performed an integer divide by zero, compiled this on x86-64
> GNU/Linux, ran GDB and saw GDB hang.
> 
> In this commit, I propose to remove all special handling of SIGFPE and
> instead just let GDB make use of the default SIGFPE action, that is,
> to terminate the process.
> 
> The only user visible change here should be:
> 
>   - If a user sends a SIGFPE to GDB using something like kill,
>     previously GDB would just print an error and remain alive, now GDB
>     will terminate.  This is inline with what happens if the user
>     sends GDB a SIGSEGV from kill though, so I don't see this as an
>     issue.
> 
>   - If a bug in GDB causes a real SIGFPE, previously the users GDB
>     session would hang.  Now the GDB session will terminate.  Again,
>     this is inline with what happens if GDB receives a SIGSEGV due to
>     an internal bug.
> 
> gdb/ChangeLog:
> 
> 	* event-top.c (handle_sigfpe): Delete.
> 	(async_float_handler): Delete.
> 	(sigfpe_token): Delete.
> 	(handle_fatal_signal): New function.
> 	(async_init_signals): Update header comment and remove use of
> 	handle_sigfpe and sigfpe_token.

Thanks, I've been meaning to do this for ages.  LGTM.

Re: [PATCH 2/5] gdb: register signal handler after setting up event token

[Pedro Alves]

On 2021-07-02 12:06 p.m., Andrew Burgess wrote:
> This commit fixes the smallest of small possible bug related to signal
> handling.  If we look in async_init_signals we see code like this:
> 
>   signal (SIGQUIT, handle_sigquit);
>   sigquit_token =
>     create_async_signal_handler (async_do_nothing, NULL, "sigquit");
> 
> Then if we look in handle_sigquit we see code like this:
> 
>   mark_async_signal_handler (sigquit_token);
>   signal (sig, handle_sigquit);
> 
> Finally, in mark_async_signal_handler we have:
> 
>   async_handler_ptr->ready = 1;
> 
> Where async_handler_ptr will be sigquit_token.
> 
> What this means is that if a SIGQUIT arrive in async_init_signals
> after handle_sigquit has been registered, but before sigquit_token has
> been initialised, then GDB will most likely crash.
> 
> The chance of this happening is tiny, but fixing this is trivial, just
> ensure we call create_async_signal_handler before calling signal, so
> lets do that.
> 
> There are no tests for this.  Trying to land a signal in the right
> spot is pretty hit and miss.  I did try changing the current HEAD GDB
> like this:
> 
>   signal (SIGQUIT, handle_sigquit);
>   raise (SIGQUIT);
>   sigquit_token =
>     create_async_signal_handler (async_do_nothing, NULL, "sigquit");
> 
> And confirmed that this did result in a crash, after my change I tried
> this:
> 
>   sigquit_token =
>     create_async_signal_handler (async_do_nothing, NULL, "sigquit");
>   signal (SIGQUIT, handle_sigquit);
>   raise (SIGQUIT);
> 
> And GDB now starts up just fine.
> 
> gdb/ChangeLog:
> 
> 	* event-top.c (async_init_signals): For each signal, call signal
> 	only after calling create_async_signal_handler.

Looks obviously correct to me.  OK.

Re: [PATCH 3/5] gdb: rewrite header comment on async_init_signals

[Pedro Alves]

On 2021-07-02 12:06 p.m., Andrew Burgess wrote:
> The header comment on the function async_init_signals was getting
> quite out of date, this commit just rewrites to (hopefully) reflect
> the current reality.
> 
> There should be no user visible change after this commit.
> 
> gdb/ChangeLog:
> 
> 	* event-top.c (async_init_signals): Rewrite the header comment.

LGTM.

I think it'd be good to rename async_init_signals -> init_signals,
as there's no point in the "async" distinction anymore and this handles
sync signals too nowadays.

The SIGTRAP and SIGQUIT comments in the body of the function about passing
to the inferior looked mystifying at first, until I realized that by "passing",
this is talking about the disposition being inherited by gdb's
children (fork-child.c, etc.).  Since we nowadays call
restore_original_signals_state right after forking, that commentary is obsolete.
The comment about vfork and BSD4.3 seems obsolete as well, since we don't bother
to restore gdb's signal dispositions in gdb after the child execs, the and vfork
parent unblocks, and nobody complained.  We dropped support for BSD4.3 years
while ago.

I think this also means that changing SIGTRAP's disposition is pointless, we can
just let it be whatever was inherited like any other signal we don't handle.

Re: [PATCH 4/5] gdb: print backtrace on fatal SIGSEGV

[Pedro Alves]

Hi!

On 2021-07-02 12:06 p.m., Andrew Burgess wrote:

> The production of the backtrace is off by default, and can switched on
> using the new commands:
> 
>   maintenance set backtrace-on-fatal-signal on|off
>   maintenance show backtrace-on-fatal-signal

OOC, what is the rationale for making it off by default?  This seems like
one of those hard to discover features, that would be better to have on
by default, since GDB bugs may be hard to reproduce -- having it on would
mean that users could paste it in bugzilla when they file a "GDB crashed"
bug.  Such bugs reports are typically useless nowadays.

> --- a/gdb/event-top.c
> +++ b/gdb/event-top.c
> @@ -46,6 +46,10 @@
>  #include "readline/readline.h"
>  #include "readline/history.h"
>  
> +#ifdef HAVE_EXECINFO_H
> +#include <execinfo.h>
> +#endif /* HAVE_EXECINFO_H */

Would you mind indenting the #include line (with # on first column), like:

#ifdef HAVE_EXECINFO_H
# include <execinfo.h>
#endif /* HAVE_EXECINFO_H */

We do that at places, but I wish we did it everywhere.  I think it makes
the scopes clearer.

> +
>  /* readline defines this.  */
>  #undef savestring
>  
> @@ -96,6 +100,33 @@ bool exec_done_display_p = false;
>     run again.  */
>  int call_stdin_event_handler_again_p;
>  
> +/* When true GDB will produce a minimal backtrace when a fatal signal is
> +   reached (within GDB code).  */
> +static bool bt_on_fatal_signal = false;
> +
> +/* Implement 'maintenance show backtrace-on-fatal-signal'.  */
> +
> +static void
> +show_bt_on_fatal_signal (struct ui_file *file, int from_tty,
> +			 struct cmd_list_element *cmd, const char *value)
> +{
> +  fprintf_filtered (file, _("Backtrace on a fatal signal is %s.\n"), value);
> +}
> +
> +/* Implement 'maintenance set backtrace-on-fatal-signal'.  */
> +
> +static void
> +set_bt_on_fatal_signal (const char *args, int from_tty, cmd_list_element *c)
> +{
> +#ifndef HAVE_EXECINFO_BACKTRACE
> +  if (bt_on_fatal_signal)
> +    {
> +      bt_on_fatal_signal = false;
> +      error (_("support for this feature is not compiled into GDB"));
> +    }
> +#endif
> +}
> +
>  /* Signal handling variables.  */
>  /* Each of these is a pointer to a function that the event loop will
>     invoke if the corresponding signal has received.  The real signal
> @@ -846,6 +877,78 @@ gdb_readline_no_editing_callback (gdb_client_data client_data)
>  }
>  \f
>  
> +/* Attempt to unblock signal SIG, return true if the signal was unblocked,
> +   otherwise, return false.  */
> +
> +static bool
> +unblock_signal (int sig)
> +{
> +#if HAVE_SIGPROCMASK
> +  sigset_t sigset;
> +  sigemptyset (&sigset);
> +  sigaddset (&sigset, sig);
> +  gdb_sigmask (SIG_UNBLOCK, &sigset, 0);
> +  return true;
> +#elif HAVE_SIGSETMASK
> +  int mask = siggetmask ();
> +  mask &= ~sigmask (sig);
> +  sigsetmask (mask);
> +  return true;
> +#endif

I don't think this SIGSETMASK fallback is useful.  See:

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

Basically, sigprocmask exists everywhere we care about, except mingw.
And then sigsetmask doesn't exist on mingw either.

I think we can remove all HAVE_SIGSETMASK fallback code from GDB.

> +
> +  return false;
> +}
> +
> +/* Called to handle fatal signals.  SIG is the signal number.  */
> +
> +static void __attribute__ ((__noreturn__))

ATTRIBUTE_NORETURN

> +handle_fatal_signal (int sig)
> +{
> +#ifdef HAVE_EXECINFO_BACKTRACE
> +  const auto sig_write = [] (const char *msg) -> void {
> +    gdb_stderr->write_async_safe (msg, strlen (msg));
> +  };

Brace:

  const auto sig_write = [] (const char *msg) -> void 
  {
    gdb_stderr->write_async_safe (msg, strlen (msg));
  };

> +
> +  if (bt_on_fatal_signal)
> +    {
> +      /* Ensure we don't get stuck trying to handle signals recursively.  */
> +      signal (sig, SIG_DFL);

Hmm, the signal will be automatically blocked on entry as we don't
use SA_NODEFER (*), so I don't think recursion can happen.  If something
synchronously raises a segfault inside the handler with the signal blocked,
the process will terminate immediately.

(* - well, except on systems with sysv semantics, which means Solaris, but
then there the disposition is already SIG_DFL on entry anyhow.)

I was pondering the race case of two threads segfaulting at the same time,
and this signal disposition juggling.  Since this sets the disposition
to SIG_DFL before printing the backtrace, it means that if two threads
segfault at the same time, the second thread can terminate the process
without a backtrace being printed.

If we we printed backtrace for thread A, and only after set the disposition
to SIG_DFL, then if thread B segfaults before thread A re-raises the signal
GDB will just terminate, which is the same as if thread A manages to re-raise,
so we'd be good.  We'll still have printed one backtrace anyhow.

I'm thinking that you should just remove that SIG_DFL call.

> +
> +      sig_write ("\n\nFatal signal: ");
> +      sig_write (strsignal (sig));
> +      sig_write ("\n");
> +
> +      /* Allow up to 25 frames of backtrace.  */
> +      void *buffer[25];
> +      int frames = backtrace (buffer, sizeof(buffer) / sizeof(buffer[0]));

ARRAY_SIZE

> +      sig_write ("----- Backtrace -----\n");
> +      if (gdb_stderr->fd () > -1)
> +	{
> +	  backtrace_symbols_fd (buffer, frames, gdb_stderr->fd ());
> +	  if (frames == (sizeof(buffer) / sizeof(buffer[0])))
> +	    sig_write ("Backtrace might be incomplete.\n");
> +	}
> +      else
> +	sig_write ("Backtrace unavailable\n");
> +      sig_write ("---------------------\n");
> +      gdb_stderr->flush ();

I've double checked that backtrace_symbols_fd is async-signal-safe to use in signals, cool.

> +# Now the actual test loop.
> +foreach test_data {{SEGV "Segmentation fault"}} {
> +    set sig [lindex ${test_data} 0]
> +    set msg [lindex ${test_data} 1]
> +    with_test_prefix ${sig} {
> +
> +	# Restart GDB.
> +	clean_restart $binfile
> +
> +	# Capture the pid of GDB.
> +	set testpid [spawn_id_get_pid $inferior_spawn_id]

This isn't always gdb's spawn id:

 # The spawn ID used for I/O interaction with the inferior.  For native
 # targets, or remote targets that can do I/O through GDB
 # (semi-hosting) this will be the same as the host/GDB's spawn ID.
 # Otherwise, the board may set this to some other spawn ID.  E.g.,
 # when debugging with GDBserver, this is set to GDBserver's spawn ID,
 # so input/output is done on gdbserver's tty.
 global inferior_spawn_id

I think you wanted gdb_spawn_id.

Please confirm the test works when testing with gdbserver.

> +
> +	# Start the inferior.
> +	runto_main
> +
> +	# Turn on the backtrace-on-fatal-signal feature.
> +	gdb_test_no_output "maint set backtrace-on-fatal-signal on"
> +
> +	# Flags for various bits of the output we expect to see, we
> +	# check for these in the gdb_test_multiple below.
> +	set saw_fatal_msg False
> +	set saw_bt_start False
> +	set saw_bt_end False

OOC, why uppercase "False"?  (and True below.)

> +
> +	# Send the fatal signal to GDB.
> +	eval exec kill -${sig} ${testpid}

This isn't remote-host testing safe.  You'd kill the poor random
$testpid process on the build machine.  Should use instead:

 remote_exec host "kill -${sig} ${testpid}"

However, actually, with remote host testing, I think this will
send the signal to ssh instead of gdb.  So I guess better to skip
testing with remote-host testing.


> +
> +	# Scan GDB's output for the backtrace.
> +	gdb_test_multiple "" "scan for backtrace" {
> +	    -re "^\r\n" {
> +		exp_continue
> +	    }
> +	    -re "^Fatal signal: ${msg}\r\n" {
> +		set saw_fatal_msg True
> +		exp_continue
> +	    }
> +	    -re "^----- Backtrace -----\r\n" {
> +		set saw_bt_start True
> +		exp_continue
> +	    }
> +	    -re ".+\r\n---------------------\r\n" {
> +		set saw_bt_end True
> +		exp_continue
> +	    }
> +	    eof {
> +		# Catch the eof case as this indicates that GDB has
> +		# gone away, which in this case, is what we expect to
> +		# happen.
> +		gdb_assert { $saw_fatal_msg }
> +		gdb_assert { $saw_bt_start }
> +		gdb_assert { $saw_bt_end }

Shouldn't we wait + close + clear spawn_id?  See e.g. gdb.base/quit.exp.

> +	    }
> +	    -re "$gdb_prompt $" {
> +		# GDB should terminate, we should never get back to
> +		# the prompt.
> +		fail $gdb_test_name
> +	    }
> +	}
> +
> +	# GDB is now dead and gone.
> +    }

Re: [PATCH 4/5] gdb: print backtrace on fatal SIGSEGV

[Pedro Alves]

On 2021-07-02 12:06 p.m., Andrew Burgess wrote:

> So, currently, this is only effecting our handling of SIGSEGV.
> Previously, if GDB hit a SEGV then we would terminate GDB with a
> SIGABRT.  After this commit we will terminate GDB with a SIGSEGV.
> This feels like an improvement to me, we should still get a core dump,
> but in many shells, the user will see a more specific message once GDB
> exits, in bash for example "Segmentation fault" rather than "Aborted".

(forgot to merge this comment with the other email)

Someone once said "A picture is worth a thousand words".  :-)
Could you show us (and include in the commit log) an example of a GDB backtrace
we'd get?

Re: [PATCH 4/5] gdb: print backtrace on fatal SIGSEGV

[Pedro Alves]

On 2021-07-02 12:47 p.m., Eli Zaretskii via Gdb-patches wrote:
>> From: Andrew Burgess <andrew.burgess@embecosm.com>
>> Date: Fri,  2 Jul 2021 12:06:07 +0100
>> So, currently, this is only effecting our handling of SIGSEGV.
>> Previously, if GDB hit a SEGV then we would terminate GDB with a
>> SIGABRT.  After this commit we will terminate GDB with a SIGSEGV.
>> This feels like an improvement to me, we should still get a core dump,
>> but in many shells, the user will see a more specific message once GDB
>> exits, in bash for example "Segmentation fault" rather than "Aborted".

> But will GDB still present the "internal error, submit a bug report"
> message when a fatal signal such as SIGSEGV is encountered?
> 

Your use of "still" seems to imply that that's what happens today, but it
isn't.  GDB currently just aborts, no questions asked.

Re: [PATCH 5/5] gdb: register SIGBUS, SIGFPE, and SIGABRT handlers

[Pedro Alves]

On 2021-07-02 12:06 p.m., Andrew Burgess wrote:
> Register handlers for SIGBUS, SIGFPE, and SIGABRT.  All of these
> signals are setup as fatal signals that will cause GDB to terminate.
> However, by passing these signals through the handle_fatal_signal
> function, a user can arrange to see a backtrace when GDB
> terminates (see maint set backtrace-on-fatal-signal).
> 
> In normal use of GDB there should be no user visible changes after
> this commit.  Only if GDB terminates with one of the above signals
> will GDB change slightly, potentially printing a backtrace before
> aborting.
> 
> gdb/ChangeLog:
> 
> 	* event-top.c (async_init_signals): Handle SIGFPE, SIGBUS, and
> 	SIGABRT.  Update header comment.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.base/bt-on-fatal-signal.exp: Add additional tests.

I'd rather this said "Also test SIGFPE, SIGBUS, and SIGABRT."

Otherwise LGTM.

Re: [PATCH 1/5] gdb: terminate upon receipt of SIGFPE

[Eli Zaretskii]

> From: Pedro Alves <pedro@palves.net>
> Cc: tom@tromey.com, gdb-patches@sourceware.org
> Date: Sat, 3 Jul 2021 23:52:28 +0100
> 
> >>  -fnon-call-exceptions
> >>  Generate code that allows trapping instructions to throw exceptions. Note that this requires platform-specific runtime support
> >>  that does not exist everywhere. Moreover, it only allows trapping instructions to throw exceptions, i.e. memory references
> >>  or floating  point instructions. It does not allow exceptions to be thrown from arbitrary signal handlers such as SIGALRM.
> >>
> >> I don't know how portable that is, but I think the Ada and Go frontends rely
> >> on it, so it's probably solid.
> 
> > This is better than crashing, I think.
> > 
> 
> We can't do the usual internal_error query in the signal handler, though,
> as that's non-async-signal safe, recurses into readline, can deadlock, etc.  All we can
> do is throw.  Which means that at the catch site / top level back in mainline code,
> we lost the context of the bug that led to the signal.

At least some of the context could be passed via the exception itself,
no?

> So I think we'll still want to print
> a backtrace like in Andrew's patch, and then if GDB is built with -fnon-call-exceptions, throw
> an exception from the signal handler, otherwise, let the default signal action
> run after printing the backtrace (i.e., terminate).  So it seems that "throw from signal
> handler" work could be done in a separate patch.  Andrew's patch is already an improvement,
> because today you just get GDB stuck in an infinite loop.  Do you agree?

I didn't object to installing the patch, I just wondered whether
terminating is the only feasible way we have to handle these
situations.

Re: [PATCH 4/5] gdb: print backtrace on fatal SIGSEGV

[Eli Zaretskii]

> From: Pedro Alves <pedro@palves.net>
> Cc: gdb-patches@sourceware.org
> Date: Sun, 4 Jul 2021 01:55:02 +0100
> 
> On 2021-07-02 12:47 p.m., Eli Zaretskii via Gdb-patches wrote:
> >> From: Andrew Burgess <andrew.burgess@embecosm.com>
> >> Date: Fri,  2 Jul 2021 12:06:07 +0100
> >> So, currently, this is only effecting our handling of SIGSEGV.
> >> Previously, if GDB hit a SEGV then we would terminate GDB with a
> >> SIGABRT.  After this commit we will terminate GDB with a SIGSEGV.
> >> This feels like an improvement to me, we should still get a core dump,
> >> but in many shells, the user will see a more specific message once GDB
> >> exits, in bash for example "Segmentation fault" rather than "Aborted".
> 
> > But will GDB still present the "internal error, submit a bug report"
> > message when a fatal signal such as SIGSEGV is encountered?
> > 
> 
> Your use of "still" seems to imply that that's what happens today, but it
> isn't.  GDB currently just aborts, no questions asked.

And the answer to the _real_ question I asked is...?

Re: [PATCH 4/5] gdb: print backtrace on fatal SIGSEGV

[Pedro Alves]

On 2021-07-04 5:32 a.m., Eli Zaretskii wrote:
>> From: Pedro Alves <pedro@palves.net>
>> Cc: gdb-patches@sourceware.org
>> Date: Sun, 4 Jul 2021 01:55:02 +0100
>>
>> On 2021-07-02 12:47 p.m., Eli Zaretskii via Gdb-patches wrote:
>>>> From: Andrew Burgess <andrew.burgess@embecosm.com>
>>>> Date: Fri,  2 Jul 2021 12:06:07 +0100
>>>> So, currently, this is only effecting our handling of SIGSEGV.
>>>> Previously, if GDB hit a SEGV then we would terminate GDB with a
>>>> SIGABRT.  After this commit we will terminate GDB with a SIGSEGV.
>>>> This feels like an improvement to me, we should still get a core dump,
>>>> but in many shells, the user will see a more specific message once GDB
>>>> exits, in bash for example "Segmentation fault" rather than "Aborted".
>>
>>> But will GDB still present the "internal error, submit a bug report"
>>> message when a fatal signal such as SIGSEGV is encountered?
>>>
>>
>> Your use of "still" seems to imply that that's what happens today, but it
>> isn't.  GDB currently just aborts, no questions asked.
> 
> And the answer to the _real_ question I asked is...?
> 

That was the answer.  The patch does not remove the "internal error ..."
message, there was never any.  It does not add one either.  Before the patch, GDB
dumps core (w/ SIGABRT), and after patch it prints a backtrace and then
dumps core (w/ SIGSEGV).

Re: [PATCH 4/5] gdb: print backtrace on fatal SIGSEGV

[Eli Zaretskii]

> From: Pedro Alves <pedro@palves.net>
> Cc: andrew.burgess@embecosm.com, gdb-patches@sourceware.org
> Date: Sun, 4 Jul 2021 15:32:19 +0100
> 
> That was the answer.  The patch does not remove the "internal error ..."
> message, there was never any.  It does not add one either.  Before the patch, GDB
> dumps core (w/ SIGABRT), and after patch it prints a backtrace and then
> dumps core (w/ SIGSEGV).

Shouldn't we add the "internal error" message?  That's what those
fatal signals are, after all, right?

Re: [PATCH 1/5] gdb: terminate upon receipt of SIGFPE

[Pedro Alves]

On 2021-07-04 5:27 a.m., Eli Zaretskii wrote:
>> From: Pedro Alves <pedro@palves.net>
>> Cc: tom@tromey.com, gdb-patches@sourceware.org
>> Date: Sat, 3 Jul 2021 23:52:28 +0100
>>
>>>>  -fnon-call-exceptions
>>>>  Generate code that allows trapping instructions to throw exceptions. Note that this requires platform-specific runtime support
>>>>  that does not exist everywhere. Moreover, it only allows trapping instructions to throw exceptions, i.e. memory references
>>>>  or floating  point instructions. It does not allow exceptions to be thrown from arbitrary signal handlers such as SIGALRM.
>>>>
>>>> I don't know how portable that is, but I think the Ada and Go frontends rely
>>>> on it, so it's probably solid.
>>
>>> This is better than crashing, I think.
>>>
>>
>> We can't do the usual internal_error query in the signal handler, though,
>> as that's non-async-signal safe, recurses into readline, can deadlock, etc.  All we can
>> do is throw.  Which means that at the catch site / top level back in mainline code,
>> we lost the context of the bug that led to the signal.
> 
> At least some of the context could be passed via the exception itself,
> no?

The PCs array that "backtrace" produces could be passed, but that's not very helpful
I think, as it's just what you get with the printed backtrace.  What I meant by lost
context is that when we ask the "internal error, do you want to ..." queries, the user can
dump core, which can then be used to debug the problem with the full context
of the crash at hand.  If we throw, then catch, the context of the crash is
mostly gone, the stack has been unwound, global state restored, etc.  A core
dump generated after the catch isn't going to be of much use.

I thought that we could maybe unconditionally fork + abort before throwing (depending
on an option) without querying, but fork in signal handlers (despite being supposedly
async-signal-safe) unfortunately isn't really safe:

 https://access.redhat.com/articles/2921161

Actually, I was wondering whether throwing a C++ exception relies on malloc even
for small exception objects, and turns out it does...  :-/   Here's an exception
thrown from a signal handler:

 #0  0x00007ffff7af7450 in malloc () from /lib64/libc.so.6
 #1  0x00007ffff7e3f294 in __cxa_allocate_exception () from /lib64/libstdc++.so.6
 #2  0x000000000040131d in handler (signal=11) at sig-exception2.cc:15

The more I think about this, the more I think that it has lots of potential
for misbehaving...  I'm currently thinking that the benefit doesn't seem worth it
of the potential complications, deadlocks, etc.

> 
>> So I think we'll still want to print
>> a backtrace like in Andrew's patch, and then if GDB is built with -fnon-call-exceptions, throw
>> an exception from the signal handler, otherwise, let the default signal action
>> run after printing the backtrace (i.e., terminate).  So it seems that "throw from signal
>> handler" work could be done in a separate patch.  Andrew's patch is already an improvement,
>> because today you just get GDB stuck in an infinite loop.  Do you agree?
> 
> I didn't object to installing the patch, I just wondered whether
> terminating is the only feasible way we have to handle these
> situations.
> 

Re: [PATCH 4/5] gdb: print backtrace on fatal SIGSEGV

[Pedro Alves]

On 2021-07-04 3:38 p.m., Eli Zaretskii wrote:
>> From: Pedro Alves <pedro@palves.net>
>> Cc: andrew.burgess@embecosm.com, gdb-patches@sourceware.org
>> Date: Sun, 4 Jul 2021 15:32:19 +0100
>>
>> That was the answer.  The patch does not remove the "internal error ..."
>> message, there was never any.  It does not add one either.  Before the patch, GDB
>> dumps core (w/ SIGABRT), and after patch it prints a backtrace and then
>> dumps core (w/ SIGSEGV).
> 
> Shouldn't we add the "internal error" message?  That's what those
> fatal signals are, after all, right?
> 

Can you clarify what you mean by "message"?  When we talk about that, I'm thinking
that you're talking about the whole shebang, including the querying whether to exit
and dump core, like:

(gdb) maint internal-error 
../../gdb/maint.c:91: internal-error: 
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Quit this debugging session? (y or n) n

This is a bug, please report it.  For instructions, see:
<http://www.gnu.org/software/gdb/bugs/>.

../../gdb/maint.c:91: internal-error: 
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Create a core file of GDB? (y or n) n
Command aborted.
(gdb) 


All this querying isn't async-signal-safe, we couldn't do it from the signal handler.

We can of course print something similar to just the parts without a query:

 A problem internal to GDB has been detected,
 further debugging may prove unreliable.

 This is a bug, please report it.  For instructions, see:
 <http://www.gnu.org/software/gdb/bugs/>.

Maybe say a "fatal signal" instead of "problem internal", something like that.

The patch doesn't do that, but it does sound like a good idea to make it do it.

This is the sort of detail that made me ask for an example of what the backtrace
looks like.

Re: [PATCH 1/5] gdb: terminate upon receipt of SIGFPE

[Eli Zaretskii]

> From: Pedro Alves <pedro@palves.net>
> Cc: tom@tromey.com, gdb-patches@sourceware.org
> Date: Sun, 4 Jul 2021 15:51:55 +0100
> 
> > At least some of the context could be passed via the exception itself,
> > no?
> 
> The PCs array that "backtrace" produces could be passed, but that's not very helpful
> I think, as it's just what you get with the printed backtrace.  What I meant by lost
> context is that when we ask the "internal error, do you want to ..." queries, the user can
> dump core, which can then be used to debug the problem with the full context
> of the crash at hand.  If we throw, then catch, the context of the crash is
> mostly gone, the stack has been unwound, global state restored, etc.  A core
> dump generated after the catch isn't going to be of much use.

I don't think that kind of detailed context will help in these cases:
fatal signals cannot be explained in easy enough terms understood by
everyone.  What I meant by "context" was something at a higher level,
like the command that triggered that.  And the fatal signal itself, of
course.  Anything beyond that won't be helpful, I think.

Re: [PATCH 4/5] gdb: print backtrace on fatal SIGSEGV

[Eli Zaretskii]

> From: Pedro Alves <pedro@palves.net>
> Cc: andrew.burgess@embecosm.com, gdb-patches@sourceware.org
> Date: Sun, 4 Jul 2021 16:03:55 +0100
> 
> > Shouldn't we add the "internal error" message?  That's what those
> > fatal signals are, after all, right?
> > 
> 
> Can you clarify what you mean by "message"?

I'll try.

> (gdb) maint internal-error 
> ../../gdb/maint.c:91: internal-error: 
> A problem internal to GDB has been detected,
> further debugging may prove unreliable.
> Quit this debugging session? (y or n) n
> 
> This is a bug, please report it.  For instructions, see:
> <http://www.gnu.org/software/gdb/bugs/>.
> 
> ../../gdb/maint.c:91: internal-error: 
> A problem internal to GDB has been detected,
> further debugging may prove unreliable.
> Create a core file of GDB? (y or n) n
> Command aborted.
> (gdb) 

Why not display all of the above?  That's what I had in mind, anyway.

> All this querying isn't async-signal-safe, we couldn't do it from the signal handler.

Can we do that after unwinding, when we are out of the signal handler?

> We can of course print something similar to just the parts without a query:
> 
>  A problem internal to GDB has been detected,
>  further debugging may prove unreliable.
> 
>  This is a bug, please report it.  For instructions, see:
>  <http://www.gnu.org/software/gdb/bugs/>.
> 
> Maybe say a "fatal signal" instead of "problem internal", something like that.

If the same message as with assertion is not feasible, then yes, the
above partial message would also be an improvement.

[PATCHv2 0/6] GDB Synchronous Signal Handling

[Andrew Burgess]

Thanks for all of the great feedback on v1 of the patch.

For patch #1, I have not made any attempt to "recover" after the fatal
signal.  I think Pedro said everything I would want to say, basically,
I'm not sure there's a general way (i.e. covering all of GDB) that we
could recover without leaving GDB is a pretty broken state.  If anyone
knows different then I'm happy to revisit this idea.

Patch #2 is unchanged.

Patch #3 is changed to rename the function as Pedro suggested, and to
remove the dead code that was pointed out.

Patch #4 I've extended the message printed along with the backtrace to
include words that are similar to those given when we hit an internal
error.  There's an example of this output in the commit message.  The
backtrace is now on by default.  As was discussed in the comment
thread for this patch, querying the user isn't really possible from
within the signal handler, and even if we could somehow "recover" from
the signal handler back to GDB's main loop, dumping a core file at
that point would be pretty pointless.

Patch #5 is mostly unchanged, small tweak to the commit message.

Patch #6 is new, addresses the fact that the new backtrace was being
produced when we hit an internal error, which was never my original
intention.

---

Andrew Burgess (6):
  gdb: terminate upon receipt of SIGFPE
  gdb: register signal handler after setting up event token
  gdb: rename async_init_signals to gdb_init_signals
  gdb: print backtrace on fatal SIGSEGV
  gdb: register SIGBUS, SIGFPE, and SIGABRT handlers
  gdb: don't print backtrace when dumping core after an internal error

 gdb/NEWS                                      |   7 +
 gdb/config.in                                 |   6 +
 gdb/configure                                 |  51 ++++
 gdb/configure.ac                              |  22 ++
 gdb/doc/gdb.texinfo                           |  18 ++
 gdb/event-top.c                               | 227 +++++++++++++-----
 gdb/event-top.h                               |   2 +-
 gdb/testsuite/gdb.base/bt-on-fatal-signal.c   |  22 ++
 gdb/testsuite/gdb.base/bt-on-fatal-signal.exp | 173 +++++++++++++
 gdb/top.c                                     |   2 +-
 gdb/ui-file.h                                 |   9 +
 gdb/utils.c                                   |   5 +
 12 files changed, 478 insertions(+), 66 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/bt-on-fatal-signal.c
 create mode 100644 gdb/testsuite/gdb.base/bt-on-fatal-signal.exp

-- 
2.25.4

[PATCHv2 1/6] gdb: terminate upon receipt of SIGFPE

[Andrew Burgess]

GDB's SIGFPE handling is broken, this is PR gdb/16505 and
PR gdb/17891.

We currently try to use an async event token to process SIGFPE.  So,
when a SIGFPE arrives the signal handler calls
mark_async_signal_handler then returns, effectively ignoring the
signal (for now).

The intention is that later the event loop will see that the async
token associated with SIGFPE has been marked and will call the async
handler, which just throws an error.

The problem is that SIGFPE is not safe to ignore.  Ignoring a
SIGFPE (unless it is generated artificially, e.g. by raise()) is
undefined behaviour, after ignoring the signal on many targets we
return to the instruction that caused the SIGFPE to be raised, which
immediately causes another SIGFPE to be raised, we get stuck in an
infinite loop.  The behaviour is certainly true on x86-64.

To view this behaviour I simply added some dummy code to GDB that
performed an integer divide by zero, compiled this on x86-64
GNU/Linux, ran GDB and saw GDB hang.

In this commit, I propose to remove all special handling of SIGFPE and
instead just let GDB make use of the default SIGFPE action, that is,
to terminate the process.

The only user visible change here should be:

  - If a user sends a SIGFPE to GDB using something like kill,
    previously GDB would just print an error and remain alive, now GDB
    will terminate.  This is inline with what happens if the user
    sends GDB a SIGSEGV from kill though, so I don't see this as an
    issue.

  - If a bug in GDB causes a real SIGFPE, previously the users GDB
    session would hang.  Now the GDB session will terminate.  Again,
    this is inline with what happens if GDB receives a SIGSEGV due to
    an internal bug.

In bug gdb/16505 there is mention that it would be nice if GDB did
more than just terminate when receiving a fatal signal.  I haven't
done that in this commit, but later commits will move in that
direction.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=16505
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=17891
---
 gdb/event-top.c | 25 +------------------------
 1 file changed, 1 insertion(+), 24 deletions(-)

diff --git a/gdb/event-top.c b/gdb/event-top.c
index 002a7dc95e0..ab5179b7d32 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -58,7 +58,6 @@ static void handle_sigquit (int sig);
 #ifdef SIGHUP
 static void handle_sighup (int sig);
 #endif
-static void handle_sigfpe (int sig);
 
 /* Functions to be invoked by the event loop in response to
    signals.  */
@@ -68,7 +67,6 @@ static void async_do_nothing (gdb_client_data);
 #ifdef SIGHUP
 static void async_disconnect (gdb_client_data);
 #endif
-static void async_float_handler (gdb_client_data);
 #ifdef SIGTSTP
 static void async_sigtstp_handler (gdb_client_data);
 #endif
@@ -111,7 +109,6 @@ static struct async_signal_handler *sighup_token;
 #ifdef SIGQUIT
 static struct async_signal_handler *sigquit_token;
 #endif
-static struct async_signal_handler *sigfpe_token;
 #ifdef SIGTSTP
 static struct async_signal_handler *sigtstp_token;
 #endif
@@ -904,7 +901,7 @@ static struct serial_event *quit_serial_event;
 
 /* Initialization of signal handlers and tokens.  There is a function
    handle_sig* for each of the signals GDB cares about.  Specifically:
-   SIGINT, SIGFPE, SIGQUIT, SIGTSTP, SIGHUP, SIGWINCH.  These
+   SIGINT, SIGQUIT, SIGTSTP, SIGHUP, SIGWINCH.  These
    functions are the actual signal handlers associated to the signals
    via calls to signal().  The only job for these functions is to
    enqueue the appropriate event/procedure with the event loop.  Such
@@ -955,9 +952,6 @@ async_init_signals (void)
     sighup_token =
       create_async_signal_handler (async_do_nothing, NULL, "sighup");
 #endif
-  signal (SIGFPE, handle_sigfpe);
-  sigfpe_token =
-    create_async_signal_handler (async_float_handler, NULL, "sigfpe");
 
 #ifdef SIGTSTP
   sigtstp_token =
@@ -1198,23 +1192,6 @@ async_sigtstp_handler (gdb_client_data arg)
 }
 #endif /* SIGTSTP */
 
-/* Tell the event loop what to do if SIGFPE is received.
-   See event-signal.c.  */
-static void
-handle_sigfpe (int sig)
-{
-  mark_async_signal_handler (sigfpe_token);
-  signal (sig, handle_sigfpe);
-}
-
-/* Event loop will call this function to process a SIGFPE.  */
-static void
-async_float_handler (gdb_client_data arg)
-{
-  /* This message is based on ANSI C, section 4.7.  Note that integer
-     divide by zero causes this, so "float" is a misnomer.  */
-  error (_("Erroneous arithmetic operation."));
-}
 \f
 
 /* Set things up for readline to be invoked via the alternate
-- 
2.25.4

[PATCHv2 2/6] gdb: register signal handler after setting up event token

[Andrew Burgess]

This commit fixes the smallest of small possible bug related to signal
handling.  If we look in async_init_signals we see code like this:

  signal (SIGQUIT, handle_sigquit);
  sigquit_token =
    create_async_signal_handler (async_do_nothing, NULL, "sigquit");

Then if we look in handle_sigquit we see code like this:

  mark_async_signal_handler (sigquit_token);
  signal (sig, handle_sigquit);

Finally, in mark_async_signal_handler we have:

  async_handler_ptr->ready = 1;

Where async_handler_ptr will be sigquit_token.

What this means is that if a SIGQUIT arrive in async_init_signals
after handle_sigquit has been registered, but before sigquit_token has
been initialised, then GDB will most likely crash.

The chance of this happening is tiny, but fixing this is trivial, just
ensure we call create_async_signal_handler before calling signal, so
lets do that.

There are no tests for this.  Trying to land a signal in the right
spot is pretty hit and miss.  I did try changing the current HEAD GDB
like this:

  signal (SIGQUIT, handle_sigquit);
  raise (SIGQUIT);
  sigquit_token =
    create_async_signal_handler (async_do_nothing, NULL, "sigquit");

And confirmed that this did result in a crash, after my change I tried
this:

  sigquit_token =
    create_async_signal_handler (async_do_nothing, NULL, "sigquit");
  signal (SIGQUIT, handle_sigquit);
  raise (SIGQUIT);

And GDB now starts up just fine.

gdb/ChangeLog:

	* event-top.c (async_init_signals): For each signal, call signal
	only after calling create_async_signal_handler.
---
 gdb/event-top.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/gdb/event-top.c b/gdb/event-top.c
index ab5179b7d32..2d3bfa6a9c9 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -918,12 +918,13 @@ async_init_signals (void)
 
   quit_serial_event = make_serial_event ();
 
-  signal (SIGINT, handle_sigint);
   sigint_token =
     create_async_signal_handler (async_request_quit, NULL, "sigint");
-  signal (SIGTERM, handle_sigterm);
+  signal (SIGINT, handle_sigint);
+
   async_sigterm_token
     = create_async_signal_handler (async_sigterm_handler, NULL, "sigterm");
+  signal (SIGTERM, handle_sigterm);
 
   /* If SIGTRAP was set to SIG_IGN, then the SIG_IGN will get passed
      to the inferior and breakpoints will be ignored.  */
@@ -940,10 +941,11 @@ async_init_signals (void)
      might be in memory, shared between the two).  Since we establish
      a handler for SIGQUIT, when we call exec it will set the signal
      to SIG_DFL for us.  */
-  signal (SIGQUIT, handle_sigquit);
   sigquit_token =
     create_async_signal_handler (async_do_nothing, NULL, "sigquit");
+  signal (SIGQUIT, handle_sigquit);
 #endif
+
 #ifdef SIGHUP
   if (signal (SIGHUP, handle_sighup) != SIG_IGN)
     sighup_token =
-- 
2.25.4

[PATCHv2 3/6] gdb: rename async_init_signals to gdb_init_signals

[Andrew Burgess]

The async_init_signals has, for some time, dealt with async and sync
signals, so removing the async prefix makes sense I think.

Additionally, as pointed out by Pedro:

  .....

The comments relating to SIGTRAP and SIGQUIT within this function are
out of date.

The comments for SIGTRAP talk about the signal disposition (SIG_IGN)
being passed to the inferior, meaning the signal disposition being
inherited by GDB's fork children.  However, we now call
restore_original_signals_state prior to forking, so the comment on
SIGTRAP is redundant.

The comments for SIGQUIT are similarly out of date, further, the
comment on SIGQUIT talks about problems with BSD4.3 and vfork,
however, we have not supported BSD4.3 for several years now.

Given the above, it seems that changing the disposition of SIGTRAP is
no longer needed, so I've deleted the signal() call for SIGTRAP.

Finally, the header comment on the function now called
gdb_init_signals was getting quite out of date, so I've updated it
to (hopefully) better reflect reality.

There should be no user visible change after this commit.
---
 gdb/event-top.c | 41 ++++++++++++++---------------------------
 gdb/event-top.h |  2 +-
 gdb/top.c       |  2 +-
 3 files changed, 16 insertions(+), 29 deletions(-)

diff --git a/gdb/event-top.c b/gdb/event-top.c
index 2d3bfa6a9c9..972b540c8b8 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -899,20 +899,21 @@ handle_sigsegv (int sig)
    handler.  */
 static struct serial_event *quit_serial_event;
 
-/* Initialization of signal handlers and tokens.  There is a function
-   handle_sig* for each of the signals GDB cares about.  Specifically:
-   SIGINT, SIGQUIT, SIGTSTP, SIGHUP, SIGWINCH.  These
-   functions are the actual signal handlers associated to the signals
-   via calls to signal().  The only job for these functions is to
-   enqueue the appropriate event/procedure with the event loop.  Such
-   procedures are the old signal handlers.  The event loop will take
-   care of invoking the queued procedures to perform the usual tasks
-   associated with the reception of the signal.  */
-/* NOTE: 1999-04-30 This is the asynchronous version of init_signals.
-   init_signals will become obsolete as we move to have to event loop
-   as the default for gdb.  */
+/* Initialization of signal handlers and tokens.  There are a number of
+   different strategies for handling different signals here.
+
+   For SIGINT, SIGTERM, SIGQUIT, SIGHUP, SIGTSTP, there is a function
+   handle_sig* for each of these signals.  These functions are the actual
+   signal handlers associated to the signals via calls to signal().  The
+   only job for these functions is to enqueue the appropriate
+   event/procedure with the event loop.  The event loop will take care of
+   invoking the queued procedures to perform the usual tasks associated
+   with the reception of the signal.
+
+   For SIGSEGV the handle_sig* function does all the work for handling this
+   signal.  */
 void
-async_init_signals (void)
+gdb_init_signals (void)
 {
   initialize_async_signal_handlers ();
 
@@ -926,21 +927,7 @@ async_init_signals (void)
     = create_async_signal_handler (async_sigterm_handler, NULL, "sigterm");
   signal (SIGTERM, handle_sigterm);
 
-  /* If SIGTRAP was set to SIG_IGN, then the SIG_IGN will get passed
-     to the inferior and breakpoints will be ignored.  */
-#ifdef SIGTRAP
-  signal (SIGTRAP, SIG_DFL);
-#endif
-
 #ifdef SIGQUIT
-  /* If we initialize SIGQUIT to SIG_IGN, then the SIG_IGN will get
-     passed to the inferior, which we don't want.  It would be
-     possible to do a "signal (SIGQUIT, SIG_DFL)" after we fork, but
-     on BSD4.3 systems using vfork, that can affect the
-     GDB process as well as the inferior (the signal handling tables
-     might be in memory, shared between the two).  Since we establish
-     a handler for SIGQUIT, when we call exec it will set the signal
-     to SIG_DFL for us.  */
   sigquit_token =
     create_async_signal_handler (async_do_nothing, NULL, "sigquit");
   signal (SIGQUIT, handle_sigquit);
diff --git a/gdb/event-top.h b/gdb/event-top.h
index b036f1385c5..4947dd27819 100644
--- a/gdb/event-top.h
+++ b/gdb/event-top.h
@@ -32,7 +32,7 @@ struct cmd_list_element;
 extern void display_gdb_prompt (const char *new_prompt);
 extern void gdb_setup_readline (int);
 extern void gdb_disable_readline (void);
-extern void async_init_signals (void);
+extern void gdb_init_signals (void);
 extern void change_line_handler (int);
 
 extern void command_line_handler (gdb::unique_xmalloc_ptr<char> &&rl);
diff --git a/gdb/top.c b/gdb/top.c
index 6e0f43d2fd9..0c49f4f9eb4 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -2403,7 +2403,7 @@ gdb_init ()
      to alter it.  */
   set_initial_gdb_ttystate ();
 
-  async_init_signals ();
+  gdb_init_signals ();
 
   /* We need a default language for parsing expressions, so simple
      things like "set width 0" won't fail if no language is explicitly
-- 
2.25.4

[PATCHv2 4/6] gdb: print backtrace on fatal SIGSEGV

[Andrew Burgess]

This commit adds a new maintenance feature, the ability to print
a (limited) backtrace if GDB dies due to a fatal signal.

The backtrace is produced using the backtrace and backtrace_symbols_fd
functions which are declared in the execinfo.h header, and both of
which are async signal safe.  A configure check has been added to
check for these features, if they are not available then the new code
is not compiled into GDB and the backtrace will not be printed.

The motivation for this new feature is to aid in debugging GDB in
situations where GDB has crashed at a users site, but the user is
reluctant to share core files, possibly due to concerns about what
might be in the memory image within the core file.  Such a user might
be happy to share a simple backtrace that was written to stderr.

The production of the backtrace is on by default, but can switched off
using the new commands:

  maintenance set backtrace-on-fatal-signal on|off
  maintenance show backtrace-on-fatal-signal

Right now, I have hooked this feature in to GDB's existing handling of
SIGSEGV only, but this will be extended to more signals in a later
commit.

One additional change I have made in this commit is that, when we
decide GDB should terminate due to the fatal signal, we now
raise the same fatal signal rather than raising SIGABRT.

Currently, this is only effecting our handling of SIGSEGV.  So,
previously, if GDB hit a SEGV then we would terminate GDB with a
SIGABRT.  After this commit we will terminate GDB with a SIGSEGV.

This feels like an improvement to me, we should still get a core dump,
but in many shells, the user will see a more specific message once GDB
exits, in bash for example "Segmentation fault" rather than "Aborted".

Finally then, here is an example of the output a user would see if GDB
should hit an internal SIGSEGV:

  Fatal signal: Segmentation fault
  ----- Backtrace -----
  ./gdb/gdb[0x8078e6]
  ./gdb/gdb[0x807b20]
  /lib64/libpthread.so.0(+0x14b20)[0x7f6648c92b20]
  /lib64/libc.so.6(__poll+0x4f)[0x7f66484d3a5f]
  ./gdb/gdb[0x1540f4c]
  ./gdb/gdb[0x154034a]
  ./gdb/gdb[0x9b002d]
  ./gdb/gdb[0x9b014d]
  ./gdb/gdb[0x9b1aa6]
  ./gdb/gdb[0x9b1b0c]
  ./gdb/gdb[0x41756d]
  /lib64/libc.so.6(__libc_start_main+0xf3)[0x7f66484041a3]
  ./gdb/gdb[0x41746e]
  ---------------------
  A fatal error internal to GDB has been detected, further
  debugging is not possible.  GDB will now terminate.

  This is a bug, please report it.  For instructions, see:
  <https://www.gnu.org/software/gdb/bugs/>.

  Segmentation fault (core dumped)

It is disappointing that backtrace_symbols_fd does not actually map
the addresses back to symbols, this appears, in part, to be due to GDB
not being built with -rdynamic as the manual page for
backtrace_symbols_fd suggests, however, even when I do add -rdynamic
to the build of GDB I only see symbols for some addresses.

We could potentially look at alternative libraries to provide the
backtrace (e.g. libunwind) however, the solution presented here, which
is available as part of glibc is probably a good baseline from which
we might improve things in future.
---
 gdb/NEWS                                      |   7 +
 gdb/config.in                                 |   6 +
 gdb/configure                                 |  51 +++++++
 gdb/configure.ac                              |  22 +++
 gdb/doc/gdb.texinfo                           |  18 +++
 gdb/event-top.c                               | 140 ++++++++++++++++--
 gdb/testsuite/gdb.base/bt-on-fatal-signal.c   |  22 +++
 gdb/testsuite/gdb.base/bt-on-fatal-signal.exp | 134 +++++++++++++++++
 gdb/ui-file.h                                 |   9 ++
 9 files changed, 398 insertions(+), 11 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/bt-on-fatal-signal.c
 create mode 100644 gdb/testsuite/gdb.base/bt-on-fatal-signal.exp

diff --git a/gdb/NEWS b/gdb/NEWS
index 062249aea98..764558789f3 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -3,6 +3,13 @@
 
 *** Changes since GDB 11
 
+maint set backtrace-on-fatal-signal on|off
+maint show backtrace-on-fatal-signal
+  This setting is 'on' by default.  When 'on' GDB will print a limited
+  backtrace to stderr in the situation where GDB terminates with a
+  fatal signal.  This only supported on some platforms where the
+  backtrace and backtrace_symbols_fd functions are available.
+
 *** Changes in GDB 11
 
 * The 'set disassembler-options' command now supports specifying options
diff --git a/gdb/config.in b/gdb/config.in
index 2c30504905b..af3680c6d95 100644
--- a/gdb/config.in
+++ b/gdb/config.in
@@ -162,6 +162,12 @@
 /* Define to 1 if your system has the etext variable. */
 #undef HAVE_ETEXT
 
+/* Define to 1 if execinfo.h backtrace functions are available. */
+#undef HAVE_EXECINFO_BACKTRACE
+
+/* Define to 1 if you have the <execinfo.h> header file. */
+#undef HAVE_EXECINFO_H
+
 /* Define to 1 if you have the `fdwalk' function. */
 #undef HAVE_FDWALK
 
diff --git a/gdb/configure b/gdb/configure
index 5d89635c043..f0b1af4a6ea 100755
--- a/gdb/configure
+++ b/gdb/configure
@@ -12315,6 +12315,18 @@ fi
 
 done
 
+for ac_header in execinfo.h
+do :
+  ac_fn_c_check_header_mongrel "$LINENO" "execinfo.h" "ac_cv_header_execinfo_h" "$ac_includes_default"
+if test "x$ac_cv_header_execinfo_h" = xyes; then :
+  cat >>confdefs.h <<_ACEOF
+#define HAVE_EXECINFO_H 1
+_ACEOF
+
+fi
+
+done
+
 
 # ------------------------- #
 # Checks for declarations.  #
@@ -16484,6 +16496,45 @@ fi
 { $as_echo "$as_me:${as_lineno-$LINENO}: result: $found" >&5
 $as_echo "$found" >&6; }
 
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking whether execinfo.h backtrace is available" >&5
+$as_echo_n "checking whether execinfo.h backtrace is available... " >&6; }
+if ${gdb_cv_execinfo_backtrace+:} false; then :
+  $as_echo_n "(cached) " >&6
+else
+  cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h.  */
+
+         #include <execinfo.h>
+
+int
+main ()
+{
+
+         int f;
+         void *b[2];
+         f = backtrace (b, 2);
+         backtrace_symbols_fd (b, f, 2);
+
+  ;
+  return 0;
+}
+_ACEOF
+if ac_fn_c_try_link "$LINENO"; then :
+  gdb_cv_execinfo_backtrace=yes
+else
+  gdb_cv_execinfo_backtrace=no
+fi
+rm -f core conftest.err conftest.$ac_objext \
+    conftest$ac_exeext conftest.$ac_ext
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $gdb_cv_execinfo_backtrace" >&5
+$as_echo "$gdb_cv_execinfo_backtrace" >&6; }
+if test "$gdb_cv_execinfo_backtrace" = yes; then
+
+$as_echo "#define HAVE_EXECINFO_BACKTRACE 1" >>confdefs.h
+
+fi
+
 
 if test "${build}" = "${host}" -a "${host}" = "${target}" ; then
    case ${host_os} in
diff --git a/gdb/configure.ac b/gdb/configure.ac
index b8c79bcac9a..93f11316a14 100644
--- a/gdb/configure.ac
+++ b/gdb/configure.ac
@@ -1291,6 +1291,7 @@ AC_CHECK_HEADERS(term.h, [], [],
 
 AC_CHECK_HEADERS([sys/socket.h])
 AC_CHECK_HEADERS([ws2tcpip.h])
+AC_CHECK_HEADERS([execinfo.h])
 
 # ------------------------- #
 # Checks for declarations.  #
@@ -1682,6 +1683,27 @@ fi
 AC_SUBST(RDYNAMIC)
 AC_MSG_RESULT($found)
 
+AC_CACHE_CHECK(
+  [whether execinfo.h backtrace is available],
+  gdb_cv_execinfo_backtrace,
+  [AC_LINK_IFELSE(
+     [AC_LANG_PROGRAM(
+        [
+         #include <execinfo.h>
+        ],
+        [
+         int f;
+         void *b[[2]];
+         f = backtrace (b, 2);
+         backtrace_symbols_fd (b, f, 2);
+        ])],
+   [gdb_cv_execinfo_backtrace=yes],
+   [gdb_cv_execinfo_backtrace=no])])
+if test "$gdb_cv_execinfo_backtrace" = yes; then
+  AC_DEFINE(HAVE_EXECINFO_BACKTRACE, 1,
+            [Define to 1 if execinfo.h backtrace functions are available.])
+fi
+
 dnl For certain native configurations, we need to check whether thread
 dnl support can be built in or not.
 dnl
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 386cef7bd4e..dc8c1a70b52 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -39715,6 +39715,24 @@
 @value{GDBN} supports.  They are used by the testsuite for exercising
 the settings infrastructure.
 
+@kindex maint set backtrace-on-fatal-signal
+@kindex maint show backtrace-on-fatal-signal
+@item maint set backtrace-on-fatal-signal [on|off]
+@itemx maint show backtrace-on-fatal-signal
+When this setting is @code{on}, if @value{GDBN} itself terminates with
+a fatal signal (e.g.@: SIGSEGV), then a limited backtrace will be
+printed to stderr.  This backtrace can be used to help diagnose
+crashes within @value{GDBN} in situations where a user is unable to
+share a corefile with the @value{GDBN} developers.
+
+If the functionality to provide this backtrace is not available for
+the platform on which GDB is running then this feature will be
+@code{off} by default, and attempting to turn this feature on will
+give an error.
+
+For platforms that do support creating the backtrace this feature is
+@code{on} by default.
+
 @kindex maint with
 @item maint with @var{setting} [@var{value}] [-- @var{command}]
 Like the @code{with} command, but works with @code{maintenance set}
diff --git a/gdb/event-top.c b/gdb/event-top.c
index 972b540c8b8..b5dfd43f621 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -46,6 +46,10 @@
 #include "readline/readline.h"
 #include "readline/history.h"
 
+#ifdef HAVE_EXECINFO_H
+# include <execinfo.h>
+#endif /* HAVE_EXECINFO_H */
+
 /* readline defines this.  */
 #undef savestring
 
@@ -96,6 +100,38 @@ bool exec_done_display_p = false;
    run again.  */
 int call_stdin_event_handler_again_p;
 
+/* When true GDB will produce a minimal backtrace when a fatal signal is
+   reached (within GDB code).  */
+static bool bt_on_fatal_signal
+#ifdef HAVE_EXECINFO_BACKTRACE
+  = true;
+#else
+  = false;
+#endif /* HAVE_EXECINFO_BACKTRACE */
+
+/* Implement 'maintenance show backtrace-on-fatal-signal'.  */
+
+static void
+show_bt_on_fatal_signal (struct ui_file *file, int from_tty,
+			 struct cmd_list_element *cmd, const char *value)
+{
+  fprintf_filtered (file, _("Backtrace on a fatal signal is %s.\n"), value);
+}
+
+/* Implement 'maintenance set backtrace-on-fatal-signal'.  */
+
+static void
+set_bt_on_fatal_signal (const char *args, int from_tty, cmd_list_element *c)
+{
+#ifndef HAVE_EXECINFO_BACKTRACE
+  if (bt_on_fatal_signal)
+    {
+      bt_on_fatal_signal = false;
+      error (_("support for this feature is not compiled into GDB"));
+    }
+#endif
+}
+
 /* Signal handling variables.  */
 /* Each of these is a pointer to a function that the event loop will
    invoke if the corresponding signal has received.  The real signal
@@ -846,6 +882,84 @@ gdb_readline_no_editing_callback (gdb_client_data client_data)
 }
 \f
 
+/* Attempt to unblock signal SIG, return true if the signal was unblocked,
+   otherwise, return false.  */
+
+static bool
+unblock_signal (int sig)
+{
+#if HAVE_SIGPROCMASK
+  sigset_t sigset;
+  sigemptyset (&sigset);
+  sigaddset (&sigset, sig);
+  gdb_sigmask (SIG_UNBLOCK, &sigset, 0);
+  return true;
+#endif
+
+  return false;
+}
+
+/* Called to handle fatal signals.  SIG is the signal number.  */
+
+static void ATTRIBUTE_NORETURN
+handle_fatal_signal (int sig)
+{
+#ifdef HAVE_EXECINFO_BACKTRACE
+  const auto sig_write = [] (const char *msg) -> void
+  {
+    gdb_stderr->write_async_safe (msg, strlen (msg));
+  };
+
+  if (bt_on_fatal_signal)
+    {
+      sig_write ("\n\n");
+      sig_write (_("Fatal signal: "));
+      sig_write (strsignal (sig));
+      sig_write ("\n");
+
+      /* Allow up to 25 frames of backtrace.  */
+      void *buffer[25];
+      int frames = backtrace (buffer, sizeof(buffer) / sizeof(buffer[0]));
+      sig_write (_("----- Backtrace -----\n"));
+      if (gdb_stderr->fd () > -1)
+	{
+	  backtrace_symbols_fd (buffer, frames, gdb_stderr->fd ());
+	  if (frames == (sizeof(buffer) / sizeof(buffer[0])))
+	    sig_write (_("Backtrace might be incomplete.\n"));
+	}
+      else
+	sig_write (_("Backtrace unavailable\n"));
+      sig_write ("---------------------\n");
+      sig_write (_("A fatal error internal to GDB has been detected, "
+		   "further\ndebugging is not possible.  GDB will now "
+		   "terminate.\n\n"));
+      sig_write (_("This is a bug, please report it."));
+      if (REPORT_BUGS_TO[0] != '\0')
+	{
+	  sig_write (_("  For instructions, see:\n"));
+	  sig_write (REPORT_BUGS_TO);
+	  sig_write (".");
+	}
+      sig_write ("\n\n");
+
+      gdb_stderr->flush ();
+    }
+#endif /* HAVE_EXECINF_BACKTRACE */
+
+  /* If possible arrange for SIG to have its default behaviour (which
+     should be to terminate the current process), unblock SIG, and reraise
+     the signal.  This ensures GDB terminates with the expected signal.  */
+  if (signal (sig, SIG_DFL) != SIG_ERR
+      && unblock_signal (sig))
+    raise (sig);
+
+  /* The above failed, so try to use SIGABRT to terminate GDB.  */
+#ifdef SIGABRT
+  signal (SIGABRT, SIG_DFL);
+#endif
+  abort ();		/* ARI: abort */
+}
+
 /* The SIGSEGV handler for this thread, or NULL if there is none.  GDB
    always installs a global SIGSEGV handler, and then lets threads
    indicate their interest in handling the signal by setting this
@@ -887,7 +1001,7 @@ handle_sigsegv (int sig)
   install_handle_sigsegv ();
 
   if (thread_local_segv_handler == nullptr)
-    abort ();			/* ARI: abort */
+    handle_fatal_signal (sig);
   thread_local_segv_handler (sig);
 }
 
@@ -1160,16 +1274,7 @@ async_sigtstp_handler (gdb_client_data arg)
   char *prompt = get_prompt ();
 
   signal (SIGTSTP, SIG_DFL);
-#if HAVE_SIGPROCMASK
-  {
-    sigset_t zero;
-
-    sigemptyset (&zero);
-    gdb_sigmask (SIG_SETMASK, &zero, 0);
-  }
-#elif HAVE_SIGSETMASK
-  sigsetmask (0);
-#endif
+  unblock_signal (SIGTSTP);
   raise (SIGTSTP);
   signal (SIGTSTP, handle_sigtstp);
   printf_unfiltered ("%s", prompt);
@@ -1320,4 +1425,17 @@ Control whether to show event loop-related debug messages."),
 			set_debug_event_loop_command,
 			show_debug_event_loop_command,
 			&setdebuglist, &showdebuglist);
+
+  add_setshow_boolean_cmd ("backtrace-on-fatal-signal", class_maintenance,
+			   &bt_on_fatal_signal, _("\
+Set whether to produce a backtrace if GDB receives a fatal signal."), _("\
+Show whether GDB will produce a backtrace if it receives a fatal signal."), _("\
+Use \"on\" to enable, \"off\" to disable.\n\
+If enabled, GDB will produce a minimal backtrace if it encounters a fatal\n\
+signal from within GDB itself.  This is a mechanism to help diagnose\n\
+crashes within GDB, not a mechanism for debugging inferiors."),
+			   set_bt_on_fatal_signal,
+			   show_bt_on_fatal_signal,
+			   &maintenance_set_cmdlist,
+			   &maintenance_show_cmdlist);
 }
diff --git a/gdb/testsuite/gdb.base/bt-on-fatal-signal.c b/gdb/testsuite/gdb.base/bt-on-fatal-signal.c
new file mode 100644
index 00000000000..d9d56c3700f
--- /dev/null
+++ b/gdb/testsuite/gdb.base/bt-on-fatal-signal.c
@@ -0,0 +1,22 @@
+/* Copyright 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/>.  */
+
+int
+main (void)
+{
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/bt-on-fatal-signal.exp b/gdb/testsuite/gdb.base/bt-on-fatal-signal.exp
new file mode 100644
index 00000000000..7a9f8e45fde
--- /dev/null
+++ b/gdb/testsuite/gdb.base/bt-on-fatal-signal.exp
@@ -0,0 +1,134 @@
+# Copyright 2021 Free Software Foundation, Inc.
+
+# 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/>.
+
+# Test the 'maint set backtrace-on-fatal-signal' behaviour.  Start up
+# GDB, turn on backtrace-on-fatal-signal, then send fatal signals to
+# GDB and ensure we see the backtrace.
+
+standard_testfile
+
+# The logic for sending signals to GDB might now work when using a
+# remote host (will the signal go to GDB, or the program that
+# established the connection to the remote host?), so just skip this
+# test for remote host setups.
+if {[is_remote host]} {
+    untested $testfile
+    return -1
+}
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile]} {
+    return -1
+}
+
+# Check we can run to main.  If this works this time then we just
+# assume that it will work later on (when we repeatedly restart GDB).
+if ![runto_main] then {
+    untested $testfile
+    return -1
+}
+
+# Check that the backtrace-on-fatal-signal feature is supported.  If
+# this target doesn't have the backtrace function available then
+# trying to turn this on will give an error, in which case we just
+# skip this test.
+gdb_test_multiple "maint set backtrace-on-fatal-signal on" "" {
+    -re "support for this feature is not compiled into GDB" {
+	untested $testfile
+	return -1
+    }
+    -re "$gdb_prompt $" {
+	pass $gdb_test_name
+    }
+}
+
+# Now the actual test loop.
+foreach test_data {{SEGV "Segmentation fault"}} {
+    set sig [lindex ${test_data} 0]
+    set msg [lindex ${test_data} 1]
+    with_test_prefix ${sig} {
+
+	# Restart GDB.
+	clean_restart $binfile
+
+	# Capture the pid of GDB.
+	set testpid [spawn_id_get_pid $gdb_spawn_id]
+
+	# Start the inferior.
+	runto_main
+
+	# Turn on the backtrace-on-fatal-signal feature.
+	gdb_test_no_output "maint set backtrace-on-fatal-signal on"
+
+	# Flags for various bits of the output we expect to see, we
+	# check for these in the gdb_test_multiple below.
+	set saw_fatal_msg false
+	set saw_bt_start false
+	set saw_bt_end false
+	set internal_error_msg_count 0
+
+	# Send the fatal signal to GDB.
+	remote_exec host "kill -${sig} ${testpid}"
+
+	# Scan GDB's output for the backtrace.  As the output we get
+	# here includes the standard "internal error" message, which
+	# gdb_test_multiple will usually handle, we are forced to make
+	# extensive use of the "-early" flag here so that all our
+	# patterns are applied before gdb_test_multiple can check for
+	# the internal error pattern.
+	gdb_test_multiple "" "scan for backtrace" {
+	    -early -re "^\r\n" {
+		exp_continue
+	    }
+	    -early -re "^Fatal signal: ${msg}\r\n" {
+		set saw_fatal_msg true
+		exp_continue
+	    }
+	    -early -re "^----- Backtrace -----\r\n" {
+		set saw_bt_start true
+		exp_continue
+	    }
+	    -early -re ".+\r\n---------------------\r\n" {
+		set saw_bt_end true
+		exp_continue
+	    }
+	    -early -re "^A fatal error internal to GDB has been detected, further\r\n" {
+		incr internal_error_msg_count
+		exp_continue
+	    }
+	    -early -re "^debugging is not possible.  GDB will now terminate\\.\r\n" {
+		incr internal_error_msg_count
+		exp_continue
+	    }
+	    eof {
+		# Catch the eof case as this indicates that GDB has
+		# gone away, which in this case, is what we expect to
+		# happen.
+		gdb_assert { $saw_fatal_msg }
+		gdb_assert { $saw_bt_start }
+		gdb_assert { $saw_bt_end }
+		gdb_assert { [expr $internal_error_msg_count == 2] }
+	    }
+	    -re "$gdb_prompt $" {
+		# GDB should terminate, we should never get back to
+		# the prompt.
+		fail $gdb_test_name
+	    }
+	}
+
+	# GDB should be dead and gone by this point, but just to be
+	# sure, force an exit.
+	gdb_exit
+    }
+}
diff --git a/gdb/ui-file.h b/gdb/ui-file.h
index 61509735c68..9593c94e673 100644
--- a/gdb/ui-file.h
+++ b/gdb/ui-file.h
@@ -83,6 +83,11 @@ class ui_file
 
   virtual void flush ()
   {}
+
+  /* If this object has an underlying file descriptor, then return it.
+     Otherwise, return -1.  */
+  virtual int fd () const
+  { return -1; }
 };
 
 typedef std::unique_ptr<ui_file> ui_file_up;
@@ -195,6 +200,10 @@ class stdio_file : public ui_file
 
   bool can_emit_style_escape () override;
 
+  /* Return the underlying file descriptor.  */
+  int fd () const override
+  { return m_fd; }
+
 private:
   /* Sets the internal stream to FILE, and saves the FILE's file
      descriptor in M_FD.  */
-- 
2.25.4

[PATCHv2 5/6] gdb: register SIGBUS, SIGFPE, and SIGABRT handlers

[Andrew Burgess]

Register handlers for SIGBUS, SIGFPE, and SIGABRT.  All of these
signals are setup as fatal signals that will cause GDB to terminate.
However, by passing these signals through the handle_fatal_signal
function, a user can arrange to see a backtrace when GDB
terminates (see maint set backtrace-on-fatal-signal).

In normal use of GDB there should be no user visible changes after
this commit.  Only if GDB terminates with one of the above signals
will GDB change slightly, potentially printing a backtrace before
aborting.

I've added new tests for SIGFPE, SIGBUS, and SIGABRT.
---
 gdb/event-top.c                               | 17 ++++++++++++++++-
 gdb/testsuite/gdb.base/bt-on-fatal-signal.exp |  5 ++++-
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/gdb/event-top.c b/gdb/event-top.c
index b5dfd43f621..349f0f26ce4 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -1025,7 +1025,10 @@ static struct serial_event *quit_serial_event;
    with the reception of the signal.
 
    For SIGSEGV the handle_sig* function does all the work for handling this
-   signal.  */
+   signal.
+
+   For SIGFPE, SIGBUS, and SIGABRT, these signals will all cause GDB to
+   terminate immediately.  */
 void
 gdb_init_signals (void)
 {
@@ -1061,6 +1064,18 @@ gdb_init_signals (void)
     create_async_signal_handler (async_sigtstp_handler, NULL, "sigtstp");
 #endif
 
+#ifdef SIGFPE
+  signal (SIGFPE, handle_fatal_signal);
+#endif
+
+#ifdef SIGBUS
+  signal (SIGBUS, handle_fatal_signal);
+#endif
+
+#ifdef SIGABRT
+  signal (SIGABRT, handle_fatal_signal);
+#endif
+
   install_handle_sigsegv ();
 }
 
diff --git a/gdb/testsuite/gdb.base/bt-on-fatal-signal.exp b/gdb/testsuite/gdb.base/bt-on-fatal-signal.exp
index 7a9f8e45fde..8875d00fdb1 100644
--- a/gdb/testsuite/gdb.base/bt-on-fatal-signal.exp
+++ b/gdb/testsuite/gdb.base/bt-on-fatal-signal.exp
@@ -54,7 +54,10 @@ gdb_test_multiple "maint set backtrace-on-fatal-signal on" "" {
 }
 
 # Now the actual test loop.
-foreach test_data {{SEGV "Segmentation fault"}} {
+foreach test_data {{SEGV "Segmentation fault"} \
+		       {FPE "Floating point exception"} \
+		       {BUS "Bus error"} \
+		       {ABRT "Aborted"}} {
     set sig [lindex ${test_data} 0]
     set msg [lindex ${test_data} 1]
     with_test_prefix ${sig} {
-- 
2.25.4

[PATCHv2 6/6] gdb: don't print backtrace when dumping core after an internal error

[Andrew Burgess]

Currently, when GDB hits an internal error, and the user selects to
dump core, the recently added feature to write a backtrace to the
console will kick in, and print a backtrace as well as dumping the
core.

This was certainly not my intention when adding the backtrace on fatal
signal functionality, this feature was intended to produce a backtrace
when GDB crashes due to some fatal signal, internal errors should have
continued to behave as they did before, unchanged.

In this commit I set the signal disposition of SIGABRT back to SIG_DFL
just prior to the call to abort() that GDB uses to trigger the core
dump, this prevents GDB reaching the code that writes the backtrace to
the console.

I've also added a test that checks we don't see a backtrace on the
console after an internal error.
---
 gdb/testsuite/gdb.base/bt-on-fatal-signal.exp | 36 +++++++++++++++++++
 gdb/utils.c                                   |  5 +++
 2 files changed, 41 insertions(+)

diff --git a/gdb/testsuite/gdb.base/bt-on-fatal-signal.exp b/gdb/testsuite/gdb.base/bt-on-fatal-signal.exp
index 8875d00fdb1..1f0d61f00ed 100644
--- a/gdb/testsuite/gdb.base/bt-on-fatal-signal.exp
+++ b/gdb/testsuite/gdb.base/bt-on-fatal-signal.exp
@@ -135,3 +135,39 @@ foreach test_data {{SEGV "Segmentation fault"} \
 	gdb_exit
     }
 }
+
+# Check that when we get an internal error and choose to dump core, we
+# don't print a backtrace to the console.
+with_test_prefix "internal-error" {
+    # Restart GDB.
+    clean_restart $binfile
+
+    set saw_bt_start false
+
+    gdb_test_multiple "maint internal-error foo" "" {
+	-early -re "internal-error: foo\r\n" {
+	    exp_continue
+	}
+	-early -re "^A problem internal to GDB has been detected,\r\n" {
+	    exp_continue
+	}
+	-early -re "^further debugging may prove unreliable\\.\r\n" {
+	    exp_continue
+	}
+	-early -re "^Quit this debugging session\\? \\(y or n\\)" {
+	    send_gdb "y\n"
+	    exp_continue
+	}
+	-early -re "^Create a core file of GDB\\? \\(y or n\\)" {
+	    send_gdb "y\n"
+	    exp_continue
+	}
+	-early -re "----- Backtrace -----\r\n" {
+	    set saw_bt_start true
+	    exp_continue
+	}
+	eof {
+	    gdb_assert { [expr ! $saw_bt_start] }
+	}
+    }
+}
diff --git a/gdb/utils.c b/gdb/utils.c
index c59c63565eb..1c226d5d85e 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -201,6 +201,11 @@ dump_core (void)
   setrlimit (RLIMIT_CORE, &rlim);
 #endif /* HAVE_SETRLIMIT */
 
+  /* Ensure that the SIGABRT we're about to raise will immediately cause
+     GDB to exit and dump core, we don't want to trigger GDB's printing of
+     a backtrace to the console here.  */
+  signal (SIGABRT, SIG_DFL);
+
   abort ();		/* ARI: abort */
 }
 
-- 
2.25.4

Re: [PATCHv2 0/6] GDB Synchronous Signal Handling

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> Thanks for all of the great feedback on v1 of the patch.
Andrew> For patch #1, I have not made any attempt to "recover" after the fatal
Andrew> signal.  I think Pedro said everything I would want to say, basically,
Andrew> I'm not sure there's a general way (i.e. covering all of GDB) that we
Andrew> could recover without leaving GDB is a pretty broken state.  If anyone
Andrew> knows different then I'm happy to revisit this idea.

I read through these and they seem fine to me.

I had initially wondered whether SIGFPE could be caught and contained,
but the subsequent thread convinced me that having gdb simply die is
fine.

thanks,
Tom

Re: [PATCHv2 0/6] GDB Synchronous Signal Handling

[Andrew Burgess]

Pedro,

You had a lot of great feedback on v1, I wondered if you had any
thoughts on the v2 series?

Thanks,
Andrew


* Andrew Burgess <andrew.burgess@embecosm.com> [2021-07-21 19:08:47 +0100]:

> Thanks for all of the great feedback on v1 of the patch.
> 
> For patch #1, I have not made any attempt to "recover" after the fatal
> signal.  I think Pedro said everything I would want to say, basically,
> I'm not sure there's a general way (i.e. covering all of GDB) that we
> could recover without leaving GDB is a pretty broken state.  If anyone
> knows different then I'm happy to revisit this idea.
> 
> Patch #2 is unchanged.
> 
> Patch #3 is changed to rename the function as Pedro suggested, and to
> remove the dead code that was pointed out.
> 
> Patch #4 I've extended the message printed along with the backtrace to
> include words that are similar to those given when we hit an internal
> error.  There's an example of this output in the commit message.  The
> backtrace is now on by default.  As was discussed in the comment
> thread for this patch, querying the user isn't really possible from
> within the signal handler, and even if we could somehow "recover" from
> the signal handler back to GDB's main loop, dumping a core file at
> that point would be pretty pointless.
> 
> Patch #5 is mostly unchanged, small tweak to the commit message.
> 
> Patch #6 is new, addresses the fact that the new backtrace was being
> produced when we hit an internal error, which was never my original
> intention.
> 
> ---
> 
> Andrew Burgess (6):
>   gdb: terminate upon receipt of SIGFPE
>   gdb: register signal handler after setting up event token
>   gdb: rename async_init_signals to gdb_init_signals
>   gdb: print backtrace on fatal SIGSEGV
>   gdb: register SIGBUS, SIGFPE, and SIGABRT handlers
>   gdb: don't print backtrace when dumping core after an internal error
> 
>  gdb/NEWS                                      |   7 +
>  gdb/config.in                                 |   6 +
>  gdb/configure                                 |  51 ++++
>  gdb/configure.ac                              |  22 ++
>  gdb/doc/gdb.texinfo                           |  18 ++
>  gdb/event-top.c                               | 227 +++++++++++++-----
>  gdb/event-top.h                               |   2 +-
>  gdb/testsuite/gdb.base/bt-on-fatal-signal.c   |  22 ++
>  gdb/testsuite/gdb.base/bt-on-fatal-signal.exp | 173 +++++++++++++
>  gdb/top.c                                     |   2 +-
>  gdb/ui-file.h                                 |   9 +
>  gdb/utils.c                                   |   5 +
>  12 files changed, 478 insertions(+), 66 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.base/bt-on-fatal-signal.c
>  create mode 100644 gdb/testsuite/gdb.base/bt-on-fatal-signal.exp
> 
> -- 
> 2.25.4
> 

Re: [PATCHv2 4/6] gdb: print backtrace on fatal SIGSEGV

[Pedro Alves]

On 2021-07-21 7:08 p.m., Andrew Burgess wrote:
> +      void *buffer[25];
> +      int frames = backtrace (buffer, sizeof(buffer) / sizeof(buffer[0]));

Missing spaces after sizeof, but better would be:

      int frames = backtrace (buffer, ARRAY_SIZE (buffer));

> +      sig_write (_("----- Backtrace -----\n"));
> +      if (gdb_stderr->fd () > -1)
> +	{
> +	  backtrace_symbols_fd (buffer, frames, gdb_stderr->fd ());
> +	  if (frames == (sizeof(buffer) / sizeof(buffer[0])))

Ditto:

	  if (frames == ARRAY_SIZE (buffer))

> +	    sig_write (_("Backtrace might be incomplete.\n"));
> +	}

Re: [PATCHv2 0/6] GDB Synchronous Signal Handling

[Pedro Alves]

Hi Andrew,

On 2021-08-10 10:33 a.m., Andrew Burgess wrote:
> Pedro,
> 
> You had a lot of great feedback on v1, I wondered if you had any
> thoughts on the v2 series?

Thanks.  I read the v2 series now, and it all looks good to me.

I just had a tiny nit comment in reply to patch #4.  No need to repost for that.