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

["Ulrich Weigand" <uweigand at de dot ibm dot com>]

Thiago Jung Bauermann wrote:

> @@ -1666,15 +1697,33 @@ ppc_linux_insert_watchpoint (CORE_ADDR addr, int len, int rw)
>  
>    if (have_ptrace_new_debug_booke)
>      {
> +      int byte_to_enable;
>        struct ppc_hw_breakpoint p;
> +      CORE_ADDR cond_addr;

The naming confused me as cond_addr turns out to hold the *data* value
to be compared against, not any address ;-)

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

This logic, together with the variety of special-purpose subroutines,
strikes me as overly restrictive on the syntactical format of the
condition expression, for example:
- why shouldn't "if VAR.MEMBER == LITERAL" be allowed?
- why shouldn't "if VAR == <constant expression>" be allowed?

What we really need here is a variant of expression evaluation that
is "extremely lazy" and stops as soon as any reference to target
memory or register contents would be made.  If we run both sides
of the BINOP_EQUAL through this new evaluator, and one side can
be fully evaluated, and the other can be evaluated to a lazy
lval_memory value, we should be in business.

This new evaluator might possibly take the form of a new "enum noside"
value as argument to evaluate_subexp, or might be a completely separate
routine (like e.g. gen_eval_for_expr).  [ Or maybe even the regular
evaluator after temporarily resetting current_target to a target that
throws an exception on every memory / register access?  That may be
a bit ugly though ... ]

(If this is too much effort for now, I'd at least ask to put the
special-purpose helpers into a ppc-private file so as not to advertise
their reuse elsewhere ...)


Also, why do we need to evaluate the expression (as opposed to the
condition) again here?  We know we're going to set the hardware
watchpoint at ADDRESS.  As long as the memory access implied in COND
happens at that address, it does not matter at all how the expression
was formulated, right?

In fact, I'm not sure why we're passing the expression into the
insert/remove watchpoint routine in the first place, as we already
get the address/length.

> +	  byte_to_enable = addr % 4;
> +	  cond_addr >>= (byte_to_enable * 8);
> +	  p.condition_mode  = (PPC_BREAKPOINT_CONDITION_AND
> +			       | PPC_BREAKPOINT_CONDITION_BE (byte_to_enable));
> +	  p.condition_value = (uint64_t) cond_addr;

I'm not really familiar with the details of the hardware here, but
is the byte-enable stuff really correct?  You always set just one
single bit in the mask; shouldn't we have one bit for every byte
that is to be enabled?

Also, don't we at some point need to consider the length; e.g. if
we have a byte-sized variable that happens to start at an address
that is a multiple of 4?

Bye,
Ulrich

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