[PATCH] Fix FFI return type for proxy classes

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

[PATCH] Fix FFI return type for proxy classes

Matthew Fortune
Hi,

As mentioned in:

https://gcc.gnu.org/ml/gcc-patches/2016-06/msg01827.html

there is a bug in lang/reflect/natVMProxy.cc where the return types
are not promoted to ffi_arg for integer types smaller than a word.

This bug will not show up on little endian architectures as the issue
gets fixed by _Jv_CallAnyMethodA which casts the (corrupted) return
value down to the correct type again effectively discarding the upper
corrupt bits. On big endian this does not work as the valid bits are
in the wrong position so the casts in _Jv_CallAnyMethodA just make
a bad value worse.

Many thanks to Tom for explaining how to trigger the affected code.
I've added a test that has proxied functions returning each of the
integer types to cover this issue. All being well that will be
sufficient to prevent regressions in this area as it was somewhat
tricky to get to the root of this! I hope the coding style of the
test is OK, I don't know the rules for java formatting in GNU
projects.

Tested on: x86_64-pc-linux-gnu (default and -m32), mips-linux-gnu
mipsel-linux-gnuabi64 with no regressions. The new test only failed
on mips-linux-gnu prior to patching libjava.

Thanks,
Matthew

libjava/
        * java/lang/reflect/natVMProxy.cc (unbox): Use ffi_arg for
        integer return types smaller than a word.
        * testsuite/libjava.jar/ReturnInvocationHandler.java: New file.
        * testsuite/libjava.jar/ReturnProxyTest.jar: Likewise.
        * testsuite/libjava.jar/ReturnProxyTest.java: Likewise.
        * testsuite/libjava.jar/ReturnProxyTest.out: Likewise.
        * testsuite/libjava.jar/ReturnProxyTest.xfail: Likewise.
        * testsuite/libjava.jar/ReturnTypes.java: Likewise.
        * testsuite/libjava.jar/ReturnTypesImpl.java: Likewise.
---
 libjava/java/lang/reflect/natVMProxy.cc            |  10 ++++----
 .../libjava.jar/ReturnInvocationHandler.java       |  24 ++++++++++++++++++
 libjava/testsuite/libjava.jar/ReturnProxyTest.jar  | Bin 0 -> 2671 bytes
 libjava/testsuite/libjava.jar/ReturnProxyTest.java |  27 +++++++++++++++++++++
 libjava/testsuite/libjava.jar/ReturnProxyTest.out  |  12 +++++++++
 .../testsuite/libjava.jar/ReturnProxyTest.xfail    |   1 +
 libjava/testsuite/libjava.jar/ReturnTypes.java     |   9 +++++++
 libjava/testsuite/libjava.jar/ReturnTypesImpl.java |  27 +++++++++++++++++++++
 8 files changed, 105 insertions(+), 5 deletions(-)
 create mode 100644 libjava/testsuite/libjava.jar/ReturnInvocationHandler.java
 create mode 100644 libjava/testsuite/libjava.jar/ReturnProxyTest.jar
 create mode 100644 libjava/testsuite/libjava.jar/ReturnProxyTest.java
 create mode 100644 libjava/testsuite/libjava.jar/ReturnProxyTest.out
 create mode 100644 libjava/testsuite/libjava.jar/ReturnProxyTest.xfail
 create mode 100644 libjava/testsuite/libjava.jar/ReturnTypes.java
 create mode 100644 libjava/testsuite/libjava.jar/ReturnTypesImpl.java

diff --git a/libjava/java/lang/reflect/natVMProxy.cc b/libjava/java/lang/reflect/natVMProxy.cc
index e46263d..19cde20 100644
--- a/libjava/java/lang/reflect/natVMProxy.cc
+++ b/libjava/java/lang/reflect/natVMProxy.cc
@@ -272,17 +272,17 @@ unbox (jobject o, jclass klass, void *rvalue, FFI_TYPE type)
   if (klass == JvPrimClass (byte))
     {
       _Jv_CheckCast (&Byte::class$, o);
-      *(jbyte*)rvalue = ((Byte*)o)->byteValue();
+      *(ffi_arg*)rvalue = ((Byte*)o)->byteValue();
     }
   else if (klass == JvPrimClass (short))
     {
       _Jv_CheckCast (&Short::class$, o);
-      *(jshort*)rvalue = ((Short*)o)->shortValue();
+      *(ffi_arg*)rvalue = ((Short*)o)->shortValue();
     }
   else if (klass == JvPrimClass (int))
     {
       _Jv_CheckCast (&Integer::class$, o);
-      *(jint*)rvalue = ((Integer*)o)->intValue();
+      *(ffi_arg*)rvalue = ((Integer*)o)->intValue();
     }
   else if (klass == JvPrimClass (long))
     {
@@ -302,12 +302,12 @@ unbox (jobject o, jclass klass, void *rvalue, FFI_TYPE type)
   else if (klass == JvPrimClass (boolean))
     {
       _Jv_CheckCast (&Boolean::class$, o);
-      *(jboolean*)rvalue = ((Boolean*)o)->booleanValue();
+      *(ffi_arg*)rvalue = ((Boolean*)o)->booleanValue();
     }
   else if (klass == JvPrimClass (char))
     {
       _Jv_CheckCast (&Character::class$, o);
-      *(jchar*)rvalue = ((Character*)o)->charValue();
+      *(ffi_arg*)rvalue = ((Character*)o)->charValue();
     }
   else
     JvFail ("Bad ffi type in proxy");
diff --git a/libjava/testsuite/libjava.jar/ReturnInvocationHandler.java b/libjava/testsuite/libjava.jar/ReturnInvocationHandler.java
new file mode 100644
index 0000000..18b52b7
--- /dev/null
+++ b/libjava/testsuite/libjava.jar/ReturnInvocationHandler.java
@@ -0,0 +1,24 @@
+import java.lang.reflect.InvocationHandler;
+import java.lang.reflect.Method;
+
+public class ReturnInvocationHandler implements InvocationHandler
+{
+  private Object obj;
+  public ReturnInvocationHandler(Object obj)
+  {
+    this.obj = obj;
+  }
+  public Object invoke(Object proxy, Method m, Object[] args) throws Throwable
+  {
+    Object result;
+    try
+    {
+      result = m.invoke(obj, args);
+    }
+    catch (Exception e)
+    {
+      throw e;
+    }
+    return result;
+  }
+}
diff --git a/libjava/testsuite/libjava.jar/ReturnProxyTest.jar b/libjava/testsuite/libjava.jar/ReturnProxyTest.jar
new file mode 100644
index 0000000000000000000000000000000000000000..00daabeccc8f28d1ccde9d8bea763874ab9351a7
GIT binary patch
literal 2671
zcma);3p`Y5AIA^Ir7`3-xs*_u#$_}mvr!C<ad|aX%%GbLMhzK^VMk4p%b-?-nq_w|
zxnENW6B}|Xm0MV=Ntw2d30;<QDeX~jW%|7Pw&#4F=bZEToag)epXYyhelAW@5Gb&8
z-5`uQe0ce9w*puL5b#7Bl%uoVCK7Q4Aaw^7;V}WWaHc#U0U47507x9zxriZlTk#S0
zAY!ShEk_V+oE`1(ZbTD;-Dlbo@hcjEOr!c!!Wbxb3O$S(OtaA4Y=Va3=oB)8;)mK9
zX`zcYG2enZ<m(G1kf}5j?g%+7%tF_d!U(6+T<F2fNFq2Ix@43ZXo4JY1zcBwt&DV}
z1OV_Q0YDox>f_&zw1{9|G6S5%flTu|LZO@ZF2O!ncL}s?seNT#a+=fkH>}m-yRCVy
zMu(*#Z5{%oRicwlsPBfXT!fFHE`Z#>uBLF+j1GUG)n2&s%_8z~iB^)Uin?job8c8S
zi`#X7;V=1U{I23eIoPCQ;}i3JT?PA0kKYf+5Q;`JvZ46gMzU8{LE-osPDBN}e9llP
z={8xqF(u2GV=*j!p>jY8ZFmCZ4iT2s_$~xWW{!7FW=cs=9#l9f?4BNX&~>~ro*4^~
z%g5nUtV?~i(~H9W9fKcMXQgFhl$|w+xE)5^*86?f&}T*odI0(6aC<l*TULk3TW5qG
z8l1>sPLTkfUIG~&d2)a4UknWmWlt#itxnRZ-h&D6y_GMCY8tYP?>C35U3)tip9@cK
z=tSE8xL!qo)Uk~VmDGCb<&XbUxpMEz6-Pr~OO$N)mz4S?eL}ikP1I-sgSm43N#yf$
z45S<UX%^RLWWCw@L>}ddjOFc1tO>J=0#{j48j@A-?<h5W1=Cd8j3oSK{6mM;csuq(
zkbUvzdJj9@8=x<TAOHaW?9b2M|F$2)i4R=wxhO3vs!8g-bz6i;<AV$eE(i}Q7m)Hj
zp?#gZMh#INDhSKwt@vZ3QnP?n^vXx^UeQfs%t@U2l%q@%F+DSVQ~kpz^ZH{L><R4?
zvglX^X$2VfnV@9Y=Wfl&AiY6|ltVKnMGHso^y3X`_6|M4C-5S>l-Q=-JsQwpG$d^h
z<I$Vwu0S}9;8yD~3-_9YY%fyZ<(zSuCdXdvai8C1@@$BWY-^c*c?PS~J@A^TPZ}{L
zRp*gPBLc^4HsPF-wmZ6ABx)SRk#o)j{#YjGV{L;q;D%8(T9t4eRMv1`E&nc>cu~j~
zLQT-sbC`VZ404lce(QBozjlj?)3qTCzrqK?@e}#NnqB5uzankmoXxyPQ0gy{L5rOm
zez=3GWtY|2wXJ%aypXK|w^zp-9?+Z4A<zcS=^ohRRr@=$E-JP{j_hhPd(r<;AUuI%
zW{bur*Od0Z$yE>VtUiR>){m{^JPy)muV!n|(8a{Zag@yc>Qm!G#8A~*xZpzeyY=oZ
z##3nIu|7Xn2i<DTk<^$sTLm3b^jn3KV>M;xGXlc4jPddjmZJ3vDVHAYRy9kg?~ltz
z=+7kO7w!;UU?1{)b;dQK<FYpA=W~@5h3>P3R|u6EFV+~0-#qNumbEdb#`e~XvC~|*
zLC(2Xx9PC{a?;h)tlm`F>(2#9!tII8il^8OT5`SRJ*7SOk(Qd4KO<K5zRCnjdWR>p
zsFgi3ck?cjq4KFSvyjO-oYvKw64w-a)Mwe=*DoE~b}Z&B>klRMMjsYMB+E8E7-C6}
zL$*lx?_hZd>k^*oJ2PiXRu-|Cc%gb+o)x$F`;DS|N%M>gR@||Yu02tj(|x76@l(#7
z^we_SxQ)n%=;Zgt7FE{SaE4wAEo5G~!A@tSx46+BYvm2-7>HEaA+{AhH~9s}=T?gs
zILu-Jyap_H^I%i{oDjsw5K7p$lYwxCpcsQziz`|LVz9-Wp5#^|YoD0So0UDL45`*T
z_r|uxmGNHyYddF=tLvHff-R0`cE6dCscP%9OKIM|;(*61fuOCWJ-T_*Y`UyIXHQ7O
zs&xY}X9Ody>{R8u0Sj`~twg$)<{1mc)hJj3Y*nanJyT$*lW_<3TvLFlBpTFh9gHux
z9UvJ-)TW)EJ;B8UX`enBb2tSLPpIl~zdEX+-E1ZzA7R9l^lGx^o6$wTw+Lm$H=x$A
zOaD8t!drn=;Y&sMumO%iAxFNw3Gu0Sont>#c7u0UiTema^&v$jM+B?YgM4DBThF$~
z*Yli_3ffwb^y}#+`!nlaNU)GITdG_z`lAB#H&J2i(Ps;fRh!J&h^_2FZeqGa7bftf
z-^Ia`Nh+~d|LA6^g^VZD9u)l4f#`(IV3D}F8=NTCtz4ZPfoTR}G_Tmk{#OZAI5d0S
zRj|Rx!`5_4CWa?W8rl=LyZrSw7{Ux8zt#YI>n&4N{z(S|61mU6j+<P>o6cy&M7>Jj
zou86#u?{KmU)fK;u_5cU5!SNKt?u!jqn2w<dLLDoUyUHm2lN^8^+<LD+m72$Ca;1i
z^j!`p%tJ)br_uIilLbwh`6y5G-@45o2=P__8E&5%Y(k8$wb;kUP}LXAzl$!sR-mZp
zCGW?nE=pU}7qzvnrDD0>FXUaEBqY~L{gZ;w0K;+VQUWGzmY&P>g!D%}@zweIHBMYj
ze8hoX2J%nhmgB^<#s4FJIYPuG!6!qP$U`9c8v+5G;(v&PkH&wQLj06kK1DGYrC%wQ
zPZ9d>(Bk>N9L<;hgci&Ge}@&%qUG3YGGf@}IrUFyX>bSt$bh#dxLj+wrLVsMqYL<D

literal 0
HcmV?d00001

diff --git a/libjava/testsuite/libjava.jar/ReturnProxyTest.java b/libjava/testsuite/libjava.jar/ReturnProxyTest.java
new file mode 100644
index 0000000..bdd0ba9
--- /dev/null
+++ b/libjava/testsuite/libjava.jar/ReturnProxyTest.java
@@ -0,0 +1,27 @@
+import java.lang.reflect.Proxy;
+
+public class ReturnProxyTest
+{
+  public static void main(String[] args)
+  {
+    ReturnTypes orig = new ReturnTypesImpl();
+    Object o = Proxy.newProxyInstance(orig.getClass().getClassLoader(),
+      new Class<?>[] { ReturnTypes.class },
+      new ReturnInvocationHandler(orig));
+    ReturnTypes rt = (ReturnTypes)o;
+
+    System.out.println(orig.getBoolean());
+    System.out.println(orig.getChar());
+    System.out.println(orig.getByte());
+    System.out.println(orig.getShort());
+    System.out.println(orig.getInt());
+    System.out.println(orig.getLong());
+
+    System.out.println(rt.getBoolean());
+    System.out.println(rt.getChar());
+    System.out.println(rt.getByte());
+    System.out.println(rt.getShort());
+    System.out.println(rt.getInt());
+    System.out.println(rt.getLong());
+  }
+}
diff --git a/libjava/testsuite/libjava.jar/ReturnProxyTest.out b/libjava/testsuite/libjava.jar/ReturnProxyTest.out
new file mode 100644
index 0000000..b141f06
--- /dev/null
+++ b/libjava/testsuite/libjava.jar/ReturnProxyTest.out
@@ -0,0 +1,12 @@
+false
+a
+-1
+-1
+-1
+-1
+false
+a
+-1
+-1
+-1
+-1
diff --git a/libjava/testsuite/libjava.jar/ReturnProxyTest.xfail b/libjava/testsuite/libjava.jar/ReturnProxyTest.xfail
new file mode 100644
index 0000000..73ffe1d
--- /dev/null
+++ b/libjava/testsuite/libjava.jar/ReturnProxyTest.xfail
@@ -0,0 +1 @@
+main=ReturnProxyTest
diff --git a/libjava/testsuite/libjava.jar/ReturnTypes.java b/libjava/testsuite/libjava.jar/ReturnTypes.java
new file mode 100644
index 0000000..9fbd6bd
--- /dev/null
+++ b/libjava/testsuite/libjava.jar/ReturnTypes.java
@@ -0,0 +1,9 @@
+public interface ReturnTypes
+{
+  public short getShort();
+  public char getChar();
+  public byte getByte();
+  public int getInt();
+  public long getLong();
+  public boolean getBoolean();
+}
diff --git a/libjava/testsuite/libjava.jar/ReturnTypesImpl.java b/libjava/testsuite/libjava.jar/ReturnTypesImpl.java
new file mode 100644
index 0000000..33fab1b
--- /dev/null
+++ b/libjava/testsuite/libjava.jar/ReturnTypesImpl.java
@@ -0,0 +1,27 @@
+public class ReturnTypesImpl implements ReturnTypes
+{
+  public short getShort()
+  {
+    return -1;
+  }
+  public char getChar()
+  {
+    return 'a';
+  }
+  public byte getByte()
+  {
+    return -1;
+  }
+  public int getInt()
+  {
+    return -1;
+  }
+  public long getLong()
+  {
+    return -1;
+  }
+  public boolean getBoolean()
+  {
+    return false;
+  }
+}
--
2.2.1

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix FFI return type for proxy classes

Tom Tromey-2
>>>>> "Matthew" == Matthew Fortune <[hidden email]> writes:

Matthew> Tested on: x86_64-pc-linux-gnu (default and -m32), mips-linux-gnu
Matthew> mipsel-linux-gnuabi64 with no regressions. The new test only failed
Matthew> on mips-linux-gnu prior to patching libjava.

Matthew> libjava/
Matthew> * java/lang/reflect/natVMProxy.cc (unbox): Use ffi_arg for
Matthew> integer return types smaller than a word.
Matthew> * testsuite/libjava.jar/ReturnInvocationHandler.java: New file.
Matthew> * testsuite/libjava.jar/ReturnProxyTest.jar: Likewise.
Matthew> * testsuite/libjava.jar/ReturnProxyTest.java: Likewise.
Matthew> * testsuite/libjava.jar/ReturnProxyTest.out: Likewise.
Matthew> * testsuite/libjava.jar/ReturnProxyTest.xfail: Likewise.
Matthew> * testsuite/libjava.jar/ReturnTypes.java: Likewise.
Matthew> * testsuite/libjava.jar/ReturnTypesImpl.java: Likewise.

Thanks for writing this.
This is ok.

Tom
Reply | Threaded
Open this post in threaded view
|

RE: [PATCH] Fix FFI return type for proxy classes

Matthew Fortune
Tom Tromey <[hidden email]> writes:

> >>>>> "Matthew" == Matthew Fortune <[hidden email]> writes:
>
> Matthew> Tested on: x86_64-pc-linux-gnu (default and -m32), mips-linux-gnu
> Matthew> mipsel-linux-gnuabi64 with no regressions. The new test only failed
> Matthew> on mips-linux-gnu prior to patching libjava.
>
> Matthew> libjava/
> Matthew> * java/lang/reflect/natVMProxy.cc (unbox): Use ffi_arg for
> Matthew> integer return types smaller than a word.
> Matthew> * testsuite/libjava.jar/ReturnInvocationHandler.java: New file.
> Matthew> * testsuite/libjava.jar/ReturnProxyTest.jar: Likewise.
> Matthew> * testsuite/libjava.jar/ReturnProxyTest.java: Likewise.
> Matthew> * testsuite/libjava.jar/ReturnProxyTest.out: Likewise.
> Matthew> * testsuite/libjava.jar/ReturnProxyTest.xfail: Likewise.
> Matthew> * testsuite/libjava.jar/ReturnTypes.java: Likewise.
> Matthew> * testsuite/libjava.jar/ReturnTypesImpl.java: Likewise.
>
> Thanks for writing this.
> This is ok.

Thanks for the review.

Committed as r238312. This also needs a backport to GCC 5 and 6 but I will
give it a week or so before proposing.

Thanks,
Matthew