Re: [PATCH 2/4] Hardware accelerated watchpoint conditions

[Joel Brobecker <brobecker at adacore dot com>]

Good news (see below).

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

> > One of the downsides of this approach is that GDB will no longer
> > be able to print that the condition will be hardware-accelerated
> > when the user creates the watchpoint. I think that's OK for now.
> 
> Yeah, I would like to have that message printed though. Wil have
> to think about it.

Me too. I think we're touching at a larger issue, there (already
pre-discussed with Eli). We'd need to talk to the target sooner,
rather than at insertion time.   I think that's a desirable change
anyway, but I'm unsure of the consequences...

> 2010-02-04  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.

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

> +/* 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...

> +  /* 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.

> +/* 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...

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


> +	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?

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

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

    p.condition_mode  = (PPC_BREAKPOINT_CONDITION_AND
                         | PPC_BREAKPOINT_CONDITION_BE (byte_to_enable));

> +	p.condition_value = (uint64_t) cond_addr;
> +      } else {

(curly braces)

> +	      || watch_address_if_var_equal_literal (exp, cond, &cond_addr))) {

(curly brace)

> +	p.condition_mode  = PPC_BREAKPOINT_CONDITION_AND
> +			    | PPC_BREAKPOINT_CONDITION_BE (byte_to_enable);

parenthesize.

> +	p.condition_value = (uint64_t) cond_addr;
> +      } else {

curly braces

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


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

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

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

>  /* 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).

> -#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?

-- 
Joel