Details

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

      Description

      === Description ===

      The patch adds checks of two constraints, fixes
      exception data flow issue and made arrangements
      for further subroutine inlining implementation.

      While adding a number of new checks, the patch
      reduces the total legth of the code by 75 lines
      and reduces verifier memory usage by removing
      structures which are not used. At least
      removing dead data just improves readability.

      === Testing ===

      Before the patch the test WideGoto.class hangs
      on verification stage.

      After the patch a verifier correctly reports:

      Uncaught exception in main:
      java.lang.VerifyError: (class: WideGoto,
      method: main([Ljava/lang/String;)V) wide should
      be followed by iload, fload, aload, lload,
      dload, istore, fstore, astore, lstore, dstore,
      ret or iinc

      The patch passes acceptance tests.

      Both patches and non-patched versions fail on
      the same thread manager assertion when trying
      to run eclipse.

      === Changes ===

      Below goes a detailed list of changes:

      • Added verification of wide instructions.
        Added verification of the total bytecode
        length.
      • Added a new file for subroutine
        implementation, added a reference to the file
        to MSVC project. Added design of subroutine
        inlining algorithm.
      • Added type flags for graph nodes and an
        appropriate constructor to create nodes of
        different types. Added an assertion to get
        instruction range only for
        VF_TYPE_NODE_CODE_RANGE nodes.
      • Simplified checks of a node type removing
        access to a last code instruction of a
        node. Removed artificial instructions for
        handler nodes and start/end nodes.
      • Removed service functions to work with
        artificial instructions. Moved instruction
        stack maps to the corresponding node maps.
        Removed second parsing of method signature when
        creating method IN and OUT maps.
      • Two times decreased a size of bytecode
        annotation structures and completely removed
        offset structures for such structures. Added
        annotations to vf_Context.
      • Removed a dynamic vector of exception
        handlers for each instruction.
      • Removed debug flag macros which are no
        longer used in the current version.
      • Simplified edge pre-counting algorithm by
        noticing that each basic block except the last
        produces at least one OUT edge, so we just need
        to make action about those blocks which branch
        execution. Used local counters in loops instead
        of one global counter to foster compiler
        optimizations.
      • Made vf_Graph class getters inline putting
        their definitions in the header file. Added
        GetEdgeCount getter. Removed unused SetNode
        method (should be added CopyNode instead for
        subroutine inlining).
      • Added a new reporter macro to add class
        and method names to any report.
      • Adopted debug facilities to work with new
        data structures.
      • When parsing a class file or getting
        exception information used local unsigned short
        type instead of reused and casted int.

      === Formatting ===

      I changed formatting of the code I touched.

      • Fixed English and removed excessive "This
        function ..." in documentation. Added
        Doxygen style documentation using @param and
        @return tags for new functions.
      • Renamed "deep" -> "depth" and "begin" ->
        "start" in variable and function names using
        input from a focus group from my cubicle.
      • Started using class library C style for
        brackets and spaces in function names.
        Consistently followed C style for local
        variable names and functions (low caps with
        underscore). Left Windows/JNI style (camel
        style with the first letter in method name
        uppercased) for C++ constructs.
      • To my ear getting a number of nodes
        doesn't imply that we are getting all nodes. So
        I renamed GetNodeNumber to GetNodeCount.
      • Reduced repeated long indirect pointer
        chains context->a->b[i]->c caching in a local
        variable.
      • Reformat long lines to fit 72 symbols.
      1. WideGoto.j
        0.4 kB
        Alexei Fedotov
      2. WideGoto.class
        0.2 kB
        Alexei Fedotov
      3. ManyLocals.java
        5 kB
        Alexei Fedotov
      4. verifier_2.patch
        164 kB
        Alexei Fedotov
      5. verifier_3.patch
        164 kB
        Alexei Fedotov
      6. verifier_4.patch
        168 kB
        Alexei Fedotov
      7. verifier_m_i_3.patch
        220 kB
        Alexei Fedotov
      8. Verifier-patch-A-litle-addon.patch
        125 kB
        Pavel Rebriy
      9. Verifier-patch-A-litle-addon.patch
        125 kB
        Pavel Rebriy

        Activity

        Hide
        Gregory Shimansky added a comment -

        VERIFIED

        Show
        Gregory Shimansky added a comment - VERIFIED
        Hide
        Pavel Rebriy added a comment -

        All changes are valid.

        Show
        Pavel Rebriy added a comment - All changes are valid.
        Hide
        Gregory Shimansky added a comment -

        Patches applied at 508525. Please check that they were applied as expected.

        I tried to create a regression test but it is not possible in the current infrastructure because WideGoto.class has to be patched after compilation. If you create a regression test for verifying wide instructions, please create subtask for this issue.

        Show
        Gregory Shimansky added a comment - Patches applied at 508525. Please check that they were applied as expected. I tried to create a regression test but it is not possible in the current infrastructure because WideGoto.class has to be patched after compilation. If you create a regression test for verifying wide instructions, please create subtask for this issue.
        Hide
        Pavel Rebriy added a comment -

        Added patch for the last changes:

        • Several functions were moved to header
        • Variable declaration is moved to the place of usage
        • Code format is corrected.
        Show
        Pavel Rebriy added a comment - Added patch for the last changes: Several functions were moved to header Variable declaration is moved to the place of usage Code format is corrected.
        Hide
        Pavel Rebriy added a comment -

        Added patch for the last changes:

        • Several functions were moved to header
        • Variable declaration is moved to the place of usage
        • Code format is corrected.
        Show
        Pavel Rebriy added a comment - Added patch for the last changes: Several functions were moved to header Variable declaration is moved to the place of usage Code format is corrected.
        Hide
        Alexei Fedotov added a comment -

        All comments are fixed.

        Note, three files are to be added:
        svn add ver_subroutine.cpp ver_subroutine.h ver_graph.h

        Show
        Alexei Fedotov added a comment - All comments are fixed. Note, three files are to be added: svn add ver_subroutine.cpp ver_subroutine.h ver_graph.h
        Hide
        Alexei Fedotov added a comment -

        Pavel Rebriy wrote me in private about verifier_4.patch

        The first letter:
        ==============

        General remarks:

        Please use option -p for diff.
        NodeHandle should be renamed to vf_NodeHandle
        If you use NodeHandle structure eliminate all notes about pointer of vf_Node_t.
        Graph::NewNode should returns NodeHandle regardless of incoming arguments thus graph looks like completed structure.
        Using signed values in union (like vf_CodeType_t or vf_NodeType_t) it is bad programming practice for my style and it is unnecessary in the context.
        It's unnecessary to put code of vf_Graph structure to ver_real.h header. It breaks reading of code.
        Function GetNodeBytecodeLen() doesn't need to operate with pointers to unsigned char, integer calculation is enough.
        All new functions and instructions should contain prefix vf_.

        Patch remarks:

        Lines 23-65: File ver_subroutine.h - Linux end of line would be preferable.
        Lines 98: Declaration of vf_get_node_stack_depth() function doesn't need doxygen-styled comment for arguments.
        Line 230: It's redundant change.
        Line 343: The local variable should be used in previous function too.
        Line 362: Type should be removed.
        Line 406: It's redundant change, comment of argument should be restored.
        Line 431: "node for each pc" -> "node for each code instruction"
        Line 435: Variable node_index should be declared before the first usage - before operator for line 440.
        Line 470: Variable bb_start should be declared within the loop.
        Line 471: Variable bytecode_len should be declared before usage - see loop at line 597.
        Line 473-474: Declaration of the loop is unreadable. Declare loop in 1 or 3 lines and move "{" to a new line.
        Line 478: Declaration of the loop is unreadable. Move "{" to a new line.
        Line 482: Add return value to NewNode, and add StackModifier for NewNode( vf_NodeType_t type) as additional parameter.
        Line 621: Map vector p_outvector should be allocated in graph pool.
        Line 1042: Type VF_TYPE_INSTR_SUBROUTINE may be change to VF_TYPE_INSTR_RET, because there is no subroutine instruction.
        Lines 1121-1132: Don't need change comments indent.
        Line 1495: It's redundant change.
        Lines 1784-1816: Remove unused function vf_get_local_var_number().
        Line 2111: It's redundant change.
        Line 2257: Bad formatting.

        The second letter:
        ==============

        General remarks:

        File ver_subroutine.cpp: Wrong end of lines.

        Patch remarks:

        Lines 845, 935, 1759, 3531, 3750: Incorrect macros usage - should be VERIFY_REPORT_METHOD instead.
        Line 3167: It's better to end_pc = (end_pc == len) ? 0 : end_pc;
        Line 3440, 3448-3450: These are redundant changes.
        Lines 3458, 3588, 3659-3675, 3688-3704: Fixing patch is missed at r501839. See HARMONY-2905.
        Lines 3796-3803, 3975-3986: Incorrect merge, see r501839.
        Line 3840: Comments of argument is missing.
        Line 3895: Variable could be declared here.
        Line 3900: Brackets () in if operator missed.

        Show
        Alexei Fedotov added a comment - Pavel Rebriy wrote me in private about verifier_4.patch The first letter: ============== General remarks: Please use option -p for diff. NodeHandle should be renamed to vf_NodeHandle If you use NodeHandle structure eliminate all notes about pointer of vf_Node_t. Graph::NewNode should returns NodeHandle regardless of incoming arguments thus graph looks like completed structure. Using signed values in union (like vf_CodeType_t or vf_NodeType_t) it is bad programming practice for my style and it is unnecessary in the context. It's unnecessary to put code of vf_Graph structure to ver_real.h header. It breaks reading of code. Function GetNodeBytecodeLen() doesn't need to operate with pointers to unsigned char, integer calculation is enough. All new functions and instructions should contain prefix vf_. Patch remarks: Lines 23-65: File ver_subroutine.h - Linux end of line would be preferable. Lines 98: Declaration of vf_get_node_stack_depth() function doesn't need doxygen-styled comment for arguments. Line 230: It's redundant change. Line 343: The local variable should be used in previous function too. Line 362: Type should be removed. Line 406: It's redundant change, comment of argument should be restored. Line 431: "node for each pc" -> "node for each code instruction" Line 435: Variable node_index should be declared before the first usage - before operator for line 440. Line 470: Variable bb_start should be declared within the loop. Line 471: Variable bytecode_len should be declared before usage - see loop at line 597. Line 473-474: Declaration of the loop is unreadable. Declare loop in 1 or 3 lines and move "{" to a new line. Line 478: Declaration of the loop is unreadable. Move "{" to a new line. Line 482: Add return value to NewNode, and add StackModifier for NewNode( vf_NodeType_t type) as additional parameter. Line 621: Map vector p_outvector should be allocated in graph pool. Line 1042: Type VF_TYPE_INSTR_SUBROUTINE may be change to VF_TYPE_INSTR_RET, because there is no subroutine instruction. Lines 1121-1132: Don't need change comments indent. Line 1495: It's redundant change. Lines 1784-1816: Remove unused function vf_get_local_var_number(). Line 2111: It's redundant change. Line 2257: Bad formatting. The second letter: ============== General remarks: File ver_subroutine.cpp: Wrong end of lines. Patch remarks: Lines 845, 935, 1759, 3531, 3750: Incorrect macros usage - should be VERIFY_REPORT_METHOD instead. Line 3167: It's better to end_pc = (end_pc == len) ? 0 : end_pc; Line 3440, 3448-3450: These are redundant changes. Lines 3458, 3588, 3659-3675, 3688-3704: Fixing patch is missed at r501839. See HARMONY-2905 . Lines 3796-3803, 3975-3986: Incorrect merge, see r501839. Line 3840: Comments of argument is missing. Line 3895: Variable could be declared here. Line 3900: Brackets () in if operator missed.
        Hide
        Alexei Fedotov added a comment -

        Added new files as a part of the patch.

        Show
        Alexei Fedotov added a comment - Added new files as a part of the patch.
        Hide
        Alexei Fedotov added a comment -

        Rebased the patch to HARMONY-2825. Note, the patch contains two new files:
        svn add vm/vmcore/src/verifier/ver_subroutine.cpp vm/vmcore/src/verifier/ver_subroutine.h

        Show
        Alexei Fedotov added a comment - Rebased the patch to HARMONY-2825 . Note, the patch contains two new files: svn add vm/vmcore/src/verifier/ver_subroutine.cpp vm/vmcore/src/verifier/ver_subroutine.h
        Hide
        Alexei Fedotov added a comment -

        Added one mre constaint: start_pc < end_pc for exception handler.

        Show
        Alexei Fedotov added a comment - Added one mre constaint: start_pc < end_pc for exception handler.
        Hide
        Alexei Fedotov added a comment -

        Improved the patch localizing wide opcode parsing. Tested with one more test.

        j=0; while true; do i=$k; k=$j; j=`expr $j + 1`; echo " double d$j = d$k + d$i;"; i=$j; done | tee ManyLocals.java

        Show
        Alexei Fedotov added a comment - Improved the patch localizing wide opcode parsing. Tested with one more test. j=0; while true; do i=$k; k=$j; j=`expr $j + 1`; echo " double d$j = d$k + d$i;"; i=$j; done | tee ManyLocals.java
        Hide
        Alexei Fedotov added a comment -

        Added a patch and a test

        Show
        Alexei Fedotov added a comment - Added a patch and a test

          People

          • Assignee:
            Gregory Shimansky
            Reporter:
            Alexei Fedotov
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development