Version 1.3.55
Cherry pick: Remove trivial gotos after removing debug positions.
CL: https://r8-review.googlesource.com/c/r8/+/27740
Cherry pick: Refine algorithm determining the active line at block entry.
CL: https://r8-review.googlesource.com/c/r8/+/27502
Cherry pick: Regression test for breakpoint issue.
CL: https://r8-review.googlesource.com/c/r8/+/33781
Bug: 122450679
Change-Id: If919d506afa783859556cf9935a201759bd1c14d
diff --git a/src/main/java/com/android/tools/r8/Version.java b/src/main/java/com/android/tools/r8/Version.java
index 8af42d8..95af09d 100644
--- a/src/main/java/com/android/tools/r8/Version.java
+++ b/src/main/java/com/android/tools/r8/Version.java
@@ -11,7 +11,7 @@
// This field is accessed from release scripts using simple pattern matching.
// Therefore, changing this field could break our release scripts.
- public static final String LABEL = "1.3.54";
+ public static final String LABEL = "1.3.55";
private Version() {
}
diff --git a/src/main/java/com/android/tools/r8/graph/DexDebugEventBuilder.java b/src/main/java/com/android/tools/r8/graph/DexDebugEventBuilder.java
index 18f23e3..fa856cb 100644
--- a/src/main/java/com/android/tools/r8/graph/DexDebugEventBuilder.java
+++ b/src/main/java/com/android/tools/r8/graph/DexDebugEventBuilder.java
@@ -58,14 +58,10 @@
// Initial known line for the method.
private int startLine = NO_LINE_INFO;
- // True if running in debug-mode with input code that contains line information, otherwise false.
- private final boolean hasDebugPositions;
-
public DexDebugEventBuilder(IRCode code, InternalOptions options) {
this.method = code.method;
this.factory = options.itemFactory;
this.options = options;
- hasDebugPositions = code.hasDebugPositions;
}
/** Add events at pc for instruction. */
@@ -89,17 +85,15 @@
startArgument(instruction.asArgument());
} else if (instruction.isDebugLocalsChange()) {
updateLocals(instruction.asDebugLocalsChange());
- }
-
- if (!position.isNone() && !position.equals(emittedPosition)) {
- if (options.debug || instruction.instructionInstanceCanThrow()) {
- emitDebugPosition(pc, position);
+ } else if (pcAdvancing) {
+ if (!position.isNone() && !position.equals(emittedPosition)) {
+ if (options.debug || instruction.instructionInstanceCanThrow()) {
+ emitDebugPosition(pc, position);
+ }
}
- }
-
- if (emittedPc != pc && pcAdvancing) {
- // For pc-advancing instructions emit any pending changes.
- emitLocalChanges(pc);
+ if (emittedPc != pc) {
+ emitLocalChanges(pc);
+ }
}
if (isBlockExit) {
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 90981ac..28adf87 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
@@ -52,6 +52,7 @@
import com.android.tools.r8.ir.code.IRCode;
import com.android.tools.r8.ir.code.If;
import com.android.tools.r8.ir.code.InstructionIterator;
+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;
@@ -59,6 +60,7 @@
import com.android.tools.r8.ir.code.StackValue;
import com.android.tools.r8.ir.code.Switch;
import com.android.tools.r8.ir.code.Value;
+import com.android.tools.r8.ir.optimize.CodeRewriter;
import com.android.tools.r8.ir.regalloc.RegisterAllocator;
import com.android.tools.r8.utils.InternalOptions;
import com.google.common.collect.BiMap;
@@ -68,6 +70,7 @@
import it.unimi.dsi.fastutil.ints.Int2ReferenceMap;
import it.unimi.dsi.fastutil.ints.Int2ReferenceOpenHashMap;
import java.util.ArrayList;
+import java.util.HashSet;
import java.util.List;
import java.util.ListIterator;
import java.util.Map;
@@ -271,6 +274,13 @@
return code;
}
+ private static boolean isTrivialFallthroughTarget(
+ BasicBlock previousBlock, BasicBlock currentBlock) {
+ return previousBlock.exit().isGoto()
+ && currentBlock.getPredecessors().size() == 1
+ && currentBlock.getPredecessors().get(0) == previousBlock;
+ }
+
// Eliminates unneeded debug positions.
//
// After this pass all remaining debug positions mark places where we must ensure a materializing
@@ -293,19 +303,39 @@
// 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);
- BasicBlock nextBlock =
- blockIndex + 1 < code.blocks.size() ? code.blocks.get(blockIndex + 1) : null;
- // Current materialized position can remain on block entry only if it is also the exit of
- // the blocks predecessors. If not, we cannot ensure the jumps will hit this line unless
- // another instruction materialized the position.
- for (BasicBlock pred : currentBlock.getPredecessors()) {
- if (pred.exit().getPosition() != currentMaterializedPosition) {
- currentMaterializedPosition = Position.none();
- break;
+ // Current materialized position must be updated to the position we can guarantee is emitted
+ // in all predecessors. The position of a fallthrough predecessor is defined by
+ // currentMaterializedPosition and unresolvedPosition (and not by the position of its exit!)
+ // If this is the entry block or a trivial fall-through with no other predecessors the
+ // materialized and unresolved positions remain unchanged.
+ if (blockIndex != 0) {
+ BasicBlock previousBlock = code.blocks.get(blockIndex - 1);
+ if (!isTrivialFallthroughTarget(previousBlock, currentBlock)) {
+ Position positionAtAllPredecessors = null;
+ for (BasicBlock pred : currentBlock.getPredecessors()) {
+ Position predExit;
+ if (pred == previousBlock) {
+ predExit =
+ unresolvedPosition != null
+ ? unresolvedPosition.getPosition()
+ : currentMaterializedPosition;
+ } else {
+ predExit = pred.exit().getPosition();
+ }
+ if (positionAtAllPredecessors == null) {
+ positionAtAllPredecessors = predExit;
+ } else if (!positionAtAllPredecessors.equals(predExit)) {
+ positionAtAllPredecessors = Position.none();
+ break;
+ }
+ }
+ unresolvedPosition = null;
+ currentMaterializedPosition = positionAtAllPredecessors;
}
}
@@ -315,6 +345,10 @@
? new Int2ReferenceOpenHashMap<>(currentBlock.getLocalsAtEntry())
: new Int2ReferenceOpenHashMap<>();
+ // Next block to decide which gotos are fall-throughs.
+ BasicBlock nextBlock =
+ blockIndex + 1 < code.blocks.size() ? code.blocks.get(blockIndex + 1) : null;
+
InstructionIterator iterator = currentBlock.iterator();
while (iterator.hasNext()) {
com.android.tools.r8.ir.code.Instruction instruction = iterator.next();
@@ -323,6 +357,12 @@
&& 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);
+ }
+ }
} else if (unresolvedPosition != null
&& unresolvedPosition.getPosition() == instruction.getPosition()
&& locals.equals(localsAtUnresolvedPosition)) {
@@ -358,11 +398,33 @@
int i = 0;
while (it.hasNext() && i < toRemove.size()) {
if (it.next() == toRemove.get(i)) {
- it.removeOrReplaceByDebugLocalRead();
+ it.remove();
++i;
}
}
}
+ // 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:
diff --git a/src/main/java/com/android/tools/r8/ir/conversion/JarSourceCode.java b/src/main/java/com/android/tools/r8/ir/conversion/JarSourceCode.java
index faa3aae..d17233f 100644
--- a/src/main/java/com/android/tools/r8/ir/conversion/JarSourceCode.java
+++ b/src/main/java/com/android/tools/r8/ir/conversion/JarSourceCode.java
@@ -503,7 +503,7 @@
}
// If there are no explicit exits from the method (ie, this method is a loop without an explict
- // return or an unhandled throw) then we cannot guarentee that a local live in a successor will
+ // return or an unhandled throw) then we cannot guarantee that a local live in a successor will
// ensure it is marked as such (via an explict 'end' marker) and thus be live in predecessors.
// In this case we insert an 'end' point on all explicit goto instructions ensuring that any
// back-edge will explicitly keep locals live at that point.
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/CodeRewriter.java b/src/main/java/com/android/tools/r8/ir/optimize/CodeRewriter.java
index 17d478a..e2180b5 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/CodeRewriter.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/CodeRewriter.java
@@ -168,7 +168,7 @@
return true;
}
- private static boolean isFallthroughBlock(BasicBlock block) {
+ public static boolean isFallthroughBlock(BasicBlock block) {
for (BasicBlock pred : block.getPredecessors()) {
if (pred.exit().fallthroughBlock() == block) {
return true;
@@ -204,16 +204,21 @@
if (!needed) {
blocksToRemove.add(block);
- for (BasicBlock pred : block.getPredecessors()) {
- pred.replaceSuccessor(block, target);
- }
- for (BasicBlock succ : block.getSuccessors()) {
- succ.getPredecessors().remove(block);
- }
- for (BasicBlock pred : block.getPredecessors()) {
- if (!target.getPredecessors().contains(pred)) {
- target.getPredecessors().add(pred);
- }
+ unlinkTrivialGotoBlock(block, target);
+ }
+ }
+
+ public static void unlinkTrivialGotoBlock(BasicBlock block, BasicBlock target) {
+ assert block.isTrivialGoto();
+ for (BasicBlock pred : block.getPredecessors()) {
+ pred.replaceSuccessor(block, target);
+ }
+ for (BasicBlock succ : block.getSuccessors()) {
+ succ.getPredecessors().remove(block);
+ }
+ for (BasicBlock pred : block.getPredecessors()) {
+ if (!target.getPredecessors().contains(pred)) {
+ target.getPredecessors().add(pred);
}
}
}
@@ -699,7 +704,6 @@
BasicBlock block = iterator.next();
BasicBlock nextBlock;
- // The marks will be used for cycle detection.
do {
nextBlock = iterator.hasNext() ? iterator.next() : null;
if (block.isTrivialGoto()) {
diff --git a/src/test/java/com/android/tools/r8/debug/BreakOnIfTest.java b/src/test/java/com/android/tools/r8/debug/BreakOnIfTest.java
new file mode 100644
index 0000000..43338ee
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/debug/BreakOnIfTest.java
@@ -0,0 +1,14 @@
+// Copyright (c) 2019, 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.debug;
+
+public class BreakOnIfTest {
+
+ public static void main(String[] args) {
+ if (args == null) {
+ System.out.println("args null");
+ }
+ System.out.println("after if");
+ }
+}
diff --git a/src/test/java/com/android/tools/r8/debug/BreakOnIfTestRunner.java b/src/test/java/com/android/tools/r8/debug/BreakOnIfTestRunner.java
new file mode 100644
index 0000000..e61d6f8
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/debug/BreakOnIfTestRunner.java
@@ -0,0 +1,47 @@
+// Copyright (c) 2019, 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.debug;
+
+import com.android.tools.r8.ToolHelper;
+import com.google.common.collect.ImmutableList;
+import java.util.Collection;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+@RunWith(Parameterized.class)
+public class BreakOnIfTestRunner extends DebugTestBase {
+
+ private static final Class CLASS = BreakOnIfTest.class;
+ private static final String FILE = CLASS.getSimpleName() + ".java";
+ private static final String NAME = CLASS.getCanonicalName();
+
+ private final DebugTestConfig config;
+
+ @Parameterized.Parameters(name = "{0}")
+ public static Collection<Object[]> setup() {
+ DelayedDebugTestConfig cf =
+ temp -> new CfDebugTestConfig().addPaths(ToolHelper.getClassPathForTests());
+ DelayedDebugTestConfig d8 =
+ temp -> new D8DebugTestConfig().compileAndAddClasses(temp, CLASS);
+ return ImmutableList.of(new Object[]{"CF", cf}, new Object[]{"D8", d8});
+ }
+
+ public BreakOnIfTestRunner(String name, DelayedDebugTestConfig config) {
+ this.config = config.getConfig(temp);
+ }
+
+ @Test
+ public void test() throws Throwable {
+ runDebugTest(
+ config,
+ NAME,
+ breakpoint(NAME, "main", 9),
+ run(),
+ checkLine(FILE, 9),
+ stepOver(),
+ checkLine(FILE, 12),
+ run());
+ }
+}