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

[Luis Machado <lgustavo at codesourcery dot 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