[PATCH] Fix read and access watchpoint can't stop

[PATCH] Fix read and access watchpoint can't stop

[Heiher]

gdb/ChangeLog:

2020-06-17  Heiher  <r@hev.cc>

	* breakpoint.c (bpstat_check_watchpoint):
	Fix read and access watchpoint can't stop.
---
 gdb/ChangeLog    | 5 +++++
 gdb/breakpoint.c | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 0a2dc2275d..ecd2ef7c65 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2020-06-17  Heiher  <r@hev.cc>
+
+	* breakpoint.c (bpstat_check_watchpoint):
+	Fix read and access watchpoint can't stop.
+
 2020-06-17  Heiher  <r@hev.cc>
 
 	* mips-linux-nat.c (mips_linux_nat_target::can_use_hw_breakpoint):
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index aead882acd..669ae191ab 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -4952,7 +4952,7 @@ bpstat_check_watchpoint (bpstat bs)
 	   this watchpoint.  */
 	must_check_value = 1;
       else if (b->watchpoint_triggered == watch_triggered_unknown
-	       && b->type == bp_hardware_watchpoint)
+	       && is_hardware_watchpoint (bs->breakpoint_at))
 	/* We were stopped by a hardware watchpoint, but the target could
 	   not report the data address.  We must check the watchpoint's
 	   value.  Access and read watchpoints are out of luck; without
-- 
2.27.0

Re: [PATCH] Fix read and access watchpoint can't stop

[Eli Zaretskii]

> From: Heiher <r@hev.cc>
> Date: Thu, 18 Jun 2020 00:17:08 +0800
> Cc: Heiher <r@hev.cc>
> 
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -4952,7 +4952,7 @@ bpstat_check_watchpoint (bpstat bs)
>  	   this watchpoint.  */
>  	must_check_value = 1;
>        else if (b->watchpoint_triggered == watch_triggered_unknown
> -	       && b->type == bp_hardware_watchpoint)
> +	       && is_hardware_watchpoint (bs->breakpoint_at))
>  	/* We were stopped by a hardware watchpoint, but the target could
>  	   not report the data address.  We must check the watchpoint's
>  	   value.  Access and read watchpoints are out of luck; without

The comment below the line you suggest to modify explains why we don't
handle access and read watchpoints here.  So I don't understand the
rationale for this change (it might be a good idea to explain it in
more detail in the log message).  What am I missing?

Thanks.

Re: [PATCH] Fix read and access watchpoint can't stop

[Hev]

Hello,

On Thu, Jun 18, 2020 at 12:38 AM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: Heiher <r@hev.cc>
> > Date: Thu, 18 Jun 2020 00:17:08 +0800
> > Cc: Heiher <r@hev.cc>
> >
> > --- a/gdb/breakpoint.c
> > +++ b/gdb/breakpoint.c
> > @@ -4952,7 +4952,7 @@ bpstat_check_watchpoint (bpstat bs)
> >          this watchpoint.  */
> >       must_check_value = 1;
> >        else if (b->watchpoint_triggered == watch_triggered_unknown
> > -            && b->type == bp_hardware_watchpoint)
> > +            && is_hardware_watchpoint (bs->breakpoint_at))
> >       /* We were stopped by a hardware watchpoint, but the target could
> >          not report the data address.  We must check the watchpoint's
> >          value.  Access and read watchpoints are out of luck; without
>
> The comment below the line you suggest to modify explains why we don't
> handle access and read watchpoints here.  So I don't understand the
> rationale for this change (it might be a good idea to explain it in
> more detail in the log message).  What am I missing?
>
> Thanks.

This problem only occurs on the target that target_stopped_data_address()
returns false, and b->watchpoint_triggered is set to
watch_triggered_unknown when watchpoint is triggered. so, In
bpstat_check_watchpoint(), the last branch needs to be hit to enable check
value:

 4961       int must_check_value = 0;
 4962
 4963       if (b->type == bp_watchpoint)
 4964     /* For a software watchpoint, we must always check the
 4965        watched value.  */
 4966     must_check_value = 1;
 4967       else if (b->watchpoint_triggered == watch_triggered_yes)
 4968     /* We have a hardware watchpoint (read, write, or access)
 4969        and the target earlier reported an address watched by
 4970        this watchpoint.  */
 4971     must_check_value = 1;
 4972       else if (b->watchpoint_triggered == watch_triggered_unknown
 4973            && b->type == bp_hardware_watchpoint)
 4974     /* We were stopped by a hardware watchpoint, but the target could
 4975        not report the data address.  We must check the watchpoint's
 4976        value.  Access and read watchpoints are out of luck; without
 4977        a data address, we can't figure it out.  */
 4978     must_check_value = 1;

However, the b->type is the actual watchpoint type, bp_hardware_watchpoint
for write-only, bp_read_watchpoint for read-only, and bp_access_watchpoint
for read-write, so it will not hit when read-only and read-write watching.

On mips we don't know the low order 3 bits of the data address, so we must
return false in stopped_data_address(). This is why we have not found the
problem on other platforms (x86/Aarch64/...).

-- 
Best regards!
Hev
https://hev.cc

Re: [PATCH] Fix read and access watchpoint can't stop

[Eli Zaretskii]

> From: Hev <r@hev.cc>
> Date: Thu, 18 Jun 2020 10:05:14 +0800
> Cc: gdb-patches@sourceware.org
> 
> > >       /* We were stopped by a hardware watchpoint, but the target could
> > >          not report the data address.  We must check the watchpoint's
> > >          value.  Access and read watchpoints are out of luck; without
> >
> > The comment below the line you suggest to modify explains why we don't
> > handle access and read watchpoints here.  So I don't understand the
> > rationale for this change (it might be a good idea to explain it in
> > more detail in the log message).  What am I missing?
> >
> > Thanks.
> 
> This problem only occurs on the target that target_stopped_data_address() returns false, and
> b->watchpoint_triggered is set to  watch_triggered_unknown when watchpoint is triggered. so, In
> bpstat_check_watchpoint(), the last branch needs to be hit to enable check value:
> 
>  4961       int must_check_value = 0;
>  4962
>  4963       if (b->type == bp_watchpoint)
>  4964     /* For a software watchpoint, we must always check the
>  4965        watched value.  */
>  4966     must_check_value = 1;
>  4967       else if (b->watchpoint_triggered == watch_triggered_yes)
>  4968     /* We have a hardware watchpoint (read, write, or access)
>  4969        and the target earlier reported an address watched by
>  4970        this watchpoint.  */
>  4971     must_check_value = 1;
>  4972       else if (b->watchpoint_triggered == watch_triggered_unknown
>  4973            && b->type == bp_hardware_watchpoint)
>  4974     /* We were stopped by a hardware watchpoint, but the target could
>  4975        not report the data address.  We must check the watchpoint's
>  4976        value.  Access and read watchpoints are out of luck; without
>  4977        a data address, we can't figure it out.  */
>  4978     must_check_value = 1;
> 
> However, the b->type is the actual watchpoint type, bp_hardware_watchpoint for write-only,
> bp_read_watchpoint for read-only, and bp_access_watchpoint for read-write, so it will not hit when read-only
> and read-write watching.
> 
> On mips we don't know the low order 3 bits of the data address, so we must return false in
> stopped_data_address(). This is why we have not found the problem on other platforms (x86/Aarch64/...).

I understand, but how will this work on MIPS, given that
is_hardware_watchpoint allows any type of watchpoints?  How will GDB
know which watchpoint triggered if there's no address?

And what is the value of bs->breakpoint_at, and how is it different
from b->type?

Thanks.

Re: [PATCH] Fix read and access watchpoint can't stop

[Hev]

Hello,

On Thu, Jun 18, 2020 at 9:35 PM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: Hev <r@hev.cc>
> > Date: Thu, 18 Jun 2020 10:05:14 +0800
> > Cc: gdb-patches@sourceware.org
> >
> > > >       /* We were stopped by a hardware watchpoint, but the target could
> > > >          not report the data address.  We must check the watchpoint's
> > > >          value.  Access and read watchpoints are out of luck; without
> > >
> > > The comment below the line you suggest to modify explains why we don't
> > > handle access and read watchpoints here.  So I don't understand the
> > > rationale for this change (it might be a good idea to explain it in
> > > more detail in the log message).  What am I missing?
> > >
> > > Thanks.
> >
> > This problem only occurs on the target that target_stopped_data_address() returns false, and
> > b->watchpoint_triggered is set to  watch_triggered_unknown when watchpoint is triggered. so, In
> > bpstat_check_watchpoint(), the last branch needs to be hit to enable check value:
> >
> >  4961       int must_check_value = 0;
> >  4962
> >  4963       if (b->type == bp_watchpoint)
> >  4964     /* For a software watchpoint, we must always check the
> >  4965        watched value.  */
> >  4966     must_check_value = 1;
> >  4967       else if (b->watchpoint_triggered == watch_triggered_yes)
> >  4968     /* We have a hardware watchpoint (read, write, or access)
> >  4969        and the target earlier reported an address watched by
> >  4970        this watchpoint.  */
> >  4971     must_check_value = 1;
> >  4972       else if (b->watchpoint_triggered == watch_triggered_unknown
> >  4973            && b->type == bp_hardware_watchpoint)
> >  4974     /* We were stopped by a hardware watchpoint, but the target could
> >  4975        not report the data address.  We must check the watchpoint's
> >  4976        value.  Access and read watchpoints are out of luck; without
> >  4977        a data address, we can't figure it out.  */
> >  4978     must_check_value = 1;
> >
> > However, the b->type is the actual watchpoint type, bp_hardware_watchpoint for write-only,
> > bp_read_watchpoint for read-only, and bp_access_watchpoint for read-write, so it will not hit when read-only
> > and read-write watching.
> >
> > On mips we don't know the low order 3 bits of the data address, so we must return false in
> > stopped_data_address(). This is why we have not found the problem on other platforms (x86/Aarch64/...).
>
> I understand, but how will this work on MIPS, given that
> is_hardware_watchpoint allows any type of watchpoints?  How will GDB
> know which watchpoint triggered if there's no address?
>
> And what is the value of bs->breakpoint_at, and how is it different
> from b->type?
>
> Thanks.

I looked at the code again. Although this changes can keep bs->stop
still equal 0 to make works, But it is not a right way.

Thanks for review.

-- 
Best regards!
Hev
https://hev.cc