RE: [PATCH 04/18] gdb/testsuite: add local variable for passing 'getting_compiler_info' to gdb_compile

["Kempke, Nils-Christian via Gdb-patches" <gdb-patches@sourceware.org>]

> -----Original Message-----
> From: Andrew Burgess <aburgess@redhat.com>
> Sent: Wednesday, May 11, 2022 12:11 PM
> To: Kempke, Nils-Christian <nils-christian.kempke@intel.com>; gdb-
> patches@sourceware.org
> Cc: JiniSusan.George@amd.com
> Subject: Re: [PATCH 04/18] gdb/testsuite: add local variable for passing
> 'getting_compiler_info' to gdb_compile
> 
> 
> I think the subject would be better as 'move local variable ....etc...'

Yes, for sure. It is now

'gdb/testsuite: move getting_compiler_info to front of gdb_compile'

> 
> Nils-Christian Kempke via Gdb-patches <gdb-patches@sourceware.org>
> writes:
> 
> > The procedure gdb_compile used to query its options via
> >
> >    [lsearch -exact $options getting_compiler_info]
> >
> > to check whether or not it was called in with the option
> > getting_compiler_info.  If it was called with this option it would
> > preprocess some test input to try and figure out the actual compiler
> > version of the compiler used.  While doing this we cannot again try
> > figure out the current compiler version via the 'getting_compiler_info'
> > options as this would cause infinite recursion.  As some parts of the
> > procedure do recursively test for the compiler version to e.g. set
> > certain flags, at several places gdb_compile queried its options for
> > this setting.
> >
> > In the procedure, there was already a variable 'getting_compiler_info'.
> > It was set to the result of the above lsearch, but too late within
> > the code.  This lead to a mixture of querying 'getting_compiler_info' or
> > doing an lserach on the options passed to the procedure.
> >
> > I found this inconsistent and instead moved the variable
> getting_compiler_info
> > to the front of the procedure.
> 
> Am I correct that this is just a clean up?  And doesn't fix an actual
> bug?  That's not a problem, and I think this is a good cleanup, I just
> wasn't 100% sure after reading the above.

Yes, also correct. I'll change the commit message a bit to make this more clear.

> >                                  It is set to 0 or 1 depending on whether or
> > not the argument is found in the procedure's options (just as before)
> > and queried instead of doing an lsearch on the procedure options in the
> > rest of the procedure.
> 
> You can actually use 'true' and 'false' in tcl, and just treat the
> variable as a boolean, so....
>
> > ---
> >  gdb/testsuite/lib/gdb.exp | 18 ++++++++++++------
> >  1 file changed, 12 insertions(+), 6 deletions(-)
> >
> > diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> > index a6f2d847a5..639a0cd1be 100644
> > --- a/gdb/testsuite/lib/gdb.exp
> > +++ b/gdb/testsuite/lib/gdb.exp
> > @@ -4378,6 +4378,13 @@ proc gdb_compile {source dest type options} {
> >
> >      set outdir [file dirname $dest]
> >
> > +    # If this is set, calling test_compiler_info will cause recursion.
> > +    if { [lsearch -exact $options getting_compiler_info] == -1 } {
> > +	set getting_compiler_info 0
> 
>    set getting_compiler_info false
> 
> > +    } else {
> > +	set getting_compiler_info 1
> 
>    set getting_compiler_info true
> 
> > +    }
> > +
> >      # Add platform-specific options if a shared library was specified using
> >      # "shlib=librarypath" in OPTIONS.
> >      set new_options {}
> > @@ -4394,7 +4401,7 @@ proc gdb_compile {source dest type options} {
> >      # default, unless you pass -Wno-unknown-warning-option as well.
> >      # We do that here, so that individual testcases don't have to
> >      # worry about it.
> > -    if {[lsearch -exact $options getting_compiler_info] == -1
> > +    if {$getting_compiler_info == 0
> 
>    if {!$getting_compiler_info
>        && .... etc ....
> 
> Which I think makes it clearer that this is a boolean flag.

Yes that is true. Thanks for pointing it out, I did not know this.
Done for v2.

> 
> 
> >  	&& [lsearch -exact $options rust] == -1
> >  	&& [lsearch -exact $options ada] == -1
> >  	&& [lsearch -exact $options f90] == -1
> > @@ -4405,7 +4412,7 @@ proc gdb_compile {source dest type options} {
> >
> >      # Treating .c input files as C++ is deprecated in Clang, so
> >      # explicitly force C++ language.
> > -    if { [lsearch -exact $options getting_compiler_info] == -1
> > +    if { $getting_compiler_info == 0
> >  	 && [lsearch -exact $options c++] != -1
> >  	 && [string match *.c $source] != 0 } {
> >
> > @@ -4426,7 +4433,7 @@ proc gdb_compile {source dest type options} {
> >      # Place (and look for) Fortran `.mod` files in the output
> >      # directory for this specific test.  For Intel compilers the -J
> >      # option is not supported so instead use the -module flag.
> > -    if { [lsearch -exact $options f90] != -1 } {
> > +    if { $getting_compiler_info == 0 && [lsearch -exact $options f90] != -1 } {
> >  	# Fortran compile.
> >  	set mod_path [standard_output_file ""]
> >  	if [test_compiler_info "gcc-*"] {
> > @@ -4439,7 +4446,6 @@ proc gdb_compile {source dest type options} {
> >
> >      set shlib_found 0
> >      set shlib_load 0
> > -    set getting_compiler_info 0
> >      foreach opt $options {
> >          if {[regexp {^shlib=(.*)} $opt dummy_var shlib_name]
> >  	    && $type == "executable"} {
> > @@ -4471,8 +4477,8 @@ proc gdb_compile {source dest type options} {
> >  	} elseif { $opt == "shlib_load" && $type == "executable" } {
> >  	    set shlib_load 1
> >  	} elseif { $opt == "getting_compiler_info" } {
> > -	    # If this is set, calling test_compiler_info will cause recursion.
> > -	    set getting_compiler_info 1
> > +	    # Ignore this setting here as it has been treated in the very
> > +	    # beginning of this procedure.  Do not append it to new_options.
> 
> I think this comment should be reworded as:
> 
> 	    # Ignore this setting here as it has been handled earlier in
>             # this procedure.  Do not append it to new_options as this
>             # will cause recursion.

Done for v2.

Thanks for reviewing!

Nils

> 
> Thanks,
> Andrew
> 
> 
> >          } elseif {[regexp "^text_segment=(.*)" $opt dummy_var addr]} {
> >              if { [linker_supports_Ttext_segment_flag] } {
> >                  # For GNU ld.
> > --
> > 2.25.1
> >
> > Intel Deutschland GmbH
> > Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
> > Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
> > Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva
> > Chairperson of the Supervisory Board: Nicole Lau
> > Registered Office: Munich
> > Commercial Register: Amtsgericht Muenchen HRB 186928

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928