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

Optimizer incorrectly removes method invocations containing throw expressions #6188

Closed
scabug opened this issue Aug 5, 2012 · 7 comments
Closed

Comments

@scabug
Copy link

scabug commented Aug 5, 2012

When compiling the following snippet with and without -optimize, I get varying semantics and presumably wrong bytecode.

Given:

import scala.util.Success

object Test {
 def main(args: Array[String]) {
   val e = new Exception("this is an exception")
   val res = Success(1).flatMap[Int](x => throw e)
   println(res)
 }
}

The expected output is:

Failure(java.lang.Exception: this is an exception)

(This is indeed what I get when running after compiling without -optimize)

When running after compiling with -optimize, the exception is incorrectly thrown:

java.lang.Exception: this is an exception
       at Test$.main(opt-bug.scala:5)
       at Test.main(opt-bug.scala)
       at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
       at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
       at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
       at java.lang.reflect.Method.invoke(Method.java:597)
       at scala.tools.nsc.util.ScalaClassLoader$$anonfun$run$1.apply(ScalaClassLoader.scala:71)
       at scala.tools.nsc.util.ScalaClassLoader$class.asContext(ScalaClassLoader.scala:31)
       at scala.tools.nsc.util.ScalaClassLoader$URLClassLoader.asContext(ScalaClassLoader.scala:139)
       at scala.tools.nsc.util.ScalaClassLoader$class.run(ScalaClassLoader.scala:71)
       at scala.tools.nsc.util.ScalaClassLoader$URLClassLoader.run(ScalaClassLoader.scala:139)
       at scala.tools.nsc.CommonRunner$class.run(ObjectRunner.scala:28)
       at scala.tools.nsc.ObjectRunner$.run(ObjectRunner.scala:45)
       at scala.tools.nsc.CommonRunner$class.runAndCatch(ObjectRunner.scala:35)
       at scala.tools.nsc.ObjectRunner$.runAndCatch(ObjectRunner.scala:45)
       at scala.tools.nsc.MainGenericRunner.runTarget$1(MainGenericRunner.scala:70)
       at scala.tools.nsc.MainGenericRunner.process(MainGenericRunner.scala:92)
       at scala.tools.nsc.MainGenericRunner$.main(MainGenericRunner.scala:101)
       at scala.tools.nsc.MainGenericRunner.main(MainGenericRunner.scala)

In inspecting the bytecode, it seems that the optimizer completely removes the flatMap invocation in the above snippet.

Bytecode without optimize (CORRECT):

public final class Test$ extends java.lang.Object{
public static final Test$ MODULE$;

public static {};
 Code:
  0:        new        #2; //class Test$
  3:        invokespecial        #12; //Method "<init>")V
  6:        return

public void main(java.lang.String[]);
 Code:
  0:        new        #16; //class java/lang/Exception
  3:        dup
  4:        ldc        #18; //String this is an exception
  6:        invokespecial        #21; //Method java/lang/Exception."<init>":(Ljava/lang/String;)V
  9:        astore_2
  10:        new        #23; //class scala/util/Success
  13:        dup
  14:        iconst_1
  15:        invokestatic        #29; //Method scala/runtime/BoxesRunTime.boxToInteger:(I)Ljava/lang/Integer;
  18:        invokespecial        #32; //Method scala/util/Success."<init>":(Ljava/lang/Object;)V
  21:        new        #34; //class Test$$anonfun$1
  24:        dup
  25:        aload_2
  26:        invokespecial        #37; //Method Test$$anonfun$1."<init>":(Ljava/lang/Exception;)V
  29:        invokevirtual        #41; //Method scala/util/Success.flatMap:(Lscala/Function1;)Lscala/util/Try;
  32:        astore_3
  33:        getstatic        #46; //Field scala/Predef$.MODULE$:Lscala/Predef$;
  36:        aload_3
  37:        invokevirtual        #49; //Method scala/Predef$.println:(Ljava/lang/Object;)V
  40:        return

}

Bytecode with optimize (INCORRECT):

public final class Test$ extends java.lang.Object{
public static final Test$ MODULE$;

public static {};
 Code:
  0:        new        #2; //class Test$
  3:        invokespecial        #12; //Method "<init>")V
  6:        return

public void main(java.lang.String[]);
 Code:
  0:        new        #16; //class java/lang/Exception
  3:        dup
  4:        ldc        #18; //String this is an exception
  6:        invokespecial        #21; //Method java/lang/Exception."<init>":(Ljava/lang/String;)V
  9:        astore_2
  10:        new        #23; //class scala/util/Success
  13:        dup
  14:        iconst_1
  15:        invokestatic        #29; //Method scala/runtime/BoxesRunTime.boxToInteger:(I)Ljava/lang/Integer;
  18:        invokespecial        #32; //Method scala/util/Success."<init>":(Ljava/lang/Object;)V
  21:        pop
  22:        aload_2
  23:        athrow

}

The diff between the two:

--- without-opt        2012-08-05 11:32:04.000000000 +0200
+++ with-opt        2012-08-05 11:32:17.000000000 +0200
@@ -20,16 +20,9 @@
   14:        iconst_1
   15:        invokestatic        #29; //Method scala/runtime/BoxesRunTime.boxToInteger:(I)Ljava/lang/Integer;
   18:        invokespecial        #32; //Method scala/util/Success."<init>":(Ljava/lang/Object;)V
-   21:        new        #34; //class Test$$anonfun$1
-   24:        dup
-   25:        aload_2
-   26:        invokespecial        #37; //Method Test$$anonfun$1."<init>":(Ljava/lang/Exception;)V
-   29:        invokevirtual        #41; //Method scala/util/Success.flatMap:(Lscala/Function1;)Lscala/util/Try;
-   32:        astore_3
-   33:        getstatic        #46; //Field scala/Predef$.MODULE$:Lscala/Predef$;
-   36:        aload_3
-   37:        invokevirtual        #49; //Method scala/Predef$.println:(Ljava/lang/Object;)V
-   40:        return
+   21:        pop
+   22:        aload_2
+   23:        athrow

}
@scabug
Copy link
Author

scabug commented Aug 5, 2012

Imported From: https://issues.scala-lang.org/browse/SI-6188?orig=1
Reporter: @heathermiller
Assignee: @magarciaEPFL
Other Milestones: 2.10.0-M6, 2.10.0
See #6157

@scabug
Copy link
Author

scabug commented Aug 5, 2012

@heathermiller said (edited on Aug 5, 2012 10:47:45 AM UTC):
Assuming that I'm correct and that this is purely an optimizer bug (and not anything wrong with Try), whenever this is resolved, the following test should be re-enabled:
heathermiller/scala@cb6066e#diff-1

@scabug
Copy link
Author

scabug commented Aug 5, 2012

@retronym said (edited on Aug 5, 2012 1:44:30 PM UTC):
Decoupled the test from Try.

  ~/code/scala/test/files/run/t6188 cat t6188_1.scala 
package t6188

object Bug {
  def withFilter[U](f: Any => Any): Unit =
    try f(0)
    catch {
      case e: Throwable =>
    }
}
  ~/code/scala/test/files/run/t6188 cat t6188_2.scala 
package t6188

object Test {
  def main(args: Array[String]) {
    val e = new Exception("this is an exception")
    Bug.withFilter((x) => throw e)
  }
}

  ~/code/scala/test/files/run/t6188 export SB="$HOME/code/scala/build/quick/bin" &&  $SB/scalac -optimize t6188_1.scala && $SB/scalac -optimize t6188_2.scala && $SB/scala t6188.Test
java.lang.Exception: this is an exception
	at t6188.Test$.main(t6188_2.scala:5)

The method name must be one of the monadic names, presumably to trigger Inliners#isMonadicMethod.

Separate compilation is also required.

Scala Version  Result
---------------------
2.8.0          OK
2.9.0          Compiler Crash ("java.lang.Error: Open block: 4 <dirtypreds> <dirtysuccs>")          
2.9.1          As reported in this ticket.
2.10.0-M1+     As reported in this ticket.
[log inliner] Analyzing t6188.Test.main count 1 with 7 blocks
[log inliner] shouldInline(t6188.Test$$anonfun$main$1.apply) score: 3
[log inliner] Inlining t6188.Test$$anonfun$main$1.apply in t6188.Test.main at pos: 114
[log inliner] Analyzing t6188.Test.main count 1 with 9 blocks
[log inliner] shouldInline(t6188.Test$$anonfun$main$1.apply) score: 3
[log inliner] Inlining t6188.Test$$anonfun$main$1.apply in t6188.Test.main at pos: 114
[log inliner] Analyzing t6188.Test.main count 1 with 11 blocks
[log inliner] !!! Found open but empty block while inlining t6188.Test.main: removing from block list.
[log inliner]  t6188.Test.main blocks before inlining: 1 (17) after: 3 (36)

@scabug
Copy link
Author

scabug commented Aug 5, 2012

@magarciaEPFL said:
The cause of this bug lies not so much in Inliner but in ICodeReader, which doesn't map bytecode-level exception handlers (in our case, that for Success.flatMap) to the resulting IMethod. Another bug in this category appears to be #6157.

The short term solution consists in preventing inlining of callees containing EHs. Doing so also avoids class file format errors where a try expression is inlined at a non-empty-stack program point (same thing that "lift try" in UnCurry avoids).

For reference, this is where ICodeReader meets bytecode-level EHs:

    while (i < exceptionEntries) {
      // skip start end PC
      in.skip(4)
      // read the handler PC
      code.jmpTargets += in.nextChar
      // skip the exception type
      in.skip(2)
      i += 1
    }
    skipAttributes()

@scabug
Copy link
Author

scabug commented Aug 6, 2012

@magarciaEPFL said:
scala/scala#1059

@scabug
Copy link
Author

scabug commented Aug 7, 2012

@magarciaEPFL said:
Fixed in scala/scala@61cc8ff

@scabug scabug closed this as completed Aug 7, 2012
@scabug
Copy link
Author

scabug commented Dec 13, 2012

@adriaanm said:
scala/scala#1764

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

1 participant