Re: [PATCH 6/6] gdb.trace: Add a testcase for tdesc in tfile.

[Pedro Alves <palves@redhat.com>]

On 02/06/2016 08:54 PM, Marcin Kościelnicki wrote:
> This tests whether $ymm15 can be correctly collected and printed from
> tfile.  It covers:
> 
...
> - using proper tdesc in encoding tracepoint actions (without that,
>   internal error happens due to $ymm15h being considered unavailable)

> This test requires https://sourceware.org/ml/gdb-patches/2016-01/msg00115.html

I'll take a look at that one soon.

> 
>  gdb/testsuite/ChangeLog               |  5 +++
>  gdb/testsuite/gdb.trace/tfile-avx.c   | 48 ++++++++++++++++++++
>  gdb/testsuite/gdb.trace/tfile-avx.exp | 82 +++++++++++++++++++++++++++++++++++

Looks mostly OK.  Thanks much for doing all of this.  Some comments below.

>  3 files changed, 135 insertions(+)
>  create mode 100644 gdb/testsuite/gdb.trace/tfile-avx.c
>  create mode 100644 gdb/testsuite/gdb.trace/tfile-avx.exp
> 
> diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
> index 046f112..c9a596c 100644
> --- a/gdb/testsuite/ChangeLog
> +++ b/gdb/testsuite/ChangeLog
> @@ -1,3 +1,8 @@
> +2016-02-06  Marcin Kościelnicki  <koriakin@0x04.net>
> +
> +	* gdb.trace/tfile-avx.c: New test.
> +	* gdb.trace/tfile-avx.exp: New test.
> +
>  2016-02-04  Yao Qi  <yao.qi@linaro.org>
>  
>  	* gdb.base/foll-exec-mode.c: Include limits.h.
> diff --git a/gdb/testsuite/gdb.trace/tfile-avx.c b/gdb/testsuite/gdb.trace/tfile-avx.c
> new file mode 100644
> index 0000000..71107b8
> --- /dev/null
> +++ b/gdb/testsuite/gdb.trace/tfile-avx.c
> @@ -0,0 +1,48 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2016 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/>.  */
> +
> +/*
> + * Test program for reading target description from tfile: collects AVX
> + * registers on x86_64.
> + */
> +
> +#include <immintrin.h>
> +
> +void
> +dummy (void)
> +{}
> +
> +static void
> +end (void)
> +{}
> +
> +int main (void)

Tests should follow GNU/GDB coding conventions too, unless
required for the functionality of the test.

So write:

void
dummy (void)
{
}

int
main (void)

...


> +{
> +  register __v8si a asm("ymm15") = {
> +    0x12340001,
> +    0x12340002,
> +    0x12340003,
> +    0x12340004,
> +    0x12340005,
> +    0x12340006,
> +    0x12340007,
> +    0x12340008,
> +  };
> +  asm volatile ("traceme: call dummy" : : "x" (a));
> +  end ();
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.trace/tfile-avx.exp b/gdb/testsuite/gdb.trace/tfile-avx.exp
> new file mode 100644
> index 0000000..1acf4cb
> --- /dev/null
> +++ b/gdb/testsuite/gdb.trace/tfile-avx.exp
> @@ -0,0 +1,82 @@
> +# Copyright 2016 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/>.
> +
> +if { ! [istarget "x86_64*-*-*linux*"] } {
> +    verbose "Skipping tfile AVX test (target is not x86_64 linux)."
> +    return
> +}

Check is_amd64_regs_target instead, to cover i386-linux/-m64


> +
> +load_lib "trace-support.exp"
> +
> +standard_testfile
> +
> +if {[prepare_for_testing $testfile.exp $testfile $srcfile \
> +     [list debug nowarnings additional_flags=-mavx]]} {
> +    return -1

Why nowarnings?

> +}
> +
> +if ![runto_main] {
> +    fail "Can't run to main to check for trace support"
> +    return -1
> +}
> +
> +if ![gdb_target_supports_trace] {
> +    unsupported "target does not support trace"
> +    return -1
> +}
> +
> +send_gdb "print \$ymm15\n"
> +gdb_expect {
> +    -re "\\$\[0-9\]+ = void.*$gdb_prompt \$" {
> +	verbose "Skipping tfile AVX test (target doesn't support AVX)."
> +	return
> +    }
> +    -re "\\$\[0-9\]+ = \\{.*}.*$gdb_prompt \$" {
> +	# All is well.
> +    }
> +}

Please use gdb_test_multiple instead of raw send_gdb/gdb_expect.

No backslash in "$gdb_prompt \$".

Use $decimal, or just drop that "\\$\[0-9\]+" part.

> +
> +gdb_test "trace traceme" ".*"
> +
> +gdb_trace_setactions "set actions for tracepoint" "" \
> +	"collect \$ymm15" "^$"
> +
> +gdb_breakpoint "end"
> +
> +gdb_test_no_output "tstart"
> +
> +gdb_test "continue" ".*Breakpoint \[0-9\]+, end .*"

$decimal

> +
> +set tracefile [standard_output_file ${testfile}]
> +
> +# Save trace frames to tfile.
> +gdb_test "tsave ${tracefile}.tf" \
> +    "Trace data saved to file '${tracefile}.tf'.*" \
> +    "save tfile trace file"
> +
> +# Change target to tfile.
> +set test "change to tfile target"
> +gdb_test_multiple "target tfile ${tracefile}.tf" "$test" {
> +    -re "A program is being debugged already.  Kill it. .y or n. " {
> +	send_gdb "y\n"
> +	exp_continue
> +    }
> +    -re "$gdb_prompt $" {
> +	pass "$test"
> +    }
> +}

gdb_test handles queries:

gdb_test \
  "target tfile ${tracefile}.tf" "" \
  "change to tfile target" \
  "A program is being debugged already.  Kill it. .y or n. $" \
  "y"

> +
> +gdb_test "tfind 0" "Found trace frame 0, tracepoint .*"
> +
> +gdb_test "print/x \$ymm15.v8_int32" "\\$\[0-9\]+ = \\{0x12340001, .*, 0x12340008}.*"
> 

$decimal, or just drop the value number match.  Drop .* at the end to tighten
the regex (gdb_test expects \r\n before the prompt).

  gdb_test "print/x \$ymm15.v8_int32" " = \\{0x12340001, .*, 0x12340008}"

Thanks,
Pedro Alves