AbstractMethodRemover: Don't remove more visible abstract methods
When running R8 on R8, AbstractMethodRemover would remove IndexedDexItem
.collectIndexedItems(IndexedItemCollection, DexMethod, int), which is an
abstract method that overrides the corresponding method on DexItem and
increases the visibility from package-private to public. This in turn
causes IllegalAccessError at runtime.
Change-Id: Ibded0725080dd78c902222b85cec17b83ef60038
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 324362a..626e5cc 100644
--- a/src/main/java/com/android/tools/r8/graph/AccessFlags.java
+++ b/src/main/java/com/android/tools/r8/graph/AccessFlags.java
@@ -68,6 +68,25 @@
return flags;
}
+ public boolean isMoreVisibleThan(AccessFlags other) {
+ return visibilityOrdinal() > other.visibilityOrdinal();
+ }
+
+ private int visibilityOrdinal() {
+ // public > protected > package > private
+ if (isPublic()) {
+ return 3;
+ }
+ if (isProtected()) {
+ return 2;
+ }
+ if (isPrivate()) {
+ return 0;
+ }
+ // Package-private
+ return 1;
+ }
+
public boolean isPublic() {
return isSet(Constants.ACC_PUBLIC);
}
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 e4e2ed7..610bfd3 100644
--- a/src/main/java/com/android/tools/r8/shaking/AbstractMethodRemover.java
+++ b/src/main/java/com/android/tools/r8/shaking/AbstractMethodRemover.java
@@ -52,7 +52,7 @@
List<DexEncodedMethod> methods = null;
for (int i = 0; i < virtualMethods.length; i++) {
DexEncodedMethod method = virtualMethods[i];
- if (scope.addMethod(method.method) || !method.accessFlags.isAbstract()) {
+ if (scope.addMethodIfMoreVisible(method) || !method.accessFlags.isAbstract()) {
if (methods != null) {
methods.add(method);
}
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 27b2da7..d4fcc67 100644
--- a/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
+++ b/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
@@ -748,7 +748,7 @@
private void transitionNonAbstractMethodsToLiveAndShadow(Iterable<DexEncodedMethod> reachable,
DexType instantiatedType, ScopedDexMethodSet seen) {
for (DexEncodedMethod encodedMethod : reachable) {
- if (seen.addMethod(encodedMethod.method)) {
+ if (seen.addMethod(encodedMethod)) {
// Abstract methods do shadow implementations but they cannot be live, as they have no
// code.
if (!encodedMethod.accessFlags.isAbstract()) {
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 39f07ee..3587cf4 100644
--- a/src/main/java/com/android/tools/r8/shaking/ScopedDexMethodSet.java
+++ b/src/main/java/com/android/tools/r8/shaking/ScopedDexMethodSet.java
@@ -3,19 +3,20 @@
// BSD-style license that can be found in the LICENSE file.
package com.android.tools.r8.shaking;
+import com.android.tools.r8.graph.DexEncodedMethod;
import com.android.tools.r8.graph.DexMethod;
import com.android.tools.r8.utils.MethodSignatureEquivalence;
import com.google.common.base.Equivalence;
import com.google.common.base.Equivalence.Wrapper;
-import java.util.HashSet;
-import java.util.Set;
+import java.util.HashMap;
+import java.util.Map;
class ScopedDexMethodSet {
private static final Equivalence<DexMethod> METHOD_EQUIVALENCE = MethodSignatureEquivalence.get();
private final ScopedDexMethodSet parent;
- private final Set<Wrapper<DexMethod>> items = new HashSet<>();
+ private final Map<Wrapper<DexMethod>, DexEncodedMethod> items = new HashMap<>();
public ScopedDexMethodSet() {
this(null);
@@ -29,14 +30,32 @@
return new ScopedDexMethodSet(this);
}
- private boolean contains(Wrapper<DexMethod> item) {
- return items.contains(item)
- || ((parent != null) && parent.contains(item));
+ private DexEncodedMethod lookup(Wrapper<DexMethod> item) {
+ DexEncodedMethod ownMethod = items.get(item);
+ return ownMethod != null ? ownMethod : (parent != null ? parent.lookup(item) : null);
}
- public boolean addMethod(DexMethod method) {
- Wrapper<DexMethod> wrapped = METHOD_EQUIVALENCE.wrap(method);
- return !contains(wrapped) && items.add(wrapped);
+ private boolean contains(Wrapper<DexMethod> item) {
+ return lookup(item) != null;
+ }
+
+ public boolean addMethod(DexEncodedMethod method) {
+ Wrapper<DexMethod> wrapped = METHOD_EQUIVALENCE.wrap(method.method);
+ if (contains(wrapped)) {
+ return false;
+ }
+ items.put(wrapped, method);
+ return true;
+ }
+
+ public boolean addMethodIfMoreVisible(DexEncodedMethod method) {
+ Wrapper<DexMethod> wrapped = METHOD_EQUIVALENCE.wrap(method.method);
+ DexEncodedMethod existing = lookup(wrapped);
+ if (existing == null || method.accessFlags.isMoreVisibleThan(existing.accessFlags)) {
+ items.put(wrapped, method);
+ return true;
+ }
+ return false;
}
public ScopedDexMethodSet getParent() {
diff --git a/src/test/examples/abstractmethodremoval/AbstractMethodRemoval.java b/src/test/examples/abstractmethodremoval/AbstractMethodRemoval.java
new file mode 100644
index 0000000..1da772d
--- /dev/null
+++ b/src/test/examples/abstractmethodremoval/AbstractMethodRemoval.java
@@ -0,0 +1,34 @@
+// Copyright (c) 2018, 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 abstractmethodremoval;
+
+import abstractmethodremoval.a.PackageBase;
+import abstractmethodremoval.a.Public;
+import abstractmethodremoval.b.Impl1;
+import abstractmethodremoval.b.Impl2;
+
+public class AbstractMethodRemoval {
+
+ private static int dummy;
+
+ public static void main(String[] args) {
+ dummy = args.length;
+ invokeFoo(new Impl1());
+ invokeFoo(new Impl2());
+ invokeFoo(new Impl2());
+ PackageBase.invokeFoo(new Impl1());
+ PackageBase.invokeFoo(new Impl2());
+ PackageBase.invokeFoo(new Impl2());
+ }
+
+ private static void invokeFoo(Public aPublic) {
+ // Enough instructions to avoid inlining.
+ aPublic.foo(dummy() + dummy() + dummy() + dummy() + dummy() + dummy() + dummy() + dummy());
+ }
+
+ private static int dummy() {
+ // Enough instructions to avoid inlining.
+ return dummy + dummy + dummy + dummy + dummy + dummy + dummy + dummy + dummy + dummy + dummy;
+ }
+}
diff --git a/src/test/examples/abstractmethodremoval/a/PackageBase.java b/src/test/examples/abstractmethodremoval/a/PackageBase.java
new file mode 100644
index 0000000..1075b57
--- /dev/null
+++ b/src/test/examples/abstractmethodremoval/a/PackageBase.java
@@ -0,0 +1,12 @@
+// Copyright (c) 2018, 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 abstractmethodremoval.a;
+
+public abstract class PackageBase {
+ abstract void foo(int i);
+
+ public static void invokeFoo(PackageBase o) {
+ o.foo(0);
+ }
+}
diff --git a/src/test/examples/abstractmethodremoval/a/Public.java b/src/test/examples/abstractmethodremoval/a/Public.java
new file mode 100644
index 0000000..492ab87
--- /dev/null
+++ b/src/test/examples/abstractmethodremoval/a/Public.java
@@ -0,0 +1,9 @@
+// Copyright (c) 2018, 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 abstractmethodremoval.a;
+
+public abstract class Public extends PackageBase {
+ @Override
+ public abstract void foo(int i);
+}
diff --git a/src/test/examples/abstractmethodremoval/b/Impl1.java b/src/test/examples/abstractmethodremoval/b/Impl1.java
new file mode 100644
index 0000000..bfdf4d1
--- /dev/null
+++ b/src/test/examples/abstractmethodremoval/b/Impl1.java
@@ -0,0 +1,13 @@
+// Copyright (c) 2018, 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 abstractmethodremoval.b;
+
+import abstractmethodremoval.a.Public;
+
+public class Impl1 extends Public {
+ @Override
+ public void foo(int i) {
+ System.out.println("Impl1.foo(" + i + ")");
+ }
+}
diff --git a/src/test/examples/abstractmethodremoval/b/Impl2.java b/src/test/examples/abstractmethodremoval/b/Impl2.java
new file mode 100644
index 0000000..a92e2a1
--- /dev/null
+++ b/src/test/examples/abstractmethodremoval/b/Impl2.java
@@ -0,0 +1,13 @@
+// Copyright (c) 2018, 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 abstractmethodremoval.b;
+
+import abstractmethodremoval.a.Public;
+
+public class Impl2 extends Public {
+ @Override
+ public void foo(int i) {
+ System.out.println("Impl2.foo(" + i + ")");
+ }
+}
diff --git a/src/test/examples/abstractmethodremoval/keep-rules.txt b/src/test/examples/abstractmethodremoval/keep-rules.txt
new file mode 100644
index 0000000..ec37858
--- /dev/null
+++ b/src/test/examples/abstractmethodremoval/keep-rules.txt
@@ -0,0 +1 @@
+-keep public class abstractmethodremoval.AbstractMethodRemoval { public static void main(...); }
diff --git a/src/test/java/com/android/tools/r8/R8RunExamplesTest.java b/src/test/java/com/android/tools/r8/R8RunExamplesTest.java
index 913278a..2ac50a6 100644
--- a/src/test/java/com/android/tools/r8/R8RunExamplesTest.java
+++ b/src/test/java/com/android/tools/r8/R8RunExamplesTest.java
@@ -27,6 +27,7 @@
@Parameters(name = "{0}_{1}_{2}_{3}_{5}_{6}")
public static Collection<String[]> data() {
String[] tests = {
+ "abstractmethodremoval.AbstractMethodRemoval",
"arithmetic.Arithmetic",
"arrayaccess.ArrayAccess",
"barray.BArray",
diff --git a/src/test/java/com/android/tools/r8/shaking/examples/TreeShakingAbstractMethodRemovalTest.java b/src/test/java/com/android/tools/r8/shaking/examples/TreeShakingAbstractMethodRemovalTest.java
new file mode 100644
index 0000000..a5b13a7
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/shaking/examples/TreeShakingAbstractMethodRemovalTest.java
@@ -0,0 +1,49 @@
+// Copyright (c) 2018, 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.shaking.examples;
+
+import com.android.tools.r8.TestBase.MinifyMode;
+import com.android.tools.r8.shaking.TreeShakingTest;
+import com.google.common.collect.ImmutableList;
+import java.util.ArrayList;
+import java.util.Collection;
+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 TreeShakingAbstractMethodRemovalTest extends TreeShakingTest {
+
+ @Parameters(name = "mode:{0}-{1} minify:{2}")
+ public static Collection<Object[]> data() {
+ List<Object[]> parameters = new ArrayList<>();
+ for (MinifyMode minify : MinifyMode.values()) {
+ parameters.add(new Object[] {Frontend.JAR, Backend.CF, minify});
+ parameters.add(new Object[] {Frontend.JAR, Backend.DEX, minify});
+ parameters.add(new Object[] {Frontend.DEX, Backend.DEX, minify});
+ }
+ return parameters;
+ }
+
+ public TreeShakingAbstractMethodRemovalTest(
+ Frontend frontend, Backend backend, MinifyMode minify) {
+ super(
+ "examples/abstractmethodremoval",
+ "abstractmethodremoval.AbstractMethodRemoval",
+ frontend,
+ backend,
+ minify);
+ }
+
+ @Test
+ public void test() throws Exception {
+ runTest(
+ null,
+ null,
+ null,
+ ImmutableList.of("src/test/examples/abstractmethodremoval/keep-rules.txt"));
+ }
+}
