Don't insert stackmap frames into class files with version < 1.6. (#667)

For certain probes additional frames needs to be inserted, but only from
class file version 1.6 on.

6 files changed, 107 insertions, 17 deletions

diff --git a/org.jacoco.core.test/src/org/jacoco/core/internal/instr/InstrSupportTest.java b/org.jacoco.core.test/src/org/jacoco/core/internal/instr/InstrSupportTest.java
index fb1cb3e3..95de8bd5 100644
--- a/org.jacoco.core.test/src/org/jacoco/core/internal/instr/InstrSupportTest.java
+++ b/org.jacoco.core.test/src/org/jacoco/core/internal/instr/InstrSupportTest.java
@@ -12,9 +12,13 @@
 package org.jacoco.core.internal.instr;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
 
+import org.jacoco.core.internal.BytecodeVersion;
 import org.junit.Before;
 import org.junit.Test;
+import org.objectweb.asm.Opcodes;
 import org.objectweb.asm.util.Printer;
 import org.objectweb.asm.util.Textifier;
 import org.objectweb.asm.util.TraceMethodVisitor;
@@ -34,6 +38,24 @@ public class InstrSupportTest {
 	}
 
 	@Test
+	public void needFrames_should_return_false_for_versions_less_than_1_6() {
+		assertFalse(InstrSupport.needsFrames(Opcodes.V1_1));
+		assertFalse(InstrSupport.needsFrames(Opcodes.V1_2));
+		assertFalse(InstrSupport.needsFrames(Opcodes.V1_3));
+		assertFalse(InstrSupport.needsFrames(Opcodes.V1_4));
+		assertFalse(InstrSupport.needsFrames(Opcodes.V1_5));
+	}
+
+	@Test
+	public void needFrames_should_return_true_for_versions_greater_than_or_equal_to_1_6() {
+		assertTrue(InstrSupport.needsFrames(Opcodes.V1_6));
+		assertTrue(InstrSupport.needsFrames(Opcodes.V1_7));
+		assertTrue(InstrSupport.needsFrames(Opcodes.V1_8));
+		assertTrue(InstrSupport.needsFrames(Opcodes.V9));
+		assertTrue(InstrSupport.needsFrames(BytecodeVersion.V10));
+	}
+
+	@Test
 	public void testAssertNotIntrumentedPositive() {
 		InstrSupport.assertNotInstrumented("run", "Foo");
 	}
diff --git a/org.jacoco.core.test/src/org/jacoco/core/test/validation/ClassFileVersionsTest.java b/org.jacoco.core.test/src/org/jacoco/core/test/validation/ClassFileVersionsTest.java
index d73fc642..87d78a21 100644
--- a/org.jacoco.core.test/src/org/jacoco/core/test/validation/ClassFileVersionsTest.java
+++ b/org.jacoco.core.test/src/org/jacoco/core/test/validation/ClassFileVersionsTest.java
@@ -15,7 +15,12 @@ import static org.junit.Assert.assertEquals;
 import static org.objectweb.asm.Opcodes.ACC_PUBLIC;
 import static org.objectweb.asm.Opcodes.ACC_SUPER;
 import static org.objectweb.asm.Opcodes.ALOAD;
+import static org.objectweb.asm.Opcodes.F_NEW;
+import static org.objectweb.asm.Opcodes.IFEQ;
+import static org.objectweb.asm.Opcodes.ILOAD;
 import static org.objectweb.asm.Opcodes.INVOKESPECIAL;
+import static org.objectweb.asm.Opcodes.INVOKEVIRTUAL;
+import static org.objectweb.asm.Opcodes.POP;
 import static org.objectweb.asm.Opcodes.RETURN;
 import static org.objectweb.asm.Opcodes.V1_1;
 import static org.objectweb.asm.Opcodes.V1_2;
@@ -30,6 +35,7 @@ import static org.objectweb.asm.Opcodes.V9;
 import java.io.IOException;
 
 import org.jacoco.core.instr.Instrumenter;
+import org.jacoco.core.internal.BytecodeVersion;
 import org.jacoco.core.internal.instr.InstrSupport;
 import org.jacoco.core.runtime.IRuntime;
 import org.jacoco.core.runtime.SystemPropertiesRuntime;
@@ -37,7 +43,9 @@ import org.junit.Test;
 import org.objectweb.asm.ClassReader;
 import org.objectweb.asm.ClassVisitor;
 import org.objectweb.asm.ClassWriter;
+import org.objectweb.asm.Label;
 import org.objectweb.asm.MethodVisitor;
+import org.objectweb.asm.Opcodes;
 
 /**
  * Test class inserted stackmap frames for different class file versions.
@@ -85,12 +93,17 @@ public class ClassFileVersionsTest {
 	}
 
 	@Test
-	public void test_1_9() throws IOException {
+	public void test_9() throws IOException {
 		testVersion(V9, true);
 	}
 
+	@Test
+	public void test_10() throws IOException {
+		testVersion(BytecodeVersion.V10, true);
+	}
+
 	private void testVersion(int version, boolean frames) throws IOException {
-		final byte[] original = createClass(version);
+		final byte[] original = createClass(version, frames);
 
 		IRuntime runtime = new SystemPropertiesRuntime();
 		Instrumenter instrumenter = new Instrumenter(runtime);
@@ -99,30 +112,52 @@ public class ClassFileVersionsTest {
 		assertFrames(instrumented, frames);
 	}
 
-	private void assertFrames(byte[] source, boolean expected) {
-		final boolean[] hasFrames = new boolean[] { false };
+	private void assertFrames(byte[] source, final boolean expected) {
+		int version = BytecodeVersion.get(source);
+		source = BytecodeVersion.downgradeIfNeeded(version, source);
 		new ClassReader(source)
 				.accept(new ClassVisitor(InstrSupport.ASM_API_VERSION) {
 
 					@Override
 					public MethodVisitor visitMethod(int access, String name,
-							String desc, String signature, String[] exceptions) {
+							String desc, String signature,
+							String[] exceptions) {
 						return new MethodVisitor(InstrSupport.ASM_API_VERSION) {
+							boolean frames = false;
 
 							@Override
 							public void visitFrame(int type, int nLocal,
-									Object[] local, int nStack, Object[] stack) {
-								hasFrames[0] = true;
+									Object[] local, int nStack,
+									Object[] stack) {
+								frames = true;
 							}
 
+							@Override
+							public void visitEnd() {
+								assertEquals(Boolean.valueOf(expected),
+										Boolean.valueOf(frames));
+							}
 						};
 					}
-
 				}, 0);
-		assertEquals(Boolean.valueOf(expected), Boolean.valueOf(hasFrames[0]));
 	}
 
-	private byte[] createClass(int version) {
+	/**
+	 * Creates a class that requires a frame before the return statement. Also
+	 * for this class instrumentation should insert another frame.
+	 * 
+	 * <code><pre>
+	 * public class Sample {
+	 *   public Sample(boolean b){
+	 *     if(b){
+	 *       toString();
+	 *     }
+	 *     return;
+	 *   }
+	 * }
+	 * </pre></code>
+	 */
+	private byte[] createClass(int version, boolean frames) {
 
 		ClassWriter cw = new ClassWriter(0);
 		MethodVisitor mv;
@@ -130,13 +165,26 @@ public class ClassFileVersionsTest {
 		cw.visit(version, ACC_PUBLIC + ACC_SUPER, "org/jacoco/test/Sample",
 				null, "java/lang/Object", null);
 
-		mv = cw.visitMethod(ACC_PUBLIC, "<init>", "()V", null, null);
+		mv = cw.visitMethod(ACC_PUBLIC, "<init>", "(Z)V", null, null);
 		mv.visitCode();
 		mv.visitVarInsn(ALOAD, 0);
 		mv.visitMethodInsn(INVOKESPECIAL, "java/lang/Object", "<init>", "()V",
 				false);
+		mv.visitVarInsn(ILOAD, 1);
+		Label l1 = new Label();
+		mv.visitJumpInsn(IFEQ, l1);
+		mv.visitVarInsn(ALOAD, 0);
+		mv.visitMethodInsn(INVOKEVIRTUAL, "java/lang/Object", "toString",
+				"()Ljava/lang/String;", false);
+		mv.visitInsn(POP);
+		mv.visitLabel(l1);
+		if (frames) {
+			mv.visitFrame(F_NEW, 2,
+					new Object[] { "org/jacoco/test/Sample", Opcodes.INTEGER },
+					0, new Object[] {});
+		}
 		mv.visitInsn(RETURN);
-		mv.visitMaxs(1, 1);
+		mv.visitMaxs(1, 2);
 		mv.visitEnd();
 
 		cw.visitEnd();
diff --git a/org.jacoco.core/src/org/jacoco/core/instr/Instrumenter.java b/org.jacoco.core/src/org/jacoco/core/instr/Instrumenter.java
index 85cba592..da956fd9 100644
--- a/org.jacoco.core/src/org/jacoco/core/instr/Instrumenter.java
+++ b/org.jacoco.core/src/org/jacoco/core/instr/Instrumenter.java
@@ -29,6 +29,7 @@ import org.jacoco.core.internal.data.CRC64;
 import org.jacoco.core.internal.flow.ClassProbesAdapter;
 import org.jacoco.core.internal.instr.ClassInstrumenter;
 import org.jacoco.core.internal.instr.IProbeArrayStrategy;
+import org.jacoco.core.internal.instr.InstrSupport;
 import org.jacoco.core.internal.instr.ProbeArrayStrategyFactory;
 import org.jacoco.core.internal.instr.SignatureRemover;
 import org.jacoco.core.runtime.IExecutionDataAccessorGenerator;
@@ -97,7 +98,8 @@ public class Instrumenter {
 		final IProbeArrayStrategy strategy = ProbeArrayStrategyFactory
 				.createFor(classId, reader, accessorGenerator);
 		final ClassVisitor visitor = new ClassProbesAdapter(
-				new ClassInstrumenter(strategy, writer), true);
+				new ClassInstrumenter(strategy, writer),
+				InstrSupport.needsFrames(originalVersion));
 		reader.accept(visitor, ClassReader.EXPAND_FRAMES);
 		final byte[] instrumented = writer.toByteArray();
 		BytecodeVersion.set(instrumented, originalVersion);
diff --git a/org.jacoco.core/src/org/jacoco/core/internal/instr/InstrSupport.java b/org.jacoco.core/src/org/jacoco/core/internal/instr/InstrSupport.java
index b6632c62..c7b7cf84 100644
--- a/org.jacoco.core/src/org/jacoco/core/internal/instr/InstrSupport.java
+++ b/org.jacoco.core/src/org/jacoco/core/internal/instr/InstrSupport.java
@@ -158,6 +158,18 @@ public final class InstrSupport {
 	static final int CLINIT_ACC = Opcodes.ACC_SYNTHETIC | Opcodes.ACC_STATIC;
 
 	/**
+	 * Determines whether the given class file version requires stackmap frames.
+	 * 
+	 * @param version
+	 *            class file version
+	 * @return <code>true</code> if frames are required
+	 */
+	public static boolean needsFrames(final int version) {
+		// consider major version only (due to 1.1 anomaly)
+		return (version & 0xff) >= Opcodes.V1_6;
+	}
+
+	/**
 	 * Ensures that the given member does not correspond to a internal member
 	 * created by the instrumentation process. This would mean that the class is
 	 * already instrumented.
@@ -173,8 +185,8 @@ public final class InstrSupport {
 	public static void assertNotInstrumented(final String member,
 			final String owner) throws IllegalStateException {
 		if (member.equals(DATAFIELD_NAME) || member.equals(INITMETHOD_NAME)) {
-			throw new IllegalStateException(format(
-					"Class %s is already instrumented.", owner));
+			throw new IllegalStateException(
+					format("Class %s is already instrumented.", owner));
 		}
 	}
 
diff --git a/org.jacoco.core/src/org/jacoco/core/internal/instr/ProbeArrayStrategyFactory.java b/org.jacoco.core/src/org/jacoco/core/internal/instr/ProbeArrayStrategyFactory.java
index 38d572af..a4fb82b0 100644
--- a/org.jacoco.core/src/org/jacoco/core/internal/instr/ProbeArrayStrategyFactory.java
+++ b/org.jacoco.core/src/org/jacoco/core/internal/instr/ProbeArrayStrategyFactory.java
@@ -45,7 +45,6 @@ public final class ProbeArrayStrategyFactory {
 
 		final String className = reader.getClassName();
 		final int version = BytecodeVersion.get(reader.b);
-		final boolean withFrames = version >= Opcodes.V1_6;
 
 		if (isInterfaceOrModule(reader)) {
 			final ProbeCounter counter = getProbeCounter(reader);
@@ -61,7 +60,7 @@ public final class ProbeArrayStrategyFactory {
 			}
 		} else {
 			return new ClassFieldProbeArrayStrategy(className, classId,
-					withFrames, accessorGenerator);
+					InstrSupport.needsFrames(version), accessorGenerator);
 		}
 	}
 
diff --git a/org.jacoco.doc/docroot/doc/changes.html b/org.jacoco.doc/docroot/doc/changes.html
index ee534423..33fa7def 100644
--- a/org.jacoco.doc/docroot/doc/changes.html
+++ b/org.jacoco.doc/docroot/doc/changes.html
@@ -20,6 +20,13 @@
 
 <h2>Snapshot Build @qualified.bundle.version@ (@build.date@)</h2>
 
+<h3>Fixed Bugs</h3>
+<ul>
+  <li>Don't insert stackmap frames into class files with version &lt; 1.6,
+      this fixes regression which was introduced in version 0.6.5
+      (GitHub <a href="https://github.com/jacoco/jacoco/issues/667">#667</a>).</li>
+</ul>
+
 <h2>Release 0.8.1 (2018/03/21)</h2>
 
 <h3>New Features</h3>