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 59EDC3858C54 for ; Tue, 4 Apr 2023 12:37:44 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 59EDC3858C54 Authentication-Results: sourceware.org; dmarc=pass (p=reject dis=none) header.from=lancelotsix.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=lancelotsix.com Received: from octopus (unknown [IPv6:2a01:e0a:c5c:d8a0:9950:3ec1:8e6e:fd70]) by lndn.lancelotsix.com (Postfix) with ESMTPSA id C2A9E806AA; Tue, 4 Apr 2023 12:37:39 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=lancelotsix.com; s=2021; t=1680611860; bh=PQwZnc27AIpVswBwsgdMEWO/lfuejpUcTT7zX9k8wlA=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=REfpR4PG3HsyE9+9sUZtQZXJrYS4NsTDvUnmEoSdsw5oy8QxR77yhj52b08NB5bdc x8KBINNScUN9XnahQWZB94T1smWPUnWbQ0SlVCE2ql4sAe96/nYt7fQqXz5YEo9lYG TUEUtmYurvebVeRc9vYYQjoDHBwpSczkz8taqTiEa8zkVFBpLGItpnlwarmkNhlNFe Awc/DKdLBZn7pmMmOOcGi3j3CLe15Y2lYWVuKw7JCFp9pE4r6NRbLlopgFhn4lzqWm CiRbVqOaU6mwJhR5P6MjndDi84N6njXfLbbWaWgkqxhPaimML70W61WYsVY+dPWM7u +nK9Ao2STE/6A== Date: Tue, 4 Apr 2023 13:37:34 +0100 From: Lancelot SIX To: Andrew Burgess Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] gdb: warn when converting h/w watchpoints to s/w Message-ID: <20230404123734.g2clksou23uit5ul@octopus> References: <20230330084850.i62rd5kbd7wfcqo6@octopus> <87355hdyg4.fsf@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <87355hdyg4.fsf@redhat.com> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.5.11 (lndn.lancelotsix.com [0.0.0.0]); Tue, 04 Apr 2023 12:37:40 +0000 (UTC) X-Spam-Status: No, score=-12.2 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 List-Id: Hi, Thanks for the update. I went through the changes and tested them on x86_64 GNU/Linux. FWIW, this looks good to me. The only remark I have, is that the test case tests that a watchpoint is "downgraded" from hardware to software, while the warning message only says "changed". In my mind this change is indeed a downgrade and I think this is what we want to communicate. So I wouldn’t mind using this term in the warning message, but I am aware this is close to bike-shedding. If you prefer the current message, I am also fine with it. Feel free to add: Reviewed-By: Lancelot Six Best, Lancelot. > > Thanks for the feedback. I updated to resolve these issues. > > Thanks, > Andrew > > --- > > commit 237037fce9954d50a013548c9575ed5ba6674ba2 > Author: Andrew Burgess > Date: Tue Mar 28 11:24:58 2023 +0100 > > gdb: warn when converting h/w watchpoints to s/w > > On amd64 (at least) if a user sets a watchpoint before the inferior > has started then GDB will assume that a hardware watchpoint can be > created. > > When the inferior starts there is a chance that the watchpoint can't > actually be create as a hardware watchpoint, in which case (currently) > GDB will silently convert the watchpoint to a software watchpoint. > Here's an example session: > > (gdb) p sizeof var > $1 = 4000 > (gdb) watch var > Hardware watchpoint 1: var > (gdb) info watchpoints > Num Type Disp Enb Address What > 1 hw watchpoint keep y var > (gdb) starti > Starting program: /home/andrew/tmp/watch > > Program stopped. > 0x00007ffff7fd3110 in _start () from /lib64/ld-linux-x86-64.so.2 > (gdb) info watchpoints > Num Type Disp Enb Address What > 1 watchpoint keep y var > (gdb) > > Notice that before the `starti` command the watchpoint is showing as a > hardware watchpoint, but afterwards it is showing as a software > watchpoint. Additionally, note that we clearly told the user we > created a hardware watchpoint: > > (gdb) watch var > Hardware watchpoint 1: var > > I think this is bad. I used `starti`, but if the user did `start` or > even `run` then the inferior is going to be _very_ slow, which will be > unexpected -- after all, we clearly told the user that we created a > hardware watchpoint, and the manual clearly says that hardware > watchpoints are fast (at least compared to s/w watchpoints). > > In this patch I propose adding a new warning which will be emitted > when GDB downgrades a h/w watchpoint to s/w. The session now looks > like this: > > (gdb) p sizeof var > $1 = 4000 > (gdb) watch var > Hardware watchpoint 1: var > (gdb) info watchpoints > Num Type Disp Enb Address What > 1 hw watchpoint keep y var > (gdb) starti > Starting program: /home/andrew/tmp/watch > warning: watchpoint 1 changed to software watchpoint > > Program stopped. > 0x00007ffff7fd3110 in _start () from /lib64/ld-linux-x86-64.so.2 > (gdb) info watchpoints > Num Type Disp Enb Address What > 1 watchpoint keep y var > (gdb) > > The important line is: > > warning: watchpoint 1 changed to software watchpoint > > It's not much, but hopefully it will be enough to indicate to the user > that something unexpected has occurred, and hopefully, they will not > be surprised when the inferior runs much slower than they expected. > > I've added an amd64 only test in gdb.arch/, I didn't want to try > adding this as a global test as other architectures might be able to > support the watchpoint request in h/w. > > Also the test is skipped for extended-remote boards as there's a > different set of options for limiting hardware watchpoints on remote > targets, and this test isn't about them. > > diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c > index 7228acfd8fe..8b8c2937d8d 100644 > --- a/gdb/breakpoint.c > +++ b/gdb/breakpoint.c > @@ -2143,6 +2143,21 @@ update_watchpoint (struct watchpoint *b, bool reparse) > } > } > > + /* Helper function to bundle possibly emitting a warning along with > + changing the type of B to bp_watchpoint. */ > + auto change_type_to_bp_watchpoint = [] (breakpoint *bp) > + { > + /* Only warn for breakpoints that have been assigned a +ve number, > + anything else is either an internal watchpoint (which we don't > + currently create) or has not yet been finalized, in which case > + this change of type will be occurring before the user is told > + the type of this watchpoint. */ > + if (bp->type == bp_hardware_watchpoint && bp->number > 0) > + warning (_("watchpoint %d changed to software watchpoint"), > + bp->number); > + bp->type = bp_watchpoint; > + }; > + > /* Change the type of breakpoint between hardware assisted or > an ordinary watchpoint depending on the hardware support and > free hardware slots. Recheck the number of free hardware slots > @@ -2200,7 +2215,7 @@ update_watchpoint (struct watchpoint *b, bool reparse) > "resources for this watchpoint.")); > > /* Downgrade to software watchpoint. */ > - b->type = bp_watchpoint; > + change_type_to_bp_watchpoint (b); > } > else > { > @@ -2221,7 +2236,7 @@ update_watchpoint (struct watchpoint *b, bool reparse) > "read/access watchpoint.")); > } > else > - b->type = bp_watchpoint; > + change_type_to_bp_watchpoint (b); > > loc_type = (b->type == bp_watchpoint? bp_loc_software_watchpoint > : bp_loc_hardware_watchpoint); > diff --git a/gdb/testsuite/gdb.arch/amd64-watchpoint-downgrade.c b/gdb/testsuite/gdb.arch/amd64-watchpoint-downgrade.c > new file mode 100644 > index 00000000000..c2f5f2cc78e > --- /dev/null > +++ b/gdb/testsuite/gdb.arch/amd64-watchpoint-downgrade.c > @@ -0,0 +1,29 @@ > +/* This testcase is part of GDB, the GNU debugger. > + > + Copyright 2023 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 . */ > + > +struct struct_type > +{ > + unsigned long long array[100]; > +}; > + > +struct struct_type global_var; > + > +int > +main () > +{ > + return 0; > +} > diff --git a/gdb/testsuite/gdb.arch/amd64-watchpoint-downgrade.exp b/gdb/testsuite/gdb.arch/amd64-watchpoint-downgrade.exp > new file mode 100644 > index 00000000000..a4814305c72 > --- /dev/null > +++ b/gdb/testsuite/gdb.arch/amd64-watchpoint-downgrade.exp > @@ -0,0 +1,67 @@ > +# Copyright 2023 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 . > + > +# Ask GDB to watch a large structure before the inferior has started, > +# GDB will assume it can place a hardware watchpoint. > +# > +# Once the inferior starts GDB realises that it is not able to watch > +# such a large structure and downgrades to a software watchpoint. > +# > +# This test checks that GDB emits a warnings about this downgrade, as > +# a software watchpoint will be significantly slower than a hardware > +# watchpoint, and the user probably wants to know about this. > + > +require target_can_use_run_cmd is_x86_64_m64_target > + > +# The remote/extended-remote target has its own set of flags to > +# control the use of s/w vs h/w watchpoints, this test isn't about > +# those, so skip the test in these cases. > +if {[target_info gdb_protocol] == "remote" > + || [target_info gdb_protocol] == "extended-remote"} { > + unsupported "using [target_info gdb_protocol] protocol" > + return -1 > +} > + > +standard_testfile > + > +if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile} \ > + { debug }] } { > + return -1 > +} > + > +# Insert the watchpoint, it should default to a h/w watchpoint. > +gdb_test "watch global_var" \ > + "Hardware watchpoint $decimal: global_var" > +set num [get_integer_valueof "\$bpnum" "*UNKNOWN*" \ > + "get watchpoint number"] > + > +# Watchpoint should initially show as a h/w watchpoint. > +gdb_test "info watchpoints" \ > + "\r\n$num\\s+hw watchpoint\\s+keep\\s+y\\s+global_var" \ > + "check info watchpoints before starting" > + > +# Start the inferior, GDB should emit a warning that the watchpoint > +# type has changed. > +gdb_test "starti" \ > + [multi_line \ > + "warning: watchpoint $num changed to software watchpoint" \ > + "" \ > + "Program stopped\\." \ > + ".*"] > + > +# Watchpoint should now have downgraded to a s/w watchpoint. > +gdb_test "info watchpoints" \ > + "\r\n$num\\s+watchpoint\\s+keep\\s+y\\s+global_var" \ > + "check info watchpoints after starting" > diff --git a/gdb/testsuite/gdb.base/watchpoint.exp b/gdb/testsuite/gdb.base/watchpoint.exp > index 513964ebf86..28276681afc 100644 > --- a/gdb/testsuite/gdb.base/watchpoint.exp > +++ b/gdb/testsuite/gdb.base/watchpoint.exp > @@ -273,7 +273,8 @@ proc test_stepping {} { > global gdb_prompt > > if {[runto marker1]} { > - gdb_test "watch ival2" ".*\[Ww\]atchpoint \[0-9\]*: ival2" > + gdb_test "watch ival2" \ > + "^watch ival2\r\n\[^\r\n\]*\[Ww\]atchpoint \[0-9\]*: ival2" > > # Well, let's not be too mundane. It should be a *bit* of a challenge > gdb_test "break func2 if 0" "Breakpoint.*at.*" > @@ -432,7 +433,8 @@ proc test_complex_watchpoint {} { > global gdb_prompt > > if {[runto marker4]} { > - gdb_test "watch ptr1->val" ".*\[Ww\]atchpoint \[0-9\]*: ptr1->val" > + gdb_test "watch ptr1->val" \ > + "^watch ptr1->val\r\n\[^\r\n\]*\[Ww\]atchpoint \[0-9\]*: ptr1->val" > gdb_test "break marker5" ".*Breakpoint.*" > > gdb_test "cont" "Continuing.*\[Ww\]atchpoint.*ptr1->val.*Old value = 1.*New value = 2.*" "test complex watchpoint" > @@ -458,7 +460,9 @@ proc test_complex_watchpoint {} { > # is the function we're now in. This should auto-delete when > # execution exits the scope of the watchpoint. > # > - gdb_test "watch local_a" ".*\[Ww\]atchpoint \[0-9\]*: local_a" "set local watch" > + gdb_test "watch local_a" \ > + "^watch local_a\r\n\[^\r\n\]*\[Ww\]atchpoint \[0-9\]*: local_a" \ > + "set local watch" > gdb_test "cont" "\[Ww\]atchpoint.*local_a.*" "trigger local watch" > > set test "self-delete local watch" > @@ -487,7 +491,8 @@ proc test_complex_watchpoint {} { > # something whose scope is larger than this invocation > # of "func2". This should also auto-delete. > # > - gdb_test "watch local_a + ival5" ".*\[Ww\]atchpoint \[0-9\]*: local_a . ival5" \ > + gdb_test "watch local_a + ival5" \ > + "^watch local_a \\+ ival5\r\n\[^\r\n\]*\[Ww\]atchpoint \[0-9\]*: local_a . ival5" \ > "set partially local watch" > gdb_test "cont" "Continuing.*\[Ww\]atchpoint .*: local_a . ival5.*" \ > "trigger1 partially local watch" > @@ -502,7 +507,8 @@ proc test_complex_watchpoint {} { > # delete. > # > gdb_continue_to_breakpoint "func2 breakpoint here, third time" > - gdb_test "watch static_b" ".*\[Ww\]atchpoint \[0-9\]*: static_b" \ > + gdb_test "watch static_b" \ > + "^watch static_b\r\n\[^\r\n\]*\[Ww\]atchpoint \[0-9\]*: static_b" \ > "set static local watch" > gdb_test "cont" "Continuing.*\[Ww\]atchpoint .*: static_b.*" \ > "trigger static local watch" > @@ -521,7 +527,8 @@ proc test_complex_watchpoint {} { > gdb_test "tbreak recurser" ".*breakpoint.*" > gdb_test "cont" "Continuing.*recurser.*" > gdb_test "next" "if \\(x > 0.*" "next past local_x initialization" > - gdb_test "watch local_x" ".*\[Ww\]atchpoint \[0-9\]*: local_x" \ > + gdb_test "watch local_x" \ > + "^watch local_x\r\n\[^\r\n\]*\[Ww\]atchpoint \[0-9\]*: local_x" \ > "set local watch in recursive call" > gdb_test "cont" "Continuing.*\[Ww\]atchpoint .*: local_x.*New value = 2.*" \ > "trigger local watch in recursive call" > @@ -536,7 +543,8 @@ proc test_complex_watchpoint {} { > gdb_test "tbreak recurser" ".*breakpoint.*" > gdb_test "cont" "Continuing.*recurser.*" "continue to recurser" > gdb_test "next" "if \\(x > 0.*" "next past local_x initialization" > - gdb_test "watch recurser::local_x" ".*\[Ww\]atchpoint \[0-9\]*: recurser::local_x" \ > + gdb_test "watch recurser::local_x" \ > + "^watch recurser::local_x\r\n\[^\r\n\]*\[Ww\]atchpoint \[0-9\]*: recurser::local_x" \ > "set local watch in recursive call with explicit scope" > gdb_test "cont" "Continuing.*\[Ww\]atchpoint .*: recurser::local_x.*New value = 2.*" \ > "trigger local watch with explicit scope in recursive call" > @@ -562,7 +570,8 @@ proc test_watchpoint_and_breakpoint {} { > if {[runto func3]} { > gdb_breakpoint [gdb_get_line_number "second x assignment"] > gdb_continue_to_breakpoint "second x assignment" > - gdb_test "watch x" ".*atchpoint \[0-9\]+: x" > + gdb_test "watch x" \ > + "^watch x\r\n\[^\r\n\]*\[Ww\]atchpoint \[0-9\]+: x" > gdb_test "next" \ > ".*atchpoint \[0-9\]+: x\r\n\r\nOld value = 0\r\nNew value = 1\r\n.*" \ > "next after watch x" > @@ -579,7 +588,8 @@ proc test_constant_watchpoint {} { > "marker1 is constant" > gdb_test "watch count + 6" ".*atchpoint \[0-9\]+: count \\+ 6" > gdb_test_no_output "delete \$bpnum" "delete watchpoint `count + 6'" > - gdb_test "watch 7 + count" ".*atchpoint \[0-9\]+: 7 \\+ count" > + gdb_test "watch 7 + count" \ > + "^watch 7 \\+ count\r\n\[^\r\n\]*atchpoint \[0-9\]+: 7 \\+ count" > gdb_test_no_output "delete \$bpnum" "delete watchpoint `7 + count'" > } > > @@ -588,7 +598,7 @@ proc test_disable_enable_software_watchpoint {} { > # for software watchpoints. > > # Watch something not memory to force a software watchpoint. > - gdb_test {watch $pc} ".*atchpoint \[0-9\]+: .pc" > + gdb_test {watch $pc} "^watch \\\$pc\r\n\[^\r\n\]*\[Ww\]atchpoint \[0-9\]+: .pc" > > gdb_test_no_output "disable \$bpnum" "disable watchpoint `\$pc'" > gdb_test_no_output "enable \$bpnum" "reenable watchpoint `\$pc'" > @@ -822,7 +832,7 @@ proc test_no_hw_watchpoints {} { > "show disable fast watches" > > gdb_test "watch ival3 if count > 1" \ > - "Watchpoint \[0-9\]*: ival3.*" \ > + "^watch ival3 if count > 1\r\nWatchpoint \[0-9\]*: ival3.*" \ > "set slow conditional watch" > > gdb_test "continue" \ > @@ -832,7 +842,7 @@ proc test_no_hw_watchpoints {} { > gdb_test_no_output "delete \$bpnum" "delete watch ival3" > > gdb_test "watch ival3 if count > 1 thread 1 " \ > - "Watchpoint \[0-9\]*: ival3.*" \ > + "watch ival3 if count > 1 thread 1 \r\nWatchpoint \[0-9\]*: ival3.*" \ > "set slow condition watch w/thread" > > gdb_test_no_output "delete \$bpnum" "delete watch w/condition and thread" >