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

[Pedro Alves <pedro at codesourcery dot com>]

On Saturday 27 November 2010 17:47:38, Pedro Alves wrote:
> On Friday 26 November 2010 21:15:22, Thiago Jung Bauermann wrote:
> 
> > So in your opinion, the watch command should always use two watchpoint
> > registers to set up a ranged watchpoint in BookE ppc? I'm a bit
> > reluctant to use all the watchpoint registers to set up one
> > watchpoint...
> 
> Blame the hardware designers, not me.  :-)
> 
> > Then comes the question of how to support the PPC_BREAKPOINT_MODE_EXACT
> > behaviour... A new flag or command to let the user "opt in" to a less
> > capable watchpoint, but one which uses less hardware resources?
> 
> Yes, that's what I think we should do.  I can help you code this,
> but I'm going computer-less real soon now until the 3rd December,
> and I'll only be able to help/review changes then onwards.
> 
> Here's what I think we should do in more detail:
> 
>  1 - make ppc-linux-nat.c insert ranged watchpoints for len > 1.
> 
>  2 - make ppc-linux-nat.c insert exact watchpoints when len == 1.

BTW, optional, but something that the ppc watchpoint support is
lacking.  Step 2.5, ppc-linux-nat.c should be taught to keep a list
of watchpoint requests, and merge watch requests for the same
addresses.  See <http://sourceware.org/ml/gdb-patches/2010-09/msg00470.html>
for an example of what that would fix.

> 
> with the above, watchpoints will behave as core gdb wants,
> while we will still use only one register in the case that
> is possible.
> 
>  3 - add a new command to let the user select that gdb should
>      trust that primitive types are always accessed through their
>      address (== exact).
>      My opinion is that it should be off by default, on the
>      grounds that most users aren't and shouldn't be that
>      familiar with the debug support of the chip or target
>      they're using.  Users want things to Just Work.
>      Every case that gdb doesn't behave the way it is
>      supposed to by default, and there's diverging behavior
>      in different target, is a case for frustration, bug
>      reports, and support requests.  Another argument is that
>      simulators/emulators (think qemu, for example) may not
>      have this limitation, and can implement the RSP
>      watchpoint packets as gdb expects (always ranged).
> 
>  4 - implementation wise, when the option added in #3 is
>      enabled, gdb (breakpoint.c) checks if the memory being watched
>      corresponds to a scalar type.  (or, going a step
>      further, if wrapped in a structure or union or array,
>      that only contains a single scalar, recursively).  If
>      true, then the "struct value" gdb used to record the
>      expression's current value (for comparison with the
>      new value whenever the low level watchpoint triggers) is
>      the same as usual, but, the watchpoint location that is associated
>      with the watchpoint is set to have length == 1, instead of
>      the whole width of the watched memory range.
>      This way, the target is requested to watch a single
>      byte at the scalar's address.  Note how due to the
>      change done in #2, ppc-linux-nat.c will insert an 
>      PPC_BREAKPOINT_MODE_EXACT in this case.
> 
>      This means that no changes to the target_insert_watchpoint
>      interface are required.  This also means that no 
>      changes to the remote protocol are required.  If
>      the new option is on, and the user wants to watch
>      an "int i", whose address is 0xD3ADB33F, the target
>      will see a "Z2,D3ADB33F,1" request.  That is, watch
>      writes to the 1 byte long range starting at D3ADB33F.
>      That is the same as saying trap only on writes to
>      D3ADB33F.
> 
>      This also means that no new breakpoint_ops type
>      is required either.  A new flag in the breakpoint will do
>      (so that the "exact" property of watchpoints already
>      created is preserved if the user flips the switch).
> 
> WDYT?  Sounds reasonable?  I'd prefer doing these changes
> as first step.  I haven't looked yet at what kind of masks
> masked watchpoints support, but if they only support masks in the form
> of (binary) 11110000, 1111111100, etc. (no alternating 1s and 0s),
> then no target_insert_watchpoint or remote protocol change is
> required either for those.

-- 
Pedro Alves