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

Simon Marchi
2018-08-01T02:56:56Z

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