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

[Pierre Marsais <pierre.marsais@lse.epita.fr>]

Hi,

Thanks for the review.

On Thu, Sep 27, 2018 at 08:44:44AM +0000, Metzger, Markus T wrote:
>> +            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?

Oops, yes. I don't find how to query the offsets/sizes remotely, however
there is XSTATE areas sizes in gdb/common/x86-xstate.h. I think we can
assume that those values are correct.

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

I'm not sure to follow you. In which cases 32-bit behaves differently
than 64-bit ?

>> +
>> +            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.

If I'm not mistaken, the compact format is only used by XSAVEC
instruction, which doesn't have the same opcode. The XSAVE instruction
seems unrelated to this format.

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

Isn't this roughly the same as:
[ istarget "x86_64-*linux*" ] && [ istarget "i?86-*linux*" ]
thus excluding all arch but 32 and 64 bit x86 ?

Anyways it seems to be working on my x86_64 linux with
$ make check TESTS=gdb.reverse/i386-xsave-reverse.exp

Regards,

-- 
Pierre "Pimzero" MARSAIS,
EPITA 2018; GISTRE | ACU | LSE