Reland "Synthesize null-throwing body for abstract methods on non-abstract classes"
This reverts commit f86a2c94f6abebf6819bd6919672ac85eef33a98.
Change-Id: I58ff6b16316e9ed5e6fe49d09e90d026293dae98
diff --git a/src/main/java/com/android/tools/r8/R8.java b/src/main/java/com/android/tools/r8/R8.java
index 43bc47d..a1ca852 100644
--- a/src/main/java/com/android/tools/r8/R8.java
+++ b/src/main/java/com/android/tools/r8/R8.java
@@ -763,7 +763,7 @@
}
// Validity checks.
- assert application.classes().stream().allMatch(DexClass::isValid);
+ assert application.classes().stream().allMatch(clazz -> clazz.isValid(options));
assert appView.rootSet().verifyKeptItemsAreKept(application, appView.appInfo());
assert appView
.graphLense()
diff --git a/src/main/java/com/android/tools/r8/dex/DexParser.java b/src/main/java/com/android/tools/r8/dex/DexParser.java
index 3ef485f..df86e96 100644
--- a/src/main/java/com/android/tools/r8/dex/DexParser.java
+++ b/src/main/java/com/android/tools/r8/dex/DexParser.java
@@ -600,8 +600,12 @@
return fields;
}
- private DexEncodedMethod[] readMethods(int size, DexMethodAnnotation[] annotations,
- DexParameterAnnotation[] parameters, boolean skipCodes) {
+ private DexEncodedMethod[] readMethods(
+ int size,
+ DexMethodAnnotation[] annotations,
+ DexParameterAnnotation[] parameters,
+ boolean skipCodes,
+ boolean ensureNonAbstract) {
DexEncodedMethod[] methods = new DexEncodedMethod[size];
int methodIndex = 0;
MemberAnnotationIterator<DexMethod, DexAnnotationSet> annotationIterator =
@@ -618,8 +622,21 @@
code = codes.get(codeOff);
}
DexMethod method = indexedItems.getMethod(methodIndex);
- methods[i] = new DexEncodedMethod(method, accessFlags, annotationIterator.getNextFor(method),
- parameterAnnotationsIterator.getNextFor(method), code);
+ DexEncodedMethod encodedMethod =
+ new DexEncodedMethod(
+ method,
+ accessFlags,
+ annotationIterator.getNextFor(method),
+ parameterAnnotationsIterator.getNextFor(method),
+ code);
+ if (accessFlags.isAbstract() && ensureNonAbstract) {
+ accessFlags.unsetAbstract();
+ encodedMethod =
+ options.isGeneratingClassFiles()
+ ? encodedMethod.toEmptyThrowingMethodCf()
+ : encodedMethod.toEmptyThrowingMethodDex();
+ }
+ methods[i] = encodedMethod;
}
return methods;
}
@@ -692,13 +709,17 @@
directMethodsSize,
annotationsDirectory.methods,
annotationsDirectory.parameters,
- classKind != ClassKind.PROGRAM);
+ classKind != ClassKind.PROGRAM,
+ options.canHaveDalvikAbstractMethodOnNonAbstractClassVerificationBug()
+ && !flags.isAbstract());
virtualMethods =
readMethods(
virtualMethodsSize,
annotationsDirectory.methods,
annotationsDirectory.parameters,
- classKind != ClassKind.PROGRAM);
+ classKind != ClassKind.PROGRAM,
+ options.canHaveDalvikAbstractMethodOnNonAbstractClassVerificationBug()
+ && !flags.isAbstract());
}
AttributesAndAnnotations attrs =
diff --git a/src/main/java/com/android/tools/r8/graph/DexClass.java b/src/main/java/com/android/tools/r8/graph/DexClass.java
index 9a1ea4c..4a2e1e3 100644
--- a/src/main/java/com/android/tools/r8/graph/DexClass.java
+++ b/src/main/java/com/android/tools/r8/graph/DexClass.java
@@ -262,6 +262,19 @@
return true;
}
+ private boolean verifyNoAbstractMethodsOnNonAbstractClasses(
+ Iterable<DexEncodedMethod> methods, InternalOptions options) {
+ if (options.canHaveDalvikAbstractMethodOnNonAbstractClassVerificationBug()) {
+ if (!isAbstract()) {
+ for (DexEncodedMethod method : methods) {
+ assert !method.isAbstract()
+ : "Non-abstract method on abstract class: `" + method.method.toSourceString() + "`";
+ }
+ }
+ }
+ return true;
+ }
+
private boolean verifyNoDuplicateMethods() {
Set<DexMethod> unique = Sets.newIdentityHashSet();
for (DexEncodedMethod method : methods()) {
@@ -572,7 +585,10 @@
return null;
}
- // Tells whether this is an interface.
+ public boolean isAbstract() {
+ return accessFlags.isAbstract();
+ }
+
public boolean isInterface() {
return accessFlags.isInterface();
}
@@ -876,9 +892,9 @@
return getKotlinInfo() != null;
}
- public boolean isValid() {
- assert !isInterface()
- || Arrays.stream(virtualMethods).noneMatch(method -> method.accessFlags.isFinal());
+ public boolean isValid(InternalOptions options) {
+ assert verifyNoAbstractMethodsOnNonAbstractClasses(virtualMethods(), options);
+ assert !isInterface() || virtualMethods().stream().noneMatch(DexEncodedMethod::isFinal);
assert verifyCorrectnessOfFieldHolders(fields());
assert verifyNoDuplicateFields();
assert verifyCorrectnessOfMethodHolders(methods());
diff --git a/src/main/java/com/android/tools/r8/graph/DexEncodedMethod.java b/src/main/java/com/android/tools/r8/graph/DexEncodedMethod.java
index cb90f1f..b44ad18 100644
--- a/src/main/java/com/android/tools/r8/graph/DexEncodedMethod.java
+++ b/src/main/java/com/android/tools/r8/graph/DexEncodedMethod.java
@@ -231,6 +231,14 @@
return compilationState != CompilationState.NOT_PROCESSED;
}
+ public boolean isAbstract() {
+ return accessFlags.isAbstract();
+ }
+
+ public boolean isFinal() {
+ return accessFlags.isFinal();
+ }
+
public boolean isInitializer() {
checkIfObsolete();
return isInstanceInitializer() || isClassInitializer();
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 0d42996..a65dd20 100644
--- a/src/main/java/com/android/tools/r8/utils/InternalOptions.java
+++ b/src/main/java/com/android/tools/r8/utils/InternalOptions.java
@@ -1250,4 +1250,12 @@
public boolean canHaveDalvikCatchHandlerVerificationBug() {
return minApiLevel < AndroidApiLevel.L.getLevel();
}
+
+ // Having an invoke instruction that targets an abstract method on a non-abstract class will fail
+ // with a verification error.
+ //
+ // See b/132953944.
+ public boolean canHaveDalvikAbstractMethodOnNonAbstractClassVerificationBug() {
+ return minApiLevel < AndroidApiLevel.L.getLevel();
+ }
}
diff --git a/src/test/java/com/android/tools/r8/R8RunArtTestsTest.java b/src/test/java/com/android/tools/r8/R8RunArtTestsTest.java
index 2b32d66..0e60a99 100644
--- a/src/test/java/com/android/tools/r8/R8RunArtTestsTest.java
+++ b/src/test/java/com/android/tools/r8/R8RunArtTestsTest.java
@@ -118,6 +118,8 @@
private static Map<String, AndroidApiLevel> needMinSdkVersion =
new ImmutableMap.Builder<String, AndroidApiLevel>()
.put("004-JniTest", AndroidApiLevel.N)
+ // Has a non-abstract class with an abstract method.
+ .put("800-smali", AndroidApiLevel.L)
// Android O
.put("952-invoke-custom", AndroidApiLevel.O)
.put("952-invoke-custom-kinds", AndroidApiLevel.O)
@@ -1098,13 +1100,20 @@
// When running R8 on dex input (D8, DX) this test non-deterministically fails
// with a compiler exception, due to invoke-virtual on an interface, or it completes but
// the output when run on Art is not as expected. b/65233869
- .put("162-method-resolution",
+ .put(
+ "162-method-resolution",
TestCondition.match(
- TestCondition.tools(DexTool.DX, DexTool.NONE),
- TestCondition.R8_COMPILER))
+ TestCondition.tools(DexTool.DX, DexTool.NONE), TestCondition.R8_COMPILER))
// Produces wrong output when compiled in release mode, which we cannot express.
- .put("015-switch",
- TestCondition.match(TestCondition.runtimes(DexVm.Version.V4_0_4)))
+ .put("015-switch", TestCondition.match(TestCondition.runtimes(DexVm.Version.V4_0_4)))
+ // To prevent "Dex file with version '37' cannot be used with min sdk level '21'", which
+ // would otherwise happen because D8 passes through the DEX code from DX.
+ .put(
+ "800-smali",
+ TestCondition.match(
+ TestCondition.tools(DexTool.DX),
+ TestCondition.D8_COMPILER,
+ TestCondition.runtimesFrom(DexVm.Version.V5_1_1)))
.build();
public static List<String> requireInliningToBeDisabled =
diff --git a/src/test/java/com/android/tools/r8/smali/SmaliBuilder.java b/src/test/java/com/android/tools/r8/smali/SmaliBuilder.java
index dc980d7..e9368e9 100644
--- a/src/test/java/com/android/tools/r8/smali/SmaliBuilder.java
+++ b/src/test/java/com/android/tools/r8/smali/SmaliBuilder.java
@@ -85,13 +85,23 @@
public class ClassBuilder extends Builder {
+ private boolean isAbstract = false;
+
ClassBuilder(String name, String superName, List<String> implementedInterfaces) {
super(name, superName, implementedInterfaces);
}
+ public ClassBuilder setAbstract() {
+ isAbstract = true;
+ return this;
+ }
+
public String toString() {
StringBuilder builder = new StringBuilder();
builder.append(".class public ");
+ if (isAbstract) {
+ builder.append("abstract ");
+ }
builder.append(DescriptorUtils.javaTypeToDescriptor(name));
builder.append("\n");
appendSuper(builder);
@@ -164,10 +174,12 @@
addClass(name, superName, ImmutableList.of());
}
- public void addClass(String name, String superName, List<String> implementedInterfaces) {
+ public ClassBuilder addClass(String name, String superName, List<String> implementedInterfaces) {
assert !classes.containsKey(name);
currentClassName = name;
- classes.put(name, new ClassBuilder(name, superName, implementedInterfaces));
+ ClassBuilder classBuilder = new ClassBuilder(name, superName, implementedInterfaces);
+ classes.put(name, classBuilder);
+ return classBuilder;
}
public void addInterface(String name) {
diff --git a/src/test/java/com/android/tools/r8/smali/SmaliDisassembleTest.java b/src/test/java/com/android/tools/r8/smali/SmaliDisassembleTest.java
index 2297ebd..bbeed31 100644
--- a/src/test/java/com/android/tools/r8/smali/SmaliDisassembleTest.java
+++ b/src/test/java/com/android/tools/r8/smali/SmaliDisassembleTest.java
@@ -380,20 +380,20 @@
@Test
public void implementsInterface() {
SmaliBuilder builder = new SmaliBuilder();
- builder.addClass("Test", "java.lang.Object", ImmutableList.of("java.util.List"));
+ builder.addClass("Test", "java.lang.Object", ImmutableList.of("java.util.List")).setAbstract();
builder.addAbstractMethod("int", "test", ImmutableList.of());
AndroidApp application = buildApplication(builder);
String expected =
- ".class public LTest;\n" +
- "\n" +
- ".super Ljava/lang/Object;\n" +
- ".implements Ljava/util/List;\n" +
- "\n" +
- ".method public abstract test()I\n" +
- ".end method\n" +
- "\n" +
- "# End of class LTest;\n";
+ ".class public abstract LTest;\n"
+ + "\n"
+ + ".super Ljava/lang/Object;\n"
+ + ".implements Ljava/util/List;\n"
+ + "\n"
+ + ".method public abstract test()I\n"
+ + ".end method\n"
+ + "\n"
+ + "# End of class LTest;\n";
assertEquals(expected, SmaliWriter.smali(application, new InternalOptions()));
