On 02/11/2016 10:12 AM, Yao Qi wrote:> Pedro Alves <palves@redhat.com> writes:
>
>> The issue you noticed exposed that regcache_raw_read_unsigned function
>> is broken for memcpy'ing a 32-bit value into a 64-bit variable, and I think
>> your patch papered over the problem for little endian, only.
>
> regcache_raw_read_unsigned has two orthogonal issues, one is VAL isn't
> cleared, and the other one is the endianness issue.  My "Clear *VAL"
> patch fixes the former, and I didn't realize the latter then.

I guess you could see it that way, though the way I imagine fixing this
handles both issues as one.

> diff --git a/gdb/gdbserver/regcache.c b/gdb/gdbserver/regcache.c
> index 2af8e24..d69ed5b 100644
> --- a/gdb/gdbserver/regcache.c
> +++ b/gdb/gdbserver/regcache.c
> @@ -21,6 +21,8 @@
>  #include "gdbthread.h"
>  #include "tdesc.h"
>  #include "rsp-low.h"
> +#include <endian.h>
> +
>  #ifndef IN_PROCESS_AGENT
>  
>  struct regcache *
> @@ -441,7 +443,27 @@ regcache_raw_read_unsigned (struct regcache *regcache, int regnum,
>            (int) sizeof (ULONGEST));
>  
>    *val = 0;
> -  collect_register (regcache, regnum, val);
> +
> +  if (__BYTE_ORDER == __LITTLE_ENDIAN)
> +    {
> +      /* Little-endian values always sit at the left end of the buffer.  */
> +      collect_register (regcache, regnum, val);
> +    }
> +  else if (__BYTE_ORDER == __BIG_ENDIAN)
> +    {
> +      /* Big-endian values sit at the right end of the buffer.  In case of
> +	 registers whose sizes are smaller than sizeof (long), we must use a
> +	 padding to access them correctly.  */
> +      int size = register_size (regcache->tdesc, regnum);
> +
> +      if (size < sizeof (ULONGEST))
> +	{
> +	  collect_register (regcache, regnum,
> +			    (char *) val + sizeof (ULONGEST) - size);
> +	}
> +      else
> +	collect_register (regcache, regnum, val);
> +    }

I was thinking we'd just use properly sized types, and the let the
compiler do the zero extension:

uint8_t u8;
uint16_t u16;
uint32_t u32;
uint64_t u64;

switch (size)
{
   case 1:
     collect_register (regcache, regnum, &u8);
     *val = u8;
     break;
   case 2:
     collect_register (regcache, regnum, &u16);
     *val = u16;
     break;
   case 4:
     collect_register (regcache, regnum, &u32);
     *val = u32;
     break;
   case 8:
     collect_register (regcache, regnum, &u64);
     *val = u64;
     break;
}

This should work in either endianess, and the '*val = 0' is no longer
necessary either.

Or maybe better, just byte the bullet and make gdbserver use
extract_unsigned_integer, like gdb.

The problem with that is that gdbserver can't currently use 'enum bfd_endian',
which IIRC, was already an issue in the get-next-pcs stuff.  I've attached a
patch series that handles that by moving bfd_endian to a separate header.
I've pushed it to the users/palves/gdbserver-extract-unsigned-integer branch
as well.

If you agree with this, I'll run the bfd_endian patch by the binutils folks.

Thanks,
Pedro Alves