Community
Participate
Working Groups
I have an aspect that captures around() a pcd and returns an Object[], though the actual methods being instrumented might return any valid POJO array, i understand that AspectJ will take care of casting at assignment. I expected the following code to work properly, but at runtime i get a java.lang.VerifyError as shown below: F:\wd\Hello>ajbrowser HelloWorld.lst java.lang.VerifyError: (class: de/rohith/PrinterWorld, method: returnArrayWithCl oning signature: ()[Ljava/lang/Integer;) Incompatible argument to function at de.rohith.HelloWorld.main(HelloWorld.java:18) Exception in thread "main" I suspect the compiler fails to notice the return types of the methods being caught at compile time. I have tested this code with both ajbrowser as well as AJDT, both result in the same error output. -----------HelloWorldAspect.java--------------- package de.rohith; import java.lang.Object; public aspect HelloWorldAspect { private int callDepth = -1; public HelloWorldAspect() { } pointcut hello(): !within(HelloWorldAspect); pointcut method(): execution(public (*[]) de..*(..)); pointcut cloning(): call(* java.lang.Object.clone()); declare warning: method() && hello(): "*[] returning method called" ; Object[] around(): cflow(method()) && cloning() && hello() { print("", thisEnclosingJoinPointStaticPart); Object[] ret = proceed(); return (Object[])ret.clone(); } private void print(String prefix, Object message) { for (int i = 0, spaces = callDepth * 2; i < spaces; i++) { System.out.print(" "); } System.out.println(prefix + message); } } -----------PrinterWorld.java------------ package de.rohith; public class PrinterWorld { private Integer[] intArray = new Integer[2]; public PrinterWorld() { } public void print() { System.out.println("Hello World!"); } public Integer returnInt() { return new Integer(3); } public Integer[] returnArrayWithCloning() { for (int i = 0; i < intArray.length; i++) { intArray[i] = new Integer(i++); } return (Integer[])intArray.clone(); } public Integer[] returnArrayWithoutCloning() { return intArray; } } -----------HelloWorld.java------------ package de.rohith; public class HelloWorld { public static void main(String[] args) { PrinterWorld p = new PrinterWorld(); p.print(); Integer i = p.returnInt(); Integer[] intArray = p.returnArrayWithCloning(); Integer[] array2 = p.returnArrayWithoutCloning(); } }
Created attachment 14155 [details] Contains all the classes to reproduce this bug
This is almost certainly related to the bug in the handling of [] in type patterns, as reported in bug 72531. Any verify error is serious and we will fix this in the 1.2.1 release. I'll post to this bug report as soon as a fix is available for download from the AspectJ download page.
This turns out to be a really interesting bug. The problem is that in 'marshalling' the parameters to call the around body, ajc gets confused and thinks that the target of the call (the intArray) is a Foo, and generates a line of code of the form: Foo foo = intArray; this is what causes the verify error. This is all wrapped up in the special treatment of array types and of the clone method. Note that clone() is redefined for an array type, to be public (you can't just call clone on an arbitrary Object), and to return a shallow copy of the array. But where does this special redefinition of the clone() method happen (in which type??). It's this unique combination of factors - a redefined method on a 'special' type that is causing the confusion. I'm off to spend some time reading in the JVM and JLS specs to see what they have to say on the topic before deciding on the right fix....
JSL 10.7.....
this is not the first time those magical JLS 10.7 array members cause trouble. We're already special casing the length field somewhere (actually, it's more special since it is represented in bytecode by the ARRAYLENGTH bytecode and not by a field at all).
related to Bug 67665
I understand from JLS 10.7 that it is unclear what type represents an array type at runtime, but if this bug is related to Bug 67665 then i think the basic assumption that AspectJ supports all the Java types fails. According to JLS 10.1, an array is a Java type. Another reason i would assume that arrays are supported as types is in "AspectJ 1.1 quick refernce" among TypePat it clearly mentions that it accepts an array type.
when I said it was related to Bug 67665, I meant that I raised _implementation_ issues of treatment of .length and .clone() there, and that there are some cleanup issues with array syntax there as well. I don't think 10.7 is at all unclear about what types represent at runtime. Array types are types. They have some methods and one field. We need to capture calls to those methods (and accesses and assignments to that field) as join points. Nobody is talking about language changes here. We're talking about fixing a bug, and I noted that array treatment in general is a place where we've had bugs before.
Ok - fix checked in. What I've done is what we thought we'd have to do. When we grab the target for a method-call shadow and we recognize it as being one of these nasty clone calls on Object, I have a quick look before the shadow to see what the 'thing' is on the stack. There are a number of possibilities (that I have captured in the testcase), basically we might encounter: - a load instruction, in which case we ask the instruction what type it is working with (querying the local variable tag) and this helps us work out that it is an array. - a field access instruction, in which case we ask the field what type it is. - a anewarray instruction, in which case we *know* its an array (although why anyone would write '(Integer[])new Integer[5].clone()' I dont know!) - a multianewarray instruction, in which case *know* its an array. I've put a lot of guards in so that we don't do this extra processing unless it meets the criteria of being this nasty case. It will also blow up if it encounters an instruction other than those expected when trying to determine the real type of the target - if we didn't blow up then we would fail later at verify time. its a bit messy but I think it is quite a robust solution.
I can't look at the code right now, but two reactions to the solution description: * the localVariable tag won't work at all if someone has stripped off debugging information. Whatever solution we have should perhaps work better if we have debugging info, but better not generate a bad classfile if we don't. * even if we're lucky enough to have debugging information, ?: can surprise you ((testA ? exp0 : exp1) (testB ? exp2 : exp3) (testC ? exp4 : exp5)).clone()
Hi Erik. Yes, I hadn't thought about them stripping that debug info off. I just couldn't think of a nicer solution to this problem (unless we say compiler restriction). I believe that at least now we will fail at compile time rather than runtime. Did you have a nicer fix in mind for this? My first go was to try and generate the code that looked like this: public static final Object clone_aroundBody0(X x, Object y) { if (y instanceof Object[]) { return ((Object[])y).clone(); } else { return y.clone(); } } but that still fails verification because 'return y.clone()' exists in the bytecode. I then thought well - is the only time we will ever try and create a method like this: public static final Object clone_aroundBody0(Object y) { return y.clone(); } when the clone was originally on an array type. If its true then in this case we could always produce: public static final Object[] clone_aroundBody0(Object[] y) { return (Object[])y.clone(); } but I wasnt sure if that was one assumption too far?
Yeah, I see your point. The proposed solution we had was worse, too, since ((Object) new int[3]) instanceof Object[] is false. I'm still worried about the fragility of this solution without debugging info, but there it is. This is exactly the case where a separate set of people working on the AspectJ semantics might come up with a better solution that we could then steal if the current one turns out to be too fragile in practice *grin*. We should probably close this as fixed.
I'm going on holiday at the weekend for two weeks and want to write down my last minute thoughts on this bug so I don't forget them whilst I'm in the Maldives. I may not get to finishing this bug off before I go. The oxford guys didn't seem to think a 1.4 compiler should still be producing the 'Object.clone()' bytecode, they seemed to think the one they are using is producing the right '<arraytype>.clone()' bytecode. I need to check if this is really the case. It could be a bug in the Eclipse compiler so I need to check it on SUN and IBM 1.4 compilers. (And check the polyglot compiler they use?) I do think my current solution will fail at compile time if we can't determine the actual type on which clone() is called - but if there is no debug information I don't think it will fail in a nice way - it will fail with an NPE as I'm assuming the local variable tag is found. It should gracefully fail if the tag is missing.
not useful targetting 1.2.1
not planning to do any more with this for now... basic verifyerror was fixed long ago.
