Re: [PATCH 5/7] [gdb/testsuite] add lib/jit-elf-helpers.exp

[Simon Marchi <simark@simark.ca>]

On 2020-02-18 7:43 a.m., Mihails Strasuns wrote:
> New utility library to be used by jit-elf tests responsible for
> compiling binary artifacts. In the next commit the compilation process
> will become more complicated because of extra mandatory flag - keeping
> it in one place will make tests less fragile.
> 
> gdb/testsuite/ChangeLog:
> 
> 2020-02-18  Mihails Strasuns  <mihails.strasuns@intel.com>
> 
> 	* lib/jit-elf-helpers.exp: new file
> 	* gdb.base/jit-elf.exp: updated to use jit-elf-helpers.exp
> 	* gdb.base/jit-elf-so.exp: updated to use jit-elf-helpers.exp
> 
> Change-Id: I2fda518406aeca55e82df45edd67cef149497bbe
> Signed-off-by: Mihails Strasuns <mihails.strasuns@intel.com>
> ---
>  gdb/testsuite/gdb.base/jit-elf-so.exp | 69 +++++++------------------
>  gdb/testsuite/gdb.base/jit-elf.exp    | 39 +-------------
>  gdb/testsuite/lib/jit-elf-helpers.exp | 73 +++++++++++++++++++++++++++
>  3 files changed, 92 insertions(+), 89 deletions(-)
>  create mode 100644 gdb/testsuite/lib/jit-elf-helpers.exp
> 
> diff --git a/gdb/testsuite/gdb.base/jit-elf-so.exp b/gdb/testsuite/gdb.base/jit-elf-so.exp
> index eee20e16c2..31a422a5d2 100644
> --- a/gdb/testsuite/gdb.base/jit-elf-so.exp
> +++ b/gdb/testsuite/gdb.base/jit-elf-so.exp
> @@ -26,67 +26,32 @@ if {[get_compiler_info]} {
>      return 1
>  }
>  
> -proc compile_jit_main {options} {
> -    global srcdir subdir testfile2 srcfile2 binfile2
> -    set testfile2 jit-elf-main
> -    set srcfile2 ${testfile2}.c
> -    set binfile2 [standard_output_file $testfile2.so]
> -    set options [concat \
> -	$options \
> -	debug]
> -    if { [gdb_compile_shlib "${srcdir}/${subdir}/${srcfile2}" "${binfile2}" \
> -	$options] != "" } {
> -	untested "Failure to compile jit-elf-main"
> -    }
> -}
> +load_lib jit-elf-helpers.exp
>  
>  proc compile_jit_dlmain {options} {
> -    global srcdir subdir testfile srcfile binfile
> -    set testfile jit-elf-dlmain
> -    set srcfile ${testfile}.c
> -    set binfile [standard_output_file $testfile]
> +    global srcdir subdir testfile_dlmain srcfile_dlmain
> +    set testfile_dlmain jit-elf-dlmain
> +    set srcfile_dlmain ${testfile_dlmain}.c
> +    set binfile [standard_output_file $testfile_dlmain]
>      set options [concat \
>  	$options \
>  	debug]
> -    if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" \
> +    if { [gdb_compile "${srcdir}/${subdir}/${srcfile_dlmain}" "${binfile}" \
>  	executable $options] != "" } {
>  	untested "Failure to compile jit-elf-main"
>      }
>  }

It looks like we're returning values through global variables, which are then used
in the top-level code.  Let's use return values if possible.  The local variable
names can remain unchanged, to minimize the diff.

One convenient way to return multiple values is using a list and lassign:

proc foo { } {
	return [list a b c]
}
lassign [foo] x y z
puts "$x $y $z"

> diff --git a/gdb/testsuite/lib/jit-elf-helpers.exp b/gdb/testsuite/lib/jit-elf-helpers.exp
> new file mode 100644
> index 0000000000..f63694b8f5
> --- /dev/null
> +++ b/gdb/testsuite/lib/jit-elf-helpers.exp
> @@ -0,0 +1,73 @@
> +# Copyright 2020 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# Compiles jit-elf-main.c as a regular executable
> +
> +proc compile_jit_main {binsuffix options} {
> +    global srcdir subdir testfile srcfile binfile
> +    set testfile jit-elf-main
> +    set srcfile ${testfile}.c
> +    set binfile [standard_output_file $testfile$binsuffix]
> +    set options [concat \
> +	$options \
> +	debug]
> +    if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" \
> +	executable $options] != "" } {
> +	untested "Failure to compile jit-elf-main"
> +    }
> +}
> +
> +# Compiles jit-elf-main.c as a shared library
> +
> +proc compile_jit_main_as_so {binsuffix options} {
> +    global srcdir subdir testfile srcfile binfile
> +    set testfile jit-elf-main
> +    set srcfile ${testfile}.c
> +    set binfile [standard_output_file $testfile$binsuffix]
> +    set options [concat \
> +	$options \
> +	additional_flags="-DMAIN=jit_dl_main" \
> +	debug]
> +    if { [gdb_compile_shlib "${srcdir}/${subdir}/${srcfile}" "${binfile}" \
> +	 $options] != "" } {
> +	untested "Failure to compile jit-elf-main"
> +    }
> +}
> +
> +# Compiles jit-elf-solib.c as multiple shared libraries
> +# distinguished by a numerical suffix
> +
> +proc compile_n_jit_so {count binsuffix options} {
> +    global srcdir subdir solib_binfile_targets

I almost mentioned it in the previous patch, but especially now since this is
split in a separate library, I would suggest make this function return
solib_binfile_targets, instead of setting a global variable, that will make it
much easier to follow.

Simon