From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 526 invoked by alias); 27 Jul 2010 11:59:32 -0000 Received: (qmail 500 invoked by uid 22791); 27 Jul 2010 11:59:29 -0000 X-SWARE-Spam-Status: No, hits=-2.0 required=5.0 tests=AWL,BAYES_00,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (38.113.113.100) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 27 Jul 2010 11:59:23 +0000 Received: (qmail 9790 invoked from network); 27 Jul 2010 11:59:21 -0000 Received: from unknown (HELO orlando.localnet) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 27 Jul 2010 11:59:21 -0000 From: Pedro Alves To: Jan Kratochvil Subject: Re: [patch] Fix linux-ia64 on SIGILL for deleted breakpoint [cleanup] Date: Tue, 27 Jul 2010 11:59:00 -0000 User-Agent: KMail/1.13.2 (Linux/2.6.32-24-generic; KDE/4.4.2; x86_64; ; ) Cc: gdb-patches@sourceware.org References: <20100719085817.GA24395@host1.dyn.jankratochvil.net> <20100725185217.GA24476@host1.dyn.jankratochvil.net> <20100725185512.GA29794@host1.dyn.jankratochvil.net> In-Reply-To: <20100725185512.GA29794@host1.dyn.jankratochvil.net> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201007271259.19225.pedro@codesourcery.com> 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/msg00417.txt.bz2 On Sunday 25 July 2010 19:55:12, Jan Kratochvil wrote: > gdb/ > 2010-07-25 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. Looks okay. A few comments below, but I'm feeling passive today, and I'll let this go in unmodified. > +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. 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. > +{ > + /* 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 altarnative SIGTRAP-like events recognizer. If > breakpoint_inserted_here_p there then gdbarch_decr_pc_after_break will be > applied. */ > @@ -2823,8 +2837,7 @@ count_events_callback (struct lwp_info *lp, void *data) > 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)++; 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. I actually wonder why we only select SIGTRAP-like events instead of all reportable events (as opposed to our own SIGSTOPs). > > return 0; > @@ -2851,8 +2864,7 @@ select_event_lwp_callback (struct lwp_info *lp, void *data) > 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; (Same comment here.) > > @@ -2912,9 +2924,7 @@ cancel_breakpoints_callback (struct lwp_info *lp, void *data) > 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 @@ retry: > 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. */ > -- Pedro Alves