[3/3,v4] Demangler crash handler

  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-05  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-05  Gary Benson  <gbenson@redhat.com>

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

* Gary Benson:

> +      sigaction (SIGSEGV, &sa, &old_sa);

> +      crash_signal = SIGSETJMP (gdb_demangle_jmp_buf);

> +      sigaction (SIGSEGV, &old_sa, NULL);

That's quite a bit of additional work for each demangler invocation.
Is this visible with things like tab completion?
  

Florian Weimer wrote:
> * Gary Benson:
> > +      sigaction (SIGSEGV, &sa, &old_sa);
> > +      crash_signal = SIGSETJMP (gdb_demangle_jmp_buf);
> > +      sigaction (SIGSEGV, &old_sa, NULL);
> 
> That's quite a bit of additional work for each demangler invocation.
> Is this visible with things like tab completion?

I tested this a while back: it's not noticable.

FWIW I did time gdb -nx -batch \
                /usr/lib64/libreoffice/program/soffice.bin \
                -ex "start" -ex "complete b" > /dev/null

That invokes the demangler some 3,100,000 times.  I don't remember
the exact times but they were of the order of 50s in both cases.

Thanks,
Gary
  

* Gary Benson:

> Florian Weimer wrote:
>> * Gary Benson:
>> > +      sigaction (SIGSEGV, &sa, &old_sa);
>> > +      crash_signal = SIGSETJMP (gdb_demangle_jmp_buf);
>> > +      sigaction (SIGSEGV, &old_sa, NULL);
>> 
>> That's quite a bit of additional work for each demangler invocation.
>> Is this visible with things like tab completion?
>
> I tested this a while back: it's not noticable.
>
> FWIW I did time gdb -nx -batch \
>                 /usr/lib64/libreoffice/program/soffice.bin \
>                 -ex "start" -ex "complete b" > /dev/null
>
> That invokes the demangler some 3,100,000 times.  I don't remember
> the exact times but they were of the order of 50s in both cases.

Good to know, thanks.
  

On 05/06/2014 2:03 PM, Gary Benson wrote:

> 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

> +
> +/* Signal handler for gdb_demangle.  */
> +
> +static void
> +gdb_demangle_signal_handler (int signo)
> +{
> +  if (gdb_demangle_attempt_core_dump)
> +    {
> +      if (fork () == 0)
> +	dump_core ();

This worries me a little, when a problem case occurs gdb will dump 
core regardless of the users ulimit setting, without first asking
the user, and doesn't tell the user that a core file was created.

This feels quite unexpected behaviour to me, especially the bit about
disregarding the ulimit setting without first asking for permission.

Catching the crash feels like a good idea, but I'd prefer that gdb ask
before circumventing the ulimit and dumping core.  Alternatively we
could just not dump core from gdb, report the bad symbol and let the
user file a bug.  With the demangler being so deterministic it should
be possible to reproduce, if not, then we just ask the user to turn
off the crash catch, adjust their ulimit (like we would with any other
gdb SEGV crash), and rerun the test.

If we really want to create the core file by default, but aren't going
to ask, then I'd propose we honour the ulimit setting, and make sure
that the user is told that a core file was just written.

Thanks,
Andrew
  

Andrew Burgess wrote:
> On 05/06/2014 2:03 PM, Gary Benson wrote:
> > 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
> 
> > +
> > +/* Signal handler for gdb_demangle.  */
> > +
> > +static void
> > +gdb_demangle_signal_handler (int signo)
> > +{
> > +  if (gdb_demangle_attempt_core_dump)
> > +    {
> > +      if (fork () == 0)
> > +	dump_core ();
> 
> This worries me a little, when a problem case occurs gdb will dump
> core regardless of the users ulimit setting, without first asking
> the user, and doesn't tell the user that a core file was created.
> 
> This feels quite unexpected behaviour to me, especially the bit
> about disregarding the ulimit setting without first asking for
> permission.
> 
> Catching the crash feels like a good idea, but I'd prefer that gdb
> ask before circumventing the ulimit and dumping core.

This part of the same patch:

+  if (core_dump_allowed == -1)
+    {
+      core_dump_allowed = can_dump_core ();
+
+      if (!core_dump_allowed)
+        gdb_demangle_attempt_core_dump = 0;
+    }

calls this:

  int
  can_dump_core (void)
  {
  #ifdef HAVE_GETRLIMIT
    struct rlimit rlim;
  
    /* Be quiet and assume we can dump if an error is returned.  */
    if (getrlimit (RLIMIT_CORE, &rlim) != 0)
      return 1;
  
    if (rlim.rlim_max == 0)
      return 0;
  #endif /* HAVE_GETRLIMIT */
  
    return 1;
  }
		  
which inhibits the core dump if the user's ulimit is 0.

> Alternatively we could just not dump core from gdb, report the bad
> symbol and let the user file a bug.  With the demangler being so
> deterministic it should be possible to reproduce, if not, then we
> just ask the user to turn off the crash catch, adjust their ulimit
> (like we would with any other gdb SEGV crash), and rerun the test.

That was and is my preferred solution, but Mark Kettenis indicated
that he would not accept the patch unless a meaningful core file was
created.

> If we really want to create the core file by default, but aren't
> going to ask, then I'd propose we honour the ulimit setting, and
> make sure that the user is told that a core file was just written.

The problem with asking is that you'd have to ask within the signal
handler, and no code that prints to the screen is safe to call from
within a signal handler.

Even indicating that a core file was written is probably impossible:
you just have to abort and hope for the best.  The nearest I could
do is set a flag in the signal handler and have the code it returns
to print "Attempting to dump core" or some such thing.

Thanks,
Gary
  

On 09/06/2014 10:01 AM, Gary Benson wrote:
> Andrew Burgess wrote:
>> On 05/06/2014 2:03 PM, Gary Benson wrote:
>>> 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
>>
>>> +
>>> +/* Signal handler for gdb_demangle.  */
>>> +
>>> +static void
>>> +gdb_demangle_signal_handler (int signo)
>>> +{
>>> +  if (gdb_demangle_attempt_core_dump)
>>> +    {
>>> +      if (fork () == 0)
>>> +	dump_core ();
>>
>> This worries me a little, when a problem case occurs gdb will dump
>> core regardless of the users ulimit setting, without first asking
>> the user, and doesn't tell the user that a core file was created.
>>
>> This feels quite unexpected behaviour to me, especially the bit
>> about disregarding the ulimit setting without first asking for
>> permission.
>>
>> Catching the crash feels like a good idea, but I'd prefer that gdb
>> ask before circumventing the ulimit and dumping core.
> 
> This part of the same patch:
> 
> +  if (core_dump_allowed == -1)
> +    {
> +      core_dump_allowed = can_dump_core ();
> +
> +      if (!core_dump_allowed)
> +        gdb_demangle_attempt_core_dump = 0;
> +    }
> 
> calls this:
> 
>   int
>   can_dump_core (void)
>   {
>   #ifdef HAVE_GETRLIMIT
>     struct rlimit rlim;
>   
>     /* Be quiet and assume we can dump if an error is returned.  */
>     if (getrlimit (RLIMIT_CORE, &rlim) != 0)
>       return 1;
>   
>     if (rlim.rlim_max == 0)
>       return 0;
>   #endif /* HAVE_GETRLIMIT */
>   
>     return 1;
>   }
> 		  
> which inhibits the core dump if the user's ulimit is 0.

Ahh, yes I see.

So the problem here is this function is geared towards the /old/ use of the function
where we are about to ask the user if we should dump core.  For that, this function 
was correct, we check the hard limit of the resource.  If the hard limit is high then
we ask the user, and dump core.

However, in doing so we circumvent the soft limit rlim.rlim_cur.  So I think my point
still stands.  The user has said "no core files please", and we create one without 
asking.  If we must go down this road then I think we need two functions to check
the two different limits.

>> Alternatively we could just not dump core from gdb, report the bad
>> symbol and let the user file a bug.  With the demangler being so
>> deterministic it should be possible to reproduce, if not, then we
>> just ask the user to turn off the crash catch, adjust their ulimit
>> (like we would with any other gdb SEGV crash), and rerun the test.
> 
> That was and is my preferred solution, but Mark Kettenis indicated
> that he would not accept the patch unless a meaningful core file was
> created.

I don't understand that position, but I'd hope he'd agree that we
should respect the user ulimit over creating a core file...

> 
>> If we really want to create the core file by default, but aren't
>> going to ask, then I'd propose we honour the ulimit setting, and
>> make sure that the user is told that a core file was just written.
> 
> The problem with asking is that you'd have to ask within the signal
> handler, and no code that prints to the screen is safe to call from
> within a signal handler.

Indeed.  I did wonder about some horrible synchronisation scheme where
the "master" gdb process queries the user then signals the fork()ed 
child to indicate if it should dump core or not .... but it felt like
huge overkill.

> Even indicating that a core file was written is probably impossible:
> you just have to abort and hope for the best.  The nearest I could
> do is set a flag in the signal handler and have the code it returns
> to print "Attempting to dump core" or some such thing.

I think an "attempting ..." style message would be enough, the 
gdb_demangle_attempt_core_dump flag could be used to indicate
if we've tried to dump core or not.

Thanks,
Andrew
  

Andrew Burgess wrote:
> On 09/06/2014 10:01 AM, Gary Benson wrote:
> > Andrew Burgess wrote:
> > > On 05/06/2014 2:03 PM, Gary Benson wrote:
> > > > 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
> > >
> > > > +
> > > > +/* Signal handler for gdb_demangle.  */
> > > > +
> > > > +static void
> > > > +gdb_demangle_signal_handler (int signo)
> > > > +{
> > > > +  if (gdb_demangle_attempt_core_dump)
> > > > +    {
> > > > +      if (fork () == 0)
> > > > +	dump_core ();
> > >
> > > This worries me a little, when a problem case occurs gdb will
> > > dump core regardless of the users ulimit setting, without first
> > > asking the user, and doesn't tell the user that a core file was
> > > created.
> > >
> > > This feels quite unexpected behaviour to me, especially the bit
> > > about disregarding the ulimit setting without first asking for
> > > permission.
> > >
> > > Catching the crash feels like a good idea, but I'd prefer that
> > > gdb ask before circumventing the ulimit and dumping core.
> > 
> > This part of the same patch:
> > 
> > +  if (core_dump_allowed == -1)
> > +    {
> > +      core_dump_allowed = can_dump_core ();
> > +
> > +      if (!core_dump_allowed)
> > +        gdb_demangle_attempt_core_dump = 0;
> > +    }
> > 
> > calls this:
> > 
> >   int
> >   can_dump_core (void)
> >   {
> >   #ifdef HAVE_GETRLIMIT
> >     struct rlimit rlim;
> >   
> >     /* Be quiet and assume we can dump if an error is returned.  */
> >     if (getrlimit (RLIMIT_CORE, &rlim) != 0)
> >       return 1;
> >   
> >     if (rlim.rlim_max == 0)
> >       return 0;
> >   #endif /* HAVE_GETRLIMIT */
> >   
> >     return 1;
> >   }
> > 		  
> > which inhibits the core dump if the user's ulimit is 0.
> 
> Ahh, yes I see.
> 
> So the problem here is this function is geared towards the /old/ use
> of the function where we are about to ask the user if we should dump
> core.  For that, this function was correct, we check the hard limit
> of the resource.  If the hard limit is high then we ask the user,
> and dump core.
> 
> However, in doing so we circumvent the soft limit rlim.rlim_cur.  So
> I think my point still stands.  The user has said "no core files
> please", and we create one without asking.  If we must go down this
> road then I think we need two functions to check the two different
> limits.

Ah, I didn't realize the code in dump_core was to override the user's
soft limit.  I will update the patch.

> > > Alternatively we could just not dump core from gdb, report the
> > > bad symbol and let the user file a bug.  With the demangler
> > > being so deterministic it should be possible to reproduce, if
> > > not, then we just ask the user to turn off the crash catch,
> > > adjust their ulimit (like we would with any other gdb SEGV
> > > crash), and rerun the test.
> > 
> > That was and is my preferred solution, but Mark Kettenis indicated
> > that he would not accept the patch unless a meaningful core file
> > was created.
> 
> I don't understand that position, but I'd hope he'd agree that we
> should respect the user ulimit over creating a core file...

Yes, this seems reasonable.

> > > If we really want to create the core file by default, but aren't
> > > going to ask, then I'd propose we honour the ulimit setting, and
> > > make sure that the user is told that a core file was just written.
> > 
> > The problem with asking is that you'd have to ask within the signal
> > handler, and no code that prints to the screen is safe to call from
> > within a signal handler.
> 
> Indeed.  I did wonder about some horrible synchronisation scheme
> where the "master" gdb process queries the user then signals the
> fork()ed child to indicate if it should dump core or not .... but
> it felt like huge overkill.

Yeah, I thought down this road too :)

> > Even indicating that a core file was written is probably
> > impossible: you just have to abort and hope for the best.
> > The nearest I could do is set a flag in the signal handler
> > and have the code it returns to print "Attempting to dump
> > core" or some such thing.
> 
> I think an "attempting ..." style message would be enough, the 
> gdb_demangle_attempt_core_dump flag could be used to indicate
> if we've tried to dump core or not.

I will add this to the updated patch.

Thanks,
Gary