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()";

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>

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>

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";
+		}
+	}
+}

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

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;

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));

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;

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...