Re: [testsuite patch] Fix paginate-*.exp race for "read1"

[Pedro Alves <palves@redhat.com>]

Hi Jan,

Thanks for noticing this.  It'd be very nice IMO to put that read1
trick in the sources somewhere, to make it easy (easier) to use.  Ideally
we'd have a simple Makefile flag to activate it, like 'make check READ1="1"'
or some such, but really just putting the files as attached to the PR, as
is, with absolutely no other glue at all, not even a Makefile, under
gdb/contrib/read1 or some such would already be great.  We can always
improve and integrate things more incrementally.  WDYT?

On 07/22/2014 06:36 PM, Jan Kratochvil wrote:

> +	global saw_continuing
>  	set saw_continuing 0
>  	set test "continue to pagination"
> -	gdb_test_multiple "$command" $test {
> -	    -re "$pagination_prompt$" {
> -		if {$saw_continuing} {
> -		    pass $test
> -		} else {
> -		    send_gdb "\n"
> -		    exp_continue
> -		}
> +	gdb_test_pagination $command $test {
> +	    global saw_continuing
> +	    if {$saw_continuing} {
> +		pass $test
> +	    } else {
> +		send_gdb "\n"
> +		exp_continue
>  	    }
> +	} {
>  	    -re "Continuing" {
> +		global saw_continuing
>  		set saw_continuing 1

The need for these "global"s indicates an issue
with uplevel/upvar in the new procedure.

>  		exp_continue
>  	    }
> -	    -notransfer -re "<return>" {
> -		# Otherwise gdb_test_multiple considers this an error.
> -		exp_continue
> -	    }
>  	}
>  
>  	# We're now stopped in a pagination query while handling a

> +proc gdb_test_pagination { command test { code_prompt3 {} } { code_append {} } } {
...
> +    append code_append {
...
> +	-re "${pagination_prompt3}$" {
> +	    if { $saw_pagination_prompt != 2 } {
> +		fail "$test (3)"
> +	    }
> +	    set saw_pagination_prompt 3
> +	    eval $code_prompt3

The issue is that $code_prompt3 and $code_append should really be
evaluated in this function's caller ...

> +	}
> +    }
> +
> +    set saw_pagination_prompt 0
> +    gdb_test_multiple $command $test $code_append

... but gdb_test_multiple evaluates the passed in $code_append
in the context of "uplevel 1" (and likewise it does a couple
upvar's with level one.  To make that work, you'd need to
rename gdb_test_multiple to gdb_test_multiple_with_level or
some such, add a 'level' parameter, and pass that as level to
the existing uplevel/upvar calls.  Then in gdb_test_pagination
you'd pass in "two levels up", like:

    gdb_test_multiple_with_level 2 $command $test $code_append

instead, and likewise gdb_test_multiple would be reimplemented in
terms of gdb_test_multiple_with_level.

But...  We don't really need ...

> +# Prevent gdb_test_multiple considering an error -re "<return>" match.
> +# For unknown reason -notransfer -re "<return>" { exp_continue } does not
> +# prevent it.
> +
> +proc gdb_test_pagination { command test { code_prompt3 {} } { code_append {} } } {
> +    global pagination_prompt1 pagination_prompt2 pagination_prompt3
> +    global gdb_prompt
> +
> +    # A regexp that matches the pagination prompt.
> +    set pagination_prompt1 "---Type <return"
> +    set pagination_prompt2 "> to continue, or q <return"
> +    set pagination_prompt3 "> to quit---"
> +

... this, if we instead tackle what IMO is the root of the
issue, and make gdb_test_multiple match the whole pagination
prompt, like in the patch below.  I should really have done
this in the first place.  :-/

This fixes the races for me, even when stressing them in
parallel mode, as mentioned in the commit log.
Does this fix them for you too?

----------
From 0c6260e734bdb28272119d50b0150fb777a458ab Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Wed, 23 Jul 2014 17:00:21 +0100
Subject: [PATCH] Fix paginate-*.exp races

These testcases have racy results:

  gdb.base/double-prompt-target-event-error.exp
  gdb.base/paginate-after-ctrl-c-running.exp
  gdb.base/paginate-bg-execution.exp
  gdb.base/paginate-execution-startup.exp
  gdb.base/paginate-inferior-exit.exp

This is easily reproducible with "read1" from:

  [reproducer for races of expect incomplete reads]
  http://sourceware.org/bugzilla/show_bug.cgi?id=12649

The '-notransfer -re "<return>" { exp_continue }' trick in the current
tests doesn't actually work.

The issue that led to the -notransfer trick was that

  "---Type <return> to continue, or q <return> to quit---"

has two "<return>"s.  If one wants gdb_test_multiple to not hit the
built-in "<return>" match that results in FAIL, one has to expect the
pagination prompt in chunks, first up to the first "<return>", then
again, up to the second.  Something around these lines:

  gdb_test_multiple "" $test {
      -re "<return>" {
	  exp_continue
      }
      -re "to quit ---" {
	  pass $test
      }
  }

The intent was for -notransfer+exp_continue to make expect fetch more
input, and rerun the matches against the now potentially fuller
buffer, and then eventually the -re that includes the full pagination
prompt regex would match instead (because it's listed higher up, it
would match first).  But, once that "<return>" -notransfer -re
matches, it keeps re-matching forever.  It seems like with
exp_continue, expect immediately retries matching, instead of first
reading in more data into the buffer, if available.

Fix this like I should have done in the first place.  There's actually
no good reason for gdb_test_multiple to only match "<return>".  We can
make gdb_test_multiple expect the whole pagination prompt text
instead, which is store in the 'pagination_prompt' global (similar to
'gdb_prompt').  Then a gdb_test_multiple caller that doesn't want the
default match to trigger, because it wants to see one pagination
prompt, does simply:

  gdb_test_multiple "" $test {
      -re "$pagination_prompt$" {
	  pass $test
      }
  }

which is just like when we don't want the default $gdb_prompt match
within gdb_test_multiple to trigger, like:

  gdb_test_multiple "" $test {
      -re "$gdb_prompt $" {
	  pass $test
      }
  }

Tested on x86_64 Fedora 20.  In addition, I've let the racy tests run
all in parallel in a loop for 30 minutes, and they never failed.

gdb/testsuite/
2014-07-24  Pedro Alves  <palves@redhat.com>

	* gdb.base/double-prompt-target-event-error.exp
	(cancel_pagination_in_target_event): Remove '-notransfer <return>'
	match.
	(cancel_pagination_in_target_event): Rework double prompt
	detection.
	* gdb.base/paginate-after-ctrl-c-running.exp
	(test_ctrlc_while_target_running_paginates): Remove '-notransfer
	<return>' match.
	* gdb.base/paginate-bg-execution.exp
	(test_bg_execution_pagination_return)
	(test_bg_execution_pagination_cancel): Remove '-notransfer
	<return>' matches.
	* gdb.base/paginate-execution-startup.exp
	(test_fg_execution_pagination_return)
	(test_fg_execution_pagination_cancel): Remove '-notransfer
	<return>' matches.
	* gdb.base/paginate-inferior-exit.exp
	(test_paginate_inferior_exited): Remove '-notransfer <return>'
	match.
	* lib/gdb-utils.exp (string_to_regexp): Move here from lib/gdb.exp.
	* lib/gdb.exp (pagination_prompt): Run text through
	string_to_regexp.
	(gdb_test_multiple): Match $pagination_prompt instead of
	"<return>".
	(string_to_regexp): Move to lib/gdb-utils.exp.
---
 .../gdb.base/double-prompt-target-event-error.exp  | 26 +++++++++++++---------
 .../gdb.base/paginate-after-ctrl-c-running.exp     |  4 ----
 gdb/testsuite/gdb.base/paginate-bg-execution.exp   |  9 --------
 .../gdb.base/paginate-execution-startup.exp        |  8 -------
 gdb/testsuite/gdb.base/paginate-inferior-exit.exp  |  4 ----
 gdb/testsuite/lib/gdb-utils.exp                    |  9 ++++++++
 gdb/testsuite/lib/gdb.exp                          | 14 +++---------
 7 files changed, 28 insertions(+), 46 deletions(-)

diff --git a/gdb/testsuite/gdb.base/double-prompt-target-event-error.exp b/gdb/testsuite/gdb.base/double-prompt-target-event-error.exp
index 5571cdf..84c6c45 100644
--- a/gdb/testsuite/gdb.base/double-prompt-target-event-error.exp
+++ b/gdb/testsuite/gdb.base/double-prompt-target-event-error.exp
@@ -75,10 +75,6 @@ proc cancel_pagination_in_target_event { command } {
 		set saw_continuing 1
 		exp_continue
 	    }
-	    -notransfer -re "<return>" {
-		# Otherwise gdb_test_multiple considers this an error.
-		exp_continue
-	    }
 	}
 
 	# We're now stopped in a pagination query while handling a
@@ -87,18 +83,28 @@ proc cancel_pagination_in_target_event { command } {
 	# output.
 	send_gdb "\003p 1\n"
 
+	# Note gdb_test_multiple has a default match for the prompt,
+	# which issues a FAIL.  Consume the first prompt.
+	set test "first prompt"
+	gdb_test_multiple "" $test {
+	    -re "$gdb_prompt" {
+		pass "first prompt"
+	    }
+	}
+
+	# We should only see one prompt more, and it should be
+	# preceeded by print's output.
 	set test "no double prompt"
 	gdb_test_multiple "" $test {
-	    -re "$gdb_prompt.*$gdb_prompt.*$gdb_prompt $" {
+	    -re "$gdb_prompt.*$gdb_prompt $" {
+		# The bug is present, and expect managed to read
+		# enough characters into the buffer to fill it with
+		# both prompts.
 		fail $test
 	    }
-	    -re "$gdb_prompt .* = 1\r\n$gdb_prompt $" {
+	    -re " = 1\r\n$gdb_prompt $" {
 		pass $test
 	    }
-	    -notransfer -re "<return>" {
-		# Otherwise gdb_test_multiple considers this an error.
-		exp_continue
-	    }
 	}
 
 	# In case the board file wants to send further commands.
diff --git a/gdb/testsuite/gdb.base/paginate-after-ctrl-c-running.exp b/gdb/testsuite/gdb.base/paginate-after-ctrl-c-running.exp
index 0ed8c92..5898d5b 100644
--- a/gdb/testsuite/gdb.base/paginate-after-ctrl-c-running.exp
+++ b/gdb/testsuite/gdb.base/paginate-after-ctrl-c-running.exp
@@ -70,10 +70,6 @@ proc test_ctrlc_while_target_running_paginates {} {
 	    -re "$gdb_prompt $" {
 		gdb_assert $saw_pagination_prompt $test
 	    }
-	    -notransfer -re "<return>" {
-		# Otherwise gdb_test_multiple considers this an error.
-		exp_continue
-	    }
 	}
 
 	# Confirm GDB can still process input.
diff --git a/gdb/testsuite/gdb.base/paginate-bg-execution.exp b/gdb/testsuite/gdb.base/paginate-bg-execution.exp
index dcff8ad..116cc2b 100644
--- a/gdb/testsuite/gdb.base/paginate-bg-execution.exp
+++ b/gdb/testsuite/gdb.base/paginate-bg-execution.exp
@@ -51,11 +51,6 @@ proc test_bg_execution_pagination_return {} {
 		send_gdb "\n"
 		exp_continue
 	    }
-	    -notransfer -re "<return>" {
-		# Otherwise gdb_test_multiple considers this an
-		# error.
-		exp_continue
-	    }
 	    -re "after sleep\[^\r\n\]+\r\n$" {
 		gdb_assert $saw_pagination_prompt $test
 	    }
@@ -96,10 +91,6 @@ proc test_bg_execution_pagination_cancel { how } {
 	    -re "$pagination_prompt$" {
 		pass $test
 	    }
-	    -notransfer -re "<return>" {
-		# Otherwise gdb_test_multiple considers this an error.
-		exp_continue
-	    }
 	}
 
 	set test "cancel pagination"
diff --git a/gdb/testsuite/gdb.base/paginate-execution-startup.exp b/gdb/testsuite/gdb.base/paginate-execution-startup.exp
index dc713ec..1628a0f 100644
--- a/gdb/testsuite/gdb.base/paginate-execution-startup.exp
+++ b/gdb/testsuite/gdb.base/paginate-execution-startup.exp
@@ -111,10 +111,6 @@ proc test_fg_execution_pagination_return {} {
 		send_gdb "\n"
 		exp_continue
 	    }
-	    -notransfer -re "<return>" {
-		# Otherwise gdb_test_multiple considers this an error.
-		exp_continue
-	    }
 	    -re "$gdb_prompt $" {
 		gdb_assert $saw_pagination_prompt $test
 	    }
@@ -154,10 +150,6 @@ proc test_fg_execution_pagination_cancel { how } {
 	    -re "$pagination_prompt$" {
 		pass $test
 	    }
-	    -notransfer -re "<return>" {
-		# Otherwise gdb_test_multiple considers this an error.
-		exp_continue
-	    }
 	}
 
 	set test "cancel pagination"
diff --git a/gdb/testsuite/gdb.base/paginate-inferior-exit.exp b/gdb/testsuite/gdb.base/paginate-inferior-exit.exp
index 0e37be9..7c63289 100644
--- a/gdb/testsuite/gdb.base/paginate-inferior-exit.exp
+++ b/gdb/testsuite/gdb.base/paginate-inferior-exit.exp
@@ -58,10 +58,6 @@ proc test_paginate_inferior_exited {} {
 		set saw_starting 1
 		exp_continue
 	    }
-	    -notransfer -re "<return>" {
-		# Otherwise gdb_test_multiple considers this an error.
-		exp_continue
-	    }
 	}
 
 	# We're now stopped in a pagination output while handling a
diff --git a/gdb/testsuite/lib/gdb-utils.exp b/gdb/testsuite/lib/gdb-utils.exp
index 7e03cbf..0af437e 100644
--- a/gdb/testsuite/lib/gdb-utils.exp
+++ b/gdb/testsuite/lib/gdb-utils.exp
@@ -28,3 +28,12 @@ proc gdb_init_commands {} {
     }
     return $commands
 }
+
+# Given an input string, adds backslashes as needed to create a
+# regexp that will match the string.
+
+proc string_to_regexp {str} {
+    set result $str
+    regsub -all {[]*+.|()^$\[\\]} $str {\\&} result
+    return $result
+}
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 7a00efb..8cb98ae 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -71,7 +71,7 @@ if ![info exists gdb_prompt] then {
 }
 
 # A regexp that matches the pagination prompt.
-set pagination_prompt "---Type <return> to continue, or q <return> to quit---"
+set pagination_prompt [string_to_regexp "---Type <return> to continue, or q <return> to quit---"]
 
 # The variable fullname_syntax_POSIX is a regexp which matches a POSIX 
 # absolute path ie. /foo/ 
@@ -649,7 +649,7 @@ proc gdb_internal_error_resync {} {
 #
 proc gdb_test_multiple { command message user_code } {
     global verbose use_gdb_stub
-    global gdb_prompt
+    global gdb_prompt pagination_prompt
     global GDB
     global inferior_exited_re
     upvar timeout timeout
@@ -873,7 +873,7 @@ proc gdb_test_multiple { command message user_code } {
 	    }
 	    set result 1
 	}
-	"<return>" {
+	-re "$pagination_prompt" {
 	    send_gdb "\n"
 	    perror "Window too small."
 	    fail "$message"
@@ -1109,14 +1109,6 @@ proc test_print_reject { args } {
     }
 }
 \f
-# Given an input string, adds backslashes as needed to create a
-# regexp that will match the string.
-
-proc string_to_regexp {str} {
-    set result $str
-    regsub -all {[]*+.|()^$\[\\]} $str {\\&} result
-    return $result
-}
 
 # Same as gdb_test, but the second parameter is not a regexp,
 # but a string that must match exactly.
-- 
1.9.3