Structure-return bug on powerpc32

Structure-return bug on powerpc32

[Wim Lewis]

I've been looking into some libffi test failures on netbsd/ppc. To  
make a long story short, under some circumstances (returning a small  
aggregate that's not a multiple of 4 bytes), ffi_call() will write  
past the end of the buffer passed to rvalue.

In the cls_6byte.c test, this manifests itself as the first field in  
g_dbl getting mysteriously set to zero after the call to ffi_call()  
in main, since g_dbl is allocated after res_dbl with no padding.  
(This surprised me a little, but the debugger confirms that &g_dbl is  
six bytes higher than &res_dbl.) As a result, the *next* test in that  
file will fail, since g_dbl no longer has the expected value.

The actual clobbering takes place at this point in src/powerpc/sysv.S:

 > L(smst_8byte):
 >         stw     %r3,0(%r30)
 >         stw     %r4,4(%r30)
 >         b       L(done_return_value)

where r30 contains 'rvalue'. This ends up writing 8 bytes even for a  
6-byte return struct.

I'm not familiar enough with ppc assembly to suggest a patch; I'm not  
really sure what the code immediately before the smst_8byte label is  
doing. One idea is to do a read-modify-write and use insrwi/rlwinm to  
modify only the portion of the word containing the returned struct.  
But the struct may be aligned at the end of a memory region, so maybe  
the only correct implementation for oddly-sized aggregates is a  
sequence of rotates and stswi or stbu instructions.


regards
    Wim Lewis / wiml@hhhh.org

Re: Structure-return bug on powerpc32

[Andreas Tobler]

Wim Lewis wrote:
> I've been looking into some libffi test failures on netbsd/ppc. To make 
> a long story short, under some circumstances (returning a small 
> aggregate that's not a multiple of 4 bytes), ffi_call() will write past 
> the end of the buffer passed to rvalue.
> 
> In the cls_6byte.c test, this manifests itself as the first field in 
> g_dbl getting mysteriously set to zero after the call to ffi_call() in 
> main, since g_dbl is allocated after res_dbl with no padding. (This 
> surprised me a little, but the debugger confirms that &g_dbl is six 
> bytes higher than &res_dbl.) As a result, the *next* test in that file 
> will fail, since g_dbl no longer has the expected value.
> 
> The actual clobbering takes place at this point in src/powerpc/sysv.S:
> 
>  > L(smst_8byte):
>  >         stw     %r3,0(%r30)
>  >         stw     %r4,4(%r30)
>  >         b       L(done_return_value)
> 
> where r30 contains 'rvalue'. This ends up writing 8 bytes even for a 
> 6-byte return struct.
> 
> I'm not familiar enough with ppc assembly to suggest a patch; I'm not 
> really sure what the code immediately before the smst_8byte label is 
> doing. One idea is to do a read-modify-write and use insrwi/rlwinm to 
> modify only the portion of the word containing the returned struct. But 
> the struct may be aligned at the end of a memory region, so maybe the 
> only correct implementation for oddly-sized aggregates is a sequence of 
> rotates and stswi or stbu instructions.
> 
> 
> regards
>    Wim Lewis / wiml@hhhh.org

Which ABI does netbsd/ppc use? The original SYSV or the same as Linux? 
If it is the original SYSV you might try to build like FreeBSD does.

Andreas

Re: Structure-return bug on powerpc32

[Wim Lewis]

On May 22, 2009, at 12:55 PM, Andreas Tobler wrote:
> Wim Lewis wrote:
>> I've been looking into some libffi test failures on netbsd/ppc. To  
>> make a long story short, under some circumstances (returning a  
>> small aggregate that's not a multiple of 4 bytes), ffi_call() will  
>> write past the end of the buffer passed to rvalue.  [....]
> Which ABI does netbsd/ppc use? The original SYSV or the same as  
> Linux? If it is the original SYSV you might try to build like  
> FreeBSD does.

NetBSD uses the same ABI as FreeBSD, as far as I know --- in fact,  
it's building libffi with TARGET=POWERPC_FREEBSD. :) The other tests  
which depend on the difference between the SYSV and "GCC" ABIs (small  
aggregates returned in registers vs. a hidden pointer argument) all  
pass. And the actual returned value is correct --- it's just stomping  
on some nearby memory as well.

The bug is not with the procedure call convention itself, but with  
the way that the return value is copied from registers into the  
buffer passed to ffi_call(). Once I found the relevant code in  
powerpc/sysv.S, it was pretty clear that it will write past the end  
of its buffer in situations like this. I'm surprised that the bug  
doesn't occur on FreeBSD as well, but perhaps there's some subtle  
point I'm missing.

I've written up a test case which should detect the problem even if  
the compiler doesn't happen to put something important immediately  
after res_dbl. It fails in the expected way on my netbsd5/ppc  
machine, and passes on the other systems I have handy (openbsd4.4/ 
i386, darwin9/i386, darwin9/x86_64). It also passes on darwin8/ppc,  
apparently because the darwin calling convention is to return even  
small aggregates using a hidden pointer argument.

Here's the test program:
     http://www.hhhh.org/wiml/tmp/odd_struct.c

It'd be interesting to run it on an architecture with more strict  
alignment requirements as well, like MIPS or SPARC.


regards
    Wim Lewis / wiml@hhhh.org

Re: Structure-return bug on powerpc32 (patch)

[Wim Lewis]

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

Here's a patch which fixes the bug. It also fixes an unrelated
problem I noticed in that section of code, which was that the structure-
return code was clobbering cr3 and cr4, which are designated as callee-saved
in the ppc abi documentation I have. (I've never seen a compiler actually
use those fields, but still.) This involved moving some of the bitfields
in the flags word around.

With this patch, libffi-3.0.8 passes four of the five tests which were
failing on netbsd5/ppc32 (the fifth test failure is an unrelated problem),
as well as the odd_struct.c tests I linked to in my last message.

-- 
   Wim Lewis <wiml@hhhh.org>, Seattle, WA, USA. PGP keyID 27F772C1

[-- Attachment #2: smst_buffer_overrun.patch --]
[-- Type: text/plain, Size: 4358 bytes --]

diff -c --recursive libffi-3.0.8-pristine/ChangeLog libffi-3.0.8/ChangeLog
*** libffi-3.0.8-pristine/ChangeLog	Fri Dec 19 08:06:04 2008
--- libffi-3.0.8/ChangeLog	Fri May 29 13:01:49 2009
***************
*** 1,3 ****
--- 1,10 ----
+ 2009-05-29  Wim Lewis  <wiml@hhhh.org>
+ 
+ 	* src/powerpc/sysv.S (small_struct_return_value): Fix overrun of
+ 	return buffer for odd-size structs.
+ 	* src/powerpc/ffi.c, src/powerpc/sysv.S: Avoid clobbering cr3 and cr4,
+ 	which are supposed to be callee-saved.
+ 
  2008-12-18  Rainer Orth  <ro@TechFak.Uni-Bielefeld.DE>
  
  	PR libffi/26048
diff -c --recursive libffi-3.0.8-pristine/src/powerpc/ffi.c libffi-3.0.8/src/powerpc/ffi.c
*** libffi-3.0.8-pristine/src/powerpc/ffi.c	Wed Nov 12 11:46:10 2008
--- libffi-3.0.8/src/powerpc/ffi.c	Fri May 29 12:30:34 2009
***************
*** 43,53 ****
    FLAG_RETURNS_64BITS   = 1 << (31-28),
  
    FLAG_RETURNS_128BITS  = 1 << (31-27), /* cr6  */
! 
!   FLAG_SYSV_SMST_R4     = 1 << (31-16), /* cr4, use r4 for FFI_SYSV 8 byte
  					   structs.  */
!   FLAG_SYSV_SMST_R3     = 1 << (31-15), /* cr3, use r3 for FFI_SYSV 4 byte
  					   structs.  */
    FLAG_ARG_NEEDS_COPY   = 1 << (31- 7),
    FLAG_FP_ARGUMENTS     = 1 << (31- 6), /* cr1.eq; specified by ABI */
    FLAG_4_GPR_ARGUMENTS  = 1 << (31- 5),
--- 43,54 ----
    FLAG_RETURNS_64BITS   = 1 << (31-28),
  
    FLAG_RETURNS_128BITS  = 1 << (31-27), /* cr6  */
!   FLAG_SYSV_SMST_R4     = 1 << (31-26), /* use r4 for FFI_SYSV 8 byte
  					   structs.  */
!   FLAG_SYSV_SMST_R3     = 1 << (31-25), /* use r3 for FFI_SYSV 4 byte
  					   structs.  */
+   /* Bits (31-24) through (31-19) store shift value for SMST */
+ 
    FLAG_ARG_NEEDS_COPY   = 1 << (31- 7),
    FLAG_FP_ARGUMENTS     = 1 << (31- 6), /* cr1.eq; specified by ABI */
    FLAG_4_GPR_ARGUMENTS  = 1 << (31- 5),
***************
*** 685,698 ****
  	      if (size <= 4)
  		{
  		  flags |= FLAG_SYSV_SMST_R3;
! 		  flags |= 8 * (4 - size) << 4;
  		  break;
  		}
  	      /* These structs are returned in r3 and r4. See above.   */
  	      if  (size <= 8)
  		{
! 		  flags |= FLAG_SYSV_SMST_R4;
! 		  flags |= 8 * (8 - size) << 4;
  		  break;
  		}
  	    }
--- 686,699 ----
  	      if (size <= 4)
  		{
  		  flags |= FLAG_SYSV_SMST_R3;
! 		  flags |= 8 * (4 - size) << 8;
  		  break;
  		}
  	      /* These structs are returned in r3 and r4. See above.   */
  	      if  (size <= 8)
  		{
! 		  flags |= FLAG_SYSV_SMST_R3 | FLAG_SYSV_SMST_R4;
! 		  flags |= 8 * (8 - size) << 8;
  		  break;
  		}
  	    }
diff -c --recursive libffi-3.0.8-pristine/src/powerpc/sysv.S libffi-3.0.8/src/powerpc/sysv.S
*** libffi-3.0.8-pristine/src/powerpc/sysv.S	Tue Feb 26 11:01:53 2008
--- libffi-3.0.8/src/powerpc/sysv.S	Fri May 29 16:58:19 2009
***************
*** 136,165 ****
  	b	L(done_return_value)
  
  L(small_struct_return_value):
! 	mtcrf	0x10,%r31	/* cr3  */
! 	bt-	15,L(smst_one_register)
! 	mtcrf	0x08,%r31	/* cr4  */
! 	bt-	16,L(smst_two_register)
! 	b       L(done_return_value)
! 
! L(smst_one_register):
! 	rlwinm  %r5,%r31,5+23,32-5,31 /* Extract the value to shift.  */
! 	slw	%r3,%r3,%r5
! 	stw	%r3,0(%r30)
! 	b	L(done_return_value)
! L(smst_two_register):
! 	rlwinm  %r5,%r31,5+23,32-5,31 /* Extract the value to shift.  */
! 	cmpwi	%r5,0
! 	subfic	%r9,%r5,32
! 	slw	%r29,%r3,%r5
! 	srw	%r9,%r4,%r9
! 	beq-	L(smst_8byte)
! 	or	%r3,%r9,%r29
! 	slw	%r4,%r4,%r5
! L(smst_8byte):
! 	stw	%r3,0(%r30)
! 	stw	%r4,4(%r30)
  	b	L(done_return_value)
  
  .LFE1:
  END(ffi_call_SYSV)
--- 136,154 ----
  	b	L(done_return_value)
  
  L(small_struct_return_value):
! 	extrwi	%r6,%r31,2,19         /* number of bytes padding = shift/8 */
! 	mtcrf	0x02,%r31	      /* copy flags to cr[24:27] (cr6) */
! 	extrwi	%r5,%r31,5,19         /* r5 <- number of bits of padding */
! 	subfic  %r6,%r6,4             /* r6 <- number of useful bytes in r3 */
! 	bf-	25,L(done_return_value) /* struct in r3 ? if not, done. */
! /* smst_one_register: */
! 	slw	%r3,%r3,%r5           /* Left-justify value in r3 */
! 	mtxer	%r6                   /* move byte count to XER ... */
! 	stswx	%r3,0,%r30            /* ... and store that many bytes */
! 	bf+	26,L(done_return_value)  /* struct in r3:r4 ? */
! 	add	%r6,%r6,%r30          /* adjust pointer */
! 	stswi	%r4,%r6,4             /* store last four bytes */
  	b	L(done_return_value)
  
  .LFE1:
  END(ffi_call_SYSV)

Re: Structure-return bug on powerpc32 (patch)

[Andreas Tobler]

Wim Lewis wrote:
> Here's a patch which fixes the bug. It also fixes an unrelated
> problem I noticed in that section of code, which was that the structure-
> return code was clobbering cr3 and cr4, which are designated as callee-saved
> in the ppc abi documentation I have. (I've never seen a compiler actually
> use those fields, but still.) This involved moving some of the bitfields
> in the flags word around.
> 
> With this patch, libffi-3.0.8 passes four of the five tests which were
> failing on netbsd5/ppc32 (the fifth test failure is an unrelated problem),
> as well as the odd_struct.c tests I linked to in my last message.

Thank you Wim!

I'll get this patch into the gcc repo, I already have the approval.

The use of mtxer and stswx is a bit costy.

AG needs to sync the repo once I committed it to gcc.

Regards,
Andreas

Re: Structure-return bug on powerpc32 (patch)

[Anthony Green]

Andreas Tobler wrote:
> Wim Lewis wrote:
>> Here's a patch which fixes the bug. It also fixes an unrelated
>> problem I noticed in that section of code, which was that the structure-
>> return code was clobbering cr3 and cr4, which are designated as 
>> callee-saved
>> in the ppc abi documentation I have. (I've never seen a compiler 
>> actually
>> use those fields, but still.) This involved moving some of the bitfields
>> in the flags word around.
>>
>> With this patch, libffi-3.0.8 passes four of the five tests which were
>> failing on netbsd5/ppc32 (the fifth test failure is an unrelated 
>> problem),
>> as well as the odd_struct.c tests I linked to in my last message.
>
> Thank you Wim!
>
> I'll get this patch into the gcc repo, I already have the approval.
>
> The use of mtxer and stswx is a bit costy.
>
> AG needs to sync the repo once I committed it to gcc.
I'll watch for the commit.

Thanks Andreas.

AG



>
> Regards,
> Andreas
>

Re: Structure-return bug on powerpc32 (patch)

[Wim Lewis]

On Jun 1, 2009, at 12:44 PM, Andreas Tobler wrote:
> I'll get this patch into the gcc repo, I already have the approval.

Thanks!

> The use of mtxer and stswx is a bit costy.

Hmm. Are we talking "pipeline stall" costly or "kernel trap" costly?  
My ppc docs are maddeningly vague on this point. Would it be faster  
as an explicit rotate/stbu/bdnz loop?