Harmony
  1. Harmony
  2. HARMONY-1905

[drlvm][opt] assertion fails in DeadCodeEliminator

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: DRLVM
    • Labels:
      None
    • Environment:
      SUSE9
    • Patch Info:
      Patch Available

      Description

      Steps to reproduce:
      ./java -Xem:opt -cp $classlib/modules/jndi/bin/test:junit.jar junit.textui.TestRunner org.apache.harmony.jndi.tests.javax.naming.BinaryRefAddrTest

      java: /export/users2/avarlamo/linux.ia32/svn-repo/drlvm/vm/jitrino/src/optimizer/deadcodeeliminator.cpp:311: static Jitrino::Opnd* Jitrino::DeadCodeEliminator::copyPropagate(Jitrino::Opnd*): Assertion `srcInst->getNode()' failed.
      SIGABRT in VM code.
      Stack trace:
      1: ?? (??:-1)
      2: abort (??:-1)
      3: __assert_fail (??:-1)
      4: Jitrino::DeadCodeEliminator::copyPropagate(Jitrino::Opnd*) (/export/users2/avarlamo/linux.ia32/svn-repo/drlvm/vm/jitrino/src/optimizer/deadcodeeliminator.cpp:312)
      5: Jitrino::DeadCodeEliminator::copyPropagate(Jitrino::Inst*) (/export/users2/avarlamo/linux.ia32/svn-repo/drlvm/vm/jitrino/src/optimizer/deadcodeeliminator.cpp:342)
      6: Jitrino::Simplifier::optimizeInst(Jitrino::Inst*) (/export/users2/avarlamo/linux.ia32/svn-repo/drlvm/vm/jitrino/src/optimizer/simplifier.cpp:192)
      7: Jitrino::SimplifierWithInstFactory::optimizeInst(Jitrino::Inst*) (/export/users2/avarlamo/linux.ia32/svn-repo/drlvm/vm/jitrino/src/optimizer/simplifier.cpp:3943)
      8: Jitrino::SimplifierWithInstFactory::simplifyControlFlowGraph() (/export/users2/avarlamo/linux.ia32/svn-repo/drlvm/vm/jitrino/src/optimizer/simplifier.cpp:3878)
      9: Jitrino::SimplificationPass::_run(Jitrino::IRManager&) (/export/users2/avarlamo/linux.ia32/svn-repo/drlvm/vm/jitrino/src/optimizer/simplifier.cpp:71)
      10: Jitrino::OptPass::run() (/export/users2/avarlamo/linux.ia32/svn-repo/drlvm/vm/jitrino/src/optimizer/optpass.cpp:61)
      11: Jitrino::runPipeline(Jitrino::CompilationContext*) (/export/users2/avarlamo/linux.ia32/svn-repo/drlvm/vm/jitrino/src/main/Jitrino.cpp:226)
      12: Jitrino::compileMethod(Jitrino::CompilationContext*) (/export/users2/avarlamo/linux.ia32/svn-repo/drlvm/vm/jitrino/src/main/Jitrino.cpp:261)
      13: Jitrino::Jitrino::CompileMethod(Jitrino::CompilationContext*) (/export/users2/avarlamo/linux.ia32/svn-repo/drlvm/vm/jitrino/src/main/Jitrino.cpp:286)
      14: JIT_compile_method_with_params (/export/users2/avarlamo/linux.ia32/svn-repo/drlvm/vm/jitrino/src/vm/drl/DrlJITInterface.cpp:282)
      15: Dll_JIT::compile_method_with_params(void*, Method*, OpenMethodExecutionParams) (/export/users2/avarlamo/linux.ia32/svn-repo/drlvm/vm/vmcore/include/dll_jit_intf.h:86)
      16: compile_do_compilation_jit(Method*, JIT*) (/export/users2/avarlamo/linux.ia32/svn-repo/drlvm/vm/vmcore/src/jit/compile.cpp:700)
      17: vm_compile_method (/export/users2/avarlamo/linux.ia32/svn-repo/drlvm/vm/vmcore/src/class_support/C_Interface.cpp:2538)
      18: DrlEMImpl::compileMethod(Method*) (/export/users2/avarlamo/linux.ia32/svn-repo/drlvm/vm/em/src/DrlEMImpl.cpp:520)
      19: CompileMethod (/export/users2/avarlamo/linux.ia32/svn-repo/drlvm/vm/em/src/em_intf.cpp:49)
      20: compile_do_compilation (/export/users2/avarlamo/linux.ia32/svn-repo/drlvm/vm/vmcore/src/jit/compile.cpp:780)
      21: compile_jit_a_method(Method*) (/export/users2/avarlamo/linux.ia32/svn-repo/drlvm/vm/vmcore/src/jit/compile.cpp:828)
      22: IP is 0x410D0172 <native code>
      23: vm_invoke_native_array_stub (/export/users2/avarlamo/linux.ia32/svn-repo/drlvm/vm/vmcore/src/util/ia32/base/invoke_native_stub_ia32.asm:41)
      24: JIT_execute_method_default(void*, _jmethodID*, jvalue*, jvalue*) (/export/users2/avarlamo/linux.ia32/svn-repo/drlvm/vm/vmcore/src/util/ia32/base/ini_iA32.cpp:199)
      25: DrlEMImpl::executeMethod(_jmethodID*, jvalue*, jvalue*) (/export/users2/avarlamo/linux.ia32/svn-repo/drlvm/vm/em/src/DrlEMImpl.cpp:489)
      26: ExecuteMethod (/export/users2/avarlamo/linux.ia32/svn-repo/drlvm/vm/em/src/em_intf.cpp:43)
      27: vm_execute_java_method_array(_jmethodID*, jvalue*, jvalue*) (/export/users2/avarlamo/linux.ia32/svn-repo/drlvm/vm/vmcore/src/jit/ini.cpp:60)
      28: call_method_no_ref_result (/export/users2/avarlamo/linux.ia32/svn-repo/drlvm/vm/vmcore/src/jni/jni_method.cpp:165)
      29: CallVoidMethodA(JNIEnv_External*, _jobject*, _jmethodID*, jvalue*) (/export/users2/avarlamo/linux.ia32/svn-repo/drlvm/vm/vmcore/src/jni/jni_method.cpp:200)
      30: invoke_primitive_method (/export/users2/avarlamo/linux.ia32/svn-repo/drlvm/vm/vmcore/src/kernel_classes/native/java_lang_reflect_VMReflection.cpp:185)
      31: Java_java_lang_reflect_VMReflection_invokeMethod (/export/users2/avarlamo/linux.ia32/svn-repo/drlvm/vm/vmcore/src/kernel_classes/native/java_lang_reflect_VMReflection.cpp:221)
      32: IP is 0x41125C09 <native code>
      33: java/lang/reflect/Method.invoke(Ljava/lang/Object;[Ljava/lang/Object;)Ljava/lang/Object; (??:-1)
      34: junit/framework/TestCase.runTest()V (TestCase.java:154)
      35: junit/framework/TestCase.runBare()V (TestCase.java:-1)
      36: junit/framework/TestResult$1.protect()V (TestResult.java:-1)
      37: junit/framework/TestResult.runProtected(Ljunit/framework/Test;Ljunit/framework/Protectable;)V (TestResult.java:-1)
      38: junit/framework/TestResult.run(Ljunit/framework/TestCase;)V (TestResult.java:-1)
      39: junit/framework/TestCase.run(Ljunit/framework/TestResult;)V (TestCase.java:-1)
      SIGSEGV in VM code.
      Stack trace:
      40: Jitrino::Ia32::StackInfo::readByteSize(unsigned char const*) const (/export/users2/avarlamo/linux.ia32/svn-repo/drlvm/vm/jitrino/src/codegenerator/ia32/Ia32StackInfo.cpp:107)
      41: Jitrino::Ia32::RuntimeInterface::getBcLocationForNative(Jitrino::MethodDesc*, unsigned long long, unsigned short*) (/export/users2/avarlamo/linux.ia32/svn-repo/drlvm/vm/jitrino/src/codegenerator/ia32/Ia32RuntimeInterface.cpp:78)
      42: Jitrino::Jitrino::GetBcLocationForNative(Jitrino::MethodDesc*, unsigned long long, unsigned short*) (/export/users2/avarlamo/linux.ia32/svn-repo/drlvm/vm/jitrino/src/main/Jitrino.cpp:358)
      43: get_bc_location_for_native (/export/users2/avarlamo/linux.ia32/svn-repo/drlvm/vm/jitrino/src/vm/drl/DrlJITInterface.cpp:590)
      44: Dll_JIT::get_bc_location_for_native(Method*, void*, unsigned short*) (/export/users2/avarlamo/linux.ia32/svn-repo/drlvm/vm/vmcore/include/dll_jit_intf.h:240)
      45: get_file_and_line (/export/users2/avarlamo/linux.ia32/svn-repo/drlvm/vm/vmcore/src/stack/stack_trace.cpp:60)
      46: st_get_java_method_info (/export/users2/avarlamo/linux.ia32/svn-repo/drlvm/vm/vmcore/src/stack/stack_dump.cpp:196)
      47: st_print_stack(Registers*) (/export/users2/avarlamo/linux.ia32/svn-repo/drlvm/vm/vmcore/src/stack/stack_dump.cpp:242)
      48: abort_handler(int, siginfo*, void*) (/export/users2/avarlamo/linux.ia32/svn-repo/drlvm/vm/vmcore/src/util/linux/signals_ia32.cpp:614)
      <end of stack trace>

      1. HARMONY-1905.diff
        2 kB
        George Timoshenko
      2. HARMONY-1905.ver2.diff
        3 kB
        George Timoshenko
      3. HARMONY-1905.ver3.diff
        3 kB
        George Timoshenko
      4. HARMONY-1905.ver3+.diff
        0.7 kB
        George Timoshenko

        Activity

        Hide
        Alexey Varlamov added a comment -

        Committed additional comment, closing

        Show
        Alexey Varlamov added a comment - Committed additional comment, closing
        Hide
        Egor Pasko added a comment -

        we can close it after applying "HARMONY-1905.ver3+.diff"
        it is a comment, so there is no check required

        Show
        Egor Pasko added a comment - we can close it after applying " HARMONY-1905 .ver3+.diff" it is a comment, so there is no check required
        Hide
        George Timoshenko added a comment -

        Sorry.

        ver3 is obsolete as it is the same as ver2

        please find an extension in the attach ver3+ (apply onto applied ver3)

        Show
        George Timoshenko added a comment - Sorry. ver3 is obsolete as it is the same as ver2 please find an extension in the attach ver3+ (apply onto applied ver3)
        Hide
        Egor Pasko added a comment -

        last two patches are identical (HAMONY-1905.ver2.diff and HARMONY-1905.ver3.diff), George, please, update the comment.

        I am OK with publicity of phiInstsOnRightPositionsInBB(...)

        Show
        Egor Pasko added a comment - last two patches are identical (HAMONY-1905.ver2.diff and HARMONY-1905 .ver3.diff), George, please, update the comment. I am OK with publicity of phiInstsOnRightPositionsInBB(...)
        Hide
        Alexey Varlamov added a comment -

        Guys, it's a pleasure to see such proactive cooperation.
        Applied the patch #3 at r475716.
        I presume no more general fix needed, can we close this issue?

        Show
        Alexey Varlamov added a comment - Guys, it's a pleasure to see such proactive cooperation. Applied the patch #3 at r475716. I presume no more general fix needed, can we close this issue?
        Hide
        George Timoshenko added a comment -

        Comment is extended as Egor sugested.

        The method is public as it can be used by some other components for validating CFG.
        It is not necessary right now, but may be userfull somtime later.

        Show
        George Timoshenko added a comment - Comment is extended as Egor sugested. The method is public as it can be used by some other components for validating CFG. It is not necessary right now, but may be userfull somtime later.
        Hide
        Egor Pasko added a comment -

        George, I would also explicitly comment what kind of instructions are allowed before PhiInst in a BB.

        Show
        Egor Pasko added a comment - George, I would also explicitly comment what kind of instructions are allowed before PhiInst in a BB.
        Hide
        Egor Pasko added a comment -

        George, much better now. Why phiInstsOnRightPositionsInBB(...) is public?

        Show
        Egor Pasko added a comment - George, much better now. Why phiInstsOnRightPositionsInBB(...) is public?
        Hide
        George Timoshenko added a comment -

        Egor is absolutely right. The first patch is quite ugly.

        Here is the new one.

        Show
        George Timoshenko added a comment - Egor is absolutely right. The first patch is quite ugly. Here is the new one.
        Hide
        Egor Pasko added a comment -

        Alexey, please, remove the "Patch Available" status until the better patch is available

        Show
        Egor Pasko added a comment - Alexey, please, remove the "Patch Available" status until the better patch is available
        Hide
        Egor Pasko added a comment -

        George,
        the patch generally implements a good idea, but it has a lot of copy-pasted code. Could you, please, put the "Phi instruction order assumption check" into a separate function, say "bool SSABuilder::phiInstsOnRightPositionsInBB(...)" and just put it:
        "assert(phiInstsOnRightPositionsInBB(block))" in both places.

        copy-pasted code is just too expensive to support

        Show
        Egor Pasko added a comment - George, the patch generally implements a good idea, but it has a lot of copy-pasted code. Could you, please, put the "Phi instruction order assumption check" into a separate function, say "bool SSABuilder::phiInstsOnRightPositionsInBB(...)" and just put it: "assert(phiInstsOnRightPositionsInBB(block))" in both places. copy-pasted code is just too expensive to support
        Hide
        George Timoshenko added a comment -

        The problem was in incorrect fixupSSA algorithm that implies phi instructions must be the second one in a block.
        In the unit test we have a BB that is a catch one. So it's instructions were:

        LabelInst
        CatchInst
        Phi

        So Phi is only the third.

        The fix is attached.

        Show
        George Timoshenko added a comment - The problem was in incorrect fixupSSA algorithm that implies phi instructions must be the second one in a block. In the unit test we have a BB that is a catch one. So it's instructions were: LabelInst CatchInst Phi So Phi is only the third. The fix is attached.
        Hide
        Egor Pasko added a comment -

        Interesting!
        I fully support the idea of refactoring PMF utilities to help in a lightweight fix for this bug. As a temporary workaround "dce,uce,dce" might help

        Show
        Egor Pasko added a comment - Interesting! I fully support the idea of refactoring PMF utilities to help in a lightweight fix for this bug. As a temporary workaround "dce,uce,dce" might help
        Hide
        Mikhail Fursov added a comment -

        Investigation shows that the bugs is the result of the recent change "uce,dce" sequence to "dce,uce" (that was the fix for another bug)
        The reason of the failure:

        uce calls fixupSSA
        fixupSSA leaves deadcode in IR
        simplifier (in the stack above) executed after and craches (with assertion)

        This bug is not critical (assertion only), so I'll try to fix it as medium priority.

        The best fix: refactoring of whole optimizer's pipeline framework (See email thread: "drlvm][jit] New task for JIT volunteers: refactoring Jitrino.OPT optimizer's pipeline" for discussion)

        Show
        Mikhail Fursov added a comment - Investigation shows that the bugs is the result of the recent change "uce,dce" sequence to "dce,uce" (that was the fix for another bug) The reason of the failure: uce calls fixupSSA fixupSSA leaves deadcode in IR simplifier (in the stack above) executed after and craches (with assertion) This bug is not critical (assertion only), so I'll try to fix it as medium priority. The best fix: refactoring of whole optimizer's pipeline framework (See email thread: "drlvm] [jit] New task for JIT volunteers: refactoring Jitrino.OPT optimizer's pipeline" for discussion)
        Hide
        Mikhail Fursov added a comment -

        I'll take a look at this failure

        Show
        Mikhail Fursov added a comment - I'll take a look at this failure

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development