[RFC] gdb.cp/static-print-quit.exp: fix racy tests (PR testsuite/12649)

[RFC] gdb.cp/static-print-quit.exp: fix racy tests (PR testsuite/12649)

[Marek Polacek]

This test fails with Jan's read1() with:

ERROR: Window too small.
UNRESOLVED: gdb.cp/static-print-quit.exp: print c

This is because we expect to read:
"...Type <return> to continue, or q <return> to quit---$"

However, when using read1(), the "<return>" part matches with
this snippet from lib/gdb.exp:

         "<return>" {
            send_gdb "\n"
            perror "Window too small."
            fail "$message"
            ...

We can prevent this if we, in our gdb_test_multiple, create a "<return>"
case with `exp_continue'.  Nonetheless, except this, we must also in this
case:

    -re " to quit---$" {
        fail $test
        return -1
    }

differentiate between success and failure, since after processing the "<return>"
part we now have in the buffer " to quit---".  Thus, it is possible to do
something like this, using a new variable `seen_return', although I must say, I don't
like it at all.  But I don't see any other way how to cure this (at least yet) without
modifying the lib/gdb.exp.  Ran the test with both read{,1}.  Comments or complaints, please?

set test "print c"
set seen_return 0
gdb_test_multiple $test $test {
    "<return>" {
	incr seen_return
	exp_continue
    }
    -re "\\$\[0-9\]+ = \{loooooooooooooooooooooooooooooooooooooooooooooong = 0, static field = \{\r\n---Type " {
	incr seen_return
	exp_continue
    }
    -re " to continue, or q " {
	incr seen_return
	exp_continue
    }
    -re " to quit---$" {
        if { $seen_return >= 2 } {
	    pass $test
	} else { 
            fail $test
            return -1
	}
    }
}

Thanks,

	Marek

Re: [RFC] gdb.cp/static-print-quit.exp: fix racy tests (PR testsuite/12649)

[Marek Polacek]

I should have add a diff as well, here it is:

diff --git a/gdb/testsuite/gdb.cp/static-print-quit.exp b/gdb/testsuite/gdb.cp/static-print-quit.exp
index b6e34aa..4bede4e 100644
--- a/gdb/testsuite/gdb.cp/static-print-quit.exp
+++ b/gdb/testsuite/gdb.cp/static-print-quit.exp
@@ -31,13 +31,27 @@ gdb_test_no_output "set width 80"
 gdb_test_no_output "set height 2"
 
 set test "print c"
+set seen_return 0
 gdb_test_multiple $test $test {
-    -re " = \{loooooooooooooooooooooooooooooooooooooooooooooong = 0, static field = \{\r\n---Type <return> to continue, or q <return> to quit---$" {
-       pass $test
+    "<return>" {
+       incr seen_return
+       exp_continue
+    }
+    -re "\\$\[0-9\]+ = \{loooooooooooooooooooooooooooooooooooooooooooooong = 0, static field = \{\r\n---Type " {
+       incr seen_return
+       exp_continue
+    }
+    -re " to continue, or q " {
+       incr seen_return
+       exp_continue
     }
     -re " to quit---$" {
-       fail $test
-       return -1
+        if { $seen_return >= 2 } {
+           pass $test
+       } else { 
+            fail $test
+            return -1
+       }
     }
 }
 

Re: [RFC] gdb.cp/static-print-quit.exp: fix racy tests (PR testsuite/12649)

[Jan Kratochvil]

On Thu, 05 May 2011 20:03:32 +0200, Marek Polacek wrote:
[...]
> We can prevent this if we, in our gdb_test_multiple, create a "<return>"
> case with `exp_continue'.  Nonetheless, except this, we must also in this
> case:
> 
>     -re " to quit---$" {
>         fail $test
>         return -1
>     }

Found comments for the testcase were:
	[patch] Fix memory corruption on aborted object print
	http://sourceware.org/ml/gdb-patches/2010-06/msg00632.html
	47c8c764a9be6d023eca450336e6d9de16970fc0

The " to quit---$" case was there probably for the gdb-7.1 case which is
missing the "static field" output, put there untested now.  I understand this
is a change of the testcase.


> differentiate between success and failure, since after processing the "<return>"
> part we now have in the buffer " to quit---".  Thus, it is possible to do
> something like this, using a new variable `seen_return', although I must say, I don't
> like it at all.

exp_continue/variables make it complicated, I would prefer splitting it to
multiple consecutive test cases.


> set test "print c"
> set seen_return 0
> gdb_test_multiple $test $test {
>     "<return>" {
> 	incr seen_return
> 	exp_continue
>     }

BTW you do not need to match exactly "<return>" as is in lib/gdb.exp - it is
enough to match any string ending with "<return>".


On Thu, 05 May 2011 20:12:07 +0200, Marek Polacek wrote:
> I should have add a diff as well, here it is:
[...]
> -    -re " = \{loooooooooooooooooooooooooooooooooooooooooooooong = 0, static field = \{\r\n---Type <return> to continue, or q <return> to quit---$" {
> -       pass $test
> +    "<return>" {
> +       incr seen_return

FYI the diff could not be applied as it has <tab>s converted to spaces.

Any comments?  I will check it in otherwise.

Tested on x86_64-fedora15-linux-gnu.


Thanks,
Jan


gdb/testsuite/
2011-05-06  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Fix a race.
	* gdb.cp/static-print-quit.exp (print c): Split to ...
	(print c - <return>, print c - q <return>, print c - to quit):
	... these.  Make the testfile untested on gdb-7.1.

--- a/gdb/testsuite/gdb.cp/static-print-quit.exp
+++ b/gdb/testsuite/gdb.cp/static-print-quit.exp
@@ -30,14 +30,29 @@ clean_restart $executable
 gdb_test_no_output "set width 80"
 gdb_test_no_output "set height 2"
 
-set test "print c"
-gdb_test_multiple $test $test {
-    -re " = \{loooooooooooooooooooooooooooooooooooooooooooooong = 0, static field = \{\r\n---Type <return> to continue, or q <return> to quit---$" {
+set test "print c - <return>"
+gdb_test_multiple "print c" $test {
+    -re "\\$\[0-9\]+ = \{loooooooooooooooooooooooooooooooooooooooooooooong = 0, static field = \{\r\n---Type <return>" {
 	pass $test
     }
+    -re "\r\n---Type <return>" {
+	# gdb-7.1 did not crash with this testcase but it had the same bug.
+	untested ${testfile}.exp
+	return 0
+    }
+}
+
+set test "print c - q <return>"
+gdb_test_multiple "" $test {
+    -re " to continue, or q <return>" {
+	pass $test
+    }
+}
+
+set test "print c - to quit"
+gdb_test_multiple "" $test {
     -re " to quit---$" {
-	fail $test
-	return -1
+	pass $test
     }
 }
 

Re: [RFC] gdb.cp/static-print-quit.exp: fix racy tests (PR testsuite/12649)

[Marek Polacek]

On 05/06/2011 04:15 AM, Jan Kratochvil wrote:
> exp_continue/variables make it complicated, I would prefer splitting it to
> multiple consecutive test cases.

Yep, splitting the case was apparently the right thing to do.  What a shame
that this didn't occurred to me before.  Thanks.

> BTW you do not need to match exactly "<return>" as is in lib/gdb.exp - it is
> enough to match any string ending with "<return>".

OK, I wish I knew this before :-).

> FYI the diff could not be applied as it has <tab>s converted to spaces.

Right, sorry.

> Any comments?  I will check it in otherwise.

I tested your patch and all seems fine.  Maybe just, I think here:

> +set test "print c - q <return>"
> +gdb_test_multiple "" $test {
> +    -re " to continue, or q <return>" {

and here:

> +set test "print c - to quit"
> +gdb_test_multiple "" $test {
>      -re " to quit---$" {

the `-re' are in fact not needed since we don't use any regexps in the
string.  However, they certainly do no harm so no need to remove them.
Thanks again, Jan.

	Marek	

RE: [RFC] gdb.cp/static-print-quit.exp: fix racy tests (PR testsuite/12649)

[Pierre Muller]

> > +set test "print c - to quit"
> > +gdb_test_multiple "" $test {
> >      -re " to quit---$" {
> 
> the `-re' are in fact not needed since we don't use any regexps in the
> string.  However, they certainly do no harm so no need to remove them.
> Thanks again, Jan.

  Just a small question about this,
from the expect beginner that I still am:

isn't the dollar sign a regexpr pattern meaning end of string?
Would it really work without -re here?


Pierre Muller
GDB pascal language maintainer

Re: [RFC] gdb.cp/static-print-quit.exp: fix racy tests (PR testsuite/12649)

[Marek Polacek]

On 05/06/2011 01:08 PM, Pierre Muller wrote:
> isn't the dollar sign a regexpr pattern meaning end of string?

It is not, although I was confused for some time.  It basically signs the end
of the read buffer.  (IIUC.)

> Would it really work without -re here?

I tested it without those -re and it still works without any differences.

	Marek

Re: [RFC] gdb.cp/static-print-quit.exp: fix racy tests (PR testsuite/12649)

[Jan Kratochvil]

On Fri, 06 May 2011 10:58:21 +0200, Marek Polacek wrote:
> I tested your patch and all seems fine.

Thanks, checked in:
	http://sourceware.org/ml/gdb-cvs/2011-05/msg00039.html


> the `-re' are in fact not needed since we don't use any regexps in the
> string.  However, they certainly do no harm so no need to remove them.

OK, I did not know :-) but I find the standard -re way when no regexp escaping
is needed in these cases as fine, kept it without change.


Thanks,
Jan