This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH V4 1/3] Add AVX512 registers support to GDB.


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]