[PATCH 4/7] [gdb/testsuite] use args as lib list for jit-elf tests
Simon Marchi
simark@simark.ca
Mon Mar 23 00:04:36 GMT 2020
Hi Mihails,
Just a few nits, in general that looks good.
On 2020-02-18 7:43 a.m., Mihails Strasuns wrote:
> Old usage: jit-elf-main lib.so 2
> New usage: jit-elf-main lib.so.1 lib.so.2
>
> Refactoring necessary to support running tests over multiple jit
> binaries rather than mapping the same binary muultiple times.
>
> gdb/testsuite/ChangeLog:
>
> 2020-02-18 Mihails Strasuns <mihails.strasuns@intel.com>
>
> * gdb.base/jit-elf-main.c: read lib list from argc/argv
> * gdb.base/jit-elf.exp: compile N jit libraries and use the list
> * gdb.base/jit-elf-so.exp: ditto
>
> Change-Id: Ie8f85ec6358604c14557b0417d6621b2f8942033
> Signed-off-by: Mihails Strasuns <mihails.strasuns@intel.com>
> ---
> gdb/testsuite/gdb.base/jit-elf-main.c | 52 +++++--------
> gdb/testsuite/gdb.base/jit-elf-so.exp | 108 ++++++++++++++++----------
> gdb/testsuite/gdb.base/jit-elf.exp | 99 +++++++++++------------
> 3 files changed, 133 insertions(+), 126 deletions(-)
>
> diff --git a/gdb/testsuite/gdb.base/jit-elf-main.c b/gdb/testsuite/gdb.base/jit-elf-main.c
> index fe0f540d6f..0e4b2e9a40 100644
> --- a/gdb/testsuite/gdb.base/jit-elf-main.c
> +++ b/gdb/testsuite/gdb.base/jit-elf-main.c
> @@ -45,9 +45,9 @@
> #endif /* !ElfW */
>
> static void
> -usage (const char *const argv0)
> +usage ()
usage (void)
> diff --git a/gdb/testsuite/gdb.base/jit-elf-so.exp b/gdb/testsuite/gdb.base/jit-elf-so.exp
> index 526414f43c..eee20e16c2 100644
> --- a/gdb/testsuite/gdb.base/jit-elf-so.exp
> +++ b/gdb/testsuite/gdb.base/jit-elf-so.exp
> @@ -26,45 +26,64 @@ if {[get_compiler_info]} {
> return 1
> }
>
> -#
> -# test running programs
> -#
> -
> -set testfile jit-elf-dlmain
> -set srcfile ${testfile}.c
> -set binfile [standard_output_file ${testfile}]
> -if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug shlib_load}] != "" } {
> - untested "failed to compile"
> - 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]
These variables don't need to have the suffix `2` anymore, let's remove it.
> + set options [concat \
> + $options \
> + debug]
> + if { [gdb_compile_shlib "${srcdir}/${subdir}/${srcfile2}" "${binfile2}" \
> + $options] != "" } {
> + untested "Failure to compile jit-elf-main"
If we fail to compile something, do we want to return some failure return status
and exit the test? This is how it works currently.
> + }
> }
>
> -set testfile2 jit-elf-main
> -set srcfile2 ${testfile2}.c
> -set binfile2 [standard_output_file ${testfile2}.so]
> -set binfile2_dlopen [shlib_target_file ${testfile2}.so]
> -if { [gdb_compile_shlib "${srcdir}/${subdir}/${srcfile2}" ${binfile2} {debug additional_flags="-DMAIN=jit_dl_main"}] != "" } {
> - untested "failed to compile main shared library"
> - return -1
> +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]
> + set options [concat \
> + $options \
> + debug]
> + if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" \
> + executable $options] != "" } {
Indent this a bit more (typically 4 columns more than the `[`), otherwise it's
easy to confuse it with a statement in the `if`. There are multiple instances
in the patch.
> + untested "Failure to compile jit-elf-main"
> + }
> }
>
> -set solib_testfile "jit-elf-solib"
> -set solib_srcfile "${srcdir}/${subdir}/${solib_testfile}.c"
> -set solib_binfile [standard_output_file ${solib_testfile}.so]
> -set solib_binfile_test_msg "SHLIBDIR/${solib_testfile}.so"
> +proc compile_n_jit_so {count options} {
`options` is unnecessary, callers always pass an empty string, you could remove it.
If it's needed in a subsequent patch, then this other patch can add the parameter
back when needed.
> + global srcdir subdir solib_binfile_targets
> + set solib_binfile_targets {}
> + set solib_testfile jit-elf-solib
> +
> + for {set i 1} {$i <= $count} {incr i} {
> + set solib_binfile [standard_output_file ${solib_testfile}.so.$i]
> + set solib_srcfile "${srcdir}/${subdir}/${solib_testfile}.c"
> + set solib_binfile_test_msg "SHLIBDIR/${solib_testfile}.so.$i"
> +
> + # Note: compiling without debug info by default: some test
> + # do symbol renaming by munging on ELF symbol table, and that
> + # wouldn't work for .debug sections. Also, output for "info
> + # function" changes when debug info is present.
> + if { [gdb_compile_shlib ${solib_srcfile} ${solib_binfile} $options] != "" } {
> + untested "Failure to compile ${solib_binfile_test_msg}"
> + }
>
> -# Note: compiling without debug info: the library goes through symbol
> -# renaming by munging on its symbol table, and that wouldn't work for .debug
> -# sections. Also, output for "info function" changes when debug info is resent.
> -if { [gdb_compile_shlib ${solib_srcfile} ${solib_binfile} {}] != "" } {
> - untested "failed to compile jit shared library"
> - return -1
> + set path [gdb_remote_download target ${solib_binfile}]
> + set solib_binfile_targets [concat $solib_binfile_targets $path]
You might want to use `lappend` here.
https://www.tcl.tk/man/tcl8.6/TclCmd/lappend.htm
> + }
> }
>
> -set solib_binfile_target [gdb_remote_download target ${solib_binfile}]
> +compile_jit_main {additional_flags="-DMAIN=jit_dl_main"}
> +compile_jit_dlmain {shlib_load}
> +compile_n_jit_so 2 {}
>
> proc one_jit_test {count match_str} {
> with_test_prefix "one_jit_test-$count" {
> - global verbose testfile srcfile2 binfile2 binfile2_dlopen solib_binfile_target solib_binfile_test_msg
> + global verbose testfile srcfile2 binfile2 binfile2_dlopen solib_binfile_targets
>
> clean_restart $testfile
> gdb_load_shlib $binfile2
> @@ -81,9 +100,16 @@ proc one_jit_test {count match_str} {
>
> gdb_breakpoint [gdb_get_line_number "break here before-dlopen" ]
> gdb_continue_to_breakpoint "break here before-dlopen"
> +
> # Poke desired values directly into inferior instead of using "set args"
> # because "set args" does not work under gdbserver.
> - gdb_test_no_output "set var jit_libname = \"$binfile2_dlopen\""
> + gdb_test_no_output "set var jit_libname = \"$binfile2\""
> + incr count
> + gdb_test "set var argc=$count"
> + gdb_test "set var argv=malloc(sizeof(char*)*$count)"
> + for {set i 1} {$i < $count} {incr i} {
> + gdb_test "set var argv\[$i\]=\"[lindex $solib_binfile_targets [expr $i-1]]\""
> + }
Instead of using malloc (which makes GDB call a function in the inferior, could
we instead just allocate a sufficiently large static array in the test program?
We want to avoid interfering with the inferior as much as possible.
>
> gdb_breakpoint [gdb_get_line_number "break here after-dlopen" ]
> gdb_continue_to_breakpoint "break here after-dlopen"
> @@ -91,10 +117,6 @@ proc one_jit_test {count match_str} {
> gdb_breakpoint "$srcfile2:[gdb_get_line_number {break here 0} $srcfile2]"
> gdb_continue_to_breakpoint "break here 0"
>
> - gdb_test_no_output "set var argc = 2"
> - gdb_test_no_output "set var libname = \"$solib_binfile_target\"" "set var libname = \"$solib_binfile_test_msg\""
> - gdb_test_no_output "set var count = $count"
> -
> gdb_breakpoint "$srcfile2:[gdb_get_line_number {break here 1} $srcfile2]"
> gdb_continue_to_breakpoint "break here 1"
>
> @@ -114,12 +136,14 @@ proc one_jit_test {count match_str} {
> }
> }
>
> -one_jit_test 1 "${hex} jit_function_0000"
> -one_jit_test 2 "${hex} jit_function_0000\[\r\n\]+${hex} jit_function_0001"
> +one_jit_test 1 "${hex} jit_function_0001"
> +one_jit_test 2 "${hex} jit_function_0001\[\r\n\]+${hex} jit_function_0002"
>
> -# We don't intend to load the .so as a JIT debuginfo reader, but we
> -# need some handy file name for a completion test.
> -gdb_test \
> - "complete jit-reader-load [standard_output_file ${solib_testfile}.s]" \
> - "jit-reader-load $solib_binfile" \
> - "test jit-reader-load filename completion"
> +foreach solib $solib_binfile_targets {
> + # We don't intend to load the .so as a JIT debuginfo reader, but we
> + # need some handy file name for a completion test.
> + gdb_test \
> + "complete jit-reader-load [standard_output_file $solib]" \
> + "jit-reader-load $solib" \
> + "test jit-reader-load filename completion"
> +}
Hmm, there is not much point doing this test multiple times, and this
actually changes what is tested. Before it tests that
jit-reader-load .../something.s
gets completed to
jit-reader-load .../something.so
With the patch, it tests that
jit-reader-load .../something.so
gets completed to
jit-reader-load .../something.so
It's a bit strange in the first place to test that in this particular file,
but anyhow, let's try to preserve the original intent of the test. You could
just test with the first element of solib_binfile_targets and shorten it by one
character with:
string range $name 0 [expr { [string length $name] - 2 }]
> diff --git a/gdb/testsuite/gdb.base/jit-elf.exp b/gdb/testsuite/gdb.base/jit-elf.exp
> index 71d3e37dfb..ba79efaf1c 100644
> --- a/gdb/testsuite/gdb.base/jit-elf.exp
> +++ b/gdb/testsuite/gdb.base/jit-elf.exp
> @@ -23,42 +23,41 @@ if {[get_compiler_info]} {
> return 1
> }
>
> -# Compile the testcase program and library. BINSUFFIX is the suffix
> -# to append to the program and library filenames, to make them unique
> -# between invocations. OPTIONS is passed to gdb_compile when
> -# compiling the program.
> -
> -proc compile_jit_test {testname binsuffix options} {
> - global testfile srcfile binfile srcdir subdir
> - global solib_testfile solib_srcfile solib_binfile solib_binfile_test_msg
> - global solib_binfile_target
> -
> +proc compile_jit_main {binsuffix options} {
Please add a comment to describe what this proc does, in the style of the comment
that was above `compile_jit_test`.
> + 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 [concat debug $options]] != "" } {
> - untested $testname
> - return -1
> - }
> -
> - set solib_testfile "jit-elf-solib"
> - set solib_srcfile "${srcdir}/${subdir}/${solib_testfile}.c"
> - set solib_binfile [standard_output_file ${solib_testfile}$binsuffix.so]
> - set solib_binfile_test_msg "SHLIBDIR/${solib_testfile}$binsuffix.so"
> -
> - # Note: compiling without debug info: the library goes through
> - # symbol renaming by munging on its symbol table, and that
> - # wouldn't work for .debug sections. Also, output for "info
> - # function" changes when debug info is present.
> - if { [gdb_compile_shlib ${solib_srcfile} ${solib_binfile} {-fPIC}] != "" } {
> - untested $testname
> - return -1
> + executable $options] != "" } {
> + untested "Failure to compile jit-elf-main"
> }
> +}
>
> - set solib_binfile_target [gdb_remote_download target ${solib_binfile}]
> +proc compile_n_jit_so {count options} {
Comment here too.
> + global srcdir subdir solib_binfile_targets
> + set solib_binfile_targets {}
> + set solib_testfile jit-elf-solib
> +
> + for {set i 1} {$i <= $count} {incr i} {
> + set solib_binfile [standard_output_file ${solib_testfile}.so.$i]
> + set solib_srcfile "${srcdir}/${subdir}/${solib_testfile}.c"
> + set solib_binfile_test_msg "SHLIBDIR/${solib_testfile}.so.$i"
> +
> + # Note: compiling without debug info by default: some test
> + # do symbol renaming by munging on ELF symbol table, and that
> + # wouldn't work for .debug sections. Also, output for "info
> + # function" changes when debug info is present.
> + if { [gdb_compile_shlib ${solib_srcfile} ${solib_binfile} $options] != "" } {
> + untested "Failure to compile ${solib_binfile_test_msg}"
> + }
>
> - return 0
> + set path [gdb_remote_download target ${solib_binfile}]
> + set solib_binfile_targets [concat $solib_binfile_targets $path]
lappend here too.
Simon
More information about the Gdb-patches
mailing list