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.
---
 gdb/breakpoint.c                              | 19 +++++-
 .../gdb.arch/amd64-watchpoint-downgrade.c     | 35 ++++++++++
 .../gdb.arch/amd64-watchpoint-downgrade.exp   | 67 +++++++++++++++++++
 gdb/testsuite/gdb.base/watchpoint.exp         | 34 ++++++----
 4 files changed, 141 insertions(+), 14 deletions(-)
 create mode 100644 gdb/testsuite/gdb.arch/amd64-watchpoint-downgrade.c
 create mode 100644 gdb/testsuite/gdb.arch/amd64-watchpoint-downgrade.exp


base-commit: f8c88b623130037546842a51beb78c66c644e05d
  

Hi Andrew

Thanks for doing this, I find this gives useful information to the user.

When applying the patch I see:

    Applying: gdb: warn when converting h/w watchpoints to s/w
    .git/rebase-apply/patch:197: indent with spaces.
            gdb_test "watch local_a" \
    .git/rebase-apply/patch:208: indent with spaces.
            gdb_test "watch local_a + ival5" \
    .git/rebase-apply/patch:218: indent with spaces.
            gdb_test "watch static_b" \
    .git/rebase-apply/patch:286: indent with spaces.
             "watch ival3 if  count > 1  thread 1 \r\nWatchpoint \[0-9\]*: ival3.*" \
    warning: 4 lines add whitespace errors.

> 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..37aa0f63936
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/amd64-watchpoint-downgrade.c
> @@ -0,0 +1,35 @@
> +/* 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
> +foo (void)
> +{
> +  return 0;
> +}

I do not think this `foo' function is needed here.  The test passes
without it with main just returning 0.

> +
> +int
> +main ()
> +{
> +  return foo ();
> +}

Best,
Lancelot.
  

Lancelot SIX <lsix@lancelotsix.com> writes:

> Hi Andrew
>
> Thanks for doing this, I find this gives useful information to the user.
>
> When applying the patch I see:
>
>     Applying: gdb: warn when converting h/w watchpoints to s/w
>     .git/rebase-apply/patch:197: indent with spaces.
>             gdb_test "watch local_a" \
>     .git/rebase-apply/patch:208: indent with spaces.
>             gdb_test "watch local_a + ival5" \
>     .git/rebase-apply/patch:218: indent with spaces.
>             gdb_test "watch static_b" \
>     .git/rebase-apply/patch:286: indent with spaces.
>              "watch ival3 if  count > 1  thread 1 \r\nWatchpoint \[0-9\]*: ival3.*" \
>     warning: 4 lines add whitespace errors.
>
>> 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..37aa0f63936
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.arch/amd64-watchpoint-downgrade.c
>> @@ -0,0 +1,35 @@
>> +/* 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
>> +foo (void)
>> +{
>> +  return 0;
>> +}
>
> I do not think this `foo' function is needed here.  The test passes
> without it with main just returning 0.

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"
  

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

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