Re: [PATCH,v2] Make language setting tests more robust

[Luis Machado <lgustavo@codesourcery.com>]

On 02/02/2017 06:36 PM, Pedro Alves wrote:
> On 02/01/2017 08:21 PM, Luis Machado wrote:
>
>> 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.
>
> Sorry to be a naysayer, but I'm not sure about this.  :-(  I recall fixing bugs
> related to gdb printing that warning when it really should not, or causing
> regressions locally due to breaking that logic, with the testsuite helping
> notice them.  I worry that this would be hiding such bugs going forward.
> Can you name the tests that where you saw the problem you mention?  Many of the tests
> you're touching are changing the language after starting gdb with no binary
> loaded, even.  Those definitely should not ever warn.  There are a few even that
> have "no prompt when changing language"-like messages.

Sounds reasonable. I think i was slightly surprised that the message is 
not coming from the command that sets the language 
(set_language_command), but from some frame manipulation function that 
gets called as part of determining the current language of the frame.

The story here is that i noticed 3 tests with such a problem:

FAIL: gdb.compile/compile-ifunc.exp: nodebug: set language c
FAIL: gdb.dwarf2/data-loc.exp: set language ada
FAIL: gdb.dwarf2/dynarr-ptr.exp: set language ada

And my build had a glibc with debugging symbols and sources, therefore 
GDB had set the initial language to asm before main.

I decided to expand the change to include an extra cleanup since i saw 
no clear purpose on having functions to set particular languages.

>
>> 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.
>
> One small advantage I see is that set_lang_<language> is a tcl symbol, so
> if you typo it, you'll get a tcl error.  Kind of like avoiding sprinkling
> magic constants in C.  You could keep the wrapper procs but define them
> in terms of set_language, like:
>
> proc set_lang_objc {} {
>    set_language "objective-c"
> }
>
> But I won't insist.
>

I wonder if the benefit from doing that justifies having extra code 
lying around. I can certainly do it if people think it is a good way 
forward. Our set of supported languages is not that big.

Thoughts anyone?

>> +# 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 be set.
>> +#
>> +# The second argument is an optional message to be output by the test.  If
>> +# the message is empty, the command to set the language will be used instead.
>> +
>> +proc set_language { language { message "" } } {
>> +    global gdb_prompt
>> +
>> +    set command "set language $language"
>> +    set lang_pattern [string_to_regexp $language]
>> +
>> +    if { $message == "" } {
>> +	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 $" {}
>> +    }
>
> Writing this as:
>
>     set command_re [string_to_regexp $command]
>
>     # Switch to the user-selected language.
>     gdb_test_multiple $command $message {
> 	-re "Warning: the current language does not match this frame.\[\r\n\]+$gdb_prompt $" {
> 	}
> 	-re "^$command_re\r\n$gdb_prompt $" {
>         }
> 	-re "$gdb_prompt" {
> 	  fail $message
> 	  return 0
> 	}
>     }
>
> Would be more robust in case the "Undefined item" output ever changes.

Indeed. I'll address this.

Thanks,
Luis