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

inliner destroys line number information #7518

Closed
scabug opened this issue May 25, 2013 · 17 comments

Comments

@scabug
Copy link

commented May 25, 2013

In the attached screenshot, debugging Scala 2.10.2-RC1, you can see the call stack shows

resetAllAttrs():37, Global {scala.tools.nsc}
adaptToImplicitMethod$1():870, Typers$Typer {scala.tools.nsc.typechecker}

I expect:

resetAllAttrs():37, Global {scala.tools.nsc}
adaptToImplicitMethod$1():881, Typers$Typer {scala.tools.nsc.typechecker}

Where line 881 is in the inlined lambda.

@scabug

This comment has been minimized.

Copy link
Author

commented May 25, 2013

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

@scabug

This comment has been minimized.

Copy link
Author

commented May 28, 2013

@JamesIry said:
Is this a 2.10.2-RC1 regression?

@scabug

This comment has been minimized.

Copy link
Author

commented May 29, 2013

@retronym said:
I don't believe so. I've pushed the fix version out.

@scabug

This comment has been minimized.

Copy link
Author

commented May 29, 2013

@retronym said (edited on May 29, 2013 7:07:58 AM UTC):
Here's a standalone test case. The same line number information is emitted in 2.10.0 as 2.10.2-RC1.

class Test {
  @inline final def foo[A](a: => A) = a

  def client {
    foo {
      println("line 6")
    }
  }
}

With -optimize, nothing on line 6.

  scala> :javap -v Test

     public void client();
  Code:
   Stack=2, Locals=2, Args_size=1
   0:  getstatic  #28; //Field scala/Predef$.MODULE$:Lscala/Predef$;
   3:  astore_1
   4:  getstatic  #33; //Field scala/Console$.MODULE$:Lscala/Console$;
   7:  ldc  #35; //String line 6
   9:  invokevirtual  #39; //Method scala/Console$.println:(Ljava/lang/Object;)V
   12:  return
  LocalVariableTable:
   Start  Length  Slot  Name   Signature
   0      13      0    this       LTest;

  LineNumberTable:
   line 5: 0
   */

Without -optimize, closure creation and apply on line 6, as expected.

  scala> :javap -v Test

    public void client();
  Code:
   Stack=4, Locals=1, Args_size=1
   0:  aload_0
   1:  new  #24; //class Test$$anonfun$client$1
   4:  dup
   5:  aload_0
   6:  invokespecial  #28; //Method Test$$anonfun$client$1."<init>":(LTest;)V
   9:  invokevirtual  #30; //Method foo:(Lscala/Function0;)Ljava/lang/Object;
   12:  pop
   13:  return
  LocalVariableTable:
   Start  Length  Slot  Name   Signature
   0      14      0    this       LTest;

  LineNumberTable:
   line 5: 0
   line 6: 1
   line 5: 9

  scala> :javap -v Test$$anonfun$client$1

   public final java.lang.Object apply();
  Code:
   Stack=1, Locals=1, Args_size=1
   0:  aload_0
   1:  invokevirtual  #37; //Method apply:()V
   4:  getstatic  #43; //Field scala/runtime/BoxedUnit.UNIT:Lscala/runtime/BoxedUnit;
   7:  areturn
  LocalVariableTable:
   Start  Length  Slot  Name   Signature
   0      8      0    this       LTest$$anonfun$client$1;

  LineNumberTable:
   line 6: 0
@scabug

This comment has been minimized.

Copy link
Author

commented May 29, 2013

@retronym said:
Here's a patch that fixes this. But I'm not familiar enough with the intent of the original code to be sure its a) appropriate and b) complete.

retronym/scala@scala:2.10.x...ticket/7518

WIP SI-7518 Position retention for inlined code …
The inliner was positioning all inlined instructions
at the method call (`targetPos). This makes debugging
of code compiled under `-optimize` difficult.

This commit retains the original position of the
inlined instruction if it is in the same source file
as the method call.

TODO: This still seems aribtrary. What was the intent
of using `targetPos`? Should we expand the above to
any source position? What about code inlined from class
files: should they be treated any differently?

James, assigning this to you for an opinion.

@scabug

This comment has been minimized.

Copy link
Author

commented May 29, 2013

@retronym said:
After thinking about this for a bit longer, its now obvious that we can only retain the line numbers in the limited (but useful!) condition used in my patch.

@scabug

This comment has been minimized.

Copy link
Author

commented May 29, 2013

@retronym said:
Just added a commit with an additional test case that shows a bug in my patch.

@scabug

This comment has been minimized.

Copy link
Author

commented May 29, 2013

@JamesIry said:
One of the stupid limitations of the JVM is that every .class file has only one associated source file name and chunks of bytecode are assigned to lines within that source file. See http://docs.oracle.com/javase/specs/jvms/se5.0/html/ClassFile.doc.html#79868 and http://docs.oracle.com/javase/specs/jvms/se5.0/html/ClassFile.doc.html#22856 .

TL;DR Inlining from one source file into another source file must lose debug information.

@scabug

This comment has been minimized.

Copy link
Author

commented May 29, 2013

@JamesIry said (edited on May 29, 2013 4:17:03 PM UTC):
Ah, but wait, Java 7 has an answer. There's a new attribute http://docs.oracle.com/javase/specs/jvms/se7/html/jvms-4.html#jvms-4.7.11 and JSR-45 specifies what goes into that attribute http://download.oracle.com/otndocs/jcp/dsol-1.0-fr-spec-oth-JSpec/. Because the attribute is invisible to JVMs prior to 7, it should be safe to add this functionality.

@scabug

This comment has been minimized.

Copy link
Author

commented May 29, 2013

@retronym said:
It would be great to start with the dumb-but-Java-6-compatible version to at least retain line number info from the same file. Inlining a closure or by-name argument is so common, I'd really like to be able to set a break point in them.

But I got a bit lost in the code trying to implement this correctly.

@scabug

This comment has been minimized.

Copy link
Author

commented May 29, 2013

@JamesIry said:
Agreed. Even if/when we can do the super-duper-extended-attribute version, we probably mostly don't want to except in cases where it's necessary as it will make class files that much larger. I'll take a look at the code.

@scabug

This comment has been minimized.

Copy link
Author

commented Jun 11, 2013

@magarciaEPFL said:
The new optimizer http://magarciaepfl.github.io/scala/ isn't prone to this bug. In the example, the closure body (which is invoked from the inlined higher-order method) contains the correct line number information:

  // access flags 0x1A
  private final static dlgt$27b8b21$1()V
   L0
    LINENUMBER 6 L0
    GETSTATIC scala/Predef$.MODULE$ : Lscala/Predef$;
    LDC "line 6"
    INVOKEVIRTUAL scala/Predef$.println (Ljava/lang/Object;)V
    RETURN
    MAXSTACK = 2
    MAXLOCALS = 0

A sidenote: Why aren't the instructions above also inlined alongside the instructions inlined for the higher-order method? Inlining a static method (e.g., the delegate above) is better done by the VM. Moreover, that's what the current optimizer does, with the consequence that:

the current optimizer duplicates a closure body whenever that closure's apply() is invoked, the resulting code size taxes the JIT compiler. 

With the new optimizer, the only way to avoid the code-duplication above is to write "optimizer-quirks-aware" code, as was done for immutable.Range in:
scala/scala@7abb0c9

@scabug

This comment has been minimized.

Copy link
Author

commented Jun 11, 2013

@retronym said:
@miguel: Just to confirm: does the new backend take care not to attach line number information from other sourcefiles, if the inlined method and inline target are in different compilation units?

@scabug

This comment has been minimized.

Copy link
Author

commented Jun 11, 2013

@magarciaEPFL said:
I guess the new backend currently doesn't pay attention to that distinction (debug info originating in other source files). I'll fix it after finding out.

@scabug

This comment has been minimized.

Copy link
Author

commented Nov 4, 2014

@retronym said:
Updating fix-by version to 2.11.5.

@scabug scabug added this to the Backlog milestone Apr 7, 2017

@SethTisue SethTisue removed the critical label Jul 27, 2017

@SethTisue

This comment has been minimized.

Copy link
Member

commented Mar 2, 2018

@lrytz should this stay open?

@lrytz

This comment has been minimized.

Copy link
Member

commented Mar 7, 2018

no, let's track the rest at scala/scala-dev#3

@lrytz lrytz closed this Mar 7, 2018

@scala scala deleted a comment from scabug Mar 6, 2019

@scala scala deleted a comment from scabug Mar 6, 2019

@scala scala deleted a comment from scabug Mar 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.