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

[Thiago Jung Bauermann <thiago.bauermann@linaro.org>]

Hannes Domani <ssbssa@yahoo.de> writes:

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

I like the solution, thanks!

Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>

Just one nit in the testcase:

> 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.
> +
> +# 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
> +}

There's no need to return a value from the top-level. Please change the
returns above to just "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.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.  */
> +  bool m_fetch_lazy_failed : 1;
> +
>    /* Location of value (if lval).  */
>    union
>    {

As a side note, because of this:

  /* Note that the fields in this structure are arranged to save a bit
     of memory.  */

  struct value
  {

I decided to check:

(top-gdb) ptype /o value
/* offset      |    size */  type = struct value {
                             private:
/*      0      |       4 */    lval_type m_lval;
/*      4: 0   |       1 */    bool m_modifiable : 1;
/*      4: 1   |       1 */    bool m_lazy : 1;
/*      4: 2   |       1 */    bool m_initialized : 1;
/*      4: 3   |       1 */    bool m_stack : 1;
/*      4: 4   |       1 */    bool m_is_zero : 1;
/*      4: 5   |       1 */    bool m_in_history : 1;
/*      4: 6   |       1 */    bool m_fetch_lazy_failed : 1;
/* XXX  1-bit hole       */
/* XXX  3-byte hole      */
/*      8      |      40 */    union {
/*                     8 */        CORE_ADDR address;

This patch doesn't change the size of struct value.

-- 
Thiago
(he/him)