[2/2,v3] Demangler crash handler
Commit Message
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.
Eli pointed out that SIGSEGV is an ANSI-standard signal but I found
various other SIGSEGV checks in GDB so I have left the preprocessor
conditionals intact for consistency. I hope this is ok.
gdb/
2014-06-04 Gary Benson <gbenson@redhat.com>
* utils.h (dump_core): New declaration.
* utils.c (dump_core): Make non-static.
* 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_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-04 Gary Benson <gbenson@redhat.com>
* gdb.texinfo (Maintenance Commands): Document new
"maint set/show catch-demangler-crashes" option.
Comments
> Date: Wed, 4 Jun 2014 11:09:57 +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.
The documentation part is OK.
> Eli pointed out that SIGSEGV is an ANSI-standard signal but I found
> various other SIGSEGV checks in GDB
They should all be removed.
Eli Zaretskii wrote:
> > From: Gary Benson <gbenson@redhat.com>
> >
> > Eli pointed out that SIGSEGV is an ANSI-standard signal but I
> > found various other SIGSEGV checks in GDB
>
> They should all be removed.
Ok, I'll do this. Should I commit the change as obvious?
Thanks,
Gary
> Date: Wed, 4 Jun 2014 14:36:03 +0100
> From: Gary Benson <gbenson@redhat.com>
> Cc: gdb-patches@sourceware.org, aburgess@broadcom.com, xdje42@gmail.com,
> fw@deneb.enyo.de, mark.kettenis@xs4all.nl, palves@redhat.com,
> tromey@redhat.com
>
> Eli Zaretskii wrote:
> > > From: Gary Benson <gbenson@redhat.com>
> > >
> > > Eli pointed out that SIGSEGV is an ANSI-standard signal but I
> > > found various other SIGSEGV checks in GDB
> >
> > They should all be removed.
>
> Ok, I'll do this. Should I commit the change as obvious?
I think so, yes.
Eli Zaretskii wrote:
> > Date: Wed, 4 Jun 2014 14:36:03 +0100
> > From: Gary Benson <gbenson@redhat.com>
> > Cc: gdb-patches@sourceware.org, aburgess@broadcom.com, xdje42@gmail.com,
> > fw@deneb.enyo.de, mark.kettenis@xs4all.nl, palves@redhat.com,
> > tromey@redhat.com
> >
> > Eli Zaretskii wrote:
> > > > From: Gary Benson <gbenson@redhat.com>
> > > >
> > > > Eli pointed out that SIGSEGV is an ANSI-standard signal but I
> > > > found various other SIGSEGV checks in GDB
> > >
> > > They should all be removed.
> >
> > Ok, I'll do this. Should I commit the change as obvious?
>
> I think so, yes.
Ok, I'll do that.
Thanks,
Gary
On Wed, Jun 4, 2014 at 7:28 AM, Gary Benson <gbenson@redhat.com> wrote:
> Eli Zaretskii wrote:
>> > Date: Wed, 4 Jun 2014 14:36:03 +0100
>> > From: Gary Benson <gbenson@redhat.com>
>> > Cc: gdb-patches@sourceware.org, aburgess@broadcom.com, xdje42@gmail.com,
>> > fw@deneb.enyo.de, mark.kettenis@xs4all.nl, palves@redhat.com,
>> > tromey@redhat.com
>> >
>> > Eli Zaretskii wrote:
>> > > > From: Gary Benson <gbenson@redhat.com>
>> > > >
>> > > > Eli pointed out that SIGSEGV is an ANSI-standard signal but I
>> > > > found various other SIGSEGV checks in GDB
>> > >
>> > > They should all be removed.
>> >
>> > Ok, I'll do this. Should I commit the change as obvious?
>>
>> I think so, yes.
>
> Ok, I'll do that.
Are we talking about #ifdef SIGSEGV in, e.g., common/signals.c?
[assuming that's correct ...]
If one goes down this path, I think the patch while perhaps "obvious"
would become a bit involved (why just SEGV?) and thus the obviousness
diminishes.
I think it diminishes to a point where the obviousness is gone.
Please submit any such patch for review.
Thanks!
Hi. A few comments inline.
Gary Benson <gbenson@redhat.com> writes:
> 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.
>
> Eli pointed out that SIGSEGV is an ANSI-standard signal but I found
> various other SIGSEGV checks in GDB so I have left the preprocessor
> conditionals intact for consistency. I hope this is ok.
>
>
> gdb/
> 2014-06-04 Gary Benson <gbenson@redhat.com>
>
> * utils.h (dump_core): New declaration.
> * utils.c (dump_core): Make non-static.
> * 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_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.
>
> [...]
>
> +/* Stack context and environment for demangler crash recovery. */
> +
> +static SIGJMP_BUF gdb_demangle_jmp_buf;
> +
> +/* Signal handler for gdb_demangle. */
> +
> +static void
> +gdb_demangle_signal_handler (int signo)
> +{
> + static int core_dumped = 0;
> +
> + if (!core_dumped)
> + {
> + if (fork () == 0)
> + dump_core ();
IIUC you're skipping the can_dump_core() check.
If the user has set ulimit -c 0, I think that needs to be obeyed.
I realize can_dump_core may call fprintf which we can't do here,
but you could still IMO call getrlimit.
IWBN to still call can_dump_core (or whatever) so that the
implementation of the check is still tucked away in a function.
> +
> + core_dumped = 1;
> + }
> +
> + 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;
> +
> +#if defined (SIGSEGV) && defined (HAVE_WORKING_FORK)
> +#if defined (HAVE_SIGACTION) && defined (SA_RESTART)
> + struct sigaction sa, old_sa;
> +
> + if (catch_demangler_crashes)
> + {
> + sa.sa_handler = gdb_demangle_signal_handler;
> + sigemptyset (&sa.sa_mask);
> + sa.sa_flags = 0;
> + sigaction (SIGSEGV, &sa, &old_sa);
> + }
> +#else
> + void (*ofunc) ();
> +
> + if (catch_demangler_crashes)
> + ofunc = (void (*)()) signal (SIGSEGV, gdb_demangle_signal_handler);
> +#endif
> +
> + if (catch_demangler_crashes)
> + crash_signal = SIGSETJMP (gdb_demangle_jmp_buf);
> +#endif
> +
> + if (crash_signal == 0)
> + result = bfd_demangle (NULL, name, options);
> +
> +#if defined (SIGSEGV) && defined (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)
For myself as a user I'd like the warning for every demangle failure.
[I'd throttle it by unique symbols though.]
> + {
> + demangler_warning (__FILE__, __LINE__,
> + _("unable to demangle '%s' "
> + "(demangler failed with signal %d)"),
> + name, crash_signal);
> +
> + error_reported = 1;
> + }
> +
> + result = NULL;
> + }
> + }
> +#endif
> +
> + return result;
> }
Doug Evans wrote:
> On Wed, Jun 4, 2014 at 7:28 AM, Gary Benson <gbenson@redhat.com> wrote:
> > Eli Zaretskii wrote:
> > > > From: Gary Benson <gbenson@redhat.com>
> > > >
> > > > Eli Zaretskii wrote:
> > > > > > From: Gary Benson <gbenson@redhat.com>
> > > > > >
> > > > > > Eli pointed out that SIGSEGV is an ANSI-standard signal
> > > > > > but I found various other SIGSEGV checks in GDB
> > > > >
> > > > > They should all be removed.
> > > >
> > > > Ok, I'll do this. Should I commit the change as obvious?
> > >
> > > I think so, yes.
> >
> > Ok, I'll do that.
>
> Are we talking about #ifdef SIGSEGV in, e.g., common/signals.c?
Yes.
> If one goes down this path, I think the patch while perhaps
> "obvious" would become a bit involved (why just SEGV?) and
> thus the obviousness diminishes.
> I think it diminishes to a point where the obviousness is gone.
> Please submit any such patch for review.
Having started looking into this I am inclined to agree.
I couldn't find an authoritative list, but the Linux kernel sources
indicate that SIGINT, SIGILL, SIGABRT, SIGFPE, SIGSEGV and SIGTERM
are ANSI. Eli said that list agreed with his references, so I'll
work on unwrapping those.
I will post the patch for review.
Thanks,
Gary
Doug Evans wrote:
> Gary Benson <gbenson@redhat.com> writes:
> > +static void
> > +gdb_demangle_signal_handler (int signo)
> > +{
> > + static int core_dumped = 0;
> > +
> > + if (!core_dumped)
> > + {
> > + if (fork () == 0)
> > + dump_core ();
>
> IIUC you're skipping the can_dump_core() check.
> If the user has set ulimit -c 0, I think that needs to be obeyed.
> I realize can_dump_core may call fprintf which we can't do here,
> but you could still IMO call getrlimit.
> IWBN to still call can_dump_core (or whatever) so that the
> implementation of the check is still tucked away in a function.
Ah, I saw can_dump_core but didn't get what it was doing.
I'll refactor it so that the check is performed once, before the
signal handler is installed, and the message is printed outside
the signal handler if a crash is caught.
> > + if (crash_signal != 0)
> > + {
> > + static int error_reported = 0;
> > +
> > + if (!error_reported)
>
> For myself as a user I'd like the warning for every demangle failure.
> [I'd throttle it by unique symbols though.]
My reasoning here was that any subsequent failures could be caused by
the first, by memory corruption or some leftover state, so they could
be bogus and lead us into wild goose chases.
I can certainly put together a patch with throttled warnings as you
describe if you prefer. Let me know :)
Thanks,
Gary
On Wed, Jun 4, 2014 at 11:25 AM, Gary Benson <gbenson@redhat.com> wrote:
> Doug Evans wrote:
>> On Wed, Jun 4, 2014 at 7:28 AM, Gary Benson <gbenson@redhat.com> wrote:
>> > Eli Zaretskii wrote:
>> > > > From: Gary Benson <gbenson@redhat.com>
>> > > >
>> > > > Eli Zaretskii wrote:
>> > > > > > From: Gary Benson <gbenson@redhat.com>
>> > > > > >
>> > > > > > Eli pointed out that SIGSEGV is an ANSI-standard signal
>> > > > > > but I found various other SIGSEGV checks in GDB
>> > > > >
>> > > > > They should all be removed.
>> > > >
>> > > > Ok, I'll do this. Should I commit the change as obvious?
>> > >
>> > > I think so, yes.
>> >
>> > Ok, I'll do that.
>>
>> Are we talking about #ifdef SIGSEGV in, e.g., common/signals.c?
>
> Yes.
>
>> If one goes down this path, I think the patch while perhaps
>> "obvious" would become a bit involved (why just SEGV?) and
>> thus the obviousness diminishes.
>> I think it diminishes to a point where the obviousness is gone.
>> Please submit any such patch for review.
>
> Having started looking into this I am inclined to agree.
>
> I couldn't find an authoritative list, but the Linux kernel sources
> indicate that SIGINT, SIGILL, SIGABRT, SIGFPE, SIGSEGV and SIGTERM
> are ANSI. Eli said that list agreed with his references, so I'll
> work on unwrapping those.
>
> I will post the patch for review.
One thing that I think should be considered is that we'll go from the
simple state of "just ifdef every signal" in places like
common/signals.c to having some signals you are required to not ifdef
and some you do, and needing to know which category every signal fits
in. I don't have a strong opinion, but I'm ok with the status quo.
> Date: Wed, 4 Jun 2014 18:11:16 -0700
> From: Doug Evans <xdje42@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>, "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>,
> Andrew Burgess <aburgess@broadcom.com>, Florian Weimer <fw@deneb.enyo.de>,
> Mark Kettenis <mark.kettenis@xs4all.nl>, Pedro Alves <palves@redhat.com>,
> Tom Tromey <tromey@redhat.com>
>
> One thing that I think should be considered is that we'll go from the
> simple state of "just ifdef every signal" in places like
> common/signals.c to having some signals you are required to not ifdef
> and some you do, and needing to know which category every signal fits
> in. I don't have a strong opinion, but I'm ok with the status quo.
The list of ANSI-standard signals is short, and it's easy to check
which signal is or isn't.
Ifdef's are ugly. Minimizing their use is a Good Thing.
@@ -374,4 +374,8 @@ extern ULONGEST align_down (ULONGEST v, int n);
extern LONGEST gdb_sign_extend (LONGEST value, int bit);
+/* Dump core trying to increase the core soft limit to hard limit first. */
+
+extern void dump_core (void);
+
#endif /* UTILS_H */
@@ -600,7 +600,7 @@ error_stream (struct ui_file *stream)
/* Dump core trying to increase the core soft limit to hard limit first. */
-static void
+void
dump_core (void)
{
#ifdef HAVE_SETRLIMIT
@@ -36,6 +36,7 @@
#include "value.h"
#include "cp-abi.h"
#include "language.h"
+#include <signal.h>
#include "safe-ctype.h"
@@ -1505,12 +1506,111 @@ cp_lookup_rtti_type (const char *name, struct block *block)
return rtti_type;
}
+#if defined (SIGSEGV) && defined (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;
+
+/* Signal handler for gdb_demangle. */
+
+static void
+gdb_demangle_signal_handler (int signo)
+{
+ static int core_dumped = 0;
+
+ if (!core_dumped)
+ {
+ if (fork () == 0)
+ dump_core ();
+
+ core_dumped = 1;
+ }
+
+ 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;
+
+#if defined (SIGSEGV) && defined (HAVE_WORKING_FORK)
+#if defined (HAVE_SIGACTION) && defined (SA_RESTART)
+ struct sigaction sa, old_sa;
+
+ if (catch_demangler_crashes)
+ {
+ sa.sa_handler = gdb_demangle_signal_handler;
+ sigemptyset (&sa.sa_mask);
+ sa.sa_flags = 0;
+ sigaction (SIGSEGV, &sa, &old_sa);
+ }
+#else
+ void (*ofunc) ();
+
+ if (catch_demangler_crashes)
+ ofunc = (void (*)()) signal (SIGSEGV, gdb_demangle_signal_handler);
+#endif
+
+ if (catch_demangler_crashes)
+ crash_signal = SIGSETJMP (gdb_demangle_jmp_buf);
+#endif
+
+ if (crash_signal == 0)
+ result = bfd_demangle (NULL, name, options);
+
+#if defined (SIGSEGV) && defined (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)
+ {
+ demangler_warning (__FILE__, __LINE__,
+ _("unable to demangle '%s' "
+ "(demangler failed with signal %d)"),
+ name, crash_signal);
+
+ error_reported = 1;
+ }
+
+ result = NULL;
+ }
+ }
+#endif
+
+ return result;
}
/* Don't allow just "maintenance cplus". */
@@ -1585,4 +1685,17 @@ _initialize_cp_support (void)
Usage: info vtbl EXPRESSION\n\
Evaluate EXPRESSION and display the virtual function table for the\n\
resulting object."));
+
+#if defined (SIGSEGV) && defined (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
}
@@ -33235,6 +33235,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}.