From f2e45fccfe53ff79ae34f9ca6cae1afa0d153e53 Mon Sep 17 00:00:00 2001 From: Nada Amin Date: Fri, 28 Sep 2012 01:32:44 +0200 Subject: Fix class loader issues in instrumentation tests. The ASM ClassWriter uses a wimpy class loader when computing common superclasses. This could cause a ClassNotFoundException in the transform method (at reader.accept). This exception gets swallowed, resulting in a class that should be instrumented to silently not be. The fix is to override getCommonSuperClass to use the correct class loader. Trivia: This bug was discovered while 'stress-testing' this instrumentation scheme on the Coursera students, to check that they implement one method in terms of another in the assignment. --- .../tools/partest/javaagent/ASMTransformer.java | 30 ++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) (limited to 'src/partest') diff --git a/src/partest/scala/tools/partest/javaagent/ASMTransformer.java b/src/partest/scala/tools/partest/javaagent/ASMTransformer.java index 494a5a99be..b6bec2f598 100644 --- a/src/partest/scala/tools/partest/javaagent/ASMTransformer.java +++ b/src/partest/scala/tools/partest/javaagent/ASMTransformer.java @@ -26,9 +26,35 @@ public class ASMTransformer implements ClassFileTransformer { className.startsWith("instrumented/")); } - public byte[] transform(ClassLoader loader, String className, Class classBeingRedefined, ProtectionDomain protectionDomain, byte[] classfileBuffer) { + public byte[] transform(final ClassLoader classLoader, String className, Class classBeingRedefined, ProtectionDomain protectionDomain, byte[] classfileBuffer) { if (shouldTransform(className)) { - ClassWriter writer = new ClassWriter(ClassWriter.COMPUTE_FRAMES | ClassWriter.COMPUTE_MAXS); + 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 + @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('.', '/'); + } + } + }; ProfilerVisitor visitor = new ProfilerVisitor(writer); ClassReader reader = new ClassReader(classfileBuffer); reader.accept(visitor, 0); -- cgit v1.2.3 From 0a967e1cc18d631a58d467d9bec0e67860ca3520 Mon Sep 17 00:00:00 2001 From: Grzegorz Kossakowski Date: Fri, 9 Nov 2012 22:22:42 -0800 Subject: Correct whitespace in `ASMTransformer.java`. Let's stick to 2 spaces for indentation (and no tabs). --- .../tools/partest/javaagent/ASMTransformer.java | 54 +++++++++++----------- 1 file changed, 27 insertions(+), 27 deletions(-) (limited to 'src/partest') diff --git a/src/partest/scala/tools/partest/javaagent/ASMTransformer.java b/src/partest/scala/tools/partest/javaagent/ASMTransformer.java index b6bec2f598..7338e2b01b 100644 --- a/src/partest/scala/tools/partest/javaagent/ASMTransformer.java +++ b/src/partest/scala/tools/partest/javaagent/ASMTransformer.java @@ -28,33 +28,33 @@ public class ASMTransformer implements ClassFileTransformer { public byte[] transform(final ClassLoader classLoader, 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 - @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('.', '/'); - } - } - }; + 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 + @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('.', '/'); + } + } + }; ProfilerVisitor visitor = new ProfilerVisitor(writer); ClassReader reader = new ClassReader(classfileBuffer); reader.accept(visitor, 0); -- cgit v1.2.3 From b2776b40b28d312a8cc0cad0f35b2c3cb81abefb Mon Sep 17 00:00:00 2001 From: Grzegorz Kossakowski Date: Fri, 9 Nov 2012 22:29:09 -0800 Subject: Set `canRetransform` flag to `false` in instrumentation. We do not need to retransform classes once they are loaded. All instrumentation byte-code is pushed at loading time. This fixes a problem with Java 7 that was failing to add a transformer because we did not declare retransformation capability in `MANIFEST.MF` file in Java agent jar. Java 6 allowed to add transformer due to a bug. --- src/partest/scala/tools/partest/javaagent/ProfilingAgent.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/partest') diff --git a/src/partest/scala/tools/partest/javaagent/ProfilingAgent.java b/src/partest/scala/tools/partest/javaagent/ProfilingAgent.java index c2e4dc69f4..3b18987040 100644 --- a/src/partest/scala/tools/partest/javaagent/ProfilingAgent.java +++ b/src/partest/scala/tools/partest/javaagent/ProfilingAgent.java @@ -20,6 +20,6 @@ public class ProfilingAgent { // and the test-case itself won't be loaded yet. We rely here on the fact that ASMTransformer does // not depend on Scala library. In case our assumptions are wrong we can always insert call to // inst.retransformClasses. - inst.addTransformer(new ASMTransformer(), true); + inst.addTransformer(new ASMTransformer(), false); } } -- cgit v1.2.3 From a9bbfec8d58f68bd9105789754373f205d9981b1 Mon Sep 17 00:00:00 2001 From: Grzegorz Kossakowski Date: Fri, 9 Nov 2012 22:58:47 -0800 Subject: 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. --- .../tools/partest/javaagent/ASMTransformer.java | 33 ++++++---------------- .../tools/partest/javaagent/ProfilerVisitor.java | 13 +++++++++ 2 files changed, 21 insertions(+), 25 deletions(-) (limited to 'src/partest') 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); -- cgit v1.2.3