Devirtualizer: Fix verification by copying catch handlers to split block
Conflicts resulting form 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 e3fa34e..a7e2552 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
@@ -436,6 +436,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 bec911e..fc2f3f4 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 2790e66..c9c99e7 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
@@ -136,6 +136,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 d851eb9..7665d39 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 0ed998a..1094700 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) {
@@ -114,6 +117,7 @@
}
}
}
+ continue;
}
if (appView.options().testing.enableInvokeSuperToInvokeVirtualRewriting) {
@@ -209,7 +213,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;
}
}
@@ -229,16 +244,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 0dd3aa4..abe6c1b 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
@@ -305,11 +305,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);
}
@@ -407,11 +406,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 3f7bb60..55048aa 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,
