harmony-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alexei Fedotov (JIRA)" <j...@apache.org>
Subject [jira] Commented: (HARMONY-3067) [drlvm][verifier] wide instructions
Date Mon, 12 Feb 2007 12:53:13 GMT

    [ https://issues.apache.org/jira/browse/HARMONY-3067?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12472280
] 

Alexei Fedotov commented on HARMONY-3067:
-----------------------------------------

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. 

> [drlvm][verifier] wide instructions
> -----------------------------------
>
>                 Key: HARMONY-3067
>                 URL: https://issues.apache.org/jira/browse/HARMONY-3067
>             Project: Harmony
>          Issue Type: Bug
>          Components: DRLVM
>            Reporter: Alexei Fedotov
>         Attachments: ManyLocals.java, verifier_2.patch, verifier_3.patch, verifier_4.patch,
WideGoto.class, WideGoto.j
>
>
> === 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.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


Mime
View raw message