From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27479 invoked by alias); 8 Dec 2019 15:44:02 -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 27470 invoked by uid 89); 8 Dec 2019 15:44:02 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-18.4 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=whoever 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 15:44:00 +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 89E791E092; Sun, 8 Dec 2019 10:43:58 -0500 (EST) Subject: Re: [PATCH] Fix Python probe breakpoints To: George Barrett Cc: gdb-patches@sourceware.org References: <6368e25a-ee80-6b07-e42a-363ab2dcbd7c@simark.ca> From: Simon Marchi Message-ID: <4aa906df-8cd4-3571-76fe-97b15422eff9@simark.ca> Date: Sun, 08 Dec 2019 15:44:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.3.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2019-12/txt/msg00272.txt.bz2 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 . */ >>> >>> +#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. */ > > 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