Re: [PATCH] ppc64le/gdbserver: Fix ppc_collect/supply_ptrace_register() routines

[Sergio Durigan Junior <sergiodj at redhat dot com>]

On Friday, September 05 2014, Edjunior Barbosa Machado wrote:

> Hi,
>
> this patch fixes the routines to collect and supply ptrace registers on ppc64le
> gdbserver. Originally written for big endian arch, they were causing several
> issues on little endian. With this fix, the number of unexpected failures in
> the testsuite dropped from 263 to 72 on ppc64le.
> Tested on ppc64{,le}. Ok?

Heya!

Thanks for the patch.  A few comments below.

> gdb/gdbserver/
> 2014-09-05  Edjunior Barbosa Machado  <emachado@linux.vnet.ibm.com>
>
> 	* linux-ppc-low.c (ppc_collect_ptrace_register): Adjust routine to take
> 	endianness into account.
> 	(ppc_supply_ptrace_register): Likewise.
>
> ---
>  gdb/gdbserver/linux-ppc-low.c |   37 ++++++++++++++++++++++++++++---------
>  1 file changed, 28 insertions(+), 9 deletions(-)
>
> diff --git a/gdb/gdbserver/linux-ppc-low.c b/gdb/gdbserver/linux-ppc-low.c
> index d743311..1898741 100644
> --- a/gdb/gdbserver/linux-ppc-low.c
> +++ b/gdb/gdbserver/linux-ppc-low.c
> @@ -202,25 +202,44 @@ ppc_cannot_fetch_register (int regno)
>  static void
>  ppc_collect_ptrace_register (struct regcache *regcache, int regno, char *buf)
>  {
> -  int size = register_size (regcache->tdesc, regno);
> -
>    memset (buf, 0, sizeof (long));
>  
> -  if (size < sizeof (long))
> -    collect_register (regcache, regno, buf + sizeof (long) - size);
> +  if (__BYTE_ORDER == __LITTLE_ENDIAN)

Why not use gdbarch_byte_order here?  We don't use __BYTE_ORDER anywhere
in the code.

> +    {
> +      /* Little-endian values always sit at the left end of the buffer.  */
> +      collect_register (regcache, regno, buf);
> +    }
> +  else if (__BYTE_ORDER == __BIG_ENDIAN)
> +    {
> +      /* Big-endian values sit at the right end of the buffer. In case of

  "... of the buffer.  In case..."

Two spaces after period :-).

> +         registers whose size is smaller than sizeof (long), we must use a

I'd prefer:

s/size is/sizes are/

> +         padding to access it correctly.  */

s/it/them/

> +      int padding = (sizeof (long) - register_size (regcache->tdesc, regno));
> +      collect_register (regcache, regno, buf + padding);

Please leave an empty newline between the declaration of the variable
and the rest of the code.

> +    }
>    else
> -    collect_register (regcache, regno, buf);
> +    perror_with_name ("Unexpected byte order");
>  }
>  
>  static void
>  ppc_supply_ptrace_register (struct regcache *regcache,
>  			    int regno, const char *buf)
>  {
> -  int size = register_size (regcache->tdesc, regno);
> -  if (size < sizeof (long))
> -    supply_register (regcache, regno, buf + sizeof (long) - size);
> +  if (__BYTE_ORDER == __LITTLE_ENDIAN)
> +    {
> +      /* Little-endian values always sit at the left end of the buffer.  */
> +      supply_register (regcache, regno, buf);
> +    }
> +  else if (__BYTE_ORDER == __BIG_ENDIAN)
> +    {
> +      /* Big-endian values sit at the right end of the buffer. In case of
> +         registers whose size is smaller than sizeof (long), we must use a
> +         padding to access it correctly.  */
> +      int padding = (sizeof (long) - register_size (regcache->tdesc, regno));
> +      supply_register (regcache, regno, buf + padding);
> +    }
>    else
> -    supply_register (regcache, regno, buf);
> +    perror_with_name ("Unexpected byte order");
>  }

Same applies for this chunk.

Otherwise, looks good (it's not an approval).

Cheers,

-- 
Sergio
GPG key ID: 0x65FC5E36
Please send encrypted e-mail if possible
http://sergiodj.net/