RE: [PATCH] Add support for recording xsave x86 instruction

["Metzger, Markus T" <markus.t.metzger@intel.com>]

Hello Pierre,

> +        case 4: /* xsave */

We should also check MOD != 3.


> +          uint64_t tmpu64;
> +          if (i386_record_lea_modrm_addr (&ir, &tmpu64))
> +            return -1;
> +          if (record_full_arch_list_add_mem (tmpu64, 512 + 64))
> +            return -1;
> +
> +          for (int i = 2; i < 64; i++) {
> +            if (!((1 << i) & tdep->xcr0))
> +              continue;
> +
> +            unsigned int size, offset, tmp1, tmp2;
> +
> +            if (!__get_cpuid_count(0xd, i, &size, &offset, &tmp1, &tmp2))
> +              return -1;

This would check the native configuration, correct?  What if we recorded
remotely on a different x86 box?

Also I think that we would need to check the inferior architecture to handle
32-bit compatibility mode.

SIZE may be zero.  We should probably check that and continue.  I'm not sure
whether we can actually run into this case but it doesn't hurt to check.

Nit: there's a space before (.


> +
> +            if (record_full_arch_list_add_mem (tmpu64 + offset, size))
> +              return -1;

Looks like this assumes the standard (non-compacted) XSAVE format.

For the compacted format, the offset must be computed by accumulating
the sizes of preceding components.


> diff --git a/gdb/testsuite/gdb.reverse/i386-xsave-reverse.c
> b/gdb/testsuite/gdb.reverse/i386-xsave-reverse.c

> +void xsave_test(void) {

Nit: space before (.


> +	char buf[4096] __attribute__ ((aligned (64))) = { 0 };

The test could query the XSAVE buffer size.


> +	asm ("xor %%eax, %%eax\n\t"
> +	     "not %%eax\n\t"
> +	     "mov %%eax, %%edx\n\t"
> +	     "xsave %0":"=m"(buf) ::"eax", "edx"); } /* end xsave_test */

The } should probably go onto the next line.


> +if ![istarget "*86*-*linux*"] then {
> +    verbose "Skipping i386 reverse tests."
> +    return
> +}

Why exclude 64-bit?


> +runto main

You'd typically check whether that succeeds and abort the test if it doesn't.


> +if [supports_process_record] {
> +    # Activate process record/replay
> +    gdb_test_no_output "record" "turn on process record"
> +}

Shouldn't we abort the test if record is not supported?


> +gdb_test "reverse-step" "xor.*" "reverse-step to xsave"
> +
> +gdb_test "print buf" ".* = '\\\\000' <repeats 4095 times>" \
> +    "verify xsave buffer after reverse xsave"

Regards,
Markus.
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928