Re: [PATCH 2/2 v2] Demangler crash handler

[Gary Benson <gbenson@redhat.com>]

Eli Zaretskii wrote:
> > Date: Mon, 19 May 2014 12:48:02 +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>
> > 
> > The main change I have made is to cause the crash handler to be
> > disabled by default.  The user must explicitly enable the handler
> > with "maint set catch-demangler-crashes on".  This will simplify
> > triage of bugs such as PR 16957 [2], a new demangler crash that
> > was reported on Friday.  The user did not supply enough
> > information to see the offending symbol, and I don't have the
> > necessary compiler or libraries to reproduce the bug locally.
> > With this patch we can instruct the user to enter "maint set
> > catch-demangler-crashes on" and repeat whatever they did to cause
> > the crash in the first place.  We can then easily obtain the first
> > offending symbol GDB encountered.
> 
> Can't say this option makes sense to me.  Isn't there a way to
> display the necessary information in a message, even though you
> catch the signal?

To clarify, the current situation in GDB is that crashes in the
demangler are not caught:

  (gdb) set lang c++
  (gdb) maint demangle _Z1-Av23*;cG~Wo2Vu
  Segmentation fault (core dumped)

With the patch, that is also the default situation.  But with the
patch, with "maint set catch-demangler-crashes on", a signal handler
is installed across calls to the demangler, so that if the demangler
crashes you get something like this:

  (gdb) set lang c++
  (gdb) maint set catch-demangler-crashes on
  (gdb) maint demangle _Z1-Av23*;cG~Wo2Vu
  /home/gary/work/archer/demangle-crashcatcher/src/gdb/cp-support.c:1590: internal-warning: unable to demangle '_Z1-Av23*;cG~Wo2Vu' (demangler failed with signal 11)
  A problem internal to GDB has been detected,
  further debugging may prove unreliable.
  Quit this debugging session? (y or n) y
  
  /home/gary/work/archer/demangle-crashcatcher/src/gdb/cp-support.c:1590: internal-warning: unable to demangle '_Z1-Av23*;cG~Wo2Vu' (demangler failed with signal 11)
  A problem internal to GDB has been detected,
  further debugging may prove unreliable.
  Create a core file of GDB? (y or n) y
  Aborted (core dumped)

> > maint set catch-demangler-crashes (on|off)
> > maint show catch-demangler-crashes
> >   Control whether the debugger should attempt to catch crashes in the
> >   symbol name demangler.  The default is not to attempt to catch
> >   crashes.  The first time a crash is caught the offending symbol is
> >   displayed and the user is presented with options to terminate the
> >   current session and/or to create a core file.
> 
> Given this description, it sounds like all the necessary information
> is already displayed when the crash is caught.  So why would we need
> an option?

Did my above explanation answer this question?

> > gdb/doc/
> > 2014-05-19  Gary Benson  <gbenson@redhat.com>
> > 
> > 	* gdb.texinfo (Maintenance Commands): Document new
> > 	"maint set/show catch-demangler-crashes" option.
> 
> This part of the patch was absent from what you sent.

My apologies.  And the whole reason I Cc'd you was because the patch
contained documentation changes :)  I've inlined that part at the end
of this mail.

> > +#ifdef SIGSEGV
> 
> AFAIK, SIGSEGV is an ANSI-standard signal, so I don't think you need a
> preprocessor conditional here.

Ok, I can remove this.

> > +  add_setshow_boolean_cmd ("catch-demangler-crashes", class_maintenance,
> > +			   &catch_demangler_crashes, _("\
> > +Set whether to attempt to catch demangler crashes."), _("\
> > +Show whether GDB will attempt to catch demangler crashes."), _("\
> 
> The "Set" and "Show" lines should be identical except for the
> initial word.

Ok, I will change the second to:

  +Show whether to attempt to catch demangler crashes."), _("\

Thanks,
Gary

--
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index a6bde12..32f33a9 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -33142,6 +33142,16 @@ 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
+@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 not to attempt to catch
+crashes.  If enabled, the first time a crash is caught the offending
+symbol is displayed and the user is presented with options to
+terminate the current session and to create a core file.
+
 @kindex maint cplus first_component
 @item maint cplus first_component @var{name}
 Print the first C@t{++} class/namespace component of @var{name}.