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)

        Activity

        Hide
        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
        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
        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
        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
        Vlad Ureche added a comment -

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

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

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

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

        Fixed in 3eb0245cd

        Show
        Miguel Garcia added a comment - Fixed in 3eb0245cd

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development