From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out1.suse.de (smtp-out1.suse.de [IPv6:2001:67c:2178:6::1c]) by sourceware.org (Postfix) with ESMTPS id CDE4B3858C66 for ; Wed, 10 May 2023 07:22:19 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org CDE4B3858C66 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=suse.de Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out1.suse.de (Postfix) with ESMTPS id 0F7C221A14; Wed, 10 May 2023 07:22:19 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1683703339; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=VKxcbVDdOJNY55i8F9AUPOzRo64OBfX4MRCCVMWo0Sc=; b=IU90bqmZRYd37eie69R1ptWw1Eos9KQrAaOKsqY/UJP0l2qUTyBtm3Mchxd9TPpIN/9nz3 Z/NmduU7yw3udcDqYx2Vt56XCMnrPARxcRqnVuD8n3k1a+KAKQfH7/84JbZvZ83LiQuhYX ErX7OiQLJz4dD502KLhXQSJ4E2agYlo= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1683703339; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=VKxcbVDdOJNY55i8F9AUPOzRo64OBfX4MRCCVMWo0Sc=; b=reUf/dyFAjzYwghO7NQwlVMDgtT5uiJVFTCPZLhI3E4t2YXxqCnWKYBXcZ9Yzb1gqDaSyI cvsYAt27Q8a8CQDg== Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap2.suse-dmz.suse.de (Postfix) with ESMTPS id A6BF7138E5; Wed, 10 May 2023 07:22:18 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id DpFRJCpGW2RsRwAAMHmgww (envelope-from ); Wed, 10 May 2023 07:22:18 +0000 Message-ID: <298cc5cd-1427-c9b7-7e7e-1077a2b776c5@suse.de> Date: Wed, 10 May 2023 09:22:17 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.10.1 Subject: Re: [PATCH 4/5] gdb/testsuite: change newline patterns used in gdb_test Content-Language: en-US To: Andrew Burgess , gdb-patches@sourceware.org References: <464e64e3a3483c228f0a73c778bcaf79e4595abd.1680293848.git.aburgess@redhat.com> <7552d3ad-c148-f0ea-b219-dd2d9458de0c@suse.de> <871qk09l5j.fsf@redhat.com> <14333280-6e66-0cc5-7e34-46176588ee37@suse.de> <87ttwv7zq8.fsf@redhat.com> <87354a90g5.fsf@redhat.com> <87r0rp7ru7.fsf@redhat.com> From: Tom de Vries In-Reply-To: <87r0rp7ru7.fsf@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-14.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 5/9/23 11:54, Andrew Burgess wrote: > Andrew Burgess writes: > >> Tom de Vries writes: >> >>> On 5/2/23 13:13, Andrew Burgess wrote: >>>> Tom de Vries writes: >>>> >>>>> On 5/1/23 16:33, Andrew Burgess wrote: >>>>>> Tom de Vries writes: >>>>>> >>>>>>> On 3/31/23 22:20, Andrew Burgess via Gdb-patches wrote: >>>>>>>> This commit makes two changes to how we match newline characters in >>>>>>>> the gdb_test proc. >>>>>>> >>>>>>> Hi, >>>>>>> >>>>>>> the -wrap used in gdb_test_multiple is defined in terms of gdb_test >>>>>>> semantics, but it doesn't seem to have been updated to match the new >>>>>>> behaviour in gdb_test. >>>>>>> >>>>>>> I've filed a PR about this regression ( >>>>>>> https://sourceware.org/bugzilla/show_bug.cgi?id=30403 ). >>>>>> >>>>>> Sorry for any problems caused. I'm not working today, but if this has >>>>>> not been addressed, I'll look at this on Tuesday. >>>>> >>>>> AFAIU it's a silent regression, so there are no problems in term of >>>>> FAILs, it's just that more work is required. >>>>> >>>>> FWIW, I think the root cause for introducing this regression silently is >>>>> that we try to implement the same thing in two different locations, and >>>>> it's just easy for things to get out of sync. I recently fixed >>>>> something similar in commit 4fa173cfd79 ("[gdb/testsuite] Fix -wrap in >>>>> presence of -prompt in gdb_test_multiple"), that's why I noticed it. >>>> >>>> So I believe the patch below brings gdb_test_multiple with '-wrap' back >>>> into line with gdb_test. I also updated a couple of other places that >>>> used the same (old) gdb_test pattern. >>>> >>>> There were nowhere near as many regressions with this change as with >>>> gdb_test. Let me know what you think. >>>> >>> >>> I've applied the patch and tested it, and saw no regression. I've also >>> reviewed the patch and LGTM. >> >> Thanks Tom, >> >> I've gone ahead and pushed this patch -- fixing up the newline issues. >> >>> >>> However, I've now also realized that the ^ bit is missing, which was >>> also added in this patch series. >>> >>> In other words, say we have: >>> ... >>> gdb_test "print 1" "^.$decimal = 1" >>> ... >>> which passes fine. >>> >>> But then we want to annotate the PASS message with the captured >>> $decimal, and rewrite into: >>> ... >>> gdb_test_multiple "print 1" "" { >>> -re -wrap "^.($decimal) = 1" { >>> set var_nr $expect_output(1,string) >>> pass "$gdb_test_name (var_nr: $var_nr)" >>> } >>> } >>> ... >>> This FAILs because ^ at the start of the pattern is not handled the same >>> way by -wrap as by gdb_test. >> >> I'm working on an updated patch that addresses the '^' feature for >> gdb_test_multiple. I'm still testing this locally, and it will probably >> be next week now before I post this -- but I will get this done, watch >> this space :) > > Tom, > > Below is a patch that extends the '^' support to gdb_test_multiple (when > -wrap is used). > > Let me know what you think. > Hi Andrew, I've applied the patch and tested it. I ran into trouble in two recent (more recent than your patch) test-cases, gdb.tui/wrap-line.exp and gdb.base/wrap-line.exp, which both need updating. Otherwise LGTM. Reviewed-by: Tom de Vries Thanks, - Tom > Thanks, > Andrew > > --- > > commit 1355a1d7eca5c1dac1c74c98634389525c78d877 > Author: Andrew Burgess > Date: Tue May 9 10:28:42 2023 +0100 > > gdb/testsuite: extend special '^' handling to gdb_test_multiple > > The commit: > > commit 08ec06d6440745ef9204d39197aa1e732df41056 > Date: Wed Mar 29 10:41:07 2023 +0100 > > gdb/testsuite: special case '^' in gdb_test pattern > > Added some special handling of '^' to gdb_test -- a leading '^' will > cause the command regexp to automatically be included in the expected > output pattern. > > It was pointed out that the '-wrap' flag of gdb_test_multiple is > supposed to work in the same way as gdb_test, and that the recent > changes for '^' had not been replicated for gdb_test_multiple. This > patch addresses this issue. > > So, after this commit, the following two constructs should have the > same meaning: > > gdb_test "command" "^output" "test name" > > gdb_test_multiple "command" "test name" { > -re -wrap "^output" { > pass $gdb_test_name > } > } > > In both cases the '^' will case gdb.exp to inject a regexp that > matches 'command' after the '^' and before the 'output', this is in > addition to adding the $gdb_prompt pattern after 'output' in the > normal way. > > The special '^' handling is only applied when '-wrap' is used, as this > is the only mode that aims to mimic gdb_test. > > While working on this patch I realised that I could actually improve > the logic for the special '^' handling in the case where the expected > output pattern is empty. I replicated these updates for both gdb_test > and gdb_test_multiple in order to keep these two paths in sync. > > There were a small number of tests that needed adjustment after this > change, mostly just removing command regexps that are now added > automatically, but the gdb.base/settings.exp case was a little weird > as it turns out trying to match a single blank line is probably harder > now than it used to be -- still, I suspect this is a pretty rare case, > so I think the benefits (improved anchoring) outweigh this small > downside (IMHO). > > diff --git a/gdb/testsuite/gdb.base/bitshift.exp b/gdb/testsuite/gdb.base/bitshift.exp > index adc5996d736..5ea0cd870ed 100644 > --- a/gdb/testsuite/gdb.base/bitshift.exp > +++ b/gdb/testsuite/gdb.base/bitshift.exp > @@ -24,19 +24,17 @@ clean_restart > # expected error. If WARNING_OR_ERROR is empty, it is expected that > # GDB prints no text other than the print result. > proc test_shift {lang cmd result_re {warning_or_error ""}} { > - set cmd_re [string_to_regexp $cmd] > - > if {$lang == "go"} { > if {$warning_or_error != ""} { > set error_re "[string_to_regexp $warning_or_error]" > gdb_test_multiple $cmd "" { > - -re -wrap "^$cmd_re\r\n$error_re" { > + -re -wrap "^$error_re" { > pass $gdb_test_name > } > } > } else { > gdb_test_multiple $cmd "" { > - -re -wrap "^$cmd_re\r\n\\$$::decimal$result_re" { > + -re -wrap "^\\$$::decimal$result_re" { > pass $gdb_test_name > } > } > @@ -49,7 +47,7 @@ proc test_shift {lang cmd result_re {warning_or_error ""}} { > } > > gdb_test_multiple $cmd "" { > - -re -wrap "^$cmd_re\r\n$warning_re\\$$::decimal$result_re" { > + -re -wrap "^$warning_re\\$$::decimal$result_re" { > pass $gdb_test_name > } > } > diff --git a/gdb/testsuite/gdb.base/maint-print-frame-id.exp b/gdb/testsuite/gdb.base/maint-print-frame-id.exp > index 2ad9b6ddfd7..9e88f37205f 100644 > --- a/gdb/testsuite/gdb.base/maint-print-frame-id.exp > +++ b/gdb/testsuite/gdb.base/maint-print-frame-id.exp > @@ -33,10 +33,6 @@ proc get_frame_id { level } { > set id "**unknown**" > > gdb_test_multiple "maint print frame-id ${level}" "" { > - -re "^maint print frame-id\[^\r\n\]+\r\n" { > - exp_continue > - } > - > -wrap -re "^frame-id for frame #\[0-9\]+: (\[^\r\n\]+)" { > set id $expect_out(1,string) > pass $gdb_test_name > diff --git a/gdb/testsuite/gdb.base/settings.exp b/gdb/testsuite/gdb.base/settings.exp > index eb127d246d2..1d9ee64ab0d 100644 > --- a/gdb/testsuite/gdb.base/settings.exp > +++ b/gdb/testsuite/gdb.base/settings.exp > @@ -542,7 +542,7 @@ proc test-string {variant} { > if {$variant != "filename"} { > # This odd expected output here is because we expect GDB to > # emit a single blank line as a result of this command. > - gdb_test "$show_cmd" "^" "$show_cmd: show default" > + gdb_test -nonl "$show_cmd" "^\r\n" "$show_cmd: show default" > } else { > gdb_test "$show_cmd" "/foo/bar" "$show_cmd: show default" > } > @@ -574,7 +574,7 @@ proc test-string {variant} { > gdb_test_no_output "$set_cmd" > # This odd expected output here is because we expect GDB to > # emit a single blank line as a result of this command. > - gdb_test "$show_cmd" "^" "$show_cmd: empty second time" > + gdb_test -nonl "$show_cmd" "^\r\n" "$show_cmd: empty second time" > } > } > > diff --git a/gdb/testsuite/gdb.dwarf2/gdb-index-nodebug.exp b/gdb/testsuite/gdb.dwarf2/gdb-index-nodebug.exp > index 5aebd2a8606..be666cb9dfd 100644 > --- a/gdb/testsuite/gdb.dwarf2/gdb-index-nodebug.exp > +++ b/gdb/testsuite/gdb.dwarf2/gdb-index-nodebug.exp > @@ -49,7 +49,7 @@ gdb_test_multiple $cmd "try to save gdb index" { > -re -wrap $no_debug_re { > pass $gdb_test_name > } > - -re -wrap "^$cmd" { > + -re -wrap "^" { > pass $gdb_test_name > } > } > diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp > index 50c10333df1..aed29652b87 100644 > --- a/gdb/testsuite/lib/gdb.exp > +++ b/gdb/testsuite/lib/gdb.exp > @@ -980,6 +980,8 @@ proc fill_in_default_prompt {prompt_regexp with_anchor} { > # pass $gdb_test_name > # } > # } > +# The special handling of '^' that is available in gdb_test is also > +# supported in gdb_test_multiple when -wrap is used. > # > # In EXPECT_ARGUMENTS, a pattern flag -early can be used. It makes sure the > # pattern is inserted before any implicit pattern added by gdb_test_multiple. > @@ -1125,6 +1127,19 @@ proc gdb_test_multiple { command message args } { > set expecting_action 1 > if { $wrap_pattern } { > # Wrap subst_item as is done for the gdb_test PATTERN argument. > + if {[string range $subst_item 0 0] eq "^"} { > + if {$command ne ""} { > + set command_regex [string_to_regexp $command] > + set subst_item [string range $subst_item 1 end] > + if {[string length "$subst_item"] > 0} { > + # We have an output pattern (other than the '^'), > + # add a newline at the start, this will eventually > + # sit between the command and the output pattern. > + set subst_item "\r\n${subst_item}" > + } > + set subst_item "^${command_regex}${subst_item}" > + } > + } > lappend $current_list \ > "(?:$subst_item)\r\n$prompt_regexp" > set wrap_pattern 0 > @@ -1465,10 +1480,16 @@ proc gdb_test { args } { > # additional pattern that matches the command immediately after > # the '^'. > if {[string range $pattern 0 0] eq "^"} { > - set command_regex [string_to_regexp $command] > - set pattern [string range $pattern 1 end] > - if {$command_regex ne ""} { > - set pattern "^${command_regex}\r\n$pattern" > + if {$command ne ""} { > + set command_regex [string_to_regexp $command] > + set pattern [string range $pattern 1 end] > + if {[string length "$pattern"] > 0} { > + # We have an output pattern (other than the '^'), add a > + # newline at the start, this will eventually sit between the > + # command and the output pattern. > + set pattern "\r\n$pattern" > + } > + set pattern "^${command_regex}${pattern}" > } > } > > @@ -6167,9 +6188,8 @@ proc with_set { var val body } { > perror "Did not manage to set $var" > } else { > # Set var. > - set cmd "set $var $val" > - gdb_test_multiple $cmd "" { > - -re -wrap "^$cmd" { > + gdb_test_multiple "set $var $val" "" { > + -re -wrap "^" { > } > -re -wrap " is set to \"?$val\"?\\." { > } > @@ -6180,9 +6200,8 @@ proc with_set { var val body } { > > # Restore saved setting. > if { $save != "" } { > - set cmd "set $var $save" > - gdb_test_multiple $cmd "" { > - -re -wrap "^$cmd" { > + gdb_test_multiple "set $var $save" "" { > + -re -wrap "^" { > } > -re -wrap "is set to \"?$save\"?( \\(\[^)\]*\\))?\\." { > } > @@ -7746,7 +7765,7 @@ proc get_valueof { fmt exp default {test ""} } { > > set val ${default} > gdb_test_multiple "print${fmt} ${exp}" "$test" { > - -re "\\$\[0-9\]* = (\[^\r\n\]*)\r\n$gdb_prompt $" { > + -re -wrap "^\\$\[0-9\]* = (\[^\r\n\]*)" { > set val $expect_out(1,string) > pass "$test" > } > @@ -7795,7 +7814,7 @@ proc get_integer_valueof { exp default {test ""} } { > > set val ${default} > gdb_test_multiple "print /d ${exp}" "$test" { > - -re "\\$\[0-9\]* = (\[-\]*\[0-9\]*).*$gdb_prompt $" { > + -re -wrap "^\\$\[0-9\]* = (\[-\]*\[0-9\]*).*" { > set val $expect_out(1,string) > pass "$test" > } >