Keep track of failed field and method resolutions
This error only materializes when we use repackaging on classes that
has invalid resolution. But we have a lot of those tests.
Bug: 172254047
Bug: 173191855
Change-Id: Ibebffff6a5279df40b5506d1a79a04cae43d4e55
diff --git a/src/main/java/com/android/tools/r8/R8.java b/src/main/java/com/android/tools/r8/R8.java
index a1435fc..5e31835 100644
--- a/src/main/java/com/android/tools/r8/R8.java
+++ b/src/main/java/com/android/tools/r8/R8.java
@@ -813,7 +813,9 @@
lens, appBuilder.build(), memberRebindingLens.getPrevious());
}
}
- assert Repackaging.verifyIdentityRepackaging(appView);
+ if (appView.appInfo().hasLiveness()) {
+ assert Repackaging.verifyIdentityRepackaging(appView.withLiveness());
+ }
// Add automatic main dex classes to an eventual manual list of classes.
if (!options.mainDexKeepRules.isEmpty()) {
diff --git a/src/main/java/com/android/tools/r8/graph/GraphLens.java b/src/main/java/com/android/tools/r8/graph/GraphLens.java
index 2328408..ca9a57a 100644
--- a/src/main/java/com/android/tools/r8/graph/GraphLens.java
+++ b/src/main/java/com/android/tools/r8/graph/GraphLens.java
@@ -572,6 +572,14 @@
return builder.build();
}
+ public ImmutableSet<DexField> rewriteFields(Set<DexField> fields) {
+ ImmutableSet.Builder<DexField> builder = ImmutableSet.builder();
+ for (DexField field : fields) {
+ builder.add(getRenamedFieldSignature(field));
+ }
+ return builder.build();
+ }
+
public <T> ImmutableMap<DexField, T> rewriteFieldKeys(Map<DexField, T> map) {
ImmutableMap.Builder<DexField, T> builder = ImmutableMap.builder();
map.forEach((field, value) -> builder.put(getRenamedFieldSignature(field), value));
diff --git a/src/main/java/com/android/tools/r8/graph/TreeFixerBase.java b/src/main/java/com/android/tools/r8/graph/TreeFixerBase.java
index c1d5981..bfc685f 100644
--- a/src/main/java/com/android/tools/r8/graph/TreeFixerBase.java
+++ b/src/main/java/com/android/tools/r8/graph/TreeFixerBase.java
@@ -4,6 +4,7 @@
package com.android.tools.r8.graph;
+import com.android.tools.r8.shaking.AppInfoWithLiveness;
import java.util.ArrayList;
import java.util.Collection;
import java.util.IdentityHashMap;
@@ -12,14 +13,14 @@
public abstract class TreeFixerBase {
- private final AppView<?> appView;
+ private final AppView<AppInfoWithLiveness> appView;
private final DexItemFactory dexItemFactory;
private final Map<DexType, DexProgramClass> programClassCache = new IdentityHashMap<>();
private final Map<DexType, DexProgramClass> synthesizedFromClasses = new IdentityHashMap<>();
private final Map<DexProto, DexProto> protoFixupCache = new IdentityHashMap<>();
- public TreeFixerBase(AppView<?> appView) {
+ public TreeFixerBase(AppView<AppInfoWithLiveness> appView) {
this.appView = appView;
this.dexItemFactory = appView.dexItemFactory();
}
@@ -46,6 +47,32 @@
return to;
}
+ /** Rewrite missing references */
+ public void recordFailedResolutionChanges() {
+ // In order for optimizations to correctly rewrite field and method references that do not
+ // resolve, we create a mapping from each failed resolution target to its reference reference.
+ appView
+ .appInfo()
+ .getFailedFieldResolutionTargets()
+ .forEach(
+ field -> {
+ DexField fixedUpField = fixupFieldReference(field);
+ if (field != fixedUpField) {
+ recordFieldChange(field, fixedUpField);
+ }
+ });
+ appView
+ .appInfo()
+ .getFailedMethodResolutionTargets()
+ .forEach(
+ method -> {
+ DexMethod fixedUpMethod = fixupMethodReference(method);
+ if (method != fixedUpMethod) {
+ recordMethodChange(method, fixedUpMethod);
+ }
+ });
+ }
+
/** Callback to allow custom handling when an encoded method changes. */
public DexEncodedMethod recordMethodChange(DexEncodedMethod from, DexEncodedMethod to) {
recordMethodChange(from.method, to.method);
diff --git a/src/main/java/com/android/tools/r8/repackaging/Repackaging.java b/src/main/java/com/android/tools/r8/repackaging/Repackaging.java
index 51e419b..eba0a87 100644
--- a/src/main/java/com/android/tools/r8/repackaging/Repackaging.java
+++ b/src/main/java/com/android/tools/r8/repackaging/Repackaging.java
@@ -8,7 +8,6 @@
import static com.android.tools.r8.utils.DescriptorUtils.DESCRIPTOR_PACKAGE_SEPARATOR;
import static com.android.tools.r8.utils.DescriptorUtils.INNER_CLASS_SEPARATOR;
-import com.android.tools.r8.graph.AppInfoWithClassHierarchy;
import com.android.tools.r8.graph.AppView;
import com.android.tools.r8.graph.DexField;
import com.android.tools.r8.graph.DexItemFactory;
@@ -73,8 +72,7 @@
return lens;
}
- public static boolean verifyIdentityRepackaging(
- AppView<? extends AppInfoWithClassHierarchy> appView) {
+ public static boolean verifyIdentityRepackaging(AppView<AppInfoWithLiveness> appView) {
// Running the tree fixer with an identity mapping helps ensure that the fixup of of items is
// complete as the rewrite replaces all items regardless of repackaging.
// The identity mapping should result in no move callbacks being called.
@@ -140,10 +138,11 @@
return null;
}
RepackagingLens.Builder lensBuilder = new RepackagingLens.Builder();
+ RepackagingTreeFixer repackagingTreeFixer =
+ new RepackagingTreeFixer(appView, mappings, lensBuilder);
List<DexProgramClass> newProgramClasses =
new ArrayList<>(
- new RepackagingTreeFixer(appView, mappings, lensBuilder)
- .fixupClasses(appView.appInfo().classesWithDeterministicOrder()));
+ repackagingTreeFixer.fixupClasses(appView.appInfo().classesWithDeterministicOrder()));
appBuilder.replaceProgramClasses(newProgramClasses);
RepackagingLens lens = lensBuilder.build(appView);
new AnnotationFixer(lens).run(appBuilder.getProgramClasses());
@@ -156,10 +155,14 @@
private final Builder lensBuilder;
public RepackagingTreeFixer(
- AppView<?> appView, BiMap<DexType, DexType> mappings, Builder lensBuilder) {
+ AppView<AppInfoWithLiveness> appView,
+ BiMap<DexType, DexType> mappings,
+ Builder lensBuilder) {
super(appView);
+ assert mappings != null;
this.mappings = mappings;
this.lensBuilder = lensBuilder;
+ recordFailedResolutionChanges();
}
@Override
diff --git a/src/main/java/com/android/tools/r8/shaking/AppInfoWithLiveness.java b/src/main/java/com/android/tools/r8/shaking/AppInfoWithLiveness.java
index 744e0fc..363cb30 100644
--- a/src/main/java/com/android/tools/r8/shaking/AppInfoWithLiveness.java
+++ b/src/main/java/com/android/tools/r8/shaking/AppInfoWithLiveness.java
@@ -89,8 +89,11 @@
*/
private final Set<DexMethod> targetedMethods;
- /** Set of targets that lead to resolution errors, such as non-existing or invalid targets. */
- private final Set<DexMethod> failedResolutionTargets;
+ /** Method targets that lead to resolution errors such as non-existing or invalid targets. */
+ private final Set<DexMethod> failedMethodResolutionTargets;
+
+ /** Field targets that lead to resolution errors, such as non-existing or invalid targets. */
+ private final Set<DexField> failedFieldResolutionTargets;
/**
* Set of program methods that are used as the bootstrap method for an invoke-dynamic instruction.
@@ -197,7 +200,8 @@
MissingClasses missingClasses,
Set<DexType> liveTypes,
Set<DexMethod> targetedMethods,
- Set<DexMethod> failedResolutionTargets,
+ Set<DexMethod> failedMethodResolutionTargets,
+ Set<DexField> failedFieldResolutionTargets,
Set<DexMethod> bootstrapMethods,
Set<DexMethod> methodsTargetedByInvokeDynamic,
Set<DexMethod> virtualMethodsTargetedByInvokeDirect,
@@ -235,7 +239,8 @@
this.deadProtoTypes = deadProtoTypes;
this.liveTypes = liveTypes;
this.targetedMethods = targetedMethods;
- this.failedResolutionTargets = failedResolutionTargets;
+ this.failedMethodResolutionTargets = failedMethodResolutionTargets;
+ this.failedFieldResolutionTargets = failedFieldResolutionTargets;
this.bootstrapMethods = bootstrapMethods;
this.methodsTargetedByInvokeDynamic = methodsTargetedByInvokeDynamic;
this.virtualMethodsTargetedByInvokeDirect = virtualMethodsTargetedByInvokeDirect;
@@ -281,7 +286,8 @@
previous.getMissingClasses(),
CollectionUtils.mergeSets(previous.liveTypes, committedItems.getCommittedProgramTypes()),
previous.targetedMethods,
- previous.failedResolutionTargets,
+ previous.failedMethodResolutionTargets,
+ previous.failedFieldResolutionTargets,
previous.bootstrapMethods,
previous.methodsTargetedByInvokeDynamic,
previous.virtualMethodsTargetedByInvokeDirect,
@@ -328,7 +334,8 @@
? Sets.difference(previous.liveTypes, prunedItems.getRemovedClasses())
: previous.liveTypes,
previous.targetedMethods,
- previous.failedResolutionTargets,
+ previous.failedMethodResolutionTargets,
+ previous.failedFieldResolutionTargets,
previous.bootstrapMethods,
previous.methodsTargetedByInvokeDynamic,
previous.virtualMethodsTargetedByInvokeDirect,
@@ -420,7 +427,8 @@
this.deadProtoTypes = previous.deadProtoTypes;
this.liveTypes = previous.liveTypes;
this.targetedMethods = previous.targetedMethods;
- this.failedResolutionTargets = previous.failedResolutionTargets;
+ this.failedMethodResolutionTargets = previous.failedMethodResolutionTargets;
+ this.failedFieldResolutionTargets = previous.failedFieldResolutionTargets;
this.bootstrapMethods = previous.bootstrapMethods;
this.methodsTargetedByInvokeDynamic = previous.methodsTargetedByInvokeDynamic;
this.virtualMethodsTargetedByInvokeDirect = previous.virtualMethodsTargetedByInvokeDirect;
@@ -533,11 +541,15 @@
}
public boolean isFailedResolutionTarget(DexMethod method) {
- return failedResolutionTargets.contains(method);
+ return failedMethodResolutionTargets.contains(method);
}
- public Set<DexMethod> getFailedResolutionTargets() {
- return failedResolutionTargets;
+ public Set<DexMethod> getFailedMethodResolutionTargets() {
+ return failedMethodResolutionTargets;
+ }
+
+ public Set<DexField> getFailedFieldResolutionTargets() {
+ return failedFieldResolutionTargets;
}
public boolean isBootstrapMethod(DexMethod method) {
@@ -997,7 +1009,8 @@
getMissingClasses().commitSyntheticItems(committedItems),
lens.rewriteTypes(liveTypes),
lens.rewriteMethods(targetedMethods),
- lens.rewriteMethods(failedResolutionTargets),
+ lens.rewriteMethods(failedMethodResolutionTargets),
+ lens.rewriteFields(failedFieldResolutionTargets),
lens.rewriteMethods(bootstrapMethods),
lens.rewriteMethods(methodsTargetedByInvokeDynamic),
lens.rewriteMethods(virtualMethodsTargetedByInvokeDirect),
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 5ec2137..f7ac800 100644
--- a/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
+++ b/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
@@ -303,7 +303,10 @@
private final SetWithReason<DexEncodedMethod> targetedMethods;
/** Set of methods that have invalid resolutions or lookups. */
- private final Set<DexMethod> failedResolutionTargets;
+ private final Set<DexMethod> failedMethodResolutionTargets;
+
+ /** Set of methods that have invalid resolutions or lookups. */
+ private final Set<DexField> failedFieldResolutionTargets;
/**
* Set of program methods that are used as the bootstrap method for an invoke-dynamic instruction.
@@ -427,7 +430,8 @@
// This set is only populated in edge cases due to multiple default interface methods.
// The set is generally expected to be empty and in the unlikely chance it is not, it will
// likely contain two methods. Thus the default capacity of 2.
- failedResolutionTargets = SetUtils.newIdentityHashSet(2);
+ failedMethodResolutionTargets = SetUtils.newIdentityHashSet(2);
+ failedFieldResolutionTargets = SetUtils.newIdentityHashSet(0);
liveMethods = new LiveMethodsSet(graphReporter::registerMethod);
liveFields = new LiveFieldsSet(graphReporter::registerField);
lambdaRewriter = options.desugarState == DesugarState.ON ? new LambdaRewriter(appView) : null;
@@ -1943,6 +1947,7 @@
FieldResolutionResult resolutionResult = appInfo.resolveField(field);
if (resolutionResult.isFailedOrUnknownResolution()) {
reportMissingField(field);
+ failedFieldResolutionTargets.add(field);
}
return resolutionResult;
}
@@ -1954,7 +1959,8 @@
ResolutionResult resolutionResult = appInfo.unsafeResolveMethodDueToDexFormat(method);
if (resolutionResult.isFailedResolution()) {
reportMissingMethod(method);
- markFailedResolutionTargets(method, resolutionResult.asFailedResolution(), context, reason);
+ markFailedMethodResolutionTargets(
+ method, resolutionResult.asFailedResolution(), context, reason);
}
return resolutionResult.asSingleResolution();
}
@@ -1966,7 +1972,8 @@
ResolutionResult resolutionResult = appInfo.resolveMethod(method, interfaceInvoke);
if (resolutionResult.isFailedResolution()) {
reportMissingMethod(method);
- markFailedResolutionTargets(method, resolutionResult.asFailedResolution(), context, reason);
+ markFailedMethodResolutionTargets(
+ method, resolutionResult.asFailedResolution(), context, reason);
}
return resolutionResult.asSingleResolution();
}
@@ -2104,6 +2111,7 @@
// TODO(zerny): Is it ok that we lookup in both the direct and virtual pool here?
DexEncodedMethod encodedMethod = clazz.lookupMethod(reference);
if (encodedMethod == null) {
+ failedMethodResolutionTargets.add(reference);
reportMissingMethod(reference);
return;
}
@@ -2892,17 +2900,17 @@
}
}
- private void markFailedResolutionTargets(
+ private void markFailedMethodResolutionTargets(
DexMethod symbolicMethod,
FailedResolutionResult failedResolution,
ProgramDefinition context,
KeepReason reason) {
- failedResolutionTargets.add(symbolicMethod);
+ failedMethodResolutionTargets.add(symbolicMethod);
failedResolution.forEachFailureDependency(
method -> {
DexProgramClass clazz = getProgramClassOrNull(method.getHolderType(), context);
if (clazz != null) {
- failedResolutionTargets.add(method.method);
+ failedMethodResolutionTargets.add(method.method);
markMethodAsTargeted(new ProgramMethod(clazz, method), reason);
}
});
@@ -2948,7 +2956,7 @@
// If invoke target is invalid (inaccessible or not an instance-method) record it and stop.
DexClassAndMethod target = resolution.lookupInvokeSuperTarget(from.getHolder(), appInfo);
if (target == null) {
- failedResolutionTargets.add(resolution.getResolvedMethod().method);
+ failedMethodResolutionTargets.add(resolution.getResolvedMethod().method);
return;
}
@@ -3461,7 +3469,8 @@
: missingClassesBuilder.ignoreMissingClasses(),
SetUtils.mapIdentityHashSet(liveTypes.getItems(), DexProgramClass::getType),
Enqueuer.toDescriptorSet(targetedMethods.getItems()),
- Collections.unmodifiableSet(failedResolutionTargets),
+ Collections.unmodifiableSet(failedMethodResolutionTargets),
+ Collections.unmodifiableSet(failedFieldResolutionTargets),
Collections.unmodifiableSet(bootstrapMethods),
Collections.unmodifiableSet(methodsTargetedByInvokeDynamic),
Collections.unmodifiableSet(virtualMethodsTargetedByInvokeDirect),
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 e049766..c52fd0f 100644
--- a/src/main/java/com/android/tools/r8/shaking/VerticalClassMerger.java
+++ b/src/main/java/com/android/tools/r8/shaking/VerticalClassMerger.java
@@ -294,7 +294,7 @@
}
// The set of targets that must remain for proper resolution error cases should not be merged.
- for (DexMethod method : appInfo.getFailedResolutionTargets()) {
+ for (DexMethod method : appInfo.getFailedMethodResolutionTargets()) {
markTypeAsPinned(method.holder, AbortReason.RESOLUTION_FOR_METHODS_MAY_CHANGE);
}
}
diff --git a/src/test/java/com/android/tools/r8/resolution/VirtualOverrideOfPrivateStaticMethodWithVirtualParentTest.java b/src/test/java/com/android/tools/r8/resolution/VirtualOverrideOfPrivateStaticMethodWithVirtualParentTest.java
index 5da5b2d..1d6dc16 100644
--- a/src/test/java/com/android/tools/r8/resolution/VirtualOverrideOfPrivateStaticMethodWithVirtualParentTest.java
+++ b/src/test/java/com/android/tools/r8/resolution/VirtualOverrideOfPrivateStaticMethodWithVirtualParentTest.java
@@ -44,6 +44,7 @@
public static class B extends A implements I {}
public static class C extends B {
+ @Override
public void f() {
System.out.println("Called C.f");
}
