From d1f3420c1bf884028f941a982090db2066ddddb3 Mon Sep 17 00:00:00 2001 From: Col-E Date: Sat, 19 Aug 2023 02:02:48 -0400 Subject: [PATCH 1/8] Skip remove redundant blocks if there are too few blocks for the IR transform to apply --- src/main/java/com/android/tools/r8/ir/code/IRCode.java | 2 ++ 1 file changed, 2 insertions(+) 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 a4db6a0438..629f81fe2e 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 @@ -1273,6 +1273,8 @@ private boolean isRedundantBlock(BasicBlock block) { } public void removeRedundantBlocks() { + if (blocks.size() <= 2) return; + List blocksToRemove = new ArrayList<>(); for (BasicBlock block : blocks) { From beb2d25a46dfa9abeea5f89666fd151fba6e66fe Mon Sep 17 00:00:00 2001 From: Col-E Date: Sat, 19 Aug 2023 03:52:09 -0400 Subject: [PATCH 2/8] Offer static variant of createInitializedType() when the input is assumed to be safely directly fed into InitializedTypeInfo --- .../com/android/tools/r8/cf/TypeVerificationHelper.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/main/java/com/android/tools/r8/cf/TypeVerificationHelper.java b/src/main/java/com/android/tools/r8/cf/TypeVerificationHelper.java index b7495c72cb..8f2e454a13 100644 --- a/src/main/java/com/android/tools/r8/cf/TypeVerificationHelper.java +++ b/src/main/java/com/android/tools/r8/cf/TypeVerificationHelper.java @@ -124,6 +124,12 @@ public TypeVerificationHelper(AppView appView, IRCode code) { DOUBLE = new InitializedTypeInfo(dexItemFactory.doubleType); } + public static InitializedTypeInfo createInitializedObjectType(DexType type) { + if (type.isPrimitiveType() || type.isArrayType()) + throw new IllegalArgumentException(); + return new InitializedTypeInfo(type); + } + public TypeInfo createInitializedType(DexType type) { if (!type.isPrimitiveType()) { return new InitializedTypeInfo(type); From 309edd0e68f3dfd7b1b4f51b549bcfe132eec011 Mon Sep 17 00:00:00 2001 From: Col-E Date: Fri, 18 Aug 2023 14:38:48 -0400 Subject: [PATCH 3/8] Insert pop in IR when a catch block has no use for the pushed exception value Must be done after goto inlining, as to properly detect if predecessors have a try-catch relation to the target block. Assumes the IR will push a MoveException into the first instruction of blocks when a catch block uses the exception value. When this is missing but the block is a catch, we will insert our pop. --- .../tools/r8/graph/DexItemFactory.java | 5 +- .../android/tools/r8/ir/code/BasicBlock.java | 48 +++++++++++- .../tools/r8/ir/code/NewArrayFilledData.java | 1 - .../android/tools/r8/ir/code/StackValue.java | 10 +++ .../tools/r8/ir/conversion/CfBuilder.java | 5 ++ .../tools/r8/ir/conversion/DexSourceCode.java | 7 +- .../tools/r8/ir/conversion/IRBuilder.java | 76 ++----------------- .../r8/ir/optimize/PhiOptimizations.java | 7 +- 8 files changed, 79 insertions(+), 80 deletions(-) diff --git a/src/main/java/com/android/tools/r8/graph/DexItemFactory.java b/src/main/java/com/android/tools/r8/graph/DexItemFactory.java index 59721ebccb..59a5fb8801 100644 --- a/src/main/java/com/android/tools/r8/graph/DexItemFactory.java +++ b/src/main/java/com/android/tools/r8/graph/DexItemFactory.java @@ -323,6 +323,8 @@ public static boolean isInternalSentinel(DexItem item) { createString("Ljava/util/concurrent/ConcurrentHashMap$KeySetView;"); public final DexString throwableDescriptor = createString(throwableDescriptorString); + public final DexString exceptionDescriptor = + createString("Ljava/lang/Exception;"); public final DexString illegalAccessErrorDescriptor = createString("Ljava/lang/IllegalAccessError;"); public final DexString illegalArgumentExceptionDescriptor = @@ -566,6 +568,7 @@ private List createMultiDexTypes() { public final DexType retentionType = createStaticallyKnownType("Ljava/lang/annotation/Retention;"); + public final DexType exceptionType = createStaticallyKnownType(exceptionDescriptor); public final DexType runtimeExceptionType = createStaticallyKnownType(runtimeExceptionDescriptor); public final DexType assertionErrorType = createStaticallyKnownType(assertionErrorDescriptor); public final DexType throwableType = createStaticallyKnownType(throwableDescriptor); @@ -3222,7 +3225,7 @@ public ReferenceTypeElement createReferenceTypeElement( .computeIfAbsent( type, t -> { - if (type.isClassType()) { + if (type.isClassType() && appView != null) { if (!appView.enableWholeProgramOptimizations()) { // Don't reason at the level of interfaces in D8. return ClassTypeElement.createForD8(type, nullability); diff --git a/src/main/java/com/android/tools/r8/ir/code/BasicBlock.java b/src/main/java/com/android/tools/r8/ir/code/BasicBlock.java index 55882f554f..f042146784 100644 --- a/src/main/java/com/android/tools/r8/ir/code/BasicBlock.java +++ b/src/main/java/com/android/tools/r8/ir/code/BasicBlock.java @@ -5,6 +5,7 @@ import static com.android.tools.r8.ir.code.IRCode.INSTRUCTION_NUMBER_DELTA; +import com.android.tools.r8.cf.TypeVerificationHelper; import com.android.tools.r8.errors.CompilationError; import com.android.tools.r8.errors.Unreachable; import com.android.tools.r8.graph.AppView; @@ -269,6 +270,46 @@ public TraversalContinuation traverseExceptionalSuccessors( return traversalContinuation; } + private boolean isCatchDelegateCandidate; + + public void markCandidacyAsCatchDelegate() { + isCatchDelegateCandidate = true; + } + + public void onFinishBuildingInstructions(AppView appView) { + if (isCatchDelegateCandidate && !isEntry() && isTargetOfUnmovedException() && !handlesStackException()) { + // There is no 'MoveException' here, but for converting to CF code we need to get rid of this value on the stack. + DexType exceptionType = appView.dexItemFactory().throwableType; + Value stackExceptionValue = StackValue.create(TypeVerificationHelper.createInitializedObjectType(exceptionType), 1, appView); + Pop pop = new Pop(stackExceptionValue); + pop.setPosition(entry().getPosition()); + stackExceptionValue.definition = null; + instructions.add(0, pop); + } + } + + private boolean isTargetOfUnmovedException() { + return predecessors.stream().allMatch(block -> { + if (!block.hasCatchHandlers()) + // Block has no catch handlers, so we as the target won't be a handler. + // + // If you're debugging this and see that the block is just a 'goto' then the IRCode + // is not inlining unnecessary blocks. + return false; + + // Check if the predecessor block moves the exception into a variable slot. + // If it does not, then we are the target of an unmoved exception. + return !block.handlesStackException(); + }); + } + + private boolean handlesStackException() { + // A block that operates on the caught exception will have a 'MoveException' as the first instruction. + if (instructions.isEmpty()) return false; + Instruction instruction = instructions.get(0); + return instruction instanceof MoveException; + } + public void addControlFlowEdgesMayChangeListener(BasicBlockChangeListener listener) { if (onControlFlowEdgesMayChangeListeners == null) { // WeakSet to allow the listeners to be garbage collected. @@ -1559,7 +1600,12 @@ public boolean isInstructionBeforeThrowingInstruction(Instruction instruction) { } public boolean isTrivialGoto() { - return instructions.size() == 1 && exit().isGoto(); + if (instructions.size() == 1) { + JumpInstruction exit = exit(); + if (exit != null) + return exit.isGoto(); + } + return false; } // Go backwards in the control flow graph until a block that is not a trivial goto block is found, diff --git a/src/main/java/com/android/tools/r8/ir/code/NewArrayFilledData.java b/src/main/java/com/android/tools/r8/ir/code/NewArrayFilledData.java index 975a0c3297..3239426405 100644 --- a/src/main/java/com/android/tools/r8/ir/code/NewArrayFilledData.java +++ b/src/main/java/com/android/tools/r8/ir/code/NewArrayFilledData.java @@ -25,7 +25,6 @@ import static com.android.tools.r8.utils.EndianUtils.*; import com.android.tools.r8.utils.ArrayUtils; -import sun.misc.Unsafe; import java.util.Arrays; diff --git a/src/main/java/com/android/tools/r8/ir/code/StackValue.java b/src/main/java/com/android/tools/r8/ir/code/StackValue.java index 0d13811d21..00d5469ed6 100644 --- a/src/main/java/com/android/tools/r8/ir/code/StackValue.java +++ b/src/main/java/com/android/tools/r8/ir/code/StackValue.java @@ -8,6 +8,8 @@ import com.android.tools.r8.ir.analysis.type.Nullability; import com.android.tools.r8.ir.analysis.type.TypeElement; +import java.util.function.Predicate; + public class StackValue extends Value { private final int height; @@ -39,6 +41,14 @@ public StackValue duplicate(int height) { return new StackValue(typeInfo, getType(), height); } + @Override + public boolean isDefinedByInstructionSatisfying(Predicate predicate) { + // Can be null for exceptions that replace the stack in catch blocks. + if (definition == null) + return false; + return super.isDefinedByInstructionSatisfying(predicate); + } + @Override public boolean isValueOnStack() { return true; diff --git a/src/main/java/com/android/tools/r8/ir/conversion/CfBuilder.java b/src/main/java/com/android/tools/r8/ir/conversion/CfBuilder.java index c86d164831..0d4018be1f 100644 --- a/src/main/java/com/android/tools/r8/ir/conversion/CfBuilder.java +++ b/src/main/java/com/android/tools/r8/ir/conversion/CfBuilder.java @@ -199,6 +199,11 @@ public CfCode build(DeadCodeRemover deadCodeRemover, Timing timing) { timing.time("Rewrite Iinc patterns", this::rewriteIincPatterns); trivialGotosCollapser.run(code, timing); + + timing.begin("Insert catch handling"); + for (BasicBlock block : code.blocks) + block.onFinishBuildingInstructions(appView); + timing.end(); timing.begin("Remove redundant debug positions"); DexBuilder.removeRedundantDebugPositions(code); timing.end(); diff --git a/src/main/java/com/android/tools/r8/ir/conversion/DexSourceCode.java b/src/main/java/com/android/tools/r8/ir/conversion/DexSourceCode.java index dc42ce66fc..deb3e4bbe0 100644 --- a/src/main/java/com/android/tools/r8/ir/conversion/DexSourceCode.java +++ b/src/main/java/com/android/tools/r8/ir/conversion/DexSourceCode.java @@ -41,9 +41,7 @@ import com.android.tools.r8.graph.DexMethod; import com.android.tools.r8.graph.DexType; import com.android.tools.r8.graph.ProgramMethod; -import com.android.tools.r8.ir.code.CanonicalPositions; -import com.android.tools.r8.ir.code.CatchHandlers; -import com.android.tools.r8.ir.code.Position; +import com.android.tools.r8.ir.code.*; import com.android.tools.r8.utils.DexDebugUtils; import java.util.*; @@ -162,7 +160,8 @@ public void buildPostlude(IRBuilder builder) { @Override public void buildBlockTransfer( IRBuilder builder, int predecessorOffset, int successorOffset, boolean isExceptional) { - // Intentionally empty. Dex front-end does not support debug locals so no transfer info needed. + if (isExceptional) + builder.markCurrentBlockAsExceptionTransfer(successorOffset); } @Override diff --git a/src/main/java/com/android/tools/r8/ir/conversion/IRBuilder.java b/src/main/java/com/android/tools/r8/ir/conversion/IRBuilder.java index 1be387ca9e..c9339faa5e 100644 --- a/src/main/java/com/android/tools/r8/ir/conversion/IRBuilder.java +++ b/src/main/java/com/android/tools/r8/ir/conversion/IRBuilder.java @@ -44,81 +44,12 @@ import com.android.tools.r8.ir.analysis.type.PrimitiveTypeElement; import com.android.tools.r8.ir.analysis.type.TypeAnalysis; import com.android.tools.r8.ir.analysis.type.TypeElement; -import com.android.tools.r8.ir.code.Add; -import com.android.tools.r8.ir.code.And; -import com.android.tools.r8.ir.code.Argument; -import com.android.tools.r8.ir.code.ArrayGet; -import com.android.tools.r8.ir.code.ArrayLength; -import com.android.tools.r8.ir.code.ArrayPut; -import com.android.tools.r8.ir.code.BasicBlock; +import com.android.tools.r8.ir.code.*; import com.android.tools.r8.ir.code.BasicBlock.EdgeType; import com.android.tools.r8.ir.code.BasicBlock.ThrowingInfo; -import com.android.tools.r8.ir.code.CatchHandlers; -import com.android.tools.r8.ir.code.CheckCast; -import com.android.tools.r8.ir.code.Cmp; import com.android.tools.r8.ir.code.Cmp.Bias; -import com.android.tools.r8.ir.code.ConstClass; -import com.android.tools.r8.ir.code.ConstMethodHandle; -import com.android.tools.r8.ir.code.ConstMethodType; -import com.android.tools.r8.ir.code.ConstNumber; -import com.android.tools.r8.ir.code.ConstString; -import com.android.tools.r8.ir.code.DebugLocalRead; -import com.android.tools.r8.ir.code.DebugLocalUninitialized; -import com.android.tools.r8.ir.code.DebugLocalWrite; -import com.android.tools.r8.ir.code.DebugPosition; -import com.android.tools.r8.ir.code.DexItemBasedConstString; -import com.android.tools.r8.ir.code.Div; -import com.android.tools.r8.ir.code.Goto; -import com.android.tools.r8.ir.code.IRCode; -import com.android.tools.r8.ir.code.IRMetadata; -import com.android.tools.r8.ir.code.If; -import com.android.tools.r8.ir.code.IfType; -import com.android.tools.r8.ir.code.ImpreciseMemberTypeInstruction; -import com.android.tools.r8.ir.code.InitClass; -import com.android.tools.r8.ir.code.InstanceGet; -import com.android.tools.r8.ir.code.InstanceOf; -import com.android.tools.r8.ir.code.InstancePut; -import com.android.tools.r8.ir.code.Instruction; -import com.android.tools.r8.ir.code.InstructionListIterator; -import com.android.tools.r8.ir.code.IntSwitch; -import com.android.tools.r8.ir.code.Invoke; -import com.android.tools.r8.ir.code.InvokeCustom; -import com.android.tools.r8.ir.code.InvokeType; -import com.android.tools.r8.ir.code.JumpInstruction; -import com.android.tools.r8.ir.code.MemberType; -import com.android.tools.r8.ir.code.Monitor; -import com.android.tools.r8.ir.code.MonitorType; -import com.android.tools.r8.ir.code.MoveException; -import com.android.tools.r8.ir.code.Mul; -import com.android.tools.r8.ir.code.Neg; -import com.android.tools.r8.ir.code.NewArrayEmpty; -import com.android.tools.r8.ir.code.NewArrayFilledData; -import com.android.tools.r8.ir.code.NewInstance; -import com.android.tools.r8.ir.code.NewUnboxedEnumInstance; -import com.android.tools.r8.ir.code.Not; -import com.android.tools.r8.ir.code.NumberConversion; -import com.android.tools.r8.ir.code.NumberGenerator; -import com.android.tools.r8.ir.code.NumericType; -import com.android.tools.r8.ir.code.Or; -import com.android.tools.r8.ir.code.Phi; import com.android.tools.r8.ir.code.Phi.RegisterReadType; import com.android.tools.r8.ir.code.Phi.StackMapPhi; -import com.android.tools.r8.ir.code.Position; -import com.android.tools.r8.ir.code.RecordFieldValues; -import com.android.tools.r8.ir.code.Rem; -import com.android.tools.r8.ir.code.Return; -import com.android.tools.r8.ir.code.SafeCheckCast; -import com.android.tools.r8.ir.code.Shl; -import com.android.tools.r8.ir.code.Shr; -import com.android.tools.r8.ir.code.StaticGet; -import com.android.tools.r8.ir.code.StaticPut; -import com.android.tools.r8.ir.code.Sub; -import com.android.tools.r8.ir.code.Throw; -import com.android.tools.r8.ir.code.Ushr; -import com.android.tools.r8.ir.code.Value; -import com.android.tools.r8.ir.code.ValueType; -import com.android.tools.r8.ir.code.ValueTypeConstraint; -import com.android.tools.r8.ir.code.Xor; import com.android.tools.r8.ir.conversion.MethodConversionOptions.MutableMethodConversionOptions; import com.android.tools.r8.naming.dexitembasedstring.NameComputationInfo; import com.android.tools.r8.origin.Origin; @@ -2372,6 +2303,11 @@ private void checkRegister(int register) { } } + public void markCurrentBlockAsExceptionTransfer(int successorOffset) { + BasicBlock target = getTarget(successorOffset); + target.markCandidacyAsCatchDelegate(); + } + // Private instruction helpers. private void addInstruction(Instruction ir) { addInstruction(ir, source.getCurrentPosition()); diff --git a/src/main/java/com/android/tools/r8/ir/optimize/PhiOptimizations.java b/src/main/java/com/android/tools/r8/ir/optimize/PhiOptimizations.java index 9a5e3b9695..f820dffac3 100644 --- a/src/main/java/com/android/tools/r8/ir/optimize/PhiOptimizations.java +++ b/src/main/java/com/android/tools/r8/ir/optimize/PhiOptimizations.java @@ -145,7 +145,8 @@ private static boolean tryMovePhiToStack( if (getRelativeStackHeightForInstruction(block, phiLoad) != 0) { return false; } - for (Value operand : phi.getOperands()) { + List operands = phi.getOperands(); + for (Value operand : operands) { if (operand.definition == null || !operand.definition.isStore()) { return false; } @@ -157,8 +158,8 @@ private static boolean tryMovePhiToStack( } } // The phi can be passed through the stack. - List stores = new ArrayList<>(); - for (Value operand : phi.getOperands()) { + List stores = new ArrayList<>(operands.size()); + for (Value operand : operands) { stores.add(operand.definition.asStore()); } for (int i = 0; i < stores.size(); i++) { From 3cc8a84fbfb03d96b6af828d731599b136f5843f Mon Sep 17 00:00:00 2001 From: Col-E Date: Sun, 20 Aug 2023 02:41:38 -0400 Subject: [PATCH 4/8] Prevent trivial goto block collapsing when the block is a try-catch recovery candidate from dex-->java translation & Improve conditions for 'isTargetOfUnmovedException' in BasicBlock Added a number of extra utility methods to BasicBlock as well --- .../android/tools/r8/ir/code/BasicBlock.java | 137 ++++++++++++++---- .../com/android/tools/r8/ir/code/IRCode.java | 11 +- .../passes/TrivialGotosCollapser.java | 19 ++- 3 files changed, 135 insertions(+), 32 deletions(-) diff --git a/src/main/java/com/android/tools/r8/ir/code/BasicBlock.java b/src/main/java/com/android/tools/r8/ir/code/BasicBlock.java index f042146784..4ca65cb6af 100644 --- a/src/main/java/com/android/tools/r8/ir/code/BasicBlock.java +++ b/src/main/java/com/android/tools/r8/ir/code/BasicBlock.java @@ -38,6 +38,7 @@ import it.unimi.dsi.fastutil.ints.Int2ReferenceMap; import it.unimi.dsi.fastutil.ints.IntArrayList; import it.unimi.dsi.fastutil.ints.IntList; + import java.util.ArrayDeque; import java.util.ArrayList; import java.util.Collection; @@ -57,11 +58,12 @@ import java.util.function.Consumer; import java.util.function.Function; import java.util.function.Predicate; +import java.util.stream.Collectors; /** * Basic block abstraction. */ -public class BasicBlock { +public class BasicBlock implements Comparable { public interface BasicBlockChangeListener { void onSuccessorsMayChange(BasicBlock block); @@ -270,40 +272,96 @@ public TraversalContinuation traverseExceptionalSuccessors( return traversalContinuation; } + public Set getAllPredecessors() { + Set set = Sets.newIdentityHashSet(); + addPredecessors(set); + return set; + } + + public Set getAllSuccessors() { + Set set = Sets.newIdentityHashSet(); + addSuccessors(set); + return set; + } + + private void addPredecessors(Set set) { + for (BasicBlock predecessor : predecessors) { + if (set.add(predecessor)) + predecessor.addPredecessors(set); + } + } + + private void addSuccessors(Set set) { + for (BasicBlock successor : successors) { + if (set.add(successor)) + successor.addSuccessors(set); + } + } + private boolean isCatchDelegateCandidate; public void markCandidacyAsCatchDelegate() { isCatchDelegateCandidate = true; } + public boolean isCatchDelegateCandidate() { + return isCatchDelegateCandidate; + } + + public boolean isOnlySuccessorCatchDelegateCandidate() { + return hasUniqueSuccessor() && getUniqueSuccessor().isCatchDelegateCandidate(); + } + public void onFinishBuildingInstructions(AppView appView) { - if (isCatchDelegateCandidate && !isEntry() && isTargetOfUnmovedException() && !handlesStackException()) { - // There is no 'MoveException' here, but for converting to CF code we need to get rid of this value on the stack. - DexType exceptionType = appView.dexItemFactory().throwableType; - Value stackExceptionValue = StackValue.create(TypeVerificationHelper.createInitializedObjectType(exceptionType), 1, appView); - Pop pop = new Pop(stackExceptionValue); - pop.setPosition(entry().getPosition()); - stackExceptionValue.definition = null; - instructions.add(0, pop); - } - } - - private boolean isTargetOfUnmovedException() { - return predecessors.stream().allMatch(block -> { - if (!block.hasCatchHandlers()) - // Block has no catch handlers, so we as the target won't be a handler. - // - // If you're debugging this and see that the block is just a 'goto' then the IRCode - // is not inlining unnecessary blocks. - return false; + if (isCatchDelegateCandidate && isEventuallySuccessorOfUnmovedException()) { + if (!isCatchHandlerForAllPredecessors()) { + // Inconsistent paths, lets pray that the catch handler predecessor is the correct place to handle the 'MoveException' + BasicBlock handlerPredecessor = getUniquePredecessorWithUnmovedExceptionHandling(); + if (handlerPredecessor != null) { + handlerPredecessor.isCatchDelegateCandidate = true; + handlerPredecessor.onFinishBuildingInstructions(appView); + } + } else { + // There is no 'MoveException' here, but for converting to CF code we need to get rid of this value on the stack. + DexType exceptionType = appView.dexItemFactory().throwableType; + Value stackExceptionValue = StackValue.create(TypeVerificationHelper.createInitializedObjectType(exceptionType), 1, appView); + Pop pop = new Pop(stackExceptionValue); + pop.setPosition(entry().getPosition()); + stackExceptionValue.definition = null; + instructions.add(0, pop); + } + } + } - // Check if the predecessor block moves the exception into a variable slot. - // If it does not, then we are the target of an unmoved exception. - return !block.handlesStackException(); + /** + * @return {@code true} when this block represents some code that originates from a {@code catch {...}} block, but the exception is never handled. + */ + public boolean isEventuallySuccessorOfUnmovedException() { + // Entry point is the base case. + if (isEntry()) + return false; + + // We handle the exception, so we're not a target of an unmoved exception. + if (handlesStackException()) + return false; + + // Check if predecessors do not handle their exceptions. + return predecessors.stream().anyMatch(predecessor -> { + // The predecessor is a handler for its predecessors, and doesn't already handle the exception. + // In turn, our block also isn't handled then. + if (predecessor.isCatchHandlerForAllPredecessors() && !predecessor.handlesStackException()) + return true; + + // The predecessor declares catch handlers, and the target is this block. + if (predecessor.hasCatchHandlers() && predecessor.hasCatchSuccessor(this)) + return true; + + // TODO: We may want to limit recursion in some form down the line. + return predecessor.isEventuallySuccessorOfUnmovedException(); }); } - private boolean handlesStackException() { + public boolean handlesStackException() { // A block that operates on the caught exception will have a 'MoveException' as the first instruction. if (instructions.isEmpty()) return false; Instruction instruction = instructions.get(0); @@ -702,6 +760,12 @@ public void removePhisByIndex(List predecessorsToRemove) { } } + + @Override + public int compareTo(BasicBlock o) { + return Integer.compare(number, o.number); + } + public boolean hasPhis() { return !phis.isEmpty(); } @@ -1017,7 +1081,7 @@ private void removeCatchHandlerWithGuard(DexType guard) { } } - private boolean isCatchHandlerForSingleGuard() { + public boolean isCatchHandlerForSingleGuard() { assert predecessors.size() == 1; BasicBlock predecessor = predecessors.get(0); assert predecessor.getCatchHandlers().getAllTargets().contains(this); @@ -1030,6 +1094,29 @@ private boolean isCatchHandlerForSingleGuard() { return true; } + public boolean isCatchHandlerForAllPredecessors() { + int count = predecessors.size(); + if (count == 0) + return false; + + for (BasicBlock predecessor : predecessors) { + if (predecessor.getCatchHandlers().getAllTargets().contains(this)) { + count--; + } + } + return count==0; + } + + public BasicBlock getUniquePredecessorWithUnmovedExceptionHandling() { + List block = new ArrayList<>(2); + for (BasicBlock predecessor : predecessors) { + if (predecessor.isEventuallySuccessorOfUnmovedException()) { + block.add(predecessor); + } + } + return (block.size() != 1) ? null : block.get(0); + } + public void detachAllSuccessors() { for (BasicBlock successor : successors) { successor.getMutablePredecessors().remove(this); 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 629f81fe2e..9b50efa0f7 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 @@ -1284,8 +1284,17 @@ public void removeRedundantBlocks() { assert block.getUniqueSuccessor().getMutablePredecessors().size() == 1; assert block.getUniqueSuccessor().getMutablePredecessors().get(0) == block; assert block.getUniqueSuccessor().getPhis().size() == 0; - // Let the successor consume this block. BasicBlock successor = block.getUniqueSuccessor(); + + // Catch candidates should not be inlined. + // These candidate flags are only set when converting from Dalvik --> Java. + // They exist to ensure enough intermediate blocks exist to fill in missing exception handling from + // the converted dalvik code. Too much aggressive inlining can lead to a block having control flow + // incoming as a catch handler and regular flow, which in Java is illegal. + if (successor.isCatchDelegateCandidate() ||successor.isOnlySuccessorCatchDelegateCandidate()) + continue; + + // Let the successor consume this block. successor.getMutablePredecessors().clear(); successor.getMutablePredecessors().addAll(block.getPredecessors()); successor.getPhis().addAll(block.getPhis()); diff --git a/src/main/java/com/android/tools/r8/ir/conversion/passes/TrivialGotosCollapser.java b/src/main/java/com/android/tools/r8/ir/conversion/passes/TrivialGotosCollapser.java index 3055661edd..1443ea74f5 100644 --- a/src/main/java/com/android/tools/r8/ir/conversion/passes/TrivialGotosCollapser.java +++ b/src/main/java/com/android/tools/r8/ir/conversion/passes/TrivialGotosCollapser.java @@ -57,11 +57,9 @@ protected CodeRewriterResult rewriteCode(IRCode code) { nextBlock = iterator.hasNext() ? iterator.next() : null; if (block.isTrivialGoto()) { collapseTrivialGoto(code, block, nextBlock, blocksToRemove); - } - if (block.exit().isIf()) { + } else if (block.exit().isIf()) { collapseIfTrueTarget(block); - } - if (block.exit().isSwitch()) { + } else if (block.exit().isSwitch()) { collapseNonFallthroughSwitchTargets(block); } block = nextBlock; @@ -147,14 +145,23 @@ private void collapseTrivialGoto( BasicBlock target = block.endOfGotoChain(); - boolean needed = false; - if (target == null) { // This implies we are in a loop of GOTOs. In that case, we will iteratively remove each // trivial GOTO one-by-one until the above base case (one block targeting itself) is left. target = block.exit().asGoto().getTarget(); } + // Class file target specific checks. + if (appView.options().isGeneratingClassFiles()) { + // If the block being looked at for collapsing is a trivial goto, but the successor is a candidate for + // catch handler recovery (from dex --> java) then we don't want to collapse it. + // We will want to keep this block so that we can handle popping the exception off the stack later. + if (block.isEventuallySuccessorOfUnmovedException() && + (block.isCatchDelegateCandidate() || block.isOnlySuccessorCatchDelegateCandidate())) + return; + } + + boolean needed = false; if (target != nextBlock) { // Not targeting the fallthrough block, determine if we need this goto. We need it if // a fallthrough can hit this block. That is the case if the block is the entry block From 37af020d1bd0dda1d00c412be87a13356e0c42a0 Mon Sep 17 00:00:00 2001 From: Col-E Date: Sun, 20 Aug 2023 02:44:20 -0400 Subject: [PATCH 5/8] Collapse trivial catch blocks with trivial goto contents into a single shared block, improved try-catch allocation in CfBuilder --- .../android/tools/r8/cf/code/CfTryCatch.java | 16 ++++ .../tools/r8/ir/code/CatchHandlers.java | 8 ++ .../tools/r8/ir/conversion/CfBuilder.java | 94 ++++++++++-------- .../passes/TrivialCatchBlockMerger.java | 96 +++++++++++++++++++ 4 files changed, 176 insertions(+), 38 deletions(-) create mode 100644 src/main/java/com/android/tools/r8/ir/conversion/passes/TrivialCatchBlockMerger.java diff --git a/src/main/java/com/android/tools/r8/cf/code/CfTryCatch.java b/src/main/java/com/android/tools/r8/cf/code/CfTryCatch.java index 67f452db8f..3f8e1f087c 100644 --- a/src/main/java/com/android/tools/r8/cf/code/CfTryCatch.java +++ b/src/main/java/com/android/tools/r8/cf/code/CfTryCatch.java @@ -44,6 +44,22 @@ public void forEachTarget(Consumer consumer) { targets.forEach(consumer); } + public boolean matchingTargets(CfTryCatch other) { + return targets.equals(other.targets); + } + + public boolean matchingGuards(CfTryCatch other) { + return guards.equals(other.guards); + } + + public boolean matchingStart(CfTryCatch other) { + return start.equals(other.start); + } + + public boolean matchingEnd(CfTryCatch other) { + return end.equals(other.end); + } + public CfLabel getStart() { return start; } diff --git a/src/main/java/com/android/tools/r8/ir/code/CatchHandlers.java b/src/main/java/com/android/tools/r8/ir/code/CatchHandlers.java index 6d8a71349d..005e38d249 100644 --- a/src/main/java/com/android/tools/r8/ir/code/CatchHandlers.java +++ b/src/main/java/com/android/tools/r8/ir/code/CatchHandlers.java @@ -13,6 +13,7 @@ import java.util.List; import java.util.Set; import java.util.function.BiConsumer; +import java.util.stream.Collectors; public class CatchHandlers implements Iterable> { @@ -83,6 +84,13 @@ public Set getUniqueTargets() { return uniqueTargets; } + public CatchHandlers replaceTarget(DexType type, T old, T target) { + List newTargets = targets.stream() + .map(t -> t == old ? target : t) + .collect(Collectors.toList()); + return new CatchHandlers<>(guards, newTargets); + } + public boolean hasCatchAll(DexItemFactory factory) { return getGuards().size() > 0 && getGuards().get(getGuards().size() - 1) == factory.throwableType; diff --git a/src/main/java/com/android/tools/r8/ir/conversion/CfBuilder.java b/src/main/java/com/android/tools/r8/ir/conversion/CfBuilder.java index 0d4018be1f..20fb9519f0 100644 --- a/src/main/java/com/android/tools/r8/ir/conversion/CfBuilder.java +++ b/src/main/java/com/android/tools/r8/ir/conversion/CfBuilder.java @@ -47,6 +47,7 @@ import com.android.tools.r8.ir.code.UninitializedThisLocalRead; import com.android.tools.r8.ir.code.Value; import com.android.tools.r8.ir.code.Xor; +import com.android.tools.r8.ir.conversion.passes.TrivialCatchBlockMerger; import com.android.tools.r8.ir.conversion.passes.TrivialGotosCollapser; import com.android.tools.r8.ir.optimize.DeadCodeRemover; import com.android.tools.r8.ir.optimize.PeepholeOptimizer; @@ -147,6 +148,11 @@ public CfCode build(DeadCodeRemover deadCodeRemover, Timing timing) { LoadStoreHelper loadStoreHelper = new LoadStoreHelper(appView, code, typeVerificationHelper); loadStoreHelper.insertLoadsAndStores(); timing.end(); + + timing.begin("Merge duplicate catch handlers"); + new TrivialCatchBlockMerger(appView).run(code, timing); + timing.end(); + // Run optimizations on phis and basic blocks in a fixpoint. if (appView.options().enableLoadStoreOptimization) { timing.begin("Load store optimizations (BasicBlockMunching)"); @@ -349,50 +355,62 @@ private CfCode buildCfCode() { do { final CatchHandlers blockHandlers = block.getCatchHandlers(); CfLabel blockLabel = getLabel(block); - if (!currentCatchHandlers.equals(blockHandlers)) { - if (!currentCatchHandlers.isEmpty()) { - // From our last block iteration, we had opened a try-catch block. - CfLabel tryCatchEnd = blockLabel; - CfTryCatch tryCatchFromCurrentHandler = CfTryCatch.fromBuilder(tryCatchStart, tryCatchEnd, currentCatchHandlers, this); - if (collapseCatchHandlers) { - // We have enabled the option to collapse adjacent try blocks into the same handler. - // - // If the catch from the current handler matches a catch from a previous handler, - // meaning that the guards (thrown types) and targets (labels/offsets) are the same, - // then we will update the try-catch block to cover the start-end range of: - // - Old start - // - Current end - boolean expandedExistingRange = false; - for (int i = 0; i < tryCatchRanges.size(); i++) { - CfTryCatch existingTryCatch = tryCatchRanges.get(i); - if (existingTryCatch.guards.equals(tryCatchFromCurrentHandler.guards) && - existingTryCatch.targets.equals(tryCatchFromCurrentHandler.targets)) { - tryCatchRanges.set(i, new CfTryCatch(existingTryCatch.start, tryCatchEnd, existingTryCatch.guards, existingTryCatch.targets)); - expandedExistingRange = true; - break; - } + updateCatchBlocks: + { + if (!currentCatchHandlers.equals(blockHandlers)) { + if (!currentCatchHandlers.isEmpty()) { + // TODO: There's some weird cases where later iterations in this do-while add + // try-catches with handler blocks EARLIER than the block range. + // This is not valid. So for now we just break out of handling in this iteration if that is detected. + Set predecessors = block.getAllPredecessors(); + if (currentCatchHandlers.getAllTargets().stream().anyMatch(predecessors::contains)) { + break updateCatchBlocks; } - // If no existing handler matched, we need to add it. - if (!expandedExistingRange) - tryCatchRanges.add(tryCatchFromCurrentHandler); - // Add the try block end label. - // If it was previously added, shift it forward to the current position. - reemitLabel(tryCatchEnd); - } else { - // Simply close the try range. - tryCatchRanges.add(tryCatchFromCurrentHandler); - emitLabel(tryCatchEnd); + // From our last block iteration, we had opened a try-catch block. + CfLabel tryCatchEnd = blockLabel; + CfTryCatch tryCatchFromCurrentHandler = CfTryCatch.fromBuilder(tryCatchStart, tryCatchEnd, currentCatchHandlers, this); + if (collapseCatchHandlers) { + // We have enabled the option to collapse adjacent try blocks into the same handler. + // + // If the catch from the current handler matches a catch from a previous handler, + // meaning that the guards (thrown types) and targets (labels/offsets) are the same, + // then we will update the try-catch block to cover the start-end range of: + // - Old start + // - Current end + boolean expandedExistingRange = false; + for (int i = 0; i < tryCatchRanges.size(); i++) { + CfTryCatch existingTryCatch = tryCatchRanges.get(i); + if (existingTryCatch.matchingGuards(tryCatchFromCurrentHandler) && existingTryCatch.matchingTargets(tryCatchFromCurrentHandler)) { + tryCatchRanges.set(i, new CfTryCatch(existingTryCatch.start, tryCatchEnd, existingTryCatch.guards, existingTryCatch.targets)); + expandedExistingRange = true; + break; + } + } + + // If no existing handler matched, we need to add it. + if (!expandedExistingRange) + tryCatchRanges.add(tryCatchFromCurrentHandler); + + // Add the try block end label. + // If it was previously added, shift it forward to the current position. + reemitLabel(tryCatchEnd); + } else { + // Simply close the try range. + tryCatchRanges.add(tryCatchFromCurrentHandler); + emitLabel(tryCatchEnd); + } + } + if (!blockHandlers.isEmpty()) { + // Open a try-catch. + tryCatchStart = blockLabel; + emitLabel(tryCatchStart); } } - if (!blockHandlers.isEmpty()) { - // Open a try-catch. - tryCatchStart = blockLabel; - emitLabel(tryCatchStart); - } - currentCatchHandlers = blockHandlers; } + currentCatchHandlers = blockHandlers; + BasicBlock nextBlock = blockIterator.hasNext() ? blockIterator.next() : null; // If previousBlock is fallthrough, then it is counted in getPredecessors().size(), but // we only want to set a pendingFrame if we have a predecessor which is not previousBlock. diff --git a/src/main/java/com/android/tools/r8/ir/conversion/passes/TrivialCatchBlockMerger.java b/src/main/java/com/android/tools/r8/ir/conversion/passes/TrivialCatchBlockMerger.java new file mode 100644 index 0000000000..6418e2bf4f --- /dev/null +++ b/src/main/java/com/android/tools/r8/ir/conversion/passes/TrivialCatchBlockMerger.java @@ -0,0 +1,96 @@ +package com.android.tools.r8.ir.conversion.passes; + +import com.android.tools.r8.graph.AppInfo; +import com.android.tools.r8.graph.AppView; +import com.android.tools.r8.ir.code.BasicBlock; +import com.android.tools.r8.ir.code.CatchHandlers; +import com.android.tools.r8.ir.code.IRCode; +import com.android.tools.r8.ir.conversion.passes.result.CodeRewriterResult; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.Sets; + +import java.util.*; + +public class TrivialCatchBlockMerger extends CodeRewriterPass { + public TrivialCatchBlockMerger(AppView appView) { + super(appView); + } + + @Override + protected String getTimingId() { + return "DuplicateCatchBlockMerger"; + } + + @Override + protected boolean shouldRewriteCode(IRCode code) { + return appView.options().isGeneratingClassFiles(); + } + + @Override + protected boolean isAcceptingSSA() { + return false; + } + + @Override + protected boolean isProducingSSA() { + return false; + } + + @Override + protected CodeRewriterResult rewriteCode(IRCode code) { + // Given the following circumstances: + // + // block A1: when exception --> B1 + // block A2: when exception --> B2 + // block B1: goto C + // block B2: goto C + // block C: ... + // + // We want to make a map that holds: + // C: [B1, B2] + Map> destinationBlockToDelegateBlock = new IdentityHashMap<>(); + for (BasicBlock block : code.getBlocks()) { + if (block.isTrivialGoto() && block.isEventuallySuccessorOfUnmovedException()) { + // We are a block with just a 'goto' and are a catch handler block. + BasicBlock destination = block.getUniqueSuccessor(); + destinationBlockToDelegateBlock.computeIfAbsent(destination, d -> new TreeSet<>()).add(block); + } + } + + // For all blocks with catch-handlers, if they're trivial, collapse references to a single block. + Set removedBlocks = Sets.newIdentityHashSet(); + for (BasicBlock block : code.getBlocks()) { + if (!block.hasCatchHandlers()) + continue; + + CatchHandlers handlers = block.getCatchHandlers(); + for (CatchHandlers.CatchHandler catchHandler : ImmutableList.copyOf(handlers)) { + BasicBlock catchTarget = catchHandler.getTarget(); + BasicBlock replacement = getDestinationOfDelegateHandler(destinationBlockToDelegateBlock, catchTarget); + if (catchTarget != replacement) { + block.replaceSuccessor(catchTarget, replacement); + + // Add to pending removal queue + removedBlocks.add(catchTarget); + + // For the block removed, tell its successors that its no longer a predecessor as it will be removed. + for (BasicBlock successor : catchTarget.getSuccessors()) + successor.getMutablePredecessors().remove(catchTarget); + } + } + } + code.removeBlocks(removedBlocks); + + return CodeRewriterResult.NONE; + } + + private BasicBlock getDestinationOfDelegateHandler(Map> destinationBlockToDelegateBlock, + BasicBlock target) { + for (Map.Entry> entry : destinationBlockToDelegateBlock.entrySet()) { + SortedSet delegates = entry.getValue(); + if (delegates.contains(target)) + return delegates.first(); + } + return target; + } +} From 0ef9406e280967220aa01cb9b4b5de92efd52cfc Mon Sep 17 00:00:00 2001 From: Col-E Date: Sun, 20 Aug 2023 02:44:31 -0400 Subject: [PATCH 6/8] Use correct nonnull --- src/main/java/com/android/tools/r8/lightir/LirCode.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/java/com/android/tools/r8/lightir/LirCode.java b/src/main/java/com/android/tools/r8/lightir/LirCode.java index cb2ce16650..2687a643a8 100644 --- a/src/main/java/com/android/tools/r8/lightir/LirCode.java +++ b/src/main/java/com/android/tools/r8/lightir/LirCode.java @@ -37,8 +37,8 @@ import com.google.common.collect.ImmutableMap; import it.unimi.dsi.fastutil.ints.Int2ReferenceMap; import it.unimi.dsi.fastutil.ints.Int2ReferenceOpenHashMap; -import org.jetbrains.annotations.NotNull; +import javax.annotation.Nonnull; import java.util.Map; import java.util.function.BiConsumer; import java.util.function.Consumer; @@ -435,7 +435,7 @@ public Code getCodeAsInlining(DexMethod caller, DexEncodedMethod callee, DexItem throw new Unimplemented(); } - @NotNull + @Nonnull @Override public Code copySubtype() { // Should not be any circumstance where this model needs to be cloned. From 23730e35f56b3a7472a44b3f94ec8186a215f7e8 Mon Sep 17 00:00:00 2001 From: Col-E Date: Sun, 20 Aug 2023 02:50:32 -0400 Subject: [PATCH 7/8] Fix stack-overflow in isEventuallySuccessorOfUnmovedException() --- .../android/tools/r8/ir/code/BasicBlock.java | 49 +++++++++++-------- 1 file changed, 29 insertions(+), 20 deletions(-) diff --git a/src/main/java/com/android/tools/r8/ir/code/BasicBlock.java b/src/main/java/com/android/tools/r8/ir/code/BasicBlock.java index 4ca65cb6af..588c80e4d5 100644 --- a/src/main/java/com/android/tools/r8/ir/code/BasicBlock.java +++ b/src/main/java/com/android/tools/r8/ir/code/BasicBlock.java @@ -337,6 +337,12 @@ public void onFinishBuildingInstructions(AppView appView) { * @return {@code true} when this block represents some code that originates from a {@code catch {...}} block, but the exception is never handled. */ public boolean isEventuallySuccessorOfUnmovedException() { + Set searched = Sets.newIdentityHashSet(); + searched.add(this); + return isEventuallySuccessorOfUnmovedException(searched); + } + + private boolean isEventuallySuccessorOfUnmovedException(Set searched) { // Entry point is the base case. if (isEntry()) return false; @@ -346,7 +352,7 @@ public boolean isEventuallySuccessorOfUnmovedException() { return false; // Check if predecessors do not handle their exceptions. - return predecessors.stream().anyMatch(predecessor -> { + return predecessors.stream().filter(searched::add).anyMatch(predecessor -> { // The predecessor is a handler for its predecessors, and doesn't already handle the exception. // In turn, our block also isn't handled then. if (predecessor.isCatchHandlerForAllPredecessors() && !predecessor.handlesStackException()) @@ -356,8 +362,7 @@ public boolean isEventuallySuccessorOfUnmovedException() { if (predecessor.hasCatchHandlers() && predecessor.hasCatchSuccessor(this)) return true; - // TODO: We may want to limit recursion in some form down the line. - return predecessor.isEventuallySuccessorOfUnmovedException(); + return predecessor.isEventuallySuccessorOfUnmovedException(searched); }); } @@ -1459,29 +1464,33 @@ public String toDetailedString() { StringBuilder builder = new StringBuilder(); builder.append("block "); builder.append(number); - builder.append(", pred-counts: " + predecessors.size()); - if (unfilledPredecessorsCount > 0) { - builder.append(" (" + unfilledPredecessorsCount + " unfilled)"); - } - builder.append(", succ-count: " + successors.size()); - builder.append(", filled: " + isFilled()); - builder.append(", sealed: " + isSealed()); + // builder.append(", pred-counts: " + predecessors.size()); + // if (unfilledPredecessorsCount > 0) { + // builder.append(" (" + unfilledPredecessorsCount + " unfilled)"); + // } + // builder.append(", succ-count: " + successors.size()); + // builder.append(", filled: " + isFilled()); + // builder.append(", sealed: " + isSealed()); builder.append('\n'); builder.append("predecessors: "); - appendBasicBlockList(builder, predecessors, b -> ""); + if (predecessors.size() > 0) { + String collect = predecessors.stream() + .map(b -> b.number + (b.hasCatchSuccessor(this) ? "*" : "")) + .collect(Collectors.joining(" ")); + builder.append(" ").append(collect); + } + //appendBasicBlockList(builder, predecessors, b -> ""); builder.append('\n'); builder.append("successors: "); - appendBasicBlockList(builder, successors, this::predecessorPostfix); + //appendBasicBlockList(builder, successors, this::predecessorPostfix); if (successors.size() > 0) { - builder.append(" ("); - if (hasCatchHandlers()) { - builder.append(catchHandlers.size()); - } else { - builder.append("no"); - } - builder.append(" try/catch successors)"); + String collect = successors.stream() + .map(b -> b.number + (this.hasCatchSuccessor(b) ? "*" : "")) + .collect(Collectors.joining(" ")); + builder.append(" ").append(collect); } builder.append('\n'); + /* if (phis != null && phis.size() > 0) { for (Phi phi : phis) { builder.append(phi.printPhi()); @@ -1492,7 +1501,7 @@ public String toDetailedString() { } } else { builder.append("no phis\n"); - } + }*/ if (localsAtEntry != null) { builder.append("locals: "); StringUtils.append(builder, localsAtEntry.int2ReferenceEntrySet(), ", ", BraceType.NONE); From 4365154ab7efa3c8fa7b69ca0b3a4d5c5bfc6624 Mon Sep 17 00:00:00 2001 From: Col-E Date: Sun, 20 Aug 2023 03:23:18 -0400 Subject: [PATCH 8/8] Code and doc cleanup for recent try-catch fix work --- .../android/tools/r8/ir/code/BasicBlock.java | 66 ++++++++++--------- .../tools/r8/ir/code/CatchHandlers.java | 8 --- .../tools/r8/ir/conversion/CfBuilder.java | 2 +- .../passes/TrivialCatchBlockMerger.java | 17 +++++ 4 files changed, 54 insertions(+), 39 deletions(-) diff --git a/src/main/java/com/android/tools/r8/ir/code/BasicBlock.java b/src/main/java/com/android/tools/r8/ir/code/BasicBlock.java index 588c80e4d5..bc2a6470fb 100644 --- a/src/main/java/com/android/tools/r8/ir/code/BasicBlock.java +++ b/src/main/java/com/android/tools/r8/ir/code/BasicBlock.java @@ -23,6 +23,8 @@ import com.android.tools.r8.ir.code.Phi.RegisterReadType; import com.android.tools.r8.ir.conversion.DexBuilder; import com.android.tools.r8.ir.conversion.IRBuilder; +import com.android.tools.r8.ir.conversion.passes.TrivialCatchBlockMerger; +import com.android.tools.r8.ir.conversion.passes.TrivialGotosCollapser; import com.android.tools.r8.utils.InternalOptions; import com.android.tools.r8.utils.IterableUtils; import com.android.tools.r8.utils.ListUtils; @@ -39,6 +41,7 @@ import it.unimi.dsi.fastutil.ints.IntArrayList; import it.unimi.dsi.fastutil.ints.IntList; +import javax.annotation.Nullable; import java.util.ArrayDeque; import java.util.ArrayList; import java.util.Collection; @@ -312,14 +315,29 @@ public boolean isOnlySuccessorCatchDelegateCandidate() { return hasUniqueSuccessor() && getUniqueSuccessor().isCatchDelegateCandidate(); } - public void onFinishBuildingInstructions(AppView appView) { + /** + * In Java when you have a catch block, the exception value replaces the entire stack and is stored to the first + * variable slot (not occupied by {@code this} in virtual methods). In Dalvik bytecode exception handling + * is quite different, and the code is optimized to remove a lot of indicators of where a catch block used to be + * in the respective Java source. + *

+ * When converting from Dalvik to Java, we need to recover these cases. If we see there is no {@link MoveException} + * IR instruction but this block is within the control flow of what should be a catch block, we will want to insert it. + * Blocks that seem likely to be the catch are {@link #markCandidacyAsCatchDelegate() marked}. + * Later in IR processing, after {@link TrivialCatchBlockMerger} and {@link TrivialGotosCollapser} operations complete, + * we see if the candidacy is still valid, and if so fill in the missing exception handling by adding a {@link Pop}. + * + * @param appView + * Application view to get a {@link DexItemFactory} reference from. + */ + public void insertPopIntoValidCatchCandidate(AppView appView) { if (isCatchDelegateCandidate && isEventuallySuccessorOfUnmovedException()) { if (!isCatchHandlerForAllPredecessors()) { // Inconsistent paths, lets pray that the catch handler predecessor is the correct place to handle the 'MoveException' BasicBlock handlerPredecessor = getUniquePredecessorWithUnmovedExceptionHandling(); if (handlerPredecessor != null) { handlerPredecessor.isCatchDelegateCandidate = true; - handlerPredecessor.onFinishBuildingInstructions(appView); + handlerPredecessor.insertPopIntoValidCatchCandidate(appView); } } else { // There is no 'MoveException' here, but for converting to CF code we need to get rid of this value on the stack. @@ -334,7 +352,8 @@ public void onFinishBuildingInstructions(AppView appView) { } /** - * @return {@code true} when this block represents some code that originates from a {@code catch {...}} block, but the exception is never handled. + * @return {@code true} when this block represents some code that originates from a {@code catch {...}} block, + * but the exception is never handled. This only happens when converting from Dalvik code. */ public boolean isEventuallySuccessorOfUnmovedException() { Set searched = Sets.newIdentityHashSet(); @@ -1099,6 +1118,9 @@ public boolean isCatchHandlerForSingleGuard() { return true; } + /** + * @return {@code true} when {@link #getPredecessors() direct predecessors} all mark this block as a catch handler. + */ public boolean isCatchHandlerForAllPredecessors() { int count = predecessors.size(); if (count == 0) @@ -1112,6 +1134,11 @@ public boolean isCatchHandlerForAllPredecessors() { return count==0; } + /** + * @return The single predecessor of this block that also satisfies {@link #isEventuallySuccessorOfUnmovedException()}. + * When there are multiple predecessors, this returns {@code null}. + */ + @Nullable public BasicBlock getUniquePredecessorWithUnmovedExceptionHandling() { List block = new ArrayList<>(2); for (BasicBlock predecessor : predecessors) { @@ -1427,19 +1454,6 @@ public Collection getIncompletePhiRegisters() { return incompletePhis.keySet(); } - private static void appendBasicBlockList( - StringBuilder builder, List list, Function postfix) { - if (list.size() > 0) { - for (BasicBlock block : list) { - builder.append(block.getNumberAsString()); - builder.append(postfix.apply(block)); - builder.append(' '); - } - } else { - builder.append('-'); - } - } - @Override public String toString() { return toDetailedString(); @@ -1464,13 +1478,8 @@ public String toDetailedString() { StringBuilder builder = new StringBuilder(); builder.append("block "); builder.append(number); - // builder.append(", pred-counts: " + predecessors.size()); - // if (unfilledPredecessorsCount > 0) { - // builder.append(" (" + unfilledPredecessorsCount + " unfilled)"); - // } - // builder.append(", succ-count: " + successors.size()); - // builder.append(", filled: " + isFilled()); - // builder.append(", sealed: " + isSealed()); + builder.append(", filled: ").append(isFilled()); + builder.append(", sealed: ").append(isSealed()); builder.append('\n'); builder.append("predecessors: "); if (predecessors.size() > 0) { @@ -1479,10 +1488,8 @@ public String toDetailedString() { .collect(Collectors.joining(" ")); builder.append(" ").append(collect); } - //appendBasicBlockList(builder, predecessors, b -> ""); builder.append('\n'); builder.append("successors: "); - //appendBasicBlockList(builder, successors, this::predecessorPostfix); if (successors.size() > 0) { String collect = successors.stream() .map(b -> b.number + (this.hasCatchSuccessor(b) ? "*" : "")) @@ -1490,18 +1497,17 @@ public String toDetailedString() { builder.append(" ").append(collect); } builder.append('\n'); - /* if (phis != null && phis.size() > 0) { for (Phi phi : phis) { builder.append(phi.printPhi()); - if (incompletePhis.values().contains(phi)) { + if (incompletePhis.containsValue(phi)) { builder.append(" (incomplete)"); } builder.append('\n'); } } else { builder.append("no phis\n"); - }*/ + } if (localsAtEntry != null) { builder.append("locals: "); StringUtils.append(builder, localsAtEntry.int2ReferenceEntrySet(), ", ", BraceType.NONE); @@ -1523,9 +1529,9 @@ public String toDetailedString() { StringUtils.appendLeftPadded(builder, line, lineColumn + 1); builder.append(": "); } - StringUtils.appendLeftPadded(builder, "" + instruction.getNumber(), numberColumn + 1); + StringUtils.appendLeftPadded(builder, String.valueOf(instruction.getNumber()), numberColumn + 1); builder.append(": "); - builder.append(instruction.toString()); + builder.append(instruction); if (DebugLocalInfo.PRINT_LEVEL != PrintLevel.NONE) { if (!instruction.getDebugValues().isEmpty()) { builder.append(" [end: "); diff --git a/src/main/java/com/android/tools/r8/ir/code/CatchHandlers.java b/src/main/java/com/android/tools/r8/ir/code/CatchHandlers.java index 005e38d249..6d8a71349d 100644 --- a/src/main/java/com/android/tools/r8/ir/code/CatchHandlers.java +++ b/src/main/java/com/android/tools/r8/ir/code/CatchHandlers.java @@ -13,7 +13,6 @@ import java.util.List; import java.util.Set; import java.util.function.BiConsumer; -import java.util.stream.Collectors; public class CatchHandlers implements Iterable> { @@ -84,13 +83,6 @@ public Set getUniqueTargets() { return uniqueTargets; } - public CatchHandlers replaceTarget(DexType type, T old, T target) { - List newTargets = targets.stream() - .map(t -> t == old ? target : t) - .collect(Collectors.toList()); - return new CatchHandlers<>(guards, newTargets); - } - public boolean hasCatchAll(DexItemFactory factory) { return getGuards().size() > 0 && getGuards().get(getGuards().size() - 1) == factory.throwableType; diff --git a/src/main/java/com/android/tools/r8/ir/conversion/CfBuilder.java b/src/main/java/com/android/tools/r8/ir/conversion/CfBuilder.java index 20fb9519f0..5f39996b06 100644 --- a/src/main/java/com/android/tools/r8/ir/conversion/CfBuilder.java +++ b/src/main/java/com/android/tools/r8/ir/conversion/CfBuilder.java @@ -208,7 +208,7 @@ public CfCode build(DeadCodeRemover deadCodeRemover, Timing timing) { timing.begin("Insert catch handling"); for (BasicBlock block : code.blocks) - block.onFinishBuildingInstructions(appView); + block.insertPopIntoValidCatchCandidate(appView); timing.end(); timing.begin("Remove redundant debug positions"); DexBuilder.removeRedundantDebugPositions(code); diff --git a/src/main/java/com/android/tools/r8/ir/conversion/passes/TrivialCatchBlockMerger.java b/src/main/java/com/android/tools/r8/ir/conversion/passes/TrivialCatchBlockMerger.java index 6418e2bf4f..e92d5e366f 100644 --- a/src/main/java/com/android/tools/r8/ir/conversion/passes/TrivialCatchBlockMerger.java +++ b/src/main/java/com/android/tools/r8/ir/conversion/passes/TrivialCatchBlockMerger.java @@ -11,6 +11,23 @@ import java.util.*; +/** + * Given the following circumstances: + * 

{@code
+ * block A1: when exception --> B1
+ * block A2: when exception --> B2
+ * block B1: goto C
+ * block B2: goto C
+ * block C: ...
+ * }
+ * We want to rewrite the blocks to: + * 
{@code
+ * block A1: when exception --> B1
+ * block A2: when exception --> B1 // Now points to the identical B1
+ * block B1: goto C // B2 removed
+ * block C: ...
+ * }
+ */ public class TrivialCatchBlockMerger extends CodeRewriterPass { public TrivialCatchBlockMerger(AppView appView) { super(appView);