Harmony
  1. Harmony
  2. HARMONY-2261

[drlvm][jit][opt] java.awt.ScrollbarTest fails on Jitrino.OPT while passes on Jitrino.JET

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: DRLVM
    • Labels:
      None
    • Environment:
      Linux SUSE9 gcc 3.3.3 / ia32
    • Patch Info:
      Patch Available

      Description

      shell> $HARMONY -Xem:jet -Xbootclasspath/a:$classlib/depends/jars/junit_3.8.2/junit.jar:$classlib/modules/awt/bin/test:$classlib/deploy/build/test/support.jar junit.textui.TestRunner java.awt.ScrollbarTest
      .............................
      Time: 0.266

      OK (29 tests)

      shell> $HARMONY -Xem:opt -Xbootclasspath/a:$classlib/depends/jars/junit_3.8.2/junit.jar:$classlib/modules/awt/bin/test:$classlib/deploy/build/test/support.jar junit.textui.TestRunner java.awt.ScrollbarTest
      ..........F..F..............F...
      Time: 0.961
      There were 3 failures:
      1) testSetMinimum(java.awt.ScrollbarTest)junit.framework.AssertionFailedError: expected:<-11> but was:<-2147483648>
      at java.awt.ScrollbarTest.checkScrollbar(ScrollbarTest.java:46)
      at java.awt.ScrollbarTest.checkScrollbar(ScrollbarTest.java:66)
      at java.awt.ScrollbarTest.testSetMinimum(ScrollbarTest.java:170)
      at java.lang.reflect.VMReflection.invokeMethod(Native Method)
      2) testSetMaximum(java.awt.ScrollbarTest)junit.framework.AssertionFailedError: expected:<10> but was:<-2147483549>
      at java.awt.ScrollbarTest.checkScrollbar(ScrollbarTest.java:47)
      at java.awt.ScrollbarTest.checkScrollbar(ScrollbarTest.java:66)
      at java.awt.ScrollbarTest.testSetMaximum(ScrollbarTest.java:197)
      at java.lang.reflect.VMReflection.invokeMethod(Native Method)
      3) testSetValues(java.awt.ScrollbarTest)junit.framework.AssertionFailedError: expected:<-201> but was:<-2147483648>
      at java.awt.ScrollbarTest.checkScrollbar(ScrollbarTest.java:46)
      at java.awt.ScrollbarTest.checkScrollbar(ScrollbarTest.java:66)
      at java.awt.ScrollbarTest.testSetValues(ScrollbarTest.java:303)
      at java.lang.reflect.VMReflection.invokeMethod(Native Method)

      FAILURES!!!
      Tests run: 29, Failures: 3, Errors: 0

      1. Test.java
        0.7 kB
        Egor Pasko
      2. patch.diff
        2 kB
        Nikolay Sidelnikov
      3. 0001-HARMONY-2261-regression-test-ver2.txt
        2 kB
        Egor Pasko
      4. 0001-HARMONY-2261-fix-RCE-is-aware-of-integer-overflows-comments-fixed.txt
        5 kB
        Egor Pasko

        Activity

        Hide
        Egor Pasko added a comment -

        method "java/awt/Scrollbar::setValuesImpl(IIIIZ)V" is compiled incorrectly by OPT, moving it to JET makes all subtests pass
        looking into it

        Show
        Egor Pasko added a comment - method "java/awt/Scrollbar::setValuesImpl(IIIIZ)V" is compiled incorrectly by OPT, moving it to JET makes all subtests pass looking into it
        Hide
        Egor Pasko added a comment -

        this is one more bug in code generator's RCE (=Redundant Comparison Elimination)
        a short reproducer attached (Test.java)
        passes if RCE=off

        Show
        Egor Pasko added a comment - this is one more bug in code generator's RCE (=Redundant Comparison Elimination) a short reproducer attached (Test.java) passes if RCE=off
        Hide
        Nikolay Sidelnikov added a comment -

        RCE issue:
        Removing of CMP is wrong when overflow flag is significant

        Show
        Nikolay Sidelnikov added a comment - RCE issue: Removing of CMP is wrong when overflow flag is significant
        Hide
        Egor Pasko added a comment -

        Nikolay,
        great patch! although it might lead to a small slowdown, but taking care of the OF is important, I see no other way to fix this issue

        please, plovide patches relative to working_vm in future (now it is relative to working_vm/vm/jitrino/src/codegenerator/ia32/)

        with the patch.diff
        Test.java ..passes
        ScrollbarTest ..passes
        jvmti, c-unit, smoke, kernel tests ..pass
        // all on SUSE9 gcc-3.3.3 ia32

        eager to commit

        Show
        Egor Pasko added a comment - Nikolay, great patch! although it might lead to a small slowdown, but taking care of the OF is important, I see no other way to fix this issue please, plovide patches relative to working_vm in future (now it is relative to working_vm/vm/jitrino/src/codegenerator/ia32/) with the patch.diff Test.java ..passes ScrollbarTest ..passes jvmti, c-unit, smoke, kernel tests ..pass // all on SUSE9 gcc-3.3.3 ia32 eager to commit
        Hide
        Alexey Varlamov added a comment -

        Egor, do you think the test should be added to regression suite?
        If so, could you please try to provide a patch for that?

        Show
        Alexey Varlamov added a comment - Egor, do you think the test should be added to regression suite? If so, could you please try to provide a patch for that?
        Hide
        Alexei Fedotov added a comment -

        IMHO, no separate regression test is needed for this issue - exisiting class library test reproduces the problem. I believe one should create and publish an instruction how to run class library tests with Jitrino.Opt. What do you think?

        Show
        Alexei Fedotov added a comment - IMHO, no separate regression test is needed for this issue - exisiting class library test reproduces the problem. I believe one should create and publish an instruction how to run class library tests with Jitrino.Opt. What do you think?
        Hide
        Egor Pasko added a comment -

        Alexei, Alexey,

        1. HUT catches it, but only in OPT mode, which takes too long to make an alarm

        2. my test is not ideal, and very sensitive to what optimizations run inside JIT
        and in which order, this behaviour would evolve, obviously. I do not believe
        the test will catch something in the future (unless the nearest future). A
        specially crafted "JIT regression test" would suit best. I am thinking
        of one to implement.

        3. I did not try the existing "regression suite". Does it allow -Xem:opt for a
        single test?

        Show
        Egor Pasko added a comment - Alexei, Alexey, 1. HUT catches it, but only in OPT mode, which takes too long to make an alarm 2. my test is not ideal, and very sensitive to what optimizations run inside JIT and in which order, this behaviour would evolve, obviously. I do not believe the test will catch something in the future (unless the nearest future). A specially crafted "JIT regression test" would suit best. I am thinking of one to implement. 3. I did not try the existing "regression suite". Does it allow -Xem:opt for a single test?
        Hide
        Egor Pasko added a comment -

        I updated some old comments in Nikolay's patch, renamed one function with misleading name, now the code is a bit more readable:
        0001-HARMONY-2261-fix-RCE-is-aware-of-integer-overflows-comments-fixed.txt

        (please, apply this patch instead of the first one)

        Show
        Egor Pasko added a comment - I updated some old comments in Nikolay's patch, renamed one function with misleading name, now the code is a bit more readable: 0001- HARMONY-2261 -fix-RCE-is-aware-of-integer-overflows-comments-fixed.txt (please, apply this patch instead of the first one)
        Hide
        Alexei Fedotov added a comment -

        Egor, you wrote,
        > A specially crafted "JIT regression test" would suit best. I am thinking of one to implement.

        Let me summarize - you add a regression test first, then a committer continues looking into this problem. Is it correct?

        Show
        Alexei Fedotov added a comment - Egor, you wrote, > A specially crafted "JIT regression test" would suit best. I am thinking of one to implement. Let me summarize - you add a regression test first, then a committer continues looking into this problem. Is it correct?
        Hide
        Egor Pasko added a comment -

        Alexei Fedotov wrote:
        > Let me summarize - you add a regression test first, then a committer continues looking into this problem. Is it correct?

        yes, you are right! here is the patch for regression test. To see how it works:

        • apply HARMONY-2565 fix for regression tests infrastructure
        • apply this patch (0001-HARMONY-2261-regression-test-ver2)
        • edit the file build/make/targets/reg.test.run.xml commenting out failed tests (now H788, H0000, H1694 fail for me)
        • run regression tests with "./build.sh reg.test"
        • see the difference when applying the fix to this test
        • report any problems
        Show
        Egor Pasko added a comment - Alexei Fedotov wrote: > Let me summarize - you add a regression test first, then a committer continues looking into this problem. Is it correct? yes, you are right! here is the patch for regression test. To see how it works: apply HARMONY-2565 fix for regression tests infrastructure apply this patch (0001- HARMONY-2261 -regression-test-ver2) edit the file build/make/targets/reg.test.run.xml commenting out failed tests (now H788, H0000, H1694 fail for me) run regression tests with "./build.sh reg.test" see the difference when applying the fix to this test report any problems
        Hide
        Alexey Varlamov added a comment -

        Applied at r489057. Thanks!

        Show
        Alexey Varlamov added a comment - Applied at r489057. Thanks!

          People

          • Assignee:
            Alexey Varlamov
            Reporter:
            Egor Pasko
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development