Re: [patch v6 10/12] test, btrace: add branch tracing tests

[Jan Kratochvil <jan.kratochvil@redhat.com>]

On Mon, 17 Dec 2012 17:01:39 +0100, markus.t.metzger@intel.com wrote:
[...]
> --- /dev/null
> +++ b/gdb/testsuite/gdb.btrace/enable.exp
> @@ -0,0 +1,89 @@
[...]
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +if { ![istarget "i?86-*-*"] && ![istarget "x86_64-*-*"] } {

All these tests could be limited to "XXX-*-linux*" I think, couldn't they?


> +    verbose "Tests ignored for all but x86 based targets."
> +    return
> +}
[...]
> +# we don't have trace, yet
> +gdb_test "btrace" "No thread\." "btrace enable 2.2"

This way it is equivalent to "No thread\." as \. is no special Tcl sequence so
it equals to '.'.  You wanted "No thread\\." so that backslash gets parsed by
the regex parser and not the Tcl parser.

Alternatively one can use {No thread\.} which skips the Tcl unbackslashing
(but then some characters like \n or \r or $variables are not available).

It is more times here.


> +gdb_test "btrace /m" "No thread\." "btrace enable 2.3"
> +gdb_test "btrace list /fal" "No thread\." "btrace enable 2.4"
> +gdb_test "btrace list /t" "No thread\." "btrace enable 2.5"
> +
> +set testfile "x86-list"

Please do not override $testfile to non-basename of the .exp file, see below
for standard_testfile recommendation instead.


> +
> +# start a debuggee program
> +if { [set binfile [btrace_assemble ${testfile}]] == "" } {
> +    untested ${testfile}.exp
> +    return -1
> +}
> +
> +gdb_reinitialize_dir $srcdir/$subdir
> +gdb_load $binfile

Why not clean_restart?


> +# automatic enabling is off.
> +if ![runto test_1] {
> +	fail "runto test function, 1"
> +	return -1
> +}
[...]
> --- /dev/null
> +++ b/gdb/testsuite/gdb.btrace/list_function.c
> @@ -0,0 +1,31 @@
[...]
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +extern int inc (int);
> +extern int dec (int);
> +
> +extern int main (void)

I find "extern" excessive/unusual during function definiton.

> +{
> +    int x = 0;
> +
> +    x = inc (x);
> +    x = dec (x);
> +
> +    return x; /* bp.1 */
> +}
> diff --git a/gdb/testsuite/gdb.btrace/list_function.exp b/gdb/testsuite/gdb.btrace/list_function.exp
> new file mode 100644
> index 0000000..180273f
> --- /dev/null
> +++ b/gdb/testsuite/gdb.btrace/list_function.exp
> @@ -0,0 +1,50 @@
[...]
> +load_lib btrace.exp
> +
> +set testfile "list_function"
> +set sources [list $testfile.c inc.c dec.c]

Use standard_testfile instead of these two sets.


> +
> +# check for btrace support
> +if { [skip_btrace_tests] } { return -1 }
[...]
> --- /dev/null
> +++ b/gdb/testsuite/lib/btrace.exp
> @@ -0,0 +1,78 @@
[...]
> +proc btrace_assemble { testfile } {
> +    global srcdir
> +    global objdir
> +    global subdir
> +
> +    set srcfile ${srcdir}/${subdir}/${testfile}.S
> +    set objfile ${objdir}/${subdir}/${testfile}.o
> +    set binfile ${objdir}/${subdir}/${testfile}.x
> +
> +    if {[target_assemble ${srcfile} ${objfile} ""] != ""} { return "" }
> +
> +    if {[target_link ${objfile} ${binfile} "-Ttext 0x400100"] != ""} { return "" }

Why is here -Ttext?  It requires at least a comment but I would prefer to
remove it.  Can't you just use additional_flags=-nostdlib for the build?

And can't you just not use -nostdlib, use standard system crt1 and just use
runto_main and no longer define "_start" in your .S file?

This btrace_assemble I find overengineered a bit, there should be
standard_testfile called in each *.exp file which will set srcfile/binfile.


> +
> +    return ${binfile}
> +}
> +
> +proc skip_btrace_tests {} {
> +    global gdb_prompt
> +
> +    set testfile "x86-list"

Please do not override $testfile.  $testfile is set by stadnard_testfile and
it should always match the gdb.btrace/TESTFILE.exp name TESTFILE.


> +    set skip 0
> +
> +    if { [set binfile [btrace_assemble ${testfile}]] == "" } {
> +        return 1
> +    }
> +
> +    gdb_exit
> +    gdb_start
> +    gdb_load $binfile
> +
> +    runto main

You could use clean_restart, couldn't you?



> +
> +    gdb_test_multiple "btrace enable" "check btrace support" {
> +        -re "You can't do that when your target is.*\r\n$gdb_prompt $" {
> +            xfail "check btrace support"
> +            set skip 1
> +        }
> +        -re "Target does not support branch tracing.*\r\n$gdb_prompt $" {
> +            xfail "check btrace support"
> +            set skip 1
> +        }
> +        -re "Could not enable branch tracing.*\r\n$gdb_prompt $" {
> +            xfail "check btrace support"
> +            set skip 1
> +        }
> +        -re "$gdb_prompt $" {
> +			pass "check btrace support"
> +		}

Tabs vs. spaces are wrong here.  Tab is always 8 characters.


> +    }
> +    gdb_exit
> +    remote_file build delete $testfile
> +
> +    return $skip
> +}
> +
> +proc btrace_reset_trace {} {
> +    gdb_test_no_output "btr disable"
> +    gdb_test_no_output "btr enable"
> +
> +    gdb_test "btr list" "No trace." "reset btrace"
> +}
> -- 
> 1.7.6.5


Thanks,
Jan