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

[Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org>]

I think the subject would be better as 'move local variable ....etc...'

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.

>                                  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.


>  	&& [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.


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