* [testsuite patch] Fix paginate-*.exp race for "read1"
@ 2014-07-22 18:38 Jan Kratochvil
2014-07-22 18:55 ` Jan Kratochvil
2014-07-24 0:50 ` [testsuite " Pedro Alves
0 siblings, 2 replies; 7+ messages in thread
From: Jan Kratochvil @ 2014-07-22 18:38 UTC (permalink / raw)
To: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 771 bytes --]
Hi Pedro,
these testcase 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
reproducible with "read1" from:
reproducer for races of expect incomplete reads
http://sourceware.org/bugzilla/show_bug.cgi?id=12649
# Prevent gdb_test_multiple considering an error -re "<return>" match.
# For unknown reason -notransfer -re "<return>" { exp_continue } does not
# prevent it.
Tested on Fedora 20 x86_64 and Fedora Rawhide x86_64 that -notransfer does not
work there:
expect-5.45-10.fc20.x86_64
expect-5.45-16.fc21.x86_64
Sure if someone gets -notransfer working this patch could be dropped.
Jan
[-- Attachment #2: 1 --]
[-- Type: text/plain, Size: 11032 bytes --]
gdb/testsuite/
2014-07-22 Jan Kratochvil <jan.kratochvil@redhat.com>
* gdb.base/double-prompt-target-event-error.exp: Use
gdb_test_pagination.
* gdb.base/paginate-after-ctrl-c-running.exp: Likewise.
* gdb.base/paginate-bg-execution.exp: Likewise.
* gdb.base/paginate-execution-startup.exp: Likewise.
* gdb.base/paginate-inferior-exit.exp: Likewise.
* lib/gdb.exp (pagination_prompt): Remove.
(gdb_test_pagination): New.
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..803e256 100644
--- a/gdb/testsuite/gdb.base/double-prompt-target-event-error.exp
+++ b/gdb/testsuite/gdb.base/double-prompt-target-event-error.exp
@@ -28,7 +28,7 @@ if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug] == -1} {
proc cancel_pagination_in_target_event { command } {
global binfile srcfile
- global gdb_prompt pagination_prompt
+ global gdb_prompt
set testline [gdb_get_line_number "after sleep"]
@@ -60,25 +60,23 @@ proc cancel_pagination_in_target_event { command } {
# Wait for pagination prompt after the "Continuing" line,
# indicating the program was running and then stopped.
+ 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
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
@@ -88,15 +86,20 @@ proc cancel_pagination_in_target_event { command } {
send_gdb "\003p 1\n"
set test "no double prompt"
- gdb_test_multiple "" $test {
- -re "$gdb_prompt.*$gdb_prompt.*$gdb_prompt $" {
- fail $test
- }
- -re "$gdb_prompt .* = 1\r\n$gdb_prompt $" {
+ global saw_prompt
+ set saw_prompt 0
+ gdb_test_pagination "" $test {
+ exp_continue
+ } {
+ -re " = 1\r\n$gdb_prompt $" {
pass $test
}
- -notransfer -re "<return>" {
- # Otherwise gdb_test_multiple considers this an error.
+ -re "\r\n$gdb_prompt " {
+ global saw_prompt
+ if { $saw_prompt != 0 } {
+ fail $test
+ }
+ set saw_prompt 1
exp_continue
}
}
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..3323fe7 100644
--- a/gdb/testsuite/gdb.base/paginate-after-ctrl-c-running.exp
+++ b/gdb/testsuite/gdb.base/paginate-after-ctrl-c-running.exp
@@ -24,7 +24,6 @@ if {[build_executable "failed to prepare" $testfile $srcfile debug] == -1} {
proc test_ctrlc_while_target_running_paginates {} {
global binfile srcfile
- global gdb_prompt pagination_prompt
set testline [gdb_get_line_number "after sleep"]
@@ -61,19 +60,13 @@ proc test_ctrlc_while_target_running_paginates {} {
# the user can respond to the pagination query.
set test "got prompt"
set saw_pagination_prompt 0
- gdb_test_multiple "" $test {
- -re "$pagination_prompt$" {
- set saw_pagination_prompt 1
- send_gdb "\n"
- exp_continue
- }
+ gdb_test_pagination "" $test {
+ send_gdb "\n"
+ exp_continue
+ } {
-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..3ba46a3 100644
--- a/gdb/testsuite/gdb.base/paginate-bg-execution.exp
+++ b/gdb/testsuite/gdb.base/paginate-bg-execution.exp
@@ -27,7 +27,6 @@ if {[build_executable "failed to prepare" $testfile $srcfile debug] == -1} {
proc test_bg_execution_pagination_return {} {
global binfile
- global pagination_prompt
with_test_prefix "paginate" {
clean_restart $binfile
@@ -43,21 +42,12 @@ proc test_bg_execution_pagination_return {} {
gdb_test "continue&" "Continuing\."
- set test "pagination handled, breakpoint hit"
- set saw_pagination_prompt 0
- gdb_test_multiple "" $test {
- -re "$pagination_prompt$" {
- set saw_pagination_prompt 1
- send_gdb "\n"
- exp_continue
- }
- -notransfer -re "<return>" {
- # Otherwise gdb_test_multiple considers this an
- # error.
- exp_continue
- }
+ gdb_test_pagination "" "pagination handled, breakpoint hit" {
+ send_gdb "\n"
+ exp_continue
+ } {
-re "after sleep\[^\r\n\]+\r\n$" {
- gdb_assert $saw_pagination_prompt $test
+ gdb_assert { $saw_pagination_prompt == 3 } $test
}
}
@@ -75,7 +65,7 @@ proc test_bg_execution_pagination_return {} {
proc test_bg_execution_pagination_cancel { how } {
global binfile
- global gdb_prompt pagination_prompt
+ global gdb_prompt
with_test_prefix "cancel with $how" {
clean_restart $binfile
@@ -92,14 +82,8 @@ proc test_bg_execution_pagination_cancel { how } {
gdb_test "continue&" "Continuing\."
set test "continue& paginates"
- gdb_test_multiple "" $test {
- -re "$pagination_prompt$" {
- pass $test
- }
- -notransfer -re "<return>" {
- # Otherwise gdb_test_multiple considers this an error.
- exp_continue
- }
+ gdb_test_pagination "" $test {
+ pass $test
}
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..4dc2376 100644
--- a/gdb/testsuite/gdb.base/paginate-execution-startup.exp
+++ b/gdb/testsuite/gdb.base/paginate-execution-startup.exp
@@ -76,7 +76,6 @@ proc probe_can_run_cmdline {} {
proc test_fg_execution_pagination_return {} {
global file_arg
global saved_gdbflags GDBFLAGS
- global gdb_prompt pagination_prompt
set GDBFLAGS $saved_gdbflags
append GDBFLAGS " -ex \"set height 2\""
@@ -92,10 +91,9 @@ proc test_fg_execution_pagination_return {} {
fail $test
return $res
}
- gdb_test_multiple "" $test {
- -re "$pagination_prompt$" {
- pass $test
- }
+ gdb_test_pagination "" $test {
+ pass $test
+ } {
-re "$gdb_prompt $" {
fail $test
}
@@ -103,20 +101,13 @@ proc test_fg_execution_pagination_return {} {
send_gdb "\n"
- set saw_pagination_prompt 0
set test "send \\n to GDB"
- gdb_test_multiple "" $test {
- -re "$pagination_prompt$" {
- set saw_pagination_prompt 1
- send_gdb "\n"
- exp_continue
- }
- -notransfer -re "<return>" {
- # Otherwise gdb_test_multiple considers this an error.
- exp_continue
- }
+ gdb_test_pagination "" $test {
+ send_gdb "\n"
+ exp_continue
+ } {
-re "$gdb_prompt $" {
- gdb_assert $saw_pagination_prompt $test
+ gdb_assert { $saw_pagination_prompt == 3 } $test
}
}
@@ -133,7 +124,7 @@ proc test_fg_execution_pagination_return {} {
proc test_fg_execution_pagination_cancel { how } {
global file_arg
global saved_gdbflags GDBFLAGS
- global gdb_prompt pagination_prompt
+ global gdb_prompt
set GDBFLAGS $saved_gdbflags
@@ -150,14 +141,8 @@ proc test_fg_execution_pagination_cancel { how } {
fail $test
return $res
}
- gdb_test_multiple "" $test {
- -re "$pagination_prompt$" {
- pass $test
- }
- -notransfer -re "<return>" {
- # Otherwise gdb_test_multiple considers this an error.
- exp_continue
- }
+ gdb_test_pagination "" $test {
+ pass $test
}
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..d43a245 100644
--- a/gdb/testsuite/gdb.base/paginate-inferior-exit.exp
+++ b/gdb/testsuite/gdb.base/paginate-inferior-exit.exp
@@ -26,7 +26,7 @@ if {[build_executable "failed to prepare" $testfile $srcfile debug] == -1} {
#
proc test_paginate_inferior_exited {} {
global binfile
- global gdb_prompt pagination_prompt
+ global gdb_prompt
global inferior_exited_re
with_test_prefix "paginate" {
@@ -45,23 +45,18 @@ proc test_paginate_inferior_exited {} {
# Wait for the "Starting program" line, indicating the program
# is running.
set saw_starting 0
- gdb_test_multiple "continue" $test {
- -re "$pagination_prompt" {
- if {$saw_starting} {
- pass $test
- } else {
- send_gdb "\n"
- exp_continue
- }
+ gdb_test_pagination "continue" $test {
+ if {$saw_starting} {
+ pass $test
+ } else {
+ send_gdb "\n"
+ exp_continue
}
+ } {
-re "Continuing" {
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.exp b/gdb/testsuite/lib/gdb.exp
index 7a00efb..d953a50 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -70,9 +70,6 @@ if ![info exists gdb_prompt] then {
set gdb_prompt "\[(\]gdb\[)\]"
}
-# A regexp that matches the pagination prompt.
-set pagination_prompt "---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/
set fullname_syntax_POSIX {/[^\n]*/}
@@ -4846,5 +4843,46 @@ proc capture_command_output { command prefix } {
return $output_string
}
+# 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---"
+
+ append code_append {
+ -re "${pagination_prompt1}" {
+ if { $saw_pagination_prompt != 0 && $saw_pagination_prompt != 3 } {
+ fail "$test (1)"
+ }
+ set saw_pagination_prompt 1
+ exp_continue
+ }
+ -re "${pagination_prompt2}" {
+ if { $saw_pagination_prompt != 1 } {
+ fail "$test (2)"
+ }
+ set saw_pagination_prompt 2
+ exp_continue
+ }
+ -re "${pagination_prompt3}$" {
+ if { $saw_pagination_prompt != 2 } {
+ fail "$test (3)"
+ }
+ set saw_pagination_prompt 3
+ eval $code_prompt3
+ }
+ }
+
+ set saw_pagination_prompt 0
+ gdb_test_multiple $command $test $code_append
+}
+
# Always load compatibility stuff.
load_lib future.exp
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [testsuite patch] Fix paginate-*.exp race for "read1"
2014-07-22 18:38 [testsuite patch] Fix paginate-*.exp race for "read1" Jan Kratochvil
@ 2014-07-22 18:55 ` Jan Kratochvil
2014-07-24 20:54 ` Jan Kratochvil
2014-07-24 0:50 ` [testsuite " Pedro Alves
1 sibling, 1 reply; 7+ messages in thread
From: Jan Kratochvil @ 2014-07-22 18:55 UTC (permalink / raw)
To: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 1448 bytes --]
Hi,
even with this "read1"-fix patch it is not fully reliable.
But this time I do not have a reliable reproducer, it just FAILs during fully
parallel Fedora GDB build; but running the Fedora GDB test by hand PASSes.
+FAIL: gdb.base/paginate-bg-execution.exp: cancel with ctrl-c: cancel pagination (timeout)
+FAIL: gdb.base/paginate-execution-startup.exp: cancel with ctrl-c: cancel pagination (timeout)
I have tried the attached patch but it has no effect.
continue&^M
Continuing.^M
(gdb) PASS: gdb.base/paginate-bg-execution.exp: cancel with ctrl-c: continue&
---Type <return> to continue, or q <return> to quit---PASS: gdb.base/paginate-bg-execution.exp: cancel with ctrl-c: continue& paginates
FAIL: gdb.base/paginate-bg-execution.exp: cancel with ctrl-c: cancel pagination (timeout)
^CQuit^M
(gdb) p 1^M
$1 = 1^M
(gdb) PASS: gdb.base/paginate-bg-execution.exp: cancel with ctrl-c: GDB accepts further input
Reading symbols from /unsafebuild-x86_64-redhat-linux-gnu/gdb/testsuite.unix.-m64/outputs/gdb.base/paginate-execution-startup/paginate-execution-startup...done.^M
---Type <return> to continue, or q <return> to quit---PASS: gdb.base/paginate-execution-startup.exp: cancel with ctrl-c: run to pagination
FAIL: gdb.base/paginate-execution-startup.exp: cancel with ctrl-c: cancel pagination (timeout)
^CQuit^M
(gdb) p 1^M
$1 = 1^M
(gdb) PASS: gdb.base/paginate-execution-startup.exp: cancel with ctrl-c: GDB accepts further input
Jan
[-- Attachment #2: gdb-testsuite-pagination-read1-003.patch --]
[-- Type: text/plain, Size: 926 bytes --]
diff --git a/gdb/testsuite/gdb.base/paginate-bg-execution.exp b/gdb/testsuite/gdb.base/paginate-bg-execution.exp
index 3ba46a3..e1f908b 100644
--- a/gdb/testsuite/gdb.base/paginate-bg-execution.exp
+++ b/gdb/testsuite/gdb.base/paginate-bg-execution.exp
@@ -88,6 +88,7 @@ proc test_bg_execution_pagination_cancel { how } {
set test "cancel pagination"
if { $how == "ctrl-c" } {
+ sleep 30
send_gdb "\003"
} else {
send_gdb "q\n"
diff --git a/gdb/testsuite/gdb.base/paginate-execution-startup.exp b/gdb/testsuite/gdb.base/paginate-execution-startup.exp
index 4dc2376..22958ab 100644
--- a/gdb/testsuite/gdb.base/paginate-execution-startup.exp
+++ b/gdb/testsuite/gdb.base/paginate-execution-startup.exp
@@ -147,6 +147,7 @@ proc test_fg_execution_pagination_cancel { how } {
set test "cancel pagination"
if { $how == "ctrl-c" } {
+ sleep 30
send_gdb "\003"
} else {
send_gdb "q\n"
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [testsuite patch] Fix paginate-*.exp race for "read1"
2014-07-22 18:38 [testsuite patch] Fix paginate-*.exp race for "read1" Jan Kratochvil
2014-07-22 18:55 ` Jan Kratochvil
@ 2014-07-24 0:50 ` Pedro Alves
2014-07-25 20:42 ` Pedro Alves
1 sibling, 1 reply; 7+ messages in thread
From: Pedro Alves @ 2014-07-24 0:50 UTC (permalink / raw)
To: Jan Kratochvil, gdb-patches
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [testsuite patch] Fix paginate-*.exp race for "read1"
2014-07-22 18:55 ` Jan Kratochvil
@ 2014-07-24 20:54 ` Jan Kratochvil
2014-07-25 9:50 ` Pedro Alves
0 siblings, 1 reply; 7+ messages in thread
From: Jan Kratochvil @ 2014-07-24 20:54 UTC (permalink / raw)
To: gdb-patches
On Tue, 22 Jul 2014 20:38:16 +0200, Jan Kratochvil wrote:
> even with this "read1"-fix patch it is not fully reliable.
> But this time I do not have a reliable reproducer, it just FAILs during fully
> parallel Fedora GDB build; but running the Fedora GDB test by hand PASSes.
>
> +FAIL: gdb.base/paginate-bg-execution.exp: cancel with ctrl-c: cancel pagination (timeout)
> +FAIL: gdb.base/paginate-execution-startup.exp: cancel with ctrl-c: cancel pagination (timeout)
These FAILs were due to
readline-6.3-2.fc21.x86_64
and they PASS now with
readline-6.3-3.fc22.x86_64
Some discussion about the update was here but I did not investigate it more:
readline-6.3 is available
https://bugzilla.redhat.com/show_bug.cgi?id=1071336
It was behaving the same also with your patch.
Jan
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [testsuite patch] Fix paginate-*.exp race for "read1"
2014-07-24 20:54 ` Jan Kratochvil
@ 2014-07-25 9:50 ` Pedro Alves
2014-07-25 10:14 ` [pushed 7.8][testsuite " Pedro Alves
0 siblings, 1 reply; 7+ messages in thread
From: Pedro Alves @ 2014-07-25 9:50 UTC (permalink / raw)
To: Jan Kratochvil, gdb-patches
On 07/24/2014 09:34 PM, Jan Kratochvil wrote:
> On Tue, 22 Jul 2014 20:38:16 +0200, Jan Kratochvil wrote:
>> even with this "read1"-fix patch it is not fully reliable.
>> But this time I do not have a reliable reproducer, it just FAILs during fully
>> parallel Fedora GDB build; but running the Fedora GDB test by hand PASSes.
>>
>> +FAIL: gdb.base/paginate-bg-execution.exp: cancel with ctrl-c: cancel pagination (timeout)
>> +FAIL: gdb.base/paginate-execution-startup.exp: cancel with ctrl-c: cancel pagination (timeout)
>
> These FAILs were due to
> readline-6.3-2.fc21.x86_64
> and they PASS now with
> readline-6.3-3.fc22.x86_64
> Some discussion about the update was here but I did not investigate it more:
> readline-6.3 is available
> https://bugzilla.redhat.com/show_bug.cgi?id=1071336
Ah, nice find.
> It was behaving the same also with your patch.
OK, I've pushed it in then, as below (added a link to
this discussion).
-------
Fix paginate-*.exp races
Jan pointed out in
<https://sourceware.org/ml/gdb-patches/2014-07/msg00553.html> that
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-25 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/testsuite/ChangeLog | 28 ++++++++++++++++++++++
.../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 +++--------
8 files changed, 56 insertions(+), 46 deletions(-)
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index f534f18..bd8cf44 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,31 @@
+2014-07-25 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.
+
2014-07-22 Jan Kratochvil <jan.kratochvil@redhat.com>
* gdb.arch/amd64-entry-value-paramref.S: New file.
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [pushed 7.8][testsuite patch] Fix paginate-*.exp race for "read1"
2014-07-25 9:50 ` Pedro Alves
@ 2014-07-25 10:14 ` Pedro Alves
0 siblings, 0 replies; 7+ messages in thread
From: Pedro Alves @ 2014-07-25 10:14 UTC (permalink / raw)
To: gdb-patches; +Cc: Jan Kratochvil
On 07/25/2014 10:46 AM, Pedro Alves wrote:
> On 07/24/2014 09:34 PM, Jan Kratochvil wrote:
>> On Tue, 22 Jul 2014 20:38:16 +0200, Jan Kratochvil wrote:
>>> even with this "read1"-fix patch it is not fully reliable.
>>> But this time I do not have a reliable reproducer, it just FAILs during fully
>>> parallel Fedora GDB build; but running the Fedora GDB test by hand PASSes.
>>>
>>> +FAIL: gdb.base/paginate-bg-execution.exp: cancel with ctrl-c: cancel pagination (timeout)
>>> +FAIL: gdb.base/paginate-execution-startup.exp: cancel with ctrl-c: cancel pagination (timeout)
>>
>> These FAILs were due to
>> readline-6.3-2.fc21.x86_64
>> and they PASS now with
>> readline-6.3-3.fc22.x86_64
>> Some discussion about the update was here but I did not investigate it more:
>> readline-6.3 is available
>> https://bugzilla.redhat.com/show_bug.cgi?id=1071336
>
> Ah, nice find.
>
>> It was behaving the same also with your patch.
>
> OK, I've pushed it in then, as below (added a link to
> this discussion).
Here's what I pushed to the 7.8 branch. It's the same,
except lib/gdb-utils.exp didn't exist in the branch yet.
Thanks,
Pedro Alves
--------------
Fix paginate-*.exp races
Jan pointed out in
<https://sourceware.org/ml/gdb-patches/2014-07/msg00553.html> that
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 7.8: lib/gdb-utils.exp didn't exist in the branch yet, so this patch adds it.]
gdb/testsuite/
2014-07-25 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: New file.
* lib/gdb.exp: Load gdb-utils.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/testsuite/ChangeLog | 28 ++++++++++++++++++++++
.../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 | 25 +++++++++++++++++++
gdb/testsuite/lib/gdb.exp | 15 ++++--------
8 files changed, 73 insertions(+), 46 deletions(-)
create mode 100644 gdb/testsuite/lib/gdb-utils.exp
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 291c81c..300ef64 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,31 @@
+2014-07-25 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.
+
2014-07-22 Jan Kratochvil <jan.kratochvil@redhat.com>
* gdb.arch/amd64-entry-value-paramref.S: New file.
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
new file mode 100644
index 0000000..b9968d0
--- /dev/null
+++ b/gdb/testsuite/lib/gdb-utils.exp
@@ -0,0 +1,25 @@
+# Copyright 2014 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+# Utility procedures, shared between test suite domains.
+
+# 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 3533ee3..7650d2a 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -27,6 +27,7 @@ if {$tool == ""} {
load_lib libgloss.exp
load_lib cache.exp
+load_lib gdb-utils.exp
global GDB
@@ -70,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/
@@ -648,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
@@ -872,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"
@@ -1108,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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [testsuite patch] Fix paginate-*.exp race for "read1"
2014-07-24 0:50 ` [testsuite " Pedro Alves
@ 2014-07-25 20:42 ` Pedro Alves
0 siblings, 0 replies; 7+ messages in thread
From: Pedro Alves @ 2014-07-25 20:42 UTC (permalink / raw)
To: Jan Kratochvil; +Cc: gdb-patches
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-07-25 18:49 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-07-22 18:38 [testsuite patch] Fix paginate-*.exp race for "read1" Jan Kratochvil
2014-07-22 18:55 ` Jan Kratochvil
2014-07-24 20:54 ` Jan Kratochvil
2014-07-25 9:50 ` Pedro Alves
2014-07-25 10:14 ` [pushed 7.8][testsuite " Pedro Alves
2014-07-24 0:50 ` [testsuite " Pedro Alves
2014-07-25 20:42 ` Pedro Alves
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox