[PATCH 2/4] Hardware accelerated watchpoint conditions

Mon Feb 8 20:01:00 GMT 2010

On Fri 05 Feb 2010 03:15:55 Joel Brobecker wrote:
> Good news (see below).

Great. :-)

> > > I think, from your patch, that GDB will still evaluate the condition
> > > once after the watchpoint and its condition trigger. I think that
> > > we might want to fix that eventually, but I am actually more than
> > > happy to ignore this minor issue for now.  I like baby steps :).
> >
> > If I'm following your thought here, I added that on purpose. Before
> > letting GDB evaluate the condition one more time, GDB wouldn't know
> > which watchpoint triggered if there were two at the same location,
> > with different conditions.
> 
> I knew that! (if the name DiNozzo comes to mind, then I don't
> know what you're talking about ;-) )

Oh, sorry then!

> Just a few minor nits - pre-approved after the comments are addressed.

I addressed your comments. I won't commit the patch yet because the kernel patch enabling this code to 
work hasn't been committed upstream yet. I expect it will be accepted soon, so I hope to get this in in 
time for 7.1.

The same applies to the first patch in this series (adapting ppc-linux-nat.c to the new ptrace interface), 
which you approved a while ago...

> > +/* This function is used to determine whether the condition associated
> > +   with bp_location B is of the form:
> > +
> > +   watch *<ADDRESS> if <VAR> == <LITERAL>
> > +
> > +   If it is, then it sets DATA_VALUE to LITERAL and returns 1.
> > +   Otherwise, it returns 0.  */
> 
> Can you add a note that VAR must be stored at ADDRESS too?  Same for
> similar functions...

Done.

> > +  /* At this point, all verifications were positive, so we can use
> > +     hardware-assisted data-matching.  Set the data value, and return
> > +     non-zero.  */
> 
> The comment here is a bit too purpose-specific. It betrays its origin :),
> but I think we should either drop it (I think that the function description
> is sufficently clear), or change it to something more neutral.  Again,
> same for all other functions that used the same kind of comment.

Dropped.

> > +/* Return greater than zero if the condition associated with
> > +   the watchpoint `b' can be treated by the hardware; zero otherwise.
> > +
> > +   Also stores the data value in `data_value'.  */
> 
> I'd drop this comment in breakpoint.h. It duplicates the descriptions
> in breakpoint.c...

Dropped.

> > +static int
> > +ppc_linux_can_use_watchpoint_cond_accel (void)
> 
> This function now needs a short description. It's kind of obvious,
> what this function does, I agree, but we're trying to be consistent
> in our style, and provide always function descriptions unless the
> function implements a routine (function pointer) that's already
> documented (eg a gdbarch method, or a target_ops routine).

I added this concise description:

/* Check whether we have at least one free DVC register.  */

> > +	if (p->hw_breaks[i].hw_break != NULL
> > +	    && p->hw_breaks[i].hw_break->condition_mode
> > +		    != PPC_BREAKPOINT_CONDITION_NONE)
> 
> I'm not quite sure, here, but I think that the GNU Coding Style asks
> us to parenthesize your not-equal condition, purely for the benefit of
> GNU indent:
> 
>      if (p->hw_breaks[i].hw_break != NULL
>          && (p->hw_breaks[i].hw_break->condition_mode
>              != PPC_BREAKPOINT_CONDITION_NONE))
> 
> Can you double-check for me?

It does mention that. Changed.

> > +      if (cond && ppc_linux_can_use_watchpoint_cond_accel ()
> > +	  && (watch_address_if_address_equal_literal (exp, cond, &cond_addr)
> > +	      || watch_var_if_var_equal_literal (exp, cond, &cond_addr)
> > +	      || watch_var_if_address_equal_literal (exp, cond, &cond_addr)
> > +	      || watch_address_if_var_equal_literal (exp, cond, &cond_addr))) {
> 
> Formatting nit: The curly brace should be on the next line.  There are
> a few instances where this needs to be fixed.

Fixed this and the other instances.

> > +	p.condition_mode  = PPC_BREAKPOINT_CONDITION_AND
> > +			    | PPC_BREAKPOINT_CONDITION_BE (byte_to_enable);
> 
> Similar to above, I think you'll need to parenthesize the rhs.
> 

Fixed this and the other instance.

> > diff --git a/gdb/target.c b/gdb/target.c
> > index e6659c9..7adc96e 100644
> > --- a/gdb/target.c
> > +++ b/gdb/target.c
> > @@ -44,6 +44,8 @@
> >  #include "exec.h"
> >  #include "inline-frame.h"
> >
> > +struct expression;
> > +
> 
> This change is unnecessary (already declared in target.h)...

Dropped.

> > diff --git a/gdb/target.h b/gdb/target.h
> > index 7103ab2..a65e900 100644
> > --- a/gdb/target.h
> > +++ b/gdb/target.h
> > @@ -36,6 +36,10 @@ struct trace_status;
> >  struct uploaded_tsv;
> >  struct uploaded_tp;
> >
> > +struct bp_location;
> > +struct breakpoint;
> > +struct expression;
> 
> I don't think that declaring struct bp_location is necessary.
> I just see that struct breakpoint should probably have been declared
> before your patch - so OK.

Dropped struct bp_location, kept the other two.

> > +    int (*to_remove_watchpoint) (CORE_ADDR, int, int, struct expression
> > *, +						      struct expression *);
> > +    int (*to_insert_watchpoint) (CORE_ADDR, int, int, struct expression
> > *, +						      struct expression *);
> 
> Would you mind adding a comment explaining that documentation of what
> these routines are expected to do is provided with the corresponding
> target_* macros? I personally think that the complete description
> of what these routines are supposed to do should be right there rather
> than with the target_ macros, but that's only a very minor matter and
> nothing to do with your patch.

Added:

   /* Documentation of what the two routines below are expected to do is
       provided with the corresponding target_* macros.  */

> > +#ifndef target_region_ok_for_hw_watchpoint
> >  #define target_region_ok_for_hw_watchpoint(addr, len) \
> >      (*current_target.to_region_ok_for_hw_watchpoint) (addr, len)
> > -
> > +#endif
> 
> This is not right - we no longer allow a macro to be overridden.

Ok, dropped.

> >  /* Set/clear a hardware watchpoint starting at ADDR, for LEN bytes. 
> > TYPE is 0 for write, 1 for read, and 2 for read/write accesses.  Returns
> > 0 for success, non-zero for failure.  */
> >
> > -#define	target_insert_watchpoint(addr, len, type)	\
> > -     (*current_target.to_insert_watchpoint) (addr, len, type)
> > +#define	target_insert_watchpoint(addr, len, type, watch_exp, cond) \
> > +     (*current_target.to_insert_watchpoint) (addr, len, type, watch_exp,
> > cond)
> 
> The macro description needs to be updated (to mention the two new
> parameters).

Updated.

> > -#define	target_remove_watchpoint(addr, len, type)	\
> > -     (*current_target.to_remove_watchpoint) (addr, len, type)
> > +#define	target_remove_watchpoint(addr, len, type, watch_exp, cond) \
> > +     (*current_target.to_remove_watchpoint) (addr, len, type, watch_exp,
> > cond)
> 
> Can you add a short documentation for this macro?

It is already documented, in a comment above target_insert_watchpoint (which
is defined right before target_remove_watchpoint):

/* Set/clear a hardware watchpoint starting at ADDR, for LEN bytes.  TYPE is 0
   for write, 1 for read, and 2 for read/write accesses.  WATCH_EXP is the
   watchpoint's expression and COND is the expression for its condition,
   or NULL if there's none.  Returns 0 for success, non-zero for failure.  */

-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center

2010-02-08  Sergio Durigan Junior  <sergiodj@linux.vnet.ibm.com>
	    Thiago Jung Bauermann  <bauerman@br.ibm.com>

	* target.h: Add opaque declarations for bp_location, breakpoint and
	expression.
	(struct target_ops) <to_insert_watchpoint>,
	<to_remove_watchpoint>: Add new arguments to pass the watchpoint
	expression and its condition.  Update all callers and implementations.
	(target_region_ok_for_hw_watchpoint): Surround with ifndef.
	* breakpoint.c (exp_is_address): New function.
	(exp_is_var): Ditto.
	(exp_is_address_equal_literal): Ditto.
	(exp_is_var_equal_literal): Ditto.
	(watch_address_if_var_equal_literal): Ditto.
	(watch_var_if_address_equal_literal): Ditto.
	(watch_var_if_var_equal_literal): Ditto.
	(watch_address_if_address_equal_literal): Ditto.
	* breakpoint.h (watch_address_if_address_equal_literal): Declare.
	(watch_var_if_var_equal_literal): Ditto.
	(watch_address_if_var_equal_literal): Ditto.
	(watch_var_if_address_equal_literal): Ditto.
	* ppc-linux-nat.c (ppc_linux_can_use_watchpoint_cond_accel): New
	function.
	(ppc_linux_insert_watchpoint): Check if it is possible to use
	hardware-accelerated condition checking.
	(ppc_linux_remove_watchpoint): Check if the watchpoint to delete
	has hardware-accelerated condition checking.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ppc476-condition.diff
Type: text/x-patch
Size: 26461 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/gdb-patches/attachments/20100208/88a51a8c/attachment.bin>