* [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: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-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-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