From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from lndn.lancelotsix.com (vps-42846194.vps.ovh.net [IPv6:2001:41d0:801:2000::2400]) by sourceware.org (Postfix) with ESMTPS id 556843858D1E for ; Sun, 17 Apr 2022 22:29:35 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 556843858D1E Received: from ubuntu.lan (unknown [IPv6:2a02:390:9086::635]) by lndn.lancelotsix.com (Postfix) with ESMTPSA id 2D00386C9E; Sun, 17 Apr 2022 22:29:34 +0000 (UTC) Date: Sun, 17 Apr 2022 22:29:29 +0000 From: Lancelot SIX To: Enze Li Cc: tom@tromey.com, gdb-patches@sourceware.org Subject: Re: [PATCH RESEND v3] gdb: fix using clear command to delete non-user breakpoints(PR cli/7161) Message-ID: <20220417222929.mqgof7ygmeafgn52@ubuntu.lan> References: <87sfqez4de.fsf@tromey.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.5.11 (lndn.lancelotsix.com [0.0.0.0]); Sun, 17 Apr 2022 22:29:34 +0000 (UTC) X-Spam-Status: No, score=-5.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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, 17 Apr 2022 22:29:37 -0000 Hi, I think Tom already OK-ed this patch, but I have minor comments, if you do not mind. > + > +proc get_maint_info_bp { var } { > + global expect_out > + global gdb_prompt > + > + gdb_test_multiple "maint info break -1" "find address of internal bp $var" { In this proc, the `var` param is only used in the messages, not in the actual command. Did you mean "maint info break $var" here? > + -re ".*(0x\[0-9a-f\]+).*$gdb_prompt $" { > + return $expect_out(1,string) > + } > + timeout { > + perror "couldn't find address of $var" > + return "" > + } > + } > + return "" > +} > + > +standard_testfile .c > + > +# This testcase just needs a "Hello world" source file, reuse > +# gdb.base/main.c instead of adding a new one. > +if { [gdb_compile "${srcdir}/${subdir}/main.c" "${binfile}" executable {debug}] != "" } { > + untested "failed to compile" > + return -1 > +} > + > +# Start with a fresh gdb. > +clean_restart ${binfile} > + > +if ![runto_main] then { > + return 0 > +} > + > +gdb_test "break main.c:21" \ > + ".*Breakpoint.* at .*" \ > + "set breakpoint" If I run this test, I have: (gdb) break -qualified main Breakpoint 1 at 0x401020: file .../gdb/testsuite/gdb.base/main.c, line 21. (gdb) run Starting program: .../gdb/testsuite/outputs/gdb.base/clear_non_user_bp/clear_non_user_bp [Thread debugging using libthread_db enabled] Using host libthread_db library ".../lib/libthread_db.so.1". Breakpoint 1, main () at .../gdb/testsuite/gdb.base/main.c:21 21 return 0; (gdb) break main.c:21 Note: breakpoint 1 also set at pc 0x401020. Breakpoint 2 at 0x401020: file .../gdb/testsuite/gdb.base/main.c, line 21. (gdb) PASS: gdb.base/clear_non_user_bp.exp: set breakpoint So you already have a user breakpoint inserted (which also happens to be at the same address as the breakpoint you want to insert). I am not sure this adds much to your testcase (but I might be wrong). Also, I do not know how maintainers feel about this (I am not one), but I feel that using gdb.base/main.c which is not directly linked to this testcase makes it easy to have main.c accidentally edited with this file not being touched. With a plain line number, we might end up with unexpected test changes. I really do not expect this file to change any time soon, but who knows, an update in the license header might lead to the line you are looking for not being 21 anymore. If you really need this breakpoint, I guess I would go for something like: gdb_break [gdb_get_line_number "return 0;"] > + > +set bp_addr [get_maint_info_bp "-1"] > + > +gdb_test "maint info break -1" \ > + "-1.*shlib events.*keep y.*$bp_addr.*" \ > + "maint info breakpoint -1 error" The last param is the name of the test, not an error message. If the test was to fail, the output would be: FAIL: main info breakpoint -1 error "FAIL" and "error" would be somewhat redundant. I would suggest dropping the last argument altogether since, if missing, the framework uses the 1st parameter (the command) as default value. Also, I feel that both main > + > +gdb_test "clear *$bp_addr" \ > + "No breakpoint at \\*$bp_addr." \ > + "clear internal breakpoint error" Here, I would suggest just dropping the " error" part of the testname. "$bp_addr" is likely to change from one configuration to another, making diffing .sum files difficult. Best, Lancelot.