Re: [PATCH] Make language setting tests more robust

[Luis Machado <lgustavo@codesourcery.com>]

On 02/01/2017 12:38 PM, Simon Marchi wrote:
> 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.
>

Oops. I was chasing some small issues with the set_language proc and 
ended up forgetting the cleanup these files.

Thanks for catching that.

>> +# 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 :).
>

I'm fine with that. I was mostly making it work like gdb_test_no_output.

I'll get this addressed in the next iteration.

>> +    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?
>

The way the code works at the moment will give a PASS if we set the 
language and then verify it has been set correctly. If it is not set 
correctly, we get a FAIL.

>> +
>> +    # 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 .*"'.
>

Yeah, that sounds reasonable. I'll address this.

>> +    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.
>

I considered just leaving it there, but with just the copyright. 
Deleting may be cleaner though. What others think?

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

Thanks for the review.