Mark widening overriding methods as targeted if also on interface
We have to ensure not to remove methods that are widening access and
also defined on interface. This CL extends the Enqueuer to mark such
methods as targeted. This is done by building up the scoped method set
according to the type hierarchy and for each widening function check
if it is also defined on one of the interfaces the class implements.
There are 13 such instances widening methods in GMS-core and all of
them was not removed before thus this CL will have very little
negative size impact.
One could do without the actual check on the interfaces to save some
runtime performance. On GMScore there is around 37 K of methods that
will now be targeted and the size increase is 2600 bytes.
Bug: 136698023
Change-Id: Ic2e0176904c109b4eb20b662f29f67fac088f9f9
diff --git a/src/main/java/com/android/tools/r8/graph/AccessFlags.java b/src/main/java/com/android/tools/r8/graph/AccessFlags.java
index a0322a1..64135fd 100644
--- a/src/main/java/com/android/tools/r8/graph/AccessFlags.java
+++ b/src/main/java/com/android/tools/r8/graph/AccessFlags.java
@@ -101,6 +101,10 @@
return visibilityOrdinal() >= other.visibilityOrdinal();
}
+ public boolean isSameVisiblity(AccessFlags other) {
+ return visibilityOrdinal() == other.visibilityOrdinal();
+ }
+
private int visibilityOrdinal() {
// public > protected > package > private
if (isPublic()) {
diff --git a/src/main/java/com/android/tools/r8/graph/AppInfoWithSubtyping.java b/src/main/java/com/android/tools/r8/graph/AppInfoWithSubtyping.java
index 6122236..1d92188 100644
--- a/src/main/java/com/android/tools/r8/graph/AppInfoWithSubtyping.java
+++ b/src/main/java/com/android/tools/r8/graph/AppInfoWithSubtyping.java
@@ -323,6 +323,39 @@
return true; // Don't know, there might be.
}
+ public boolean methodDefinedInInterfaces(DexEncodedMethod method, DexType implementingClass) {
+ DexClass holder = definitionFor(implementingClass);
+ if (holder == null) {
+ return false;
+ }
+ for (DexType iface : holder.interfaces.values) {
+ if (methodDefinedInInterface(method, iface)) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ public boolean methodDefinedInInterface(DexEncodedMethod method, DexType iface) {
+ DexClass potentialHolder = definitionFor(iface);
+ if (potentialHolder == null) {
+ return false;
+ }
+ assert potentialHolder.isInterface();
+ for (DexEncodedMethod virtualMethod : potentialHolder.virtualMethods) {
+ if (virtualMethod.method.hasSameProtoAndName(method.method)
+ && virtualMethod.accessFlags.isSameVisiblity(method.accessFlags)) {
+ return true;
+ }
+ }
+ for (DexType parentInterface : potentialHolder.interfaces.values) {
+ if (methodDefinedInInterface(method, parentInterface)) {
+ return true;
+ }
+ }
+ return false;
+ }
+
// For mapping invoke interface instruction to target methods.
public Set<DexEncodedMethod> lookupInterfaceTargets(DexMethod method) {
assert checkIfObsolete();
diff --git a/src/main/java/com/android/tools/r8/shaking/AbstractMethodRemover.java b/src/main/java/com/android/tools/r8/shaking/AbstractMethodRemover.java
index cf06ea6..efeaff7 100644
--- a/src/main/java/com/android/tools/r8/shaking/AbstractMethodRemover.java
+++ b/src/main/java/com/android/tools/r8/shaking/AbstractMethodRemover.java
@@ -7,6 +7,7 @@
import com.android.tools.r8.graph.DexEncodedMethod;
import com.android.tools.r8.graph.DexType;
import com.android.tools.r8.logging.Log;
+import com.android.tools.r8.shaking.ScopedDexMethodSet.AddMethodIfMoreVisibleResult;
import java.util.ArrayList;
import java.util.List;
@@ -54,7 +55,7 @@
List<DexEncodedMethod> methods = null;
for (int i = 0; i < virtualMethods.size(); i++) {
DexEncodedMethod method = virtualMethods.get(i);
- if (scope.addMethodIfMoreVisible(method)
+ if (scope.addMethodIfMoreVisible(method) != AddMethodIfMoreVisibleResult.NOT_ADDED
|| !method.accessFlags.isAbstract()
|| appInfo.isPinned(method.method)) {
if (methods != null) {
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 7d54da3..b8f4076 100644
--- a/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
+++ b/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
@@ -64,6 +64,7 @@
import com.android.tools.r8.shaking.RootSetBuilder.ConsequentRootSet;
import com.android.tools.r8.shaking.RootSetBuilder.IfRuleEvaluator;
import com.android.tools.r8.shaking.RootSetBuilder.RootSet;
+import com.android.tools.r8.shaking.ScopedDexMethodSet.AddMethodIfMoreVisibleResult;
import com.android.tools.r8.utils.InternalOptions;
import com.android.tools.r8.utils.StringDiagnostic;
import com.android.tools.r8.utils.Timing;
@@ -273,6 +274,14 @@
*/
private final ProguardConfiguration.Builder compatibility;
+ /**
+ * A cache of ScopedDexMethodSet for each live type used for determining that virtual methods that
+ * cannot be removed because they are widening access for another virtual method defined earlier
+ * in the type hierarchy. See b/136698023 for more information.
+ */
+ private final Map<DexType, ScopedDexMethodSet> scopedMethodsForLiveTypes =
+ new IdentityHashMap<>();
+
private final GraphConsumer keptGraphConsumer;
public Enqueuer(
@@ -868,6 +877,11 @@
//
private void markTypeAsLive(DexType type) {
+ markTypeAsLive(
+ type, scopedMethodsForLiveTypes.computeIfAbsent(type, ignore -> new ScopedDexMethodSet()));
+ }
+
+ private void markTypeAsLive(DexType type, ScopedDexMethodSet seen) {
type = type.toBaseType(appView.dexItemFactory());
if (!type.isClassType()) {
// Ignore primitive types.
@@ -886,7 +900,11 @@
markInterfaceTypeAsLiveViaInheritanceClause(iface);
}
if (holder.superType != null) {
- markTypeAsLive(holder.superType);
+ ScopedDexMethodSet seenForSuper =
+ scopedMethodsForLiveTypes.computeIfAbsent(
+ holder.superType, ignore -> new ScopedDexMethodSet());
+ seen.setParent(seenForSuper);
+ markTypeAsLive(holder.superType, seenForSuper);
if (holder.isNotProgramClass()) {
// Library classes may only extend other implement library classes.
ensureNotFromProgramOrThrow(holder.superType, type);
@@ -895,9 +913,38 @@
}
}
}
+
+ KeepReason reason = KeepReason.reachableFromLiveType(type);
+
+ // We cannot remove virtual methods defined earlier in the type hierarchy if it is widening
+ // access and is defined in an interface:
+ //
+ // public interface I {
+ // void clone();
+ // }
+ //
+ // class Model implements I {
+ // public void clone() { ... } <-- this cannot be removed
+ // }
+ //
+ // Any class loading of Model with Model.clone() removed will result in an illegal access
+ // error because their exists an existing implementation (here it is Object.clone()). This is
+ // only a problem in the DEX VM. We have to make this check no matter the output because
+ // CF libraries can be used by Android apps. See b/136698023 for more information.
+ holder
+ .virtualMethods()
+ .forEach(
+ m -> {
+ if (seen.addMethodIfMoreVisible(m)
+ == AddMethodIfMoreVisibleResult.ADDED_MORE_VISIBLE
+ && holder.isProgramClass()
+ && appView.appInfo().methodDefinedInInterfaces(m, holder.type)) {
+ markMethodAsTargeted(m, reason);
+ }
+ });
+
// We also need to add the corresponding <clinit> to the set of live methods, as otherwise
// static field initialization (and other class-load-time sideeffects) will not happen.
- KeepReason reason = KeepReason.reachableFromLiveType(type);
if (holder.isProgramClass() && holder.hasClassInitializer()) {
DexEncodedMethod clinit = holder.getClassInitializer();
if (clinit != null && clinit.getOptimizationInfo().mayHaveSideEffects()) {
diff --git a/src/main/java/com/android/tools/r8/shaking/ScopedDexMethodSet.java b/src/main/java/com/android/tools/r8/shaking/ScopedDexMethodSet.java
index 6d76acd..7326fad 100644
--- a/src/main/java/com/android/tools/r8/shaking/ScopedDexMethodSet.java
+++ b/src/main/java/com/android/tools/r8/shaking/ScopedDexMethodSet.java
@@ -13,9 +13,15 @@
class ScopedDexMethodSet {
+ public enum AddMethodIfMoreVisibleResult {
+ NOT_ADDED,
+ ADDED_NOT_EXISTING,
+ ADDED_MORE_VISIBLE
+ }
+
private static final Equivalence<DexMethod> METHOD_EQUIVALENCE = MethodSignatureEquivalence.get();
- private final ScopedDexMethodSet parent;
+ private ScopedDexMethodSet parent;
private final Map<Wrapper<DexMethod>, DexEncodedMethod> items = new HashMap<>();
public ScopedDexMethodSet() {
@@ -48,21 +54,28 @@
return true;
}
- public boolean addMethodIfMoreVisible(DexEncodedMethod method) {
+ public AddMethodIfMoreVisibleResult addMethodIfMoreVisible(DexEncodedMethod method) {
Wrapper<DexMethod> wrapped = METHOD_EQUIVALENCE.wrap(method.method);
DexEncodedMethod existing = lookup(wrapped);
- if (existing == null
- || method.accessFlags.isMoreVisibleThan(
- existing.accessFlags,
- method.method.holder.getPackageName(),
- existing.method.holder.getPackageName())) {
+ if (existing == null) {
items.put(wrapped, method);
- return true;
+ return AddMethodIfMoreVisibleResult.ADDED_NOT_EXISTING;
}
- return false;
+ if (method.accessFlags.isMoreVisibleThan(
+ existing.accessFlags,
+ method.method.holder.getPackageName(),
+ existing.method.holder.getPackageName())) {
+ items.put(wrapped, method);
+ return AddMethodIfMoreVisibleResult.ADDED_MORE_VISIBLE;
+ }
+ return AddMethodIfMoreVisibleResult.NOT_ADDED;
}
public ScopedDexMethodSet getParent() {
return parent;
}
+
+ public void setParent(ScopedDexMethodSet parent) {
+ this.parent = parent;
+ }
}
diff --git a/src/test/java/com/android/tools/r8/shaking/b136698023/LibraryBridgeTest.java b/src/test/java/com/android/tools/r8/shaking/b136698023/LibraryBridgeTest.java
index 4eb9d4d..2ea5e50 100644
--- a/src/test/java/com/android/tools/r8/shaking/b136698023/LibraryBridgeTest.java
+++ b/src/test/java/com/android/tools/r8/shaking/b136698023/LibraryBridgeTest.java
@@ -13,7 +13,6 @@
import com.android.tools.r8.TestParametersCollection;
import java.io.IOException;
import java.util.concurrent.ExecutionException;
-import org.junit.Ignore;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
@@ -96,7 +95,6 @@
}
@Test
- @Ignore("b/136698023")
public void testRemovingClone()
throws ExecutionException, CompilationFailedException, IOException {
testForR8(parameters.getBackend())
@@ -110,7 +108,6 @@
}
@Test
- @Ignore("b/136698023")
public void testRemovingXClone()
throws ExecutionException, CompilationFailedException, IOException {
testForR8(parameters.getBackend())
@@ -124,7 +121,6 @@
}
@Test
- @Ignore("b/136698023")
public void testRemovingXCloneImpl()
throws ExecutionException, CompilationFailedException, IOException {
testForR8(parameters.getBackend())
@@ -138,7 +134,6 @@
}
@Test
- @Ignore("b/136698023")
public void testRemovingXCloneWithDefinitionInLibrary()
throws ExecutionException, CompilationFailedException, IOException {
R8TestCompileResult library =
