Version 1.5.44 Cherry pick: Reland "Synthesize null-throwing body for abstract methods on non-abstract classes" CL: https://r8-review.googlesource.com/c/r8/+/39220 Cherry pick: Synthesize throwing body when merging abstract method to non-abstract class CL: https://r8-review.googlesource.com/c/r8/+/39222 Cherry pick: Disable value propagation for constructors CL: https://r8-review.googlesource.com/c/r8/+/39223 Cherry pick: Remove redundant debug positions before numbering instructions CL: https://r8-review.googlesource.com/c/r8/+/39300 Cherry-pick: Fix arg type of null in outline candidates. CL: https://r8-review.googlesource.com/c/r8/+/39200 Bug: 132953944, 134462736 Change-Id: Ief4886f697b22c91765a1175b7cc39c235f358c6
diff --git a/src/main/java/com/android/tools/r8/R8.java b/src/main/java/com/android/tools/r8/R8.java index 5c0e9f7..daf3d43 100644 --- a/src/main/java/com/android/tools/r8/R8.java +++ b/src/main/java/com/android/tools/r8/R8.java
@@ -718,7 +718,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/Version.java b/src/main/java/com/android/tools/r8/Version.java index 668e101..1041a82 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.5.43"; + public static final String LABEL = "1.5.44"; private Version() { }
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 a301563..b62bcaa 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(); } @@ -847,9 +863,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 97fd72c..d2f8e2d 100644 --- a/src/main/java/com/android/tools/r8/graph/DexEncodedMethod.java +++ b/src/main/java/com/android/tools/r8/graph/DexEncodedMethod.java
@@ -227,6 +227,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(); @@ -566,6 +574,12 @@ return generateCodeFromTemplate(1, 0, insn); } + public DexEncodedMethod toEmptyThrowingMethod(InternalOptions options) { + return options.isGeneratingClassFiles() + ? toEmptyThrowingMethodCf() + : toEmptyThrowingMethodDex(); + } + public DexEncodedMethod toEmptyThrowingMethodDex() { checkIfObsolete(); assert !shouldNotHaveCode();
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 a6e556d..7803adc 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
@@ -182,13 +182,13 @@ // large for the if encoding. rewriteIfs(); - // Reset the state of the builder to start from scratch. - reset(); - // Remove redundant debug position instructions. They would otherwise materialize as // unnecessary nops. removeRedundantDebugPositions(ir); + // Reset the state of the builder to start from scratch. + reset(); + // Populate the builder info objects. numberOfInstructions = 0; @@ -667,10 +667,12 @@ // Helper used by the info objects. private Info getInfo(com.android.tools.r8.ir.code.Instruction instruction) { + assert instruction.getNumber() >= 0; return instructionToInfo[instructionNumberToIndex(instruction.getNumber())]; } private void setInfo(com.android.tools.r8.ir.code.Instruction instruction, Info info) { + assert instruction.getNumber() >= 0; if (!(info instanceof FallThroughInfo)) { previousNonFallthroughInfo = info; }
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 615b4c1..3e5ff2f 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
@@ -219,6 +219,13 @@ return; } DexEncodedMethod definition = current.lookupSingleTarget(appView, callingContext); + if (definition != null && definition.isInstanceInitializer()) { + // Member value propagation does not apply to constructors. Removing a call to a constructor + // that is marked as having no side effects could lead to verification errors, due to + // uninitialized instances being used. + return; + } + ProguardMemberRuleLookup lookup = lookupMemberRule(definition); if (lookup == null) { // Since -assumenosideeffects rules are always applied to all overriding methods, we can @@ -233,8 +240,10 @@ if (lookup.type == RuleType.ASSUME_NO_SIDE_EFFECTS && outValueNullOrNotUsed) { // Remove invoke if marked as having no side effects and the return value is not used. iterator.removeOrReplaceByDebugLocalRead(); - invokeReplaced = true; - } else if (!outValueNullOrNotUsed) { + return; + } + + if (!outValueNullOrNotUsed) { // Check to see if a constant value can be assumed. invokeReplaced = tryConstantReplacementFromProguard(
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/Outliner.java b/src/main/java/com/android/tools/r8/ir/optimize/Outliner.java index 81236f8..33c8c96 100644 --- a/src/main/java/com/android/tools/r8/ir/optimize/Outliner.java +++ b/src/main/java/com/android/tools/r8/ir/optimize/Outliner.java
@@ -956,7 +956,7 @@ // For values of lattice type java.lang.Object and only one interface use the interface as // the type of the outline argument. If there are several interfaces these interfaces don't // have a common super interface nor are they implemented by a common superclass so the - // argument type of the ouline will be java.lang.Object. + // argument type of the outline will be java.lang.Object. if (valueClassTypeLatticeElement.getClassType() == objectType && valueClassTypeLatticeElement.getInterfaces().size() == 1) { return valueClassTypeLatticeElement.getInterfaces().iterator().next(); @@ -966,8 +966,8 @@ } else if (valueLatticeElement.isArrayType()) { return value.getTypeLattice().asArrayTypeLatticeElement().getArrayType(itemFactory); } else if (valueLatticeElement.isNullType()) { - // For values which are always null use java.lang.Object as the outline argument type. - return objectType; + // For values which are always null use the actual type at the call site. + return argumentTypeFromInvoke(invoke, argumentIndex); } else { assert valueLatticeElement.isPrimitive(); assert valueLatticeElement.asPrimitiveTypeLatticeElement().hasDexType();
diff --git a/src/main/java/com/android/tools/r8/shaking/VerticalClassMerger.java b/src/main/java/com/android/tools/r8/shaking/VerticalClassMerger.java index 25c61d8..0bc0284 100644 --- a/src/main/java/com/android/tools/r8/shaking/VerticalClassMerger.java +++ b/src/main/java/com/android/tools/r8/shaking/VerticalClassMerger.java
@@ -925,6 +925,12 @@ // Update the holder of [virtualMethod] using renameMethod(). DexEncodedMethod resultingVirtualMethod = renameMethod(virtualMethod, availableMethodSignatures, Rename.NEVER); + if (appView.options().canHaveDalvikAbstractMethodOnNonAbstractClassVerificationBug() + && !target.isAbstract()) { + resultingVirtualMethod.accessFlags.unsetAbstract(); + resultingVirtualMethod = + resultingVirtualMethod.toEmptyThrowingMethod(appView.options()); + } deferredRenamings.map(virtualMethod.method, resultingVirtualMethod.method); deferredRenamings.recordMove(virtualMethod.method, resultingVirtualMethod.method); add(virtualMethods, resultingVirtualMethod, MethodSignatureEquivalence.get());
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 5901ee9..951feef 100644 --- a/src/main/java/com/android/tools/r8/utils/InternalOptions.java +++ b/src/main/java/com/android/tools/r8/utils/InternalOptions.java
@@ -1106,4 +1106,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 3dfac08..148f1b1 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) @@ -1071,13 +1073,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/ir/optimize/membervaluepropagation/NoConstructorPropagationTest.java b/src/test/java/com/android/tools/r8/ir/optimize/membervaluepropagation/NoConstructorPropagationTest.java new file mode 100644 index 0000000..904342a --- /dev/null +++ b/src/test/java/com/android/tools/r8/ir/optimize/membervaluepropagation/NoConstructorPropagationTest.java
@@ -0,0 +1,62 @@ +// Copyright (c) 2019, the R8 project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +package com.android.tools.r8.ir.optimize.membervaluepropagation; + +import com.android.tools.r8.NeverClassInline; +import com.android.tools.r8.NeverInline; +import com.android.tools.r8.TestBase; +import com.android.tools.r8.TestParameters; +import com.android.tools.r8.TestParametersCollection; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; +import org.junit.runners.Parameterized.Parameters; + +@RunWith(Parameterized.class) +public class NoConstructorPropagationTest extends TestBase { + + private final TestParameters parameters; + + @Parameters(name = "{0}") + public static TestParametersCollection data() { + return getTestParameters().withAllRuntimes().build(); + } + + public NoConstructorPropagationTest(TestParameters parameters) { + this.parameters = parameters; + } + + @Test + public void test() throws Exception { + testForR8(parameters.getBackend()) + .addInnerClasses(NoConstructorPropagationTest.class) + .addKeepMainRule(TestClass.class) + .addKeepRules( + "-assumenosideeffects class " + Greeter.class.getTypeName() + " {", + " <init>(...);", + "}") + .enableClassInliningAnnotations() + .enableInliningAnnotations() + .setMinApi(parameters.getRuntime()) + .run(parameters.getRuntime(), TestClass.class) + .assertSuccessWithOutputLines("Hello world!"); + } + + static class TestClass { + + public static void main(String[] args) { + new Greeter().greet(); + } + } + + @NeverClassInline + static class Greeter { + + @NeverInline + public void greet() { + System.out.println("Hello world!"); + } + } +}
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/outliner/classtypes/B134462736.java b/src/test/java/com/android/tools/r8/ir/optimize/outliner/classtypes/B134462736.java new file mode 100644 index 0000000..177bc91 --- /dev/null +++ b/src/test/java/com/android/tools/r8/ir/optimize/outliner/classtypes/B134462736.java
@@ -0,0 +1,105 @@ +// Copyright (c) 2019, the R8 project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +package com.android.tools.r8.ir.optimize.outliner.classtypes; + +import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent; +import static org.hamcrest.MatcherAssert.assertThat; + +import com.android.tools.r8.NeverInline; +import com.android.tools.r8.TestBase; +import com.android.tools.r8.TestParameters; +import com.android.tools.r8.TestParametersCollection; +import com.android.tools.r8.utils.InternalOptions.OutlineOptions; +import com.android.tools.r8.utils.StringUtils; +import com.android.tools.r8.utils.codeinspector.ClassSubject; +import com.android.tools.r8.utils.codeinspector.CodeInspector; +import com.android.tools.r8.utils.codeinspector.CodeMatchers; +import com.android.tools.r8.utils.codeinspector.MethodSubject; +import com.google.common.collect.ImmutableList; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; + +@RunWith(Parameterized.class) +public class B134462736 extends TestBase { + + private final TestParameters parameters; + + @Parameterized.Parameters(name = "{0}") + public static TestParametersCollection data() { + return getTestParameters().withAllRuntimes().build(); + } + + public B134462736(TestParameters parameters) { + this.parameters = parameters; + } + + private void validateOutlining(CodeInspector inspector) { + ClassSubject outlineClass = inspector.clazz(OutlineOptions.CLASS_NAME); + MethodSubject outline0Method = + outlineClass.method( + "void", + "outline0", + ImmutableList.of( + StringBuilder.class.getTypeName(), + String.class.getTypeName(), + String.class.getTypeName(), + TestClass.class.getTypeName(), + String.class.getTypeName())); + assertThat(outline0Method, isPresent()); + ClassSubject classSubject = inspector.clazz(TestClass.class); + assertThat( + classSubject.uniqueMethodWithName("method1"), CodeMatchers.invokesMethod(outline0Method)); + assertThat( + classSubject.uniqueMethodWithName("method2"), CodeMatchers.invokesMethod(outline0Method)); + } + + @Test + public void test() throws Exception { + String expectedOutput = StringUtils.lines("12 null", "12 null"); + testForR8(parameters.getBackend()) + .enableInliningAnnotations() + .addInnerClasses(B134462736.class) + .addKeepMainRule(TestClass.class) + .setMinApi(parameters.getRuntime()) + .noMinification() + .addOptionsModification( + options -> { + options.outline.threshold = 2; + options.outline.minSize = 2; + }) + .compile() + .inspect(this::validateOutlining) + .run(parameters.getRuntime(), TestClass.class) + .assertSuccessWithOutput(expectedOutput); + } + + public static class TestClass { + @NeverInline + public void consumer(String arg1, String arg2) { + System.out.println(arg1 + " " + arg2); + } + + @NeverInline + public void method1(StringBuilder builder, String arg1, String arg2) { + builder.append(arg1); + builder.append(arg2); + consumer(builder.toString(), null); + } + + @NeverInline + public void method2(StringBuilder builder, String arg1, String arg2) { + builder.append(arg1); + builder.append(arg2); + consumer(builder.toString(), null); + } + + public static void main(String[] args) { + TestClass instance = new TestClass(); + instance.method1(new StringBuilder(), "1", "2"); + instance.method2(new StringBuilder(), "1", "2"); + } + } +} \ No newline at end of file
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()));