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

[Thiago Jung Bauermann <bauerman at br dot ibm dot com>]

On Thu, 2010-11-25 at 17:31 +0000, Pedro Alves wrote:
> On Wednesday 24 November 2010 21:05:55, Thiago Jung Bauermann wrote:
> > On Tue, 2010-11-23 at 23:16 +0000, Pedro Alves wrote:
> > > On Tuesday 23 November 2010 21:51:40, Thiago Jung Bauermann wrote:
> That patch helped in the array case (maybe it was even a regression
> that was fixed?), but even the structure case, gdb always behaved
> the "ranged" way, AFAIK.  E.g., if you "(gdb) watch" an int, assuming
> 32-bit, gdb is supposed to be watching the range of 4 bytes starting
> from the int's address.  And watching "int i;" is supposed to be
> the same as watching "struct {int i;} i";

Is there a difference between "int i" and "struct {int i;} i" besides
having to access the integer using "i.i" instead of "i"?

But I see your point, and agree.

> > But also, the target needs to
> > know that it's being asked to watch a region which is expected to have
> > accesses in the middle of it. This is because of the difference of how
> > hardware watchpoints work in server and embedded PowerPC processors. In
> > the former, a hardware watchpoint triggers when any byte within an 8
> > byte window starting at the given address is accessed. In the latter,
> > only accesses at the given address trigger the watchpoint.
> 
> Hmm..  Oh?  Without knowing a think about BookE, that is not what I'd
> infer from:
> 
> static int
> ppc_linux_region_ok_for_hw_watchpoint (CORE_ADDR addr, int len)
> {
>   /* Handle sub-8-byte quantities.  */
>   if (len <= 0)
>     return 0;
> 
>   /* The new BookE ptrace interface tells if there are alignment restrictions
>      for watchpoints in the processors.  In that case, we use that information
>      to determine the hardcoded watchable region for watchpoints.  */
>   if (have_ptrace_booke_interface ())
>     {
>       if (booke_debug_info.data_bp_alignment
> 	  && (addr + len > (addr & ~(booke_debug_info.data_bp_alignment - 1))
> 	      + booke_debug_info.data_bp_alignment))

data_bp_alignment tells GDB that the processor only accepts watchpoints
at certain address intervals. This code assumes that if that is the
case, then the watchpoint triggers with any access within the interval.
This covers server processors. For BookE processors, data_bp_alignment
should be 0 (since they can put a watchpoint at any address) and the
function will return 1 regardless of len. Perhaps I should check for
data_bp_alignment > 1 since 1 could also express that a watchpoint can
be put anywere.

> 	return 0;
>     }
>   /* addr+len must fall in the 8 byte watchable region for DABR-based
>      processors (i.e., server processors).  Without the new BookE ptrace
>      interface, DAC-based processors (i.e., embedded processors) will use
>      addresses aligned to 4-bytes due to the way the read/write flags are
>      passed in the old ptrace interface.  */
>   else if (((ppc_linux_get_hwcap () & PPC_FEATURE_BOOKE)
> 	   && (addr + len) > (addr & ~3) + 4)
> 	   || (addr + len) > (addr & ~7) + 8)
>     return 0;

This code just preserves GDB behaviour when the new BookE ptrace
interface is not available. I agree it's buggy but I didn't think it was
a problem to address in this patch series (also, not sure how to fix it
now that you mention that GDB expects watchpoints to be ranged).

> > So for an embedded powerpc processor, it's not enough to know that it
> > should watch a 4 byte region at a given address. If it represents an
> > integer and thus it can expect accesses only at that address then a
> > regular watchpoint is enough. But if it's an array of four chars, then
> > it needs two watchpoint registers to set up a hardware watchpoint...
> 
> It all looks to me that the "ranged" notion is backwards.
> The default watchpoint that GDB expects to handle _is_ ranged.
> E.g., that's how it works on x86, and DABR based PPC.
> 
> That's why this:
> 
> +* GDB now supports ranged watchpoints, which stop the inferior when it
> +  accesses any address within a specified memory range.
> 
> Is just confusing, because this is not a new GDB feature.

So I should be more specific and say:

"GDB now supports hardware accelerated watchpoints which cover a memory
region on embedded PowerPC processors running the Linux kernel."

WDYT?

> BTW, it's not only useful to watch the whole range of a 
> structure or arrays.  It's often useful to watch what random
> code is clobbering an int or pointer.  The bad write may well be
> to the middle of the integer or pointer.  I bet that
> users end up surprised if PPC doesn't catch that with
> gdb's "watch" command.

Good point.

> > So that's way I created a target_insert_ranged_watchpoint. The other
> > option would be to add a flag to target_insert_watchpoint...
> 
> It appears to me that if there should be a new kind of way to
> insert watchpoints, it should be to allow setting watchpoints
> that only work if the accesses are aligned, that is to support
> the behaviour you describe with PPC_BREAKPOINT_MODE_EXACT
> in embedded processors.

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...

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?
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center