Summary: | Aspectj generate code does not de-compile cleanly. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | [Tools] AspectJ | Reporter: | Ronald R. DiFRango <ron.difrango> | ||||||||||
Component: | Compiler | Assignee: | Andrew Clement <aclement> | ||||||||||
Status: | RESOLVED FIXED | QA Contact: | |||||||||||
Severity: | enhancement | ||||||||||||
Priority: | P3 | CC: | jim-aj | ||||||||||
Version: | 1.1.1 | ||||||||||||
Target Milestone: | 1.2.1 | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | Windows XP | ||||||||||||
Whiteboard: | |||||||||||||
Attachments: |
|
Description
Ronald R. DiFRango
2003-11-07 14:40:53 EST
Created attachment 6699 [details]
Source to re-produce problem.
Created attachment 6700 [details]
Source to re-prodcue problem.
Created attachment 6701 [details]
Decompiled output.
Created attachment 6702 [details]
Decompiled output.
Adding this to 1.2 plan as a placeholder to spend some effort to see what we can do if anything to improve existing decompiler toleration of ajc generated code. The long term solution is to build an AspectJ decompiler. This is not currently in plan (and wouldn't actually help Ron here since I recall from discussion that the reason for decompiling is to understand what ajc is generating, and an AspectJ decompiler should decompile back to AspectJ source). Anywhere we generate handlers, we need the load/store combo to keep the decompilers happy (see the JRocket fix)... Yuck, this is a nasty problem - nastier than the JRockit issue as it isnt really related to *generating* new handlers so much as instrumenting existing ones. Help, jim, can you tell me what you think about this... Here is what I have found. Here is a simple version of Rons code that shows the problem: class ExceptionCatcher { RuntimeException re = new RuntimeException(); public void catchException() { try { throw re; } catch (Exception e) { } } public static void main(String[] args) { new ExceptionCatcher().catchException(); } } aspect ExceptionHandler { before(): handler(*) { System.out.println("Starting before block"); } } If I compile this, I am basically adding advice to the start of the catch block. If I jad the resulting ExceptionCatcher then it fails as Ron describes. A javap -c ExceptionCatcher reports: public void catchException(); Code: 0: aload_0 1: getfield #16; //Field re:Ljava/lang/RuntimeException; 4: athrow 5: nop 6: invokestatic #38; //Method ExceptionHandler.aspectOf:() LExceptionHandler; 9: invokevirtual #41; //Method ExceptionHandler.ajc$before$ExceptionHandler$1$768387dd:()V 12: pop 13: return Exception table: from to target type 0 5 5 Class java/lang/Exception Notice where the 'pop' instruction is. Now, if I try and create a piece of Java that simulates what ajc is doing: class ExceptionCatcher { RuntimeException re = new RuntimeException(); public void catchException() { try { throw re; } catch (Exception e) { ExceptionHandler.aspectOf().ajc$before$ExceptionHandler$1$768387dd(); } } public static void main(String[] args) { new ExceptionCatcher().catchException(); } } And then I compile that with ajc (no aspect is included in the compilation step), then I get a new version of ExceptionCatcher that jads successfully. The bytecode is: public void catchException(); Code: 0: aload_0 1: getfield #16; //Field re:Ljava/lang/RuntimeException; 4: athrow 5: pop 6: invokestatic #27; //Method ExceptionHandler.aspectOf:() LExceptionHandler; 9: invokevirtual #30; //Method ExceptionHandler.ajc$before$ExceptionHandler$1$768387dd:()V 12: return Exception table: from to target type 0 5 5 Class java/lang/Exception Notice that the pop instruction in this case is before the call to the aspect method. I had originally thought the fix might be to move the shadow start point for the handler by one instruction (over the pop) but that causes a bit of a mess in the harness tests. Right, some progress - I think I can see that the method I'm interested in is BcelShadow.prepareForMungers() which messes with the exception handler code, inserting the NOP amongst other things. After Miks demo earlier today at AOSD, I asked Erik what he thought about this bug. He said: proposed fix: AspectJ should pattern match between START OF HANDLER pop START OF HANDLER push if we expose the exception, we need to delete the pop if it's there if we don't, we should just insert code _after_ the pop. ===== I *hope* to get this in before 1.2 final. This turns out to be more complicated ... as these things do. prepareForMungers() looks at the shape of the handler and inserts a NOP (I assume to use as the shadow to munge). I can adjust prepareForMungers() to insert the NOP after the POP (using what Erik described and optionally including the POP instruction when identifying a handler join point). But if I do that then at the point we do the munging, I need to perhaps delete the POP depending on whether the exception is being used in the advice being woven. I can determine if the exception is being used, but deleting the POP seems tricky. At the point we do the munging the start of the shadow is the NOP, and jumping to a previous instruction seems to move me outside of the shadow - and I don't think I want to start affecting code 'surrounding' the shadow ?? I don't have a decompiler to play with, so all of these suggestions are untested. You should be careful fixing this bug as the last thing we want is to generate truly invalid bytecode just to make broken decompilers happier. The first thing that I'd try is to call initializeArgVars() at the end of the "else if (getKind() == ExceptionHandler) {" block in prepareForMungers(). This call will force the use of a frame slot to hold the exception value and the added store instruction will mean that the value isn't kept on the stack during the advice. After doing this, you'll still have an ALOAD/POP combo just outside the shadow for cases where the exception isn't used in the original handler. If this still bothers the decompiler, you can try to remove it at the end of prepareMungers(). At the end of that method there's a block for "if (getKind ().argsOnStack() && argVars != null) {". You could add a special case to the beginning of this block to check for an ExceptionHandler shadow type AND a first instruction outside the shadow that's a POP. In that case you want to remove the POP and NOT generate the args. Ok - I've implemented a fix based around what Jim was suggesting. (I must say this seemed easier to do having 3 months more compiler experience than when I last posted on the bug!). All changes in BcelShadow.prepareForMungers() We currently shove in a 'NOP' when preparing an exception handler for munging. This is to give us an anchor when we insert extra instructions that expose context for use in the advice. This confuses the decompiler as it wants to either see a POP or an ASTORE as the first instruction in the handler code. So I've removed the NOP and made it always do a STORE to stick the stack contents into a local variable - which we may or may not use the value of. Effectively where we were optionally storing the value on the stack (the caught exception), we now always store it but may not use it. Once the STORE is in the right place we rework the exception ranges to include our new instruction. (They need to point to it rather than to the instruction after it). The other piece of the fix is to repair the stack as we exit the handler. Normally we stick in an ALOAD after the advice so the stack is ready for use by the actual code originally written in the catch block. However, in the case we are looking at here, the first instruction in the real catch block is a POP as the exception is never used. It is redundant to generate an ALOAD and then execute a POP - and it confuses decompilers. So in the case where we have done this clever ASTORE business (i.e. we are a handler shadow), we check if the next instruction is a POP. if it is then we don't generate the ALOAD and splat the POP with a NOP (easier to replace the POP than remove it with all the targeter management that would be involved). If we now compile the sample program: class ExceptionCatcher { RuntimeException re = new RuntimeException(); public void catchException() { try { throw re; } catch (Exception e) { } } public static void main(String[] args) { new ExceptionCatcher().catchException(); } } aspect ExceptionHandler { before(): handler(*) { System.out.println("Starting before block"); } } We get this bytecode: public void catchException(); org.aspectj.weaver.MethodDeclarationLineNumber: length = 0x4 00 00 00 05 Code: Stack=1, Locals=2, Args_size=1 0: aload_0 1: getfield #16; //Field re:Ljava/lang/RuntimeException; 4: athrow 5: astore_1 6: invokestatic #39; //Method ExceptionHandler.aspectOf:()LExceptionHandler; 9: invokevirtual #42; //Method ExceptionHandler.ajc$before$ExceptionHandler$1$768387dd:()V 12: nop 13: return Exception table: from to target type 0 5 5 Class java/lang/Exception At instruction #5 you can see the new ASTORE_1. At instruction #12 you can see where we replaced the POP with a NOP. In the exception table you can see the target is our new ASTORE_1. And it decompiles just fine. (I also fixed the common singleton version of the aspectOf() method so that it decompiles cleanly - I haven't investigated decompilation of the variants of this method created when using per clauses) I'll close the bug when the fix is available. Fix available: BUILD COMPLETE - build.371 Date of build: 09/07/2004 12:33:43 Time to build: 101 minutes 39 seconds Last changed: 09/07/2004 11:47:45 Latest good AspectJ jar available at: download.eclipse.org/technology/ajdt/dev/aspectj-DEVELOPMENT.jar Fix released as part of AspectJ 1.2.1 |