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

Inconsistent stackmap frames issue while instrumenting Kotlin anonymous class #275

Open
serkan-ozal opened this issue Aug 25, 2019 · 20 comments

Comments

@serkan-ozal
Copy link

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

chibash commented Aug 27, 2019

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

serkan-ozal commented Aug 27, 2019

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

chibash commented Aug 27, 2019

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.

@chibash
Copy link
Member

chibash commented Aug 27, 2019

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

Hi @chibash
Is there any ETA for the fix?

@serkan-ozal
Copy link
Author

Hi @chibash ,
Any update on this issue?

@chibash
Copy link
Member

chibash commented Sep 18, 2019

I'm working...

@chibash
Copy link
Member

chibash commented Sep 20, 2019

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

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

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

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

chibash commented Sep 22, 2019

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

@serkan-ozal
Copy link
Author

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

@chibash I have sent this one: #276

@serkan-ozal
Copy link
Author

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

chibash commented Sep 23, 2019

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

@serkan-ozal
Copy link
Author

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

@chibash
Copy link
Member

chibash commented Sep 23, 2019

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

@chibash
Copy link
Member

chibash commented Oct 2, 2019

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

odl-github pushed a commit to opendaylight/odlparent that referenced this issue Oct 10, 2019
3.25 fixes:
- jboss-javassist/javassist#72
- jboss-javassist/javassist#241
- jboss-javassist/javassist#242
- jboss-javassist/javassist#246
- jboss-javassist/javassist#252
3.26 fixes:
- jboss-javassist/javassist#265
- jboss-javassist/javassist#270
- jboss-javassist/javassist#271
- jboss-javassist/javassist#275

Of these #270 is most important, as it fixes an issue we've seen
with powermock downstream.

Change-Id: Ib4d75d6411e71438436249a8eb9313ccf4411ca2
Signed-off-by: Robert Varga <robert.varga@pantheon.tech>
odl-github pushed a commit to opendaylight/odlparent that referenced this issue Oct 17, 2019
3.25 fixes:
- jboss-javassist/javassist#72
- jboss-javassist/javassist#241
- jboss-javassist/javassist#242
- jboss-javassist/javassist#246
- jboss-javassist/javassist#252
3.26 fixes:
- jboss-javassist/javassist#265
- jboss-javassist/javassist#270
- jboss-javassist/javassist#271
- jboss-javassist/javassist#275

Of these #270 is most important, as it fixes an issue we've seen
with powermock downstream.

Change-Id: Ib4d75d6411e71438436249a8eb9313ccf4411ca2
Signed-off-by: Robert Varga <robert.varga@pantheon.tech>
(cherry picked from commit 6a404f1)
odl-github pushed a commit to opendaylight/odlparent that referenced this issue Oct 17, 2019
3.25 fixes:
- jboss-javassist/javassist#72
- jboss-javassist/javassist#241
- jboss-javassist/javassist#242
- jboss-javassist/javassist#246
- jboss-javassist/javassist#252
3.26 fixes:
- jboss-javassist/javassist#265
- jboss-javassist/javassist#270
- jboss-javassist/javassist#271
- jboss-javassist/javassist#275

Of these #270 is most important, as it fixes an issue we've seen
with powermock downstream.

Change-Id: Ib4d75d6411e71438436249a8eb9313ccf4411ca2
Signed-off-by: Robert Varga <robert.varga@pantheon.tech>
(cherry picked from commit 6a404f1)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants