Re: [PATCH][gdb/testsuite] Introduce gdb_test_ext

[Andrew Burgess <andrew.burgess@embecosm.com>]

* Tom de Vries <tdevries@suse.de> [2019-09-19 21:01:01 +0200]:

> On 19-09-19 18:18, Andrew Burgess wrote:
> > * Tom de Vries <tdevries@suse.de> [2019-09-19 13:13:23 +0200]:
> > 
> >> Hi,
> >>
> >> In commit 25e5c20918 "[gdb/testsuite] Allow some tests in gdb.base/store.exp
> >> to be unsupported" we replace a gdb_test:
> >> ...
> >>     gdb_test "print l" " = ${l}" \
> >>        "${prefix}; print old l, expecting ${l}"
> >> ...
> >> with a gdb_test_multiple:
> >> ...
> >>     set supported 1
> >>     set test "${prefix}; print old l, expecting ${l}"
> >>     gdb_test_multiple "print l" "$test"  {
> >>        -re " = <optimized out>\r\n$gdb_prompt $" {
> >>            unsupported $test
> >>            set supported 0
> >>        }
> >>        -re " = ${l}\r\n$gdb_prompt $" {
> >>            pass $test
> >>        }
> >>     }
> >> ...
> >> in order to handle the UNSUPPORTED case.
> >>
> >> This has the drawback that we have to be explicit about the gdb_prompt, and
> >> move the gdb_test arguments around to fit the gdb_test_multiple format.
> >>
> >> Introduce a new proc gdb_test_ext that behaves as gdb_test, but also allows
> >> extension, allowing us to rewrite the gdb_test_multiple above in a form
> >> resembling the original gdb_test:
> >> ...
> >>     set supported 1
> >>     gdb_test_ext "print l" " = ${l}" \
> >>        "${prefix}; print old l, expecting ${l}" \
> >>        -- [list "unsupported" " = <optimized out>" "set supported 0"]
> > 
> > I've also thought about this sort of problem in the past, and would
> > like to propose a similar, but slightly different solution.
> > 
> > My idea is more like a trimmed down gdb_test_multiple, so for your
> > example above you would write this:
> > 
> >     gdb_test_ext "print l" "${prefix}; print old l, expecting ${l}" {
> > 	" = ${l}" {
> > 	    pass $gdb_test_ext_name
> > 	}
> > 	" = <optimized out>" {
> > 	    unsupported $gdb_test_ext_name
> > 	    set supported 0
> > 	}
> >     }
> > 
> > You don't put '-re' before each pattern, this is because they aren't
> > full patterns, gdb_test_ext will be extending them.
> > 
> > Unlike your solution the 'pass' case is not created automatically, you
> > need to write it yourself, so maybe that's a negative.  The advantages
> > I see of this approach is that there's not special handling for
> > different "types" of alternatives as in your original code, the action
> > block can contain anything 'unsupported', 'fail', etc.  Plus it's
> > formatted as a code body, which I like.
> > 
> 
> The solution as I proposed it doesn't limit itself to require each
> alternative to be handled as either supported or pass or fail or
> somesuch.  It just adds a means to extend gdb_test using a keyword that
> determines how the keyword arguments are handled.

I don't really understand what you're trying to say here, sorry.
Looking at the code it still appears that each case would need to be
added specifically, so if tomorrow I want a 'fail' case, I'd need to
add it to lib/gdb.exp - or am I not understanding?

> 
> So, I've added the style you propose here as "generic", and rewrote one
> of the two places I update in store.exp using the "generic" style for
> demonstration purposes.
> 
> I envision the usage like this: you'd usually use "unsupported" or
> similar to skip all the repetitive code and focus on the bits that
> actually contain content, and for special cases where that doesn't fit
> you'd use "generic". You can go further and add a "freeform" or some
> such where you'd have to write out the entire regexp.
> 
> The nice thing is that you can add keywords and corresponding handling
> as you go, whereas the gdb_test_multiple-like solution you propose only
> has one way of handling its arguments, which of course does makes things
> consistent and clear, but is not very extensible.

I'm still not seeing which arguments can only be handled in one way.
Could you give an example maybe?  I do see that you're suggestion
improves the existing test - removing the need to set an extra 'test'
variable, and not having to add $gdb_prompt, but when all is said and
done, the pattern is still just a pattern, and the code is still just
code - how is this any different to gdb_test_multiple?

> 
> > One other thing which I've wanted for _ages_ is to avoid having to set
> > the test name into a separate variable, which your solution offers
> > too.  The solution I offer is '$gdb_test_ext_name', this variable is
> > set auto-magically by the call to 'gdb_test_ext, and is available in
> > all of the action bodies for calls to pass/fail/unsupported/etc.
> > 
> 
> Nice trick, I've copied that for usage in the "generic" method.
> 
> So, WDYT?

I'm still not convinced.

On further thought, I actually think there's no need for an extra
function at all, we can get all the benefit (as I see it) by possibly
updating gdb_test_multiple.  I'm travelling right now so can't code
this up, but I think a solution that does something like this:

     gdb_test_multiple "command" "test name" {
       -re "full regexp here$gdb_prompt" {
         pass $gdb_test_multiple_name
       }
       -output "pattern without prompt" {
         fail $gdb_test_multiple_name
       }
     }

So using '-re' and '-output' to specialise the behaviour of
gdb_test_multiple, and adding in the $gdb_test_multiple_name variable.

When I get back to my desk I'll try to code this up.

I'm know the above isn't going to satisfy you though - it's basically
an iteration on what I already proposed - maybe you could expand on
the benefits of you solution a bit more.

Thanks,
Andrew



> 
> Thanks,
> - Tom
> 

> [gdb/testsuite] Introduce gdb_test_ext
> 
> In commit 25e5c20918 "[gdb/testsuite] Allow some tests in gdb.base/store.exp
> to be unsupported" we replace a gdb_test:
> ...
>     gdb_test "print l" " = ${l}" \
>        "${prefix}; print old l, expecting ${l}"
> ...
> with a gdb_test_multiple:
> ...
>     set supported 1
>     set test "${prefix}; print old l, expecting ${l}"
>     gdb_test_multiple "print l" "$test"  {
>        -re " = <optimized out>\r\n$gdb_prompt $" {
>            unsupported $test
>            set supported 0
>        }
>        -re " = ${l}\r\n$gdb_prompt $" {
>            pass $test
>        }
>     }
> ...
> in order to handle the UNSUPPORTED case.
> 
> This has the drawback that we have to be explicit about the gdb_prompt, and
> move the gdb_test arguments around to fit the gdb_test_multiple format.
> 
> Introduce a new proc gdb_test_ext that behaves as gdb_test, but also allows
> extension, allowing us to rewrite the gdb_test_multiple above in a form
> resembling the original gdb_test:
> ...
>     set supported 1
>     gdb_test_ext "print l" " = ${l}" \
>        "${prefix}; print old l, expecting ${l}" \
>        -- [list "unsupported" " = <optimized out>" "set supported 0"]
> ...
> 
> Tested on x86_64-linux.
> 
> gdb/testsuite/ChangeLog:
> 
> 2019-09-19  Tom de Vries  <tdevries@suse.de>
> 
> 	* lib/gdb.exp (gdb_test_ext): New proc.
> 	* gdb.base/store.exp: Use gdb_test_ext.
> 
> ---
>  gdb/testsuite/gdb.base/store.exp | 27 ++++--------
>  gdb/testsuite/lib/gdb.exp        | 95 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 103 insertions(+), 19 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.base/store.exp b/gdb/testsuite/gdb.base/store.exp
> index 9c19ce15a7..89a594f96a 100644
> --- a/gdb/testsuite/gdb.base/store.exp
> +++ b/gdb/testsuite/gdb.base/store.exp
> @@ -56,16 +56,8 @@ proc check_set { t l r new add } {
>      }
>  
>      set supported 1
> -    set test "${prefix}; print old l, expecting ${l}"
> -    gdb_test_multiple "print l" "$test"  {
> -	-re " = <optimized out>\r\n$gdb_prompt $" {
> -	    unsupported $test
> -	    set supported 0
> -	}
> -	-re " = ${l}\r\n$gdb_prompt $" {
> -	    pass $test
> -	}
> -    }
> +    gdb_test_ext "print l" " = ${l}" "${prefix}; print old l, expecting ${l}" \
> +	-- [list "unsupported" " = <optimized out>" "set supported 0"]
>      if { $supported } {
>  	gdb_test "print r" " = ${r}" \
>  	    "${prefix}; print old r, expecting ${r}"
> @@ -102,16 +94,13 @@ proc up_set { t l r new } {
>  	"${prefix}; up"
>  
>      set supported 1
> -    set test "${prefix}; print old l, expecting ${l}"
> -    gdb_test_multiple "print l" "$test"  {
> -	-re " = <optimized out>\r\n$gdb_prompt $" {
> -	    unsupported $test
> -	    set supported 0
> +    gdb_test_ext "print l" " = ${l}" "${prefix}; print old l, expecting ${l}" \
> +	-- {
> +	    "generic" " = <optimized out>" {
> +		set supported 0
> +		unsupported $gdb_test_ext_name
> +	    }
>  	}
> -	-re " = ${l}\r\n$gdb_prompt $" {
> -	    pass $test
> -	}
> -    }
>      if { $supported } {
>  	gdb_test "print r" " = ${r}" \
>  	    "${prefix}; print old r, expecting ${r}"
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index acbeb01376..d01ca25ef7 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -1103,6 +1103,101 @@ proc gdb_test { args } {
>       }]
>  }
>  
> +# As gdb_test, but with additional parameters, listed after a "--" separator.
> +# Handled extra parameters:
> +# - [list "unsupported" <pattern> [<code>]]
> +# The idea is to prevent the need to rewrite gdb_test into gdb_test_multiple
> +# if some modification is needed.
> +proc gdb_test_ext { args } {
> +    global gdb_prompt
> +    upvar timeout timeout
> +
> +    # Find the '--' separator.
> +    set pos -1
> +    set index 0
> +    while { $index < [llength $args] } {
> +	if { [lindex $args $index] == "--" } {
> +	    set pos $index
> +	    break
> +	}
> +	set index [expr $index + 1]
> +    }
> +    if { $pos == -1 } {
> +	error "No -- argument found"
> +    }
> +
> +    if { $pos > 2 } then {
> +	set message [lindex $args 2]
> +    } else {
> +	set message [lindex $args 0]
> +    }
> +    set command [lindex $args 0]
> +    set pattern [lindex $args 1]
> +
> +    set user_code {}
> +    lappend user_code {
> +	-re "\[\r\n\]*(?:$pattern)\[\r\n\]+$gdb_prompt $" {
> +	    if ![string match "" $message] then {
> +		pass "$message"
> +            }
> +        }
> +    }
> +
> +    if { $pos == 5 } {
> +	set question_string [lindex $args 3]
> +	set response_string [lindex $args 4]
> +	lappend user_code {
> +	    -re "(${question_string})$" {
> +		send_gdb "$response_string\n"
> +		exp_continue
> +	    }
> +	}
> +   }
> +
> +    set index [expr $pos + 1]
> +    while { $index < [llength $args] } {
> +	set arg [lindex $args $index]
> +	set index [expr $index + 1]
> +	set kind [lindex $arg 0]
> +	switch $kind {
> +	    "unsupported" {
> +		set unsupported_pattern [lindex $arg 1]
> +		set unsupported_code [lindex $arg 2]
> +		if { $unsupported_code == "" } {
> +		    set unsupported_code "expr true"
> +		}
> +		lappend user_code {
> +		    -re "\[\r\n\]*(?:$unsupported_pattern)\[\r\n\]+$gdb_prompt $" {
> +			unsupported $message
> +			uplevel $unsupported_code
> +		    }
> +		}
> +	    }
> +	    "generic" {
> +		# In order to support the gdb_test_ext_name variable we need to
> +		# push the variable into the parent scope.  Before we blindly do
> +		# that check the user hasn't already defined that variable.  If
> +		# they haven't, go ahead and create it for them.
> +		if { [uplevel { info exists gdb_test_ext_name }] } {
> +		    error "variable gdb_test_ext_name unexpectedly exists"
> +		    return -1
> +		}
> +		uplevel set gdb_test_ext_name \"$message\"
> +		set generic_pattern [lindex $arg 1]
> +		set generic_code [lindex $arg 2]
> +		lappend user_code {
> +		    -re "\[\r\n\]*(?:$generic_pattern)\[\r\n\]+$gdb_prompt $" {
> +			uplevel $generic_code
> +		    }
> +		}
> +	    }
> +	}
> +    }
> +
> +    set user_code [join $user_code " "]
> +    return [gdb_test_multiple $command $message $user_code]
> +}
> +
>  # Return 1 if version MAJOR.MINOR is at least AT_LEAST_MAJOR.AT_LEAST_MINOR.
>  proc version_at_least { major minor at_least_major at_least_minor} {
>      if { $major > $at_least_major } {