Skip to content
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

Godin
Copy link
Member

@Godin 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 marchof added type: bug 🐛 Something isn't working component: core labels Nov 17, 2016
@marchof
Copy link
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
Copy link
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
Copy link
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 Travis build fails with java.lang.ClassFormatError after integration with JaCoCo java.lang.ClassFormatError: Short length on BootstrapMethods in class file Nov 17, 2016
@Godin
Copy link
Member

Godin commented Nov 17, 2016

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

@marchof
Copy link
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
Copy link
Member

Godin commented Nov 17, 2016

@marchof
Copy link
Member

marchof commented Nov 17, 2016

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

@Godin
Copy link
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
Copy link
Contributor Author

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
Copy link
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
Copy link
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
Copy link
Contributor Author

Updated my comment. Thanks!

@Godin
Copy link
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
Copy link
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
Copy link
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
Copy link
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
Copy link
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
Copy link
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 December 29, 2016 12:54
@Godin Godin force-pushed the issue-462 branch 2 times, most recently from 1df652e to 08ef8a2 Compare December 29, 2016 13:49
private static int run(final String className, final byte[] bytes)
throws ClassNotFoundException, NoSuchMethodException,
InvocationTargetException, IllegalAccessException {
return (Integer) new ClassLoader() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can also use TargetLoader here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

* {@code java.lang.ClassFormatError: Short length on BootstrapMethods in class file}
* during instrumentation.
*/
public class AsmBugTest {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We encountered several ASM bug in the past. So the test name looks a bit too generic for me. Also we tend to name our test cases like what is tested, not what is a bug. I would prefer something like BootstrapMethodReferenceTest.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@Godin Godin merged commit b208594 into master Jan 4, 2017
@Godin Godin deleted the issue-462 branch January 4, 2017 11:59
@nikhilnanivadekar
Copy link
Contributor Author

@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
Copy link
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
Copy link
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.
Labels
component: core type: bug 🐛 Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants