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

[Thiago Jung Bauermann <bauerman at br dot ibm dot com>]

On Tue, 2010-12-28 at 15:29 +0000, Pedro Alves wrote:
> 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.

I don't blame you. What I need is beyond what the current resource
accounting code in GDB is capable of doing, and I'm trying to extend it
in the least intrusive way possible. I don't think it's worth rewriting
it, especially after your hints that it should just be removed. This
leads to some hacks in my patch...

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

What's going on here is this: extra_resources_needed_watchpoint is
called at two slightly different contexts:

In hw_watchpoint_used_count, where it is asked to answer about each
breakpoint location in all the existing watchpoints. This is easy, it
just calls target_region_ok_for_hw_watchpoint (discounting one because
of the convention where I make GDB assume that each bp_location consumes
one resource, so any additional ones are "extra" (I'm not very happy
about this convention, but it allows me not to rewrite the resource
accounting code)).

In update_watchpoint, where it needs to answer about the watchpoint
currently being examined by update_watchpoint. The problem here is that
the watchpoint doesn't have any bp_location yet. The bp_locations will
be created just a few moments later. So at this point
extra_resources_needed_watchpoint just says that no extra resources are
needed (because bl is NULL).

This hack is needed because the resource accounting code examines
high-level breakpoints to decide about resource utilization, but it
should be using breakpoint locations instead. To make it examine
breakpoint locations, the resource accounting functions would have to be
called at different places from where they are called now, where
changing from a hardware watchpoint to a software one, or refusing to
create a hardware breakpoint (for example) would be cumbersome, since
breakpoint locations are created late in the breakpoint/watchpoint
creation process.

It can be done, but it's a fair amount of work which may not be
worthwhile given that the current disposition is to get rid of this
mechanism altogether. Perhaps I should just change ppc-linux-nat.c to
always accept hardware breakpoints and watchpoints, and then just fail
later if it overcommitted (as other ports like i386 do)? Then I believe
this patch would be simpler in this regard.

> > +
> > +               /* 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,".

It wouldn't be exactly the same. In this patch I changed
hw_watchpoint_used_count to iterate over the breakpoint locations
instead of the breakpoints (which is what actually makes sense). But
since the watchpoint currently being examined by update_watchpoint
doesn't have any location, then it wouldn't be accounted for.

I could change update_watchpoint to create the breakpoint locations
before doing the resource accounting so that I could remove the hacks
described here, but it was not a trivial change (but not magic either)
and I wondered whether it was worth it.

> >                 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 looked at different places in GDB which determined the array size, if
available, and there are different ways to do it. I chose to use the
code from c-lang.c:c_get_string, which I wrote. :-)

gdbtypes.c:create_array_type says:

  TYPE_NFIELDS (result_type) = 1;
  TYPE_FIELDS (result_type) =
    (struct field *) TYPE_ZALLOC (result_type, sizeof (struct field));
  TYPE_INDEX_TYPE (result_type) = range_type;

So I believe it is correct.

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

That information is wrong, since the array is actually of an incomplete
type. But it would not have been a problem for is_scalar_type_recursive
since it only considers arrays with exactly one element.
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center