Re: [PATCH 3/3 v4] Demangler crash handler

[Andrew Burgess <aburgess at broadcom dot com>]

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