search

eclipse-aspectj / aspectj

Other
303 stars 86 forks source link

JaCoCo 0.8.8 with AspectJ #170

Closed maginseb closed 2 years ago

maginseb commented 2 years ago

I face the following problem when running tests that use both JaCoCo (0.8.8) and AspectJ LTW (1.9.9.1). 

java.lang.RuntimeException: Crashed whilst crashing with this exception: java.lang.RuntimeException: Unknown constant type 17
    at org.aspectj.weaver.bcel.BcelWeaver.weave(BcelWeaver.java:1727)
    at org.aspectj.weaver.bcel.BcelWeaver.weaveWithoutDump(BcelWeaver.java:1650)
    at org.aspectj.weaver.bcel.BcelWeaver.weaveAndNotify(BcelWeaver.java:1417)
    at org.aspectj.weaver.bcel.BcelWeaver.weave(BcelWeaver.java:1192)
    at org.aspectj.weaver.tools.WeavingAdaptor.getWovenBytes(WeavingAdaptor.java:549)
    at org.aspectj.weaver.tools.WeavingAdaptor.weaveClass(WeavingAdaptor.java:385)
    at org.aspectj.weaver.loadtime.Aj.preProcess(Aj.java:115)
    at org.aspectj.weaver.loadtime.ClassPreProcessorAgentAdapter.transform(ClassPreProcessorAgentAdapter.java:51)
    at java.instrument/java.lang.instrument.ClassFileTransformer.transform(ClassFileTransformer.java:246)
    at java.instrument/sun.instrument.TransformerManager.transform(TransformerManager.java:188)
    at java.instrument/sun.instrument.InstrumentationImpl.transform(InstrumentationImpl.java:563)
    at java.base/java.lang.ClassLoader.defineClass1(Native Method)
    at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:1017)
    at java.base/java.security.SecureClassLoader.defineClass(SecureClassLoader.java:174)
    at java.base/jdk.internal.loader.BuiltinClassLoader.defineClass(BuiltinClassLoader.java:800)
    at java.base/jdk.internal.loader.BuiltinClassLoader.findClassOnClassPathOrNull(BuiltinClassLoader.java:698)
    at java.base/jdk.internal.loader.BuiltinClassLoader.loadClassOrNull(BuiltinClassLoader.java:621)
    at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:579)
    at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:178)
    at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:522)
    at de.example.Example.sayHello_aroundBody0(Example.java:7)
    at de.example.Example$AjcClosure1.run(Example.java:1)
    at org.aspectj.runtime.reflect.JoinPointImpl.proceed(JoinPointImpl.java:179)
    at de.example.ExampleAspect.around(ExampleAspect.java:13)
    at de.example.Example.sayHello(Example.java:1)
    at de.example.ExampleTest.testHello(ExampleTest.java:11)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.base/java.lang.reflect.Method.invoke(Method.java:566)
    at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
    at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
    at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
    at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
    at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
    at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
    at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:63)
    at org.junit.runners.ParentRunner$4.run(ParentRunner.java:331)
    at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:79)
    at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:329)
    at org.junit.runners.ParentRunner.access$100(ParentRunner.java:66)
    at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:293)
    at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
    at org.junit.runners.ParentRunner.run(ParentRunner.java:413)
    at org.apache.maven.surefire.junit4.JUnit4Provider.execute(JUnit4Provider.java:365)
    at org.apache.maven.surefire.junit4.JUnit4Provider.executeWithRerun(JUnit4Provider.java:273)
    at org.apache.maven.surefire.junit4.JUnit4Provider.executeTestSet(JUnit4Provider.java:238)
    at org.apache.maven.surefire.junit4.JUnit4Provider.invoke(JUnit4Provider.java:159)
    at org.apache.maven.surefire.booter.ForkedBooter.invokeProviderInSameClassLoader(ForkedBooter.java:384)
    at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:345)
    at org.apache.maven.surefire.booter.ForkedBooter.execute(ForkedBooter.java:126)
    at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:418)
    Caused by: java.lang.RuntimeException: Unknown constant type 17
    at org.aspectj.apache.bcel.classfile.ConstantPool.constantToString(ConstantPool.java:232)
    at org.aspectj.weaver.bcel.LazyMethodGen$BodyPrinter.printInstruction(LazyMethodGen.java:777)
    at org.aspectj.weaver.bcel.LazyMethodGen$BodyPrinter.print(LazyMethodGen.java:721)
    at org.aspectj.weaver.bcel.LazyMethodGen$BodyPrinter.run(LazyMethodGen.java:649)
    at org.aspectj.weaver.bcel.LazyMethodGen.print(LazyMethodGen.java:610)
    at org.aspectj.weaver.bcel.LazyClassGen.printOne(LazyClassGen.java:917)
    at org.aspectj.weaver.bcel.LazyClassGen.print(LazyClassGen.java:876)
    at org.aspectj.weaver.bcel.LazyClassGen.toLongString(LazyClassGen.java:864)
    at org.aspectj.weaver.bcel.BcelWeaver.weave(BcelWeaver.java:1725)
    ... 53 more

I found two related bugs on this topic: https://github.com/jacoco/jacoco/issues/909 and https://github.com/eclipse/org.aspectj/issues/68

https://github.com/jacoco/jacoco/issues/909 recommends a change of order for the JaCoCo and AspectJ Java agents, so that the AspectJ agent is executed first. This, however, will prevent JaCoCo from correctly calculating the coverage for classes that are touched be the AspectJ agent.

Explanation based on the JaCoCo documentation: Jacoco uses classIds to connect coverage at runtime with the class files (JaCoCo-documentation). ClassIds are calculated based on the byte code of classes. When the bytecode of a class is modified, JaCoCo calculates an incorrect classId and the coverage of this class cannot be determined. Therefore, they recommend in their documentation: "If you use another Java agent make sure the JaCoCo agent is specified at first in the command line. This way the JaCoCo agent should see the original class files."

Example

I have built an example project that easily allows you to reproduce this issue: https://github.com/maginseb/jacoco-aspectj-error

mvn clean verify Order of the Java agents is as recommended by JaCoCo (JaCoCo first, AspectJ second) The result is the exception regarding the Unknown constant type 17

mvn clean verify -P switch-order Order of the Java agents is as recommended in https://github.com/jacoco/jacoco/issues/909 (AspectJ first, JaCoCo second) The result is a messed up code coverage. No class is covered based on the report even though the coverage should be close to 100%

kriegaex commented 2 years ago

Thanks for opening this issue and for the reproducer project (which I have not tried yet). I might be able to take a closer look during the weekend, but cannot promise. I think I might have spotted something which could be an oversight in an older (1.9.0) version of AspectJ, i.e. a possible bug. But I cannot say for sure yet, I only looked at some source code for 3 minutes.

kriegaex commented 2 years ago

I have looked into the issue briefly. It seems that there is an issue in BCEL, which we use internally in AspectJ. You may want to track the issue BCEL-362, which I have just created. There is a chance that we are doing something wrong in AspectJ, but at the moment I believe that the issue is upstream.

maginseb commented 2 years ago

Makes sense, the issue happens in the BCEL-stack. Thank you, I will track the issue, that you have created!

kriegaex commented 2 years ago

@maginseb, I have some news for you:

  1. As @aclement told me, AspectJ was forked off of BCEL a very long time ago (more than 15 years ago at least). Several optimisations have been added, and the code was manually upgraded to more recent JDK versions step by step. The AspectJ derivative does not resemble upstream BCEL very much anymore.
  2. It is still true that BCEL 6.5.0 is partially lacking support for condy (constant-dynamic), as I verified while using BCEL upon JaCoCo-enhanced files without using AspectJ at all. I might be able to fix this in BCEL, but that would not help you with AspectJ's BCEL fork, only when using upstream BCEL.
  3. I also managed to back-port some condy enhancements to my local AspectJ development version, which avoids the Unknown constant type 17 error.
  4. The above fix exposes the next problem, which is actually unrelated to condy support as such but owed to the fact that by default, the AspectJ weaver inlines @Around advice. If at the same time you also instrument your aspect code with JaCoCo, the constant-dynamic variables named $jacocoData and their initialisation methods $jacocoInit created by JaCoCo in both the aspect and target classes clash with each other because of identical names and types, which causes
    • either ArrayIndexOutOfBoundsException, if the advice code wants to write coverage data to an index greater than the array size in the target method,
    • or, if you are lucky, only messed up coverage data for coverage indices shared by the advice and the target method.

In order to fix the issue in no. 4, you have two choices, which surprisingly even work with the current AspectJ release, because luckily they both avoid touching the existing JaCoCo condy code:

maginseb commented 2 years ago

@kriegaex Thank you very much for your help. I greatly appreciate your effort. I have tried both of your recommendations, they work perfectly.

In case someone else stumbles across this issue:

I have updated my example https://github.com/maginseb/jacoco-aspectj-error It includes the recommended fixes now.

kriegaex commented 2 years ago

I locally also tested with a modified version of your original reproducer. Thanks for that, it is always easier to fix something reproducible. Besides, I have also provided a BCEL fix both in AspectJ and for BCEL itself, because independently of the solution for this particular problem, both tools had problems parsing condy byte code.