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

[Pedro Alves <palves@redhat.com>]

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