Devirtualizer: Fix verification by copying catch handlers to split block Conflicts resulting from https://r8-review.googlesource.com/c/r8/+/55236 where resolved for the cherry-pick. Bug: 174167294 Change-Id: I7e2c1a3c154be219904fb97e74b5342a3ff3bee4
diff --git a/src/main/java/com/android/tools/r8/ir/code/BasicBlockInstructionListIterator.java b/src/main/java/com/android/tools/r8/ir/code/BasicBlockInstructionListIterator.java index e7b3924..0e70023 100644 --- a/src/main/java/com/android/tools/r8/ir/code/BasicBlockInstructionListIterator.java +++ b/src/main/java/com/android/tools/r8/ir/code/BasicBlockInstructionListIterator.java
@@ -390,6 +390,16 @@ return newBlock; } + @Override + public BasicBlock splitCopyCatchHandlers( + IRCode code, ListIterator<BasicBlock> blockIterator, InternalOptions options) { + BasicBlock splitBlock = split(code, blockIterator, false); + assert !block.hasCatchHandlers(); + assert splitBlock.hasCatchHandlers(); + block.copyCatchHandlers(code, blockIterator, splitBlock, options); + return splitBlock; + } + private boolean canThrow(IRCode code) { InstructionIterator iterator = code.instructionIterator(); while (iterator.hasNext()) {
diff --git a/src/main/java/com/android/tools/r8/ir/code/IRCodeInstructionListIterator.java b/src/main/java/com/android/tools/r8/ir/code/IRCodeInstructionListIterator.java index 47ce330..a7dc933 100644 --- a/src/main/java/com/android/tools/r8/ir/code/IRCodeInstructionListIterator.java +++ b/src/main/java/com/android/tools/r8/ir/code/IRCodeInstructionListIterator.java
@@ -79,6 +79,12 @@ } @Override + public BasicBlock splitCopyCatchHandlers( + IRCode code, ListIterator<BasicBlock> blockIterator, InternalOptions options) { + throw new Unimplemented(); + } + + @Override public BasicBlock inlineInvoke( AppView<?> appView, IRCode code,
diff --git a/src/main/java/com/android/tools/r8/ir/code/InstructionListIterator.java b/src/main/java/com/android/tools/r8/ir/code/InstructionListIterator.java index b15b08c..184ff69 100644 --- a/src/main/java/com/android/tools/r8/ir/code/InstructionListIterator.java +++ b/src/main/java/com/android/tools/r8/ir/code/InstructionListIterator.java
@@ -128,6 +128,9 @@ return split(code, null); } + BasicBlock splitCopyCatchHandlers( + IRCode code, ListIterator<BasicBlock> blockIterator, InternalOptions options); + /** * Split the block into three blocks. The first split is at the point of the {@link ListIterator} * cursor and the second split is <code>instructions</code> after the cursor. The existing
diff --git a/src/main/java/com/android/tools/r8/ir/code/LinearFlowInstructionListIterator.java b/src/main/java/com/android/tools/r8/ir/code/LinearFlowInstructionListIterator.java index 5b230bc..f608f08 100644 --- a/src/main/java/com/android/tools/r8/ir/code/LinearFlowInstructionListIterator.java +++ b/src/main/java/com/android/tools/r8/ir/code/LinearFlowInstructionListIterator.java
@@ -88,6 +88,12 @@ } @Override + public BasicBlock splitCopyCatchHandlers( + IRCode code, ListIterator<BasicBlock> blockIterator, InternalOptions options) { + return currentBlockIterator.splitCopyCatchHandlers(code, blockIterator, options); + } + + @Override public BasicBlock inlineInvoke( AppView<?> appView, IRCode code,
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/Devirtualizer.java b/src/main/java/com/android/tools/r8/ir/optimize/Devirtualizer.java index ad2c233..4e22682 100644 --- a/src/main/java/com/android/tools/r8/ir/optimize/Devirtualizer.java +++ b/src/main/java/com/android/tools/r8/ir/optimize/Devirtualizer.java
@@ -25,6 +25,7 @@ import com.android.tools.r8.ir.code.InvokeVirtual; import com.android.tools.r8.ir.code.Value; import com.android.tools.r8.shaking.AppInfoWithLiveness; +import com.android.tools.r8.utils.InternalOptions; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Sets; @@ -46,9 +47,11 @@ public class Devirtualizer { private final AppView<AppInfoWithLiveness> appView; + private final InternalOptions options; public Devirtualizer(AppView<AppInfoWithLiveness> appView) { this.appView = appView; + this.options = appView.options(); } public void devirtualizeInvokeInterface(IRCode code) { @@ -113,6 +116,7 @@ } } } + continue; } if (appView.options().testing.enableInvokeSuperToInvokeVirtualRewriting) { @@ -208,7 +212,18 @@ if (castedReceiverCache.containsKey(receiver) && castedReceiverCache.get(receiver).containsKey(holderType)) { Value cachedReceiver = castedReceiverCache.get(receiver).get(holderType); - if (dominatorTree.dominatedBy(block, cachedReceiver.definition.getBlock())) { + BasicBlock cachedReceiverBlock = cachedReceiver.definition.getBlock(); + BasicBlock dominatorBlock = null; + if (cachedReceiverBlock.hasCatchHandlers()) { + if (cachedReceiverBlock.hasUniqueNormalSuccessor()) { + dominatorBlock = cachedReceiverBlock.getUniqueNormalSuccessor(); + } else { + assert false; + } + } else { + dominatorBlock = cachedReceiverBlock; + } + if (dominatorBlock != null && dominatorTree.dominatedBy(block, dominatorBlock)) { newReceiver = cachedReceiver; } } @@ -228,16 +243,18 @@ // We need to add this checkcast *before* the devirtualized invoke-virtual. assert it.peekPrevious() == devirtualizedInvoke; it.previous(); - // If the current block has catch handlers, split the new checkcast on its own block. - // Because checkcast is also a throwing instr, we should split before adding it. - // Otherwise, catch handlers are bound to a block with checkcast, not invoke IR. + + // If the current block has catch handlers, then split the block before adding the new + // check-cast instruction. The catch handlers are copied to the split block to ensure + // that all throwing instructions are covered by a catch-all catch handler in case of + // monitor instructions (see also b/174167294). BasicBlock blockWithDevirtualizedInvoke = - block.hasCatchHandlers() ? it.split(code, blocks) : block; + block.hasCatchHandlers() + ? it.splitCopyCatchHandlers(code, blocks, options) + : block; if (blockWithDevirtualizedInvoke != block) { // If we split, add the new checkcast at the end of the currently visiting block. - it = block.listIterator(code, block.getInstructions().size()); - it.previous(); - it.add(checkCast); + block.listIterator(code, block.getInstructions().size() - 1).add(checkCast); // Update the dominator tree after the split. dominatorTree = new DominatorTree(code); // Restore the cursor.
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/MemberValuePropagation.java b/src/main/java/com/android/tools/r8/ir/optimize/MemberValuePropagation.java index 4bd9f0d..87fffcd 100644 --- a/src/main/java/com/android/tools/r8/ir/optimize/MemberValuePropagation.java +++ b/src/main/java/com/android/tools/r8/ir/optimize/MemberValuePropagation.java
@@ -312,11 +312,10 @@ // Insert the definition of the replacement. replacement.setPosition(position); if (block.hasCatchHandlers()) { - BasicBlock splitBlock = iterator.split(code, blocks, false); - splitBlock.listIterator(code).add(replacement); - assert !block.hasCatchHandlers(); - assert splitBlock.hasCatchHandlers(); - block.copyCatchHandlers(code, blocks, splitBlock, appView.options()); + iterator + .splitCopyCatchHandlers(code, blocks, appView.options()) + .listIterator(code) + .add(replacement); } else { iterator.add(replacement); } @@ -416,11 +415,10 @@ // Insert the definition of the replacement. replacement.setPosition(position); if (block.hasCatchHandlers()) { - BasicBlock splitBlock = iterator.split(code, blocks, false); - splitBlock.listIterator(code).add(replacement); - assert !block.hasCatchHandlers(); - assert splitBlock.hasCatchHandlers(); - block.copyCatchHandlers(code, blocks, splitBlock, appView.options()); + iterator + .splitCopyCatchHandlers(code, blocks, appView.options()) + .listIterator(code) + .add(replacement); } else { iterator.add(replacement); }
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/devirtualize/DevirtualizeWithCatchHandlersTest.java b/src/test/java/com/android/tools/r8/ir/optimize/devirtualize/DevirtualizeWithCatchHandlersTest.java index e45f34c..a126f0f 100644 --- a/src/test/java/com/android/tools/r8/ir/optimize/devirtualize/DevirtualizeWithCatchHandlersTest.java +++ b/src/test/java/com/android/tools/r8/ir/optimize/devirtualize/DevirtualizeWithCatchHandlersTest.java
@@ -41,12 +41,7 @@ .setMinApi(parameters.getApiLevel()) .compile() .run(parameters.getRuntime(), Main.class) - .assertSuccessWithOutputLinesIf( - parameters.isCfRuntime() || parameters.getDexRuntimeVersion().isDalvik(), "A") - // TODO(b/174167294): Should succeed. - .assertFailureWithErrorThatThrowsIf( - parameters.isDexRuntime() && !parameters.getDexRuntimeVersion().isDalvik(), - VerifyError.class); + .assertSuccessWithOutputLines("A"); } static class Main {
diff --git a/src/test/java/com/android/tools/r8/ir/regalloc/RegisterMoveSchedulerTest.java b/src/test/java/com/android/tools/r8/ir/regalloc/RegisterMoveSchedulerTest.java index b4d2c3a..537a861 100644 --- a/src/test/java/com/android/tools/r8/ir/regalloc/RegisterMoveSchedulerTest.java +++ b/src/test/java/com/android/tools/r8/ir/regalloc/RegisterMoveSchedulerTest.java
@@ -144,6 +144,12 @@ } @Override + public BasicBlock splitCopyCatchHandlers( + IRCode code, ListIterator<BasicBlock> blockIterator, InternalOptions options) { + throw new Unimplemented(); + } + + @Override public BasicBlock inlineInvoke( AppView<?> appView, IRCode code,