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

match in finally&static init causes VerifyError ("register 16 contains wrong type") #5929

Closed
scabug opened this issue Jun 15, 2012 · 23 comments

Comments

@scabug
Copy link

scabug commented Jun 15, 2012

I do not know how to boil this down to a small example, unfortunately. This happens when I try and compile ScalaTest under Scala 2.10.0-M4. I'm not sure this is a Scala 2.10 issues. May be some other fluke, but here's the stack trace and instructions on how to reproduce it:

runtest:
[scalatest] *** RUN ABORTED *** (21 seconds, 76 milliseconds)
[scalatest] java.lang.VerifyError: (class: org/scalatest/BeforeAndAfterEachFunctions$class, method: runTest signature: (Lorg/scalatest/BeforeAndAfterEachFunctions;Ljava/lang/String;Lorg/scalatest/Reporter;Lorg/scalatest/Stopper;Lscala/collection/immutable/Map;Lorg/scalatest/Tracker;)V) Register 16 contains wrong type
[scalatest] at org.scalatest.BeforeAndAfterFunctionsExtendingSuite.(BeforeAndAfterFunctionsSuite.scala:415)
[scalatest] at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
[scalatest] at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:39)
[scalatest] at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:27)
[scalatest] at java.lang.reflect.Constructor.newInstance(Constructor.java:513)
[scalatest] at java.lang.Class.newInstance0(Class.java:355)
[scalatest] at java.lang.Class.newInstance(Class.java:308)
[scalatest] at org.scalatest.tools.DiscoverySuite$$anonfun$1.apply(DiscoverySuite.scala:40)
[scalatest] at org.scalatest.tools.DiscoverySuite$$anonfun$1.apply(DiscoverySuite.scala:34)
[scalatest] at scala.collection.TraversableLike$$anonfun$map$1.apply(TraversableLike.scala:239)
[scalatest] ...

To (attempt to) reproduce it, check out this branch:

https://scalatest.googlecode.com/svn/branches/r18for210M4

And type:

ant compile; ant gencode; ant test

@scabug
Copy link
Author

scabug commented Jun 15, 2012

Imported From: https://issues.scala-lang.org/browse/SI-5929?orig=1
Reporter: @bvenners
Affected Versions: 2.10.0

@scabug
Copy link
Author

scabug commented Jun 15, 2012

@paulp said:
I immediately notice it downloading this:

org.scalacheck:scalacheck_2.10.0-M3:1.10-SNAPSHOT

Why would it be downloading that to build against M4.

@scabug
Copy link
Author

scabug commented Jun 15, 2012

@retronym said:
Minimized:

object test {
  null match {
    case _ =>
  }

  try {
    ()
  } finally {
    null match {
      case _ =>
    }
  }
}

@scabug
Copy link
Author

scabug commented Jun 15, 2012

@paulp said:
Jason's minimization fails in M3 as well, but works in M2 and 2.9.2.

@scabug
Copy link
Author

scabug commented Jun 15, 2012

@retronym said:
Works with -Xoldpatmat; assigning to Adriaan.

Highlights of the voyage:

[[syntax trees at end of              constructors]] // scalatest-bug.scala
package <empty> {
  object test extends Object {
    def <init>(): ... = {
      test.super.<init>();
      {
        case <synthetic> val x1: Null = null;
        case3(){
          matchEnd2(scala.runtime.BoxedUnit.UNIT)
        };
        matchEnd2(x: scala.runtime.BoxedUnit){
          ()
        }
      };
      try {
        ()
      } finally {
        case <synthetic> val x1: Null = null;
        case3(){
          matchEnd2(scala.runtime.BoxedUnit.UNIT)
        };
        matchEnd2(x: scala.runtime.BoxedUnit){
          ()
        }
      };
      ()
    }
  }
}

Bytecode:

private test$();
  Code:
   Stack=1, Locals=6, Args_size=1
   0:	aload_0
   1:	invokespecial	#16; //Method java/lang/Object."<init>":()V
   4:	aload_0
   5:	putstatic	#18; //Field MODULE$:Ltest$;
   8:	aconst_null
   9:	astore_1
   10:	getstatic	#24; //Field scala/runtime/BoxedUnit.UNIT:Lscala/runtime/BoxedUnit;
   13:	astore_2
   14:	aconst_null
   15:	astore	5
   17:	getstatic	#24; //Field scala/runtime/BoxedUnit.UNIT:Lscala/runtime/BoxedUnit;
   20:	astore_3
   21:	goto	33
   24:	astore	4
   26:	aconst_null
   27:	astore	5
   29:	getstatic	#24; //Field scala/runtime/BoxedUnit.UNIT:Lscala/runtime/BoxedUnit;
   32:	astore_3
   33:	aload	4
   35:	athrow

@scabug
Copy link
Author

scabug commented Jun 15, 2012

@bvenners said:
Hi Paul, The reason is there was no ScalaCheck build against M4 at the time. I just checked and there still isn't. I went with the most recent. That could potentially be the cause of the runtime error I'm seeing. - Bill

@scabug
Copy link
Author

scabug commented Jun 15, 2012

@paulp said:
It could have been, and in the future you'll probably want to point out that you're using incompatible jars sooner than later, but that isn't the cause as can be seen in jason's independent reproduction.

@scabug
Copy link
Author

scabug commented Jun 16, 2012

@retronym said:
A variation on the theme:

class testS {
  null match {
    case _ => ""
  }

  try {
    ""
  } finally {
    val x = null match {
      case _ => ""
    }
    ()
  }
}

        class testS extends Object {
          def <init>(): testS = {
            testS.super.<init>();
            {
              case <synthetic> val x1: Null = null;
              case3(){
                matchEnd2("")
              };
              matchEnd2(x: String){
                x
              }
            };
            try {
              ""
            } finally {
              val x: String = {
                case <synthetic> val x1: Null = null;
                case3(){
                  matchEnd2("")
                };
                matchEnd2(x: String){
                  x
                }
              };
              ()
            };
            ()
          }
        };


  Code:
   Stack=2, Locals=7, Args_size=1
   0:	aload_0
   1:	invokespecial	#10; //Method java/lang/Object."<init>":()V
   4:	aconst_null
   5:	astore_1
   6:	ldc	#12; //String 
   8:	astore_2
   9:	aload_2
   10:	pop
   11:	ldc	#12; //String 
   13:	aconst_null
   14:	astore	6
   16:	ldc	#12; //String 
   18:	astore_3
   19:	goto	30
   22:	astore	4
   24:	aconst_null
   25:	astore	6
   27:	ldc	#12; //String 
   29:	astore_3
   30:	aload_3
   31:	astore	5
   33:	aload	4
   35:	athrow
  Exception table:
   from   to  target type
    11    13    22   any

java.lang.VerifyError: (class: testS, method: <init> signature: ()V) Inconsistent stack height 0 != 1
	at .<init>(<console>:9)
java.lang.VerifyError: (class: testS, method: <init> signature: ()V) Inconsistent stack height 0 != 1

@scabug
Copy link
Author

scabug commented Jun 28, 2012

@adriaanm said:
I'm going to say this is a bug in GenICode's implementation of finally blocks.

I improved the pattern matcher a bit to avoid emitting all these BoxedUnits,
and then the diff between -optimize and -optimize -Xoldpatmat becomes

new pattern matcher:

   0:	aload_0
   1:	invokespecial	#10; //Method java/lang/Object."<init>":()V
   4:	goto	8 // this should be a return! (it's completely wrong to jump to 8)
   7:	astore_1
   8:	aload_1
   9:	athrow

correct bytecode:

   0:	aload_0
   1:	invokespecial	#10; //Method java/lang/Object."<init>":()V
   4:	return
   5:	astore_1
   6:	aload_1
   7:	athrow

test code (bytecode above is for testIncompatStackHeight)

class testIncompatStackHeight {
  null match { case _ => }

  try { "" } finally { null match { case _ => } }
}

class testExpectedObjectRef {
  null match { case _ => }

  try { } finally { null match { case _ => } }
}

@scabug
Copy link
Author

scabug commented Jun 28, 2012

@adriaanm said (edited on Jun 28, 2012 1:53:25 PM UTC):
I'd say it's basically #2850 reincarnated
(for easy reference, that one was fixed by scala/scala@57f14277da)

@scabug
Copy link
Author

scabug commented Jun 28, 2012

@paulp said:
It is similar. What's happening in this one is that there are two different "val x1"s created by the pattern matcher. One is declared in and should be scoped in the finally block, but instead we land with two x1s owned by the constructor. At least that's what I see.

@scabug
Copy link
Author

scabug commented Jun 28, 2012

@adriaanm said:
interesting! so I can't get rid of the first match, but this also triggers it:

  def x = {
    null match { case _ => }

    try { 1 } finally { while(true) { } }
  }

@scabug
Copy link
Author

scabug commented Jun 28, 2012

@paulp said:
Huh. That one diversifies the situation. Notice that yours creates a label for "while$11", now that's an interestingly wrong appearing mangle.

[log icode] Added (method while$11,method while$11) to labels.

Why is the label parameter owned by NoSymbol I guess? "def x" renamed to "def quux" for clarity:

  Local(value x1,REF(class Null),arg=false) with s/e/r (0,0,) in method quux->class A->package <empty>
  Local(value x,REF(class BoxedUnit),arg=false) with s/e/r (0,0,) in 
  Local(variable exc1,REF(class Throwable),arg=false) with s/e/r (0,0,) in method quux->class A->package <empty>

@scabug
Copy link
Author

scabug commented Jun 28, 2012

@paulp said:
Mmm, I see, it is the same thing because every label is being duplicated for the finally block, even the ones which don't come into scope until the finally block.

@scabug
Copy link
Author

scabug commented Jun 28, 2012

@dragos said:
You need to have a forward jump in order to trigger the bug.

A forward jump triggers a 'scanForLabels' call, trying to find where the target of that jump is, and create a label. Later on, when the LabelDef is generated, that label is anchored and assigned an offset (or basic block number).

scanForLabel will blindly add all LabelDefs it can find, including the ones in the finally handler. Those shouldn't be added, because they will be re-freshed when the finalizer is duplicated. When the time comes for the finalizer to be generated, since all labels have been 'scanned', they apear to be bound when the duplicator runs, so they don't get fresh symbols. And that's why the jump in the inlined finalizer will end up on the wrong side of the fence.

@scabug
Copy link
Author

scabug commented Jun 28, 2012

@dragos said (edited on Jun 28, 2012 3:33:15 PM UTC):
The only reason why scanForLabels exists is to find the symbols for the label parameters. At the time this code was written, they were only in the tree, but now MethodTypes have them as well. So we can get rid of the whole scanForLabels, and create a lazy Label at the point of use. It will likely speed up the compiler a bit, if there are lots of forward jumps in large methods.

@scabug
Copy link
Author

scabug commented Jun 28, 2012

@paulp said:
Thanks a lot iulian, that was enough for me to fix it.

@scabug
Copy link
Author

scabug commented Jun 28, 2012

@paulp said:
I guess I'll take a vacation from vacation and fix this one.

@scabug
Copy link
Author

scabug commented Jun 28, 2012

@dragos said:
I tried to fix it as well, and implemented the solution I described. It fixes this case, but fails while compiling Predef.scala (genericArayOps).

The invariant I was counting on was that, for all LabelDefs, ldef.symbol.info.params == ldef.params.map(_.symbol). So the symbols in the LabelDef tree (a list of Idents with symbols), are the same as the ones in the method type of the label symbol. It's not always the case :(

I'll stop now. :)

@scabug
Copy link
Author

scabug commented Jun 28, 2012

@dragos said:
My semi-futile effort: https://github.com/dragos/scala/tree/issue/fix-5929

@scabug
Copy link
Author

scabug commented Jun 28, 2012

@paulp said:
Oh good, hopefully I'll have better luck. Would anyone like to comment on the correctness of this just-written-and-untested piece of machinery?

  /** A foreach traverser which prunes anytime a new scope would be
   *  opened - that is, it doesn't recurse into Blocks, Templates,
   *  or catch/finally blocks.  (Am I missing anything?)
   *  You might use this if you wanted to traverse the statements of
   *  a DefDef body without running into trees which are not in scope
   *  for the whole body.
   */
  class ForeachTreeInScopeTraverser(f: Tree => Unit) extends Traverser {
    override def traverse(t: Tree) {
      f(t)
      t match {
        case Try(body, _, _)        => traverse(body)
        case _: Template | _: Block => // prune
        case _                      => super.traverse(t)
      }
    }
  }

@scabug
Copy link
Author

scabug commented Jun 28, 2012

@paulp said:
I withdraw my naive optimism. You guys will like what I'm really working on though, I promise.

@scabug
Copy link
Author

scabug commented Jul 2, 2012

@adriaanm said:
scala/scala#809

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

No branches or pull requests

2 participants