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.

Sign up for GitHub

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

scala optimizer generates incorrect code (2.11 regression) #9403

Closed
scabug opened this issue Jul 16, 2015 · 9 comments
Closed

scala optimizer generates incorrect code (2.11 regression) #9403

scabug opened this issue Jul 16, 2015 · 9 comments
Assignees
@lrytz
Labels
bytecode optimizer regression valueclass
Milestone
2.11.8

Comments

@scabug
Copy link

scabug commented Jul 16, 2015

I found an instance where the scala optimizer is generating incorrect bytecode which results in broken integer arithmetic (given a 39 where it should give a -5000). It happens for all versions of scala 2.11.x but worked correctly in 2.10.x. It only happens with -optimize.

I've put the complete code necessary to reproduce on github. But I'll paste the important bits here. The following two files must in different projects (e.g. a main and a test project) in order to trigger the bug:

package bug // in main
final case class Bust (x: Long) extends AnyVal {
  @inline def bar: Long = 10000L / (if (x < 0) -2 else 2)
}
package bug // in test
object Busted {
  def main(args: Array[String]) {
    val p = Bust(-1L)
    val y = p.bar
    println(y) // should print -5000 but prints 39
  }
}

Looking at the bytecode the problem happens when the bar method is inlined. Scala 2.10 generates this

Compiled from "Busted.scala"
public final class bug.Busted$ {
  public static final bug.Busted$ MODULE$;

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

  public void main(java.lang.String[]);
    Code:
       0: getstatic     #19                 // Field bug/Bust$.MODULE$:Lbug/Bust$;
       3: astore_2      
       4: ldc2_w        #20                 // long 10000l
       7: ldc2_w        #22                 // long -1l
      10: lconst_0      
      11: lcmp          
      12: iflt          19
      15: iconst_2      
      16: goto          21
      19: bipush        -2
      21: i2l           
      22: ldiv          
      23: lstore_3      
      24: getstatic     #28                 // Field scala/Predef$.MODULE$:Lscala/Predef$;
      27: lload_3       
      28: invokestatic  #34                 // Method scala/runtime/BoxesRunTime.boxToLong:(J)Ljava/lang/Long;
      31: invokevirtual #38                 // Method scala/Predef$.println:(Ljava/lang/Object;)V
      34: return        
}

whereas Scala 2.11 is generating this:

Compiled from "Busted.scala"
public final class bug.Busted$ {
  public static final bug.Busted$ MODULE$;

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

  public void main(java.lang.String[]);
    Code:
       0: getstatic     #19                 // Field bug/Bust$.MODULE$:Lbug/Bust$;
       3: astore_2      
       4: ldc2_w        #20                 // long 10000l
       7: ldc2_w        #22                 // long -1l
      10: lconst_0      
      11: lcmp          
      12: iflt          19
      15: iconst_2      
      16: goto          22
      19: sipush        254
      22: i2l           
      23: ldiv          
      24: lstore_3      
      25: getstatic     #28                 // Field scala/Predef$.MODULE$:Lscala/Predef$;
      28: lload_3       
      29: invokestatic  #34                 // Method scala/runtime/BoxesRunTime.boxToLong:(J)Ljava/lang/Long;
      32: invokevirtual #38                 // Method scala/Predef$.println:(Ljava/lang/Object;)V
      35: return        
}

The only meaningful difference is that bipush -2 is getting replaced with sipush 254, which is clearly incorrect.
@scabug
Copy link
Author

scabug commented Jul 16, 2015

Imported From: https://issues.scala-lang.org/browse/SI-9403?orig=1
Reporter: Jeff Olson (jdolson)
Affected Versions: 2.11.0, 2.11.1, 2.11.2, 2.11.3, 2.11.4, 2.11.5, 2.11.6, 2.11.7

@scabug
Copy link
Author

scabug commented Jul 17, 2015

@som-snytt said:
Workaround is use -2L.

@scabug
Copy link
Author

scabug commented Jul 20, 2015

Jeff Olson (jdolson) said:
Yes, I can workaround this particular bug in any number of ways. The real problem is that I can't trust the optimizer in 2.11. The above example is just one particular bug that I was able to isolate. I have more, I just haven't been able to isolate them yet. I have a value class many of whose methods are marked @inline that I use extensively, and many of the unit tests that use this class (directly or indirectly) are failing. These tests all pass if I leave off -optimise.

This is a show-stopper for me. I can't upgrade my project to Scala 2.11 because of it.

@scabug
Copy link
Author

scabug commented Jul 22, 2015

@lrytz said:
found the bug - quite surprised it wasn't reported before. PR coming soon. bonus points if you see how 2 and 254 are related :)

@scabug
Copy link
Author

scabug commented Jul 22, 2015

Jeff Olson (jdolson) said:
Awesome. I'm willing to put money on the fact that -2 was getting treated as signed byte and then an unsigned byte and then a signed Int/Long.

@scabug
Copy link
Author

scabug commented Jul 22, 2015

@lrytz said:
spoiler in scala/scala#4653

@scabug scabug closed this as completed Jul 27, 2015
@scabug
Copy link
Author

scabug commented Jul 27, 2015

Jeff Olson (jdolson) said:
Thanks for fixing this so quickly guys. One question for you: Not knowing the internals of scalac very well, I don't know how or where ICodeReader is used. Clearly it must be used by the optimizer. Is this bug limited to -optimise or are there other things that could trigger the bug even without -optimise. In other words, is scala 2.11.7 immune to this issue if -optimise is not specified?

@scabug
Copy link
Author

scabug commented Jul 27, 2015

@retronym said (edited on Jul 27, 2015 4:15:17 AM UTC):
ICodeReader is only used in the optimizer, and only used in the GenASM backend (which is the default in the Scala 2.11 series.)

It is responsible for reading bytecode of previously compiled classes and translating them into an intermediate representation called ICode that is used in the compilation pipeline. This allows inlining of methods under separate compilation.

So disabling -optimize is a valid workaround for this bug.

@scabug
Copy link
Author

scabug commented Jul 27, 2015

Jeff Olson (jdolson) said:
Ok. Thanks Jason. Good to know.

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

@lrytz lrytz

Labels
bytecode optimizer regression valueclass
Projects
None yet
Milestone
2.11.8
Development

No branches or pull requests
2 participants
@lrytz @scabug