From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 99192 invoked by alias); 8 Dec 2019 02:35:35 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 99175 invoked by uid 89); 8 Dec 2019 02:35:35 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-18.3 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.1 spammy=UNTESTED X-HELO: simark.ca Received: from simark.ca (HELO simark.ca) (158.69.221.121) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sun, 08 Dec 2019 02:35:32 +0000 Received: from [10.0.0.11] (unknown [192.222.164.54]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id C51D91E5F1; Sat, 7 Dec 2019 21:35:30 -0500 (EST) From: Simon Marchi Subject: Re: [PATCH] Fix Python probe breakpoints To: George Barrett , gdb-patches@sourceware.org References: Message-ID: <6368e25a-ee80-6b07-e42a-363ab2dcbd7c@simark.ca> Date: Sun, 08 Dec 2019 02:35:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2019-12/txt/msg00265.txt.bz2 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 > > 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 > > 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 . */ > > +#ifdef USE_PROBES > +#include > +#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