Re: [patch][python] Add breakpoint support.

[Phil Muldoon <pmuldoon at redhat dot com>]

On 03/29/2010 03:26 PM, Eli Zaretskii wrote:
>> Date: Mon, 29 Mar 2010 14:51:31 +0100
>> From: Phil Muldoon <pmuldoon@redhat.com>

> Thanks.  I have a few comments to the documentation part.
> 
>> 	* breakpoint.c (condition_command): Simplify.  Move condition
>> 	setting code too ...
>                      ^^^
> A typo.


Thanks.


>> +@tindex gdb.Breakpoint
>> +@tindex Breakpoint
> 
> You already added index entries with the same names, albeit in
> different letter-case, elsewhere in the manual.  Why is it a good idea
> to have these here as well?


Not sure what you mean here, can you please elaborate?


>> +Create a new breakpoint.  @var{expression} is a string naming the
>> +location of the breakpoint, or an expression that defines a
>> +watchpoint.
> 
> This sounds like the watchpoint expression is not a string.  

It is, but this comment is referring to the argument being a string.
So for a watchpoint the term "expression" here just uses the
vernacular that GDB uses around watchpoints and breakpoints.  So the
actual call would still look something like:


python foo = gdb.Breakpoint("foo_var", type=gdb.BP_WATCHPOINT, wp_type=WP_WRITE)


>  How about this variant:
> 
>   @defmethod Breakpoint __init__ spec @r{[}type@r{]} @r{[}wp_class@r{]}
>   ...
>   @var{spec} specifies the location of a breakpoint or the expression
>   to watch for a watchpoint.


That's fine with me, and clearer.


>> +either: @var{BP_BREAKPOINT} or @var{BP_WATCHPOINT}.  If
> 
> BP_BREAKPOINT and BP_WATCHPOINT should be in @code, not in @var.  They
> stand for themselves, not for something else (contrast with your
> @var{expression} above).


Ok, thanks.


>> +If no type is provided, it is presumed to be @var{BP_BREAKPOINT}.
> 
> "@var{type} defaults to @code{BP_BREAKPOINT}." is much shorter and
> more clear, I think.


Ok, thanks.


>> +The optional @var{wp_class} argument defines the type of watchpoint to
>                                                     ^^^^
> Since you call it "wp_class", why not use "class" here instead of
> "type"?  You already have @var{type} that has a different role.


Thanks for catching that, it is clearer.


>> +create, if @var{type} is defined as @var{BP_WATCHPOINT}. If a 
>                                                           ^^
> Two spaces, please (here and elsewhere).


Grrr. I really need to setup my FILL settings with emacs
properly.  Sorry for the period-two-space-noise.


 
>> +If a watchpoint type is not provided, it is assumed to be a @var{WP_READ}
>> +type. 
> 
> ?? Really?  WP_WRITE sounds a more logical choice, as it's the default
> watchpoint type in GDB.


I do not have any strong feelings here.  I can make it write by
default if you think it would be more sensible.


>> +@defmethod Breakpoint is_valid
>> +Return @code{True} if this @code{Breakpoint} object is valid,
>> +@code{False} otherwise.  A @code{Breakpoint} object can become invalid
>> +if the user deletes the breakpoint.  In this case, the object still
>> +exists, but the underlying breakpoint does not.
> 
> What if execution leaves the scope in which the watchpoint is defined:
> does this make it invalid?

No, the watchpoint is still valid (it still exists, despite being out
of scope).  The validity check just checks for the existence of the "struct
bp" that is part of the Python breakpoint.  But, on that note, point
well taken; it needs to be explained.

 
>> +This attribute holds the breakpoint's number -- the identifier used by
>                                                 ^^
> 3 dashes in a row here, please.
> 
>> +This attribute holds the breakpoint's type -- the identifier used to
> 
> Ditto.


Ok, thanks.

 
>> +@findex BP_READ_WATCHPOINT
>> +@findex gdb.BP_READ_WATCHPOINT
>> +@item BP_READ_WATCHPOINT
>> +Hardware assisted read watchpoint.
>> +
>> +@findex BP_ACCESS_WATCHPOINT
>> +@findex gdb.BP_ACCESS_WATCHPOINT
>> +@item BP_ACCESS_WATCHPOINT
>> +Hardware assisted access watchpoint.
>> +@end table
> 
> If we have these BP_* constants, why do we also need the WP_*
> constants (and the wp_class argument)?

These refer to the possible types of breakpoints that exist in GDB.
>From an API point of view it cannot guarantee that the watchpoint
created from the Python API will be a hardware watchpoint.  So I
wanted to avoid using these in the watchpoint creation.  But if the
user checks the type after creation, and if the watchpoint is a
hardware assisted watchpoint, one of these will be the return.  On
that note, I need to explain the hardware assisted issue in the
section earlier on breakpoint creation.

 
>> +@defivar Breakpoint location
>> +This attribute holds the location of the breakpoint, as specified by
>> +the user.  It is a string.  If the breakpoint does not have a location
>> +(that is, it is a watchpoint) an exception will be raised.
> 
> Is it wise to raise an exception?  Why not return something sensible
> instead?
> 
>> +the user.  It is a string.  If the breakpoint does not have an
>> +expression (the breakpoint is not a watchpoint) an exception will be
>> +raised.
> 
> Same here.


Would returning Py_NONE work? 


> 
>> +@defivar Breakpoint commands
>> +This attribute holds the commands attached to the breakpoint.  If
>> +there are commands, this returns a string holding all the commands,
>                        ^^^^^^^^^^^^
> An attribute cannot "return" anything, it holds a value, right?
> 

Yes, quite true. Will fix

Cheers,

Phil