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

[Luis Machado <lgustavo@codesourcery.com>]

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"