Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

correct icode for match with tailcall nested in method call #6089

Closed
scabug opened this issue Jul 16, 2012 · 8 comments
Closed

correct icode for match with tailcall nested in method call #6089

scabug opened this issue Jul 16, 2012 · 8 comments
Assignees
Milestone

Comments

@scabug
Copy link

@scabug scabug commented Jul 16, 2012

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)
@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 16, 2012

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 16, 2012

@adriaanm said:
thanks, that looks critically minimal

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 16, 2012

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

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 16, 2012

@adriaanm said:
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.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 17, 2012

@magarciaEPFL said:
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.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 17, 2012

@adriaanm said:
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?

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 17, 2012

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

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 18, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.