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

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

Thiago Jung Bauermann wrote:

> Indeed. I didn't realised that. I changed the patch to use the approach
> found in can_use_hardware_watchpoint: evaluate the expression and then
> go through the value chain generated by the evaluation to check whether
> there's any memory or register access. What do you think?

Ah, excellent point.  I agree this should work fine, thanks.

> > DATA_LEN is not described.  I'm wondering why two lengths are needed here?
> 
> I described it now. We need two lengths because the watched value and
> the constant in the condition may have different types. For instance, if
> you have a "short int foo" and your condition is "foo == 2", then the
> watched variable is of type short int and the condition constant is an
> int.

Still seems unnecessarily complex to me, but see below ...

> +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)
> +{
> +  int i, num_byte_enable, align_offset, num_bytes_off_dvc,
> +      rightmost_enabled_byte;
> +  CORE_ADDR addr_end_data, addr_end_dvc;
> +
> +  /* The DVC register compares bytes within fixed-length windows which
> +     are word-aligned, with length equal to that of the DVC register.
> +     We need to calculate where our watch region is relative to that
> +     window and enable comparison of the bytes which fall within it.  */
> +
> +  align_offset = addr % booke_debug_info.sizeof_condition;
> +  addr_end_data = addr + len;
> +  addr_end_dvc = (addr - align_offset
> +		  + booke_debug_info.sizeof_condition);
> +  num_bytes_off_dvc = (addr_end_data > addr_end_dvc)?
> +			 addr_end_data - addr_end_dvc : 0;
> +  num_byte_enable = len - num_bytes_off_dvc;
> +  /* Here, bytes are numbered from right to left.  */
> +  rightmost_enabled_byte = (addr_end_data < addr_end_dvc)?
> +			      addr_end_dvc - addr_end_data : 0;
> +
> +  *condition_mode = PPC_BREAKPOINT_CONDITION_AND;
> +  for (i = 0; i < num_byte_enable; i++)
> +    *condition_mode |= PPC_BREAKPOINT_CONDITION_BE (i + rightmost_enabled_byte);
> +
> +  /* Now we need to match the position within the DVC of the comparison
> +     value with where the watch region is relative to the window
> +     (i.e., the ALIGN_OFFSET).  */
> +
> +  /* The size of the user-provided data value matters because the value is
> +     "left-aligned" within DATA_VALUE, e.g: a 1-byte data type will occupy the
> +     first byte of DATA_VALUE, a two-byte data type the first two, and so on.

Well, but that's only because you arranged the value in that weird way
in the first place :-)   See check_condition below:
  *data_value = value_as_long (val);
  *data_value_len = TYPE_LENGTH (value_type (val));
  *data_value = *data_value << (sizeof (CORE_ADDR) - *data_value_len) * 8;

> +  if (data_value_len - len > align_offset)
> +    /* The user-provided value type is larger than the watched value type,
> +       and it is also to the right of the offset within the DVC register
> +       where it should be.  */
> +    *condition_value = (uint64_t) data_value << (data_value_len - len
> +						 - align_offset) * 8;
> +  else
> +    /* The user-provided value type is either smaller than the watched
> +       value type, or else it is equal or larger than the watched value
> +       type but to the left of the offset within the DVC register where
> +       it should be.  */
> +    *condition_value = (uint64_t) data_value >> (align_offset
> +						 - (data_value_len - len)) * 8;

Unless I'm missing something, this also makes the additional assumption
that the width of the DVC register equals the size of a CORE_ADDR:
In check_condition, you ensure the value is "left-aligned" within a
CORE_ADDR, and here you assume the value is left-aligned within the
DVC registers size ...

It seems to me this whole thing could be simplified by removing the
DATA_VALUE_LEN, *not* shifting the value at all in check_condition,
and replacing the above if-else construct by something along the
lines of:

   *condition_value = ((uint_64_t) data_value >> num_bytes_off_dvc * 8)
		       << (rightmost_enabled_byte * 8);

Thoughts?

As a side remark, if num_bytes_off_dvc is non-zero, it seems the hardware
will only check *part* of the condition.  That's probably better than
nothing, but means the definition of target_can_accel_watchpoint_condition:

  /* Return non-zero if the target is capable of using hardware to evaluate
     the condition expression, thus only triggering the watchpoint when it is
     true.  */

is a bit optimistic.  Maybe it should be reworded along the lines of

  /* Return non-zero if the target is capable of using hardware to evaluate
     the condition expression.  In this case, if the condition is false when
     the watched memory location changes, execution may continue without
     the debugger being notified.  */


> +  for (; v; v = value_next (v))
> +    {
> +      if (VALUE_LVAL (v) == lval_memory)
> +	{
> +	  /* A lazy memory lvalue is one that GDB never needed to fetch;
> +	     we either just used its address (e.g., `a' in `a.b') or
> +	     we never needed it at all (e.g., `a' in `a,b').  */
> +	  if (!value_lazy (v))
> +	    found_memory_cnt++;
> +	}
> +      else if (VALUE_LVAL (v) != not_lval
> +	       && deprecated_value_modifiable (v) == 0)
> +	return -1;	/* ??? What does this represent? */

These are values from the history (e.g. $1).  In this context, they
should be treated as non-lvalues, so they should be fine ...

> +/* Find the current value of EXP.  Return the value in *VALP and *RESULTP
> +   and the chain of intermediate and final values in *VAL_CHAIN.  RESULTP
> +   and VAL_CHAIN may be NULL if the caller does not need them.
> +
> +   If a memory error occurs while evaluating the expression, *RESULTP will
> +   be set to NULL.  *RESULTP may be a lazy value, if the result could
> +   not be read from memory.  It is used to determine whether a value
> +   is user-specified (we should watch the whole value) or intermediate
> +   (we should watch only the bit used to locate the final value).
> +
> +   If the final value, or any intermediate value, could not be read
> +   from memory, *VALP will be set to NULL.  *VAL_CHAIN will still be
> +   set to any referenced values.  *VALP will never be a lazy value.
> +
> +   If VAL_CHAIN is non-NULL, *VAL_CHAIN will be released from the
> +   value chain.  The caller must free the values individually.  If
> +   VAL_CHAIN is NULL, all generated values will be left on the value
> +   chain.  */
> +static void
> +fetch_subexp_value (struct expression *exp, int *pc, struct value **valp,
> +			struct value **resultp, struct value **val_chain)

This whole function is a 1:1 copy of breakpoint.c:fetch_watchpoint_value,
right?  I'd prefer if this function were moved to expr.c and used from
both places.

> +/* Frees all the elements in a chain of values.  */
> +static void
> +free_value_chain (struct value *v)
> +{
> +  struct value *next;
> +
> +  for (; v; v = next)
> +    {
> +      next = value_next (v);
> +      value_free (v);
> +    }
> +}

Likewise, this ought to go to value.c.

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

Typo "belwo"

Otherwise, this is looking really good now!

Thanks,
Ulrich

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