* [PATCH] Fix hardware watchpoints in replay mode [not found] <20230913155330.1558-1-ssbssa.ref@yahoo.de> @ 2023-09-13 15:53 ` Hannes Domani 2023-09-14 12:20 ` Guinevere Larsen 0 siblings, 1 reply; 3+ messages in thread From: Hannes Domani @ 2023-09-13 15:53 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] Fix hardware watchpoints in replay mode 2023-09-13 15:53 ` [PATCH] Fix hardware watchpoints in replay mode Hannes Domani @ 2023-09-14 12:20 ` Guinevere Larsen 2023-09-14 16:08 ` Hannes Domani 0 siblings, 1 reply; 3+ messages in thread From: Guinevere Larsen @ 2023-09-14 12:20 UTC (permalink / raw) To: Hannes Domani, gdb-patches 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] Fix hardware watchpoints in replay mode 2023-09-14 12:20 ` Guinevere Larsen @ 2023-09-14 16:08 ` Hannes Domani 0 siblings, 0 replies; 3+ messages in thread From: Hannes Domani @ 2023-09-14 16:08 UTC (permalink / raw) To: gdb-patches, Guinevere Larsen 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-09-14 16:08 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20230913155330.1558-1-ssbssa.ref@yahoo.de> 2023-09-13 15:53 ` [PATCH] Fix hardware watchpoints in replay mode Hannes Domani 2023-09-14 12:20 ` Guinevere Larsen 2023-09-14 16:08 ` 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).