Re: [PATCH] Add bp_location to Python interface

Phil Muldoon
2011-12-08T14:28:55Z

Kevin Pouget <kevin.pouget@gmail.com> 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