Re: [PATCHv3 2/2] gdb: Initial baremetal riscv support

[Simon Marchi <simark at simark dot ca>]

Hi Andrew,

I took a deeper look at the test which I had just skimmed previously,
I have a few comments on that.

One general comment, I would suggest naming the test "infcall-nested-structs"
to make it clear that it tests inferior function calls.

On 2018-03-02 03:09 PM, Andrew Burgess wrote:
> diff --git a/gdb/testsuite/gdb.base/nested-structs.exp b/gdb/testsuite/gdb.base/nested-structs.exp
> new file mode 100644
> index 00000000000..c359acde48c
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/nested-structs.exp
> @@ -0,0 +1,189 @@
> +# This testcase is part of GDB, the GNU debugger.
> +
> +# Copyright 2018 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/>.
> +
> +
> +# Some targets can't call functions, so don't even bother with this
> +# test.
> +
> +if [target_info exists gdb,cannot_call_functions] {
> +    unsupported "this target can not call functions"
> +    continue
> +}
> +
> +if [gdb_skip_float_test] {
> +    unsupported "this target doesn't support float"
> +    continue
> +}
> +
> +set int_types { tc ts ti tl tll }
> +set float_types { tf td tld }
> +set complex_types { tfc tdc tldc }
> +
> +set compile_flags {debug}
> +if [support_complex_tests] {
> +    lappend compile_flags "additional_flags=-DTEST_COMPLEX"
> +}
> +
> +# Given N (0..25), return the corresponding alphabetic letter in lower
> +# or upper case.  This is ment to be i18n proof.

ment -> meant.

Also, I'm curious, what do you mean exactly by i18n proof?

> +
> +proc i2a { n } {
> +    return [string range "abcdefghijklmnopqrstuvwxyz" $n $n]
> +}

The i2a proc does not seem to be used (other than in I2A), so can you instead implement
I2A directly and not have i2a?

> +
> +proc I2A { n } {
> +    return [string toupper [i2a $n]]
> +}
> +
> +# Compile a variant of nested-structs.c using TYPES to specify the
> +# types of the struct fields within the source.  Run up to main.
> +# Also updates the global "testfile" to reflect the most recent build.
> +
> +proc start_nested_structs_test { types } {
> +    global testfile
> +    global srcfile
> +    global binfile
> +    global subdir
> +    global srcdir
> +    global gdb_prompt

gdb_prompt is unused in this proc.

> +    global compile_flags
> +
> +    standard_testfile .c
> +
> +    # Create the additional flags
> +    set flags $compile_flags
> +
> +    set n 0

That "set n 0" can be removed.

> +    for {set n 0} {$n<[llength ${types}]} {incr n} {
> +	set m [I2A ${n}]
> +	set t [lindex ${types} $n]
> +	lappend flags "additional_flags=-Dt${m}=${t}"
> +	append testfile "-" "$t"
> +    }

> +
> +    set binfile [standard_output_file ${testfile}]
> +    if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable "${flags}"] != "" } {
> +	verbose -log "failed to compile"
> +	return 0
> +    }
> +
> +    # Start with a fresh gdb.
> +    gdb_exit
> +    gdb_start
> +    gdb_reinitialize_dir $srcdir/$subdir
> +    gdb_load ${binfile}

Can you use "clean_restart ${binfile}" instead?

> +
> +    # Make certain that the output is consistent
> +    gdb_test_no_output "set print sevenbit-strings"
> +    gdb_test_no_output "set print address off"
> +    gdb_test_no_output "set print pretty off"
> +    gdb_test_no_output "set width 0"
> +    gdb_test_no_output "set print elements 300"
> +
> +    # Advance to main
> +    if { ![runto_main] } then {
> +	verbose -log "failed to run to main"

The usual pattern is to put

  fail "can't run to main"

if runto_main fails.  Strangely, I don't think runto_main will cause a FAIL
if it fails (it calls runto with no-message, which disables the FAILs).  I
see that the failure would be noticed because of the unresolved below, but
let's follow the usual pattern, in case somebody decides to copy paste your
code :).

> +	return 0
> +    }
> +
> +    return 1
> +}
> +
> +# Assuming GDB is stopped at main within a test binary, run some tests
> +# passing structures, and reading return value structures.
> +
> +proc run_tests {} {
> +    global gdb_prompt
> +
> +    foreach {name} {struct01 struct02 struct03 struct04} {
> +	gdb_test "p/d check_arg_${name} (ref_val_${name})" "= 1"
> +
> +	set refval ""
> +	set test1 "fetch reference value for ${name}"
> +	gdb_test_multiple "p ref_val_${name}" "${test1}" {
> +	    -re " = (.*)\r\n$gdb_prompt $" {
> +		pass "$test1"
> +		set refval $expect_out(1,string)
> +	    }
> +	}

I wonder if you can use the get_valueof proc from lib/gdb.exp here and below.
It would save a few lines and make the code more straight to the point.

> +
> +	set test2 "check return value ${name}"
> +	if { ${refval} != "" } {
> +	    set answer ""
> +	    set test3 "fetch return value for ${name}"
> +	    gdb_test_multiple "p rtn_str_${name} ()" "$test3" {
> +		-re " = (.*)\r\n$gdb_prompt $" {
> +		    pass $test3
> +		    set answer $expect_out(1,string)
> +		}
> +	    }
> +
> +	    if { ${answer} == ${refval} } {
> +		pass $test2
> +	    } else {
> +		fail $test2
> +	    }

When you have that pattern, you can use

  gdb_assert {${answer} == ${refval}} ${test2}

> +	} else {
> +	    unresolved $test2
> +	}
> +    }
> +}
> +
> +# Set up a test prefix, compile the test binary, run to main, and then
> +# run some tests.
> +
> +proc start_gdb_and_run_tests { types } {
> +    set prefix "types"
> +
> +    foreach t $types {
> +	append prefix "-" "${t}"
> +    }
> +
> +    with_test_prefix $prefix {
> +	if { ! [start_nested_structs_test $types] } {
> +	    unresolved "failed to start test correctly"
> +	} else {
> +	    run_tests
> +	}
> +    }
> +}
> +
> +if [support_complex_tests] {
> +    foreach ta $complex_types {
> +	start_gdb_and_run_tests $ta
> +    }
> +}
> +
> +foreach ta $float_types {
> +    start_gdb_and_run_tests $ta
> +}

Would it make sense to run the test with int_types only?

  foreach ta $int_types {
      start_gdb_and_run_tests $ta
  }

If so, your test could be useful on targets that are marked as
"skip_float_test".  Instead of skipping this test entirely, they
could run the int tests, and the tests that deal with float would
be in an

  if ![gdb_skip_float_test] {
      ...
  }

> +
> +foreach ta $int_types {
> +    foreach tb $float_types {
> +	start_gdb_and_run_tests [list $ta $tb]
> +    }
> +}
> +
> +foreach ta $float_types {
> +    foreach tb $int_types {
> +	start_gdb_and_run_tests [list $ta $tb]
> +    }
> +
> +    foreach tb $float_types {
> +	start_gdb_and_run_tests [list $ta $tb]
> +    }
> +}
> 

Thanks,

Simon