Comment 1 Ronald R. DiFRango 2003-11-07 14:43:15 EST

Created attachment 6699 [details]
Source to re-produce problem.

Comment 2 Ronald R. DiFRango 2003-11-07 14:43:35 EST

Created attachment 6700 [details]
Source to re-prodcue problem.

Comment 3 Ronald R. DiFRango 2003-11-07 14:44:12 EST

Created attachment 6701 [details]
Decompiled output.

Comment 4 Ronald R. DiFRango 2003-11-07 14:44:51 EST

Created attachment 6702 [details]
Decompiled output.

Comment 5 Adrian Colyer 2003-12-08 09:09:00 EST

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).

Comment 6 Adrian Colyer 2004-03-16 05:08:33 EST

Anywhere we generate handlers, we need the load/store combo to keep the 
decompilers happy (see the JRocket fix)...

Comment 7 Andrew Clement 2004-03-16 11:34:18 EST

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.

Comment 8 Andrew Clement 2004-03-16 15:12:21 EST

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.

Comment 9 Andrew Clement 2004-03-24 16:56:57 EST

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.

Comment 10 Andrew Clement 2004-04-01 11:43:15 EST

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 ??  

Comment 11 Jim Hugunin 2004-04-04 20:57:59 EDT

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.

Comment 12 Andrew Clement 2004-09-01 11:31:54 EDT

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.

Comment 13 Andrew Clement 2004-09-07 09:41:27 EDT

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

Comment 14 Adrian Colyer 2004-10-21 04:31:37 EDT

Fix released as part of AspectJ 1.2.1