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

Ulrich Weigand
2011-09-13T15:23:06Z

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