Re: [PATCH] Add bp_location to Python interface

[Phil Muldoon <pmuldoon at redhat dot com>]

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.

>> 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.  You could
just put in that "from an implementation point of view" paragraph in
the documentation somewhere?


>>> +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?

>
>>> +
>>> + Â/* 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. 


>>> +
>>> +/* 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.

>>> +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.

The Maintainers may differ on this, wait for their opinion.

>>> +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.

>> 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.

Cheers,

Phil