Re: [PATCH 4/5] gdb/testsuite: change newline patterns used in gdb_test

[Tom de Vries <tdevries@suse.de>]

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.

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"
>   	}
>