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

Support Java 9 class file format #406

Merged
merged 1 commit into from May 20, 2016

Conversation

3 participants
@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof May 13, 2016

Member

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 :-/

Member

marchof commented May 13, 2016

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 :-/

@marchof marchof modified the milestone: Java 9 May 13, 2016

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin May 13, 2016

Member

@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.

Member

Godin commented May 13, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin May 13, 2016

Member

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

Member

Godin commented May 13, 2016

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

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof May 14, 2016

Member

What are the options?

  1. Use a custom/patched version of ASM
  2. Patch the class files by flipping the version tag down to 52.0 (and back after instrumentation)
  3. 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.

Member

marchof commented May 14, 2016

What are the options?

  1. Use a custom/patched version of ASM
  2. Patch the class files by flipping the version tag down to 52.0 (and back after instrumentation)
  3. 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

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin May 14, 2016

Member

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

Member

Godin commented May 14, 2016

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

@Godin Godin self-assigned this May 14, 2016

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin May 14, 2016

Member

@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.

Member

Godin commented May 14, 2016

@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.

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

This comment has been minimized.

@marchof

marchof May 16, 2016

Member

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

@marchof

marchof May 16, 2016

Member

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

This comment has been minimized.

@Godin

Godin May 16, 2016

Member

@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?

@Godin

Godin May 16, 2016

Member

@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?

This comment has been minimized.

@marchof

marchof May 16, 2016

Member

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

marchof May 16, 2016

Member

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

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof May 16, 2016

Member

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

Member

marchof commented May 16, 2016

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

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof May 16, 2016

Member

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

Member

marchof commented May 16, 2016

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

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin May 16, 2016

Member

@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?

Member

Godin commented May 16, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof May 16, 2016

Member

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

Member

marchof commented May 16, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@bjkail

bjkail May 16, 2016

Contributor

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).

Contributor

bjkail commented May 16, 2016

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

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin May 16, 2016

Member

@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, "");
}

?

Member

Godin commented May 16, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@bjkail

bjkail May 17, 2016

Contributor

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.

Contributor

bjkail commented May 17, 2016

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

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof May 17, 2016

Member

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

Member

marchof commented May 17, 2016

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

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin May 20, 2016

Member

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".

Member

Godin commented May 20, 2016

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".

@Godin Godin merged commit 3cf1d3c into master May 20, 2016

4 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

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

@Godin Godin deleted the issue-406 branch May 20, 2016

@jacoco jacoco locked and limited conversation to collaborators Jan 11, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.