This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


Thiago Jung Bauermann wrote:
> On Thu, 2010-02-11 at 19:23 +0100, Ulrich Weigand wrote:
> > 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 ... ]
> 
> I ended up doing the following to validate the watchpoint condition:
> 
> - verify whether the expression is an equality
> - evaluate each side with evaluate_subexp (with EVAL_NORMAL, i.e., with 
>   side-effects)
> - one of the resulting values should be not_lval, and the other should 
>   be lval_memory
> 
> I don't think fully evaluating the expression is a problem because GDB
> already evaluates it each time the watchpoint triggers, so it should be
> harmless to do the same here.
> 
> What do you think?

Unfortunately, I think this doesn't quite work: the problem is not that
accesses have been evaluated (that should be fine as you said), but rather
the problem is that you do not guarantee that the comparison value is
*constant*.  Now you do have the check against not_lval, which will
indeed exclude simple non-constant values like direct register or
memory references.  However, if the right side is a *complex* but
non-constant expression, you'll also see non_lval and accept it.

For example, I think your code would currently (erroneously) claim to
be able to use hardware assists to watch something like:

  watch x if x == y + z

with global variables x, y, z.

The prior attempt got this correct (but sub-optimal) by disallowing
any complex expression.  What we really want is to allow any *constant*
expresssion, i.e. any expression whose value does not depend on the
contents of registers or memory.

> For completeness, I changed the code to calculate the DVC taking all
> aspects in consideration. It became a bit complicated, so I put many
> comments to explain what it is doing.

OK, thanks for the explanation.

> One important thing about this patch is that I had to add a new target
> method: target_can_accel_watchpoint_condition. It is called in
> watchpoint_locations_match to avoid making GDB insert only one of two
> bp_locations which are at the same address but with different
> conditions. When the target supports evaluating the watchpoint
> expression in hardware, GDB needs to insert both hardware watchpoints
> even if they are at the same address, so that their different conditions
> have a chance to trigger.

Hmm, I'm wondering -- it seems a bit odd that there is no feedback
from target_insert_watchpoint itself whether the condition could be
honored or not -- it's just silently ignored, and you have to consider
this extra target hook.  Maybe a more straightforward interface would
be to have target_insert_watchpoint fail if called with a condition
it cannot implement -- common code could then retry with a NULL
condition, and reset the breakpoint type to remember that for this
watchpoint, software condition checks need to be applied ...

(But this is really a separate issue from the main purpose of this
patch; please do not consider this as a matter that would block
acceptance of the patch as is, as far as I'm concerned.)

> +/* Calculate the enable bits and the contents of the Data Value Compare
> +   debug register present in BookE processors.
> +
> +   ADDR is the address to be watched, LEN is the length of watched data
> +   and DATA_VALUE is the value which will trigger the watchpoint.  On exit,
> +   CONDITION_MODE will hold the enable bits for the DVC, and CONDITION_VALUE
> +   will hold the value which should be put in the DVC register.  */

DATA_LEN is not described.  I'm wondering why two lengths are needed here?

> +static void
> +calculate_dvc (CORE_ADDR addr, int len, CORE_ADDR data_value,
> +	       int data_value_len, uint32_t *condition_mode,
> +	       uint64_t *condition_value)

> +/* Verifies whether the expression COND can be implemented using the
> +   DVC (Data Value Compare) register in BookE processors.  The expression
> +   must test the watch value for equality with a constant expression.  */
> +static int
> +check_condition (CORE_ADDR watch_addr, struct expression *cond,
> +		 CORE_ADDR *data_value, int *data_value_len)
> +{
> +  int pc = 1, copy_len;
> +  struct value *v_left, *v_right, *val;
> +
> +  if (cond->elts[0].opcode != BINOP_EQUAL)
> +    return 0;
> +
> +  v_left = evaluate_subexp (NULL_TYPE, cond, &pc, EVAL_NORMAL);
> +  v_right = evaluate_subexp (NULL_TYPE, cond, &pc, EVAL_NORMAL);
> +
> +  if (VALUE_LVAL (v_left) == lval_memory && VALUE_LVAL (v_right) == not_lval
> +      && value_address (v_left) == watch_addr)
> +    val = v_right;
> +  else if (VALUE_LVAL (v_left) == not_lval && VALUE_LVAL (v_right) == lval_memory
> +	   && value_address (v_right) == watch_addr)
> +    val = v_left;
> +  else
> +    return 0;

See the discussion above; this does not ensure that VAL represents a
*constant* expression.

> +  *data_value = 0;
> +  *data_value_len = TYPE_LENGTH (value_type (val));
> +  copy_len = *data_value_len > sizeof (CORE_ADDR)?
> +               sizeof (CORE_ADDR) : *data_value_len;
> +  memcpy (data_value, value_contents (val), copy_len);

There seem to be byte order assumptions here.  In theory, in a multi-arch
setup, the byte order of VAL might differ from the native byte order of
a CORE_ADDR ...   Can this not be done via value_as_long, possibly 
followed by shifts?


Thanks,
Ulrich

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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]