Re: [PATCH] [testsuite] Probe awatch/rwatch support

[Yao Qi <qiyaoltc at gmail dot com>]

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.

-- 
Yao (éå)