[PATCH v6] gdb/python: Add BreakpointLocation type

[PATCH v6] gdb/python: Add BreakpointLocation type

[Simon Farre]

PR python/18385

v6:
This version addresses the points Pedro gave in review to this patch.

Added the attributes `function`, `fullname` and `thread_groups`
as per request by Pedro with the argument that it more resembles the
output of the MI-command "-break-list".  Added documentation for these attributes.

Cleaned up left overs from copy+paste in test suite, removed hard coding
of line numbers where possible.

Refactored some code to use more c++-y style range for loops
wrt to breakpoint locations.

Changed terminology, naming was very inconsistent. Used a variety of "parent",
"owner". Now "owner" is the only term used, and the field in the
gdb_breakpoint_location_object now also called "owner".

v5:

Changes in response to review by Tom Tromey:
- Replaced manual INCREF/DECREF calls with
  gdbpy_ref ptrs in places where possible.
- Fixed non-gdb style conforming formatting
- Get parent of bploc increases ref count of parent.
- moved bploc Python definition to py-breakpoint.c

The INCREF of self in bppy_get_locations is due
to the individual locations holding a reference to
it's owner. This is decremented at de-alloc time.

The reason why this needs to be here is, if the user writes
for instance;

py loc = gdb.breakpoints()[X].locations[Y]

The breakpoint owner object is immediately going
out of scope (GC'd/dealloced), and the location
object requires it to be alive for as long as it is alive.

Thanks for your review, Tom!

v4:
Fixed remaining doc issues as per request
by Eli.

v3:
Rewritten commit message, shortened + reworded,
added tests.

Currently, the Python API lacks the ability to
query breakpoints for their installed locations,
and subsequently, can't query any information about them, or
enable/disable individual locations.

This patch solves this by adding Python type gdb.BreakpointLocation.
The type is never instantiated by the user of the Python API directly,
but is produced by the gdb.Breakpoint.locations attribute returning
a list of gdb.BreakpointLocation.

gdb.Breakpoint.locations:
The attribute for retrieving the currently installed breakpoint
locations for gdb.Breakpoint. Matches behavior of
the "info breakpoints" command in that it only
returns the last known or currently inserted breakpoint locations.

BreakpointLocation contains 4 attributes

3 read-only attributes:
parent: location owner's Python companion object
source: file path and line number tuple: (string, long) / None
address: installed address of the location

1 writeable attribute:
enabled: get/set enable/disable this location (bool)

Access/calls to these, can all throw Python exceptions (documented in
the online documentation), and that's due to the nature
of how breakpoint locations can be invalidated
"behind the scenes", either by them being removed
from the original breakpoint or changed,
like for instance when a new symbol file is loaded, at
which point all breakpoint locations are re-created by GDB.
Therefore this patch has chosen to be non-intrusive:
it's up to the Python user to re-request the locations if
they become invalid.

Also there's event handlers that handle new object files etc, if a Python
user is storing breakpoint locations in some larger state they've
built up, refreshing the locations is easy and it only comes
with runtime overhead when the Python user wants to use them.

gdb.BreakpointLocation Python type
struct "gdbpy_breakpoint_location_object" is found in python-internal.h

Its definition, layout, methods and functions
are found in the same file as gdb.Breakpoint (py-breakpoint.c)

1 change was also made to breakpoint.h/c to make it possible
to enable and disable a bp_location* specifically,
without having its LOC_NUM, as this number
also can change arbitrarily behind the scenes.

Fixed formatting to follow GNU.

Updated docs & news file as per request.

Testsuite: tests the .source attribute and the disabling of
individual locations.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=18385
---
 gdb/NEWS                                     |   5 +
 gdb/breakpoint.c                             |  59 ++++
 gdb/breakpoint.h                             |   5 +
 gdb/doc/python.texi                          |  71 +++++
 gdb/python/py-breakpoint.c                   | 280 +++++++++++++++++++
 gdb/python/python-internal.h                 |  22 ++
 gdb/python/python.c                          |   1 +
 gdb/testsuite/gdb.python/py-bp-locations.c   |  32 +++
 gdb/testsuite/gdb.python/py-bp-locations.exp |  64 +++++
 9 files changed, 539 insertions(+)
 create mode 100644 gdb/testsuite/gdb.python/py-bp-locations.c
 create mode 100644 gdb/testsuite/gdb.python/py-bp-locations.exp

diff --git a/gdb/NEWS b/gdb/NEWS
index 760cb2b7abc..96b6ae04684 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -38,6 +38,11 @@ maintenance info line-table
      This is the same format that GDB uses when printing address, symbol,
      and offset information from the disassembler.
 
+  ** New Python type gdb.BreakpointLocation.
+     The new attribute 'locations' of gdb.Breakpoint returns a list of
+     gdb.BreakpointLocation objects specifying the locations where the
+     breakpoint is inserted into the debuggee.
+
 *** Changes in GDB 12
 
 * DBX mode is deprecated, and will be removed in GDB 13
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index b21d83d8019..716c57e055b 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -175,6 +175,8 @@ static void enable_breakpoint_disp (struct breakpoint *, enum bpdisp,
 
 static void decref_bp_location (struct bp_location **loc);
 
+static int find_loc_num_by_location (bp_location* loc);
+
 static struct bp_location *allocate_bp_location (struct breakpoint *bpt);
 
 /* update_global_location_list's modes of operation wrt to whether to
@@ -13660,6 +13662,63 @@ enable_disable_bp_num_loc (int bp_num, int loc_num, bool enable)
   gdb::observers::breakpoint_modified.notify (loc->owner);
 }
 
+/* Enable or disable a breakpoint location LOC.  ENABLE
+   specifies whether to enable or disable.  */
+
+void
+enable_disable_bp_location (bp_location *loc, bool enable)
+{
+  if (loc == nullptr)
+    error (_("Breakpoint location is invalid."));
+
+  if (loc->owner == nullptr)
+    error (_("Breakpoint location does not have an owner breakpoint."));
+
+  if (loc->disabled_by_cond && enable)
+    {
+      int loc_num = find_loc_num_by_location (loc);
+      if (loc_num == -1)
+	error (_("Breakpoint location LOC_NUM could not be found."));
+      else
+	error (_("Breakpoint %d's condition is invalid at location %d, "
+		"cannot enable."), loc->owner->number, loc_num);
+    }
+
+  if (loc->enabled != enable)
+    {
+      loc->enabled = enable;
+      mark_breakpoint_location_modified (loc);
+    }
+
+  if (target_supports_enable_disable_tracepoint ()
+	  && current_trace_status ()->running && loc->owner
+	  && is_tracepoint (loc->owner))
+	  target_disable_tracepoint (loc);
+
+  update_global_location_list (UGLL_DONT_INSERT);
+  gdb::observers::breakpoint_modified.notify (loc->owner);
+}
+
+/* Calculates LOC_NUM for LOC by traversing the bp_location chain of LOC's
+   owner.  1-based indexing.  -1 signals NOT FOUND.  */
+
+static int
+find_loc_num_by_location (bp_location *loc)
+{
+  if (loc != nullptr && loc->owner != nullptr)
+    {
+      /* Locations use 1-based indexing.  */
+      int loc_num = 1;
+      for (bp_location *it : loc->owner->locations ())
+	{
+	  if (it == loc)
+	    return loc_num;
+	  loc_num++;
+	}
+    }
+  return -1;
+}
+
 /* Enable or disable a range of breakpoint locations.  BP_NUM is the
    number of the breakpoint, and BP_LOC_RANGE specifies the
    (inclusive) range of location numbers of that breakpoint to
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index e412c4d4113..b39aaef9c16 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1803,4 +1803,9 @@ extern void catch_exception_event (enum exception_event_kind ex_event,
 				   const char *regex, bool tempflag,
 				   int from_tty);
 
+/* Enable or disable a breakpoint location LOC.  ENABLE
+   specifies whether to enable or disable.  */
+
+extern void enable_disable_bp_location (bp_location *loc, bool enable);
+
 #endif /* !defined (BREAKPOINT_H) */
diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index 7c414b01d70..85825ea16e7 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -6046,6 +6046,15 @@ the user.  It is a string.  If the breakpoint does not have a location
 attribute is not writable.
 @end defvar
 
+@defvar Breakpoint.locations
+Get the most current list of breakpoint locations that are inserted for this
+breakpoint, with elements of type @code{gdb.BreakpointLocation}
+(described below).  This functionality matches that of the
+@code{info breakpoint} command (@pxref{Set Breaks}), in that it only retrieves
+the most current list of locations, thus the list itself when returned is
+not updated behind the scenes.  This attribute is not writable.
+@end defvar
+
 @defvar Breakpoint.expression
 This attribute holds a breakpoint expression, as specified by
 the user.  It is a string.  If the breakpoint does not have an
@@ -6066,6 +6075,68 @@ commands, separated by newlines.  If there are no commands, this
 attribute is @code{None}.  This attribute is writable.
 @end defvar
 
+@subheading Breakpoint Locations
+
+A breakpoint location is one of the actual places where a breakpoint has been
+set, represented in the Python API by the @code{gdb.BreakpointLocation}
+type.  This type is never instantiated by the user directly, but is retrieved
+from @code{Breakpoint.locations} which returns a list of breakpoint
+locations where it is currently set.  Breakpoint locations can become
+invalid if new symbol files are loaded or dynamically loaded libraries are
+closed.  Accessing the attributes of an invalidated breakpoint location will
+throw a @code{RuntimeError} exception.  Access the @code{Breakpoint.locations}
+attribute again to retrieve the new and valid breakpoints location list.
+
+@defvar BreakpointLocation.source
+This attribute returns the source file path and line number where this location
+was set. The type of the attribute is a tuple of @var{string} and
+@var{long}.  If the breakpoint location doesn't have a source location,
+it returns None, which is the case for watchpoints and catchpoints.
+This will throw a @code{RuntimeError} exception if the location
+has been invalidated. This attribute is not writable.
+@end defvar
+
+@defvar BreakpointLocation.address
+This attribute returns the address where this location was set.
+This attribute is of type long.  This will throw a @code{RuntimeError}
+exception if the location has been invalidated.  This attribute is
+not writable.
+@end defvar
+
+@defvar BreakpointLocation.enabled
+This attribute holds the value for whether or not this location is enabled.
+This attribute is writable (boolean).  This will throw a @code{RuntimeError}
+exception if the location has been invalidated.
+@end defvar
+
+@defvar BreakpointLocation.owner
+This attribute holds a reference to the @code{gdb.Breakpoint} owner object,
+from which this @code{gdb.BreakpointLocation} was retrieved from.
+This will throw a @code{RuntimeError} exception if the location has been
+invalidated.  This attribute is not writable.
+@end defvar
+
+@defvar BreakpointLocation.function
+This attribute gets the name of the function where this location was set.
+If no function could be found this attribute returns @code{None}.
+This will throw a @code{RuntimeError} exception if the location has
+been invalidated.  This attribute is not writable.
+@end defvar
+
+@defvar BreakpointLocation.fullname
+This attribute gets the full name of where this location was set.  If no
+full name could be found, this attribute returns @code{None}.
+This will throw a @code{RuntimeError} exception if the location has
+been invalidated.  This attribute is not writable.
+@end defvar
+
+@defvar BreakpointLocation.thread_groups
+This attribute gets the thread groups it was set in.  It returns a @code{Tuple}
+of the thread group ID's.  This will throw a @code{RuntimeError}
+exception if the location has been invalidated.  This attribute
+is not writable.
+@end defvar
+
 @node Finish Breakpoints in Python
 @subsubsection Finish Breakpoints
 
diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
index 74de0d90e23..3a73f0790cc 100644
--- a/gdb/python/py-breakpoint.c
+++ b/gdb/python/py-breakpoint.c
@@ -34,6 +34,20 @@
 #include "py-event.h"
 #include "linespec.h"
 
+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;
+};
+
 /* Debugging of Python breakpoints.  */
 
 static bool pybp_debug;
@@ -692,6 +706,35 @@ bppy_get_ignore_count (PyObject *self, void *closure)
   return gdb_py_object_from_longest (self_bp->bp->ignore_count).release ();
 }
 
+/* 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));
+  for (bp_location *loc : self_bp->bp->locations ())
+    {
+      gdbpy_ref<py_bploc_t> py_bploc (PyObject_New (py_bploc_t,
+				    &breakpoint_location_object_type));
+      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);
+      if (PyList_Append (list.get (), (PyObject *) py_bploc.get ()) != 0)
+	{
+	  return nullptr;
+	}
+      py_bploc->bp_loc = ref.release ();
+    }
+  return list.release ();
+}
+
 /* Internal function to validate the Python parameters/keywords
    provided to bppy_init.  */
 
@@ -1182,6 +1225,20 @@ gdbpy_initialize_breakpoints (void)
   return 0;
 }
 
+/* Initialize the Python BreakpointLocation code.  */
+
+int
+gdbpy_initialize_breakpoint_locations ()
+{
+  if (PyType_Ready (&breakpoint_location_object_type) < 0)
+    return -1;
+
+  if (gdb_pymodule_addobject (gdb_module, "BreakpointLocation",
+			(PyObject *) &breakpoint_location_object_type) < 0)
+    return -1;
+  return 0;
+}
+
 \f
 
 /* Helper function that overrides this Python object's
@@ -1264,6 +1321,8 @@ or None if no condition set."},
     "Whether this breakpoint is a temporary breakpoint."},
   { "pending", bppy_get_pending, NULL,
     "Whether this breakpoint is a pending breakpoint."},
+  { "locations", bppy_get_locations, NULL,
+    "Get locations where this breakpoint was set"},
   { NULL }  /* Sentinel.  */
 };
 
@@ -1330,3 +1389,224 @@ _initialize_py_breakpoint ()
 	show_pybp_debug,
 	&setdebuglist, &showdebuglist);
 }
+
+/* Python function to set the enabled state of a breakpoint location.  */
+
+static int
+bplocpy_set_enabled (PyObject *py_self, PyObject *newvalue, void *closure)
+{
+  auto *self = (gdbpy_breakpoint_location_object *) py_self;
+  BPPY_SET_REQUIRE_VALID (self->owner);
+  BPLOCPY_SET_REQUIRE_VALID (self->owner, self);
+
+  if (newvalue == nullptr)
+    {
+      PyErr_SetString (PyExc_TypeError,
+			_("Cannot delete 'enabled' attribute."));
+      return -1;
+    }
+  else if (!PyBool_Check (newvalue))
+    {
+      PyErr_SetString (PyExc_TypeError,
+			_("The value of 'enabled' must be a boolean."));
+      return -1;
+    }
+
+  int cmp = PyObject_IsTrue (newvalue);
+  if (cmp < 0)
+    return -1;
+
+  try
+    {
+      enable_disable_bp_location (self->bp_loc, cmp == 1);
+    }
+  catch (const gdb_exception &except)
+    {
+      GDB_PY_SET_HANDLE_EXCEPTION (except);
+    }
+  return 0;
+}
+
+/* Python function to test whether or not the breakpoint location is enabled.  */
+
+static PyObject *
+bplocpy_get_enabled (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->enabled)
+    Py_RETURN_TRUE;
+  else
+    Py_RETURN_FALSE;
+}
+
+/* Python function to get address of breakpoint location.  */
+
+static PyObject *
+bplocpy_get_address (PyObject *py_self, void *closure)
+{
+  auto *self = (gdbpy_breakpoint_location_object *) py_self;
+  BPPY_REQUIRE_VALID (self->owner);
+  BPLOCPY_REQUIRE_VALID (self->owner, self);
+  return PyLong_FromLong (self->bp_loc->address);
+}
+
+/* Python function to get owner of breakpoint location, which
+   is of type gdb.Breakpoint.  */
+
+static PyObject *
+bplocpy_get_owner (PyObject *py_self, void *closure)
+{
+  auto *self = (gdbpy_breakpoint_location_object *) py_self;
+  BPPY_REQUIRE_VALID (self->owner);
+  BPLOCPY_REQUIRE_VALID (self->owner, self);
+  Py_INCREF (self->owner);
+  return (PyObject *) self->owner;
+}
+
+/* Python function to get the source file name path and line number
+   where this breakpoint location was set.   */
+
+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));
+      /* 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
+	  || PyTuple_SetItem (tup.get (), 1,
+			PyLong_FromLong (self->bp_loc->line_number)) == -1)
+	return nullptr;
+      return tup.release ();
+    }
+  else
+    Py_RETURN_NONE;
+}
+
+/* Python function to get the function name of where this location was set.  */
+
+static PyObject *
+bplocpy_get_function (PyObject *py_self, void *closure)
+{
+  auto *self = (gdbpy_breakpoint_location_object *) py_self;
+  BPPY_REQUIRE_VALID (self->owner);
+  BPLOCPY_REQUIRE_VALID (self->owner, self);
+  const auto fn_name = self->bp_loc->function_name.get ();
+  if (fn_name != nullptr)
+    return host_string_to_python_string (fn_name).release ();
+  Py_RETURN_NONE;
+}
+
+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));
+
+  for (inferior *inf : all_inferiors ())
+    {
+      if (inf->pspace == self->bp_loc->pspace)
+	if (PyList_Append (list.get (), PyLong_FromLong (inf->num)) != 0)
+	  return nullptr;
+    }
+  return PyList_AsTuple (list.get ());
+}
+
+static PyObject *
+bplocpy_get_fullname (PyObject *py_self, void *closure)
+{
+  auto *self = (gdbpy_breakpoint_location_object *) py_self;
+  BPPY_REQUIRE_VALID (self->owner);
+  BPLOCPY_REQUIRE_VALID (self->owner, self);
+  const auto symtab = self->bp_loc->symtab;
+  if (symtab != nullptr && symtab->fullname != nullptr)
+    {
+      gdbpy_ref<> fullname
+	= host_string_to_python_string (symtab->fullname);
+      return fullname.release ();
+    }
+  Py_RETURN_NONE;
+}
+
+/* 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);
+  Py_TYPE (py_self)->tp_free (py_self);
+}
+
+/* Attribute get/set Python definitions. */
+
+static gdb_PyGetSetDef bp_location_object_getset[] = {
+  { "enabled", bplocpy_get_enabled, bplocpy_set_enabled,
+    "Boolean telling whether the breakpoint is enabled.", NULL },
+  { "owner", bplocpy_get_owner, NULL,
+    "Get the breakpoint owner object", NULL },
+  { "address", bplocpy_get_address, NULL,
+    "Get address of where this location was set", NULL},
+  { "source", bplocpy_get_source_location, NULL,
+    "Get file and line number of where this location was set", NULL},
+  { "function", bplocpy_get_function, NULL,
+    "Get function of where this location was set", NULL },
+  { "fullname", bplocpy_get_fullname, NULL,
+    "Get fullname of where this location was set", NULL },
+  { "thread_groups", bplocpy_get_thread_groups, NULL,
+    "Get thread groups where this location is in", NULL },
+  { NULL }  /* Sentinel.  */
+};
+
+PyTypeObject breakpoint_location_object_type =
+{
+  PyVarObject_HEAD_INIT (NULL, 0)
+  "gdb.BreakpointLocation",		/*tp_name*/
+  sizeof (gdbpy_breakpoint_location_object), /*tp_basicsize*/
+  0,					/*tp_itemsize*/
+  bplocpy_dealloc,  			/*tp_dealloc*/
+  0,					/*tp_print*/
+  0,					/*tp_getattr*/
+  0,					/*tp_setattr*/
+  0,					/*tp_compare*/
+  0,					/*tp_repr*/
+  0,					/*tp_as_number*/
+  0,					/*tp_as_sequence*/
+  0,					/*tp_as_mapping*/
+  0,					/*tp_hash */
+  0,					/*tp_call*/
+  0,					/*tp_str*/
+  0,					/*tp_getattro*/
+  0,          				/*tp_setattro */
+  0,					/*tp_as_buffer*/
+  Py_TPFLAGS_DEFAULT,			/*tp_flags*/
+  "GDB breakpoint location object",	/* tp_doc */
+  0,					/* tp_traverse */
+  0,					/* tp_clear */
+  0,					/* tp_richcompare */
+  0,					/* tp_weaklistoffset */
+  0,					/* tp_iter */
+  0,					/* tp_iternext */
+  0,					/* tp_methods */
+  0,					/* tp_members */
+  bp_location_object_getset,		/* tp_getset */
+  0,					/* tp_base */
+  0,					/* tp_dict */
+  0,					/* tp_descr_get */
+  0,					/* tp_descr_set */
+  0,					/* tp_dictoffset */
+  0,					/* tp_init */
+  0,					/* tp_alloc */
+};
diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
index d947b96033b..307b74b6925 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -322,6 +322,26 @@ struct gdbpy_breakpoint_object
 	}                                                               \
     } while (0)
 
+/* 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 {                                                                    \
+      if ((Breakpoint)->bp != (Location)->bp_loc->owner)                    \
+	{                                                                   \
+	  PyErr_Format (PyExc_RuntimeError,                                 \
+			_("Breakpoint location is invalid."));              \
+	  return -1;                                                        \
+	}                                                                   \
+    } while (0)
 
 /* Variables used to pass information between the Breakpoint
    constructor and the breakpoint-created hook function.  */
@@ -505,6 +525,8 @@ int gdbpy_initialize_objfile (void)
   CPYCHECKER_NEGATIVE_RESULT_SETS_EXCEPTION;
 int gdbpy_initialize_breakpoints (void)
   CPYCHECKER_NEGATIVE_RESULT_SETS_EXCEPTION;
+int gdbpy_initialize_breakpoint_locations ()
+  CPYCHECKER_NEGATIVE_RESULT_SETS_EXCEPTION;
 int gdbpy_initialize_finishbreakpoints (void)
   CPYCHECKER_NEGATIVE_RESULT_SETS_EXCEPTION;
 int gdbpy_initialize_lazy_string (void)
diff --git a/gdb/python/python.c b/gdb/python/python.c
index 7a9c8c1b66e..479cf6a296a 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -2057,6 +2057,7 @@ do_start_initialization ()
       || gdbpy_initialize_pspace () < 0
       || gdbpy_initialize_objfile () < 0
       || gdbpy_initialize_breakpoints () < 0
+      || gdbpy_initialize_breakpoint_locations () < 0
       || gdbpy_initialize_finishbreakpoints () < 0
       || gdbpy_initialize_lazy_string () < 0
       || gdbpy_initialize_linetable () < 0
diff --git a/gdb/testsuite/gdb.python/py-bp-locations.c b/gdb/testsuite/gdb.python/py-bp-locations.c
new file mode 100644
index 00000000000..072bf3d6c12
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-bp-locations.c
@@ -0,0 +1,32 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2022-2022 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see  <http://www.gnu.org/licenses/>.  */
+
+short add (short a, short b)
+{
+  return a + b;
+}
+
+int add (int c, int d)
+{
+  return c + d;
+}
+
+int main (void)
+{
+  short sum_a = add (2, 4);
+  int sum_b = add (2, 4);
+}
\ No newline at end of file
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..0f06147ffc7
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-bp-locations.exp
@@ -0,0 +1,64 @@
+# Copyright (C) 2022-2022 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+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.
+gdb_breakpoint "add"
+
+set expected_line_a [gdb_get_line_number "a + b"]
+set expected_line_b [gdb_get_line_number "c + d"]
+
+# Test that the locations return correct source locations.
+gdb_test "python print(gdb.breakpoints()\[1\].locations\[0\].source)" \
+	 ".*('.*py-bp-locations.c', $expected_line_a).*"
+gdb_test "python print(gdb.breakpoints()\[1\].locations\[1\].source)" \
+	 ".*('.*py-bp-locations.c', $expected_line_b).*"
+
+# 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" owner breakpoint and verify all locations still are enabled.
+gdb_test "python gdb.breakpoints()\[1\].enabled = False" "" "Disable add owner breakpoint."
+
+gdb_test "python print(gdb.breakpoints()\[1\].locations\[0\].enabled)" "True" "First location still enabled"
+
+gdb_test "python print(gdb.breakpoints()\[1\].locations\[1\].enabled)" "True" "Second location still enabled"
+
+# 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

base-commit: 04f4c17c7a14ebb6c2212267b2ebc83f1376fe20
-- 
2.34.1

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

[Tom Tromey]

>>>>> 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

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

[Simon Farre]

Thanks Tom for your review!

I have addressed your issues and they can be found in patch v7

Simon