diff options
13 files changed, 236 insertions, 42 deletions
diff --git a/org.aspectj.matcher/src/main/java/org/aspectj/weaver/Advice.java b/org.aspectj.matcher/src/main/java/org/aspectj/weaver/Advice.java index d6c8ea87f..cd786e724 100644 --- a/org.aspectj.matcher/src/main/java/org/aspectj/weaver/Advice.java +++ b/org.aspectj.matcher/src/main/java/org/aspectj/weaver/Advice.java @@ -83,6 +83,10 @@ public abstract class Advice extends ShadowMunger { ret.concreteAspect = inAspect; return ret; } + + public boolean isAroundAdvice() { + return attribute.getKind() == AdviceKind.Around; + } public static Advice makeSoftener(World world, Pointcut entry, TypePattern exceptionType, ResolvedType inAspect, IHasSourceLocation loc) { diff --git a/org.aspectj.matcher/src/main/java/org/aspectj/weaver/Shadow.java b/org.aspectj.matcher/src/main/java/org/aspectj/weaver/Shadow.java index 587d19c15..1a8ed5290 100644 --- a/org.aspectj.matcher/src/main/java/org/aspectj/weaver/Shadow.java +++ b/org.aspectj.matcher/src/main/java/org/aspectj/weaver/Shadow.java @@ -46,7 +46,8 @@ public abstract class Shadow { private ResolvedMember resolvedSignature; protected final Shadow enclosingShadow; protected List<ShadowMunger> mungers = Collections.emptyList(); - + protected boolean needAroundClosureStacking = false; + public int shadowId = nextShadowID++; // every time we build a shadow, it gets a new id // ---- @@ -628,6 +629,20 @@ public abstract class Shadow { /** Actually implement the (non-empty) mungers associated with this shadow */ private void implementMungers() { World world = getIWorld(); + needAroundClosureStacking = false; + int annotationStyleWithAroundAndProceedCount = 0; + for (ShadowMunger munger: mungers) { + if (munger.getDeclaringType()!= null && + munger.getDeclaringType().isAnnotationStyleAspect() && + munger.isAroundAdvice() && + munger.bindsProceedingJoinPoint()) { + annotationStyleWithAroundAndProceedCount++; + if (annotationStyleWithAroundAndProceedCount>1) { + needAroundClosureStacking = true; + break; + } + } + } for (ShadowMunger munger : mungers) { if (munger.implementOn(this)) { world.reportMatch(munger, this); diff --git a/org.aspectj.matcher/src/main/java/org/aspectj/weaver/ShadowMunger.java b/org.aspectj.matcher/src/main/java/org/aspectj/weaver/ShadowMunger.java index c07d7e8e7..a0e49becc 100644 --- a/org.aspectj.matcher/src/main/java/org/aspectj/weaver/ShadowMunger.java +++ b/org.aspectj.matcher/src/main/java/org/aspectj/weaver/ShadowMunger.java @@ -303,5 +303,13 @@ public abstract class ShadowMunger implements PartialOrder.PartialComparable, IH // } // newShadowMunger.binaryFile = null; // } + + public boolean bindsProceedingJoinPoint() { + return false; + } + + public boolean isAroundAdvice() { + return false; + } } diff --git a/runtime/src/main/java/org/aspectj/lang/ProceedingJoinPoint.java b/runtime/src/main/java/org/aspectj/lang/ProceedingJoinPoint.java index d75aa4df4..ac85681ef 100644 --- a/runtime/src/main/java/org/aspectj/lang/ProceedingJoinPoint.java +++ b/runtime/src/main/java/org/aspectj/lang/ProceedingJoinPoint.java @@ -30,6 +30,19 @@ public interface ProceedingJoinPoint extends JoinPoint { void set$AroundClosure(AroundClosure arc); /** + * The joinpoint needs to know about its closure so that proceed can delegate to closure.run(). + * This internal method should not be called directly, and won't be visible to the end-user when + * packed in a jar (synthetic method). This should maintain a stack of closures as multiple around + * advice with proceed are targeting a joinpoint and the stack will need to be unwound when + * exiting nested advice. Passing a non null arc indicates a push, passing null indicates a pop. + * + * @param arc the around closure to associate with this joinpoint + */ + default void stack$AroundClosure(AroundClosure arc) { + throw new UnsupportedOperationException(); + } + + /** * Proceed with the next advice or target method invocation * * @return the result of proceeding diff --git a/runtime/src/main/java/org/aspectj/runtime/internal/AroundClosure.java b/runtime/src/main/java/org/aspectj/runtime/internal/AroundClosure.java index 41c165149..c7a9acc1e 100644 --- a/runtime/src/main/java/org/aspectj/runtime/internal/AroundClosure.java +++ b/runtime/src/main/java/org/aspectj/runtime/internal/AroundClosure.java @@ -72,6 +72,21 @@ public abstract class AroundClosure { * @param flags indicating whether this/target found at joinpoint and bound * @return the associated ProceedingJoinPoint */ + public ProceedingJoinPoint linkStackClosureAndJoinPoint(int flags) { + //TODO is this cast safe ? + ProceedingJoinPoint jp = (ProceedingJoinPoint)state[state.length-1]; + jp.stack$AroundClosure(this); + this.bitflags = flags; + return jp; + } + + /** + * This method is called to implicitly associate the closure with the joinpoint + * as required for @AJ aspect proceed() + * + * @param flags indicating whether this/target found at joinpoint and bound + * @return the associated ProceedingJoinPoint + */ public ProceedingJoinPoint linkClosureAndJoinPoint(int flags) { //TODO is this cast safe ? ProceedingJoinPoint jp = (ProceedingJoinPoint)state[state.length-1]; @@ -79,4 +94,10 @@ public abstract class AroundClosure { this.bitflags = flags; return jp; } + + public void unlink() { + ProceedingJoinPoint jp = (ProceedingJoinPoint)state[state.length-1]; + jp.stack$AroundClosure(null); + } + } diff --git a/runtime/src/main/java/org/aspectj/runtime/reflect/JoinPointImpl.java b/runtime/src/main/java/org/aspectj/runtime/reflect/JoinPointImpl.java index 69bff1e3c..0263ee841 100644 --- a/runtime/src/main/java/org/aspectj/runtime/reflect/JoinPointImpl.java +++ b/runtime/src/main/java/org/aspectj/runtime/reflect/JoinPointImpl.java @@ -13,6 +13,8 @@ package org.aspectj.runtime.reflect; +import java.util.Stack; + import org.aspectj.lang.JoinPoint; import org.aspectj.lang.ProceedingJoinPoint; import org.aspectj.lang.Signature; @@ -134,47 +136,73 @@ class JoinPointImpl implements ProceedingJoinPoint { return staticPart.toLongString(); } - // To proceed we need a closure to proceed on - private AroundClosure arc; + // To proceed we need a closure to proceed on. Generated code + // will either be using arc or arcs but not both. arcs being non-null + // indicates it is in use (even if an empty stack) + private AroundClosure arc = null; + private Stack<AroundClosure> arcs = null; public void set$AroundClosure(AroundClosure arc) { this.arc = arc; } + public void stack$AroundClosure(AroundClosure arc) { + // If input parameter arc is null this is the 'unlink' call from AroundClosure + if (arcs == null) { + arcs = new Stack<AroundClosure>(); + } + if (arc==null) { + this.arcs.pop(); + } else { + this.arcs.push(arc); + } + } + public Object proceed() throws Throwable { // when called from a before advice, but be a no-op - if (arc == null) - return null; - else - return arc.run(arc.getState()); + if (arcs == null) { + if (arc == null) { + return null; + } else { + return arc.run(arc.getState()); + } + } else { + return arcs.peek().run(arcs.peek().getState()); + } } public Object proceed(Object[] adviceBindings) throws Throwable { // when called from a before advice, but be a no-op - if (arc == null) + AroundClosure ac = null; + if (arcs == null) { + ac = arc; + } else { + ac = arcs.peek(); + } + + if (ac == null) { return null; - else { - + } 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(); + int flags = ac.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(); - + Object[] state = ac.getState(); + // 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); @@ -202,8 +230,8 @@ class JoinPointImpl implements ProceedingJoinPoint { // This previous variant doesn't seem to cope with only binding target at a joinpoint // which has both this and target. It forces you to supply this even if you didn't bind // it. -// firstArgumentIndexIntoAdviceBindings = (hasThis ? 1 : 0) + 1; -// state[hasThis ? 1 : 0] = adviceBindings[hasThis ? 1 : 0]; + // firstArgumentIndexIntoAdviceBindings = (hasThis ? 1 : 0) + 1; + // state[hasThis ? 1 : 0] = adviceBindings[hasThis ? 1 : 0]; int targetPositionInAdviceBindings = (hasThis && bindsThis) ? 1 : 0; firstArgumentIndexIntoAdviceBindings = ((hasThis&&bindsThis)?1:0)+((hasTarget&&bindsTarget&&!thisTargetTheSame)?1:0); @@ -213,12 +241,12 @@ class JoinPointImpl implements ProceedingJoinPoint { // 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; @@ -226,7 +254,7 @@ class JoinPointImpl implements ProceedingJoinPoint { // state[i] = adviceBindings[formalIndex]; // } // } - return arc.run(state); + return ac.run(state); } } diff --git a/tests/src/test/java/org/aspectj/systemtest/ajc1611/Ajc1611Tests.java b/tests/src/test/java/org/aspectj/systemtest/ajc1611/Ajc1611Tests.java index d974123ee..8f44cfa59 100644 --- a/tests/src/test/java/org/aspectj/systemtest/ajc1611/Ajc1611Tests.java +++ b/tests/src/test/java/org/aspectj/systemtest/ajc1611/Ajc1611Tests.java @@ -103,17 +103,17 @@ public class Ajc1611Tests extends org.aspectj.testing.XMLBasedAjcTestCase { runTest("pr328840"); } - // public void testAnnoStyleAdviceChain_333274() { - // runTest("anno style advice chain"); - // } - // - // public void testAnnoStyleAdviceChain_333274_2() { - // runTest("code style advice chain"); - // } - // - // public void testAnnoStyleAdviceChain_333274_3() { - // runTest("code style advice chain - no inline"); - // } + public void testAnnoStyleAdviceChain_333274() { + runTest("anno style advice chain"); + } + + public void testAnnoStyleAdviceChain_333274_2() { + runTest("code style advice chain"); + } + + public void testAnnoStyleAdviceChain_333274_3() { + runTest("code style advice chain - no inline"); + } // --- diff --git a/tests/src/test/java/org/aspectj/systemtest/ajc193/Ajc193Tests.java b/tests/src/test/java/org/aspectj/systemtest/ajc193/Ajc193Tests.java index 1199db262..6a52df395 100644 --- a/tests/src/test/java/org/aspectj/systemtest/ajc193/Ajc193Tests.java +++ b/tests/src/test/java/org/aspectj/systemtest/ajc193/Ajc193Tests.java @@ -5,7 +5,7 @@ * which accompanies this distribution, and is available at * http://www.eclipse.org/legal/epl-v10.html *******************************************************************************/ -package org.aspectj.systemtest.ajc193; +package org.aspectj.systemtest.ajc193; import java.io.File; @@ -18,9 +18,13 @@ import junit.framework.Test; /** * @author Andy Clement - */ + */ public class Ajc193Tests extends XMLBasedAjcTestCaseForJava10OrLater { + public void testNestedAroundProceed() { + runTest("nested around proceed"); + } + public void testDeclareMixinOverweavingControl() { runTest("overweaving decm - control"); } diff --git a/tests/src/test/resources/org/aspectj/systemtest/ajc1611/ajc1611.xml b/tests/src/test/resources/org/aspectj/systemtest/ajc1611/ajc1611.xml index beb922924..1f42edfd6 100644 --- a/tests/src/test/resources/org/aspectj/systemtest/ajc1611/ajc1611.xml +++ b/tests/src/test/resources/org/aspectj/systemtest/ajc1611/ajc1611.xml @@ -117,7 +117,7 @@ </ajc-test> <ajc-test dir="bugs1611/pr333274" title="code style advice chain"> - <compile files="ma2/Annotation1.java ma2/aspect1/Aspect1.java ma2/aspect3/Aspect3.java ma2/Main.java ma2/Precedence.java" options="-1.5 "/> + <compile files="ma2/Annotation1.java ma2/aspect1/Aspect1.java ma2/aspect3/Aspect3.java ma2/Main.java ma2/Precedence.java" options="-1.5 -XnoInline"/> <run class="ma2.Main"> <stdout> <line text=">In Aspect1"/> diff --git a/tests/src/test/resources/org/aspectj/systemtest/ajc190/ajc190.xml b/tests/src/test/resources/org/aspectj/systemtest/ajc190/ajc190.xml index d6507dace..2936888cb 100644 --- a/tests/src/test/resources/org/aspectj/systemtest/ajc190/ajc190.xml +++ b/tests/src/test/resources/org/aspectj/systemtest/ajc190/ajc190.xml @@ -77,8 +77,8 @@ </ajc-test> <ajc-test dir="bugs190/modules/fff" title="compile module including aspects"> - <compile files="module-info.java pkg/Demo.java otherpkg/Azpect.java" modulepath="$runtime" outjar="demomodule.jar" options="-1.9"/> - <run modulepath="$runtime:demomodule.jar" module="demo/pkg.Demo"> + <compile files="module-info.java pkg/Demo.java otherpkg/Azpect.java" modulepath="$runtimemodule" outjar="demomodule.jar" options="-1.9"/> + <run modulepath="$runtimemodule:demomodule.jar" module="demo/pkg.Demo"> <stdout> <line text="Azpect running"/> <line text="Demo running"/> diff --git a/tests/src/test/resources/org/aspectj/systemtest/ajc193/ajc193.xml b/tests/src/test/resources/org/aspectj/systemtest/ajc193/ajc193.xml index 7d64d493e..2a4be40f8 100644 --- a/tests/src/test/resources/org/aspectj/systemtest/ajc193/ajc193.xml +++ b/tests/src/test/resources/org/aspectj/systemtest/ajc193/ajc193.xml @@ -2,6 +2,42 @@ <suite> + <ajc-test dir="bugs193/333274" vm="1.8" title="nested around proceed"> + <compile files="ma/aspect2/Aspect2.java,ma/aspect2/Annotation2.java,ma/aspect3/Aspect3.java,ma/aspect3/Annotation3.java,ma/Precedence.java,ma/Main.java,ma/aspect1/Aspect1.java,ma/aspect1/Annotation1.java" options="-showWeaveInfo -1.8 -XnoInline"> + <message kind="weave" text="Join point 'method-execution(int ma.Main$Dummy.retryTranslateAndTimeLimited())' in Type 'ma.Main$Dummy' (Main.java:16) advised by around advice from 'ma.aspect3.Aspect3' (Aspect3.java:11)"/> + <message kind="weave" text="Join point 'method-execution(int ma.Main$Dummy.retryTranslateAndTimeLimited())' in Type 'ma.Main$Dummy' (Main.java:16) advised by around advice from 'ma.aspect2.Aspect2' (Aspect2.java:11)"/> + <message kind="weave" text="Join point 'method-execution(int ma.Main$Dummy.retryTranslateAndTimeLimited())' in Type 'ma.Main$Dummy' (Main.java:16) advised by around advice from 'ma.aspect1.Aspect1' (Aspect1.java:12)"/> + </compile> + <!-- + >In Aspect1 +>In Aspect2 +>In Aspect3 +Method call +<In Aspect3 +<In Aspect2 +=In Aspect1 +Method call +<In Aspect1 + --> + <run class="ma.Main"> + <stdout> + <line text=">In Aspect1"/> + <line text=">In Aspect2"/> + <line text=">In Aspect3"/> + <line text="Method call"/> + <line text="<In Aspect3"/> + <line text="<In Aspect2"/> + <line text="=In Aspect1"/> + <line text=">In Aspect2"/> + <line text=">In Aspect3"/> + <line text="Method call"/> + <line text="<In Aspect3"/> + <line text="<In Aspect2"/> + <line text="<In Aspect1"/> + </stdout> + </run> + </ajc-test> + <ajc-test dir="bugs193/543657" vm="1.8" title="overweaving decm - control"> <compile files="MoodIndicator.java,Code1.java" options="-showWeaveInfo -1.8"> <message kind="weave" text="Mixing interface 'MoodIndicator$Moody' (MoodIndicator.java) into type 'Code1' (Code1.java)"/> diff --git a/weaver/src/main/java/org/aspectj/weaver/bcel/BcelAdvice.java b/weaver/src/main/java/org/aspectj/weaver/bcel/BcelAdvice.java index d22b17d12..924e6e10c 100644 --- a/weaver/src/main/java/org/aspectj/weaver/bcel/BcelAdvice.java +++ b/weaver/src/main/java/org/aspectj/weaver/bcel/BcelAdvice.java @@ -76,7 +76,17 @@ class BcelAdvice extends Advice { super(attribute, pointcut, simplify(attribute.getKind(), adviceSignature)); this.concreteAspect = concreteAspect; } - + + public boolean bindsProceedingJoinPoint() { + UnresolvedType[] parameterTypes = signature.getParameterTypes(); + for (int i=0;i<parameterTypes.length;i++) { + if (parameterTypes[i].equals(UnresolvedType.PROCEEDING_JOINPOINT)) { + return true; + } + } + return false; + } + /** * A heavyweight BcelMethod object is only required for around advice that will be inlined. For other kinds of advice it is * possible to save some space. @@ -603,6 +613,7 @@ class BcelAdvice extends Advice { } else { previousIsClosure = true; il.append(closureInstantiation.copy()); + shadow.closureVarInitialized = true; } } } else if ("Lorg/aspectj/lang/JoinPoint$StaticPart;".equals(getSignature().getParameterTypes()[i] diff --git a/weaver/src/main/java/org/aspectj/weaver/bcel/BcelShadow.java b/weaver/src/main/java/org/aspectj/weaver/bcel/BcelShadow.java index c93a0f26e..6d0cb4f60 100644 --- a/weaver/src/main/java/org/aspectj/weaver/bcel/BcelShadow.java +++ b/weaver/src/main/java/org/aspectj/weaver/bcel/BcelShadow.java @@ -70,6 +70,7 @@ import org.aspectj.weaver.patterns.NotPointcut; import org.aspectj.weaver.patterns.OrPointcut; import org.aspectj.weaver.patterns.ThisOrTargetPointcut; + /* * Some fun implementation stuff: * @@ -881,6 +882,12 @@ public class BcelShadow extends Shadow { private Map<ResolvedType, AnnotationAccessVar> withinAnnotationVars = null; private Map<ResolvedType, AnnotationAccessVar> withincodeAnnotationVars = null; private boolean allArgVarsInitialized = false; + + // If in annotation style and the relevant advice is using PJP then this will + // be set to true when the closure variable is initialized - if it gets set + // (which means link() has been called) then we will need to call unlink() + // after the code has been run. + boolean closureVarInitialized = false; @Override public Var getThisVar() { @@ -2836,6 +2843,8 @@ public class BcelShadow extends Shadow { } } + BcelVar aroundClosureInstance = null; + public void weaveAroundClosure(BcelAdvice munger, boolean hasDynamicTest) { InstructionFactory fact = getFactory(); @@ -2934,21 +2943,66 @@ public class BcelShadow extends Shadow { bitflags |= 0x000001; } + closureVarInitialized = false; + // 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()) { + + aroundClosureInstance = genTempVar(AjcMemberMaker.AROUND_CLOSURE_TYPE); + closureInstantiation.append(fact.createDup(1)); + aroundClosureInstance.appendStore(closureInstantiation, fact); + // stick the bitflags on the stack and call the variant of linkClosureAndJoinPoint that takes an int closureInstantiation.append(fact.createConstant(Integer.valueOf(bitflags))); - closureInstantiation.append(Utility.createInvoke(getFactory(), getWorld(), - new MemberImpl(Member.METHOD, UnresolvedType.forName("org.aspectj.runtime.internal.AroundClosure"), - Modifier.PUBLIC, "linkClosureAndJoinPoint", String.format("%s%s", "(I)", "Lorg/aspectj/lang/ProceedingJoinPoint;")))); + if (needAroundClosureStacking) { + closureInstantiation.append(Utility.createInvoke(getFactory(), getWorld(), + new MemberImpl(Member.METHOD, UnresolvedType.forName("org.aspectj.runtime.internal.AroundClosure"), + Modifier.PUBLIC, "linkStackClosureAndJoinPoint", String.format("%s%s", "(I)", "Lorg/aspectj/lang/ProceedingJoinPoint;")))); + + } else { + closureInstantiation.append(Utility.createInvoke(getFactory(), getWorld(), + new MemberImpl(Member.METHOD, UnresolvedType.forName("org.aspectj.runtime.internal.AroundClosure"), + Modifier.PUBLIC, "linkClosureAndJoinPoint", String.format("%s%s", "(I)", "Lorg/aspectj/lang/ProceedingJoinPoint;")))); + } + } InstructionList advice = new InstructionList(); advice.append(munger.getAdviceArgSetup(this, null, closureInstantiation)); // invoke the advice - advice.append(munger.getNonTestAdviceInstructions(this)); + InstructionHandle tryUnlinkPosition = advice.append(munger.getNonTestAdviceInstructions(this)); + + if (needAroundClosureStacking) { + // Call AroundClosure.unlink() in a 'finally' block + if (munger.getConcreteAspect() != null && munger.getConcreteAspect().isAnnotationStyleAspect() + && munger.getDeclaringAspect() != null + && munger.getDeclaringAspect().resolve(world).isAnnotationStyleAspect() + && closureVarInitialized) { + + // Call unlink when 'normal' flow occurring + aroundClosureInstance.appendLoad(advice, fact); + InstructionHandle unlinkInsn = advice.append(Utility.createInvoke(getFactory(), getWorld(), new MemberImpl(Member.METHOD, UnresolvedType + .forName("org.aspectj.runtime.internal.AroundClosure"), Modifier.PUBLIC, "unlink", + "()V"))); + + InstructionHandle jumpOverHandler = advice.append(InstructionConstants.NOP); + + // Call unlink in finally block + InstructionHandle handlerStart = advice.append(InstructionConstants.POP); + aroundClosureInstance.appendLoad(advice, fact); + advice.append(Utility.createInvoke(getFactory(), getWorld(), new MemberImpl(Member.METHOD, UnresolvedType + .forName("org.aspectj.runtime.internal.AroundClosure"), Modifier.PUBLIC, "unlink", + "()V"))); + advice.append(InstructionConstants.ACONST_NULL); + advice.append(InstructionConstants.ATHROW); + InstructionHandle jumpTarget = advice.append(InstructionConstants.NOP); + jumpOverHandler.setInstruction(InstructionFactory.createBranchInstruction(Constants.GOTO, jumpTarget)); + enclosingMethod.addExceptionHandler(tryUnlinkPosition, unlinkInsn, handlerStart, null/* ==finally */, false); + } + } + advice.append(returnConversionCode); if (getKind() == Shadow.MethodExecution && linenumber > 0) { advice.getStart().addTargeter(new LineNumberTag(linenumber)); |