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

[Tom Tromey via Gdb-patches <gdb-patches@sourceware.org>]

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

Simon> +static int find_loc_num_by_location (bp_location* loc);

I think it would be better to rearrange this so that the forward
declaration isn't needed.

Simon> +@defvar Breakpoint.locations
Simon> +Get the most current list of breakpoint locations that are inserted for this
Simon> +breakpoint, with elements of type @code{gdb.BreakpointLocation}
Simon> +(described below).  This functionality matches that of the
Simon> +@code{info breakpoint} command (@pxref{Set Breaks}), in that it only retrieves
Simon> +the most current list of locations, thus the list itself when returned is
Simon> +not updated behind the scenes.  This attribute is not writable.
Simon> +@end defvar

I used this patch for some work I'm doing and I found this description a
bit hard to follow.  Maybe the "This functionality..."  sentence could
be replaced with one that just says "This does not cause gdb to update
the underlying list of locations"?

Simon> +@defvar BreakpointLocation.source
Simon> +This attribute returns the source file path and line number where this location
Simon> +was set. The type of the attribute is a tuple of @var{string} and
Simon> +@var{long}.  If the breakpoint location doesn't have a source location,

I think nowadays there is only 'int' in Python.

This occurs a bit later in the text as well.

Simon> +/* Python function to get the breakpoint locations of an owner breakpoint.  */
Simon> +
Simon> +static PyObject*

Space before "*".

Simon> +static PyObject *
Simon> +bplocpy_get_source_location (PyObject *py_self, void *closure)
Simon> +{
[...]
Simon> +      auto line = gdb_py_object_from_ulongest (self->bp_loc->line_number);

I think a null check is needed here.

Simon> +static PyObject *
Simon> +bplocpy_get_thread_groups (PyObject *py_self, void *closure)
[...]
Simon> +	if (PyList_Append (list.get (),
Simon> +	    gdb_py_object_from_ulongest(inf->num).release()) != 0)
Simon> +	  return nullptr;

The formatting looks weird here, but also gdb_py_object_from_ulongest
has to be called separately and the result checked for nullptr.
Note there's also a missing space before the "(".

Simon> diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
Simon> index d947b96033b..7c4648e6042 100644
Simon> --- a/gdb/python/python-internal.h
Simon> +++ b/gdb/python/python-internal.h
Simon> @@ -322,12 +322,10 @@ struct gdbpy_breakpoint_object
Simon>  	}                                                               \
Simon>      } while (0)
 
Simon> -
Simon>  /* Variables used to pass information between the Breakpoint
Simon>     constructor and the breakpoint-created hook function.  */
Simon>  extern gdbpy_breakpoint_object *bppy_pending_object;
 
Simon> -
Simon>  struct thread_object
Simon>  {
Simon>    PyObject_HEAD

This seems like a spurious change.

Simon> +   Copyright 2022-2022 Free Software Foundation, Inc.

Don't need a range here.

Simon> --- /dev/null
Simon> +++ b/gdb/testsuite/gdb.python/py-bp-locations.exp
Simon> @@ -0,0 +1,64 @@
Simon> +# Copyright (C) 2022-2022 Free Software Foundation, Inc.

Here either.

thanks,
Tom