* [PATCH] Fix FFI return type for closures in the java interpreter
@ 2016-06-27 20:21 Matthew Fortune
2016-06-27 21:23 ` Tom Tromey
0 siblings, 1 reply; 8+ messages in thread
From: Matthew Fortune @ 2016-06-27 20:21 UTC (permalink / raw)
To: java-patches,
'gcc-patches@gcc.gnu.org' (gcc-patches@gcc.gnu.org)
Cc: tromey, per, aph, Aurelien Jarno
Hi,
I've identified a latent bug in the java interpreter that affects MIPS
n32 and n64 ABIs both little and big endian and, I presume, any 64-bit
big endian target with int as 32-bit.
A full description is in my original post:
https://gcc.gnu.org/ml/java-patches/2016-q2/msg00020.html
Patch tested on x86_64-pc-linux-gnu where the new testcase works both
before and after with no regressions. Also tested on mips64el-linux-gnu
where the test fails before and works after with no regressions.
Aurelien provided the test case and I tried to follow an existing test
to integrate it. Let me know if there is anything else I need to do.
I mentioned in my earlier post about a possible similar issue in the
lang/reflect/natVMProxy.cc code (unbox function) by code inspection. I
don't know how to trigger this code but perhaps someone can advise.
OK to commit?
Thanks,
Matthew
libjava/
* interpret-run.cc: Use ffi_arg for FFI integer return types.
libjava/testsuite/
* libjava.jar/arraysort.java: New file.
* libjava.jar/arraysort.jar: New file.
* libjava.jar/arraysort.out: New file.
* libjava.jar/arraysort.xfail: New file.
---
libjava/interpret-run.cc | 2 +-
libjava/testsuite/libjava.jar/arraysort.jar | Bin 0 -> 1864 bytes
libjava/testsuite/libjava.jar/arraysort.java | 44 ++++++++++++++++++++++++++
libjava/testsuite/libjava.jar/arraysort.out | 10 ++++++
libjava/testsuite/libjava.jar/arraysort.xfail | 1 +
5 files changed, 56 insertions(+), 1 deletion(-)
create mode 100644 libjava/testsuite/libjava.jar/arraysort.jar
create mode 100644 libjava/testsuite/libjava.jar/arraysort.java
create mode 100644 libjava/testsuite/libjava.jar/arraysort.out
create mode 100644 libjava/testsuite/libjava.jar/arraysort.xfail
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:
diff --git a/libjava/testsuite/libjava.jar/arraysort.jar b/libjava/testsuite/libjava.jar/arraysort.jar
new file mode 100644
index 0000000000000000000000000000000000000000..ee051e45a836f99563e3e15ce748795d01e87e4e
GIT binary patch
literal 1864
zcmWIWW@h1H00Eo28y;W=l;C8LVeoYgan$wnbJGtE;bdT59F`k?28c^5xEUB(K+3>G
z0MG~#AcuqDY3&V<s1}nIrHl*=T}%uNf<Pk@i;5B}i}Q<0R1Ec!a}tY-!AAWH%?;m%
zX;f*Dum2$jfm(0wsh>hc|A-l0JQ1!kM^U9M!>OsNp&~w^S#_!D?W9NbpB}La^B>!6
zf679aMLA>g&6#s+KR<h$W5562J;soUmj#kK3N?RDe(mvo$tuTG1GhgD!*%lbwYTq`
z_@Y`$iRYz)^+%u8mr9QD#}u8P$TM+Q=Ipp;$CKjkB(5wo7O)aI^60{bV-vDwZ`(1m
zr`oBk^1KjFl)c9C7drR<WnTTbB$GdT^-L%4_C{Xb*ZY^8^?Q87WtO7V^EXyJGwma|
z*DlT5+<i67xX!%ot%Z<>(F@shrQeTEeQ8w}Z?^h=dPVkenOk>e|0rMi-+iCS#$T&;
zxb+?@$^5Q+eCgY*AKuMvY}5IuFJ*KjdYf*_<wxtZW1e^{b`o1w9&#e%%9ie+5}$b!
z`NiWG?hkNSy5$wePtO$x4e#dioe#RF-ZAA=@RE7rKRg^3Ns70eJ+pCn%a@IvNiUh*
zX6AIQeJb>eb%V2AOY9rgI(hadp)!}ML_B6{E_pAF8h>l&Hv~Th#@!2G-0~yG9}%$^
zeA`brP$af}+PN(~Q~1icBJwUtiwKBj>~#=bGtFDV$*IHe@;8UJEyia{vJUOGe>C}g
z5|fLP`~&$P#;@#{rzvdpe4g^{X7<~y-{U?%zP{a?Ve^ei7Y;qRo*QkyE^?dl!P6HC
zm(*VfPQ7d6lPlI&aWRo2)B5(#-mTwnp8dM^>D#As&*rwx+cP`7yr)PjJ9O2qwbs+-
zs#%@2FezQ0SKL-{d5O=zWi8Kcv{lZib91l^){$QJ&?fnVOY&}>duu`;3B5DiVye=2
zo^64`iUrJ!Z1Klfe;iz(mC@R-IK$j$U*LL?9eWpxpR!@#Z(6b{WPQkVR&LM!clSdD
zZhu|O&n)ySzt_Qfo#mcI3O$aKQm<x8-3?DT?3F8cNzvxIb|&Xxp1uDst-I1&)Op)T
zi6_UtL9vJHW%3cB++~g_J{lDbo>_kD!*)g{N%TBRDxb7|=4GQK9^Lm!Ofv;es2mP4
ze)r#q<H?pLk+cl<zS7c@7ex1RFT7)U#=GLgg_|iHTymY)maO8K>U%4})!c`Zd%OFq
zppy}cCcZE-DiKM&a4pjD7`K&|QJBQ9JGriFnmx|4swlJ8CETphbhur&a)Goh<C;fX
zTej~1vhI9Lgs&rCzDL6c9_LTjj;+s`o0s?C#=0Jx+)sX`e-At~%jC-lyQ}vpbg>Ux
ze!fI;)CNNh%gay7wyn9IwWm#R(yN93y@?JxCi`{Xi&8l3U2pHAohveP$&4RcV;yc<
zJO2)E*|%eZUlrS((_vPv=dGm{C|b?j(K&zNYqh)2*;ws;Us#p4-(x-5qyAZPvigg?
z2QJL3`p={MFt}&+#o!~4A6iz;OL`@;SzeO+xW4`0f0c(<2{B*rGirLWv8>3u<B3gP
zh{ZBH*H+h;*7KeS%asOtS6tmPEhBV(7U%q5F^gBdGMcD<ZldoTg_6E-r7z#CN`kX)
zslHhlRC^`cUR1_Gytd)*m5&^4hK(GPB#zn~+t5&`wfh}kDf8@iTQWG`8GK|%<P}hH
z`7P2pDFImWr2w%Aa&hVF=;!I?8XThM>jp15TQQ3}(bI-p4F)_62ligDPYK!J$}aBC
zdhnoU0l(Bj1x*#fkKbQa6&M#;dFZZDnPpUd;mEDsjvT$;F4)^|+1QpOq7?Bd(VXkx
z4E=i-_XQq260Ng$wqc=!U&vbq*AEwUp3N=S5AbGWl4HhIyh{LG4FUoTZyiB2Qen>u
zDeN%{dXRCrih7uF3=B&eoq@(-DfofLf(m{D#-f%$$i^}Q%O$w6u=0qop_owxF%(!!
f0o|(4isW^ah{F{MtZbm*VgbTXpb>Y0`WYAikEqBO
literal 0
HcmV?d00001
diff --git a/libjava/testsuite/libjava.jar/arraysort.java b/libjava/testsuite/libjava.jar/arraysort.java
new file mode 100644
index 0000000..56c181d
--- /dev/null
+++ b/libjava/testsuite/libjava.jar/arraysort.java
@@ -0,0 +1,44 @@
+import java.util.Arrays;
+import java.util.Comparator;
+
+public class arraysort
+{
+ private static final Comparator<String> STRING_COMPARATOR = new Comparator<String>()
+ {
+ public int compare(String str1, String str2)
+ {
+ return str1.compareTo(str2);
+ }
+ };
+
+ static void dumpArray(String[] strings)
+ {
+ int i;
+
+ for (i = 0 ; i < strings.length ; i++)
+ {
+ System.out.println("[" + i + "] " + strings[i]);
+ }
+ }
+
+ public static void main(String[] args)
+ {
+ String[] strings;
+
+ strings = new String[4];
+
+ strings[0] = "a";
+ strings[1] = "c";
+ strings[2] = "b";
+ strings[3] = "d";
+
+ System.out.println("Array of string, before:");
+ dumpArray(strings);
+
+ Arrays.sort(strings, STRING_COMPARATOR);
+
+ System.out.println("Array of string, after:");
+ dumpArray(strings);
+ }
+}
+
diff --git a/libjava/testsuite/libjava.jar/arraysort.out b/libjava/testsuite/libjava.jar/arraysort.out
new file mode 100644
index 0000000..938ce9f
--- /dev/null
+++ b/libjava/testsuite/libjava.jar/arraysort.out
@@ -0,0 +1,10 @@
+Array of string, before:
+[0] a
+[1] c
+[2] b
+[3] d
+Array of string, after:
+[0] a
+[1] b
+[2] c
+[3] d
diff --git a/libjava/testsuite/libjava.jar/arraysort.xfail b/libjava/testsuite/libjava.jar/arraysort.xfail
new file mode 100644
index 0000000..2bbbe56
--- /dev/null
+++ b/libjava/testsuite/libjava.jar/arraysort.xfail
@@ -0,0 +1 @@
+main=arraysort
--
2.4.1
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix FFI return type for closures in the java interpreter
2016-06-27 20:21 [PATCH] Fix FFI return type for closures in the java interpreter Matthew Fortune
@ 2016-06-27 21:23 ` Tom Tromey
2016-06-28 8:09 ` Tom Tromey
0 siblings, 1 reply; 8+ messages in thread
From: Tom Tromey @ 2016-06-27 21:23 UTC (permalink / raw)
To: Matthew Fortune
Cc: java-patches,
'gcc-patches@gcc.gnu.org' (gcc-patches@gcc.gnu.org),
per, aph, Aurelien Jarno
>>>>> "Matthew" == Matthew Fortune <Matthew.Fortune@imgtec.com> writes:
Matthew> I've identified a latent bug in the java interpreter that affects MIPS
Matthew> n32 and n64 ABIs both little and big endian and, I presume, any 64-bit
Matthew> big endian target with int as 32-bit.
Thanks.
Matthew> I mentioned in my earlier post about a possible similar issue in the
Matthew> lang/reflect/natVMProxy.cc code (unbox function) by code inspection. I
Matthew> don't know how to trigger this code but perhaps someone can advise.
It's a bit complicated, and it's been a while since I looked at any of
this, but I think what you want to do is make an InvocationHandler that
handles some method returning "int" (maybe hashCode would work), then
make a Proxy class that wraps it. Then, make an instance of the proxy
class and call the method.
Matthew> libjava/
Matthew> * interpret-run.cc: Use ffi_arg for FFI integer return types.
Matthew> libjava/testsuite/
Matthew> * libjava.jar/arraysort.java: New file.
Matthew> * libjava.jar/arraysort.jar: New file.
Matthew> * libjava.jar/arraysort.out: New file.
Matthew> * libjava.jar/arraysort.xfail: New file.
This is ok.
Tom
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix FFI return type for closures in the java interpreter
2016-06-27 21:23 ` Tom Tromey
@ 2016-06-28 8:09 ` Tom Tromey
2016-06-28 15:50 ` Matthew Fortune
0 siblings, 1 reply; 8+ messages in thread
From: Tom Tromey @ 2016-06-28 8:09 UTC (permalink / raw)
To: Tom Tromey
Cc: Matthew Fortune, java-patches,
'gcc-patches@gcc.gnu.org' (gcc-patches@gcc.gnu.org),
per, aph, Aurelien Jarno
>>>>> "Matthew" == Matthew Fortune <Matthew.Fortune@imgtec.com> writes:
Matthew> I've identified a latent bug in the java interpreter that affects MIPS
Matthew> n32 and n64 ABIs both little and big endian and, I presume, any 64-bit
Matthew> big endian target with int as 32-bit.
[...]
Matthew> libjava/
Matthew> * interpret-run.cc: Use ffi_arg for FFI integer return types.
Matthew> libjava/testsuite/
Matthew> * libjava.jar/arraysort.java: New file.
Matthew> * libjava.jar/arraysort.jar: New file.
Matthew> * libjava.jar/arraysort.out: New file.
Matthew> * libjava.jar/arraysort.xfail: New file.
Tom> This is ok.
It occurred to me that this might not be correct on platforms using the
Java raw API; which I think is just x86.
I'm actually not sure -- I don't remember (if I ever knew) if the raw
API has the same return-value promotion rules as the ordinary API.
Could you check? I think a -m32 build ought to show it. Maybe your
x86-64 build already did this?
thanks,
Tom
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH] Fix FFI return type for closures in the java interpreter
2016-06-28 8:09 ` Tom Tromey
@ 2016-06-28 15:50 ` Matthew Fortune
2016-06-28 21:44 ` Tom Tromey
0 siblings, 1 reply; 8+ messages in thread
From: Matthew Fortune @ 2016-06-28 15:50 UTC (permalink / raw)
To: Tom Tromey
Cc: java-patches,
'gcc-patches@gcc.gnu.org' (gcc-patches@gcc.gnu.org),
per, aph, Aurelien Jarno
Tom Tromey <tom@tromey.com> writes:
> >>>>> "Matthew" == Matthew Fortune <Matthew.Fortune@imgtec.com> writes:
> Matthew> I've identified a latent bug in the java interpreter that affects MIPS
> Matthew> n32 and n64 ABIs both little and big endian and, I presume, any 64-bit
> Matthew> big endian target with int as 32-bit.
> [...]
>
> Matthew> libjava/
> Matthew> * interpret-run.cc: Use ffi_arg for FFI integer return types.
> Matthew> libjava/testsuite/
> Matthew> * libjava.jar/arraysort.java: New file.
> Matthew> * libjava.jar/arraysort.jar: New file.
> Matthew> * libjava.jar/arraysort.out: New file.
> Matthew> * libjava.jar/arraysort.xfail: New file.
>
> Tom> This is ok.
>
> It occurred to me that this might not be correct on platforms using the
> Java raw API; which I think is just x86.
>
> I'm actually not sure -- I don't remember (if I ever knew) if the raw
> API has the same return-value promotion rules as the ordinary API.
>
> Could you check? I think a -m32 build ought to show it. Maybe your
> x86-64 build already did this?
I'm not sure this will matter if the only arch is x86 as ffi_arg will be
32-bit anyway there. There would need to be a 64bit arch using the raw
api. I don't really understand what the raw api is, the references to it
in the code seemed cryptic.
I can do a -m32 test anyway, I only did x86_64 as I don't think that
32-bit architectures will be affected at all.
> Matthew> I mentioned in my earlier post about a possible similar issue in the
> Matthew> lang/reflect/natVMProxy.cc code (unbox function) by code inspection. I
> Matthew> don't know how to trigger this code but perhaps someone can advise.
>
> It's a bit complicated, and it's been a while since I looked at any of
> this, but I think what you want to do is make an InvocationHandler that
> handles some method returning "int" (maybe hashCode would work), then
> make a Proxy class that wraps it. Then, make an instance of the proxy
> class and call the method.
Thanks for the pointers, a bit of googling got me to the right code. I can
confirm that my guess was right and the same bug exists in the natVMProxy.cc
code. It will break for all big endian targets when using types smaller than
a word. I have only proved the bug exists so far but not tried a fix.
Thanks,
Matthew
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix FFI return type for closures in the java interpreter
2016-06-28 15:50 ` Matthew Fortune
@ 2016-06-28 21:44 ` Tom Tromey
2016-07-12 10:20 ` Matthew Fortune
0 siblings, 1 reply; 8+ messages in thread
From: Tom Tromey @ 2016-06-28 21:44 UTC (permalink / raw)
To: Matthew Fortune
Cc: Tom Tromey, java-patches,
'gcc-patches@gcc.gnu.org' (gcc-patches@gcc.gnu.org),
per, aph, Aurelien Jarno
>>>>> "Matthew" == Matthew Fortune <Matthew.Fortune@imgtec.com> writes:
Matthew> I'm not sure this will matter if the only arch is x86 as
Matthew> ffi_arg will be 32-bit anyway there.
Aha, right. Thanks for looking.
Matthew> There would need to be a
Matthew> 64bit arch using the raw api. I don't really understand what
Matthew> the raw api is, the references to it in the code seemed
Matthew> cryptic.
IIRC it's to exploit the x86 calling convention to make ffi calls a bit
more efficient for libgcj.
Tom
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH] Fix FFI return type for closures in the java interpreter
2016-06-28 21:44 ` Tom Tromey
@ 2016-07-12 10:20 ` Matthew Fortune
2016-07-12 16:32 ` Tom Tromey
0 siblings, 1 reply; 8+ messages in thread
From: Matthew Fortune @ 2016-07-12 10:20 UTC (permalink / raw)
To: Tom Tromey
Cc: java-patches,
'gcc-patches@gcc.gnu.org' (gcc-patches@gcc.gnu.org),
per, aph, Aurelien Jarno
Tom Tromey <tom@tromey.com> writes:
> >>>>> "Matthew" == Matthew Fortune <Matthew.Fortune@imgtec.com> writes:
>
> Matthew> I'm not sure this will matter if the only arch is x86 as
> Matthew> ffi_arg will be 32-bit anyway there.
>
> Aha, right. Thanks for looking.
>
> Matthew> There would need to be a
> Matthew> 64bit arch using the raw api. I don't really understand what
> Matthew> the raw api is, the references to it in the code seemed
> Matthew> cryptic.
>
> IIRC it's to exploit the x86 calling convention to make ffi calls a bit
> more efficient for libgcj.
Sorry for the long delay...
I have tested this now with -m32 multilib on x86_64-pc-linux-gnu and there
are no regressions.
> Matthew> libjava/
> Matthew> * interpret-run.cc: Use ffi_arg for FFI integer return types.
> Matthew> libjava/testsuite/
> Matthew> * libjava.jar/arraysort.java: New file.
> Matthew> * libjava.jar/arraysort.jar: New file.
> Matthew> * libjava.jar/arraysort.out: New file.
> Matthew> * libjava.jar/arraysort.xfail: New file.
>
> This is ok.
> Could you check? I think a -m32 build ought to show it. Maybe your
> x86-64 build already did this?
Still OK to commit?
Thanks,
Matthew
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix FFI return type for closures in the java interpreter
2016-07-12 10:20 ` Matthew Fortune
@ 2016-07-12 16:32 ` Tom Tromey
2016-07-13 21:36 ` Matthew Fortune
0 siblings, 1 reply; 8+ messages in thread
From: Tom Tromey @ 2016-07-12 16:32 UTC (permalink / raw)
To: Matthew Fortune
Cc: Tom Tromey, java-patches,
'gcc-patches@gcc.gnu.org' (gcc-patches@gcc.gnu.org),
per, aph, Aurelien Jarno
>>>>> "Matthew" == Matthew Fortune <Matthew.Fortune@imgtec.com> writes:
Matthew> Sorry for the long delay...
No problem.
>> This is ok.
>> Could you check? I think a -m32 build ought to show it. Maybe your
>> x86-64 build already did this?
Matthew> Still OK to commit?
Yes, thanks.
Tom
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH] Fix FFI return type for closures in the java interpreter
2016-07-12 16:32 ` Tom Tromey
@ 2016-07-13 21:36 ` Matthew Fortune
0 siblings, 0 replies; 8+ messages in thread
From: Matthew Fortune @ 2016-07-13 21:36 UTC (permalink / raw)
To: Tom Tromey
Cc: java-patches,
'gcc-patches@gcc.gnu.org' (gcc-patches@gcc.gnu.org),
per, aph, Aurelien Jarno
Tom Tromey <tom@tromey.com> writes:
> >>>>> "Matthew" == Matthew Fortune <Matthew.Fortune@imgtec.com> writes:
>
> Matthew> Sorry for the long delay...
>
> No problem.
>
> >> This is ok.
> >> Could you check? I think a -m32 build ought to show it. Maybe your
> >> x86-64 build already did this?
>
> Matthew> Still OK to commit?
>
> Yes, thanks.
Committed as r238311. I'll wait a short while before proposing backport
to GCC 5 and 6 branches.
Matthew
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-07-13 21:36 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-27 20:21 [PATCH] Fix FFI return type for closures in the java interpreter Matthew Fortune
2016-06-27 21:23 ` Tom Tromey
2016-06-28 8:09 ` Tom Tromey
2016-06-28 15:50 ` Matthew Fortune
2016-06-28 21:44 ` Tom Tromey
2016-07-12 10:20 ` Matthew Fortune
2016-07-12 16:32 ` Tom Tromey
2016-07-13 21:36 ` 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).