[PATCH 2/4] Hardware accelerated watchpoint conditions

[Thiago Jung Bauermann]

On Wed, 2010-06-09 at 15:27 +0200, Ulrich Weigand wrote:
> Thiago Jung Bauermann wrote:
> 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.

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?

If this doesn't work then I'll go back to the simple approach of the
first patch. :-)

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

We need to determine whether the condition can be implemented in
hardware before target_insert_watchpoint is called because
update_global_location_list will flag duplicate watchpoint locations and
those won't be inserted at all...

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

Thanks! I'm in favor of avoiding adding unnecessary flags and hooks, but
in this case I think it's necessary.

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

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.

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

I didn't realise that. This version follows your suggestion.

Thanks and regards,
Thiago Jung Bauermann
IBM Linux Technology Center


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

	* breakpoint.c (insert_bp_location): Pass watchpoint condition in
	target_insert_watchpoint.
	(remove_breakpoint_1) Pass watchpoint condition in
	target_remove_watchpoint.
	(watchpoint_locations_match): Call
	target_can_accel_watchpoint_condition.
	* ppc-linux-nat.c: Include exceptions.h and wrapper.h
	(ppc_linux_region_ok_for_hw_watchpoint): Formatting fix.
	(can_use_watchpoint_cond_accel): New function.
	(calculate_dvc): Likewise.
	(num_memory_accesses): Likewise.
	(fetch_subexp_value): Likewise.
	(free_value_chain): Likewise.
	(check_condition): Likewise.
	(ppc_linux_can_accel_watchpoint_condition): Likewise
	(ppc_linux_insert_watchpoint): Call can_use_watchpoint_cond_accel,
	check_condition and calculate_dvc.
	(ppc_linux_remove_watchpoint): Likewise.
	(_initialize_ppc_linux_nat): Set to_can_accel_watchpoint_condition to
	ppc_linux_can_accel_watchpoint_condition
	* target.c (debug_to_insert_watchpoint): Add argument for watchpoint
	condition.
	(debug_to_remove_watchpoint): Likewise.
	(debug_to_can_accel_watchpoint_condition): New function.
	(update_current_target): Set to_can_accel_watchpoint_condition.
	(setup_target_debug): Set to_can_accel_watchpoint_condition.
	* target.h: Add opaque declarations for struct expression.
	(struct target_ops) <to_insert_watchpoint>,
	<to_remove_watchpoint>: Add new arguments to pass the watchpoint
	<to_can_accel_watchpoint_condition>: New member.
	condition.  Update all callers and implementations.
	(target_can_accel_watchpoint_condition): New macro.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: condition.diff
Type: text/x-patch
Size: 33195 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/gdb-patches/attachments/20100623/81c39471/attachment.bin>