[testsuite] Probe awatch/rwatch support

  On 04/14/2015 12:45 PM, Yao Qi wrote:
> From: Yao Qi <yao.qi@linaro.org>
> 
> I see some awatch/rwatch related fails on one arm board which doesn't hw
> watchpoint or it is not enabled in the kernel, like this,
> 
>   rwatch global^M
>   Expression cannot be implemented with read/access watchpoint.^M
>   (gdb) FAIL: gdb.base/break-idempotent.exp: always-inserted off: rwatch: once: rwatch global
> 
> Although we've had a proc skip_hw_watchpoint_access_tests to check
> whether rwatch/awatch is supported according to target triplet, it
> isn't accurate because HW watchpoint support varies on different
> processor implementations and linux kernel of the same arch.
> 
> This patch is to probe the awatch/rwatch support in a new proc
> gdb_read_access_watchpoint, and callers just bail out the test
> if awatch/rwatch isn't supported or isn't successfully created.
> This patch skips these tests on native arm-linux testing, so fails
> go away.
> 
> -FAIL: gdb.base/break-idempotent.exp: always-inserted off: rwatch: once: rwatch global
> -FAIL: gdb.base/break-idempotent.exp: always-inserted off: rwatch: twice: rwatch global
> -FAIL: gdb.base/break-idempotent.exp: always-inserted off: awatch: once: awatch global
> -FAIL: gdb.base/break-idempotent.exp: always-inserted off: awatch: twice: awatch global
> -FAIL: gdb.base/break-idempotent.exp: always-inserted on: rwatch: once: rwatch global
> -FAIL: gdb.base/break-idempotent.exp: always-inserted on: rwatch: twice: rwatch global
> -FAIL: gdb.base/break-idempotent.exp: always-inserted on: awatch: once: awatch global
> -FAIL: gdb.base/break-idempotent.exp: always-inserted on: awatch: twice: awatch global
> -FAIL: gdb.base/watch-read.exp: set hardware read watchpoint on global variable
> -FAIL: gdb.base/watch-read.exp: read watchpoint triggers on first read (timeout)
> -FAIL: gdb.base/watch-read.exp: read watchpoint triggers on read after value changed (timeout)
> -FAIL: gdb.base/watch-read.exp: set write watchpoint on global variable (timeout)
> -FAIL: gdb.base/watch-read.exp: write watchpoint triggers (timeout)
> -FAIL: gdb.base/watch-read.exp: only write watchpoint triggers when value changes (timeout)
> -FAIL: gdb.base/watch-read.exp: read watchpoint triggers when value doesn't change, trapping reads and writes (timeout)
> -FAIL: gdb.base/watch-read.exp: only read watchpoint triggers when value doesn't change (timeout)
> -FAIL: gdb.base/watch_thread_num.exp: Watchpoint on shared variable
> -FAIL: gdb.base/watch_thread_num.exp: info breakpoint shows watchpoint is thread-specific
> -FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 1 (timeout)
> -FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered iteration 1 (timeout)
> -FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 2 (timeout)
> -FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered iteration 2 (timeout)
> -FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 3 (timeout)
> -FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered iteration 3 (timeout)
> -FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 4 (timeout)
> -FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered iteration 4 (timeout)
> -FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 5 (timeout)
> -FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered iteration 5 (timeout)
> -FAIL: gdb.base/watchpoint-hw-hit-once.exp: continue
> -FAIL: gdb.base/watchpoint-hw-hit-once.exp: continue to break-at-exit (the program exited)
> -FAIL: gdb.multi/watchpoint-multi.exp: awatch c on inferior 2
> 
> this patch should also fix fails in gdb.base/watch_thread_num.exp on
> s390x, which were reporeted here:
> 
>   https://www.sourceware.org/ml/gdb-testers/2015-q2/msg01663.html
> 

s390 fails like this:

 No breakpoints or watchpoints.
 (gdb) awatch shared_var thread 12
 Target does not support this type of hardware watchpoint.
 (gdb) FAIL: gdb.base/watch_thread_num.exp: Watchpoint on shared variable

> gdb/testsuite:
> 
> 2015-04-14  Yao Qi  <yao.qi@linaro.org>
> 
> 	* gdb.base/break-idempotent.exp (set_breakpoint): Match the
> 	output for read/access watchpoint.
> 	* gdb.base/watch-read.exp: Invoke gdb_read_access_watchpoint
> 	and return if it returns false.
> 	* gdb.base/watch_thread_num.exp: Likewise.
> 	* gdb.base/watchpoint-hw-hit-once.exp: Likewise.
> 	* gdb.multi/watchpoint-multi.exp: Likewise.
> 	* gdb.base/watchpoint-reuse-slot.exp: Match the output
> 	for read/access watchpoint.
> 	* lib/gdb.exp (gdb_read_access_watchpoint): New proc.
> ---
>  gdb/testsuite/gdb.base/break-idempotent.exp       |  7 +++++++
>  gdb/testsuite/gdb.base/watch-read.exp             |  8 ++++----
>  gdb/testsuite/gdb.base/watch_thread_num.exp       |  7 ++++---
>  gdb/testsuite/gdb.base/watchpoint-hw-hit-once.exp |  4 +++-
>  gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp  |  3 +++
>  gdb/testsuite/gdb.multi/watchpoint-multi.exp      |  6 +++---
>  gdb/testsuite/lib/gdb.exp                         | 25 +++++++++++++++++++++++
>  7 files changed, 49 insertions(+), 11 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.base/break-idempotent.exp b/gdb/testsuite/gdb.base/break-idempotent.exp
> index c5dae96..ef7db22 100644
> --- a/gdb/testsuite/gdb.base/break-idempotent.exp
> +++ b/gdb/testsuite/gdb.base/break-idempotent.exp
> @@ -107,6 +107,13 @@ proc set_breakpoint { break_command } {
>  	    -re "Could not insert hardware watchpoint.*$gdb_prompt $" {
>  		unsupported $test
>  	    }
> +	    -re "Expression cannot be implemented with read/access watchpoint..*$gdb_prompt $" {
> +		if { $break_command == "watch" } {
> +		    fail $test
> +		} else {
> +		    unsupported $test
> +		}
> +	    }
>  	    -re "atchpoint \[0-9\]+: global\r\n$gdb_prompt $" {
>  		pass $test
>  	    }
> diff --git a/gdb/testsuite/gdb.base/watch-read.exp b/gdb/testsuite/gdb.base/watch-read.exp
> index ddba14e..4bf9a9e 100644
> --- a/gdb/testsuite/gdb.base/watch-read.exp
> +++ b/gdb/testsuite/gdb.base/watch-read.exp
> @@ -42,10 +42,10 @@ set read_line [gdb_get_line_number "read line" $srcfile]
>  
>  # Test running to a read of `global', with a read watchpoint set
>  # watching it.
> -
> -gdb_test "rwatch global" \
> -    "Hardware read watchpoint .*: global" \
> -    "set hardware read watchpoint on global variable"
> +if { ![gdb_read_access_watchpoint "rwatch" "global" \
> +	   "set hardware read watchpoint on global variable"] } {
> +    return
> +}

This seems to be the only case the test message was changed.
Was that intended?

I think that the API looks a bit awkward to use.  I mean, the need to
pass the type as parameter.  Why not wrap it in a couple procedures:

proc gdb_read_watchpoint { loc message } { 
  gdb_read_access_watchpoint "rwatch" $loc $message 
}

proc gdb_access_watchpoint { loc message } { 
  gdb_read_access_watchpoint "awatch" $loc $message 
}


or even gdb_rwatch and gdb_awatch, leaving gdb_read_access_watchpoint
as an implementation detail.

>  
>  # The first read is on entry to the loop.
>  
> diff --git a/gdb/testsuite/gdb.base/watch_thread_num.exp b/gdb/testsuite/gdb.base/watch_thread_num.exp
> index d559f22..1995e5d 100644
> --- a/gdb/testsuite/gdb.base/watch_thread_num.exp
> +++ b/gdb/testsuite/gdb.base/watch_thread_num.exp
> @@ -71,9 +71,10 @@ delete_breakpoints
>  # simultaneously, on targets with continuable watchpoints, such as
>  # x86.  See PR breakpoints/10116.
>  
> -gdb_test "awatch shared_var thread $thread_num" \
> -    "Hardware access \\(read/write\\) watchpoint .*: shared_var.*" \
> -    "Watchpoint on shared variable"
> +if { ![gdb_read_access_watchpoint "awatch" "shared_var thread $thread_num" \
> +	   "Watchpoint on shared variable"] } {
> +    return
> +}
>  
>  gdb_test "info breakpoint \$bpnum" \
>      "stop only in thread $thread_num" \
> diff --git a/gdb/testsuite/gdb.base/watchpoint-hw-hit-once.exp b/gdb/testsuite/gdb.base/watchpoint-hw-hit-once.exp
> index 7ae76e0..d0aa2b9 100644
> --- a/gdb/testsuite/gdb.base/watchpoint-hw-hit-once.exp
> +++ b/gdb/testsuite/gdb.base/watchpoint-hw-hit-once.exp
> @@ -27,7 +27,9 @@ if ![runto_main] {
>      return -1
>  }
>  
> -gdb_test "rwatch watchee"
> +if { ![gdb_read_access_watchpoint "rwatch" "watchee" "rwatch watchee"] } {
> +    return
> +}
>  
>  gdb_breakpoint [gdb_get_line_number "dummy = 2"]
>  
> diff --git a/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp b/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp
> index abe81d6..d9a7cee 100644
> --- a/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp
> +++ b/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp
> @@ -106,6 +106,9 @@ foreach cmd {"watch" "awatch" "rwatch"} {
>  	-re "Target does not support.*$gdb_prompt $" {
>  	    unsupported $test
>  	}
> +	-re "Expression cannot be implemented with read/access watchpoint..*$gdb_prompt $" {
> +	    unsupported $test
> +	}

This seems to revealing something wrong in gdb itself.  This is watching
a single byte or a global variable.  Odd to see that error, which
should mean that the expression involved non-memory values.

If I'm reading the code correctly, either your board is target remote,
and you're setting "set remote hardware-watchpoint-length-limit 0";
or this is native and arm-linux-nat.c:arm_linux_can_use_hw_breakpoint
isn't return 0 to indicate no support.  For the remote case, seems to
me that setting that setting to 0 effectively means watchpoints are
not supported, and thus remote_check_watch_resources should be changed
to return 0 if remote_hw_watchpoint_length_limit==0.

The native case seems like a bug in the backend too -- it should
return 0 when a watchpoint type isn't supported, which can be detected
by arm_linux_get_hw_watchpoint_count returning 0.

And then the core code is checking whether "set can-use-hw-watchpoints"
was used to disable watchpoints, but isn't checking whether the target
supports the watchpoint type at all.

As much as I dislike the watchpoint resource accounting...  I think this
patch will make the errors more sensible, and probably trigger the
pre-existing unsupported watchpoint detection paths in the testsuite
(when there are any).

Completely untested...

From b42292102c769163f91b2bab161710e6e9843de3 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Tue, 14 Apr 2015 15:17:24 +0100
Subject: [PATCH] check whether the target supports the watchpoint type

---
 gdb/arm-linux-nat.c | 12 ++++++++++--
 gdb/breakpoint.c    |  4 ++++
 gdb/remote.c        |  4 ++++
 gdb/target.h        |  2 +-
 4 files changed, 19 insertions(+), 3 deletions(-)
  

Pedro Alves <palves@redhat.com> writes:

> s390 fails like this:
>
>  No breakpoints or watchpoints.
>  (gdb) awatch shared_var thread 12
>  Target does not support this type of hardware watchpoint.
>  (gdb) FAIL: gdb.base/watch_thread_num.exp: Watchpoint on shared variable
>

Since we use gdb_read_access_watchpoint, output "Target does not support
this type of hardware watchpoint." is matched and UNSUPPORTED is emitted.

>> -
>> -gdb_test "rwatch global" \
>> -    "Hardware read watchpoint .*: global" \
>> -    "set hardware read watchpoint on global variable"
>> +if { ![gdb_read_access_watchpoint "rwatch" "global" \
>> +	   "set hardware read watchpoint on global variable"] } {
>> +    return
>> +}
>
> This seems to be the only case the test message was changed.
> Was that intended?

Test message isn't changed.  It is:

PASS: gdb.base/watch-read.exp: set hardware read watchpoint on global variable

>
> I think that the API looks a bit awkward to use.  I mean, the need to
> pass the type as parameter.  Why not wrap it in a couple procedures:
>
> proc gdb_read_watchpoint { loc message } { 
>   gdb_read_access_watchpoint "rwatch" $loc $message 
> }
>
> proc gdb_access_watchpoint { loc message } { 
>   gdb_read_access_watchpoint "awatch" $loc $message 
> }
>
>
> or even gdb_rwatch and gdb_awatch, leaving gdb_read_access_watchpoint
> as an implementation detail.
>

I am fine to change the API to what you suggested...

>>  
>>  # The first read is on entry to the loop.
>>  
>> diff --git a/gdb/testsuite/gdb.base/watch_thread_num.exp b/gdb/testsuite/gdb.base/watch_thread_num.exp
>> index d559f22..1995e5d 100644
>> --- a/gdb/testsuite/gdb.base/watch_thread_num.exp
>> +++ b/gdb/testsuite/gdb.base/watch_thread_num.exp
>> @@ -71,9 +71,10 @@ delete_breakpoints
>>  # simultaneously, on targets with continuable watchpoints, such as
>>  # x86.  See PR breakpoints/10116.
>>  
>> -gdb_test "awatch shared_var thread $thread_num" \
>> -    "Hardware access \\(read/write\\) watchpoint .*: shared_var.*" \
>> -    "Watchpoint on shared variable"
>> +if { ![gdb_read_access_watchpoint "awatch" "shared_var thread $thread_num" \
>> +	   "Watchpoint on shared variable"] } {
>> +    return
>> +}
>>  
>>  gdb_test "info breakpoint \$bpnum" \
>>      "stop only in thread $thread_num" \
>> diff --git a/gdb/testsuite/gdb.base/watchpoint-hw-hit-once.exp b/gdb/testsuite/gdb.base/watchpoint-hw-hit-once.exp
>> index 7ae76e0..d0aa2b9 100644
>> --- a/gdb/testsuite/gdb.base/watchpoint-hw-hit-once.exp
>> +++ b/gdb/testsuite/gdb.base/watchpoint-hw-hit-once.exp
>> @@ -27,7 +27,9 @@ if ![runto_main] {
>>      return -1
>>  }
>>  
>> -gdb_test "rwatch watchee"
>> +if { ![gdb_read_access_watchpoint "rwatch" "watchee" "rwatch watchee"] } {
>> +    return
>> +}
>>  
>>  gdb_breakpoint [gdb_get_line_number "dummy = 2"]
>>  
>> diff --git a/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp b/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp
>> index abe81d6..d9a7cee 100644
>> --- a/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp
>> +++ b/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp
>> @@ -106,6 +106,9 @@ foreach cmd {"watch" "awatch" "rwatch"} {
>>  	-re "Target does not support.*$gdb_prompt $" {
>>  	    unsupported $test
>>  	}
>> +	-re "Expression cannot be implemented with read/access watchpoint..*$gdb_prompt $" {
>> +	    unsupported $test
>> +	}
>
> This seems to revealing something wrong in gdb itself.  This is watching
> a single byte or a global variable.  Odd to see that error, which
> should mean that the expression involved non-memory values.
>

I don't see anything wrong in GDB here.  If rwatch or awatch is used,
and target doesn't have hardware watchpoint, this error should be
printed.  This behaviour is consistent with the doc:

Currently, the @code{awatch} and @code{rwatch} commands can only set
hardware watchpoints, because accesses to data that don't change the
value of the watched expression cannot be detected without examining
every instruction as it is being executed, and @value{GDBN} does not do
that currently.  If @value{GDBN} finds that it is unable to set a
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
hardware breakpoint with the @code{awatch} or @code{rwatch} command, it
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
will print a message like this:

@smallexample
Expression cannot be implemented with read/access watchpoint.
@end smallexample

> If I'm reading the code correctly, either your board is target remote,
> and you're setting "set remote hardware-watchpoint-length-limit 0";
> or this is native and arm-linux-nat.c:arm_linux_can_use_hw_breakpoint
> isn't return 0 to indicate no support.  For the remote case, seems to

I run tests on native arm-linux gdb.  I don't know
target_can_use_hardware_watchpoint returns 0 is to indicate no support.
I'll update the comments to target_can_use_hardware_watchpoint.

> me that setting that setting to 0 effectively means watchpoints are
> not supported, and thus remote_check_watch_resources should be changed
> to return 0 if remote_hw_watchpoint_length_limit==0.
>
> The native case seems like a bug in the backend too -- it should
> return 0 when a watchpoint type isn't supported, which can be detected
> by arm_linux_get_hw_watchpoint_count returning 0.
>
> And then the core code is checking whether "set can-use-hw-watchpoints"
> was used to disable watchpoints, but isn't checking whether the target
> supports the watchpoint type at all.
>
> As much as I dislike the watchpoint resource accounting...  I think this
> patch will make the errors more sensible, and probably trigger the
> pre-existing unsupported watchpoint detection paths in the testsuite
> (when there are any).

I'll give a try to see how errors are changes after your patch.
  

On 04/14/2015 05:06 PM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>> s390 fails like this:
>>
>>  No breakpoints or watchpoints.
>>  (gdb) awatch shared_var thread 12
>>  Target does not support this type of hardware watchpoint.
>>  (gdb) FAIL: gdb.base/watch_thread_num.exp: Watchpoint on shared variable
>>
> 
> Since we use gdb_read_access_watchpoint, output "Target does not support
> this type of hardware watchpoint." is matched and UNSUPPORTED is emitted.
> 
>>> -
>>> -gdb_test "rwatch global" \
>>> -    "Hardware read watchpoint .*: global" \
>>> -    "set hardware read watchpoint on global variable"
>>> +if { ![gdb_read_access_watchpoint "rwatch" "global" \
>>> +	   "set hardware read watchpoint on global variable"] } {
>>> +    return
>>> +}
>>
>> This seems to be the only case the test message was changed.
>> Was that intended?
> 
> Test message isn't changed.  It is:
> 
> PASS: gdb.base/watch-read.exp: set hardware read watchpoint on global variable

Sorry, I misread.

> 
>>
>> I think that the API looks a bit awkward to use.  I mean, the need to
>> pass the type as parameter.  Why not wrap it in a couple procedures:
>>
>> proc gdb_read_watchpoint { loc message } { 
>>   gdb_read_access_watchpoint "rwatch" $loc $message 
>> }
>>
>> proc gdb_access_watchpoint { loc message } { 
>>   gdb_read_access_watchpoint "awatch" $loc $message 
>> }
>>
>>
>> or even gdb_rwatch and gdb_awatch, leaving gdb_read_access_watchpoint
>> as an implementation detail.
>>
> 
> I am fine to change the API to what you suggested...
> 
>>>  
>>>  # The first read is on entry to the loop.
>>>  
>>> diff --git a/gdb/testsuite/gdb.base/watch_thread_num.exp b/gdb/testsuite/gdb.base/watch_thread_num.exp
>>> index d559f22..1995e5d 100644
>>> --- a/gdb/testsuite/gdb.base/watch_thread_num.exp
>>> +++ b/gdb/testsuite/gdb.base/watch_thread_num.exp
>>> @@ -71,9 +71,10 @@ delete_breakpoints
>>>  # simultaneously, on targets with continuable watchpoints, such as
>>>  # x86.  See PR breakpoints/10116.
>>>  
>>> -gdb_test "awatch shared_var thread $thread_num" \
>>> -    "Hardware access \\(read/write\\) watchpoint .*: shared_var.*" \
>>> -    "Watchpoint on shared variable"
>>> +if { ![gdb_read_access_watchpoint "awatch" "shared_var thread $thread_num" \
>>> +	   "Watchpoint on shared variable"] } {
>>> +    return
>>> +}
>>>  
>>>  gdb_test "info breakpoint \$bpnum" \
>>>      "stop only in thread $thread_num" \
>>> diff --git a/gdb/testsuite/gdb.base/watchpoint-hw-hit-once.exp b/gdb/testsuite/gdb.base/watchpoint-hw-hit-once.exp
>>> index 7ae76e0..d0aa2b9 100644
>>> --- a/gdb/testsuite/gdb.base/watchpoint-hw-hit-once.exp
>>> +++ b/gdb/testsuite/gdb.base/watchpoint-hw-hit-once.exp
>>> @@ -27,7 +27,9 @@ if ![runto_main] {
>>>      return -1
>>>  }
>>>  
>>> -gdb_test "rwatch watchee"
>>> +if { ![gdb_read_access_watchpoint "rwatch" "watchee" "rwatch watchee"] } {
>>> +    return
>>> +}
>>>  
>>>  gdb_breakpoint [gdb_get_line_number "dummy = 2"]
>>>  
>>> diff --git a/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp b/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp
>>> index abe81d6..d9a7cee 100644
>>> --- a/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp
>>> +++ b/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp
>>> @@ -106,6 +106,9 @@ foreach cmd {"watch" "awatch" "rwatch"} {
>>>  	-re "Target does not support.*$gdb_prompt $" {
>>>  	    unsupported $test
>>>  	}
>>> +	-re "Expression cannot be implemented with read/access watchpoint..*$gdb_prompt $" {
>>> +	    unsupported $test
>>> +	}
>>
>> This seems to revealing something wrong in gdb itself.  This is watching
>> a single byte or a global variable.  Odd to see that error, which
>> should mean that the expression involved non-memory values.
>>
> 
> I don't see anything wrong in GDB here.  If rwatch or awatch is used,
> and target doesn't have hardware watchpoint, this error should be
> printed.  This behaviour is consistent with the doc:
> 
> Currently, the @code{awatch} and @code{rwatch} commands can only set
> hardware watchpoints, because accesses to data that don't change the
> value of the watched expression cannot be detected without examining
> every instruction as it is being executed, and @value{GDBN} does not do
> that currently.  If @value{GDBN} finds that it is unable to set a
>                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> hardware breakpoint with the @code{awatch} or @code{rwatch} command, it
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> will print a message like this:
> 
> @smallexample
> Expression cannot be implemented with read/access watchpoint.
> @end smallexample

I think that text is in need of TLC too then.  It makes more sense to say
something like that when we try to watch an expression like "rwatch $pc".
That's an expression that can't be watched with a read watch watchpoint,
as it involves watching non-memory values, which we have no hardware
help for.  Not being able to watch an expression the hardware can
watch some memory address (e.g., memory region to be watched is too wide)
I think ideally should get a different error message.  The class of invalid
watchpoints that try to watch non-memory (rwatch $register) can always be detected
upfront by gdb, it's always a user error.  The "insufficient/not-capable-enough" hardware
class of invalid watchpoint may only be detectable at insert time (which
is what would happen if we got rid of all this bogus resource accounting).

> 
>> If I'm reading the code correctly, either your board is target remote,
>> and you're setting "set remote hardware-watchpoint-length-limit 0";
>> or this is native and arm-linux-nat.c:arm_linux_can_use_hw_breakpoint
>> isn't return 0 to indicate no support.  For the remote case, seems to
> 
> I run tests on native arm-linux gdb.  I don't know
> target_can_use_hardware_watchpoint returns 0 is to indicate no support.
> I'll update the comments to target_can_use_hardware_watchpoint.

Yeah, this whole resource accounting code is $expletive ugly.

See all the callers in breakpoint.c:

		  if (target_resources_ok == 0 && !sw_mode)
		    error (_("Target does not support this type of "
			     "hardware watchpoint."));
		  else if (target_resources_ok < 0 && !sw_mode)
		    error (_("There are not enough available hardware "
			     "resources for this watchpoint."));
...
      target_resources_ok =
	target_can_use_hardware_watchpoint (bp_hardware_breakpoint,
					    i + 1, 0);
      if (target_resources_ok == 0)
	error (_("No hardware breakpoint support in the target."));
      else if (target_resources_ok < 0)
	error (_("Hardware breakpoints used exceeds limit."));
...
  can_use_bp = target_can_use_hardware_watchpoint (bp_hardware_breakpoint,
						   bp_count, 0);
  if (can_use_bp < 0)
    error (_("Hardware breakpoints used exceeds limit."));
...
     target_resources_ok =
	target_can_use_hardware_watchpoint (bp_hardware_breakpoint,
					    i + 1, 0);
      if (target_resources_ok == 0)
	error (_("No hardware breakpoint support in the target."));
      else if (target_resources_ok < 0)
	error (_("Hardware breakpoints used exceeds limit."));

So:

== 0 means "no support"

 < 0 means "not enough debug regs slots, sorry"

> 
>> me that setting that setting to 0 effectively means watchpoints are
>> not supported, and thus remote_check_watch_resources should be changed
>> to return 0 if remote_hw_watchpoint_length_limit==0.
>>
>> The native case seems like a bug in the backend too -- it should
>> return 0 when a watchpoint type isn't supported, which can be detected
>> by arm_linux_get_hw_watchpoint_count returning 0.
>>
>> And then the core code is checking whether "set can-use-hw-watchpoints"
>> was used to disable watchpoints, but isn't checking whether the target
>> supports the watchpoint type at all.
>>
>> As much as I dislike the watchpoint resource accounting...  I think this
>> patch will make the errors more sensible, and probably trigger the
>> pre-existing unsupported watchpoint detection paths in the testsuite
>> (when there are any).
> 
> I'll give a try to see how errors are changes after your patch.

Thanks.

Pedro Alves
  

Pedro Alves <palves@redhat.com> writes:

Hi Pedro,
Your patch fixes some fails, but following piece causes a regression

> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 96d2a14..aa7bc02 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -2104,6 +2104,10 @@ update_watchpoint (struct watchpoint *b, int reparse)
>  	      if (!can_use_hw_watchpoints)
>  		error (_("Can't set read/access watchpoint when "
>  			 "hardware watchpoints are disabled."));
> +	      else if (target_can_use_hardware_watchpoint (b->base.type, 1, 0)
> +		       == 0)
> +		error (_("Target does not support this type of "
> +			 "hardware watchpoint."));
>  	      else
>  		error (_("Expression cannot be implemented with "
>  			 "read/access watchpoint."));

rwatch $pc^M
Target does not support this type of hardware watchpoint.^M
(gdb) FAIL: gdb.base/watchpoint.exp: rwatch disallowed for register based expression

without your patch, it is:

rwatch $pc^M
Expression cannot be implemented with read/access watchpoint.^M
(gdb) PASS: gdb.base/watchpoint.exp: rwatch disallowed for register based expression

I've already had a fix to the regression, that is, return more
information from can_use_hardware_watchpoint, so that the caller
(update_watchpoint) can emit sensible errors accordingly.  The patch
looks ugly, so still need some time to improve it.  In the meantime,
other bits of your patch is still very useful, and fixes some fails
without any regressions.  I'll post them out first.