Version 1.3.47.
Merge: Workaround Art instance-of verifier bug.
CL: https://r8-review.googlesource.com/c/r8/+/32222
R=zerny@google.com
Bug: 120985556
Change-Id: Ia0fdb2c71c67a1c04b1734546badb1d400d60015
diff --git a/src/main/java/com/android/tools/r8/Version.java b/src/main/java/com/android/tools/r8/Version.java
index 3d5acb9..62b346a 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.46";
+ public static final String LABEL = "1.3.47";
private Version() {
}
diff --git a/src/main/java/com/android/tools/r8/ir/code/InstanceOf.java b/src/main/java/com/android/tools/r8/ir/code/InstanceOf.java
index c5dc5e4..acce406 100644
--- a/src/main/java/com/android/tools/r8/ir/code/InstanceOf.java
+++ b/src/main/java/com/android/tools/r8/ir/code/InstanceOf.java
@@ -41,7 +41,7 @@
public void buildDex(DexBuilder builder) {
int dest = builder.allocatedRegister(dest(), getNumber());
int value = builder.allocatedRegister(value(), getNumber());
- builder.add(this, new com.android.tools.r8.code.InstanceOf(dest, value, type));
+ builder.addInstanceOf(this, new com.android.tools.r8.code.InstanceOf(dest, value, type));
}
@Override
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 4e11690..8927476 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
@@ -21,6 +21,7 @@
import com.android.tools.r8.code.IfLtz;
import com.android.tools.r8.code.IfNe;
import com.android.tools.r8.code.IfNez;
+import com.android.tools.r8.code.InstanceOf;
import com.android.tools.r8.code.Instruction;
import com.android.tools.r8.code.Move16;
import com.android.tools.r8.code.MoveFrom16;
@@ -103,6 +104,9 @@
// getInfo/setInfo methods to access the mapping.
private Info[] instructionToInfo;
+ // Keeps track of the previous non-fallthrough info added to the dex builder.
+ private Info previousNonFallthroughInfo;
+
// The number of ingoing and outgoing argument registers for the code.
private int inRegisterCount = 0;
private int outRegisterCount = 0;
@@ -418,6 +422,38 @@
}
}
+ private boolean needsNopBetweenMoveAndInstanceOf(InstanceOf instanceOf) {
+ if (!options.canHaveArtInstanceOfVerifierBug()) {
+ return false;
+ }
+ if (previousNonFallthroughInfo instanceof MoveInfo) {
+ MoveInfo moveInfo = (MoveInfo) previousNonFallthroughInfo;
+ int moveSrcRegister = moveInfo.srcRegister(this);
+ int moveDestRegister = moveInfo.destRegister(this);
+ // If the previous move materializes as a move and we have the pattern:
+ //
+ // move vA, vB
+ // instance-of vB, vA, Type
+ //
+ // we insert a nop between the move and the instance-of instruction to make sure
+ // that we do not trigger a verifier bug in Art. See b/120985556.
+ if (moveSrcRegister != moveDestRegister
+ && moveSrcRegister == instanceOf.A
+ && moveDestRegister == instanceOf.B) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ public void addInstanceOf(com.android.tools.r8.ir.code.InstanceOf ir, InstanceOf instanceOf) {
+ if (needsNopBetweenMoveAndInstanceOf(instanceOf)) {
+ add(ir, new Nop(), instanceOf);
+ } else {
+ add(ir, instanceOf);
+ }
+ }
+
public void addIf(If branch) {
assert nextBlock == branch.fallthroughBlock();
add(branch, new IfInfo(branch));
@@ -512,6 +548,9 @@
}
private void setInfo(com.android.tools.r8.ir.code.Instruction instruction, Info info) {
+ if (!(info instanceof FallThroughInfo)) {
+ previousNonFallthroughInfo = info;
+ }
instructionToInfo[instructionNumberToIndex(instruction.getNumber())] = info;
}
@@ -1158,11 +1197,18 @@
return (Move) getIR();
}
+ public int srcRegister(DexBuilder builder) {
+ return builder.argumentOrAllocateRegister(getMove().src(), getMove().getNumber());
+ }
+
+ public int destRegister(DexBuilder builder) {
+ return builder.allocatedRegister(getMove().dest(), getMove().getNumber());
+ }
+
@Override
public int computeSize(DexBuilder builder) {
- Move move = getMove();
- int srcRegister = builder.argumentOrAllocateRegister(move.src(), move.getNumber());
- int destRegister = builder.allocatedRegister(move.dest(), move.getNumber());
+ int srcRegister = srcRegister(builder);
+ int destRegister = destRegister(builder);
if (srcRegister == destRegister) {
size = 1;
} else if (srcRegister <= Constants.U4BIT_MAX && destRegister <= Constants.U4BIT_MAX) {
@@ -1179,8 +1225,8 @@
public void addInstructions(DexBuilder builder, List<Instruction> instructions) {
Move move = getMove();
MoveType moveType = MoveType.fromValueType(move.outType());
- int src = builder.argumentOrAllocateRegister(move.src(), move.getNumber());
- int dest = builder.allocatedRegister(move.dest(), move.getNumber());
+ int src = srcRegister(builder);
+ int dest = destRegister(builder);
Instruction instruction = null;
switch (size) {
case 1:
diff --git a/src/main/java/com/android/tools/r8/utils/InternalOptions.java b/src/main/java/com/android/tools/r8/utils/InternalOptions.java
index 2612d0b..1299091 100644
--- a/src/main/java/com/android/tools/r8/utils/InternalOptions.java
+++ b/src/main/java/com/android/tools/r8/utils/InternalOptions.java
@@ -752,4 +752,32 @@
public boolean canHaveTracingPastInstructionsStreamBug() {
return minApiLevel < AndroidApiLevel.L.getLevel();
}
+
+ // The art verifier incorrectly propagates type information for the following pattern:
+ //
+ // move vA, vB
+ // instance-of vB, vA, Type
+ // if-eqz/nez vB
+ //
+ // In that case it will assume that vB has object type after the if. Therefore, if the
+ // result of the instance-of operation is reused in a boolean context the verifier will
+ // fail with a type conflict.
+ //
+ // In order to make sure that cannot happen, we insert a nop between the move and
+ // the instance-of instruction so that this pattern in the art verifier does not
+ // match.
+ //
+ // move vA, vB
+ // nop
+ // instance-of vB, vA, Type
+ // if-eqz/nez vB
+ //
+ // This happens rarely, but it can happen in debug mode where the move
+ // put a value into a new register which has associated locals information.
+ //
+ // See b/120985556.
+ public boolean canHaveArtInstanceOfVerifierBug() {
+ // TODO(ager): Update this with an actual bound when the issue has been fixed.
+ return true;
+ }
}