[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

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

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.

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.
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_STRUCT1	64
 #define CIF_FLAGS_STRUCT2	128
+#define CIF_FLAGS_SINT8		256
+#define CIF_FLAGS_SINT16	512
 /* Perform machine dependent cif processing */
@@ -200,6 +202,14 @@ ffi_prep_cif_machdep (ffi_cif *cif)
       cif->flags = CIF_FLAGS_DINT;
+    case FFI_TYPE_SINT16:
+      cif->flags = CIF_FLAGS_SINT16;
+      break;
+    case FFI_TYPE_SINT8:
+      cif->flags = CIF_FLAGS_SINT8;
+      break;
       cif->flags = CIF_FLAGS_INT;
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:
 	btst	#7,%d2
-	jbeq	noretval
+	jbeq	retsint8
 	move.w	%d0,(%a1)
+	jbra	epilogue
+	btst	#8,%d2
+	jbeq	retsint16
+	extb.l	%d0
+	move.l	%d0,(%a1)
+	jbra	epilogue
+	btst	#9,%d2
+	jbeq	noretval
+	ext.l	%d0
+	move.l	%d0,(%a1)
@@ -201,8 +216,10 @@ CALLFUNC(ffi_closure_SYSV):
 	lsr.l	#1,%d0
 	jne	1f
 	jcc	.Lcls_epilogue
 	move.l	-12(%fp),%d0
+	| no CIF_FLAGS_*
 	unlk	%fp
@@ -210,6 +227,7 @@ CALLFUNC(ffi_closure_SYSV):
 	lsr.l	#2,%d0
 	jne	1f
 	jcs	.Lcls_ret_float
 	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
 #if defined(__MC68881__) || defined(__HAVE_68881__)
 	fmove.d	(%a0),%fp0
@@ -242,17 +261,31 @@ CALLFUNC(ffi_closure_SYSV):
 	jra	.Lcls_epilogue
 	lsr.l	#2,%d0
-	jne	.Lcls_ret_struct2
+	jne	1f
 	jcs	.Lcls_ret_struct1
 	move.l	(%a0),%a0
 	move.l	%a0,%d0
 	jra	.Lcls_epilogue
 	move.b	(%a0),%d0
 	jra	.Lcls_epilogue
+	lsr.l	#2,%d0
+	jne	1f
+	jcs	.Lcls_ret_sint8
 	move.w	(%a0),%d0
 	jra	.Lcls_epilogue
+	move.l	(%a0),%d0
+	extb.l	%d0
+	jra	.Lcls_epilogue
+	move.l	(%a0),%d0
+	ext.l	%d0
+	jra	.Lcls_epilogue
 	.size	CALLFUNC(ffi_closure_SYSV),.-CALLFUNC(ffi_closure_SYSV)

Reply to: