On 2019-12-09 3:18 p.m., George Barrett wrote:
> The documentation for make-breakpoint from the Guile API and the `spec'
> variant of the gdb.Breakpoint constructor from the Python API state that
> the format acceptable for location strings is the same as that accepted
> by the break command. However, using the -probe qualifier at the
> beginning of the location string causes a GDB internal error as it
> attempts to decode a probe location in the wrong code path. Without this
> functionality, there doesn't appear to be another way to set breakpoints
> on probe points from Python or Guile scripts.
>
> This patch introduces a new helper function that returns a
> breakpoint_ops instance appropriate for a parsed location and updates
> the Guile and Python bindings to use said function, rather than the
> current hard-coded use of bkpt_breakpoint_ops. Since this logic is
> duplicated in the handling of the `break' and `trace' commands, those
> are also updated to call into the new helper function.
Hi George,
breakpoint_ops_for_event_location_type is only used in breakpoint.c, so it should
be made static. Otherwise, I noted a few styling comments, nothing major.
>
> gdb/ChangeLog:
> 2019-12-10 George Barrett <bob@bob131.so>
>
> Fix scripted probe breakpoints.
> * breakpoint.c (tracepoint_probe_breakpoint_ops): Move
> declaration forward
End with a period.
> (breakpoint_ops_for_event_location_type)
> (breakpoint_ops_for_event_location): Add function definitions.
> (break_command_1, trace_command): Use
> breakpoint_ops_for_event_location.
> * breakpoint.h (breakpoint_ops_for_event_location_type)
> (breakpoint_ops_for_event_location): Add function declarations.
> * guile/scm-breakpoint.c (gdbscm_register_breakpoint_x): Use
> breakpoint_ops_for_event_location.
> * python/py-breakpoint.c (bppy_init): Use
> breakpoint_ops_for_event_location.
>
> gdb/testsuite/ChangeLog:
> 2019-12-10 George Barrett <bob@bob131.so>
>
> Test scripted probe breakpoints.
> * gdb.guile/scm-breakpoint.c (main): Add probe point.
> * gdb.python/py-breakpoint.c (main): Likewise.
> * gdb.guile/scm-breakpoint.exp (test_bkpt_probe): Add probe
> specifier test.
> * gdb.python/py-breakpoint.exp (test_bkpt_probe): Likewise.
> ---
> gdb/breakpoint.c | 61 +++++++++++++++-------
> gdb/breakpoint.h | 14 +++++
> gdb/guile/scm-breakpoint.c | 4 +-
> gdb/python/py-breakpoint.c | 5 +-
> gdb/testsuite/gdb.guile/scm-breakpoint.c | 7 +++
> gdb/testsuite/gdb.guile/scm-breakpoint.exp | 23 ++++++++
> gdb/testsuite/gdb.python/py-breakpoint.c | 7 +++
> gdb/testsuite/gdb.python/py-breakpoint.exp | 20 +++++++
> 8 files changed, 120 insertions(+), 21 deletions(-)
>
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 583f46d852..4296584dd3 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -246,6 +246,9 @@ struct breakpoint_ops bkpt_breakpoint_ops;
> /* Breakpoints set on probes. */
> static struct breakpoint_ops bkpt_probe_breakpoint_ops;
>
> +/* Breakpoints on tracepoints placed in a static probe. */
> +static struct breakpoint_ops tracepoint_probe_breakpoint_ops;
This comment doesn't really make sense to me. I would reduce it to:
/* Tracepoints set on probes. */
> +
> /* Dynamic printf class type. */
> struct breakpoint_ops dprintf_breakpoint_ops;
>
> @@ -9165,6 +9168,40 @@ decode_static_tracepoint_spec (const char **arg_p)
>
> /* See breakpoint.h. */
>
> +const struct breakpoint_ops *
> +breakpoint_ops_for_event_location_type (enum event_location_type location_type,
> + int is_tracepoint)
int -> bool
And use true/false at call sites.
> +{
> + if (is_tracepoint) {
> + if (location_type == PROBE_LOCATION) {
> + return &tracepoint_probe_breakpoint_ops;
> + } else {
> + return &tracepoint_breakpoint_ops;
> + }
GNU style would use braces like this:
if (is_tracepoint
{
if (location_type == PROBE_LOCATION)
{
...
}
}
But when there's just a single-line statement, we get rid of the braces, so
it would become:
if (is_tracepoint
{
if (location_type == PROBE_LOCATION)
return ...
}
> + } else {
> + if (location_type == PROBE_LOCATION) {
> + return &bkpt_probe_breakpoint_ops;
> + } else {
> + return &bkpt_breakpoint_ops;
> + }
> + }
> +}
> +
> +/* See breakpoint.h. */
> +
> +const struct breakpoint_ops *
> +breakpoint_ops_for_event_location (const struct event_location *location,
> + int is_tracepoint)
int -> bool
> +{
> + if (location) {
> + return breakpoint_ops_for_event_location_type
> + (event_location_type (location), is_tracepoint);
> + }
Fix the braces here too.
> diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
> index 4170737416..ea6e11ed02 100644
> --- a/gdb/python/py-breakpoint.c
> +++ b/gdb/python/py-breakpoint.c
> @@ -799,6 +799,7 @@ bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
> = (qualified != NULL && PyObject_IsTrue (qualified)
> ? symbol_name_match_type::FULL
> : symbol_name_match_type::WILD);
> + const struct breakpoint_ops *ops;
Declare this below, where it's initialized (in the same statement).
Simon
@@ -246,6 +246,9 @@ struct breakpoint_ops bkpt_breakpoint_ops;
/* Breakpoints set on probes. */
static struct breakpoint_ops bkpt_probe_breakpoint_ops;
+/* Breakpoints on tracepoints placed in a static probe. */
+static struct breakpoint_ops tracepoint_probe_breakpoint_ops;
+
/* Dynamic printf class type. */
struct breakpoint_ops dprintf_breakpoint_ops;
@@ -9165,6 +9168,40 @@ decode_static_tracepoint_spec (const char **arg_p)
/* See breakpoint.h. */
+const struct breakpoint_ops *
+breakpoint_ops_for_event_location_type (enum event_location_type location_type,
+ int is_tracepoint)
+{
+ if (is_tracepoint) {
+ if (location_type == PROBE_LOCATION) {
+ return &tracepoint_probe_breakpoint_ops;
+ } else {
+ return &tracepoint_breakpoint_ops;
+ }
+ } else {
+ if (location_type == PROBE_LOCATION) {
+ return &bkpt_probe_breakpoint_ops;
+ } else {
+ return &bkpt_breakpoint_ops;
+ }
+ }
+}
+
+/* See breakpoint.h. */
+
+const struct breakpoint_ops *
+breakpoint_ops_for_event_location (const struct event_location *location,
+ int is_tracepoint)
+{
+ if (location) {
+ return breakpoint_ops_for_event_location_type
+ (event_location_type (location), is_tracepoint);
+ }
+ return is_tracepoint ? &tracepoint_breakpoint_ops : &bkpt_breakpoint_ops;
+}
+
+/* See breakpoint.h. */
+
int
create_breakpoint (struct gdbarch *gdbarch,
const struct event_location *location,
@@ -9344,16 +9381,10 @@ break_command_1 (const char *arg, int flag, int from_tty)
enum bptype type_wanted = (flag & BP_HARDWAREFLAG
? bp_hardware_breakpoint
: bp_breakpoint);
- struct breakpoint_ops *ops;
event_location_up location = string_to_event_location (&arg, current_language);
-
- /* Matching breakpoints on probes. */
- if (location != NULL
- && event_location_type (location.get ()) == PROBE_LOCATION)
- ops = &bkpt_probe_breakpoint_ops;
- else
- ops = &bkpt_breakpoint_ops;
+ const struct breakpoint_ops *ops =
+ breakpoint_ops_for_event_location (location.get (), 0 /* is_tracepoint */);
create_breakpoint (get_current_arch (),
location.get (),
@@ -12802,8 +12833,7 @@ tracepoint_decode_location (struct breakpoint *b,
struct breakpoint_ops tracepoint_breakpoint_ops;
-/* The breakpoint_ops structure to be use on tracepoints placed in a
- static probe. */
+/* Virtual table for tracepoints on static probes. */
static void
tracepoint_probe_create_sals_from_location
@@ -12824,8 +12854,6 @@ tracepoint_probe_decode_location (struct breakpoint *b,
return bkpt_probe_decode_location (b, location, search_pspace);
}
-static struct breakpoint_ops tracepoint_probe_breakpoint_ops;
-
/* Dprintf breakpoint_ops methods. */
static void
@@ -14467,15 +14495,10 @@ set_tracepoint_count (int num)
static void
trace_command (const char *arg, int from_tty)
{
- struct breakpoint_ops *ops;
-
event_location_up location = string_to_event_location (&arg,
current_language);
- if (location != NULL
- && event_location_type (location.get ()) == PROBE_LOCATION)
- ops = &tracepoint_probe_breakpoint_ops;
- else
- ops = &tracepoint_breakpoint_ops;
+ const struct breakpoint_ops *ops =
+ breakpoint_ops_for_event_location (location.get (), 1 /* is_tracepoint */);
create_breakpoint (get_current_arch (),
location.get (),
@@ -1352,6 +1352,20 @@ extern void init_catchpoint (struct breakpoint *b,
extern void install_breakpoint (int internal, std::unique_ptr<breakpoint> &&b,
int update_gll);
+/* Returns the breakpoint ops appropriate for use with with LOCATION_TYPE and
+ according to IS_TRACEPOINT. Use this to ensure, for example, that you pass
+ the correct ops to create_breakpoint for probe locations. */
+
+extern const struct breakpoint_ops *breakpoint_ops_for_event_location_type
+ (enum event_location_type location_type, int is_tracepoint);
+
+/* Convenience wrapper for breakpoint_ops_for_event_location_type. If LOCATION
+ is null, returns bkpt_breakpoint_ops (or tracepoint_breakpoint_ops, if
+ IS_TRACEPOINT is non-zero). */
+
+extern const struct breakpoint_ops *breakpoint_ops_for_event_location
+ (const struct event_location *location, int is_tracepoint);
+
/* Flags that can be passed down to create_breakpoint, etc., to affect
breakpoint creation in several ways. */
@@ -437,13 +437,15 @@ gdbscm_register_breakpoint_x (SCM self)
{
case bp_breakpoint:
{
+ const breakpoint_ops *ops =
+ breakpoint_ops_for_event_location (eloc.get (), 0);
create_breakpoint (get_current_arch (),
eloc.get (), NULL, -1, NULL,
0,
0, bp_breakpoint,
0,
AUTO_BOOLEAN_TRUE,
- &bkpt_breakpoint_ops,
+ ops,
0, 1, internal, 0);
break;
}
@@ -799,6 +799,7 @@ bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
= (qualified != NULL && PyObject_IsTrue (qualified)
? symbol_name_match_type::FULL
: symbol_name_match_type::WILD);
+ const struct breakpoint_ops *ops;
if (spec != NULL)
{
@@ -828,13 +829,15 @@ bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
location = new_explicit_location (&explicit_loc);
}
+ ops = breakpoint_ops_for_event_location (location.get (), 0);
+
create_breakpoint (python_gdbarch,
location.get (), NULL, -1, NULL,
0,
temporary_bp, bp_breakpoint,
0,
AUTO_BOOLEAN_TRUE,
- &bkpt_breakpoint_ops,
+ ops,
0, 1, internal_bp, 0);
break;
}
@@ -15,6 +15,10 @@
You should have received a copy of the GNU General Public License
along with this program. If not, see <http://www.gnu.org/licenses/>. */
+#ifdef USE_PROBES
+#include <sys/sdt.h>
+#endif
+
int result = 0;
int multiply (int i)
@@ -38,6 +42,9 @@ int main (int argc, char *argv[])
{
result += multiply (foo); /* Break at multiply. */
result += add (bar); /* Break at add. */
+#ifdef USE_PROBES
+ DTRACE_PROBE1 (test, result_updated, result);
+#endif
}
return 0; /* Break at end. */
@@ -499,6 +499,28 @@ proc test_bkpt_address {} {
".*Breakpoint ($decimal)+ at .*$srcfile, line ($decimal)+\."
}
+proc test_bkpt_probe {} {
+ global decimal hex testfile srcfile
+
+ if { [prepare_for_testing "failed to prepare" ${testfile}-probes \
+ ${srcfile} {additional_flags=-DUSE_PROBES}] } {
+ return -1
+ }
+
+ if ![gdb_guile_runto_main] then {
+ return
+ }
+
+ gdb_scm_test_silent_cmd \
+ "guile (define bp1 (make-breakpoint \"-probe test:result_updated\"))" \
+ "create probe breakpoint"
+
+ gdb_test \
+ "guile (register-breakpoint! bp1)" \
+ "Breakpoint $decimal at $hex" \
+ "register probe breakpoint"
+}
+
test_bkpt_basic
test_bkpt_deletion
test_bkpt_cond_and_cmds
@@ -508,3 +530,4 @@ test_bkpt_internal
test_bkpt_eval_funcs
test_bkpt_registration
test_bkpt_address
+test_bkpt_probe
@@ -15,6 +15,10 @@
You should have received a copy of the GNU General Public License
along with this program. If not, see <http://www.gnu.org/licenses/>. */
+#ifdef USE_PROBES
+#include <sys/sdt.h>
+#endif
+
int result = 0;
namespace foo_ns
@@ -46,6 +50,9 @@ int main (int argc, char *argv[])
{
result += multiply (foo); /* Break at multiply. */
result += add (bar); /* Break at add. */
+#ifdef USE_PROBES
+ DTRACE_PROBE1 (test, result_updated, result);
+#endif
}
return 0; /* Break at end. */
@@ -695,6 +695,25 @@ proc_with_prefix test_bkpt_qualified {} {
"-q in spec string and qualified false"
}
+proc_with_prefix test_bkpt_probe {} {
+ global decimal hex testfile srcfile
+
+ if { [prepare_for_testing "failed to prepare" ${testfile}-probes \
+ ${srcfile} {debug c++ additional_flags=-DUSE_PROBES}] } {
+ return -1
+ }
+
+ if ![runto_main] then {
+ fail "cannot run to main."
+ return 0
+ }
+
+ gdb_test \
+ "python gdb.Breakpoint(\"-probe test:result_updated\")" \
+ "Breakpoint $decimal at $hex" \
+ "-probe in spec string"
+}
+
test_bkpt_basic
test_bkpt_deletion
test_bkpt_cond_and_cmds
@@ -708,3 +727,4 @@ test_bkpt_pending
test_bkpt_events
test_bkpt_explicit_loc
test_bkpt_qualified
+test_bkpt_probe