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

Jump to bottom

crashy interplay between inlining, dead code elimination #8315

Closed
scabug opened this issue Feb 19, 2014 · 9 comments
Closed

crashy interplay between inlining, dead code elimination #8315

scabug opened this issue Feb 19, 2014 · 9 comments
Assignees
@retronym
Labels
has PR optimizer
Milestone
2.11.0

Comments

@scabug
Copy link

scabug commented Feb 19, 2014 

class A {
  def crash(as: Listt): Unit = {
    map(as, (_: Any) => return)
  }

  final def map(x: Listt, f: Any => Any): Any = {
    if (x eq Nill) "" else f("")
  }
}

object Nill extends Listt
class Listt
scalac-hash v2.10.3 -Ylog:inliner,dce -Yinline -Ydead-code -Dlow=0 -Dhigh=1000000000 test/files/pos/nonlocal-unchecked.scala 2>&1 | tee sandbox/bad.log
[log inliner]    class  Nill                                     // analyzing 1 methods in Nill
[log inliner]    class  A$$anonfun$crash$1               // analyzing 3 methods in A$$anonfun$crash$1
[log inliner]    class  Listt                            // analyzing 1 methods in Listt
[log inliner]    class  A                                // analyzing 3 methods in A
[log inliner]       +2  A.map                            // ok to inline into A.crash
[log inliner]       +3  A.$anonfun$crash$1.apply         // ok to inline into A.crash
[log inliner]       +3  A.$anonfun$crash$1.apply         // ok to inline into A.crash
[log inliner]   access  nonLocalReturnKey1$1             // making public
[log inliner]  inlined  A.crash                          // 3 inlined: A.map, anonfun$crash$1.apply, anonfun$crash$1.apply
[log inliner] <<tldr>>  A.crash                          // crash: instructions 28 -> 56, blocks 6 -> 15
[log dce] Analyzing 3 methods in A.
in: 9 -> IState(, )
2 -> IState(, )
16 -> IState(, )
10 -> IState(, )
3 -> IState(, )
7 -> IState((value nonLocalReturnKey1,1,3), )
14 -> IState(, )
11 -> IState(, )
4 -> IState(, )
1 -> IState(, )
8 -> IState(, )
12 -> IState(, )
5 -> IState(, )
out: 9 -> IState(, )
2 -> IState(, )
16 -> IState(, )
10 -> IState(, )
3 -> IState(, )
7 -> IState(, )
14 -> IState(, )
11 -> IState(, )
4 -> IState(, )
1 -> IState((value nonLocalReturnKey1,1,3), )
8 -> IState(, )
12 -> IState(, )
5 -> IState(, )
java.util.NoSuchElementException: key not found: 13
...
Exception in thread "main" java.lang.RuntimeException: Could not find element key not found: 13
	at scala.sys.package$.error(package.scala:27)
	at scala.tools.nsc.backend.icode.analysis.DataFlowAnalysis$class.forwardAnalysis(DataFlowAnalysis.scala:82)
	at scala.tools.nsc.backend.icode.analysis.ReachingDefinitions$ReachingDefinitionsAnalysis.forwardAnalysis(ReachingDefinitions.scala:62)
	at scala.tools.nsc.backend.icode.analysis.ReachingDefinitions$ReachingDefinitionsAnalysis.run(ReachingDefinitions.scala:143)
	at scala.tools.nsc.backend.opt.DeadCodeElimination$DeadCode.collectRDef(DeadCodeElimination.scala:110)
@scabug
Copy link
Author

scabug commented Feb 19, 2014

Imported From: https://issues.scala-lang.org/browse/SI-8315?orig=1
Reporter: @retronym
Affected Versions: 2.10.3, 2.11.0-M8

@scabug
Copy link
Author

scabug commented Feb 19, 2014

@magarciaEPFL said:
Just checked, the new optimizer http://magarciaepfl.github.io/scala/ isn't prone to this bug.

@scabug
Copy link
Author

scabug commented Feb 19, 2014

@retronym said:
Here is the icode diff over the inliner phase:

diff -u Aicode.icode Ainliner.icode
--- Aicode.icode	2014-02-19 13:47:56.000000000 +0100
+++ Ainliner.icode	2014-02-19 13:47:56.000000000 +0100
@@ -3,9 +3,9 @@

   // methods
   def crash(as: Listt#7618 (REF(class Listt#7618))): Unit#2633 {
-  locals: value as#14838, value nonLocalReturnKey1#15623, value ex#16011
+  locals: value as#14838, value nonLocalReturnKey1#15623, value ex#16011, variable x1#26553, variable f1#26554, variable $inlThis1#26551, variable $retVal1#26552, variable v11#26558, variable $inlThis2#26556, variable $retVal2#26557, variable x$11#26561, variable $inlThis3#26559, variable $retVal3#26560
   startBlock: 1
-  blocks: [1,2,3,4,5,7]
+  blocks: [1,2,3,4,5,7,8,9,10,11,12,13,14,15,16]

   1:
     2	NEW REF(class Object#147)
@@ -23,10 +23,50 @@
     3	THIS(A)
     3	LOAD_LOCAL(value nonLocalReturnKey1#15623)
     3	CALL_METHOD A$$anonfun$crash$1.<init> (static-instance)
-    3	CALL_METHOD A.map (dynamic)
+    3	STORE_LOCAL(variable f1#26554)
+    3	STORE_LOCAL(variable x1#26553)
+    3	STORE_LOCAL(variable $inlThis1#26551)
+    3	JUMP 9
+
+  9:
+    3	LOAD_LOCAL(variable x1#26553)
+    3	LOAD_MODULE object Nill#7625
+    3	CJUMP (REF(type AnyRef#2673))EQ ? 10 : 11
+
+  10:
+    3	CONSTANT("")
+    3	JUMP 12
+
+  12:
+    3	JUMP 8
+
+  8:
     3	DROP REF(class Object#147)
     3	JUMP 2

+  11:
+    3	LOAD_LOCAL(variable f1#26554)
+    3	CONSTANT("")
+    3	STORE_LOCAL(variable v11#26558)
+    3	STORE_LOCAL(variable $inlThis2#26556)
+    3	JUMP 14
+
+  14:
+    3	LOAD_LOCAL(variable $inlThis2#26556)
+    3	LOAD_LOCAL(variable v11#26558)
+    3	STORE_LOCAL(variable x$11#26561)
+    3	STORE_LOCAL(variable $inlThis3#26559)
+    3	JUMP 16
+
+  16:
+    3	NEW REF(class NonLocalReturnControl$mcV$sp#19684)
+    3	DUP(REF(class NonLocalReturnControl$mcV$sp#19684))
+    3	LOAD_LOCAL(variable $inlThis3#26559)
+    3	LOAD_FIELD value nonLocalReturnKey1$1#25361
+    3	LOAD_FIELD scala.runtime.BoxedUnit.UNIT
+    3	CALL_METHOD scala.runtime.NonLocalReturnControl$mcV$sp.<init> (static-instance)
+    3	THROW(NonLocalReturnControl$mcV$sp)
+
   3:
     3	LOAD_EXCEPTION(class NonLocalReturnControl#3949)
     3	STORE_LOCAL(value ex#16011)
@@ -49,7 +89,7 @@

   }
   Exception handlers:
-    catch (NonLocalReturnControl) in ArrayBuffer(7) starting at: 3
+    catch (NonLocalReturnControl) in ArrayBuffer(7, 8, 9, 10, 11, 12, 13, 14, 15, 16) starting at: 3
       consisting of blocks: List(5, 4, 3)
       with finalizer: null

@scabug
Copy link
Author

scabug commented Feb 19, 2014

@retronym said:
-Ycheck:inliner warns about:

3	LOAD_LOCAL(variable $inlThis3#26559)
+    3	LOAD_FIELD value nonLocalReturnKey1$1#25361

[log dce] ** Checking Block 16 [S: 3] [P: 14] <closed>
warning: !! ICode checker fatality in A.crash
  at: Block 16 [S: 3] [P: 14] <closed>
  error message:  value nonLocalReturnKey1$1#25361 is not defined in class A

I believe that the error message itself might be misleading; that error is issued after determining that Object doesn't contain the field.

        def checkField(obj: TypeKind, field: Symbol): Unit = obj match {
          case REFERENCE(sym) =>
            if (sym.info.member(field.name) == NoSymbol)
              icodeError(" " + field + " is not defined in class " + clasz)
          case _ =>
            icodeError(" expected reference type, but " + obj + " found")
        }

Object is the type (unconditionally) attributed to $inlThis.

        // store the '$this' into the special local
        val inlinedThis = newLocal("$inlThis", REFERENCE(ObjectClass))

@scabug
Copy link
Author

scabug commented Feb 19, 2014

@retronym said:
@miguel: do you have any pointers for how to patch the ICode optimizer in the meantime?

@scabug
Copy link
Author

scabug commented Feb 19, 2014

@magarciaEPFL said:
I'm afraid I don't know how to fix this bug.

@scabug
Copy link
Author

scabug commented Feb 20, 2014

@retronym said:
Here's another test case with the non-local return control flow desugared:

https://github.com/retronym/scala/compare/ticket;8315?expand=1

@scabug
Copy link
Author

scabug commented Feb 20, 2014

@retronym said:
scala/scala#3567 (comment)

@scabug scabug closed this as completed Feb 22, 2014
@scabug
Copy link
Author

scabug commented May 19, 2014

@adriaanm said:
Git log says this was fixed in 2.11.0 -- assuming 2.11.1 fixversion was a typo

@scabug scabug added this to the 2.11.0 milestone Apr 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@retronym retronym

Labels
has PR optimizer
Projects
None yet
Milestone
2.11.0
Development

No branches or pull requests
2 participants
@retronym @scabug