Re: [patch 1/2] Convert hardware watchpoints to use breakpoint_ops

[Thiago Jung Bauermann <bauerman@br.ibm.com>]

On Mon, 2010-11-08 at 10:43 -0800, Joel Brobecker wrote:
> Just for the record, I've started looking at both patches, and already
> spent a good hour on them. But I think that I should give myself some
> time to to let every piece sink in, before I make an official review.

Thank you very much! I'll be glad to clarify any question that may come
up during your review of the patch. No need to think too hard about what
the patch does in some obscure part, I'll be glad to explain. :-)

> I urge another one of us to look at this change, because it's not
> completely obvious. In particular, I think it was my suggestion
> that prompted the move to using breakpoint_ops for watchpoints.
> But it's not obvious that there is a gain.  I think the gain shows up
> in the second patch, where we avoid having:

Yes, I think the 2nd patch became cleaner when I modified it to use
breakpoint_ops. And yes, it was your idea. :-)

IMHO the code would be even cleaner if I/we ported the existing
breakpoints and watchpoints code completely to breakpoint_ops,
eliminating functions like print_it_typical. It would make this part of
GDB more object-oriented. Though it would perhaps increase the total
line count instead of decreasing it, so it's not a clear win.
 
>   1. To define new kinds for range and mask watchpoints
> 
>   2. To program dispatching manually, Eg:
> 
>      if (bp->print_one)
>        bp->print_one (...)
>      else if (bp->type == hw_watchpoint)
>        print_one_normal_watchpoint (...)
>      else if (bp->type == range_watchpoint)
>        print_one_range_watchpoint (...)
>      else if (bp->type == mask_watchpoint)
>        [...]

It also avoids adding special cases just for masked and ranged
watchpoints. Instead, I added a method to breakpoints_op and if it's
defined, I just call it. For instance, in update_watchpoint:

+               if (b->ops && b->ops->extra_resources_needed)
+                 mem_cnt += b->ops->extra_resources_needed (b);


-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center