This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH V4 1/3] Add AVX512 registers support to GDB.
- From: Pedro Alves <palves at redhat dot com>
- To: Michael Sturm <michael dot sturm at intel dot com>
- Cc: eliz at gnu dot org, mark dot kettenis at xs4all dot nl, walfred dot tedeschi at intel dot com, gdb-patches at sourceware dot org
- Date: Fri, 11 Apr 2014 17:32:54 +0100
- Subject: Re: [PATCH V4 1/3] Add AVX512 registers support to GDB.
- Authentication-results: sourceware.org; auth=none
- References: <1396441423-31480-1-git-send-email-michael dot sturm at intel dot com> <1396441423-31480-2-git-send-email-michael dot sturm at intel dot com>
On 04/02/2014 01:23 PM, Michael Sturm wrote:
> diff --git a/gdb/testsuite/gdb.arch/i386-avx512.c b/gdb/testsuite/gdb.arch/i386-avx512.c
> new file mode 100644
> index 0000000..4e86828
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/i386-avx512.c
> @@ -0,0 +1,249 @@
> +#include <stdio.h>
It's always a good idea to strive to avoid a dependency on stdio.h, so
that tests can run against bare metal targets that might not have that.
AFAICS, you don't really need this: it's only there for a puts at
the end that doesn't seem to serve a functional purpose. Could
you remove it?
Would you mind making the test follow GNU coding formatting
throughout?
> +#include "i386-cpuid.h"
> +
> +typedef struct {
typedef struct
{
> + double f[8];
> +} v8sd_t;
> +
> +short k_data[] = {
short k_data[] =
{
> +int
> +have_avx512 (void)
> +{
> +
> +void move_k_data_to_reg()
Line break and "void":
void
move_k_data_to_reg (void)
> +void move_k_data_to_memory()
> +{
> +void move_zmm_data_to_reg()
> +{
> +void move_zmm_data_to_memory()
> +{
Same in these.
> +
> +int
> +main (int argc, char **argv)
> +{
> + if (have_avx512 ())
> + {
> + /* Test for K registers. */
> + move_k_data_to_reg();
Space before parens in call.
> + asm ("nop"); /* first breakpoint here */
> +
> + move_k_data_to_memory();
Ditto. (and more below.)
> +set test "print have_avx512 ()"
> +gdb_test_multiple "$test" $test {
> + -re ".. = 1\r\n$gdb_prompt $" {
> + pass "check whether processor supports AVX512"
> + }
> + -re ".. = 0\r\n$gdb_prompt $" {
> + unsupported "processor does not support AVX512; skipping AVX512 tests"
> + return
Returns directly in a -re branch in probe-style tests don't catch
the case of the print failing in some way caught internally
by gdb_test_multiple (internal error, timeout, etc.).
> + }
> +}
Here's how I usually write these things:
set supports_avx512 0
set test "probe AVX512 support"
gdb_test_multiple "print have_avx512 ()" $test {
-re ".. = 1\r\n$gdb_prompt $" {
pass $test
set supports_avx512 1
}
-re ".. = 0\r\n$gdb_prompt $" {
pass $test
}
}
if { !$supports_avx512 } {
unsupported "processor does not support AVX512"
return
}
Note that by making the test a "probe" test, we make
it a pass even in the '= 0' case -- the test indeed
probed successfully.
> +
> +for { set r 0 } { $r < $nr_regs } { incr r } {
> + set val [expr $r + 1]
> + gdb_test "print/x k_data\[$r\]" \
> + ".. = 0x$val$val$val$val" \
> + "check contents of k_data\[$r\]"
> +
> +gdb_test "break [gdb_get_line_number "third breakpoint here"]" \
> + "Breakpoint .* at .*i386-avx512.c.*" \
> + "set third breakpoint in main"
Indentation here and all the way through below isn't right.
Also, see:
https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Make_sure_test_messages_are_unique
Otherwise looks good to me.
Thanks,
--
Pedro Alves