[RFA][branch] Fix DVC calculation for booke ppc

[RFA][branch] Fix DVC calculation for booke ppc

[Thiago Jung Bauermann]

Hi,

I was testing the booke powerpc hardware debug features I've been
working on that made it to the branch, and found out that hardware
accelerated watchpoint conditions aren't working anymore.

The bug was introduced by a patch of mine (doh).
ppc-linux-nat.c:calculate_dvc uses the watchpoint length that is passed
to target_insert_watchpoint to calculate the contents of the Data Value
Compare register. The problem is that for the ranged watchpoints feature
I changed GDB to pass 1 as length if the "set powerpc exact-watchpoints"
flag is on. This messes up things for calculate_dvc.

This patch makes check_condition obtain the length of the watchpoint
region from the condition expression, so that it can be passed to
calculate_dvc. This works because for a condition to be eligible for
hardware acceleration, it needs to have a strict form:

(gdb) watch ADDRESS|VARIABLE \
       if  ADDRESS|VARIABLE == CONSTANT EXPRESSION

which means that the ADDRESS|VARIABLE part is the same as the watchpoint
region.

There are no regressions on ppc-linux and ppc64-linux. Ok for HEAD and
the branch?
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center


2011-04-04  Thiago Jung Bauermann  <bauerman@br.ibm.com>

	* ppc-linux-nat.c (check_condition): Add len output parameter.
	Set it based on the memory region referenced in the condition
	expression.  Update all callers.

diff --git a/gdb/ppc-linux-nat.c b/gdb/ppc-linux-nat.c
index f0c7f61..6f11715 100644
--- a/gdb/ppc-linux-nat.c
+++ b/gdb/ppc-linux-nat.c
@@ -1865,10 +1865,11 @@ num_memory_accesses (struct value *v)
    DVC (Data Value Compare) register in BookE processors.  The expression
    must test the watch value for equality with a constant expression.
    If the function returns 1, DATA_VALUE will contain the constant against
-   which the watch value should be compared.  */
+   which the watch value should be compared and LEN will contain the size
+   of the constant.  */
 static int
 check_condition (CORE_ADDR watch_addr, struct expression *cond,
-		 CORE_ADDR *data_value)
+		 CORE_ADDR *data_value, int *len)
 {
   int pc = 1, num_accesses_left, num_accesses_right;
   struct value *left_val, *right_val, *left_chain, *right_chain;
@@ -1900,11 +1901,23 @@ check_condition (CORE_ADDR watch_addr, struct expression *cond,
   if (num_accesses_left == 1 && num_accesses_right == 0
       && VALUE_LVAL (left_val) == lval_memory
       && value_address (left_val) == watch_addr)
-    *data_value = value_as_long (right_val);
+    {
+      *data_value = value_as_long (right_val);
+
+      /* DATA_VALUE is the constant in RIGHT_VAL, but actually has
+	 the same type as the memory region referenced by LEFT_VAL.  */
+      *len = TYPE_LENGTH (check_typedef (value_type (left_val)));
+    }
   else if (num_accesses_left == 0 && num_accesses_right == 1
 	   && VALUE_LVAL (right_val) == lval_memory
 	   && value_address (right_val) == watch_addr)
-    *data_value = value_as_long (left_val);
+    {
+      *data_value = value_as_long (left_val);
+
+      /* DATA_VALUE is the constant in LEFT_VAL, but actually has
+	 the same type as the memory region referenced by RIGHT_VAL.  */
+      *len = TYPE_LENGTH (check_typedef (value_type (right_val)));
+    }
   else
     {
       free_value_chain (left_chain);
@@ -1930,7 +1943,7 @@ ppc_linux_can_accel_watchpoint_condition (CORE_ADDR addr, int len, int rw,
 
   return (have_ptrace_booke_interface ()
 	  && booke_debug_info.num_condition_regs > 0
-	  && check_condition (addr, cond, &data_value));
+	  && check_condition (addr, cond, &data_value, &len));
 }
 
 /* Set up P with the parameters necessary to request a watchpoint covering
@@ -1950,7 +1963,8 @@ create_watchpoint_request (struct ppc_hw_breakpoint *p, CORE_ADDR addr,
 
       use_condition = (insert? can_use_watchpoint_cond_accel ()
 			: booke_debug_info.num_condition_regs > 0);
-      if (cond && use_condition && check_condition (addr, cond, &data_value))
+      if (cond && use_condition && check_condition (addr, cond,
+						    &data_value, &len))
 	calculate_dvc (addr, len, data_value, &p->condition_mode,
 		       &p->condition_value);
       else

Re: [RFA][branch] Fix DVC calculation for booke ppc

[Ulrich Weigand]

Thiago Jung Bauermann wrotE:

> The bug was introduced by a patch of mine (doh).
> ppc-linux-nat.c:calculate_dvc uses the watchpoint length that is passed
> to target_insert_watchpoint to calculate the contents of the Data Value
> Compare register. The problem is that for the ranged watchpoints feature
> I changed GDB to pass 1 as length if the "set powerpc exact-watchpoints"
> flag is on. This messes up things for calculate_dvc.
> 
> This patch makes check_condition obtain the length of the watchpoint
> region from the condition expression, so that it can be passed to
> calculate_dvc. This works because for a condition to be eligible for
> hardware acceleration, it needs to have a strict form:
> 
> (gdb) watch ADDRESS|VARIABLE \
>        if  ADDRESS|VARIABLE == CONSTANT EXPRESSION
> 
> which means that the ADDRESS|VARIABLE part is the same as the watchpoint
> region.

Huh.  That strikes me as a hack to work around another hack :-/

I may have missed some of the discussion behind the current implementation.
Could you explain again:

- why you're passing an incorrect length of 1 if the "exact watchpoints"
  flag is on?

- why you're only supporting hardware-accelerated conditions if the length
  is 1?

Note that while you say that the condition needs to have a strict form,
you don't currently actually *verify* this: if you have a command of the
form "watch A if B == C", you only verify that A and B *start* at the
same address -- you really need to also verify that A and B have the
same length -- but you cannot because the length of A is not available
to the target since you're always getting 1 for length.


It seems to me the "right way" would be for the common parts to always
pass correct information (address, length, condition, ...) to the target,
and then for the target to look at the request and choose the best
possible hardware means to implement this particular request ...

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [RFA][branch] Fix DVC calculation for booke ppc

[Thiago Jung Bauermann]

On Tue, 2011-04-05 at 18:42 +0200, Ulrich Weigand wrote:
> Thiago Jung Bauermann wrotE:
> > The bug was introduced by a patch of mine (doh).
> > ppc-linux-nat.c:calculate_dvc uses the watchpoint length that is passed
> > to target_insert_watchpoint to calculate the contents of the Data Value
> > Compare register. The problem is that for the ranged watchpoints feature
> > I changed GDB to pass 1 as length if the "set powerpc exact-watchpoints"
> > flag is on. This messes up things for calculate_dvc.
> > 
> > This patch makes check_condition obtain the length of the watchpoint
> > region from the condition expression, so that it can be passed to
> > calculate_dvc. This works because for a condition to be eligible for
> > hardware acceleration, it needs to have a strict form:
> > 
> > (gdb) watch ADDRESS|VARIABLE \
> >        if  ADDRESS|VARIABLE == CONSTANT EXPRESSION
> > 
> > which means that the ADDRESS|VARIABLE part is the same as the watchpoint
> > region.
> 
> Huh.  That strikes me as a hack to work around another hack :-/

That's one way of looking at it, yes. :-)

> I may have missed some of the discussion behind the current implementation.
> Could you explain again:
> 
> - why you're passing an incorrect length of 1 if the "exact watchpoints"
>   flag is on?

It's not incorrect, really. An exact watchpoint triggers only when the
first byte of its memory region is accessed. Another way of saying it is
that an exact watchpoint watches a 1-byte memory region at the given
address (or at the address of the given variable).

> - why you're only supporting hardware-accelerated conditions if the length
>   is 1?

That is a limitation of BookE processors. You can't make a DVC register
control a ranged watchpoint.

> Note that while you say that the condition needs to have a strict form,
> you don't currently actually *verify* this: if you have a command of the
> form "watch A if B == C", you only verify that A and B *start* at the
> same address -- you really need to also verify that A and B have the
> same length -- but you cannot because the length of A is not available
> to the target since you're always getting 1 for length.

How important is it to enforce that? The "watch A if B == C" case could
occur for a union (watch u.i if u.c == 'a'). It could even be useful in
that situation. Is there a drawback in permitting this?

> It seems to me the "right way" would be for the common parts to always
> pass correct information (address, length, condition, ...) to the target,
> and then for the target to look at the request and choose the best
> possible hardware means to implement this particular request ...

I agree it's the right way, but is the gain worth the wide-reaching
change?
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [RFA][branch] Fix DVC calculation for booke ppc

[Ulrich Weigand]

Thiago Jung Bauermann wrote:
> On Tue, 2011-04-05 at 18:42 +0200, Ulrich Weigand wrote:
> > I may have missed some of the discussion behind the current implementation.
> > Could you explain again:
> > 
> > - why you're passing an incorrect length of 1 if the "exact watchpoints"
> >   flag is on?
> 
> It's not incorrect, really. An exact watchpoint triggers only when the
> first byte of its memory region is accessed. Another way of saying it is
> that an exact watchpoint watches a 1-byte memory region at the given
> address (or at the address of the given variable).

Ah, I see.  What was confusing me is that if you watch, say, a 4-byte integer
variable, even an exact watchpoint will trigger if its low byte changes.

But in fact it triggers only if the access happens via a 4-byte store using
the exact address of the variable -- and in that case, a write actually
occurs to the high byte too, even if it is the same value.

Yes, I agree that length 1 is arguably a correct way to represent this
semantics, then.

> > - why you're only supporting hardware-accelerated conditions if the length
> >   is 1?
> 
> That is a limitation of BookE processors. You can't make a DVC register
> control a ranged watchpoint.

Huh, OK.

> > Note that while you say that the condition needs to have a strict form,
> > you don't currently actually *verify* this: if you have a command of the
> > form "watch A if B == C", you only verify that A and B *start* at the
> > same address -- you really need to also verify that A and B have the
> > same length -- but you cannot because the length of A is not available
> > to the target since you're always getting 1 for length.
> 
> How important is it to enforce that? The "watch A if B == C" case could
> occur for a union (watch u.i if u.c == 'a'). It could even be useful in
> that situation. Is there a drawback in permitting this?

I had been under the impression that the correctness of your patch would
*rely* on that property being enforced; it seemed to me you were trying
to reconstruct the length of the original variable being watched from the
length of the variable occuring in the condition.

However, that is actually incorrect -- the length of the variable being
watched is indeed irrelevant, even when evaluating a condition.   What
is needed is in fact exactly the length of the variable occuring in the
condition, which is what your patch determines.


Given that, I withdraw my objection; sorry for the confusion and thanks
for your extra explanation.

Your patch is OK for mainline.  (The branch is for Joel to decide.)

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [RFA][branch] Fix DVC calculation for booke ppc

[Thiago Jung Bauermann]

On Wed, 2011-04-06 at 00:51 +0200, Ulrich Weigand wrote:
> Given that, I withdraw my objection; sorry for the confusion and thanks
> for your extra explanation.

No problem. Thanks for the speedy review!

> Your patch is OK for mainline.  (The branch is for Joel to decide.)

Committed to HEAD.
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [RFA][branch] Fix DVC calculation for booke ppc

[Thiago Jung Bauermann]

On Wed, 2011-04-06 at 00:51 +0200, Ulrich Weigand wrote:
> Given that, I withdraw my objection; sorry for the confusion and thanks
> for your extra explanation.
> 
> Your patch is OK for mainline.  (The branch is for Joel to decide.)

Joel, would you mind having a look at this patch when you have some
time? Not sure if you didn't have yet, or if it just fell through the
cracks...
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [RFA][branch] Fix DVC calculation for booke ppc

[Joel Brobecker]

> > Your patch is OK for mainline.  (The branch is for Joel to decide.)
> 
> Joel, would you mind having a look at this patch when you have some
> time? Not sure if you didn't have yet, or if it just fell through the
> cracks...

I've just been extra busy the last couple of weeks, but I should
have more time soon. This seems OK for the branch as well.

Cheers,
-- 
Joel

Re: [RFA][branch] Fix DVC calculation for booke ppc

[Thiago Jung Bauermann]

On Mon, 2011-04-18 at 08:29 -0700, Joel Brobecker wrote:
> > > Your patch is OK for mainline.  (The branch is for Joel to decide.)
> > 
> > Joel, would you mind having a look at this patch when you have some
> > time? Not sure if you didn't have yet, or if it just fell through the
> > cracks...
> 
> I've just been extra busy the last couple of weeks, but I should
> have more time soon. This seems OK for the branch as well.

Committed. Thanks for reviewing this!
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center