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

  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(-)
  

On 07/24/2014 01:40 AM, Pedro Alves wrote:

> 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?

I took a stab at this:

   https://sourceware.org/ml/gdb-patches/2014-07/msg00679.html

Thanks,
Pedro Alves