puniverse / quasar

Fibers, Channels and Actors for the JVM
http://docs.paralleluniverse.co/quasar/
Other
4.56k stars 574 forks source link

Bytecode verification failure when using abstract class and inlined lambda (kotlin) #280

Closed exFalso closed 7 years ago

exFalso commented 7 years ago

This is as far as I got reducing the test case:

abstract class AbstractClass {
    @Suspendable
    abstract fun call(): Unit
}

class MyFiber(val ac: AbstractClass) : Fiber<Unit>() {
    @Suspendable
    override fun run() {
        function { ac.call() }
    }
}

class SomeClass(a: Unit)

inline fun function(block: () -> Unit) {
    SomeClass(block())
}

fun main(args: Array<String>) {
    MyFiber(object : AbstractClass() {
        @Suspendable
        override fun call() {
        }
    })
}

Running the above main produces:

Exception in thread "main" java.lang.VerifyError: Bad type on operand stack
Exception Details:
  Location:
    MyFiber.run()V @88: invokestatic
  Reason:
    Type uninitialized 55 (current frame, stack[0]) is not assignable to 'java/lang/Object'
  Current Frame:
    bci: @88
    flags: { }
    locals: { 'MyFiber', top, top, uninitialized 55, uninitialized 55, top, 'co/paralleluniverse/fibers/Stack', integer, null }
    stack: { uninitialized 55, 'co/paralleluniverse/fibers/Stack', integer }
  Bytecode:
    0x0000000: 013a 08b8 005f 593a 06c6 002a 1906 0436
    0x0000010: 07b6 0063 aa00 0000 0000 0014 0000 0001
    0x0000020: 0000 0001 0000 0052 1906 b600 679a 0006
    0x0000030: 013a 0603 3607 00bb 0013 593a 044e 2ab4
    0x0000040: 0017 1906 c600 4019 0604 06b6 006b 1906
    0x0000050: 03b8 006f 2d19 0604 b800 6f19 0419 0605
    0x0000060: b800 6f03 3607 1906 04b6 0073 c000 134e
    0x0000070: 1906 05b6 0073 c000 133a 0419 0603 b600
    0x0000080: 73c0 0019 b600 1cb2 0010 3a05 2d19 0419
    0x0000090: 05b7 0020 5719 06c6 0008 1906 b600 76b1
    0x00000a0: 59b6 007c c100 5599 0009 b600 7ca7 000d
    0x00000b0: 1906 c600 0819 06b6 0076 bf            
  Exception Handler Table:
    bci [51, 160] => handler: 186
    bci [51, 160] => handler: 186
    bci [51, 160] => handler: 160
    bci [51, 160] => handler: 176
  Stackmap Table:
    full_frame(@40,{Object[#2],Top,Top,Top,Top,Top,Object[#91],Integer,Null},{})
    full_frame(@51,{Object[#2],Top,Top,Top,Top,Top,Object[#91],Top,Null},{})
    full_frame(@102,{Object[#2],Top,Top,Top,Top,Top,Object[#91],Integer,Null},{})
    same_locals_1_stack_item_frame(@132,Object[#25])
    full_frame(@159,{Object[#2],Top,Top,Top,Top,Object[#12],Object[#91],Integer,Null},{})
    full_frame(@160,{Object[#2],Top,Top,Top,Top,Top,Object[#91],Top,Null},{Object[#89]})
    same_locals_1_stack_item_frame(@176,Object[#120])
    same_locals_1_stack_item_frame(@186,Object[#120])
circlespainter commented 7 years ago

Thanks, this indeed reproduces the issue.

pron commented 7 years ago

This seems to be a tough one, as Kotlin produces some very weird code:

  protected void run();
    descriptor: ()V
    flags: ACC_PROTECTED
    Code:
      stack=3, locals=6, args_size=1
         0: nop
         1: new           #19                 // class SomeClass
         4: dup
         5: astore        4
         7: astore_3
         8: aload_0
         9: getfield      #23                 // Field ac:LAbstractClass;
        12: invokevirtual #28                 // Method AbstractClass.call:()V
        15: getstatic     #16                 // Field kotlin/Unit.INSTANCE:Lkotlin/Unit;
        18: astore        5
        20: aload_3
        21: aload         4
        23: aload         5
        25: invokespecial #32                 // Method SomeClass."<init>":(Lkotlin/Unit;)V
        28: pop
        29: return
      LocalVariableTable:
        Start  Length  Slot  Name   Signature
            8       7     1 $i$a$1$function   I
            1      28     2 $i$f$function   I
            0      30     0  this   LMyFiber;

The SomeClass instance is allocated in bci 1, but the constructor is only called in bci 25, after the suspendable call. When we want to capture the stack before the call, there is an uninitialized object there, that is basically inaccessible to any Java operation. I am not quite sure how to resolve this.

This code looks so weird that it may be a Kotlin bug.

pron commented 7 years ago

OTOH, this code:

inline fun function(block: () -> Unit) {
    val x = block()
    SomeClass(x)
}

is compiled to the more reasonable bytecode:

  protected void run();
    descriptor: ()V
    flags: ACC_PROTECTED
    Code:
      stack=3, locals=4, args_size=1
         0: nop
         1: nop
         2: aload_0
         3: getfield      #21                 // Field ac:LAbstractClass;
         6: invokevirtual #26                 // Method AbstractClass.call:()V
         9: getstatic     #16                 // Field kotlin/Unit.INSTANCE:Lkotlin/Unit;
        12: astore_2
        13: new           #28                 // class SomeClass
        16: dup
        17: aload_2
        18: invokespecial #32                 // Method SomeClass."<init>":(Lkotlin/Unit;)V
        21: pop
        22: return
      LocalVariableTable:
        Start  Length  Slot  Name   Signature
            2       7     1 $i$a$1$function   I
           13       9     2  x$iv   Lkotlin/Unit;
            1      21     3 $i$f$function   I
            0      23     0  this   LMyFiber;

Which works fine with Quasar's instrumentation.

exFalso commented 7 years ago

Interesting! Am I understanding it correctly that inlined lambda calls in argument lists are somehow expanded between the allocation and the initialisation of the outer object? This does smell like a Kotlin bug... Could a "workaround" be to move all news that don't have a corresponding init before the suspendable call to somewhere after the call (e.g. just before the init)? Although this would require some non-trivial analysis I guess. I'll raise an issue with JB

pron commented 7 years ago

I guess -- that's certainly what's happening in this case -- but I haven't taken a close look at other cases of Kotlin compilation.

In general, calls to new and to invokespecial <init> should be close together. That they're not is technically valid (in terms of the JVM specification), but weird and probably unnecessary. As the new operation triggers no user code, there is no reason to issue it so early. BTW, this is precisely why Quasar does not support blocking inside constructors, because the object is in an uninitialized state, and the JVM does not allow saving any reference to it, aside from temporary references on the operand stack -- an uninitialized object has this temporary type, uninitialized, that, as the error state, cannot be considered an Object.

exFalso commented 7 years ago

For reference: https://youtrack.jetbrains.com/issue/KT-19251

mikehearn commented 7 years ago

I think the way this is going, is that it'll actually be considered a Quasar bug.

What Kotlin is doing is - as you observe - technically valid. They have a change to the compiler that can push new closer to invokespecial, but it changes evaluation ordering and is thus in their view not backwards compatible. The issue being that new does run user code; the <clinit>. So changing the emission ordering changes when the class c'tor runs.

My guess is that for Quasar to be fully JVMS compliant in this edge case will require more assistance from the JVM side to be able to process stacks containing uninitialised classes. Kotlin may change its behaviour in future, and I guess we can opt-in to the required behaviour now, but it'd also be required for our users to set this flag and that ... would be annoying.

At the very least, can Quasar perhaps detect this scenario early and bail out with an error message explaining to the user about the Kotlin compiler flag?