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

compiler crash with -optimize and separate compilation #9121

Closed
scabug opened this issue Jan 28, 2015 · 12 comments
Closed

compiler crash with -optimize and separate compilation #9121

scabug opened this issue Jan 28, 2015 · 12 comments

Comments

@scabug
Copy link

scabug commented Jan 28, 2015

see https://github.com/giabao/scala-bug

sorry, I don't know exactly what Component the bug is related to.

@scabug
Copy link
Author

scabug commented Jan 28, 2015

Imported From: https://issues.scala-lang.org/browse/SI-9121?orig=1
Reporter: Gia Bảo (giabao)
Affected Versions: 2.11.5, 2.11.6
Attachments:

@scabug
Copy link
Author

scabug commented Jan 30, 2015

Gia Bảo (giabao) said:

// Implicits.scala
import rx.Observable
import rx.functions.Func1
import scala.language.implicitConversions

object Implicits {
  implicit class ScalaObservable[T](val underlying: Observable[T]) extends AnyVal {
    //if remove `@inline` annotation then test will success
    @inline def scMap[R](f: T => R): Observable[R] = underlying.map[R](f.toRx)

    //if change `f.toRx` to `new SFunc1(f)` then test will success
    //@inline def scMap[R](f: T => R): Observable[R] = underlying.map[R](new SFunc1(f))
  }

  final class SFunc1[T1, R](f: T1 => R) extends Func1[T1, R] {
    def call(t1: T1): R = f(t1)
  }
  implicit class RichFunction1[T1, R](val f: T1 => R) extends AnyVal {
    def toRx = new SFunc1(f)
  }
}
// LogView.scala
import com.sandinh.couchbase.document.JsDocument
import play.api.libs.json.{JsValue, Json}
import rx.Observable

//if we `import com.sandinh.rx.Implicits._` instead of `import Implicits._`
//then both sbt clean compile & increment compile will be failed
import com.sandinh.rx.Implicits._

//if we `import Implicits._` instead of `import com.sandinh.rx.Implicits._`
//then clean compile will success, but after that, if we change this file
// ex, by adding/ deleting `private` modifier from `def row2Obs`,
// then run increment compile => failed!
//import Implicits._

object LogView {
  private def row2Obs(doc: Observable[JsDocument]): Observable[JsValue] =
    doc.scMap(d => Json.obj(d.id -> d.content))
}
// build.sbt}
name := "test"
version := "1.0"

//tested (fail) with both scala 2.11.5 & 2.11.6-20150127.023613-7
//resolvers += Resolver.sonatypeRepo("snapshots")
//scalaVersion := "2.11.6-SNAPSHOT"

scalaVersion := "2.11.5"

scalacOptions ++= Seq("-encoding", "UTF-8"
  ,"-optimise" //only fail with -optimise option
//  ,"-Ybackend:GenBCode" //tested (fail) with both GenASM & GenBCode backends
)

libraryDependencies += "com.sandinh" %% "couchbase-scala" % "6.1.0-SNAPSHOT"

//tested (fail) with & without this setting:
libraryDependencies <+= scalaVersion(v => "org.scala-lang" %  "scala-reflect" % v)

@scabug
Copy link
Author

scabug commented Jan 30, 2015

Gia Bảo (giabao) said (edited on Jan 30, 2015 11:56:12 AM UTC):
sbt clean compile crash (see sbt-clean-compile.txt) if use Implicit value class from external library:

  1. in LogView

    • uncomment import com.sandinh.rx.Implicits._
    • comment import Implicits._
  2. run sbt commands
    clean
    compile

@scabug
Copy link
Author

scabug commented Jan 30, 2015

Gia Bảo (giabao) said:
sbt increment compile crash (see sbt-increment-compile.txt):

  1. in LogView

    • uncomment import Implicits._
    • comment import com.sandinh.rx.Implicits._
  2. run sbt commands
    clean
    compile

  3. change LogView ex, by adding/ deleting private modifier from def row2Obs

  4. run sbt (increment) compile command

@scabug
Copy link
Author

scabug commented Feb 2, 2015

@retronym said (edited by @lrytz on Feb 5, 2015 8:17:13 PM UTC):
Thank you for taking the time to produce a standalone example. I managed to whittle it down a bit further:

tail $(find test/files/run -type f | grep t9121)
==> test/files/run/t9121/A_1.scala <==
package p1
object Implicits {
  class ScalaObservable(val underlying: Any) extends AnyVal {
    //if remove `@inline` annotation then test will success
    @inline def scMap[R](f: String): Any = f.toRx
  }

  implicit class RichFunction1[T1, R](val f: String) extends AnyVal {
    def toRx: Any = ""
  }
}

==> test/files/run/t9121/B_2.scala <==
import p1.Implicits._

object Test {
  def main(args: Array[String]): Unit = {
    new ScalaObservable("").scMap("")
  }
}

==> test/files/run/t9121.flags <==
-optimize

The crash manifests when the inliner reconsitutes the symbol/type information from A_1 when separately compiling B_2. It seems we end up with a LOAD_FIELD() ICode instruction that refers to a method symbol, rather than a field symbol. I haven't managed to figure out why this is happening, though.

@scabug
Copy link
Author

scabug commented Feb 2, 2015

@retronym said:
Lukas, can I assign this one to you?

@scabug
Copy link
Author

scabug commented Feb 2, 2015

@lrytz said:
sure

@scabug
Copy link
Author

scabug commented Feb 2, 2015

@retronym said:
This diff might lead the investigation in the right direction:

git diff
diff --git a/src/compiler/scala/tools/nsc/symtab/classfile/ICodeReader.scala b/src/compiler/scala/tools/nsc/symtab/classfile/ICodeReader.scala
index bd1fa4e..e1583b1 100644
--- a/src/compiler/scala/tools/nsc/symtab/classfile/ICodeReader.scala
+++ b/src/compiler/scala/tools/nsc/symtab/classfile/ICodeReader.scala
@@ -46,12 +46,12 @@ abstract class ICodeReader extends ClassfileParser {
         val sym1 = enteringIcode {
           sym.linkedClassOfClass.info
           sym.info.decl(part.encode)
-        }//.suchThat(module == _.isModule)
+        }

         sym = sym1 orElse sym.info.decl(part.encode.toTypeName)
       }
     }
-    sym
+    sym.filter(_.isModule == module)
   }

That gets as far as a new crash:

Exception in thread "main" java.lang.AssertionError: assertion failed:
  Different class symbols have the same bytecode-level internal name:
     name: p1/Implicits$RichFunction1
   oldsym: p1.Implicits$RichFunction1
  tracked: p1.Implicits$RichFunction1

The problem is that the module and the factory method from the implicit class have the same name.

@scabug
Copy link
Author

scabug commented Feb 5, 2015

@lrytz said:
the problem is the follwoing (after Jason's patch to sym.filter(_.isModule == module))

ICodeReader is loading the bytecode of Implicits$ScalaObservable$.class, there's the instruction GETSTATIC p1/Implicits$RichFunction1$.MODULE$.
The ICodeReader will try to create a LOAD_MODULE and find the module symbol for RichFunction1 (the $ is dropped) using forceMangledName. This splits the name "p1/Implicits$RichFunction1" into the three parts, and does a lookup of the TermName "RichFunction1" in class "Implicits". This is done enteringIcode (see here), so the module is not found (classes / modules are members of packages after flatten). It would work before flatten (tested). The phase travel to enteringIcode was added in this commit. CC @dragos

Once the forceMangledName lookup has failed, the ICodeReader will instead use rootMirror.getModuleByName("p1/Implicits$RichFunction1") (see here). This lookup succeeds because a classfile with that name exists, and the ClassfileParser created a class and a module symbol with the JAVA flag (see here). Note that this is predicated with if (!isScala), which is true - the classfile does not have a ScalaSignature. (The classfile has only the Scala marker attribute, so !isScalaRaw would be false. What is the reason we use isScala there instead?)

The assertion failure that Jason (and me) observed in the backend is then due to the fact that this JAVA marked module symbol is used: if you call javaBinaryName on such a symbol, the $ is not added to the name. So we end up having two class symbols (the ordinary, unpickled symbol for class RichFunction1, and the JAVA-marked module symbol with raw name Implicits$RichFunction1) that have the same internal name.

I'm not sure how much of that we can safely change before 2.11.6.

@scabug
Copy link
Author

scabug commented Feb 5, 2015

@dragos said:
Lukas, I agree with you on both accounts:

  • the phase travel to ICode is wrong
  • it should use isScalaRaw for testing if the classfile comes from Scala or not

@scabug
Copy link
Author

scabug commented Feb 5, 2015

@lrytz said:
OK, I can run those changes over the community build, but they do seem a bit risky, especially the second...

@scabug
Copy link
Author

scabug commented Apr 25, 2016

@lrytz said:
scala/scala#5122

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