Scala Programming Language
  1. Scala Programming Language
  2. SI-6863

UnCurry forgets to liftTree, causing java.lang.VerifyError down the road

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: Scala 2.10.0-RC5, Scala 2.10.0, Scala 2.11.0
    • Fix Version/s: Scala 2.10.1
    • Component/s: Misc Compiler
    • Labels:

      Description

      I'm trying to use a modified version of https://github.com/romix/akka-kryo-serialization with Scala 2.10.0-RC5. It compiles fine (without -optimise, with the flag it crashes, but that's a different issue), but then at class load-time I get the following error:

      Could not run test org.hyflow.KryoTest: java.lang.VerifyError: (class: com/romix/scala/serialization/kryo/ScalaSetSerializer, method: create signature: (Lcom/esotericsoftware/kryo/Kryo;Lcom/esotericsoftware/kryo/io/Input;Ljava/lang/Class; )Lscala/collection/Set; ) Inconsistent stack height 1 != 3

      Even Soot refuses to load the class properly, and gives the following:

      Considering 111: invokenonvirtual[111]
      Exception in thread "main" java.lang.RuntimeException: TypeStack merging failed; unequal stack lengths: 1 and 3
      at soot.coffi.TypeStack.merge(TypeStack.java:137)
      at soot.coffi.CFG.jimplify(CFG.java:1196)
      at soot.coffi.CFG.jimplify(CFG.java:955)
      ...

      The code that causes the problem appears to be at the end of this block, as the three branches are collected into the final result (line labeled 111 in the 2.10 bytecode file):

      var coll: Map[Any, Any] = 
      if(classOf[SortedMap[_,_]].isAssignableFrom(typ)) {
      // Read ordering and set it for this collection 
      implicit val mapOrdering = kryo.readClassAndObject(input).asInstanceOf[scala.math.Ordering[Any]]
      	try typ.getDeclaredConstructor(classOf[scala.math.Ordering[_]]).newInstance(mapOrdering).asInstanceOf[Map[Any,Any]].empty 
      	catch { case _ => kryo.newInstance(typ).asInstanceOf[Map[Any,Any]].empty }
      } else {
      	kryo.newInstance(typ).asInstanceOf[Map[Any,Any]].empty
      }
      

      This code worked on 2.9.1, and it appears the generated bytecode is indeed different. I've attached the source code excerpt and disassembled bytecode for 2.9.1 and 2.10.0-RC5, and highlighted the line with the problem in the 2.10 bytecode.

      1. scala-2.10.txt
        7 kB
        Alex Turcu
      2. scala-2.9.scala
        1 kB
        Alex Turcu
      3. scala-2.9.shimp
        20 kB
        Alex Turcu
      4. scala-2.9.txt
        5 kB
        Alex Turcu

        Activity

        Hide
        Adriaan Moors added a comment -

        Based on a meatspace discussion with James, I agree we should pursue Miguel's factory approach for 2.11 and try to implement James's extra-variable fix for 2.10.1-RC1.

        Show
        Adriaan Moors added a comment - Based on a meatspace discussion with James, I agree we should pursue Miguel's factory approach for 2.11 and try to implement James's extra-variable fix for 2.10.1-RC1.
        Hide
        Miguel Garcia added a comment -

        The extra-variable is actually lightweight, and apparently LambdaLift isn't too late to apply it (I'm guessing here). Perhaps detecting new Foo(try

        {...}catch{...}

        ) during LambdaLift is also amenable to a local rewriting

        var temp = try {} catch {}
        new Foo(temp)
        

        I agree getting this fixed first in a binary-compatible way is the best for now.

        Show
        Miguel Garcia added a comment - The extra-variable is actually lightweight, and apparently LambdaLift isn't too late to apply it (I'm guessing here). Perhaps detecting new Foo(try {...}catch{...} ) during LambdaLift is also amenable to a local rewriting var temp = try {} catch {} new Foo(temp) I agree getting this fixed first in a binary-compatible way is the best for now.
        Hide
        James Iry added a comment -

        It turns out to be even simpler than all this discussion. So simple I feel quite stupid for not seeing it the first time through. The original problem was that var x = try

        {...} catch {...}

        works fine but that var x = { blah; try

        {...} catch{...}

        does not. Well, in lambda lift there's a transformation that turns var x = try

        {block}

        catch

        {case blah => catchBlock}

        into var x = try

        {new Ref(block)}

        catch

        {case blah => new Ref(catchBlock)}

        . But that didn't catch the case where x was initialized from a block. The fix is easy. Just look at a block used for captured var assignment and do the same rewrite on the block's result expression. Do it recursively since we could have var x = {blah {blurg { try...}}.

        Show
        James Iry added a comment - It turns out to be even simpler than all this discussion. So simple I feel quite stupid for not seeing it the first time through. The original problem was that var x = try {...} catch {...} works fine but that var x = { blah; try {...} catch{...} does not. Well, in lambda lift there's a transformation that turns var x = try {block} catch {case blah => catchBlock} into var x = try {new Ref(block)} catch {case blah => new Ref(catchBlock)} . But that didn't catch the case where x was initialized from a block. The fix is easy. Just look at a block used for captured var assignment and do the same rewrite on the block's result expression. Do it recursively since we could have var x = {blah {blurg { try...}}.
        Hide
        James Iry added a comment -

        The previous comment turns out to be, um, wrong. Ish. We need to deal with var x = if(blah) try/catch and var x = blah match

        {case whatever => try/catch}

        . Etc. The final pull request does the "val temp = whatever; new *Ref(temp)" transformation as a last ditch approach when it can't figure things out, but try/catch, if/else, match/case, and block expressions all get a more direct translation moving the new *Ref construction to "leaf" expressions that don't have try/catch.

        The pull is: https://github.com/scala/scala/pull/1927

        A proposed alternative for 2.11+ is here: http://james-iry.blogspot.com/2013/01/scala-trycatch-lifting-proposal.html

        Show
        James Iry added a comment - The previous comment turns out to be, um, wrong. Ish. We need to deal with var x = if(blah) try/catch and var x = blah match {case whatever => try/catch} . Etc. The final pull request does the "val temp = whatever; new *Ref(temp)" transformation as a last ditch approach when it can't figure things out, but try/catch, if/else, match/case, and block expressions all get a more direct translation moving the new *Ref construction to "leaf" expressions that don't have try/catch. The pull is: https://github.com/scala/scala/pull/1927 A proposed alternative for 2.11+ is here: http://james-iry.blogspot.com/2013/01/scala-trycatch-lifting-proposal.html
        Hide
        Miguel Garcia added a comment -

        One of the sub-problems when try-catch's are left in expression positions is a post-CleanUp step to guarantee empty-stack-on-try-entry. Other backends (in particular MSIL) also stand to benefit.

        A proposal (in the form of pseudocode) for that transformation can be found at https://groups.google.com/d/msg/scala-internals/VkEL7wOVQpE/aSiNnF3ym-cJ

        Show
        Miguel Garcia added a comment - One of the sub-problems when try-catch's are left in expression positions is a post-CleanUp step to guarantee empty-stack-on-try-entry. Other backends (in particular MSIL) also stand to benefit. A proposal (in the form of pseudocode) for that transformation can be found at https://groups.google.com/d/msg/scala-internals/VkEL7wOVQpE/aSiNnF3ym-cJ

          People

          • Assignee:
            James Iry
            Reporter:
            Alex Turcu
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development