Re: [patch 2/2] Implement support for PowerPC BookE masked and ranged watchpoints

[Eli Zaretskii <eliz at gnu dot org>]

> From: Thiago Jung Bauermann <bauerman@br.ibm.com>
> Date: Sat, 23 Oct 2010 02:22:41 -0200
> 
> This is a new version of the patch.

Thanks.

> It doesn't rely on an old_ops field
> in struct breakpoint. Instead, it makes the breakpoint_ops methods for
> ranged watchpoint accept ranged software watchpoints (this was trivial
> and involved only new cases in some print methods), and adds a
> works_in_software_mode method which is called by update_watchpoint if it
> determines that it needs to downgrade a hardware watchpoint to a
> software watchpoint. In case it can't be done, it'll throw an exception.

In what cases will the software mode be used for ranged watchpoints?
Does this mean that architectures that don't have hardware support for
such watchpoints will be able to use them in software mode?

And what about masked watchpoints -- can they also be used in software
mode?

I'm quite sure this was all mentioned in the discussions leading to
this patch, so if I can read about that elsewhere, please just point
me to the relevant thread.

> +	/* Since we don't know the exact trigger address (from
> +	   stopped_data_address) Just tell the user we've triggered
> +	   a mask watchpoint.  */

"Just" should be "just" (no capitalization).  And a comma before it
would be appropriate, too.

> +  mention (b);
> +  ui_out_text (uiout, "\nCheck the underlying instruction \
> +at PC for address and value related to this watchpoint trigger.\n");

Shouldn't this be in _() ?

> +    case bp_hardware_watchpoint:
> +      ui_out_text (uiout, "Masked hardware watchpoint ");

Isn't it better to say "Masked hardware (write) watchpoint"?

> +The watchpoint will stop execution of your program whenever the inferior\n\
> +writes to any address within the [start-address, end-address] interval."));

The literal difference between "your program" and "the inferior" could
confuse the user.  Suggest the following simplification:

  The watchpoint will stop execution of the inferior whenever it writes
  to any address within the [start-address, end-address] interval.

Also, isn't "range" better than "interval" here?

The same goes for all the other similar commands.

> +  /* Tell how many additional hardware resources (debug registers) are needed
> +     for this breakpoint.  We always count at least one resource.  If this
> +     element is NULL, then no additional resource is accounted for.  */
> +  int (*extra_resources_needed) (const struct breakpoint *);

I think the last sentence of the comment is unclear.  What does it
mean "no resource is accounted for"?  If that's to say "no additional
resources are needed", then why not say that explicitly?

> +  /* Display one line of extra information about this breakpoint,
> +     for "info breakpoints".  */
> +  void (*print_one_detail) (const struct breakpoint *, struct ui_out *);

Examples of such "extras" would be beneficial here.  Also, are there
any limitations on this extra information, like maximum length?

> +The @code{@r{[}mask @var{maskvalue}@r{]}} clause is used to create a masked
> +watchpoint if the current architecture supports the feature (currently,
> +only available in the PowerPC Embedded architecture.
> +See @ref{PowerPC Embedded}).

First, using "clause" here is suboptimal.  I would suggest "argument"
instead.  Second, the part in the parentheses is wrong: you cannot
put several sentences in a parenthesis that itself is part of a
sentence.  And third, you need a period after @ref{}, or else makeinfo
will bitch at you.  How about the following rewording (which also
avoids using the passive tense)?

  The @code{@r{[}mask @var{maskvalue}@r{]}} argument allows creation
  of masked watchpoints, if the current architecture supports this
  feature.  (Currently, this is only available on PowerPC Embedded
  architecture, see @ref{PowerPC Embedded}.)

> +PowerPC embedded processors support additional types of hardware watchpoints,
> +namely masked watchpoints and ranged watchpoints.

Suggest to reword to make more concise:

  PowerPC embedded processors support masked watchpoints and ranged
  watchpoints.

> +A @dfn{masked watchpoint} is defined by an address and a mask.  It triggers
> +when the address of a memory access matches the watchpoint address when both
> +are masked by the watchpoint mask.  That is, the bits set in the mask determine
> +which bits are relevant in the address comparison.

This description is too code-oriented.  Dopes the following catch what
you wanted to say?

  A @dfn{masked watchpoint} specifies a mask in addition to an address
  to watch.  The mask specifies that some bits of an address (the bits
  which are reset in the mask) should be ignored when matching the
  address accessed by the inferior against the watchpoint address.
  Thus, a masked watchpoint watches many addresses
  simultaneously---those addresses whose unmasked bits are identical
  to the unmasked bits in the watchpoint address.

>                                                  To set a masked watchpoint
> +in @value{GDBN}, use the @code{mask} parameter in the @code{watch} command
                                        ^^^^^^^^^
"argument"

> +(see @ref{Set Watchpoints}), as in:
   ^^^^^^^^^^^^^^^^^^^^^^^^^^^
"(@pxref{Set Watchpoints})"

> +A @dfn{ranged watchpoint} is defined by a start address and an end address
> +specifying a region of memory inside which any access will trigger the
> +watchpoint.  Both the start and end addresses are within the memory region.

"Defined by a start address and an end address" is inaccurate, because
it can also be defined by a start address and length.  But there's no
need to go into details here, since you are about to tell them anyway.
So I would say only this:

  A @dfn{ranged watchpoint} watches a contiguous range of addresses.

> +@item watch-range @var{start-address, +length}

@var{} should be used only for a single symbol.  It's not just a
markup, it also shows where each meta-syntactic variable starts and
ends.  Portions of the test that are not part of the symbol should not
be in @var{}.  So:

  @item watch-range @var{start-address}, +@var{length}
  @itemx watch-range @var{start-address}, @var{end-address}

And the same for the rest.

> +The watchpoint will stop execution of your program whenever the inferior
> +writes, reads, or accesses (respectively for watch-range, awatch-range
> +and rwatch-range) any address within the @code{[start-address, end-address]}
> +interval.         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   ^^^^^^^^
I would say "any address within the specified range".  This is more
concise and also more accurate, since end-address is not the only
possible way of specifying a range watchpoint.

> +	  printf_filtered (_("Empty search range.\n"));

"search range" or "address range"?  The former assumes the user wanted
to search something, but with range watchpoints this is not what the
user wants.

> +	error (_("Search space too large."));

Same here.

> +	error (_("Invalid search space, end preceeds start."));

And here.

> +	error (_("Invalid length."));

"Invalid (negative) length of address range."

> +      /* We don't support searching all of memory
> +	 (i.e. start=0, end = 0xff..ff).
> +	 Bail to avoid overflows later on.  */

"Bail out", I think.

> +  /* The watchpoint will trigger if the address of the memory access is
> +     within the defined range, as follows: p.addr <= address < p.addr2.  */
> +  p.addr            = (uint64_t) addr;
> +  p.addr2           = (uint64_t) addr + len;

But the documentation says that the end address is included, not
excluded.  Can we eliminate this source of confusion?