Instrumentation tool using the ASM tree API

Question

This is for an instrumentation tool using the ASM tree api. This method assigns a handler to a field, which is used in instrumented methods. As a result of my instrumentation, the instrumented class implements an interface and is a handler itself, so a value of this behaves like a NullObject concerning handled methods - it doesn't do anything different but never throws an Exception (when the handled method in the instrumented class is invoked, it will cause a NullPointerException if the handler is null because methods are called on it).

Now, there's 3 different ways to avoid NullPointerExceptions (my enum):

• checkBeforeCall: on every call of the handled method, this is used as the handler if it is not set to anything else but null. That's a "slow" approach if the handled method is called very often, as an if is required per call. Pretty please avoid telling me that that's ok and I shouldn't use the other two enum values - that's beside the point :-)
• assignAfterSuper: after the super constructor is called, the handler field is initialized. This causes little to no overhead at runtime, but it causes problems if the handled method was overridden and is called in the super constructor, because it will throw the NPE.
• assignBeforeSuper: The handler is set to this before the super constructor is called. No Java compiler will ever produce this bytecode, but it's valid and the scala guys do it as well. The handler field is then fully initialized after the super constructor was called.

I provide all three methods as the first one is fail-safe, the second is valid and you should be ok in most scenarios and the third is best but could potentially cause trouble in tools.

Now, for each of these initializations, there are two ways to set the handler. One - if you have the instrumented instance - is to call a "setHandler" method. That's ok if you have full access to the object, but you're helpless if you don't. The other is to provide a static method, which is called to create a handler. The instrumented instance is passed (so it can be returned itself if no handler is needed) and the result is then used to initialize the field.

Passing this to a static method before the super constructor was called is invalid bytecode. So I use this "spawner" method (if it was specified) after super(...) was called.

So the gory situation: I have an instruction list which I modify. In the relevant two enum-cases I need to consider for constructor instrumentation, I also execute the code for assignAfterSuper on assignBeforeSuper, but only if a static initializer is used (otherwise, it's already set to this). I'm modifiying an instruction list and use a little more state information and factoring any methods out doesn't really make sense, as the parameter list would be longer than the method itself and readability would be diminished. Factoring it out into different classes would also be problematic, as it hurts readability and I have to switch open files when I want to understand the methods behavior or I have to duplicate all code further up or I have to factor it out but can't see what else happens to my InsnList (which is dangerous, because it could mess up the result. I'd like to keep the InsnList for one MethodNode inside of the method so I can see at first glance which changes occur).

private void assignHandler(MethodNode method,
                String handleeInternalName, String handlerField) {
    if (this.guard == NullPointerGuard.checkBeforeCall) {
        return;
    }
    InsnList instructions = method.instructions;
    final boolean usesLabels = instructions.getFirst() instanceof LabelNode;
    // search for call of constructor super(...) or this(...)
    AbstractInsnNode constructorCall = instructions.getFirst();
    while (!(constructorCall.getOpcode() == INVOKESPECIAL
            && constructorCall instanceof MethodInsnNode
            && "<init>".equals(((MethodInsnNode) constructorCall).name))) {
        constructorCall = constructorCall.getNext();
    }
    // skip modification on delegation to this(...)
    if (handleeInternalName.equals(((MethodInsnNode) constructorCall).owner)) {
        return;
    }
    // not delegating to this(...), inject initialization of Handler field
    AbstractInsnNode node = instructions.getFirst();
    InsnList storeHandler = null;
    switch (this.guard) {
    case assignBeforeSuper:
        storeHandler = storeHandlerInField(handleeInternalName, handlerField, false);
        if (usesLabels) {
            storeHandler.add(new LabelNode());
            instructions.insert(node, storeHandler);
        } else {
            instructions.insertBefore(node, storeHandler);
        }
        if (!usesSpawner()) {
            break;
        } // else fallthrough
    case assignAfterSuper:
        storeHandler = storeHandlerInField(handleeInternalName, handlerField, true);
        if (usesLabels) {
            storeHandler.insertBefore(storeHandler.getFirst(), new LabelNode());
        }
        instructions.insert(constructorCall, storeHandler);
        break;
    default:
        throw new InstrumentationException(this.guard + " is not implemented yet");
    } 
    method.maxStack += usesSpawner() ? 2 : 1;
}

I am concerned about the fall-through inside the switch, but before I restructured it this way, I had either duplicated code for the actions or duplicated checks for the conditions. Unfortunately, each case cannot easily be extracted into a method.

Now, the structure is transparent, can easily be read and understood (as long as one knows how switch works) and there is no more redundancy. Still, this feels weird. Though I'm really proud I have found this solution, I wonder if there's a better way.

Answer 1

You use of switch is unconventional and in consequence hard to follow. I'd avoid it for that reason. What I see is that that both case are very similar, and so I'd try to refactor them into a function.

Here's my approach:

private void assignHandler(MethodNode method,
                String handleeInternalName, String handlerField) {
    if (this.guard == NullPointerGuard.checkBeforeCall) {
        return;
    }
    MethodInsNode constructorCall = findConstructorCall(method);

    // skip modification on delegation to this(...)
    if (handleeInternalName.equals(constructorCall.owner)) {
        return;
    }
    switch (this.guard) {
    case assignBeforeSuper:
        insertHandler(method, handleeInternalName, handlerField, null);
        if( usesSpawner() )
        {
            insertHandler(method, handleeInternalName, handlerField, constructorCall);
        }
        break;
    case assignAfterSuper:
        insertHandler(method, handleeInternalName, handlerField, constructorCall);
         break;
    default:
        throw new InstrumentationException(this.guard + " is not implemented yet");
    } 
}

void insertHandler(MethodNode method, String handleeInternalName, String handlerField, AbstractInsnNode node) // node specified node
 // to insert before, or null for beginning of method
{
    InsnList instructions = method.instructions;
    final boolean usesLabels = instructions.getFirst() instanceof LabelNode;
    InsnList storeHandler = storeHandlerInField(handleeInternalName, handlerField, node != null);


    // the rest of this function probably belongs in a InstructionInserter class 
    if(useLabels)
    {
        if(node == null)
        {
            storeHandler.add( new LabelNode() );
            node = instructions.getFirst(); // insert after existing label node
        }
        else
        {
            storeHandler.insert( new LabelNode() );
        }
    }


    if(node == null)
    {
        instructions.insert(storeHandler);
    }
    else
    {
        instructions.insert(node, storeHandler);
    }

    method.maxStack++;
}