Re: [PATCH v2 2/5] Change xstate_bv handling to use 8 bytes of data.

Sturm, Michael
2016-12-05T13:52:47Z

Hi Luis,

thanks for your feedback - please see my comments inline below.

Thanks and Regards,
Michael

On 02/12/2016 02:26, Luis Machado wrote:
> On 12/01/2016 07:38 AM, Michael Sturm wrote:
>> The size of the state-component bitmap as specified in
>> Intel(R) 64 and IA-32 Architectures Software Developer's Manual,
>> Chapter 13.4.2 is 8 bytes.
>> So far, the data types used for xstate_bv_p (gdb_byte*),
>> clear_bv (unsigned int) and tdep->xcr0 (uint64_t) were
>> inconsistent. But, since the xstate components were still
>> fitting into a single byte, the code still worked
>> as expected.
>> However, with the addition of the PKU feature (bit 9),
>> using one byte for the bitmap will no longer be sufficient.
>>
>> This patch changes related code to use 64 bit data types
>> consistently and changes read/write acces of the XSAVE
>> header in the xsave buffer to use the endianess-aware
>> functions extract_unsigned_integer and store_unsigned_integer.
>>
>> gdb/Changelog:
>> 2016-04-18 Michael Sturm <michael.sturm@intel.com>
>>
>> * i387-tdep.c (i387_supply_xsave): Change type
>> of clear_bv to ULONGEST. Replace gdb_byte *xstate_bv_p
>> with ULONGEST xstate_bv and use extract_unsigned_integer
>> and store_unsigned_integer to read/write its value from
>> the xsave buffer. This is required to make sure that
>> eventual differences in endianess between host and
>> target are taken care off.
>> (i387_collect_xsave): Replace gdb_byte *xstate_bv_p
>> with ULONGEST initial_xstate_bv and use
>> extract_unsigned_integer/store_unsigned_integer to
>> read/write its value from the xsave buffer.
>> Change type of clear_bv to ULONGEST.
>
> Too lengthy a description. It only needs to say what was done, not why.
Ok, will be fixed in next revision.
>
>>
>> gdbserver/Changelog:
>> 2016-04-18 Michael Sturm <michael.sturm@intel.com>
>>
>> * i387-fp.c (i387_cache_to_xsave): Change type of clear_bv to
>> unsigned long long.
>> (i387_fxsave_to_cache): Likewise.
>>
>> Change-Id: I0de254158960b4f7bcbc9fe2fb857034fa1f7ca5
>> Signed-off-by: Michael Sturm <michael.sturm@intel.com>
>> ---
>> gdb/gdbserver/i387-fp.c | 8 ++++----
>> gdb/i387-tdep.c | 33 +++++++++++++++++++++------------
>> 2 files changed, 25 insertions(+), 16 deletions(-)
>>
>> diff --git a/gdb/gdbserver/i387-fp.c b/gdb/gdbserver/i387-fp.c
>> index a90729a..d0b0736 100644
>> --- a/gdb/gdbserver/i387-fp.c
>> +++ b/gdb/gdbserver/i387-fp.c
>> @@ -273,14 +273,14 @@ i387_cache_to_xsave (struct regcache *regcache,
>> void *buf)
>> struct i387_xsave *fp = (struct i387_xsave *) buf;
>> int i;
>> unsigned long val, val2;
>> - unsigned int clear_bv;
>> unsigned long long xstate_bv = 0;
>> + unsigned long long clear_bv = 0;
>> char raw[64];
>> char *p;
>> /* Amd64 has 16 xmm regs; I386 has 8 xmm regs. */
>> int num_xmm_registers = register_size (regcache->tdesc, 0) == 8 ?
>> 16 : 8;
>>
>> - /* The supported bits in `xstat_bv' are 1 byte. Clear part in
>> + /* The supported bits in `xstat_bv' are 8 bytes. Clear part in
>> vector registers if its bit in xstat_bv is zero. */
>
> Might as well fixup the "Clear part in" that sounds odd. Does it mean
> some specific bits need to be cleared?
This part of the comment is there for a long time - I think the original
author wanted to make a connection between
the bits in xstate_bv and the actual memory corresponding to the
respective register "part". What happens is that if
the bit corresponding to X86_XSTATE_AVX is set in clear_bv, zero out
corresponding memory in the xstate buffer.
>
>> clear_bv = (~fp->xstate_bv) & x86_xcr0;
>>
>> @@ -643,12 +643,12 @@ i387_xsave_to_cache (struct regcache *regcache,
>> const void *buf)
>> struct i387_fxsave *fxp = (struct i387_fxsave *) buf;
>> int i, top;
>> unsigned long val;
>> - unsigned int clear_bv;
>> + unsigned long long clear_bv;
>> gdb_byte *p;
>> /* Amd64 has 16 xmm regs; I386 has 8 xmm regs. */
>> int num_xmm_registers = register_size (regcache->tdesc, 0) == 8 ?
>> 16 : 8;
>>
>> - /* The supported bits in `xstat_bv' are 1 byte. Clear part in
>> + /* The supported bits in `xstat_bv' are 8 bytes. Clear part in
>
> Same here.
Please see above.
>
>> vector registers if its bit in xstat_bv is zero. */
>> clear_bv = (~fp->xstate_bv) & x86_xcr0;
>>
>> diff --git a/gdb/i387-tdep.c b/gdb/i387-tdep.c
>> index f7a3b55..ef3a631 100644
>> --- a/gdb/i387-tdep.c
>> +++ b/gdb/i387-tdep.c
>> @@ -898,7 +898,7 @@ i387_supply_xsave (struct regcache *regcache, int
>> regnum,
>> struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
>> const gdb_byte *regs = (const gdb_byte *) xsave;
>> int i;
>> - unsigned int clear_bv;
>> + ULONGEST clear_bv;
>
> Is there a reason you're using ULONGEST here and unsigned long long
> above? We should make those uses consistent.
Well, above we're in GDBserver, where unsigned long long is the type
used in the rest of the file. Here, we're in GDB, were ULONGEST
seems to be the preferred type. I wanted to be consistent with what is
used in the respective files so far.
>
>> static const gdb_byte zero[MAX_REGISTER_SIZE] = { 0 };
>> enum
>> {
>> @@ -950,12 +950,15 @@ i387_supply_xsave (struct regcache *regcache,
>> int regnum,
>>
>> if (regclass != none)
>> {
>> - /* Get `xstat_bv'. */
>> - const gdb_byte *xstate_bv_p = XSAVE_XSTATE_BV_ADDR (regs);
>> + /* Get `xstat_bv'. The supported bits in `xstat_bv' are 8
>> bytes. */
>
> Two spaces after periods.
Will be fixed in the next revision.
>
>> + enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>> + ULONGEST xstate_bv = 0;
>>
>> - /* The supported bits in `xstat_bv' are 1 byte. Clear part in
>> - vector registers if its bit in xstat_bv is zero. */
>> - clear_bv = (~(*xstate_bv_p)) & tdep->xcr0;
>> + xstate_bv = extract_unsigned_integer (XSAVE_XSTATE_BV_ADDR
>> (regs),
>> + 8, byte_order);
>> +
>> + /* Clear part in vector registers if its bit in xstat_bv is
>> zero. */
>
> See comment above about "Clear part in"
Please see above.
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