This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFA] Implement support for PowerPC BookE ranged breakpoints
- From: Thiago Jung Bauermann <bauerman at br dot ibm dot com>
- To: Eli Zaretskii <eliz at gnu dot org>
- Cc: gdb-patches at sourceware dot org
- Date: Mon, 31 Jan 2011 16:59:04 -0200
- Subject: Re: [RFA] Implement support for PowerPC BookE ranged breakpoints
- References: <1296177985.2843.82.camel@hactar> <83bp31idgm.fsf@gnu.org>
On Fri, 2011-01-28 at 11:28 +0200, Eli Zaretskii wrote:
> > From: Thiago Jung Bauermann <bauerman@br.ibm.com>
> > Date: Thu, 27 Jan 2011 23:26:25 -0200
> Thanks.
Thanks for the review.
> > +* When locally debugging programs on PowerPC BookE processors running
> ^^^^^^^
> You mean "natively", right? I don't think we use "local debugging"
> elsewhere in our documentation, do we?
Oops. Fixed.
> > + a Linux kernel version 2.6.34 or later, GDB supports ranged breakpoints,
> > + which stop execution of the inferior whenever it executes any address
> > + within the specified range. See the "PowerPC Embedded" section in the
>
> "whenever it executes an instruction at any address within the
> specified range". (You cannot "execute" an address.)
It was an analogy with "read an address" and "write an address", but I
guess even those expressions are not that good. :-) Fixed.
> > + if (b->disposition == disp_del)
> > + printf_filtered (_("Temporary ranged breakpoint"));
> > + else
> > + printf_filtered (_("Ranged breakpoint"));
> > + printf_filtered (_(" %d"), b->number);
>
> This snippet violates one of the rules of translation-ready software:
> don't construct phrases from parts. I suggest to make " %d" part of
> each of the two possibilities.
Agreed. Fixed.
> > +static void
> > +print_recreate_ranged_breakpoint (struct breakpoint *b, struct ui_file *fp)
> > +{
> > + fprintf_unfiltered (fp, "break-range %s", b->exp_string);
>
> Should this string be in _() ?
No. This function is called by the "save breakpoints" command an is
expected to write to fp a GDB command which will recreate the
breakpoint.
> > + sals_start.sals = (struct symtab_and_line *)
> > + xmalloc (sizeof (struct symtab_and_line));
> > + sals_start.nelts = 1;
>
> Spaces and TABs mixup alert!
Hum, I couldn't find the mixup. the lines above and below xmalloc need
to use spaces because they start at column 7. The xmalloc line uses TAB
since it starts at column 9 (and it doesn't have any spaces only one
TAB).
> > +The breakpoint will stop execution of the inferior whenever it\n\
> > +executes any address within the [start-address, end-address] range\n\
> ^^^^^^^^^^^^^^^^^^^^
> Same correction as in NEWS.
Fixed.
> > +PowerPC embedded processors support hardware accelerated ranged breakpoints.
> > +A @dfn{ranged breakpoint} stops execution of the inferior whenever it
>
> Use @dfn when you first introduce a term, in this case in the previous
> sentence.
Ok.
> Also, please add here a @cindex entry about "ranged breakpoint".
Done.
> > +executes any address within the range it specifies. To set a ranged
> ^^^^^^^^^^^^^^^^^^^^
> Same correction as in NEWS.
Fixed.
> > +Set a breakpoint for an address range.
> > +@var{start-location} and @var{end-location} can specify a function name,
> > +a line number, an offset of lines from the current line or from the start
> > +location, or an address of an instruction (@xref{Specify Location},
>
> You want a "see @ref here", not @xref. The latter will produce "See"
> with a capital S, which will look like a typo. In general, @xref is
> supposed to use only at the beginning of a sentence.
Fixed.
> > +The breakpoint will stop execution of the inferior whenever it
> > +executes any address within the specified range, (including
> ^^^^^^^^^^^^^^^^^^^^
> Same correction as above.
Fixed.
> The patch for NEWS and the manual is okay with those changes.
Great! The changes will show up next time I post the patch, after the
code review.
--
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center