Re: RFA: fix handling of catch signal SIGTRAP/SIGINT

[Philippe Waroquiers <philippe.waroquiers@skynet.be>]

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