Re: [PATCH] Fix hardware watchpoints in replay mode

[Guinevere Larsen <blarsen@redhat.com>]

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