public inbox for java-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] Fix FFI return type for proxy classes
@ 2016-07-12 10:39 Matthew Fortune
  2016-07-12 17:06 ` Tom Tromey
  0 siblings, 1 reply; 3+ messages in thread
From: Matthew Fortune @ 2016-07-12 10:39 UTC (permalink / raw)
  To: 'gcc-patches@gcc.gnu.org' (gcc-patches@gcc.gnu.org),
	java-patches
  Cc: Tom Tromey, aurelien

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2016-07-13 21:37 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-12 10:39 [PATCH] Fix FFI return type for proxy classes Matthew Fortune
2016-07-12 17:06 ` Tom Tromey
2016-07-13 21:37   ` Matthew Fortune

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).