Frames are re-calculated for large methods #177

Godin · 2016-05-27T16:30:37Z

JaCoCo cannot re-calculate stackmap frames as this is an expensive operation and would require knowing the type hierarchy (which is not available). Also ASM default implementation for the type hierarchy actually loads(!) the classes.

Therefore in JaCoCo special care has been taken to incrementally update the stackmap frames during instrumentation.

It has been reported that the stackmap frame are re-calculated anyways is special situations, stack trace:

java.lang.ClassNotFoundException: javax.servlet.jsp.tagext.BodyContent
         at org.jacoco.asm.ClassWriter.getCommonSuperClass(Unknown Source)
         at org.jacoco.asm.ClassWriter.a(Unknown Source)
         at org.jacoco.asm.Frame.a(Unknown Source)
         at org.jacoco.asm.Frame.a(Unknown Source)
         at org.jacoco.asm.MethodWriter.visitMaxs(Unknown Source)
         at org.jacoco.asm.ClassReader.a(Unknown Source)
         at org.jacoco.asm.ClassReader.b(Unknown Source)
         at org.jacoco.asm.ClassReader.accept(Unknown Source)
         at org.jacoco.asm.ClassReader.accept(Unknown Source)
         at org.jacoco.asm.ClassWriter.toByteArray(Unknown Source)
         at org.jacoco.core.instr.Instrumenter.instrument(Instrumenter.java:79)
         at org.jacoco.core.instr.Instrumenter.instrument(Instrumenter.java:139)

Looking at ASM source code it turned out that ASM triggers a frame re-calculation if the methods exceeds a certain size. See MethodWriter.resizeInstructions(). Shit.

Corresponding ASM issue: http://forge.ow2.org/tracker/?func=detail&aid=317551&group_id=23&atid=100023

marchof · 2013-12-25T17:08:24Z

Comment from MethodWriter.resizeInstructions():

/*
 * Resizing an existing stack map frame table is really hard.
 * Not only the table must be parsed to update the offets, but
 * new frames may be needed for jump instructions that were
 * inserted by this method. And updating the offsets or
 * inserting frames can change the format of the following
 * frames, in case of packed frames. In practice the whole table
 * must be recomputed. For this the frames are marked as
 * potentially invalid. This will cause the whole class to be
 * reread and rewritten with the COMPUTE_FRAMES option (see the
 * ClassWriter.toByteArray method). This is not very efficient
 * but is much easier and requires much less code than any other
 * method I can think of.
 */

marchof · 2014-03-10T12:05:19Z

Also see #31 regarding long methods.

peter-fu · 2014-12-18T10:20:15Z

+1

I'm working on a large project based on Play! Framework 2.2.3 (Java). It fails to instrument the generated Routes$$anonfun$routes$1.class. And I think this issue should be the cause of it.

Here is the stack trace ...

java.io.IOException: Error while instrumenting class Routes$$anonfun$routes$1.class.
    at org.jacoco.core.instr.Instrumenter.instrumentError(Instrumenter.java:152)
    at org.jacoco.core.instr.Instrumenter.instrument(Instrumenter.java:124)
    at de.johoop.jacoco4sbt.Instrumentation$$anonfun$instrumentAction$3.apply(Instrumentation.scala:49)
    at de.johoop.jacoco4sbt.Instrumentation$$anonfun$instrumentAction$3.apply(Instrumentation.scala:46)
    at scala.collection.TraversableLike$$anonfun$map$1.apply(TraversableLike.scala:244)
    at scala.collection.TraversableLike$$anonfun$map$1.apply(TraversableLike.scala:244)
    at scala.collection.mutable.ResizableArray$class.foreach(ResizableArray.scala:59)
    at scala.collection.mutable.ArrayBuffer.foreach(ArrayBuffer.scala:47)
    at scala.collection.TraversableLike$class.map(TraversableLike.scala:244)
    at scala.collection.AbstractTraversable.map(Traversable.scala:105)
    at de.johoop.jacoco4sbt.Instrumentation$class.instrumentAction(Instrumentation.scala:46)
    at de.johoop.jacoco4sbt.JacocoPlugin$jacoco$.instrumentAction(JacocoPlugin.scala:59)
    at de.johoop.jacoco4sbt.JacocoPlugin$SharedSettings$$anonfun$settings$5.apply(JacocoPlugin.scala:84)
    at de.johoop.jacoco4sbt.JacocoPlugin$SharedSettings$$anonfun$settings$5.apply(JacocoPlugin.scala:84)
    at scala.Function6$$anonfun$tupled$1.apply(Function6.scala:35)
    at scala.Function6$$anonfun$tupled$1.apply(Function6.scala:34)
    at scala.Function1$$anonfun$compose$1.apply(Function1.scala:47)
    at sbt.$tilde$greater$$anonfun$$u2219$1.apply(TypeFunctions.scala:42)
    at sbt.std.Transform$$anon$4.work(System.scala:64)
    at sbt.Execute$$anonfun$submit$1$$anonfun$apply$1.apply(Execute.scala:237)
    at sbt.Execute$$anonfun$submit$1$$anonfun$apply$1.apply(Execute.scala:237)
    at sbt.ErrorHandling$.wideConvert(ErrorHandling.scala:18)
    at sbt.Execute.work(Execute.scala:244)
    at sbt.Execute$$anonfun$submit$1.apply(Execute.scala:237)
    at sbt.Execute$$anonfun$submit$1.apply(Execute.scala:237)
    at sbt.ConcurrentRestrictions$$anon$4$$anonfun$1.apply(ConcurrentRestrictions.scala:160)
    at sbt.CompletionService$$anon$2.call(CompletionService.scala:30)
    at java.util.concurrent.FutureTask.run(FutureTask.java:262)
    at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:471)
    at java.util.concurrent.FutureTask.run(FutureTask.java:262)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:615)
    at java.lang.Thread.run(Thread.java:744)
Caused by: java.lang.RuntimeException: java.lang.ClassNotFoundException: play.api.mvc.Handler
    at org.objectweb.asm.ClassWriter.getCommonSuperClass(ClassWriter.java:1684)
    at org.objectweb.asm.ClassWriter.getMergedType(ClassWriter.java:1654)
    at org.objectweb.asm.Frame.merge(Frame.java:1426)
    at org.objectweb.asm.Frame.merge(Frame.java:1325)
    at org.objectweb.asm.MethodWriter.visitMaxs(MethodWriter.java:1475)
    at org.objectweb.asm.ClassReader.readCode(ClassReader.java:1554)
    at org.objectweb.asm.ClassReader.readMethod(ClassReader.java:1017)
    at org.objectweb.asm.ClassReader.accept(ClassReader.java:693)
    at org.objectweb.asm.ClassReader.accept(ClassReader.java:506)
    at org.objectweb.asm.ClassWriter.toByteArray(ClassWriter.java:995)
    at org.jacoco.core.instr.Instrumenter.instrument(Instrumenter.java:84)
    at org.jacoco.core.instr.Instrumenter.instrument(Instrumenter.java:122)
    at de.johoop.jacoco4sbt.Instrumentation$$anonfun$instrumentAction$3.apply(Instrumentation.scala:49)
    at de.johoop.jacoco4sbt.Instrumentation$$anonfun$instrumentAction$3.apply(Instrumentation.scala:46)
    at scala.collection.TraversableLike$$anonfun$map$1.apply(TraversableLike.scala:244)
    at scala.collection.TraversableLike$$anonfun$map$1.apply(TraversableLike.scala:244)
    at scala.collection.mutable.ResizableArray$class.foreach(ResizableArray.scala:59)
    at scala.collection.mutable.ArrayBuffer.foreach(ArrayBuffer.scala:47)
    at scala.collection.TraversableLike$class.map(TraversableLike.scala:244)
    at scala.collection.AbstractTraversable.map(Traversable.scala:105)
    at de.johoop.jacoco4sbt.Instrumentation$class.instrumentAction(Instrumentation.scala:46)
    at de.johoop.jacoco4sbt.JacocoPlugin$jacoco$.instrumentAction(JacocoPlugin.scala:59)
    at de.johoop.jacoco4sbt.JacocoPlugin$SharedSettings$$anonfun$settings$5.apply(JacocoPlugin.scala:84)
    at de.johoop.jacoco4sbt.JacocoPlugin$SharedSettings$$anonfun$settings$5.apply(JacocoPlugin.scala:84)
    at scala.Function6$$anonfun$tupled$1.apply(Function6.scala:35)
    at scala.Function6$$anonfun$tupled$1.apply(Function6.scala:34)
    at scala.Function1$$anonfun$compose$1.apply(Function1.scala:47)
    at sbt.$tilde$greater$$anonfun$$u2219$1.apply(TypeFunctions.scala:42)
    at sbt.std.Transform$$anon$4.work(System.scala:64)
    at sbt.Execute$$anonfun$submit$1$$anonfun$apply$1.apply(Execute.scala:237)
    at sbt.Execute$$anonfun$submit$1$$anonfun$apply$1.apply(Execute.scala:237)
    at sbt.ErrorHandling$.wideConvert(ErrorHandling.scala:18)
    at sbt.Execute.work(Execute.scala:244)
    at sbt.Execute$$anonfun$submit$1.apply(Execute.scala:237)
    at sbt.Execute$$anonfun$submit$1.apply(Execute.scala:237)
    at sbt.ConcurrentRestrictions$$anon$4$$anonfun$1.apply(ConcurrentRestrictions.scala:160)
    at sbt.CompletionService$$anon$2.call(CompletionService.scala:30)
    at java.util.concurrent.FutureTask.run(FutureTask.java:262)
    at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:471)
    at java.util.concurrent.FutureTask.run(FutureTask.java:262)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:615)
    at java.lang.Thread.run(Thread.java:744)

peter-fu · 2014-12-18T14:11:01Z

Workaround to my case: split the routes file to several files in order to make the generated classes small enough to execute jacoco.

Godin · 2014-12-19T18:03:11Z

@peter-fu another option: if you don't need to monitor coverage of auto-generated classes, then you can simply exclude them from instrumentation and analysis by JaCoCo

peter-fu · 2014-12-21T12:33:19Z

@Godin thanks for the reply.

Your workaround should work for JaCoCo users but not for jacoco4sbt users.

I'm using jacoco4sbt 2.1.6. And I've verified that with jacoco.excludes in jacoco.Config := Seq("views*", "Routes*", "controllers*routes*", "controllers*Reverse*", "controllers*javascript*", "controller*ref*"), the Routes.class is still got instrumented.

Also verified in code
https://github.com/sbt/jacoco4sbt/blob/2.1.6/src/main/scala/de/johoop/jacoco4sbt/JacocoPlugin.scala#L84

Basically jacoco4sbt is instrumenting everything.

marchof · 2014-12-21T13:16:02Z

@peter-fu Maybe you can report this to jacoco4sbt to make at least a work-around possible?

peter-fu · 2014-12-21T14:49:53Z

@marchof Sure. It's reported here sbt/sbt-jacoco#44.

However, that won't help if the target class is what user needs to test against. So I don't know whether that change will be made at the jacoco4sbt side. Without fixing this issue, the splitting idea is still a more general workaround than excluding.

marchof · 2014-12-21T14:51:02Z

@peter-fu Absolutely. The best way would be to fix the issue in ASM.

marchof · 2014-12-27T09:38:51Z

Opened an issue against ASM: http://forge.ow2.org/tracker/?func=detail&aid=317551&group_id=23&atid=100023

antonioalegria · 2015-02-02T16:44:37Z

Hi, it seems the ASM guys are not pursuing this issue. Is there any workaround that jacoco sbt users can take?

marchof · 2015-02-02T20:35:35Z

You may exclude the problematic class from instrumentation.

kazzmir · 2015-05-15T00:09:51Z

marchof, were you able to generate a test case and test the patch in that ASM bug?

(easier to post here than the ASM bug)

marchof · 2016-05-25T18:27:19Z

@Godin Ok, let's follow-up here: Could have been a patch (long time ago I looked at it). Here is what I wanted to do at that time:

Generate a class with a method big enough to trigger the condition described in MethodWriter.resizeInstructions()
Verify that getCommonSuperClass() gets called
Build and test JaCoCo against a patched ASM version
Verify that JaCoCo works properly and getCommonSuperClass() is not called any more
Report result back to ASM

Godin · 2016-05-25T18:45:08Z

@marchof okay, thanks for the info. I'll try to test it.

Godin · 2016-05-27T18:59:23Z

@marchof reproducer attached.

It reproduces problem with released ASM 5.0.4 and with r1773, which is base for patch http://forge.ow2.org/tracker/download.php/23/100023/317551/3773/bugfix317551.patch from http://forge.ow2.org/tracker/?func=detail&aid=317551&group_id=23&atid=100023 But unfortunately even construction of a class for reproducer fails on r1773 with patch:

java.lang.NullPointerException
    at org.objectweb.asm.Frame.push(Frame.java:691)
    at org.objectweb.asm.Frame.execute(Frame.java:978)
    at org.objectweb.asm.CurrentFrame.execute(CurrentFrame.java:50)
    at org.objectweb.asm.MethodWriter.visitInsn(MethodWriter.java:746)
    at org.objectweb.asm.ClassReader.readCode(ClassReader.java:1363)
    at org.objectweb.asm.ClassReader.readMethod(ClassReader.java:1032)
    at org.objectweb.asm.ClassReader.accept(ClassReader.java:708)
    at org.objectweb.asm.ClassReader.accept(ClassReader.java:521)
    at org.objectweb.asm.ClassWriter.toByteArray(ClassWriter.java:1005)
    at org.jacoco.core.test.validation.ResizeInstructionsTest.createClass(ResizeInstructionsTest.java:116)
    at org.jacoco.core.test.validation.ResizeInstructionsTest.test(ResizeInstructionsTest.java:42)

So that if reproducer looks correct for you, then I suppose we can report back that patch is not working.

marchof · 2016-05-27T19:04:26Z

@Godin Now I remember that I came to similar point ;-)

http://forge.ow2.org/tracker/?func=detail&aid=317555&group_id=23&atid=100023

I somehow failed to attach my reproducer at that time (be aware: ow2 tools are tricky)!

Godin · 2016-05-27T19:08:49Z

@marchof 😆 okay, so that patch actually should be applied on top of r1776 , let me try

Godin · 2016-05-27T19:52:24Z

@marchof same exception as before, which is btw different from the one you faced and contains traces of a patch - org.objectweb.asm.CurrentFrame is new file from patch

Godin · 2016-05-27T20:07:50Z

@marchof hmmm, however it works, when generated Short.MAX_VALUE - 7 NOPs instead of Short.MAX_VALUE

Godin · 2016-05-27T20:09:28Z

and Short.MAX_VALUE - 3 causes similar NPE during instrumentation, but not during construction of class:

Caused by: java.lang.NullPointerException
    at org.objectweb.asm.Frame.push(Frame.java:691)
    at org.objectweb.asm.Frame.push(Frame.java:710)
    at org.objectweb.asm.Frame.execute(Frame.java:1308)
    at org.objectweb.asm.CurrentFrame.execute(CurrentFrame.java:50)
    at org.objectweb.asm.MethodWriter.visitMethodInsn(MethodWriter.java:917)
    at org.objectweb.asm.ClassReader.readCode(ClassReader.java:1492)
    at org.objectweb.asm.ClassReader.readMethod(ClassReader.java:1032)
    at org.objectweb.asm.ClassReader.accept(ClassReader.java:708)
    at org.objectweb.asm.ClassReader.accept(ClassReader.java:521)
    at org.objectweb.asm.ClassWriter.toByteArray(ClassWriter.java:1005)
    at org.jacoco.core.instr.Instrumenter.instrument(Instrumenter.java:85)
    at org.jacoco.core.instr.Instrumenter.instrument(Instrumenter.java:108)

Godin · 2016-12-29T14:55:00Z

@marchof could you please review? Similarly to #462 (comment) test uses a bit of bytecode generation.

bjkail · 2017-01-03T15:49:24Z

@Godin I don't understand the assertEquals(true, computedCommonSuperClass);. Per marchof's comment, we want to assert that the new ASM does not call getCommonSuperclass due to an internal resizeInstructions.

Godin · 2017-01-03T17:50:33Z

@bjkail construction of original class happens with COMPUTE_FRAMES and this assertion ensures that method getCommonSuperclass is called during construction of original class, i.e. that reproducer makes sense. After that during instrumentation there is no ClassNotFoundException, i.e. that upgrade of ASM library solves the issue and there is no computation of frames and no invocation of getCommonSuperclass during instrumentation.

marchof · 2017-01-03T21:50:00Z

@Godin Not sure how this test asserts that getCommonSuperClass() is not called during instrumentation? java/lang/Integer should load without any problems.

I think we should now add an assertion in Instrumenter that forbids execution of getCommonSuperClass() at all.

marchof · 2017-01-03T21:52:05Z

org.jacoco.core.test/src/org/jacoco/core/test/validation/ResizeInstructionsTest.java

+
+	private static void load(final String className, final byte[] bytes)
+			throws ClassNotFoundException {
+		new ClassLoader() {


Use existing TargetLoader instead?

marchof · 2017-01-03T21:53:19Z

org.jacoco.core.test/src/org/jacoco/core/test/validation/ResizeInstructionsTest.java

+		mv.visitEnd();
+		cw.visitEnd();
+		final byte[] original = cw.toByteArray();
+		assertEquals(true, computedCommonSuperClass);


I prefer assertTrue(computedCommonSuperClass) and I think we use it consistently in other tests.

You right. Fixed.

Godin · 2017-01-03T22:16:43Z

@marchof

Not sure how this test asserts that getCommonSuperClass() is not called during instrumentation? java/lang/Integer should load without any problems.

Default implementation of String getCommonSuperClass(final String type1, final String type2) that is used during instrumentation loads both type1 and type2, so while loading of java/lang/Integer is ok, loading of created class Example will fail with ClassNotFoundException if this method is called. You can observe this in a build where test was added or by downgrading ASM to 0.5.1.

I think we should now add an assertion in Instrumenter that forbids execution of getCommonSuperClass() at all.

Indeed. I'll add this.

…ucted class

marchof

LGTM now, thanks!

marchof

Hm, Github didn't let me approve this, let's try again.

marchof · 2017-01-04T06:36:14Z

@Godin Somehow "Add your review" and then checking "Approve" does not seem to work currently. Anything I overlooked here?

marchof

Another try to Approve this.

Godin · 2017-01-04T10:19:18Z

@marchof don't know why you wasn't able to approve. I can bypass this check...

marchof

@Godin I try one more time to approve this PR. If it dosn't work please bypass this check. Thx!

ghost assigned marchof Dec 25, 2013

peter-fu mentioned this pull request Dec 21, 2014

Could not initialize class org.jacoco.core.internal.flow.ClassProbesAdapter sbt/sbt-jacoco#42

Open

peter-fu mentioned this pull request Dec 21, 2014

Respect jacoco.excludes in the instrumentation process sbt/sbt-jacoco#44

Open

marchof mentioned this pull request Dec 23, 2014

java.lang.instrument.IllegalClassFormatException:.... Caused by: java.lang.RuntimeException: java.lang.ClassNotFoundException #267

Closed

marchof added the milestone candidate label May 22, 2015

marchof mentioned this pull request May 25, 2016

Dead lock on class loader #54

Open

Godin assigned Godin and unassigned marchof May 25, 2016

Godin force-pushed the issue-177 branch from e541095 to 094c9a8 Compare May 27, 2016 18:43

(wip) add test

4dc7420

Godin force-pushed the issue-177 branch 2 times, most recently from 619c6b9 to 36e7412 Compare December 29, 2016 14:05

Godin added 2 commits December 29, 2016 15:06

(wip) upgrade ASM to 5.2

36e7412

(wip) update changelog

ae98e69

Godin requested a review from marchof December 29, 2016 14:55

Godin and others added 2 commits January 3, 2017 23:22

Merge branch 'master' into issue-177

2fa7c74

Happy New Year 2017!

9f74dc7

marchof reviewed Jan 3, 2017

View reviewed changes

Godin added 4 commits January 3, 2017 23:34

(wip) use assertTrue instead of assertEquals

ce4edff

(wip) use TargetLoader

cfd54a0

(wip) demonstrate that getCommonSuperClass requires loading of constr…

1873e16

…ucted class

(wip) forbid execution of getCommonSuperClass

31d7e26

Godin requested a review from marchof January 3, 2017 23:28

marchof reviewed Jan 4, 2017

View reviewed changes

Merge branch 'master' into issue-177

0ad3368

Godin merged commit ed97906 into master Jan 4, 2017

Godin deleted the issue-177 branch January 4, 2017 11:15

jacoco locked and limited conversation to collaborators Jan 11, 2017

Frames are re-calculated for large methods #177

Frames are re-calculated for large methods #177

Conversation

Godin commented May 27, 2016

marchof commented Dec 25, 2013

marchof commented Mar 10, 2014

peter-fu commented Dec 18, 2014

peter-fu commented Dec 18, 2014

Godin commented Dec 19, 2014

peter-fu commented Dec 21, 2014

marchof commented Dec 21, 2014

peter-fu commented Dec 21, 2014

marchof commented Dec 21, 2014

marchof commented Dec 27, 2014

antonioalegria commented Feb 2, 2015

marchof commented Feb 2, 2015

kazzmir commented May 15, 2015

marchof commented May 25, 2016

Godin commented May 25, 2016

Godin commented May 27, 2016 • edited

marchof commented May 27, 2016

Godin commented May 27, 2016

Godin commented May 27, 2016

Godin commented May 27, 2016

Godin commented May 27, 2016

Godin commented Dec 29, 2016

bjkail commented Jan 3, 2017

Godin commented Jan 3, 2017 • edited

marchof commented Jan 3, 2017

marchof Jan 3, 2017

Choose a reason for hiding this comment

Godin Jan 3, 2017

Choose a reason for hiding this comment

marchof Jan 3, 2017

Choose a reason for hiding this comment

Godin Jan 3, 2017

Choose a reason for hiding this comment

Godin commented Jan 3, 2017

marchof left a comment

Choose a reason for hiding this comment

marchof left a comment

Choose a reason for hiding this comment

marchof commented Jan 4, 2017

marchof left a comment

Choose a reason for hiding this comment

Godin commented Jan 4, 2017

marchof left a comment

Choose a reason for hiding this comment

Godin commented May 27, 2016 •

edited

Godin commented Jan 3, 2017 •

edited