Re: [PATCH] Prevent downgrading of hardware watchpoints if possible

[Guinevere Larsen <guinevere@redhat.com>]

On 1/9/26 4:48 PM, Hannes Domani wrote:
> 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
> ---

Hi Hannes!

I took a look over this test, and I can't really review the technical 
parts, but I tested locally and the test does cause the bug before the 
change, and the code changes do solve the test, so I'm happy to add my 
tested tag.

Tested-By: Guinevere Larsen <guinevere@redhat.com>

That said, I have one question and the pre-commit.exp test did point out 
an issue, inlined.

>   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 af4de248ab6..13d95325604 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -2293,11 +2293,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 ());
>   
> @@ -10716,12 +10715,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..94ef335c77f
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/watchpoint-hw-no-degradation.exp
> @@ -0,0 +1,38 @@
> +# Copyright 2009-2026 Free Software Foundation, Inc.
Shouldn't the copyright year just be 2026?
> +
> +# 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 -1
> +}
> +
> +if {![runto_main]} {
> +    return -1
> +}
> +
> +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 a52d4a6742c..c076617d102 100644
> --- a/gdb/value.c
> +++ b/gdb/value.c
> @@ -1546,6 +1546,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;
> @@ -4122,6 +4123,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 8dc3192f637..77919082fe3 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
> @@ -681,6 +688,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 sucessfully.  */
sucessfully ==> successfully
> +  bool m_fetch_lazy_failed : 1;
> +
>     /* Location of value (if lval).  */
>     union
>     {


-- 
Cheers,
Guinevere Larsen
it/its
she/her (deprecated)