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());
+  }
+}