This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: RFA: fix handling of catch signal SIGTRAP/SIGINT
- From: Philippe Waroquiers <philippe dot waroquiers at skynet dot be>
- To: Pedro Alves <palves at redhat dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Thu, 02 May 2013 23:49:44 +0200
- Subject: Re: RFA: fix handling of catch signal SIGTRAP/SIGINT
- References: <1367433782 dot 2626 dot 142 dot camel at soleil> <5182B441 dot 3000703 at redhat dot com>
On Thu, 2013-05-02 at 19:45 +0100, Pedro Alves wrote:
> On 05/01/2013 07:43 PM, Philippe Waroquiers wrote:
> > catch signal SIGTRAP/SIGINT is not working when the signal
> > is catched specifically with 'catch signal SIGTRAP'.
> >
> > This is because the function signal_catchpoint_breakpoint_hit
> > still checks !INTERNAL_SIGNAL (signal_number) even
> > when the signal_number is member of c->signals_to_be_caught
> >
> > The attached patch fixes this, and modifies gdb.base/catch-signal.exp
> > to test that SIGINT (one of the two internal signals) is properly
> > catched.
>
> Hmm, this seems to have been done on purpose. The patch submission
> description mentioned:
>
> "I chose to have "catch signal" ignore signals that are used internally
> by gdb. Instead, users can use "catch signal all" to catch even those.
> I think this is a more useful default."
>
> And that's indeed what the line:
>
> return c->catch_all || !INTERNAL_SIGNAL (signal_number);
>
> does.
>From the doc and the above, I understand the idea is to have 3 different
"use cases":
1. catch signal
2. catch signal all
3. catch signal ... 1 or more explicit signals ...
(the explicit signals are the same as what can be given to handle).
The line above properly implemented the difference between 1 and 2
but was also used for signals listed in 3. This was ok for not internals
signals, but was always ignoring internal signals member of
signals_to_be_caught.
So, I think the condition "|| !INTERNAL_SIGNAL" is still needed
otherwise the case 1. will change of behaviour.
>
> But I agree with you. "catch signal SIGINT" is explicit, so it's
> surprising that it doesn't work.
>
> In addition, it'd perhaps make sense to instead go the other way
> around and make "catch signal all" _not_ catch "internal" signals.
> Perhaps add a "catch signal internal" so the user wouldn't
> have to know which are "internal". "catch signal all internal"
> would then catch really all. Effectively, do the opposite
> filtering of what we do today. An alternative, could be to leave "all" to
> really mean all, and support "catch signal pass", meaning catch
> signals that are set to pass (SIGTRAP/SIGINT are set to no-pass), etc.
> Maybe add "all-user" for "all minus internal". Lots of options.
> I'm not sure what my preference is.
>From my point of view, the behaviour described by the doc is quite
ok (but needs this patch :).
> this...
> > /* Not the same. */
> > - if (!iter)
> > - return 0;
> > + gdb_assert (!iter);
> > + return 0;
> > }
> > -
> > - return c->catch_all || !INTERNAL_SIGNAL (signal_number);
> > + else
> > + return c->catch_all || !INTERNAL_SIGNAL (signal_number);
>
> ... makes the whole "|| !INTERNAL_SIGNAL (signal_number)" part unnecessary,
> isn't it? IOW, just
>
> return c->catch_all;
>
> would be the same?
As described above, I think the '|| !INTERNAL_SIGNAL' is needed to
only catch non internal signals when 'catch signal' was given by the
user rather than 'catch signal all'.
>
> There are other uses of INTERNAL_SIGNAL(signal_number) in the file.
> Wouldn't they need updating too?
I checked the other uses, I think these are ok e.g. INTERNAL_SIGNAL
is not used when an explicit list of signal is given.
Thanks for the detailed review. Waiting for more feedback from Tromey,
I will already prepare another version.
Philippe