[PATCH v5] gdb/python: Add BreakpointLocation type

[Simon Farre]

Thanks Pedro for taking your time to go over my patch!

Is there a good reason we're not exposing all the attributes MI exposes?
> For example, with:
>
>  (top-gdb) b main
>  (top-gdb) interpreter-exec mi "-break-list"
>
> we see:
>
> ...
> locations=[{number="3.1",
>             enabled="y",
>             addr="0x00000000000ed06c",
>             func="main(int, char**)",
>             file="/home/pedro/gdb/binutils-gdb/src/gdb/gdb.c",
>
> fullname="/net/cascais.nfs/brno/pedro/gdb/binutils-gdb/src/gdb/gdb.c",
>             line="25",
>             thread-groups=["i1"]},
> So at least "func", "fullname" and "thread-groups" (inferiors) seem to be
> missing.
>
> See here:
>
> https://sourceware.org/gdb/onlinedocs/gdb/GDB_002fMI-Breakpoint-Information.html
>
> I will make sure to add those for v6 - there was actually no particular
reason they were left out actually.

I'll also fix the grammatical and spelling errors.

Or perhaps:
>     error (_("Breakpoint location does not have an owner breakpoint."));
>
> But note you've named the location's owner field "parent", for some reason,
> though the documentation for that field says it is the "owner"...
> Better to use the same terminology everywhere, IMO.  Why not call the field
> "owner" instead of inventing a new term, if GDB ends up calling it owner,
> and the manual describes it as such, anyhow?
>

You're absolutely right. This was a mistake. This should be "owner" and
will be changed.


> > +static PyObject *
> > +bplocpy_get_source_location (PyObject *py_self, void *closure)
> > +{
> > +  gdbpy_breakpoint_location_object *self
> > +             = (gdbpy_breakpoint_location_object *) py_self;
> > +  BPPY_REQUIRE_VALID (self->parent);
> > +  BPLOCPY_REQUIRE_VALID (self->parent, self);
> > +  if (self->bp_loc->symtab)
> > +    {
> > +      gdbpy_ref<> tup (PyTuple_New (2));
> > +      /* symtab->filename is never NULL. */
> > +      gdbpy_ref<> filename
> > +     = host_string_to_python_string (self->bp_loc->symtab->filename);
> > +
> > +      if (PyTuple_SetItem (tup.get (), 0, filename.release ()) == -1
>
> If this returns -1, doesn't filename leak?
>

PyTuple_SetItem steals a reference from filename, if it fails it decrements
it.
(https://stackoverflow.com/questions/6111843/limitations-of-pytuple-setitem)


> > +   You should have received a copy of the GNU General Public License
> > +   along with this program.  If not, see  <http://www.gnu.org/licenses/>.
> */
> > +
> > +double add (double DoubleA, double DoubleB)
>
> We try to follow the GDB conventions in the testcases as well.
>
> It would be better to use a non-floating type for this, as some
> targets don't support floating point (see gdb_skip_float_test), and
> there's no
> reason the test shouldn't run on such targets.
>
>
> > +{
> > +  return DoubleA + DoubleB;
> > +}
> > +
> > +int add (int IntA, int IntB)
> > +{
> > +  return IntA + IntB;
> > +}
> > +
> > +int main (void)
> > +{
> > +  double d = add (1.0, 2.0);
> > +  int i = add (1, 2);
> > +}
> > diff --git a/gdb/testsuite/gdb.python/py-bp-locations.exp
> b/gdb/testsuite/gdb.python/py-bp-locations.exp
> > new file mode 100644
> > index 00000000000..b06b62de971
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.python/py-bp-locations.exp
> > @@ -0,0 +1,62 @@
> > +# Copyright (C) 2022-2022 Free Software Foundation, Inc.
> > +
> > +
> > +if { ![support_displaced_stepping] } {
> > +    unsupported "displaced stepping"
> > +    return -1
> > +}
>
> This looks leftover from some copied-from testcase.  I don't see a relation
> to this testcase.
>
> > +
> > +load_lib gdb-python.exp
> > +
> > +standard_testfile
> > +
> > +if {[gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}"
> executable {c++ debug}] != "" } {
> > +    return -1
> > +}
> > +
> > +save_vars { GDBFLAGS } {
> > +    clean_restart $testfile
> > +}
> > +
> > +if { [skip_python_tests] } { continue }
> > +
> > +if ![runto_main] {
> > +    return -1
> > +}
> > +
> > +# set breakpoint with 2 locations
>
> Start sentences with uppercase, and finish them with period.
>
> > +gdb_breakpoint "add"
> > +
> > +# test that the locations return correct source locations
>
> Ditto, and more below.
>
> > +gdb_test "python print(gdb.breakpoints()\[1\].locations\[0\].source)" \
> > +      ".*('.*py-bp-locations.c', 20).*"
> > +gdb_test "python print(gdb.breakpoints()\[1\].locations\[1\].source)" \
> > +      ".*('.*py-bp-locations.c', 25).*"
>
> It would be better to use gdb_get_line_number instead of hardcoding line
> numbers.
>
> > +
> > +# disable first location and make sure we don't hit it
> > +gdb_test "python gdb.breakpoints()\[1\].locations\[0\].enabled = False"
> ""
> > +gdb_continue_to_breakpoint "" ".*25.*"
> > +
> > +if ![runto_main] {
> > +    return -1
> > +}
> > +
> > +gdb_breakpoint "add"
> > +# disable "add" master breakpoint and verify all locations are disabled.
>
> owner, parent, now master?  :-P
>

Consistency in naming is quite obviously not my strongest skill :P. I'll
get to changing them.