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

[Hannes Domani <ssbssa@yahoo.de>]

 Am Montag, 29. Juni 2026 um 22:39:21 MESZ hat Guinevere Larsen <guinevere@redhat.com> Folgendes geschrieben:

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


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

Yes.


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

Fixed locally as well.

Since then I've enabled pre-commit as well, so I shouldn't miss these
problems any more.


Hannes