[jboss-jira] [JBoss JIRA] (JASSIST-204) Getting java.lang.VerifyError when adding try/catch to a method using CtMethod.insertAfter, if the method returns when the JVM stack is not empty - Inconsistent stack height 0 != 1, or Inconsistent stackmap frames at branch target

[Dordoriko Beanie (JIRA)]

    [ https://issues.jboss.org/browse/JASSIST-204?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12803677#comment-12803677 ] 

Dordoriko Beanie commented on JASSIST-204:
------------------------------------------

Hello again Chiba-san,

I apologize for the late reply, I did not understand that you were awaiting my response.
I think that either one of the approaches that you suggested can be implemented in such a way that it will have zero cost for "normal" cases (assuming that you have the ability to calculate the stack depth for any given instruction)

- Find out the return statements or exception handling points that should be covered, and calculate the stack depth for each
Then either -
- Generate a copy of the 'insert after' statements per stack depth (and not per return point)
- Perform the goto to the correct 'insert after' chunk according to the stack depth of the particular point
Or - 
- Generate one copy of the 'insert after' statements, preceded by N pop instructions, N equal the maximum stack depth that was detected minus the minimum stack depth that was detected
- Perform the goto to the correct pop instruction, or directly to the first real instruction of 'insert after' for the least-deep return points

I believe that in both of these approaches you should be able to produce identical bytecodes for 'normal' cases that worked correctly before (because obviously in all those cases the stack depths were equal, otherwise the verify would have failed for the 'merged' points).
Then, the cost for 'unusual' cases like this one is not such a big worry - hopefully they are rare.

I have a slight preference for the code duplication approach over the dummy pop approach, but I suppose that if one approach is less complex to implement then it's fine either way.

Note that a synthetic case can be formed where the size penalty of the pop approach will be bigger (if there are two return statements with stack depth differing by 100 or so), but this is just theoretical, in reality I don't think there will be more than one or two pops.


Thank you very much,
 Dori

                
> Getting java.lang.VerifyError when adding try/catch to a method using CtMethod.insertAfter, if the method returns when the JVM stack is not empty - Inconsistent stack height 0 != 1, or Inconsistent stackmap frames at branch target
> --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
>
>                 Key: JASSIST-204
>                 URL: https://issues.jboss.org/browse/JASSIST-204
>             Project: Javassist
>          Issue Type: Bug
>    Affects Versions: 3.17.0-GA, 3.18.0-GA
>         Environment: Occurred on Windows with standard HotSpot JVM (1.6.0_24, 1.7.0_21, ..), also with IBM J9 JVM on AIX etc.
>            Reporter: Dordoriko Beanie
>            Assignee: Shigeru Chiba
>              Labels: VerifyError, height, insertBefore, stack
>         Attachments: popnopbug.zip
>
>
> After instrumenting certain classes with insertAfter(), I am consistently getting java.lang.VerifyError when trying to load the instrumented class(either "Inconsistent stack height 0 != 1" or "Inconsistent stackmap frames at branch target", depending on whether I'm on a newer 1.7 environment or an older one).  The class was perfectly loadable prior to instrumentation, even though it exhibits a slightly unusual property that its return-type instructions are not necessarily executed when the stack is empty (not counting the return parameter, if applicable).
> The problem originally occurred with some class file that was apparently weaved by AspectJ 1.5.2a, but it was then reproduced by combining the following circumstances:
> 1) That the instrumented method has the "odd" property that it has an areturn/ireturn/return instruction which is invoked when the stack is not empty (when there are one or more values that were pushed onto the current frame and weren't popped).  Note that I was unable to produce this with a freshly compiled .java source, so instead I took a simple .class file and changed one of the popping instructions to a nop instruction using a hex editor.
> 2) That the code added by insertAfter() contains a try/catch structure or something similar (which forces the stack height to be reset in case of catch).  It is possible that this wasn't really necessary and that similar symptoms could occur by just having multiple return statements with different stack heights.
> When this happens, javassist replaces the original return-type instruction with a goto (when the stack height is N), but also adds a goto to the same label from the end of the catch clause (when the stack height is 0).
> runtest_modified_nop_java17.bat
> Exception in thread "main" java.lang.VerifyError: Inconsistent stackmap frames at branch target 35 in method com.test.HelloWorld.g()Ljava/lang/String; at offset 20
>         at java.lang.Class.getDeclaredMethods0(Native Method)
>         at java.lang.Class.privateGetDeclaredMethods(Unknown Source)
>         at java.lang.Class.getMethod0(Unknown Source)
>         at java.lang.Class.getMethod(Unknown Source)
>         at sun.launcher.LauncherHelper.getMainMethod(Unknown Source)
>         at sun.launcher.LauncherHelper.checkAndLoadMain(Unknown Source)
> runtest_modified_nop_java12.bat
> Exception in thread "main" java.lang.VerifyError: (class: com/test/HelloWorld, method: g signature: ()Ljava/lang/String;) Inconsistent stack height 0 != 1
>         at java.lang.Class.getDeclaredMethods0(Native Method)
>         at java.lang.Class.privateGetDeclaredMethods(Unknown Source)
>         at java.lang.Class.getMethod0(Unknown Source)
>         at java.lang.Class.getMethod(Unknown Source)
>         at sun.launcher.LauncherHelper.getMainMethod(Unknown Source)
>         at sun.launcher.LauncherHelper.checkAndLoadMain(Unknown Source)

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira