[1/2] watchpoint-reuse-slot.exp: skip some tests on targets have different wp and bp registers

Message ID 1426257692-30461-1-git-send-email-qiyaoltc@gmail.com
State New, archived
Headers

Commit Message

Yao Qi March 13, 2015, 2:41 p.m. UTC
  From: Yao Qi <yao.qi@linaro.org>

watchpoint-reuse-slot.exp sets two hardware breakpoint and/or watchpoint,
to test the same debugging register can be used correctly.  However,
on some targets, such as arm and aarch64, hardware has different
registers for breakpoint and watchpoint, so don't have to do test
if one breakpoint and one watchpoint are requested and target hardware
has different debugging registers for breakpoint and watchpoint.

gdb/testsuite:

2015-03-13  Yao Qi  <yao.qi@linaro.org>

	* gdb.base/watchpoint-reuse-slot.exp (dbg_registers_for_watch_and_break):
	New proc.
	(top level): Skip tests if breakpoint and watchpoint are
	requested and dbg_registers_for_watch_and_break returns false.
---
 gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)
  

Comments

Pedro Alves March 16, 2015, 12:34 p.m. UTC | #1
On 03/13/2015 02:41 PM, Yao Qi wrote:
> From: Yao Qi <yao.qi@linaro.org>
> 
> watchpoint-reuse-slot.exp sets two hardware breakpoint and/or watchpoint,
> to test the same debugging register can be used correctly.  However,
> on some targets, such as arm and aarch64, hardware has different
> registers for breakpoint and watchpoint, so don't have to do test
> if one breakpoint and one watchpoint are requested and target hardware
> has different debugging registers for breakpoint and watchpoint.

Hmm, is this just to save test time?  If so, I'd prefer not skipping,
as it may always catch other bugs, in the target backends or
the kernel.

Despite the test's file name, the test doesn't actually create two
breakpoints/watchpoints at the same time, as mentioned at the top
of the file.

In any case:

> +# Return true if the same debugging register can be used for both

s/can be/is/

> +# watchpoint and breakpoint.
> +
> +proc dbg_registers_for_watch_and_break {} {

I think "same" should be in the proc name:

 same_dbg_registers_for_watch_and_break

> +    if { [istarget "arm*-linux*"] || [istarget "aarch64*-*-linux*"] } {
> +	# arm and aarch64 has different registers for watchpoint and

s/has/have/

Thanks,
Pedro Alves
  
Yao Qi March 16, 2015, 2:01 p.m. UTC | #2
Pedro Alves <palves@redhat.com> writes:

> Hmm, is this just to save test time?  If so, I'd prefer not skipping,
> as it may always catch other bugs, in the target backends or
> the kernel.

No, watchpoint-reuse-slot.exp sets some HW breakpoint/watchpoint on some
address doesn't meet the alignment requirements by kernel, kernel
will reject the ptrace (PTRACE_SETHBPREGS) call, and some fails are
caused, for example:

(gdb) PASS: gdb.base/watchpoint-reuse-slot.exp: always-inserted off: watch x hbreak: : width 1, iter 0: base + 0: delete $bpnum
hbreak *(buf.byte + 0 + 1)^M
Hardware assisted breakpoint 80 at 0x410a61^M
(gdb) PASS: gdb.base/watchpoint-reuse-slot.exp: always-inserted off: watch x hbreak: : width 1, iter 0: base + 1: hbreak *(buf.byte + 0 + 1)
stepi^M
Warning:^M
Cannot insert hardware breakpoint 80.^M
Could not insert hardware breakpoints:^M
You may have requested too many hardware breakpoints/watchpoints.^M
^M
(gdb) FAIL: gdb.base/watchpoint-reuse-slot.exp: always-inserted off: watch x hbreak: : width 1, iter 0: base + 1: stepi advanced

hbreak *(buf.byte + 0 + 1)^M
Hardware assisted breakpoint 440 at 0x410a61^M
Warning:^M
Cannot insert hardware breakpoint 440.^M
Could not insert hardware breakpoints:^M
You may have requested too many hardware breakpoints/watchpoints.^M
^M
(gdb) FAIL: gdb.base/watchpoint-reuse-slot.exp: always-inserted on: watch x hbreak: : width 1, iter 0: base + 1: hbreak *(buf.byte + 0 + 1)

Do you suggest that we don't skip these tests even requested
breakpoint/watchpoint don't go in the same slot (debugging register)? so
that the test can cover more.  If the requested address of HW
breakpoint/watchpoint doesn't meet the arch/kernel requirements, we can
skip it, is it OK?

The inner loop of test has two parts, "base + 0" and "base + 1",

		    append prefix "$cmd1 x $cmd2: "
		    with_test_prefix "$prefix: width $width, iter $x" {
			with_test_prefix "base + 0" {
			    watch_command $cmd1 $x 0 $width
			    stepi
			    gdb_test_no_output "delete \$bpnum"
			}
			with_test_prefix "base + 1" {
			    watch_command $cmd2 $x 1 $width
			    stepi
			    gdb_test_no_output "delete \$bpnum"
			}
		    }

if we skip "base + 1" part, do we skip "base + 0" too? if not, prefix in
test summary "$cmd1 x $cmd2: " doesn't reflect the fact.

>
> Despite the test's file name, the test doesn't actually create two
> breakpoints/watchpoints at the same time, as mentioned at the top
> of the file.

Yes, only one breakpoint/watchpoint is inserted at a time.
  
Pedro Alves March 16, 2015, 3:23 p.m. UTC | #3
On 03/16/2015 02:01 PM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>> Hmm, is this just to save test time?  If so, I'd prefer not skipping,
>> as it may always catch other bugs, in the target backends or
>> the kernel.
> 
> No, watchpoint-reuse-slot.exp sets some HW breakpoint/watchpoint on some
> address doesn't meet the alignment requirements by kernel, kernel
> will reject the ptrace (PTRACE_SETHBPREGS) call, and some fails are
> caused, for example:

OK, then different wp and bp registers really is an orthogonal
predicate.  A better one is around the alignment requirements
of a breakpoint.

> 
> (gdb) PASS: gdb.base/watchpoint-reuse-slot.exp: always-inserted off: watch x hbreak: : width 1, iter 0: base + 0: delete $bpnum
> hbreak *(buf.byte + 0 + 1)^M
> Hardware assisted breakpoint 80 at 0x410a61^M
> (gdb) PASS: gdb.base/watchpoint-reuse-slot.exp: always-inserted off: watch x hbreak: : width 1, iter 0: base + 1: hbreak *(buf.byte + 0 + 1)
> stepi^M
> Warning:^M
> Cannot insert hardware breakpoint 80.^M
> Could not insert hardware breakpoints:^M
> You may have requested too many hardware breakpoints/watchpoints.^M
> ^M
> (gdb) FAIL: gdb.base/watchpoint-reuse-slot.exp: always-inserted off: watch x hbreak: : width 1, iter 0: base + 1: stepi advanced
> 
> hbreak *(buf.byte + 0 + 1)^M
> Hardware assisted breakpoint 440 at 0x410a61^M
> Warning:^M
> Cannot insert hardware breakpoint 440.^M
> Could not insert hardware breakpoints:^M
> You may have requested too many hardware breakpoints/watchpoints.^M
> ^M
> (gdb) FAIL: gdb.base/watchpoint-reuse-slot.exp: always-inserted on: watch x hbreak: : width 1, iter 0: base + 1: hbreak *(buf.byte + 0 + 1)
> 
> Do you suggest that we don't skip these tests even requested
> breakpoint/watchpoint don't go in the same slot (debugging register)?

Yes.

> so
> that the test can cover more.  If the requested address of HW
> breakpoint/watchpoint doesn't meet the arch/kernel requirements, we can
> skip it, is it OK?

Yes, that makes sense.

> 
> The inner loop of test has two parts, "base + 0" and "base + 1",
> 
> 		    append prefix "$cmd1 x $cmd2: "
> 		    with_test_prefix "$prefix: width $width, iter $x" {
> 			with_test_prefix "base + 0" {
> 			    watch_command $cmd1 $x 0 $width
> 			    stepi
> 			    gdb_test_no_output "delete \$bpnum"
> 			}
> 			with_test_prefix "base + 1" {
> 			    watch_command $cmd2 $x 1 $width
> 			    stepi
> 			    gdb_test_no_output "delete \$bpnum"
> 			}
> 		    }
> 
> if we skip "base + 1" part, do we skip "base + 0" too? if not, prefix in
> test summary "$cmd1 x $cmd2: " doesn't reflect the fact.

I think skipping both is fine.

Thanks,
Pedro Alves
  

Patch

diff --git a/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp b/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp
index 844bf3a..df6eeb6 100644
--- a/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp
+++ b/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp
@@ -157,6 +157,19 @@  proc watch_command {cmd base offset width} {
     }
 }
 
+# Return true if the same debugging register can be used for both
+# watchpoint and breakpoint.
+
+proc dbg_registers_for_watch_and_break {} {
+    if { [istarget "arm*-linux*"] || [istarget "aarch64*-*-linux*"] } {
+	# arm and aarch64 has different registers for watchpoint and
+	# breakpoint.
+	return 0
+    }
+
+    return 1
+}
+
 # Run test proper.  See intro for description.
 
 foreach always_inserted {"off" "on" } {
@@ -171,6 +184,16 @@  foreach always_inserted {"off" "on" } {
 		    continue
 		}
 
+		if {($cmd1 == "hbreak" && $cmd2 != "hbreak"
+		     || $cmd1 != "hbreak" && $cmd2 == "hbreak")
+		    && ![dbg_registers_for_watch_and_break]} {
+		    # One breakpoint and watchpoint is requested, but
+		    # different registers are used for breakpoint and
+		    # watchpoint, then, the same slot can't be reused.
+		    # Skip it.
+		    continue
+		}
+
 		for {set x 0} {$x < 4} {incr x} {
 		    set prefix "always-inserted $always_inserted: "
 		    append prefix "$cmd1 x $cmd2: "