From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16143 invoked by alias); 2 May 2013 18:45:44 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 16109 invoked by uid 89); 2 May 2013 18:45:37 -0000 X-Spam-SWARE-Status: No, score=-9.0 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.1 Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Thu, 02 May 2013 18:45:28 +0000 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r42IjOpH025512 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 2 May 2013 14:45:25 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r42IjMqr006532; Thu, 2 May 2013 14:45:23 -0400 Message-ID: <5182B441.3000703@redhat.com> Date: Thu, 02 May 2013 18:45:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130311 Thunderbird/17.0.4 MIME-Version: 1.0 To: Philippe Waroquiers CC: gdb-patches@sourceware.org Subject: Re: RFA: fix handling of catch signal SIGTRAP/SIGINT References: <1367433782.2626.142.camel@soleil> In-Reply-To: <1367433782.2626.142.camel@soleil> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2013-05/txt/msg00040.txt.bz2 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 > + > + * 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 > > * 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 > + > + * 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 > > * 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