Fix instrumentation to not violate Java Virtual Machine Specification regarding initialization of final fields #434

Godin · 2016-07-26T19:43:27Z

Currently FieldProbeArrayStrategy creates static final field $jacocoData, value for which is set by bytecode instruction putstatic in method $jacocoInit, but according to JVMS:

if the field is final the instruction must occur in the < clinit > method of the current class. Otherwise, an IllegalAccessError is thrown.

Starting with OpenJDK 9 EA b127 this condition is checked for class files with version >=53 - http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/c558d46c1af2#l11.76 Unfortunately we compile classes into 52 even with JDK 9, because maven-shade-plugin can't handle 53, and that's why we don't get failure in core validation tests, but later in integration tests for Ant on one of JDK classes:

Exception in thread "main" java.lang.IllegalAccessError: Update to static final field java.sql.Timestamp.$jacocoData attempted from a different method (java.sql.Timestamp) than the initializer method <clinit> 
    at java.sql.Timestamp.$jacocoInit(java.sql@9-ea/Timestamp.java)
    at java.sql.Timestamp.<clinit>(java.sql@9-ea/Timestamp.java)
    at org.jacoco.ant.TestTarget.main(TestTarget.java:36)

There is also a report in our mailing list - https://groups.google.com/d/msg/jacoco/jHw39ZUjUNY/L-gHeMeBBQAJ

See https://bugs.openjdk.java.net/browse/JDK-8157181 which is associated with this check and which also states:

Methods that do not satisfy condition violate the assumptions of the compilers. Compiling such methods results in different behavior of compiled and interpreted code.

and which discusses removal of dependency on class file version.

Also might be that this check will be backported - see linked tickets, e.g. https://bugs.openjdk.java.net/browse/JDK-8161989

So that we should try to find a way to compile classes (or at least test targets) into bytecode version 53 ( #411 ).

In any case - field must not be final. But unfortunately this will make FieldProbeArrayStrategy unusable for interfaces with methods, because JVMS states that

Fields of interfaces must have their ACC_PUBLIC, ACC_STATIC, and ACC_FINAL flags set

bjkail · 2016-07-28T16:58:15Z

For interfaces, can we do similar to FieldProbeArrayStrategy except generate clinit (ala your commit 8ebd1d1) but just don't PUTSTATIC in $jacocoInit? i.e., if a static method is somehow called before the PUTSTATIC in clinit (somehow? in case something like Marc's BadCycles is possible?), then we fetch using the slow runtime indirection.

Godin · 2016-07-28T17:18:02Z

@bjkail that's exactly what I was prototyping here = clinit + slow path, and there is no more $jacocoInit for interfaces, unless I overlooked something (we definitely should compile test classes into bytecode version 53). btw, seems that I managed to create an example similar to BadCycles, but with interfaces.

Godin · 2016-07-29T10:44:58Z

@bjkail I was wrong about the example.

Godin · 2016-07-29T12:55:23Z

@marchof while this request is not yet ready for code review and merge, because requires some cleanup and polishing of code, could you please have a look on idea in general to be sure that we are on a good track here and something wasn't overlooked by me? For example - maybe you know an example of "bad cycles" with interfaces?

I personally don't see other alternatives, not counting idea of "companion classes", which is anyway not ready for prime time, while this one looks quite good and really close to finalization.

Godin · 2016-08-03T10:12:36Z

@marchof IMO now this PR completely ready to be merged, so could you please review?

bjkail · 2016-08-04T18:28:48Z

FWIW, the core changes look good to me.

Godin · 2016-08-04T18:42:21Z

@bjkail if my understanding of "FWIW" is correct, then - be sure your opinion matters! 😉 @marchof is probably just a bit busy being at JCrete 😆

marchof · 2016-08-12T11:54:19Z

Tried to create a cyclic initialization example for interfaces. Results in a JVM crash: https://gist.github.com/marchof/c0ac04a2abb4b163bf410e8018a72cb7

Will report to OpenJDK.

bjkail · 2016-08-12T13:54:50Z

@Godin I assume that once OpenJDK fixes the bug that marchof found that the behavior for cyclic initialization of interfaces will have the same behavior as classes, so I guess it's best to add the slow path logic.

Godin · 2016-08-12T14:40:06Z

@marchof you can also mention that this crashes on the latest JDK 9 EA b131.

@bjkail probably yes.

marchof · 2016-08-12T15:33:59Z

@Godin Unfortunatelly OpenJDK-Bugs are "write once". Internal ID is JI-9042792.

Godin · 2016-08-12T15:36:14Z

@marchof let me send an email ;)

Godin · 2016-08-14T02:02:47Z

"slow path" implemented, so @marchof could you please review?

bjkail · 2016-08-15T15:49:24Z

InstrumentingLoader uses 4-space indentation rather than tabs. Otherwise, looks good to me.

Godin · 2016-08-15T18:42:34Z

@bjkail reformatted InstrumentingLoader, thx.

marchof · 2016-08-15T19:34:25Z

@Godin Thanks a lot for figuring all this out and finally resolving it! And sorry for my silence. I just started working through the change set and will drop some comments at the corresponding places.

marchof · 2016-08-15T19:37:05Z

org.jacoco.core/src/org/jacoco/core/internal/instr/InstrSupport.java

+	 * 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"


">" missing here (the whole quote becomes rendered as a link)

@marchof fixed

marchof · 2016-08-15T20:54:47Z

@Godin Ok, review done from my side, none of my 8 comments is a blocker.

P.S: Can you please check whether you see 8 comments? I only see them in the "Files changed" page, not on "Conversation".

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

Godin · 2016-08-15T22:23:57Z

@marchof addressed all 8 comments in one way or another.

P.S. sometimes GitHub Web UI requires page reload and not just switching between tabs

marchof · 2016-08-16T02:25:32Z

@Godin Merged! Many thanks to @Godin and @bjkail for all the investigation which was possible to resolve this - even two JDK bugs were created for this ;-)

Good to know that in the meantime we have enough knowledge in the team to work on all tricky implementation details of JaCoCo::Core.

…for jacoco/jacoco#434 required for Java 9) git-svn-id: http://josm.openstreetmap.de/svn/trunk@10821 0c6e7542-c601-0410-84e7-c038aed88b3b

don-vip · 2016-08-16T18:48:03Z

We use the new 0.7.8 snapshot in JOSM that includes this PR and can confirm it works! We have a few errors caused by java.lang.IncompatibleClassChangeError, I wonder if it's a Jacoco or a Groovy issue?

The stacktrace is

java.lang.IncompatibleClassChangeError: Method org.openstreetmap.josm.gui.mappaint.mapcss.Condition.createKeyCondition(Ljava/lang/String;ZLorg/openstreetmap/josm/gui/mappaint/mapcss/Condition$KeyMatchType;Lorg/openstreetmap/josm/gui/mappai
    at org.openstreetmap.josm.gui.mappaint.mapcss.Condition$createKeyCondition.call(Unknown Source)
    at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCall(CallSiteArray.java:48)
    at org.codehaus.groovy.runtime.callsite.AbstractCallSite.call(AbstractCallSite.java:113)
    at org.codehaus.groovy.runtime.callsite.AbstractCallSite.call(AbstractCallSite.java:149)
    at org.openstreetmap.josm.gui.mappaint.mapcss.KeyConditionTest.applies_1(KeyConditionTest.groovy:81)
    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)

See https://josm.openstreetmap.de/jenkins/job/Java-EarlyAccess-JOSM/jdk=JDK9/lastCompletedBuild/testReport/org.openstreetmap.josm.gui.mappaint.mapcss/KeyConditionTest/applies_1/

Godin · 2016-08-16T19:17:37Z

@don-vip There is no traces of JaCoCo in the provided stack trace, while there are clearly traces of Groovy, so very likely it is caused by Groovy. If this happens even without JaCoCo, then for sure it is caused by Groovy.

Godin added type: bug 🐛 Something isn't working component: core labels Jul 26, 2016

Godin added this to the 0.7.8 milestone Jul 26, 2016

Godin self-assigned this Jul 26, 2016

Godin force-pushed the issue-434 branch 4 times, most recently from d52a5ce to 8ebd1d1 Compare July 28, 2016 11:34

Godin modified the milestones: Java 9, 0.7.8 Jul 29, 2016

Godin mentioned this pull request Jul 29, 2016

Java 9 Support: Profile for 1.9? #411

Merged

Godin force-pushed the issue-434 branch from f5e09cf to 67fd0ba Compare August 2, 2016 23:39

Godin changed the title ~~FieldProbeArrayStrategy should not violate Java VM Specification~~ Fix instrumentation to not violate Java Virtual Machine Specification regarding initialization of final fields Aug 3, 2016

Godin force-pushed the issue-434 branch 2 times, most recently from ba923a0 to 06f52f0 Compare August 3, 2016 09:34

Godin mentioned this pull request Aug 7, 2016

Java 9: IllegalAccessError on CLDRLocaleDataMetaInfo.$jacocoData #439

Closed

Godin force-pushed the issue-434 branch from 07ce0a3 to 70707a2 Compare August 14, 2016 14:08

Godin force-pushed the issue-434 branch from 70707a2 to 1ae260f Compare August 15, 2016 18:41

marchof reviewed Aug 15, 2016
View reviewed changes

Godin force-pushed the issue-434 branch from 1ae260f to 492b150 Compare August 15, 2016 22:19

Do not violate JVMS regarding initialization of final fields

492b150

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

marchof merged commit 28a112c into master Aug 16, 2016

marchof deleted the issue-434 branch August 16, 2016 02:07

Godin mentioned this pull request Aug 16, 2016

Improve ValidationTestBase #440

Merged

Godin mentioned this pull request Sep 8, 2016

Update comments about "bad cycles" with interfaces #448

Merged

TimvdLippe mentioned this pull request Sep 20, 2016

Update Travis configuration to also build on oraclejdk9 mockito/mockito#639

Merged

don-vip mentioned this pull request Oct 1, 2016

ReflectionException: Cannot read class file for some JDK9 classes jqno/equalsverifier#152

Closed

Godin mentioned this pull request Dec 11, 2016

Publish new Jacoco release #467

Closed

jacoco locked and limited conversation to collaborators Jan 11, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix instrumentation to not violate Java Virtual Machine Specification regarding initialization of final fields #434

Fix instrumentation to not violate Java Virtual Machine Specification regarding initialization of final fields #434

Godin commented Jul 26, 2016 •

edited

Loading

bjkail commented Jul 28, 2016

Godin commented Jul 28, 2016 •

edited

Loading

Godin commented Jul 29, 2016

Godin commented Jul 29, 2016

Godin commented Aug 3, 2016

bjkail commented Aug 4, 2016

Godin commented Aug 4, 2016

marchof commented Aug 12, 2016

bjkail commented Aug 12, 2016

Godin commented Aug 12, 2016

marchof commented Aug 12, 2016

Godin commented Aug 12, 2016

Godin commented Aug 14, 2016

bjkail commented Aug 15, 2016

Godin commented Aug 15, 2016

marchof commented Aug 15, 2016

marchof Aug 15, 2016

Godin Aug 15, 2016

marchof commented Aug 15, 2016

Godin commented Aug 15, 2016

marchof commented Aug 16, 2016

don-vip commented Aug 16, 2016

Godin commented Aug 16, 2016

Fix instrumentation to not violate Java Virtual Machine Specification regarding initialization of final fields #434

Fix instrumentation to not violate Java Virtual Machine Specification regarding initialization of final fields #434

Conversation

Godin commented Jul 26, 2016 • edited Loading

bjkail commented Jul 28, 2016

Godin commented Jul 28, 2016 • edited Loading

Godin commented Jul 29, 2016

Godin commented Jul 29, 2016

Godin commented Aug 3, 2016

bjkail commented Aug 4, 2016

Godin commented Aug 4, 2016

marchof commented Aug 12, 2016

bjkail commented Aug 12, 2016

Godin commented Aug 12, 2016

marchof commented Aug 12, 2016

Godin commented Aug 12, 2016

Godin commented Aug 14, 2016

bjkail commented Aug 15, 2016

Godin commented Aug 15, 2016

marchof commented Aug 15, 2016

marchof Aug 15, 2016

Choose a reason for hiding this comment

Godin Aug 15, 2016

Choose a reason for hiding this comment

marchof commented Aug 15, 2016

Godin commented Aug 15, 2016

marchof commented Aug 16, 2016

don-vip commented Aug 16, 2016

Godin commented Aug 16, 2016

Godin commented Jul 26, 2016 •

edited

Loading

Godin commented Jul 28, 2016 •

edited

Loading