-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Fix instrumentation to not violate Java Virtual Machine Specification regarding initialization of final fields #434
Conversation
d52a5ce
to
8ebd1d1
Compare
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. |
@bjkail that's exactly what I was prototyping here = clinit + slow path, and there is no more |
@bjkail I was wrong about the example. |
@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. |
ba923a0
to
06f52f0
Compare
@marchof IMO now this PR completely ready to be merged, so could you please review? |
FWIW, the core changes look good to me. |
Tried to create a cyclic initialization example for interfaces. Results in a JVM crash: https://gist.github.com/marchof/c0ac04a2abb4b163bf410e8018a72cb7 Will report to OpenJDK. |
@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 Unfortunatelly OpenJDK-Bugs are "write once". Internal ID is JI-9042792. |
@marchof let me send an email ;) |
"slow path" implemented, so @marchof could you please review? |
InstrumentingLoader uses 4-space indentation rather than tabs. Otherwise, looks good to me. |
@bjkail reformatted |
@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. |
* 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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
">" missing here (the whole quote becomes rendered as a link)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marchof fixed
@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).
@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 |
…for jacoco/jacoco#434 required for Java 9) git-svn-id: http://josm.openstreetmap.de/svn/trunk@10821 0c6e7542-c601-0410-84e7-c038aed88b3b
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
|
@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. |
Currently
FieldProbeArrayStrategy
createsstatic final
field$jacocoData
, value for which is set by bytecode instructionputstatic
in method$jacocoInit
, but according to JVMS: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: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:
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 makeFieldProbeArrayStrategy
unusable for interfaces with methods, because JVMS states that