Add a workaround for verifier type propagation bug
Bug: b/335663487
Change-Id: Ia1d1b573edf83a1b6cc6f9f851724e342485812d
diff --git a/src/main/java/com/android/tools/r8/ir/analysis/type/ClassTypeElement.java b/src/main/java/com/android/tools/r8/ir/analysis/type/ClassTypeElement.java
index e966ed6..6871e22 100644
--- a/src/main/java/com/android/tools/r8/ir/analysis/type/ClassTypeElement.java
+++ b/src/main/java/com/android/tools/r8/ir/analysis/type/ClassTypeElement.java
@@ -26,6 +26,7 @@
import java.util.Queue;
import java.util.Set;
import java.util.function.Function;
+import java.util.function.Predicate;
import java.util.stream.Collectors;
public class ClassTypeElement extends ReferenceTypeElement {
@@ -129,6 +130,11 @@
}
@Override
+ public boolean isClassType(Predicate<ClassTypeElement> predicate) {
+ return predicate.test(this);
+ }
+
+ @Override
public ClassTypeElement asClassType() {
return this;
}
diff --git a/src/main/java/com/android/tools/r8/ir/analysis/type/TypeElement.java b/src/main/java/com/android/tools/r8/ir/analysis/type/TypeElement.java
index a353a7e..3f1fc3d 100644
--- a/src/main/java/com/android/tools/r8/ir/analysis/type/TypeElement.java
+++ b/src/main/java/com/android/tools/r8/ir/analysis/type/TypeElement.java
@@ -15,6 +15,7 @@
import java.util.Collections;
import java.util.Set;
import java.util.function.Function;
+import java.util.function.Predicate;
/** The base abstraction of lattice elements for local type analysis. */
public abstract class TypeElement {
@@ -292,6 +293,10 @@
return false;
}
+ public boolean isClassType(Predicate<ClassTypeElement> predicate) {
+ return false;
+ }
+
@SuppressWarnings("ReferenceEquality")
public final boolean isClassType(DexType type) {
assert type.isClassType();
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/RuntimeWorkaroundCodeRewriter.java b/src/main/java/com/android/tools/r8/ir/optimize/RuntimeWorkaroundCodeRewriter.java
index f8caa1b..44ab531 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/RuntimeWorkaroundCodeRewriter.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/RuntimeWorkaroundCodeRewriter.java
@@ -75,13 +75,38 @@
public static boolean workaroundInstanceOfTypeWeakeningInVerifier(
AppView<?> appView, IRCode code) {
boolean didReplaceInstructions = false;
+ if (!appView.options().canHaveArtFalsyInstanceOfVerifierBug()
+ || !code.metadata().mayHaveInstanceOf()) {
+ return didReplaceInstructions;
+ }
+ DexType objectType = appView.dexItemFactory().objectType;
for (BasicBlock block : code.getBlocks()) {
InstructionListIterator instructionIterator = block.listIterator(code);
while (instructionIterator.hasNext()) {
InstanceOf instanceOf = instructionIterator.nextUntil(Instruction::isInstanceOf);
- if (instanceOf != null && instanceOf.value().getType().isNullType()) {
+ if (instanceOf == null) {
+ continue;
+ }
+ TypeElement valueType = instanceOf.value().getType();
+
+ // Workaround b/288273207.
+ if (valueType.isNullType()) {
instructionIterator.replaceCurrentInstructionWithConstFalse(code);
didReplaceInstructions = true;
+ continue;
+ }
+
+ // Workaround b/335663487.
+ if (appView.enableWholeProgramOptimizations()
+ && valueType.isClassType(t -> t.getClassType().isNotIdenticalTo(objectType))) {
+ TypeElement instanceOfType =
+ instanceOf.type().toTypeElement(appView, valueType.nullability());
+ if (instanceOfType.isClassType(t -> t.getClassType().isNotIdenticalTo(objectType))
+ && !instanceOfType.lessThanOrEqual(valueType, appView)
+ && !valueType.lessThanOrEqual(instanceOfType, appView)) {
+ instructionIterator.replaceCurrentInstructionWithConstFalse(code);
+ didReplaceInstructions = true;
+ }
}
}
}
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 581faa0..0bfd3c9 100644
--- a/src/main/java/com/android/tools/r8/utils/InternalOptions.java
+++ b/src/main/java/com/android/tools/r8/utils/InternalOptions.java
@@ -3073,6 +3073,22 @@
return canHaveBugPresentUntil(AndroidApiLevel.Q);
}
+ // The art verifier incorrectly propagates type information for instance-of instructions that
+ // always evaluate to false (b/288273207, b/335663487).
+ //
+ // Type t = new Type()
+ // Object o = t
+ // if (o instanceof UnrelatedToType) {
+ // t.f <- VerifyError: Cannot read field f from object of type UnrelatedToType.
+ // }
+ //
+ // This can happen with D8, but is most likely to hit in R8 after inlining.
+ //
+ // Fixed in Android V.
+ public boolean canHaveArtFalsyInstanceOfVerifierBug() {
+ return canHaveBugPresentUntilInclusive(AndroidApiLevel.U);
+ }
+
// Some Art Lollipop version do not deal correctly with long-to-int conversions.
//
// In particular, the following code performs an out of bounds array access when the
diff --git a/src/test/java/com/android/tools/r8/workaround/IncorrectTypeRefinementForThrowableTest.java b/src/test/java/com/android/tools/r8/workaround/IncorrectTypeRefinementForThrowableTest.java
index bf3f43e..0a242c9 100644
--- a/src/test/java/com/android/tools/r8/workaround/IncorrectTypeRefinementForThrowableTest.java
+++ b/src/test/java/com/android/tools/r8/workaround/IncorrectTypeRefinementForThrowableTest.java
@@ -26,21 +26,32 @@
}
@Test
- public void testD8() throws Exception {
+ public void testJvm() throws Exception {
+ parameters.assumeJvmTestParameters();
+ testForJvm(parameters)
+ .addInnerClasses(getClass())
+ .run(parameters.getRuntime(), Main.class)
+ .assertSuccessWithEmptyOutput();
+ }
+
+ @Test
+ public void testD8Debug() throws Exception {
parameters.assumeDexRuntime();
testForD8(parameters.getBackend())
.addInnerClasses(getClass())
- .release()
+ .debug()
.setMinApi(parameters)
.run(parameters.getRuntime(), Main.class)
.assertSuccessWithEmptyOutput();
}
@Test
- public void testJvm() throws Exception {
- parameters.assumeJvmTestParameters();
- testForJvm(parameters)
+ public void testD8Release() throws Exception {
+ parameters.assumeDexRuntime();
+ testForD8(parameters.getBackend())
.addInnerClasses(getClass())
+ .release()
+ .setMinApi(parameters)
.run(parameters.getRuntime(), Main.class)
.assertSuccessWithEmptyOutput();
}
diff --git a/src/test/java/com/android/tools/r8/workaround/InvalidVerifierTypePropagationAfterInstanceOfTest.java b/src/test/java/com/android/tools/r8/workaround/InvalidVerifierTypePropagationAfterInstanceOfTest.java
index c9c4c4b..047dadc 100644
--- a/src/test/java/com/android/tools/r8/workaround/InvalidVerifierTypePropagationAfterInstanceOfTest.java
+++ b/src/test/java/com/android/tools/r8/workaround/InvalidVerifierTypePropagationAfterInstanceOfTest.java
@@ -31,7 +31,7 @@
testForJvm(parameters)
.addInnerClasses(getClass())
.run(parameters.getRuntime(), Main.class)
- .apply(this::checkRunResult);
+ .apply(runResult -> checkRunResult(runResult, false));
}
@Test
@@ -41,7 +41,7 @@
.applyIf(parameters.isDexRuntime(), b -> b.setMinApi(parameters))
.debug()
.run(parameters.getRuntime(), Main.class)
- .apply(this::checkRunResult);
+ .apply(runResult -> checkRunResult(runResult, false));
}
@Test
@@ -51,7 +51,7 @@
.applyIf(parameters.isDexRuntime(), b -> b.setMinApi(parameters))
.release()
.run(parameters.getRuntime(), Main.class)
- .apply(this::checkRunResult);
+ .apply(runResult -> checkRunResult(runResult, false));
}
@Test
@@ -62,14 +62,15 @@
.setMinApi(parameters)
.compile()
.run(parameters.getRuntime(), Main.class)
- .apply(this::checkRunResult);
+ .apply(runResult -> checkRunResult(runResult, true));
}
- private void checkRunResult(TestRunResult<?> runResult) {
+ private void checkRunResult(TestRunResult<?> runResult, boolean isR8) {
runResult.applyIf(
parameters.isCfRuntime()
|| parameters.getDexRuntimeVersion().isOlderThan(Version.V7_0_0)
- || parameters.getDexRuntimeVersion().isNewerThanOrEqual(Version.V15_0_0),
+ || parameters.getDexRuntimeVersion().isNewerThanOrEqual(Version.V15_0_0)
+ || isR8,
TestRunResult::assertSuccessWithEmptyOutput,
rr -> runResult.assertFailureWithErrorThatThrows(VerifyError.class));
}
