From a82248f50ca1fca33a27fa5f5d306075e4012bad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20Doeraene?= Date: Fri, 29 Sep 2023 10:16:12 +0200 Subject: [PATCH 1/2] BCode: Track the exact types on the stack, rather than only the height. --- .../tools/backend/jvm/BCodeBodyBuilder.scala | 113 ++++++++++-------- .../tools/backend/jvm/BCodeSkelBuilder.scala | 56 ++++++++- 2 files changed, 115 insertions(+), 54 deletions(-) diff --git a/compiler/src/dotty/tools/backend/jvm/BCodeBodyBuilder.scala b/compiler/src/dotty/tools/backend/jvm/BCodeBodyBuilder.scala index e7b5a0dad1bf..974f26f7adeb 100644 --- a/compiler/src/dotty/tools/backend/jvm/BCodeBodyBuilder.scala +++ b/compiler/src/dotty/tools/backend/jvm/BCodeBodyBuilder.scala @@ -79,14 +79,14 @@ trait BCodeBodyBuilder extends BCodeSkelBuilder { tree match { case Assign(lhs @ DesugaredSelect(qual, _), rhs) => - val savedStackHeight = stackHeight + val savedStackSize = stack.recordSize() val isStatic = lhs.symbol.isStaticMember if (!isStatic) { - genLoadQualifier(lhs) - stackHeight += 1 + val qualTK = genLoad(qual) + stack.push(qualTK) } genLoad(rhs, symInfoTK(lhs.symbol)) - stackHeight = savedStackHeight + stack.restoreSize(savedStackSize) lineNumber(tree) // receiverClass is used in the bytecode to access the field. using sym.owner may lead to IllegalAccessError val receiverClass = qual.tpe.typeSymbol @@ -150,9 +150,9 @@ trait BCodeBodyBuilder extends BCodeSkelBuilder { } genLoad(larg, resKind) - stackHeight += resKind.size + stack.push(resKind) genLoad(rarg, if (isShift) INT else resKind) - stackHeight -= resKind.size + stack.pop() (code: @switch) match { case ADD => bc add resKind @@ -189,19 +189,19 @@ trait BCodeBodyBuilder extends BCodeSkelBuilder { if (isArrayGet(code)) { // load argument on stack assert(args.length == 1, s"Too many arguments for array get operation: $tree"); - stackHeight += 1 + stack.push(k) genLoad(args.head, INT) - stackHeight -= 1 + stack.pop() generatedType = k.asArrayBType.componentType bc.aload(elementType) } else if (isArraySet(code)) { val List(a1, a2) = args - stackHeight += 1 + stack.push(k) genLoad(a1, INT) - stackHeight += 1 + stack.push(INT) genLoad(a2) - stackHeight -= 2 + stack.pop(2) generatedType = UNIT bc.astore(elementType) } else { @@ -235,7 +235,7 @@ trait BCodeBodyBuilder extends BCodeSkelBuilder { val resKind = if (hasUnitBranch) UNIT else tpeTK(tree) val postIf = new asm.Label - genLoadTo(thenp, resKind, LoadDestination.Jump(postIf, stackHeight)) + genLoadTo(thenp, resKind, LoadDestination.Jump(postIf, stack.recordSize())) markProgramPoint(failure) genLoadTo(elsep, resKind, LoadDestination.FallThrough) markProgramPoint(postIf) @@ -294,8 +294,10 @@ trait BCodeBodyBuilder extends BCodeSkelBuilder { ) } - def genLoad(tree: Tree): Unit = { - genLoad(tree, tpeTK(tree)) + def genLoad(tree: Tree): BType = { + val generatedType = tpeTK(tree) + genLoad(tree, generatedType) + generatedType } /* Generate code for trees that produce values on the stack */ @@ -364,6 +366,7 @@ trait BCodeBodyBuilder extends BCodeSkelBuilder { case t @ Ident(_) => (t, Nil) } + val savedStackSize = stack.recordSize() if (!fun.symbol.isStaticMember) { // load receiver of non-static implementation of lambda @@ -372,10 +375,12 @@ trait BCodeBodyBuilder extends BCodeSkelBuilder { // AbstractValidatingLambdaMetafactory.validateMetafactoryArgs val DesugaredSelect(prefix, _) = fun: @unchecked - genLoad(prefix) + val prefixTK = genLoad(prefix) + stack.push(prefixTK) } genLoadArguments(env, fun.symbol.info.firstParamTypes map toTypeKind) + stack.restoreSize(savedStackSize) generatedType = genInvokeDynamicLambda(NoSymbol, fun.symbol, env.size, functionalInterface) case app @ Apply(_, _) => @@ -494,9 +499,9 @@ trait BCodeBodyBuilder extends BCodeSkelBuilder { dest match case LoadDestination.FallThrough => () - case LoadDestination.Jump(label, targetStackHeight) => - if targetStackHeight < stackHeight then - val stackDiff = stackHeight - targetStackHeight + case LoadDestination.Jump(label, targetStackSize) => + val stackDiff = stack.heightDiffWrt(targetStackSize) + if stackDiff != 0 then if expectedType == UNIT then bc dropMany stackDiff else @@ -599,7 +604,7 @@ trait BCodeBodyBuilder extends BCodeSkelBuilder { if dest == LoadDestination.FallThrough then val resKind = tpeTK(tree) val jumpTarget = new asm.Label - registerJumpDest(labelSym, resKind, LoadDestination.Jump(jumpTarget, stackHeight)) + registerJumpDest(labelSym, resKind, LoadDestination.Jump(jumpTarget, stack.recordSize())) genLoad(expr, resKind) markProgramPoint(jumpTarget) resKind @@ -657,7 +662,7 @@ trait BCodeBodyBuilder extends BCodeSkelBuilder { markProgramPoint(loop) if isInfinite then - val dest = LoadDestination.Jump(loop, stackHeight) + val dest = LoadDestination.Jump(loop, stack.recordSize()) genLoadTo(body, UNIT, dest) dest else @@ -672,7 +677,7 @@ trait BCodeBodyBuilder extends BCodeSkelBuilder { val failure = new asm.Label genCond(cond, success, failure, targetIfNoJump = success) markProgramPoint(success) - genLoadTo(body, UNIT, LoadDestination.Jump(loop, stackHeight)) + genLoadTo(body, UNIT, LoadDestination.Jump(loop, stack.recordSize())) markProgramPoint(failure) end match LoadDestination.FallThrough @@ -765,10 +770,10 @@ trait BCodeBodyBuilder extends BCodeSkelBuilder { // on the stack (contrary to what the type in the AST says). // scala/bug#10290: qual can be `this.$outer()` (not just `this`), so we call genLoad (not just ALOAD_0) - genLoad(superQual) - stackHeight += 1 + val superQualTK = genLoad(superQual) + stack.push(superQualTK) genLoadArguments(args, paramTKs(app)) - stackHeight -= 1 + stack.pop() generatedType = genCallMethod(fun.symbol, InvokeStyle.Super, app.span) // 'new' constructor call: Note: since constructors are @@ -790,9 +795,10 @@ trait BCodeBodyBuilder extends BCodeSkelBuilder { assert(classBTypeFromSymbol(ctor.owner) == rt, s"Symbol ${ctor.owner.showFullName} is different from $rt") mnode.visitTypeInsn(asm.Opcodes.NEW, rt.internalName) bc dup generatedType - stackHeight += 2 + stack.push(rt) + stack.push(rt) genLoadArguments(args, paramTKs(app)) - stackHeight -= 2 + stack.pop(2) genCallMethod(ctor, InvokeStyle.Special, app.span) case _ => @@ -825,12 +831,11 @@ trait BCodeBodyBuilder extends BCodeSkelBuilder { else if (app.hasAttachment(BCodeHelpers.UseInvokeSpecial)) InvokeStyle.Special else InvokeStyle.Virtual - val savedStackHeight = stackHeight + val savedStackSize = stack.recordSize() if invokeStyle.hasInstance then - genLoadQualifier(fun) - stackHeight += 1 + stack.push(genLoadQualifier(fun)) genLoadArguments(args, paramTKs(app)) - stackHeight = savedStackHeight + stack.restoreSize(savedStackSize) val DesugaredSelect(qual, name) = fun: @unchecked // fun is a Select, also checked in genLoadQualifier val isArrayClone = name == nme.clone_ && qual.tpe.widen.isInstanceOf[JavaArrayType] @@ -888,7 +893,10 @@ trait BCodeBodyBuilder extends BCodeSkelBuilder { bc iconst elems.length bc newarray elmKind - stackHeight += 3 // during the genLoad below, there is the result, its dup, and the index + // during the genLoad below, there is the result, its dup, and the index + stack.push(generatedType) + stack.push(generatedType) + stack.push(INT) var i = 0 var rest = elems @@ -901,7 +909,7 @@ trait BCodeBodyBuilder extends BCodeSkelBuilder { i = i + 1 } - stackHeight -= 3 + stack.pop(3) generatedType } @@ -917,7 +925,7 @@ trait BCodeBodyBuilder extends BCodeSkelBuilder { val (generatedType, postMatch, postMatchDest) = if dest == LoadDestination.FallThrough then val postMatch = new asm.Label - (tpeTK(tree), postMatch, LoadDestination.Jump(postMatch, stackHeight)) + (tpeTK(tree), postMatch, LoadDestination.Jump(postMatch, stack.recordSize())) else (expectedType, null, dest) @@ -1179,7 +1187,7 @@ trait BCodeBodyBuilder extends BCodeSkelBuilder { } /* Emit code to Load the qualifier of `tree` on top of the stack. */ - def genLoadQualifier(tree: Tree): Unit = { + def genLoadQualifier(tree: Tree): BType = { lineNumber(tree) tree match { case DesugaredSelect(qualifier, _) => genLoad(qualifier) @@ -1188,6 +1196,7 @@ trait BCodeBodyBuilder extends BCodeSkelBuilder { case Some(sel) => genLoadQualifier(sel) case None => assert(t.symbol.owner == this.claszSymbol) + UNIT } case _ => abort(s"Unknown qualifier $tree") } @@ -1200,14 +1209,14 @@ trait BCodeBodyBuilder extends BCodeSkelBuilder { btpes match case btpe :: btpes1 => genLoad(arg, btpe) - stackHeight += btpe.size + stack.push(btpe) loop(args1, btpes1) case _ => case _ => - val savedStackHeight = stackHeight + val savedStackSize = stack.recordSize() loop(args, btpes) - stackHeight = savedStackHeight + stack.restoreSize(savedStackSize) end genLoadArguments def genLoadModule(tree: Tree): BType = { @@ -1307,13 +1316,13 @@ trait BCodeBodyBuilder extends BCodeSkelBuilder { }.sum bc.genNewStringBuilder(approxBuilderSize) - stackHeight += 1 // during the genLoad below, there is a reference to the StringBuilder on the stack + stack.push(jlStringBuilderRef) // during the genLoad below, there is a reference to the StringBuilder on the stack for (elem <- concatArguments) { val elemType = tpeTK(elem) genLoad(elem, elemType) bc.genStringBuilderAppend(elemType) } - stackHeight -= 1 + stack.pop() bc.genStringBuilderEnd } else { @@ -1331,7 +1340,7 @@ trait BCodeBodyBuilder extends BCodeSkelBuilder { var totalArgSlots = 0 var countConcats = 1 // ie. 1 + how many times we spilled - val savedStackHeight = stackHeight + val savedStackSize = stack.recordSize() for (elem <- concatArguments) { val tpe = tpeTK(elem) @@ -1339,7 +1348,9 @@ trait BCodeBodyBuilder extends BCodeSkelBuilder { // Unlikely spill case if (totalArgSlots + elemSlots >= MaxIndySlots) { - stackHeight = savedStackHeight + countConcats + stack.restoreSize(savedStackSize) + for _ <- 0 until countConcats do + stack.push(StringRef) bc.genIndyStringConcat(recipe.toString, argTypes.result(), constVals.result()) countConcats += 1 totalArgSlots = 0 @@ -1364,10 +1375,10 @@ trait BCodeBodyBuilder extends BCodeSkelBuilder { val tpe = tpeTK(elem) argTypes += tpe.toASMType genLoad(elem, tpe) - stackHeight += 1 + stack.push(tpe) } } - stackHeight = savedStackHeight + stack.restoreSize(savedStackSize) bc.genIndyStringConcat(recipe.toString, argTypes.result(), constVals.result()) // If we spilled, generate one final concat @@ -1562,9 +1573,9 @@ trait BCodeBodyBuilder extends BCodeSkelBuilder { } else { val tk = tpeTK(l).maxType(tpeTK(r)) genLoad(l, tk) - stackHeight += tk.size + stack.push(tk) genLoad(r, tk) - stackHeight -= tk.size + stack.pop() genCJUMP(success, failure, op, tk, targetIfNoJump) } } @@ -1679,9 +1690,9 @@ trait BCodeBodyBuilder extends BCodeSkelBuilder { } genLoad(l, ObjectRef) - stackHeight += 1 + stack.push(ObjectRef) genLoad(r, ObjectRef) - stackHeight -= 1 + stack.pop() genCallMethod(equalsMethod, InvokeStyle.Static) genCZJUMP(success, failure, Primitives.NE, BOOL, targetIfNoJump) } @@ -1697,9 +1708,9 @@ trait BCodeBodyBuilder extends BCodeSkelBuilder { } else if (isNonNullExpr(l)) { // SI-7852 Avoid null check if L is statically non-null. genLoad(l, ObjectRef) - stackHeight += 1 + stack.push(ObjectRef) genLoad(r, ObjectRef) - stackHeight -= 1 + stack.pop() genCallMethod(defn.Any_equals, InvokeStyle.Virtual) genCZJUMP(success, failure, Primitives.NE, BOOL, targetIfNoJump) } else { @@ -1709,9 +1720,9 @@ trait BCodeBodyBuilder extends BCodeSkelBuilder { val lNonNull = new asm.Label genLoad(l, ObjectRef) - stackHeight += 1 + stack.push(ObjectRef) genLoad(r, ObjectRef) - stackHeight -= 1 + stack.pop() locals.store(eqEqTempLocal) bc dup ObjectRef genCZJUMP(lNull, lNonNull, Primitives.EQ, ObjectRef, targetIfNoJump = lNull) diff --git a/compiler/src/dotty/tools/backend/jvm/BCodeSkelBuilder.scala b/compiler/src/dotty/tools/backend/jvm/BCodeSkelBuilder.scala index f892c2bb753c..49ff83aa716a 100644 --- a/compiler/src/dotty/tools/backend/jvm/BCodeSkelBuilder.scala +++ b/compiler/src/dotty/tools/backend/jvm/BCodeSkelBuilder.scala @@ -40,12 +40,62 @@ trait BCodeSkelBuilder extends BCodeHelpers { lazy val NativeAttr: Symbol = requiredClass[scala.native] + final class BTypesStack: + // Anecdotally, growing past 16 to 32 is common; growing past 32 is rare + private var stack = new Array[BType](32) + private var size = 0 + + def push(btype: BType): Unit = + if size == stack.length then + stack = java.util.Arrays.copyOf(stack, stack.length * 2) + stack(size) = btype + size += 1 + + def pop(): Unit = pop(1) + + def pop(count: Int): Unit = + assert(size >= count) + size -= count + + def height: Int = heightBetween(0, size) + + private def heightBetween(start: Int, end: Int): Int = + var result = 0 + var i = start + while i != end do + result += stack(i).size + i += 1 + result + + def recordSize(): BTypesStack.Size = BTypesStack.intToSize(size) + + def restoreSize(targetSize: BTypesStack.Size): Unit = + val targetSize1 = BTypesStack.sizeToInt(targetSize) + assert(size >= targetSize1) + size = targetSize1 + + def heightDiffWrt(targetSize: BTypesStack.Size): Int = + val targetSize1 = BTypesStack.sizeToInt(targetSize) + assert(size >= targetSize1) + heightBetween(targetSize1, size) + + def clear(): Unit = + size = 0 + end BTypesStack + + object BTypesStack: + opaque type Size = Int + + private def intToSize(size: Int): Size = size + private def sizeToInt(size: Size): Int = size + end BTypesStack + /** The destination of a value generated by `genLoadTo`. */ enum LoadDestination: /** The value is put on the stack, and control flows through to the next opcode. */ case FallThrough /** The value is put on the stack, and control flow is transferred to the given `label`. */ - case Jump(label: asm.Label, targetStackHeight: Int) + case Jump(label: asm.Label, targetStackSize: BTypesStack.Size) /** The value is RETURN'ed from the enclosing method. */ case Return /** The value is ATHROW'n. */ @@ -369,7 +419,7 @@ trait BCodeSkelBuilder extends BCodeHelpers { var earlyReturnVar: Symbol = null var shouldEmitCleanup = false // stack tracking - var stackHeight = 0 + val stack = new BTypesStack // line numbers var lastEmittedLineNr = -1 @@ -589,7 +639,7 @@ trait BCodeSkelBuilder extends BCodeHelpers { earlyReturnVar = null shouldEmitCleanup = false - stackHeight = 0 + stack.clear() lastEmittedLineNr = -1 } From 52e8e74c3b7e92cc0244145714ac2ce97d78f7cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20Doeraene?= Date: Fri, 29 Sep 2023 13:25:37 +0200 Subject: [PATCH 2/2] Opt: Get rid of the LiftTry phase; instead handle things in the back-end. When we enter a `try-catch` at the JVM level, we have to make sure that the stack is empty. That's because, upon exception, the JVM wipes the stack, and we must not lose operands that are already on the stack that we will still use. Previously, this was achieved with a transformation phase, `LiftTry`, which lifted problematic `try-catch`es in local `def`s, called `liftedTree$x`. It analyzed the tree to predict which `try-catch`es would execute on a non-empty stack when eventually compiled to the JVM. This approach has several shortcomings. It exhibits performance cliffs, as the generated def can then cause more variables to be boxed in to `XRef`s. These were the only extra defs created for implementation reasons rather than for language reasons. As a user of the language, it is hard to predict when such a lifted def will be needed. The additional `liftedTree` methods also show up on stack traces and obfuscate them. Debugging can be severely hampered as well. Phases executing after `LiftTry`, notably `CapturedVars`, also had to take care not to create more problematic situations as a result of their transformations, which is hard to predict and to remember. Finally, Scala.js and Scala Native do not have the same restriction, so they received suboptimal code for no reason. In this commit, we entirely remove the `LiftTry` phase. Instead, we enhance the JVM back-end to deal with the situation. When starting a `try-catch` on a non-empty stack, we stash the entire contents of the stack into local variables. After the `try-catch`, we pop all those local variables back onto the stack. We also null out the leftover vars not to prevent garbage collection. This new approach solves all of the problems mentioned above. --- .../tools/backend/jvm/BCodeSkelBuilder.scala | 12 +++ .../tools/backend/jvm/BCodeSyncAndTry.scala | 58 +++++++++++- compiler/src/dotty/tools/dotc/Compiler.scala | 1 - .../src/dotty/tools/dotc/core/NameKinds.scala | 1 - .../tools/dotc/transform/CapturedVars.scala | 38 +------- .../dotty/tools/dotc/transform/LiftTry.scala | 88 ------------------- docs/_docs/internals/overall-structure.md | 1 - tests/neg/t1672b.scala | 1 - tests/run/i1692.scala | 2 +- tests/run/i1692b.scala | 2 +- tests/run/i4866.check | 2 - tests/run/i4866.scala | 21 ----- 12 files changed, 75 insertions(+), 152 deletions(-) delete mode 100644 compiler/src/dotty/tools/dotc/transform/LiftTry.scala delete mode 100644 tests/run/i4866.check delete mode 100644 tests/run/i4866.scala diff --git a/compiler/src/dotty/tools/backend/jvm/BCodeSkelBuilder.scala b/compiler/src/dotty/tools/backend/jvm/BCodeSkelBuilder.scala index 49ff83aa716a..70d4d758d83b 100644 --- a/compiler/src/dotty/tools/backend/jvm/BCodeSkelBuilder.scala +++ b/compiler/src/dotty/tools/backend/jvm/BCodeSkelBuilder.scala @@ -45,6 +45,8 @@ trait BCodeSkelBuilder extends BCodeHelpers { private var stack = new Array[BType](32) private var size = 0 + def isEmpty: Boolean = size == 0 + def push(btype: BType): Unit = if size == stack.length then stack = java.util.Arrays.copyOf(stack, stack.length * 2) @@ -81,6 +83,16 @@ trait BCodeSkelBuilder extends BCodeHelpers { def clear(): Unit = size = 0 + + def acquireFullStack(): IArray[BType] = + val res = IArray.unsafeFromArray(stack.slice(0, size)) + size = 0 + res + + def restoreFullStack(fullStack: IArray[BType]): Unit = + assert(size == 0 && stack.length >= fullStack.length) + fullStack.copyToArray(stack) + size = fullStack.length end BTypesStack object BTypesStack: diff --git a/compiler/src/dotty/tools/backend/jvm/BCodeSyncAndTry.scala b/compiler/src/dotty/tools/backend/jvm/BCodeSyncAndTry.scala index b5ed27511e7e..74e1c5812b14 100644 --- a/compiler/src/dotty/tools/backend/jvm/BCodeSyncAndTry.scala +++ b/compiler/src/dotty/tools/backend/jvm/BCodeSyncAndTry.scala @@ -118,6 +118,11 @@ trait BCodeSyncAndTry extends BCodeBodyBuilder { /* * Emitting try-catch is easy, emitting try-catch-finally not quite so. + * + * For a try-catch, the only thing we need to care about is to stash the stack away + * in local variables and load them back in afterwards, in case the incoming stack + * is not empty. + * * A finally-block (which always has type Unit, thus leaving the operand stack unchanged) * affects control-transfer from protected regions, as follows: * @@ -190,7 +195,7 @@ trait BCodeSyncAndTry extends BCodeBodyBuilder { } } - // ------ (0) locals used later ------ + // ------ locals used later ------ /* * `postHandlers` is a program point denoting: @@ -203,6 +208,13 @@ trait BCodeSyncAndTry extends BCodeBodyBuilder { */ val postHandlers = new asm.Label + // stack stash + val needStackStash = !stack.isEmpty && !caseHandlers.isEmpty + val acquiredStack = if needStackStash then stack.acquireFullStack() else null + val stashLocals = + if acquiredStack == null then null + else acquiredStack.uncheckedNN.filter(_ != UNIT).map(btpe => locals.makeTempLocal(btpe)) + val hasFinally = (finalizer != tpd.EmptyTree) /* @@ -222,6 +234,17 @@ trait BCodeSyncAndTry extends BCodeBodyBuilder { */ val finCleanup = if (hasFinally) new asm.Label else null + /* ------ (0) Stash the stack into local variables, if necessary. + * From top of the stack down to the bottom. + * ------ + */ + + if stashLocals != null then + val stashLocalsNN = stashLocals.uncheckedNN // why is this necessary? + for i <- (stashLocalsNN.length - 1) to 0 by -1 do + val local = stashLocalsNN(i) + bc.store(local.idx, local.tk) + /* ------ (1) try-block, protected by: * (1.a) the EHs due to case-clauses, emitted in (2), * (1.b) the EH due to finally-clause, emitted in (3.A) @@ -367,6 +390,39 @@ trait BCodeSyncAndTry extends BCodeBodyBuilder { emitFinalizer(finalizer, tmp, isDuplicate = false) // the only invocation of emitFinalizer with `isDuplicate == false` } + /* ------ (5) Unstash the stack, if it was stashed before. + * From bottom of the stack to the top. + * If there is a non-UNIT result, we need to temporarily store + * that one in a local variable while we unstash. + * ------ + */ + + if stashLocals != null then + val stashLocalsNN = stashLocals.uncheckedNN // why is this necessary? + + val resultLoc = + if kind == UNIT then null + else if tmp != null then locals(tmp) // reuse the same local + else locals.makeTempLocal(kind) + if resultLoc != null then + bc.store(resultLoc.idx, kind) + + for i <- 0 until stashLocalsNN.size do + val local = stashLocalsNN(i) + bc.load(local.idx, local.tk) + if local.tk.isRef then + bc.emit(asm.Opcodes.ACONST_NULL) + bc.store(local.idx, local.tk) + + stack.restoreFullStack(acquiredStack.nn) + + if resultLoc != null then + bc.load(resultLoc.idx, kind) + if kind.isRef then + bc.emit(asm.Opcodes.ACONST_NULL) + bc.store(resultLoc.idx, kind) + end if // stashLocals != null + kind } // end of genLoadTry() diff --git a/compiler/src/dotty/tools/dotc/Compiler.scala b/compiler/src/dotty/tools/dotc/Compiler.scala index 126e54a7b49e..d34fa247b155 100644 --- a/compiler/src/dotty/tools/dotc/Compiler.scala +++ b/compiler/src/dotty/tools/dotc/Compiler.scala @@ -101,7 +101,6 @@ class Compiler { new Getters, // Replace non-private vals and vars with getter defs (fields are added later) new SpecializeFunctions, // Specialized Function{0,1,2} by replacing super with specialized super new SpecializeTuples, // Specializes Tuples by replacing tuple construction and selection trees - new LiftTry, // Put try expressions that might execute on non-empty stacks into their own methods new CollectNullableFields, // Collect fields that can be nulled out after use in lazy initialization new ElimOuterSelect, // Expand outer selections new ResolveSuper, // Implement super accessors diff --git a/compiler/src/dotty/tools/dotc/core/NameKinds.scala b/compiler/src/dotty/tools/dotc/core/NameKinds.scala index 6210553bda43..3e8d398d05d6 100644 --- a/compiler/src/dotty/tools/dotc/core/NameKinds.scala +++ b/compiler/src/dotty/tools/dotc/core/NameKinds.scala @@ -316,7 +316,6 @@ object NameKinds { val TailTempName: UniqueNameKind = new UniqueNameKind("$tmp") val ExceptionBinderName: UniqueNameKind = new UniqueNameKind("ex") val SkolemName: UniqueNameKind = new UniqueNameKind("?") - val LiftedTreeName: UniqueNameKind = new UniqueNameKind("liftedTree") val SuperArgName: UniqueNameKind = new UniqueNameKind("$superArg$") val DocArtifactName: UniqueNameKind = new UniqueNameKind("$doc") val UniqueInlineName: UniqueNameKind = new UniqueNameKind("$i") diff --git a/compiler/src/dotty/tools/dotc/transform/CapturedVars.scala b/compiler/src/dotty/tools/dotc/transform/CapturedVars.scala index 32bcc53184b1..8964beb26051 100644 --- a/compiler/src/dotty/tools/dotc/transform/CapturedVars.scala +++ b/compiler/src/dotty/tools/dotc/transform/CapturedVars.scala @@ -25,9 +25,6 @@ class CapturedVars extends MiniPhase with IdentityDenotTransformer: override def description: String = CapturedVars.description - override def runsAfterGroupsOf: Set[String] = Set(LiftTry.name) - // lifting tries changes what variables are considered to be captured - private[this] var Captured: Store.Location[util.ReadOnlySet[Symbol]] = _ private def captured(using Context) = ctx.store(Captured) @@ -131,42 +128,15 @@ class CapturedVars extends MiniPhase with IdentityDenotTransformer: * * intRef.elem = expr * - * rewrite using a temporary var to - * - * val ev$n = expr - * intRef.elem = ev$n - * - * That way, we avoid the problem that `expr` might contain a `try` that would - * run on a non-empty stack (which is illegal under JVM rules). Note that LiftTry - * has already run before, so such `try`s would not be eliminated. - * - * If the ref type lhs is followed by a cast (can be an artifact of nested translation), - * drop the cast. - * - * If the ref type is `ObjectRef` or `VolatileObjectRef`, immediately assign `null` - * to the temporary to make the underlying target of the reference available for - * garbage collection. Nullification is omitted if the `expr` is already `null`. - * - * var ev$n: RHS = expr - * objRef.elem = ev$n - * ev$n = null.asInstanceOf[RHS] + * the lhs can be followed by a cast as an artifact of nested translation. + * In that case, drop the cast. */ override def transformAssign(tree: Assign)(using Context): Tree = - def absolved: Boolean = tree.rhs match - case Literal(Constant(null)) | Typed(Literal(Constant(null)), _) => true - case _ => false - def recur(lhs: Tree): Tree = lhs match + tree.lhs match case TypeApply(Select(qual@Select(_, nme.elem), nme.asInstanceOf_), _) => - recur(qual) - case Select(_, nme.elem) if refInfo.boxedRefClasses.contains(lhs.symbol.maybeOwner) => - val tempDef = transformFollowing(SyntheticValDef(TempResultName.fresh(), tree.rhs, flags = Mutable)) - val update = cpy.Assign(tree)(lhs, ref(tempDef.symbol)) - def reset = cpy.Assign(tree)(ref(tempDef.symbol), nullLiteral.cast(tempDef.symbol.info)) - val res = if refInfo.objectRefClasses(lhs.symbol.maybeOwner) && !absolved then reset else unitLiteral - transformFollowing(Block(tempDef :: update :: Nil, res)) + cpy.Assign(tree)(qual, tree.rhs) case _ => tree - recur(tree.lhs) object CapturedVars: val name: String = "capturedVars" diff --git a/compiler/src/dotty/tools/dotc/transform/LiftTry.scala b/compiler/src/dotty/tools/dotc/transform/LiftTry.scala deleted file mode 100644 index 6acb1013d509..000000000000 --- a/compiler/src/dotty/tools/dotc/transform/LiftTry.scala +++ /dev/null @@ -1,88 +0,0 @@ -package dotty.tools.dotc -package transform - -import MegaPhase._ -import core.DenotTransformers._ -import core.Symbols._ -import core.Contexts._ -import core.Types._ -import core.Flags._ -import core.Decorators._ -import core.NameKinds.LiftedTreeName -import NonLocalReturns._ -import util.Store - -/** Lifts try's that might be executed on non-empty expression stacks - * to their own methods. I.e. - * - * try body catch handler - * - * is lifted to - * - * { def liftedTree$n() = try body catch handler; liftedTree$n() } - * - * However, don't lift try's without catch expressions (try-finally). - * Lifting is needed only for try-catch expressions that are evaluated in a context - * where the stack might not be empty. `finally` does not attempt to continue evaluation - * after an exception, so the fact that values on the stack are 'lost' does not matter - * (copied from https://github.com/scala/scala/pull/922). - */ -class LiftTry extends MiniPhase with IdentityDenotTransformer { thisPhase => - import ast.tpd._ - - override def phaseName: String = LiftTry.name - - override def description: String = LiftTry.description - - private var NeedLift: Store.Location[Boolean] = _ - private def needLift(using Context): Boolean = ctx.store(NeedLift) - - override def initContext(ctx: FreshContext): Unit = - NeedLift = ctx.addLocation(false) - - private def liftingCtx(p: Boolean)(using Context) = - if (needLift == p) ctx else ctx.fresh.updateStore(NeedLift, p) - - override def prepareForApply(tree: Apply)(using Context): Context = - liftingCtx(true) - - override def prepareForDefDef(tree: DefDef)(using Context): Context = - liftingCtx(false) - - override def prepareForValDef(tree: ValDef)(using Context): Context = - if !tree.symbol.exists - || tree.symbol.isSelfSym - || tree.symbol.owner == ctx.owner.enclosingMethod - && !tree.symbol.is(Lazy) - // The current implementation wraps initializers of lazy vals in - // calls to an initialize method, which means that a `try` in the - // initializer needs to be lifted. Note that the new scheme proposed - // in #6979 would avoid this. - then ctx - else liftingCtx(true) - - override def prepareForAssign(tree: Assign)(using Context): Context = - if (tree.lhs.symbol.maybeOwner == ctx.owner.enclosingMethod) ctx - else liftingCtx(true) - - override def prepareForReturn(tree: Return)(using Context): Context = - if (!isNonLocalReturn(tree)) ctx - else liftingCtx(true) - - override def prepareForTemplate(tree: Template)(using Context): Context = - liftingCtx(false) - - override def transformTry(tree: Try)(using Context): Tree = - if (needLift && tree.cases.nonEmpty) { - report.debuglog(i"lifting tree at ${tree.span}, current owner = ${ctx.owner}") - val fn = newSymbol( - ctx.owner, LiftedTreeName.fresh(), Synthetic | Method, - MethodType(Nil, tree.tpe.widenIfUnstable), coord = tree.span) - tree.changeOwnerAfter(ctx.owner, fn, thisPhase) - Block(DefDef(fn, tree) :: Nil, ref(fn).appliedToNone) - } - else tree -} -object LiftTry: - val name = "liftTry" - val description: String = "lift any try that might be executed on a non-empty expression stack" diff --git a/docs/_docs/internals/overall-structure.md b/docs/_docs/internals/overall-structure.md index 5bb43eb946a8..ab936ddd8512 100644 --- a/docs/_docs/internals/overall-structure.md +++ b/docs/_docs/internals/overall-structure.md @@ -151,7 +151,6 @@ phases. The current list of phases is specified in class [Compiler] as follows: new InterceptedMethods, // Special handling of `==`, `|=`, `getClass` methods new Getters, // Replace non-private vals and vars with getter defs (fields are added later) new SpecializeFunctions, // Specialized Function{0,1,2} by replacing super with specialized super - new LiftTry, // Put try expressions that might execute on non-empty stacks into their own methods new CollectNullableFields, // Collect fields that can be nulled out after use in lazy initialization new ElimOuterSelect, // Expand outer selections new ResolveSuper, // Implement super accessors diff --git a/tests/neg/t1672b.scala b/tests/neg/t1672b.scala index 84ebb6155e6d..b67664615048 100644 --- a/tests/neg/t1672b.scala +++ b/tests/neg/t1672b.scala @@ -41,7 +41,6 @@ object Test1772B { } } - // the `liftedTree` local method will prevent a tail call here. @tailrec def bar(i : Int) : Int = { if (i == 0) 0 diff --git a/tests/run/i1692.scala b/tests/run/i1692.scala index 09459a073d6e..9416e6101efd 100644 --- a/tests/run/i1692.scala +++ b/tests/run/i1692.scala @@ -22,7 +22,7 @@ class LazyNullable(a: => Int) { lazy val l4Inf = eInf private[this] val i = "I" - // null out i even though the try ends up lifted, because the LazyVals phase runs before the LiftTry phase + // null out i even though the try needs stack stashing lazy val l5 = try i catch { case e: Exception => () } } diff --git a/tests/run/i1692b.scala b/tests/run/i1692b.scala index bd2108038ef4..8f23a0e5a3b3 100644 --- a/tests/run/i1692b.scala +++ b/tests/run/i1692b.scala @@ -24,7 +24,7 @@ class LazyNullable(a: => Int) { @threadUnsafe lazy val l4Inf = try eInf finally () // null out e, since private[this] is inferred private[this] val i = "I" - // null out i even though the try ends up lifted, because the LazyVals phase runs before the LiftTry phase + // null out i even though the try needs stack stashing @threadUnsafe lazy val l5 = try i catch { case e: Exception => () } } diff --git a/tests/run/i4866.check b/tests/run/i4866.check deleted file mode 100644 index f16e2a9c94df..000000000000 --- a/tests/run/i4866.check +++ /dev/null @@ -1,2 +0,0 @@ -Foo #lifted: 0 -FooLifted #lifted: 1 diff --git a/tests/run/i4866.scala b/tests/run/i4866.scala deleted file mode 100644 index 092770fb21cd..000000000000 --- a/tests/run/i4866.scala +++ /dev/null @@ -1,21 +0,0 @@ -// scalajs: --skip - -// Test that try-finally aren't lifted, but try-catch are. - -class Foo { - val field = try 1 finally () -} - -class FooLifted { - val field = try 1 catch { case e: Exception => () } finally () -} - -object Test extends App { - def numLifted(o: Object) = { - def isLifted(m: java.lang.reflect.Method) = m.getName.startsWith("lifted") - o.getClass.getDeclaredMethods.count(isLifted) - } - - println("Foo #lifted: " + numLifted(new Foo)) - println("FooLifted #lifted: " + numLifted(new FooLifted)) -}