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

Frames are re-calculated for large methods #177

Merged
merged 10 commits into from Jan 4, 2017

Conversation

Projects
None yet
7 participants
@Godin
Member

Godin commented May 27, 2016

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

@ghost ghost assigned marchof Dec 25, 2013

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Dec 25, 2013

Member

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.
 */
Member

marchof commented Dec 25, 2013

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

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Mar 10, 2014

Member

Also see #31 regarding long methods.

Member

marchof commented Mar 10, 2014

Also see #31 regarding long methods.

@peter-fu

This comment has been minimized.

Show comment
Hide comment
@peter-fu

peter-fu Dec 18, 2014

+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 commented Dec 18, 2014

+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

This comment has been minimized.

Show comment
Hide comment
@peter-fu

peter-fu Dec 18, 2014

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

peter-fu commented Dec 18, 2014

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

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Dec 19, 2014

Member

@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

Member

Godin commented Dec 19, 2014

@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

This comment has been minimized.

Show comment
Hide comment
@peter-fu

peter-fu Dec 21, 2014

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

peter-fu commented Dec 21, 2014

@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

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Dec 21, 2014

Member

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

Member

marchof commented Dec 21, 2014

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

@peter-fu

This comment has been minimized.

Show comment
Hide comment
@peter-fu

peter-fu Dec 21, 2014

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

peter-fu commented Dec 21, 2014

@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

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Dec 21, 2014

Member

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

Member

marchof commented Dec 21, 2014

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

@marchof

This comment has been minimized.

Show comment
Hide comment
Member

marchof commented Dec 27, 2014

@antonioalegria

This comment has been minimized.

Show comment
Hide comment
@antonioalegria

antonioalegria Feb 2, 2015

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

antonioalegria commented Feb 2, 2015

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

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Feb 2, 2015

Member

You may exclude the problematic class from instrumentation.

Member

marchof commented Feb 2, 2015

You may exclude the problematic class from instrumentation.

@kazzmir

This comment has been minimized.

Show comment
Hide comment
@kazzmir

kazzmir May 15, 2015

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)

kazzmir commented May 15, 2015

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

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof May 25, 2016

Member

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

marchof commented May 25, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin May 25, 2016

Member

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

Member

Godin commented May 25, 2016

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

@Godin Godin assigned Godin and unassigned marchof May 25, 2016

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin May 27, 2016

Member

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

Member

Godin commented May 27, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof May 27, 2016

Member

@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)!

Member

marchof commented May 27, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin May 27, 2016

Member

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

Member

Godin commented May 27, 2016

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

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin May 27, 2016

Member

@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

Member

Godin commented May 27, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin May 27, 2016

Member

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

Member

Godin commented May 27, 2016

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

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin May 27, 2016

Member

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

Godin commented May 27, 2016

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

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin May 27, 2016

Member

which is understandable - lower values do not cause resizing

Member

Godin commented May 27, 2016

which is understandable - lower values do not cause resizing

Godin added some commits Dec 29, 2016

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Dec 29, 2016

Member

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

Member

Godin commented Dec 29, 2016

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

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

@bjkail

This comment has been minimized.

Show comment
Hide comment
@bjkail

bjkail Jan 3, 2017

Contributor

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

Contributor

bjkail commented Jan 3, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Jan 3, 2017

Member

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

Member

Godin commented Jan 3, 2017

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

Godin added some commits Jan 3, 2017

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Jan 3, 2017

Member

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

Member

marchof commented Jan 3, 2017

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

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Jan 3, 2017

Member

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

Member

Godin commented Jan 3, 2017

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

@Godin Godin requested a review from marchof Jan 3, 2017

@marchof

LGTM now, thanks!

@marchof

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

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Jan 4, 2017

Member

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

Member

marchof commented Jan 4, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Jan 4, 2017

Member

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

Member

Godin commented Jan 4, 2017

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

@Godin Godin merged commit ed97906 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-177 branch Jan 4, 2017

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