On Thu, Dec 8, 2011 at 3:28 PM, Phil Muldoon wrote: Thanks for taking a look at the patch, I reply inline >> +     py-bploc.o \ > > Nit, alphabetically ordered please. > >> +     python/py-bploc.c \ > > Ditto. > >> +py-bploc.o: $(srcdir)/python/py-bploc.c >> +     $(COMPILE) $(PYTHON_CFLAGS) $(srcdir)/python/py-bploc.c >> +     $(POSTCOMPILE) > > Could you me the order of this to correspond with the order above. fixed >> +  ** A new method "gdb.Breakpoint.locations" has been added, as well as >> +     the class gdb.BpLocation to provide further details about breakpoint >> +     locations. >> + > > Not sure if this will make it for 7.4.  If not you would have to start a > new section for Python in NEWS. I'm not sure either, I changed it to 7.4 for now, we'll change it later if we can make it in time >> +@findex gdb.locations >> +@defun gdb.locations () >> +Return a tuple containing the @code{gdb.BpLocation} objects (see below) >> +associated with this breakpoint. >> +@end defun > > Perhaps a "containing a sequence of @code{gdb.BpLocation} objects". 'tuple' and 'sequence' sound a bit redundant to me but I'll trust your English and Python skills on that! >> +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 > 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? > @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. > 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": (gdb) info breakpoints Num Type Disp Enb Address What 2 breakpoint keep y 2.1 y 0x08049074 in main at src/host/matrix/main.c:223 inf 2 2.2 y 0x00118125 in main at src/entry.c:38 inf 2 2.3 y 0x08049074 in main at src/host/matrix/main.c:223 inf 1 2.4 y 0x00118125 in main at src/entry.c:38 inf 1 py print "\n".join(["%s -> inf = %d, addr = %s" % (loc, loc.inferior.num, loc.address) for loc in gdb.breakpoints()[0].locations()]) -> inf = 2, addr = 0x8049074 -> inf = 2, addr = 0x118125 -> inf = 1, addr = 0x8049074 -> inf = 1, addr = 0x118125 >> +@defvar BpLocation.address >> +This attribute holds a @code{gdb.Value} object corresponding to the address >> +at which the breakpoint has been inserted. >> +@end defvar > > To make sure I have the logic correctly, if a breakpoint has multiple > addresses then there will be on  BpLocation, for each  address? yes, as described above >> +@defun BpLocation.is_valid () >> +Returns @code{True} if the @code{gdb.BpLocation} object is valid, >> +@code{False} if not.  A @code{gdb.BpLocation} object will become invalid >> +if breakpoint to which is belong is deleted. >> +@end defun > > Typo, last sentence. sentence rephrased >> +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 >> + >> +  /* 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] :) >> + >> +/* 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. >> +static PyTypeObject bploc_object_type; >> + >> +/* Call by free_bp_location when loc is about to be freed.  */ >> + >> +void >> +gdbpy_bplocation_free (struct bp_location *loc) >> +{ >> +  if (loc->py_bploc_obj) >> +    { >> +      loc->py_bploc_obj->loc = NULL; >> +      Py_DECREF (loc->py_bploc_obj); >> +    } >> +} >> + >> +/* Dissociate the bp_location from the Python object.  */ >> + >> +static void >> +bplocpy_dealloc (PyObject *self) >> +{ >> +  bploc_object *self_bploc = (bploc_object *) self; >> + >> +  if (self_bploc->loc != NULL) >> +    self_bploc->loc->py_bploc_obj = NULL; >> + >> +  Py_XDECREF (self_bploc->py_address); >> + >> +  self->ob_type->tp_free (self); >> +} >> + >> +/* Create or acquire a ref to the bp_location object (gdb.BpLocation) >> +   that encapsulates the struct bp_location from 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 >> +  if (loc->py_bploc_obj) >> +    { >> +      Py_INCREF (loc->py_bploc_obj); >> +      return (PyObject *) loc->py_bploc_obj; >> +    } >> + >> +  bploc_obj = PyObject_New (bploc_object, &bploc_object_type); >> +  if (!bploc_obj) >> +    { >> +      PyErr_SetString (PyExc_MemoryError, >> +                       _("Could not allocate BpLocation object.")); >> +      return NULL; >> +    } > > Remove the error setting here.  If a new object cannot be allocated, the > exception will already be set by Python.  So we would be overwriting > that with a more generic message.  So just return NULL. fine >> +/* Python function to get the BP owning this location, is any.  */ > > Typo, "is". fixed >> +/* Python function to get the address of this breakpoint location. The >> +   gdb.Value object will be cached if this is the first access. Returns >> +   NULL in case of failure, with a python exception set.  */ > > Two spaces after ".". > Nit, Python not python. fixed >> +static PyObject * >> +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 > 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 >> + >> +/* Python function which returns the BpLocation objects associated >> +   with this breakpoint.  */ >> + >> +static PyObject * >> +bppy_locations (PyObject *self, PyObject *args) >> +{ >> +  breakpoint_object *self_bp = (breakpoint_object *) self; >> +  PyObject *list; >> +  struct bp_location *loc; >> +  int err; >> + >> +  BPPY_REQUIRE_VALID (self_bp); >> + >> +  list = PyList_New (0); >> +  if (!list) >> +    return NULL; >> + >> +  err = 0; >> +  for (loc = self_bp->bp->loc; loc; loc = loc->next) >> +    { >> +      PyObject *loc_obj =  bplocation_to_bplocation_object (loc); >> +      err = PyList_Append (list, loc_obj); >> +      if (err == -1) >> +        { >> +          Py_DECREF (list); >> +          return NULL; >> +        } >> +      Py_DECREF (loc_obj); >> +    } >> + >> +  return PyList_AsTuple (list); >> +} > > 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 ? > Cheers, > > Phil Thanks for your review, I hope I addressed or discussed everything cordially, Kevin -- 2011-12-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.