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

java.lang.ClassFormatError: Short length on BootstrapMethods in class file #462

Merged
merged 9 commits into from Jan 4, 2017

Conversation

Projects
None yet
3 participants
@Godin
Member

Godin commented Dec 29, 2016

Posted by @ nikhilnanivadekar :

I am trying to integrate JaCoCo for code coverage of Eclipse Collections

However the build fails with the below error:
java.lang.ClassFormatError: Short length on BootstrapMethods in class file org/eclipse/collections/impl/collector/Collectors2Test

The Collectors2Test.java is 1748 lines long with very small test methods. So, this should not cause any issues as they are well below 64KB method limit of Java.
If I break up this class in two separate files then the build goes through fine. Even after online research I am not able to find a way to fix the Travis build with JaCoCo integration and without splitting the Collectors2Test.java. The build goes through fine in IntelliJ.

The project is setup with Maven build and has surefire plugin. argLine is set in relevant modules where JaCoCo test coverage is run.

GitHub repos and build links:

  1. Eclipse Collections master
    Repo
    Travis build (passing)

  2. JaCoCo integration with Collectors2Test split in two:
    Repo
    JaCoCo integration commit
    Travis build (passing)

  3. JaCoCo integration with Collectors2Test not split:
    Repo
    JaCoCo integration commit
    Travis build (failing)

Steps to reproduce

  1. Fork and clone the eclipse collections repo
  2. Apply commit 62c5ca4cf04c6ab692f135bf1ef03c1834d5b92a on top of it
  3. Enable Travis integration for your fork.
  4. Push the commit from step 2
  5. Check the unit test build. It generally takes about 5-6 mins to fail.
@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Nov 17, 2016

Member

@nikhilnanivadekar could you please provide the original file org/eclipse/collections/impl/collector/Collectors2Test.class (not splittet). Thx,

Member

marchof commented Nov 17, 2016

@nikhilnanivadekar could you please provide the original file org/eclipse/collections/impl/collector/Collectors2Test.class (not splittet). Thx,

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Nov 17, 2016

Member

@nikhilnanivadekar I think that @marchof was asking for class file. I'm able to reproduce this on my machine, so here is file - class.zip (in zip, because GitHub doesn't allow to attach class directly).

Member

Godin commented Nov 17, 2016

@nikhilnanivadekar I think that @marchof was asking for class file. I'm able to reproduce this on my machine, so here is file - class.zip (in zip, because GitHub doesn't allow to attach class directly).

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Nov 17, 2016

Member

@nikhilnanivadekar in the meantime as workaround you can exclude class from the instrumentation:

diff --git i/unit-tests/pom.xml w/unit-tests/pom.xml
index a14a705..10c60ba 100755
--- i/unit-tests/pom.xml
+++ w/unit-tests/pom.xml
@@ -102,6 +102,12 @@
                 <groupId>org.jacoco</groupId>
                 <artifactId>jacoco-maven-plugin</artifactId>
                 <version>${jacoco.plugin.version}</version>
+                <configuration>
+                  <excludes>
+                    <!-- https://github.com/jacoco/jacoco/issues/462 -->
+                    <exclude>org.eclipse.collections.impl.collector.Collectors2Test</exclude>
+                  </excludes>
+                </configuration>
             </plugin>

This does not affect coverage measurement of the main code since this is test file.
Build fully passes on my machine after this modification.

Member

Godin commented Nov 17, 2016

@nikhilnanivadekar in the meantime as workaround you can exclude class from the instrumentation:

diff --git i/unit-tests/pom.xml w/unit-tests/pom.xml
index a14a705..10c60ba 100755
--- i/unit-tests/pom.xml
+++ w/unit-tests/pom.xml
@@ -102,6 +102,12 @@
                 <groupId>org.jacoco</groupId>
                 <artifactId>jacoco-maven-plugin</artifactId>
                 <version>${jacoco.plugin.version}</version>
+                <configuration>
+                  <excludes>
+                    <!-- https://github.com/jacoco/jacoco/issues/462 -->
+                    <exclude>org.eclipse.collections.impl.collector.Collectors2Test</exclude>
+                  </excludes>
+                </configuration>
             </plugin>

This does not affect coverage measurement of the main code since this is test file.
Build fully passes on my machine after this modification.

@Godin Godin changed the title from Travis build fails with java.lang.ClassFormatError after integration with JaCoCo to java.lang.ClassFormatError: Short length on BootstrapMethods in class file Nov 17, 2016

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Nov 17, 2016

Member

@marchof if I'm not mistaken, seems that one bootstrap method is lost during instrumentation

Member

Godin commented Nov 17, 2016

@marchof if I'm not mistaken, seems that one bootstrap method is lost during instrumentation

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Nov 17, 2016

Member

@Godin Interesting about this file is that there are >> 300 method references (which compiles to INVOKEDYNAMIC). I assume this is a ASM Bug.

Member

marchof commented Nov 17, 2016

@Godin Interesting about this file is that there are >> 300 method references (which compiles to INVOKEDYNAMIC). I assume this is a ASM Bug.

@Godin

This comment has been minimized.

Show comment
Hide comment
@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Nov 17, 2016

Member

@Godin So we have another issue we would need an ASM release for.

Member

marchof commented Nov 17, 2016

@Godin So we have another issue we would need an ASM release for.

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Nov 17, 2016

Member

For the record: I tried JaCoCo with ASM from trunk, which includes fix for the mentioned bug, and it works without exclusion for Eclipse Collections.

Member

Godin commented Nov 17, 2016

For the record: I tried JaCoCo with ASM from trunk, which includes fix for the mentioned bug, and it works without exclusion for Eclipse Collections.

@nikhilnanivadekar

This comment has been minimized.

Show comment
Hide comment
@nikhilnanivadekar

nikhilnanivadekar Nov 17, 2016

Contributor

Any idea when will the bug fix be released? If it will be soon enough, I would rather wait for it and not add an exclusion.

Contributor

nikhilnanivadekar commented Nov 17, 2016

Any idea when will the bug fix be released? If it will be soon enough, I would rather wait for it and not add an exclusion.

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Nov 17, 2016

Member

@nikhilnanivadekar Please let's discuss the separate issue on the mailing list.

Unfortunatelly we have no insight in ASM project when they plan the next release.

Member

marchof commented Nov 17, 2016

@nikhilnanivadekar Please let's discuss the separate issue on the mailing list.

Unfortunatelly we have no insight in ASM project when they plan the next release.

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Nov 17, 2016

Member

@nikhilnanivadekar so far we are not aware about plans of ASM developers regarding release. And would be better to not mix with other topics here. Thanks for your understanding.

Member

Godin commented Nov 17, 2016

@nikhilnanivadekar so far we are not aware about plans of ASM developers regarding release. And would be better to not mix with other topics here. Thanks for your understanding.

@nikhilnanivadekar

This comment has been minimized.

Show comment
Hide comment
@nikhilnanivadekar

nikhilnanivadekar Nov 17, 2016

Contributor

Updated my comment. Thanks!

Contributor

nikhilnanivadekar commented Nov 17, 2016

Updated my comment. Thanks!

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Nov 17, 2016

Member

Ouch, didn't noticed Marc's answer.

I'm going to ask in their mailing list about release, however without expectations - previously didn't get response on same question in bug tracker and there seems to be no activity since September.

Member

Godin commented Nov 17, 2016

Ouch, didn't noticed Marc's answer.

I'm going to ask in their mailing list about release, however without expectations - previously didn't get response on same question in bug tracker and there seems to be no activity since September.

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Dec 10, 2016

Member

For the record - started thread about release in ASM mailing list: http://mail.ow2.org/wws/arc/asm/2016-12/msg00005.html

Member

Godin commented Dec 10, 2016

For the record - started thread about release in ASM mailing list: http://mail.ow2.org/wws/arc/asm/2016-12/msg00005.html

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Dec 11, 2016

Member

A note to clarify relation of example given here with description of ASM bug: In presence of Serializable lambdas javac will generate method $deserializeLambda$. This method is big in given example due to amount of lambdas and its instrumentation triggers recalculation of stack map frames similarly to #177 and ASM looses bootstrap method during this process as described in ASM bug.

Member

Godin commented Dec 11, 2016

A note to clarify relation of example given here with description of ASM bug: In presence of Serializable lambdas javac will generate method $deserializeLambda$. This method is big in given example due to amount of lambdas and its instrumentation triggers recalculation of stack map frames similarly to #177 and ASM looses bootstrap method during this process as described in ASM bug.

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Dec 23, 2016

Member

ASM 5.2 has been released, for upgrade we just need to wait its appearance in Maven Central repository.

@marchof I'm wondering if we should add test for such case to prevent regressions in future, or just update dependency and hope that it won't regress? Could be noted that change in ASM wasn't explicitly covered by tests. 😉 WDYT?

Member

Godin commented Dec 23, 2016

ASM 5.2 has been released, for upgrade we just need to wait its appearance in Maven Central repository.

@marchof I'm wondering if we should add test for such case to prevent regressions in future, or just update dependency and hope that it won't regress? Could be noted that change in ASM wasn't explicitly covered by tests. 😉 WDYT?

@Godin Godin added this to the 0.7.9 milestone Dec 23, 2016

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Dec 23, 2016

Member

@Godin Of course we add a test -- if we're able to create a reproducer e.g. as a validation test! 😎

Member

marchof commented Dec 23, 2016

@Godin Of course we add a test -- if we're able to create a reproducer e.g. as a validation test! 😎

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Dec 29, 2016

Member

I chose to not add exact example with lambda deserialization as it depends on javac and its internals, and actually not so trivial to also trigger resizing in ASM. But added test that performs a bit of bytecode generation instead. As you can see - it fails before ASM upgrade and passes after. So @marchof could you please review this change?

Member

Godin commented Dec 29, 2016

I chose to not add exact example with lambda deserialization as it depends on javac and its internals, and actually not so trivial to also trigger resizing in ASM. But added test that performs a bit of bytecode generation instead. As you can see - it fails before ASM upgrade and passes after. So @marchof could you please review this change?

@Godin Godin self-assigned this Dec 29, 2016

@Godin Godin requested a review from marchof Dec 29, 2016

Godin added some commits Dec 29, 2016

Godin added some commits Jan 3, 2017

@marchof

marchof approved these changes Jan 4, 2017

Godin added some commits Jan 4, 2017

@Godin Godin merged commit b208594 into master Jan 4, 2017

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 deleted the issue-462 branch Jan 4, 2017

@nikhilnanivadekar

This comment has been minimized.

Show comment
Hide comment
@nikhilnanivadekar

nikhilnanivadekar Jan 4, 2017

Contributor

@Godin Thank you very much for fixing this.
What are the plans for a JaCoCo version release so that I can upgrade it in Eclipse Collections and remove the workaround?

Contributor

nikhilnanivadekar commented Jan 4, 2017

@Godin Thank you very much for fixing this.
What are the plans for a JaCoCo version release so that I can upgrade it in Eclipse Collections and remove the workaround?

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Jan 5, 2017

Member

@nikhilnanivadekar we can't tell you the exact date yet - there is no strict plan so far.

And as was said in eclipse/eclipse-collections#166 (comment) : EclEmma Eclipse Plugin also needs an update, otherwise execution of tests with coverage in Eclipse using EclEmma will be failing without the current workaround. This update will take some time as we need to not only release JaCoCo, but also promote it as well as new ASM version thru Eclipse CQs into Orbit, followed by a first release of EclEmma under Eclipse umbrella.

All in all my advice for you will be - no need to rush especially that workaround doesn't cause any troubles, a little of patience and stay tuned for announcements about releases 😉

Member

Godin commented Jan 5, 2017

@nikhilnanivadekar we can't tell you the exact date yet - there is no strict plan so far.

And as was said in eclipse/eclipse-collections#166 (comment) : EclEmma Eclipse Plugin also needs an update, otherwise execution of tests with coverage in Eclipse using EclEmma will be failing without the current workaround. This update will take some time as we need to not only release JaCoCo, but also promote it as well as new ASM version thru Eclipse CQs into Orbit, followed by a first release of EclEmma under Eclipse umbrella.

All in all my advice for you will be - no need to rush especially that workaround doesn't cause any troubles, a little of patience and stay tuned for announcements about releases 😉

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Jan 6, 2017

Member

For the record: here is a ticket about update for EclEmma - https://bugs.eclipse.org/bugs/show_bug.cgi?id=510059

Member

Godin commented Jan 6, 2017

For the record: here is a ticket about update for EclEmma - https://bugs.eclipse.org/bugs/show_bug.cgi?id=510059

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