Merge "Update assert in vertical class merger"
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 1c74f79..247a5fe 100644
--- a/src/main/java/com/android/tools/r8/graph/GraphLense.java
+++ b/src/main/java/com/android/tools/r8/graph/GraphLense.java
@@ -424,14 +424,30 @@
return this == getIdentityLense();
}
- public <T extends DexDefinition> boolean assertDefinitionNotModified(Iterable<T> items) {
- for (DexDefinition item : items) {
- DexReference dexReference = item.toReference();
- DexReference lookupedReference = lookupReference(dexReference);
+ public <T extends DexDefinition> boolean assertDefinitionsNotModified(Iterable<T> definitions) {
+ for (DexDefinition definition : definitions) {
+ DexReference reference = definition.toReference();
// We allow changes to bridge methods as these get retargeted even if they are kept.
boolean isBridge =
- item.isDexEncodedMethod() && item.asDexEncodedMethod().accessFlags.isBridge();
- assert isBridge || dexReference == lookupedReference;
+ definition.isDexEncodedMethod() && definition.asDexEncodedMethod().accessFlags.isBridge();
+ assert isBridge || lookupReference(reference) == reference;
+ }
+ return true;
+ }
+
+ public <T extends DexReference> boolean assertReferencesNotModified(Iterable<T> references) {
+ for (DexReference reference : references) {
+ if (reference.isDexField()) {
+ DexField field = reference.asDexField();
+ assert getRenamedFieldSignature(field) == field;
+ } else if (reference.isDexMethod()) {
+ DexMethod method = reference.asDexMethod();
+ assert getRenamedMethodSignature(method) == method;
+ } else {
+ assert reference.isDexType();
+ DexType type = reference.asDexType();
+ assert lookupType(type) == type;
+ }
}
return true;
}
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 2ca33b0..6a90323 100644
--- a/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
+++ b/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
@@ -2111,26 +2111,32 @@
this.callSites = previous.callSites;
this.brokenSuperInvokes = lense.rewriteMethodsConservatively(previous.brokenSuperInvokes);
this.prunedTypes = rewriteItems(previous.prunedTypes, lense::lookupType);
- assert lense.assertDefinitionNotModified(previous.noSideEffects.keySet());
+ assert lense.assertDefinitionsNotModified(previous.noSideEffects.keySet());
this.noSideEffects = previous.noSideEffects;
- assert lense.assertDefinitionNotModified(previous.assumedValues.keySet());
+ assert lense.assertDefinitionsNotModified(previous.assumedValues.keySet());
this.assumedValues = previous.assumedValues;
- assert lense.assertDefinitionNotModified(
- previous.alwaysInline.stream().map(this::definitionFor).filter(Objects::nonNull)
+ assert lense.assertDefinitionsNotModified(
+ previous.alwaysInline.stream()
+ .map(this::definitionFor)
+ .filter(Objects::nonNull)
.collect(Collectors.toList()));
this.alwaysInline = previous.alwaysInline;
this.forceInline = lense.rewriteMethodsWithRenamedSignature(previous.forceInline);
this.neverInline = lense.rewriteMethodsWithRenamedSignature(previous.neverInline);
- assert lense.assertDefinitionNotModified(
- previous.neverMerge.stream().map(this::definitionFor).filter(Objects::nonNull)
+ assert lense.assertDefinitionsNotModified(
+ previous.neverMerge.stream()
+ .map(this::definitionFor)
+ .filter(Objects::nonNull)
.collect(Collectors.toList()));
this.neverClassInline = rewriteItems(previous.neverClassInline, lense::lookupType);
this.neverMerge = previous.neverMerge;
this.identifierNameStrings =
lense.rewriteReferencesConservatively(previous.identifierNameStrings);
// Switchmap classes should never be affected by renaming.
- assert lense.assertDefinitionNotModified(
- previous.switchMaps.keySet().stream().map(this::definitionFor).filter(Objects::nonNull)
+ assert lense.assertDefinitionsNotModified(
+ previous.switchMaps.keySet().stream()
+ .map(this::definitionFor)
+ .filter(Objects::nonNull)
.collect(Collectors.toList()));
this.switchMaps = previous.switchMaps;
this.ordinalsMaps = rewriteKeys(previous.ordinalsMaps, lense::lookupType);
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 0d69178..45ca1af 100644
--- a/src/main/java/com/android/tools/r8/shaking/VerticalClassMerger.java
+++ b/src/main/java/com/android/tools/r8/shaking/VerticalClassMerger.java
@@ -608,28 +608,67 @@
timing.begin("fixup");
GraphLense result = new TreeFixer().fixupTypeReferences(mergingGraphLense);
timing.end();
- assert result.assertDefinitionNotModified(
- appInfo
- .alwaysInline
- .stream()
+ assert result.assertDefinitionsNotModified(
+ appInfo.alwaysInline.stream()
.map(appInfo::definitionFor)
.filter(Objects::nonNull)
.collect(Collectors.toList()));
- assert result.assertDefinitionNotModified(appInfo.noSideEffects.keySet());
- // TODO(christofferqa): Enable this assert.
- // assert result.assertNotModified(appInfo.pinnedItems);
- assert verifyGraphLense(graphLense);
+ assert verifyGraphLense(result);
return result;
}
private boolean verifyGraphLense(GraphLense graphLense) {
+ assert graphLense.assertDefinitionsNotModified(appInfo.noSideEffects.keySet());
+
+ // Note that the method assertReferencesNotModified() relies on getRenamedFieldSignature() and
+ // getRenamedMethodSignature() instead of lookupField() and lookupMethod(). This is important
+ // for this check to succeed, since it is not guaranteed that calling lookupMethod() with a
+ // pinned method will return the method itself.
+ //
+ // Consider the following example.
+ //
+ // class A {
+ // public void method() {}
+ // }
+ // class B extends A {
+ // @Override
+ // public void method() {}
+ // }
+ // class C extends B {
+ // @Override
+ // public void method() {}
+ // }
+ //
+ // If A.method() is pinned, then A cannot be merged into B, but B can still be merged into C.
+ // Now, if there is an invoke-super instruction in C that hits B.method(), then this needs to
+ // be rewritten into an invoke-direct instruction. In particular, there could be an instruction
+ // `invoke-super A.method` in C. This would hit B.method(). Therefore, the graph lens records
+ // that `invoke-super A.method` instructions, which are in one of the methods from C, needs to
+ // be rewritten to `invoke-direct C.method$B`. This is valid even though A.method() is actually
+ // pinned, because this rewriting does not affect A.method() in any way.
+ assert graphLense.assertReferencesNotModified(appInfo.pinnedItems);
+
for (DexProgramClass clazz : appInfo.classes()) {
for (DexEncodedMethod encodedMethod : clazz.methods()) {
DexMethod method = encodedMethod.method;
DexMethod originalMethod = graphLense.getOriginalMethodSignature(method);
+ DexMethod renamedMethod = graphLense.getRenamedMethodSignature(originalMethod);
- // Must be able to map back.
- assert method == graphLense.getRenamedMethodSignature(originalMethod);
+ // Must be able to map back and forth.
+ if (encodedMethod.hasCode() && encodedMethod.getCode() instanceof SynthesizedBridgeCode) {
+ // For virtual methods, the vertical class merger creates two methods in the sub class
+ // in order to deal with invoke-super instructions (one that is private and one that is
+ // virtual). Therefore, it is not possible to go back and forth. Instead, we check that
+ // the two methods map back to the same original method, and that the original method
+ // can be mapped to the implementation method.
+ DexMethod implementationMethod =
+ ((SynthesizedBridgeCode) encodedMethod.getCode()).invocationTarget;
+ DexMethod originalImplementationMethod = graphLense.getOriginalMethodSignature(method);
+ assert originalMethod == originalImplementationMethod;
+ assert implementationMethod == renamedMethod;
+ } else {
+ assert method == renamedMethod;
+ }
// Verify that all types are up-to-date. After vertical class merging, there should be no
// more references to types that have been merged into another type.
diff --git a/src/main/java/com/android/tools/r8/shaking/VerticalClassMergerGraphLense.java b/src/main/java/com/android/tools/r8/shaking/VerticalClassMergerGraphLense.java
index 6b66465..20a05ab 100644
--- a/src/main/java/com/android/tools/r8/shaking/VerticalClassMergerGraphLense.java
+++ b/src/main/java/com/android/tools/r8/shaking/VerticalClassMergerGraphLense.java
@@ -222,16 +222,32 @@
Map<DexType, DexType> mergedClasses,
DexItemFactory dexItemFactory,
Map<DexProto, DexProto> cache) {
+ assert !signature.holder.isArrayType();
DexType newHolder = mergedClasses.getOrDefault(signature.holder, signature.holder);
DexProto newProto =
dexItemFactory.applyClassMappingToProto(
- signature.proto, type -> mergedClasses.getOrDefault(type, type), cache);
+ signature.proto,
+ type -> getTypeAfterClassMerging(type, mergedClasses, dexItemFactory),
+ cache);
if (signature.holder.equals(newHolder) && signature.proto.equals(newProto)) {
return signature;
}
return dexItemFactory.createMethod(newHolder, newProto, signature.name);
}
+ private static DexType getTypeAfterClassMerging(
+ DexType type, Map<DexType, DexType> mergedClasses, DexItemFactory dexItemFactory) {
+ if (type.isArrayType()) {
+ DexType baseType = type.toBaseType(dexItemFactory);
+ DexType newBaseType = mergedClasses.getOrDefault(baseType, baseType);
+ if (newBaseType != baseType) {
+ return type.replaceBaseType(newBaseType, dexItemFactory);
+ }
+ return type;
+ }
+ return mergedClasses.getOrDefault(type, type);
+ }
+
public boolean hasMappingForSignatureInContext(DexType context, DexMethod signature) {
Map<DexMethod, GraphLenseLookupResult> virtualToDirectMethodMap =
contextualVirtualToDirectMethodMaps.get(context);
diff --git a/src/test/java/com/android/tools/r8/kotlin/KotlinLambdaMergingTest.java b/src/test/java/com/android/tools/r8/kotlin/KotlinLambdaMergingTest.java
index c7a8ef2..b8c4b47 100644
--- a/src/test/java/com/android/tools/r8/kotlin/KotlinLambdaMergingTest.java
+++ b/src/test/java/com/android/tools/r8/kotlin/KotlinLambdaMergingTest.java
@@ -22,7 +22,6 @@
import java.util.List;
import java.util.concurrent.ExecutionException;
import java.util.function.Consumer;
-import org.junit.Ignore;
import org.junit.Test;
public class KotlinLambdaMergingTest extends AbstractR8KotlinTestBase {
@@ -455,7 +454,6 @@
});
}
- @Ignore("b/121107286: assertion failure in VerticalClassMerger#verifyGraphLense")
@Test
public void testSingleton() throws Exception {
final String mainClassName = "lambdas_singleton.MainKt";
