[3/3,v5] Demangler crash handler

Message ID 20140609152434.GD27494@blade.nx
State Committed
Headers

Commit Message

Gary Benson June 9, 2014, 3:24 p.m. UTC
  This patch wraps calls to the demangler with a segmentation fault
handler.  The first time a segmentation fault is caught a core file
is generated and the user is prompted to file a bug and offered the
choice to exit or to continue their GDB session.  A maintainence
option is provided to allow the user to disable the crash handler
if required.

gdb/
2014-06-09  Gary Benson  <gbenson@redhat.com>

	* configure.ac [AC_CHECK_FUNCS] <sigaltstack>: New check.
	* configure: Regenerate.
	* config.in: Likewise.
	* main.c (signal.h): New include.
	(setup_alternate_signal_stack): New function.
	(captured_main): Call the above.
	* cp-support.c (signal.h): New include.
	(catch_demangler_crashes): New flag.
	(SIGJMP_BUF): New define.
	(SIGSETJMP): Likewise.
	(SIGLONGJMP): Likewise.
	(gdb_demangle_jmp_buf): New static global.
	(gdb_demangle_attempt_core_dump): Likewise.
	(gdb_demangle_signal_handler): New function.
	(gdb_demangle): If catch_demangler_crashes is set, install the
	above signal handler before calling bfd_demangle, and restore
	the original signal handler afterwards.  Display the offending
	symbol and call demangler_warning the first time a segmentation
	fault is caught.
	(_initialize_cp_support): New maint set/show command.

gdb/doc/
2014-06-09  Gary Benson  <gbenson@redhat.com>

	* gdb.texinfo (Maintenance Commands): Document new
	"maint set/show catch-demangler-crashes" option.
  

Comments

Eli Zaretskii June 9, 2014, 4:20 p.m. UTC | #1
> Date: Mon, 9 Jun 2014 16:24:34 +0100
> From: Gary Benson <gbenson@redhat.com>
> Cc: Andrew Burgess <aburgess@broadcom.com>, Doug Evans <xdje42@gmail.com>,
>         Eli Zaretskii <eliz@gnu.org>, Florian Weimer <fw@deneb.enyo.de>,
>         Mark Kettenis <mark.kettenis@xs4all.nl>,
>         Pedro Alves <palves@redhat.com>, Tom Tromey <tromey@redhat.com>
> 
> This patch wraps calls to the demangler with a segmentation fault
> handler.  The first time a segmentation fault is caught a core file
> is generated and the user is prompted to file a bug and offered the
> choice to exit or to continue their GDB session.  A maintainence
> option is provided to allow the user to disable the crash handler
> if required.

OK for the documentation part.

Thanks.
  
Gary Benson June 9, 2014, 6:24 p.m. UTC | #2
Eli Zaretskii wrote:
> > Date: Mon, 9 Jun 2014 16:24:34 +0100
> > From: Gary Benson <gbenson@redhat.com>
> > Cc: Andrew Burgess <aburgess@broadcom.com>, Doug Evans <xdje42@gmail.com>,
> >         Eli Zaretskii <eliz@gnu.org>, Florian Weimer <fw@deneb.enyo.de>,
> >         Mark Kettenis <mark.kettenis@xs4all.nl>,
> >         Pedro Alves <palves@redhat.com>, Tom Tromey <tromey@redhat.com>
> > 
> > This patch wraps calls to the demangler with a segmentation fault
> > handler.  The first time a segmentation fault is caught a core file
> > is generated and the user is prompted to file a bug and offered the
> > choice to exit or to continue their GDB session.  A maintainence
> > option is provided to allow the user to disable the crash handler
> > if required.
> 
> OK for the documentation part.

Thanks Eli.

Cheers,
Gary

--
http://gbenson.net/
  
Pierre Muller July 13, 2014, 4:11 p.m. UTC | #3
Hi Gary,
I think your patch generates
build failure on cygwin32:

../../../binutils-gdb/gdb/cp-support.c: In function 'gdb_demangle':
../../../binutils-gdb/gdb/cp-support.c:1560:21: erreur: 'SA_ONSTACK'
undeclared (first use in this function)
       sa.sa_flags = SA_ONSTACK;
                     ^
../../../binutils-gdb/gdb/cp-support.c:1560:21: note: each undeclared
identifier is reported only once for each function it appears in
Makefile:1075: recipe for target 'cp-support.o' failed

The reason is that SA_ONSTACK
is not defined in cygwin's /usr/include/signal.h header

whereas SA_RESTART is defined in signal header,
and HAVE_SIGACTION is set in config.h


> +#if defined (HAVE_SIGACTION) && defined (SA_RESTART)
> +      sa.sa_handler = gdb_demangle_signal_handler;
> +      sigemptyset (&sa.sa_mask);
> +      sa.sa_flags = SA_ONSTACK;
> +      sigaction (SIGSEGV, &sa, &old_sa);
> +#else
> +      ofunc = (void (*)()) signal (SIGSEGV,
> gdb_demangle_signal_handler);
> +#endif

A simple patch would probably be to add a separate check
#ifdef SA_ONSTACK
  sa.sa_flags = SA_O?STACK;
#endif

But I honestly don't know enough about
Cygwin signal emulation to know if this is a correct fix or not.

  Maybe Corinna Vinschen can comment on this?

Pierre Muller
  
Corinna Vinschen July 14, 2014, 8:36 a.m. UTC | #4
On Jul 13 18:11, Pierre Muller wrote:
>    Hi Gary,
> I think your patch generates
> build failure on cygwin32:
> 
> ../../../binutils-gdb/gdb/cp-support.c: In function 'gdb_demangle':
> ../../../binutils-gdb/gdb/cp-support.c:1560:21: erreur: 'SA_ONSTACK'
> undeclared (first use in this function)
>        sa.sa_flags = SA_ONSTACK;
>                      ^
> ../../../binutils-gdb/gdb/cp-support.c:1560:21: note: each undeclared
> identifier is reported only once for each function it appears in
> Makefile:1075: recipe for target 'cp-support.o' failed
> 
> The reason is that SA_ONSTACK
> is not defined in cygwin's /usr/include/signal.h header
> 
> whereas SA_RESTART is defined in signal header,
> and HAVE_SIGACTION is set in config.h
> 
> 
> > +#if defined (HAVE_SIGACTION) && defined (SA_RESTART)
> > +      sa.sa_handler = gdb_demangle_signal_handler;
> > +      sigemptyset (&sa.sa_mask);
> > +      sa.sa_flags = SA_ONSTACK;
> > +      sigaction (SIGSEGV, &sa, &old_sa);
> > +#else
> > +      ofunc = (void (*)()) signal (SIGSEGV,
> > gdb_demangle_signal_handler);
> > +#endif
> 
> A simple patch would probably be to add a separate check
> #ifdef SA_ONSTACK
>   sa.sa_flags = SA_O?STACK;
> #endif
> 
> But I honestly don't know enough about
> Cygwin signal emulation to know if this is a correct fix or not.
> 
>   Maybe Corinna Vinschen can comment on this?

Right, Cygwin doesn't support SA_ONSTACK yet.  It should be
possible to add, but there are no immediate plans to do so.

As for the above, wouldn't it be better to add something like

  #ifndef SA_ONSTACK
  #define SA_ONSTACK 0
  #endif

to some header?


Corinna
  
Gary Benson July 14, 2014, 12:02 p.m. UTC | #5
Corinna Vinschen wrote:
> On Jul 13 18:11, Pierre Muller wrote:
> > A simple patch would probably be to add a separate check
> > #ifdef SA_ONSTACK
> >   sa.sa_flags = SA_O?STACK;
> > #endif
> 
> Right, Cygwin doesn't support SA_ONSTACK yet.  It should be
> possible to add, but there are no immediate plans to do so.
> 
> As for the above, wouldn't it be better to add something like
> 
>   #ifndef SA_ONSTACK
>   #define SA_ONSTACK 0
>   #endif
> 
> to some header?

I'd prefer not to.  Having the conditional where the handler
is installed makes it obvious that the alternate stack is not
available on all systems.

Thanks,
Gary
  

Patch

diff --git a/gdb/configure.ac b/gdb/configure.ac
index 903f378..f41ba2f 100644
--- a/gdb/configure.ac
+++ b/gdb/configure.ac
@@ -1328,7 +1328,7 @@  AC_CHECK_FUNCS([canonicalize_file_name realpath getrusage getuid getgid \
 		sigaction sigprocmask sigsetmask socketpair \
 		ttrace wborder wresize setlocale iconvlist libiconvlist btowc \
 		setrlimit getrlimit posix_madvise waitpid lstat \
-		ptrace64])
+		ptrace64 sigaltstack])
 AM_LANGINFO_CODESET
 GDB_AC_COMMON
 
diff --git a/gdb/configure b/gdb/configure
index 56c92d3..e23a58f 100755
--- a/gdb/configure
+++ b/gdb/configure
@@ -10507,7 +10507,7 @@  for ac_func in canonicalize_file_name realpath getrusage getuid getgid \
 		sigaction sigprocmask sigsetmask socketpair \
 		ttrace wborder wresize setlocale iconvlist libiconvlist btowc \
 		setrlimit getrlimit posix_madvise waitpid lstat \
-		ptrace64
+		ptrace64 sigaltstack
 do :
   as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh`
 ac_fn_c_check_func "$LINENO" "$ac_func" "$as_ac_var"
diff --git a/gdb/config.in b/gdb/config.in
index cd4ce92..8585b49 100644
--- a/gdb/config.in
+++ b/gdb/config.in
@@ -366,6 +366,9 @@ 
 /* Define to 1 if you have the `sigaction' function. */
 #undef HAVE_SIGACTION
 
+/* Define to 1 if you have the `sigaltstack' function. */
+#undef HAVE_SIGALTSTACK
+
 /* Define to 1 if you have the <signal.h> header file. */
 #undef HAVE_SIGNAL_H
 
diff --git a/gdb/main.c b/gdb/main.c
index a9fc378..7031cc3 100644
--- a/gdb/main.c
+++ b/gdb/main.c
@@ -45,6 +45,7 @@ 
 
 #include "filenames.h"
 #include "filestuff.h"
+#include <signal.h>
 
 /* The selected interpreter.  This will be used as a set command
    variable, so it should always be malloc'ed - since
@@ -288,6 +289,27 @@  get_init_files (const char **system_gdbinit,
   *local_gdbinit = localinit;
 }
 
+/* Try to set up an alternate signal stack for SIGSEGV handlers.
+   This allows us to handle SIGSEGV signals generated when the
+   normal process stack is exhausted.  If this stack is not set
+   up (sigaltstack is unavailable or fails) and a SIGSEGV is
+   generated when the normal stack is exhausted then the program
+   will behave as though no SIGSEGV handler was installed.  */
+
+static void
+setup_alternate_signal_stack (void)
+{
+#ifdef HAVE_SIGALTSTACK
+  stack_t ss;
+
+  ss.ss_sp = xmalloc (SIGSTKSZ);
+  ss.ss_size = SIGSTKSZ;
+  ss.ss_flags = 0;
+
+  sigaltstack(&ss, NULL);
+#endif
+}
+
 /* Call command_loop.  If it happens to return, pass that through as a
    non-zero return status.  */
 
@@ -785,6 +807,9 @@  captured_main (void *data)
       quiet = 1;
   }
 
+  /* Try to set up an alternate signal stack for SIGSEGV handlers.  */
+  setup_alternate_signal_stack ();
+
   /* Initialize all files.  Give the interpreter a chance to take
      control of the console via the deprecated_init_ui_hook ().  */
   gdb_init (gdb_program_name);
diff --git a/gdb/cp-support.c b/gdb/cp-support.c
index 91533e8..f4dde70 100644
--- a/gdb/cp-support.c
+++ b/gdb/cp-support.c
@@ -35,6 +35,7 @@ 
 #include "expression.h"
 #include "value.h"
 #include "cp-abi.h"
+#include <signal.h>
 
 #include "safe-ctype.h"
 
@@ -1482,12 +1483,142 @@  cp_lookup_rtti_type (const char *name, struct block *block)
   return rtti_type;
 }
 
+#ifdef HAVE_WORKING_FORK
+
+/* If nonzero, attempt to catch crashes in the demangler and print
+   useful debugging information.  */
+
+static int catch_demangler_crashes = 1;
+
+/* Wrap set/long jmp so that it's more portable.  */
+
+#if defined(HAVE_SIGSETJMP)
+#define SIGJMP_BUF		sigjmp_buf
+#define SIGSETJMP(buf)		sigsetjmp((buf), 1)
+#define SIGLONGJMP(buf,val)	siglongjmp((buf), (val))
+#else
+#define SIGJMP_BUF		jmp_buf
+#define SIGSETJMP(buf)		setjmp(buf)
+#define SIGLONGJMP(buf,val)	longjmp((buf), (val))
+#endif
+
+/* Stack context and environment for demangler crash recovery.  */
+
+static SIGJMP_BUF gdb_demangle_jmp_buf;
+
+/* If nonzero, attempt to dump core from the signal handler.  */
+
+static int gdb_demangle_attempt_core_dump = 1;
+
+/* Signal handler for gdb_demangle.  */
+
+static void
+gdb_demangle_signal_handler (int signo)
+{
+  if (gdb_demangle_attempt_core_dump)
+    {
+      if (fork () == 0)
+	dump_core ();
+
+      gdb_demangle_attempt_core_dump = 0;
+    }
+
+  SIGLONGJMP (gdb_demangle_jmp_buf, signo);
+}
+
+#endif
+
 /* A wrapper for bfd_demangle.  */
 
 char *
 gdb_demangle (const char *name, int options)
 {
-  return bfd_demangle (NULL, name, options);
+  char *result = NULL;
+  int crash_signal = 0;
+
+#ifdef HAVE_WORKING_FORK
+#if defined (HAVE_SIGACTION) && defined (SA_RESTART)
+  struct sigaction sa, old_sa;
+#else
+  void (*ofunc) ();
+#endif
+  static int core_dump_allowed = -1;
+
+  if (core_dump_allowed == -1)
+    {
+      core_dump_allowed = can_dump_core (LIMIT_CUR);
+
+      if (!core_dump_allowed)
+	gdb_demangle_attempt_core_dump = 0;
+    }
+
+  if (catch_demangler_crashes)
+    {
+#if defined (HAVE_SIGACTION) && defined (SA_RESTART)
+      sa.sa_handler = gdb_demangle_signal_handler;
+      sigemptyset (&sa.sa_mask);
+      sa.sa_flags = SA_ONSTACK;
+      sigaction (SIGSEGV, &sa, &old_sa);
+#else
+      ofunc = (void (*)()) signal (SIGSEGV, gdb_demangle_signal_handler);
+#endif
+
+      crash_signal = SIGSETJMP (gdb_demangle_jmp_buf);
+    }
+#endif
+
+  if (crash_signal == 0)
+    result = bfd_demangle (NULL, name, options);
+
+#ifdef HAVE_WORKING_FORK
+  if (catch_demangler_crashes)
+    {
+#if defined (HAVE_SIGACTION) && defined (SA_RESTART)
+      sigaction (SIGSEGV, &old_sa, NULL);
+#else
+      signal (SIGSEGV, ofunc);
+#endif
+
+      if (crash_signal != 0)
+	{
+	  static int error_reported = 0;
+
+	  if (!error_reported)
+	    {
+	      char *short_msg, *long_msg;
+	      struct cleanup *back_to;
+
+	      short_msg = xstrprintf (_("unable to demangle '%s' "
+				      "(demangler failed with signal %d)"),
+				    name, crash_signal);
+	      back_to = make_cleanup (xfree, short_msg);
+
+	      long_msg = xstrprintf ("%s:%d: %s: %s", __FILE__, __LINE__,
+				    "demangler-warning", short_msg);
+	      make_cleanup (xfree, long_msg);
+
+	      target_terminal_ours ();
+	      begin_line ();
+	      if (core_dump_allowed)
+		fprintf_unfiltered (gdb_stderr,
+				    _("%s\nAttempting to dump core.\n"),
+				    long_msg);
+	      else
+		warn_cant_dump_core (long_msg);
+
+	      demangler_warning (__FILE__, __LINE__, "%s", short_msg);
+
+	      do_cleanups (back_to);
+
+	      error_reported = 1;
+	    }
+
+	  result = NULL;
+	}
+    }
+#endif
+
+  return result;
 }
 
 /* Don't allow just "maintenance cplus".  */
@@ -1562,4 +1693,17 @@  _initialize_cp_support (void)
 Usage: info vtbl EXPRESSION\n\
 Evaluate EXPRESSION and display the virtual function table for the\n\
 resulting object."));
+
+#ifdef HAVE_WORKING_FORK
+  add_setshow_boolean_cmd ("catch-demangler-crashes", class_maintenance,
+			   &catch_demangler_crashes, _("\
+Set whether to attempt to catch demangler crashes."), _("\
+Show whether to attempt to catch demangler crashes."), _("\
+If enabled GDB will attempt to catch demangler crashes and\n\
+display the offending symbol."),
+			   NULL,
+			   NULL,
+			   &maintenance_set_cmdlist,
+			   &maintenance_show_cmdlist);
+#endif
 }
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 9f7fa18..242117b 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -33236,6 +33236,17 @@  Expand symbol tables.
 If @var{regexp} is specified, only expand symbol tables for file
 names matching @var{regexp}.
 
+@kindex maint set catch-demangler-crashes
+@kindex maint show catch-demangler-crashes
+@cindex demangler crashes
+@item maint set catch-demangler-crashes [on|off]
+@itemx maint show catch-demangler-crashes
+Control whether @value{GDBN} should attempt to catch crashes in the
+symbol name demangler.  The default is to attempt to catch crashes.
+If enabled, the first time a crash is caught, a core file is created,
+the offending symbol is displayed and the user is presented with the
+option to terminate the current session.
+
 @kindex maint cplus first_component
 @item maint cplus first_component @var{name}
 Print the first C@t{++} class/namespace component of @var{name}.