* [PATCH v5] gdb/python: Add BreakpointLocation type
@ 2022-04-20 8:37 Simon Farre
2022-05-14 22:16 ` [PING][PATCH " Simon Farre
2022-05-30 18:46 ` [PATCH " Pedro Alves
0 siblings, 2 replies; 4+ messages in thread
From: Simon Farre @ 2022-04-20 8:37 UTC (permalink / raw)
To: gdb-patches
v5:
PR python/18385
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-interal.h
It's definition, layout, methods and functions
is found in the same file as gdb.Breakpoint (py-breakpoint.c)
1 change were also made to breakpoint.h/c to make it possible
to enable and disable a bp_location* specifically,
without having it's 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 | 63 +++++
gdb/breakpoint.h | 5 +
gdb/doc/python.texi | 50 ++++
gdb/python/py-breakpoint.c | 242 +++++++++++++++++++
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 | 62 +++++
9 files changed, 482 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..e3106c53fca 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,67 @@ 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 belong to an owner."));
+
+ 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)
+ {
+ if (loc->owner)
+ {
+ // begin at 1.1, 1.2 ... locs are not 0-indexed at cmdline
+ int loc_num = 1;
+ for (bp_location *it = loc->owner->loc; it != nullptr; it = it->next)
+ {
+ if (it == loc)
+ return loc_num;
+ loc_num++;
+ }
+ }
+ return -1;
+ }
+ 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..fe7f5d85338 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,47 @@ 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 that a breakpoint has been
+inserted, 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 inserted. 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 inserted. 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 inserted.
+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.parent
+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
+
@node Finish Breakpoints in Python
@subsubsection Finish Breakpoints
diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
index 74de0d90e23..c88d5eaa468 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;
+
+ /* Breakpoint parent. */
+ gdbpy_breakpoint_object *parent;
+};
+
/* Debugging of Python breakpoints. */
static bool pybp_debug;
@@ -692,6 +706,38 @@ 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 from an owner breakpoint */
+
+static PyObject*
+bppy_get_locations (PyObject *self, void *closure)
+{
+ using py_bploc_t = gdbpy_breakpoint_location_object;
+ gdbpy_breakpoint_object *self_bp = (gdbpy_breakpoint_object *) self;
+ BPPY_REQUIRE_VALID (self_bp);
+
+ gdbpy_ref<> list (PyList_New (0));
+ for (bp_location* loc = self_bp->bp->loc; loc; loc = loc->next)
+ {
+ 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->parent = 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)
+ {
+ /* Python C API docs do not describe if elements succesfully
+ added have their ref count decremented if this error is thrown.
+ If they don't, we leak references here. */
+ 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 +1228,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 +1324,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 installed"},
{ NULL } /* Sentinel. */
};
@@ -1330,3 +1392,183 @@ _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)
+{
+ gdbpy_breakpoint_location_object *self
+ = (gdbpy_breakpoint_location_object *) py_self;
+ BPPY_SET_REQUIRE_VALID (self->parent);
+ BPLOCPY_SET_REQUIRE_VALID (self->parent, self);
+
+ if (newvalue == NULL)
+ {
+ 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)
+{
+ gdbpy_breakpoint_location_object *self
+ = (gdbpy_breakpoint_location_object *) py_self;
+ BPPY_REQUIRE_VALID (self->parent);
+ BPLOCPY_REQUIRE_VALID (self->parent, self);
+
+ /* If the owner is disabled, all locations are disabled. */
+ if (self->parent->bp->enable_state == enable_state::bp_disabled)
+ Py_RETURN_FALSE;
+
+ if (self->bp_loc->enabled == enable_state::bp_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)
+{
+ gdbpy_breakpoint_location_object *self
+ = (gdbpy_breakpoint_location_object *) py_self;
+ BPPY_REQUIRE_VALID (self->parent);
+ BPLOCPY_REQUIRE_VALID (self->parent, self);
+ return PyLong_FromLong (self->bp_loc->address);
+}
+
+/* Python function to get parent of breakpoint location, which
+ is of type gdb.Breakpoint. */
+
+static PyObject *
+bplocpy_get_parent (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);
+ Py_INCREF (self->parent);
+ return (PyObject *) self->parent;
+}
+
+/* Python function to get the source file name path and line number
+ where this breakpoint location was installed. */
+
+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
+ || PyTuple_SetItem (tup.get (), 1,
+ PyLong_FromLong (self->bp_loc->line_number)) == -1)
+ {
+ return nullptr;
+ }
+ return tup.release ();
+ }
+ else
+ Py_RETURN_NONE;
+}
+
+/* De-allocation function to be called for the Python object. */
+
+static void
+bplocpy_dealloc (PyObject *py_self)
+{
+ gdbpy_breakpoint_location_object *self
+ = (gdbpy_breakpoint_location_object *) py_self;
+ bp_location_ref_policy::decref (self->bp_loc);
+ Py_DECREF (self->parent);
+ 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 },
+ { "parent", bplocpy_get_parent, NULL,
+ "Get the parent breakpoint object", NULL },
+ { "address", bplocpy_get_address, NULL,
+ "Get install address of this breakpoint location", NULL},
+ { "source", bplocpy_get_source_location, NULL,
+ "Get file and line number where breakpoint was installed", 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..784c3a02b8a 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's owner are the same; throw a Python
+ exception if it is 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 it is 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..388f5f28e58
--- /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/>. */
+
+double add (double DoubleA, double DoubleB)
+{
+ 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.
+
+# 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/>.
+
+if { ![support_displaced_stepping] } {
+ unsupported "displaced stepping"
+ return -1
+}
+
+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"
+
+# test that the locations return correct source locations
+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).*"
+
+# 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.
+gdb_test "python gdb.breakpoints()\[1\].enabled = False" "" "disable add master breakpoint"
+gdb_breakpoint 32
+
+# Disable "master breakpoint" and test that all locations are disabled.
+gdb_continue_to_breakpoint "end of main" ".*32.*"
base-commit: 04f4c17c7a14ebb6c2212267b2ebc83f1376fe20
--
2.32.0
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PING][PATCH v5] gdb/python: Add BreakpointLocation type
2022-04-20 8:37 [PATCH v5] gdb/python: Add BreakpointLocation type Simon Farre
@ 2022-05-14 22:16 ` Simon Farre
2022-05-30 18:46 ` [PATCH " Pedro Alves
1 sibling, 0 replies; 4+ messages in thread
From: Simon Farre @ 2022-05-14 22:16 UTC (permalink / raw)
To: Simon Farre via Gdb-patches
Ping! Would love if someone could check it out (Tromey has initially
reviewed). Are there any further particular steps that I need to take? Docs
has been reviewed and OK'd.
Cheers!
Simon
On Wed, Apr 20, 2022 at 10:37 AM Simon Farre <simon.farre.cx@gmail.com>
wrote:
> v5:
> PR python/18385
>
> 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-interal.h
>
> It's definition, layout, methods and functions
> is found in the same file as gdb.Breakpoint (py-breakpoint.c)
>
> 1 change were also made to breakpoint.h/c to make it possible
> to enable and disable a bp_location* specifically,
> without having it's 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 | 63 +++++
> gdb/breakpoint.h | 5 +
> gdb/doc/python.texi | 50 ++++
> gdb/python/py-breakpoint.c | 242 +++++++++++++++++++
> 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 | 62 +++++
> 9 files changed, 482 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..e3106c53fca 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,67 @@ 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 belong to an owner."));
> +
> + 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)
> + {
> + if (loc->owner)
> + {
> + // begin at 1.1, 1.2 ... locs are not 0-indexed at cmdline
> + int loc_num = 1;
> + for (bp_location *it = loc->owner->loc; it != nullptr; it =
> it->next)
> + {
> + if (it == loc)
> + return loc_num;
> + loc_num++;
> + }
> + }
> + return -1;
> + }
> + 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..fe7f5d85338 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,47 @@ 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 that a breakpoint has
> been
> +inserted, 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 inserted. 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 inserted. 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 inserted.
> +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.parent
> +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
> +
> @node Finish Breakpoints in Python
> @subsubsection Finish Breakpoints
>
> diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
> index 74de0d90e23..c88d5eaa468 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;
> +
> + /* Breakpoint parent. */
> + gdbpy_breakpoint_object *parent;
> +};
> +
> /* Debugging of Python breakpoints. */
>
> static bool pybp_debug;
> @@ -692,6 +706,38 @@ 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 from an owner
> breakpoint */
> +
> +static PyObject*
> +bppy_get_locations (PyObject *self, void *closure)
> +{
> + using py_bploc_t = gdbpy_breakpoint_location_object;
> + gdbpy_breakpoint_object *self_bp = (gdbpy_breakpoint_object *) self;
> + BPPY_REQUIRE_VALID (self_bp);
> +
> + gdbpy_ref<> list (PyList_New (0));
> + for (bp_location* loc = self_bp->bp->loc; loc; loc = loc->next)
> + {
> + 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->parent = 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)
> + {
> + /* Python C API docs do not describe if elements succesfully
> + added have their ref count decremented if this error is
> thrown.
> + If they don't, we leak references here. */
> + 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 +1228,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;
> +}
> +
>
>
> /* Helper function that overrides this Python object's
> @@ -1264,6 +1324,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 installed"},
> { NULL } /* Sentinel. */
> };
>
> @@ -1330,3 +1392,183 @@ _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)
> +{
> + gdbpy_breakpoint_location_object *self
> + = (gdbpy_breakpoint_location_object *) py_self;
> + BPPY_SET_REQUIRE_VALID (self->parent);
> + BPLOCPY_SET_REQUIRE_VALID (self->parent, self);
> +
> + if (newvalue == NULL)
> + {
> + 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)
> +{
> + gdbpy_breakpoint_location_object *self
> + = (gdbpy_breakpoint_location_object *) py_self;
> + BPPY_REQUIRE_VALID (self->parent);
> + BPLOCPY_REQUIRE_VALID (self->parent, self);
> +
> + /* If the owner is disabled, all locations are disabled. */
> + if (self->parent->bp->enable_state == enable_state::bp_disabled)
> + Py_RETURN_FALSE;
> +
> + if (self->bp_loc->enabled == enable_state::bp_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)
> +{
> + gdbpy_breakpoint_location_object *self
> + = (gdbpy_breakpoint_location_object *) py_self;
> + BPPY_REQUIRE_VALID (self->parent);
> + BPLOCPY_REQUIRE_VALID (self->parent, self);
> + return PyLong_FromLong (self->bp_loc->address);
> +}
> +
> +/* Python function to get parent of breakpoint location, which
> + is of type gdb.Breakpoint. */
> +
> +static PyObject *
> +bplocpy_get_parent (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);
> + Py_INCREF (self->parent);
> + return (PyObject *) self->parent;
> +}
> +
> +/* Python function to get the source file name path and line number
> + where this breakpoint location was installed. */
> +
> +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
> + || PyTuple_SetItem (tup.get (), 1,
> + PyLong_FromLong (self->bp_loc->line_number)) == -1)
> + {
> + return nullptr;
> + }
> + return tup.release ();
> + }
> + else
> + Py_RETURN_NONE;
> +}
> +
> +/* De-allocation function to be called for the Python object. */
> +
> +static void
> +bplocpy_dealloc (PyObject *py_self)
> +{
> + gdbpy_breakpoint_location_object *self
> + = (gdbpy_breakpoint_location_object *) py_self;
> + bp_location_ref_policy::decref (self->bp_loc);
> + Py_DECREF (self->parent);
> + 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 },
> + { "parent", bplocpy_get_parent, NULL,
> + "Get the parent breakpoint object", NULL },
> + { "address", bplocpy_get_address, NULL,
> + "Get install address of this breakpoint location", NULL},
> + { "source", bplocpy_get_source_location, NULL,
> + "Get file and line number where breakpoint was installed", 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..784c3a02b8a 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's owner are the same; throw a
> Python
> + exception if it is 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 it is 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..388f5f28e58
> --- /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/>.
> */
> +
> +double add (double DoubleA, double DoubleB)
> +{
> + 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.
> +
> +# 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/>.
> +
> +if { ![support_displaced_stepping] } {
> + unsupported "displaced stepping"
> + return -1
> +}
> +
> +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"
> +
> +# test that the locations return correct source locations
> +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).*"
> +
> +# 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.
> +gdb_test "python gdb.breakpoints()\[1\].enabled = False" "" "disable add
> master breakpoint"
> +gdb_breakpoint 32
> +
> +# Disable "master breakpoint" and test that all locations are disabled.
> +gdb_continue_to_breakpoint "end of main" ".*32.*"
>
> base-commit: 04f4c17c7a14ebb6c2212267b2ebc83f1376fe20
> --
> 2.32.0
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v5] gdb/python: Add BreakpointLocation type
2022-04-20 8:37 [PATCH v5] gdb/python: Add BreakpointLocation type Simon Farre
2022-05-14 22:16 ` [PING][PATCH " Simon Farre
@ 2022-05-30 18:46 ` Pedro Alves
2022-05-31 14:14 ` Simon Farre
1 sibling, 1 reply; 4+ messages in thread
From: Pedro Alves @ 2022-05-30 18:46 UTC (permalink / raw)
To: Simon Farre, gdb-patches
On 2022-04-20 09:37, Simon Farre via Gdb-patches wrote:
> v5:
> PR python/18385
>
> 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)
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
Scroll down to where it reads
"A location in a multi-location breakpoint is represented as a tuple with the following fields:"
Note that the enable/disable state is a tristate in MI. Not sure what you want to do with that
in Python.
>
> 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-interal.h
interal -> internal
>
> It's definition, layout, methods and functions
It's -> Its
> is found in the same file as gdb.Breakpoint (py-breakpoint.c)
is found -> are found.
>
> 1 change were also made to breakpoint.h/c to make it possible
were -> was
> to enable and disable a bp_location* specifically,
> without having it's LOC_NUM, as this number
it's -> its
> 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 | 63 +++++
> gdb/breakpoint.h | 5 +
> gdb/doc/python.texi | 50 ++++
> gdb/python/py-breakpoint.c | 242 +++++++++++++++++++
> 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 | 62 +++++
> 9 files changed, 482 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..e3106c53fca 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,67 @@ 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 belong to an owner."));
"to an owner" sounds odd. Should say at least "to an owner breakpoint".
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?
> +
> + 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)
if (loc != nullptr)
> + {
> + if (loc->owner)
if (loc->owner != nullptr)
You can actually combine both conditions in one if:
if (loc != nulptr && loc->owner != nullptr)
> + {
> + // begin at 1.1, 1.2 ... locs are not 0-indexed at cmdline
GDB doesn't use // style comments. Also, we always write full sentences
starting with an uppercase letter and end sentences with a period and two double spaces.
You can just say:
/* Locations use 1-based indexing. */
> + int loc_num = 1;
> + for (bp_location *it = loc->owner->loc; it != nullptr; it = it->next)
This can be more simply written using the locations() range function, like so:
for (bp_location *it = loc->owner->locations ())
> + {
> + if (it == loc)
> + return loc_num;
> + loc_num++;
> + }
> + }
> + return -1;
This "return -1" is unnecessary given the one just below. If you combine the ifs, then
this one naturally goes away anyhow.
> + }
> + 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..fe7f5d85338 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,47 @@ 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 that a breakpoint has been
> +inserted, represented in the Python API by the @code{gdb.BreakpointLocation}
has been inserted -> has been set. The location may well exist in the breakpoint,
but not be inserted at the given moment. E.g., if the program is stopped, then
the breakpoint isn't inserted. See the "set breakpoint always-inserted" setting.
> +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 inserted. 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 inserted. 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 inserted.
> +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.parent
> +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
> +
> @node Finish Breakpoints in Python
> @subsubsection Finish Breakpoints
>
> diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
> index 74de0d90e23..c88d5eaa468 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;
> +
> + /* Breakpoint parent. */
> + gdbpy_breakpoint_object *parent;
> +};
> +
> /* Debugging of Python breakpoints. */
>
> static bool pybp_debug;
> @@ -692,6 +706,38 @@ 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 from an owner breakpoint */
End sentences with a period and double space.
> +
> +static PyObject*
> +bppy_get_locations (PyObject *self, void *closure)
> +{
> + using py_bploc_t = gdbpy_breakpoint_location_object;
> + gdbpy_breakpoint_object *self_bp = (gdbpy_breakpoint_object *) self;
> + BPPY_REQUIRE_VALID (self_bp);
> +
> + gdbpy_ref<> list (PyList_New (0));
> + for (bp_location* loc = self_bp->bp->loc; loc; loc = loc->next)
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->parent = 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)
> + {
> + /* Python C API docs do not describe if elements succesfully
> + added have their ref count decremented if this error is thrown.
> + If they don't, we leak references here. */
> + 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 +1228,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 +1324,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 installed"},
s/installed/set/
> { NULL } /* Sentinel. */
> };
>
> @@ -1330,3 +1392,183 @@ _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)
> +{
> + gdbpy_breakpoint_location_object *self
> + = (gdbpy_breakpoint_location_object *) py_self;
Indentation is incorrect here, and everywhere else this pattern appears in the
patch. The "=" should be indented by two spaces, like:
{
gdbpy_breakpoint_location_object *self
= (gdbpy_breakpoint_location_object *) py_self;
You can also write it like this to avoid writing the type twice:
auto *self = static_cast<gdbpy_breakpoint_location_object *> (py_self);
... and it fits in one line.
> + BPPY_SET_REQUIRE_VALID (self->parent);
> + BPLOCPY_SET_REQUIRE_VALID (self->parent, self);
> +
> + if (newvalue == NULL)
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)
> +{
> + gdbpy_breakpoint_location_object *self
> + = (gdbpy_breakpoint_location_object *) py_self;
> + BPPY_REQUIRE_VALID (self->parent);
> + BPLOCPY_REQUIRE_VALID (self->parent, self);
> +
> + /* If the owner is disabled, all locations are disabled. */
> + if (self->parent->bp->enable_state == enable_state::bp_disabled)
> + Py_RETURN_FALSE;
Locations are controllable independently of the owner breakpoint, and
when the owner breakpoint is disabled, "info breakpoints" shows the locations
as enabled, though with "y-" (this "y-" thing was a recent change). Since this is
an API, we don't need to care about distinguishing "y-" vs "y", as the user can
programatically figure out what is going on. But I don't think Python should say the
location is disabled when in fact it is enabled, even if the owner breakpoint is
disabled. Again, the user can check the enabled state of the parent breakpoint if they
care. OTOH, reporting the location as enabled, let's the Python user know which
locations will trigger breaks if the owner breakpoint is re-enabled.
> +
> + if (self->bp_loc->enabled == enable_state::bp_enabled)
The enable_state enum is only used by the owner breakpoint's enable_state
field. bp_location's enabled flag is just a boolean:
/* Is this particular location enabled. */
bool enabled = false;
> + 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)
> +{
> + gdbpy_breakpoint_location_object *self
> + = (gdbpy_breakpoint_location_object *) py_self;
> + BPPY_REQUIRE_VALID (self->parent);
> + BPLOCPY_REQUIRE_VALID (self->parent, self);
> + return PyLong_FromLong (self->bp_loc->address);
> +}
> +
> +/* Python function to get parent of breakpoint location, which
> + is of type gdb.Breakpoint. */
> +
> +static PyObject *
> +bplocpy_get_parent (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);
> + Py_INCREF (self->parent);
> + return (PyObject *) self->parent;
> +}
> +
> +/* Python function to get the source file name path and line number
> + where this breakpoint location was installed. */
> +
> +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 (tup.get (), 1,
> + PyLong_FromLong (self->bp_loc->line_number)) == -1)
> + {
> + return nullptr;
> + }
No {} for single line statements.
> + return tup.release ();
> + }
> + else
> + Py_RETURN_NONE;
> +}
> +
> +/* De-allocation function to be called for the Python object. */
> +
> +static void
> +bplocpy_dealloc (PyObject *py_self)
> +{
> + gdbpy_breakpoint_location_object *self
> + = (gdbpy_breakpoint_location_object *) py_self;
> + bp_location_ref_policy::decref (self->bp_loc);
> + Py_DECREF (self->parent);
> + 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 },
> + { "parent", bplocpy_get_parent, NULL,
> + "Get the parent breakpoint object", NULL },
> + { "address", bplocpy_get_address, NULL,
> + "Get install address of this breakpoint location", NULL},
> + { "source", bplocpy_get_source_location, NULL,
> + "Get file and line number where breakpoint was installed", 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..784c3a02b8a 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's owner are the same; throw a Python
> + exception if it is not. */
"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;
In BPLOCPY_REQUIRE_VALID you say "LOCATION's owner", while here you say
LOCATION->OWNER. Wouldn't hurt to make them consistent.
> throw a Python
> + exception if it is not.
Ditto.
> 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..388f5f28e58
> --- /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/>. */
> +
> +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.
> +
> +# 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/>.
> +
> +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
> +gdb_test "python gdb.breakpoints()\[1\].enabled = False" "" "disable add master breakpoint"
> +gdb_breakpoint 32
> +
> +# Disable "master breakpoint" and test that all locations are disabled.
> +gdb_continue_to_breakpoint "end of main" ".*32.*"
>
> base-commit: 04f4c17c7a14ebb6c2212267b2ebc83f1376fe20
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v5] gdb/python: Add BreakpointLocation type
2022-05-30 18:46 ` [PATCH " Pedro Alves
@ 2022-05-31 14:14 ` Simon Farre
0 siblings, 0 replies; 4+ messages in thread
From: Simon Farre @ 2022-05-31 14:14 UTC (permalink / raw)
To: Pedro Alves; +Cc: Simon Farre via Gdb-patches
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-05-31 14:14 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-20 8:37 [PATCH v5] gdb/python: Add BreakpointLocation type Simon Farre
2022-05-14 22:16 ` [PING][PATCH " Simon Farre
2022-05-30 18:46 ` [PATCH " Pedro Alves
2022-05-31 14:14 ` Simon Farre
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).