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

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

[Edjunior Barbosa Machado]

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?

Thanks and regards,
--
Edjunior

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)
+    {
+      /* 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
+         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));
+      collect_register (regcache, regno, buf + padding);
+    }
   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");
 }
 
 
-- 
1.7.9.5

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

[Sergio Durigan Junior]

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/

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

[Ulrich Weigand]

Sergio Durigan Junior wrote:
> On Friday, September 05 2014, Edjunior Barbosa Machado wrote:
> > 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.

> > +  if (__BYTE_ORDER == __LITTLE_ENDIAN)
> 
> Why not use gdbarch_byte_order here?  We don't use __BYTE_ORDER anywhere
> in the code.

Well, this is gdbserver code, so there is no gdbarch ...

In gdbserver, we usually check for host properties, so the above check
seems fine to me.

> Same applies for this chunk.
> 
> Otherwise, looks good (it's not an approval).

I agree with the rest of Sergio's comments.

However, there is one additional problem:

>+      /* 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));
>+      collect_register (regcache, regno, buf + padding);

This will be wrong for registers larger than "long", e.g. vector registers.
The old code handled them correctly, but this new code does not.


Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

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

[Edjunior Barbosa Machado]

Thanks Sergio and Ulrich for the review. Here is the patch with the suggested
modifications.

Thanks and regards,
--
Edjunior

gdb/gdbserver/
2014-09-08  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 |   45 ++++++++++++++++++++++++++++++++---------
 1 file changed, 36 insertions(+), 9 deletions(-)

diff --git a/gdb/gdbserver/linux-ppc-low.c b/gdb/gdbserver/linux-ppc-low.c
index d743311..8fd4b38 100644
--- a/gdb/gdbserver/linux-ppc-low.c
+++ b/gdb/gdbserver/linux-ppc-low.c
@@ -202,25 +202,52 @@ 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)
+    {
+      /* 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
+         registers whose sizes are smaller than sizeof (long), we must use a
+         padding to access them correctly.  */
+      int size = register_size (regcache->tdesc, regno);
+
+      if (size < sizeof (long))
+	collect_register (regcache, regno, buf + sizeof (long) - size);
+      else
+	collect_register (regcache, regno, buf);
+    }
   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 sizes are smaller than sizeof (long), we must use a
+         padding to access them correctly.  */
+      int size = register_size (regcache->tdesc, regno);
+
+      if (size < sizeof (long))
+	supply_register (regcache, regno, buf + sizeof (long) - size);
+      else
+	supply_register (regcache, regno, buf);
+    }
   else
-    supply_register (regcache, regno, buf);
+    perror_with_name ("Unexpected byte order");
 }
 
 
-- 
1.7.9.5

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

[Ulrich Weigand]

Edjunior Barbosa Machado wrote:

> gdb/gdbserver/
> 2014-09-08  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.

This is OK.

Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

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

[Edjunior Barbosa Machado]

On 09/08/2014 11:43 AM, Ulrich Weigand wrote:
> Edjunior Barbosa Machado wrote:
> 
>> gdb/gdbserver/
>> 2014-09-08  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.
> 
> This is OK.
> 
> Thanks,
> Ulrich
> 

Thanks! I've checked in:

https://sourceware.org/ml/gdb-cvs/2014-09/msg00022.html
--
Edjunior

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

[Sergio Durigan Junior]

On Monday, September 08 2014, Ulrich Weigand wrote:

>> Why not use gdbarch_byte_order here?  We don't use __BYTE_ORDER anywhere
>> in the code.
>
> Well, this is gdbserver code, so there is no gdbarch ...
>
> In gdbserver, we usually check for host properties, so the above check
> seems fine to me.

Ouch, missed that, sorry.

> However, there is one additional problem:
>
>>+      /* 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));
>>+      collect_register (regcache, regno, buf + padding);
>
> This will be wrong for registers larger than "long", e.g. vector registers.
> The old code handled them correctly, but this new code does not.

This part seemed "strange" to me as well, but then I figured you guys
know more about PPC than I do :-).

Anyway, thanks for the corrections, Ulrich.

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