Re: [PATCH] Make language setting tests more robust

Simon Marchi
2017-02-01T18:38:29Z

On 2017-02-01 10:17, Luis Machado wrote:
> This patch robustifies the setting of the language in gdb. Right now
> most of
> the tests assume "set language" is a silent command and use
> gdb_test_no_output
> for testing, but it may output a warning stating the language the user
> wants
> to select does not match the frame's language. When this happens we
> have a
> failure. This is also the case with "show language", where it outputs
> the
> language currently-selected but may also output said warning.
>
> One case of the warning being displayed happens when one has debug
> information
> for glibc, which may cause GDB to identify the frame as having an "asm"
> language. Therefore setting it to something else will get GDB's
> attention.
>
> This patch addresses the problem by creating a function in lib/gdb.exp
> to
> set the language. That function will also handle potential warnings and
> check
> to make sure the language was properly selected.
>
> Also, i noticed most of the languages have their own
> set_lang_<language> proc,
> and they are all the same. Therefore i've removed those and switched
> to using
> only the new set_language proc.
>
> I tried to confirm why set_lang_<language> was replicated, but my
> conclusion
> was that it was just the way the code worked and people just
> copy/pasted from
> an existing language .exp file.
>
> Overall i see no regressions with Ubuntu 16.04 x86-64, though the
> number of
> tests changed slightly due to the way set_language works. No
> additional
> failures nonetheless.

I have just looked at the stuff in lib/, it looks good to me, that's a
nice cleanup. A few comments below.

> diff --git a/gdb/testsuite/lib/d-support.exp
> b/gdb/testsuite/lib/d-support.exp
> index ed568d3..100fb63 100644
> --- a/gdb/testsuite/lib/d-support.exp
> +++ b/gdb/testsuite/lib/d-support.exp
> @@ -16,17 +16,6 @@
> # Auxiliary function to set the language to D.
> # The result is 1 (true) for success, 0 (false) for failure.

Remove this comment.

> -proc set_lang_d {} {
> - if [gdb_test_no_output "set language d"] {
> - return 0
> - }
> - if [gdb_test "show language" ".* source language is \"d\"." \
> - "set language to \"d\""] {
> - return 0
> - }
> - return 1
> -}
> -
> # D version of runto_main.
>
> proc d_runto_main { } {
> diff --git a/gdb/testsuite/lib/fortran.exp
> b/gdb/testsuite/lib/fortran.exp
> index 867e4fc..2f4820b 100644
> --- a/gdb/testsuite/lib/fortran.exp
> +++ b/gdb/testsuite/lib/fortran.exp
> @@ -18,17 +18,6 @@
> # Auxiliary function to set the language to fortran.
> # The result is 1 (true) for success, 0 (false) for failure.

Comment.

> -proc set_lang_fortran {} {
> - if [gdb_test_no_output "set language fortran"] {
> - return 0
> - }
> - if [gdb_test "show language" ".* source language is \"fortran\"."
> \
> - "set language to \"fortran\""] {
> - return 0
> - }
> - return 1
> -}
> -
> proc fortran_int4 {} {
> if {[test_compiler_info {gcc-4-[012]-*}]} {
> return "int4"
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index 48bd7ee..327bce8 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -6018,6 +6018,47 @@ proc multi_line_input { args } {
> return [join $args "\n"]
> }
>
> +# Set the language and handle possible warnings output by GDB if we
> select a
> +# language that differs from the current frame's language.
> +#
> +# The first argument is the language to set.
> +# The second argument is an optional message to be output by the test.
> +
> +proc set_language { args } {

Instead of "args", it might be clearer to use a default argument for the
second arg:

proc set_language {Â language { message "" } } {

And lower:

if { $message == "" } {
set message $command
}

Not sure about the exact TCL syntax, but I think you get the point :).

> + global gdb_prompt
> +
> + set language [lindex $args 0]
> + set command "set language $language"
> + set lang_pattern [string_to_regexp $language]
> +
> + # Do we have an optional user-provided test message?
> + if {[llength $args] > 1} {
> + set message [lindex $args 1]
> + } else {
> + set message $command
> + }
> +
> + # Switch to the user-selected language.
> + gdb_test_multiple $command $message {
> + -re "Undefined item: \"$lang_pattern\"\.\[\r\n\]+$gdb_prompt" {
> + fail $message
> + return 0
> + }
> + -re "Warning: the current language does not match this
> frame.\[\r\n\]+$gdb_prompt $" {
> + }
> + -re "$gdb_prompt $" {}
> + }

Do you want to add a "pass" in there somewhere, or we keep it silent if
it succeeds?

> +
> + # Verify the language has been set correctly. GDB may output a
> warning
> + # stating the user-provided language doesn't match the frame's
> language.
> + # We ignore that warning for testing purposes.
> + if {$language != "auto"} {
> + if [gdb_test "show language" ".* source language is
> \"$lang_pattern\".*" $message] {
> + return 0
> + }
> + }

In the else, you could verify that it matches 'The current source
language is "auto; currently .*"'.

> + return 1
> +}
>
> # Always load compatibility stuff.
> load_lib future.exp
> diff --git a/gdb/testsuite/lib/go.exp b/gdb/testsuite/lib/go.exp
> index c29b7c1..cb98bdc 100644
> --- a/gdb/testsuite/lib/go.exp
> +++ b/gdb/testsuite/lib/go.exp
> @@ -19,17 +19,6 @@
> # Auxiliary function to set the language to Go.
> # The result is 1 (true) for success, 0 (false) for failure.

Comment.

> -proc set_lang_go {} {
> - if [gdb_test_no_output "set language go"] {
> - return 0
> - }
> - if [gdb_test "show language" ".* source language is \"go\"." \
> - "set language to \"go\""] {
> - return 0
> - }
> - return 1
> -}
> -
> # Go version of runto_main.
>
> proc go_runto_main { } {
> diff --git a/gdb/testsuite/lib/objc.exp b/gdb/testsuite/lib/objc.exp
> index 7189f03..9891758 100644
> --- a/gdb/testsuite/lib/objc.exp
> +++ b/gdb/testsuite/lib/objc.exp
> @@ -17,14 +17,3 @@
>
> # Auxiliary function to set the language to fortran.
> # The result is 1 (true) for success, 0 (false) for failure.

Comment. Also, note that it mentions fortran :). Actually, you could
probably just delete that file.

> -
> -proc set_lang_objc {} {
> - if [gdb_test_no_output "set language objective-c"] {
> - return 0
> - }
> - if [gdb_test "show language" ".* source language is
> \"objective-c\"." \
> - "set language to \"objective-c\""] {
> - return 0
> - }
> - return 1
> -}
> diff --git a/gdb/testsuite/lib/pascal.exp
> b/gdb/testsuite/lib/pascal.exp
> index a0562c3..f0eb36b 100644
> --- a/gdb/testsuite/lib/pascal.exp
> +++ b/gdb/testsuite/lib/pascal.exp
> @@ -167,17 +167,3 @@ proc gdb_compile_pascal {source destfile type
> options} {
> return "Pascal compilation failed."
> }
> }
> -
> -# Auxiliary function to set the language to pascal.
> -# The result is 1 (true) for success, 0 (false) for failure.
> -
> -proc set_lang_pascal {} {
> - if [gdb_test_no_output "set language pascal"] {
> - return 0
> - }
> - if [gdb_test "show language" ".* source language is \"pascal\"." \
> - "set language to \"pascal\""] {
> - return 0
> - }
> - return 1
> -}
> diff --git a/gdb/testsuite/lib/rust-support.exp
> b/gdb/testsuite/lib/rust-support.exp
> index 2a12cca..a80d60b 100644
> --- a/gdb/testsuite/lib/rust-support.exp
> +++ b/gdb/testsuite/lib/rust-support.exp
> @@ -15,16 +15,6 @@
>
> # Auxiliary function to set the language to Rust.
> # The result is 1 (true) for success, 0 (false) for failure.

Comment.

> -proc set_lang_rust {} {
> - if [gdb_test_no_output "set language rust"] {
> - return 0
> - }
> - if [gdb_test "show language" ".* source language is \"rust\"." \
> - "set language to \"rust\""] {
> - return 0
> - }
> - return 1
> -}
>
> proc gdb_compile_rust {sources dest options} {
> if {[llength $sources] > 1} {