Bytecode generation for tailrec methods uses temporary variables which confuses debuggers #14773
Comments
ghost
added
itype:bug
stat:needs triage
No idea. But the way we the tailrec phase works in Scala 3 is different from Scala 2: it emits
@adpi2 is this also an issue in the metals debugger? |
smarter
added
area:backend
itype:question
and removed
stat:needs triage
|
I'm seeing the same issue in the Metals debugger with |
This is confirmed by the bytecode. I understand that it was a conscious choice, but the effects on JVM bytecode might not have been anticipated. |
|
Here's how the issue is manifested in IntelliJ IDEA. First video is Scala 2.13.8. Second video is 3.1.1. Screen.Recording.2022-03-25.at.11.49.37.movScreen.Recording.2022-03-25.at.11.50.21.mov |
|
And here's the same setup in Metals, first video Scala 2.13.8, second video Scala 3.1.1. Screen.Recording.2022-03-25.at.11.52.55.movScreen.Recording.2022-03-25.at.11.53.41.mov |
|
As another curiosity, both debuggers stop 3 times in Scala 2 ((x, y) have the values (2,3), (1, 4), (0, 5)), while they only stop 2 times in Scala 3. |
|
As @smarter said, tailrec functions are internally transformed in a much different way than in Scala 2. This is during tree transformations, several phases before emitting bytecode. Scala 2 had arbitrary labels and gotos, whereas Scala 3 only has "labeled blocks" (a form similar to This change of scheme has an impact on the JVM bytecode that is emitted, indeed. This was expected. I did not anticipate that it would impair debuggers, however (the semantics are the same, obviously). It would be possible to alter the bytecode generation to reuse the slots of Regarding the extra temporaries used to change the values of the temporaries when doing the recursive call, that is a bit more complicated. In theory, a local bytecode optimizer could get rid of them, but this is more difficult to analyze. Unless fixing that would also be required to improve the debugging experience, I would not suggest investing time in that specific case. |
|
The second case where only the temporary results of |
smarter
changed the title
|
@vasilmkd-jetbrains can you estimate how difficult it would be to teach the deubbger about this pattern? Maybe it would help if the synthetic local variables had names in the bytecode? Intuitively, it seems to me Scala 3 is emitting fine bytecode for tailrec. But I see that by assigning to the locals of parameters the way it's done in Scala 2, the debuggers would just work out of the box. |
|
I guess teaching the debugger is not too difficult, the question remains whether it's an unintended behavior that is going to be changed, or intended and will not change in future Scala versions. Before that is answered, I'm not sure it's wise to do anything on the debugger end. |
…vars.
The tailrec phase generates code that looks like
class Bar {
def foo(x1: Int, x2: Int): Int = {
var this$: Bar = this;
var taillocal$x1: Int = x1;
var taillocal$x2: Int = x2;
...
// body where `this`, `x1` and `x2` are replaced by
// `this$`, `taillocal$x1` and `taillocal$x2`
}
}
This generates bytecode where the `this` value and the parameters
never actually change, and are never used. Instead, the synthetic
mutable variables are used instead.
As described in the linked issue, this confuses debuggers, which
only display the never-changing original `this` value and
parameters.
In this commit, we intercept this shape of code in the back-end.
We reliable identify tailrec-generated `ValDef`s from their
semantic names, with an additional safety check that they are
`Synthetic | Mutable`. When we find this shape, we do not allocate
local slots for the `var`s, and instead reuse the slots for `this`
and the parameters. We skip past the `ValDef`s so that code
generation does not re-emit useless assignments.
|
PR available: #14865. |
…rec-methods Fix #14773: Reuse the param slots for the tailrec local mutable vars.
…vars.
The tailrec phase generates code that looks like
class Bar {
def foo(x1: Int, x2: Int): Int = {
var this$: Bar = this;
var taillocal$x1: Int = x1;
var taillocal$x2: Int = x2;
...
// body where `this`, `x1` and `x2` are replaced by
// `this$`, `taillocal$x1` and `taillocal$x2`
}
}
This generates bytecode where the `this` value and the parameters
never actually change, and are never used. Instead, the synthetic
mutable variables are used instead.
As described in the linked issue, this confuses debuggers, which
only display the never-changing original `this` value and
parameters.
In this commit, we intercept this shape of code in the back-end.
We reliable identify tailrec-generated `ValDef`s from their
semantic names, with an additional safety check that they are
`Synthetic | Mutable`. When we find this shape, we do not allocate
local slots for the `var`s, and instead reuse the slots for `this`
and the parameters. We skip past the `ValDef`s so that code
generation does not re-emit useless assignments.
Compiler version
Scala 3.1.1
Minimized code
Scala 2.13.8 compiles the code as follows (only the
addmethod bytecode is shown, once in a more compact form, just the bytecodes, and then a verbose form with local variables as well).Please take a note that only 3 local variables are used to compile the method (a reference to
this, which is the outer object, irrelevant for the issue, and a local variable for each of thexandyfunction params.Now let's look at the bytecode generated by Scala 3.1.1, for the same exact code. Again, the code is shown twice, once in a more compact form, and once in a verbose form that shows the local variables.
As you can see, the same code is compiled using 7 local variables, 3 named ones (the same as in Scala 2) and 4 unnamed ones. Let's try to figure out a sort of mapping between them.
Right at the beginning of the function code, we have
iload_2andistore_3, which moves the value of the third (0 based indexing) local variable (namedy) into an unnamed local variable with index 3.Following that, we have
iload_1andistore 4, again, moving the value of the first local variable (namedx) into an unnamed local variable with index 4. Thus, right at the beginning of the function, both function parameters were just copied over into new local variables.Later down in the function, we can see that the results of
x - 1andy + 1are stored again in these 2 unnamed variables (index 4 forxand index 3 fory), so, this means that effectively, thexandyfunction parameters are unused during the lifecycle of the function (they contain the original values that the function was called with), while the unnamed local variables with index 4 and index 3 serve as the effective stores of value forxandyduring the recursive execution of the function.Furthermore, we have a couple of
istore 5andistore 6instructions which only transiently store the results ofx - 1andy + 1, right before they are moved to the local variables 4 and 3, respectively, and the function body is executed again. Compared to Scala 2, this function uses 4 variables and ignores 2, to do a job that can be done with just the 2 function parameters as the stores of value.As a real world effect of this situation, the IntelliJ IDEA debugger for Scala 3 (any version) does not work correctly with tail-recursive functions. The debugger shows the function parameters
xandyas stack frame values, and they never change their values, which is supported by the bytecode above. Of course, other local values can be shown, but the experience is not great.My question thus is, is this an intentional change in Scala 3 (I personally cannot see that being the case, because the code generation scheme produces much larger bytecode with larger stack requirements), or is this an ommission in Scala 3 due to some changes in the bytecode generation in Scala 2 not being ported over to dotty during development?
In any case, how should tooling builders approach this issue? Should we try to issue a fix in Scala 3, or try to work around it in the tooling? I don't believe this is a problem for IntelliJ only, most debuggers would interpret the bytecode as it is produced.
Thanks in advance.
The text was updated successfully, but these errors were encountered: