* [PATCH v2] gdb/python: Add BreakpointLocation type
@ 2022-04-04 18:30 Simon Farre
2022-04-05 20:26 ` Bruno Larsen
0 siblings, 1 reply; 2+ messages in thread
From: Simon Farre @ 2022-04-04 18:30 UTC (permalink / raw)
To: gdb-patches
Updated docs & news file as per request. Feedback wanted & appreciated!
Currently, the Python API lacks the ability to
query breakpoints for their installed locations,
and subsequently, can't query any information about them either
(like source code file & line number, or address, etc).
This type aims to solve this problem by adding the type
gdb.BreakpointLocation. This type is never instantiated
by the user of the Python API directly, but instead is produced
by the gdb.Breakpoint.locations attribute, which
also is added in this patch, which returns a list of gdb.BreakpointLocation.
As it's introducing a new typethe patch itself is somewhat large (some 200~ lines)
but I think it's pretty self-contained and hopefully explained well enough.
gdb.Breakpoint.locations:
An attribute for retrieving the currently installed breakpoint
locations for gdb.Breakpoint. This functionality matches
that of the "info breakpoints" command in that it only
returns the last known/installed breakpoints, the returned list
itself does not get updated behind the scenes, from the Python
user's perspective. This is also described shortly in the documentation.
In this patch, BreakpointLocation contains 4 attributes
3 read-only attributes:
parent: returns the location owner's Python object
source: returns a file path and line number tuple: (string, long) / None
address: returns installed address of the location as (long)
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 for some other reason.
Therefore, at time of access, gdb.BreakpointLocation
checks it's bp_location->owner and compares it to the
Python companion object to breakpoint* it was provided
at creation to make sure that these match. This comparison
is done with the macros BPLOCPY_REQUIRE_VALID and
BPLOCPY_SET_REQUIRE_VALID, similar to the macros
that gdb.Breakpoint has been using (python-internal.h)
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.
---
gdb/NEWS | 5 +
gdb/breakpoint.c | 62 +++++++++++
gdb/breakpoint.h | 5 +
gdb/doc/python.texi | 49 ++++++++
gdb/python/py-breakpoint.c | 210 +++++++++++++++++++++++++++++++++++
gdb/python/python-internal.h | 33 ++++++
gdb/python/python.c | 1 +
7 files changed, 365 insertions(+)
diff --git a/gdb/NEWS b/gdb/NEWS
index 0fbc4537303..4289cebae19 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -18,6 +18,11 @@
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 1b340bd78d1..06b955c5ff4 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
@@ -13659,6 +13661,66 @@ 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 signals NOT FOUND. */
+
+static int
+find_loc_num_by_location(bp_location* loc) {
+ if (loc)
+ {
+ if (loc->owner)
+ {
+ int loc_num = 1;
+ // First; find out what LOC_NUM this location (currently) has.
+ 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..b066adf2889 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 installed for this
+breakpoint, with elements of type @code{gdb.BreakpointLocation} (described below).
+This functionality matches that of the @pxref{Set Breaks, info breakpoints}
+command 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 writeable.
+@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,46 @@ 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 (string, 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 writeable.
+@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 writeable.
+@end defvar
+
+@defvar BreakpointLocation.enabled
+This attribute holds the value for whether or not this location is enabled.
+This attribute is writeable (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 writeable.
+@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..d361402a02b 100644
--- a/gdb/python/py-breakpoint.c
+++ b/gdb/python/py-breakpoint.c
@@ -692,6 +692,36 @@ bppy_get_ignore_count (PyObject *self, void *closure)
return gdb_py_object_from_longest (self_bp->bp->ignore_count).release ();
}
+static PyObject*
+bppy_get_locations (PyObject *self, void *closure)
+{
+ 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_breakpoint_location_object *py_bploc = PyObject_New (gdbpy_breakpoint_location_object, &breakpoint_location_object_type);
+ bp_location_ref_ptr ref = bp_location_ref_ptr::new_reference(loc);
+ py_bploc->parent = self;
+
+ Py_INCREF(self);
+ // Append increases ref count of py_bploc
+ if(PyList_Append (list.get(), (PyObject*)py_bploc) == 0)
+ {
+ Py_DECREF((PyObject*)py_bploc);
+ }
+ else
+ {
+ Py_DECREF((PyObject*)py_bploc);
+ Py_DECREF(self);
+ return PyErr_Format (PyExc_RuntimeError, _("Failed to get breakpoint locations"));
+ }
+ py_bploc->bp_loc = ref.release();
+ }
+ return list.release();
+}
+
/* Internal function to validate the Python parameters/keywords
provided to bppy_init. */
@@ -1182,6 +1212,19 @@ gdbpy_initialize_breakpoints (void)
return 0;
}
+/* Initialize the Python BreakpointLocation code. */
+int
+gdbpy_initialize_breakpoint_locations(void)
+{
+ 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 +1307,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 +1375,168 @@ _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 *self_, PyObject *newvalue, void *closure)
+{
+ gdbpy_breakpoint_location_object *self = (gdbpy_breakpoint_location_object *) self_;
+ gdbpy_breakpoint_object *parent = (gdbpy_breakpoint_object *)self->parent;
+ BPPY_SET_REQUIRE_VALID (parent);
+ BPLOCPY_SET_REQUIRE_VALID(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 *self_, void *closure)
+{
+ gdbpy_breakpoint_location_object *self = (gdbpy_breakpoint_location_object *) self_;
+ gdbpy_breakpoint_object *parent = (gdbpy_breakpoint_object *)self->parent;
+ BPPY_REQUIRE_VALID (parent);
+ BPLOCPY_REQUIRE_VALID(parent, self);
+
+ // if the owner is disabled, all locations are disabled
+ if (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 *self_, void *closure)
+{
+ gdbpy_breakpoint_location_object *self = (gdbpy_breakpoint_location_object *) self_;
+ gdbpy_breakpoint_object *parent = (gdbpy_breakpoint_object *)self->parent;
+ BPPY_REQUIRE_VALID (parent);
+ BPLOCPY_REQUIRE_VALID(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 *self_, void *closure)
+{
+ gdbpy_breakpoint_location_object *self = (gdbpy_breakpoint_location_object *) self_;
+ BPPY_REQUIRE_VALID ((gdbpy_breakpoint_object *)self->parent);
+ BPLOCPY_REQUIRE_VALID((gdbpy_breakpoint_object *)self->parent, self);
+ return 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 *self_, void *closure)
+{
+ gdbpy_breakpoint_location_object *self = (gdbpy_breakpoint_location_object *) self_;
+ gdbpy_breakpoint_object *parent = (gdbpy_breakpoint_object *)self->parent;
+ BPPY_REQUIRE_VALID (parent);
+ BPLOCPY_REQUIRE_VALID(parent, self);
+ if(self->bp_loc->symtab)
+ {
+ PyObject* tuple = PyTuple_New(2);
+ // symtab->filename is never NULL
+ PyTuple_SetItem(tuple, 0, host_string_to_python_string (self->bp_loc->symtab->filename).release ());
+ PyTuple_SetItem(tuple, 1, PyLong_FromLong(self->bp_loc->line_number));
+ return tuple;
+ }
+ else
+ {
+ Py_RETURN_NONE;
+ }
+}
+
+/* De-allocation function to be called for the Python object. Decrement
+ reference count to the python companion object of the owner, as well as the
+ bp_location object. */
+static void
+bplocpy_dealloc(PyObject* self_)
+{
+ gdbpy_breakpoint_location_object *self = (gdbpy_breakpoint_location_object *) self_;
+ bp_location_ref_policy::decref(self->bp_loc);
+ Py_DECREF(self->parent);
+ Py_TYPE(self_)->tp_free(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 */
+};
\ No newline at end of file
diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
index d947b96033b..6ada82e9db5 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -284,6 +284,18 @@ extern PyTypeObject frame_object_type
CPYCHECKER_TYPE_OBJECT_FOR_TYPEDEF ("frame_object");
extern PyTypeObject thread_object_type
CPYCHECKER_TYPE_OBJECT_FOR_TYPEDEF ("thread_object");
+extern PyTypeObject breakpoint_location_object_type
+ CPYCHECKER_TYPE_OBJECT_FOR_TYPEDEF ("breakpoint_location_object");
+
+struct gdbpy_breakpoint_location_object
+{
+ PyObject_HEAD
+
+ bp_location* bp_loc;
+
+ /* Breakpoint parent. */
+ PyObject *parent;
+};
struct gdbpy_breakpoint_object
{
@@ -322,6 +334,25 @@ 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 +536,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 (void)
+ 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..f2aba38a021 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
base-commit: 552f1157c6262e274295e9c8a131de13fa7b00a3
--
2.32.0
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH v2] gdb/python: Add BreakpointLocation type
2022-04-04 18:30 [PATCH v2] gdb/python: Add BreakpointLocation type Simon Farre
@ 2022-04-05 20:26 ` Bruno Larsen
0 siblings, 0 replies; 2+ messages in thread
From: Bruno Larsen @ 2022-04-05 20:26 UTC (permalink / raw)
To: Simon Farre, gdb-patches
Hello Simon!
Thanks for taking this big task. I am not an expert on breakpoints - or really on anything - so maybe some of my questions are not exactly related to your patch, but I will do my best anyway.
Also, I can't approve any patches, but I hope my comments help with some easy fixes or at least get the conversation going
On 4/4/22 15:30, Simon Farre via Gdb-patches wrote:
> Updated docs & news file as per request. Feedback wanted & appreciated!
>
> Currently, the Python API lacks the ability to
> query breakpoints for their installed locations,
> and subsequently, can't query any information about them either
> (like source code file & line number, or address, etc).
>
> This type aims to solve this problem by adding the type
> gdb.BreakpointLocation. This type is never instantiated
> by the user of the Python API directly, but instead is produced
> by the gdb.Breakpoint.locations attribute, which
> also is added in this patch, which returns a list of gdb.BreakpointLocation.
> As it's introducing a new typethe patch itself is somewhat large (some 200~ lines)
> but I think it's pretty self-contained and hopefully explained well enough.
>
> gdb.Breakpoint.locations:
> An attribute for retrieving the currently installed breakpoint
> locations for gdb.Breakpoint. This functionality matches
> that of the "info breakpoints" command in that it only
> returns the last known/installed breakpoints, the returned list
> itself does not get updated behind the scenes, from the Python
> user's perspective. This is also described shortly in the documentation.
>
> In this patch, BreakpointLocation contains 4 attributes
>
> 3 read-only attributes:
> parent: returns the location owner's Python object
> source: returns a file path and line number tuple: (string, long) / None
> address: returns installed address of the location as (long)
>
> 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 for some other reason.
>
> Therefore, at time of access, gdb.BreakpointLocation
> checks it's bp_location->owner and compares it to the
> Python companion object to breakpoint* it was provided
> at creation to make sure that these match. This comparison
> is done with the macros BPLOCPY_REQUIRE_VALID and
> BPLOCPY_SET_REQUIRE_VALID, similar to the macros
> that gdb.Breakpoint has been using (python-internal.h)
>
> 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.
> ---
> gdb/NEWS | 5 +
> gdb/breakpoint.c | 62 +++++++++++
> gdb/breakpoint.h | 5 +
> gdb/doc/python.texi | 49 ++++++++
> gdb/python/py-breakpoint.c | 210 +++++++++++++++++++++++++++++++++++
> gdb/python/python-internal.h | 33 ++++++
> gdb/python/python.c | 1 +
> 7 files changed, 365 insertions(+)
>
> diff --git a/gdb/NEWS b/gdb/NEWS
> index 0fbc4537303..4289cebae19 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -18,6 +18,11 @@
> 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 1b340bd78d1..06b955c5ff4 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
> @@ -13659,6 +13661,66 @@ 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)
I find this name a bit confusing, maybe bp_location_set_enable would be better, following the same convention you used when making the python API.
> +{
> + if (loc == nullptr)
> + error (_("Breakpoint location is invalid."));
> +
> + if (loc->owner == nullptr)
> + error (_("Breakpoint location does not belong to an owner."));
Will this function be called by an end user directly (as part of the API) or is this a GDB-internal only function? If it is the second, I think you could have these "if"s as asserts. Makes for easier debugging if GDB throws a loud hissy fit instead of quietly complaining. It is a bad idea if this function can be directly called by the user, though, so take this advice with a pile of salt.
Also, you have a lot of trailing whitespaces in this patch. Almost every line in this function so far has them. I won't be pointing them all out, but you should be able to spot them by using `git show` and seeing where it showed red spots at the end of lines.
> +
> + 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);
Please change the 8 white spaces here to a tab. This is valid for all the patch, so I won't be poiting every single instance where it happens
> + }
> +
> + 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 signals NOT FOUND. */
> +
> +static int
> +find_loc_num_by_location(bp_location* loc) {
Please add a whitespace before the (
> + if (loc)
> + {
> + if (loc->owner)
> + {
> + int loc_num = 1;
Any specific reason why breakpointLocations are being indexed starting at 1, instead of 0, if errors are -1 anyway?
> + // First; find out what LOC_NUM this location (currently) has.
This comment looks like it is leftover from a former iteration of the code.
> + for(bp_location* it = loc->owner->loc; it != nullptr; it = it->next)
> + {
> + if(it == loc)
> + return loc_num;
> + loc_num++;
> + }
minor nit, but I would add the loc_num++ to the "for" line.
> + }
> + 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..b066adf2889 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 installed for this
> +breakpoint, with elements of type @code{gdb.BreakpointLocation} (described below).
> +This functionality matches that of the @pxref{Set Breaks, info breakpoints}
> +command 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 writeable.
> +@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,46 @@ 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 (string, 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 writeable.
> +@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 writeable.
> +@end defvar
> +
> +@defvar BreakpointLocation.enabled
> +This attribute holds the value for whether or not this location is enabled.
> +This attribute is writeable (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 writeable.
> +@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..d361402a02b 100644
> --- a/gdb/python/py-breakpoint.c
> +++ b/gdb/python/py-breakpoint.c
> @@ -692,6 +692,36 @@ bppy_get_ignore_count (PyObject *self, void *closure)
> return gdb_py_object_from_longest (self_bp->bp->ignore_count).release ();
> }
>
> +static PyObject*
> +bppy_get_locations (PyObject *self, void *closure)
> +{
> + 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_breakpoint_location_object *py_bploc = PyObject_New (gdbpy_breakpoint_location_object, &breakpoint_location_object_type);
> + bp_location_ref_ptr ref = bp_location_ref_ptr::new_reference(loc);
> + py_bploc->parent = self;
> +
> + Py_INCREF(self);
> + // Append increases ref count of py_bploc
Please use /* */ instead of // for comments. Also, comments should end with periods and two white spaces before the */
> + if(PyList_Append (list.get(), (PyObject*)py_bploc) == 0)
> + {
> + Py_DECREF((PyObject*)py_bploc);
> + }
> + else
> + {
> + Py_DECREF((PyObject*)py_bploc);
> + Py_DECREF(self);
> + return PyErr_Format (PyExc_RuntimeError, _("Failed to get breakpoint locations"));
> + }
> + py_bploc->bp_loc = ref.release();
> + }
> + return list.release();
> +}
> +
> /* Internal function to validate the Python parameters/keywords
> provided to bppy_init. */
>
> @@ -1182,6 +1212,19 @@ gdbpy_initialize_breakpoints (void)
> return 0;
> }
>
> +/* Initialize the Python BreakpointLocation code. */
> +int
> +gdbpy_initialize_breakpoint_locations(void)
> +{
> + 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 +1307,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 +1375,168 @@ _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 *self_, PyObject *newvalue, void *closure)
> +{
> + gdbpy_breakpoint_location_object *self = (gdbpy_breakpoint_location_object *) self_;
> + gdbpy_breakpoint_object *parent = (gdbpy_breakpoint_object *)self->parent;
> + BPPY_SET_REQUIRE_VALID (parent);
> + BPLOCPY_SET_REQUIRE_VALID(parent, self);
You missed a lot of white spaces before ( up until this point. I won't be listing them all to not sounds too aggressive.
> +
> + 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);
This identation here is a bit off, should be a tab.
> + }
> + 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 *self_, void *closure)
> +{
> + gdbpy_breakpoint_location_object *self = (gdbpy_breakpoint_location_object *) self_;
> + gdbpy_breakpoint_object *parent = (gdbpy_breakpoint_object *)self->parent;
> + BPPY_REQUIRE_VALID (parent);
> + BPLOCPY_REQUIRE_VALID(parent, self);
> +
> + // if the owner is disabled, all locations are disabled
Same comments about whitespaces and comments
> + if (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 *self_, void *closure)
> +{
> + gdbpy_breakpoint_location_object *self = (gdbpy_breakpoint_location_object *) self_;
> + gdbpy_breakpoint_object *parent = (gdbpy_breakpoint_object *)self->parent;
> + BPPY_REQUIRE_VALID (parent);
> + BPLOCPY_REQUIRE_VALID(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 *self_, void *closure)
> +{
> + gdbpy_breakpoint_location_object *self = (gdbpy_breakpoint_location_object *) self_;
> + BPPY_REQUIRE_VALID ((gdbpy_breakpoint_object *)self->parent);
> + BPLOCPY_REQUIRE_VALID((gdbpy_breakpoint_object *)self->parent, self);
> + return 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 *self_, void *closure)
> +{
> + gdbpy_breakpoint_location_object *self = (gdbpy_breakpoint_location_object *) self_;
> + gdbpy_breakpoint_object *parent = (gdbpy_breakpoint_object *)self->parent;
> + BPPY_REQUIRE_VALID (parent);
> + BPLOCPY_REQUIRE_VALID(parent, self);
> + if(self->bp_loc->symtab)
> + {
> + PyObject* tuple = PyTuple_New(2);
> + // symtab->filename is never NULL
> + PyTuple_SetItem(tuple, 0, host_string_to_python_string (self->bp_loc->symtab->filename).release ());
> + PyTuple_SetItem(tuple, 1, PyLong_FromLong(self->bp_loc->line_number));
> + return tuple;
> + }
> + else
> + {
> + Py_RETURN_NONE;
> + }
> +}
> +
> +/* De-allocation function to be called for the Python object. Decrement
> + reference count to the python companion object of the owner, as well as the
> + bp_location object. */
> +static void
> +bplocpy_dealloc(PyObject* self_)
> +{
> + gdbpy_breakpoint_location_object *self = (gdbpy_breakpoint_location_object *) self_;
> + bp_location_ref_policy::decref(self->bp_loc);
> + Py_DECREF(self->parent);
> + Py_TYPE(self_)->tp_free(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 */
> +};
> \ No newline at end of file
> diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
> index d947b96033b..6ada82e9db5 100644
> --- a/gdb/python/python-internal.h
> +++ b/gdb/python/python-internal.h
> @@ -284,6 +284,18 @@ extern PyTypeObject frame_object_type
> CPYCHECKER_TYPE_OBJECT_FOR_TYPEDEF ("frame_object");
> extern PyTypeObject thread_object_type
> CPYCHECKER_TYPE_OBJECT_FOR_TYPEDEF ("thread_object");
> +extern PyTypeObject breakpoint_location_object_type
> + CPYCHECKER_TYPE_OBJECT_FOR_TYPEDEF ("breakpoint_location_object");
> +
> +struct gdbpy_breakpoint_location_object
> +{
> + PyObject_HEAD
> +
> + bp_location* bp_loc;
> +
> + /* Breakpoint parent. */
> + PyObject *parent;
> +};
>
> struct gdbpy_breakpoint_object
> {
> @@ -322,6 +334,25 @@ 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)
I don't understand the difference between these two macros, and the name looks a bit confusing. since it says "require valid", I would expect some form of explicit validity check, and that the second macro would somehow set the breakpoint to be valid.
I feel like the comment could be changed to be something like "breakpoint location is considered valid if it's owner is the given breakpoint", and the arguments be flipped around, so it is more clear that we are testing the validity of the _location_ not the breakpoint.
finally, for the names, maybe the first macro could be renamed to BPLOCPY_REQUIRE_VALID_RETURN_ERROR, and the second either renamed to BPLOCPY_REQUIRED_VALID or BPLOCPY_REQUIRED_VALID_RETURN_INT, so the changes are more noticeable.
>
> /* Variables used to pass information between the Breakpoint
> constructor and the breakpoint-created hook function. */
> @@ -505,6 +536,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 (void)
> + 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..f2aba38a021 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
>
> base-commit: 552f1157c6262e274295e9c8a131de13fa7b00a3
--
Cheers!
Bruno Larsen
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-04-05 20:26 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-04 18:30 [PATCH v2] gdb/python: Add BreakpointLocation type Simon Farre
2022-04-05 20:26 ` Bruno Larsen
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).