From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id 56F8A3857827 for ; Tue, 11 Oct 2022 00:44:36 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 56F8A3857827 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 29B0iUIF021220 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 10 Oct 2022 20:44:35 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 29B0iUIF021220 Received: from [10.0.0.11] (unknown [217.28.27.60]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 727F31E0D5; Mon, 10 Oct 2022 20:44:30 -0400 (EDT) Message-ID: <24defaae-caaa-5432-1f52-20fe08140f30@polymtl.ca> Date: Mon, 10 Oct 2022 20:44:29 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.3.1 Subject: Re: [PATCH] gdb/testsuite: fix race in gdb.base/async-shell.exp Content-Language: en-US To: Tom de Vries , gdb-patches@sourceware.org References: <20221010012153.83170-1-simon.marchi@polymtl.ca> <33899f7a-f6b4-178d-813e-12ec0cf3065b@suse.de> From: Simon Marchi In-Reply-To: <33899f7a-f6b4-178d-813e-12ec0cf3065b@suse.de> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Tue, 11 Oct 2022 00:44:30 +0000 X-Spam-Status: No, score=-3038.0 required=5.0 tests=BAYES_00, BODY_8BITS, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 11 Oct 2022 00:44:50 -0000 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