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

[Joel Brobecker <brobecker at adacore dot com>]

Global Maintainers:

This is touching an area of the target vector that I think could be
improved. Eli actually mentioned some of the issues in this area, a few
days ago.  I don't think that we should stall this patch pending
a rework of the watchpoint support, but I would like to see an
implementation that does not make the long term cleanup more difficult
than it already is.  So, feedback and ideas would be welcome.

Thiago, Luis,

Sorry for the delay looking into this; I needed a little bit of time
to let this brew a little...

> > In the case of BookE processors, the condition will be accelerated
> > when the user types "watch A if A == B", where A and B are either
> > address (e.g. "*0x12345678") or variables.

I like the principle that detection of the situations where h/w
acceleration of the condition can be done, so that the user does not
have to do anything to get access to this feature.

I think, from your patch, that GDB will still evaluate the condition
once after the watchpoint and its condition trigger. I think that
we might want to fix that eventually, but I am actually more than
happy to ignore this minor issue for now.  I like baby steps :).
In fact, let's worry about functionality only, even if it means
writing inefficient code. We can implement caches and optimizations
later, if it helps getting this in faster.

My first general observation is that this patch introduces new
target_ops routine in an area that, IMO, could use some simplification
(refer to the first comment above). I think we can avoid that.

Conceptually, I think that the target now needs to be able to look
at the watchpoint expression, and the watchpoint condition, in order
to determine whether the condition evaluation can be evaluated
by hardware. I propose that we expand the insert_watchpoint routine
to pass this data, as 2 extra parameters; and let these routines
decide whether to use hardware acceleration or not for the condition.

One of the downsides of this approach is that GDB will no longer
be able to print that the condition will be hardware-accelerated
when the user creates the watchpoint. I think that's OK for now.

The upside is that the code will not have to worry about this
aspect of the watchpoint when inserting/removing it. Thus,
the following hunk would be simplified:

> -				      bpt->watchpoint_type);
> +      if (bpt->owner->hw_point_flag == HW_POINT_COND_HW_ACCEL)
> +	val = target_insert_cond_accel_watchpoint (bpt->address,
> +						   bpt->length,
> +						   bpt->watchpoint_type,
> +						   bpt->cond_hw_addr);
> +      else
> +	val = target_insert_watchpoint (bpt->address,
> +					bpt->length,
> +					bpt->watchpoint_type);

I am also a bit suspicious of adding the following bits to the bp_location
structure directly:

> +  /* Flag to indicate if the condition is going to be accelerated
> +     by hardware.  If its value is non-zero, then GDB checks the
> +     condition using hardware acceleration; otherwise it uses the
> +     regular software-based checking.  */
> +  int cond_hw_accel : 1;
> +
> +  /* If the condition can be hardware-accelerated, then we must
> +     get the condition's variable address so that GDB can
> +     properly set the evaluation via hardware.  */
> +  CORE_ADDR cond_hw_addr;

I think that they are too target specific. Initially, we can do without,
with our not-so-efficient approach, by recomputing these address every
time we insert the watchpoint.   I think that'd still be an interesting
first step.   Then we can look at storing that info as a private data
structure, allocated by the target.  I think that this would be doable
without too many changes to the current framework - we just need to
be a little careful as watchpoint expressions and conditions get
re-evaluated sometimes.

> +static int
> +exp_is_address_p (struct breakpoint *b)

If you want, you can drop the _p prefix in this type of situation.
I think the use of the verb _is_ in the names makes it obvious that
this function is a predicate.

> +int
> +default_watch_address_if_var_equal_literal_p (struct bp_location *b,
> +					      CORE_ADDR *data_value)

This is nitpicking on function names, but I don't understand the
"default" here. Would there be some non-default implementations
of the same routine? Perhaps: watch_address_if_var_equal_literal_p
is just going to be sufficient?

> +  /* Make sure that the two addresses are the same.  */
> +  if (exp_address != cond_address)
> +    {
> +      printf_filtered (_("\
> +Memory location for the watchpoint expression and its condition need\n\
> +to be the same.\n"));
> +      return 0;
> +    }

Not sure whether you really meant to keep this message. It sounds like
an error, when in fact this could occur in a perfectly legitimate
situation, no? For instance, if the user entered:

    (gdb) watch A if B == 3

The message could be kept as a debug trace, but it should be more
neutral, IMO, something like:

   address of variable in condition does not match expression being watched.

would provide all the information without suggesting that something
might have gone wrong.

Let me know what you think. I realize that I'm significantly reducing
the scope of the first attempt at getting h/w-accelerated watchpoint
conditions, but I'm hoping it'll help focusing on each issue separately.
I'm perfectly happy to make this feature another patch series on its own,
in parallel to the other hardware-breakpoint patches. We can also take it
one patch at a time. Whatever works best for you.

BTW:

> @@ -186,6 +186,11 @@ struct bp_target_info
>       is used to determine the type of breakpoint to insert.  */
>    CORE_ADDR placed_address;
> 
> +  /* If this is a ranged hardware breakpoint, then we can use this
> +     field in order to store the length of the range that will be
> +     watched for execution.  */
> +  ULONGEST length;
> +

Oops! This should be part of another patch ;-).

-- 
Joel