From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 14625 invoked by alias); 8 Sep 2007 11:25:08 -0000 Received: (qmail 14571 invoked by uid 22791); 8 Sep 2007 11:25:07 -0000 X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (65.74.133.4) by sourceware.org (qpsmtpd/0.31) with ESMTP; Sat, 08 Sep 2007 11:24:57 +0000 Received: (qmail 32265 invoked from network); 8 Sep 2007 11:24:55 -0000 Received: from unknown (HELO h38.net64.aknet.ru) (vladimir@127.0.0.2) by mail.codesourcery.com with ESMTPA; 8 Sep 2007 11:24:55 -0000 From: Vladimir Prus To: Eli Zaretskii Subject: Re: [4/9] associate bpstat with location Date: Sat, 08 Sep 2007 11:25:00 -0000 User-Agent: KMail/1.9.6 Cc: gdb-patches@sources.redhat.com References: <200709080018.25052.vladimir@codesourcery.com> In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-15" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200709081524.48816.vladimir@codesourcery.com> 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: 2007-09/txt/msg00107.txt.bz2 On Saturday 08 September 2007 15:07:08 Eli Zaretskii wrote: > > From: Vladimir Prus > > Date: Sat, 8 Sep 2007 00:18:24 +0400 > > > > (bpstat_find_breakpoint): Look at bpstat's location's > > owner, not at bpstat->breakpoint_at. > > (bpstat_find_step_resume_breakpoint): Likewise. > > (bpstat_num): Likewise. > > (print_it_typical): Likewise. > > (print_bp_stop_message): Likewise. > > (watchpoint_check): Likewise. > > (bpstat_what): Likewise. > > (bpstat_get_triggered_catchpoints): Likewise. > > (breakpoint_auto_delete): Likewise. > > (delete_breakpoint): Likewise. > > A minor stylistic point: could we please avoid the annoying > "Likewise"s? The canonical way of writing a ChangeLog entry for > several functions with an identical change is this: > > (bpstat_find_breakpoint, bpstat_find_step_resume_breakpoint) > (bpstat_num, print_it_typical): Look at bpstat's location's > owner, not at bpstat->breakpoint_at. > > I'm quite sure the GNU coding standards describe this. (Yes, I know > that our ChangeLog's abuse "Likewise" too much.) I don't have an opinion here; I don't think this has any practical difference to future readers of ChangeLog. > > - b = (*bsp)->breakpoint_at; > > + /* We assume we'll never have several bpstats that > > + correspond to a single breakpoint -- otherwise, > > + this function might return the same number more > > + than once and this will look ugly. */ > > + b = (*bsp)->breakpoint_at ? (*bsp)->breakpoint_at->owner : NULL; > > Is the assumption in the comment above really valid? I happen to put > several breakpoints on the same line quite a lot (each breakpoint has > a different condition and/or different commands list). The assumption is about different case -- that you'll never had two bpstats that both correspond to a single breakpoint. Having two breakpoints that correspond to single line of code is different, and is not a problem. > Can we do better, even if it requires to try harder? The reason why the assumption is valid is because the only way to have several bpstats refer to one breakpoint is when breakpoint has two locations, and both locations have the same address. That makes no sense -- there's no per-location data that can make those locations different in behaviour, and so having two locations with same address would be a bug. > > case bp_access_watchpoint: > > if (bs->old_val != NULL) > > { > > - annotate_watchpoint (bs->breakpoint_at->number); > > + annotate_watchpoint (b->number); > > Watchpoints also? Did you make corresponding changes in the code that > sets watchpoints? No. This patch is not supposed to have any change in behaviour whatsoever, it merely moves a data member. For watchpoints, you need extra indirection to get from bpstat to breakpoint number. That indirection is useful for code breakpoints, in future patches, and has no effect on watchpoints. - Volodya