[PATCH 2/4] Hardware accelerated watchpoint conditions
Thiago Jung Bauermann
bauerman@br.ibm.com
Thu Jul 1 14:51:00 GMT 2010
Hi,
On Wed, 2010-06-23 at 21:57 +0200, Ulrich Weigand wrote:
> Thiago Jung Bauermann wrote:
> > + /* 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;
That was true when using memcpy instead of value_as_long, and I thought
that keeping it that way made things simpler for calculate_dvc. It turns
out that I was wrong.
> > + 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?
I made the changes you suggested and they indeed simplified the code.
> 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. */
>
I changed the comment to read:
/* 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.
Due to limitations in the hardware implementation, it may be capable of
avoiding triggering the watchpoint in some cases where the condition
expression is false, but may report some false positives as well.
For this reason, GDB will still evaluate the condition expression when
the watchpoint triggers. */
What do you think?
> > + 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 ...
I removed that else if then. I'll submit a separate patch to replace
that comment in can_use_hardware_watchpoint.
> > +/* 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.
I copied that function to ppc-linux-nat.c because I expected I'd need
bigger changes in it. It turned out I only needed a pc argument for
evaluate_subexp. I put the function in eval.c and changed callers of
fetch_watchpoint_value in breakpoint.c to use fetch_subexp_value with a
dummy pc argument.
> > +/* 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.
Done.
> > + /* Documentation of what the two routines belwo are expected to do is
> > + provided with the corresponding target_* macros. */
>
> Typo "belwo"
Fixed.
> Otherwise, this is looking really good now!
Great! Thanks for your review and suggestions! There were indeed
definite improvements in the patch.
Thanks and regards,
Thiago Jung Bauermann
IBM Linux Technology Center
2010-07-01 Sergio Durigan Junior <sergiodj@linux.vnet.ibm.com>
Thiago Jung Bauermann <bauerman@br.ibm.com>
* breakpoint.c (fetch_watchpoint_value): Rename to fetch_subexp_value
and move to eval.c. Change callers.
(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.
* eval.c: Include wrapper.h.
(fetch_subexp_value): Moved from breakpoint.c.
* ppc-linux-nat.c (ppc_linux_region_ok_for_hw_watchpoint):
Formatting fix.
(can_use_watchpoint_cond_accel): New function.
(calculate_dvc): Likewise.
(num_memory_accesses): 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 declaration 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.
* value.c (free_value_chain): New function.
* value.h (fetch_subexp_value): New prototype.
(free_value_chain): Likewise.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: condition.diff
Type: text/x-patch
Size: 38929 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/gdb-patches/attachments/20100701/4857ad88/attachment.bin>
More information about the Gdb-patches
mailing list