Remove trivial fallthrough blocks before debug positions.
Fixes: b/230337727
Change-Id: If5324b353972ffa33b6cdb784bb4883dc1169052
diff --git a/src/main/java/com/android/tools/r8/code/Instruction.java b/src/main/java/com/android/tools/r8/code/Instruction.java
index 8671b68..580d5ef 100644
--- a/src/main/java/com/android/tools/r8/code/Instruction.java
+++ b/src/main/java/com/android/tools/r8/code/Instruction.java
@@ -343,6 +343,10 @@
@Override
public final int acceptCompareTo(Instruction other, CompareToVisitor visitor) {
int opcodeDiff = visitor.visitInt(getCompareToId(), other.getCompareToId());
+ if (opcodeDiff != 0) {
+ return opcodeDiff;
+ }
+ opcodeDiff = visitor.visitInt(getOffset(), other.getOffset());
return opcodeDiff != 0 ? opcodeDiff : internalAcceptCompareTo(other, visitor);
}
diff --git a/src/main/java/com/android/tools/r8/ir/code/IRCode.java b/src/main/java/com/android/tools/r8/ir/code/IRCode.java
index bf8f7ad..ffc5782 100644
--- a/src/main/java/com/android/tools/r8/ir/code/IRCode.java
+++ b/src/main/java/com/android/tools/r8/ir/code/IRCode.java
@@ -516,7 +516,9 @@
}
public void removeBlocks(Collection<BasicBlock> blocksToRemove) {
- blocks.removeAll(blocksToRemove);
+ if (!blocksToRemove.isEmpty()) {
+ blocks.removeAll(blocksToRemove);
+ }
}
/**
diff --git a/src/main/java/com/android/tools/r8/ir/conversion/CfSourceCode.java b/src/main/java/com/android/tools/r8/ir/conversion/CfSourceCode.java
index 1a39011..5270193 100644
--- a/src/main/java/com/android/tools/r8/ir/conversion/CfSourceCode.java
+++ b/src/main/java/com/android/tools/r8/ir/conversion/CfSourceCode.java
@@ -412,6 +412,11 @@
if (needsGeneratedMethodSynchronization) {
buildMethodEnterSynchronization(builder);
}
+ Position entryPosition = getCanonicalDebugPositionAtOffset(0);
+ if (!state.getPosition().equals(entryPosition)) {
+ state.setPosition(entryPosition);
+ builder.addDebugPosition(state.getPosition());
+ }
recordStateForTarget(0, state.getSnapshot());
inPrelude = false;
}
diff --git a/src/main/java/com/android/tools/r8/ir/conversion/DexBuilder.java b/src/main/java/com/android/tools/r8/ir/conversion/DexBuilder.java
index 2bd4cfa..de631d3 100644
--- a/src/main/java/com/android/tools/r8/ir/conversion/DexBuilder.java
+++ b/src/main/java/com/android/tools/r8/ir/conversion/DexBuilder.java
@@ -54,7 +54,6 @@
import com.android.tools.r8.ir.code.InstructionIterator;
import com.android.tools.r8.ir.code.InstructionListIterator;
import com.android.tools.r8.ir.code.IntSwitch;
-import com.android.tools.r8.ir.code.JumpInstruction;
import com.android.tools.r8.ir.code.Move;
import com.android.tools.r8.ir.code.NewArrayFilledData;
import com.android.tools.r8.ir.code.Position;
@@ -72,7 +71,7 @@
import it.unimi.dsi.fastutil.ints.Int2ReferenceOpenHashMap;
import java.util.ArrayList;
import java.util.Arrays;
-import java.util.HashSet;
+import java.util.Collections;
import java.util.List;
import java.util.ListIterator;
import java.util.Map;
@@ -351,6 +350,50 @@
&& currentBlock.getPredecessors().get(0) == previousBlock;
}
+ private static void removeTrivialFallthroughBlocks(IRCode code) {
+ for (int blockIndex = 1; blockIndex < code.blocks.size() - 1; blockIndex++) {
+ // We skip the last block as it has no fallthrough. We also skip checking the entry block as
+ // it has no predecessors and must define the initial position. Any subsequent block must be
+ // statically reachable and thus have predecessors.
+ BasicBlock currentBlock = code.blocks.get(blockIndex);
+ assert !currentBlock.getPredecessors().isEmpty();
+ if (currentBlock.size() != 2) {
+ continue;
+ }
+ DebugPosition debugPosition = currentBlock.entry().asDebugPosition();
+ com.android.tools.r8.ir.code.Goto exit = currentBlock.exit().asGoto();
+ if (debugPosition == null || exit == null || debugPosition.getPosition().isNone()) {
+ continue;
+ }
+ BasicBlock nextBlock = code.blocks.get(blockIndex + 1);
+ if (exit.getTarget() != nextBlock) {
+ continue;
+ }
+ // The block is a trivial position block that falls through to the following.
+ // If the position is equal to each predecessor block then the line is already active on
+ // each entry to the fallthrough and it can be safely removed.
+ boolean allMatch = true;
+ Position position = debugPosition.getPosition();
+ for (BasicBlock pred : currentBlock.getPredecessors()) {
+ // Do to the target == next check this cannot be a trivial loop.
+ assert pred != currentBlock;
+ Position predExit = pred.exit().getPosition();
+ if (!position.equals(predExit)) {
+ allMatch = false;
+ break;
+ }
+ }
+ if (allMatch) {
+ currentBlock.removeInstruction(debugPosition);
+ CodeRewriter.unlinkTrivialGotoBlock(currentBlock, nextBlock);
+ code.removeBlocks(Collections.singleton(currentBlock));
+ // Having removed the block at blockIndex, the previous block may now be a trivial
+ // fallthrough. Rewind to that point and retry. This avoids iterating to a fixed point.
+ blockIndex = Math.max(0, blockIndex - 2);
+ }
+ }
+ }
+
// Eliminates unneeded debug positions.
//
// After this pass all remaining debug positions mark places where we must ensure a materializing
@@ -359,6 +402,12 @@
if (!code.metadata().mayHaveDebugPosition()) {
return;
}
+
+ // We must start by removing any blocks that are already trivial fallthrough blocks with no
+ // position change. With these removed it is then sound to make the fallthrough judgement when
+ // determining if a goto will materialize or not.
+ removeTrivialFallthroughBlocks(code);
+
// Current position known to have a materializing instruction associated with it.
Position currentMaterializedPosition = Position.none();
@@ -373,7 +422,6 @@
// Compute the set of all positions that can be removed.
// (Delaying removal to avoid ConcurrentModificationException).
List<DebugPosition> toRemove = new ArrayList<>();
- Set<BasicBlock> trivialBlocks = new HashSet<>();
for (int blockIndex = 0; blockIndex < code.blocks.size(); blockIndex++) {
BasicBlock currentBlock = code.blocks.get(blockIndex);
@@ -425,12 +473,11 @@
&& currentMaterializedPosition == instruction.getPosition()) {
// Here we don't need to check locals state as the line is already active.
toRemove.add(instruction.asDebugPosition());
- if (currentBlock.getInstructions().size() == 2) {
- JumpInstruction exit = currentBlock.exit();
- if (exit.isGoto() && exit.getPosition() == currentMaterializedPosition) {
- trivialBlocks.add(currentBlock);
- }
- }
+ assert currentBlock.size() != 2
+ || currentBlock.exit().getPosition() != currentMaterializedPosition
+ || !currentBlock.exit().isGoto()
+ || currentBlock.exit().asGoto().getTarget() != nextBlock
+ : "Unexpected trivial fallthrough block. This should be removed already.";
} else if (unresolvedPosition != null
&& unresolvedPosition.getPosition() == instruction.getPosition()
&& locals.equals(localsAtUnresolvedPosition)) {
@@ -474,29 +521,6 @@
}
assert i == toRemove.size();
}
-
- // Remove all trivial goto blocks that have a position known to be emitted in their predecessor.
- if (!trivialBlocks.isEmpty()) {
- List<BasicBlock> blocksToRemove = new ArrayList<>();
- ListIterator<BasicBlock> iterator = code.listIterator();
- // Skip the entry block.
- assert code.blocks.size() > 1;
- iterator.next();
- BasicBlock nextBlock = iterator.next();
- do {
- BasicBlock block = nextBlock;
- nextBlock = iterator.hasNext() ? iterator.next() : null;
- if (block.isTrivialGoto()
- && trivialBlocks.contains(block)
- && block.exit().asGoto().getTarget() != block
- && !CodeRewriter.isFallthroughBlock(block)) {
- BasicBlock target = block.exit().asGoto().getTarget();
- blocksToRemove.add(block);
- CodeRewriter.unlinkTrivialGotoBlock(block, target);
- }
- } while (iterator.hasNext());
- code.removeBlocks(blocksToRemove);
- }
}
// Rewrite ifs with offsets that are too large for the if encoding. The rewriting transforms:
@@ -522,8 +546,10 @@
BasicBlock trueTarget = theIf.getTrueTarget();
BasicBlock newBlock =
BasicBlock.createGotoBlock(
- ir.blocks.size(), theIf.getPosition(), ir.metadata(), trueTarget);
+ ir.getNextBlockNumber(), theIf.getPosition(), ir.metadata(), trueTarget);
theIf.setTrueTarget(newBlock);
+ newBlock.getMutablePredecessors().add(block);
+ trueTarget.replacePredecessor(block, newBlock);
theIf.invert();
it.add(newBlock);
}
diff --git a/src/test/java/com/android/tools/r8/TestParametersBuilder.java b/src/test/java/com/android/tools/r8/TestParametersBuilder.java
index 48e94f0..cf33980 100644
--- a/src/test/java/com/android/tools/r8/TestParametersBuilder.java
+++ b/src/test/java/com/android/tools/r8/TestParametersBuilder.java
@@ -93,6 +93,10 @@
return withCfRuntime(CfRuntime.getDefaultCfRuntime().getVm());
}
+ public TestParametersBuilder withDefaultRuntimes() {
+ return withDefaultDexRuntime().withDefaultCfRuntime();
+ }
+
/** Add all available CF runtimes. */
public TestParametersBuilder withCfRuntimes() {
return withCfRuntimeFilter(vm -> true);
diff --git a/src/test/java/com/android/tools/r8/debuginfo/NopLineRegression230337727Test.java b/src/test/java/com/android/tools/r8/debuginfo/NopLineRegression230337727Test.java
new file mode 100644
index 0000000..ccdac2d
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/debuginfo/NopLineRegression230337727Test.java
@@ -0,0 +1,141 @@
+// Copyright (c) 2022, the R8 project authors. Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+package com.android.tools.r8.debuginfo;
+
+import static org.junit.Assert.assertTrue;
+
+import com.android.tools.r8.TestBase;
+import com.android.tools.r8.TestParameters;
+import com.android.tools.r8.TestParametersCollection;
+import com.android.tools.r8.cf.CfVersion;
+import com.android.tools.r8.transformers.ClassFileTransformer.MethodPredicate;
+import com.android.tools.r8.utils.AndroidApiLevel;
+import com.android.tools.r8.utils.codeinspector.MethodSubject;
+import java.util.Arrays;
+import java.util.List;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.objectweb.asm.Label;
+import org.objectweb.asm.Opcodes;
+
+@RunWith(Parameterized.class)
+public class NopLineRegression230337727Test extends TestBase implements Opcodes {
+
+ @Parameterized.Parameters(name = "{0}")
+ public static TestParametersCollection data() {
+ return getTestParameters().withDefaultRuntimes().withApiLevel(AndroidApiLevel.B).build();
+ }
+
+ private final TestParameters parameters;
+
+ public NopLineRegression230337727Test(TestParameters parameters) {
+ this.parameters = parameters;
+ }
+
+ @Test
+ public void test() throws Exception {
+ testForRuntime(parameters)
+ .addProgramClassFileData(getTestClassTransformed())
+ .run(parameters.getRuntime(), TestClass.class)
+ .inspect(
+ inspector -> {
+ MethodSubject foo = inspector.clazz(TestClass.class).uniqueMethodWithName("foo");
+ assertTrue(
+ foo.getLineNumberTable().getLines().toString(),
+ foo.getLineNumberTable().getLines().contains(11));
+ });
+ }
+
+ static class TestClass {
+
+ public static boolean boo() {
+ return true;
+ }
+
+ public static void foo(List<Integer> args) {
+ System.nanoTime();
+ // Code added by transformer for the kotlin code:
+ // if (boo()) {
+ // l.forEach {}
+ // }
+ // if (boo()) {}
+ }
+
+ public static void main(String[] args) {
+ foo(Arrays.asList(1, 2, 3));
+ }
+ }
+
+ private static byte[] getTestClassTransformed() throws Exception {
+ // Code generated by kotlin but with frames, locals and checkNotNullParameter removed.
+ String holder = binaryName(TestClass.class);
+ return transformer(TestClass.class)
+ .setVersion(CfVersion.V1_5) // Avoid need of stack frames
+ .setMaxs(MethodPredicate.onName("foo"), 2, 7)
+ .transformMethodInsnInMethod(
+ "foo",
+ (opcode, owner, name, descriptor, isInterface, visitor) -> {
+ Label label1 = new Label();
+ visitor.visitLabel(label1);
+ visitor.visitLineNumber(4, label1);
+ visitor.visitMethodInsn(INVOKESTATIC, holder, "boo", "()Z", false);
+ Label label2 = new Label();
+ visitor.visitJumpInsn(IFEQ, label2);
+ Label label3 = new Label();
+ visitor.visitLabel(label3);
+ visitor.visitLineNumber(5, label3);
+ visitor.visitVarInsn(ALOAD, 0);
+ visitor.visitTypeInsn(CHECKCAST, "java/lang/Iterable");
+ visitor.visitVarInsn(ASTORE, 1);
+ visitor.visitInsn(ICONST_0);
+ visitor.visitVarInsn(ISTORE, 2);
+ Label label5 = new Label();
+ visitor.visitLabel(label5);
+ visitor.visitLineNumber(10, label5);
+ visitor.visitVarInsn(ALOAD, 1);
+ visitor.visitMethodInsn(
+ INVOKEINTERFACE,
+ "java/lang/Iterable",
+ "iterator",
+ "()Ljava/util/Iterator;",
+ true);
+ visitor.visitVarInsn(ASTORE, 3);
+ Label label6 = new Label();
+ visitor.visitLabel(label6);
+ visitor.visitVarInsn(ALOAD, 3);
+ visitor.visitMethodInsn(
+ INVOKEINTERFACE, "java/util/Iterator", "hasNext", "()Z", true);
+ Label label7 = new Label();
+ visitor.visitJumpInsn(IFEQ, label7);
+ visitor.visitVarInsn(ALOAD, 3);
+ visitor.visitMethodInsn(
+ INVOKEINTERFACE, "java/util/Iterator", "next", "()Ljava/lang/Object;", true);
+ visitor.visitVarInsn(ASTORE, 4);
+ visitor.visitVarInsn(ALOAD, 4);
+ visitor.visitTypeInsn(CHECKCAST, "java/lang/Number");
+ visitor.visitMethodInsn(INVOKEVIRTUAL, "java/lang/Number", "intValue", "()I", false);
+ visitor.visitVarInsn(ISTORE, 5);
+ visitor.visitInsn(ICONST_0);
+ visitor.visitVarInsn(ISTORE, 6);
+ Label label10 = new Label();
+ visitor.visitLabel(label10);
+ visitor.visitLineNumber(5, label10);
+ visitor.visitInsn(NOP);
+ visitor.visitJumpInsn(GOTO, label6);
+ visitor.visitLabel(label7);
+ visitor.visitLineNumber(11, label7);
+ visitor.visitInsn(NOP);
+ visitor.visitLabel(label2);
+ visitor.visitLineNumber(7, label2);
+ visitor.visitMethodInsn(INVOKESTATIC, holder, "boo", "()Z", false);
+ Label label12 = new Label();
+ visitor.visitJumpInsn(IFEQ, label12);
+ visitor.visitLabel(label12);
+ visitor.visitLineNumber(8, label12);
+ visitor.visitInsn(RETURN);
+ })
+ .transform();
+ }
+}
diff --git a/src/test/java/com/android/tools/r8/transformers/ClassFileTransformer.java b/src/test/java/com/android/tools/r8/transformers/ClassFileTransformer.java
index 889f7a7..9b4d076 100644
--- a/src/test/java/com/android/tools/r8/transformers/ClassFileTransformer.java
+++ b/src/test/java/com/android/tools/r8/transformers/ClassFileTransformer.java
@@ -1204,23 +1204,22 @@
}
public ClassFileTransformer setMaxStackHeight(MethodPredicate predicate, int newMaxStack) {
- return addMethodTransformer(
- new MethodTransformer() {
- @Override
- public void visitMaxs(int maxStack, int maxLocals) {
- super.visitMaxs(
- MethodPredicate.testContext(predicate, getContext()) ? newMaxStack : maxStack,
- maxLocals);
- }
- });
+ return setMaxs(predicate, newMaxStack, null);
}
- public ClassFileTransformer computeMaxs() {
+ public ClassFileTransformer setMaxs(
+ MethodPredicate predicate, Integer newMaxStack, Integer newMaxLocals) {
return addMethodTransformer(
new MethodTransformer() {
@Override
public void visitMaxs(int maxStack, int maxLocals) {
- super.visitMaxs(maxStack, maxLocals);
+ if (MethodPredicate.testContext(predicate, getContext())) {
+ super.visitMaxs(
+ newMaxStack != null ? newMaxStack : maxStack,
+ newMaxLocals != null ? newMaxLocals : maxLocals);
+ } else {
+ super.visitMaxs(maxStack, maxLocals);
+ }
}
});
}
