From bb9d2de08e63266a93ac1167f87b07813561d559 Mon Sep 17 00:00:00 2001 From: aclement Date: Tue, 1 Nov 2005 21:55:21 +0000 Subject: [PATCH] pr93253: lazytjp the default --- .../org/aspectj/ajdt/ajc/BuildArgParser.java | 4 +- .../compiler/batch/PerformanceTestCase.java | 3 +- .../lazyTjpXLintWarning/LazyTjpTest4.java | 4 + .../lazyTjpXLintWarning/LazyTjpTest5.java | 22 +++++ tests/bugs/lazyTjpXLintWarning/Scenario3.aj | 2 +- .../aspectj/systemtest/xlint/XLintTests.java | 59 ++++++++++++- .../aspectj/systemtest/xlint/xlint-tests.xml | 78 +++++++++++++++++ weaver/src/org/aspectj/weaver/Lint.java | 8 +- .../aspectj/weaver/XlintDefault.properties | 4 +- .../org/aspectj/weaver/bcel/BcelAdvice.java | 19 ++++- .../org/aspectj/weaver/bcel/BcelShadow.java | 83 +++++++++++++++---- 11 files changed, 263 insertions(+), 23 deletions(-) create mode 100644 tests/bugs/lazyTjpXLintWarning/LazyTjpTest5.java diff --git a/org.aspectj.ajdt.core/src/org/aspectj/ajdt/ajc/BuildArgParser.java b/org.aspectj.ajdt.core/src/org/aspectj/ajdt/ajc/BuildArgParser.java index bbfb8faf9..84498d494 100644 --- a/org.aspectj.ajdt.core/src/org/aspectj/ajdt/ajc/BuildArgParser.java +++ b/org.aspectj.ajdt.core/src/org/aspectj/ajdt/ajc/BuildArgParser.java @@ -383,6 +383,7 @@ public class BuildArgParser extends Main { public void parseOption(String arg, LinkedList args) { // XXX use ListIterator.remove() int nextArgIndex = args.indexOf(arg)+1; // XXX assumes unique // trim arg? + buildConfig.setXlazyTjp(true); // now default - MINOR could be pushed down and made default at a lower level if (LangUtil.isEmpty(arg)) { showWarning("empty arg found"); } else if (arg.equals("-inpath")) {; @@ -523,7 +524,8 @@ public class BuildArgParser extends Main { } else if (arg.equals("-XserializableAspects")) { buildConfig.setXserializableAspects(true); } else if (arg.equals("-XlazyTjp")) { - buildConfig.setXlazyTjp(true); + // do nothing as this is now on by default + showWarning("-XlazyTjp should no longer be used, build tjps lazily is now the default"); } else if (arg.startsWith("-Xreweavable")) { showWarning("-Xreweavable is on by default"); if (arg.endsWith(":compress")) { diff --git a/org.aspectj.ajdt.core/testsrc/org/aspectj/ajdt/internal/compiler/batch/PerformanceTestCase.java b/org.aspectj.ajdt.core/testsrc/org/aspectj/ajdt/internal/compiler/batch/PerformanceTestCase.java index bbbe9c58b..1565332d2 100644 --- a/org.aspectj.ajdt.core/testsrc/org/aspectj/ajdt/internal/compiler/batch/PerformanceTestCase.java +++ b/org.aspectj.ajdt.core/testsrc/org/aspectj/ajdt/internal/compiler/batch/PerformanceTestCase.java @@ -40,7 +40,8 @@ public class PerformanceTestCase extends CommandTestCase { // joinpoint method-execution(int LazyTjp.doit3(int)) because around advice is used [Xlint:canNotImplementLazyTjp]' // into an error so that we can use checkCompiles() ability to check errors occur. // Pass -proceedOnError to ensure even though we get that message, we still get the class file on disk - checkCompile("src1/LazyTjp.aj", new String[] {"-XlazyTjp","-Xlint:error","-proceedOnError"}, new int[] {96}); + checkCompile("src1/LazyTjp.aj", new String[] {"-Xlint:error","-proceedOnError"}, new int[] {96}); TestUtil.runMain("out", "LazyTjp"); } + } diff --git a/tests/bugs/lazyTjpXLintWarning/LazyTjpTest4.java b/tests/bugs/lazyTjpXLintWarning/LazyTjpTest4.java index 148a2f556..1025d1f31 100644 --- a/tests/bugs/lazyTjpXLintWarning/LazyTjpTest4.java +++ b/tests/bugs/lazyTjpXLintWarning/LazyTjpTest4.java @@ -13,6 +13,10 @@ public class LazyTjpTest4 { before() : execution(public void LazyTjpTest4.test1()) { System.out.println(thisJoinPoint); } + + before() : execution(public void LazyTjpTest4.test1()) { + System.out.println(thisJoinPoint); + } } } diff --git a/tests/bugs/lazyTjpXLintWarning/LazyTjpTest5.java b/tests/bugs/lazyTjpXLintWarning/LazyTjpTest5.java new file mode 100644 index 000000000..caa86a83d --- /dev/null +++ b/tests/bugs/lazyTjpXLintWarning/LazyTjpTest5.java @@ -0,0 +1,22 @@ +public class LazyTjpTest5 { + + public void test1 () { } + + private static aspect Aspect1 { + + private static boolean enabled = true; + + after () : if(enabled) && execution(public void LazyTjpTest5.test1()) { + System.out.println(thisJoinPoint); + } + + before() : execution(public void LazyTjpTest5.test1()) { + System.out.println(thisJoinPoint); + } + + void around() : execution(public void LazyTjpTest5.test1()) { + System.out.println(thisJoinPoint); + } + } + +} diff --git a/tests/bugs/lazyTjpXLintWarning/Scenario3.aj b/tests/bugs/lazyTjpXLintWarning/Scenario3.aj index 7cbb765fd..cbee2ba7c 100644 --- a/tests/bugs/lazyTjpXLintWarning/Scenario3.aj +++ b/tests/bugs/lazyTjpXLintWarning/Scenario3.aj @@ -7,7 +7,7 @@ public aspect Scenario3 { pointcut toBeTraced() : execution(* main(..)); Object around() : toBeTraced() { - // Object[] args = thisJoinPoint.getArgs(); + //Object[] args = thisJoinPoint.getArgs(); return proceed(); } diff --git a/tests/src/org/aspectj/systemtest/xlint/XLintTests.java b/tests/src/org/aspectj/systemtest/xlint/XLintTests.java index 21e0fd1f9..defcab2e3 100644 --- a/tests/src/org/aspectj/systemtest/xlint/XLintTests.java +++ b/tests/src/org/aspectj/systemtest/xlint/XLintTests.java @@ -12,6 +12,7 @@ package org.aspectj.systemtest.xlint; import java.io.File; import junit.framework.Test; import org.aspectj.testing.XMLBasedAjcTestCase; +import org.aspectj.weaver.bcel.BcelShadow; public class XLintTests extends org.aspectj.testing.XMLBasedAjcTestCase { @@ -100,8 +101,64 @@ public class XLintTests extends org.aspectj.testing.XMLBasedAjcTestCase { runTest("XLint warning for call PCD's using subtype of defining type (-1.3 -Xlint:ignore)"); } + + // the following five tests check various scenarios around the lazyTjp XLint message + public void test020(){ + runTest("no XLint warning: thisJoinPoint potentially lazy and nothing stopping it"); + assertTrue("Something prevented the lazytjp optimization from working??",BcelShadow.appliedLazyTjpOptimization); + } + + public void test021(){ + runTest("XLint warning: thisJoinPoint potentially lazy but stopped by around advice which doesn't use tjp"); + assertFalse("lazytjp optimization should have failed to be applied because of around advice at the jp", + BcelShadow.appliedLazyTjpOptimization); + } + + public void test022(){ + runTest("no XLint warning: thisJoinPoint not lazy (no if PCD) but would have been stopped anyway by around advice"); + assertFalse("lazytjp optimization should have failed to be applied because of around advice *and* before advice has no if() at the jp", + BcelShadow.appliedLazyTjpOptimization); + } + + public void test023(){ + runTest("no XLint warning: thisJoinPoint cannot be built lazily"); + assertFalse("lazytjp optimization should have failed to be applied because before advice has no if() at the jp", + BcelShadow.appliedLazyTjpOptimization); + } + + public void test024(){ + runTest("XLint warning: thisJoinPoint potentially lazy but stopped by around advice which uses tjp"); + assertFalse("lazytjp optimization should have failed to be applied because around advice uses tjp", + BcelShadow.appliedLazyTjpOptimization); + } + + public void test025(){ + runTest("check for xlazytjp warning if actually supplied"); + assertTrue("Something prevented the lazytjp optimization from working??",BcelShadow.appliedLazyTjpOptimization); + } + + public void test026(){ + runTest("lazytjp: warning when around advice uses tjp"); + } + + public void test027() { + runTest("lazytjp: warning when if missing on before advice"); + } + + public void test028() { + runTest("lazytjp: warning when if missing on after advice"); + } + + public void test029() { + runTest("lazytjp: multiple clashing advice preventing lazytjp"); + } + + public void test030() { + runTest("lazytjp: interfering before and around"); + } + // FIXME asc put this back in ! -// public void test020() { +// public void test031() { // if (is15VMOrGreater) // runTest("7 lint warnings"); // } diff --git a/tests/src/org/aspectj/systemtest/xlint/xlint-tests.xml b/tests/src/org/aspectj/systemtest/xlint/xlint-tests.xml index 05052cce7..077c4a4bc 100644 --- a/tests/src/org/aspectj/systemtest/xlint/xlint-tests.xml +++ b/tests/src/org/aspectj/systemtest/xlint/xlint-tests.xml @@ -2,6 +2,84 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/weaver/src/org/aspectj/weaver/Lint.java b/weaver/src/org/aspectj/weaver/Lint.java index 5713af3f9..f4518694d 100644 --- a/weaver/src/org/aspectj/weaver/Lint.java +++ b/weaver/src/org/aspectj/weaver/Lint.java @@ -54,6 +54,9 @@ public class Lint { public final Kind canNotImplementLazyTjp = new Kind("canNotImplementLazyTjp", "can not implement lazyTjp on this joinpoint {0} because around advice is used"); + public final Kind multipleAdviceStoppingLazyTjp = + new Kind("multipleAdviceStoppingLazyTjp", "can not implement lazyTjp at joinpoint {0} because of advice conflicts, see secondary locations to find conflicting advice"); + public final Kind needsSerialVersionUIDField = new Kind("needsSerialVersionUIDField", "serialVersionUID of type {0} needs to be set because of {1}"); @@ -92,7 +95,10 @@ public class Lint { public final Kind uncheckedAdviceConversion = new Kind("uncheckedAdviceConversion","unchecked conversion when advice applied at shadow {0}, expected {1} but advice uses {2}"); - + + public final Kind noGuardForLazyTjp = + new Kind("noGuardForLazyTjp","can not build thisJoinPoint lazily for this advice since it has no suitable guard. The advice applies at {0}"); + public Lint(World world) { this.world = world; } diff --git a/weaver/src/org/aspectj/weaver/XlintDefault.properties b/weaver/src/org/aspectj/weaver/XlintDefault.properties index 3f14de786..e0044b3d2 100644 --- a/weaver/src/org/aspectj/weaver/XlintDefault.properties +++ b/weaver/src/org/aspectj/weaver/XlintDefault.properties @@ -9,7 +9,9 @@ shadowNotInStructure = ignore unmatchedSuperTypeInCall = warning -canNotImplementLazyTjp = warning +canNotImplementLazyTjp = ignore +multipleAdviceStoppingLazyTjp=ignore +noGuardForLazyTjp=ignore uncheckedAdviceConversion = warning diff --git a/weaver/src/org/aspectj/weaver/bcel/BcelAdvice.java b/weaver/src/org/aspectj/weaver/bcel/BcelAdvice.java index f349e55c5..f8618e303 100644 --- a/weaver/src/org/aspectj/weaver/bcel/BcelAdvice.java +++ b/weaver/src/org/aspectj/weaver/bcel/BcelAdvice.java @@ -19,11 +19,12 @@ import java.util.Collection; import java.util.Collections; import java.util.Map; +import org.aspectj.apache.bcel.generic.InstructionConstants; import org.aspectj.apache.bcel.generic.InstructionFactory; import org.aspectj.apache.bcel.generic.InstructionHandle; import org.aspectj.apache.bcel.generic.InstructionList; -import org.aspectj.apache.bcel.generic.InstructionConstants; import org.aspectj.bridge.IMessage; +import org.aspectj.bridge.ISourceLocation; import org.aspectj.bridge.Message; import org.aspectj.weaver.Advice; import org.aspectj.weaver.AdviceKind; @@ -141,8 +142,22 @@ public class BcelAdvice extends Advice { } if ((getExtraParameterFlags() & ThisJoinPoint) != 0) { - ((BcelShadow)shadow).requireThisJoinPoint(pointcutTest != Literal.TRUE && getKind() != AdviceKind.Around); + boolean hasGuardTest = pointcutTest != Literal.TRUE && getKind() != AdviceKind.Around; + boolean isAround = getKind() == AdviceKind.Around; + ((BcelShadow)shadow).requireThisJoinPoint(hasGuardTest,isAround); ((BcelShadow)shadow).getEnclosingClass().warnOnAddedStaticInitializer(shadow,getSourceLocation()); + if (!hasGuardTest && world.getLint().multipleAdviceStoppingLazyTjp.isEnabled()) { + // collect up the problematic advice + ((BcelShadow)shadow).addAdvicePreventingLazyTjp(this); + } + if (!isAround && !hasGuardTest && world.getLint().noGuardForLazyTjp.isEnabled()) { + // can't build tjp lazily, no suitable test... + world.getLint().noGuardForLazyTjp.signal( + new String[] {shadow.toString()}, + getSourceLocation(), + new ISourceLocation[] { ((BcelShadow)shadow).getSourceLocation() } + ); + } } if ((getExtraParameterFlags() & ThisEnclosingJoinPointStaticPart) != 0) { diff --git a/weaver/src/org/aspectj/weaver/bcel/BcelShadow.java b/weaver/src/org/aspectj/weaver/bcel/BcelShadow.java index 0045130a0..d91056bc0 100644 --- a/weaver/src/org/aspectj/weaver/bcel/BcelShadow.java +++ b/weaver/src/org/aspectj/weaver/bcel/BcelShadow.java @@ -134,6 +134,9 @@ public class BcelShadow extends Shadow { private final LazyMethodGen enclosingMethod; private boolean fallsThrough; //XXX not used anymore + // SECRETAPI - for testing, this will tell us if the optimization succeeded *on the last shadow processed* + public static boolean appliedLazyTjpOptimization; + // Some instructions have a target type that will vary // from the signature (pr109728) (1.4 declaring type issue) private String actualInstructionTargetType; @@ -241,6 +244,14 @@ public class BcelShadow extends Shadow { } } + // records advice that is stopping us doing the lazyTjp optimization + private List badAdvice = null; + + public void addAdvicePreventingLazyTjp(BcelAdvice advice) { + if (badAdvice == null) badAdvice = new ArrayList(); + badAdvice.add(advice); + } + protected void prepareForMungers() { // if we're a constructor call, we need to remove the new:dup or the new:dup_x1:swap, // and store all our @@ -289,15 +300,43 @@ public class BcelShadow extends Shadow { } // now we ask each munger to request our state - isThisJoinPointLazy = world.isXlazyTjp(); - + isThisJoinPointLazy = true;//world.isXlazyTjp(); // lazy is default now + + badAdvice = null; for (Iterator iter = mungers.iterator(); iter.hasNext();) { ShadowMunger munger = (ShadowMunger) iter.next(); munger.specializeOn(this); } + initializeThisJoinPoint(); + if (thisJoinPointVar!=null && !isThisJoinPointLazy && badAdvice!=null && badAdvice.size()>1) { + // something stopped us making it a lazy tjp + // can't build tjp lazily, no suitable test... + int valid = 0; + for (Iterator iter = badAdvice.iterator(); iter.hasNext();) { + BcelAdvice element = (BcelAdvice) iter.next(); + ISourceLocation sLoc = element.getSourceLocation(); + if (sLoc!=null && sLoc.getLine()>0) valid++; + } + if (valid!=0) { + ISourceLocation[] badLocs = new ISourceLocation[valid]; + int i = 0; + for (Iterator iter = badAdvice.iterator(); iter.hasNext();) { + BcelAdvice element = (BcelAdvice) iter.next(); + ISourceLocation sLoc = element.getSourceLocation(); + if (sLoc!=null) badLocs[i++]=sLoc; + } + world.getLint().multipleAdviceStoppingLazyTjp.signal( + new String[] {this.toString()}, + getSourceLocation(),badLocs + ); + } + } + badAdvice=null; + + // If we are an expression kind, we require our target/arguments on the stack // before we do our actual thing. However, they may have been removed // from the stack as the shadowMungers have requested state. @@ -972,13 +1011,20 @@ public class BcelShadow extends Shadow { public final Var getThisEnclosingJoinPointStaticPartVar() { return getThisEnclosingJoinPointStaticPartBcelVar(); } - - public void requireThisJoinPoint(boolean hasGuardTest) { - if (!hasGuardTest) { - isThisJoinPointLazy = false; - } else { - lazyTjpConsumers++; - } + + public void requireThisJoinPoint(boolean hasGuardTest, boolean isAround) { + if (!isAround){ + if (!hasGuardTest) { + isThisJoinPointLazy = false; + } else { + lazyTjpConsumers++; + } + } +// if (!hasGuardTest) { +// isThisJoinPointLazy = false; +// } else { +// lazyTjpConsumers++; +// } if (thisJoinPointVar == null) { thisJoinPointVar = genTempVar(UnresolvedType.forName("org.aspectj.lang.JoinPoint")); } @@ -986,7 +1032,7 @@ public class BcelShadow extends Shadow { public Var getThisJoinPointVar() { - requireThisJoinPoint(false); + requireThisJoinPoint(false,false); return thisJoinPointVar; } @@ -998,6 +1044,7 @@ public class BcelShadow extends Shadow { } if (isThisJoinPointLazy) { + appliedLazyTjpOptimization = true; createThisJoinPoint(); // make sure any state needed is initialized, but throw the instructions out if (lazyTjpConsumers == 1) return; // special case only one lazyTjpUser @@ -1008,6 +1055,7 @@ public class BcelShadow extends Shadow { il.append(thisJoinPointVar.createStore(fact)); range.insert(il, Range.OutsideBefore); } else { + appliedLazyTjpOptimization = false; InstructionFactory fact = getFactory(); InstructionList il = createThisJoinPoint(); il.append(thisJoinPointVar.createStore(fact)); @@ -1021,11 +1069,13 @@ public class BcelShadow extends Shadow { ShadowMunger munger = (ShadowMunger) i.next(); if (munger instanceof Advice) { if ( ((Advice)munger).getKind() == AdviceKind.Around) { - world.getLint().canNotImplementLazyTjp.signal( - new String[] {toString()}, - getSourceLocation(), - new ISourceLocation[] { munger.getSourceLocation() } - ); + if (munger.getSourceLocation()!=null) { // do we know enough to bother reporting? + world.getLint().canNotImplementLazyTjp.signal( + new String[] {toString()}, + getSourceLocation(), + new ISourceLocation[] { munger.getSourceLocation() } + ); + } return false; } } @@ -1039,8 +1089,10 @@ public class BcelShadow extends Shadow { InstructionList il = new InstructionList(); if (isThisJoinPointLazy) { + // If we're lazy, build the join point right here. il.append(createThisJoinPoint()); + // Does someone else need it? If so, store it for later retrieval if (lazyTjpConsumers > 1) { il.append(thisJoinPointVar.createStore(fact)); @@ -1050,6 +1102,7 @@ public class BcelShadow extends Shadow { il.insert(thisJoinPointVar.createLoad(fact)); } } else { + // If not lazy, its already been built and stored, just retrieve it thisJoinPointVar.appendLoad(il, fact); } -- 2.39.5