From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from lndn.lancelotsix.com (lndn.lancelotsix.com [51.195.220.111]) by sourceware.org (Postfix) with ESMTPS id 5F4E23858CDA for ; Sun, 25 Sep 2022 18:11:31 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 5F4E23858CDA Received: from ubuntu.lan (cust120-dsl54.idnet.net [212.69.54.120]) by lndn.lancelotsix.com (Postfix) with ESMTPSA id 4268F8758D; Sun, 25 Sep 2022 18:11:30 +0000 (UTC) Date: Sun, 25 Sep 2022 18:10:53 +0000 From: Lancelot SIX To: Johnson Sun Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] [PR python/29603] Fix deletion of Watchpoints Message-ID: <20220925181053.mscd3dlvndgjzluu@ubuntu.lan> References: <20220925053349.918439-1-j3.soon777@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20220925053349.918439-1-j3.soon777@gmail.com> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.5.11 (lndn.lancelotsix.com [0.0.0.0]); Sun, 25 Sep 2022 18:11:30 +0000 (UTC) X-Spam-Status: No, score=-12.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 25 Sep 2022 18:11:37 -0000 Hi, Thanks for the patch! First, I have not really explored why, but running your testcase with current master GDB gives me: $ make check-gdb TESTS=gdb.python/py-watchpoint.exp Running .../gdb/testsuite/gdb.python/py-watchpoint.exp ... === gdb Summary === # of expected passes 6 which is strange. I was expecting FAILs there. Is it only on my system (ubuntu 22.04 on x86_64)? I can however reproduce the problem by running the ticket’s testcase manually and observe the issue you describe. On Sun, Sep 25, 2022 at 01:33:50PM +0800, Johnson Sun via Gdb-patches wrote: > Currently, during normal stop (i.e., stop and waiting for the next command), > GDB automatically deletes local Watchpoints that are out of scope. However, > local Watchpoints are not deleted if a Python script decides not to normal > stop upon hit, causing them to be hit many more times than they should, as > reported in PR python/29603. This was happening because the watchpoint is not > disabled when going out of scope. > > This commit fixes this issue by disabling the watchpoint when going out of > scope. It also adds a test to ensure this feature isn't regressed in the > future. > > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29603 > --- > gdb/breakpoint.c | 1 + > gdb/testsuite/gdb.python/py-watchpoint.c | 27 +++++++++++++ > gdb/testsuite/gdb.python/py-watchpoint.exp | 44 ++++++++++++++++++++++ > gdb/testsuite/gdb.python/py-watchpoint.py | 31 +++++++++++++++ > 4 files changed, 103 insertions(+) > create mode 100644 gdb/testsuite/gdb.python/py-watchpoint.c > create mode 100644 gdb/testsuite/gdb.python/py-watchpoint.exp > create mode 100644 gdb/testsuite/gdb.python/py-watchpoint.py > > diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c > index bff3bac7d1a..b78ae9c4993 100644 > --- a/gdb/breakpoint.c > +++ b/gdb/breakpoint.c > @@ -1832,6 +1832,7 @@ watchpoint_del_at_next_stop (struct watchpoint *w) > w->related_breakpoint = w; > } > w->disposition = disp_del_at_next_stop; > + disable_breakpoint(w); >From what I understand, the problem you are trying to solve is linked to extension language (python in this case), so it seems odd to me that the fix is in a non-extension related code path. If my understanding is correct, the python extension you provide should work similarly to typing say "watch i if 0". So my approach would be to have something similar between checking the watchpoint condition and evaluating the `stop` method from you python class. In the condition case, `disable_breakpoint` is not used before the breakpoint is deleted. Instead, the code checks "b->disposition != disp_del_at_next_stop" (in bpstat_check_breakpoint_conditions) before trying to evaluate the condition. I guess that this is what you also want to do in your case. In the same function, you can have the following change: diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 002f4a935b1..e8d3fd0d50a 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -5339,7 +5339,8 @@ bpstat_check_breakpoint_conditions (bpstat *bs, thread_info *thread) /* Evaluate extension language breakpoints that have a "stop" method implemented. */ - bs->stop = breakpoint_ext_lang_cond_says_stop (b); + if (b->disposition != disp_del_at_next_stop) + bs->stop = breakpoint_ext_lang_cond_says_stop (b); if (is_watchpoint (b)) { I believe this is what you are looking for. That being said, this is my approach to solve your problem, a maintainer might have a better way to address this. > } > > /* Extract a bitfield value from value VAL using the bit parameters contained in > diff --git a/gdb/testsuite/gdb.python/py-watchpoint.c b/gdb/testsuite/gdb.python/py-watchpoint.c > new file mode 100644 > index 00000000000..6d02e87e571 > --- /dev/null > +++ b/gdb/testsuite/gdb.python/py-watchpoint.c > @@ -0,0 +1,27 @@ > +/* This testcase is part of GDB, the GNU debugger. > + > + Copyright 2022 Free Software Foundation, Inc. > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; either version 3 of the License, or > + (at your option) any later version. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program. If not, see . */ > + > +#include > + > +int > +main () > +{ > + int i; > + for (i = 0; i < 3; i++) > + printf ("%d", i); > + return 0; > +} > diff --git a/gdb/testsuite/gdb.python/py-watchpoint.exp b/gdb/testsuite/gdb.python/py-watchpoint.exp > new file mode 100644 > index 00000000000..863f3eff66a > --- /dev/null > +++ b/gdb/testsuite/gdb.python/py-watchpoint.exp > @@ -0,0 +1,44 @@ > +# Copyright (C) 2022 Free Software Foundation, Inc. > +# > +# This program is free software; you can redistribute it and/or modify > +# it under the terms of the GNU General Public License as published by > +# the Free Software Foundation; either version 3 of the License, or > +# (at your option) any later version. > +# > +# This program is distributed in the hope that it will be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License > +# along with this program. If not, see . > + > +# This file is part of the GDB testsuite. It checks Watchpoints > +# are deleted after used. > + > +load_lib gdb-python.exp > + > +standard_testfile > + > +if {[prepare_for_testing "failed to prepare" $testfile $srcfile]} { > + return -1 > +} > + > +# Skip all tests if Python scripting is not enabled. > +if { [skip_python_tests] } { continue } > + > +if ![runto_main] then { > + return 0 > +} > + > +# > +# Check Watchpoints are deleted after used > +# > + > +gdb_test "set can-use-hw-watchpoints 0" ".*" "Don't use hardware watchpoints" In this case, you could probably use gdb_test_no_output > +gdb_test "python print (len(gdb.breakpoints()))" "1" "check default BP count" It looks odd to have a space after print and not before the other parenthesis. To follow python coding convention, I would remove the space between "print" and the "(". Same remark holds for the two other occurrences of this pattern below. Best, Lancelot. > +gdb_test "source ${srcdir}/${subdir}/${testfile}.py" ".*Python script imported.*" \ > + "import python scripts" > +gdb_test "python print (len(gdb.breakpoints()))" "2" "check modified BP count" > +gdb_test "continue" ".*Watchpoint Hit: 4.*" "run until program stops" > +gdb_test "python print (len(gdb.breakpoints()))" "1" "check BP count" > diff --git a/gdb/testsuite/gdb.python/py-watchpoint.py b/gdb/testsuite/gdb.python/py-watchpoint.py > new file mode 100644 > index 00000000000..855c820b245 > --- /dev/null > +++ b/gdb/testsuite/gdb.python/py-watchpoint.py > @@ -0,0 +1,31 @@ > +# Copyright (C) 2022 Free Software Foundation, Inc. > +# > +# This program is free software; you can redistribute it and/or modify > +# it under the terms of the GNU General Public License as published by > +# the Free Software Foundation; either version 3 of the License, or > +# (at your option) any later version. > +# > +# This program is distributed in the hope that it will be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License > +# along with this program. If not, see . > + > +# This file is part of the GDB testsuite. It checks Watchpoints > +# are deleted after used. > + > +class MyBreakpoint(gdb.Breakpoint): > + def __init__(self, *args, **kwargs): > + gdb.Breakpoint.__init__(self, *args, **kwargs) > + self.i = 0 > + def stop(self): > + self.i += 1 > + print("Watchpoint Hit:", self.i) > + gdb.execute('print i') > + return False > + > +MyBreakpoint('i', gdb.BP_WATCHPOINT) > + > +print("Python script imported") > -- > 2.34.1 > > base-commit: c99b2113a478a075e8d8f7c3848a92f1ce73f847