diff options
| -rw-r--r-- | lib/aspectj/lib/aspectjrt.jar | bin | 114560 -> 115002 bytes | |||
| -rw-r--r-- | lib/test/aspectjrt.jar | bin | 114560 -> 115002 bytes | |||
| -rw-r--r-- | runtime/src/org/aspectj/runtime/internal/AroundClosure.java | 21 | ||||
| -rw-r--r-- | runtime/src/org/aspectj/runtime/reflect/JoinPointImpl.java | 68 | ||||
| -rw-r--r-- | tests/src/org/aspectj/systemtest/ajc151/AtAroundTests.java | 61 | ||||
| -rw-r--r-- | weaver/src/org/aspectj/weaver/bcel/BcelShadow.java | 189 |
6 files changed, 270 insertions, 69 deletions
diff --git a/lib/aspectj/lib/aspectjrt.jar b/lib/aspectj/lib/aspectjrt.jar Binary files differindex 80d738327..5336ba4cb 100644 --- a/lib/aspectj/lib/aspectjrt.jar +++ b/lib/aspectj/lib/aspectjrt.jar diff --git a/lib/test/aspectjrt.jar b/lib/test/aspectjrt.jar Binary files differindex 80d738327..5336ba4cb 100644 --- a/lib/test/aspectjrt.jar +++ b/lib/test/aspectjrt.jar diff --git a/runtime/src/org/aspectj/runtime/internal/AroundClosure.java b/runtime/src/org/aspectj/runtime/internal/AroundClosure.java index ce852dfad..91f7f923c 100644 --- a/runtime/src/org/aspectj/runtime/internal/AroundClosure.java +++ b/runtime/src/org/aspectj/runtime/internal/AroundClosure.java @@ -10,6 +10,7 @@ * Contributors: * Xerox/PARC initial implementation * Alex Vasseur wired up for @AJ proceeding + * Andy Clement 23-06-06 added extras for @AJ * ******************************************************************/ @@ -19,6 +20,12 @@ import org.aspectj.lang.ProceedingJoinPoint; public abstract class AroundClosure { protected Object[] state; + + // Records with the related joinpoint has a this or a target and whether + // either of them are bound in the pointcut. Set in the 'link' call made + // at each matching join point... (see pr126167) + // bit6 being 1 means the flags haven't been initialized + protected int bitflags = 0x100000; protected Object[] preInitializationState; public AroundClosure() { @@ -28,7 +35,7 @@ public abstract class AroundClosure { this.state = state; } - + public int getFlags() {return bitflags;} public Object[] getState() { return state; @@ -54,4 +61,16 @@ public abstract class AroundClosure { jp.set$AroundClosure(this); return jp; } + + /** + * This method is called to implicitly associate the closure with the joinpoint + * as required for @AJ aspect proceed() + */ + public ProceedingJoinPoint linkClosureAndJoinPoint(int flags) { + //TODO is this cast safe ? + ProceedingJoinPoint jp = (ProceedingJoinPoint)state[state.length-1]; + jp.set$AroundClosure(this); + this.bitflags = flags; + return jp; + } } diff --git a/runtime/src/org/aspectj/runtime/reflect/JoinPointImpl.java b/runtime/src/org/aspectj/runtime/reflect/JoinPointImpl.java index 5e4dfa004..bb468feef 100644 --- a/runtime/src/org/aspectj/runtime/reflect/JoinPointImpl.java +++ b/runtime/src/org/aspectj/runtime/reflect/JoinPointImpl.java @@ -106,17 +106,73 @@ class JoinPointImpl implements ProceedingJoinPoint { if (arc == null) return null; else { + + // Based on the bit flags in the AroundClosure we can determine what to + // expect in the adviceBindings array. We may or may not be expecting + // the first value to be a new this or a new target... (see pr126167) + int flags = arc.getFlags(); + boolean unset = (flags &0x100000)!=0; + boolean thisTargetTheSame = (flags &0x010000)!=0; + boolean hasThis = (flags &0x001000)!=0; + boolean bindsThis = (flags &0x000100)!=0; + boolean hasTarget = (flags &0x000010)!=0; + boolean bindsTarget = (flags &0x000001)!=0; + // state is always consistent with caller?,callee?,formals...,jp Object[] state = arc.getState(); - for (int i = state.length-2; i >= 0; i--) { - int formalIndex = (adviceBindings.length - 1) - (state.length-2) + i; - if (formalIndex >= 0 && formalIndex < adviceBindings.length) { - state[i] = adviceBindings[formalIndex]; - } + + // these next two numbers can differ because some join points have a this and + // target that are the same (eg. call) - and yet you can bind this and target + // separately. + + // In the state array, [0] may be this, [1] may be target + + int firstArgumentIndexIntoAdviceBindings = 0; + int firstArgumentIndexIntoState = 0; + firstArgumentIndexIntoState+=(hasThis?1:0); + firstArgumentIndexIntoState+=(hasTarget&&!thisTargetTheSame?1:0); + if (hasThis) { + if (bindsThis) { + // replace [0] (this) + firstArgumentIndexIntoAdviceBindings=1; + state[0]=adviceBindings[0]; + } else { + // leave state[0] alone, its OK + } + } + if (hasTarget) { + if (bindsTarget) { + if (thisTargetTheSame) { + // this and target are the same so replace state[0] + firstArgumentIndexIntoAdviceBindings=1+(bindsThis?1:0); + state[0]=adviceBindings[(bindsThis?1:0)]; + } else { + // need to replace the target, and it is different to this, whether + // that means replacing state[0] or state[1] depends on whether + // the join point has a this + firstArgumentIndexIntoAdviceBindings=(hasThis?1:0)+1; + state[hasThis?1:0]=adviceBindings[hasThis?1:0]; + } + } else { + // leave state[0]/state[1] alone, they are OK + } } + + // copy the rest across + for (int i=firstArgumentIndexIntoAdviceBindings;i<adviceBindings.length;i++) { + state[firstArgumentIndexIntoState+(i-firstArgumentIndexIntoAdviceBindings)]=adviceBindings[i]; + } + + // old code that did this, didnt allow this/target overriding +// for (int i = state.length-2; i >= 0; i--) { +// int formalIndex = (adviceBindings.length - 1) - (state.length-2) + i; +// if (formalIndex >= 0 && formalIndex < adviceBindings.length) { +// state[i] = adviceBindings[formalIndex]; +// } +// } return arc.run(state); } } - + } diff --git a/tests/src/org/aspectj/systemtest/ajc151/AtAroundTests.java b/tests/src/org/aspectj/systemtest/ajc151/AtAroundTests.java index c926c7db6..e64b8945b 100644 --- a/tests/src/org/aspectj/systemtest/ajc151/AtAroundTests.java +++ b/tests/src/org/aspectj/systemtest/ajc151/AtAroundTests.java @@ -24,49 +24,60 @@ import org.aspectj.testing.XMLBasedAjcTestCase; * @author AndyClement * */ - - public class AtAroundTests extends XMLBasedAjcTestCase { - public void testCodeBasic() { runTest("code style - basic"); } - public void testAtBasic() { runTest("annotation style - basic"); } + public void testCodeBasic() { runTest("code style - basic"); } + public void testAtBasicNoInline() { runTest("annotation style - basic - noinline"); } + public void testAtBasic() { runTest("annotation style - basic"); } + public void testCodeBindingTarget() { runTest("code style - correct usage, binding and passing same target for call"); } + public void testAtBindingTargetNoInline() { runTest("annotation style - correct usage, binding and passing same target for call - noinline"); } + public void testAtBindingTarget() { runTest("annotation style - correct usage, binding and passing same target for call"); } + + public void testCodeBindingTarget2() { runTest("code style - correct usage, binding and passing new target for call"); } + public void testAtBindingTargetNoInline2() { runTest("annotation style - correct usage, binding and passing new target for call - noinline"); } + public void testAtBindingTarget2() { runTest("annotation style - correct usage, binding and passing new target for call"); } + public void testCodeErrorCase1() { runTest("code style - forget to pass target");} // Don't think we can report correct errors for @AJ as the parameters are specified as an object array - // public void testAtErrorCase1() { runTest("annotation style - forget to pass target");} + //public void testAtErrorCase1() { runTest("annotation style - forget to pass target");} + + public void testCodeBindThisCallChangeProceed() { runTest("code style - bind this on call - change on proceed - no effect");} + public void testAtBindThisCallChangeProceedNoInline() { runTest("annotation style - bind this on call - change on proceed - no effect - noinline");} + public void testAtBindThisCallChangeProceed() { runTest("annotation style - bind this on call - change on proceed - no effect");} + + public void testCodeBindThisExecutionChangeProceed() { runTest("code style - bind this on execution - change on proceed - works");} + public void testAtBindThisExecutionChangeProceedNoInline() { runTest("annotation style - bind this on execution - change on proceed - works - noinline");} + public void testAtBindThisExecutionChangeProceed() { runTest("annotation style - bind this on execution - change on proceed - works");} + + public void testCodeBindBothExecutionChangeProceed() { runTest("code style - bind this and target on execution - change on proceed - works");} + public void testAtBindBothExecutionChangeProceedNoInline() { runTest("annotation style - bind this and target on execution - change on proceed - works - noinline");} + public void testAtBindBothExecutionChangeProceed() { runTest("annotation style - bind this and target on execution - change on proceed - works");} public void testCodeErrorCase2() { runTest("code style - incorrect arg types");} // Don't think we can report correct errors for @AJ as the parameters are specified as an object array // public void testAtErrorCase2() { runTest("annotation style - incorrect arg types");} - - public void testCodeBindingTarget() { runTest("code style - correct usage, binding and passing new target for call"); } - public void testAtBindingTarget() { runTest("annotation style - correct usage, binding and passing new target for call"); } - public void testCodeChangingTarget() { runTest("code style - changing target for call"); } - public void testAtChangingTarget() { runTest("annotation style - changing target for call"); } - public void testCodeChangingTargetDifferingOrder() { runTest("code style - changing target for call - reverse order"); } // @AJ cant cope with the changing of the order of arguments bound and passed through proceed //public void testAtChangingTargetDifferingOrder() { runTest("annotation style - changing target for call - reverse order"); } - - public void testCodeBindThisCallChangeProceed() { runTest("code style - bind this on call - change on proceed - no effect");} - //public void testAtBindThisCallChangeProceed() { runTest("annotation style - bind this on call - change on proceed - no effect");} + + public void testCodeBindBothCallChangeProceed() { runTest("code style - bind this and target on call - change on proceed - works");} + public void testAtBindBothCallChangeProceedNoInline() { runTest("annotation style - bind this and target on call - change on proceed - works - noinline");} + public void testAtBindBothCallChangeProceed() { runTest("annotation style - bind this and target on call - change on proceed - works");} - public void testCodeBindThisExecutionChangeProceed() { runTest("code style - bind this on execution - change on proceed - works");} - //public void testAtBindThisExecutionChangeProceed() { runTest("annotation style - bind this on execution - change on proceed - works");} + public void testBreakingIt1() { runTest("breaking it - one");} + public void testBreakingIt2() { runTest("breaking it - two");} + + public void testBugCase1() { runTest("bug case one");} + public void testBugCase2() { runTest("bug case two");} + public void testMultipleArgs() { runTest("multiple args");} - public void testCodeBindBothExecutionChangeProceed() { runTest("code style - bind this and target on execution - change on proceed - works");} - //public void testAtBindBothExecutionChangeProceed() { runTest("annotation style - bind this and target on execution - change on proceed - works");} - - public void testCodeBindBothCallChangeProceed() { runTest("code style - bind this and target on call - change on proceed - works");} - //public void testAtBindBothCallChangeProceed() { runTest("annotation style - bind this and target on call - change on proceed - works");} - public void testCodeSubsetArguments() { runTest("code style - works with subset of arguments in advice");} + // cant do this for annotation style //public void testAtSubsetArguments() { runTest("annotation style - works with subset of arguments in advice");} - - - // + // --- public static Test suite() { return XMLBasedAjcTestCase.loadSuite(AtAroundTests.class); } diff --git a/weaver/src/org/aspectj/weaver/bcel/BcelShadow.java b/weaver/src/org/aspectj/weaver/bcel/BcelShadow.java index a6523a392..bfa5c23ef 100644 --- a/weaver/src/org/aspectj/weaver/bcel/BcelShadow.java +++ b/weaver/src/org/aspectj/weaver/bcel/BcelShadow.java @@ -2174,10 +2174,6 @@ public class BcelShadow extends Shadow { range.insert(entryInstructions, Range.InsideBefore); } - public void weaveAroundInline( - BcelAdvice munger, - boolean hasDynamicTest) - { /* Implementation notes: * * AroundInline still extracts the instructions of the original shadow into @@ -2208,6 +2204,7 @@ public class BcelShadow extends Shadow { * new method for the advice can also be re-lined. We are not doing that * presently. */ + public void weaveAroundInline(BcelAdvice munger,boolean hasDynamicTest) { // !!! THIS BLOCK OF CODE SHOULD BE IN A METHOD CALLED weaveAround(...); Member mungerSig = munger.getSignature(); @@ -2246,9 +2243,8 @@ public class BcelShadow extends Shadow { // specific test for @AJ proceedInInners if (munger.getConcreteAspect().isAnnotationStyleAspect()) { - // if we can't find one proceed() - // we suspect that the call is happening in an inner class - // so we don't inline it. + // if we can't find one proceed() we suspect that the call + // is happening in an inner class so we don't inline it. // Note: for code style, this is done at Aspect compilation time. boolean canSeeProceedPassedToOther = false; InstructionHandle curr = adviceMethod.getBody().getStart(); @@ -2285,29 +2281,25 @@ public class BcelShadow extends Shadow { final InstructionFactory fact = getFactory(); // now generate the aroundBody method + // eg. "private static final void method_aroundBody0(M, M, String, org.aspectj.lang.JoinPoint)" LazyMethodGen extractedMethod = extractMethod( - NameMangler.aroundCallbackMethodName( - getSignature(), - getEnclosingClass()), + NameMangler.aroundCallbackMethodName(getSignature(),getEnclosingClass()), Modifier.PRIVATE, - munger - ); - + munger); // now extract the advice into its own method String adviceMethodName = - NameMangler.aroundCallbackMethodName( - getSignature(), - getEnclosingClass()) + "$advice"; + NameMangler.aroundCallbackMethodName(getSignature(),getEnclosingClass()) + "$advice"; - List argVarList = new ArrayList(); - List proceedVarList = new ArrayList(); + List argVarList = new ArrayList(); + List proceedVarList = new ArrayList(); int extraParamOffset = 0; // Create the extra parameters that are needed for passing to proceed // This code is very similar to that found in makeCallToCallback and should // be rationalized in the future + if (thisVar != null) { argVarList.add(thisVar); proceedVarList.add(new BcelVar(thisVar.getType(), extraParamOffset)); @@ -2612,13 +2604,6 @@ public class BcelShadow extends Shadow { /** * ATAJ Handle the inlining for @AJ aspects * - * @param fact - * @param callbackMethod - * @param munger - * @param localAdviceMethod - * @param argVarList - * @param isProceedWithArgs - * @return */ private InstructionList getRedoneProceedCallForAnnotationStyle( InstructionFactory fact, @@ -2630,8 +2615,6 @@ public class BcelShadow extends Shadow { { // Notes: // proceedingjp is on stack (since user was calling pjp.proceed(...) - // the boxed args to proceed() are on stack as well (Object[]) unless - // the call is to pjp.proceed(<noarg>) // new Object[]{new Integer(argAdvice1-1)};// arg of proceed // call to proceed(..) is NOT made @@ -2646,15 +2629,19 @@ public class BcelShadow extends Shadow { // int res = .. from original code //Note: we just don't care about the proceed map etc + // (we would need to care if we allow repositioning of arguments in advice signature) InstructionList ret = new InstructionList(); // store the Object[] array on stack if proceed with args if (isProceedWithArgs) { + + // STORE the Object[] into a local variable Type objectArrayType = Type.getType("[Ljava/lang/Object;"); int localProceedArgArray = localAdviceMethod.allocateLocal(objectArrayType); ret.append(InstructionFactory.createStore(objectArrayType, localProceedArgArray)); + // STORE the ProceedingJoinPoint instance into a local variable Type proceedingJpType = Type.getType("Lorg/aspectj/lang/ProceedingJoinPoint;"); int localJp = localAdviceMethod.allocateLocal(proceedingJpType); ret.append(InstructionFactory.createStore(proceedingJpType, localJp)); @@ -2664,22 +2651,74 @@ public class BcelShadow extends Shadow { // TODO do we want to try catch ClassCast and AOOBE exception ? // special logic when withincode is static or not - int startIndex = 0; - if (thisVar != null) { - startIndex = 1; - //TODO this logic is actually depending on target as well - test me - ret.append(new ALOAD(0));//thisVar + + // This next bit of code probably makes more sense if you read its implementation for + // weaveAroundClosure() - see JoinPointImpl.proceed(Object[]). Basically depending + // on whether the join point has a this/target and whether the pointcut binds this/target + // then the arguments to the 'new' proceed call need to be reorganized. (pr126167) + boolean relatedPointcutBindsThis = bindsThis(munger); + boolean relatedPointcutBindsTarget = bindsTarget(munger); + boolean targetIsSameAsThis = getKind().isTargetSameAsThis(); + + // two numbers can differ because a pointcut may bind both this/target and yet at the + // join point this and target are the same (eg. call) + int indexIntoObjectArrayForArguments=0; + int indexIntoCallbackMethodForArguments = 0; + if (hasThis()) { + if (relatedPointcutBindsThis) { + if (!(relatedPointcutBindsTarget && targetIsSameAsThis)) { + // they have supplied new this as first entry in object array + ret.append(InstructionFactory.createLoad(objectArrayType, localProceedArgArray)); + ret.append(Utility.createConstant(fact, 0)); + ret.append(InstructionFactory.createArrayLoad(Type.OBJECT)); + ret.append(Utility.createConversion(fact,Type.OBJECT,callbackMethod.getArgumentTypes()[0])); + indexIntoCallbackMethodForArguments++; + } + indexIntoObjectArrayForArguments=1; + } else { + // use local variable 0 (which is 'this' for a non-static method) + ret.append(new ALOAD(0)); + indexIntoCallbackMethodForArguments++; + } } -// if (bindsThisOrTarget(munger.getPointcut())) { - for (int i = startIndex, len=callbackMethod.getArgumentTypes().length; i < len; i++) { + if (hasTarget()) { + if (relatedPointcutBindsTarget) { + if (getKind().isTargetSameAsThis()) { + ret.append(InstructionFactory.createLoad(objectArrayType, localProceedArgArray)); + ret.append(Utility.createConstant(fact, relatedPointcutBindsThis?1:0)); + ret.append(InstructionFactory.createArrayLoad(Type.OBJECT)); + ret.append(Utility.createConversion(fact,Type.OBJECT,callbackMethod.getArgumentTypes()[0])); + indexIntoObjectArrayForArguments++; + indexIntoCallbackMethodForArguments++; + } else { + int position =(hasThis()&& relatedPointcutBindsThis?1:0); + ret.append(InstructionFactory.createLoad(objectArrayType, localProceedArgArray)); + ret.append(Utility.createConstant(fact, position)); + ret.append(InstructionFactory.createArrayLoad(Type.OBJECT)); + ret.append(Utility.createConversion(fact,Type.OBJECT,callbackMethod.getArgumentTypes()[position])); + indexIntoObjectArrayForArguments=position+1; + indexIntoCallbackMethodForArguments++; + } + } else { + if (getKind().isTargetSameAsThis()) { + //ret.append(new ALOAD(0)); + } else { + ret.append(InstructionFactory.createLoad(localAdviceMethod.getArgumentTypes()[0],hasThis()?1:0)); + indexIntoCallbackMethodForArguments++; + } + } + } + + + for (int i = indexIntoCallbackMethodForArguments, len=callbackMethod.getArgumentTypes().length; i < len; i++) { Type stateType = callbackMethod.getArgumentTypes()[i]; BcelWorld.fromBcel(stateType).resolve(world); if ("Lorg/aspectj/lang/JoinPoint;".equals(stateType.getSignature())) { ret.append(new ALOAD(localJp));// from localAdvice signature } else { ret.append(InstructionFactory.createLoad(objectArrayType, localProceedArgArray)); - ret.append(Utility.createConstant(fact, i-startIndex)); + ret.append(Utility.createConstant(fact, i-indexIntoCallbackMethodForArguments +indexIntoObjectArrayForArguments)); ret.append(InstructionFactory.createArrayLoad(Type.OBJECT)); ret.append(Utility.createConversion( fact, @@ -2688,6 +2727,7 @@ public class BcelShadow extends Shadow { )); } } + } else { Type proceedingJpType = Type.getType("Lorg/aspectj/lang/ProceedingJoinPoint;"); int localJp = localAdviceMethod.allocateLocal(proceedingJpType); @@ -2802,7 +2842,72 @@ public class BcelShadow extends Shadow { // return ret; } - public void weaveAroundClosure(BcelAdvice munger, boolean hasDynamicTest) { + private boolean bindsThis(BcelAdvice munger) { + UsesThisVisitor utv = new UsesThisVisitor(); + munger.getPointcut().accept(utv, null); + return utv.usesThis; + } + + private boolean bindsTarget(BcelAdvice munger) { + UsesTargetVisitor utv = new UsesTargetVisitor(); + munger.getPointcut().accept(utv, null); + return utv.usesTarget; + } + + private static class UsesThisVisitor extends IdentityPointcutVisitor { + boolean usesThis = false; + + public Object visit(ThisOrTargetPointcut node, Object data) { + if (node.isThis() && node.isBinding()) usesThis=true; + return node; + } + + public Object visit(AndPointcut node, Object data) { + if (!usesThis) node.getLeft().accept(this, data); + if (!usesThis) node.getRight().accept(this, data); + return node; + } + + public Object visit(NotPointcut node, Object data) { + if (!usesThis) node.getNegatedPointcut().accept(this, data); + return node; + } + + public Object visit(OrPointcut node, Object data) { + if (!usesThis) node.getLeft().accept(this, data); + if (!usesThis) node.getRight().accept(this, data); + return node; + } + } + + + private static class UsesTargetVisitor extends IdentityPointcutVisitor { + boolean usesTarget = false; + + public Object visit(ThisOrTargetPointcut node, Object data) { + if (!node.isThis() && node.isBinding()) usesTarget=true; + return node; + } + + public Object visit(AndPointcut node, Object data) { + if (!usesTarget) node.getLeft().accept(this, data); + if (!usesTarget) node.getRight().accept(this, data); + return node; + } + + public Object visit(NotPointcut node, Object data) { + if (!usesTarget) node.getNegatedPointcut().accept(this, data); + return node; + } + + public Object visit(OrPointcut node, Object data) { + if (!usesTarget) node.getLeft().accept(this, data); + if (!usesTarget) node.getRight().accept(this, data); + return node; + } + } + + public void weaveAroundClosure(BcelAdvice munger, boolean hasDynamicTest) { InstructionFactory fact = getFactory(); enclosingMethod.setCanInline(false); @@ -2895,9 +3000,19 @@ public class BcelShadow extends Shadow { } } + // initialize the bit flags for this shadow + int bitflags =0x000000; + if (getKind().isTargetSameAsThis()) bitflags|=0x010000; + if (hasThis()) bitflags|=0x001000; + if (bindsThis(munger)) bitflags|=0x000100; + if (hasTarget()) bitflags|=0x000010; + if (bindsTarget(munger)) bitflags|=0x000001; + // ATAJ for @AJ aspect we need to link the closure with the joinpoint instance if (munger.getConcreteAspect()!=null && munger.getConcreteAspect().isAnnotationStyleAspect() && munger.getDeclaringAspect()!=null && munger.getDeclaringAspect().resolve(world).isAnnotationStyleAspect()) { + // stick the bitflags on the stack and call the variant of linkClosureAndJoinPoint that takes an int + closureInstantiation.append(fact.createConstant(new Integer(bitflags))); closureInstantiation.append(Utility.createInvoke( getFactory(), getWorld(), @@ -2906,7 +3021,7 @@ public class BcelShadow extends Shadow { UnresolvedType.forName("org.aspectj.runtime.internal.AroundClosure"), Modifier.PUBLIC, "linkClosureAndJoinPoint", - "()Lorg/aspectj/lang/ProceedingJoinPoint;" + "(I)Lorg/aspectj/lang/ProceedingJoinPoint;" ) )); } |