From: Andrew Burgess <aburgess@redhat.com>
To: Tom de Vries <tdevries@suse.de>, gdb-patches@sourceware.org
Subject: Re: [PATCH 4/5] gdb/testsuite: change newline patterns used in gdb_test
Date: Fri, 12 May 2023 13:54:12 +0100 [thread overview]
Message-ID: <87r0rl677v.fsf@redhat.com> (raw)
In-Reply-To: <298cc5cd-1427-c9b7-7e7e-1077a2b776c5@suse.de>
Tom de Vries <tdevries@suse.de> writes:
> On 5/9/23 11:54, Andrew Burgess wrote:
>> Andrew Burgess <aburgess@redhat.com> writes:
>>
>>> Tom de Vries <tdevries@suse.de> writes:
>>>
>>>> On 5/2/23 13:13, Andrew Burgess wrote:
>>>>> Tom de Vries <tdevries@suse.de> writes:
>>>>>
>>>>>> On 5/1/23 16:33, Andrew Burgess wrote:
>>>>>>> Tom de Vries <tdevries@suse.de> 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.
I made the obvious fixes for these two tests, and pushed this patch.
Thanks for the review.
Andrew
>
> Otherwise LGTM.
>
> Reviewed-by: Tom de Vries <tdevries@suse.de>
>
> Thanks,
> - Tom
>
>> Thanks,
>> Andrew
>>
>> ---
>>
>> commit 1355a1d7eca5c1dac1c74c98634389525c78d877
>> Author: Andrew Burgess <aburgess@redhat.com>
>> 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"
>> }
>>
next prev parent reply other threads:[~2023-05-12 12:54 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-31 20:20 [PATCH 0/5] gdb/testsuite: stricter matching for gdb_test Andrew Burgess
2023-03-31 20:20 ` [PATCH 1/5] gdb/testsuite: fix occasional failure in gdb.base/clear_non_user_bp.exp Andrew Burgess
2023-03-31 20:20 ` [PATCH 2/5] gdb: remove some trailing newlines from warning messages Andrew Burgess
2023-03-31 20:20 ` [PATCH 3/5] gdb/testsuite: use 'return' in gdb_test_no_output Andrew Burgess
2023-03-31 20:20 ` [PATCH 4/5] gdb/testsuite: change newline patterns used in gdb_test Andrew Burgess
2023-04-27 19:39 ` Simon Marchi
2023-04-28 14:05 ` Andrew Burgess
2023-04-28 15:51 ` Andrew Burgess
2023-04-28 15:57 ` Simon Marchi
2023-04-28 18:37 ` Simon Marchi
2023-04-28 21:50 ` Andrew Burgess
2023-05-02 19:16 ` Simon Marchi
2023-04-29 15:20 ` Tom de Vries
2023-05-01 14:33 ` Andrew Burgess
2023-05-01 15:10 ` Tom de Vries
2023-05-02 11:13 ` Andrew Burgess
2023-05-02 14:48 ` Tom de Vries
2023-05-05 17:01 ` Andrew Burgess
2023-05-09 9:54 ` Andrew Burgess
2023-05-10 7:22 ` Tom de Vries
2023-05-12 12:54 ` Andrew Burgess [this message]
2023-03-31 20:20 ` [PATCH 5/5] gdb/testsuite: special case '^' in gdb_test pattern Andrew Burgess
2023-04-17 16:12 ` [PATCH 0/5] gdb/testsuite: stricter matching for gdb_test Tom Tromey
2023-04-27 12:58 ` Andrew Burgess
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87r0rl677v.fsf@redhat.com \
--to=aburgess@redhat.com \
--cc=gdb-patches@sourceware.org \
--cc=tdevries@suse.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).