From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18121 invoked by alias); 8 Dec 2011 14:28:55 -0000 Received: (qmail 18102 invoked by uid 22791); 8 Dec 2011 14:28:50 -0000 X-SWARE-Spam-Status: No, hits=-7.2 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,SPF_HELO_PASS X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 08 Dec 2011 14:28:36 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id pB8ESZvJ016565 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 8 Dec 2011 09:28:35 -0500 Received: from localhost.localdomain (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id pB8ESXv1005740; Thu, 8 Dec 2011 09:28:34 -0500 From: Phil Muldoon To: Kevin Pouget Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] Add bp_location to Python interface References: Reply-to: pmuldoon@redhat.com X-URL: http://www.redhat.com Date: Thu, 08 Dec 2011 14:56:00 -0000 In-Reply-To: (Kevin Pouget's message of "Thu, 8 Dec 2011 10:25:54 +0100") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2011-12/txt/msg00265.txt.bz2 Kevin Pouget writes: > 11 files changed, 472 insertions(+), 0 deletions(-) > create mode 100644 gdb/python/py-bploc.c > > diff --git a/gdb/Makefile.in b/gdb/Makefile.in > index b71db33..4893d3b 100644 > --- a/gdb/Makefile.in > +++ b/gdb/Makefile.in > @@ -281,6 +281,7 @@ SUBDIR_PYTHON_OBS = \ > py-block.o \ > py-bpevent.o \ > py-breakpoint.o \ > + py-bploc.o \ Nit, alphabetically ordered please. > py-cmd.o \ > py-continueevent.o \ > py-event.o \ > @@ -312,6 +313,7 @@ SUBDIR_PYTHON_SRCS = \ > python/py-block.c \ > python/py-bpevent.c \ > python/py-breakpoint.c \ > + python/py-bploc.c \ Ditto. > +py-bploc.o: $(srcdir)/python/py-bploc.c > + $(COMPILE) $(PYTHON_CFLAGS) $(srcdir)/python/py-bploc.c > + $(POSTCOMPILE) > + > py-cmd.o: $(srcdir)/python/py-cmd.c > $(COMPILE) $(PYTHON_CFLAGS) $(srcdir)/python/py-cmd.c > $(POSTCOMPILE) Could you me the order of this to correspond with the order above. > + ** 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. > +@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". > +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. 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). 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? > + > +@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. 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). > +@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? > +@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. > +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"? > + > + /* 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? > + > +/* 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. > +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. > + 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. > +/* Python function to get the BP owning this location, is any. */ Typo, "is". > +/* 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. > +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. 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. > + > +/* 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. Cheers, Phil