Fix for Bugzilla #39479, #40109

based on patch contributed by Andy Clement

Generalizes the patch with a method org.aspectj.weaver.bcel.Utility.copyInstruction
that works-around the bug in Select.copy().  Changed all calls to
Instruction.copy() to use this new method, would be nice to add the
rule:
   	 * declare error:
   	 *     call(* Instruction.copy()) && within(org.aspectj.weaver)
   	 *       && !withincode(* Utility.copyInstruction(Instruction)):
   	 *     "use Utility.copyInstruction to work-around bug in Select.copy()";

diff --git a/tests/ajcTests.xml b/tests/ajcTests.xml
index 8cf4bbc767808ec038ecbe9d7f4c8a8048375fd7..eed7526f93d27a32d937d3cbd19a011d430dec4a 100644 (file)
--- a/tests/ajcTests.xml
+++ b/tests/ajcTests.xml
@@ -6408,4 +6408,16 @@
             <message kind="error" line="13"/>       
         </compile>
     </ajc-test>
+    
+    <ajc-test dir="bugs" pr="39479"
+        title="NPE in bcel.LazyMethodGen when delegating from one ctor to a second that includes a switch">
+        <compile files="NewSwitch.java"/>
+        <run class="NewSwitch"/>
+    </ajc-test>
+
+    <ajc-test dir="bugs" pr="40109"
+        title="switch statement in aspects crashes weaving">
+        <compile files="SwitchInAround.java"/>
+        <run class="SwitchInAround"/>
+    </ajc-test>
 </suite>

diff --git a/tests/ajcTestsFailing.xml b/tests/ajcTestsFailing.xml
index 25314b7775c1c0f86c6844d971a23bd1ec0585c2..4292bacbde668edfac1b68eac739aef9a3753a53 100644 (file)
--- a/tests/ajcTestsFailing.xml
+++ b/tests/ajcTestsFailing.xml
@@ -9,11 +9,5 @@
         <compile files="lib.jar,TestAspect.aj,Test.java"/>
         <run class="Test"/>
     </ajc-test>
-    
-    <ajc-test dir="bugs" pr="39479"
-        title="NPE in bcel.LazyMethodGen when delegating from one ctor to a second that includes a switch">
-        <compile files="NewSwitch.java"/>
-        <run class="NewSwitch"/>
-    </ajc-test>
 
 </suite>

diff --git a/tests/bugs/SwitchInAround.java b/tests/bugs/SwitchInAround.java
new file mode 100644 (file)
index 0000000..c86d71a
--- /dev/null
+++ b/tests/bugs/SwitchInAround.java
@@ -0,0 +1,27 @@
+import org.aspectj.testing.Tester;
+
+public class SwitchInAround {
+       public static void main(String[] args) {
+               SwitchInAround o = new SwitchInAround();
+               Tester.checkEqual(o.doit(1), "1");
+               Tester.checkEqual(o.doit(2), "2");
+               Tester.checkEqual(o.doit(3), "default");
+       }
+       
+       public String doit(int i) {
+               return "doit";
+       }
+}
+
+privileged aspect A {
+       String around(int index): args(index) && call(String doit(int)) {
+               switch(index) {
+                       case 1:
+                               return "1";
+                       case 2:
+                               return "2";
+                       default:
+                               return "default";
+               }
+       }
+}
\ No newline at end of file

diff --git a/weaver/src/org/aspectj/weaver/bcel/BcelClassWeaver.java b/weaver/src/org/aspectj/weaver/bcel/BcelClassWeaver.java
index 0a4a9d3185ba5046f65477c88b1b1ae23a66598a..97c8fbdaed352e9e7d82a2e314659ca3e5eb46bd 100644 (file)
--- a/weaver/src/org/aspectj/weaver/bcel/BcelClassWeaver.java
+++ b/weaver/src/org/aspectj/weaver/bcel/BcelClassWeaver.java
@@ -443,7 +443,7 @@ class BcelClassWeaver implements IClassWeaver {
                        src != null;
                        src = src.getNext()) 
                {
-                       Instruction fresh = src.getInstruction().copy();
+                       Instruction fresh = Utility.copyInstruction(src.getInstruction());
                        InstructionHandle dest;
                        if (fresh instanceof CPInstruction) {
                                // need to reset index to go to new constant pool.  This is totally

diff --git a/weaver/src/org/aspectj/weaver/bcel/BcelShadow.java b/weaver/src/org/aspectj/weaver/bcel/BcelShadow.java
index 058231bab4c6ec1511c1dfc213643486984fccc7..00b2d2678a6032b8b210cbb15e3ecc7cdff729ce 100644 (file)
--- a/weaver/src/org/aspectj/weaver/bcel/BcelShadow.java
+++ b/weaver/src/org/aspectj/weaver/bcel/BcelShadow.java
@@ -953,7 +953,7 @@ public class BcelShadow extends Shadow {
         for (InstructionHandle ih = range.getStart(); ih != range.getEnd(); ih = ih.getNext()) {
             if (ih.getInstruction() instanceof ReturnInstruction) {
                 returns.add(ih);
-                ret = ih.getInstruction().copy();
+                ret = Utility.copyInstruction(ih.getInstruction());
             }
         }
         InstructionList retList;

diff --git a/weaver/src/org/aspectj/weaver/bcel/LazyMethodGen.java b/weaver/src/org/aspectj/weaver/bcel/LazyMethodGen.java
index f5d87a3e737c7a29128ba95d6098421d4974a511..569ac9793f224a4c959267777e419c4cc00a5139 100644 (file)
--- a/weaver/src/org/aspectj/weaver/bcel/LazyMethodGen.java
+++ b/weaver/src/org/aspectj/weaver/bcel/LazyMethodGen.java
@@ -728,7 +728,7 @@ public final class LazyMethodGen {
                 continue;
             }
             Instruction i = ih.getInstruction();
-            Instruction c = i.copy(); // Use clone for shallow copy
+            Instruction c = Utility.copyInstruction(i);
 
             if (c instanceof BranchInstruction)
                 map.put(ih, fresh.append((BranchInstruction) c));

diff --git a/weaver/src/org/aspectj/weaver/bcel/ShadowRange.java b/weaver/src/org/aspectj/weaver/bcel/ShadowRange.java
index 3bbc1cd8af525a282dbf7ff9d4997bc71ad4ec59..cf69bea9c1cede0bc5a8e214bb2393ec63b93e61 100644 (file)
--- a/weaver/src/org/aspectj/weaver/bcel/ShadowRange.java
+++ b/weaver/src/org/aspectj/weaver/bcel/ShadowRange.java
@@ -71,7 +71,7 @@ final class ShadowRange extends Range {
         for (InstructionHandle oldIh = start.getNext(); oldIh != end; oldIh = oldIh.getNext()) {
                    // first we copy the instruction itself.  
             Instruction oldI = oldIh.getInstruction();
-            Instruction freshI = (oldI == RANGEINSTRUCTION) ? oldI : oldI.copy();
+            Instruction freshI = (oldI == RANGEINSTRUCTION) ? oldI : Utility.copyInstruction(oldI);
 
                        // Now we add it to the new instruction list.
             InstructionHandle freshIh;

diff --git a/weaver/src/org/aspectj/weaver/bcel/Utility.java b/weaver/src/org/aspectj/weaver/bcel/Utility.java
index d2179010b709a39afbb40f1849caf0a21b07ab1e..93c2347bc361b8c84b40d0a887874639befa0211 100644 (file)
--- a/weaver/src/org/aspectj/weaver/bcel/Utility.java
+++ b/weaver/src/org/aspectj/weaver/bcel/Utility.java
@@ -21,6 +21,7 @@ import org.apache.bcel.Constants;
 import org.apache.bcel.classfile.ClassParser;
 import org.apache.bcel.classfile.JavaClass;
 import org.apache.bcel.classfile.Method;
+import org.apache.bcel.generic.ArrayType;
 import org.apache.bcel.generic.BIPUSH;
 import org.apache.bcel.generic.BasicType;
 import org.apache.bcel.generic.BranchInstruction;
@@ -34,9 +35,10 @@ import org.apache.bcel.generic.InstructionList;
 import org.apache.bcel.generic.InstructionTargeter;
 import org.apache.bcel.generic.LDC;
 import org.apache.bcel.generic.ObjectType;
-import org.apache.bcel.generic.ArrayType;
 import org.apache.bcel.generic.ReferenceType;
 import org.apache.bcel.generic.SIPUSH;
+import org.apache.bcel.generic.SWITCH;
+import org.apache.bcel.generic.Select;
 import org.apache.bcel.generic.TargetLostException;
 import org.apache.bcel.generic.Type;
 import org.aspectj.weaver.BCException;
@@ -419,6 +421,41 @@ public class Utility {
             throw new BCException("this really can't happen");
         }
        }
+       
+       /**
+        * Fix for Bugzilla #39479, #40109 patch contributed by Andy Clement
+        * 
+        * Need to manually copy Select instructions - if we rely on the the 'fresh' object
+        * created by copy(), the InstructionHandle array 'targets' inside the Select
+        * object will not have been deep copied, so modifying targets in fresh will modify
+        * the original Select - not what we want !  (It is a bug in BCEL to do with cloning
+        * Select objects).
+        * 
+        * <pre>
+        * declare error:
+        *     call(* Instruction.copy()) && within(org.aspectj.weaver)
+        *       && !withincode(* Utility.copyInstruction(Instruction)):
+        *     "use Utility.copyInstruction to work-around bug in Select.copy()";
+        * </pre>
+        */
+       public static Instruction copyInstruction(Instruction i) {
+               if (i instanceof Select) {
+                       Select freshSelect = (Select)i;
+                                 
+                       // Create a new targets array that looks just like the existing one
+                       InstructionHandle[] targets = new InstructionHandle[freshSelect.getTargets().length];
+                       for (int ii = 0; ii < targets.length; ii++) {
+                         targets[ii] = freshSelect.getTargets()[ii];
+                       }
+                                 
+                       // Create a new select statement with the new targets array
+                       SWITCH switchStatement = new SWITCH(freshSelect.getMatchs(),targets,freshSelect.getTarget());
+                       return (Select)switchStatement.getInstruction();        
+               } else {
+                       return i.copy(); // Use clone for shallow copy...
+               }
+       }
+       
 
        /** returns -1 if no source line attribute */
        // this naive version overruns the JVM stack size, if only Java understood tail recursion...