Re: Watchpoint resource accounting broken (Re: [5/6] breakpoints_ops for all kinds of breakpoints: new watchpoints instance type

["Ulrich Weigand" <uweigand@de.ibm.com>]

Pedro Alves wrote:
> Does the patch below work?  Never consider the current watchpoint when
> going over the breakpoint list counting resources, and then 
> add the resources of the current watchpoint on top.  This way we
> don't have to care of the current watchpoint being on the list yet or
> not.  As bonus, we no longer have to frog the watchpoint's type before
> knowing if it'll fit.

Yes, this patch looks good to me, and does fix the problem I've
been seeing.  Thanks!

> On Monday 12 September 2011 16:16:53, Edjunior Barbosa Machado wrote:
> > I noticed this problem too and was considering use "i + reg_cnt" instead
> > of only "i" when calling hw_watchpoint_used_count() (actually, I saw gdb
> > used to use this previously).
> > However, with this change, due to the same problem with the watchpoint
> > added to breakpoint list that Ulrich mentioned, watchpoints added before
> > run the inferior will not work.
> 
> Can you expand on this?  I don't think I'm understanding that problem.

In addition to what Edjunior replied, this reminds me that there is now
some unnecessary code duplication in the can_use_hardware_watchpoint
routine:

          reg_cnt = can_use_hardware_watchpoint (val_chain);
[snip]
              /* Add in the resources needed for B.  */
              i += hw_watchpoint_use_count (&b->base);

The return value of this hw_watchpoint_use_count call ought to be
always equal to reg_cnt; and in fact the whole logic in
can_use_hardware_watchpoint ought to be redundant with what
update_watchpoint does anyway.

However, this is not completely true:

1. can_use_hardware_watchpoint respects the can_use_hw_watchpoints flag

2. can_use_hardware_watchpoint takes note of sub-expressions that
   cannot be implemented via HW watchpoints (e.g. lval_register)
   [ -- this seems to be incomplete anyway these days ]

It seems to me that it would be preferable to integrate these missing
checks into the code in update_watchpoint, and then remove the
can_use_hardware_watchpoint routine ...

[ But that's clearly a follow-on cleanup, and shouldn't prevent your
  bug-fix patch from going in now. ]

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com