Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Upgrade to ASM 6 #600

Merged
merged 1 commit into from Oct 19, 2017
Merged

Upgrade to ASM 6 #600

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 0 additions & 6 deletions jacoco-maven-plugin.test/it/it-java9-offline-instrumentation/pom.xml
View file
Open in desktop
Expand Up @@ -50,12 +50,6 @@
<goals>
<goal>instrument</goal>
</goals>
<configuration>
<!-- FIXME remove this workaround after update of ASM to 6.0 -->
<excludes>
<exclude>module-info.class</exclude>
</excludes>
</configuration>
</execution>
<execution>
<id>restore-instrumented-classes</id>
Expand Down
26 changes: 23 additions & 3 deletions org.jacoco.build/pom.xml
View file
Open in desktop
Expand Up @@ -141,7 +141,7 @@
<argLine>${jvm.args}</argLine>

<!-- Dependencies versions -->
<asm.version>5.2</asm.version>
<asm.version>6.0</asm.version>
<ant.version>1.7.1</ant.version>
<args4j.version>2.0.28</args4j.version>
<junit.version>4.8.2</junit.version>
Expand Down Expand Up @@ -210,7 +210,27 @@
<!-- Third-party dependencies -->
<dependency>
<groupId>org.ow2.asm</groupId>
<artifactId>asm-debug-all</artifactId>
<artifactId>asm</artifactId>
<version>${asm.version}</version>
</dependency>
<dependency>
<groupId>org.ow2.asm</groupId>
<artifactId>asm-commons</artifactId>
<version>${asm.version}</version>
</dependency>
<dependency>
<groupId>org.ow2.asm</groupId>
<artifactId>asm-tree</artifactId>
<version>${asm.version}</version>
</dependency>
<dependency>
<groupId>org.ow2.asm</groupId>
<artifactId>asm-analysis</artifactId>
<version>${asm.version}</version>
</dependency>
<dependency>
<groupId>org.ow2.asm</groupId>
<artifactId>asm-util</artifactId>
<version>${asm.version}</version>
</dependency>
<dependency>
Expand Down Expand Up @@ -370,7 +390,7 @@
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-shade-plugin</artifactId>
<version>2.4.3</version>
<version>3.1.0</version>
</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
Expand Down
2 changes: 1 addition & 1 deletion org.jacoco.cli.test/src/org/jacoco/cli/internal/commands/InstrumentTest.java
View file
Open in desktop
Expand Up @@ -137,7 +137,7 @@ private void assertInstrumented(File classfile) throws IOException {
ClassReader reader = new ClassReader(in);
in.close();
final Set<String> fields = new HashSet<String>();
reader.accept(new ClassVisitor(Opcodes.ASM5) {
reader.accept(new ClassVisitor(Opcodes.ASM6) {
@Override
public FieldVisitor visitField(int access, String name, String desc,
String signature, Object value) {
Expand Down
5 changes: 2 additions & 3 deletions org.jacoco.core.test/src/org/jacoco/core/analysis/AnalyzerTest.java
View file
Open in desktop
Expand Up @@ -36,7 +36,6 @@
import java.util.zip.ZipOutputStream;

import org.jacoco.core.data.ExecutionDataStore;
import org.jacoco.core.internal.Java9Support;
import org.jacoco.core.internal.data.CRC64;
import org.jacoco.core.test.TargetLoader;
import org.junit.Before;
Expand Down Expand Up @@ -93,8 +92,8 @@ public void testAnalyzeClassFromByteArray() throws IOException {
@Test
public void testAnalyzeClassIdMatch() throws IOException {
// class IDs are always calculated after downgrade of the version
final byte[] bytes = Java9Support.downgradeIfRequired(
TargetLoader.getClassDataAsBytes(AnalyzerTest.class));
final byte[] bytes = TargetLoader
.getClassDataAsBytes(AnalyzerTest.class);
executionData.get(Long.valueOf(CRC64.checksum(bytes)),
"org/jacoco/core/analysis/AnalyzerTest", 200);
analyzer.analyzeClass(bytes, "Test");
Expand Down
3 changes: 1 addition & 2 deletions org.jacoco.core.test/src/org/jacoco/core/internal/flow/FrameSnapshotTest.java
View file
Open in desktop
Expand Up @@ -115,8 +115,7 @@ public void after_aaload_stack_should_contain_null_when_input_array_is_null() {
analyzer.visitInsn(Opcodes.AALOAD);
frame = FrameSnapshot.create(analyzer, 0);

// FIXME should be Opcodes.NULL after update of ASM to 6.0
final Object[] stack = arr("java/lang/Object");
final Object[] stack = arr(Opcodes.NULL);
expectedVisitor.visitFrame(Opcodes.F_FULL, 1, arr("Foo"), 1, stack);
}

Expand Down
4 changes: 2 additions & 2 deletions org.jacoco.core.test/src/org/jacoco/core/internal/instr/ProbeArrayStrategyFactoryTest.java
View file
Open in desktop
Expand Up @@ -283,7 +283,7 @@ private static class ClassVisitorMock extends ClassVisitor {
private final List<AddedMethod> methods = new ArrayList<AddedMethod>();

ClassVisitorMock() {
super(Opcodes.ASM5);
super(Opcodes.ASM6);
}

@Override
Expand All @@ -300,7 +300,7 @@ public MethodVisitor visitMethod(int access, String name, String desc,
String signature, String[] exceptions) {
final AddedMethod m = new AddedMethod(access, name, desc);
methods.add(m);
return new MethodVisitor(Opcodes.ASM5) {
return new MethodVisitor(Opcodes.ASM6) {
@Override
public void visitFrame(int type, int nLocal, Object[] local,
int nStack, Object[] stack) {
Expand Down
8 changes: 4 additions & 4 deletions org.jacoco.core.test/src/org/jacoco/core/test/validation/ClassFileVersionsTest.java
View file
Open in desktop
Expand Up @@ -25,11 +25,11 @@
import static org.objectweb.asm.Opcodes.V1_6;
import static org.objectweb.asm.Opcodes.V1_7;
import static org.objectweb.asm.Opcodes.V1_8;
import static org.objectweb.asm.Opcodes.V9;

import java.io.IOException;

import org.jacoco.core.instr.Instrumenter;
import org.jacoco.core.internal.Java9Support;
import org.jacoco.core.internal.instr.InstrSupport;
import org.jacoco.core.runtime.IRuntime;
import org.jacoco.core.runtime.SystemPropertiesRuntime;
Expand Down Expand Up @@ -86,7 +86,7 @@ public void test_1_8() throws IOException {

@Test
public void test_1_9() throws IOException {
testVersion(Java9Support.V1_9, true);
testVersion(V9, true);
}

private void testVersion(int version, boolean frames) throws IOException {
Expand All @@ -101,8 +101,8 @@ private void testVersion(int version, boolean frames) throws IOException {

private void assertFrames(byte[] source, boolean expected) {
final boolean[] hasFrames = new boolean[] { false };
new ClassReader(Java9Support.downgradeIfRequired(source)).accept(
new ClassVisitor(InstrSupport.ASM_API_VERSION) {
new ClassReader(source)
.accept(new ClassVisitor(InstrSupport.ASM_API_VERSION) {

@Override
public MethodVisitor visitMethod(int access, String name,
Expand Down
4 changes: 1 addition & 3 deletions org.jacoco.core.test/src/org/jacoco/core/test/validation/FramesTest.java
View file
Open in desktop
Expand Up @@ -18,7 +18,6 @@
import java.io.StringWriter;

import org.jacoco.core.instr.Instrumenter;
import org.jacoco.core.internal.Java9Support;
import org.jacoco.core.internal.instr.InstrSupport;
import org.jacoco.core.runtime.IRuntime;
import org.jacoco.core.runtime.SystemPropertiesRuntime;
Expand Down Expand Up @@ -88,8 +87,7 @@ private void testFrames(byte[] source) throws IOException {
}

private byte[] calculateFrames(byte[] source) {
ClassReader rc = new ClassReader(
Java9Support.downgradeIfRequired(source));
ClassReader rc = new ClassReader(source);
ClassWriter cw = new ClassWriter(ClassWriter.COMPUTE_FRAMES);

// Adjust Version to 1.6 to enable frames:
Expand Down
22 changes: 11 additions & 11 deletions org.jacoco.core.test/src/org/jacoco/core/test/validation/ResizeInstructionsTest.java
View file
Open in desktop
Expand Up @@ -11,12 +11,11 @@
*******************************************************************************/
package org.jacoco.core.test.validation;

import static org.junit.Assert.assertNotSame;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertSame;
import static org.junit.Assert.assertTrue;

import org.jacoco.core.instr.Instrumenter;
import org.jacoco.core.internal.Java9Support;
import org.jacoco.core.internal.instr.InstrSupport;
import org.jacoco.core.runtime.IRuntime;
import org.jacoco.core.runtime.RuntimeData;
Expand Down Expand Up @@ -58,9 +57,11 @@ private class Inner {
*/
@Test
public void should_not_loose_InnerClasses_attribute() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After upgrade this test fails on JDK 5 due to another ASM bug - https://gitlab.ow2.org/asm/asm/issues/317800 😆

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However ASM 5.2 already had an issues in a similar scenario - see #487

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that scenarios are limited to class files without frames and with big methods that require wide jumps, and that there was problem #487 in the exact same scenarios, I do not consider this as regression.

In these scenarios after update of ASM, we'll be producing class files with frames that are not supposed to be present. While this can cause problems of reading of produced classes by the ASM as described in https://gitlab.ow2.org/asm/asm/issues/317800 , seems that JVM can read such classes and I believe that doesn't perform validation of frames, so this should not cause execution problems.

And taking into account that this update will resolve #487, #544, #584 and #606, I propose to update test to make it pass, and merge update into master.

Also seems that we can implement removal of these unwanted frames after instrumentation if there were no frames in original class - see my comment in ASM issue (https://gitlab.ow2.org/asm/asm/issues/317800#note_9609). However I propose to defer this and merge update without this - can implement later and maybe soon there will be ASM 6.0.1 with fix (https://gitlab.ow2.org/asm/asm/merge_requests/46).

@marchof WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Godin Let's move forward to 6.0 without the workaround -- a you propose. I assume that 6.0.1 will be there before we get the first bug report for old class files.

final ClassWriter cw = new ClassWriter(0);
final ClassReader cr = new ClassReader(Java9Support.downgradeIfRequired(
TargetLoader.getClassDataAsBytes(Inner.class)));
// FIXME fails without COMPUTE_FRAMES because of
// https://gitlab.ow2.org/asm/asm/issues/317800
final ClassWriter cw = new ClassWriter(ClassWriter.COMPUTE_FRAMES);
final ClassReader cr = new ClassReader(
TargetLoader.getClassDataAsBytes(Inner.class));
cr.accept(new ClassVisitor(InstrSupport.ASM_API_VERSION, cw) {
@Override
public void visitEnd() {
Expand All @@ -79,11 +80,10 @@ public void visitEnd() {
final Class<?> outer = targetLoader.add(ResizeInstructionsTest.class,
TargetLoader.getClassDataAsBytes(ResizeInstructionsTest.class));
final Class<?> inner = targetLoader.add(Inner.class, bytes);
// FIXME should not be null after update of ASM to 6.0
assertNotSame(outer, inner.getEnclosingClass());
assertNull(inner.getEnclosingClass());
assertNotSame(outer, inner.getDeclaringClass());
assertNull(inner.getDeclaringClass());
assertSame(outer, inner.getEnclosingClass());
assertNotNull(inner.getEnclosingClass());
assertSame(outer, inner.getDeclaringClass());
assertNotNull(inner.getDeclaringClass());
}

/**
Expand Down
4 changes: 1 addition & 3 deletions org.jacoco.core.test/src/org/jacoco/core/test/validation/StructuredLockingTest.java
View file
Open in desktop
Expand Up @@ -20,7 +20,6 @@
import java.util.Set;

import org.jacoco.core.instr.Instrumenter;
import org.jacoco.core.internal.Java9Support;
import org.jacoco.core.runtime.IRuntime;
import org.jacoco.core.runtime.SystemPropertiesRuntime;
import org.jacoco.core.test.TargetLoader;
Expand Down Expand Up @@ -64,8 +63,7 @@ private void assertStructuredLocking(byte[] source) throws Exception {
byte[] instrumented = instrumenter.instrument(source, "TestTarget");

ClassNode cn = new ClassNode();
new ClassReader(Java9Support.downgradeIfRequired(instrumented))
.accept(cn, 0);
new ClassReader(instrumented).accept(cn, 0);
for (MethodNode mn : cn.methods) {
assertStructuredLocking(cn.name, mn);
}
Expand Down
18 changes: 17 additions & 1 deletion org.jacoco.core/pom.xml
View file
Open in desktop
Expand Up @@ -27,7 +27,23 @@
<dependencies>
<dependency>
<groupId>org.ow2.asm</groupId>
<artifactId>asm-debug-all</artifactId>
<artifactId>asm</artifactId>
</dependency>
<dependency>
<groupId>org.ow2.asm</groupId>
<artifactId>asm-commons</artifactId>
</dependency>
<dependency>
<groupId>org.ow2.asm</groupId>
<artifactId>asm-tree</artifactId>
</dependency>
<dependency>
<groupId>org.ow2.asm</groupId>
<artifactId>asm-analysis</artifactId>
</dependency>
<dependency>
<groupId>org.ow2.asm</groupId>
<artifactId>asm-util</artifactId>
</dependency>
</dependencies>

Expand Down
7 changes: 3 additions & 4 deletions org.jacoco.core/src/org/jacoco/core/analysis/Analyzer.java
View file
Open in desktop
Expand Up @@ -23,7 +23,7 @@
import org.jacoco.core.data.ExecutionData;
import org.jacoco.core.data.ExecutionDataStore;
import org.jacoco.core.internal.ContentTypeDetector;
import org.jacoco.core.internal.Java9Support;
import org.jacoco.core.internal.InputStreams;
import org.jacoco.core.internal.Pack200Streams;
import org.jacoco.core.internal.analysis.ClassAnalyzer;
import org.jacoco.core.internal.analysis.ClassCoverageImpl;
Expand Down Expand Up @@ -124,8 +124,7 @@ public void analyzeClass(final ClassReader reader) {
public void analyzeClass(final byte[] buffer, final String location)
throws IOException {
try {
analyzeClass(
new ClassReader(Java9Support.downgradeIfRequired(buffer)));
analyzeClass(new ClassReader(buffer));
} catch (final RuntimeException cause) {
throw analyzerError(location, cause);
}
Expand All @@ -146,7 +145,7 @@ public void analyzeClass(final InputStream input, final String location)
throws IOException {
final byte[] buffer;
try {
buffer = Java9Support.readFully(input);
buffer = InputStreams.readFully(input);
} catch (final IOException e) {
throw analyzerError(location, e);
}
Expand Down
13 changes: 3 additions & 10 deletions org.jacoco.core/src/org/jacoco/core/instr/Instrumenter.java
View file
Open in desktop
Expand Up @@ -22,7 +22,7 @@
import java.util.zip.ZipOutputStream;

import org.jacoco.core.internal.ContentTypeDetector;
import org.jacoco.core.internal.Java9Support;
import org.jacoco.core.internal.InputStreams;
import org.jacoco.core.internal.Pack200Streams;
import org.jacoco.core.internal.flow.ClassProbesAdapter;
import org.jacoco.core.internal.instr.ClassInstrumenter;
Expand Down Expand Up @@ -105,14 +105,7 @@ protected String getCommonSuperClass(final String type1,
public byte[] instrument(final byte[] buffer, final String name)
throws IOException {
try {
if (Java9Support.isPatchRequired(buffer)) {
final byte[] result = instrument(
new ClassReader(Java9Support.downgrade(buffer)));
Java9Support.upgrade(result);
return result;
} else {
return instrument(new ClassReader(buffer));
}
return instrument(new ClassReader(buffer));
} catch (final RuntimeException e) {
throw instrumentError(name, e);
}
Expand All @@ -135,7 +128,7 @@ public byte[] instrument(final InputStream input, final String name)
throws IOException {
final byte[] bytes;
try {
bytes = Java9Support.readFully(input);
bytes = InputStreams.readFully(input);
} catch (final IOException e) {
throw instrumentError(name, e);
}
Expand Down
2 changes: 1 addition & 1 deletion org.jacoco.core/src/org/jacoco/core/internal/ContentTypeDetector.java
View file
Open in desktop
Expand Up @@ -82,7 +82,7 @@ private static int determineType(final InputStream in) throws IOException {
case Opcodes.V1_6:
case Opcodes.V1_7:
case Opcodes.V1_8:
case Java9Support.V1_9:
case Opcodes.V9:
return CLASSFILE;
}
}
Expand Down
49 changes: 49 additions & 0 deletions org.jacoco.core/src/org/jacoco/core/internal/InputStreams.java
View file
Open in desktop
@@ -0,0 +1,49 @@
/*******************************************************************************
* Copyright (c) 2009, 2017 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;

import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.io.InputStream;

/**
* Utilities for {@link InputStream}s.
*/
public final class InputStreams {

private InputStreams() {
}

/**
* Reads all bytes from an input stream into a byte array. The provided
* {@link InputStream} is not closed by this method.
*
* @param is
* the input stream to read from
* @return a byte array containing all the bytes from the stream
* @throws IOException
* if an I/O error occurs
*/
public static byte[] readFully(final InputStream is) throws IOException {
final byte[] buf = new byte[1024];
final ByteArrayOutputStream out = new ByteArrayOutputStream();
while (true) {
final int r = is.read(buf);
if (r == -1) {
break;
}
out.write(buf, 0, r);
}
return out.toByteArray();
}

}