Re: [PATCH] Add bp_location to Python interface

[Kevin Pouget <kevin.pouget@gmail.com>]

On Tue, Jan 10, 2012 at 3:50 PM, Kevin Pouget <kevin.pouget@gmail.com> wrote:
> On Mon, Jan 9, 2012 at 6:23 PM, Eli Zaretskii <eliz@gnu.org> wrote:
>>> From: Kevin Pouget <kevin.pouget@gmail.com>
>>> Date: Mon, 9 Jan 2012 12:46:30 +0100
>>> Cc: pmuldoon@redhat.com
>>>
>>> ping
>>
>> Sorry for missing it the first time.
>
> no problem, thanks for your feedbacks, I replied inline
>
>>> --- a/gdb/NEWS
>>> +++ b/gdb/NEWS
>>> @@ -9,6 +9,12 @@
>>>  * The binary "gdbtui" can no longer be built or installed.
>>>    Use "gdb -tui" instead.
>>>
>>> +* Python scripting
>>> +
>>> +  ** A new method "gdb.Breakpoint.locations" has been added, as well as
>>> +     the class gdb.BpLocation to provide further details about breakpoint
>>> +     locations.
>>> +
>>
>> This is OK.
>>
>>> +@defun gdb.locations ()
>>> +Return a tuple containing a sequence of @code{gdb.BpLocation} objects
>>> +(see below) associated with this breakpoint.  A breakpoint with no location
>>> +is a pending breakpoint (@xref{Set Breaks, , pending breakpoints}).
>>
>> @pxref, not @xref, as this cross-reference is inside parentheses.
>>
>>> +A breakpoint location is represented with a @code{gdb.BpLocation} object,
>>                                        ^^^^
>> "by"
>
> fixed
>
>>> +which offers the following attributes (all read only) and methods.
>>> +Please note that breakpoint locations are very transient entities in
>>> +@value{GDBN}, so one should avoid keeping references to them.
>>
>> I'd use "volatile" instead of "transient".
>
> fixed
>
>> Also, perhaps a sentence
>> or two about _why_ the locations are volatile would help.  E.g., if I
>> knew what actions invalidate locations, I could avoid them and leave
>> the locations valid for longer.
>
> That's a point Phil also noted so I've try to explain it a bit
> further, although my knowledge about the internal mechanisms involved
> is quite limited. A maintainer will have to confirm these words. I
> also mentioned explicitly that it's the *objects* related to the
> locations which are volatile, because the location (ie, address)
> itself actually doesn't change:
>
>> Please note that breakpoint location objects are very volatile entities in
>> @value{GDBN}, so one should avoid keeping references to them.  They can be
>> destructed and recreated on any breakpoint creation and deletion, shared-
>> library event, inferior function call, @dots{}
>
>
>
>>> +owns this location.  This attribute is not writable.  From an implementation
>>> +point of view, there is a @code{1 ... n} relation between a breakpoint and
>>
>> "1 ... n" here means one to many?  If so, I suggest to say that
>> literally.
>>
>> In any case, "@code{1 ... n}" is not a good idea, because of the
>> whitespace and because we use @dots{} instead of literal periods.  If
>> "one to many" is not what you meant, I can suggest how to mark up
>> whatever needs to be written here.
>
> No, "one-to-many" is what I meant, I've changed it (spelled with the
> dashes, right?)
>
>>> +This attribute holds a reference to the @code{gdb.Inferior} inferior object
>>
>> I'd drop the second instance of "inferior", it looks redundant.
>
> dropped
>
>>> +@defun BpLocation.is_valid ()
>>> +Returns @code{True} if the @code{gdb.BpLocation} object is valid,
>>> +@code{False} if not.  A @code{gdb.BpLocation} object may be invalidated by
>>> +GDB at any moment for internal reasons.  All other @code{gdb.BpLocation}
>>> +methods and attributes will throw an exception if the object is invalid.
>>
>> @value{GDBN} instead of "GDB".
>>
>> In any case, the last 2 sentences sound scary: I could interpret them
>> as meaning I cannot trust the locations at all.  If that is indeed so,
>> what use are they?
>
> that's already discussed above, but I don't want you to be scared, so
> let me explain what I meant:
> it's not "at any moment", but rather "after any call to GDB's Python
> interface". We may want to say that it's only breakpoint or
> execution-related calls, but _I_ can't ensure that this is true, and
> it 'might' change in the future:
>
>> A @code{gdb.BpLocation} object may be invalidated during
>> any call to @{GDB}'s API for internal reasons (for instance, but not limited to,
>> breakpoint or execution-related mechanisms).

Sorry there is a missing 'value' in @{GDB} here and in the patch, it
will be fixed for the next submission.

And a typo,
> @defvar BpLocation.owner
> ... even if two breakpoints are set on at same address.
I removed the redundant 'on'.


Kevin

> (I'm not sure what's the right Python term here for 'mechanisms',
> reading/writing an attribute may trigger internal functions, so 'call'
> or 'function' seem not to suit very well)
>
>
> Cordially,
>
> Kevin