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

["Andrew Burgess" <aburgess@broadcom.com>]

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