Re: [RFA] Implement support for PowerPC BookE ranged breakpoints

[Thiago Jung Bauermann <bauerman@br.ibm.com>]

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