From patchwork Thu Jul 24 00:40:14 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 2157 Received: (qmail 802 invoked by alias); 24 Jul 2014 00:40:21 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 792 invoked by uid 89); 24 Jul 2014 00:40:20 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL, BAYES_00, RP_MATCHES_RCVD, SPF_HELO_PASS, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Thu, 24 Jul 2014 00:40:18 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s6O0eHAA021217 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Wed, 23 Jul 2014 20:40:17 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s6O0eF0e001071; Wed, 23 Jul 2014 20:40:16 -0400 Message-ID: <53D055EE.40008@redhat.com> Date: Thu, 24 Jul 2014 01:40:14 +0100 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.6.0 MIME-Version: 1.0 To: Jan Kratochvil , gdb-patches@sourceware.org Subject: Re: [testsuite patch] Fix paginate-*.exp race for "read1" References: <20140722173635.GA9532@host2.jankratochvil.net> In-Reply-To: <20140722173635.GA9532@host2.jankratochvil.net> 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 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-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 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.