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

proposal for debugging position information #21

Merged
merged 3 commits into from
Apr 18, 2017
Merged

proposal for debugging position information #21

merged 3 commits into from
Apr 18, 2017

Conversation

fommil
Copy link
Contributor

@fommil fommil commented Mar 22, 2017

needs costs / timescale estimates from @retronym and/or others.

At scalasphere, there was a lot of good will between Scala IDE, IntelliJ and ENSIME to collaborate more with common libraries. That is reflected well in our GSoC proposals.

But there was this extra piece of work on the debugger that we felt really needed the expertise of somebody at EPFL - i.e. proximity to the scala research or development teams.

BTW, one disadvantage of this approach is that it will mean that an extra step is required to navigate to the source of a stack trace in an exception message (unless there is a way of preserving line numbers for methods, but not code blocks).

@larsrh
Copy link
Contributor

larsrh commented Mar 22, 2017

Sounds like a reasonable proposal. I'm happy to champion this proposal, but would also suggest inviting @fommil to the next meeting to answer any technical questions.

@niktrop
Copy link

niktrop commented Mar 23, 2017

I think the way to go is to add an additional strata to scalac-generated classfiles. Currently scalac uses the default "Java" stratum. There may be another "ScalaOffset" stratum and by convention Location.lineNumber("ScalaOffset") will return the start offset (or column) of the executed expression. If this information is missing, all existing tools will work as before.

@propensive
Copy link
Contributor

@fommil would certainly be welcome to present this at the next meeting. This will be a physical meeting, alongside Scala Days Copenhagen, but people can always attend via video link.

@fommil
Copy link
Contributor Author

fommil commented Mar 28, 2017

@propensive cool. I am just waiting for feedback from somebody who can scope this. I am not in a position to do so... I guess this is a bottleneck for all SC proposals? Is there potential to change the process to include a formal scoping stage from the people doing the work? That's certainly the way it'd be done in industry (it'd be rare for the customer to give an estimate... that's definitely for the developer to do)

@propensive
Copy link
Contributor

propensive commented Mar 28, 2017

The cost/time "scope" is only intended to be a guideline for discussion and hiring. You're not the first person to comment that it seems back-to-front, but it puts the burden on the proposer to do some initial research into the work involved. And equally, it requires the Scala Center to check that the scope is consistent with expectations. Bear in mind that the Scala Center may have to hire someone the developer to complete the work, so it's not a typical customer/supplier relationship.

It might be worthwhile asking a third-party who could have a reasonable guess. Maybe @dragos?

@dragos
Copy link

dragos commented Mar 29, 2017

I like the idea of using JSR-45 and generating more debug information in classfiles. However, it'd be great to provide more complete use-cases for what constitutes a successful implementation of this proposal (that'd help with scoping also). What are the things that debuggers do for Java but can't do for Scala now?

Here's my attempt, but maybe I am missing something that @fommil had in mind:

  1. cleaner step-into for trait methods and inlined methods (without "jumping" to the class definition first)
  2. fine-grained breakpoint support when multiple closures are on the same line
  3. precise Scala-level type information for local variables (may be useful for evaluate-expression with a correct scope-at-breakpoint)
  4. stepping through pattern matches (not sure what's the status now, it used to be quite "jumpy")

I'm not sure that nr. 2 can be done with the standard JSR-45 attributes. Some information may need to be encoded into the vendor-specific section, meaning debuggers will have to be modified in order to understand it.

JSR-45 assumes source-to-source translation and that all debugger functionality can be expressed correctly in terms of the final-source stratum, which is usually Java. Since Scala doesn't have such a source-to-source equivalent, there's two implementation approaches I see:

  • create such a stratum (it doesn't need to be compilable Java, just close enough for debugging purposes, probably something around the erasure phase). This would be Scala with all desugarings applied and with no more than one method call per line (or whatever granularity we need for breakpoints). Then use this as final-source and have debuggers use this stratum when setting breakpoints. Debuggers should have most support in place, but we need to make sure we "play by the book". The advantage is using a standard, the disadvantage is (again) some possible loss in precision. We could benefit from knowing how Scala.js does it /cc @sjrd
  • stuff range-position information into the vendor-specific attribute and modify debuggers to use it. Note that range positions aren't very reliable after typer, so there will be some whack-a-mole development for fixing all code-generation that doesn't use proper positions. Valuable work for sure, but hard to estimate. The advantage I see in this approach is that it's just exposing what's already there and let debuggers and other tools figure out what to do with it.

In terms of scoping, I am guessing the second option is slightly smaller (if we limit this to the compiler-side of the work), but we'd need at least one debugger using it to know it's actually working. If I were to take a guess at the effort required, I'd say this would be around 6 months for someone who is familiar with the compiler.

@sjrd
Copy link
Contributor

sjrd commented Mar 29, 2017

In Scala.js, things are easier because Source Maps were defined with column information in mind. In fact, a source map can be reduced to be defined as "a (partial) function from (line, column) in the .js file to a triplet (file, line, column) in some arbitrary source file". The file part is a (possibly relative) URL.

Encoding-wise, it looks more like a function from a range position (startLine, startColumn, endLine, endColumn) to a point position (file, line, column), but that's not fundamental.

Note that there is no notion of source range position. But there the (point) positions are definitely (line, column), both in source and in .js.

This function targetPointPos => sourcePointPos is enough to produce good stack traces, for example, or to provide decent stepping.

In theory, it should also be good enough to set breakpoints. Even though one source position can be associated to multiple .js positions, one would hope that JS debuggers would associate breakpoints to the source position, and actually break at each .js position that maps to that source position. But in practice they don't so far; instead they choose an arbitrary .js position that maps to where you put the breakpoint, and then usually things go really awry (especially since they only remember the line in .js position, and the beginning that .js line can actually refer to a different source line position; which means as soon as you place the breakpoint, the breakpoint itself jumps to some other source line ... completely unusable). That's another fight, though.

@propensive
Copy link
Contributor

Thanks for the input, @dragos & @sjrd! Not that it was ever especially lacking, but I'm sure this will add a lot of credibility to the proposal.

@retronym
Copy link

Debuggers could implement smart stepping (to skip accessor methods, or trait forwarders) without extra metadata by recognizing some fairly straightforward patterns in the bytecode.

Intra-line breakpoints are implemented in IntelliJ already for Java and Scala. AFAIK, their approach there is to disambiguate which bytecode indexes refer to some range within a line based on analysis of the typechecked code, rather than relying on extra metadata in the bytecode.

One use case that might be impossible to implement without extra metadata is support for breakpoints in inlined methods.

class A {
  @inline def foo = 1 // breakpoint here wil not be triggered.
}}
object Test {
   def main(...) { new A().foo }
}

@wpopielarski
Copy link

The big problem I see in Scala IDE debugger and Intellij one is visiting some for comprehended lambdas with STEP_INTO. In Intellij these lambdas are just skipped and in Scala IDE it lands somewhere in acme.Clazz$Lambda$6.1234567 and jumps out the for block. I think, as my guts are saying to me, that it can be solved with Scala stratum and some wise code ahead analysis to set breakpoints dynamically in for blocks. I have impression that JDI isn't too good in setting breakpoints because there are problems with lambdas among Java devs too.

@dragos
Copy link

dragos commented Mar 30, 2017

One use case that might be impossible to implement without extra metadata is support for breakpoints in inlined methods.

Another case would be expanded macro code. I think @xeno-by had a prototype to generate user-readable source code, and if that was distributed together with the original source code, IDEs could display and break into macro code.

@fommil
Copy link
Contributor Author

fommil commented Mar 30, 2017

@wpopielarski is Scala IDE doing the "intra-line breakpoints" like IntelliJ? It sounds like this proposal would clear all that up, make it less prone to error (in so much as range positions will be correct) and make it usable by all tooling.

@wpopielarski
Copy link

@fommil, first: there is no big problem with stopping in breakpoint. Scala IDE uses just plain JDI requests and events to stop execution. Of course the problem is if there is more bytecode lines which reflect source line breakpoint. So if you set breakpoint in:

class A
...
list.map { _.toString } //<- here breakpoint

you will get at least 2 hits. The first is associated with surrounding class A, the second with inner lambda. Afaik Intellij tries to guess the exact class name (in pre-Java8 they knew that it would be something like A$anon$1 and this guess could be supported with PSI trees analysis) and sets breakpoint in this class only. Now with Java 8 invoke dynamic I don't know they are doing it. On other hand Scala IDE relies on JDI events only. So this is the reason why we are hitting same line multiple times. The fact that this is good behavior is seen on actual top stack frame (it is changing with next hits).
Now the BIG problem is with STEP_INTO mode. There is no breakpoints set by Scala IDE. In this case JDI just decides where it can stop. And then we are wandering in strange xyz.MysterousLambda$6.123456 classes. And worst thing is that you cannot stop in lambda body unless you set directly breakpoint there.

@fommil
Copy link
Contributor Author

fommil commented Apr 2, 2017

Thanks all for your feedback! I'll try to incorporate this into an update over the following days / weeks.

@dragos I'm not sure what the appetite of the advisory board would be for a 6 month project. Perhaps we should offer some alternatives. If we limit the Proof of Concept to just inserting RangePosition in the strata, do you think something could be achieved in 2 months? Updating calling libraries and having a working debugger could come later.

@dragos
Copy link

dragos commented Apr 3, 2017

Sure, sounds good. 6 months is a conservative approximation for something that covers all cases. I think a PoC could be achieved in 2 months.

@fommil
Copy link
Contributor Author

fommil commented Apr 15, 2017

thanks all, I've updated the proposal with the most relevant points from this discussion.

@fommil fommil changed the title WIP: proposal for debugging position information proposal for debugging position information Apr 15, 2017
@propensive
Copy link
Contributor

@fommil Could you try to get this proposed by @larsrh or @bvenners? Then I can merge it!

@propensive
Copy link
Contributor

To be clear, it just needs the name of an AB member at the top.

@larsrh
Copy link
Contributor

larsrh commented Apr 18, 2017

Consider this proposed by me.

@propensive
Copy link
Contributor

Thanks, Lars!

@propensive propensive merged commit 55c5694 into scalacenter:master Apr 18, 2017
@fommil fommil deleted the debugging-symbols branch April 18, 2017 17:12
@fommil
Copy link
Contributor Author

fommil commented Apr 18, 2017

Here's hoping!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants