Hi, While working on getting mips64el support in place for Debian we found a bug relating to libgcj/libffi and closures for MIPS n32 and n64. The bug is essentially that return types <= 32bit do not end up correctly sign extended in registers when a function is called via a closure. A test case showing what I think the problem boils down to is at the end of this email. Switch between the two lines in foo_binding to see the difference. Note that this will not fail on x86 but I believe it will fail on any big endian architecture. The root of the problem seems to be in a oddity of FFI that integer return values less than word (or rather register) size are returned as an ffi_arg. The java interpreter does not appear to adhere to this and the patch below seems to fix the issue. Can anyone comment if this looks like the right approach? I think there may be a similar issue in the lang/reflect/natVMProxy.cc code (unbox function) by code inspection alone but haven't investigated further. If this looks OK I'll do some full testsuite runs. Thanks, Matthew libjava/ * interpret-run.cc: Return integers as ffi_arg instead of jint. diff --git a/libjava/interpret-run.cc b/libjava/interpret-run.cc index a4c2d4d..6be354e 100644 --- a/libjava/interpret-run.cc +++ b/libjava/interpret-run.cc @@ -1838,7 +1838,7 @@ details. */ return; insn_ireturn: - *(jint *) retp = POPI (); + *(ffi_arg *) retp = POPI (); return; insn_return: Here is the test case to show the effect of getting int vs ffi_arg wrong on n32/n64. It will say that the result is negative only when using ffi_arg as the return type. #include <stdio.h> #include <ffi.h> __attribute__((noinline)) int foo(void) { return -1; } /* Acts like puts with the file given at time of enclosure. */ void foo_binding(ffi_cif *cif, void *ret, void* args[], void* none) { *(int *)ret = foo(); // *(ffi_arg *)ret = foo(); } typedef int (*foo_t)(); int main() { ffi_cif cif; ffi_closure *closure; void *bound_foo; int rc; closure = ffi_closure_alloc(sizeof(ffi_closure), &bound_foo); if (closure) { /* Initialize the cif */ if (ffi_prep_cif(&cif, FFI_DEFAULT_ABI, 0, &ffi_type_sint32, NULL) == FFI_OK) { if (ffi_prep_closure_loc(closure, &cif, foo_binding, NULL, bound_foo) == FFI_OK) { rc = ((foo_t)bound_foo)(); fprintf(stderr, "is result negative: %s\n", (rc < 0)? "yes":"no"); } } } ffi_closure_free(closure); return 0; }
Matthew Fortune <Matthew.Fortune@imgtec.com> writes: > The root of the problem seems to be in a oddity of FFI that integer return > values less than word (or rather register) size are returned as an > ffi_arg. Yes, this is expected behaviour. > The java interpreter does not appear to adhere to this and the patch below > seems to fix the issue. Can anyone comment if this looks like the right > approach? On the surface it looks good to me. Thanks, AG
Anthony Green <green@redhat.com> writes:
> Matthew Fortune <Matthew.Fortune@imgtec.com> writes:
>
> > The root of the problem seems to be in a oddity of FFI that integer return
> > values less than word (or rather register) size are returned as an
> > ffi_arg.
>
> Yes, this is expected behaviour.
>
> > The java interpreter does not appear to adhere to this and the patch below
> > seems to fix the issue. Can anyone comment if this looks like the right
> > approach?
>
> On the surface it looks good to me.
Thanks, I'll get on with testing then. As you can see I was somewhat unsure
of the fix, I initially had patched libffi for MIPS n32 until I realised
the ffi_arg quirk. It seems a few projects have fallen foul of this with
32-bit integers and 64-bit architectures. Libguile had the same issue some
time ago.
Matthew
Hi, On 2016-06-22 11:54, Matthew Fortune wrote: > Hi, > > While working on getting mips64el support in place for Debian we found a > bug relating to libgcj/libffi and closures for MIPS n32 and n64. The bug > is essentially that return types <= 32bit do not end up correctly sign > extended in registers when a function is called via a closure. A test case > showing what I think the problem boils down to is at the end of this email. > Switch between the two lines in foo_binding to see the difference. Note > that this will not fail on x86 but I believe it will fail on any big endian > architecture. > > The root of the problem seems to be in a oddity of FFI that integer return > values less than word (or rather register) size are returned as an ffi_arg. > The java interpreter does not appear to adhere to this and the patch below > seems to fix the issue. Can anyone comment if this looks like the right > approach? > > I think there may be a similar issue in the lang/reflect/natVMProxy.cc > code (unbox function) by code inspection alone but haven't investigated > further. > > If this looks OK I'll do some full testsuite runs. I have been testing Matthew's patch in the last days. I confirm this patch correctly fixes the original issue I reported him, the fact that the Arrays.sort() method sort arrays incorrectly. This in turns cause ecj to fail finding most of the identifiers. With this patch I can use ecj and rebuild it with itself without any problem. Aurelien -- Aurelien Jarno GPG: 4096R/1DDD8C9B aurelien@aurel32.net http://www.aurel32.net