[PATCH] Fix scripted probe breakpoints

[Simon Marchi]

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