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

[Pedro Alves <palves@redhat.com>]

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