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

[Pedro Alves <pedro at codesourcery dot com>]

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:
> > > Ok?
> > 
> > Apologies for jumping in so late, but, I don't think I understand
> > why does the user need to know that a watchpoint is "ranged"
> > or not.  All (low-level) watchpoints are ranged,
> 
> Good point. At the time these patches were made, GDB didn't behave that
> way, but it was fixed one year ago already[1]. I guess I just kept the
> old mentality...

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";

> 
> > and what's
> > relevant here is only the width of the range the hardware can watch,
> > but that is already a parameter to the target_insert_watchpoint
> > method.  So I guess my question is, why can't the target backend
> > manage whether to use a range or "normal" watchpoint when asked to
> > inserted a watchpoint?  Is it the watch resource accounting done
> > by breakpoint.c (which is known to be something that should just
> > go away)?
> 
> Yes, the resource accounting is an issue. 

> 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))
	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;

  return 1;
}

The use of "addr + len" in both BookE cases above
seems to allow setting a BookE exact PPC_BREAKPOINT_MODE_EXACT
watchpoint watching [0x..002, 0x..003] (say, eith an
"int foo" global, watch ((char *)&foo)[2]), but, if this
only traps on exact address matches, rather than on accesses
within the watchable regision, why is there a "len" contemplated?
Confusing.

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

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.

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

-- 
Pedro Alves