[patch] Handle return small struct in rs600 (size is not 4/8)

[patch] Handle return small struct in rs600 (size is not 4/8)

[Yao Qi]

[-- Attachment #1: Type: text/plain, Size: 1276 bytes --]

Hi,
It looks to me that ppc-sysv-tdep.c:do_ppc_sysv_return_value doesn't
consider the case that returning a small struct (size <= 8) whose size
is not 4 or 8.

Supposing we have a struct defined as below,

struct C
{char c1; char c2; char c3;};
struct C c;
c.c1 = 'a'; c.c2 = 'b'; c.c3 = 'c';

The raw memory content of c is 0x616263XX (big-endian) or 0xXX636261
(little-endian).  When returning c, according to Power Arch ABI:
"Aggregates or unions whose size is less than or equal to eight bytes
shall be returned in r3 and r4, as if they were first stored in memory
area and then the low-addressed word were loaded in r3 and the
high-addressed word were loaded into r4.", the content of r3 should be
0x616263 (big-endian) or 0x636261 (little-endian).

When gdb reads r3's content via regcache_cooked_read into a buf, the
content of buf looks like this,
           buf:  [0] [1] [2] [3]
big-endian    :  00  61  62  63
little-endian :  61  62  63  00

later, when we copy the contents of buf to readbuf, we should skip 00.
Current code in gdb doesn't consider this, but it works in
little-endian.  This patch is going to fix this issue.

Regression tested on a powerpc variant board.  Many fails in
gdb.base/structs.exp are fixed.  Is this patch OK?

-- 
Yao (齐尧)

[-- Attachment #2: 0001-return_value-read-write-endianess.patch --]
[-- Type: text/x-patch, Size: 1889 bytes --]


	gdb/
	* ppc-sysv-tdep.c (do_ppc_sysv_return_value): Handle return small struct whose size is not 4 or 8.
---
 gdb/ppc-sysv-tdep.c |   16 ++++++++++++++--
 1 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/gdb/ppc-sysv-tdep.c b/gdb/ppc-sysv-tdep.c
index afe3bd2..48afc7a 100644
--- a/gdb/ppc-sysv-tdep.c
+++ b/gdb/ppc-sysv-tdep.c
@@ -795,12 +795,18 @@ do_ppc_sysv_return_value (struct gdbarch *gdbarch, struct value *function,
 	  /* The value is right-padded to 8 bytes and then loaded, as
 	     two "words", into r3/r4.  */
 	  gdb_byte regvals[MAX_REGISTER_SIZE * 2];
+	  int offset = (2 * tdep->wordsize - TYPE_LENGTH (type)) % tdep->wordsize;
+
 	  regcache_cooked_read (regcache, tdep->ppc_gp0_regnum + 3,
 				regvals + 0 * tdep->wordsize);
 	  if (TYPE_LENGTH (type) > tdep->wordsize)
 	    regcache_cooked_read (regcache, tdep->ppc_gp0_regnum + 4,
 				  regvals + 1 * tdep->wordsize);
-	  memcpy (readbuf, regvals, TYPE_LENGTH (type));
+
+	  if (byte_order == BFD_ENDIAN_BIG)
+	    memcpy (readbuf, regvals + offset, TYPE_LENGTH (type));
+	  else
+	    memcpy (readbuf, regvals, TYPE_LENGTH (type));
 	}
       if (writebuf)
 	{
@@ -808,8 +814,14 @@ do_ppc_sysv_return_value (struct gdbarch *gdbarch, struct value *function,
 	  /* The value is padded out to 8 bytes and then loaded, as
 	     two "words" into r3/r4.  */
 	  gdb_byte regvals[MAX_REGISTER_SIZE * 2];
+	  int offset = (2 * tdep->wordsize - TYPE_LENGTH (type)) % tdep->wordsize;
+
 	  memset (regvals, 0, sizeof regvals);
-	  memcpy (regvals, writebuf, TYPE_LENGTH (type));
+	  if (byte_order == BFD_ENDIAN_BIG)
+	    memcpy (regvals + offset, writebuf, TYPE_LENGTH (type));
+	  else
+	    memcpy (regvals, writebuf, TYPE_LENGTH (type));
+
 	  regcache_cooked_write (regcache, tdep->ppc_gp0_regnum + 3,
 				 regvals + 0 * tdep->wordsize);
 	  if (TYPE_LENGTH (type) > tdep->wordsize)
-- 
1.7.0.4

Re: [patch] Handle return small struct in ppc (size is not 4/8)

[Yao Qi]

I think the accurate arch name is `ppc' rather than `rs6000' in subject.
 The rest of mail and patch is unchanged.

-- 
Yao (齐尧)

Re: [patch] Handle return small struct in rs600 (size is not 4/8)

[Mark Kettenis]

> X-SWARE-Spam-Status: No, hits=-2.3 required=5.0	tests=AWL,BAYES_00,RP_MATCHES_RCVD,TW_CP,TW_EG
> Date: Mon, 15 Aug 2011 23:22:17 +0800
> From: Yao Qi <yao@codesourcery.com>
> 
> Hi,
> It looks to me that ppc-sysv-tdep.c:do_ppc_sysv_return_value doesn't
> consider the case that returning a small struct (size <= 8) whose size
> is not 4 or 8.
> 
> Supposing we have a struct defined as below,
> 
> struct C
> {char c1; char c2; char c3;};
> struct C c;
> c.c1 = 'a'; c.c2 = 'b'; c.c3 = 'c';
> 
> The raw memory content of c is 0x616263XX (big-endian) or 0xXX636261
> (little-endian).  When returning c, according to Power Arch ABI:
> "Aggregates or unions whose size is less than or equal to eight bytes
> shall be returned in r3 and r4, as if they were first stored in memory
> area and then the low-addressed word were loaded in r3 and the
> high-addressed word were loaded into r4.", the content of r3 should be
> 0x616263 (big-endian) or 0x636261 (little-endian).

That's not how I read the ABI.  If you store that struct in a
zero-initialized 8-byte buffer you'll have the following sequence of 8
bytes:

0x61 0x62 0x63 0x00 0x00 0x00 0x00 0x00

Viewed as two big-endian words this becomes:

0x61626300 0x00000000

and as two little-endian words this becomes:

0x00636261 0x00000000

So in the little-endian case r3 will indeed be 0x636261 like you say,
but in the big-endian case r3 will be 0x61626300.

> When gdb reads r3's content via regcache_cooked_read into a buf, the
> content of buf looks like this,
>            buf:  [0] [1] [2] [3]
> big-endian    :  00  61  62  63
> little-endian :  61  62  63  00

If that's really what you're seeing, then GCC must not implement this
part of the ABI correctly.  Not really surprising since GCC has a long
history of getting corner cases like this wrong.

Now the question is if this just happens to be broken in the
particular version of GCC you're using or whether this has always been
broken.  Eh, wait a moment...

...what version of GDB are you looking at?  The current code in
ppc-sysv-tdep.c already handles the broken way GCC implements this.
Just make sure your target uses ppc_sysv_abi_broken_return_value()
instead of ppc_sysv_abi_return_value().  The NetBSD/powerpc and
OpenBSD/powerpc targets already do this.  Guessing that you're on
Linux and that GCC is the primary compiler on that platform, it
probably needs that same treatment.

> Regression tested on a powerpc variant board.  Many fails in
> gdb.base/structs.exp are fixed.  Is this patch OK?

So no, I'd say this isn't ok.

Re: [patch] Handle return small struct in rs600 (size is not 4/8)

[Yao Qi]

On 08/16/2011 12:05 AM, Mark Kettenis wrote:
>> X-SWARE-Spam-Status: No, hits=-2.3 required=5.0	tests=AWL,BAYES_00,RP_MATCHES_RCVD,TW_CP,TW_EG
>> Date: Mon, 15 Aug 2011 23:22:17 +0800
>> From: Yao Qi <yao@codesourcery.com>
>>
>> Hi,
>> It looks to me that ppc-sysv-tdep.c:do_ppc_sysv_return_value doesn't
>> consider the case that returning a small struct (size <= 8) whose size
>> is not 4 or 8.
>>
>> Supposing we have a struct defined as below,
>>
>> struct C
>> {char c1; char c2; char c3;};
>> struct C c;
>> c.c1 = 'a'; c.c2 = 'b'; c.c3 = 'c';
>>
>> The raw memory content of c is 0x616263XX (big-endian) or 0xXX636261
>> (little-endian).  When returning c, according to Power Arch ABI:
>> "Aggregates or unions whose size is less than or equal to eight bytes
>> shall be returned in r3 and r4, as if they were first stored in memory
>> area and then the low-addressed word were loaded in r3 and the
>> high-addressed word were loaded into r4.", the content of r3 should be
>> 0x616263 (big-endian) or 0x636261 (little-endian).
> 
> That's not how I read the ABI.  If you store that struct in a
> zero-initialized 8-byte buffer you'll have the following sequence of 8
> bytes:
> 
> 0x61 0x62 0x63 0x00 0x00 0x00 0x00 0x00
> 
> Viewed as two big-endian words this becomes:
> 
> 0x61626300 0x00000000
> 
> and as two little-endian words this becomes:
> 
> 0x00636261 0x00000000

Yea, that looks right to me.

> 
> So in the little-endian case r3 will indeed be 0x636261 like you say,
> but in the big-endian case r3 will be 0x61626300.
> 

Looks like we have different interpretation to ABI doc here.  Here is
the doc,

"Aggregates or unions whose size is less than or equal to eight bytes
shall be returned in r3 and r4, as if they were first stored in memory
area and then the low-addressed word were loaded in r3 and the
high-addressed word were loaded into r4."

I think the description "shall be returned in r3 and r4, as if they were
first stored in memory area and then the low-addressed word were loaded
in r3 ...." is not very clear on the length of data.  In this case,
struct C variable is returned, and its content is 0x61 0x62 0x63.  They
(3 bytes) are stored in memory, and (3 bytes) are loaded into r3.  Since
3 bytes, not 4, are loaded to r3, so I believe r3 should be 0x616263,
instead of 0x61626300.

>> When gdb reads r3's content via regcache_cooked_read into a buf, the
>> content of buf looks like this,
>>            buf:  [0] [1] [2] [3]
>> big-endian    :  00  61  62  63
>> little-endian :  61  62  63  00
> 
> If that's really what you're seeing, then GCC must not implement this
> part of the ABI correctly.  Not really surprising since GCC has a long
> history of getting corner cases like this wrong.
> 

The big-endian case is what I see.

> Now the question is if this just happens to be broken in the
> particular version of GCC you're using or whether this has always been
> broken.  Eh, wait a moment...
> 

Yeah, that makes sense.  I'll have to read gcc to see how gcc does.

-- 
Yao (齐尧)

Re: [patch] Handle return small struct in rs600 (size is not 4/8)

[Pedro Alves]

On Monday 15 August 2011 17:56:09, Yao Qi wrote:
> I think the description "shall be returned in r3 and r4, as if they were
> first stored in memory area and then the low-addressed word were loaded
> in r3 ...." is not very clear on the length of data.  In this case,
> struct C variable is returned, and its content is 0x61 0x62 0x63.  They
> (3 bytes) are stored in memory, and (3 bytes) are loaded into r3.  Since
> 3 bytes, not 4, are loaded to r3, so I believe r3 should be 0x616263,
> instead of 0x61626300.

I agree with Mark's interpretation (FWIW).

By my reading, for a 3-byte struct c, if the raw memory content of c stored
at AS-IF-ADDRESS in memory is 0x616263XX, then, when you load the
contents of the low-addressed word (4 bytes) starting at AS-IF-ADDRESS
to r3, you get 0x616263XX in r3.

For a 5-byte struct c, if the raw memory content of c at AS-IF-ADDRESS
is 0x6162636465XXXXXX, then, when you load the contents of the
low-addressed word starting at AS-IF-ADDRESS to r3, you
get 0x61626364 in r3; and then loading the contents of high-word at
AS-IF-ADDRESS gets you 0x65XXXXXX in r4.

-- 
Pedro Alves

Re: [patch] Handle return small struct in rs600 (size is not 4/8)

[Andreas Schwab]

Yao Qi <yao@codesourcery.com> writes:

> I think the description "shall be returned in r3 and r4, as if they were
> first stored in memory area and then the low-addressed word were loaded
> in r3 ...." is not very clear on the length of data.  In this case,
> struct C variable is returned, and its content is 0x61 0x62 0x63.  They
> (3 bytes) are stored in memory, and (3 bytes) are loaded into r3.  Since
> 3 bytes, not 4, are loaded to r3

When you load a (32-bit) register from memory, you always read 4 bytes.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [patch] Handle return small struct in rs600 (size is not 4/8)

[Yao Qi]

On 08/16/2011 02:54 AM, Andreas Schwab wrote:
> Yao Qi <yao@codesourcery.com> writes:
> 
>> I think the description "shall be returned in r3 and r4, as if they were
>> first stored in memory area and then the low-addressed word were loaded
>> in r3 ...." is not very clear on the length of data.  In this case,
>> struct C variable is returned, and its content is 0x61 0x62 0x63.  They
>> (3 bytes) are stored in memory, and (3 bytes) are loaded into r3.  Since
>> 3 bytes, not 4, are loaded to r3
> 
> When you load a (32-bit) register from memory, you always read 4 bytes.
> 

Andreas, thanks for your clarification.  Please ignore my patch, and
sorry for the noise.

-- 
Yao (齐尧)

Re: [patch] Handle return small struct in rs600 (size is not 4/8)

[Andreas Schwab]

Mark Kettenis <mark.kettenis@xs4all.nl> writes:

> ...what version of GDB are you looking at?  The current code in
> ppc-sysv-tdep.c already handles the broken way GCC implements this.
> Just make sure your target uses ppc_sysv_abi_broken_return_value()
> instead of ppc_sysv_abi_return_value().  The NetBSD/powerpc and
> OpenBSD/powerpc targets already do this.  Guessing that you're on
> Linux and that GCC is the primary compiler on that platform, it
> probably needs that same treatment.

I don't see any failures in gdb.base/structs.exp on ppc-linux or
ppc64-linux.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."