[2/2,v2] Demangler crash handler

  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

--
  

> Date: Mon, 19 May 2014 16:48:23 +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
> 
> > 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)

Yes, I knew all that (because I've read all the deliberations here
about this feature).  I'm asking why do we need this option, instead
of having its ON effect by default?

> --- 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

Please add here

 @cindex demangler crashes

Otherwise, this part is OK.  Thanks.
  

Eli Zaretskii wrote:
> > Date: Mon, 19 May 2014 16:48:23 +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
> > 
> > > 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)
>
> Yes, I knew all that (because I've read all the deliberations here
> about this feature).  I'm asking why do we need this option, instead
> of having its ON effect by default?

Ah, my apologies.  On by default is my preference--it seems to work,
and it doesn't rob performance--but I don't think that will to be
accepted because there's no way to do this without (sig)longjmp, and
that isn't safe to call from a signal handler.  A disabled-by-default
catcher is at least somewhat helpful in triaging all these demangler
bugs that are coming in now people are starting to use C++11 features.

> > --- 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
> 
> Please add here
> 
>  @cindex demangler crashes
> 
> Otherwise, this part is OK.  Thanks.

Thank you.
Gary
  

Wouldn't a new "set debug demangle on" option, that would make
GDB output:

 (gdb) bt / whatever-gdb-command-that-triggers-demangling
 demangling _ZN2CV1mEi ... CV::m(int)
 demangling _Zwhatever ... <NULL>

be just as helpful, and, even potentially be helpful to debug
scenarios where GDB/libiberty might get the demangling wrong,
but not cause a crash?

Seems like a natural and easy sell to me.
  

> Date: Mon, 19 May 2014 21:41:37 +0100
> From: Pedro Alves <palves@redhat.com>
> CC: gdb-patches@sourceware.org, aburgess@broadcom.com, xdje42@gmail.com,
>         fw@deneb.enyo.de, mark.kettenis@xs4all.nl, tromey@redhat.com
> 
> Wouldn't a new "set debug demangle on" option, that would make
> GDB output:
> 
>  (gdb) bt / whatever-gdb-command-that-triggers-demangling
>  demangling _ZN2CV1mEi ... CV::m(int)
>  demangling _Zwhatever ... <NULL>
> 
> be just as helpful, and, even potentially be helpful to debug
> scenarios where GDB/libiberty might get the demangling wrong,
> but not cause a crash?
> 
> Seems like a natural and easy sell to me.

Yes, I agree.
  

Pedro Alves wrote:
> Wouldn't a new "set debug demangle on" option, that would make
> GDB output:
> 
>  (gdb) bt / whatever-gdb-command-that-triggers-demangling
>  demangling _ZN2CV1mEi ... CV::m(int)
>  demangling _Zwhatever ... <NULL>
> 
> be just as helpful, and, even potentially be helpful to debug
> scenarios where GDB/libiberty might get the demangling wrong,
> but not cause a crash?

It would be as helpful for obtaining the symbol, although you would
get a lot more output (hundreds of thousands/millions of lines vs one
line).  It would not help the user to debug their program because GDB
would still crash.

Thanks,
Gary

--
http://gbenson.net/
  

* Pedro Alves:

> Wouldn't a new "set debug demangle on" option, that would make
> GDB output:
>
>  (gdb) bt / whatever-gdb-command-that-triggers-demangling
>  demangling _ZN2CV1mEi ... CV::m(int)
>  demangling _Zwhatever ... <NULL>
>
> be just as helpful, and, even potentially be helpful to debug
> scenarios where GDB/libiberty might get the demangling wrong,
> but not cause a crash?

Here's another idea: You could strcpy the input string to a mapped
file (say, ~/.config/gdb/demangler) and read that on startup, printing
it and adding the offending symbol to a blacklist (say,
~/.config/gdb/demangler.blacklist).