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

Luis Machado
2016-12-02T01:26:38Z

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.

>
> 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?

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

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

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

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