openj9
openj9 copied to clipboard
eclipse-openj9
Inconsistent output compared to others (openjdk version:8,11,17)
Java -version output
openjdk version "1.8.0_352"
IBM Semeru Runtime Open Edition (build 1.8.0_352-b08)
Eclipse OpenJ9 VM (build openj9-0.35.0, JRE 1.8.0 Mac OS X amd64-64-Bit Compressed References 20221031_543 (JIT enabled, AOT enabled)
OpenJ9 - e04a7f6c1
OMR - 85a21674f
JCL - b213334935 based on jdk8u352-b08)
openjdk version "11.0.17" 2022-10-18
IBM Semeru Runtime Open Edition 11.0.17.0 (build 11.0.17+8)
Eclipse OpenJ9 VM 11.0.17.0 (build openj9-0.35.0, JRE 11 Mac OS X amd64-64-Bit Compressed References 20221031_510 (JIT enabled, AOT enabled)
OpenJ9 - e04a7f6c1
OMR - 85a21674f
JCL - a94c231303 based on jdk-11.0.17+8)
openjdk version "17.0.5" 2022-10-18
IBM Semeru Runtime Open Edition 17.0.5.0 (build 17.0.5+8)
Eclipse OpenJ9 VM 17.0.5.0 (build openj9-0.35.0, JRE 17 Mac OS X amd64-64-Bit Compressed References 20221018_304 (JIT enabled, AOT enabled)
OpenJ9 - e04a7f6c1
OMR - 85a21674f
JCL - 32d2c409a33 based on jdk-17.0.5+8)
Output from java -version.
Summary of problem
Given the same test program, the output of j9 is different from that of hotspot and graalvm. Hotspot and graalvm give a result of 2764148, while j9 gives 1434332532.
Diagnostic files
I reproduced it using jdk11 with -Xint so it's not caused by the JIT. The following is the disassembly of the class file, but compiling and running this java code seems to be an endless loop, so the .jasm is doing something different.
public class ChecksumDiff01 {
public static int maxN;
public static int maxM;
public static int CHECKSUM;
public static volatile short field_4 = -100;
static {
maxN = 100;
maxM = 50;
CHECKSUM = 0;
CHECKSUM = checksum(CHECKSUM, CHECKSUM);
}
public static void main(String[] paramArrayOfString) {
byte b = 0;
while (b < maxN) {
byte b1 = 0;
int i = maxN;
CHECKSUM = checksum(CHECKSUM, i);
while (b1 < b + i) {
opt1();
CHECKSUM = checksum(CHECKSUM, ++b1);
}
CHECKSUM = checksum(CHECKSUM, ++b);
}
System.out.println("CHECKSUM: " + CHECKSUM);
}
public static void opt1() {
byte b = 59;
while (b < 63) {
byte b1 = 63;
while (b1 < 75) {
CHECKSUM = checksum(CHECKSUM, field_4 *= -2);
if (maxM > -2) {
b += 4;
b1 += 4;
}
}
}
}
public static int checksum(int paramInt1, int paramInt2) {
paramInt1 += Integer.hashCode(paramInt2);
return Integer.hashCode(paramInt1);
}
public static int checksum(int paramInt, short paramShort) {
paramInt += Short.hashCode(paramShort);
return Integer.hashCode(paramInt);
}
}
I've reduced it to the following. The compiled java code gives the same answer (-34400) on both Hotspot and OpenJ9. With the jasm, Hotspot gives 96672. The difference is the java code generates some i2s instructions after the imul before the dup. If there is a problem here, it seems to be with Hotspot.
public class ChecksumDiff01 {
public static int CHECKSUM = 0;
public static short field_4 = -17200;
public static void main(String[] paramArrayOfString) {
CHECKSUM = (field_4 *= -2) + (field_4 *= -2);
System.out.println("CHECKSUM: " + CHECKSUM);
}
}
super public class ChecksumDiff01
version 52:0
{
public static Field CHECKSUM:I;
public static Field field_4:S;
static Method "<clinit>":"()V"
stack 1 locals 0
{
iconst_0;
putstatic Field CHECKSUM:"I";
sipush -17200;
putstatic Field field_4:"S";
return;
}
public static Method main:"([Ljava/lang/String;)V"
stack 3 locals 1
{
getstatic Field field_4:"S";
bipush -2;
imul;
dup;
putstatic Field field_4:"S";
getstatic Field field_4:"S";
bipush -2;
imul;
dup;
putstatic Field field_4:"S";
iadd;
putstatic Field CHECKSUM:"I";
getstatic Field java/lang/System.out:"Ljava/io/PrintStream;";
new class java/lang/StringBuilder;
dup;
invokespecial Method java/lang/StringBuilder."<init>":"()V";
ldc String "CHECKSUM: ";
invokevirtual Method java/lang/StringBuilder.append:"(Ljava/lang/String;)Ljava/lang/StringBuilder;";
getstatic Field CHECKSUM:"I";
invokevirtual Method java/lang/StringBuilder.append:"(I)Ljava/lang/StringBuilder;";
invokevirtual Method java/lang/StringBuilder.toString:"()Ljava/lang/String;";
invokevirtual Method java/io/PrintStream.println:"(Ljava/lang/String;)V";
return;
}
} // end Class ChecksumDiff01
Thanks for your reply! we have reproduced this issue, and we will submit this issue to hotspot for further investigation.
Please see:
https://bugs.openjdk.org/browse/JDK-8300025
AFAICS the jasm code does not represent the Java source code. So if there is a difference between hotspot and J9 here it seems to me to be a J9 issue.
With the following
0: getstatic #1 // Field field_4:S
3: bipush -2
5: imul
6: dup
7: putstatic #1 // Field field_4:S
10: getstatic #1 // Field field_4:S
13: bipush -2
15: imul
16: dup
17: putstatic #1 // Field field_4:S
20: iadd
21: putstatic #3 // Field CHECKSUM:I
24: getstatic #2 // Field java/lang/System.out:Ljava/io/PrintStream;
27: getstatic #3 // Field CHECKSUM:I
30: invokevirtual #4 // Method java/io/PrintStream.println:(I)V
33: return
J9: -34400 hotspot: 96672
With
0: getstatic #7 // Field field_4:S
3: bipush -2
5: imul
6: putstatic #13 // Field tmp1_s:I
9: getstatic #13 // Field tmp1_s:I
12: bipush -2
14: imul
15: putstatic #17 // Field tmp2_s:I
18: getstatic #13 // Field tmp1_s:I
21: getstatic #17 // Field tmp2_s:I
24: iadd
25: putstatic #20 // Field CHECKSUM:I
28: getstatic #23 // Field java/lang/System.out:Ljava/io/PrintStream;
31: getstatic #20 // Field CHECKSUM:I
34: invokevirtual #29 // Method java/io/PrintStream.println:(I)V
37: return
Both j9 and hotspot: -34400
Biggest difference is that the second snippet is doing getstatic again for the addition, whereas first snippet did a dup to save the params on the stack
One other thing, Im assuming the get/putstatics are not doing any narrowing on hotspot, if thats not true then that may be the difference
One other thing, Im assuming the get/putstatics are not doing any narrowing on hotspot, if thats not true then that may be the difference
That could well be the case - we did some explicit narrowing a while back but it appears to be for boolean only (and only on put).
This would also explain why the explicit i2s changes the behaviour.
So its definitely the get/putstatic
0: getstatic #1 // Field field_4:S
3: bipush -2
5: imul
6: dup
7: putstatic #6 // Field tmp1_s:I
10: putstatic #4 // Field tmp2_s:S
13: getstatic #4 // Field tmp2_s:S
16: bipush -2
18: imul
19: putstatic #2 // Field tmp3_s:I
22: getstatic #6 // Field tmp1_s:I
25: getstatic #2 // Field tmp3_s:I
28: iadd
29: putstatic #5 // Field CHECKSUM:I
32: getstatic #3 // Field java/lang/System.out:Ljava/io/PrintStream;
35: getstatic #5 // Field CHECKSUM:I
38: invokevirtual #7 // Method java/io/PrintStream.println:(I)V
41: return
J9: -34400, hotspot:96672
I don't think we have to do anything on this. The spec doesn't require or prohibit narrowing of short, char in the same way it does for boolean.
@tajila, @pshipton: please see the resolution of https://bugs.openjdk.org/browse/JDK-8319310.
There had been an assumption that writes to char/byte/short fields would truncate the int value, but that hadn't been spelled out. JVMS 22 is now spelling it out: a putfield/putstatic to types char/byte/short must truncate the int, just as is done for arrays of these types.
putstatic already special cases boolean, so we'll need 2 extra bits for 8-bit and 16-bit. I don't see any reason to distinguish char/short when narrowing.
putfield already has the bits, but they are only used in the JIT today (not set in any field description).
I don't see any reason to distinguish char/short when narrowing.
Actually, I'm probably wrong here. Since we store the fields int-sized and getfield moves them directly to a stack slot, short probably needs to be sign-extended so the int value is correct.
putfield already has the bits, but they are only used in the JIT today (not set in any field description).
This is also incorrect - the bits are set via a table in fieldutil.c.
putfield is easy, putstatic will require some thought to find those extra bits.
Presumably, we'll want Unsafe and JNI (and our private MethodHandle implementation) to act the same way.
MH is probably lower priority as we don't use it in the releases that enforce this behaviour.
Presumably, we'll want Unsafe and JNI (and our private MethodHandle implementation) to act the same way. MH is probably lower priority as we don't use it in the releases that enforce this behaviour.
Yes, we will want this in all those areas.
The fix should be back ported all the way to JDK8 for compatibility with RI.
For static fields, the flags are in the flagsAndClass field, with the low 8 bits as flags. The current layout is:
#define J9StaticFieldRefBaseType 0x1
#define J9StaticFieldRefDouble 0x2
#define J9StaticFieldRefVolatile 0x4
#define J9StaticFieldRefBoolean 0x8
#define J9StaticFieldRefPutResolved 0x10
#define J9StaticFieldRefFinal 0x20
#define J9StaticFieldIsNullRestricted 0x40
With only one bit free, we'll need to refactor the bits. There are 8 types (boolean, byte, char, short, int, long, float, double) which can be represented in the 3 bits for J9StaticFieldRefBaseType, J9StaticFieldRefDouble, J9StaticFieldRefBoolean. We can decide later if we want to put those 3 bits together (e.g. move volatile up to value 8).
@keithc-ca Please remind me of the preferred approach to different encodings of flags between releases in DDR (previously AlgorithmVersion).
Define a constant in an appropriate header file with some non-zero value (think flag encoding version) and then add the same name in CompatibilityConstants29.dat with the value zero. Then DDR can use that new constant to know how to interpret flagsAndClass.
We'll need to keep the base type bit to handle object fields. To match the instance field bits, I'll rename it so it represents object fields rather than primitive types.
If we combine the double and long cases, we can use the free bit pattern for object. I don't see any practical reason to distinguish between the the two 64-bit types, removing the need for the base type bit.
If in the future we need extra bits in the RAM ref, we could easily derive the type from the ROM ref at some cost.
I think JNI requires no modification because it uses correctly sized and signed types for the new value parameters.
Fun C fact - when casting up to a larger width, the sign or zero extension is based solely on the source type. The target type is used only for the final width and signedness of the result (i.e. (uint32_t)(int8_t)value will sign-extend value from 8 to 32 bits).
Interpreter Unsafe looks fine as well (it also uses correctly-sized and signed types in the intermediate API calls which should cause the correct things to occur when casting up to 32 bits).