* [PATCH] [testsuite] Probe awatch/rwatch support @ 2015-04-14 11:46 Yao Qi 2015-04-14 15:01 ` Pedro Alves 0 siblings, 1 reply; 5+ messages in thread From: Yao Qi @ 2015-04-14 11:46 UTC (permalink / raw) To: gdb-patches 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 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 +} # 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 + } -re "$gdb_prompt $" { pass $test lappend cmds $cmd diff --git a/gdb/testsuite/gdb.multi/watchpoint-multi.exp b/gdb/testsuite/gdb.multi/watchpoint-multi.exp index dcf2f1b..7cd437f 100644 --- a/gdb/testsuite/gdb.multi/watchpoint-multi.exp +++ b/gdb/testsuite/gdb.multi/watchpoint-multi.exp @@ -60,9 +60,9 @@ gdb_load $binfile gdb_breakpoint main {temporary} gdb_test "run" "Temporary breakpoint.* main .*" "start to main inferior 2" -gdb_test "awatch c" \ - "Hardware access \\(read/write\\) watchpoint \[0-9\]+: c" \ - "awatch c on inferior 2" +if { ![gdb_read_access_watchpoint "awatch" "c" "awatch c on inferior 2"] } { + return +} gdb_breakpoint "marker_exit" diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp index f1616e3..1448fba 100644 --- a/gdb/testsuite/lib/gdb.exp +++ b/gdb/testsuite/lib/gdb.exp @@ -455,6 +455,31 @@ proc gdb_breakpoint { function args } { return 1 } +# Insert read/access watchpoint on location LOC. TYPE is either rwatch +# or awatch. Emit PASS, FAIL or UNSUPPORTED in test summary. Return 1 +# for success, 0 for failure. + +proc gdb_read_access_watchpoint { type loc message } { + global gdb_prompt + + gdb_test_multiple "$type $loc" $message { + -re "Target does not support this type of hardware watchpoint\\.\r\n$gdb_prompt $" { + unsupported $message + } + -re "Could not insert hardware watchpoint.*$gdb_prompt $" { + unsupported $message + } + -re "Expression cannot be implemented with read/access watchpoint..*$gdb_prompt $" { + unsupported $message + } + -re "Hardware .* watchpoint .*: .*\r\n$gdb_prompt $" { + pass $message + return 1 + } + } + return 0; +} + # Set breakpoint at function and run gdb until it breaks there. # Since this is the only breakpoint that will be set, if it stops # at a breakpoint, we will assume it is the one we want. We can't -- 1.9.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] [testsuite] Probe awatch/rwatch support 2015-04-14 11:46 [PATCH] [testsuite] Probe awatch/rwatch support Yao Qi @ 2015-04-14 15:01 ` Pedro Alves 2015-04-14 16:06 ` Yao Qi 2015-04-17 9:34 ` Yao Qi 0 siblings, 2 replies; 5+ messages in thread From: Pedro Alves @ 2015-04-14 15:01 UTC (permalink / raw) To: Yao Qi, gdb-patches 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) (¤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. */ -- 1.9.3 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] [testsuite] Probe awatch/rwatch support 2015-04-14 15:01 ` Pedro Alves @ 2015-04-14 16:06 ` Yao Qi 2015-04-14 16:36 ` Pedro Alves 2015-04-17 9:34 ` Yao Qi 1 sibling, 1 reply; 5+ messages in thread From: Yao Qi @ 2015-04-14 16:06 UTC (permalink / raw) To: Pedro Alves; +Cc: Yao Qi, gdb-patches 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 (齐尧) ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] [testsuite] Probe awatch/rwatch support 2015-04-14 16:06 ` Yao Qi @ 2015-04-14 16:36 ` Pedro Alves 0 siblings, 0 replies; 5+ messages in thread From: Pedro Alves @ 2015-04-14 16:36 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] [testsuite] Probe awatch/rwatch support 2015-04-14 15:01 ` Pedro Alves 2015-04-14 16:06 ` Yao Qi @ 2015-04-17 9:34 ` Yao Qi 1 sibling, 0 replies; 5+ messages in thread From: Yao Qi @ 2015-04-17 9:34 UTC (permalink / raw) To: Pedro Alves; +Cc: Yao Qi, gdb-patches 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. -- Yao (齐尧) ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-04-17 9:34 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-04-14 11:46 [PATCH] [testsuite] Probe awatch/rwatch support Yao Qi 2015-04-14 15:01 ` Pedro Alves 2015-04-14 16:06 ` Yao Qi 2015-04-14 16:36 ` Pedro Alves 2015-04-17 9:34 ` Yao Qi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox