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

Luis Machado
2017-02-06T14:54:15Z

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