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

["Andrew Burgess" <aburgess at broadcom dot com>]

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" ""