http://sourceware.org/ml/gdb-patches/2014-07/msg00593.html Subject: Re: [testsuite patch] 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 "" { > - # 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 "" match. > +# For unknown reason -notransfer -re "" { 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 + set pagination_prompt2 "> to continue, or q + 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 Date: Wed, 23 Jul 2014 17:00:21 +0100 Subject: [PATCH] Fix paginate-*.exp races 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 "" { exp_continue }' trick in the current tests doesn't actually work. The issue that led to the -notransfer trick was that "---Type to continue, or q to quit---" has two ""s. If one wants gdb_test_multiple to not hit the built-in "" match that results in FAIL, one has to expect the pagination prompt in chunks, first up to the first "", then again, up to the second. Something around these lines: gdb_test_multiple "" $test { -re "" { 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 "" -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 "". 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 * gdb.base/double-prompt-target-event-error.exp (cancel_pagination_in_target_event): Remove '-notransfer ' 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 ' match. * gdb.base/paginate-bg-execution.exp (test_bg_execution_pagination_return) (test_bg_execution_pagination_cancel): Remove '-notransfer ' matches. * gdb.base/paginate-execution-startup.exp (test_fg_execution_pagination_return) (test_fg_execution_pagination_cancel): Remove '-notransfer ' matches. * gdb.base/paginate-inferior-exit.exp (test_paginate_inferior_exited): Remove '-notransfer ' 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 "". (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 "" { - # 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 "" { - # 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 "" { - # 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 "" { - # 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 "" { - # 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 "" { - # 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 "" { - # 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 "" { - # 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.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,15 @@ if ![info exists gdb_prompt] then { } # A regexp that matches the pagination prompt. +# 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 +} -set pagination_prompt "---Type to continue, or q to quit---" +set pagination_prompt [string_to_regexp "---Type to continue, or q 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 } - "" { + -re "$pagination_prompt" { send_gdb "\n" perror "Window too small." fail "$message" @@ -1109,14 +1109,6 @@ proc test_print_reject { args } { } } -# 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