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

Gary Benson
2014-06-09T11:48:54Z

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

--