Re: [PATCH] gdb/testsuite: fix race in gdb.base/async-shell.exp

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2022-10-10 03:07, Tom de Vries wrote:
> On 10/10/22 03:21, Simon Marchi wrote:
>> I see some random failures in this test:
>>
>> It can be reliably reproduced (on Linux) with this change:
>>
>>      diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
>>      index 44cc28b30051..2a3c8253ba5a 100644
>>      --- a/gdb/testsuite/lib/gdb.exp
>>      +++ b/gdb/testsuite/lib/gdb.exp
>>      @@ -1301,6 +1301,7 @@ proc gdb_test_multiple { command message args } {
>>           }
>>           set gdb_test_name "$message"
>>
>>      +    sleep 2
>>           set result 0
>>           set code [catch {gdb_expect $code} string]
>>
> 
> I first tried reproducing on openSUSE Leap 15.4, and failed because
> that one has glibc 2.31, but managed on openSUSE Tumbleweed with glibc
> 2.36, which has the "pthread merged into glibc" feature that's been
> around since 2.34.

Ah, that must explain why this failure appeared after upgrading my test
machine from Ubuntu 20.04 to 22.04.  I'll add a precision about that in
the commit message.

>> The problem is that the `run &` command prints some things after the
>> prompt:
>>
>>      (gdb) [Thread debugging using libthread_db enabled]
>>      Using host libthread_db library "/usr/lib/../lib/libthread_db.so.1".
>>
>> If DejaGNU is quick enough, it will consume only up to the prompt.  But
> 
> FWIW, I would use expect instead of DejaGNU, because it's more specific.

Done.

>> if it is slow enough, it will consume those messages at the same time as
>> the prompt, in which case the gdb_test used for "run &" won't match.  By
>> default, the prompt used by gdb_test uses a `$` to anchor the match at
>> the end of the buffer.  If there's anything following the prompt, it
>> won't match.
>>
>> The diff above aadds a delay between sending the command and consuming
> 
> adds

Done.

> I've tested it and confirmed that it fixes the problem.
> 
> FWIW, I can also imagine we'll add this to gdb_test_multiple at some point, but that's certainly not necessary for now.

Ack.

> 
>> Change-Id: I9051d8800d1c10a2e95db1a575991f7723492f1b
>> ---
>>   gdb/testsuite/gdb.base/async-shell.exp |  2 +-
>>   gdb/testsuite/lib/gdb.exp              | 29 +++++++++++++++++++-------
>>   2 files changed, 23 insertions(+), 8 deletions(-)
>>
>> diff --git a/gdb/testsuite/gdb.base/async-shell.exp b/gdb/testsuite/gdb.base/async-shell.exp
>> index e2d71826bab2..2c09863ad593 100644
>> --- a/gdb/testsuite/gdb.base/async-shell.exp
>> +++ b/gdb/testsuite/gdb.base/async-shell.exp
>> @@ -34,7 +34,7 @@ save_vars { GDBFLAGS } {
>>     set gdbindex_warning_re "warning: Skipping \[^\r\n\]+ \\.gdb_index section \[^\r\n\]*\r\nDo \"set use-deprecated-index-sections on\" before the file is read\r\nto use the section anyway\\."
>>   -gdb_test "run &" "Starting program: \[^\r\n\]*(\r\n$gdbindex_warning_re)?"
>> +gdb_test -no-prompt-anchor "run &" "Starting program: \[^\r\n\]*(\r\n$gdbindex_warning_re)?"
>>     # `sleep 5' here would workaround the bug, do not sleep here.
>>   # "shell" could eat waitpid event from the asynchronous inferior process.
>> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
>> index 44cc28b30051..c8d2aa511fb8 100644
>> --- a/gdb/testsuite/lib/gdb.exp
>> +++ b/gdb/testsuite/lib/gdb.exp
>> @@ -861,9 +861,18 @@ proc gdb_internal_error_resync {} {
>>   }
>>     # Fill in the default prompt if PROMPT_REGEXP is empty.
>> -proc fill_in_default_prompt {prompt_regexp} {
>> +#
>> +# If WITH_ANCHOR is true and the default prompt is used, append a `$` at the end
>> +# of the regexp, to anchor the match at the end of the buffer.
>> +proc fill_in_default_prompt {prompt_regexp with_anchor} {
>>       if { "$prompt_regexp" == "" } {
>> -    return "$::gdb_prompt $"
>> +    set prompt "$::gdb_prompt "
>> +
>> +    if { $with_anchor } {
>> +        append prompt "$"
>> +    }
>> +
>> +    return $prompt
>>       }
>>       return $prompt_regexp
>>   }
>> @@ -993,7 +1002,7 @@ proc gdb_test_multiple { command message args } {
>>       error "Too few arguments to gdb_test_multiple"
>>       }
>>   -    set prompt_regexp [fill_in_default_prompt $prompt_regexp]
>> +    set prompt_regexp [fill_in_default_prompt $prompt_regexp true]
>>         if { $message == "" } {
>>       set message $command
>> @@ -1365,6 +1374,10 @@ proc gdb_test_multiline { name args } {
>>   #
>>   # -prompt PROMPT_REGEXP specifies a regexp matching the expected prompt
>>   #   after the command output.  If empty, defaults to "$gdb_prompt $".
>> +# -no-prompt-anchor specifies that if the default prompt regexp is used, it
>> +#   should not be anchored at the end of the buffer.  This means that the
>> +#   pattern can match even if there is stuff output after the prompt.  Does not
>> +#   have any effect if -prompt is specified.
>>   # -lbl specifies that line-by-line matching will be used.
>>   # -nopass specifies that a PASS should not be issued.
>>   #
>> @@ -1379,6 +1392,7 @@ proc gdb_test { args } {
>>         parse_args {
>>       {prompt ""}
>> +    {no-prompt-anchor}
>>       {lbl}
>>       {nopass}
>>       }
>> @@ -1394,7 +1408,7 @@ proc gdb_test { args } {
>>       set message $command
>>       }
>>   -    set prompt [fill_in_default_prompt $prompt]
>> +    set prompt [fill_in_default_prompt $prompt [expr !${no-prompt-anchor}]]
>>         set saw_question 0
>>   @@ -1471,20 +1485,21 @@ if { [tcl_version_at_least 8 5] == 0 } {
>>   # gdb_test_no_output [-prompt PROMPT_REGEXP] [-nopass] COMMAND [MESSAGE]
>>   # Send a command to GDB and verify that this command generated no output.
>>   #
>> -# See gdb_test for a description of the -prompt, -nopass, COMMAND, and
>> -# MESSAGE parameters.
>> +# See gdb_test for a description of the -prompt, -no-prompt-anchor, -nopass,
>> +# COMMAND, and MESSAGE parameters.
>>     proc gdb_test_no_output { args } {
>>       global gdb_prompt
>>         parse_args {
>>       {prompt ""}
>> +    {no-prompt-anchor}
>>       {nopass}
>>       }
>>         lassign $args command message
>>   -    set prompt [fill_in_default_prompt $prompt]
>> +    set prompt [fill_in_default_prompt $prompt [expr !${no-prompt-anchor}]]
>>   
> 
> FWIW, I've wondered whether giving fill_in_default_prompt a
> no-prompt-anchor parameter instead of with_anchor would be better,
> avoiding the inversion here and using consistent naming for the same
> concept across different procs, but I haven't been able to make up my
> mind about whether I really like it better ... I thought I just
> mention it.

That's what I did initially, but then it feels weird for the call sites
that do want the prompt, they have to pass false to mean "I want
no-no-prompt-anchor".  That makes a double negative, which I find
confusing.  I don't have a strong opinion otherwise.

> Anyway, LGTM.

Thanks for the review, will push (and add you Approved-By).

Simon