Uploaded image for project: 'Scala Programming Language'
  1. Scala Programming Language
  2. SI-5672

generate correct bytecode for switch in argument

    Details

      Description

      the optimizer generates broken bytecode for 'println(1 match

      { case x => x}

      )'

      compiler built from https://github.com/scala/scala/commit/4804efef205adbe359dd353d51ab16e1e0337dc5 or later (virtpatmat on by default)

        Attachments

          Activity

          Hide
          ureche Vlad Ureche added a comment -

          Wouldn't it make sense to diagnose the problem and fix it in case it also affects other code patterns?

          Show
          ureche Vlad Ureche added a comment - Wouldn't it make sense to diagnose the problem and fix it in case it also affects other code patterns?
          Hide
          magarcia Miguel Garcia added a comment -

          Here's the tiny switch as of -Xprint:cleanup . Besides loading the scrutinee's value onto the stack, it consists of just the default case:

              def broken(): Unit = scala.this.Predef.println({
                case <synthetic> val x1: Int = 1;
                (x1: Int) match {
                  case _ => scala.Int.box(x1)
                }
              });
          

          In more detail ( -Yshow-trees ) the argument to println looks like:

            Block( // tree.tpe=Object
              ValDef( // case val x1: Int
                case <synthetic> <triedcooking>
                "x1"
                <tpt> // tree.tpe=Int
                1
              )
              Match( // tree.tpe=Object
                Typed( // tree.tpe=Int
                  "x1" // case val x1: Int, tree.tpe=Int
                  <tpt> // tree.tpe=Int
                )
                CaseDef( // tree.tpe=Object
                  "_" // tree.tpe=Int
                  Apply( // def box(x: Int): Integer in object Int, tree.tpe=Object
                    "scala"."Int"."box" // def box(x: Int): Integer in object Int, tree.tpe=(x: Int)Integer
                    "x1" // case val x1: Int, tree.tpe=Int
                  )
                )
              )
            )
          

          The VerifyError manifests only with -Yinline , although nothing gets inlined (confirm with -Ylog:inline ). Instead, malformed code emerges from IMethod.normalize() . Here's before:

            def broken(): Unit {
            locals: value x1
            startBlock: 1
            blocks: [1,2,3]
            
            1: 
              3	LOAD_MODULE object Predef
              3	CONSTANT(1)
              3	STORE_LOCAL(value x1)
              3	SCOPE_ENTER value x1
              3	LOAD_LOCAL(value x1)
              3	SWITCH ...
              
            3: 
              3	LOAD_LOCAL(value x1)
              3	BOX INT
              3	JUMP 2
              
            2: 
              3	CALL_METHOD scala.Predef.println (dynamic)
              3	RETURN(UNIT)
              
            }
          

          And here's after. There's a LOAD_LOCAL too many, because the SWITCH instruction was simply elided as if it were an unconditional jump and the scrutinee was left on the stack.

            // methods
            def broken(): Unit {
            locals: value x1
            startBlock: 1
            blocks: [1]
            
            1: 
              3	LOAD_MODULE object Predef
              3	CONSTANT(1)
              3	STORE_LOCAL(value x1)
              3	SCOPE_ENTER value x1
              3	LOAD_LOCAL(value x1)
              3	LOAD_LOCAL(value x1)
              3	BOX INT
              3	CALL_METHOD scala.Predef.println (dynamic)
              3	RETURN(UNIT)
              
            }
          

          An assertion in IMethod.normalize() would have saved the day: bb.removeLastInstruction works under the assumption that bb.lastInstruction.isInstanceOf[JUMP] . The following covers corner cases:

              val lastInstr = bb.lastInstruction
              /* Ticket SI-5672
               * Besides removing the control-flow instruction at the end of `bb` (usually a JUMP), we have to pop any values it pushes.
               * Examples:
               *   `SWITCH` consisting of just the default case, or
               *   `CJUMP(targetBlock, targetBlock, _, _)` ie where success and failure targets coincide (consumes two stack values).
               */
              val oldTKs = lastInstr.consumedTypes
              assert(lastInstr.consumed == oldTKs.size, "Someone forgot to override consumedTypes() in " +  lastInstr)
           
                bb.removeLastInstruction
                for(tk <- oldTKs.reverse) { bb.emit(DROP(tk), lastInstr.pos) }
                succ.toList foreach { i => bb.emit(i, i.pos) }
                code.removeBlock(succ)
                exh foreach { e => e.covered = e.covered - succ }
           
              nextBlock -= bb
          

          The end result after -optimize is clutter free:

          public void broken();
            Code:
             Stack=2, Locals=1, Args_size=1
             0:   getstatic       #20; //Field scala/Predef$.MODULE$:Lscala/Predef$;
             3:   iconst_1
             4:   invokestatic    #27; //Method scala/runtime/BoxesRunTime.boxToInteger:(I)Ljava/lang/Integer;
             7:   invokevirtual   #31; //Method scala/Predef$.println:(Ljava/lang/Object;)V
             10:  return
          

          Show
          magarcia Miguel Garcia added a comment - Here's the tiny switch as of -Xprint:cleanup . Besides loading the scrutinee's value onto the stack, it consists of just the default case: def broken(): Unit = scala.this.Predef.println({ case <synthetic> val x1: Int = 1; (x1: Int) match { case _ => scala.Int.box(x1) } }); In more detail ( -Yshow-trees ) the argument to println looks like: Block( // tree.tpe=Object ValDef( // case val x1: Int case <synthetic> <triedcooking> "x1" <tpt> // tree.tpe=Int 1 ) Match( // tree.tpe=Object Typed( // tree.tpe=Int "x1" // case val x1: Int, tree.tpe=Int <tpt> // tree.tpe=Int ) CaseDef( // tree.tpe=Object "_" // tree.tpe=Int Apply( // def box(x: Int): Integer in object Int, tree.tpe=Object "scala"."Int"."box" // def box(x: Int): Integer in object Int, tree.tpe=(x: Int)Integer "x1" // case val x1: Int, tree.tpe=Int ) ) ) ) The VerifyError manifests only with -Yinline , although nothing gets inlined (confirm with -Ylog:inline ). Instead, malformed code emerges from IMethod.normalize() . Here's before: def broken(): Unit { locals: value x1 startBlock: 1 blocks: [1,2,3] 1: 3 LOAD_MODULE object Predef 3 CONSTANT(1) 3 STORE_LOCAL(value x1) 3 SCOPE_ENTER value x1 3 LOAD_LOCAL(value x1) 3 SWITCH ... 3: 3 LOAD_LOCAL(value x1) 3 BOX INT 3 JUMP 2 2: 3 CALL_METHOD scala.Predef.println (dynamic) 3 RETURN(UNIT) } And here's after. There's a LOAD_LOCAL too many, because the SWITCH instruction was simply elided as if it were an unconditional jump and the scrutinee was left on the stack. // methods def broken(): Unit { locals: value x1 startBlock: 1 blocks: [1] 1: 3 LOAD_MODULE object Predef 3 CONSTANT(1) 3 STORE_LOCAL(value x1) 3 SCOPE_ENTER value x1 3 LOAD_LOCAL(value x1) 3 LOAD_LOCAL(value x1) 3 BOX INT 3 CALL_METHOD scala.Predef.println (dynamic) 3 RETURN(UNIT) } An assertion in IMethod.normalize() would have saved the day: bb.removeLastInstruction works under the assumption that bb.lastInstruction.isInstanceOf[JUMP] . The following covers corner cases: val lastInstr = bb.lastInstruction /* Ticket SI-5672 * Besides removing the control-flow instruction at the end of `bb` (usually a JUMP), we have to pop any values it pushes. * Examples: * `SWITCH` consisting of just the default case, or * `CJUMP(targetBlock, targetBlock, _, _)` ie where success and failure targets coincide (consumes two stack values). */ val oldTKs = lastInstr.consumedTypes assert(lastInstr.consumed == oldTKs.size, "Someone forgot to override consumedTypes() in " + lastInstr)   bb.removeLastInstruction for(tk <- oldTKs.reverse) { bb.emit(DROP(tk), lastInstr.pos) } succ.toList foreach { i => bb.emit(i, i.pos) } code.removeBlock(succ) exh foreach { e => e.covered = e.covered - succ }   nextBlock -= bb The end result after -optimize is clutter free: public void broken(); Code: Stack=2, Locals=1, Args_size=1 0: getstatic #20; //Field scala/Predef$.MODULE$:Lscala/Predef$; 3: iconst_1 4: invokestatic #27; //Method scala/runtime/BoxesRunTime.boxToInteger:(I)Ljava/lang/Integer; 7: invokevirtual #31; //Method scala/Predef$.println:(Ljava/lang/Object;)V 10: return
          Hide
          ureche Vlad Ureche added a comment -

          Great work Miguel! Thanks a lot for looking into this!

          Show
          ureche Vlad Ureche added a comment - Great work Miguel! Thanks a lot for looking into this!
          Hide
          magarcia Miguel Garcia added a comment -

          Pull request with fix at https://github.com/scala/scala/pull/596

          Show
          magarcia Miguel Garcia added a comment - Pull request with fix at https://github.com/scala/scala/pull/596
          Hide
          magarcia Miguel Garcia added a comment -

          Fixed in 3eb0245cd

          Show
          magarcia Miguel Garcia added a comment - Fixed in 3eb0245cd

            People

            • Assignee:
              magarcia Miguel Garcia
              Reporter:
              moors Adriaan Moors
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: