Re: [PATCH v6] gdb/python: Add BreakpointLocation type

[Tom Tromey <tom@tromey.com>]

>>>>> Simon Farre via Gdb-patches <gdb-patches@sourceware.org> writes:

> PR python/18385
> v6:
> This version addresses the points Pedro gave in review to this patch.

Thanks for the patch.

> +extern PyTypeObject breakpoint_location_object_type
> +    CPYCHECKER_TYPE_OBJECT_FOR_TYPEDEF ("breakpoint_location_object");
> +
> +struct gdbpy_breakpoint_location_object
> +{
> +  PyObject_HEAD
> +
> +  /* The gdb breakpoint location object.  */
> +  bp_location *bp_loc;
> +
> +  /* Location's breakpoint owner.  */
> +  gdbpy_breakpoint_object *owner;

It would probably be good to mention in these comments that these are
owning references.

> +/* Python function to get the breakpoint locations of an owner breakpoint.  */
> +
> +static PyObject*
> +bppy_get_locations (PyObject *self, void *closure)
> +{
> +  using py_bploc_t = gdbpy_breakpoint_location_object;
> +  auto *self_bp = (gdbpy_breakpoint_object *) self;
> +  BPPY_REQUIRE_VALID (self_bp);
> +
> +  gdbpy_ref<> list (PyList_New (0));

This is missing a null check.
The function can just return null if this fails.

> +  for (bp_location *loc : self_bp->bp->locations ())
> +    {
> +      gdbpy_ref<py_bploc_t> py_bploc (PyObject_New (py_bploc_t,
> +				    &breakpoint_location_object_type));

I think this needs a null check.

> +      bp_location_ref_ptr ref = bp_location_ref_ptr::new_reference (loc);
> +      py_bploc->owner = self_bp;
> +
> +      /* The location takes a reference to the owner breakpoint.  Decrements
> +         when they are de-allocated in bplocpy_dealloc */
> +      Py_INCREF (self);

I think it would be clearer to move the incref above the assignment to
'owner' and group these together.  That way it's more obvious (IMO) that
the ownership is being transferred to py_bloc.

> +      if (PyList_Append (list.get (), (PyObject *) py_bploc.get ()) != 0)
> +	{
> +	  return nullptr;
> +	}

You can remove the braces here.

> +      py_bploc->bp_loc = ref.release ();

It would be fine to hoist this up before the PyList_Append as well.  I
suppose it doesn't really matter but I was wondering why one assignment
is done before and one after.

> +  return PyLong_FromLong (self->bp_loc->address);

This should use gdb_py_object_from_ulongest.

> +static PyObject *
> +bplocpy_get_source_location (PyObject *py_self, void *closure)
> +{
> +  auto *self = (gdbpy_breakpoint_location_object *) py_self;
> +  BPPY_REQUIRE_VALID (self->owner);
> +  BPLOCPY_REQUIRE_VALID (self->owner, self);
> +  if (self->bp_loc->symtab)
> +    {
> +      gdbpy_ref<> tup (PyTuple_New (2));

I think this needs a null check.

> +      /* symtab->filename is never NULL. */
> +      gdbpy_ref<> filename
> +	= host_string_to_python_string (self->bp_loc->symtab->filename);

Same here.

> +
> +      if (PyTuple_SetItem (tup.get (), 0, filename.release ()) == -1
> +	  || PyTuple_SetItem (tup.get (), 1,
> +			PyLong_FromLong (self->bp_loc->line_number)) == -1)

Normally gdb uses the gdb_py wrappers instead of PyLong_FromLong, to
avoid problems with size and/or signed-ness.

> +static PyObject *
> +bplocpy_get_thread_groups (PyObject *py_self, void *closure)
> +{
> +  auto *self = (gdbpy_breakpoint_location_object *) py_self;
> +  BPPY_REQUIRE_VALID (self->owner);
> +  BPLOCPY_REQUIRE_VALID (self->owner, self);
> +  gdbpy_ref<> list (PyList_New (0));

Null check.

> +
> +  for (inferior *inf : all_inferiors ())
> +    {
> +      if (inf->pspace == self->bp_loc->pspace)
> +	if (PyList_Append (list.get (), PyLong_FromLong (inf->num)) != 0)

gdb_py_object_from_longest

> +	  return nullptr;
> +    }
> +  return PyList_AsTuple (list.get ());

This seems fine but it seems like it would also be fine to just return
the list here.

> +/* De-allocation function to be called for the Python object.  */
> +
> +static void
> +bplocpy_dealloc (PyObject *py_self)
> +{
> +  auto *self = (gdbpy_breakpoint_location_object *) py_self;
> +  bp_location_ref_policy::decref (self->bp_loc);
> +  Py_DECREF (self->owner);

I wonder if Py_XDECREF is needed here, like if there can ever be a case
where 'owner' isn't set.  What happens if you try to create one of these
objects manually?

Also it's not too normal, I think, to directly refer to the refcount
policy classes.  Anyway the issue here is if bp_loc can be null, the
policy class will cause a crash (whereas ref_ptr ensures this can't
happen).

> +/* Require that BREAKPOINT and LOCATION->OWNER are the same; throw a Python
> +   exception if they are not.  */
> +#define BPLOCPY_REQUIRE_VALID(Breakpoint, Location)                         \
> +    do {                                                                    \
> +      if ((Breakpoint)->bp != (Location)->bp_loc->owner)                    \
> +	return PyErr_Format (PyExc_RuntimeError,                            \
> +          _("Breakpoint location is invalid."));                            \
> +    } while (0)
> +
> +/* Require that BREAKPOINT and LOCATION->OWNER are the same; throw a Python
> +   exception if they are not.  This macro is for use in setter functions.  */
> +#define BPLOCPY_SET_REQUIRE_VALID(Breakpoint, Location)                     \

Do these macros need to be in python-internal.h?  Seems like they'd be
fine in the .c file.

> +# Set breakpoint at end and make sure we run past the still enabled locations,
> +gdb_breakpoint 32
> +gdb_continue_to_breakpoint "end of main" ".*32.*"
> \ No newline at end of file

Should be a newline.

Tom