Re: [PATCH] gdb: warn when converting h/w watchpoints to s/w

[Lancelot SIX <lsix@lancelotsix.com>]

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 <lancelot.six@amd.com>

Best,
Lancelot.

> 
> Thanks for the feedback.  I updated to resolve these issues.
> 
> Thanks,
> Andrew
> 
> ---
> 
> commit 237037fce9954d50a013548c9575ed5ba6674ba2
> Author: Andrew Burgess <aburgess@redhat.com>
> 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 <http://www.gnu.org/licenses/>.  */
> +
> +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 <http://www.gnu.org/licenses/>.
> +
> +# 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"
>