Re: [patch] Add watchpoint support to save_breakpoints.py (and add supporting methods to python-breakpoint.c)

[Phil Muldoon <pmuldoon@redhat.com>]

On 07/31/2009 07:21 PM, Tom Tromey wrote:
> Phil>  +@defivar Breakpoint expression
> Phil>  +This attribute holds the breakpoint expression, as specified by
> Phil>  +the user.  It is a string.  This attribute is not writable.
> Phil>  +@end defivar
>
> This should mention that the expression is only valid for watchpoints.
>    

Ok.

> And, I think this patch should update the documentation to mention that
> the location field is only valid for breakpoints.
>
> Then, the corresponding getter functions should enforce this constraint.
>    

Ok.

> It seems a little strange to have both address and expression.  I wonder
> if we should just make up a new attribute name encompassing both.
>    


The original patch I wrote did this, but I elected to just follow 
convention and mirror GDB as closely as possible in this case. Not that 
I am advocating this position as preferable, but it was just a choice at 
the time. I can't think of a case where one would field would be 
explicitly required; can you?


> It seems like there should be a BP_ constant for a temporary breakpoint.
> What do you think?
>    


There are lots of BP_ constants, but I only added what I needed in the 
watchpoint context. I can add the BP_ constant for a temporary 
breakpoint or I can add all of the sane BP_ constants if needed? What do 
you think here?


> Phil>  +  {"BP_NONE", bp_none},
> Phil>  +  {"BP_BREAKPOINT", bp_breakpoint},
> Phil>  +  {"BP_WATCHPOINT", bp_watchpoint},
> Phil>  +  {"BP_HARDWARE_WATCHPOINT", bp_hardware_watchpoint},
> Phil>  +  {"BP_READ_WATCHPOINT", bp_read_watchpoint},
> Phil>  +  {"BP_ACCESS_WATCHPOINT", bp_access_watchpoint},
>
> Add a space after each "{" and before each "}".
>    

Ok.

> Phil>  +  BPPY_REQUIRE_VALID ((breakpoint_object *) self);
> Phil>  +  str = ((breakpoint_object *) self)->bp->exp_string;
>
> The repeated casts here look ugly.
>
>    


I'm guilty here of cut and paste coding from the other bppy_get * 
functions. I'll fix up this cast, and others.


> I agree that omitting catches and other thing is ok.  I wonder, however,
> whether we should simply skip those in gdbpy_breakpoint_created.
>    


In the longer term I really would like to support catchpoints. 
Especially with Sergio's catch syscall feature. I think for now, they 
should be skipped. They are not supported, and there is no need to alloc 
space for them if they are never touched. What if it should be very 
restrictive, there are lots of random internal/maintenance breakpoints 
we don't support in the save function.

Regards

Phil