[PATCH] Fix libffi on m68k-linux-gnu, completely

[PATCH] Fix libffi on m68k-linux-gnu, completely

[Thorsten Glaser]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3419 bytes --]

Note: Doko and Andreas, you’re mentioned further below.


Andreas Schwab dixit:

>Alan Hourihane <alanh@fairlite.co.uk> writes:
>
>> Fixed the test, as it was broken.
>
>No, it isn't.

Hi you two, I’ve seen you fixed all failures than the (above discussed)
return_sc. I took care of that one today. (If I get my hands on the
person responsible for the lsr.l codepath, when the other routine uses
something so much easier to spot, thus being so temptingly easy, making
me not look harder… this cost me quite some time today!)

Hello libffi developers upstream, please do apply the attached patch
against git master, and then mark m68k-linux-gnu as working/tested
platform. I’m getting a full testsuite pass for both 3.0.10 with the
m68k code updated, and no new failures for 3.0.11 with the patch applied
although I believe some 3.0.11 checks to be broken:

The cls_uchar.c test has:

static void cls_ret_uchar_fn(ffi_cif* cif __UNUSED__, void* resp,
    void** args, void* userdata __UNUSED__) {
	*(ffi_arg*)resp = *(unsigned char *)args[0];

The cls_uchar_va.c test has (T expanded):

static void cls_ret_T_fn(ffi_cif* cif __UNUSED__, void* resp,
    void** args, void* userdata __UNUSED__) {
	*(unsigned char *)resp = *(unsigned char *)args[0];

Now, on big endian platforms, the result is quite different:

Assuming we call them with args[0]=0x7F, then:
Before: *(unsigned long *)resp = { 0x??, 0x??, 0x??, 0x?? }
After1: *(unsigned long *)resp = { 0x00, 0x00, 0x00, 0x7F }
After2: *(unsigned long *)resp = { 0x7F, 0x??, 0x??, 0x?? }

The failing tests are contributed by ARM, who are using
Little Endian, or so I’m told, so it’s no surprise they
didn’t notice. What’s the correct method to access *resp
in a closure, casting to the “real” return type or to an
ffi_arg pointer (the latter is used by all other/older
tests, i.e. all those predating arm64 support).

The failing tests (both before and after my patch) are:
libffi.call/cls_uchar_va.c cls_ushort_va.c va_1.c
The following exemplary patch makes one of them pass:
--- cls_uchar_va.c      2012-12-02 21:34:19.000000000 +0000
+++ cls_uchar_va2.c     2012-12-02 22:26:37.000000000 +0000
@@ -12,9 +12,9 @@
 static void cls_ret_T_fn(ffi_cif* cif __UNUSED__, void* resp, void** args,
                         void* userdata __UNUSED__)
  {
-   *(T *)resp = *(T *)args[0];
+   *(ffi_arg *)resp = *(T *)args[0];
 
-   printf("%d: %d %d\n", *(T *)resp, *(T *)args[0], *(T *)args[1]);
+   printf("%d: %d %d\n", (int)(*(ffi_arg *)resp), *(T *)args[0], *(T *)args[1]);
  }
 
 typedef T (*cls_ret_T)(T, ...);


And, WTF: Why is there a .pc directory in the git repository?


Hello Debian libffi maintainer, you will receive patches in a separate
message against unstable (3.0.10) and experimental (3.0.11) as well as
gcc-4.7 (uses something close to 3.0.10 but not quite).

Andreas, I think you can commit the GCC side of the patch as well, so
I’ll keep debian-68k on Cc for the followup message with them.

bye,
//mirabilos
-- 
Darwinism never[…]applied to wizardkind. There's a more than fair amount of[…]
stupidity in its gene-pool[…]never eradicated[…]magic evens the odds that way.
It's[…]harder to die for us than[…]muggles[…]wonder if, as technology[…]better
[…]same will[…]happen there too. Dursleys' continued existence indicates so.

[-- Attachment #2: Type: TEXT/PLAIN, Size: 3618 bytes --]

From 9dd3345b2ef98b1fc18a1381cfe46b0381d71777 Mon Sep 17 00:00:00 2001
From: Thorsten Glaser <tg@mirbsd.org>
Date: Sun, 2 Dec 2012 20:11:37 +0000
Subject: [PATCH] Fix 8-bit and 16-bit signed calls on m68k

Note: return_sc only tests 8-bit calls; I wrote myself a small
return_ss testcase which is the same except using signed short

TODO: src/m68k/sysv.S uses two different styles, one uses a
bit test command and simple jumps, the other rather convoluted
right-shifts through carry by two and three-way jumps. This
should be unified and documented better; also, the label naming
differs greatly between the call and the closure function.

Signed-off-by: Thorsten Glaser <tg@mirbsd.org>
---
 src/m68k/ffi.c  |   10 ++++++++++
 src/m68k/sysv.S |   39 ++++++++++++++++++++++++++++++++++++---
 2 files changed, 46 insertions(+), 3 deletions(-)

diff --git a/src/m68k/ffi.c b/src/m68k/ffi.c
index 37a0784..0dee938 100644
--- a/src/m68k/ffi.c
+++ b/src/m68k/ffi.c
@@ -123,6 +123,8 @@ ffi_prep_args (void *stack, extended_cif *ecif)
 #define CIF_FLAGS_POINTER	32
 #define CIF_FLAGS_STRUCT1	64
 #define CIF_FLAGS_STRUCT2	128
+#define CIF_FLAGS_SINT8		256
+#define CIF_FLAGS_SINT16	512
 
 /* Perform machine dependent cif processing */
 ffi_status
@@ -200,6 +202,14 @@ ffi_prep_cif_machdep (ffi_cif *cif)
       cif->flags = CIF_FLAGS_DINT;
       break;
 
+    case FFI_TYPE_SINT16:
+      cif->flags = CIF_FLAGS_SINT16;
+      break;
+
+    case FFI_TYPE_SINT8:
+      cif->flags = CIF_FLAGS_SINT8;
+      break;
+
     default:
       cif->flags = CIF_FLAGS_INT;
       break;
diff --git a/src/m68k/sysv.S b/src/m68k/sysv.S
index f6f4ef9..42f520b 100644
--- a/src/m68k/sysv.S
+++ b/src/m68k/sysv.S
@@ -3,6 +3,7 @@
    sysv.S - Copyright (c) 2012 Alan Hourihane
 	    Copyright (c) 1998, 2012 Andreas Schwab
 	    Copyright (c) 2008 Red Hat, Inc. 
+	    Copyright (c) 2012 Thorsten Glaser
    
    m68k Foreign Function Interface 
 
@@ -168,8 +169,22 @@ retstruct1:
 
 retstruct2:
 	btst	#7,%d2
-	jbeq	noretval
+	jbeq	retsint8
 	move.w	%d0,(%a1)
+	jbra	epilogue
+
+retsint8:
+	btst	#8,%d2
+	jbeq	retsint16
+	extb.l	%d0
+	move.l	%d0,(%a1)
+	jbra	epilogue
+
+retsint16:
+	btst	#9,%d2
+	jbeq	noretval
+	ext.l	%d0
+	move.l	%d0,(%a1)
 
 noretval:
 epilogue:
@@ -201,8 +216,10 @@ CALLFUNC(ffi_closure_SYSV):
 	lsr.l	#1,%d0
 	jne	1f
 	jcc	.Lcls_epilogue
+	| CIF_FLAGS_INT
 	move.l	-12(%fp),%d0
 .Lcls_epilogue:
+	| no CIF_FLAGS_*
 	unlk	%fp
 	rts
 1:
@@ -210,6 +227,7 @@ CALLFUNC(ffi_closure_SYSV):
 	lsr.l	#2,%d0
 	jne	1f
 	jcs	.Lcls_ret_float
+	| CIF_FLAGS_DINT
 	move.l	(%a0)+,%d0
 	move.l	(%a0),%d1
 	jra	.Lcls_epilogue
@@ -224,6 +242,7 @@ CALLFUNC(ffi_closure_SYSV):
 	lsr.l	#2,%d0
 	jne	1f
 	jcs	.Lcls_ret_ldouble
+	| CIF_FLAGS_DOUBLE
 #if defined(__MC68881__) || defined(__HAVE_68881__)
 	fmove.d	(%a0),%fp0
 #else
@@ -242,17 +261,31 @@ CALLFUNC(ffi_closure_SYSV):
 	jra	.Lcls_epilogue
 1:
 	lsr.l	#2,%d0
-	jne	.Lcls_ret_struct2
+	jne	1f
 	jcs	.Lcls_ret_struct1
+	| CIF_FLAGS_POINTER
 	move.l	(%a0),%a0
 	move.l	%a0,%d0
 	jra	.Lcls_epilogue
 .Lcls_ret_struct1:
 	move.b	(%a0),%d0
 	jra	.Lcls_epilogue
-.Lcls_ret_struct2:
+1:
+	lsr.l	#2,%d0
+	jne	1f
+	jcs	.Lcls_ret_sint8
+	| CIF_FLAGS_STRUCT2
 	move.w	(%a0),%d0
 	jra	.Lcls_epilogue
+.Lcls_ret_sint8:
+	move.l	(%a0),%d0
+	extb.l	%d0
+	jra	.Lcls_epilogue
+1:
+	| CIF_FLAGS_SINT16
+	move.l	(%a0),%d0
+	ext.l	%d0
+	jra	.Lcls_epilogue
 	CFI_ENDPROC()
 
 	.size	CALLFUNC(ffi_closure_SYSV),.-CALLFUNC(ffi_closure_SYSV)
-- 
1.6.3.1

Testsuite fixes (was Re: [PATCH] Fix libffi on m68k-linux-gnu, completely)

[Thorsten Glaser]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 676 bytes --]

Dixi quod…

>although I believe some 3.0.11 checks to be broken:

And indeed, with a few minor changes on top of git master,
I still get a full run of PASS plus one XPASS on amd64-linux!

With the other patches (from this message’s parent) and
these applied, I get a full PASS on m68k-linux as well.

So, please git am these three diffs ☺

bye,
//mirabilos
-- 
FWIW, I'm quite impressed with mksh interactively. I thought it was much
*much* more bare bones. But it turns out it beats the living hell out of
ksh93 in that respect. I'd even consider it for my daily use if I hadn't
wasted half my life on my zsh setup. :-) -- Frank Terbeck in #!/bin/mksh

[-- Attachment #2: Type: TEXT/PLAIN, Size: 1936 bytes --]

From 5cb15a3bad1f0fb360520dd48bfc938c821cdcca Mon Sep 17 00:00:00 2001
From: Thorsten Glaser <tg@mirbsd.org>
Date: Sun, 2 Dec 2012 23:20:56 +0000
Subject: [PATCH 1/2] Fix tests writing to a closure retval via pointer casts

As explained in <Pine.BSM.4.64L.1212022014490.23442@herc.mirbsd.org>
all other tests that do the same cast to an ffi_arg pointer instead.

PASS on amd64-linux (Xen domU) and m68k-linux (ARAnyM)

Signed-off-by: Thorsten Glaser <tg@mirbsd.org>
---
 testsuite/libffi.call/cls_uchar_va.c  |    4 ++--
 testsuite/libffi.call/cls_ushort_va.c |    4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/testsuite/libffi.call/cls_uchar_va.c b/testsuite/libffi.call/cls_uchar_va.c
index 19cd4f3..6491c5b 100644
--- a/testsuite/libffi.call/cls_uchar_va.c
+++ b/testsuite/libffi.call/cls_uchar_va.c
@@ -12,9 +12,9 @@ typedef unsigned char T;
 static void cls_ret_T_fn(ffi_cif* cif __UNUSED__, void* resp, void** args,
 			 void* userdata __UNUSED__)
  {
-   *(T *)resp = *(T *)args[0];
+   *(ffi_arg *)resp = *(T *)args[0];
 
-   printf("%d: %d %d\n", *(T *)resp, *(T *)args[0], *(T *)args[1]);
+   printf("%d: %d %d\n", (int)(*(ffi_arg *)resp), *(T *)args[0], *(T *)args[1]);
  }
 
 typedef T (*cls_ret_T)(T, ...);
diff --git a/testsuite/libffi.call/cls_ushort_va.c b/testsuite/libffi.call/cls_ushort_va.c
index b2b5a3b..37aa106 100644
--- a/testsuite/libffi.call/cls_ushort_va.c
+++ b/testsuite/libffi.call/cls_ushort_va.c
@@ -12,9 +12,9 @@ typedef unsigned short T;
 static void cls_ret_T_fn(ffi_cif* cif __UNUSED__, void* resp, void** args,
 			 void* userdata __UNUSED__)
  {
-   *(T *)resp = *(T *)args[0];
+   *(ffi_arg *)resp = *(T *)args[0];
 
-   printf("%d: %d %d\n", *(T *)resp, *(T *)args[0], *(T *)args[1]);
+   printf("%d: %d %d\n", (int)(*(ffi_arg *)resp), *(T *)args[0], *(T *)args[1]);
  }
 
 typedef T (*cls_ret_T)(T, ...);
-- 
1.6.3.1


[-- Attachment #3: Type: TEXT/PLAIN, Size: 1582 bytes --]

From c7028a2c3661f7863d6a87798ffde0a868e0dd0b Mon Sep 17 00:00:00 2001
From: Thorsten Glaser <tg@mirbsd.org>
Date: Sun, 2 Dec 2012 23:22:11 +0000
Subject: [PATCH 2/2] Fix vararg test

The C standard indeed promotes char and short to int on varargs;
ffi does the same, but to do that, it must be told the correct
types in arg_types[], not the already-promoted types.

XPASS on amd64-linux, thus adjusting the dejagnu header.
PASS on m68k-linux (given my other patches applied).

Signed-off-by: Thorsten Glaser <tg@mirbsd.org>
---
 testsuite/libffi.call/va_1.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/testsuite/libffi.call/va_1.c b/testsuite/libffi.call/va_1.c
index 5c7cce9..cf4dd85 100644
--- a/testsuite/libffi.call/va_1.c
+++ b/testsuite/libffi.call/va_1.c
@@ -5,7 +5,7 @@
    Originator:	        ARM Ltd. */
 
 /* { dg-do run } */
-/* { dg-output "" { xfail avr32*-*-* x86_64-*-*-* } } */
+/* { dg-output "" { xfail avr32*-*-* } } */
 
 #include "ffitest.h"
 #include <stdarg.h>
@@ -132,10 +132,10 @@ main (void)
   arg_types[1] = &s_type;
   arg_types[2] = &l_type;
   arg_types[3] = &s_type;
-  arg_types[4] = &ffi_type_uint;
-  arg_types[5] = &ffi_type_sint;
-  arg_types[6] = &ffi_type_uint;
-  arg_types[7] = &ffi_type_sint;
+  arg_types[4] = &ffi_type_uchar;
+  arg_types[5] = &ffi_type_schar;
+  arg_types[6] = &ffi_type_ushort;
+  arg_types[7] = &ffi_type_sshort;
   arg_types[8] = &ffi_type_uint;
   arg_types[9] = &ffi_type_sint;
   arg_types[10] = &ffi_type_ulong;
-- 
1.6.3.1

Re: libffi fixes for testsuite failures (bugs) on m68k and for bugs in testsuite

[Thorsten Glaser]

Dixi quod…

>Hi you two, I’ve seen you fixed all failures than the (above discussed)
>return_sc. I took care of that one today.

And:

>Hello libffi developers upstream, please do apply the attached patch

And:

>Andreas, I think you can commit the GCC side of the patch as well, so

And:

>>although I believe some 3.0.11 checks to be broken:
[…]
>So, please git am these three diffs ☺

Now:

Ping? I’ve looked at libffi git and GCC SVN and yet see nothing.
The various mailing list archives also do not have a response,
anything, at all.

bye,
//mirabilos
-- 
I want one of these. They cost 720 € though… good they don’t have the HD hole,
which indicates 3½″ floppies with double capacity… still. A tad too much, atm.
‣ http://www.floppytable.com/floppytable-images-1.html