Bug#660525: [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.
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.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
Reply to: