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

 Ping.


Am Montag, 27. Juli 2026 um 17:02:56 MESZ hat Hannes Domani <[email protected]> Folgendes geschrieben:

> 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 <[email protected]>
> Reviewed-by: Thiago Jung Bauermann <[email protected]>
> ---
> 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