Harmony
  1. Harmony
  2. HARMONY-2259

[drlvm][jit][opt] tests.api.java.lang.reflect.ProxyTest 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/ia32
    • Patch Info:
      Patch Available

      Description

      $subj.

      shell> $HARMONY -Xem:jet -cp $classlib/depends/jars/junit_3.8.2/junit.jar:$classlib/modules/luni/bin/test:$classlib/deploy/build/test/support.jar junit.textui.TestRunner tests.api.java.lang.reflect.ProxyTest ....
      Time: 0.109

      OK (4 tests)

      shell> $HARMONY -Xem:opt -cp $classlib/depends/jars/junit_3.8.2/junit.jar:$classlib/modules/luni/bin/test:$classlib/deploy/build/test/support.jar junit.textui.TestRunner tests.api.java.lang.reflect.ProxyTest
      ..F..
      Time: 0.465
      There was 1 failure:
      1) test_newProxyInstanceLjava_lang_ClassLoader$Ljava_lang_ClassLjava_lang_reflect_InvocationHandler(tests.api.java.lang.reflect.ProxyTest)junit.framework.AssertionFailedError: Problem converting exception
      at tests.api.java.lang.reflect.ProxyTest.test_newProxyInstanceLjava_lang_ClassLoader$Ljava_lang_ClassLjava_lang_reflect_InvocationHandler(ProxyTest.java:145)
      at java.lang.reflect.VMReflection.invokeMethod(Native Method)

      FAILURES!!!
      Tests run: 4, Failures: 1, Errors: 0

      1. HARMONY-2259.reg.patch
        3 kB
        George Timoshenko
      2. HARMONY-2259.reg.patch
        4 kB
        George Timoshenko
      3. HARMONY-2259.patch
        3 kB
        George Timoshenko
      4. HARMONY-2259.patch
        3 kB
        George Timoshenko
      5. HARMONY-2259.patch
        4 kB
        George Timoshenko
      6. H2259.java
        2 kB
        George Timoshenko

        Activity

        Hide
        Alexey Varlamov added a comment -

        Applied at r489054, thank you!

        Show
        Alexey Varlamov added a comment - Applied at r489054, thank you!
        Hide
        George Timoshenko added a comment -

        Egor,

        thank a lot for your note.

        reg test patch fixed

        (Yes, I've run reg tests)

        Show
        George Timoshenko added a comment - Egor, thank a lot for your note. reg test patch fixed (Yes, I've run reg tests)
        Hide
        Egor Pasko added a comment -

        George, regression test for this issue should be run in OPT mode. You can see how to do that in HARMONY-2261 regression patch.
        BTW, did you try "./build.sh reg.test"?

        Show
        Egor Pasko added a comment - George, regression test for this issue should be run in OPT mode. You can see how to do that in HARMONY-2261 regression patch. BTW, did you try "./build.sh reg.test"?
        Hide
        George Timoshenko added a comment -

        regression test for the issue

        Show
        George Timoshenko added a comment - regression test for the issue
        Hide
        Egor Pasko added a comment -

        George, thanks for the developed comment! I like the latest patch!

        Show
        Egor Pasko added a comment - George, thanks for the developed comment! I like the latest patch!
        Hide
        George Timoshenko added a comment -

        > * // fixup empty catch blocks <-- this is not a comment that explains the situation, can it be improved?

        Show
        George Timoshenko added a comment - > * // fixup empty catch blocks <-- this is not a comment that explains the situation, can it be improved?
        Hide
        George Timoshenko added a comment -

        >* can we avoid (2) so that our HLO CFG is always more readable?
        Probably we can do the same thing in (after) translator (not in CodeSelector as I've suggested).
        Fell free to check if it is possible.

        >* can the test be reduced (and converted to a regression patch)?
        H2259.java is reduced enough IMO.

        Show
        George Timoshenko added a comment - >* can we avoid (2) so that our HLO CFG is always more readable? Probably we can do the same thing in (after) translator (not in CodeSelector as I've suggested). Fell free to check if it is possible. >* can the test be reduced (and converted to a regression patch)? H2259.java is reduced enough IMO.
        Hide
        Egor Pasko added a comment -

        So, the problem is:
        1. CodeSelector expects CatchInst to be in_the CatchBlock (to create the out-edge)
        2. Catch instruction has been moved out of catch nodes (due to some translator optimization, probably)

        my questions are:

        • can we avoid (2) so that our HLO CFG is always more readable?
        • // fixup empty catch blocks <-- this is not a comment that explains the situation, can it be improved?
        • can the test be reduced (and converted to a regression patch)?
        Show
        Egor Pasko added a comment - So, the problem is: 1. CodeSelector expects CatchInst to be in_the CatchBlock (to create the out-edge) 2. Catch instruction has been moved out of catch nodes (due to some translator optimization, probably) my questions are: can we avoid (2) so that our HLO CFG is always more readable? // fixup empty catch blocks <-- this is not a comment that explains the situation, can it be improved? can the test be reduced (and converted to a regression patch)?
        Hide
        George Timoshenko added a comment -

        some details of CodeSelector's bad behavior:

        In HLO catch handlers are defined by two insructions:

        PseudoCatchInst - is actually a LabelInst that headed the first BB after a dispatch block.
        CatchInst - an instruction that defines an operand (which contains the exception)

        For the pattern above we have three exception types that matches to the one catch handler. In CFG it looks loke this (The fourth exception type is excluded from consideration):

        DISPATCH
        / | \
        / | \
        / | \
        CatchBlock1 CatchBlock2 CatchBlock3
        \ | /
        \ | /
        \ | /
        BasicBlock
        (CatchHandler body starts here)

        PseudoCatchInst(java/lang/Error) - starts CatchBlock1
        PseudoCatchInst(java/lang/RuntimeException) - starts CatchBlock2
        PseudoCatchInst(SubException) - starts CatchBlock3

        CatchInst(java/lang/Throwable) - starts the catch handler's body. It's type is Throwable as it is common ancestor for other three types.

        Now to the CodeSelector. It does not care about PseudoCatchInsts at all.
        Firstly it creates a set of nodes. And selects instructions one by one into them. (PseudoCatch is ignored but the CatchInst is not).
        Nodes that corresponds to CB1, CB2 and CB3 are empty.
        Secondly selector creates edges between nodes basing on the last instructions of the nodes.
        (As CB1, CB2 and CB3 are empty no edges appeare between them and catch handler's body starter)

        So the information on that exception types is lost and java/lang/Throwable starts being directed to the wrong HANDLER4.
        (CodeGenerator keeps exception types on the edges, not on the Instructions as it is being done by HLO)

        The solution I proposed redirects DISPATCH->CatchBlock[1,2,3] edges to the catch handler's body starter skipping all the way from CatchBlock[1,2,3] to the real excpetion handler.

        Show
        George Timoshenko added a comment - some details of CodeSelector's bad behavior: In HLO catch handlers are defined by two insructions: PseudoCatchInst - is actually a LabelInst that headed the first BB after a dispatch block. CatchInst - an instruction that defines an operand (which contains the exception) For the pattern above we have three exception types that matches to the one catch handler. In CFG it looks loke this (The fourth exception type is excluded from consideration): DISPATCH / | \ / | \ / | \ CatchBlock1 CatchBlock2 CatchBlock3 \ | / \ | / \ | / BasicBlock (CatchHandler body starts here) PseudoCatchInst(java/lang/Error) - starts CatchBlock1 PseudoCatchInst(java/lang/RuntimeException) - starts CatchBlock2 PseudoCatchInst(SubException) - starts CatchBlock3 CatchInst(java/lang/Throwable) - starts the catch handler's body. It's type is Throwable as it is common ancestor for other three types. Now to the CodeSelector. It does not care about PseudoCatchInsts at all. Firstly it creates a set of nodes. And selects instructions one by one into them. (PseudoCatch is ignored but the CatchInst is not). Nodes that corresponds to CB1, CB2 and CB3 are empty. Secondly selector creates edges between nodes basing on the last instructions of the nodes. (As CB1, CB2 and CB3 are empty no edges appeare between them and catch handler's body starter) So the information on that exception types is lost and java/lang/Throwable starts being directed to the wrong HANDLER4. (CodeGenerator keeps exception types on the edges, not on the Instructions as it is being done by HLO) The solution I proposed redirects DISPATCH->CatchBlock [1,2,3] edges to the catch handler's body starter skipping all the way from CatchBlock [1,2,3] to the real excpetion handler.
        Hide
        George Timoshenko added a comment -

        improved version

        Show
        George Timoshenko added a comment - improved version
        Hide
        George Timoshenko added a comment -

        patch is not tested yet

        H2259.java passes with it

        Show
        George Timoshenko added a comment - patch is not tested yet H2259.java passes with it
        Hide
        George Timoshenko added a comment -

        Huh

        it was really interesting.

        this "magic" method: "/$Proxy0::string" has 4 exception catchers and 2 exception handlers:

        1 java/lang/Error HANDLER_1
        2 java/lang/RuntimeException HANDLER_1
        3 SubException HANDLER_1
        4 java/lang/Throwable HANDLER_2

        HANDLER_1 - just rethrows an exception

        HANDLER_2 creates UndeclaredThrowableException and throws it

        the problem was that CodeSelector of Jitrino::OPT transformed this "picture" and in CG we had:

        1.SubException HANDLER_1
        2 java/lang/Throwable HANDLER_2

        The patch is being prepared

        Show
        George Timoshenko added a comment - Huh it was really interesting. this "magic" method: "/$Proxy0::string" has 4 exception catchers and 2 exception handlers: 1 java/lang/Error HANDLER_1 2 java/lang/RuntimeException HANDLER_1 3 SubException HANDLER_1 4 java/lang/Throwable HANDLER_2 HANDLER_1 - just rethrows an exception HANDLER_2 creates UndeclaredThrowableException and throws it the problem was that CodeSelector of Jitrino::OPT transformed this "picture" and in CG we had: 1.SubException HANDLER_1 2 java/lang/Throwable HANDLER_2 The patch is being prepared
        Hide
        George Timoshenko added a comment -

        Oops. I was wrong.

        Interpreter is OK. (I did not know that -Xint does not mean interpreter)
        the testcase passes with -Xem:interpreter

        Show
        George Timoshenko added a comment - Oops. I was wrong. Interpreter is OK. (I did not know that -Xint does not mean interpreter) the testcase passes with -Xem:interpreter
        Hide
        George Timoshenko added a comment -

        The testcase H2259 is made from ProxyTest.
        The failure is the same in opt-, server, server-static modes.

        Interesting thing: interpreter (-Xint) fails exactly the same

        The problem is (I've investigated OPT mode):

        The 'invoke' method throws ArrayStoreException as it is supposed.
        The 'string' method contains respective exception handler. It looks correct.
        (I looked into compilation log files for these methods)

        But the exception (it's type) is magically (i have not got this when walking through assembler at runtime) being transformed from ArrayStoreException to UndeclaredThrowableException.

        Does anybody know something about this mechanism?

        Show
        George Timoshenko added a comment - The testcase H2259 is made from ProxyTest. The failure is the same in opt-, server, server-static modes. Interesting thing: interpreter (-Xint) fails exactly the same The problem is (I've investigated OPT mode): The 'invoke' method throws ArrayStoreException as it is supposed. The 'string' method contains respective exception handler. It looks correct. (I looked into compilation log files for these methods) But the exception (it's type) is magically (i have not got this when walking through assembler at runtime) being transformed from ArrayStoreException to UndeclaredThrowableException. Does anybody know something about this mechanism?
        Hide
        George Timoshenko added a comment -

        Short testcase

        Show
        George Timoshenko added a comment - Short testcase

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development