From 13b319a40f353f3c07eb0a7c4a40f3e3f6381573 Mon Sep 17 00:00:00 2001 From: jhugunin Date: Wed, 16 Jul 2003 23:19:54 +0000 Subject: [PATCH] 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 | 12 ++++++ tests/ajcTestsFailing.xml | 6 --- tests/bugs/SwitchInAround.java | 27 +++++++++++++ .../aspectj/weaver/bcel/BcelClassWeaver.java | 2 +- .../org/aspectj/weaver/bcel/BcelShadow.java | 2 +- .../aspectj/weaver/bcel/LazyMethodGen.java | 2 +- .../org/aspectj/weaver/bcel/ShadowRange.java | 2 +- .../src/org/aspectj/weaver/bcel/Utility.java | 39 ++++++++++++++++++- 8 files changed, 81 insertions(+), 11 deletions(-) create mode 100644 tests/bugs/SwitchInAround.java diff --git a/tests/ajcTests.xml b/tests/ajcTests.xml index 8cf4bbc76..eed7526f9 100644 --- a/tests/ajcTests.xml +++ b/tests/ajcTests.xml @@ -6408,4 +6408,16 @@ + + + + + + + + + + diff --git a/tests/ajcTestsFailing.xml b/tests/ajcTestsFailing.xml index 25314b777..4292bacbd 100644 --- a/tests/ajcTestsFailing.xml +++ b/tests/ajcTestsFailing.xml @@ -9,11 +9,5 @@ - - - - - diff --git a/tests/bugs/SwitchInAround.java b/tests/bugs/SwitchInAround.java new file mode 100644 index 000000000..c86d71a3c --- /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 0a4a9d318..97c8fbdae 100644 --- 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 058231bab..00b2d2678 100644 --- 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 f5d87a3e7..569ac9793 100644 --- 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 3bbc1cd8a..cf69bea9c 100644 --- 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 d2179010b..93c2347bc 100644 --- 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). + * + * 
+   	 * declare error:
+   	 *     call(* Instruction.copy()) && within(org.aspectj.weaver)
+   	 *       && !withincode(* Utility.copyInstruction(Instruction)):
+   	 *     "use Utility.copyInstruction to work-around bug in Select.copy()";
+   	 *
+ */ + 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... -- 2.39.5