jboss-javassist / javassist

Java bytecode engineering toolkit
www.javassist.org
Other
4.12k stars 698 forks source link

`Inconsistent stackmap frames` issue while instrumenting Kotlin anonymous class #275

Open serkan-ozal opened 5 years ago

serkan-ozal commented 5 years ago

Hi @chibash ,

https://github.com/serkan-ozal/javassist-issue-report/blob/master/src/main/java/ozal/serkan/javassist/issue/report/RunnerInstrumentation.java reproduces the following issue:

Exception in thread "main" java.lang.VerifyError: Inconsistent stackmap frames at branch target 170
Exception Details:
  Location:
    ozal/serkan/javassist/issue/report/Runner$run$result$1.invokeSuspend(Ljava/lang/Object;)Ljava/lang/Object; @98: goto
  Reason:
    Current frame's stack size doesn't match stackmap.
  Current Frame:
    bci: @98
    flags: { }
    locals: { 'ozal/serkan/javassist/issue/report/Runner$run$result$1', 'java/lang/Object', 'kotlinx/coroutines/CoroutineScope', top, top, 'java/lang/Object' }
    stack: { 'java/lang/Object', 'java/lang/Object' }
  Stackmap Frame:
    bci: @170
    flags: { }
    locals: { 'ozal/serkan/javassist/issue/report/Runner$run$result$1', 'java/lang/Object', top, top, top, 'java/lang/Object' }
    stack: { 'java/lang/Object' }
  Bytecode:
    0x0000000: b200 b812 bab6 0062 b800 203a 052a b400
    0x0000010: 24aa 0000 0000 008f 0000 0000 0000 0001
    0x0000020: 0000 0017 0000 0054 2b59 c100 2699 000a
    0x0000030: c000 26b4 002a bf57 2ab4 002c 4d00 b200
    0x0000040: 320a b600 36bb 0038 592a 01b7 003c c000
    0x0000050: 072a 2a04 b500 24b8 0042 5919 05a6 0019
    0x0000060: 1905 a700 482b 59c1 0026 9900 0ac0 0026
    0x0000070: b400 2abf 572b c000 444e a700 223a 04b2
    0x0000080: 004a bb00 4c59 b700 4f12 51b6 0055 1904
    0x0000090: b600 58b6 005c b600 6212 644e 2da7 000d
    0x00000a0: bb00 6659 1268 b700 6abf 3a07 b200 b812
    0x00000b0: bcb6 0062 1907 b0                      
  Exception Handler Table:
    bci [61, 90] => handler: 125
    bci [101, 122] => handler: 125
  Stackmap Table:
    full_frame(@40,{Object[#2],Object[#113],Top,Top,Top,Object[#113]},{})
    same_locals_1_stack_item_frame(@55,Object[#113])
    same_frame(@101)
    same_locals_1_stack_item_frame(@116,Object[#113])
    full_frame(@118,{Object[#2],Object[#113],Top,Top,Top,Object[#113]},{Object[#113]})
    same_locals_1_stack_item_frame(@125,Object[#26])
    full_frame(@156,{Object[#2],Object[#113],Top,Object[#68],Top,Object[#113]},{})
    full_frame(@160,{Object[#2],Object[#113],Top,Top,Top,Object[#113]},{})
    full_frame(@170,{Object[#2],Object[#113],Top,Top,Top,Object[#113]},{Object[#113]})

    at ozal.serkan.javassist.issue.report.Runner.run(Runner.kt:19)
    at ozal.serkan.javassist.issue.report.RunnerInstrumentation.main(RunnerInstrumentation.java:35)

The problem is that while instrumenting ozal.serkan.javassist.issue.report.Runner$run$result$1 anonymous Kotlin class, the stackmap inconsistency raises.

chibash commented 5 years ago

Hi,

I cannot investigate this problem without the class file (or disassembled dump) for ozal.serkan.javassist.issue.report.Runner$run$result$1

serkan-ozal commented 5 years ago

Hi @chibash ,

I have added all the class files which you can find here: https://github.com/serkan-ozal/javassist-issue-report/tree/master/target/classes/ozal/serkan/javassist/issue/report

By the way, Javassist version is 3.25.0-GA

chibash commented 5 years ago

Can you insert ctClass.debugWriteFile('./debug') before Line 35

Class clazz = ctClass.toClass();

and upload the generated file under ./debug? The generated file is the bytecode after the instrumentation. You may have to create ./debug before running the code.

serkan-ozal commented 5 years ago

Hi @chibash

Here it is: https://github.com/serkan-ozal/javassist-issue-report/blob/master/debug/ozal/serkan/javassist/issue/report/Runner%24run%24result%241.class

chibash commented 5 years ago

It seems that the Kotlin compiler generates somewhat nasty bytecode; it pushes an unused value onto the stack. This seems to confuse Javassist. Fixing it will take time...

serkan-ozal commented 5 years ago

Hi @chibash Is there any ETA for the fix?

serkan-ozal commented 5 years ago

Hi @chibash , Any update on this issue?

chibash commented 5 years ago

I'm working...

chibash commented 5 years ago

Can you try master:HEAD? I've added insertAfter(String,boolean,boolean). You must replace insertAfter(String) with insertAfter(soruce_code,false,true). The third argument true should generate the byte code compatible with Kotlin's.

The second argument false specifies the code does not work as a finally clause. This has nothing to do with your bug.

serkan-ozal commented 5 years ago

Hi @chibash ,

JvstTest5.testRedundantInsertAfter fails on my local with JDK 11.0.3.

testRedundantInsertAfter(javassist.JvstTest5)  Time elapsed: 0.008 sec  <<< FAILURE!
junit.framework.AssertionFailedError: expected:<1> but was:<93>
    at junit.framework.Assert.fail(Assert.java:57)
    at junit.framework.Assert.failNotEquals(Assert.java:329)
    at junit.framework.Assert.assertEquals(Assert.java:78)
    at junit.framework.Assert.assertEquals(Assert.java:234)
    at junit.framework.Assert.assertEquals(Assert.java:241)
    at junit.framework.TestCase.assertEquals(TestCase.java:409)
    at javassist.JvstTest5.testRedundantInsertAfter(JvstTest5.java:559)
serkan-ozal commented 5 years ago

@chibash By the way, issue seems to be fixed with your latest commit but as I said the JvstTest5.testRedundantInsertAfter fails as mentioned above

serkan-ozal commented 5 years ago

@chibash

Additionally, isn't it better to check whether or not the target class is Kotlin class on Javassist side?

I think, the code snipped shown above can be used in the insertAftermethod internally (without requiring 3rd argument for specifying generating the byte code compatible with Kotlin) to decide whether or not it is Kotlin class for injecting redundany byte code.

static boolean isKotlinClass(CtClass clazz) {
        return clazz.hasAnnotation("kotlin.Metadata");
}
chibash commented 5 years ago

Checking kotlin.Metadata seems a good idea. Can you give us a link to the official document (or so) about that annotation?

serkan-ozal commented 5 years ago

@chibash Here it is: https://kotlinlang.org/api/latest/jvm/stdlib/kotlin/-metadata/index.html

Same approach is also used by Spring framework: https://github.com/spring-projects/spring-framework/pull/1060/files#diff-244315b45582a5b7573a15644b16c373R48

serkan-ozal commented 5 years ago

@chibash I have sent this one: https://github.com/jboss-javassist/javassist/pull/276

serkan-ozal commented 5 years ago

@chibash By the way, as I said JvstTest5.testRedundantInsertAfter still fails on my local:

testRedundantInsertAfter(javassist.JvstTest5)  Time elapsed: 0.008 sec  <<< FAILURE!
junit.framework.AssertionFailedError: expected:<1> but was:<93>
    at junit.framework.Assert.fail(Assert.java:57)
    at junit.framework.Assert.failNotEquals(Assert.java:329)
    at junit.framework.Assert.assertEquals(Assert.java:78)
    at junit.framework.Assert.assertEquals(Assert.java:234)
    at junit.framework.Assert.assertEquals(Assert.java:241)
    at junit.framework.TestCase.assertEquals(TestCase.java:409)
    at javassist.JvstTest5.testRedundantInsertAfter(JvstTest5.java:559)
chibash commented 5 years ago

I made commit 855ca00 for #276. Please also see my comments for #276.

serkan-ozal commented 5 years ago

Thanks @chibash When you are going to release 3.26.0-GA?

chibash commented 5 years ago

I plan to release it at the end of this month.

chibash commented 5 years ago

I've just released. It'll be available from maven within several days.