Avoid merging types in alwaysInline, noSideEffects, pinnedItems
Change-Id: I4ce5fd87d06c948e21be18d02ab5a65850b46cb6
diff --git a/src/main/java/com/android/tools/r8/graph/GraphLense.java b/src/main/java/com/android/tools/r8/graph/GraphLense.java
index 096de5a..2c12a90 100644
--- a/src/main/java/com/android/tools/r8/graph/GraphLense.java
+++ b/src/main/java/com/android/tools/r8/graph/GraphLense.java
@@ -99,6 +99,25 @@
return this instanceof IdentityGraphLense;
}
+ public boolean assertNotModified(Iterable<DexItem> items) {
+ for (DexItem item : items) {
+ if (item instanceof DexClass) {
+ DexType type = ((DexClass) item).type;
+ assert lookupType(type) == type;
+ } else if (item instanceof DexEncodedMethod) {
+ DexEncodedMethod method = (DexEncodedMethod) item;
+ // We allow changes to bridge methods as these get retargeted even if they are kept.
+ assert method.accessFlags.isBridge() || lookupMethod(method.method) == method.method;
+ } else if (item instanceof DexEncodedField) {
+ DexField field = ((DexEncodedField) item).field;
+ assert lookupField(field) == field;
+ } else {
+ assert false;
+ }
+ }
+ return true;
+ }
+
private static class IdentityGraphLense extends GraphLense {
@Override
diff --git a/src/main/java/com/android/tools/r8/shaking/Enqueuer.java b/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
index dae98ad..d42c68b 100644
--- a/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
+++ b/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
@@ -1659,18 +1659,18 @@
this.staticInvokes = rewriteMethodsConservatively(previous.staticInvokes, lense);
this.prunedTypes = rewriteItems(previous.prunedTypes, lense::lookupType);
// TODO(herhut): Migrate these to Descriptors, as well.
- assert assertNotModifiedByLense(previous.noSideEffects.keySet(), lense);
+ assert lense.assertNotModified(previous.noSideEffects.keySet());
this.noSideEffects = previous.noSideEffects;
- assert assertNotModifiedByLense(previous.assumedValues.keySet(), lense);
+ assert lense.assertNotModified(previous.assumedValues.keySet());
this.assumedValues = previous.assumedValues;
- assert assertNotModifiedByLense(previous.alwaysInline, lense);
+ assert lense.assertNotModified(previous.alwaysInline);
this.alwaysInline = previous.alwaysInline;
this.identifierNameStrings =
rewriteMixedItemsConservatively(previous.identifierNameStrings, lense);
// Switchmap classes should never be affected by renaming.
- assert assertNotModifiedByLense(
+ assert lense.assertNotModified(
previous.switchMaps.keySet().stream().map(this::definitionFor).filter(Objects::nonNull)
- .collect(Collectors.toList()), lense);
+ .collect(Collectors.toList()));
this.switchMaps = previous.switchMaps;
this.ordinalsMaps = rewriteKeys(previous.ordinalsMaps, lense::lookupType);
this.protoLiteFields = previous.protoLiteFields;
@@ -1735,27 +1735,6 @@
return true;
}
- private boolean assertNotModifiedByLense(Iterable<DexItem> items, GraphLense lense) {
- for (DexItem item : items) {
- if (item instanceof DexClass) {
- DexType type = ((DexClass) item).type;
- assert lense.lookupType(type) == type;
- } else if (item instanceof DexEncodedMethod) {
- DexEncodedMethod method = (DexEncodedMethod) item;
- // We only allow changes to bridge methods, as these get retargeted even if they
- // are kept.
- assert method.accessFlags.isBridge()
- || lense.lookupMethod(method.method) == method.method;
- } else if (item instanceof DexEncodedField) {
- DexField field = ((DexEncodedField) item).field;
- assert lense.lookupField(field) == field;
- } else {
- assert false;
- }
- }
- return true;
- }
-
private SortedSet<DexMethod> joinInvokedMethods(Map<DexType, Set<DexMethod>> invokes) {
return joinInvokedMethods(invokes, Function.identity());
}
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 a8fd776..0c770cc 100644
--- a/src/main/java/com/android/tools/r8/shaking/VerticalClassMerger.java
+++ b/src/main/java/com/android/tools/r8/shaking/VerticalClassMerger.java
@@ -96,8 +96,7 @@
extractPinnedItems(appInfo.pinnedItems, pinnedTypes);
// TODO(christofferqa): Remove the invariant that the graph lense should not modify any
- // methods from the sets alwaysInline and noSideEffects (see use of assertNotModifiedBy-
- // Lense).
+ // methods from the sets alwaysInline and noSideEffects (see use of assertNotModified).
extractPinnedItems(appInfo.alwaysInline, pinnedTypes);
extractPinnedItems(appInfo.noSideEffects.keySet(), pinnedTypes);
@@ -139,36 +138,41 @@
private void extractPinnedItems(Iterable<DexItem> items, Set<DexType> pinnedTypes) {
for (DexItem item : items) {
- // Note: Nothing to do for the case where item is a DexType, since we check for this case
- // using appInfo.isPinned.
- if (item instanceof DexField) {
- // Pin the type of the field.
- DexField field = (DexField) item;
- DexClass clazz = appInfo.definitionFor(field.type);
- if (clazz != null && clazz.isProgramClass()) {
- pinnedTypes.add(clazz.type);
+ if (item instanceof DexType || item instanceof DexClass) {
+ DexType type = item instanceof DexType ? (DexType) item : ((DexClass) item).type;
+ // We check for the case where the type is pinned according to appInfo.isPinned, so only
+ // add it here if it is not the case.
+ if (!appInfo.isPinned(type)) {
+ markTypeAsPinned(type, pinnedTypes);
}
- } else if (item instanceof DexMethod) {
- // Pin the return type and the parameter types of the method.
- DexMethod method = (DexMethod) item;
- DexClass clazz = appInfo.definitionFor(method.proto.returnType);
- if (clazz != null && clazz.isProgramClass()) {
- // If we were to merge [other] into its sub class, then we would implicitly change the
- // signature of this method, and therefore break the invariant.
- pinnedTypes.add(clazz.type);
- }
+ } else if (item instanceof DexField || item instanceof DexEncodedField) {
+ // Pin the holder and the type of the field.
+ DexField field =
+ item instanceof DexField ? (DexField) item : ((DexEncodedField) item).field;
+ markTypeAsPinned(field.clazz, pinnedTypes);
+ markTypeAsPinned(field.type, pinnedTypes);
+ } else if (item instanceof DexMethod || item instanceof DexEncodedMethod) {
+ // Pin the holder, the return type and the parameter types of the method. If we were to
+ // merge any of these types into their sub classes, then we would implicitly change the
+ // signature of this method.
+ DexMethod method =
+ item instanceof DexMethod ? (DexMethod) item : ((DexEncodedMethod) item).method;
+ markTypeAsPinned(method.holder, pinnedTypes);
+ markTypeAsPinned(method.proto.returnType, pinnedTypes);
for (DexType parameterType : method.proto.parameters.values) {
- clazz = appInfo.definitionFor(parameterType);
- if (clazz != null && clazz.isProgramClass()) {
- // If we were to merge [other] into its sub class, then we would implicitly change the
- // signature of this method, and therefore break the invariant.
- pinnedTypes.add(clazz.type);
- }
+ markTypeAsPinned(parameterType, pinnedTypes);
}
}
}
}
+ private void markTypeAsPinned(DexType type, Set<DexType> pinnedTypes) {
+ DexClass clazz = appInfo.definitionFor(type);
+ if (clazz != null && clazz.isProgramClass()) {
+ pinnedTypes.add(type);
+ }
+ }
+
private boolean isMergeCandidate(DexProgramClass clazz, Set<DexType> pinnedTypes) {
if (appInfo.instantiatedTypes.contains(clazz.type)
|| appInfo.isPinned(clazz.type)
@@ -269,6 +273,9 @@
timing.begin("fixup");
GraphLense result = new TreeFixer().fixupTypeReferences(mergingGraphLense);
timing.end();
+ assert result.assertNotModified(appInfo.alwaysInline);
+ assert result.assertNotModified(appInfo.noSideEffects.keySet());
+ assert result.assertNotModified(appInfo.pinnedItems);
return result;
}
