[PATCH] Fix PR32843, testsuite bug wrt sign/zero-extension of return values

[PATCH] Fix PR32843, testsuite bug wrt sign/zero-extension of return values

[Richard Guenther]

This patch:

2007-07-09  Richard Guenther  <rguenther@suse.de>

	cp/
	* decl.c (start_preparsed_function): Do not promote return type.

	* c-decl.c (start_function): Do not promote return type.

changed how we extend a signed char to fill %eax on x86 from sign to
zero extension which now makes libffi.call/return_sc.c fail because
this testcase explicitly checks for sign-extension of signed character
return values.

Fixed by verifying the return value as signed char instead.

This patch was approved by Andreas Tobler in the PR audit trail and
committed to mainline.

Richard.

2007-07-26  Richard Guenther  <rguenther@suse.de>

	PR testsuite/32843
	* testsuite/libffi.call/return_sc.c (main): Verify call
	result as signed char, not ffi_arg.

Index: testsuite/libffi.call/return_sc.c
===================================================================
*** testsuite/libffi.call/return_sc.c	(revision 126677)
--- testsuite/libffi.call/return_sc.c	(working copy)
*************** int main (void)
*** 30,36 ****
         sc < (signed char) 127; sc++)
      {
        ffi_call(&cif, FFI_FN(return_sc), &rint, values);
!       CHECK(rint == (ffi_arg) sc);
      }
    exit(0);
  }
--- 30,36 ----
         sc < (signed char) 127; sc++)
      {
        ffi_call(&cif, FFI_FN(return_sc), &rint, values);
!       CHECK((signed char) rint == sc);
      }
    exit(0);
  }

Re: [PATCH] Fix PR32843, testsuite bug wrt sign/zero-extension of return values

[Paolo Bonzini]

> This patch was approved by Andreas Tobler in the PR audit trail and
> committed to mainline.

Couldn't it be also a FFI bug?  If the ABI is unclear, I would have 
expected libffi to do the cast itself...

Paolo

Re: [PATCH] Fix PR32843, testsuite bug wrt sign/zero-extension of return values

[Richard Guenther]

On Thu, 26 Jul 2007, Paolo Bonzini wrote:

> 
> > This patch was approved by Andreas Tobler in the PR audit trail and
> > committed to mainline.
> 
> Couldn't it be also a FFI bug?  If the ABI is unclear, I would have expected
> libffi to do the cast itself...

Maybe.  Still it shouldn't matter -- the only valid thing to do with
a signed char is to use it as signed char.  Everything else requires
a proper conversion.

Richard.

Re: [PATCH] Fix PR32843, testsuite bug wrt sign/zero-extension of return values

[Andrew Haley]

Richard Guenther writes:
 > On Thu, 26 Jul 2007, Paolo Bonzini wrote:
 > 
 > > 
 > > > This patch was approved by Andreas Tobler in the PR audit trail and
 > > > committed to mainline.
 > > 
 > > Couldn't it be also a FFI bug?  If the ABI is unclear, I would have expected
 > > libffi to do the cast itself...
 > 
 > Maybe.  Still it shouldn't matter -- the only valid thing to do with
 > a signed char is to use it as signed char.  Everything else requires
 > a proper conversion.

Perhaps so, but even if this is not strictly speaking a libffi bug, it
is a libffi quality of implementation issue, and it does potentially
break code.

The intention of the testcase that has been broken is precisely to
check that signed characters are sign extended.  When this fails
libffi should be changed, not the testcase.

Andrew.

Re: [PATCH] Fix PR32843, testsuite bug wrt sign/zero-extension of return values

[Richard Guenther]

On Thu, 26 Jul 2007, Andrew Haley wrote:

> Richard Guenther writes:
>  > On Thu, 26 Jul 2007, Paolo Bonzini wrote:
>  > 
>  > > 
>  > > > This patch was approved by Andreas Tobler in the PR audit trail and
>  > > > committed to mainline.
>  > > 
>  > > Couldn't it be also a FFI bug?  If the ABI is unclear, I would have expected
>  > > libffi to do the cast itself...
>  > 
>  > Maybe.  Still it shouldn't matter -- the only valid thing to do with
>  > a signed char is to use it as signed char.  Everything else requires
>  > a proper conversion.
> 
> Perhaps so, but even if this is not strictly speaking a libffi bug, it
> is a libffi quality of implementation issue, and it does potentially
> break code.
> 
> The intention of the testcase that has been broken is precisely to
> check that signed characters are sign extended.  When this fails
> libffi should be changed, not the testcase.

If you are familiar with libffi can you try to fix this then and
revert the testsuite change?

Thanks a lot,
Richard.

Re: [PATCH] Fix PR32843, testsuite bug wrt sign/zero-extension of return values

[Andrew Haley]

Richard Guenther writes:
 > On Thu, 26 Jul 2007, Andrew Haley wrote:
 > 
 > > Richard Guenther writes:
 > >  > On Thu, 26 Jul 2007, Paolo Bonzini wrote:
 > >  > 
 > >  > > 
 > >  > > > This patch was approved by Andreas Tobler in the PR audit trail and
 > >  > > > committed to mainline.
 > >  > > 
 > >  > > Couldn't it be also a FFI bug?  If the ABI is unclear, I would have expected
 > >  > > libffi to do the cast itself...
 > >  > 
 > >  > Maybe.  Still it shouldn't matter -- the only valid thing to do with
 > >  > a signed char is to use it as signed char.  Everything else requires
 > >  > a proper conversion.
 > > 
 > > Perhaps so, but even if this is not strictly speaking a libffi bug, it
 > > is a libffi quality of implementation issue, and it does potentially
 > > break code.
 > > 
 > > The intention of the testcase that has been broken is precisely to
 > > check that signed characters are sign extended.  When this fails
 > > libffi should be changed, not the testcase.
 > 
 > If you are familiar with libffi can you try to fix this then and
 > revert the testsuite change?

Sure.

Andrew.

Re: [PATCH] Fix PR32843, testsuite bug wrt sign/zero-extension of return values

[Andrew Haley]

Andrew Haley writes:
 > Richard Guenther writes:
 >  > On Thu, 26 Jul 2007, Andrew Haley wrote:
 >  > 
 >  > > Richard Guenther writes:
 >  > >  > On Thu, 26 Jul 2007, Paolo Bonzini wrote:
 >  > >  > 
 >  > >  > > 
 >  > >  > > > This patch was approved by Andreas Tobler in the PR audit trail and
 >  > >  > > > committed to mainline.
 >  > >  > > 
 >  > >  > > Couldn't it be also a FFI bug?  If the ABI is unclear, I would have expected
 >  > >  > > libffi to do the cast itself...
 >  > >  > 
 >  > >  > Maybe.  Still it shouldn't matter -- the only valid thing to do with
 >  > >  > a signed char is to use it as signed char.  Everything else requires
 >  > >  > a proper conversion.
 >  > > 
 >  > > Perhaps so, but even if this is not strictly speaking a libffi bug, it
 >  > > is a libffi quality of implementation issue, and it does potentially
 >  > > break code.
 >  > > 
 >  > > The intention of the testcase that has been broken is precisely to
 >  > > check that signed characters are sign extended.  When this fails
 >  > > libffi should be changed, not the testcase.
 >  > 
 >  > If you are familiar with libffi can you try to fix this then and
 >  > revert the testsuite change?
 > 
 > Sure.

OK, I've looked and every libffi target except x86 has explicit code
to do sign extend retvals.  I presume that such code isn't present for
x86 because hitherto $eax has always been properly sign extended for
the return type.

So, are we *absolutely* sure that gcc hasn't broken the ABI?  I can
certainly imagine some cases where a change like this one (gcc no
longer sign extends retvals) would break code in interesting ways.

Andrew.

Re: [PATCH] Fix PR32843, testsuite bug wrt sign/zero-extension of return values

[Richard Guenther]

On Fri, 27 Jul 2007, Andrew Haley wrote:

> Andrew Haley writes:
> 
> OK, I've looked and every libffi target except x86 has explicit code
> to do sign extend retvals.  I presume that such code isn't present for
> x86 because hitherto $eax has always been properly sign extended for
> the return type.
> 
> So, are we *absolutely* sure that gcc hasn't broken the ABI?  I can
> certainly imagine some cases where a change like this one (gcc no
> longer sign extends retvals) would break code in interesting ways.

Well, if every other libffi target has explicit sign extension code
then the change (which is to generic code!) if it breaks the ABI shows
that the x86 target machinery is lacking code to do sign extend retvals.

Of course I'd like to see what other language frontends to for returning
signed/unsigned quantities of QImode or HImode on x86.  For example
Fortran does zero extension in 4.2.1 (well, really the backend is -
the frontend simply returns HImode, like mainline now does for C as well
after the change):

foo_:
        pushl   %ebp
        movl    %esp, %ebp
        movl    8(%ebp), %eax
        movzwl  (%eax), %eax
        popl    %ebp
        ret

function foo(bar)
  integer*2 foo,bar
  foo = bar
end

if backends other than x86 still sign-extend for QImode returns in C
the it is the x86 backend that needs to be fixed if we decide that
it makes a difference that we need to care about.

Richard.

Re: [PATCH] Fix PR32843, testsuite bug wrt sign/zero-extension of return values

[Andrew Haley]

This fixes the real bug, which is that x86 libffi wasn't correctly
sign/zero extending retvals.  I also changed the long chain of
if/then/elses into a switch and tidied things a bit.

While I was doing this, I found a bug in ffi_call_SYSV: in some cases
we were popping too many values from the stack, but that was being
concealed by a later movl %ebp,%esp in the epilogue.  As it turns out,
there isn't any need to pop the args from the stack after calling the
user's target function, so I just removed the code that did that.

Passes full testsuite on 32/64, no regressions.

Andrew.


2007-07-30  Andrew Haley  <aph@redhat.com>

	PR testsuite/32843
	* src/x86/ffi.c (ffi_prep_cif_machdep): in x86 case, add code for
	signed/unsigned int8/16.
	* src/x86/sysv.S (ffi_call_SYSV): Rewrite to:
	Use a jump table.
	Remove code to pop args from the stack after call.
	Special-case signed/unsigned int8/16.
	* testsuite/libffi.call/return_sc.c (main): Revert.

Index: src/x86/ffi.c
===================================================================
--- src/x86/ffi.c	(revision 127059)
+++ src/x86/ffi.c	(working copy)
@@ -121,7 +121,12 @@
     case FFI_TYPE_VOID:
 #ifdef X86
     case FFI_TYPE_STRUCT:
+    case FFI_TYPE_UINT8:
+    case FFI_TYPE_UINT16:
+    case FFI_TYPE_SINT8:
+    case FFI_TYPE_SINT16:
 #endif
+
     case FFI_TYPE_SINT64:
     case FFI_TYPE_FLOAT:
     case FFI_TYPE_DOUBLE:
Index: src/x86/sysv.S
===================================================================
--- src/x86/sysv.S	(revision 127059)
+++ src/x86/sysv.S	(working copy)
@@ -59,16 +59,15 @@
 
 	call  *28(%ebp)
 
-	/* Remove the space we pushed for the args  */
-	movl  16(%ebp),%ecx
-	addl  %ecx,%esp
-
 	/* Load %ecx with the return type code  */
 	movl  20(%ebp),%ecx	
 
+	/* Protect %esi.  We're going to pop it in the epilogue.  */
+	pushl %esi
+
 	/* If the return value pointer is NULL, assume no return value.  */
 	cmpl  $0,24(%ebp)
-	jne   retint
+	jne  0f
 
 	/* Even if there is no space for the return value, we are 
 	   obliged to handle floating-point values.  */
@@ -78,51 +77,84 @@
 
         jmp   epilogue
 
-retint:
-	cmpl  $FFI_TYPE_INT,%ecx
-	jne   retfloat
-	/* Load %ecx with the pointer to storage for the return value  */
-	movl  24(%ebp),%ecx	
-	movl  %eax,0(%ecx)
-	jmp   epilogue
+0:
+	call  1f
+
+.Lstore_table:
+	.long	noretval-.Lstore_table	/* FFI_TYPE_VOID */
+	.long	retint-.Lstore_table	/* FFI_TYPE_INT */
+	.long	retfloat-.Lstore_table	/* FFI_TYPE_FLOAT */
+	.long	retdouble-.Lstore_table	/* FFI_TYPE_DOUBLE */
+	.long	retlongdouble-.Lstore_table	/* FFI_TYPE_LONGDOUBLE */
+	.long	retuint8-.Lstore_table	/* FFI_TYPE_UINT8 */
+	.long	retsint8-.Lstore_table	/* FFI_TYPE_SINT8 */
+	.long	retuint16-.Lstore_table	/* FFI_TYPE_UINT16 */
+	.long	retsint16-.Lstore_table	/* FFI_TYPE_SINT16 */
+	.long	retint-.Lstore_table	/* FFI_TYPE_UINT32 */
+	.long	retint-.Lstore_table	/* FFI_TYPE_SINT32 */
+	.long	retint64-.Lstore_table	/* FFI_TYPE_UINT64 */
+	.long	retint64-.Lstore_table	/* FFI_TYPE_SINT64 */
+	.long	retstruct-.Lstore_table	/* FFI_TYPE_STRUCT */
+	.long	retint-.Lstore_table	/* FFI_TYPE_POINTER */
+
+1:
+	pop  %esi
+	add  (%esi, %ecx, 4), %esi
+	jmp  *%esi
+
+	/* Sign/zero extend as appropriate.  */
+retsint8:
+	movsbl  %al, %eax
+	jmp  retint
+
+retsint16:
+	movswl  %ax, %eax
+	jmp  retint
+
+retuint8:
+	movzbl  %al, %eax
+	jmp  retint
+
+retuint16:
+	movzwl  %ax, %eax
+	jmp  retint
 
 retfloat:
-	cmpl  $FFI_TYPE_FLOAT,%ecx
-	jne   retdouble
 	/* Load %ecx with the pointer to storage for the return value  */
 	movl  24(%ebp),%ecx	
 	fstps (%ecx)
 	jmp   epilogue
 
 retdouble:
-	cmpl  $FFI_TYPE_DOUBLE,%ecx
-	jne   retlongdouble
 	/* Load %ecx with the pointer to storage for the return value  */
 	movl  24(%ebp),%ecx	
 	fstpl (%ecx)
 	jmp   epilogue
 
 retlongdouble:
-	cmpl  $FFI_TYPE_LONGDOUBLE,%ecx
-	jne   retint64
 	/* Load %ecx with the pointer to storage for the return value  */
 	movl  24(%ebp),%ecx	
 	fstpt (%ecx)
 	jmp   epilogue
 	
 retint64:	
-	cmpl  $FFI_TYPE_SINT64,%ecx
-        jne   retstruct
 	/* Load %ecx with the pointer to storage for the return value  */
 	movl  24(%ebp),%ecx	
 	movl  %eax,0(%ecx)
 	movl  %edx,4(%ecx)
+	jmp   epilogue
 	
+retint:
+	/* Load %ecx with the pointer to storage for the return value  */
+	movl  24(%ebp),%ecx	
+	movl  %eax,0(%ecx)
+
 retstruct:
 	/* Nothing to do!  */
 
 noretval:
 epilogue:
+        popl %esi
         movl %ebp,%esp
         popl %ebp
         ret
@@ -162,7 +194,15 @@
 	movl	-12(%ebp), %ecx
 	cmpl	$FFI_TYPE_INT, %eax
 	je	.Lcls_retint
-	cmpl	$FFI_TYPE_FLOAT, %eax
+
+	/* Handle FFI_TYPE_UINT8, FFI_TYPE_SINT8, FFI_TYPE_UINT16,
+	   FFI_TYPE_SINT16, FFI_TYPE_UINT32, FFI_TYPE_SINT32.  */
+	cmpl	$FFI_TYPE_UINT64, %eax
+	jge	0f
+	cmpl	$FFI_TYPE_UINT8, %eax
+	jge	.Lcls_retint
+	
+0:	cmpl	$FFI_TYPE_FLOAT, %eax
 	je	.Lcls_retfloat
 	cmpl	$FFI_TYPE_DOUBLE, %eax
 	je	.Lcls_retdouble
Index: testsuite/libffi.call/return_sc.c
===================================================================
--- testsuite/libffi.call/return_sc.c	(revision 127059)
+++ testsuite/libffi.call/return_sc.c	(working copy)
@@ -30,7 +30,7 @@
        sc < (signed char) 127; sc++)
     {
       ffi_call(&cif, FFI_FN(return_sc), &rint, values);
-      CHECK((signed char) rint == sc);
+      CHECK(rint == (ffi_arg) sc);
     }
   exit(0);
 }

Re: [PATCH] Fix PR32843, testsuite bug wrt sign/zero-extension of return values

[Andrew Haley]

The PR32843 fix caused libgcj regressions.  This turned out to be
because ffi_closure_raw_SYSV duplicates code from ffi_closure_SYSV but
in a way that is difficult to share, and the libffi testsuite doesn't
test raw closures at all.  The PR32843 patch needed to be applied in
two places, not just one.  :-(

Fixed thusly.

Andrew.


2007-08-06  Andrew Haley  <aph@redhat.com>

	PR testsuite/32843
	* src/x86/sysv.S (ffi_closure_raw_SYSV): Handle FFI_TYPE_UINT8,
	FFI_TYPE_SINT8, FFI_TYPE_UINT16, FFI_TYPE_SINT16, FFI_TYPE_UINT32,
	FFI_TYPE_SINT32.

Index: sysv.S
===================================================================
*** sysv.S	(revision 127238)
--- sysv.S	(working copy)
*************** ffi_closure_raw_SYSV:
*** 266,271 ****
--- 266,278 ----
  	movl	CIF_FLAGS_OFFSET(%esi), %eax		 /* rtype */
  	cmpl	$FFI_TYPE_INT, %eax
  	je	.Lrcls_retint
+ 	/* Handle FFI_TYPE_UINT8, FFI_TYPE_SINT8, FFI_TYPE_UINT16,
+ 	   FFI_TYPE_SINT16, FFI_TYPE_UINT32, FFI_TYPE_SINT32.  */
+ 	cmpl	$FFI_TYPE_UINT64, %eax
+ 	jge	0f
+ 	cmpl	$FFI_TYPE_UINT8, %eax
+ 	jge	.Lcls_retint
+ 0:	
  	cmpl	$FFI_TYPE_FLOAT, %eax
  	je	.Lrcls_retfloat
  	cmpl	$FFI_TYPE_DOUBLE, %eax

Re: [PATCH] Fix PR32843, testsuite bug wrt sign/zero-extension of return values

[Andrew Haley]

I typo'd yesterday's checkin.  Sorry for the inconvenience.

Andrew.


2007-08-07  Andrew Haley  <aph@redhat.com>

	* src/x86/sysv.S (ffi_closure_raw_SYSV): Fix typo in previous
	checkin.

Index: sysv.S
===================================================================
--- sysv.S	(revision 127266)
+++ sysv.S	(working copy)
@@ -272,7 +272,7 @@
 	cmpl	$FFI_TYPE_UINT64, %eax
 	jge	0f
 	cmpl	$FFI_TYPE_UINT8, %eax
-	jge	.Lcls_retint
+	jge	.Lrcls_retint
 0:
 	cmpl	$FFI_TYPE_FLOAT, %eax
 	je	.Lrcls_retfloat