[v2] Fix hardware watchpoints in replay mode

Message ID 20230914162732.2077-1-ssbssa@yahoo.de
State New
Headers
Series [v2] Fix hardware watchpoints in replay mode |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Testing passed

Commit Message

Hannes Domani Sept. 14, 2023, 4:27 p.m. UTC
  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.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=21969
---
 gdb/record-full.c                           | 4 +++-
 gdb/testsuite/gdb.reverse/watch-reverse.exp | 4 ++++
 2 files changed, 7 insertions(+), 1 deletion(-)
  

Comments

Guinevere Larsen Sept. 15, 2023, 10:09 a.m. UTC | #1
On 14/09/2023 18:27, Hannes Domani via Gdb-patches wrote:
> 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.
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=21969
> ---

Hi! Thanks for the quick turn around for this patch!

>   gdb/record-full.c                           | 4 +++-
>   gdb/testsuite/gdb.reverse/watch-reverse.exp | 4 ++++
>   2 files changed, 7 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

So I did some more digging on this, based on your response to the v1 
email, copied here for convenience:

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

The reason it shows a SIGTRAP is that record_full_stop_reason ended up 
over-written anyway to stop_no_reason, so you are right that just 
flipping isn't enough. If you were to put this call in an "else if", 
instead of unrelated ifs, the issue stops happening. But I wonder if 
this is the right place to solve it anyway. I think 
record_check_stopped_by_breakpoint should just not overwrite 
record_full_stop_reason at all times.

It seems that since record_check_stopped_by_breakpoint was introduced in 
9e8915c6cee5c37637521b424d723e990e06d597 by Pedro (back in 2015), it was 
already indiscriminately overwriting the reason. I changed that locally 
and see no regressions and it also fixes this issue. Pedro, do you 
happen to remember why you added this back then?

>   			  (aspace, tmp_pc, &record_full_stop_reason))
>   			{
>   			  if (record_debug)
> diff --git a/gdb/testsuite/gdb.reverse/watch-reverse.exp b/gdb/testsuite/gdb.reverse/watch-reverse.exp
> index 6b81a6fdf88..a7126ac9b1d 100644
> --- a/gdb/testsuite/gdb.reverse/watch-reverse.exp
> +++ b/gdb/testsuite/gdb.reverse/watch-reverse.exp
> @@ -118,8 +118,12 @@ gdb_test "continue" \
>       ".*\[Ww\]atchpoint.*ival3.*Old value = 0.*New value = -1.*ival3 = count; ival4 = count;.*" \
>       "watchpoint hit in reverse, fifth time"
>   
> +gdb_test_no_output "disable \$bpnum" "disable non-hw watchpoint"
> +
>   gdb_test_no_output "set can-use-hw-watchpoints 1" "enable hw watchpoints"
>   
> +gdb_test_no_output "enable \$bpnum" "enable hw watchpoint"
> +
>   ###
>   ###
>   ###

This is a better solution! nice catch :)

I did forget to mention in last email (sorry =/ ) but there is a second 
test to do with watchpoints, called watch-precsave.exp, that - from what 
I gather - should always be the same as watch-reverse, but it uses a 
pre-recorded execution instead. I tried the same trick of disabling and 
re-enabling the watchpoints, but it didn't trigger the bug on that one 
(and just blindly clean-restarting and redoing the start also didn't 
work), I think it needs some work too. Or at least some investigation 
about if the bug does trigger on that scenario.
  
Hannes Domani Oct. 2, 2023, 5:50 p.m. UTC | #2
Ping.


Am Donnerstag, 14. September 2023, 18:28:30 MESZ hat Hannes Domani via Gdb-patches <gdb-patches@sourceware.org> Folgendes geschrieben:

> 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.
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=21969
> ---
> gdb/record-full.c                          | 4 +++-
> gdb/testsuite/gdb.reverse/watch-reverse.exp | 4 ++++
> 2 files changed, 7 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)
> diff --git a/gdb/testsuite/gdb.reverse/watch-reverse.exp b/gdb/testsuite/gdb.reverse/watch-reverse.exp
> index 6b81a6fdf88..a7126ac9b1d 100644
> --- a/gdb/testsuite/gdb.reverse/watch-reverse.exp
> +++ b/gdb/testsuite/gdb.reverse/watch-reverse.exp
> @@ -118,8 +118,12 @@ gdb_test "continue" \
>     ".*\[Ww\]atchpoint.*ival3.*Old value = 0.*New value = -1.*ival3 = count; ival4 = count;.*" \
>     "watchpoint hit in reverse, fifth time"
>
> +gdb_test_no_output "disable \$bpnum" "disable non-hw watchpoint"
> +
> gdb_test_no_output "set can-use-hw-watchpoints 1" "enable hw watchpoints"
>
> +gdb_test_no_output "enable \$bpnum" "enable hw watchpoint"
> +
> ###
> ###
> ###
> --
> 2.35.1
  
Hannes Domani Nov. 1, 2023, 1:57 p.m. UTC | #3
Ping.


Am Montag, 2. Oktober 2023, 19:51:03 MESZ hat Hannes Domani via Gdb-patches <gdb-patches@sourceware.org> Folgendes geschrieben:

Ping.


Am Donnerstag, 14. September 2023, 18:28:30 MESZ hat Hannes Domani via Gdb-patches <gdb-patches@sourceware.org> Folgendes geschrieben:

> 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.
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=21969
> ---
> gdb/record-full.c                          | 4 +++-
> gdb/testsuite/gdb.reverse/watch-reverse.exp | 4 ++++
> 2 files changed, 7 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)
> diff --git a/gdb/testsuite/gdb.reverse/watch-reverse.exp b/gdb/testsuite/gdb.reverse/watch-reverse.exp
> index 6b81a6fdf88..a7126ac9b1d 100644
> --- a/gdb/testsuite/gdb.reverse/watch-reverse.exp
> +++ b/gdb/testsuite/gdb.reverse/watch-reverse.exp
> @@ -118,8 +118,12 @@ gdb_test "continue" \
>     ".*\[Ww\]atchpoint.*ival3.*Old value = 0.*New value = -1.*ival3 = count; ival4 = count;.*" \
>     "watchpoint hit in reverse, fifth time"
>
> +gdb_test_no_output "disable \$bpnum" "disable non-hw watchpoint"
> +
> gdb_test_no_output "set can-use-hw-watchpoints 1" "enable hw watchpoints"
>
> +gdb_test_no_output "enable \$bpnum" "enable hw watchpoint"
> +
> ###
> ###
> ###
> --
> 2.35.1
  
Hannes Domani Nov. 14, 2023, 10:53 a.m. UTC | #4
Ping.


Am Mittwoch, 1. November 2023, 14:58:05 MEZ hat Hannes Domani <ssbssa@yahoo.de> Folgendes geschrieben:

> Ping.
>
>
> Am Montag, 2. Oktober 2023, 19:51:03 MESZ hat Hannes Domani via Gdb-patches <gdb-patches@sourceware.org> Folgendes geschrieben:
>
> Ping.
>
>
> Am Donnerstag, 14. September 2023, 18:28:30 MESZ hat Hannes Domani via Gdb-patches <gdb-patches@sourceware.org> Folgendes geschrieben:
>
> > 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.
> >
> > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=21969
> > ---
> > gdb/record-full.c                          | 4 +++-
> > gdb/testsuite/gdb.reverse/watch-reverse.exp | 4 ++++
> > 2 files changed, 7 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)
> > diff --git a/gdb/testsuite/gdb.reverse/watch-reverse.exp b/gdb/testsuite/gdb.reverse/watch-reverse.exp
> > index 6b81a6fdf88..a7126ac9b1d 100644
> > --- a/gdb/testsuite/gdb.reverse/watch-reverse.exp
> > +++ b/gdb/testsuite/gdb.reverse/watch-reverse.exp
> > @@ -118,8 +118,12 @@ gdb_test "continue" \
> >     ".*\[Ww\]atchpoint.*ival3.*Old value = 0.*New value = -1.*ival3 = count; ival4 = count;.*" \
> >     "watchpoint hit in reverse, fifth time"
> >
> > +gdb_test_no_output "disable \$bpnum" "disable non-hw watchpoint"
> > +
> > gdb_test_no_output "set can-use-hw-watchpoints 1" "enable hw watchpoints"
> >
> > +gdb_test_no_output "enable \$bpnum" "enable hw watchpoint"
> > +
> > ###
> > ###
> > ###
> > --
> > 2.35.1
>
  
Guinevere Larsen Nov. 20, 2023, 1:09 p.m. UTC | #5
On 14/09/2023 18:27, Hannes Domani via Gdb-patches wrote:
> 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.
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=21969
> ---
I have sent a review to this patch. Your spam filter must have catched 
it or something similar. If you can't find it, here's the link for the 
public inbox: 
https://inbox.sourceware.org/gdb-patches/2eb65a2d-aa80-5930-3033-bfd82baa2ed9@redhat.com/
  
Hannes Domani Nov. 20, 2023, 3:59 p.m. UTC | #6
Am Montag, 20. November 2023, 14:09:34 MEZ hat Guinevere Larsen <blarsen@redhat.com> Folgendes geschrieben:

> On 14/09/2023 18:27, Hannes Domani via Gdb-patches wrote:
> > 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.
> >
> > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=21969
> > ---
> I have sent a review to this patch. Your spam filter must have catched
> it or something similar. If you can't find it, here's the link for the
> public inbox:
> https://inbox.sourceware.org/gdb-patches/2eb65a2d-aa80-5930-3033-bfd82baa2ed9@redhat.com/

I saw that, but it doesn't really make clear what I should do now.
And from what I can remember of our discussion on IRC, you wanted to wait if Pedro had anything to add.


Regards
Hannes
  
Guinevere Larsen Nov. 21, 2023, 12:52 p.m. UTC | #7
On 20/11/2023 16:59, Hannes Domani wrote:
>   Am Montag, 20. November 2023, 14:09:34 MEZ hat Guinevere Larsen <blarsen@redhat.com> Folgendes geschrieben:
>
>> On 14/09/2023 18:27, Hannes Domani via Gdb-patches wrote:
>>> 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.
>>>
>>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=21969
>>> ---
>> I have sent a review to this patch. Your spam filter must have catched
>> it or something similar. If you can't find it, here's the link for the
>> public inbox:
>> https://inbox.sourceware.org/gdb-patches/2eb65a2d-aa80-5930-3033-bfd82baa2ed9@redhat.com/
> I saw that, but it doesn't really make clear what I should do now.
> And from what I can remember of our discussion on IRC, you wanted to wait if Pedro had anything to add.
>
Ah right, sorry, this last month has been pretty hectic, our 
conversation completely slipped my mind.

I took a second, long look at record_check_stopped_by_breakpoint and 
convinced myself, I don't see why it should overwrite the reason if gdb 
is not stopped at a watchpoint. I would say send in a v3 with that 
solution instead, CC Pedro, and I'll give some time for him to react 
before approving the changes.

Also, the changes you made to gdb.reverse/watch-reverse.exp should also 
be made on gdb.reverse/watch-precsave.exp, since it has the same problem 
that you identified with watch-reverse.
  
Hannes Domani Nov. 21, 2023, 2:03 p.m. UTC | #8
Am Dienstag, 21. November 2023 um 13:52:42 MEZ hat Guinevere Larsen <blarsen@redhat.com> Folgendes geschrieben:

> On 20/11/2023 16:59, Hannes Domani wrote:
> >  Am Montag, 20. November 2023, 14:09:34 MEZ hat Guinevere Larsen <blarsen@redhat.com> Folgendes geschrieben:
> >
> >> On 14/09/2023 18:27, Hannes Domani via Gdb-patches wrote:
> >>> 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.
> >>>
> >>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=21969
> >>> ---
> >> I have sent a review to this patch. Your spam filter must have catched
> >> it or something similar. If you can't find it, here's the link for the
> >> public inbox:
> >> https://inbox.sourceware.org/gdb-patches/2eb65a2d-aa80-5930-3033-bfd82baa2ed9@redhat.com/
> > I saw that, but it doesn't really make clear what I should do now.
> > And from what I can remember of our discussion on IRC, you wanted to wait if Pedro had anything to add.
> >
> Ah right, sorry, this last month has been pretty hectic, our
> conversation completely slipped my mind.

All right, I'll do that.


> I took a second, long look at record_check_stopped_by_breakpoint and
> convinced myself, I don't see why it should overwrite the reason if gdb
> is not stopped at a watchpoint. I would say send in a v3 with that
> solution instead, CC Pedro, and I'll give some time for him to react
> before approving the changes.
>
> Also, the changes you made to gdb.reverse/watch-reverse.exp should also
> be made on gdb.reverse/watch-precsave.exp, since it has the same problem
> that you identified with watch-reverse.

We also discussed this on IRC, it doesn't work for watch-precsave.exp because
restored recordings are handled like corefile targets, and there hardware watchpoints
are always disabled.
But I can mention this in the commit message as well.


Regards
Hannes
  

Patch

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)
diff --git a/gdb/testsuite/gdb.reverse/watch-reverse.exp b/gdb/testsuite/gdb.reverse/watch-reverse.exp
index 6b81a6fdf88..a7126ac9b1d 100644
--- a/gdb/testsuite/gdb.reverse/watch-reverse.exp
+++ b/gdb/testsuite/gdb.reverse/watch-reverse.exp
@@ -118,8 +118,12 @@  gdb_test "continue" \
     ".*\[Ww\]atchpoint.*ival3.*Old value = 0.*New value = -1.*ival3 = count; ival4 = count;.*" \
     "watchpoint hit in reverse, fifth time"
 
+gdb_test_no_output "disable \$bpnum" "disable non-hw watchpoint"
+
 gdb_test_no_output "set can-use-hw-watchpoints 1" "enable hw watchpoints"
 
+gdb_test_no_output "enable \$bpnum" "enable hw watchpoint"
+
 ###
 ###
 ###