On Tue, Dec 13, 2011 at 4:28 PM, Kevin Pouget wrote: > > On Fri, Dec 9, 2011 at 3:10 PM, Phil Muldoon wrote: > > Kevin Pouget writes: > > > >> On Thu, Dec 8, 2011 at 3:28 PM, Phil Muldoon 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, ping patch refreshed against current HEAD Thanks, Kevin -- 2012-01-09 Kevin Pouget 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.