Comment 1 Robin Green 2005-07-21 15:18:03 EDT

Created attachment 25148 [details]
Class file to be weaved

Comment 2 Robin Green 2005-07-21 15:19:27 EDT

Created attachment 25149 [details]
Aspect

Comment 3 Adrian Colyer 2005-07-22 04:36:16 EDT

Moving to P1 as any verify error is bad...

Comment 4 Andrew Clement 2005-08-24 10:26:24 EDT

Sorry its taken a while to get to this ... darn generics has slowed us down so much.

I assume you used javac or something to build Test.java before using ajc to
binary weave it?

What I'm doing to recreate is:

javac -d outputfolder Test.java
ajc -inpath outputfolder Tracer.aj
java Test

and I get:

Exception in thread "main" java.lang.VerifyError: (class: Test, method:
newTest_aroundBody2 signature: (I)LTest;) Illegal default target in switch

disassembling that method:

private static final Test newTest_aroundBody2(int);
  Code:
   Stack=2, Locals=1, Args_size=1
   0:	iload_0
   1:	tableswitch{ //0 to 0
		0: -1157627302;
		default: 16 }
   20:	invokespecial	#3; //Method "<init>":()V
   23:	areturn
  LineNumberTable: 
   line 16: 0
   line 19: 16

before any advice got tangled up in things, the newTest() method looked like this:

public static Test newTest(int);
  Code:
   Stack=2, Locals=1, Args_size=1
   0:	iload_0
   1:	lookupswitch{ //0
		default: 12 }
   12:	new	#2; //class Test
   15:	dup
   16:	invokespecial	#3; //Method "<init>":()V
   19:	areturn
  LineNumberTable: 
   line 16: 0
   line 19: 12

If I compile it all together with ajc:

ajc Test.java Tracer.aj 

then the result works - it seems that the switch statement is optimized away
because it only has a default entry.  I will take a quick look at tableswitch
but I don't think this will be fixed for M3.

Comment 5 Andrew Clement 2005-08-24 10:47:23 EDT

I've reduced the testcase to something simpler.  Here is the test class:

public class Test {

    public static Test newTest(int i) {
        switch(i) {
          default: return null;
        }
    }
}

and here is the aspect:

aspect Tracer {
  Object around (): execution(* newTest(..)) { return proceed();}
}

If you follow the steps I outlined earlier, this also blows up.  I tried
modifying the switch to include an entry:

    public static Test newTest(int i) {
        switch(i) {
          case 1:  return null;
          default: return null;
        }
    }

so that I could just compile entirely with ajc but that works fine, the
aroundbody in the result looks like:

private static final Test newTest_aroundBody0(int);
  Code:
   Stack=1, Locals=1, Args_size=1
   0:   iload_0
   1:   tableswitch{ //1 to 1
                1: 20;
                default: 22 }
   20:  aconst_null
   21:  areturn
   22:  aconst_null
   23:  areturn
  LineNumberTable:
   line 10: 0
   line 11: 20
   line 12: 22

starting to suspect BCEL more and more...

Comment 6 Andrew Clement 2005-08-25 07:37:50 EDT

Ok... BCEL bug.  The problem occurs when we perform 'extractInstructions' for
creating the body of the around advice.  In order to do that we use a routine
called 'copyInstruction'.

If you look in the code generated by the javac run, it looks like this:

public static Test newTest(int);
  Code:
   Stack=1, Locals=1, Args_size=1
   0:   iload_0
   1:   lookupswitch{ //0
                default: 12 }
   12:  aconst_null
   13:  areturn
  LineNumberTable:
   line 4: 0
   line 5: 12

Notice that a lookupswitch has been used.  The copyinstruction() routine copies
a select (like lookupswitch) by just building a new 'SELECT' object and letting
BCEL work out the best way to represent the select based on the number of
targets in the select.  BCEL thinks its better to represent a select with no
targets (the case we have here) with a TABLESWITCH rather than LOOKUPSWITCH.

However, thats wrong - and there are two bugs lurking.  My first fix is to say
if there are no targets then use a LOOKUPSWITCH - my around body now looks like
this:

private static final Test newTest_aroundBody0(int);
  Code:
   Stack=1, Locals=1, Args_size=1
   0:   iload_0
   1:   lookupswitch{ //0
                default: 12 }
   12:  aconst_null
   13:  areturn
  LineNumberTable:
   line 4: 0
   line 5: 12

perfect!

The other lurking problem is that a TABLESWITCH is defined as:

TABLESWITCH (0xaa)
<0-3> padding bytes
defaultbytes1-4
lowbyte1-4
highbyte1-4
jumpoffsets...

So in our case we create a TABLESWITCH where lowbyte=0 and highbyte=0 but the
definition of the instruction says that the number of jumpoffsets must be
(high-low+1).  BCEL doesnt write out any entries in the case where there are no
targets when it should write out at least *one* - if it doesnt then decompilers
and verifiers will choke.

However... I haven't fixed this problem as there seems to be more issues lurking
in TABLESWITCH - if we write out those 4 missing bytes, we'll need to ensure the
'length' of the instruction is managed correctly too.  Given that so far the
only way I've seen a damaged TABLESWITCH is when its been incorrectly created
instead of a LOOKUPSWITCH (and that can no longer happen) I won't fix
TABLESWITCH until I see a bug that shows the problem.

fix checked in, waiting on build.

Comment 7 Andrew Clement 2005-08-25 10:27:23 EDT

fix available in latest dev build, see download page.