Re: [patch 2/3] Implement support for PowerPC BookE ranged watchpoints

[Pedro Alves <pedro@codesourcery.com>]

On Monday 27 December 2010 21:19:03, Thiago Jung Bauermann wrote:

> @@ -1394,25 +1397,33 @@ update_watchpoint (struct breakpoint *b, int reparse)
>         if ((b->type == bp_watchpoint || b->type == bp_hardware_watchpoint)
>             && reparse)
>           {
> -           int i, mem_cnt, other_type_used;
> -
> -           /* We need to determine how many resources are already used
> -              for all other hardware watchpoints to see if we still have
> -              enough resources to also fit this watchpoint in as well.
> -              To avoid the hw_watchpoint_used_count call below from counting
> -              this watchpoint, make sure that it is marked as a software
> -              watchpoint.  */
> -           b->type = bp_watchpoint;
> -           i = hw_watchpoint_used_count (bp_hardware_watchpoint,
> -                                         &other_type_used);
> -           mem_cnt = can_use_hardware_watchpoint (val_chain);
> -
> -           if (!mem_cnt)
> +           int reg_cnt;
> +
> +           reg_cnt = can_use_hardware_watchpoint (val_chain, b->exact);
> +
> +           if (!reg_cnt)
>               b->type = bp_watchpoint;
>             else
>               {
> -               int target_resources_ok = target_can_use_hardware_watchpoint
> -                 (bp_hardware_watchpoint, i + mem_cnt, other_type_used);
> +               int i, other_type_used, target_resources_ok;
> +               enum bptype orig_type;
> +
> +               if (b->ops && b->ops->extra_resources_needed)
> +                 reg_cnt += b->ops->extra_resources_needed (b, NULL);

I'm taking a look at the resource accounting changes, which I
mostly ignored before, and I'm confused.  can_use_hardware_watchpoint
(above) calls target_region_ok_for_hw_watchpoint, which has been changed
to return the number of resources normally necessary.  Yet,
extra_resources_needed_watchpoint calls target_region_ok_for_hw_watchpoint
as well (discounting 1, it seems).  Aren't we ending up with gdb
thinking it needs more resources than are really necessary?

> +
> +               /* We need to determine how many resources are already used
> +                  for all other hardware watchpoints to see if we still have
> +                  enough resources to also fit this watchpoint in as well.
> +                  To avoid the hw_watchpoint_used_count call below from
> +                  counting this watchpoint, make sure that it is marked as a
> +                  software watchpoint.  */
> +               orig_type = b->type;
> +               b->type = bp_watchpoint;

Something I notice: It sounds like if you remove this hack above, 

> +               i = hw_watchpoint_used_count (bp_hardware_watchpoint,
> +                                             &other_type_used);

then this above accounts for the new watchpoint too, so...

> +
> +               target_resources_ok = target_can_use_hardware_watchpoint
> +                 (bp_hardware_watchpoint, i + reg_cnt, other_type_used)

... this would just be ", i,", instead of ", i + reg_cnt,".

>                 if (target_resources_ok <= 0)
>                   b->type = bp_watchpoint;
>                 else

> > What does the "TYPE_NFIELDS (t) == 1" do ?
> 
> TYPE_NFIELDS (t) == 1 && TYPE_CODE (TYPE_INDEX_TYPE (t)) == TYPE_CODE_RANGE
> is a way to determine that the given array type has known bounds.

Didn't know about that.  I thought TYPE_NFIELDS only made sense
for structs/classes/unions.  My grep foo isn't finding other similar uses.

I tried:

 compunit1.c:
 extern int array[];

 void foo ()
 {
 ...
 }

 compunit2.c:

 int array[];

and in foo, ptype array gave me array[0], TYPE_NFIELDS==1.

> > Should be "Use of exact watchpoints is ...", I think?  Thus getting rid
> > of the small lie that not all watchpoints (in the user perspective) will
> > use just one debug register, leaving the just one debug register
> > issue explanation to the help string (as you already have).
> 
> Good point. Changed to:

(...)

Thanks.

-- 
Pedro Alves