From 633812ec8bfbce53eef0422afa948936e30fe7ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20Doeraene?= Date: Wed, 6 Apr 2022 17:08:24 +0200 Subject: [PATCH] Fix #14773: Reuse the param slots for the tailrec local mutable 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. --- .../tools/backend/jvm/BCodeSkelBuilder.scala | 47 +++++++++- .../dotty/tools/dotc/transform/TailRec.scala | 2 +- .../backend/jvm/DottyBytecodeTests.scala | 90 +++++++++---------- 3 files changed, 84 insertions(+), 55 deletions(-) diff --git a/compiler/src/dotty/tools/backend/jvm/BCodeSkelBuilder.scala b/compiler/src/dotty/tools/backend/jvm/BCodeSkelBuilder.scala index f668a1b68a5f..824a93ed506f 100644 --- a/compiler/src/dotty/tools/backend/jvm/BCodeSkelBuilder.scala +++ b/compiler/src/dotty/tools/backend/jvm/BCodeSkelBuilder.scala @@ -4,6 +4,8 @@ package jvm import scala.language.unsafeNulls +import scala.annotation.tailrec + import scala.collection.{ mutable, immutable } import scala.tools.asm @@ -484,6 +486,14 @@ trait BCodeSkelBuilder extends BCodeHelpers { slots.getOrElse(locSym, makeLocal(locSym)) } + def reuseLocal(sym: Symbol, loc: Local): Unit = + val existing = slots.put(sym, loc) + if (existing.isDefined) + report.error("attempt to create duplicate local var.", ctx.source.atSpan(sym.span)) + + def reuseThisSlot(sym: Symbol): Unit = + reuseLocal(sym, Local(symInfoTK(sym), sym.javaSimpleName, 0, sym.is(Synthetic))) + private def makeLocal(sym: Symbol, tk: BType): Local = { assert(nxtIdx != -1, "not a valid start index") val loc = Local(tk, sym.javaSimpleName, nxtIdx, sym.is(Synthetic)) @@ -753,18 +763,47 @@ trait BCodeSkelBuilder extends BCodeHelpers { .addFlagIf(isNative, asm.Opcodes.ACC_NATIVE) // native methods of objects are generated in mirror classes // TODO needed? for(ann <- m.symbol.annotations) { ann.symbol.initialize } - initJMethod(flags, params.map(_.symbol)) + val paramSyms = params.map(_.symbol) + initJMethod(flags, paramSyms) if (!isAbstractMethod && !isNative) { + // #14773 Reuse locals slots for tailrec-generated mutable vars + val trimmedRhs: Tree = + @tailrec def loop(stats: List[Tree]): List[Tree] = + stats match + case (tree @ ValDef(TailLocalName(_, _), _, _)) :: rest if tree.symbol.isAllOf(Mutable | Synthetic) => + tree.rhs match + case This(_) => + locals.reuseThisSlot(tree.symbol) + loop(rest) + case rhs: Ident if paramSyms.contains(rhs.symbol) => + locals.reuseLocal(tree.symbol, locals(rhs.symbol)) + loop(rest) + case _ => + stats + case _ => + stats + end loop + + rhs match + case Block(stats, expr) => + val trimmedStats = loop(stats) + if trimmedStats eq stats then + rhs + else + Block(trimmedStats, expr) + case _ => + rhs + end trimmedRhs def emitNormalMethodBody(): Unit = { val veryFirstProgramPoint = currProgramPoint() - genLoad(rhs, returnType) + genLoad(trimmedRhs, returnType) - rhs match { + trimmedRhs match { case (_: Return) | Block(_, (_: Return)) => () - case (_: Apply) | Block(_, (_: Apply)) if rhs.symbol eq defn.throwMethod => () + case (_: Apply) | Block(_, (_: Apply)) if trimmedRhs.symbol eq defn.throwMethod => () case tpd.EmptyTree => report.error("Concrete method has no definition: " + dd + ( if (ctx.settings.Ydebug.value) "(found: " + methSymbol.owner.info.decls.toList.mkString(", ") + ")" diff --git a/compiler/src/dotty/tools/dotc/transform/TailRec.scala b/compiler/src/dotty/tools/dotc/transform/TailRec.scala index 9ee3aa5b1a8c..c2555c3d5b95 100644 --- a/compiler/src/dotty/tools/dotc/transform/TailRec.scala +++ b/compiler/src/dotty/tools/dotc/transform/TailRec.scala @@ -253,7 +253,7 @@ class TailRec extends MiniPhase { val tpe = if (enclosingClass.is(Module)) enclosingClass.thisType else enclosingClass.classInfo.selfType - val sym = newSymbol(method, nme.SELF, Synthetic | Mutable, tpe) + val sym = newSymbol(method, TailLocalName.fresh(nme.SELF), Synthetic | Mutable, tpe) varForRewrittenThis = Some(sym) sym } diff --git a/compiler/test/dotty/tools/backend/jvm/DottyBytecodeTests.scala b/compiler/test/dotty/tools/backend/jvm/DottyBytecodeTests.scala index 0acf19e341bd..9c5f9c167bf9 100644 --- a/compiler/test/dotty/tools/backend/jvm/DottyBytecodeTests.scala +++ b/compiler/test/dotty/tools/backend/jvm/DottyBytecodeTests.scala @@ -971,41 +971,35 @@ class TestBCode extends DottyBytecodeTest { val factMeth = getMethod(fooClass, "fact") assertSameCode(factMeth, List( + Label(0), + VarOp(ILOAD, 1), + Op(ICONST_0), + Jump(IF_ICMPNE, Label(7)), + VarOp(ILOAD, 2), + Jump(GOTO, Label(26)), + Label(7), VarOp(ALOAD, 0), VarOp(ASTORE, 3), - VarOp(ILOAD, 2), + VarOp(ILOAD, 1), + Op(ICONST_1), + Op(ISUB), VarOp(ISTORE, 4), + VarOp(ILOAD, 2), VarOp(ILOAD, 1), + Op(IMUL), VarOp(ISTORE, 5), - Label(6), - VarOp(ILOAD, 5), - Op(ICONST_0), - Jump(IF_ICMPNE, Label(13)), - VarOp(ILOAD, 4), - Jump(GOTO, Label(32)), - Label(13), VarOp(ALOAD, 3), - VarOp(ASTORE, 6), - VarOp(ILOAD, 5), - Op(ICONST_1), - Op(ISUB), - VarOp(ISTORE, 7), + VarOp(ASTORE, 0), VarOp(ILOAD, 4), + VarOp(ISTORE, 1), VarOp(ILOAD, 5), - Op(IMUL), - VarOp(ISTORE, 8), - VarOp(ALOAD, 6), - VarOp(ASTORE, 3), - VarOp(ILOAD, 7), - VarOp(ISTORE, 5), - VarOp(ILOAD, 8), - VarOp(ISTORE, 4), - Jump(GOTO, Label(35)), - Label(32), + VarOp(ISTORE, 2), + Jump(GOTO, Label(29)), + Label(26), Op(IRETURN), - Label(35), - Jump(GOTO, Label(6)), - Op(ATHROW), + Label(29), + Jump(GOTO, Label(0)), + Op(NOP), Op(ATHROW), )) @@ -1015,39 +1009,35 @@ class TestBCode extends DottyBytecodeTest { val sumMeth = getMethod(intListClass, "sum") assertSameCode(sumMeth, List( + Label(0), VarOp(ALOAD, 0), - VarOp(ASTORE, 2), - VarOp(ILOAD, 1), - VarOp(ISTORE, 3), - Label(4), - VarOp(ALOAD, 2), Field(GETFIELD, "IntList", "tail", "LIntList;"), - VarOp(ASTORE, 4), - VarOp(ALOAD, 4), - Jump(IFNONNULL, Label(16)), - VarOp(ILOAD, 3), + VarOp(ASTORE, 2), VarOp(ALOAD, 2), + Jump(IFNONNULL, Label(12)), + VarOp(ILOAD, 1), + VarOp(ALOAD, 0), Field(GETFIELD, "IntList", "head", "I"), Op(IADD), - Jump(GOTO, Label(30)), - Label(16), - VarOp(ALOAD, 4), - VarOp(ASTORE, 5), - VarOp(ILOAD, 3), + Jump(GOTO, Label(26)), + Label(12), VarOp(ALOAD, 2), + VarOp(ASTORE, 3), + VarOp(ILOAD, 1), + VarOp(ALOAD, 0), Field(GETFIELD, "IntList", "head", "I"), Op(IADD), - VarOp(ISTORE, 6), - VarOp(ALOAD, 5), - VarOp(ASTORE, 2), - VarOp(ILOAD, 6), - VarOp(ISTORE, 3), - Jump(GOTO, Label(33)), - Label(30), + VarOp(ISTORE, 4), + VarOp(ALOAD, 3), + VarOp(ASTORE, 0), + VarOp(ILOAD, 4), + VarOp(ISTORE, 1), + Jump(GOTO, Label(29)), + Label(26), Op(IRETURN), - Label(33), - Jump(GOTO, Label(4)), - Op(ATHROW), + Label(29), + Jump(GOTO, Label(0)), + Op(NOP), Op(ATHROW), )) }