[PATCH] gdb.base/commands.exp: fix racy test

[PATCH] gdb.base/commands.exp: fix racy test

[Marek Polacek]

Here is a first attempt to fix some racy tests.  The problem is with
the ".*" part which just slurps everything, this clearly shows when we
use preloaded read() which returns just one char at a time.  I've also
split gdb_test_multiple to avoid unnecessary exp_continue. Ok?

Signed-off-by: Marek Polacek <mpolacek@redhat.com>
---
 gdb/testsuite/ChangeLog             |    4 ++++
 gdb/testsuite/gdb.base/commands.exp |   12 ++++++++----
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 9ae251b..7874b8c 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,7 @@
+2011-04-13  Marek Polacek <mpolacek@redhat.com>
+
+	* gdb.base/commands.exp (redefine_backtrace_test): Fix race.
+
 2011-04-04  Tom Tromey  <tromey@redhat.com>
 
 	* gdb.cp/maint.exp (test_help): Update.
diff --git a/gdb/testsuite/gdb.base/commands.exp b/gdb/testsuite/gdb.base/commands.exp
index 3bff053..a0827c6 100644
--- a/gdb/testsuite/gdb.base/commands.exp
+++ b/gdb/testsuite/gdb.base/commands.exp
@@ -704,18 +704,22 @@ proc redefine_backtrace_test {} {
     global gdb_prompt
 
     gdb_test_multiple "define backtrace" "define backtrace" {
-	-re "Really redefine built-in.*$" {
+	-re "Really redefine built-in command \"backtrace\"\\? \\(y or n\\) $"  {
 	    send_gdb "y\n"
-	    exp_continue
 	}
+    }
 
-	-re "End with"  {
+    # We send nothing this time.
+    gdb_test_multiple "" "expect response to define backtrace" {
+	-re "End with a line saying just \"end\".\r\n>$"  {
 	    pass "define backtrace in redefine_backtrace_test"
 	}
-        default {
+
+	default {
 	    fail "(timeout or eof) define backtrace in redefine_backtrace_test"
 	}
     }
+
     gdb_test "echo hibob\\n\nend" \
 	    "" \
 	    "enter commands in redefine_backtrace_test"
-- 
1.7.3.4

Re: [PATCH] gdb.base/commands.exp: fix racy test

[Andreas Schwab]

Marek Polacek <mpolacek@redhat.com> writes:

>      gdb_test_multiple "define backtrace" "define backtrace" {
> -	-re "Really redefine built-in.*$" {
> +	-re "Really redefine built-in command \"backtrace\"\\? \\(y or n\\) $"  {

Alternatively, you could use "[^\n]*" in place of ".*".

Andreas.

-- 
Andreas Schwab, schwab@redhat.com
GPG Key fingerprint = D4E8 DBE3 3813 BB5D FA84  5EC7 45C6 250E 6F00 984E
"And now for something completely different."

Re: [PATCH] gdb.base/commands.exp: fix racy test (PR testsuite/12649)

[Jan Kratochvil]

Hi Marek,

On Wed, 13 Apr 2011 09:58:15 +0200, Marek Polacek wrote:
> The problem is with the ".*" part which just slurps everything,

in normal case, while with the following reproducer it does not slurp
everything which then falsely matches
proc gdb_test_multiple { command message user_code } {
        -re "\\((y or n|y or \\\[n\\\]|\\\[y\\\] or n)\\) " {
            fail "$message (got interactive prompt)"


> when we use preloaded read() which returns just one char at a time.

The problem reproducer in PR testsuite/12649 has no been mentioned here.


> --- a/gdb/testsuite/ChangeLog
> +++ b/gdb/testsuite/ChangeLog
> @@ -1,3 +1,7 @@
> +2011-04-13  Marek Polacek <mpolacek@redhat.com>
                           ^^^ two spaces here

The ChangeLog entry should be posted as plain text before the final commit, for
example now it already no longer applies to FSF GDB HEAD now.


> +	* gdb.base/commands.exp (redefine_backtrace_test): Fix race.

The description should be more detailed.  For example you do not mention you
have created a new testcase `expect response to define backtrace'.



> --- a/gdb/testsuite/gdb.base/commands.exp
> +++ b/gdb/testsuite/gdb.base/commands.exp
> @@ -704,18 +704,22 @@ proc redefine_backtrace_test {} {
>      global gdb_prompt
>  
>      gdb_test_multiple "define backtrace" "define backtrace" {
> -	-re "Really redefine built-in.*$" {
> +	-re "Really redefine built-in command \"backtrace\"\\? \\(y or n\\) $"  {
>  	    send_gdb "y\n"

Optional FYI: This send_gdb could be moved below into gdb_test_multiple:
    gdb_test_multiple "y" "expect response to define backtrace" {

But please do a PASS call here for better troubleshooting in the future:
	    pass "define backtrace"

> -	    exp_continue
>  	}
> +    }
>  
> -	-re "End with"  {
> +    # We send nothing this time.
> +    gdb_test_multiple "" "expect response to define backtrace" {
> +	-re "End with a line saying just \"end\".\r\n>$"  {

nitpick - `.' is a metacharacter here, it could be escaped:
   	-re "End with a line saying just \"end\"\\.\r\n>$"  {

>  	    pass "define backtrace in redefine_backtrace_test"

This test has name "expect response to define backtrace" which gets used
during various default FAIL cases by gdb_test_multiple.  The PASS name should
match the possible FAIL name, therefore rather some:

            pass "expect response to define backtrace"

>  	}
> -        default {
> +
> +	default {
>  	    fail "(timeout or eof) define backtrace in redefine_backtrace_test"
>  	}

This is redundant for gdb_test_multiple now but it is in fact unrelated
change.

I would prefer one re-post before the approval.


Thanks,
Jan

Re: [PATCH] gdb.base/commands.exp: fix racy test (PR testsuite/12649)

[Marek Polacek]

On 04/13/2011 11:20 AM, Jan Kratochvil wrote:
> I would prefer one re-post before the approval.

Sure!  Thanks for review.  Here's amended version (if there's still anything that
could be improved, just tell me):

2011-04-13  Marek Polacek  <mpolacek@redhat.com>

        * gdb.base/commands.exp (redefine_backtrace_test): Fix race. 
        New testcase `expect response to define backtrace'.  Also remove
        redundant `default' block.

Signed-off-by: Marek Polacek <mpolacek@redhat.com>
---
 gdb/testsuite/gdb.base/commands.exp |   15 +++++++--------
 1 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/gdb/testsuite/gdb.base/commands.exp b/gdb/testsuite/gdb.base/commands.exp
index 3bff053..a0ac25b 100644
--- a/gdb/testsuite/gdb.base/commands.exp
+++ b/gdb/testsuite/gdb.base/commands.exp
@@ -704,18 +704,17 @@ proc redefine_backtrace_test {} {
     global gdb_prompt
 
     gdb_test_multiple "define backtrace" "define backtrace" {
-	-re "Really redefine built-in.*$" {
-	    send_gdb "y\n"
-	    exp_continue
+	-re "Really redefine built-in command \"backtrace\"\\? \\(y or n\\) $"  {
+	    pass "define backtrace"
 	}
+    }
 
-	-re "End with"  {
-	    pass "define backtrace in redefine_backtrace_test"
-	}
-        default {
-	    fail "(timeout or eof) define backtrace in redefine_backtrace_test"
+    gdb_test_multiple "y" "expect response to define backtrace" {
+	-re "End with a line saying just \"end\"\\.\r\n>$"  {
+	    pass "expect response to define backtrace"
 	}
     }
+
     gdb_test "echo hibob\\n\nend" \
 	    "" \
 	    "enter commands in redefine_backtrace_test"
-- 
1.7.3.4

Re: [PATCH] gdb.base/commands.exp: fix racy test (PR testsuite/12649)

[Jan Kratochvil]

Hi Marek,

On Wed, 13 Apr 2011 17:36:25 +0200, Marek Polacek wrote:
> 2011-04-13  Marek Polacek  <mpolacek@redhat.com>
> 
>         * gdb.base/commands.exp (redefine_backtrace_test): Fix race. 
>         New testcase `expect response to define backtrace'.  Also remove
>         redundant `default' block.

checked in for you.  If you expect more contributions you should consider
write-after-approval sourceware account:
	http://sourceware.org/cgi-bin/pdw/ps_form.cgi


> Signed-off-by: Marek Polacek <mpolacek@redhat.com>

FYI this is redundant for GNU projects, FSF copyright assignment covered by
Red Hat in your case applies instead.


Thanks,
Jan


http://sourceware.org/ml/gdb-cvs/2011-04/msg00076.html

--- src/gdb/testsuite/ChangeLog	2011/04/11 17:40:41	1.2661
+++ src/gdb/testsuite/ChangeLog	2011/04/13 15:54:04	1.2662
@@ -1,3 +1,9 @@
+2011-04-13  Marek Polacek  <mpolacek@redhat.com>
+
+	* gdb.base/commands.exp (redefine_backtrace_test): Fix race. 
+	New testcase `expect response to define backtrace'.  Also remove
+	redundant `default' block.
+
 2011-04-11  Jan Kratochvil  <jan.kratochvil@redhat.com>
 
 	* gdb.python/py-prettyprint.c (struct hint_error): New.
--- src/gdb/testsuite/gdb.base/commands.exp	2011/01/01 15:33:41	1.42
+++ src/gdb/testsuite/gdb.base/commands.exp	2011/04/13 15:54:05	1.43
@@ -704,18 +704,17 @@
     global gdb_prompt
 
     gdb_test_multiple "define backtrace" "define backtrace" {
-	-re "Really redefine built-in.*$" {
-	    send_gdb "y\n"
-	    exp_continue
+	-re "Really redefine built-in command \"backtrace\"\\? \\(y or n\\) $"  {
+	    pass "define backtrace"
 	}
+    }
 
-	-re "End with"  {
-	    pass "define backtrace in redefine_backtrace_test"
-	}
-        default {
-	    fail "(timeout or eof) define backtrace in redefine_backtrace_test"
+    gdb_test_multiple "y" "expect response to define backtrace" {
+	-re "End with a line saying just \"end\"\\.\r\n>$"  {
+	    pass "expect response to define backtrace"
 	}
     }
+
     gdb_test "echo hibob\\n\nend" \
 	    "" \
 	    "enter commands in redefine_backtrace_test"

Re: [PATCH] gdb.base/commands.exp: fix racy test (PR testsuite/12649)

[Marek Polacek]

On 04/13/2011 05:55 PM, Jan Kratochvil wrote:
> checked in for you.  If you expect more contributions you should consider
> write-after-approval sourceware account:
> 	http://sourceware.org/cgi-bin/pdw/ps_form.cgi

Thanks.  Yes, I do.  I've already send a request.

	Marek

Re: [PATCH] gdb.base/commands.exp: fix racy test (PR testsuite/12649)

[Jan Kratochvil]

On Wed, 13 Apr 2011 18:10:52 +0200, Marek Polacek wrote:
> Thanks.  Yes, I do.  I've already send a request.

As you got it approved please add yourself into gdb/MAINTAINERS file section
`Write After Approval'.


Thanks,
Jan