[PATCH] Add bp_location to Python interface
Kevin Pouget
kevin.pouget@gmail.com
Tue Dec 13 15:55:00 GMT 2011
On Fri, Dec 9, 2011 at 3:10 PM, Phil Muldoon <pmuldoon@redhat.com> wrote:
> Kevin Pouget <kevin.pouget@gmail.com> writes:
>
>> On Thu, Dec 8, 2011 at 3:28 PM, Phil Muldoon <pmuldoon@redhat.com> wrote:
>>
>
>>>> +A breakpoint location is represented with a @code{gdb.BpLocation} object,
>>>> +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.
>>
>>>
>>> While it is good to note that, I'm not sure what we are explaining here
>>> other than when the breakpoint is deleted, all location objects that are
>>> associated with that object are invalid too. Or, are you noting we
>>> should allow the user to interact after the fact? If that is the case,
>>> what is "is_valid" for? Why note the transient nature of the object?
>>
>>> I'm not sure what we are explaining here other than when the breakpoint is deleted
>>
>> that's what I wanted to emphasize here, bp_locations are cleaned/reset
>> more often than 'high-level' breakpoints are removed. My knowledge
>> about that is quite limited, but for instance this (cleaned up) stack:
>>
>> #0 gdbpy_bplocation_free py-bploc.c:60
>> #1 free_bp_location (loc=) breakpoint.c:5685
>> #2 decref_bp_location (blp=) breakpoint.c:5707
>> #3 update_global_location_list (should_insert=1) breakpoint.c:10575
>> #4 update_breakpoint_locations (b=, sals=..., sals_end=...)
>> breakpoint.c:11787
>> #5 breakpoint_re_set_default (b=) breakpoint.c:11937
>> #6 breakpoint_re_set_one (bint=) breakpoint.c:11968
>> #8 breakpoint_re_set () breakpoint.c:11992
>> #9 solib_add (pattern=0x0, from_tty=0, target=, readsyms=1) solib.c:926
>> #10 bpstat_what (bs_head=) breakpoint.c:4487
>> #11 handle_inferior_event (ecs=) infrun.c:4394
>> #12 wait_for_inferior ()
>>
>> shows that bplocations might be removed when a shared library is loaded
>
> Good point, I did not think of this scenario. Looking at several
> sections of GDB code, update_global_location_list can just be called
> whenever.
>
>>> If the breakpoint is deleted and the user still has reference to a
>>> location object, I think we should just run a validation routine and
>>> refuse to do anything but raise an exception at that point (like
>>> is_valid, but triggered for all function/attribute calls).
>>
>> hum, I think that's what I already do in the code :)
>>
>> I've updated the gdb.BpLocation.is_valid documentation, maybe it's clearer?
>
> Yeah, I saw it later. Forgot to delete that comment!
>
>>> @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.
>>> @end defun
>>
>>>> +
>>>> +@defvar BpLocation.owner
>>>> +This attribute holds a reference to the @code{gdb.Breakpoint} object which
>>>> +owns this location.
>>>> +@end defvar
>>
>>> Note which attributes are read only/writable. This and others.
>> I specify a few lines above that all the attributes are read only, but
>> I've noted it on each attribute as well.
>
> Right, but there is no guarantee that the user will read the entirety
> of that section's documentation. So we always put attribute access rights
> in the attribute section.
ok, I thought that as soon as it was clearly mentioned somewhere, that
was enough. But it doesn't cost anything to me to write it for each
attribute, so no problem!
>>> Also many different breakpoints can be at one location. I'm not sure if it
>>> is worth pointing out here that "this breakpoint" only owns the
>>> location insofar as the scope of that breakpoint (there could be other
>>> breakpoints set to that location).
>>
>> from an implementation point of view, the is a BP <1--------n>
>> BpLocation relation, even if two locations have the same address.
>>
>> Also, I think that the end-users won't have problems to understand
>> these BpLocations, as they're already exposed with "info breakpoints":
>
> I was purely talking from a scripting point of view, but ok. My view
> is, if the user has to run experiments to collect data on the actual
> behavior, then our documentation needs to be a little better.
yes, no doubt about that!
> You could just put in that "from an implementation point of view" paragraph in
> the documentation somewhere?
I've added it to 'BpLocation.owner' description:
@defvar BpLocation.owner
This attribute holds a reference to the @code{gdb.Breakpoint} object which
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
its locations, even if two breakpoints are set on at same address.
>>>> +struct bploc_object
>>>> +{
>>>> + PyObject_HEAD
>>>> +
>>>> + /* The location corresponding to this py object. NULL is the location
>>>> + has been deleted. */
>>>> + struct bp_location *loc;
>>>
>>> Typo in the comment "NULL is the location has been deleted.". Also nit
>>> pick shouldn't it be "The location corresponding to a gdb.Breakpoint
>>> object"?
>>
>> typo fixed, but not the nit: it's the same idea as breakpoint,
>> if the 'backend' breakpoint is deleted, 'struct breakpoint_object . bp' is NULL,
>> if the 'backend' location is deleted, 'struct bploc_object . loc' is
>> NULL
>
> My objection was to "py object", I would just find it clearer to name
> the object. Also, the explanation above would go a long way for future
> hackers to understand the relationship. Maybe add that to the code too?
You're right, it looks obvious to me, but it would take time to figure
out by itself.
Here are a few more bits of comments:
/* The location corresponding to this gdb.BpLocation object. It's the same
idea as gdb.Breakpoint, if the 'backend' location is deleted, LOC is
set to NULL. No reference to the location is owned here (in terms of
ref. counting) in order not to change the internal behavior. */
struct bp_location *loc;
>>
>>>> +
>>>> + /* 1 if the owner BP has been deleted, 0 otherwise. */
>>>> + int invalid_owner;
>>>> +
>>>> + /* Cache for the gdb.Value object corresponding to loc->address. */
>>>> + PyObject *py_address;
>>>> +};
>>>
>>> I'm not sure if breakpoint locations can change. I do not think so, but
>>> why do we need to cache loc->address?
>>
>> I assumed that it can't change, but didn't actually investigate all
>> the implementation.
>> My feeling is "caching is easy, creating a Py object has a cost, so
>> let's cache!", but I've got no idea about the actual cost, so I'll
>> change it if someone insists [and convinces me] :)
>
> I don't know enough about breakpoint locations to be able to advise you,
> but what does update_global_location_list do? Just shuffle add/delete
> locations to the list? I am not against caching. Just not sure why we
> need it. I tend towards the conservative with things like these, if
> only purely because we keep missing reference counts in the existing
> code.
I somehow managed to convinced myself to remove this address caching,
1/I'm not sure about the actual py object creation, certainly quite low,
2/BpLocation objects have a short lifespan, so the cached addresses
would be discarded quite frequently
so all in all, I can't see anymore any relevant reason to cache it :)
>>>> +
>>>> +/* Require that LOCATION be a valid bp_location; throw a Python
>>>> + exception if it is invalid. */
>>>> +#define BPLOCPY_REQUIRE_VALID(Location) \
>>>> + do { \
>>>> + if ((Location)->loc == NULL) \
>>>> + return PyErr_Format (PyExc_RuntimeError, \
>>>> + _("BpLocation invalid.")); \
>>>> + } while (0)
>>>
>>> I prefer error messages a little more descriptive. That is just a
>>> personal thing of mine, but "BpLocation invalid" would seem cryptic when
>>> thrown in the context of a script.
>>
>> I'm not sure how I could improve it, at this point (checking validity
>> at the beginning of all the attributes), we just know that the backend
>> location has been freed, but we don't know why.
>
> "BpLocation object, breakpoint location is NULL". Or something like that.
I'm still not sure, I think that when I read "BpLocation invalid", I'd
take a look at the 'is_valid' method description, and understand
that "A @code{gdb.BpLocation} object may be invalidated by GDB at any
moment for internal reasons."
'breakpoint location is NULL' sounds like an internal error to me ...
Maybe "BpLocation invalid: internal location object freed up/removed by GDB." ?
>>>> +PyObject *
>>>> +bplocation_to_bplocation_object (struct bp_location *loc)
>>>> +{
>>>> + bploc_object *bploc_obj;
>>>> +
>>>> + gdb_assert (loc != NULL);
>>>
>>> Is this a fatal error that we need to shutdown GDB for? gdb_assert
>>> seems pretty strong from an API point of view.
>>
>> hum, I'm not sure here,
>> I wrote this line to actually shutdown GDB instead of segfaulting on
>> the next line,
>> for instance 'py-pspace.c::pspace_to_pspace_object' and
>> 'py-objfile.c::objfile_to_objfile_object" will just segfault if
>> there're called with a NULL parameter.
>>
>> throwing an Python/GDB exception wouldn't make much sense to me
>> either, as there end-user won't be able to deal with it
>
> Neither would killing GDB. I almost never use gdb_assert because of the
> possible negative user implications. Can you discern that GDB has
> entered an unstable state because a function has received a NULL
> argument for a bp_location? If you cannot answer "yes" with a high
> degree of certainty, it is my opinion we should raise an exception and
> return NULL. We can make sure in the Python API that, if this is the
> case, we will return the exception to the script boundary for the script
> to deal with. From an API point-of-view I think we should defer to
> script authors the opportunity to deal with and decide what to do in
> this scenario. If GDB is truly unstable, then I agree, assert is
> correct. I think the other two cases are bugs.
I get your point and quite agree with you, the only thing is that
we're talking about 'gdb_assert' and not 'assert' itself.
> Neither would killing GDB.
and here's my point, 'gdb_assert' does't kill GDB, but raises a
warning message asking the user if he wants to quit or continue.
My feeling about *assert is that it's a development/beta-test
features, which might/should be turned into noop for production.
> From an API point-of-view I think we should defer to script authors the opportunity to deal with
I don't think so, because (from my developer point of view) I see it
as very improbable that this situation would occur, or even impossible
if I were a good programmer!
I don't think that API should cover internal 'buggy' situations,
otherwise they would be endless!
> The Maintainers may differ on this, wait for their opinion.
yes, I'm curious about different point of views
>>>> +bplocpy_get_address (PyObject *self, void *closure)
>>>> +{
>>>> + bploc_object *self_bploc = (bploc_object *) self;
>>>> +
>>>> + BPLOCPY_REQUIRE_VALID (self_bploc);
>>>> +
>>>> + if (!self_bploc->py_address)
>>>> + {
>>>> + /* Get the address Value object as a void *. */
>>>> + volatile struct gdb_exception except;
>>>> + struct type *void_ptr_type;
>>>> + struct value *val = NULL ; /* Initialize to appease gcc warning. */
>>>> +
>>>> + TRY_CATCH (except, RETURN_MASK_ALL)
>>>> + {
>>>> + void_ptr_type = lookup_pointer_type (
>>>> + builtin_type (python_gdbarch)->builtin_void);
>>>> + val = value_from_pointer (void_ptr_type, self_bploc->loc->address);
>>>> + }
>>>> + GDB_PY_HANDLE_EXCEPTION (except);
>>>> +
>>>> + self_bploc->py_address = value_to_value_object (val);
>>>> + Py_XINCREF (self_bploc->py_address);
>>>> + }
>>>> + Py_XINCREF (self_bploc->py_address);
>>>
>>> I don't really mind it, but I prefer explicit return NULL when dealing
>>> with cases of exceptions. I find the other logic hard to read. This is
>>> not a request for a change. Is there a case where py_address will be
>>> NULL? Yes, there is, value_to_value_object can return NULL. If it
>>> returns NULL, then there is an exception set. I much prefer to exit
>>> then and there, other the conditional XINCREF step, and returning at the
>>> function exit. Still, this is just a stylistic thing, and probably
>>> personal thing. The second XINCREF can just be a plain old INCREF as we
>>> already tested for NULL.
>>
>> makes sense to me, my "single return point" idea doesn't stand against
>> error handling
>
> Note this is purely a personal style issue, there was nothing wrong with
> the correctness of your code.
sure, I try to make it clear when I don't agree with the suggestion;
this one was perfectly fine,
I think that's actually the way I wrote it at the first time :)
>>> This brings me to the address cache argument. Is it worthwhile managing
>>> the cache increment counts instead of just returning the address each
>>> time? I ask as I am not sure if you have done any metrics that indicate
>>> this is a slow operation.
>>
>> no, I didn't run any measurement so far; I'll wait for a maintainer
>> point of view about that
>
> Ok.
>
>>> We need to make sure that the this is not a watchpoint.
>>
>> why not?
>> I don't know much about watchpoints, but as per my quick
>> investigations, they share the same high-level structure (struct
>> breakpoint), and they seem to use bp_location the same way ?
>
> I'm not sure as watchpoints are based on expressions. Maybe it is ok,
> but note that watchpoints are removed and reinserted on scope of the
> expression. It might be fine. A few tests would prove the issue.
I'll try to build some test to ensure that it's fine
thanks for your comments and reviews,
Kevin
--
2011-12-13 Kevin Pouget <kevin.pouget@st.com>
Add bp_location to Python interface
* Makefile.in (SUBDIR_PYTHON_OBS): Add py-bploc.o
(SUBDIR_PYTHON_SRCS): Add python/py-bploc.c
Add build rule for this file.
* breakpoint.h (struct bploc_object): Forward declaration.
(struct bp_location): Add py_bploc_obj.
* breakpoint.c (free_bp_location): Call gdbpy_bplocation_free.
* python/py-bploc.c: New file.
* python/py-breakpoint.c (bppy_locations): New function.
(breakpoint_object_methods): New method binding: locations().
* python/python-internal.h (bploc_object): New typedef.
(bplocation_to_bplocation_object): New prototype.
(gdbpy_initialize_bplocation): Likewise.
* python/python.c (gdbpy_bplocation_free): New empty stub.
(_initialize_python): Call gdbpy_initialize_bplocation.
* python/python.h (gdbpy_bplocation_free): New prototype.
doc/
Add bp_location to Python interface
* gdb.texinfo (Breakpoints In Python): Document
gdb.Breakpoint.locations and gdb.BpLocation.
testsuite/
Add bp_location to Python interface
* gdb.python/py-breakpoint.exp: Test gdb.BpLocation.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-bp_location-to-Python-interface.patch
Type: text/x-patch
Size: 22545 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/gdb-patches/attachments/20111213/f5bc243d/attachment.bin>
More information about the Gdb-patches
mailing list