Version 1.2.46.
Merge: Workaround mediatek constructor inlining bug.
CL: https://r8-review.googlesource.com/26260
R=sgjesse@google.com
Bug: 68378480
Change-Id: I4c95310656da2262f48a9b356ac138551d84fa78
diff --git a/src/main/java/com/android/tools/r8/Version.java b/src/main/java/com/android/tools/r8/Version.java
index 0b00160..cc8472a 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.2.45";
+ public static final String LABEL = "1.2.46";
private Version() {
}
diff --git a/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java b/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java
index 1252d82..cc0a5f9 100644
--- a/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java
+++ b/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java
@@ -20,6 +20,7 @@
import com.android.tools.r8.graph.DexProgramClass;
import com.android.tools.r8.graph.DexString;
import com.android.tools.r8.graph.DexType;
+import com.android.tools.r8.graph.DexTypeList;
import com.android.tools.r8.graph.GraphLense;
import com.android.tools.r8.ir.analysis.constant.SparseConditionalConstantPropagation;
import com.android.tools.r8.ir.analysis.type.TypeAnalysis;
@@ -836,7 +837,8 @@
// Always perform dead code elimination before register allocation. The register allocator
// does not allow dead code (to make sure that we do not waste registers for unneeded values).
DeadCodeRemover.removeDeadCode(code, codeRewriter, graphLense, options);
- materializeInstructionBeforeLongOperationsWorkaround(code, options);
+ materializeInstructionBeforeLongOperationsWorkaround(code);
+ workaroundForwardingInitializerBug(code);
LinearScanRegisterAllocator registerAllocator = new LinearScanRegisterAllocator(code, options);
registerAllocator.allocateRegisters(options.debug);
printMethod(code, "After register allocation (non-SSA)");
@@ -852,8 +854,42 @@
return registerAllocator;
}
- private static void materializeInstructionBeforeLongOperationsWorkaround(
- IRCode code, InternalOptions options) {
+ private void workaroundForwardingInitializerBug(IRCode code) {
+ if (!options.canHaveForwardingInitInliningBug()) {
+ return;
+ }
+ // Only constructors.
+ if (!code.method.isInstanceInitializer()) {
+ return;
+ }
+ // Only constructors with certain signatures.
+ DexTypeList paramTypes = code.method.method.proto.parameters;
+ if (paramTypes.size() != 3 ||
+ paramTypes.values[0] != options.itemFactory.doubleType ||
+ paramTypes.values[1] != options.itemFactory.doubleType ||
+ !paramTypes.values[2].isClassType()) {
+ return;
+ }
+ // Only if the constructor contains a super constructor call taking only parameters as
+ // inputs.
+ for (BasicBlock block : code.blocks) {
+ InstructionListIterator it = block.listIterator();
+ Instruction superConstructorCall = it.nextUntil((i) ->
+ i.isInvokeDirect() &&
+ i.asInvokeDirect().getInvokedMethod().name == options.itemFactory.constructorMethodName &&
+ i.asInvokeDirect().arguments().size() == 4 &&
+ i.asInvokeDirect().arguments().stream().allMatch(Value::isArgument));
+ if (superConstructorCall != null) {
+ // We force a materializing const instruction in front of the super call to make
+ // sure that there is at least one temporary register in the method. That disables
+ // the inlining that is crashing on these devices.
+ ensureInstructionBefore(code, superConstructorCall, it);
+ break;
+ }
+ }
+ }
+
+ private void materializeInstructionBeforeLongOperationsWorkaround(IRCode code) {
if (!options.canHaveDex2OatLinkedListBug()) {
return;
}
@@ -862,25 +898,25 @@
Instruction firstMaterializing =
it.nextUntil(IRConverter::isMaterializingInstructionOnArtArmVersionM);
if (needsInstructionBeforeLongOperation(firstMaterializing)) {
- ensureInstructionBeforeLongOperation(code, block, firstMaterializing, it);
+ ensureInstructionBefore(code, firstMaterializing, it);
}
}
}
- private static void ensureInstructionBeforeLongOperation(
- IRCode code, BasicBlock block, Instruction firstMaterializing, InstructionListIterator it) {
+ private static void ensureInstructionBefore(
+ IRCode code, Instruction addBefore, InstructionListIterator it) {
// Force materialize a constant-zero before the long operation.
Instruction check = it.previous();
- assert firstMaterializing == check;
- Value fixitValue = code.createValue(ValueType.INT);
+ assert addBefore == check;
// Forced definition of const-zero
+ Value fixitValue = code.createValue(ValueType.INT);
Instruction fixitDefinition = new AlwaysMaterializingDefinition(fixitValue);
- fixitDefinition.setBlock(block);
- fixitDefinition.setPosition(firstMaterializing.getPosition());
+ fixitDefinition.setBlock(addBefore.getBlock());
+ fixitDefinition.setPosition(addBefore.getPosition());
it.add(fixitDefinition);
// Forced user of the forced definition to ensure it has a user and thus live range.
Instruction fixitUser = new AlwaysMaterializingUser(fixitValue);
- fixitUser.setBlock(block);
+ fixitUser.setBlock(addBefore.getBlock());
it.add(fixitUser);
}
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 1818f88..470f64a 100644
--- a/src/main/java/com/android/tools/r8/utils/InternalOptions.java
+++ b/src/main/java/com/android/tools/r8/utils/InternalOptions.java
@@ -645,4 +645,17 @@
public boolean canHaveNumberConversionRegisterAllocationBug() {
return minApiLevel < AndroidApiLevel.L.getLevel();
}
+
+ // Some Lollipop mediatek VMs have a peculiar bug where the inliner crashes if there is a
+ // simple constructor that just forwards its arguments to the super constructor. Strangely,
+ // this happens only for specific signatures: so far the only reproduction we have is for
+ // a constructor accepting two doubles and one object.
+ //
+ // To workaround this we insert a materializing const instruction before the super init
+ // call. Having a temporary register seems to disable the buggy optimizations.
+ //
+ // See b/68378480.
+ public boolean canHaveForwardingInitInliningBug() {
+ return minApiLevel < AndroidApiLevel.M.getLevel();
+ }
}
diff --git a/src/test/java/com/android/tools/r8/regress/b68378480/B68378480.java b/src/test/java/com/android/tools/r8/regress/b68378480/B68378480.java
new file mode 100644
index 0000000..044ddff
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/regress/b68378480/B68378480.java
@@ -0,0 +1,86 @@
+// Copyright (c) 2018 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.regress.b68378480;
+
+import static com.android.tools.r8.utils.DexInspectorMatchers.isPresent;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertThat;
+import static org.junit.Assert.assertTrue;
+
+import com.android.tools.r8.CompilationFailedException;
+import com.android.tools.r8.CompilationMode;
+import com.android.tools.r8.D8Command;
+import com.android.tools.r8.ToolHelper;
+import com.android.tools.r8.graph.DexCode;
+import com.android.tools.r8.ir.code.SingleConstant;
+import com.android.tools.r8.origin.Origin;
+import com.android.tools.r8.utils.AndroidApiLevel;
+import com.android.tools.r8.utils.AndroidApp;
+import com.android.tools.r8.utils.DexInspector;
+import com.android.tools.r8.utils.DexInspector.ClassSubject;
+import com.android.tools.r8.utils.DexInspector.MethodSubject;
+import com.google.common.collect.ImmutableList;
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.List;
+import java.util.concurrent.ExecutionException;
+import org.junit.Test;
+
+
+class SuperClass {
+ SuperClass(double a, double b, Object c) {
+ }
+}
+
+class SubClass extends SuperClass {
+ SubClass(double a, double b, Object c) {
+ super(a, b, c);
+ }
+}
+
+class Main {
+ public static void main(String[] args) {
+ new SubClass(1.1, 2.2, null);
+ }
+}
+
+public class B68378480 {
+
+ private DexCode compileClassesGetSubClassInit(int minApi)
+ throws IOException, CompilationFailedException, ExecutionException {
+ D8Command.Builder builder = D8Command.builder()
+ .setMode(CompilationMode.RELEASE)
+ .setMinApiLevel(minApi);
+ List<Class> classes = ImmutableList.of(SuperClass.class, SubClass.class, Main.class);
+ for (Class c : classes) {
+ builder.addClassProgramData(ToolHelper.getClassAsBytes(c), Origin.unknown());
+ }
+ AndroidApp app = ToolHelper.runD8(builder);
+ DexInspector inspector = new DexInspector(app);
+ ClassSubject clazz = inspector.clazz(SubClass.class);
+ assertThat(clazz, isPresent());
+ MethodSubject method =
+ clazz.method("void", "<init>", ImmutableList.of("double", "double", "java.lang.Object"));
+ assertThat(method, isPresent());
+ DexCode code = method.getMethod().getCode().asDexCode();
+ return code;
+ }
+
+ @Test
+ public void addExtraLocalToConstructor()
+ throws IOException, CompilationFailedException, ExecutionException {
+ DexCode code = compileClassesGetSubClassInit(AndroidApiLevel.L_MR1.getLevel());
+ assertTrue(code.registerSize > code.incomingRegisterSize);
+ assertTrue(Arrays.stream(code.instructions).anyMatch((i) -> i instanceof SingleConstant));
+ }
+
+ @Test
+ public void doNotAddExtraLocalToConstructor()
+ throws IOException, CompilationFailedException, ExecutionException {
+ DexCode code = compileClassesGetSubClassInit(AndroidApiLevel.M.getLevel());
+ assertEquals(code.registerSize, code.incomingRegisterSize);
+ assertTrue(Arrays.stream(code.instructions).noneMatch((i) -> i instanceof SingleConstant));
+ }
+}
