Re: [PATCH] Fix hardware watchpoints in replay mode

[Hannes Domani <ssbssa@yahoo.de>]

 Am Donnerstag, 14. September 2023, 14:20:54 MESZ hat Guinevere Larsen <blarsen@redhat.com> Folgendes geschrieben:

> On 13/09/2023 17:53, Hannes Domani via Gdb-patches wrote:
> > This regression was introduced by 9e8915c6cee5c37637521b424d723e990e06d597.
> >
> > The problem is that record_check_stopped_by_breakpoint always
> > overwrites record_full_stop_reason, thus loosing the
> > TARGET_STOPPED_BY_WATCHPOINT value which would be checked afterwards.
> >
> > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=21969
>
> Hi! Thanks for working on this!
>
> In general, when writing a commit message, its best to summarize the
> problem again, even if it is described in a bug tracker, because the
> next time this comes up, someone who already has the sources might have
> no internet access for a while, for example. In this situation, I would
> normally summarize this bug, explain the issue (that's not covered in
> the bug), then explain the solution, like so:
>
> "Recent changes introduced by commit
> 9e8915c6cee5c37637521b424d723e990e06d597 caused a regression that meant
> hardware watchpoint stops would not trigger in reverse execution or
> replay mode. This was documented in PR breakpoints/21969. The problem is
> that record_check_stopped_by_breakpoint always overwrites
> record_full_stop_reason, thus loosing the TARGET_STOPPED_BY_WATCHPOINT
> value which would be checked afterwards.
>
> This commit fixes that by checking if record_full_stop_reason is
> TARGET_STOPPED_BY_BREAKPOINT and, if so, not overriding it."
>
> and adding the "Bug:" trailer at the end.

OK, I will add this.


> You are, of course, free to change this wording as much as you like, but
> having a fuller context is very useful when things like bug tracker
> changes break links in the commit history, for example.
>
>
> Additionally, have you tried making a testcase for this issue? I see
> that gdb.reverse/watch-reverse.exp tests hardware watchpoints in theory,
> but since this bug isn't triggered by it, it could probably do with some
> changes , so this doesn't regress again. My guess is that you need to
> clean the execution history before switching to hardware watchpoints, so
> that they get properly saved, but I haven't tested that yet.

I just found out why it works in watch-reverse.exp, it's because after
`set can-use-hw-watchpoints 1` the watchpoint has to be disabled & enabled,
otherwise it will stay a non-hw watchpoint.
Once I added this, there were 12 failures without my fix.

So I will add this to the v2.


> > ---
> >  gdb/record-full.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/gdb/record-full.c b/gdb/record-full.c
> > index faf8b595d22..da785ef4b2a 100644
> > --- a/gdb/record-full.c
> > +++ b/gdb/record-full.c
> > @@ -1382,7 +1382,9 @@ record_full_wait_1 (struct target_ops *ops,
> >
> >                /* check breakpoint */
> >                tmp_pc = regcache_read_pc (regcache);
> > -              if (record_check_stopped_by_breakpoint
> > +              if (record_full_stop_reason
> > +              != TARGET_STOPPED_BY_WATCHPOINT
> > +              && record_check_stopped_by_breakpoint
> >                (aspace, tmp_pc, &record_full_stop_reason))
> >              {
> >                if (record_debug)
>
> I think a simpler solution would be to move the hw watchpoint check
> further up, you won't need to check twice for the STOPPED_BY_WATCHPOINT
> reason.

I just tried this, with this change it suddenly shows SIGTRAP instead of
hit watchpoints, so I will keep it as it is now.


Regards
Hannes