[PATCH] hardware watchpoints turned off, inferior not yet started

[PATCH] hardware watchpoints turned off, inferior not yet started

[Andrew Burgess]

The following seems confusing to me:

$ gdb -q watch.x
Reading symbols from /some/path/to/watch.x...done.
(gdb) set can-use-hw-watchpoints 0
(gdb) watch -l my_var 
Hardware watchpoint 1: -location my_var
(gdb) 

Notice that despite turning hardware watchpoints off
the watchpoint created is reported to be a hardware
watchpoint.  Once I actually start the inferior the
watchpoint is downgraded to a software watchpoint,
but still, this feels like it might cause confusion,
and is pretty easy to fix.

OK to apply?

Thanks,
Andrew


gdb/ChangeLog

2013-10-16  Andrew Burgess  <aburgess@broadcom.com>

	* breakpoint.c (update_watchpoint): Create software watchpoints if
	the inferior has not yet started and hardware watchpoints are
	turned off.

gdb/testsuite/ChangeLog

2013-10-16  Andrew Burgess  <aburgess@broadcom.com>

	* gdb.base/watchpoints.exp: Add test for setting software
	watchpoint before starting the inferior.


diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 5ce50de..2902dc1 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1800,6 +1800,8 @@ update_watchpoint (struct watchpoint *b, int reparse)
       /* Without execution, memory can't change.  No use to try and
 	 set watchpoint locations.  The watchpoint will be reset when
 	 the target gains execution, through breakpoint_re_set.  */
+      if (!can_use_hw_watchpoints)
+	b->base.type = bp_watchpoint;
     }
   else if (within_current_scope && b->exp)
     {
diff --git a/gdb/testsuite/gdb.base/watchpoints.exp b/gdb/testsuite/gdb.base/watchpoints.exp
index 7c10d81..2442bcd 100644
--- a/gdb/testsuite/gdb.base/watchpoints.exp
+++ b/gdb/testsuite/gdb.base/watchpoints.exp
@@ -29,6 +29,17 @@ if {[prepare_for_testing $testfile.exp $testfile $srcfile debug]} {
     return -1
 }
 
+    # Ensure that if we turn off hardware watchpoints and set a watch point
+    # before starting the inferior the watchpoint created will not be a
+    # hardware watchpoint.
+    gdb_test_no_output "set can-use-hw-watchpoints 0" ""
+    gdb_test "watch ival1" "Watchpoint \[0-9\]+: ival1" \
+        "Set software watchpoint before starting the inferior"
+
+    # This will turn hardware watchpoints back on and delete the watchpoint
+    # we just created.
+    clean_restart ${binfile}
+
     # Disable hardware watchpoints if necessary.
     if [target_info exists gdb,no_hardware_watchpoints] {
         gdb_test_no_output "set can-use-hw-watchpoints 0" ""

Re: [PATCH] hardware watchpoints turned off, inferior not yet started

[Pedro Alves]

On 10/16/2013 10:39 AM, Andrew Burgess wrote:
> The following seems confusing to me:
> 
> $ gdb -q watch.x
> Reading symbols from /some/path/to/watch.x...done.
> (gdb) set can-use-hw-watchpoints 0
> (gdb) watch -l my_var 
> Hardware watchpoint 1: -location my_var
> (gdb) 
> 
> Notice that despite turning hardware watchpoints off
> the watchpoint created is reported to be a hardware
> watchpoint.  Once I actually start the inferior the
> watchpoint is downgraded to a software watchpoint,
> but still, this feels like it might cause confusion,
> and is pretty easy to fix.

It seems wrong to me to create it as bp_hardware_watchpoint
in the first place.  That's done in watch_command_1, with:

  if (accessflag == hw_read)
    bp_type = bp_read_watchpoint;
  else if (accessflag == hw_access)
    bp_type = bp_access_watchpoint;
  else
    bp_type = bp_hardware_watchpoint;

If "can-use-hw-watchpoints" is off, then I think it'd
be also better to prohibit creating read and access
watchpoints around here.

> @@ -1800,6 +1800,8 @@ update_watchpoint (struct watchpoint *b, int reparse)
>        /* Without execution, memory can't change.  No use to try and
>  	 set watchpoint locations.  The watchpoint will be reset when
>  	 the target gains execution, through breakpoint_re_set.  */
> +      if (!can_use_hw_watchpoints)
> +	b->base.type = bp_watchpoint;
>      }

... this change I think makes it so that access/read
watchpoints get converted to software watchpoints, which is wrong.

-- 
Pedro Alves

Re: [PATCH] hardware watchpoints turned off, inferior not yet started

[Andrew Burgess]

On 16/10/2013 1:32 PM, Pedro Alves wrote:
> On 10/16/2013 10:39 AM, Andrew Burgess wrote:
>> The following seems confusing to me:
>>
>> $ gdb -q watch.x
>> Reading symbols from /some/path/to/watch.x...done.
>> (gdb) set can-use-hw-watchpoints 0
>> (gdb) watch -l my_var 
>> Hardware watchpoint 1: -location my_var
>> (gdb) 
>>
>> Notice that despite turning hardware watchpoints off
>> the watchpoint created is reported to be a hardware
>> watchpoint.  Once I actually start the inferior the
>> watchpoint is downgraded to a software watchpoint,
>> but still, this feels like it might cause confusion,
>> and is pretty easy to fix.
> 
> It seems wrong to me to create it as bp_hardware_watchpoint
> in the first place.  That's done in watch_command_1, with:
> 
>   if (accessflag == hw_read)
>     bp_type = bp_read_watchpoint;
>   else if (accessflag == hw_access)
>     bp_type = bp_access_watchpoint;
>   else
>     bp_type = bp_hardware_watchpoint;
> 
> If "can-use-hw-watchpoints" is off, then I think it'd
> be also better to prohibit creating read and access
> watchpoints around here.

Though I agree, I looked at doing this and given that we'd
ideally use the 'works_in_software_mode' method from the
breakpoint ops strucutre (rather than replicate the code
inline here) it felt a little messy.

Instead I added a comment here explaining the reasoning,
then fixed up the code below...

> 
>> @@ -1800,6 +1800,8 @@ update_watchpoint (struct watchpoint *b, int reparse)
>>        /* Without execution, memory can't change.  No use to try and
>>  	 set watchpoint locations.  The watchpoint will be reset when
>>  	 the target gains execution, through breakpoint_re_set.  */
>> +      if (!can_use_hw_watchpoints)
>> +	b->base.type = bp_watchpoint;
>>      }
> 
> ... this change I think makes it so that access/read
> watchpoints get converted to software watchpoints, which is wrong.

OK I see that now, I got confused by code later within
update_watchpoint that seems to unconditionally fallback
to bp_watchpoint, but I see now how the read/access watchpoints
actually result in an error. Thanks for pointing this out.

I extended this code to cover this case and added tests.

I know this isn't exactly what you asked for but would this be
acceptable?  If you're still unhappy I'll rewrite to create the
watchpoints in software more.

Thanks,
Andrew

gdb/ChangeLog

	* breakpoint.c (update_watchpoint): Create software watchpoints if
	the inferior has not yet started and hardware watchpoints are
	turned off.
	(watch_command_1): Move watchpoint type selection closer to
	watchpoint creation, and extend the comments.

gdb/testsuite/ChangeLog

	* gdb.base/watchpoints.exp: Add test for setting software
	watchpoints of different types before starting the inferior.

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 5ce50de..e8a2ea8 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1795,11 +1795,19 @@ update_watchpoint (struct watchpoint *b, int reparse)
      don't try to insert watchpoint.  We don't automatically delete
      such watchpoint, though, since failure to parse expression
      is different from out-of-scope watchpoint.  */
-  if ( !target_has_execution)
+  if (!target_has_execution)
     {
       /* Without execution, memory can't change.  No use to try and
 	 set watchpoint locations.  The watchpoint will be reset when
 	 the target gains execution, through breakpoint_re_set.  */
+      if (!can_use_hw_watchpoints)
+	{
+	  if (b->base.ops->works_in_software_mode (&b->base))
+	    b->base.type = bp_watchpoint;
+	  else
+	    error (_("Target does not support software "
+		     "watchpoints of this type."));
+	}
     }
   else if (within_current_scope && b->exp)
     {
@@ -11081,13 +11089,6 @@ watch_command_1 (const char *arg, int accessflag, int from_tty,
   if (*tok)
     error (_("Junk at end of command."));
 
-  if (accessflag == hw_read)
-    bp_type = bp_read_watchpoint;
-  else if (accessflag == hw_access)
-    bp_type = bp_access_watchpoint;
-  else
-    bp_type = bp_hardware_watchpoint;
-
   frame = block_innermost_frame (exp_valid_block);
 
   /* If the expression is "local", then set up a "watchpoint scope"
@@ -11124,7 +11125,17 @@ watch_command_1 (const char *arg, int accessflag, int from_tty,
 	}
     }
 
-  /* Now set up the breakpoint.  */
+  /* Now set up the breakpoint.  We create all watchpoints as hardware
+     watchpoints here even if hardware watchpoints are turned off, a call
+     to update_watchpoint later in this function will cause the type to
+     drop back to bp_watchpoint (software watchpoint) if required.  */
+
+  if (accessflag == hw_read)
+    bp_type = bp_read_watchpoint;
+  else if (accessflag == hw_access)
+    bp_type = bp_access_watchpoint;
+  else
+    bp_type = bp_hardware_watchpoint;
 
   w = XCNEW (struct watchpoint);
   b = &w->base;
diff --git a/gdb/testsuite/gdb.base/watchpoints.exp b/gdb/testsuite/gdb.base/watchpoints.exp
index 7c10d81..ced4fb2 100644
--- a/gdb/testsuite/gdb.base/watchpoints.exp
+++ b/gdb/testsuite/gdb.base/watchpoints.exp
@@ -29,6 +29,27 @@ if {[prepare_for_testing $testfile.exp $testfile $srcfile debug]} {
     return -1
 }
 
+    # Ensure that if we turn off hardware watchpoints and set a watch point
+    # before starting the inferior the watchpoint created will not be a
+    # hardware watchpoint.
+    gdb_test_no_output "set can-use-hw-watchpoints 0" ""
+    gdb_test "watch ival1" "Watchpoint \[0-9\]+: ival1" \
+        "Set software watchpoint before starting the inferior"
+
+    # The next tests are written to match the current state of gdb: access
+    # and read watchpoints require hardware watchpoint support, with this
+    # turned off these can't be created.
+    gdb_test "awatch ival1" \
+        "Target does not support software watchpoints of this type." \
+        "Set software watchpoint before starting the inferior"
+    gdb_test "rwatch ival1" \
+        "Target does not support software watchpoints of this type." \
+        "Set software watchpoint before starting the inferior"
+
+    # This will turn hardware watchpoints back on and delete the watchpoint
+    # we just created.
+    clean_restart ${binfile}
+
     # Disable hardware watchpoints if necessary.
     if [target_info exists gdb,no_hardware_watchpoints] {
         gdb_test_no_output "set can-use-hw-watchpoints 0" ""




 

Re: [PATCH] hardware watchpoints turned off, inferior not yet started

[Pedro Alves]

On 10/16/2013 04:43 PM, Andrew Burgess wrote:
> On 16/10/2013 1:32 PM, Pedro Alves wrote:
>> ... this change I think makes it so that access/read
>> watchpoints get converted to software watchpoints, which is wrong.
> 
> OK I see that now, I got confused by code later within
> update_watchpoint that seems to unconditionally fallback
> to bp_watchpoint, but I see now how the read/access watchpoints
> actually result in an error. Thanks for pointing this out.
> 
> I extended this code to cover this case and added tests.

> gdb/ChangeLog
> 
> 	* breakpoint.c (update_watchpoint): Create software watchpoints if
> 	the inferior has not yet started and hardware watchpoints are
> 	turned off.

"Create" isn't really accurate.  This function is called for resets too.
So, something like
 "file foo; start; watch; kill; set can-use-hw-watchpoint 0; file foo"
will trigger that code path too, which I think will end up resulting
in access/read watchpoints being disabled, with something like:

  "file foo; start; awatch; kill; set can-use-hw-watchpoint 0; file foo"

(Replace the last "file" with anything that triggers a reset.)

BTW, could you confirm that?

Anyway, I suggest:

 	* breakpoint.c (update_watchpoint): If hardware watchpoints are
 	forced off, downgrade them to software watchpoints if possible,
	and error out if not possible.

> +    # Ensure that if we turn off hardware watchpoints and set a watch point
> +    # before starting the inferior the watchpoint created will not be a
> +    # hardware watchpoint.
> +    gdb_test_no_output "set can-use-hw-watchpoints 0" ""
> +    gdb_test "watch ival1" "Watchpoint \[0-9\]+: ival1" \
> +        "Set software watchpoint before starting the inferior"
> +
> +    # The next tests are written to match the current state of gdb: access
> +    # and read watchpoints require hardware watchpoint support, with this
> +    # turned off these can't be created.
> +    gdb_test "awatch ival1" \
> +        "Target does not support software watchpoints of this type." \
> +        "Set software watchpoint before starting the inferior"
> +    gdb_test "rwatch ival1" \
> +        "Target does not support software watchpoints of this type." \
> +        "Set software watchpoint before starting the inferior"

The "Set software watchpoint before starting the inferior" string will
appear in gdb.sum for the three tests.  Please make those unique per test.

Otherwise OK.

Thanks,
-- 
Pedro Alves

Re: [PATCH] hardware watchpoints turned off, inferior not yet started

[Andrew Burgess]

On 16/10/2013 5:28 PM, Pedro Alves wrote:
> On 10/16/2013 04:43 PM, Andrew Burgess wrote:
>> On 16/10/2013 1:32 PM, Pedro Alves wrote:
>>> ... this change I think makes it so that access/read
>>> watchpoints get converted to software watchpoints, which is wrong.
>>
>> OK I see that now, I got confused by code later within
>> update_watchpoint that seems to unconditionally fallback
>> to bp_watchpoint, but I see now how the read/access watchpoints
>> actually result in an error. Thanks for pointing this out.
>>
>> I extended this code to cover this case and added tests.
> 
>> gdb/ChangeLog
>>
>> 	* breakpoint.c (update_watchpoint): Create software watchpoints if
>> 	the inferior has not yet started and hardware watchpoints are
>> 	turned off.
> 
> "Create" isn't really accurate.  This function is called for resets too.
> So, something like
>  "file foo; start; watch; kill; set can-use-hw-watchpoint 0; file foo"
> will trigger that code path too, which I think will end up resulting
> in access/read watchpoints being disabled, with something like:
> 
>   "file foo; start; awatch; kill; set can-use-hw-watchpoint 0; file foo"

You're sort-of correct, without my patch the above commands will not trigger
any error, then when the inferior is started I see 4 errors like this:

"Error in re-setting breakpoint 2: Expression cannot be implemented with read/access watchpoint."

Then the inferior starts and the the watchpoint is hit, we've placed a
hardware watchpoint.  The reason for this is that the code in
breakpoint_re_set catches the error from update_watchpoint, but does nothing
with it.  This feels a little strange and I have a follow-up patch in
this area.

After my patch then we do indeed see the error earlier, when the second
"file foo" is issued.  For the same reason as above, this error doesn't
have and impact on the watchpoint, and when we start the inferior we see
another 4 errors followed by a hardware watchpoint being placed.

> Anyway, I suggest:
> 
>  	* breakpoint.c (update_watchpoint): If hardware watchpoints are
>  	forced off, downgrade them to software watchpoints if possible,
> 	and error out if not possible.

Fixed.

> The "Set software watchpoint before starting the inferior" string will
> appear in gdb.sum for the three tests.  Please make those unique per test.

Fixed.

> 
> Otherwise OK.

I've included the latest version here, is this OK to apply?


Thanks,
Andrew

gdb/ChangeLog

	* breakpoint.c (update_watchpoint): If hardware watchpoints are
	forced off, downgrade them to software watchpoints if possible,
	and error out if not possible.
	(watch_command_1): Move watchpoint type selection closer to
	watchpoint creation, and extend the comments.
    
gdb/testsuite/ChangeLog

	* gdb.base/watchpoints.exp: Add test for setting software
	watchpoints of different types before starting the inferior.

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 5ce50de..c630b87 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1795,11 +1795,18 @@ update_watchpoint (struct watchpoint *b, int reparse)
      don't try to insert watchpoint.  We don't automatically delete
      such watchpoint, though, since failure to parse expression
      is different from out-of-scope watchpoint.  */
-  if ( !target_has_execution)
+  if (!target_has_execution)
     {
       /* Without execution, memory can't change.  No use to try and
 	 set watchpoint locations.  The watchpoint will be reset when
 	 the target gains execution, through breakpoint_re_set.  */
+      if (!can_use_hw_watchpoints)
+	{
+	  if (b->base.ops->works_in_software_mode (&b->base))
+	    b->base.type = bp_watchpoint;
+	  else
+	    error (_("Software read/access watchpoints not supported."));
+	}
     }
   else if (within_current_scope && b->exp)
     {
@@ -11081,13 +11088,6 @@ watch_command_1 (const char *arg, int accessflag, int from_tty,
   if (*tok)
     error (_("Junk at end of command."));
 
-  if (accessflag == hw_read)
-    bp_type = bp_read_watchpoint;
-  else if (accessflag == hw_access)
-    bp_type = bp_access_watchpoint;
-  else
-    bp_type = bp_hardware_watchpoint;
-
   frame = block_innermost_frame (exp_valid_block);
 
   /* If the expression is "local", then set up a "watchpoint scope"
@@ -11124,7 +11124,17 @@ watch_command_1 (const char *arg, int accessflag, int from_tty,
 	}
     }
 
-  /* Now set up the breakpoint.  */
+  /* Now set up the breakpoint.  We create all watchpoints as hardware
+     watchpoints here even if hardware watchpoints are turned off, a call
+     to update_watchpoint later in this function will cause the type to
+     drop back to bp_watchpoint (software watchpoint) if required.  */
+
+  if (accessflag == hw_read)
+    bp_type = bp_read_watchpoint;
+  else if (accessflag == hw_access)
+    bp_type = bp_access_watchpoint;
+  else
+    bp_type = bp_hardware_watchpoint;
 
   w = XCNEW (struct watchpoint);
   b = &w->base;
diff --git a/gdb/testsuite/gdb.base/watchpoints.exp b/gdb/testsuite/gdb.base/watchpoints.exp
index 7c10d81..63c47ce 100644
--- a/gdb/testsuite/gdb.base/watchpoints.exp
+++ b/gdb/testsuite/gdb.base/watchpoints.exp
@@ -29,6 +29,29 @@ if {[prepare_for_testing $testfile.exp $testfile $srcfile debug]} {
     return -1
 }
 
+with_test_prefix "before inferior start" {
+    # Ensure that if we turn off hardware watchpoints and set a watch point
+    # before starting the inferior the watchpoint created will not be a
+    # hardware watchpoint.
+    gdb_test_no_output "set can-use-hw-watchpoints 0" ""
+    gdb_test "watch ival1" "Watchpoint \[0-9\]+: ival1" \
+        "create watchpoint"
+
+    # The next tests are written to match the current state of gdb: access
+    # and read watchpoints require hardware watchpoint support, with this
+    # turned off these can't be created.
+    gdb_test "awatch ival1" \
+        "Target does not support software watchpoints of this type." \
+        "create access watchpoint"
+    gdb_test "rwatch ival1" \
+        "Target does not support software watchpoints of this type." \
+        "create read watchpoint"
+}
+
+    # This will turn hardware watchpoints back on and delete the watchpoint
+    # we just created.
+    clean_restart ${binfile}
+
     # Disable hardware watchpoints if necessary.
     if [target_info exists gdb,no_hardware_watchpoints] {
         gdb_test_no_output "set can-use-hw-watchpoints 0" ""

Re: [PATCH] hardware watchpoints turned off, inferior not yet started

[Andrew Burgess]

On 18/10/2013 10:14 AM, Andrew Burgess wrote:
> 
> I've included the latest version here, is this OK to apply?

No! I stupidly left part of this patch merged with another
commit in my local tree, as a result, the patch I posted
fails its own tests.

Sorry for the noise, here's a new version, is this OK to apply?

Andrew

gdb/ChangeLog

	* breakpoint.c (update_watchpoint): If hardware watchpoints are
	forced off, downgrade them to software watchpoints if possible,
	and error out if not possible.
	(watch_command_1): Move watchpoint type selection closer to
	watchpoint creation, and extend the comments.

gdb/testsuite/ChangeLog

	* gdb.base/watchpoints.exp: Add test for setting software
	watchpoints of different types before starting the inferior.

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 5ce50de..c630b87 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1795,11 +1795,18 @@ update_watchpoint (struct watchpoint *b, int reparse)
      don't try to insert watchpoint.  We don't automatically delete
      such watchpoint, though, since failure to parse expression
      is different from out-of-scope watchpoint.  */
-  if ( !target_has_execution)
+  if (!target_has_execution)
     {
       /* Without execution, memory can't change.  No use to try and
 	 set watchpoint locations.  The watchpoint will be reset when
 	 the target gains execution, through breakpoint_re_set.  */
+      if (!can_use_hw_watchpoints)
+	{
+	  if (b->base.ops->works_in_software_mode (&b->base))
+	    b->base.type = bp_watchpoint;
+	  else
+	    error (_("Software read/access watchpoints not supported."));
+	}
     }
   else if (within_current_scope && b->exp)
     {
@@ -11081,13 +11088,6 @@ watch_command_1 (const char *arg, int accessflag, int from_tty,
   if (*tok)
     error (_("Junk at end of command."));
 
-  if (accessflag == hw_read)
-    bp_type = bp_read_watchpoint;
-  else if (accessflag == hw_access)
-    bp_type = bp_access_watchpoint;
-  else
-    bp_type = bp_hardware_watchpoint;
-
   frame = block_innermost_frame (exp_valid_block);
 
   /* If the expression is "local", then set up a "watchpoint scope"
@@ -11124,7 +11124,17 @@ watch_command_1 (const char *arg, int accessflag, int from_tty,
 	}
     }
 
-  /* Now set up the breakpoint.  */
+  /* Now set up the breakpoint.  We create all watchpoints as hardware
+     watchpoints here even if hardware watchpoints are turned off, a call
+     to update_watchpoint later in this function will cause the type to
+     drop back to bp_watchpoint (software watchpoint) if required.  */
+
+  if (accessflag == hw_read)
+    bp_type = bp_read_watchpoint;
+  else if (accessflag == hw_access)
+    bp_type = bp_access_watchpoint;
+  else
+    bp_type = bp_hardware_watchpoint;
 
   w = XCNEW (struct watchpoint);
   b = &w->base;
diff --git a/gdb/testsuite/gdb.base/watchpoints.exp b/gdb/testsuite/gdb.base/watchpoints.exp
index 7c10d81..b70e86c 100644
--- a/gdb/testsuite/gdb.base/watchpoints.exp
+++ b/gdb/testsuite/gdb.base/watchpoints.exp
@@ -29,6 +29,29 @@ if {[prepare_for_testing $testfile.exp $testfile $srcfile debug]} {
     return -1
 }
 
+with_test_prefix "before inferior start" {
+    # Ensure that if we turn off hardware watchpoints and set a watch point
+    # before starting the inferior the watchpoint created will not be a
+    # hardware watchpoint.
+    gdb_test_no_output "set can-use-hw-watchpoints 0" ""
+    gdb_test "watch ival1" "Watchpoint \[0-9\]+: ival1" \
+        "create watchpoint"
+
+    # The next tests are written to match the current state of gdb: access
+    # and read watchpoints require hardware watchpoint support, with this
+    # turned off these can't be created.
+    gdb_test "awatch ival1" \
+        "Software read/access watchpoints not supported." \
+        "create access watchpoint"
+    gdb_test "rwatch ival1" \
+        "Software read/access watchpoints not supported." \
+        "create read watchpoint"
+}
+
+    # This will turn hardware watchpoints back on and delete the watchpoint
+    # we just created.
+    clean_restart ${binfile}
+
     # Disable hardware watchpoints if necessary.
     if [target_info exists gdb,no_hardware_watchpoints] {
         gdb_test_no_output "set can-use-hw-watchpoints 0" ""

Re: [PATCH] hardware watchpoints turned off, inferior not yet started

[Pedro Alves]

On 10/18/2013 10:14 AM, Andrew Burgess wrote:
> After my patch then we do indeed see the error earlier, when the second
> "file foo" is issued.

BTW, not super important, but thinking out loud, I think it'd
make sense to have the "set can-use-hw-watchpoints" command
itself trigger a breakpoint/watchpoint reset.

-- 
Pedro Alves

Re: [PATCH] hardware watchpoints turned off, inferior not yet started

[Pedro Alves]

On 10/18/2013 03:37 PM, Andrew Burgess wrote:
> On 18/10/2013 10:14 AM, Andrew Burgess wrote:
>>
>> I've included the latest version here, is this OK to apply?
> 
> No! I stupidly left part of this patch merged with another
> commit in my local tree, as a result, the patch I posted
> fails its own tests.
> 
> Sorry for the noise, here's a new version, is this OK to apply?

Yep.  Thanks!

-- 
Pedro Alves

Re: [PATCH] hardware watchpoints turned off, inferior not yet started

[Andrew Burgess]

On 18/10/2013 4:25 PM, Pedro Alves wrote:
> On 10/18/2013 10:14 AM, Andrew Burgess wrote:
>> After my patch then we do indeed see the error earlier, when the second
>> "file foo" is issued.
> 
> BTW, not super important, but thinking out loud, I think it'd
> make sense to have the "set can-use-hw-watchpoints" command
> itself trigger a breakpoint/watchpoint reset.

I initially planned to do just this, but the manual says that after
a "set can-use-hw-breakpoints 0" previous breakpoints will remain
hardware while future ones will be software.

This clearly breaks a little (depending on interpretation) with
restarting the inferior / changing the file but we'd either need
to update the manual or carry additional information around on the
watchpoint to indicate if it's supposed to be HW/SW.

Thanks for taking the time to review my patch :)

Andrew

Re: [PATCH] hardware watchpoints turned off, inferior not yet started

[Andrew Burgess]

On 18/10/2013 4:33 PM, Pedro Alves wrote:
> On 10/18/2013 03:37 PM, Andrew Burgess wrote:
>> On 18/10/2013 10:14 AM, Andrew Burgess wrote:
>>
>> Sorry for the noise, here's a new version, is this OK to apply?
> 
> Yep.  Thanks!
> 

Committed.

Thanks,
Andrew

Re: [PATCH] hardware watchpoints turned off, inferior not yet started

[Pedro Alves]

On 11/07/2013 11:45 AM, Jose E. Marchesi wrote:
> 
> [Following https://sourceware.org/ml/gdb-patches/2013-10/msg00563.html]
> 
> Hi.
> 
> The patch posted above introduced a regression in sparc64-*-linux-gnu,
> and possibly in other targets not supporting hardware watchpoints at
> all.
> 
> Today I was surprised to find this in a sparc64-*-linux-gnu target:
> 
>  $ gdb foo
>  ... intro text ...
>  (gdb) watch global_var
>  Hardware watchpoint 1: global_var
> 
> The cause for this regression is this code in update_watchpoint
> (breakpoint.c):
> 
> +  if (!target_has_execution)
>      {
>        /* Without execution, memory can't change.  No use to try and
>          set watchpoint locations.  The watchpoint will be reset when
>          the target gains execution, through breakpoint_re_set.  */
> +      if (!can_use_hw_watchpoints)
> +       {
> +         if (b->base.ops->works_in_software_mode (&b->base))
> +           b->base.type = bp_watchpoint;
> +         else
> +           error (_("Software read/access watchpoints not supported."));
> +       }

I'm not following how this could have caused this regression.  Before
the patch, AFAICS, there was _nothing_ here that degraded the watchpoint.

I actually don't think there's a regression here at all.  Only if you
compare back a few GDB releases could you perhaps call this a regression
(to when before watchpoint support was made to go all through the
target vector).

> 
> The watchpoint must be downgraded to a software watchpoint if the target
> does not support hw watchpoints, even if can_use_hw_watchpoints is 1.

If the program isn't running yet, then target_can_use_hardware_watchpoint
will always return false for all native targets.  So this must also
be e.g., changing behavior for x86?

At this point, before starting the inferior, we don't really know
whether the user will do "run" (spawning a local process), or
"target remote" (or some other target), so we can't really know
whether hardware watchpoints will work or not.

If you're connected to an extended-remote gdbserver, then
remote_check_watch_resources does end up being called by
target_can_use_hardware_watchpoint, so one could claim that
it makes some sense to call it here, like in your patch
(though I'm of the opinion that all this resource accounting
stuff is fundamentally broken).

But then your new test will fail for extended-remote gdbserver,
because a hardware watchpoint will indeed be created.
Maybe we should just declare that "watch" if the program is
not running always creates a software watchpoint, that might
or not get "upgraded" once execution starts, making all targets
behave the same...

> The following patch fixes this and also adds an additional test to
> testsuite/watchpoints.exp to catch this problem.
> 
> 2013-11-07  Jose E. Marchesi  <jose.marchesi@oracle.com>
> 
> 	* breakpoint.c (update_watchpoint): Downgrade hw watchpoints to sw
> 	watchpoints in targets not supporting them, when the inferior is
>         not running.
> 
> 2013-11-07  Jose E. Marchesi  <jose.marchesi@oracle.com>
> 
> 	* gdb.base/watchpoints.exp: Add a test to ensure that a watchpoint
> 	is not created as a hw watchpoint in targets not supporting them,
> 	even if can-use-hw-watchpoints is 1 when the inferior is not
>         running.
> 
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index ffe73fd..597e6f9 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -1800,7 +1800,10 @@ update_watchpoint (struct watchpoint *b, int reparse)
>        /* Without execution, memory can't change.  No use to try and
>  	 set watchpoint locations.  The watchpoint will be reset when
>  	 the target gains execution, through breakpoint_re_set.  */
> -      if (!can_use_hw_watchpoints)
> +      int i = hw_breakpoint_used_count ();
> +      int target_supports_hw_watchpoints =
> +	target_can_use_hardware_watchpoint (bp_hardware_watchpoint, i + 1, 0);

'=' goes on the next line.

The breakpoint type here might be an access or read watchpoint too.  It's
really not correct to always pass bp_hardware_watchpoint down to
target_can_use_hardware_watchpoint.

"supports" in the variable name is a bit misleading, because "i+1"
might be too many resources, even though the target might support
hardware watchpoints.


> +      if (!target_supports_hw_watchpoints || !can_use_hw_watchpoints)
>  	{
>  	  if (b->base.ops->works_in_software_mode (&b->base))
>  	    b->base.type = bp_watchpoint;
> --- a/gdb/testsuite/gdb.base/watchpoints.exp
> +++ b/gdb/testsuite/gdb.base/watchpoints.exp
> @@ -30,6 +30,15 @@ if {[prepare_for_testing $testfile.exp $testfile $srcfile debug]} {
>  }
>  
>  with_test_prefix "before inferior start" {
> +    if [target_info exists gdb,no_hardware_watchpoints] {
> +	# Ensure that a watchpoint is not created as a hw watchpoint
> +	# even if can-use-hw-watchpoints is 1 when the inferior is not
> +	# running.
> +	gdb_test_no_output "set can-use-hw-watchpoints 1"
> +	gdb_test "watch ival1" "Watchpoint \[0-9\]+: ival1" \
> +	    "create sw watchpoint"
> +    }
> +
>      # Ensure that if we turn off hardware watchpoints and set a watch point
>      # before starting the inferior the watchpoint created will not be a
>      # hardware watchpoint.
> 
> 

-- 
Pedro Alves

Re: [PATCH] hardware watchpoints turned off, inferior not yet started

[Jose E. Marchesi]

[Following https://sourceware.org/ml/gdb-patches/2013-10/msg00563.html]

Hi.

The patch posted above introduced a regression in sparc64-*-linux-gnu,
and possibly in other targets not supporting hardware watchpoints at
all.

Today I was surprised to find this in a sparc64-*-linux-gnu target:

 $ gdb foo
 ... intro text ...
 (gdb) watch global_var
 Hardware watchpoint 1: global_var

The cause for this regression is this code in update_watchpoint
(breakpoint.c):

+  if (!target_has_execution)
     {
       /* Without execution, memory can't change.  No use to try and
         set watchpoint locations.  The watchpoint will be reset when
         the target gains execution, through breakpoint_re_set.  */
+      if (!can_use_hw_watchpoints)
+       {
+         if (b->base.ops->works_in_software_mode (&b->base))
+           b->base.type = bp_watchpoint;
+         else
+           error (_("Software read/access watchpoints not supported."));
+       }

The watchpoint must be downgraded to a software watchpoint if the target
does not support hw watchpoints, even if can_use_hw_watchpoints is 1.

The following patch fixes this and also adds an additional test to
testsuite/watchpoints.exp to catch this problem.

2013-11-07  Jose E. Marchesi  <jose.marchesi@oracle.com>

	* breakpoint.c (update_watchpoint): Downgrade hw watchpoints to sw
	watchpoints in targets not supporting them, when the inferior is
        not running.

2013-11-07  Jose E. Marchesi  <jose.marchesi@oracle.com>

	* gdb.base/watchpoints.exp: Add a test to ensure that a watchpoint
	is not created as a hw watchpoint in targets not supporting them,
	even if can-use-hw-watchpoints is 1 when the inferior is not
        running.

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index ffe73fd..597e6f9 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1800,7 +1800,10 @@ update_watchpoint (struct watchpoint *b, int reparse)
       /* Without execution, memory can't change.  No use to try and
 	 set watchpoint locations.  The watchpoint will be reset when
 	 the target gains execution, through breakpoint_re_set.  */
-      if (!can_use_hw_watchpoints)
+      int i = hw_breakpoint_used_count ();
+      int target_supports_hw_watchpoints =
+	target_can_use_hardware_watchpoint (bp_hardware_watchpoint, i + 1, 0);
+      if (!target_supports_hw_watchpoints || !can_use_hw_watchpoints)
 	{
 	  if (b->base.ops->works_in_software_mode (&b->base))
 	    b->base.type = bp_watchpoint;
--- a/gdb/testsuite/gdb.base/watchpoints.exp
+++ b/gdb/testsuite/gdb.base/watchpoints.exp
@@ -30,6 +30,15 @@ if {[prepare_for_testing $testfile.exp $testfile $srcfile debug]} {
 }
 
 with_test_prefix "before inferior start" {
+    if [target_info exists gdb,no_hardware_watchpoints] {
+	# Ensure that a watchpoint is not created as a hw watchpoint
+	# even if can-use-hw-watchpoints is 1 when the inferior is not
+	# running.
+	gdb_test_no_output "set can-use-hw-watchpoints 1"
+	gdb_test "watch ival1" "Watchpoint \[0-9\]+: ival1" \
+	    "create sw watchpoint"
+    }
+
     # Ensure that if we turn off hardware watchpoints and set a watch point
     # before starting the inferior the watchpoint created will not be a
     # hardware watchpoint.