Do not recompute stack frames when instrumenting bytecode.

It turns out that we do not need to do that. See comment in
`ProfilerVisitor.java`. Also, since recomputing stack frame
map was the only reason we needed to implement
`getCommonSuperClass` we can now remove its implementation
that was causing problems on Java 7 due to a cyclic dependency
involving class loader because we would try to load a class
we are currently transforming and transformer is triggered just
before classloading.

//cc @namin who worked on this code with me.

2 files changed, 21 insertions, 25 deletions

diff --git a/src/partest/scala/tools/partest/javaagent/ASMTransformer.java b/src/partest/scala/tools/partest/javaagent/ASMTransformer.java
index 7338e2b01b..878c8613d5 100644
--- a/src/partest/scala/tools/partest/javaagent/ASMTransformer.java
+++ b/src/partest/scala/tools/partest/javaagent/ASMTransformer.java
@@ -26,33 +26,16 @@ public class ASMTransformer implements ClassFileTransformer {
         className.startsWith("instrumented/"));
   }
 	
-	public byte[] transform(final ClassLoader classLoader, String className, Class<?> classBeingRedefined, ProtectionDomain protectionDomain, byte[] classfileBuffer) {
+	public byte[] transform(final ClassLoader classLoader, final String className, Class<?> classBeingRedefined, ProtectionDomain protectionDomain, byte[] classfileBuffer) {
 	  if (shouldTransform(className)) {
-	    ClassWriter writer = new ClassWriter(ClassWriter.COMPUTE_FRAMES | ClassWriter.COMPUTE_MAXS) {
-	      // this is copied verbatim from the superclass,
-	      // except that we use the outer class loader
+	    ClassWriter writer = new ClassWriter(ClassWriter.COMPUTE_MAXS) {
 	      @Override protected String getCommonSuperClass(final String type1, final String type2) {
-	        Class<?> c, d;
-	        try {
-	          c = Class.forName(type1.replace('/', '.'), false, classLoader);
-	          d = Class.forName(type2.replace('/', '.'), false, classLoader);
-	        } catch (Exception e) {
-	          throw new RuntimeException(e.toString());
-	        }
-	        if (c.isAssignableFrom(d)) {
-	          return type1;
-	        }
-	        if (d.isAssignableFrom(c)) {
-	          return type2;
-	        }
-	        if (c.isInterface() || d.isInterface()) {
-	          return "java/lang/Object";
-	        } else {
-	          do {
-	            c = c.getSuperclass();
-	          } while (!c.isAssignableFrom(d));
-	          return c.getName().replace('.', '/');
-	        }
+	        // Since we are not recomputing stack frame map, this should never be called we override this method because
+	        // default implementation uses reflection for implementation and might try to load the class that we are
+	        // currently processing. That leads to weird results like swallowed exceptions and classes being not
+	        // transformed.
+	        throw new RuntimeException("Unexpected call to getCommonSuperClass(" + type1 + ", " + type2 +
+	            ") while transforming " + className);
 	      }
 	    };
   		ProfilerVisitor visitor = new ProfilerVisitor(writer);
diff --git a/src/partest/scala/tools/partest/javaagent/ProfilerVisitor.java b/src/partest/scala/tools/partest/javaagent/ProfilerVisitor.java
index ac83f66506..8306327b14 100644
--- a/src/partest/scala/tools/partest/javaagent/ProfilerVisitor.java
+++ b/src/partest/scala/tools/partest/javaagent/ProfilerVisitor.java
@@ -33,6 +33,19 @@ public class ProfilerVisitor extends ClassVisitor implements Opcodes {
       // only instrument non-abstract methods
       if((access & ACC_ABSTRACT) == 0) {
         assert(className != null);
+        /* The following instructions do not modify compressed stack frame map so
+         * we don't need to worry about recalculating stack frame map. Specifically,
+         * let's quote "ASM 4.0, A Java bytecode engineering library" guide (p. 40):
+         *
+         *   In order to save space, a compiled method does not contain one frame per
+         *   instruction: in fact it contains only the frames for the instructions
+         *   that correspond to jump targets or exception handlers, or that follow
+         *   unconditional jump instructions. Indeed the other frames can be easily
+         *   and quickly inferred from these ones.
+         *
+         * Instructions below are just loading constants and calling a method so according
+         * to definition above they do not contribute to compressed stack frame map.
+         */
         mv.visitLdcInsn(className);
         mv.visitLdcInsn(name);
         mv.visitLdcInsn(desc);