[PATCH] Fix hardware watchpoints in replay mode

[PATCH] Fix hardware watchpoints in replay mode

[Hannes Domani]

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

Re: [PATCH] Fix hardware watchpoints in replay mode

[Guinevere Larsen]

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.

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.

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

-- 
Cheers,
Guinevere Larsen
She/Her/Hers

Re: [PATCH] Fix hardware watchpoints in replay mode

[Hannes Domani]

 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