* [PATCH v2] Fix hardware watchpoints in replay mode
[not found] <20230914162732.2077-1-ssbssa.ref@yahoo.de>
@ 2023-09-14 16:27 ` Hannes Domani
2023-09-15 10:09 ` Guinevere Larsen
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Hannes Domani @ 2023-09-14 16:27 UTC (permalink / raw)
To: gdb-patches
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] Fix hardware watchpoints in replay mode
2023-09-14 16:27 ` [PATCH v2] Fix hardware watchpoints in replay mode Hannes Domani
@ 2023-09-15 10:09 ` Guinevere Larsen
2023-10-02 17:50 ` Hannes Domani
2023-11-20 13:09 ` Guinevere Larsen
2 siblings, 0 replies; 9+ messages in thread
From: Guinevere Larsen @ 2023-09-15 10:09 UTC (permalink / raw)
To: Hannes Domani, gdb-patches, Pedro Alves
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.
--
Cheers,
Guinevere Larsen
She/Her/Hers
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] Fix hardware watchpoints in replay mode
2023-09-14 16:27 ` [PATCH v2] Fix hardware watchpoints in replay mode Hannes Domani
2023-09-15 10:09 ` Guinevere Larsen
@ 2023-10-02 17:50 ` Hannes Domani
2023-11-01 13:57 ` Hannes Domani
2023-11-20 13:09 ` Guinevere Larsen
2 siblings, 1 reply; 9+ messages in thread
From: Hannes Domani @ 2023-10-02 17:50 UTC (permalink / raw)
To: gdb-patches
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] Fix hardware watchpoints in replay mode
2023-10-02 17:50 ` Hannes Domani
@ 2023-11-01 13:57 ` Hannes Domani
2023-11-14 10:53 ` Hannes Domani
0 siblings, 1 reply; 9+ messages in thread
From: Hannes Domani @ 2023-11-01 13:57 UTC (permalink / raw)
To: gdb-patches
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] Fix hardware watchpoints in replay mode
2023-11-01 13:57 ` Hannes Domani
@ 2023-11-14 10:53 ` Hannes Domani
0 siblings, 0 replies; 9+ messages in thread
From: Hannes Domani @ 2023-11-14 10:53 UTC (permalink / raw)
To: gdb-patches
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] Fix hardware watchpoints in replay mode
2023-09-14 16:27 ` [PATCH v2] Fix hardware watchpoints in replay mode Hannes Domani
2023-09-15 10:09 ` Guinevere Larsen
2023-10-02 17:50 ` Hannes Domani
@ 2023-11-20 13:09 ` Guinevere Larsen
2023-11-20 15:59 ` Hannes Domani
2 siblings, 1 reply; 9+ messages in thread
From: Guinevere Larsen @ 2023-11-20 13:09 UTC (permalink / raw)
To: Hannes Domani, gdb-patches
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/
--
Cheers,
Guinevere Larsen
She/Her/Hers
> 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"
> +
> ###
> ###
> ###
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] Fix hardware watchpoints in replay mode
2023-11-20 13:09 ` Guinevere Larsen
@ 2023-11-20 15:59 ` Hannes Domani
2023-11-21 12:52 ` Guinevere Larsen
0 siblings, 1 reply; 9+ messages in thread
From: Hannes Domani @ 2023-11-20 15:59 UTC (permalink / raw)
To: gdb-patches, Guinevere Larsen
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] Fix hardware watchpoints in replay mode
2023-11-20 15:59 ` Hannes Domani
@ 2023-11-21 12:52 ` Guinevere Larsen
2023-11-21 14:03 ` Hannes Domani
0 siblings, 1 reply; 9+ messages in thread
From: Guinevere Larsen @ 2023-11-21 12:52 UTC (permalink / raw)
To: Hannes Domani, gdb-patches
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.
--
Cheers,
Guinevere Larsen
She/Her/Hers
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] Fix hardware watchpoints in replay mode
2023-11-21 12:52 ` Guinevere Larsen
@ 2023-11-21 14:03 ` Hannes Domani
0 siblings, 0 replies; 9+ messages in thread
From: Hannes Domani @ 2023-11-21 14:03 UTC (permalink / raw)
To: gdb-patches, Guinevere Larsen
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-11-21 14:03 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20230914162732.2077-1-ssbssa.ref@yahoo.de>
2023-09-14 16:27 ` [PATCH v2] Fix hardware watchpoints in replay mode Hannes Domani
2023-09-15 10:09 ` Guinevere Larsen
2023-10-02 17:50 ` Hannes Domani
2023-11-01 13:57 ` Hannes Domani
2023-11-14 10:53 ` Hannes Domani
2023-11-20 13:09 ` Guinevere Larsen
2023-11-20 15:59 ` Hannes Domani
2023-11-21 12:52 ` Guinevere Larsen
2023-11-21 14:03 ` Hannes Domani
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).