Re: [PATCH] Fix Python probe breakpoints

[Simon Marchi <simark@simark.ca>]

On 2019-12-07 11:46 a.m., George Barrett wrote:
> The documentation for the `spec' variant of the gdb.Breakpoint
> constructor states that the accepted format is the same as the break
> command. However, using the -probe qualifier at the beginning of the
> breakpoint specifier 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 scripts.
> 
> This patch changes the constructor to test whether the parsed breakpoint
> location is a probe, and if so it uses the probe-specific breakpoint ops
> instead.

Hi George,

Thanks for the patch, especially for providing a test case along with it.

> 
> gdb/ChangeLog:
> 2019-12-08  George Barrett  <bob@bob131.so>
> 
> 	Fix Python probe breakpoints.
> 	* breakpoint.c: Make bkpt_probe_breakpoint_ops non-static.
> 	* breakpoint.h: Add declaration for bkpt_probe_breakpoint_ops.
> 	* python/py-breakpoint.c: Use probe ops if the specifier is a
> 	probe specifier.

Put the name of the modified variable or function in parenthesis after the file
name, like:

	* breakpoint.c (bkpt_probe_breakpoint_ops): Make non-static.

> 
> gdb/testsuite/ChangeLog:
> 2019-12-08  George Barrett  <bob@bob131.so>
> 
> 	Test Python probe breakpoints.
> 	* gdb.python/py-breakpoint.c: Add probe point.
> 	* gdb.python/py-breakpoint.exp: Add probe specifier test.
> ---
>  gdb/breakpoint.c                           |  2 +-
>  gdb/breakpoint.h                           |  1 +
>  gdb/python/py-breakpoint.c                 |  5 ++++-
>  gdb/testsuite/gdb.python/py-breakpoint.c   |  7 +++++++
>  gdb/testsuite/gdb.python/py-breakpoint.exp | 22 ++++++++++++++++++++++
>  5 files changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 583f46d852..0e3f6d954a 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -244,7 +244,7 @@ static struct breakpoint_ops momentary_breakpoint_ops;
>  struct breakpoint_ops bkpt_breakpoint_ops;
>  
>  /* Breakpoints set on probes.  */
> -static struct breakpoint_ops bkpt_probe_breakpoint_ops;
> +struct breakpoint_ops bkpt_probe_breakpoint_ops;
>  
>  /* Dynamic printf class type.  */
>  struct breakpoint_ops dprintf_breakpoint_ops;
> diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
> index a9d689d02a..47ca1174ab 100644
> --- a/gdb/breakpoint.h
> +++ b/gdb/breakpoint.h
> @@ -1307,6 +1307,7 @@ extern void tbreak_command (const char *, int);
>  extern struct breakpoint_ops base_breakpoint_ops;
>  extern struct breakpoint_ops bkpt_breakpoint_ops;
>  extern struct breakpoint_ops tracepoint_breakpoint_ops;
> +extern struct breakpoint_ops bkpt_probe_breakpoint_ops;
>  extern struct breakpoint_ops dprintf_breakpoint_ops;
>  
>  extern void initialize_breakpoint_ops (void);
> diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
> index 4170737416..ddd8eeecae 100644
> --- a/gdb/python/py-breakpoint.c
> +++ b/gdb/python/py-breakpoint.c
> @@ -834,7 +834,10 @@ bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
>  			       temporary_bp, bp_breakpoint,
>  			       0,
>  			       AUTO_BOOLEAN_TRUE,
> -			       &bkpt_breakpoint_ops,
> +			       (event_location_type (location.get ())
> +				 == PROBE_LOCATION
> +				    ? &bkpt_probe_breakpoint_ops
> +				    : &bkpt_breakpoint_ops),
>  			       0, 1, internal_bp, 0);
>  	    break;
>  	  }

A suggestion to improve maintainability.  To avoid duplicating the logic, define a function
"breakpoint_ops_for_event_location_type" that does this conversion, and use it in the
break_command_1 function too.  Can you check if Guile exhibits the same problem?  I'm guessing
that the gdbscm_register_breakpoint_x should receive the same treatment.

> diff --git a/gdb/testsuite/gdb.python/py-breakpoint.c b/gdb/testsuite/gdb.python/py-breakpoint.c
> index d102a5f306..bf5bec1200 100644
> --- a/gdb/testsuite/gdb.python/py-breakpoint.c
> +++ b/gdb/testsuite/gdb.python/py-breakpoint.c
> @@ -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
> +      STAP_PROBE1 (test, result_updated, result);
> +#endif
>      }
>  
>    return 0; /* Break at end. */
> diff --git a/gdb/testsuite/gdb.python/py-breakpoint.exp b/gdb/testsuite/gdb.python/py-breakpoint.exp
> index 625977c0ad..9e3cb475af 100644
> --- a/gdb/testsuite/gdb.python/py-breakpoint.exp
> +++ b/gdb/testsuite/gdb.python/py-breakpoint.exp
> @@ -695,6 +695,27 @@ 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}] } {
> +	untested "breakpoint probe test failed"

Hmm, this should say that the compilation failed (not the test), like
"breakpoint probe test compilation failed". But anyway, prepare_for_testing
already prints an "untested":

  UNTESTED: gdb.python/py-breakpoint.exp: test_bkpt_probe: failed to prepare

so I think you could just remove it.

> +	return -1
> +    }
> +
> +    if ![runto_main] then {
> +	fail "cannot run to main."
> +	return 0
> +    }
> +
> +    delete_breakpoints

If this delete_breakpoints isn't necessary, let's remove it.

Thanks,

Simon