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

[Pedro Alves <palves at redhat dot com>]

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.

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.

> Ok to apply ?

I'd like to hear Tromey's input.

> 
> Index: gdb/ChangeLog
> ===================================================================
> RCS file: /cvs/src/src/gdb/ChangeLog,v
> retrieving revision 1.15494
> diff -u -p -r1.15494 ChangeLog
> --- gdb/ChangeLog	30 Apr 2013 23:19:41 -0000	1.15494
> +++ gdb/ChangeLog	1 May 2013 13:50:36 -0000
> @@ -1,3 +1,9 @@
> +2013-05-01  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
> +
> +	* break-catch-sig.c (signal_catchpoint_breakpoint_hit): do not
> +	ignore SIGINT and SIGTRAP in case these internals signals are
> +	catched explicitely.
> +
>  2013-04-30  Doug Evans  <dje@google.com>
>  
>  	* dwarf2read.c (lookup_dwo_unit): Return NULL if DWO not found.
> Index: gdb/break-catch-sig.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/break-catch-sig.c,v
> retrieving revision 1.3
> diff -u -p -r1.3 break-catch-sig.c
> --- gdb/break-catch-sig.c	12 Feb 2013 18:27:27 -0000	1.3
> +++ gdb/break-catch-sig.c	1 May 2013 13:50:36 -0000
> @@ -199,13 +199,13 @@ signal_catchpoint_breakpoint_hit (const 
>             VEC_iterate (gdb_signal_type, c->signals_to_be_caught, i, iter);
>             i++)
>  	if (signal_number == iter)
> -	  break;
> +	  return 1;

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?

There are other uses of INTERNAL_SIGNAL(signal_number) in the file.
Wouldn't they need updating too?

>  }
>  
>  /* Implement the "print_it" breakpoint_ops method for signal
> Index: gdb/testsuite/ChangeLog
> ===================================================================
> RCS file: /cvs/src/src/gdb/testsuite/ChangeLog,v
> retrieving revision 1.3640
> diff -u -p -r1.3640 ChangeLog
> --- gdb/testsuite/ChangeLog	30 Apr 2013 12:33:51 -0000	1.3640
> +++ gdb/testsuite/ChangeLog	1 May 2013 13:50:39 -0000
> @@ -1,3 +1,8 @@
> +2013-05-01  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
> +
> +	* gdb.base/catch-sig.c (main): add raise SIGINT.
> +	* gdb.base/catch-sig.exp: test catch signal SIGINT.

These are supposed to be full sentences, so start them with Uppercase.
Write as:

	* gdb.base/catch-sig.c (main): Raise SIGINT.
	* gdb.base/catch-sig.exp: Test "catch signal SIGINT".

> +
>  2013-03-27  Walfred Tedeschi  <walfred.tedeschi@intel.com>
>  
>  	* gdb.xml/maint_print_struct.exp: New file.
> Index: gdb/testsuite/gdb.base/catch-signal.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/testsuite/gdb.base/catch-signal.c,v
> retrieving revision 1.2
> diff -u -p -r1.2 catch-signal.c
> --- gdb/testsuite/gdb.base/catch-signal.c	12 Feb 2013 18:27:28 -0000	1.2
> +++ gdb/testsuite/gdb.base/catch-signal.c	1 May 2013 13:50:39 -0000
> @@ -42,5 +42,7 @@ main ()
>    raise (SIGHUP);		/* third HUP */
>  
>    raise (SIGHUP);		/* fourth HUP */
> +
> +  raise (SIGINT);               /* first INT */

The other lines seem to align due to use of tabs.  Your new
line doesn't seem to use tabs.

>  }
>  
> Index: gdb/testsuite/gdb.base/catch-signal.exp
> ===================================================================
> RCS file: /cvs/src/src/gdb/testsuite/gdb.base/catch-signal.exp,v
> retrieving revision 1.3
> diff -u -p -r1.3 catch-signal.exp
> --- gdb/testsuite/gdb.base/catch-signal.exp	12 Feb 2013 18:27:28 -0000	1.3
> +++ gdb/testsuite/gdb.base/catch-signal.exp	1 May 2013 13:50:39 -0000
> @@ -71,6 +71,23 @@ proc test_catch_signal {signame} {
>  	gdb_breakpoint ${srcfile}:[gdb_get_line_number "fourth HUP"]
>  	gdb_continue_to_breakpoint "fourth HUP"
>  	delete_breakpoints
> +
> +	# Verify an signal used by gdb is properly catched

"a signal", or perhaps better "an internal signal".  "caught".

> +	gdb_breakpoint ${srcfile}:[gdb_get_line_number "first INT"]
> +	gdb_continue_to_breakpoint "first INT"
> +	set test "override SIGINT to catch"
> +	gdb_test_multiple "handle SIGINT nostop print nopass" "$test" {
> +	    -re "SIGINT is used by the debugger.*Are you sure you want to change it.*y or n.*" {
> +		gdb_test_multiple "y" "$test" {
> +		    -re 	    "SIGINT.*No.*Yes.*No.*" {

Something odd with the spaces after -re here, but ...

> +			pass "$test"
> +		    }
> +		}
> +	    }
> +	}

	... you can use a single gdb_test that handles the question.  See its
description in gdb.exp of the QUESTION/RESPONSE parameters.

> +	gdb_test "catch signal SIGINT" "Catchpoint .*"
> +	gdb_test "continue" "Catchpoint .*"

This is catching internal signals.  I think it'd be wise
to make the regex a little bit more script, by having it
expect the "signal SIGINT" part too, lest gdb grows a bug in
the future that would make the test catch a SIGTRAP instead.

-- 
Pedro Alves