[RFC] interpreter use of closures and return types

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[RFC] interpreter use of closures and return types

Matthew Fortune
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;
}
Reply | Threaded
Open this post in threaded view
|

Re: [RFC] interpreter use of closures and return types

Anthony Green-5
Matthew Fortune <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

RE: [RFC] interpreter use of closures and return types

Matthew Fortune
Anthony Green <[hidden email]> writes:

> Matthew Fortune <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: [RFC] interpreter use of closures and return types

Aurelien Jarno
In reply to this post by Matthew Fortune
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
[hidden email]                 http://www.aurel32.net