This is the mail archive of the archer@sourceware.org mailing list for the Archer project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]