Re: [PATCH] Make language setting tests more robust

Luis Machado
2017-02-01T19:22:36Z

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.