[PATCH] wp-replication: Fix test case loop

[PATCH] wp-replication: Fix test case loop

[Andreas Arnez]

When executing wp-replication.exp on S/390, the loop that counts the
number of available hardware watchpoints does not terminate properly,
because *all* "watch" commands yield "hardware watchpoints".

Since the C source code is not prepared for more than NR_THREADS
hardware watchpoints anyhow, I suggest to add the appropriate exit
condition to the loop.


2013-07-05  Andreas Arnez  <arnez@linux.vnet.ibm.com>

	* gdb.threads/wp-replication.exp: Stop counting available hardware
	watchpoints after NR_THREADS iterations.

diff --git a/gdb/testsuite/gdb.threads/wp-replication.exp b/gdb/testsuite/gdb.threads/wp-replication.exp
index 8927a43..af3594d 100644
--- a/gdb/testsuite/gdb.threads/wp-replication.exp
+++ b/gdb/testsuite/gdb.threads/wp-replication.exp
@@ -81,6 +81,10 @@ while { $done == 0 } {
   gdb_test_multiple "continue" "watchpoint created successfully" {
     -re ".*Breakpoint 2, empty_cycle \\(\\).*$gdb_prompt $" {
       incr hwatch_count
+      if { $hwatch_count == $NR_THREADS } {
+	set done 1
+	break
+      }
     }
     -re ".*Could not insert hardware watchpoint.*$gdb_prompt $" {
       set done 1

Re: [PATCH] wp-replication: Fix test case loop

[Luis Machado]

Hi,

On 07/05/2013 01:10 PM, Andreas Arnez wrote:
> When executing wp-replication.exp on S/390, the loop that counts the
> number of available hardware watchpoints does not terminate properly,
> because *all* "watch" commands yield "hardware watchpoints".
>
> Since the C source code is not prepared for more than NR_THREADS
> hardware watchpoints anyhow, I suggest to add the appropriate exit
> condition to the loop.

That's an interesting behavior. Are the resources on s390 nearly 
unlimited for hardware watchpoints? Does it run out of hardware 
watchpoints eventually?

>
>
> 2013-07-05  Andreas Arnez  <arnez@linux.vnet.ibm.com>
>
> 	* gdb.threads/wp-replication.exp: Stop counting available hardware
> 	watchpoints after NR_THREADS iterations.
>
> diff --git a/gdb/testsuite/gdb.threads/wp-replication.exp b/gdb/testsuite/gdb.threads/wp-replication.exp
> index 8927a43..af3594d 100644
> --- a/gdb/testsuite/gdb.threads/wp-replication.exp
> +++ b/gdb/testsuite/gdb.threads/wp-replication.exp
> @@ -81,6 +81,10 @@ while { $done == 0 } {
>     gdb_test_multiple "continue" "watchpoint created successfully" {
>       -re ".*Breakpoint 2, empty_cycle \\(\\).*$gdb_prompt $" {
>         incr hwatch_count
> +      if { $hwatch_count == $NR_THREADS } {
> +	set done 1
> +	break
> +      }
>       }
>       -re ".*Could not insert hardware watchpoint.*$gdb_prompt $" {
>         set done 1
>

If s390 doesn't run out of hardware watchpoints, i'd add a comment to 
the testcase explaining why we have this condition.

Luis

Re: [PATCH] wp-replication: Fix test case loop

[Andreas Arnez]

Luis Machado <lgustavo@codesourcery.com> writes:

> On 07/05/2013 01:10 PM, Andreas Arnez wrote:
>> When executing wp-replication.exp on S/390, the loop that counts the
>> number of available hardware watchpoints does not terminate properly,
>> because *all* "watch" commands yield "hardware watchpoints".
>>
>> Since the C source code is not prepared for more than NR_THREADS
>> hardware watchpoints anyhow, I suggest to add the appropriate exit
>> condition to the loop.
>
> That's an interesting behavior. Are the resources on s390 nearly
> unlimited for hardware watchpoints? Does it run out of hardware
> watchpoints eventually?

Well, that would be nice, wouldn't it?  But sorry, no.  On s390 the
hardware can generate so-called "PER storage alteration events" (PER =
program event recording) for a single contiguous memory area.
s390-nat.c chooses this area such that all desired watchpoints lie
within.

Thus what happens in wp-replication.exp is that the PER ending address
is increased with every loop iteration, but no hardware limit is
reached.

> If s390 doesn't run out of hardware watchpoints, i'd add a comment to
> the testcase explaining why we have this condition.

Sure.  How about this then?


diff --git a/gdb/testsuite/gdb.threads/wp-replication.exp b/gdb/testsuite/gdb.threads/wp-replication.exp
index 8927a43..c6a13b9 100644
--- a/gdb/testsuite/gdb.threads/wp-replication.exp
+++ b/gdb/testsuite/gdb.threads/wp-replication.exp
@@ -81,6 +81,14 @@ while { $done == 0 } {
   gdb_test_multiple "continue" "watchpoint created successfully" {
     -re ".*Breakpoint 2, empty_cycle \\(\\).*$gdb_prompt $" {
       incr hwatch_count
+
+      # Some targets (like S/390) behave as though supporting
+      # unlimited hardware watchpoints.  In this case we just take a
+      # safe exit out of the loop.
+      if { $hwatch_count == $NR_THREADS } {
+	set done 1
+	break
+      }
     }
     -re ".*Could not insert hardware watchpoint.*$gdb_prompt $" {
       set done 1

Re: [PATCH] wp-replication: Fix test case loop

[Luis Machado]

On 07/05/2013 02:09 PM, Andreas Arnez wrote:
> Luis Machado <lgustavo@codesourcery.com> writes:
>
>> On 07/05/2013 01:10 PM, Andreas Arnez wrote:
>>> When executing wp-replication.exp on S/390, the loop that counts the
>>> number of available hardware watchpoints does not terminate properly,
>>> because *all* "watch" commands yield "hardware watchpoints".
>>>
>>> Since the C source code is not prepared for more than NR_THREADS
>>> hardware watchpoints anyhow, I suggest to add the appropriate exit
>>> condition to the loop.
>>
>> That's an interesting behavior. Are the resources on s390 nearly
>> unlimited for hardware watchpoints? Does it run out of hardware
>> watchpoints eventually?
>
> Well, that would be nice, wouldn't it?  But sorry, no.  On s390 the
> hardware can generate so-called "PER storage alteration events" (PER =
> program event recording) for a single contiguous memory area.
> s390-nat.c chooses this area such that all desired watchpoints lie
> within.
>
> Thus what happens in wp-replication.exp is that the PER ending address
> is increased with every loop iteration, but no hardware limit is
> reached.
>

I see now. Thanks.

What do you think of forcing the testcase to use non-contiguous areas 
for the watchpoints instead of using a testcase that does not work/test 
properly for s390?

What if we add watchpoints every x bytes or so? Would that be enough to 
make s390 give up on using a single hardware watchpoint for a big range 
of contiguous addresses?

>> If s390 doesn't run out of hardware watchpoints, i'd add a comment to
>> the testcase explaining why we have this condition.
>
> Sure.  How about this then?
>
>
> diff --git a/gdb/testsuite/gdb.threads/wp-replication.exp b/gdb/testsuite/gdb.threads/wp-replication.exp
> index 8927a43..c6a13b9 100644
> --- a/gdb/testsuite/gdb.threads/wp-replication.exp
> +++ b/gdb/testsuite/gdb.threads/wp-replication.exp
> @@ -81,6 +81,14 @@ while { $done == 0 } {
>     gdb_test_multiple "continue" "watchpoint created successfully" {
>       -re ".*Breakpoint 2, empty_cycle \\(\\).*$gdb_prompt $" {
>         incr hwatch_count
> +
> +      # Some targets (like S/390) behave as though supporting
> +      # unlimited hardware watchpoints.  In this case we just take a
> +      # safe exit out of the loop.
> +      if { $hwatch_count == $NR_THREADS } {
> +	set done 1
> +	break
> +      }
>       }
>       -re ".*Could not insert hardware watchpoint.*$gdb_prompt $" {
>         set done 1
>

This looks good for me if the above is too tricky to implement.

Thanks,
Luis

Re: [PATCH] wp-replication: Fix test case loop

[Andreas Arnez]

Luis Machado <lgustavo@codesourcery.com> writes:

> What do you think of forcing the testcase to use non-contiguous areas
> for the watchpoints instead of using a testcase that does not
> work/test properly for s390?
>
> What if we add watchpoints every x bytes or so? Would that be enough
> to make s390 give up on using a single hardware watchpoint for a big
> range of contiguous addresses?

No, the current code never gives up.

> This looks good for me if the above is too tricky to implement.

OK, thanks.

Uli, would you commit this then?

Re: [PATCH] wp-replication: Fix test case loop

[Ulrich Weigand]

Andreas Arnez wrote:
> Uli, would you commit this then?

I've checked this in now.

Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com