Do not violate JVMS regarding initialization of final fields (#434)

Without this change instrumented classes can't pass checks
and cause IllegalAccessError starting from OpenJDK 9 EA b127
(see https://bugs.openjdk.java.net/browse/JDK-8157181).

10 files changed, 277 insertions, 30 deletions

diff --git a/org.jacoco.core/src/org/jacoco/core/internal/instr/FieldProbeArrayStrategy.java b/org.jacoco.core/src/org/jacoco/core/internal/instr/ClassFieldProbeArrayStrategy.java
index 26902c23..3b7bf91a 100644
--- a/org.jacoco.core/src/org/jacoco/core/internal/instr/FieldProbeArrayStrategy.java
+++ b/org.jacoco.core/src/org/jacoco/core/internal/instr/ClassFieldProbeArrayStrategy.java
@@ -18,11 +18,11 @@ import org.objectweb.asm.MethodVisitor;
 import org.objectweb.asm.Opcodes;
 
 /**
- * The strategy for regular classes and Java 8 interfaces which adds a static
- * field to hold the probe array and a static initialization method requesting
- * the probe array from the runtime.
+ * The strategy for regular classes adds a static field to hold the probe array
+ * and a static initialization method requesting the probe array from the
+ * runtime.
  */
-class FieldProbeArrayStrategy implements IProbeArrayStrategy {
+class ClassFieldProbeArrayStrategy implements IProbeArrayStrategy {
 
 	/**
 	 * Frame stack with a single boolean array.
@@ -37,26 +37,22 @@ class FieldProbeArrayStrategy implements IProbeArrayStrategy {
 	private final String className;
 	private final long classId;
 	private final boolean withFrames;
-	private final boolean isInterface;
-	private final int fieldAccess;
 	private final IExecutionDataAccessorGenerator accessorGenerator;
 
-	FieldProbeArrayStrategy(final String className, final long classId,
-			final boolean withFrames, final boolean isInterface,
-			final int fieldAccess,
+	ClassFieldProbeArrayStrategy(final String className, final long classId,
+			final boolean withFrames,
 			final IExecutionDataAccessorGenerator accessorGenerator) {
 		this.className = className;
 		this.classId = classId;
 		this.withFrames = withFrames;
-		this.isInterface = isInterface;
-		this.fieldAccess = fieldAccess;
 		this.accessorGenerator = accessorGenerator;
 	}
 
-	public int storeInstance(final MethodVisitor mv, final int variable) {
+	public int storeInstance(final MethodVisitor mv, final boolean clinit,
+			final int variable) {
 		mv.visitMethodInsn(Opcodes.INVOKESTATIC, className,
 				InstrSupport.INITMETHOD_NAME, InstrSupport.INITMETHOD_DESC,
-				isInterface);
+				false);
 		mv.visitVarInsn(Opcodes.ASTORE, variable);
 		return 1;
 	}
@@ -67,7 +63,7 @@ class FieldProbeArrayStrategy implements IProbeArrayStrategy {
 	}
 
 	private void createDataField(final ClassVisitor cv) {
-		cv.visitField(fieldAccess, InstrSupport.DATAFIELD_NAME,
+		cv.visitField(InstrSupport.DATAFIELD_ACC, InstrSupport.DATAFIELD_NAME,
 				InstrSupport.DATAFIELD_DESC, null, null);
 	}
 
diff --git a/org.jacoco.core/src/org/jacoco/core/internal/instr/ClassInstrumenter.java b/org.jacoco.core/src/org/jacoco/core/internal/instr/ClassInstrumenter.java
index 6cea48c6..a70c6731 100644
--- a/org.jacoco.core/src/org/jacoco/core/internal/instr/ClassInstrumenter.java
+++ b/org.jacoco.core/src/org/jacoco/core/internal/instr/ClassInstrumenter.java
@@ -70,7 +70,7 @@ public class ClassInstrumenter extends ClassProbesVisitor {
 		}
 		final MethodVisitor frameEliminator = new DuplicateFrameEliminator(mv);
 		final ProbeInserter probeVariableInserter = new ProbeInserter(access,
-				desc, frameEliminator, probeArrayStrategy);
+				name, desc, frameEliminator, probeArrayStrategy);
 		return new MethodInstrumenter(probeVariableInserter,
 				probeVariableInserter);
 	}
diff --git a/org.jacoco.core/src/org/jacoco/core/internal/instr/IProbeArrayStrategy.java b/org.jacoco.core/src/org/jacoco/core/internal/instr/IProbeArrayStrategy.java
index 8973785c..87a415a2 100644
--- a/org.jacoco.core/src/org/jacoco/core/internal/instr/IProbeArrayStrategy.java
+++ b/org.jacoco.core/src/org/jacoco/core/internal/instr/IProbeArrayStrategy.java
@@ -26,11 +26,13 @@ public interface IProbeArrayStrategy {
 	 * 
 	 * @param mv
 	 *            visitor to create code
+	 * @param clinit
+	 *            true in case of {@code <clinit>} method
 	 * @param variable
 	 *            variable index to store probe array to
 	 * @return maximum stack size required by the generated code
 	 */
-	int storeInstance(MethodVisitor mv, int variable);
+	int storeInstance(MethodVisitor mv, boolean clinit, int variable);
 
 	/**
 	 * Adds additional class members required by this strategy. This method is
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 ab054df3..5d34a66f 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
@@ -34,14 +34,37 @@ public final class InstrSupport {
 	/**
 	 * Access modifiers of the field that stores coverage information of a
 	 * class.
+	 *
+	 * According to Java Virtual Machine Specification <a href=
+	 * "https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-6.html#jvms-6.5.putstatic">
+	 * §6.5.putstatic</a> this field must not be final:
+	 *
+	 * <blockquote>
+	 * <p>
+	 * if the field is final, it must be declared in the current class, and the
+	 * instruction must occur in the {@code <clinit>} method of the current
+	 * class.
+	 * </p>
+	 * </blockquote>
 	 */
 	public static final int DATAFIELD_ACC = Opcodes.ACC_SYNTHETIC
-			| Opcodes.ACC_PRIVATE | Opcodes.ACC_STATIC | Opcodes.ACC_TRANSIENT
-			| Opcodes.ACC_FINAL;
+			| Opcodes.ACC_PRIVATE | Opcodes.ACC_STATIC | Opcodes.ACC_TRANSIENT;
 
 	/**
 	 * Access modifiers of the field that stores coverage information of a Java
 	 * 8 interface.
+	 *
+	 * According to Java Virtual Machine Specification <a href=
+	 * "https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-4.html#jvms-4.5-200-A.3">
+	 * §4.5</a>:
+	 *
+	 * <blockquote>
+	 * <p>
+	 * Fields of interfaces must have their ACC_PUBLIC, ACC_STATIC, and
+	 * ACC_FINAL flags set; they may have their ACC_SYNTHETIC flag set and must
+	 * not have any of the other flags.
+	 * </p>
+	 * </blockquote>
 	 */
 	public static final int DATAFIELD_INTF_ACC = Opcodes.ACC_SYNTHETIC
 			| Opcodes.ACC_PUBLIC | Opcodes.ACC_STATIC | Opcodes.ACC_FINAL;
@@ -71,6 +94,67 @@ public final class InstrSupport {
 			| Opcodes.ACC_PRIVATE | Opcodes.ACC_STATIC;
 
 	/**
+	 * Name of the interface initialization method.
+	 *
+	 * According to Java Virtual Machine Specification <a href=
+	 * "https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-2.html#jvms-2.9-200">
+	 * §2.9</a>:
+	 *
+	 * <blockquote>
+	 * <p>
+	 * A class or interface has at most one class or interface initialization
+	 * method and is initialized by invoking that method. The initialization
+	 * method of a class or interface has the special name {@code <clinit>},
+	 * takes no arguments, and is void.
+	 * </p>
+	 * <p>
+	 * Other methods named {@code <clinit>} in a class file are of no
+	 * consequence. They are not class or interface initialization methods. They
+	 * cannot be invoked by any Java Virtual Machine instruction and are never
+	 * invoked by the Java Virtual Machine itself.
+	 * </p>
+	 * <p>
+	 * In a class file whose version number is 51.0 or above, the method must
+	 * additionally have its ACC_STATIC flag set in order to be the class or
+	 * interface initialization method.
+	 * </p>
+	 * <p>
+	 * This requirement was introduced in Java SE 7. In a class file whose
+	 * version number is 50.0 or below, a method named {@code <clinit>} that is
+	 * void and takes no arguments is considered the class or interface
+	 * initialization method regardless of the setting of its ACC_STATIC flag.
+	 * </p>
+	 * </blockquote>
+	 *
+	 * And <a href=
+	 * "https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-4.html#jvms-4.6-200-A.6">
+	 * §4.6</a>:
+	 *
+	 * <blockquote>
+	 * <p>
+	 * Class and interface initialization methods are called implicitly by the
+	 * Java Virtual Machine. The value of their access_flags item is ignored
+	 * except for the setting of the ACC_STRICT flag.
+	 * </p>
+	 * </blockquote>
+	 */
+	static final String CLINIT_NAME = "<clinit>";
+
+	/**
+	 * Descriptor of the interface initialization method.
+	 *
+	 * @see #CLINIT_NAME
+	 */
+	static final String CLINIT_DESC = "()V";
+
+	/**
+	 * Access flags of the interface initialization method generated by JaCoCo.
+	 *
+	 * @see #CLINIT_NAME
+	 */
+	static final int CLINIT_ACC = Opcodes.ACC_SYNTHETIC | Opcodes.ACC_STATIC;
+
+	/**
 	 * 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.
diff --git a/org.jacoco.core/src/org/jacoco/core/internal/instr/InterfaceFieldProbeArrayStrategy.java b/org.jacoco.core/src/org/jacoco/core/internal/instr/InterfaceFieldProbeArrayStrategy.java
new file mode 100644
index 00000000..00228e9b
--- /dev/null
+++ b/org.jacoco.core/src/org/jacoco/core/internal/instr/InterfaceFieldProbeArrayStrategy.java
@@ -0,0 +1,154 @@
+/*******************************************************************************
+ * Copyright (c) 2009, 2016 Mountainminds GmbH & Co. KG and Contributors
+ * All rights reserved. This program and the accompanying materials
+ * are made available under the terms of the Eclipse Public License v1.0
+ * which accompanies this distribution, and is available at
+ * http://www.eclipse.org/legal/epl-v10.html
+ *
+ * Contributors:
+ *    Evgeny Mandrikov - initial API and implementation
+ *
+ *******************************************************************************/
+package org.jacoco.core.internal.instr;
+
+import org.jacoco.core.runtime.IExecutionDataAccessorGenerator;
+import org.objectweb.asm.ClassVisitor;
+import org.objectweb.asm.Label;
+import org.objectweb.asm.MethodVisitor;
+import org.objectweb.asm.Opcodes;
+
+/**
+ * This strategy for Java 8 interfaces adds a static method requesting the probe
+ * array from the runtime, a static field to hold the probe array and adds code
+ * for its initialization into interface initialization method.
+ */
+class InterfaceFieldProbeArrayStrategy implements IProbeArrayStrategy {
+
+	/**
+	 * Frame stack with a single boolean array.
+	 */
+	private static final Object[] FRAME_STACK_ARRZ = new Object[] { InstrSupport.DATAFIELD_DESC };
+
+	/**
+	 * Empty frame locals.
+	 */
+	private static final Object[] FRAME_LOCALS_EMPTY = new Object[0];
+
+	private final String className;
+	private final long classId;
+	private final int probeCount;
+	private final IExecutionDataAccessorGenerator accessorGenerator;
+
+	private boolean seenClinit = false;
+
+	InterfaceFieldProbeArrayStrategy(final String className, final long classId,
+			final int probeCount,
+			final IExecutionDataAccessorGenerator accessorGenerator) {
+		this.className = className;
+		this.classId = classId;
+		this.probeCount = probeCount;
+		this.accessorGenerator = accessorGenerator;
+	}
+
+	public int storeInstance(final MethodVisitor mv, final boolean clinit,
+			final int variable) {
+		if (clinit) {
+			final int maxStack = accessorGenerator.generateDataAccessor(classId,
+					className, probeCount, mv);
+
+			// Stack[0]: [Z
+
+			mv.visitInsn(Opcodes.DUP);
+
+			// Stack[1]: [Z
+			// Stack[0]: [Z
+
+			mv.visitFieldInsn(Opcodes.PUTSTATIC, className,
+					InstrSupport.DATAFIELD_NAME, InstrSupport.DATAFIELD_DESC);
+
+			// Stack[0]: [Z
+
+			mv.visitVarInsn(Opcodes.ASTORE, variable);
+
+			seenClinit = true;
+			return Math.max(maxStack, 2);
+		} else {
+			mv.visitMethodInsn(Opcodes.INVOKESTATIC, className,
+					InstrSupport.INITMETHOD_NAME, InstrSupport.INITMETHOD_DESC,
+					true);
+			mv.visitVarInsn(Opcodes.ASTORE, variable);
+			return 1;
+		}
+	}
+
+	public void addMembers(final ClassVisitor cv, final int probeCount) {
+		createDataField(cv);
+		createInitMethod(cv, probeCount);
+		if (!seenClinit) {
+			createClinitMethod(cv, probeCount);
+		}
+	}
+
+	private void createDataField(final ClassVisitor cv) {
+		cv.visitField(InstrSupport.DATAFIELD_INTF_ACC,
+				InstrSupport.DATAFIELD_NAME, InstrSupport.DATAFIELD_DESC, null,
+				null);
+	}
+
+	private void createInitMethod(final ClassVisitor cv, final int probeCount) {
+		final MethodVisitor mv = cv.visitMethod(InstrSupport.INITMETHOD_ACC,
+				InstrSupport.INITMETHOD_NAME, InstrSupport.INITMETHOD_DESC,
+				null, null);
+		mv.visitCode();
+
+		// Load the value of the static data field:
+		mv.visitFieldInsn(Opcodes.GETSTATIC, className,
+				InstrSupport.DATAFIELD_NAME, InstrSupport.DATAFIELD_DESC);
+		mv.visitInsn(Opcodes.DUP);
+
+		// Stack[1]: [Z
+		// Stack[0]: [Z
+
+		// Skip initialization when we already have a data array:
+		final Label alreadyInitialized = new Label();
+		mv.visitJumpInsn(Opcodes.IFNONNULL, alreadyInitialized);
+
+		// Stack[0]: [Z
+
+		mv.visitInsn(Opcodes.POP);
+		final int size = accessorGenerator.generateDataAccessor(classId,
+				className, probeCount, mv);
+
+		// Stack[0]: [Z
+
+		// Return the class' probe array:
+		mv.visitFrame(Opcodes.F_NEW, 0, FRAME_LOCALS_EMPTY, 1,
+				FRAME_STACK_ARRZ);
+		mv.visitLabel(alreadyInitialized);
+		mv.visitInsn(Opcodes.ARETURN);
+
+		mv.visitMaxs(Math.max(size, 2), 0); // Maximum local stack size is 2
+		mv.visitEnd();
+	}
+
+	private void createClinitMethod(final ClassVisitor cv,
+			final int probeCount) {
+		final MethodVisitor mv = cv.visitMethod(InstrSupport.CLINIT_ACC,
+				InstrSupport.CLINIT_NAME, InstrSupport.CLINIT_DESC, null, null);
+		mv.visitCode();
+
+		final int maxStack = accessorGenerator.generateDataAccessor(classId,
+				className, probeCount, mv);
+
+		// Stack[0]: [Z
+
+		mv.visitFieldInsn(Opcodes.PUTSTATIC, className,
+				InstrSupport.DATAFIELD_NAME, InstrSupport.DATAFIELD_DESC);
+
+		mv.visitInsn(Opcodes.RETURN);
+
+		mv.visitMaxs(maxStack, 0);
+		mv.visitEnd();
+	}
+
+}
diff --git a/org.jacoco.core/src/org/jacoco/core/internal/instr/LocalProbeArrayStrategy.java b/org.jacoco.core/src/org/jacoco/core/internal/instr/LocalProbeArrayStrategy.java
index f13dddfe..ff0b884e 100644
--- a/org.jacoco.core/src/org/jacoco/core/internal/instr/LocalProbeArrayStrategy.java
+++ b/org.jacoco.core/src/org/jacoco/core/internal/instr/LocalProbeArrayStrategy.java
@@ -38,7 +38,8 @@ class LocalProbeArrayStrategy implements IProbeArrayStrategy {
 		this.accessorGenerator = accessorGenerator;
 	}
 
-	public int storeInstance(final MethodVisitor mv, final int variable) {
+	public int storeInstance(final MethodVisitor mv, final boolean clinit,
+			final int variable) {
 		final int maxStack = accessorGenerator.generateDataAccessor(classId,
 				className, probeCount, mv);
 		mv.visitVarInsn(Opcodes.ASTORE, variable);
diff --git a/org.jacoco.core/src/org/jacoco/core/internal/instr/NoneProbeArrayStrategy.java b/org.jacoco.core/src/org/jacoco/core/internal/instr/NoneProbeArrayStrategy.java
index 6b29518f..0c4d572b 100644
--- a/org.jacoco.core/src/org/jacoco/core/internal/instr/NoneProbeArrayStrategy.java
+++ b/org.jacoco.core/src/org/jacoco/core/internal/instr/NoneProbeArrayStrategy.java
@@ -20,7 +20,8 @@ import org.objectweb.asm.MethodVisitor;
  */
 class NoneProbeArrayStrategy implements IProbeArrayStrategy {
 
-	public int storeInstance(final MethodVisitor mv, final int variable) {
+	public int storeInstance(final MethodVisitor mv, final boolean clinit,
+			final int variable) {
 		throw new UnsupportedOperationException();
 	}
 
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 8ce2f238..bd185fe9 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
@@ -28,7 +28,8 @@ public final class ProbeArrayStrategyFactory {
 
 	/**
 	 * Creates a suitable strategy instance for the class described by the given
-	 * reader.
+	 * reader. Created instance must be used only to process a class or
+	 * interface for which it has been created and must be used only once.
 	 * 
 	 * @param reader
 	 *            reader to get information about the class
@@ -50,16 +51,15 @@ public final class ProbeArrayStrategyFactory {
 				return new NoneProbeArrayStrategy();
 			}
 			if (version >= Opcodes.V1_8 && counter.hasMethods()) {
-				return new FieldProbeArrayStrategy(className, classId,
-						withFrames, true, InstrSupport.DATAFIELD_INTF_ACC,
-						accessorGenerator);
+				return new InterfaceFieldProbeArrayStrategy(className, classId,
+						counter.getCount(), accessorGenerator);
 			} else {
 				return new LocalProbeArrayStrategy(className, classId,
 						counter.getCount(), accessorGenerator);
 			}
 		} else {
-			return new FieldProbeArrayStrategy(className, classId, withFrames,
-					false, InstrSupport.DATAFIELD_ACC, accessorGenerator);
+			return new ClassFieldProbeArrayStrategy(className, classId,
+					withFrames, accessorGenerator);
 		}
 	}
 
diff --git a/org.jacoco.core/src/org/jacoco/core/internal/instr/ProbeCounter.java b/org.jacoco.core/src/org/jacoco/core/internal/instr/ProbeCounter.java
index 0028fcda..d45a8f9d 100644
--- a/org.jacoco.core/src/org/jacoco/core/internal/instr/ProbeCounter.java
+++ b/org.jacoco.core/src/org/jacoco/core/internal/instr/ProbeCounter.java
@@ -30,7 +30,7 @@ class ProbeCounter extends ClassProbesVisitor {
 	@Override
 	public MethodProbesVisitor visitMethod(final int access, final String name,
 			final String desc, final String signature, final String[] exceptions) {
-		if (!"<clinit>".equals(name)) {
+		if (!InstrSupport.CLINIT_NAME.equals(name)) {
 			methods = true;
 		}
 		return null;
diff --git a/org.jacoco.core/src/org/jacoco/core/internal/instr/ProbeInserter.java b/org.jacoco.core/src/org/jacoco/core/internal/instr/ProbeInserter.java
index 6cf84609..5e0084b9 100644
--- a/org.jacoco.core/src/org/jacoco/core/internal/instr/ProbeInserter.java
+++ b/org.jacoco.core/src/org/jacoco/core/internal/instr/ProbeInserter.java
@@ -27,6 +27,12 @@ class ProbeInserter extends MethodVisitor implements IProbeInserter {
 
 	private final IProbeArrayStrategy arrayStrategy;
 
+	/**
+	 * <code>true</code> if method is a class or interface initialization
+	 * method.
+	 */
+	private final boolean clinit;
+
 	/** Position of the inserted variable. */
 	private final int variable;
 
@@ -37,7 +43,9 @@ class ProbeInserter extends MethodVisitor implements IProbeInserter {
 	 * Creates a new {@link ProbeInserter}.
 	 * 
 	 * @param access
-	 *            access flags of the adapted method.
+	 *            access flags of the adapted method
+	 * @param name
+	 *            the method's name
 	 * @param desc
 	 *            the method's descriptor
 	 * @param mv
@@ -46,9 +54,10 @@ class ProbeInserter extends MethodVisitor implements IProbeInserter {
 	 *            callback to create the code that retrieves the reference to
 	 *            the probe array
 	 */
-	ProbeInserter(final int access, final String desc, final MethodVisitor mv,
+	ProbeInserter(final int access, final String name, final String desc, final MethodVisitor mv,
 			final IProbeArrayStrategy arrayStrategy) {
 		super(JaCoCo.ASM_API_VERSION, mv);
+		this.clinit = InstrSupport.CLINIT_NAME.equals(name);
 		this.arrayStrategy = arrayStrategy;
 		int pos = (Opcodes.ACC_STATIC & access) == 0 ? 1 : 0;
 		for (final Type t : Type.getArgumentTypes(desc)) {
@@ -82,7 +91,7 @@ class ProbeInserter extends MethodVisitor implements IProbeInserter {
 
 	@Override
 	public void visitCode() {
-		accessorStackSize = arrayStrategy.storeInstance(mv, variable);
+		accessorStackSize = arrayStrategy.storeInstance(mv, clinit, variable);
 		mv.visitCode();
 	}