Support Java 9 class file format #406

Godin · 2016-05-14T18:56:08Z

https://bugs.openjdk.java.net/browse/JDK-8150011

marchof · 2016-05-13T13:04:10Z

Although I don't think the format has changed we need a version of ASM which accepts the new header.

The last time ASM was released around the same time when Java 8 was released. The reason is that OpenJDK releases the final spec at the same time as the JDK :-/

Godin · 2016-05-13T15:51:23Z

@marchof according to my observations the list of changes in https://bugs.openjdk.java.net/browse/JDK-8148785 is exhaustive, and so indeed for ASM the only change is to accept newer version as in https://bugs.openjdk.java.net/browse/JDK-8151858

However I'm wondering how we can keep on testing classes compiled with JDK 9 EA when javac from one of the next builds will start producing newer version.

Godin · 2016-05-13T18:06:34Z

This change is not in jdk-9+118 , but I guess will be in jdk-9+119.

marchof · 2016-05-14T07:40:40Z

What are the options?

Use a custom/patched version of ASM
Patch the class files by flipping the version tag down to 52.0 (and back after instrumentation)
Talk to ASM guys whether they want to publish a updated version

Also we need to add a new entry in ContentTypeDetector.determineType(). If you agree on solution 2 I can prepare a patch for this. What I would do is to create a internal Java9Support class which encapsulates the temporary hacks until we get a new ASM version.

Also in any case we should try option 3 in parallel.

Godin · 2016-05-14T09:08:09Z

@marchof actually I already have patch for option 2, but need to test it on updated javac.

Godin · 2016-05-14T21:12:31Z

@marchof could you please review? Note that this is not sufficient to be able to build JaCoCo with compilation into new class files, because maven-shade-plugin also requires update. However this allows JaCoCo to work properly with new class files - I added integration test (instrumentation with agent + report generation on new class files) and tested with local dev build of latest OpenJDK 9.

marchof · 2016-05-16T07:04:41Z

org.jacoco.core/src/org/jacoco/core/internal/Java9Support.java

+/**
+ * Patching for Java 9 classes, so that ASM can read them.
+ */
+public final class Java9Support {


I see several JavaDoc warnings on this class. Even if the code is temporary I would prefer if our code comes without warnings.

@marchof I suppose that those warnings are generated by Eclipse? I don't see warnings in IntelliJ , even when explicitly executing all built-in inspections for Javadocs. So could you please elaborate what needs to be fixed?

Basically we require to have JavaDoc on all public Java classes and members.

Javadoc: Missing comment for public declaration Java9Support.java /org.jacoco.core/src/org/jacoco/core/internal line 25 Java Problem

Javadoc: The method readClass(InputStream, boolean) from the type ClassReader is not visible Java9Support.java /org.jacoco.core/src/org/jacoco/core/internal line 31 Java Problem

Javadoc: Missing tag for return type Java9Support.java /org.jacoco.core/src/org/jacoco/core/internal line 33 Java Problem

Javadoc: Missing tag for parameter is Java9Support.java /org.jacoco.core/src/org/jacoco/core/internal line 33 Java Problem

Javadoc: Missing tag for parameter close Java9Support.java /org.jacoco.core/src/org/jacoco/core/internal line 33 Java Problem

Javadoc: Missing tag for declared exception IOException Java9Support.java /org.jacoco.core/src/org/jacoco/core/internal line 34 Java Problem

Javadoc: Missing comment for public declaration Java9Support.java /org.jacoco.core/src/org/jacoco/core/internal line 79 Java Problem

Javadoc: Missing comment for public declaration Java9Support.java /org.jacoco.core/src/org/jacoco/core/internal line 83 Java Problem

Javadoc: Missing comment for public declaration Java9Support.java /org.jacoco.core/src/org/jacoco/core/internal line 87 Java Problem

Javadoc: Missing comment for public declaration Java9Support.java /org.jacoco.core/src/org/jacoco/core/internal line 94 Java Problem

marchof · 2016-05-16T07:24:44Z

I wonder whether we should drop Analyzer.analyzeClass(ClassReader) from the public API? This is a asymmetry to Instrumenter anyways.

marchof · 2016-05-16T07:31:32Z

@Godin Beside my comments I'm ok with this PR!

Godin · 2016-05-16T09:31:08Z

@marchof I simplified code and added Javadocs.

About asymmetry: indeed it bothers. This is the only place where we expose ClassReader, but not the only place where we expose ASM APIs. Removal - is an API change. I don't think that it is critical for experimental support of Java 9 class file format. So IMO we should consider separately question about marking it as deprecated and further removal. WDYT?

marchof · 2016-05-16T09:34:41Z

@Godin I agree to remove this API in a separate request. I just came across this as it won't work for Java 9.

bjkail · 2016-05-16T17:38:04Z

FYI (sorry if this is the wrong forum for this comment), we override analyzeClass(ClassReader) in order to filter which classes should be analyzed: for historical reasons, module boundaries in our product are not cleanly separated by archive, so we use package filters here. It would be ideal if there were still some way for us to do that efficiently before you remove that method (we can also skip the ICoverageVisitor.visitCoverage event, but that's less efficient since Analyzer's ASM processing still occurs).

Godin · 2016-05-16T18:02:52Z

@bjkail No need to apologize. In fact thank you for this valuable information.

As far as I understood you're doing something like

Analyzer analyzer = new Analyzer(...);
ClassReader reader = new ClassReader(...);
if (notFiltered(reader)) {
  analyzer.analyzeClass(reader);
}

if so, wondering whether you can replace it by

Analyzer analyzer = new Analyzer(...);
ClassReader reader = new ClassReader(...);
if (notFiltered(reader)) {
  analyzer.analyzeClass(reader.b, "");
}

?

bjkail · 2016-05-17T02:05:03Z

Well, more like this:

Analyzer analyzer = new Analyzer(...) {
  @Override
  public void analyzeClass(ClassReader r) {
    if (notFiltered(r.getClassName())) {
      super.analyzeClass(r);
    }
  }
};

// Use Analyzer's existing recursive archive processing logic.
analyzer.analyzeAll(...);

Regardless, we could instead override analyzeClass(InputStream, String) to do as you suggest at the cost of a duplicate ClassReader constructor call, but I suppose that's relatively lightweight when compared to overriding visitExecution. Well, the Analyzer javadoc doesn't actually guarantee that analyzeAll will invoke the analyzeClass methods, but it is nice to be able to reuse Analyzer's recursive archive processing, so it would be nice if the javadoc did make that guarantee.

marchof · 2016-05-17T06:23:45Z

For the filtering API story we will need proper call-backs anyways.

Godin · 2016-05-20T20:42:52Z

JDK 9 EA build 119 is available for testing, so let's go ahead with merging of those changes. And let's handle raised points separately from this PR, if it is needed or would be needed.

And for the record without those changes execution of JaCoCo with JDK 9 EA b 119 fails with message like

Exception in thread "main" java.lang.reflect.InvocationTargetException
     at jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(java.base@9-ea/Native Method)
     at jdk.internal.reflect.NativeMethodAccessorImpl.invoke(java.base@9-ea/NativeMethodAccessorImpl.java:62)
     at jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(java.base@9-ea/DelegatingMethodAccessorImpl.java:43)
     at java.lang.reflect.Method.invoke(java.base@9-ea/Method.java:531)
     at sun.instrument.InstrumentationImpl.loadClassAndStartAgent(java.instrument@9-ea/InstrumentationImpl.java:396)
     at sun.instrument.InstrumentationImpl.loadClassAndCallPremain(java.instrument@9-ea/InstrumentationImpl.java:408)
Caused by: java.lang.RuntimeException: Class java/util/UUID could not be instrumented.
     at org.jacoco.agent.rt.internal_9472e99.core.runtime.ModifiedSystemClassRuntime.createFor(ModifiedSystemClassRuntime.java:139)
     at org.jacoco.agent.rt.internal_9472e99.core.runtime.ModifiedSystemClassRuntime.createFor(ModifiedSystemClassRuntime.java:100)
     at org.jacoco.agent.rt.internal_9472e99.PreMain.createRuntime(PreMain.java:55)
     at org.jacoco.agent.rt.internal_9472e99.PreMain.premain(PreMain.java:47)
     ... 6 more
Caused by: java.lang.NoSuchFieldException: $jacocoAccess
     at java.lang.Class.getField(java.base@9-ea/Class.java:1881)
     at org.jacoco.agent.rt.internal_9472e99.core.runtime.ModifiedSystemClassRuntime.createFor(ModifiedSystemClassRuntime.java:137)
     ... 9 more

which could be improved - something like "cannot read class".

marchof modified the milestone: Java 9 May 13, 2016

Godin added component: core type: enhancement labels May 13, 2016

Godin self-assigned this May 14, 2016

Godin force-pushed the issue-406 branch 2 times, most recently from 0c24111 to c3697d8 Compare May 14, 2016 19:18

marchof reviewed May 16, 2016
View reviewed changes

Godin force-pushed the issue-406 branch from c3697d8 to f52e818 Compare May 16, 2016 09:19

Godin force-pushed the issue-406 branch from f52e818 to b4dfd8f Compare May 20, 2016 19:12

Add experimental support for Java 9 class files

b4dfd8f

Godin merged commit 3cf1d3c into master May 20, 2016

Godin modified the milestones: 0.7.7, Java 9 May 20, 2016

Godin deleted the issue-406 branch May 20, 2016 21:38

jacoco locked and limited conversation to collaborators Jan 11, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Java 9 class file format #406

Support Java 9 class file format #406

Godin commented May 14, 2016

marchof commented May 13, 2016

Godin commented May 13, 2016 •

edited

Godin commented May 13, 2016

marchof commented May 14, 2016 •

edited by Godin

Godin commented May 14, 2016

Godin commented May 14, 2016

marchof May 16, 2016

Godin May 16, 2016

marchof May 16, 2016

marchof commented May 16, 2016

marchof commented May 16, 2016

Godin commented May 16, 2016

marchof commented May 16, 2016

bjkail commented May 16, 2016

Godin commented May 16, 2016 •

edited

bjkail commented May 17, 2016

marchof commented May 17, 2016

Godin commented May 20, 2016

Support Java 9 class file format #406

Support Java 9 class file format #406

Conversation

Godin commented May 14, 2016

marchof commented May 13, 2016

Godin commented May 13, 2016 • edited

Godin commented May 13, 2016

marchof commented May 14, 2016 • edited by Godin

Godin commented May 14, 2016

Godin commented May 14, 2016

marchof May 16, 2016

Choose a reason for hiding this comment

Godin May 16, 2016

Choose a reason for hiding this comment

marchof May 16, 2016

Choose a reason for hiding this comment

marchof commented May 16, 2016

marchof commented May 16, 2016

Godin commented May 16, 2016

marchof commented May 16, 2016

bjkail commented May 16, 2016

Godin commented May 16, 2016 • edited

bjkail commented May 17, 2016

marchof commented May 17, 2016

Godin commented May 20, 2016

Godin commented May 13, 2016 •

edited

marchof commented May 14, 2016 •

edited by Godin

Godin commented May 16, 2016 •

edited