[patch] libffi darwin x86-32bit, fix return_sc testcase.

[patch] libffi darwin x86-32bit, fix return_sc testcase.

[Andreas Tobler]

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

Hello all,

first, a happy new year to all!

The attached patch brings libffi for darwin x86 (32-bit) back to zero 
failures, at least what is covered by the test suite ;)

I implemented the fix according to Andrew's fix back in July 07 for 
linux x86. Except that I had to treat small structs in special way.

I tested the patch on a Core 2 Duo with 10.5.1, including a libjava test 
suite run. Nothing in regard of regression. But also no improvments for 
TestClosureGC.jar. This is another iteration.

Is this ok for trunk?

It 'fixes' PR 32843 for darwin x86...

Thanks IA,

Regards,
Andreas

2008-01-02  Andreas Tobler  <a.tobler@schweiz.org>

         PR testsuite/32843
         * src/x86/ffi.c (ffi_prep_cif_machdep): Add code for
         signed/unsigned int8/16 for X86_DARWIN.
         Updated copyright info.
         Handle one and two byte structs with special cif->flags.
         * src/x86/ffitarget.h: Add special types for one and two byte 
structs.
         Updated copyright info.
         * src/x86/darwin.S (ffi_call_SYSV): Rewrite to use a jump table 
like
         sysv.S
         Remove code to pop args from the stack after call.
         Special-case signed/unsigned for int8/16, one and two byte structs.
         (ffi_closure_raw_SYSV): Handle FFI_TYPE_UINT8,
         FFI_TYPE_SINT8, FFI_TYPE_UINT16, FFI_TYPE_SINT16, FFI_TYPE_UINT32,
         FFI_TYPE_SINT32.
         Updated copyright info.




[-- Attachment #2: pr32843-darwin-1.diff --]
[-- Type: text/plain, Size: 8443 bytes --]

Index: src/x86/ffi.c
===================================================================
--- src/x86/ffi.c	(revision 131248)
+++ src/x86/ffi.c	(working copy)
@@ -3,9 +3,10 @@
            Copyright (c) 2002  Ranjit Mathew
            Copyright (c) 2002  Bo Thorsen
            Copyright (c) 2002  Roger Sayle
-   
-   x86 Foreign Function Interface 
+	   Copyright (C) 2008  Free Software Foundation, Inc.
 
+   x86 Foreign Function Interface
+
    Permission is hereby granted, free of charge, to any person obtaining
    a copy of this software and associated documentation files (the
    ``Software''), to deal in the Software without restriction, including
@@ -121,10 +122,13 @@
     case FFI_TYPE_VOID:
 #ifdef X86
     case FFI_TYPE_STRUCT:
+#else
+# if defined(X86) || defined(X86_DARWIN)
     case FFI_TYPE_UINT8:
     case FFI_TYPE_UINT16:
     case FFI_TYPE_SINT8:
     case FFI_TYPE_SINT16:
+# endif
 #endif
 
     case FFI_TYPE_SINT64:
@@ -142,11 +146,11 @@
     case FFI_TYPE_STRUCT:
       if (cif->rtype->size == 1)
         {
-          cif->flags = FFI_TYPE_SINT8; /* same as char size */
+          cif->flags = FFI_TYPE_SMALL_STRUCT_1B; /* same as char size */
         }
       else if (cif->rtype->size == 2)
         {
-          cif->flags = FFI_TYPE_SINT16; /* same as short size */
+          cif->flags = FFI_TYPE_SMALL_STRUCT_2B; /* same as short size */
         }
       else if (cif->rtype->size == 4)
         {
Index: src/x86/ffitarget.h
===================================================================
--- src/x86/ffitarget.h	(revision 131248)
+++ src/x86/ffitarget.h	(working copy)
@@ -1,5 +1,7 @@
 /* -----------------------------------------------------------------*-C-*-
    ffitarget.h - Copyright (c) 1996-2003  Red Hat, Inc.
+   Copyright (C) 2008  Free Software Foundation, Inc.
+
    Target configuration macros for x86 and x86-64.
 
    Permission is hereby granted, free of charge, to any person obtaining
@@ -68,6 +70,8 @@
 /* ---- Definitions for closures ----------------------------------------- */
 
 #define FFI_CLOSURES 1
+#define FFI_TYPE_SMALL_STRUCT_1B (FFI_TYPE_LAST + 1)
+#define FFI_TYPE_SMALL_STRUCT_2B (FFI_TYPE_LAST + 2)
 
 #if defined (X86_64) || (defined (__x86_64__) && defined (X86_DARWIN))
 #define FFI_TRAMPOLINE_SIZE 24
Index: src/x86/darwin.S
===================================================================
--- src/x86/darwin.S	(revision 131248)
+++ src/x86/darwin.S	(working copy)
@@ -1,8 +1,9 @@
 /* -----------------------------------------------------------------------
-   sysv.S - Copyright (c) 1996, 1998, 2001, 2002, 2003, 2005  Red Hat, Inc.
-   
-   X86 Foreign Function Interface 
+   darwin.S - Copyright (c) 1996, 1998, 2001, 2002, 2003, 2005  Red Hat, Inc.
+	Copyright (C) 2008  Free Software Foundation, Inc.
 
+   X86 Foreign Function Interface
+
    Permission is hereby granted, free of charge, to any person obtaining
    a copy of this software and associated documentation files (the
    ``Software''), to deal in the Software without restriction, including
@@ -60,16 +61,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.  */
@@ -77,79 +77,102 @@
 	jne   noretval
 	fstp  %st(0)
 
-        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:
+	.align 4
+	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 */
+	.long   retstruct1b-.Lstore_table	/* FFI_TYPE_SMALL_STRUCT_1B */
+	.long   retstruct2b-.Lstore_table	/* FFI_TYPE_SMALL_STRUCT_2B */
+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	
+	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	
+	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	
+	movl  24(%ebp),%ecx
 	fstpt (%ecx)
 	jmp   epilogue
-	
-retint64:	
-	cmpl  $FFI_TYPE_SINT64,%ecx
-        jne   retstruct1b
+
+retint64:
 	/* Load %ecx with the pointer to storage for the return value  */
-	movl  24(%ebp),%ecx	
+	movl  24(%ebp),%ecx
 	movl  %eax,0(%ecx)
 	movl  %edx,4(%ecx)
 	jmp   epilogue
-	
-retstruct1b:	
-	cmpl  $FFI_TYPE_SINT8,%ecx
-        jne   retstruct2b
+
+retstruct1b:
 	/* Load %ecx with the pointer to storage for the return value  */
-	movl  24(%ebp),%ecx	
+	movl  24(%ebp),%ecx
 	movb  %al,0(%ecx)
 	jmp   epilogue
 
-retstruct2b:	
-	cmpl  $FFI_TYPE_SINT16,%ecx
-        jne   retstruct
+retstruct2b:
 	/* Load %ecx with the pointer to storage for the return value  */
-	movl  24(%ebp),%ecx	
+	movl  24(%ebp),%ecx
 	movw  %ax,0(%ecx)
 	jmp   epilogue
 
+retint:
+	/* Load %ecx with the pointer to storage for the return value  */
+	movl  24(%ebp),%ecx
+	movl  %eax,0(%ecx)
+
 retstruct:
-	cmpl  $FFI_TYPE_STRUCT,%ecx
-        jne   noretval
 	/* Nothing to do!  */
-        addl $4,%esp
-        popl %ebp
-        ret
 
 noretval:
 epilogue:
-        addl $8,%esp
-        movl %ebp,%esp
-        popl %ebp
-        ret
+	popl %esi
+	movl %ebp,%esp
+	popl %ebp
+	ret
+
 .LFE1:
 .ffi_call_SYSV_end:
 
@@ -177,7 +200,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
@@ -185,10 +216,10 @@
 	je	.Lcls_retldouble
 	cmpl	$FFI_TYPE_SINT64, %eax
 	je	.Lcls_retllong
-	cmpl	$FFI_TYPE_SINT8, %eax
-	je	.Lcls_retstruct1
-	cmpl	$FFI_TYPE_SINT16, %eax
-	je	.Lcls_retstruct2
+	cmpl	$FFI_TYPE_SMALL_STRUCT_1B, %eax
+	je	.Lcls_retstruct1b
+	cmpl	$FFI_TYPE_SMALL_STRUCT_2B, %eax
+	je	.Lcls_retstruct2b
 	cmpl	$FFI_TYPE_STRUCT, %eax
 	je	.Lcls_retstruct
 .Lcls_epilogue:
@@ -211,10 +242,10 @@
 	movl	(%ecx), %eax
 	movl	4(%ecx), %edx
 	jmp	.Lcls_epilogue
-.Lcls_retstruct1:
+.Lcls_retstruct1b:
 	movsbl	(%ecx), %eax
 	jmp	.Lcls_epilogue
-.Lcls_retstruct2:
+.Lcls_retstruct2b:
 	movswl	(%ecx), %eax
 	jmp	.Lcls_epilogue
 .Lcls_retstruct:
@@ -256,6 +287,14 @@
 	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	.Lrcls_retint
+0:
 	cmpl	$FFI_TYPE_FLOAT, %eax
 	je	.Lrcls_retfloat
 	cmpl	$FFI_TYPE_DOUBLE, %eax

Re: [patch] libffi darwin x86-32bit, fix return_sc testcase.

[Andrew Haley]

Andreas Tobler writes:
 > Hello all,
 > 
 > first, a happy new year to all!
 > 
 > The attached patch brings libffi for darwin x86 (32-bit) back to zero 
 > failures, at least what is covered by the test suite ;)
 > 
 > I implemented the fix according to Andrew's fix back in July 07 for 
 > linux x86. Except that I had to treat small structs in special way.

I will again point out that this is merely a workaround for a gcc code
generation bug.  Rather than fix the symptom, a libffi test failure,
we should fix the real problem: we are not generating ABI-compatible
code on x86.

This bug was introduced by

Author: rguenth
Date: Mon Jul  9 09:53:11 2007
New Revision: 126480

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=32843

Andrew.

-- 
Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, UK
Registered in England and Wales No. 3798903

Re: [patch] libffi darwin x86-32bit, fix return_sc testcase.

[Richard Guenther]

On Thu, 3 Jan 2008, Andrew Haley wrote:

> Andreas Tobler writes:
>  > Hello all,
>  > 
>  > first, a happy new year to all!
>  > 
>  > The attached patch brings libffi for darwin x86 (32-bit) back to zero 
>  > failures, at least what is covered by the test suite ;)
>  > 
>  > I implemented the fix according to Andrew's fix back in July 07 for 
>  > linux x86. Except that I had to treat small structs in special way.
> 
> I will again point out that this is merely a workaround for a gcc code
> generation bug.  Rather than fix the symptom, a libffi test failure,
> we should fix the real problem: we are not generating ABI-compatible
> code on x86.
> 
> This bug was introduced by
> 
> Author: rguenth
> Date: Mon Jul  9 09:53:11 2007
> New Revision: 126480
> 
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=32843

We obviously disagree about what the ABI specifies.  r126480 made the
C/C++ frontend behavior the same as other frontends behavior (for
example Fortran behavior, which I explicitly checked).  As GCC supports
both promoting function return values or not in the backends and the
x86 backend explicitly does _not_ enable this promotion it agrees
with my reading of the ABI.

Richard.

Re: [patch] libffi darwin x86-32bit, fix return_sc testcase.

[Andrew Haley]

Richard Guenther writes:

 > We obviously disagree about what the ABI specifies.  r126480 made
 > the C/C++ frontend behavior the same as other frontends behavior
 > (for example Fortran behavior, which I explicitly checked).  As GCC
 > supports both promoting function return values or not in the
 > backends and the x86 backend explicitly does _not_ enable this
 > promotion it agrees with my reading of the ABI.

But the ABI isn't in doubt: it is required to 

> Functions pass all integer-valued arguments as words, expanding or
> padding signed or unsigned bytes and halfwords as needed'

and this surely applies to return values as well as arguments passed
to functions.  It is utterly perverse to assume that return values are
treated differently from arguments passed to functions.  So if the
back end isn't promoting return values then the back end is wrong.

Andrew.

-- 
Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, UK
Registered in England and Wales No. 3798903

Re: [patch] libffi darwin x86-32bit, fix return_sc testcase.

[Andreas Tobler]

Andrew Haley wrote:
> Richard Guenther writes:
> 
>  > We obviously disagree about what the ABI specifies.  r126480 made
>  > the C/C++ frontend behavior the same as other frontends behavior
>  > (for example Fortran behavior, which I explicitly checked).  As GCC
>  > supports both promoting function return values or not in the
>  > backends and the x86 backend explicitly does _not_ enable this
>  > promotion it agrees with my reading of the ABI.
> 
> But the ABI isn't in doubt: it is required to 
> 
>> Functions pass all integer-valued arguments as words, expanding or
>> padding signed or unsigned bytes and halfwords as needed'
> 
> and this surely applies to return values as well as arguments passed
> to functions.  It is utterly perverse to assume that return values are
> treated differently from arguments passed to functions.  So if the
> back end isn't promoting return values then the back end is wrong.

I'm not in the position to judge about this, I can only read the ABI and 
I have to give my vote to aph, at least from what _I_ understand.
Maybe my understanding lacks some basic issues, in this case I'd like to 
be advised to understand it right. But until then, I suppose this patch 
is hold on ice, right?
This blocks my further work and I fear this issue might take some moons 
to be resolved/clarified.

Who has the ability to clarify this issue? Do we need to expand the 
to/cc list to the _right_ people? Who are the right people?

I'd like to have this clarified, even if I can't help that much in 
clarifying....

Thanks,
Andreas

Re: [patch] libffi darwin x86-32bit, fix return_sc testcase.

[Andrew Haley]

Andreas Tobler writes:
 > Andrew Haley wrote:
 > > Richard Guenther writes:
 > > 
 > >  > We obviously disagree about what the ABI specifies.  r126480 made
 > >  > the C/C++ frontend behavior the same as other frontends behavior
 > >  > (for example Fortran behavior, which I explicitly checked).  As GCC
 > >  > supports both promoting function return values or not in the
 > >  > backends and the x86 backend explicitly does _not_ enable this
 > >  > promotion it agrees with my reading of the ABI.
 > > 
 > > But the ABI isn't in doubt: it is required to 
 > > 
 > >> Functions pass all integer-valued arguments as words, expanding or
 > >> padding signed or unsigned bytes and halfwords as needed'
 > > 
 > > and this surely applies to return values as well as arguments passed
 > > to functions.  It is utterly perverse to assume that return values are
 > > treated differently from arguments passed to functions.  So if the
 > > back end isn't promoting return values then the back end is wrong.
 > 
 > I'm not in the position to judge about this, I can only read the ABI and 
 > I have to give my vote to aph, at least from what _I_ understand.
 > Maybe my understanding lacks some basic issues, in this case I'd like to 
 > be advised to understand it right. But until then, I suppose this patch 
 > is hold on ice, right?
 > This blocks my further work and I fear this issue might take some moons 
 > to be resolved/clarified.
 > 
 > Who has the ability to clarify this issue? Do we need to expand the 
 > to/cc list to the _right_ people? Who are the right people?
 > 
 > I'd like to have this clarified, even if I can't help that much in 
 > clarifying....

The Darwin libffi patch is OK.  There's no reason why it shouldn't go in.

I'll submit a patch for the x86 backend next week.

Andrew.

-- 
Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, UK
Registered in England and Wales No. 3798903

Re: [patch] libffi darwin x86-32bit, fix return_sc testcase.

[Andreas Tobler]

I committed the attached patch as it causes a failure on x86 linux.

Sorry.

This time tested on linux as well.

Andreas


2008-01-06  Andreas Tobler  <a.tobler@schweiz.org>

	* src/x86/ffi.c (ffi_prep_cif_machdep): Fix thinko.

Index: src/x86/ffi.c
===================================================================
--- src/x86/ffi.c	(revision 131343)
+++ src/x86/ffi.c	(working copy)
@@ -122,13 +122,12 @@
      case FFI_TYPE_VOID:
  #ifdef X86
      case FFI_TYPE_STRUCT:
-#else
-# if defined(X86) || defined(X86_DARWIN)
+#endif
+#if defined(X86) || defined(X86_DARWIN)
      case FFI_TYPE_UINT8:
      case FFI_TYPE_UINT16:
      case FFI_TYPE_SINT8:
      case FFI_TYPE_SINT16:
-# endif
  #endif

      case FFI_TYPE_SINT64: