Re: [PATCH 0/2 v3] Demangler crash handler

[Andrew Burgess <aburgess@broadcom.com>]

On 04/06/2014 2:34 PM, Gary Benson wrote:
> Mark Kettenis wrote:
>>> From: Gary Benson <gbenson@redhat.com>
>>> Is this ok to commit?
>>
>> Not for me.  You're running too much code between entering the
>> SIGSEGV handler and dumping core for my taste.
> 
> I don't understand.  This is the signal handler:
> 
>   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);
>   }
> 
> and this is dump_core:
> 
>   void
>   dump_core (void)
>   {
>   #ifdef HAVE_SETRLIMIT
>     struct rlimit rlim = { RLIM_INFINITY, RLIM_INFINITY };
>   
>     setrlimit (RLIMIT_CORE, &rlim);
>   #endif /* HAVE_SETRLIMIT */
>   
>     abort ();	/* NOTE: GDB has only three calls to abort().  */
>   }
> 
> This is what happens:
> 
>   1. the signal handler is entered
>   2. fork
>   3. setrlimit
>   4. abort
> 
> I could remove the setrlimit but presumably somebody added that to
> make a successful core dump more likely.
> 
> Would you accept this patch if I changed the dump_core to abort
> in gdb_demangle_signal_handler?  (and updated all the comments
> about having only three calls to abort)?
> 
>> I still very much agree with Pedro; this should not be necessary.
> 
> "Should" is the operative word here.  It *should* not be necessary
> because the demangler *should* never crash.  But this isn't utopia.
> The demangler is code, and code has bugs.  People make mistakes.
> Things are valid now that may not be valid in the future.  And GDB
> should not just crash if some symbol in the inferior isn't handled
> or doesn't make sense or whatever.

By this logic should / would we not extend the SIGSEGV handler to cover
all gdb code?  If the target is running in synchronous mode we'd
install our SEGV handler when the target stops and remove it when the
target restarts (asynchronous mode would need more thought), then any
bugs in gdb that cause a SEGV would result in a core dump ...

I'm just not sure why the demangler should get special treatment.

Thanks,
Andrew