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

[Pedro Alves <palves at redhat dot com>]

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(-)

diff --git a/gdb/arm-linux-nat.c b/gdb/arm-linux-nat.c
index bb8358c..afc5817 100644
--- a/gdb/arm-linux-nat.c
+++ b/gdb/arm-linux-nat.c
@@ -771,12 +771,20 @@ arm_linux_can_use_hw_breakpoint (struct target_ops *self,
   if (type == bp_hardware_watchpoint || type == bp_read_watchpoint
       || type == bp_access_watchpoint || type == bp_watchpoint)
     {
-      if (cnt + ot > arm_linux_get_hw_watchpoint_count ())
+      int count = arm_linux_get_hw_watchpoint_count ();
+
+      if (count == 0)
+	return 0;
+      else if (cnt + ot > count)
 	return -1;
     }
   else if (type == bp_hardware_breakpoint)
     {
-      if (cnt > arm_linux_get_hw_breakpoint_count ())
+      int count = arm_linux_get_hw_breakpoint_count ();
+
+      if (count == 0)
+	return 0;
+      else if (cnt > count)
 	return -1;
     }
   else
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."));
diff --git a/gdb/remote.c b/gdb/remote.c
index e5236b8..a9baa6d 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -8468,6 +8468,10 @@ remote_check_watch_resources (struct target_ops *self,
     }
   else
     {
+      /* Setting "hardware-watchpoint-length-limit" to 0 effectively
+	 means you can't watch anything.  */
+      if (remote_hw_watchpoint_length_limit == 0)
+	return 0;
       if (remote_hw_watchpoint_limit == 0)
 	return 0;
       else if (remote_hw_watchpoint_limit < 0)
diff --git a/gdb/target.h b/gdb/target.h
index 6c57a69..2292c49 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -1834,7 +1834,7 @@ extern char *target_thread_name (struct thread_info *);
 
 #define target_can_use_hardware_watchpoint(TYPE,CNT,OTHERTYPE) \
  (*current_target.to_can_use_hw_breakpoint) (&current_target,  \
-					     TYPE, CNT, OTHERTYPE);
+					     TYPE, CNT, OTHERTYPE)
 
 /* Returns the number of debug registers needed to watch the given
    memory region, or zero if not supported.  */
-- 
1.9.3