Fix enum unboxing bug causing VerifyError
Bug: b/446814391
Change-Id: Ifef42ef87d02b17be6b1e2c805b80521a8ee1032
diff --git a/src/main/java/com/android/tools/r8/graph/DexMethod.java b/src/main/java/com/android/tools/r8/graph/DexMethod.java
index 4abdb68..ff4f333 100644
--- a/src/main/java/com/android/tools/r8/graph/DexMethod.java
+++ b/src/main/java/com/android/tools/r8/graph/DexMethod.java
@@ -346,7 +346,10 @@
@Override
public DexMethod withHolder(DexReference reference, DexItemFactory dexItemFactory) {
- return dexItemFactory.createMethod(reference.getContextType(), proto, name);
+ DexType newHolder = reference.getContextType();
+ return newHolder.isIdenticalTo(holder)
+ ? this
+ : dexItemFactory.createMethod(newHolder, proto, name);
}
public DexMethod withName(String name, DexItemFactory dexItemFactory) {
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/enums/EnumUnboxerImpl.java b/src/main/java/com/android/tools/r8/ir/optimize/enums/EnumUnboxerImpl.java
index 3154d72..297ebe1 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/enums/EnumUnboxerImpl.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/enums/EnumUnboxerImpl.java
@@ -1407,6 +1407,9 @@
// Check if this is a checkNotNull() user. In this case, we can create a copy of the method
// that takes an int instead of java.lang.Object and call that method instead.
if (singleTarget != null) {
+ if (singleTarget.getAccessFlags().isAbstract()) {
+ return Reason.INVALID_INVOKE;
+ }
EnumUnboxerMethodClassification classification =
singleTarget.getOptimizationInfo().getEnumUnboxerMethodClassification();
if (classification.isCheckNotNullClassification()) {
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/enums/EnumUnboxingLens.java b/src/main/java/com/android/tools/r8/ir/optimize/enums/EnumUnboxingLens.java
index 5068f5a..3b7abb1 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/enums/EnumUnboxingLens.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/enums/EnumUnboxingLens.java
@@ -50,7 +50,6 @@
private final AbstractValueFactory abstractValueFactory;
private final Map<DexMethod, RewrittenPrototypeDescription> prototypeChangesPerMethod;
private final EnumDataMap unboxedEnums;
- private final Set<DexMethod> dispatchMethods;
EnumUnboxingLens(
AppView<?> appView,
@@ -58,14 +57,12 @@
BidirectionalOneToManyRepresentativeMap<DexMethod, DexMethod> renamedSignatures,
BidirectionalManyToOneRepresentativeMap<DexType, DexType> typeMap,
Map<DexMethod, DexMethod> methodMap,
- Map<DexMethod, RewrittenPrototypeDescription> prototypeChangesPerMethod,
- Set<DexMethod> dispatchMethods) {
+ Map<DexMethod, RewrittenPrototypeDescription> prototypeChangesPerMethod) {
super(appView, fieldMap, methodMap, typeMap, renamedSignatures);
assert !appView.unboxedEnums().isEmpty();
this.abstractValueFactory = appView.abstractValueFactory();
this.prototypeChangesPerMethod = prototypeChangesPerMethod;
this.unboxedEnums = appView.unboxedEnums();
- this.dispatchMethods = dispatchMethods;
}
@Override
@@ -108,15 +105,12 @@
public DexMethod lookupRefinedDispatchMethod(
DexMethod method,
- DexMethod context,
- InvokeType type,
- GraphLens codeLens,
AbstractValue unboxedEnumValue,
DexType enumType) {
- assert codeLens == getPrevious();
- DexMethod reference = lookupMethod(method, context, type, codeLens).getReference();
- if (!dispatchMethods.contains(reference) || !unboxedEnumValue.isSingleNumberValue()) {
- return null;
+ DexMethod enumMethod = method.withHolder(enumType, dexItemFactory());
+ DexMethod rewrittenEnumMethod = newMethodSignatures.getRepresentativeValue(enumMethod);
+ if (!unboxedEnumValue.isSingleNumberValue()) {
+ return rewrittenEnumMethod;
}
// We know the exact type of enum, so there is no need to go for the dispatch method. Instead,
// we compute the exact target from the enum instance.
@@ -126,11 +120,12 @@
.get(enumType)
.valuesTypes
.getOrDefault(unboxedIntToOrdinal(unboxedEnum), enumType);
+ if (instanceType.isIdenticalTo(enumType)) {
+ return rewrittenEnumMethod;
+ }
DexMethod specializedMethod = method.withHolder(instanceType, dexItemFactory());
- DexMethod superEnumMethod = method.withHolder(enumType, dexItemFactory());
DexMethod refined =
- newMethodSignatures.getRepresentativeValueOrDefault(
- specializedMethod, newMethodSignatures.getRepresentativeValue(superEnumMethod));
+ newMethodSignatures.getRepresentativeValueOrDefault(specializedMethod, rewrittenEnumMethod);
assert refined != null;
return refined;
}
@@ -408,8 +403,7 @@
originalCheckNotNullMethodSignature, checkNotNullMethod.getReference());
}
- public EnumUnboxingLens build(
- AppView<AppInfoWithLiveness> appView, Set<DexMethod> dispatchMethods) {
+ public EnumUnboxingLens build(AppView<AppInfoWithLiveness> appView) {
assert !typeMap.isEmpty();
return new EnumUnboxingLens(
appView,
@@ -417,8 +411,7 @@
newMethodSignatures,
typeMap,
methodMap,
- ImmutableMap.copyOf(prototypeChangesPerMethod),
- dispatchMethods);
+ ImmutableMap.copyOf(prototypeChangesPerMethod));
}
}
}
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/enums/EnumUnboxingRewriter.java b/src/main/java/com/android/tools/r8/ir/optimize/enums/EnumUnboxingRewriter.java
index 7cca071..0c5c644 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/enums/EnumUnboxingRewriter.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/enums/EnumUnboxingRewriter.java
@@ -200,6 +200,7 @@
rewriteInvokeMethodWithReceiver(
code,
eventConsumer,
+ affectedPhis,
convertedEnums,
blocks,
block,
@@ -438,6 +439,7 @@
private void rewriteInvokeMethodWithReceiver(
IRCode code,
EnumUnboxerMethodProcessorEventConsumer eventConsumer,
+ Set<Phi> affectedPhis,
Map<Instruction, DexType> convertedEnums,
BasicBlockIterator blocks,
BasicBlock block,
@@ -499,18 +501,23 @@
} else if (invoke.isInvokeVirtual() || invoke.isInvokeInterface()) {
DexMethod refinedDispatchMethodReference =
enumUnboxingLens.lookupRefinedDispatchMethod(
- invokedMethod,
- context.getReference(),
- invoke.getType(),
- enumUnboxingLens.getPrevious(),
- invoke.getArgument(0).getAbstractValue(appView, context),
- enumType);
+ invokedMethod, invoke.getReceiver().getAbstractValue(appView, context), enumType);
if (refinedDispatchMethodReference != null) {
DexClassAndMethod refinedDispatchMethod =
appView.definitionFor(refinedDispatchMethodReference);
assert refinedDispatchMethod != null;
assert refinedDispatchMethod.isProgramMethod();
- replaceEnumInvoke(iterator, invoke, refinedDispatchMethod.asProgramMethod());
+ InvokeStatic replacement =
+ replaceEnumInvoke(iterator, invoke, refinedDispatchMethod.asProgramMethod());
+ if (replacement.hasOutValue()
+ && refinedDispatchMethodReference.getReturnType().isIntType()
+ && !invokedMethod.getReturnType().isIntType()) {
+ Value rewrittenOutValue = code.createValue(TypeElement.getInt());
+ replacement.outValue().replaceUsers(rewrittenOutValue);
+ replacement.setOutValue(rewrittenOutValue);
+ affectedPhis.addAll(rewrittenOutValue.uniquePhiUsers());
+ convertedEnums.put(replacement, enumType);
+ }
}
}
} else if (invokedMethod == factory.stringBuilderMethods.appendObject
@@ -859,12 +866,12 @@
.ensureGetInstanceFieldMethod(appView, field, context, eventConsumer);
}
- private void replaceEnumInvoke(
+ private InvokeStatic replaceEnumInvoke(
InstructionListIterator iterator, InvokeMethod invoke, ProgramMethod method) {
- replaceEnumInvoke(iterator, invoke, method, invoke.arguments());
+ return replaceEnumInvoke(iterator, invoke, method, invoke.arguments());
}
- private void replaceEnumInvoke(
+ private InvokeStatic replaceEnumInvoke(
InstructionListIterator iterator,
InvokeMethod invoke,
ProgramMethod method,
@@ -877,6 +884,7 @@
assert !replacement.hasOutValue()
|| !replacement.getInvokedMethod().getReturnType().isVoidType();
iterator.replaceCurrentInstruction(replacement);
+ return replacement;
}
private boolean validateArrayAccess(ArrayAccess arrayAccess) {
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/enums/EnumUnboxingTreeFixer.java b/src/main/java/com/android/tools/r8/ir/optimize/enums/EnumUnboxingTreeFixer.java
index a6bda7d..53051d0 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/enums/EnumUnboxingTreeFixer.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/enums/EnumUnboxingTreeFixer.java
@@ -158,9 +158,7 @@
.fixupClassesConcurrentlyByConnectedProgramComponents(Timing.empty(), executorService);
// Install the new graph lens before processing any checkNotZero() methods.
- Set<DexMethod> dispatchMethodReferences = Sets.newIdentityHashSet();
- dispatchMethods.forEach((method, code) -> dispatchMethodReferences.add(method.getReference()));
- EnumUnboxingLens lens = lensBuilder.build(appView, dispatchMethodReferences);
+ EnumUnboxingLens lens = lensBuilder.build(appView);
appView.rewriteWithLens(lens, executorService, timing);
// Rewrite outliner with lens.
diff --git a/src/test/java/com/android/tools/r8/enumunboxing/InvokeInterfaceNoDevirtualizationEnumUnboxingTest.java b/src/test/java/com/android/tools/r8/enumunboxing/InvokeInterfaceNoDevirtualizationEnumUnboxingTest.java
index 0d04b20..3eb7953 100644
--- a/src/test/java/com/android/tools/r8/enumunboxing/InvokeInterfaceNoDevirtualizationEnumUnboxingTest.java
+++ b/src/test/java/com/android/tools/r8/enumunboxing/InvokeInterfaceNoDevirtualizationEnumUnboxingTest.java
@@ -3,10 +3,15 @@
// BSD-style license that can be found in the LICENSE file.
package com.android.tools.r8.enumunboxing;
+import static org.junit.Assume.assumeFalse;
+import static org.junit.Assume.assumeTrue;
+
import com.android.tools.r8.NeverInline;
+import com.android.tools.r8.NoAccessModification;
import com.android.tools.r8.NoVerticalClassMerging;
import com.android.tools.r8.TestParameters;
-import com.android.tools.r8.utils.codeinspector.AssertUtils;
+import com.google.common.collect.ImmutableList;
+import java.io.IOException;
import java.util.List;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -22,7 +27,7 @@
@Parameters(name = "{0} valueOpt: {1} keep: {2}")
public static List<Object[]> data() {
- return enumUnboxingTestParameters();
+ return enumUnboxingTestParameters(getTestParameters().withAllRuntimesAndApiLevels().build());
}
public InvokeInterfaceNoDevirtualizationEnumUnboxingTest(
@@ -33,43 +38,66 @@
}
@Test
- public void test() throws Exception {
- AssertUtils.assertFailsCompilation(
- () ->
- testForR8(parameters)
- .addInnerClasses(getClass())
- .addKeepMainRule(Main.class)
- .addKeepRules(enumKeepRules.getKeepRules())
- // Disable devirtualization so that the enum unboxing rewriter sees the call to
- // I#greet instead of MyEnum#greet.
- .addOptionsModification(options -> options.enableDevirtualization = false)
- .addOptionsModification(
- options -> enableEnumOptions(options, enumValueOptimization))
- .enableInliningAnnotations()
- .enableNoVerticalClassMergingAnnotations()
- .compile()
- .run(parameters.getRuntime(), Main.class)
- .assertSuccessWithOutputLines("Hello, world!"));
+ public void testJvm() throws Exception {
+ parameters.assumeJvmTestParameters();
+ assumeFalse(enumValueOptimization);
+ assumeTrue(enumKeepRules.isNone());
+ testForJvm(parameters)
+ .addProgramClasses(I.class)
+ .addProgramClassFileData(getProgramClassFileData())
+ .run(parameters.getRuntime(), Main.class)
+ .assertSuccessWithOutputLines("Hello, world!");
+ }
+
+ @Test
+ public void testR8() throws Exception {
+ testForR8(parameters)
+ .addProgramClasses(I.class)
+ .addProgramClassFileData(getProgramClassFileData())
+ .addKeepMainRule(Main.class)
+ .addKeepRules(enumKeepRules.getKeepRules())
+ .addOptionsModification(options -> enableEnumOptions(options, enumValueOptimization))
+ .enableInliningAnnotations()
+ .enableNoAccessModificationAnnotationsForClasses()
+ .enableNoVerticalClassMergingAnnotations()
+ .compile()
+ .run(parameters.getRuntime(), Main.class)
+ .assertSuccessWithOutputLines("Hello, world!");
+ }
+
+ private static List<byte[]> getProgramClassFileData() throws IOException {
+ return ImmutableList.of(
+ transformer(Main.class)
+ .replaceClassDescriptorInMethodInstructions(
+ descriptor(MyEnumAccessor.class), "LMyEnumAccessor;")
+ .transform(),
+ transformer(MyEnum.class).setClassDescriptor("LMyEnum;").transform(),
+ transformer(MyEnumAccessor.class)
+ .setClassDescriptor("LMyEnumAccessor;")
+ .replaceClassDescriptorInMethodInstructions(descriptor(MyEnum.class), "LMyEnum;")
+ .transform());
}
static class Main {
public static void main(String[] args) {
- MyEnum e = System.currentTimeMillis() > 0 ? MyEnum.A : MyEnum.B;
- greet(e);
+ greet(MyEnumAccessor.get());
}
static void greet(I i) {
+ // Cannot be rewritten to call MyEnum#greet since MyEnum is not public and in another package.
i.greet();
}
}
@NoVerticalClassMerging
- interface I {
+ public interface I {
void greet();
}
+ // Moved to separate package by transformer.
+ @NoAccessModification
enum MyEnum implements I {
A,
B;
@@ -80,4 +108,12 @@
System.out.println("Hello, world!");
}
}
+
+ // Moved to separate package by transformer.
+ public static class MyEnumAccessor {
+
+ public static I get() {
+ return System.currentTimeMillis() > 0 ? MyEnum.A : MyEnum.B;
+ }
+ }
}
diff --git a/src/test/java/com/android/tools/r8/enumunboxing/enummerging/AbstractMethodErrorEnumMergingTest.java b/src/test/java/com/android/tools/r8/enumunboxing/enummerging/AbstractMethodErrorEnumMergingTest.java
index 7937ef7..b11d6de 100644
--- a/src/test/java/com/android/tools/r8/enumunboxing/enummerging/AbstractMethodErrorEnumMergingTest.java
+++ b/src/test/java/com/android/tools/r8/enumunboxing/enummerging/AbstractMethodErrorEnumMergingTest.java
@@ -10,6 +10,7 @@
import com.android.tools.r8.TestParameters;
import com.android.tools.r8.enumunboxing.EnumUnboxingTestBase;
import com.android.tools.r8.utils.StringUtils;
+import com.android.tools.r8.utils.codeinspector.EnumUnboxingInspector;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.ArrayList;
@@ -49,25 +50,22 @@
@Test
public void testReference() throws Exception {
- testForD8(parameters.getBackend())
+ testForD8(parameters)
.addProgramClassFileData(inputProgram())
.addOptionsModification(opt -> enableEnumOptions(opt, enumValueOptimization))
- .setMinApi(parameters)
.run(parameters.getRuntime(), Main.class)
.assertSuccessWithOutput(EXPECTED_RESULT);
}
@Test
public void testEnumUnboxing() throws Exception {
- testForR8(parameters.getBackend())
+ testForR8(parameters)
.addProgramClassFileData(inputProgram())
.addKeepMainRule(Main.class)
.addKeepRules(enumKeepRules.getKeepRules())
- .addEnumUnboxingInspector(
- inspector -> inspector.assertUnboxed(MyEnum2Cases.class, MyEnum1Case.class))
+ .addEnumUnboxingInspector(EnumUnboxingInspector::assertNoEnumsUnboxed)
.enableInliningAnnotations()
.addOptionsModification(opt -> enableEnumOptions(opt, enumValueOptimization))
- .setMinApi(parameters)
.run(parameters.getRuntime(), Main.class)
.assertSuccessWithOutput(EXPECTED_RESULT);
}
