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

[Pedro Alves <palves at redhat dot com>]

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