Bug 230817 - LazyMethodGen.remap() NullPointerException
Summary: LazyMethodGen.remap() NullPointerException
Status: RESOLVED FIXED
Alias: None
Product: AspectJ
Classification: Tools
Component: Compiler (show other bugs)
Version: unspecified   Edit
Hardware: PC Windows Vista
: P3 major (vote)
Target Milestone: 1.6.1   Edit
Assignee: aspectj inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-05-06 22:43 EDT by Andrew Clement CLA
Modified: 2008-05-07 11:31 EDT (History)
0 users

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Clement CLA 2008-05-06 22:43:15 EDT
This has been reported a few times by different users, but has always proved tough to diagnose.  The typical stack trace is something like:

java.lang.NullPointerException
org.aspectj.weaver.bcel.LazyMethodGen.remap(LazyMethodGen.java:1237)
org.aspectj.weaver.bcel.LazyMethodGen.addExceptionHandlers(LazyMethodGen.java:1132)
org.aspectj.weaver.bcel.LazyMethodGen.packBody(LazyMethodGen.java:1078)
org.aspectj.weaver.bcel.LazyMethodGen.pack(LazyMethodGen.java:977)
org.aspectj.weaver.bcel.LazyMethodGen.getMethod(LazyMethodGen.java:484)
org.aspectj.weaver.bcel.LazyClassGen.writeBack(LazyClassGen.java:512)
org.aspectj.weaver.bcel.LazyClassGen.getJavaClassBytesIncludingReweavable(LazyClassGen.java:652)
org.aspectj.weaver.bcel.BcelWeaver.getClassFilesFor(BcelWeaver.java:1420)
org.aspectj.weaver.bcel.BcelWeaver.weaveAndNotify(BcelWeaver.java:1390)

And that is an exception on this line in remap()

ih = ih.getNext();

called from the addExceptionHandlers() line:

 gen.addExceptionHandler(
                remap(r.getRealStart(), map), 
                remap(r.getRealEnd(), map),
                remap(r.getHandler(), map),
                (r.getCatchType() == null)
                ? null 
                : (ObjectType) BcelWorld.makeBcelType(r.getCatchType()));

During weaving, an instruction list is augmented with extra entries representing where shadows start and end (method-call, method-execution, handler, etc).  When weaving is complete we want to remove these temporary elements and use the remaining instructions to produce the method bytecode.  Now sometimes these temporary entries are targeted by other elements (line number tags, local variable tags and exception handlers usually).  During packing we use remap() to reposition the locations so they move off of temporary placeholders and onto real instructions that will make it out into the bytecode.  What the above exception tells us is that we started walking over temporary placeholder entries, but before we came to a real instruction, we ran out of instructions!  This cannot happen, and indicates something went seriously wrong, we should always encounter an instruction when remapping off a temporary element.

After some digging it is actually the remap() call for the handler (not the start or end) that leads to the problem.  The handler represents where to jump to in the code when an exception of the specified type occurs between the instructions pointed at by start and end.

I sent a debug build to a user encountering this problem (I could not recreate it) and in that I was looking at where in fact the handler was pointing before we called remap().  I learned that when this problem occurs, the handler is not pointing to anywhere in the method currently being processed (not good).

In a second debug build I tried to ascertain who was setting these handlers to point to nowhere.  This debug never triggered, no-one was setting them to point to nowhere...

I happened to notice whilst going through the instructions in the debug output that cobertura was being used, a coverage toolkit that works by doing bytecode manipulation to insert calls out to a library.  AspectJ was being called after cobertura and I asked the user to try the steps the other way round - it worked fine.  Indicating cobertura was doing something to the bytecode that gave us problems.  After much messing about, I recreated it by applying around advice to within(*) to all the classes in rt.jar (I just used that as a very large standalone jar file I could weave into).

I learned that Cobertura creates catch blocks that look a little different to what javac and other compilers create.  The typical bytecode sequence a compiler produces for a catch block starts with a STORE instruction, to store the exception being caught (whether the body of the catch block uses it or not).  But the cobertura catch blocks started with an INVOKESTATIC instruction, a call out to another method.  What does this mean?  It means the same instruction has two shadows, a 'handler' shadow and a 'method-call' shadow - and it turns out this is what causes our problem.  If around advice is applied to the call join point and it cannot be inlined then the body of the call shadow (the call itself) is pulled out into a new method.  Because the handler was the same instruction, this meant the handler *was also being pulled out* into the new method, leaving behind an exception handler that jumped to an invalid location (in fact it 'jumped' to an instruction in a different method!).  So the reason I never saw the handler location being set incorrectly is that it was set correctly up front, but then dragged out with the method-call shadow into the wrong place.  In bytecode terms it looks like this:

   method-execution()
    |               ICONST_0
    |               ISTORE_2
    |               SIPUSH -1
    |               ISTORE_3
    | catch java.lang.Exception (1806389629) -> E0
    | | method-call(ProjectData ProjectData.getGlobalProjectData())
    | | |           INVOKESTATIC ProjectData.getGlobalProjectData ()
    | | method-call(ProjectData getGlobalProjectData())
    | |             LDC "SomeString"
    | | method-call(ClassData getOrCreateClassData(java.lang.String))
    | | |           INVOKEVIRTUAL  ProjectData.getOrCreateClassData (LString;)
    | | method-call(ClassData ProjectData.getOrCreateClassData(String))
    | |             SIPUSH 106
    | | method-call(void ClassData.touch(int))
    | | |           INVOKEVIRTUAL ClassData.touch (I)V
    | | method-call(void ClassData.touch(int))
    | |             ALOAD_1
    | | method-call(Object Expression.getValue())
    | | |           INVOKEVIRTUAL Expression.getValue ()
    | | method-call(Object Expression.getValue())
    | catch java.lang.Exception (1806389629) -> E0
    |               ARETURN
    | method-call(nProjectData ProjectData.getGlobalProjectData())
    | |         E0: INVOKESTATIC ProjectData.getGlobalProjectData ()
    | method-call(ProjectData ProjectData.getGlobalProjectData())
    |               LDC "Object"

We can see the problem in that final method-call.  The target for the exception handler seen earlier (E0) is within the method-call shadow.

What to do?
Comment 1 Andrew Clement CLA 2008-05-06 22:48:49 EDT
The general solution is to recognize when the exception handler is occurring against something we don't expect to see (we expect to see a STORE). And when that happens, we introduce a NOP, and move the exception handler to point at the NOP.  So, we morph the code above.

First we introduce the NOP:

    | catch java.lang.Exception (1806389629) -> E0
    |               ARETURN
    |               NOP   
    | method-call(ProjectData getGlobalProjectData())
    | |         E0: INVOKESTATIC ProjectData.getGlobalProjectData()
    | method-call(ProjectData ProjectData.getGlobalProjectData())

Then we move all targeters from the invoke to the NOP

    | catch java.lang.Exception (1806389629) -> E0
    |               ARETURN
    |           E0: NOP   
    | method-call(ProjectData getGlobalProjectData())
    | |             INVOKESTATIC ProjectData.getGlobalProjectData()
    | method-call(ProjectData ProjectData.getGlobalProjectData())

Now the two join points are not overlapping and when the method call is extracted into a new method, the handler stays where it is and the remap() call succeeds.

I've tested this and it works.  Changes are in:
 BcelClassWeaver.match(LazyMethodGen mg,InstructionHandle ih,BcelShadow enclosingShadow,List shadowAccumulator) 

I had to reorder that match code so it looked at exception handlers first rather than last, this enables us to deal with the handler, modify the instructions if necessary and then go on to deal with matching on the invokestatic as a method-call join point.

As the change is to deal with catch blocks that start with anything other than store or nop, this solution will likely enable us to cope better with other unusual (but valid) bytecode sequences too.
Comment 2 Andrew Clement CLA 2008-05-07 11:31:56 EDT
Changed committed.