[PATCH v2] Prevent downgrading of hardware watchpoints if possible

[PATCH v2] Prevent downgrading of hardware watchpoints if possible

[Hannes Domani]

The lazy flag of a value can tell us if the value contents are available.
But this could be either because it was simply never actually accessed,
or it tried to be accessed, and failed.
The latter are interesting for watchpoints, the former are not.

Currently it only uses lazy values if they are at the head of the value
chain, but this is fragile logic, and degrades some watchpoints to
software watchpoints where it's actually not necessary.

So this adds a new value flag 'm_fetch_lazy_failed' which tells if the
watchpoint expression actually tried to access the value and failed.
And this is used instead of the value chain location to tell if a lazy
value should be used as a watchpoint location.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=27423
Tested-By: Guinevere Larsen <guinevere@redhat.com>
Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
---
v2:
  - value.h: fixed typo in comment
  - watchpoint-hw-no-degradation.exp: fixed copyright year and removed
    values from top-level returns
---
 gdb/breakpoint.c                              | 11 ++--
 .../gdb.base/watchpoint-hw-no-degradation.c   | 51 +++++++++++++++++++
 .../gdb.base/watchpoint-hw-no-degradation.exp | 38 ++++++++++++++
 gdb/value.c                                   |  3 ++
 gdb/value.h                                   | 12 ++++-
 5 files changed, 108 insertions(+), 7 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/watchpoint-hw-no-degradation.c
 create mode 100644 gdb/testsuite/gdb.base/watchpoint-hw-no-degradation.exp

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index e4df4df04a7..462c16df7c5 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -2305,11 +2305,10 @@ update_watchpoint (struct watchpoint *b, bool reparse)
 
 	  /* If it's a memory location, and GDB actually needed
 	     its contents to evaluate the expression, then we
-	     must watch it.  If the first value returned is
-	     still lazy, that means an error occurred reading it;
+	     must watch it.  If an error occurred reading it,
 	     watch it anyway in case it becomes readable.  */
 	  if (v->lval () == lval_memory
-	      && (v == val_chain[0] || ! v->lazy ()))
+	      && (! v->lazy () || v->fetch_lazy_failed ()))
 	    {
 	      struct type *vtype = check_typedef (v->type ());
 
@@ -10708,12 +10707,12 @@ can_use_hardware_watchpoint (const std::vector<value_ref_ptr> &vals)
 
       if (v->lval () == lval_memory)
 	{
-	  if (v != head && v->lazy ())
+	  if (v->lazy () && ! v->fetch_lazy_failed ())
 	    /* A lazy memory lvalue in the chain is one that GDB never
 	       needed to fetch; we either just used its address (e.g.,
 	       `a' in `a.b') or we never needed it at all (e.g., `a'
-	       in `a,b').  This doesn't apply to HEAD; if that is
-	       lazy then it was not readable, but watch it anyway.  */
+	       in `a,b').  If it failed to fetch a lazy value, then it
+	       was not readable, so watch it in this case as well.  */
 	    ;
 	  else
 	    {
diff --git a/gdb/testsuite/gdb.base/watchpoint-hw-no-degradation.c b/gdb/testsuite/gdb.base/watchpoint-hw-no-degradation.c
new file mode 100644
index 00000000000..cfff2a44f90
--- /dev/null
+++ b/gdb/testsuite/gdb.base/watchpoint-hw-no-degradation.c
@@ -0,0 +1,51 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2026 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/>.
+
+*/
+
+#include <sys/mman.h>
+#include <unistd.h>
+#include <stdio.h>
+
+int
+main (void)
+{
+  size_t len = sysconf(_SC_PAGESIZE);
+
+  /* Map and unmap memory block to get address.  */
+  void *p = mmap (0, len, PROT_READ|PROT_WRITE, MAP_ANON|MAP_PRIVATE, -1, 0);
+  if (p == MAP_FAILED)
+    {
+      perror ("mmap");
+      return 1;
+    }
+  munmap (p, len);
+
+  /* Now memory block at address P is inaccessible.
+     Remap block at same address, so it becomes accessible again.  */
+  p = mmap (p, len, PROT_READ|PROT_WRITE,
+	    MAP_ANON|MAP_PRIVATE|MAP_FIXED, -1, 0);
+  if (p == MAP_FAILED)
+    {
+      perror ("mmap");
+      return 1;
+    }
+
+  *(int *) p = 1;
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/watchpoint-hw-no-degradation.exp b/gdb/testsuite/gdb.base/watchpoint-hw-no-degradation.exp
new file mode 100644
index 00000000000..9c545e22e2d
--- /dev/null
+++ b/gdb/testsuite/gdb.base/watchpoint-hw-no-degradation.exp
@@ -0,0 +1,38 @@
+# Copyright 2026 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/>.
+
+# Test if watchpoint doesn't degrade to a software watchpoint if part of
+# expression isn't accessible at time of watchpoint creation.
+
+require allow_hw_watchpoint_access_tests
+
+standard_testfile
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} {
+    return
+}
+
+if {![runto_main]} {
+    return
+}
+
+gdb_breakpoint [gdb_get_line_number "mmap (p, len"]
+gdb_continue_to_breakpoint "mmap"
+
+gdb_test "eval \"watch *(int *)%p == 0\",p" \
+	 "Hardware watchpoint $decimal: .*"
+
+gdb_test "continue" \
+	 "Old value = <unreadable>.*New value = 0.*"
diff --git a/gdb/value.c b/gdb/value.c
index 1f6d7949d4d..e4e3e407ddc 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -1552,6 +1552,7 @@ value::copy () const
   val->m_bitpos = m_bitpos;
   val->m_bitsize = m_bitsize;
   val->m_lazy = m_lazy;
+  val->m_fetch_lazy_failed = m_fetch_lazy_failed;
   val->m_embedded_offset = embedded_offset ();
   val->m_pointed_to_offset = m_pointed_to_offset;
   val->m_modifiable = m_modifiable;
@@ -4119,6 +4120,8 @@ value::fetch_lazy ()
      value.  */
   gdb_assert (m_optimized_out.empty ());
   gdb_assert (m_unavailable.empty ());
+  /* Will be reset with set_lazy () at the end if successful.  */
+  m_fetch_lazy_failed = true;
   if (m_is_zero)
     {
       /* Nothing.  */
diff --git a/gdb/value.h b/gdb/value.h
index 4cc0d7d0902..01a4c00d722 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -138,6 +138,7 @@ struct value
       m_stack (false),
       m_is_zero (false),
       m_in_history (false),
+      m_fetch_lazy_failed (false),
       m_type (type_),
       m_enclosing_type (type_)
   {
@@ -279,7 +280,13 @@ struct value
   { return m_lazy; }
 
   void set_lazy (bool val)
-  { m_lazy = val; }
+  {
+    m_lazy = val;
+    m_fetch_lazy_failed = false;
+  }
+
+  bool fetch_lazy_failed () const
+  { return m_fetch_lazy_failed; }
 
   /* If a value represents a C++ object, then the `type' field gives the
      object's compile-time type.  If the object actually belongs to some
@@ -689,6 +696,9 @@ struct value
   /* True if this a value recorded in value history; false otherwise.  */
   bool m_in_history : 1;
 
+  /* True if fetch_lazy () did not finish successfully.  */
+  bool m_fetch_lazy_failed : 1;
+
   /* Location of value (if lval).  */
   union
   {
-- 
2.54.0