Re: [PATCH v2] Fix scripted probe breakpoints

[Simon Marchi <simark@simark.ca>]

On 2019-12-09 4:28 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.

Thanks, that LGTM.  I presume you don't have push access, so would you like
me to push the patch on your behalf?

I found some other really small nits, I'll fix them before pushing if I end up
pushing for you.

> +/* See breakpoint.h.  */
> +
> +const struct breakpoint_ops *
> +breakpoint_ops_for_event_location (const struct event_location *location,
> +				   bool is_tracepoint)
> +{
> +  if (location)

I missed this last time, but also in GNU style we compare pointers and integers explicitly,
so either one of these would be good:

  if (location != NULL)
  if (location != nullptr)

> diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
> index a9d689d02a..071ae79d68 100644
> --- a/gdb/breakpoint.h
> +++ b/gdb/breakpoint.h
> @@ -1352,6 +1352,15 @@ 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 and
> +   according to IS_TRACEPOINT.  Use this to ensure, for example, that you pass
> +   the correct ops to create_breakpoint for probe locations.  If LOCATION is
> +   null, returns bkpt_breakpoint_ops (or tracepoint_breakpoint_ops, if

I would change null to NULL or nullptr to match the language nomenclature.

> +   IS_TRACEPOINT is non-zero).  */

non-zero -> true

Simon