Fix Python probe breakpoints
Commit Message
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.
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.
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(-)
Comments
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
On Sat, Dec 07, 2019 at 09:35:30PM -0500, Simon Marchi wrote:
> 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.
Thanks for the correction, I was having a bit of trouble making sure I
understood the instructions on the wiki correctly.
> 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.
Sure. Should the addition of that function be a separate patch?
> Can you check if Guile exhibits the same problem? I'm guessing that the
> gdbscm_register_breakpoint_x should receive the same treatment.
A quick peek at the source indicates the Guile code has the same problem, but
I can't really test it as it fails to build against recent libguile.
> On 2019-12-07 11:46 a.m., George Barrett wrote:
> > 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. */
Just to check: is it okay to introduce a SystemTap dependency here? I just
realised this could be made slightly more portable by using DTRACE_PROBE1
instead of STAP_PROBE1; AFAICT that should work on BSD and Solaris, but sdt.h
is probably not a standard header on those platforms either and it might still
exclude things like Cygwin...
> 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.
Ack.
> If this delete_breakpoints isn't necessary, let's remove it.
Ack.
> Thanks,
Thanks
On 2019-12-07 10:10 p.m., George Barrett wrote:
> On Sat, Dec 07, 2019 at 09:35:30PM -0500, Simon Marchi wrote:
>> 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.
>
> Thanks for the correction, I was having a bit of trouble making sure I
> understood the instructions on the wiki correctly.
Well it was really not bad for a first time!
>> 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.
>
> Sure. Should the addition of that function be a separate patch?
I think this is simple enough to be in the same patch. If there had been dozens
of call sites to fix, it would have made sense to introduce the function in a
separate patch, but I don't think it's necessary here.
>> Can you check if Guile exhibits the same problem? I'm guessing that the
>> gdbscm_register_breakpoint_x should receive the same treatment.
>
> A quick peek at the source indicates the Guile code has the same problem, but
> I can't really test it as it fails to build against recent libguile.
Indeed, it only builds with guile 2.0 (someone needs to port the code to guile 2.2).
Most distros have a "guile-2.0" package (the name may vary). When installed, it's
then possible to configure gdb to use it with the --with-guile=guile-2.0 configure flag.
>> On 2019-12-07 11:46 a.m., George Barrett wrote:
>>> 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. */
>
> Just to check: is it okay to introduce a SystemTap dependency here? I just
> realised this could be made slightly more portable by using DTRACE_PROBE1
> instead of STAP_PROBE1; AFAICT that should work on BSD and Solaris, but sdt.h
> is probably not a standard header on those platforms either and it might still
> exclude things like Cygwin...
I'm not very familiar with this, but indeed using DTRACE_PROBE1 looks like a good
idea. The sys/sdt.h headers seems to be the right one on FreeBSD and Solaris. If
it's not, it won't be hard to fix for whoever wants to fix it.
Btw, I expect that your patch will be small enough that it doesn't require a copyright
assignment. But if you plan on contributing patches larger than this, you will need
to file a copyright assignment with the FSF, contact me off-list if you'd like to do that.
Thanks,
Simon
>>>>> "George" == George Barrett <bob@bob131.so> writes:
George> The documentation for the `spec' variant of the gdb.Breakpoint
George> constructor states that the accepted format is the same as the break
George> command. However, using the -probe qualifier at the beginning of the
George> breakpoint specifier causes a GDB internal error as it attempts to
George> decode a probe location in the wrong code path. Without this
George> functionality, there doesn't appear to be another way to set breakpoints
George> on probe points from Python scripts.
Thank you for the patch.
George> gdb/ChangeLog:
George> 2019-12-08 George Barrett <bob@bob131.so>
George> Fix Python probe breakpoints.
George> * breakpoint.c: Make bkpt_probe_breakpoint_ops non-static.
George> * breakpoint.h: Add declaration for bkpt_probe_breakpoint_ops.
George> * python/py-breakpoint.c: Use probe ops if the specifier is a
George> probe specifier.
George> gdb/testsuite/ChangeLog:
George> 2019-12-08 George Barrett <bob@bob131.so>
George> Test Python probe breakpoints.
George> * gdb.python/py-breakpoint.c: Add probe point.
George> * gdb.python/py-breakpoint.exp: Add probe specifier test.
I wasn't sure about this approach, but after some reflection, I guess I
can accept it. Mostly I was concerned that the Python API here would be
sort of ugly. But, maybe it's not so bad.
So, this is ok.
Tom
@@ -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;
@@ -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);
@@ -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;
}
@@ -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. */
@@ -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"
+ return -1
+ }
+
+ if ![runto_main] then {
+ fail "cannot run to main."
+ return 0
+ }
+
+ delete_breakpoints
+ 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 +729,4 @@ test_bkpt_pending
test_bkpt_events
test_bkpt_explicit_loc
test_bkpt_qualified
+test_bkpt_probe