Fix dispatch for toString
Bug: b/271385332
Change-Id: I20a8be42185325707515b8eb281cd8a85ea6f5ef
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 63595f7..da8ac44 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
@@ -108,7 +108,7 @@
if (superEnum != previousContext.getHolderType()) {
DexMethod reference = previous.getReference();
if (reference.getHolderType() != superEnum) {
- // Rebind the reference to the superEnum if that is not the case.
+ // We are in an enum subtype where super-invokes are rebound differently.
reference = reference.withHolder(superEnum, dexItemFactory());
}
result = newMethodSignatures.getRepresentativeValue(reference);
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 94470b9..4aa7645 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
@@ -208,21 +208,31 @@
rewriteNameMethod(iterator, invoke, enumType, context, eventConsumer);
continue;
} else if (invokedMethod.match(factory.enumMembers.toString)) {
+ DexMethod reboundMethod =
+ invokedMethod.withHolder(unboxedEnumsData.representativeType(enumType), factory);
DexMethod lookupMethod =
enumUnboxingLens
.lookupMethod(
- invokedMethod,
+ reboundMethod,
context.getReference(),
invoke.getType(),
enumUnboxingLens.getPrevious())
.getReference();
- // If the lookupMethod is different, then a toString method was on the enumType
- // class, which was moved, and the lens code rewriter will rewrite the invoke to
- // that method.
- if (invoke.isInvokeSuper() || lookupMethod == invokedMethod) {
+ // If the SuperEnum had declared a toString() override, then the unboxer moves it to
+ // the local utility class method corresponding to that override.
+ // If a SubEnum had declared a toString() override, then the unboxer records a
+ // synthetic move from SuperEnum.toString() to the dispatch method on the local
+ // utility class.
+ // When they are the same, then there are no overrides of toString().
+ if (lookupMethod == reboundMethod) {
rewriteNameMethod(iterator, invoke, enumType, context, eventConsumer);
- continue;
+ } else {
+ DexClassAndMethod dexClassAndMethod = appView.definitionFor(lookupMethod);
+ assert dexClassAndMethod != null;
+ assert dexClassAndMethod.isProgramMethod();
+ replaceEnumInvoke(iterator, invoke, dexClassAndMethod.asProgramMethod());
}
+ continue;
} else if (invokedMethod == factory.objectMembers.getClass) {
rewriteNullCheck(iterator, invoke, context, eventConsumer);
continue;
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 92f6d70..8d04a32 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
@@ -10,12 +10,14 @@
import com.android.tools.r8.contexts.CompilationContext.ProcessorContext;
import com.android.tools.r8.graph.AppView;
import com.android.tools.r8.graph.DexClass;
+import com.android.tools.r8.graph.DexClassAndMethod;
import com.android.tools.r8.graph.DexEncodedField;
import com.android.tools.r8.graph.DexEncodedField.Builder;
import com.android.tools.r8.graph.DexEncodedMethod;
import com.android.tools.r8.graph.DexField;
import com.android.tools.r8.graph.DexItemFactory;
import com.android.tools.r8.graph.DexMethod;
+import com.android.tools.r8.graph.DexMethodSignature;
import com.android.tools.r8.graph.DexProgramClass;
import com.android.tools.r8.graph.DexProto;
import com.android.tools.r8.graph.DexType;
@@ -77,7 +79,6 @@
import java.util.Set;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.ExecutorService;
-import java.util.function.Consumer;
import java.util.function.Predicate;
class EnumUnboxingTreeFixer {
@@ -529,6 +530,25 @@
}));
}
+ private void processMethod(
+ ProgramMethod method,
+ PrunedItems.Builder prunedItemsBuilder,
+ DexMethodSignatureSet nonPrivateVirtualMethods,
+ LocalEnumUnboxingUtilityClass localUtilityClass,
+ Map<DexMethod, DexEncodedMethod> localUtilityMethods) {
+ if (method.getDefinition().isClassInitializer()
+ && enumDataMap.representativeType(method.getHolderType()) != method.getHolderType()) {
+ assert method.getDefinition().getCode().isEmptyVoidMethod();
+ prunedItemsBuilder.addRemovedMethod(method.getReference());
+ } else if (method.getDefinition().isInstanceInitializer()) {
+ prunedItemsBuilder.addRemovedMethod(method.getReference());
+ } else if (method.getDefinition().isNonPrivateVirtualMethod()) {
+ nonPrivateVirtualMethods.add(method.getReference());
+ } else {
+ directMoveAndMap(localUtilityClass, localUtilityMethods, method);
+ }
+ }
+
private Collection<DexEncodedMethod> createLocalUtilityMethods(
DexProgramClass unboxedEnum,
Set<DexProgramClass> subEnums,
@@ -541,77 +561,133 @@
localUtilityClass
.getDefinition()
.forEachMethod(method -> localUtilityMethods.put(method.getReference(), method));
- // First generate all methods from the super enums, for any override, generate the dispatch
- // method and both the move and super contextual move.
- DexMethodSignatureSet methodsWithOverride = DexMethodSignatureSet.create();
- forEachWhilePruningInstanceInitializers(
- unboxedEnum,
- prunedItemsBuilder,
- method -> {
- DexEncodedMethod superEnumUtilityMethod =
- installLocalUtilityMethod(localUtilityClass, localUtilityMethods, method);
- Map<DexMethod, DexMethod> overrideToUtilityMethods = new IdentityHashMap<>();
- if (method.getDefinition().isNonPrivateVirtualMethod()) {
- for (DexProgramClass subEnum : subEnums) {
- ProgramMethod override = subEnum.lookupProgramMethod(method.getReference());
- if (override != null) {
- methodsWithOverride.add(method);
- DexEncodedMethod subEnumLocalUtilityMethod =
- installLocalUtilityMethod(localUtilityClass, localUtilityMethods, override);
- overrideToUtilityMethods.put(
- override.getReference(), subEnumLocalUtilityMethod.getReference());
- }
- }
- }
- if (overrideToUtilityMethods.isEmpty()) {
- lensBuilder.moveAndMap(
- method.getReference(),
- superEnumUtilityMethod.getReference(),
- method.getDefinition().isStatic());
- } else {
- DexMethod dispatch =
- installDispatchMethod(
- localUtilityClass,
- localUtilityMethods,
- method,
- superEnumUtilityMethod,
- overrideToUtilityMethods)
- .getReference();
- recordEmulatedDispatch(
- method.getReference(), superEnumUtilityMethod.getReference(), dispatch);
- for (DexMethod override : overrideToUtilityMethods.keySet()) {
- recordEmulatedDispatch(override, overrideToUtilityMethods.get(override), dispatch);
- }
- }
- });
- // TODO(b/271385332): Check dispatch methods without implementation in the superEnum.
+
+ // First generate all methods but the ones requiring emulated dispatch.
+ DexMethodSignatureSet nonPrivateVirtualMethods = DexMethodSignatureSet.create();
+ unboxedEnum.forEachProgramMethod(
+ method ->
+ processMethod(
+ method,
+ prunedItemsBuilder,
+ nonPrivateVirtualMethods,
+ localUtilityClass,
+ localUtilityMethods));
// Second for each subEnum generate the remaining methods if not already generated.
for (DexProgramClass subEnum : subEnums) {
- forEachWhilePruningInstanceInitializers(
- subEnum,
- prunedItemsBuilder,
- method -> {
- if (method.getDefinition().isClassInitializer()) {
- assert method.getDefinition().getCode().isEmptyVoidMethod();
- prunedItemsBuilder.addRemovedMethod(method.getReference());
- return;
- }
- if (!methodsWithOverride.contains(method.getReference())) {
- DexEncodedMethod newLocalUtilityMethod =
- installLocalUtilityMethod(localUtilityClass, localUtilityMethods, method);
- // We cannot remap the subtype <clinit> to the same method than the superEnum one,
- // so we have to extend the design to support subtype <clinit> here.
- assert !method.getDefinition().isClassInitializer();
- lensBuilder.moveAndMap(
- method.getReference(),
- newLocalUtilityMethod.getReference(),
- method.getDefinition().isStatic());
- }
- });
+ subEnum.forEachProgramMethod(
+ method ->
+ processMethod(
+ method,
+ prunedItemsBuilder,
+ nonPrivateVirtualMethods,
+ localUtilityClass,
+ localUtilityMethods));
}
+
+ // Then analyze each method that may require emulated dispatch.
+ for (DexMethodSignature nonPrivateVirtualMethod : nonPrivateVirtualMethods) {
+ processVirtualMethod(
+ nonPrivateVirtualMethod, unboxedEnum, subEnums, localUtilityClass, localUtilityMethods);
+ }
+
return localUtilityMethods.values();
}
+ private void processVirtualMethod(
+ DexMethodSignature nonPrivateVirtualMethod,
+ DexProgramClass unboxedEnum,
+ Set<DexProgramClass> subEnums,
+ LocalEnumUnboxingUtilityClass localUtilityClass,
+ Map<DexMethod, DexEncodedMethod> localUtilityMethods) {
+ // Emulated dispatch is required if there is a "super method" in the superEnum or above,
+ // and at least one override.
+ DexMethod reference = nonPrivateVirtualMethod.withHolder(unboxedEnum.getType(), factory);
+ ProgramMethodSet subimplementations = ProgramMethodSet.create();
+ for (DexProgramClass subEnum : subEnums) {
+ ProgramMethod subMethod = subEnum.lookupProgramMethod(reference);
+ if (subMethod != null) {
+ subimplementations.add(subMethod);
+ }
+ }
+ DexClassAndMethod superMethod = unboxedEnum.lookupProgramMethod(reference);
+ if (superMethod == null) {
+ assert !subimplementations.isEmpty();
+ superMethod = appView.appInfo().lookupSuperTarget(reference, unboxedEnum, appView);
+ assert superMethod == null || superMethod.getReference() == factory.enumMembers.toString;
+ }
+ if (superMethod == null || subimplementations.isEmpty()) {
+ // No emulated dispatch is required, just move everything.
+ if (superMethod != null) {
+ assert superMethod.isProgramMethod();
+ directMoveAndMap(localUtilityClass, localUtilityMethods, superMethod.asProgramMethod());
+ }
+ for (ProgramMethod override : subimplementations) {
+ directMoveAndMap(localUtilityClass, localUtilityMethods, override);
+ }
+ return;
+ }
+ // These methods require emulated dispatch.
+ emulatedDispatchMoveAndMap(
+ localUtilityClass, localUtilityMethods, superMethod, subimplementations);
+ }
+
+ private void emulatedDispatchMoveAndMap(
+ LocalEnumUnboxingUtilityClass localUtilityClass,
+ Map<DexMethod, DexEncodedMethod> localUtilityMethods,
+ DexClassAndMethod superMethod,
+ ProgramMethodSet subimplementations) {
+ assert !subimplementations.isEmpty();
+ DexMethod superUtilityMethod;
+ if (superMethod.isProgramMethod()) {
+ superUtilityMethod =
+ installLocalUtilityMethod(
+ localUtilityClass, localUtilityMethods, superMethod.asProgramMethod())
+ .getReference();
+ } else {
+ // All methods but toString() are final or non-virtual.
+ // We could support other cases by setting correctly the superUtilityMethod here.
+ assert superMethod.getReference() == factory.enumMembers.toString;
+ superUtilityMethod = localUtilityClass.computeToStringUtilityMethod(factory);
+ }
+ Map<DexMethod, DexMethod> overrideToUtilityMethods = new IdentityHashMap<>();
+ for (ProgramMethod subMethod : subimplementations) {
+ DexEncodedMethod subEnumLocalUtilityMethod =
+ installLocalUtilityMethod(localUtilityClass, localUtilityMethods, subMethod);
+ overrideToUtilityMethods.put(
+ subMethod.getReference(), subEnumLocalUtilityMethod.getReference());
+ }
+ DexMethod dispatch =
+ installDispatchMethod(
+ localUtilityClass,
+ localUtilityMethods,
+ subimplementations.iterator().next(),
+ superUtilityMethod,
+ overrideToUtilityMethods)
+ .getReference();
+ if (superMethod.isProgramMethod()) {
+ recordEmulatedDispatch(superMethod.getReference(), superUtilityMethod, dispatch);
+ } else {
+ lensBuilder.mapToDispatch(
+ superMethod
+ .getReference()
+ .withHolder(localUtilityClass.getSynthesizingContext().getType(), factory),
+ dispatch);
+ }
+ for (DexMethod override : overrideToUtilityMethods.keySet()) {
+ recordEmulatedDispatch(override, overrideToUtilityMethods.get(override), dispatch);
+ }
+ }
+
+ private void directMoveAndMap(
+ LocalEnumUnboxingUtilityClass localUtilityClass,
+ Map<DexMethod, DexEncodedMethod> localUtilityMethods,
+ ProgramMethod method) {
+ DexEncodedMethod utilityMethod =
+ installLocalUtilityMethod(localUtilityClass, localUtilityMethods, method);
+ lensBuilder.moveAndMap(
+ method.getReference(), utilityMethod.getReference(), method.getDefinition().isStatic());
+ }
+
public void recordEmulatedDispatch(DexMethod from, DexMethod move, DexMethod dispatch) {
// Move is used for getRenamedSignature and to remap invoke-super.
// Map is used to remap all the other invokes.
@@ -622,14 +698,14 @@
private DexEncodedMethod installDispatchMethod(
LocalEnumUnboxingUtilityClass localUtilityClass,
Map<DexMethod, DexEncodedMethod> localUtilityMethods,
- ProgramMethod method,
- DexEncodedMethod superEnumUtilityMethod,
+ ProgramMethod representative,
+ DexMethod superUtilityMethod,
Map<DexMethod, DexMethod> map) {
assert !map.isEmpty();
DexMethod newLocalUtilityMethodReference =
factory.createFreshMethodNameWithoutHolder(
- "_dispatch_" + method.getName().toString(),
- fixupProto(factory.prependHolderToProto(method.getReference())),
+ "_dispatch_" + representative.getName().toString(),
+ fixupProto(factory.prependHolderToProto(representative.getReference())),
localUtilityClass.getType(),
newMethodSignature -> !localUtilityMethods.containsKey(newMethodSignature));
Int2ObjectMap<DexMethod> methodMap = new Int2ObjectArrayMap<>();
@@ -637,7 +713,7 @@
map.forEach(
(methodReference, newMethodReference) ->
typeToMethod.put(methodReference.getHolderType(), newMethodReference));
- DexProgramClass unboxedEnum = method.getHolder();
+ DexProgramClass unboxedEnum = localUtilityClass.getSynthesizingContext();
assert enumDataMap.get(unboxedEnum).valuesTypes != null;
enumDataMap
.get(unboxedEnum)
@@ -650,10 +726,7 @@
});
CfCodeWithLens codeWithLens =
new EnumUnboxingMethodDispatchCfCodeProvider(
- appView,
- localUtilityClass.getType(),
- superEnumUtilityMethod.getReference(),
- methodMap)
+ appView, localUtilityClass.getType(), superUtilityMethod, methodMap)
.generateCfCode();
DexEncodedMethod newLocalUtilityMethod =
DexEncodedMethod.builder()
@@ -661,8 +734,8 @@
.setAccessFlags(MethodAccessFlags.createPublicStaticSynthetic())
.setCode(codeWithLens)
.setClassFileVersion(unboxedEnum.getInitialClassFileVersion())
- .setApiLevelForDefinition(superEnumUtilityMethod.getApiLevelForDefinition())
- .setApiLevelForCode(superEnumUtilityMethod.getApiLevelForCode())
+ .setApiLevelForDefinition(representative.getDefinition().getApiLevelForDefinition())
+ .setApiLevelForCode(representative.getDefinition().getApiLevelForCode())
.build();
dispatchMethods.put(
newLocalUtilityMethod.asProgramMethod(localUtilityClass.getDefinition()), codeWithLens);
@@ -685,20 +758,6 @@
return newLocalUtilityMethod;
}
- private void forEachWhilePruningInstanceInitializers(
- DexProgramClass clazz,
- PrunedItems.Builder prunedItemsBuilder,
- Consumer<? super ProgramMethod> consumer) {
- clazz.forEachProgramMethod(
- method -> {
- if (method.getDefinition().isInstanceInitializer()) {
- prunedItemsBuilder.addRemovedMethod(method.getReference());
- } else {
- consumer.accept(method);
- }
- });
- }
-
private DexEncodedMethod createLocalUtilityMethod(
ProgramMethod method,
LocalEnumUnboxingUtilityClass localUtilityClass,
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/enums/LocalEnumUnboxingUtilityClass.java b/src/main/java/com/android/tools/r8/ir/optimize/enums/LocalEnumUnboxingUtilityClass.java
index 4e7e556..a7fdb84 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/enums/LocalEnumUnboxingUtilityClass.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/enums/LocalEnumUnboxingUtilityClass.java
@@ -11,6 +11,7 @@
import com.android.tools.r8.graph.ClassAccessFlags;
import com.android.tools.r8.graph.DexField;
import com.android.tools.r8.graph.DexItemFactory;
+import com.android.tools.r8.graph.DexMethod;
import com.android.tools.r8.graph.DexProgramClass;
import com.android.tools.r8.graph.DexProto;
import com.android.tools.r8.graph.DexString;
@@ -67,24 +68,35 @@
return method;
}
+ private DexString computeGetInstanceFieldMethodName(DexField field, DexItemFactory factory) {
+ String fieldName = field.getName().toString();
+ if (field.getHolderType() == getSynthesizingContext().getType()) {
+ return factory.createString(
+ "get" + fieldName.substring(0, 1).toUpperCase() + fieldName.substring(1));
+ }
+ assert field == factory.enumMembers.nameField || field == factory.enumMembers.ordinalField;
+ return field.getName();
+ }
+
+ private DexProto computeGetInstanceFieldMethodProto(DexField field, DexItemFactory factory) {
+ return factory.createProto(field.getType(), factory.intType);
+ }
+
+ public DexMethod computeToStringUtilityMethod(DexItemFactory factory) {
+ DexField nameField = factory.enumMembers.nameField;
+ DexString name = computeGetInstanceFieldMethodName(nameField, factory);
+ DexProto proto = computeGetInstanceFieldMethodProto(nameField, factory);
+ return factory.createMethod(getDefinition().getType(), proto, name);
+ }
+
private ProgramMethod ensureGetInstanceFieldMethod(
AppView<AppInfoWithLiveness> appView, DexField field) {
DexItemFactory dexItemFactory = appView.dexItemFactory();
- String fieldName = field.getName().toString();
- DexString methodName;
- if (field.getHolderType() == getSynthesizingContext().getType()) {
- methodName =
- dexItemFactory.createString(
- "get" + fieldName.substring(0, 1).toUpperCase() + fieldName.substring(1));
- } else {
- assert field == appView.dexItemFactory().enumMembers.nameField
- || field == appView.dexItemFactory().enumMembers.ordinalField;
- methodName = field.getName();
- }
+ DexString methodName = computeGetInstanceFieldMethodName(field, dexItemFactory);
return internalEnsureMethod(
appView,
methodName,
- dexItemFactory.createProto(field.getType(), dexItemFactory.intType),
+ computeGetInstanceFieldMethodProto(field, dexItemFactory),
method ->
new EnumUnboxingCfCodeProvider.EnumUnboxingInstanceFieldCfCodeProvider(
appView, getType(), data, field)
diff --git a/src/test/java/com/android/tools/r8/enumunboxing/enummerging/SuperToStringEnumMergingTest.java b/src/test/java/com/android/tools/r8/enumunboxing/enummerging/SuperToStringEnumMergingTest.java
new file mode 100644
index 0000000..a88db9a
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/enumunboxing/enummerging/SuperToStringEnumMergingTest.java
@@ -0,0 +1,109 @@
+// Copyright (c) 2023, 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.enumunboxing.enummerging;
+
+import com.android.tools.r8.NeverInline;
+import com.android.tools.r8.TestParameters;
+import com.android.tools.r8.enumunboxing.EnumUnboxingTestBase;
+import com.android.tools.r8.utils.StringUtils;
+import java.util.List;
+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 SuperToStringEnumMergingTest extends EnumUnboxingTestBase {
+
+ private static final String EXPECTED_RESULT =
+ StringUtils.lines("> A", "B", "! C", "> - A", "- B", "! - C");
+
+ private final TestParameters parameters;
+ private final boolean enumValueOptimization;
+ private final EnumKeepRules enumKeepRules;
+
+ @Parameters(name = "{0} valueOpt: {1} keep: {2}")
+ public static List<Object[]> data() {
+ return enumUnboxingTestParameters();
+ }
+
+ public SuperToStringEnumMergingTest(
+ TestParameters parameters, boolean enumValueOptimization, EnumKeepRules enumKeepRules) {
+ this.parameters = parameters;
+ this.enumValueOptimization = enumValueOptimization;
+ this.enumKeepRules = enumKeepRules;
+ }
+
+ @Test
+ public void testEnumUnboxing() throws Exception {
+ testForR8(parameters.getBackend())
+ .addInnerClasses(SuperToStringEnumMergingTest.class)
+ .addKeepMainRule(Main.class)
+ .addKeepRules(enumKeepRules.getKeepRules())
+ .addOptionsModification(opt -> opt.testing.enableEnumWithSubtypesUnboxing = true)
+ .addEnumUnboxingInspector(
+ inspector -> inspector.assertUnboxed(EnumToString.class, EnumToStringOverride.class))
+ .enableInliningAnnotations()
+ .addOptionsModification(opt -> enableEnumOptions(opt, enumValueOptimization))
+ .addOptionsModification(opt -> opt.testing.enableEnumUnboxingDebugLogs = true)
+ .setMinApi(parameters)
+ .allowDiagnosticInfoMessages()
+ .run(parameters.getRuntime(), Main.class)
+ .assertSuccessWithOutput(EXPECTED_RESULT);
+ }
+
+ enum EnumToString {
+ A {
+ @NeverInline
+ @Override
+ public String toString() {
+ return "> " + super.toString();
+ }
+ },
+ B,
+ C {
+ @NeverInline
+ @Override
+ public String toString() {
+ return "! " + super.toString();
+ }
+ };
+ }
+
+ enum EnumToStringOverride {
+ A {
+ @NeverInline
+ @Override
+ public String toString() {
+ return "> " + super.toString();
+ }
+ },
+ B,
+ C {
+ @NeverInline
+ @Override
+ public String toString() {
+ return "! " + super.toString();
+ }
+ };
+
+ @NeverInline
+ @Override
+ public String toString() {
+ return "- " + super.toString();
+ }
+ }
+
+ static class Main {
+
+ public static void main(String[] args) {
+ System.out.println(EnumToString.A.toString());
+ System.out.println(EnumToString.B.toString());
+ System.out.println(EnumToString.C.toString());
+ System.out.println(EnumToStringOverride.A.toString());
+ System.out.println(EnumToStringOverride.B.toString());
+ System.out.println(EnumToStringOverride.C.toString());
+ }
+ }
+}
