Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JDK8 OpenJ9 Gives Different Results from HotSpot and Android Runtime #15306

Closed
connglli opened this issue Jun 13, 2022 · 26 comments · Fixed by eclipse-omr/omr#6747
Closed

JDK8 OpenJ9 Gives Different Results from HotSpot and Android Runtime #15306

connglli opened this issue Jun 13, 2022 · 26 comments · Fixed by eclipse-omr/omr#6747

Comments

@connglli
Copy link

Java version

openjdk version "1.8.0_342-internal"
OpenJDK Runtime Environment (build 1.8.0_342-internal-_2022_06_10_15_18-b00)
Eclipse OpenJ9 VM (build master-3d06b2f9c, JRE 1.8.0 Linux amd64-64-Bit Compressed References 20220610_000000 (JIT enabled, AOT enabled)
OpenJ9   - 3d06b2f9c
OMR      - cf8ddbd1a
JCL      - 2bb179375a based on jdk8u342-b05)

Javac version

javac 1.8.0_342-internal

Summary of problem

The following test makes OpenJ9 (with above version) to produce different result every time run.

class Test {
  int N;
  long instanceCount;
  int iFld;
  float fFld;
  long vMeth_check_sum;

  void vMeth(long l1, short s2, double d1) {
    int i6, i8, i10, i11 = 12, i12 = 210;
    for (i6 = 7; i6 < 149; ++i6)
      for (i8 = 1; i8 < 4; i8++)
        for (i10 = 1; i10 < 2; ++i10) {
          i11 += i12;
          iFld = i10;
        }
    vMeth_check_sum += i11;
  }

  int iMeth1(boolean b1, float f1, int i5) {
    float fArr1[] = new float[N];
    for (int ax$11 = 0; ax$11 < 6361; ax$11 += 1)
      vMeth(5063039435690207991L, (short) 1019863587, 0.2494915619941982);
    long meth_res = Double.doubleToLongBits(checkSum(fArr1));
    return (int) meth_res;
  }

  int iMeth(long l, short s1, float f) {
    int i1, i4 = 11;
    boolean b = true;
    byte[] byArr = new byte[N];
    for (i1 = 11; i1 < 212; i1++) i4 *= iMeth1(b, 1.457F, i1);
    long meth_res = checkSum(byArr);
    return (int) meth_res;
  }

  void mainTest(String[] strArr1) {
    short s = 20160;
    s = (short) iMeth(instanceCount, s, fFld);
    System.out.println(vMeth_check_sum);
  }

  public static void main(String[] strArr) {
    Test _instance = new Test();
    _instance.mainTest(strArr);
  }

  public static long checkSum(byte[] a) {
    long sum = 0;
    for (int j = 0; j < a.length; j++) {
      sum += (byte) (a[j] / (j + 1) + a[j] % (j + 1));
    }
    return sum;
  }

  public static double checkSum(float[] a) {
    double sum = 0;
    for (int j = 0; j < a.length; j++) {
      sum += (a[j] / (j + 1) + a[j] % (j + 1));
    }
    return sum;
  }
}

Results of OpenJ9: every time you run it, OpenJ9 gives a different result:

$ ./OpenJ9/jdk8/bin/java Test
70388485152
$ ./OpenJ9/jdk8/bin/java Test
66974154792
$ ./OpenJ9/jdk8/bin/java Test
80631476232

However, HotSpot (JDK8/11/17) and the Android Runtime (ART) are always giving a deterministic and the same result:

$ ./HotSpot/jdk8u/bin/java Test
114395409792
$ ./HotSpot/jdk11u/bin/java Test
114395409792
$ ./HotSpot/jdk17u/bin/java Test
114395409792
$ ./Android/host-art/host/linux-x86/bin/art --no-compile --64 -- -cp test.jar Test
114395409792
@pshipton
Copy link
Member

pshipton commented Jun 13, 2022

When I try it I often get the correct answer, but occasionally I see a wrong answer.
It also occurs on the last release jdk8u332 - 0.32, and on jdk8u322 - 0.30. I didn't try earlier versions.
Running with -Xint produces the correct answer, I'll start by assuming this is a JIT issue.

The following runs it in a loop so it's easier to reproduce the failures.

import java.io.*;

public class Run {
public static void main(String[] args) throws Exception {
	long count = 0;
	while(true) {
		ProcessBuilder pb = new ProcessBuilder(args);
		Process proc = pb.start();
		InputStream stream = proc.getInputStream();
		BufferedReader reader = new BufferedReader(new InputStreamReader(stream));
		String line = reader.readLine();
		if (!line.equals("114395409792")) {
			System.out.println("Iteration " + count + ": " + line);
		}
		count += 1;
	}
}
}

@pshipton
Copy link
Member

This seems a serious problem so I'm starting with a blocker status for the next release.

@0xdaryl pls take a look.

@pshipton
Copy link
Member

pshipton commented Jun 13, 2022

I also tried it on plinux (11.0.14.1) where it was (almost) always giving the wrong answer.

@pshipton
Copy link
Member

pshipton commented Jun 13, 2022

I found that on plinux, the problem still reproduces with a smaller case. Even on xlinux you can remove the checkSum() of the arrays and still reproduce it.

class TestCalc {
  long vMeth_check_sum;

  void vMeth() {
    int i6, i8, i10, i11 = 12, i12 = 210;
    for (i6 = 7; i6 < 149; ++i6)
      for (i8 = 1; i8 < 4; i8++)
        for (i10 = 1; i10 < 2; ++i10) {
          i11 += i12;
        }
    vMeth_check_sum += i11;
  }

  int iMeth1() {
    for (int ax$11 = 0; ax$11 < 6361; ax$11 += 1)
      vMeth();
    return 0;
  }

  int iMeth() {
    int i1, i4 = 11;
    for (i1 = 11; i1 < 212; i1++) i4 *= iMeth1();
    return 0;
  }

  void mainTest(String[] strArr1) {
    iMeth();
    System.out.println(vMeth_check_sum);
  }

  public static void main(String[] strArr) {
    TestCalc _instance = new TestCalc();
    _instance.mainTest(strArr);
  }
}

@pshipton
Copy link
Member

You can change the loop in iMeth() or the outer loop in vMeth(), but if the inner loops in vMeth() are modified to start from zero, then the problem stops occurring.

@0xdaryl
Copy link
Contributor

0xdaryl commented Jun 14, 2022

Seems to occur when Test.iMeth1() is compiled at scorching. ExpressionsSimplification may be the culprit.

@connglli
Copy link
Author

@pshipton

Could you please check if the following Test.java is a duplicate of this issue? Though they looks quite different, they have similar symptoms to each other, i.e., they give different results every time you run it.

If it is indeed a duplicate, just skip it and I won't report other tests in our hands that behave similar symptoms; otherwise, I will start a new issue thread for you guys.

class Test {
  final int N = 256;
  long instanceCount;
  float fFld;
  int iFld;
  short sFld;
  long vMeth_check_sum;

  void vMeth(int i2) {
    int i17 = -244, i19 = 23560, iArr[] = new int[N], iArr2[][] = new int[N][N];
    init(iArr2, 11);
    for (int i3 : iArr) {
      int i16;
      i3 = (int) instanceCount++;
      i2 *= instanceCount;
      switch ((i2 >>> 1) % 6 + 6) {
        case 6:
          for (i17 = 2; i17 > 1; i17 -= 3) i16 = (int) instanceCount;
        case 7:
          iArr2[(i17 >>> 1) % N][i19 % N] *= fFld;
        case 8:
          break;
        case 10:
          vMeth_check_sum += checkSum(iArr2);
      }
    }
  }

  void vSmallMeth(int i, short s, int i1) {
    vMeth(i);
  }

  void mainTest(String[] strArr1) {
    for (int smallinvoc = 0; smallinvoc < 238; smallinvoc++) vSmallMeth(25982, sFld, iFld);
    System.out.println(vMeth_check_sum);
  }

  public static void main(String[] strArr) {
    boolean ax$16;
    Test _instance = new Test();
    _instance.mainTest(strArr);
  }

  public static void init(int[][] a, int seed) {
    for (int j = 0; j < a.length; j++) {
      init(a[j], seed);
    }
  }

  public static void init(int[] a, int seed) {
    for (int j = 0; j < a.length; j++) {
      a[j] = (j % 2 == 0) ? seed + j : seed - j;
    }
  }

  public static long checkSum(int[] a) {
    long sum = 0;
    for (int j = 0; j < a.length; j++) {
      sum += (a[j] / (j + 1) + a[j] % (j + 1));
    }
    return sum;
  }

  public static long checkSum(int[][] a) {
    long sum = 0;
    for (int j = 0; j < a.length; j++) {
      sum += checkSum(a[j]);
    }
    return sum;
  }
}

FYI. The result of HotSpot JDK8/11/17 and the Android Runtime is -7725525924.

Great thanks that for your patience on validating our bug reports. You guys are one of the most active and optimistic developers I've met. :-)

@pshipton
Copy link
Member

I assume @0xdaryl or a delegate will do that investigation.

@connglli
Copy link
Author

Thanks for you patience and enthusiasm @pshipton, as well as @0xdaryl.

@pshipton
Copy link
Member

We have questions about using the test cases you've provided in our regression test suite. Are there any existing licenses or concerns about putting the test cases under the Eclipse v2 license? @llxia is interested in having you open a Pull Request to deliver the test cases, if you are are agreeable to that. We'd try to make it easy so you don't need to be concerned much about test frameworks. You'd have to sign the Eclipse ECA agreement for that, which is the typical requirement for delivering code.
See https://github.com/eclipse-openj9/openj9/blob/master/CONTRIBUTING.md

@connglli
Copy link
Author

As for the first question, we're working on an automated testing tool based on many existing fuzzers; but it hasn't been released yet. Currently, tests that we have reported is a hybrid of our tool and a fuzzer namely Java*Fuzzer which is licensed by Apache V2. I'm not quite familiar with the licenses, but as far as I know Apache V2 grants you the permission to adapt/modify their code. For ours, you are free to use them. Therefore, I think it's sufficient that you add the Java*Fuzzer's license when including then in your regression test suites.

As for the second question, sorry that currently we don't have that time to make formal contributions using PR following some guidelines. Issuetracker is much more convenient to us at present. But we could consider that when we are free.

A big thanks for your invitation.

@pshipton
Copy link
Member

Sounds good, thanks. Eclipse OpenJ9 is actually dual licensed under Eclipse v2 or Apache v2. We can accept other compatible licenses as well.

@0xdaryl
Copy link
Contributor

0xdaryl commented Jun 21, 2022

This is not a 0.33 regression. It fails at least as far back as 0.24 with JDK11.

@0xdaryl
Copy link
Contributor

0xdaryl commented Jun 30, 2022

A proper fix won't be ready for 0.33 and won't have time to soak in the head stream. This is not a regression, so moving this out to 0.34. @pshipton

@connglli
Copy link
Author

The following is a much simpler test for this bug (buggy when a() is compiled also at scorching level)

class T {
  void a() {
    int c = 3, d, f = 34829, x = 11801, y = 56517;
    for (d = 4; d < 101; ++d) {
      c += 58095;
      for (int ax$8 = -3901; ax$8 < 2545; ax$8 += 9)
        try {
          short[] ax$4 = {}; ax$4[x] = (short) f;
        } catch (Throwable ax$7) {}
      for (x = 2; x < 52; ++x) { y = (int) 196L; y -= c; }
    }
    System.out.println(y);
  }

  public static void main(String[] args) {
    T t = new T();
    for (int i = 0; i < 10; i++) t.a();
  }
}

@hzongaro
Copy link
Member

Peter @pshipton, having looked at another problem with ExpressionsSimplification (#15534), I think it might be safer to push this out to the next release rather than trying to rush in a fix for 0.35.

@hzongaro
Copy link
Member

@connglli, regarding the test you provided in your comment of June 15, it appears that that is a different problem. The incorrect output produced with that test can be avoided by specifying the option -Xjit:disableGlobalVP. It seems to be expose a bug in the Global Value Propagation optimization.

@connglli
Copy link
Author

Hi, @hzongaro. Thanks for your effort on reviewing my bug reports.

@hzongaro
Copy link
Member

It looks like the test case in the comment of 27 July exhibits a slightly different problem in ExpressionSimplification than the problem seen in the original test case.

In the original test, Expressions Simplification calculates the value of i11 when it pulls it out of the loop, it multiplies the value of i12 by the wrong number of iterations in calculating the final value of i11.

  void vMeth(long l1, short s2, double d1) {
    int i6, i8, i10, i11 = 12, i12 = 210;
    for (i6 = 7; i6 < 149; ++i6)
      for (i8 = 1; i8 < 4; i8++)
        for (i10 = 1; i10 < 2; ++i10) {
          i11 += i12;
          iFld = i10;
        }
    vMeth_check_sum += i11;
  }

In the case of the smaller test case, the problem is that ExpressionSimplification transforms the assignment in this loop

 for (x = 2; x < 52; ++x) { y = (int) 196L; y -= c; }

into the following, failing to recognize that every iteration of the loop is setting y to 196-c, not performing a summation.

y = 196 - c*50;
for (x = 2; x < 52; ++x);

@connglli
Copy link
Author

That's interesting. Thanks @hzongaro

@hzongaro
Copy link
Member

That second problem also affects negation and exclusive-or operators:

class TFor2 {
  void negate(int j) {
    int i = 1;
    for (int x = 2; x < 52; ++x) { i = -j; }
    System.out.println(i);
  }

  void xor(int j) {
    int i = 1;
    for (int x = 2; x < 52; ++x) { i = 2 ^ j; }
    System.out.println(i);
  }

  public static void main(String[] args) {
    TFor2 tfor2 = new TFor2();
    for (int i = 0; i < 10; i++) tfor2.negate(17);
    for (int i = 0; i < 10; i++) tfor2.xor(17);
  }
}

Running with java -Xint TFor2, the output is

-17
-17
-17
-17
-17
-17
-17
-17
-17
-17
19
19
19
19
19
19
19
19
19
19

Running with java -Xjit:count=1,disableAsyncCompilation,{TFor2.[xn]*}\(optLevel=scorching\) TFor2

-17
-2090610944
-2090610944
-2090610944
-2090610944
-2090610944
-2090610944
-2090610944
-2090610944
-2090610944
19
-2090610944
-2090610944
-2090610944
-2090610944
-2090610944
-2090610944
-2090610944
-2090610944
-2090610944

@hzongaro
Copy link
Member

hzongaro commented May 8, 2023

Hi, @connglli. I wanted to follow up on a comment from last year, asking whether you would be willing to contribute tests that you've created to OpenJ9. You mentioned in a response that you might be able to contribute tests when you had free time. Have you given this any further thought?

@connglli
Copy link
Author

connglli commented May 8, 2023

Sure @hzongaro, I could. I still have some mis-computations/mis-compilations in hand. But firstly, I have to check if they have been fixed by your recent updates. If not, I'll create a PR ASAP after this Friday because I have an important talk on that day and I have to prepare for it these days. This may take a couple of days. Are you or @llxia still willing to help me with the PR as mentioned in the previous comment when I'm all ready?

@llxia
Copy link
Contributor

llxia commented May 8, 2023

Thanks @connglli . Once you are ready, could you create a new folder under https://github.com/eclipse-openj9/openj9/tree/master/test/functional with your test cases? You wouldn't need to adapt the tests to the testing framework that we use. We will take it from there and incorporate the tests into our framework/pipeline.

You would need to sign the Eclipse Foundation Contributor Agreement. Please see Contributing to Eclipse OpenJ9.

@connglli
Copy link
Author

connglli commented May 12, 2023

@llxia @hzongaro @pshipton I have added some tests in PR 17404. I have prepended a "WIP" in the title as learned from Contributing to Eclipse OpenJ9 such that you can work on it. Feel free to ping me if you need any further help or if I have understood you incorrectly. Thanks!

@connglli
Copy link
Author

connglli commented Oct 11, 2023

As for the first question, we're working on an automated testing tool based on many existing fuzzers; but it hasn't been released yet. Currently, tests that we have reported is a hybrid of our tool and a fuzzer namely Java*Fuzzer which is licensed by Apache V2. I'm not quite familiar with the licenses, but as far as I know Apache V2 grants you the permission to adapt/modify their code. For ours, you are free to use them. Therefore, I think it's sufficient that you add the Java*Fuzzer's license when including then in your regression test suites.

As for the second question, sorry that currently we don't have that time to make formal contributions using PR following some guidelines. Issuetracker is much more convenient to us at present. But we could consider that when we are free.

A big thanks for your invitation.

@pshipton @hzongaro @llxia Hi all, our work has finally been accepted by SOSP 2023 and our tool has been released under Artemis. For anyone who is interested in the technical details, feel free to navigate to our paper for technical details! Hope Artemis helps you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment