Skip to content

Commit

Permalink
Null out local vars created by the inliner
Browse files Browse the repository at this point in the history
This should fix scala/bug#9137
  • Loading branch information
lrytz committed Aug 25, 2018
1 parent e1f6d2e commit d384e38
Show file tree
Hide file tree
Showing 4 changed files with 18 additions and 6 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,6 @@ import scala.tools.nsc.backend.jvm.opt.BytecodeUtils._
* call). However, the receiver is an value on the stack and consumed while interpreting the
* instruction - so we can only gain some knowledge if we know that the receiver was an alias of
* some other local variable or stack slot. Therefore we use the AliasingFrame class.
*
* TODO:
* Finally, we'd also like to exploit the knowledge gained from `if (x == null)` tests: x is known
* to be null in one branch, not null in the other. This will make use of alias tracking as well.
* We still have to figure out how to do this exactly in the analyzer framework.
*/

/**
Expand Down
16 changes: 15 additions & 1 deletion src/compiler/scala/tools/nsc/backend/jvm/opt/Inliner.scala
Original file line number Diff line number Diff line change
Expand Up @@ -671,6 +671,7 @@ abstract class Inliner {

// add a STORE instruction for each expected argument, including for THIS instance if any
val argStores = new InsnList
val nullOutArgLocals = new InsnList
var nextLocalIndex = callsiteMethod.maxLocals
if (!isStaticMethod(callee)) {
if (!receiverKnownNotNull) {
Expand All @@ -682,6 +683,8 @@ abstract class Inliner {
argStores.add(nonNullLabel)
}
argStores.add(new VarInsnNode(ASTORE, nextLocalIndex))
nullOutArgLocals.add(new InsnNode(ACONST_NULL))
nullOutArgLocals.add(new VarInsnNode(ASTORE, nextLocalIndex))
nextLocalIndex += 1
}

Expand All @@ -692,9 +695,15 @@ abstract class Inliner {
for(argTp <- calleeParamTypes) {
val opc = argTp.getOpcode(ISTORE) // returns the correct xSTORE instruction for argTp
argStores.insert(new VarInsnNode(opc, nextLocalIndex)) // "insert" is "prepend" - the last argument is on the top of the stack
if (opc == ASTORE) {
nullOutArgLocals.add(new InsnNode(ACONST_NULL))
nullOutArgLocals.add(new VarInsnNode(ASTORE, nextLocalIndex))
}
nextLocalIndex += argTp.getSize
}

val hasNullOutInsn = nullOutArgLocals.size > 0

clonedInstructions.insert(argStores)

// label for the exit of the inlined functions. xRETURNs are replaced by GOTOs to this label.
Expand All @@ -714,6 +723,8 @@ abstract class Inliner {
}
}

clonedInstructions.add(nullOutArgLocals)

// replace xRETURNs:
// - store the return value (if any)
// - clear the stack of the inlined method (insert DROPs)
Expand Down Expand Up @@ -794,7 +805,10 @@ abstract class Inliner {
// When adding a null check for the receiver, a DUP is inserted, which might cause a new maxStack.
// If the callsite has other argument values than the receiver on the stack, these are pop'ed
// and stored into locals before the null check, so in that case the maxStack doesn't grow.
val stackSlotForNullCheck = if (!isStaticMethod(callee) && !receiverKnownNotNull && calleeParamTypes.isEmpty) 1 else 0
val stackSlotForNullCheck =
if (!isStaticMethod(callee) && !receiverKnownNotNull && calleeParamTypes.isEmpty) 1
else if (hasNullOutInsn) 1
else 0
callsiteStackHeight + stackSlotForNullCheck
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import org.openjdk.jmh.infra.Blackhole

import scala.collection.JavaConverters.asScalaIteratorConverter
import scala.tools.asm.tree.ClassNode
import scala.tools.nsc.backend.jvm.analysis.ProdConsAnalyzer

@BenchmarkMode(Array(Mode.AverageTime))
@Fork(2)
Expand Down
2 changes: 2 additions & 0 deletions test/junit/scala/tools/nsc/backend/jvm/opt/InlinerTest.scala
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ class InlinerTest extends BytecodeTesting {
VarOp(ALOAD, 0), VarOp(ASTORE, 1), // store this
Op(ICONST_1), VarOp(ISTORE, 2), Jump(GOTO, Label(10)), // store return value
Label(10), VarOp(ILOAD, 2), // load return value
Op(ACONST_NULL), VarOp(ASTORE, 1), // null out inliner local
VarOp(ALOAD, 0), Invoke(INVOKEVIRTUAL, "C", "f", "()I", false), Op(IADD), Op(IRETURN)))

// line numbers are kept, so there's a line 2 (from the inlined f)
Expand Down Expand Up @@ -117,6 +118,7 @@ class InlinerTest extends BytecodeTesting {
Jump(GOTO, Label(11)),
Label(11),
VarOp(ALOAD, 2),
Op(ACONST_NULL), VarOp(ASTORE, 1),
Op(ATHROW))

assertSameCode(convertMethod(g), gBeforeLocalOpt)
Expand Down

0 comments on commit d384e38

Please sign in to comment.