RFA: fix handling of catch signal SIGTRAP/SIGINT

RFA: fix handling of catch signal SIGTRAP/SIGINT

[Philippe Waroquiers]

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.

Ok to apply ?

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;
       /* 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);
 }
 
 /* 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.
+
 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 */
 }
 
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
+	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.*" {
+			pass "$test"
+		    }
+		}
+	    }
+	}
+	gdb_test "catch signal SIGINT" "Catchpoint .*"
+	gdb_test "continue" "Catchpoint .*"
+        
     }
 }
 

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

[Pedro Alves]

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

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

[Philippe Waroquiers]

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

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

[Pedro Alves]

On 05/02/2013 10:49 PM, Philippe Waroquiers wrote:
>>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.

Indeed.  Makes sense.  I should have checked the manual.

>>From my point of view, the behaviour described by the doc is quite
> ok (but needs this patch :).

I agree.

> 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'.

Agreed.

>> 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.

That's good info.

> Thanks for the detailed review. Waiting for more feedback from Tromey,
> I will already prepare another version.

Knowing the "catch signal" vs "catch signal all" difference, the updated
patch becomes obviously correct then.  If you had sent it, I'd
probably okay it.  ;-)

Thanks,
-- 
Pedro Alves

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

[Philippe Waroquiers]

On Fri, 2013-05-03 at 15:24 +0100, Pedro Alves wrote:
> Knowing the "catch signal" vs "catch signal all" difference, the updated
> patch becomes obviously correct then.  If you had sent it, I'd
> probably okay it.  ;-)
Here is an updated version.

Philippe


Index: gdb/ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/ChangeLog,v
retrieving revision 1.15497
diff -u -p -r1.15497 ChangeLog
--- gdb/ChangeLog	1 May 2013 14:54:18 -0000	1.15497
+++ gdb/ChangeLog	3 May 2013 17:32:48 -0000
@@ -1,3 +1,9 @@
+2013-05-03  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
+
+	* break-catch-sig.c (signal_catchpoint_breakpoint_hit): Do not
+	ignore SIGINT and SIGTRAP in case these internal signals are
+	caught explicitely.
+
 2013-05-01  Joel Brobecker  <brobecker@adacore.com>
 
 	* darwin-nat.c (darwin_read_write_inferior): Change types
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	3 May 2013 17:32:48 -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;
       /* 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);
 }
 
 /* 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.3641
diff -u -p -r1.3641 ChangeLog
--- gdb/testsuite/ChangeLog	3 May 2013 15:43:57 -0000	1.3641
+++ gdb/testsuite/ChangeLog	3 May 2013 17:32:53 -0000
@@ -1,3 +1,8 @@
+2013-05-03  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
+
+	* gdb.base/catch-sig.c (main): Raise SIGINT.
+	* gdb.base/catch-sig.exp: Test "catch signal SIGINT".
+
 2013-05-03  Hafiz Abid Qadeer  <abidh@codesourcery.com>
 
 	* status-stop.exp (test_tstart_tstart): Check for error
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	3 May 2013 17:32:53 -0000
@@ -42,5 +42,7 @@ main ()
   raise (SIGHUP);		/* third HUP */
 
   raise (SIGHUP);		/* fourth HUP */
+
+  raise (SIGINT);		/* first INT */
 }
 
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	3 May 2013 17:32:53 -0000
@@ -71,6 +71,19 @@ proc test_catch_signal {signame} {
 	gdb_breakpoint ${srcfile}:[gdb_get_line_number "fourth HUP"]
 	gdb_continue_to_breakpoint "fourth HUP"
 	delete_breakpoints
+
+	# Verify an internal signal used by gdb is properly caught.
+	gdb_breakpoint ${srcfile}:[gdb_get_line_number "first INT"]
+	gdb_continue_to_breakpoint "first INT"
+	set test "override SIGINT to catch"
+	gdb_test "handle SIGINT nostop print nopass" \
+	    "SIGINT.*No.*Yes.*No.*" \
+	    "$test" \
+	    "SIGINT is used by the debugger.*Are you sure you want to change it.*y or n.*" \
+	    y
+	gdb_test "catch signal SIGINT" "Catchpoint .*"
+	gdb_test "continue" "Catchpoint .* SIGINT.*"
+        
     }
 }
 

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

[Pedro Alves]

On 05/03/2013 06:39 PM, Philippe Waroquiers wrote:
> On Fri, 2013-05-03 at 15:24 +0100, Pedro Alves wrote:
>> Knowing the "catch signal" vs "catch signal all" difference, the updated
>> patch becomes obviously correct then.  If you had sent it, I'd
>> probably okay it.  ;-)
> Here is an updated version.

OK, thanks.

-- 
Pedro Alves

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

[Philippe Waroquiers]

On Fri, 2013-05-03 at 19:13 +0100, Pedro Alves wrote:
> On 05/03/2013 06:39 PM, Philippe Waroquiers wrote:
> > On Fri, 2013-05-03 at 15:24 +0100, Pedro Alves wrote:
> >> Knowing the "catch signal" vs "catch signal all" difference, the updated
> >> patch becomes obviously correct then.  If you had sent it, I'd
> >> probably okay it.  ;-)
> > Here is an updated version.
> 
> OK, thanks.
Thanks for the review, committed.

Philippe