From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22453 invoked by alias); 2 May 2013 21:49:36 -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 22427 invoked by uid 89); 2 May 2013 21:49:30 -0000 X-Spam-SWARE-Status: No, score=-3.2 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.1 Received: from mailrelay011.isp.belgacom.be (HELO mailrelay011.isp.belgacom.be) (195.238.6.178) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Thu, 02 May 2013 21:49:27 +0000 X-Belgacom-Dynamic: yes X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: AjgDAI/eglFtgZKL/2dsb2JhbAANRYZ1uQuCYgMBgRWDEwEBAQMBIwRSBQsLDgoCAiYCAlcGiBmvWHKQfoEjjSRfB4JAgRMDq2w Received: from 139.146-129-109.adsl-dyn.isp.belgacom.be (HELO [192.168.1.4]) ([109.129.146.139]) by relay.skynet.be with ESMTP; 02 May 2013 23:49:25 +0200 Subject: Re: RFA: fix handling of catch signal SIGTRAP/SIGINT From: Philippe Waroquiers To: Pedro Alves Cc: gdb-patches@sourceware.org In-Reply-To: <5182B441.3000703@redhat.com> References: <1367433782.2626.142.camel@soleil> <5182B441.3000703@redhat.com> Content-Type: text/plain; charset="UTF-8" Date: Thu, 02 May 2013 21:49:00 -0000 Message-ID: <1367531384.3007.83.camel@soleil> Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-SW-Source: 2013-05/txt/msg00041.txt.bz2 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