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

[Andrew Burgess <aburgess@redhat.com>]

Lancelot SIX <lsix@lancelotsix.com> writes:

> 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>

I switched to use 'downgraded' in the message and pushed this change.

Thanks,
Andrew



>
> 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"
>>