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

correct icode for match with tailcall nested in method call

    Details

      Description

      Critical hit. This took me forever to minimize, but I guess I'm glad I did.

      case class Foo(x: Int)
      
      object Test {
        def bippo(result: Boolean): Boolean = result
        def bungus(m: Foo): Boolean         = bippo(m match { case Foo(2) => bungus(m) })
      
        def main(args: Array[String]): Unit = bungus(Foo(0))
      }
      
      // With -Xoldpatmat: correct behavior
      //
      // scala.MatchError: Foo(0) (of class Foo)
      //  at Test$.bungus(a.scala:5)
      //  at Test$.main(a.scala:7)
      //  at Test.main(a.scala)
      //
      // With virtpatmat:
      //
      // java.lang.VerifyError: (class: Test$, method: bungus signature: (LFoo;)Z) Inconsistent stack height 1 != 0
      //  at Test.main(a.scala)
      //  at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
      //  at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
      //  at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
      //  at java.lang.reflect.Method.invoke(Method.java:601)
      

        Activity

        Hide
        Adriaan Moors added a comment -

        thanks, that looks critically minimal

        Show
        Adriaan Moors added a comment - thanks, that looks critically minimal
        Hide
        Paul Phillips added a comment -

        Normally I'd also have pinpointed the problem and dropped off a patch, but I was just so sick of it.

        Show
        Paul Phillips added a comment - Normally I'd also have pinpointed the problem and dropped off a patch, but I was just so sick of it.
        Hide
        Adriaan Moors added a comment -

        It's probably due to the tail call in the nested labeldef.
        Last time something like this came up, Iulian&I tracked it to
        https://github.com/scala/scala/blob/master/src/compiler/scala/tools/nsc/backend/icode/GenICode.scala#L872

        That's where I'll start exploring tomorrow.

        Show
        Adriaan Moors added a comment - It's probably due to the tail call in the nested labeldef. Last time something like this came up, Iulian&I tracked it to https://github.com/scala/scala/blob/master/src/compiler/scala/tools/nsc/backend/icode/GenICode.scala#L872 That's where I'll start exploring tomorrow.
        Hide
        Miguel Garcia added a comment -

        What are the new invariants of LabelDef? Without making them explicit once and for all, this game of patching GenICode leads to exhaustion.

            def bungus(m: Foo): Boolean = {
              <synthetic> val _$this: ... = Test.this;
              _bungus(_$this: ..., m: Foo){
                Test.this.bippo({
                  case <synthetic> val x1: Foo = m;
                  case6(){
                    if (x1.ne(null))
                      if (2.==(x1.x()))
                        {
                          val x3: Int = x1.x();
                          matchEnd5(_bungus(Test.this, m))
                        }
                      else
                        case7()
                    else
                      case7()
                  };
                  case7(){
                    matchEnd5(throw new MatchError(x1))
                  };
                  matchEnd5(x: Boolean){
                    x
                  }
                })
              }
            };
        

        I'm asking because that matchEnd5(_bungus(Test.this, m)) reads like "jump to matchEnd5 after loading on the stack the result of jumping to _bungus"

        The invariants, what are the invariants.

        Show
        Miguel Garcia added a comment - What are the new invariants of LabelDef ? Without making them explicit once and for all, this game of patching GenICode leads to exhaustion. def bungus(m: Foo): Boolean = { <synthetic> val _$this: ... = Test.this; _bungus(_$this: ..., m: Foo){ Test.this.bippo({ case <synthetic> val x1: Foo = m; case6(){ if (x1.ne(null)) if (2.==(x1.x())) { val x3: Int = x1.x(); matchEnd5(_bungus(Test.this, m)) } else case7() else case7() }; case7(){ matchEnd5(throw new MatchError(x1)) }; matchEnd5(x: Boolean){ x } }) } }; I'm asking because that matchEnd5(_bungus(Test.this, m)) reads like "jump to matchEnd5 after loading on the stack the result of jumping to _bungus" The invariants, what are the invariants.
        Hide
        Adriaan Moors added a comment -

        True. We should get us some invariants.

        The labeldefs/jumps emitted by patmat are all forward jumps that "respect the nesting of the labeldefs",
        i.e., they never jump "up the nesting stack", if that makes sense.

        The exception is when tailcalls kicks in and starts transforming.

        I figured the matchEnd5 jump would be discarded at the line number in GenICode I linked to.

        In this case, you could just drop the matchEnd5 call around _bungus(..), but its argument might also be an if-then-else that only conditionally tailcalls.

        Do you have any thoughts on how to fix this properly?

        Show
        Adriaan Moors added a comment - True. We should get us some invariants. The labeldefs/jumps emitted by patmat are all forward jumps that "respect the nesting of the labeldefs", i.e., they never jump "up the nesting stack", if that makes sense. The exception is when tailcalls kicks in and starts transforming. I figured the matchEnd5 jump would be discarded at the line number in GenICode I linked to. In this case, you could just drop the matchEnd5 call around _bungus(..), but its argument might also be an if-then-else that only conditionally tailcalls. Do you have any thoughts on how to fix this properly?
        Hide
        Adriaan Moors added a comment -

        ok, figured it out I think: was a mistake in how i determine which patmat labels are in tailpos

        Show
        Adriaan Moors added a comment - ok, figured it out I think: was a mistake in how i determine which patmat labels are in tailpos
        Show
        Adriaan Moors added a comment - https://github.com/scala/scala/pull/939

          People

          • Assignee:
            Adriaan Moors
            Reporter:
            Paul Phillips
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development