Summary: | around advice does not work when LTW | ||||||
---|---|---|---|---|---|---|---|
Product: | [Tools] AspectJ | Reporter: | liu wen zhong <liuwenz> | ||||
Component: | Compiler | Assignee: | Andrew Clement <aclement> | ||||
Status: | RESOLVED FIXED | QA Contact: | |||||
Severity: | normal | ||||||
Priority: | P3 | CC: | matthew_webster | ||||
Version: | 1.5.0RC1 | ||||||
Target Milestone: | 1.5.1 | ||||||
Hardware: | PC | ||||||
OS: | Windows XP | ||||||
Whiteboard: | |||||||
Attachments: |
|
Description
liu wen zhong
2005-12-19 05:32:32 EST
my test Hello class: public class Hello { /** * @param args */ public static void main(String[] args) { // TODO Auto-generated method stub sayHello(); } public static void sayHello() { System.out.println("Hello"); sayWorld(); } public static int sayWorld() { System.out.println("World"); return 0; } } Matthew is going to try and recreate this with the supplied info. This can easily be reproduces with supplied testcase (thanks). The priority should be to find a good workaround if possible. 1. User confirms that before/after rather than works 2. I can confirm that defining a concrete sub-aspect using code style (rather than XML) also works but defining one using annotation style fails in a similar way. I also get the failure with both source and binary weaving. 3. It may work if the abstract aspect is defined using annotation style This can easily be reproduced with supplied testcase (thanks). The priority should be to find a good workaround if possible. 1. User confirms that before/after rather than around advice works 2. I can confirm that defining a concrete sub-aspect using code style (rather than XML) also works but defining one using annotation style fails in a similar way. I also get the failure with both source and binary weaving. 3. It may work if the abstract aspect is defined using annotation style BTW looking at the weaver options "-XlazyTjp" is unecessary because it is now the default (although ironically it does not apply to around advice) and -Xreweavable should not be used (-XnotReweavable is the default for LTW) because it is unlikely that the code will need to be rewoven before going straight into the JVM and probably hurts performance (see Bug 114897). It does occur because of the mixing of styles. The abstract aspect being code style, then the concrete sub-aspect (generated from the XML) being annotation style. The problem is that when 'collecting' up the advice as part of processing the sub-aspect, the super aspect is returning 'true' for hasExtraParameter() and yet the extraVar is null. A simple guard on the null value gets us over the problem in this case - but there is possibly something to look into, examining why hasExtraParameter() is returning true. the null guard is in 1.5.0 final. Created attachment 32085 [details]
Failing LTW testcase
Unfortunately while the NPE is cured we have a VerifyError instead. The World1 sub-aspect is generated and defined. The test is woven but cannot be defined.
java.lang.VerifyError: (class: Hello, method: sayWorld signature: ()I) Unable to pop operand off an empty stack
at java.lang.Class.getDeclaredMethods0(Native Method)
at java.lang.Class.privateGetDeclaredMethods(Class.java:2365)
at java.lang.Class.getMethod0(Class.java:2611)
at java.lang.Class.getMethod(Class.java:1579)
at org.aspectj.tools.ajc.AjcTestCase.run(AjcTestCase.java:608)
at org.aspectj.testing.RunSpec.execute(RunSpec.java:56)
at org.aspectj.testing.AjcTest.runTest(AjcTest.java:68)
at org.aspectj.testing.XMLBasedAjcTestCase.runTest(XMLBasedAjcTestCase.java:111)
at org.aspectj.systemtest.ajc150.Ajc150Tests.testNPEInBcelAdviceWithConcreteAspect_pr121385(Ajc150Tests.java:873)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
at java.lang.reflect.Method.invoke(Method.java:585)
at junit.framework.TestCase.runTest(TestCase.java:154)
at junit.framework.TestCase.runBare(TestCase.java:127)
at junit.framework.TestResult$1.protect(TestResult.java:106)
at junit.framework.TestResult.runProtected(TestResult.java:124)
at junit.framework.TestResult.run(TestResult.java:109)
at junit.framework.TestCase.run(TestCase.java:118)
at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:478)
at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:344)
at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:196)
info AspectJ Weaver Version DEVELOPMENT built on
info register classloader org.aspectj.weaver.loadtime.WeavingURLClassLoader
info using /C:/temp/ajcSandbox/ajcTest24128.tmp/META-INF/aop.xml
info generating class 'World1'
info weaving 'Hello'
info weaver operating in reweavable mode. Need to verify any required types exist.
weaveinfo Join point 'method-execution(void Hello.sayHello())' in Type 'Hello' (Hello.java:7) advised by around advice from 'World1' (World.aj:7)
weaveinfo Join point 'method-execution(int Hello.sayWorld())' in Type 'Hello' (Hello.java:12) advised by around advice from 'World1' (World.aj:7)
info generating class 'Hello$AjcClosure1'
info generating class 'Hello$AjcClosure3'
Interesting that there is something missing off the stack and the code now commented out with a null check would load a value onto the stack. The problem may lie with trying to define the sub-aspect while initializing the weaving adaptor. When running in a different environment (AOSGi) I noticed that when World1 is defined the super-aspect World is loaded and weaving is attempted. In the harness this does not happen as the WeavingURLClassLoader uses a flag to avoid recursion during adaptor initialization. However although aspect World1 is woven during the generation process, World will not be woven (see Bug 119657). There are 2 other concrete aop.xml tests in the harness in AtAjLTWTests . Changing them to use around instead of before advice only causes the AspectJ test to fail with a VerifyError; the @AspectJ one passes. So this may be a problem with around advice and/or mixing styles. However these tests use inner aspects so are not the same as the new test. Matthew - here is a patch - can you apply it to the weaver project and see if it helps your situation? ==========8<================ Index: src/org/aspectj/weaver/bcel/BcelAdvice.java =================================================================== RCS file: /home/technology/org.aspectj/modules/weaver/src/org/aspectj/weaver/bcel/BcelAdvice.java,v retrieving revision 1.43 diff -u -r1.43 BcelAdvice.java --- src/org/aspectj/weaver/bcel/BcelAdvice.java 19 Dec 2005 15:06:51 -0000 1.43 +++ src/org/aspectj/weaver/bcel/BcelAdvice.java 21 Dec 2005 16:33:35 -0000 @@ -400,7 +400,8 @@ if (exposedState.getAspectInstance() != null) { il.append(BcelRenderer.renderExpr(fact, world, exposedState.getAspectInstance())); } - final boolean isAnnotationStyleAspect = getConcreteAspect()!=null && getConcreteAspect().isAnnotationStyleAspect(); + boolean x = this.getDeclaringAspect().resolve(world).isAnnotationStyleAspect(); + final boolean isAnnotationStyleAspect = getConcreteAspect()!=null && getConcreteAspect().isAnnotationStyleAspect() && x; boolean previousIsClosure = false; for (int i = 0, len = exposedState.size(); i < len; i++) { if (exposedState.isErroneousVar(i)) continue; // Erroneous vars have already had error msgs reported! @@ -455,12 +456,14 @@ } else if (hasExtraParameter()) { previousIsClosure = false; //extra var can be null here (@Aj aspect extends abstract code style, advice in code style) - if (extraVar != null) { +// if (extraVar != null) { extraVar.appendLoadAndConvert( il, fact, getExtraParameterType().resolve(world)); - } + +// } else +// il.append(InstructionConstants.ACONST_NULL); } else { previousIsClosure = false; getConcreteAspect().getWorld().getMessageHandler().handleMessage( Index: src/org/aspectj/weaver/bcel/BcelShadow.java =================================================================== RCS file: /home/technology/org.aspectj/modules/weaver/src/org/aspectj/weaver/bcel/BcelShadow.java,v retrieving revision 1.86 diff -u -r1.86 BcelShadow.java --- src/org/aspectj/weaver/bcel/BcelShadow.java 12 Dec 2005 10:48:46 -0000 1.86 +++ src/org/aspectj/weaver/bcel/BcelShadow.java 21 Dec 2005 16:33:36 -0000 @@ -2221,7 +2221,7 @@ munger.getAdviceArgSetup( this, null, - (munger.getConcreteAspect().isAnnotationStyleAspect())? + (munger.getConcreteAspect().isAnnotationStyleAspect() && munger.getDeclaringAspect().resolve(world).isAnnotationStyleAspect())? this.loadThisJoinPoint(): new InstructionList(InstructionConstants.ACONST_NULL))); // adviceMethodInvocation = ==========8<================ the bug appears to be that we use two mechanisms to build the arguments for an around advice call - which mechanism we use depends on whether the advice was declared in a code style aspect or an annotation style aspect. The problem here is that we ask the advice for the style used by the concrete aspect and the concrete aspect is annotation style. But of course the advice was declared in a code style aspect. We shouldn't check the concrete aspect but the declaring aspect for the advice. Changing the code to do this check fixes this problem and we can remove the null check that was added just before 1.5.0 final. proper fix checked in. will be in the next dev build. fix available. |