Re: [patch] libffi, fix powerpc-unknown-freebsd

Re: [patch] libffi, fix powerpc-unknown-freebsd

[David Edelsohn]

	* src/powerpc/ffitarget.h (ffi_abi): Add FFI_LINUX and
	FFI_LINUX_SOFT_FLOAT to the POWERPC_FREEBSD enum.
	Adjust copyright notice.
	* src/powerpc/ffi.c (ffi_prep_cif_machdep): Fix bit position
	calculation for FFI_SYSV_TYPE_SMALL_STRUCT.
	(ffi_closure_helper_SYSV): Fix return type for
	FFI_SYSV_TYPE_SMALL_STRUCT.
	Adjust copyright notice.

Okay.

Please add more documentation for FFI_SYSV_TYPE_SMALL_STRUCT -- what
it represents and that it uses two bits.

Thanks, David

Re: [patch] libffi, fix powerpc-unknown-freebsd

[Andreas Tobler]

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

David Edelsohn wrote:
> 	* src/powerpc/ffitarget.h (ffi_abi): Add FFI_LINUX and
> 	FFI_LINUX_SOFT_FLOAT to the POWERPC_FREEBSD enum.
> 	Adjust copyright notice.
> 	* src/powerpc/ffi.c (ffi_prep_cif_machdep): Fix bit position
> 	calculation for FFI_SYSV_TYPE_SMALL_STRUCT.
> 	(ffi_closure_helper_SYSV): Fix return type for
> 	FFI_SYSV_TYPE_SMALL_STRUCT.
> 	Adjust copyright notice.
> 
> Okay.
> 
> Please add more documentation for FFI_SYSV_TYPE_SMALL_STRUCT -- what
> it represents and that it uses two bits.

Sigh, I expected that... I should know you :)

Here another iteration.

Instead of playing with FFI_SYSV_TYPE_SMALL_STRUCT for bit shifts I 
introduced two new flags which tell us if we need r3 or r3 and r4 for 
structs passing.

The issue is this, I need to tell the ppc_closure.S exactly which type I 
expect when returning. And unfortunatley the implementation needs a 
distinction for every type and size. Type 1 to 15 are already used, so I 
can start with using type 16 and up.

If it is still not clear, let me know, I try to be more verbose then.

The attached patch was tested under fbsd-7.0 gcc-4.3 and trunk.
Currently a ppc-linux build is running for base config and also for 
soft-float.


2008-08-21  Andreas Tobler  <a.tobler@schweiz.org>

	* src/powerpc/ffitarget.h (ffi_abi): Add FFI_LINUX and
	FFI_LINUX_SOFT_FLOAT to the POWERPC_FREEBSD enum.
	Adjust copyright notice.
	* src/powerpc/ffi.c: Add two new flags to indicate if we have one
	register or two register to use for FFI_SYSV structs.
	(ffi_prep_cif_machdep): Pass the right register flag introduced above.
	(ffi_closure_helper_SYSV): Fix the return type for
	FFI_SYSV_TYPE_SMALL_STRUCT. Comment.
	Adjust copyright notice.

[-- Attachment #2: fbsd_ffi3.diff --]
[-- Type: text/plain, Size: 3069 bytes --]

Index: src/powerpc/ffitarget.h
===================================================================
--- src/powerpc/ffitarget.h	(revision 139412)
+++ src/powerpc/ffitarget.h	(working copy)
@@ -1,6 +1,6 @@
 /* -----------------------------------------------------------------*-C-*-
    ffitarget.h - Copyright (c) 1996-2003  Red Hat, Inc.
-   Copyright (C) 2007 Free Software Foundation, Inc
+   Copyright (C) 2007, 2008 Free Software Foundation, Inc
    Target configuration macros for PowerPC.
 
    Permission is hereby granted, free of charge, to any person obtaining
@@ -77,6 +77,8 @@
   FFI_SYSV,
   FFI_GCC_SYSV,
   FFI_LINUX64,
+  FFI_LINUX,
+  FFI_LINUX_SOFT_FLOAT,
   FFI_DEFAULT_ABI = FFI_SYSV,
 #endif
 
Index: src/powerpc/ffi.c
===================================================================
--- src/powerpc/ffi.c	(revision 139412)
+++ src/powerpc/ffi.c	(working copy)
@@ -1,6 +1,6 @@
 /* -----------------------------------------------------------------------
    ffi.c - Copyright (c) 1998 Geoffrey Keating
-   Copyright (C) 2007 Free Software Foundation, Inc
+   Copyright (C) 2007, 2008 Free Software Foundation, Inc
 
    PowerPC Foreign Function Interface
 
@@ -43,6 +43,10 @@
 
   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),
@@ -679,14 +683,14 @@
 		 The same applies for the structs returned in r3/r4.  */
 	      if (size <= 4)
 		{
-		  flags |= 1 << (31 - FFI_SYSV_TYPE_SMALL_STRUCT - 1);
+		  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 |= 1 << (31 - FFI_SYSV_TYPE_SMALL_STRUCT - 2);
+		  flags |= FLAG_SYSV_SMST_R4;
 		  flags |= 8 * (8 - size) << 4;
 		  break;
 		}
@@ -1248,10 +1252,15 @@
 
   /* Tell ffi_closure_SYSV how to perform return type promotions.
      Because the FFI_SYSV ABI returns the structures <= 8 bytes in r3/r4
-     we have to tell ffi_closure_SYSV how to treat them.  */
+     we have to tell ffi_closure_SYSV how to treat them. We combine the base
+     type FFI_SYSV_TYPE_SMALL_STRUCT - 1  with the size of the struct.
+     So a one byte struct gets the return type 16. Return type 1 to 15 are
+     already used and we never have a struct with size zero. That is the reason
+     for the subtraction of 1. See the comment in ffitarget.h about ordering.
+  */
   if (cif->abi == FFI_SYSV && cif->rtype->type == FFI_TYPE_STRUCT
       && size <= 8)
-    return FFI_SYSV_TYPE_SMALL_STRUCT + size;
+    return (FFI_SYSV_TYPE_SMALL_STRUCT - 1) + size;
 #if FFI_TYPE_LONGDOUBLE != FFI_TYPE_DOUBLE
   else if (cif->rtype->type == FFI_TYPE_LONGDOUBLE
 	   && cif->abi != FFI_LINUX && cif->abi != FFI_LINUX_SOFT_FLOAT)

Re: [patch] libffi, fix powerpc-unknown-freebsd

[David Edelsohn]

On Thu, Aug 21, 2008 at 5:53 PM, Andreas Tobler <andreast-list@fgznet.ch> wrote:
> Instead of playing with FFI_SYSV_TYPE_SMALL_STRUCT for bit shifts I
> introduced two new flags which tell us if we need r3 or r3 and r4 for
> structs passing.
>
> The issue is this, I need to tell the ppc_closure.S exactly which type I
> expect when returning. And unfortunatley the implementation needs a
> distinction for every type and size. Type 1 to 15 are already used, so I can
> start with using type 16 and up.

I mainly want a comment that two bits are used, especially in
ffitarget.h.  Thanks for the additional explanation.

Okay with that.

Thanks, David

Re: [patch] libffi, fix powerpc-unknown-freebsd

[Andreas Tobler]

David Edelsohn wrote:
> On Thu, Aug 21, 2008 at 5:53 PM, Andreas Tobler <andreast-list@fgznet.ch> wrote:
>> Instead of playing with FFI_SYSV_TYPE_SMALL_STRUCT for bit shifts I
>> introduced two new flags which tell us if we need r3 or r3 and r4 for
>> structs passing.
>>
>> The issue is this, I need to tell the ppc_closure.S exactly which type I
>> expect when returning. And unfortunatley the implementation needs a
>> distinction for every type and size. Type 1 to 15 are already used, so I can
>> start with using type 16 and up.
> 
> I mainly want a comment that two bits are used, especially in
> ffitarget.h.  Thanks for the additional explanation.

Thanks for review.

I have an understanding issue here, what bits are you referring to? The 
FLAG_SYSV_SMST_R4 and FLAG_SYSV_SMST_R3 from ffi.c?
Or the situation that I use FFI_SYSV_TYPE_SMALL_STRUCT and the two flags?
If the former, then my impression is that they don't need to be 
mentioned in ffitarget.h, otherwise we'd have to describe every flag 
used in ffi.c in ffitarget.h as well.
If the latter, I can put a note into ffitarget.h mentioning that we need 
additional separation in ffi.c to clearly distinguish what we return.

Else, if I completely misunderstood you, would you mind explaining me 
what you would like to read?

Thanks in advance,
Andreas

Re: [patch] libffi, fix powerpc-unknown-freebsd

[David Edelsohn]

On Sun, Aug 24, 2008 at 4:08 PM, Andreas Tobler <andreast-list@fgznet.ch> wrote:

> I have an understanding issue here, what bits are you referring to? The
> FLAG_SYSV_SMST_R4 and FLAG_SYSV_SMST_R3 from ffi.c?
> Or the situation that I use FFI_SYSV_TYPE_SMALL_STRUCT and the two flags?
> If the former, then my impression is that they don't need to be mentioned in
> ffitarget.h, otherwise we'd have to describe every flag used in ffi.c in
> ffitarget.h as well.
> If the latter, I can put a note into ffitarget.h mentioning that we need
> additional separation in ffi.c to clearly distinguish what we return.

FLAG_SYSV_TYPE_SMALL_STRUCT uses two bits in flags, if I understand
the code correctly.  I think it would be helpful to note that in
ffitarget.h

Thanks, David

Re: [patch] libffi, fix powerpc-unknown-freebsd

[Andreas Tobler]

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

David Edelsohn wrote:
> On Sun, Aug 24, 2008 at 4:08 PM, Andreas Tobler <andreast-list@fgznet.ch> wrote:
> 
>> I have an understanding issue here, what bits are you referring to? The
>> FLAG_SYSV_SMST_R4 and FLAG_SYSV_SMST_R3 from ffi.c?
>> Or the situation that I use FFI_SYSV_TYPE_SMALL_STRUCT and the two flags?
>> If the former, then my impression is that they don't need to be mentioned in
>> ffitarget.h, otherwise we'd have to describe every flag used in ffi.c in
>> ffitarget.h as well.
>> If the latter, I can put a note into ffitarget.h mentioning that we need
>> additional separation in ffi.c to clearly distinguish what we return.
> 
> FLAG_SYSV_TYPE_SMALL_STRUCT uses two bits in flags, if I understand
> the code correctly.  I think it would be helpful to note that in
> ffitarget.h

I will commit the attached to trunk in a few minutes. For 4.3 I'll wait 
until 4.3.3 opens, ok?

Thank you for review.

Regards,
Andreas


2008-08-25  Andreas Tobler  <a.tobler@schweiz.org>

	* src/powerpc/ffitarget.h (ffi_abi): Add FFI_LINUX and
	FFI_LINUX_SOFT_FLOAT to the POWERPC_FREEBSD enum.
	Add note about flag bits used for FFI_SYSV_TYPE_SMALL_STRUCT.
	Adjust copyright notice.
	* src/powerpc/ffi.c: Add two new flags to indicate if we have one
	register or two register to use for FFI_SYSV structs.
	(ffi_prep_cif_machdep): Pass the right register flag introduced above.
	(ffi_closure_helper_SYSV): Fix the return type for
	FFI_SYSV_TYPE_SMALL_STRUCT. Comment.
	Adjust copyright notice.


[-- Attachment #2: fbsd_ffi4.diff --]
[-- Type: text/plain, Size: 3548 bytes --]

Index: src/powerpc/ffi.c
===================================================================
--- src/powerpc/ffi.c	(revision 139412)
+++ src/powerpc/ffi.c	(working copy)
@@ -1,6 +1,6 @@
 /* -----------------------------------------------------------------------
    ffi.c - Copyright (c) 1998 Geoffrey Keating
-   Copyright (C) 2007 Free Software Foundation, Inc
+   Copyright (C) 2007, 2008 Free Software Foundation, Inc
 
    PowerPC Foreign Function Interface
 
@@ -43,6 +43,10 @@
 
   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),
@@ -679,14 +683,14 @@
 		 The same applies for the structs returned in r3/r4.  */
 	      if (size <= 4)
 		{
-		  flags |= 1 << (31 - FFI_SYSV_TYPE_SMALL_STRUCT - 1);
+		  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 |= 1 << (31 - FFI_SYSV_TYPE_SMALL_STRUCT - 2);
+		  flags |= FLAG_SYSV_SMST_R4;
 		  flags |= 8 * (8 - size) << 4;
 		  break;
 		}
@@ -1248,10 +1252,15 @@
 
   /* Tell ffi_closure_SYSV how to perform return type promotions.
      Because the FFI_SYSV ABI returns the structures <= 8 bytes in r3/r4
-     we have to tell ffi_closure_SYSV how to treat them.  */
+     we have to tell ffi_closure_SYSV how to treat them. We combine the base
+     type FFI_SYSV_TYPE_SMALL_STRUCT - 1  with the size of the struct.
+     So a one byte struct gets the return type 16. Return type 1 to 15 are
+     already used and we never have a struct with size zero. That is the reason
+     for the subtraction of 1. See the comment in ffitarget.h about ordering.
+  */
   if (cif->abi == FFI_SYSV && cif->rtype->type == FFI_TYPE_STRUCT
       && size <= 8)
-    return FFI_SYSV_TYPE_SMALL_STRUCT + size;
+    return (FFI_SYSV_TYPE_SMALL_STRUCT - 1) + size;
 #if FFI_TYPE_LONGDOUBLE != FFI_TYPE_DOUBLE
   else if (cif->rtype->type == FFI_TYPE_LONGDOUBLE
 	   && cif->abi != FFI_LINUX && cif->abi != FFI_LINUX_SOFT_FLOAT)
Index: src/powerpc/ffitarget.h
===================================================================
--- src/powerpc/ffitarget.h	(revision 139412)
+++ src/powerpc/ffitarget.h	(working copy)
@@ -1,6 +1,6 @@
 /* -----------------------------------------------------------------*-C-*-
    ffitarget.h - Copyright (c) 1996-2003  Red Hat, Inc.
-   Copyright (C) 2007 Free Software Foundation, Inc
+   Copyright (C) 2007, 2008 Free Software Foundation, Inc
    Target configuration macros for PowerPC.
 
    Permission is hereby granted, free of charge, to any person obtaining
@@ -77,6 +77,8 @@
   FFI_SYSV,
   FFI_GCC_SYSV,
   FFI_LINUX64,
+  FFI_LINUX,
+  FFI_LINUX_SOFT_FLOAT,
   FFI_DEFAULT_ABI = FFI_SYSV,
 #endif
 
@@ -95,7 +97,9 @@
 /* Needed for soft-float long-double-128 support.  */
 #define FFI_TYPE_UINT128 (FFI_TYPE_LAST + 1)
 
-/* Needed for FFI_SYSV small structure returns.  */
+/* Needed for FFI_SYSV small structure returns.
+   We use two flag bits, (FLAG_SYSV_SMST_R3, FLAG_SYSV_SMST_R4) which are
+   defined in ffi.c, to determine the exact return type and its size.  */
 #define FFI_SYSV_TYPE_SMALL_STRUCT (FFI_TYPE_LAST + 2)
 
 #if defined(POWERPC64) || defined(POWERPC_AIX)

Re: [patch] libffi, fix powerpc-unknown-freebsd

[Andreas Tobler]

Andreas Tobler wrote:
> David Edelsohn wrote:
>> On Sun, Aug 24, 2008 at 4:08 PM, Andreas Tobler 
>> <andreast-list@fgznet.ch> wrote:
>>
>>> I have an understanding issue here, what bits are you referring to? The
>>> FLAG_SYSV_SMST_R4 and FLAG_SYSV_SMST_R3 from ffi.c?
>>> Or the situation that I use FFI_SYSV_TYPE_SMALL_STRUCT and the two 
>>> flags?
>>> If the former, then my impression is that they don't need to be 
>>> mentioned in
>>> ffitarget.h, otherwise we'd have to describe every flag used in ffi.c in
>>> ffitarget.h as well.
>>> If the latter, I can put a note into ffitarget.h mentioning that we need
>>> additional separation in ffi.c to clearly distinguish what we return.
>>
>> FLAG_SYSV_TYPE_SMALL_STRUCT uses two bits in flags, if I understand
>> the code correctly.  I think it would be helpful to note that in
>> ffitarget.h
> 
> I will commit the attached to trunk in a few minutes. For 4.3 I'll wait 
> until 4.3.3 opens, ok?

Commited now to 4.3.3

Thanks,
Andreas

> 2008-08-25  Andreas Tobler  <a.tobler@schweiz.org>
> 
>     * src/powerpc/ffitarget.h (ffi_abi): Add FFI_LINUX and
>     FFI_LINUX_SOFT_FLOAT to the POWERPC_FREEBSD enum.
>     Add note about flag bits used for FFI_SYSV_TYPE_SMALL_STRUCT.
>     Adjust copyright notice.
>     * src/powerpc/ffi.c: Add two new flags to indicate if we have one
>     register or two register to use for FFI_SYSV structs.
>     (ffi_prep_cif_machdep): Pass the right register flag introduced above.
>     (ffi_closure_helper_SYSV): Fix the return type for
>     FFI_SYSV_TYPE_SMALL_STRUCT. Comment.
>     Adjust copyright notice.
> 

[patch] libffi, fix powerpc-unknown-freebsd

[Andreas Tobler]

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

Hi all,

yeah, I shot myself in the toe. When I added FFI_LINUX_SOFT_FLOAT 
support I did mess up with SMALL_STRUCTS on FFI_SYSV. Reminds me to 
always TEST!

This patch fixes libffi compilation and test suite on 
powerpc-unknown-freebsd.

Tested on 4.3 and 4.4, aka. trunk.

On 4.3 no failures, on trunk some issues with _Unwind_GetIPInfo in the 
unwind tests. These I'll address later. These are _not_ part of my mess.

Ok for 4.3 and trunk?

Test Run By andreast on Mon Aug 18 21:35:35 2008
Native configuration is powerpc-unknown-freebsd7.0

		=== libffi tests ===

Schedule of variations:
     unix

		=== libffi Summary ===

# of expected passes		1389
# of unsupported tests		10


Thanks,
Andreas

2008-08-18  Andreas Tobler  <a.tobler@schweiz.org>

	* src/powerpc/ffitarget.h (ffi_abi): Add FFI_LINUX and
	FFI_LINUX_SOFT_FLOAT to the POWERPC_FREEBSD enum.
	Adjust copyright notice.
	* src/powerpc/ffi.c (ffi_prep_cif_machdep): Fix bit position
	calculation for FFI_SYSV_TYPE_SMALL_STRUCT.
	(ffi_closure_helper_SYSV): Fix return type for
	FFI_SYSV_TYPE_SMALL_STRUCT.
	Adjust copyright notice.

[-- Attachment #2: fbsd_ffi.diff --]
[-- Type: text/plain, Size: 2153 bytes --]

Index: src/powerpc/ffitarget.h
===================================================================
--- src/powerpc/ffitarget.h	(revision 139196)
+++ src/powerpc/ffitarget.h	(working copy)
@@ -1,6 +1,6 @@
 /* -----------------------------------------------------------------*-C-*-
    ffitarget.h - Copyright (c) 1996-2003  Red Hat, Inc.
-   Copyright (C) 2007 Free Software Foundation, Inc
+   Copyright (C) 2007, 2008 Free Software Foundation, Inc
    Target configuration macros for PowerPC.
 
    Permission is hereby granted, free of charge, to any person obtaining
@@ -77,6 +77,8 @@
   FFI_SYSV,
   FFI_GCC_SYSV,
   FFI_LINUX64,
+  FFI_LINUX,
+  FFI_LINUX_SOFT_FLOAT,
   FFI_DEFAULT_ABI = FFI_SYSV,
 #endif
 
Index: src/powerpc/ffi.c
===================================================================
--- src/powerpc/ffi.c	(revision 139196)
+++ src/powerpc/ffi.c	(working copy)
@@ -1,6 +1,6 @@
 /* -----------------------------------------------------------------------
    ffi.c - Copyright (c) 1998 Geoffrey Keating
-   Copyright (C) 2007 Free Software Foundation, Inc
+   Copyright (C) 2007, 2008 Free Software Foundation, Inc
 
    PowerPC Foreign Function Interface
 
@@ -679,14 +679,14 @@
 		 The same applies for the structs returned in r3/r4.  */
 	      if (size <= 4)
 		{
-		  flags |= 1 << (31 - FFI_SYSV_TYPE_SMALL_STRUCT - 1);
+		  flags |= 1 << (31 - FFI_SYSV_TYPE_SMALL_STRUCT + 1);
 		  flags |= 8 * (4 - size) << 4;
 		  break;
 		}
 	      /* These structs are returned in r3 and r4. See above.   */
 	      if  (size <= 8)
 		{
-		  flags |= 1 << (31 - FFI_SYSV_TYPE_SMALL_STRUCT - 2);
+		  flags |= 1 << (31 - FFI_SYSV_TYPE_SMALL_STRUCT);
 		  flags |= 8 * (8 - size) << 4;
 		  break;
 		}
@@ -1251,7 +1251,7 @@
      we have to tell ffi_closure_SYSV how to treat them.  */
   if (cif->abi == FFI_SYSV && cif->rtype->type == FFI_TYPE_STRUCT
       && size <= 8)
-    return FFI_SYSV_TYPE_SMALL_STRUCT + size;
+    return (FFI_SYSV_TYPE_SMALL_STRUCT - 1) + size;
 #if FFI_TYPE_LONGDOUBLE != FFI_TYPE_DOUBLE
   else if (cif->rtype->type == FFI_TYPE_LONGDOUBLE
 	   && cif->abi != FFI_LINUX && cif->abi != FFI_LINUX_SOFT_FLOAT)