From patchwork Tue Apr 14 15:01:26 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 6215 Received: (qmail 41757 invoked by alias); 14 Apr 2015 15:01:41 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 41716 invoked by uid 89); 14 Apr 2015 15:01:41 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL, BAYES_00, SPF_HELO_PASS, SPF_PASS, T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Tue, 14 Apr 2015 15:01:30 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (Postfix) with ESMTPS id E1850AC7A8; Tue, 14 Apr 2015 15:01:28 +0000 (UTC) Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t3EF1Rjo013495; Tue, 14 Apr 2015 11:01:27 -0400 Message-ID: <552D2BC6.5060109@redhat.com> Date: Tue, 14 Apr 2015 16:01:26 +0100 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: Yao Qi , gdb-patches@sourceware.org Subject: Re: [PATCH] [testsuite] Probe awatch/rwatch support References: <1429011949-28215-1-git-send-email-qiyaoltc@gmail.com> In-Reply-To: <1429011949-28215-1-git-send-email-qiyaoltc@gmail.com> On 04/14/2015 12:45 PM, Yao Qi wrote: > From: Yao Qi > > 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 > > * 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 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) (¤t_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. */