Re: [PATCH 8/8] Test case for functions with non-contiguous ranges

[Simon Marchi <simon.marchi@ericsson.com>]

On 2018-06-26 02:57 AM, Kevin Buettner wrote:
> See comments in the new files for what this is about - I tried to
> explain it all there.

This test looks really nice and thorough.

> +# This test can only be run on targets which support DWARF-2 and use gas.
> +if {![dwarf2_support]} {
> +    verbose "Skipping DW_AT_ranges test."

I think "unsupported" would be more appropriate.

> +    return 0
> +}
> +
> +# The .c files use __attribute__.

This comment seems wrong/outdated.

> +if [get_compiler_info] {
> +    return -1
> +}
> +if !$gcc_compiled {
> +    verbose "Skipping DW_AT_ranges test."

"unsupported" here too?

> +    return 0
> +}
>
> +
> +standard_testfile dw2-ranges-func.c dw2-ranges-func-dw.S
> +
> +# We need to know the size of integer and address types in order to
> +# write some of the debugging info we'd like to generate.
> +#
> +# For that, we ask GDB by debugging our test program.  Any program
> +# would do, but since we already have it specifically for this
> +# testcase, might as well use that.
> +
> +if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
> +    return -1
> +}
> +
> +set asm_file [standard_output_file $srcfile2]
> +Dwarf::assemble $asm_file {
> +    global srcdir subdir srcfile srcfile2
> +    declare_labels integer_label volatile_label ranges_label ranges2_label L
> +    set int_size [get_sizeof "int" 4]
> +
> +    # Find start address and length for our functions.
> +    set main_func \
> +	[function_range main [list ${srcdir}/${subdir}/$srcfile]]
> +    set foo_func \
> +	[function_range foo [list ${srcdir}/${subdir}/$srcfile]]
> +    set foo_low_func \
> +	[function_range foo_low [list ${srcdir}/${subdir}/$srcfile]]
> +    set bar_func \
> +	[function_range bar [list ${srcdir}/${subdir}/$srcfile]]
> +    set baz_func \
> +	[function_range baz [list ${srcdir}/${subdir}/$srcfile]]
> +
> +    set main_start [lindex $main_func 0]
> +    set main_len [lindex $main_func 1]
> +    set foo_start [lindex $foo_func 0]
> +    set foo_end {$foo_start + [lindex $foo_func 1]}
> +    set foo_low_start [lindex $foo_low_func 0]
> +    set foo_low_len [lindex $foo_low_func 1]
> +    set foo_low_end {$foo_low_start + $foo_low_len}
> +    set bar_start [lindex $bar_func 0]
> +    set bar_len [lindex $bar_func 1]
> +    set baz_start [lindex $baz_func 0]
> +    set baz_len [lindex $baz_func 1]

If you want to make this shorter, you could use lassign:

  https://www.tcl.tk/man/tcl/TclCmd/lassign.htm

Something like:

  lassign [function_range main [list ${srcdir}/${subdir}/$srcfile]] \
    main_start main_len

I see that you use expressions like " [lindex $main_func 0]" below, it would
be clearer to always use the *_start or *_len variables.

> +    # Generate ranges data.
> +    ranges {is_64 [is_64_target]} {
> +	ranges_label: sequence {
> +	    {range {$foo_start } $foo_end}
> +	    {range {$foo_low_start} $foo_low_end}
> +	}
> +	ranges2_label: sequence {
> +	    {range {$foo_start } $foo_end}
> +	    {range {$foo_low_start} $foo_low_end}
> +	    {range {$main_start} $main_start + $main_len}
> +	    {range {$bar_start} $bar_start + $bar_len}
> +	    {range {$baz_start} $baz_start + $baz_len}
> +	}

ranges_label and ranges2_label could perhaps have more expressive names?

> +set test "foo and foo_low are at different addresses"
> +if {$foo_low_addr == $foo_addr} {
> +    fail $test
> +} else {
> +    pass $test
> +}

This can be

  gdb_assert {$foo_low_addr != $foo_addr} "foo and foo_low are at different addresses"

> +
> +# This more permissive RE for "break foo" will allow a breakpoint on
> +# multiple locations to PASS.  */
> +gdb_test "break foo" \
> +    "Breakpoint.*at.*" \
> +    "break foo (2nd time)"

We should avoid having trailing parenthesis in test messages:

https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Do_not_use_.22tail_parentheses.22_on_test_messages

In a test like yours, that is really multiple independent scenarios tested one
after the other, I like to structure the test like this:

proc_with_prefix test_something {} {
  ...
}

proc_with_prefix test_something_else {} {
  ...
}

test_something
test_something_else


Using proc_with_prefix automatically prefixes test names with the name of the
proc (which is usually self-explanatory).  This avoids have to manually
differentiate test names such as in "break foo (2nd time)".

I also like to do a clean_restart at the beginning of each test procedure, to
reduce the chance of inter-dependency between each test procedure, though it's
not strictly necessary.

Simon