From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31216 invoked by alias); 27 Jul 2010 21:22:54 -0000 Received: (qmail 31204 invoked by uid 22791); 27 Jul 2010 21:22:53 -0000 X-SWARE-Spam-Status: No, hits=-6.0 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 27 Jul 2010 21:22:44 +0000 Received: from int-mx05.intmail.prod.int.phx2.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.18]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o6RLMaCR007178 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Tue, 27 Jul 2010 17:22:37 -0400 Received: from host1.dyn.jankratochvil.net (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx05.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id o6RLMYNo003656 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Tue, 27 Jul 2010 17:22:36 -0400 Received: from host1.dyn.jankratochvil.net (localhost [127.0.0.1]) by host1.dyn.jankratochvil.net (8.14.4/8.14.4) with ESMTP id o6RLMYpq003138; Tue, 27 Jul 2010 23:22:34 +0200 Received: (from jkratoch@localhost) by host1.dyn.jankratochvil.net (8.14.4/8.14.4/Submit) id o6RLMXAH003137; Tue, 27 Jul 2010 23:22:33 +0200 Date: Tue, 27 Jul 2010 21:22:00 -0000 From: Jan Kratochvil To: Pedro Alves Cc: gdb-patches@sourceware.org Subject: Re: [patch] Fix linux-ia64 on SIGILL for deleted breakpoint [cleanup] Message-ID: <20100727212233.GA676@host1.dyn.jankratochvil.net> References: <20100719085817.GA24395@host1.dyn.jankratochvil.net> <20100725185217.GA24476@host1.dyn.jankratochvil.net> <20100725185512.GA29794@host1.dyn.jankratochvil.net> <201007271259.19225.pedro@codesourcery.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <201007271259.19225.pedro@codesourcery.com> User-Agent: Mutt/1.5.20 (2009-12-10) X-IsSubscribed: yes 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 X-SW-Source: 2010-07/txt/msg00449.txt.bz2 On Tue, 27 Jul 2010 13:59:18 +0200, Pedro Alves wrote: > > +static int > > +linux_nat_lp_status_is_event (struct lwp_info *lp) > > Static helper functions that aren't a part of the target_ops > interface (as opposed to linux_nat_resume, say), or external, > don't usually take the "linux_nat_" prefix, so you could shorten > the function name. There is a problem that linux_nat_set_status_is_event is not static and therefore it is correctly ^linux_nat_ prefixed. I thought all the other related functions should have matching name, which assigns them the incorrect ^linux_nat_ prefix. > I'm not particularly fond of the function names. The reason is > that: > > - linux_nat_status_is_event > > Returns true for breakpoint events, and other SIGTRAP events. > Returns false for other kinds of events (random signals), though > we could call these events too. > > - linux_nat_lp_status_is_event > > Returns true for breakpoint events, and other SIGTRAP events > Returns false for other kinds of events (random signals), and > fork/exec/clone/exit events, though we could call these events too. > > Note the subtle differences. Yes... the problem is the variables already are not systematic. There should be that lp->status_p flag discussed in the comments, TARGET_SIGNAL_TRAP is incorrectly used as an abstraction of inferior event (even for SIGILL/SIGSEGV/etc.), lp->signalled should be called lp->sigstop_sent, lp->resumed should be called lp->want_to_resume, target_signal should be called gdb_signal, host signal should be called target signal, lp->waitstatus.kind TARGET_WAITKIND_* I cannot describe right offhand now. I have to admit I mostly gave up on building systematic functions and naming on top of it. > This appears to be changing behaviour (the new waitstatus check within > the new function), but I doubt it makes a difference. I'm pointing > it out, because you're presenting the patch as cleanup. Sorry for the naming, I did not indend to consider this patch as a no functionality change patch. (Although it should not be a user visible change.) Checked-in. Thanks, Jan http://sourceware.org/ml/gdb-cvs/2010-07/msg00171.html --- src/gdb/ChangeLog 2010/07/27 20:51:37 1.12026 +++ src/gdb/ChangeLog 2010/07/27 21:22:07 1.12027 @@ -1,5 +1,11 @@ 2010-07-27 Jan Kratochvil + * linux-nat.c (linux_nat_lp_status_is_event): New function. + (count_events_callback, select_event_lwp_callback) + (cancel_breakpoints_callback, linux_nat_wait_1): Use it. + +2010-07-27 Jan Kratochvil + * ia64-linux-nat.c (ia64_linux_status_is_event): New function. (_initialize_ia64_linux_nat): Install it. * linux-nat.c (sigtrap_is_event, linux_nat_status_is_event) --- src/gdb/linux-nat.c 2010/07/27 20:51:37 1.179 +++ src/gdb/linux-nat.c 2010/07/27 21:22:09 1.180 @@ -2617,6 +2617,20 @@ static int (*linux_nat_status_is_event) (int status) = sigtrap_is_event; +/* Check for SIGTRAP-like events in LP. */ + +static int +linux_nat_lp_status_is_event (struct lwp_info *lp) +{ + /* We check for lp->waitstatus in addition to lp->status, because we can + have pending process exits recorded in lp->status + and W_EXITCODE(0,0) == 0. We should probably have an additional + lp->status_p flag. */ + + return (lp->waitstatus.kind == TARGET_WAITKIND_IGNORE + && linux_nat_status_is_event (lp->status)); +} + /* Set alternative SIGTRAP-like events recognizer. If breakpoint_inserted_here_p there then gdbarch_decr_pc_after_break will be applied. */ @@ -2823,8 +2837,7 @@ gdb_assert (count != NULL); /* Count only resumed LWPs that have a SIGTRAP event pending. */ - if (lp->status != 0 && lp->resumed - && linux_nat_status_is_event (lp->status)) + if (lp->resumed && linux_nat_lp_status_is_event (lp)) (*count)++; return 0; @@ -2851,8 +2864,7 @@ gdb_assert (selector != NULL); /* Select only resumed LWPs that have a SIGTRAP event pending. */ - if (lp->status != 0 && lp->resumed - && linux_nat_status_is_event (lp->status)) + if (lp->resumed && linux_nat_lp_status_is_event (lp)) if ((*selector)-- == 0) return 1; @@ -2912,9 +2924,7 @@ delete or disable the breakpoint, but the LWP will have already tripped on it. */ - if (lp->waitstatus.kind == TARGET_WAITKIND_IGNORE - && lp->status != 0 - && linux_nat_status_is_event (lp->status) + if (linux_nat_lp_status_is_event (lp) && cancel_breakpoint (lp)) /* Throw away the SIGTRAP. */ lp->status = 0; @@ -3433,8 +3443,7 @@ always cancels breakpoint hits in all threads. */ if (non_stop - && lp->waitstatus.kind == TARGET_WAITKIND_IGNORE - && linux_nat_status_is_event (lp->status) + && linux_nat_lp_status_is_event (lp) && cancel_breakpoint (lp)) { /* Throw away the SIGTRAP. */