This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [4/9] associate bpstat with location
On Saturday 08 September 2007 15:07:08 Eli Zaretskii wrote:
> > From: Vladimir Prus <vladimir@codesourcery.com>
> > 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