Account for package private targets in bridge hoisting
Bug: b/233866639
Change-Id: I0e47c87b399e19f5acda4a7262126b43b117f5ef
diff --git a/src/main/java/com/android/tools/r8/optimize/bridgehoisting/BridgeHoisting.java b/src/main/java/com/android/tools/r8/optimize/bridgehoisting/BridgeHoisting.java
index 09966ca..77e0ec3 100644
--- a/src/main/java/com/android/tools/r8/optimize/bridgehoisting/BridgeHoisting.java
+++ b/src/main/java/com/android/tools/r8/optimize/bridgehoisting/BridgeHoisting.java
@@ -141,7 +141,7 @@
for (DexProgramClass subclass : subclasses) {
for (DexEncodedMethod method : subclass.virtualMethods()) {
BridgeInfo bridgeInfo = method.getOptimizationInfo().getBridgeInfo();
- if (bridgeInfo != null) {
+ if (bridgeInfo != null && bridgeInfo.isVirtualBridgeInfo()) {
candidates.add(equivalence.wrap(method.getReference()));
}
}
@@ -158,6 +158,18 @@
return;
}
+ // Bail out if the bridge is also declared in the parent class. In that case, hoisting would
+ // change the behavior of calling the bridge on an instance of the parent class.
+ MethodResolutionResult res =
+ appView.appInfo().resolveMethodOnClass(clazz.getSuperType(), method);
+ if (res.isSingleResolution()) {
+ if (!res.getResolvedMethod().isAbstract()) {
+ return;
+ }
+ } else if (res.isMultiMethodResolutionResult()) {
+ return;
+ }
+
// Go through each of the subclasses and find the bridges that can be hoisted. The bridge holder
// classes are stored in buckets grouped by the behavior of the body of the bridge (which is
// implicitly defined by the signature of the invoke-virtual instruction).
@@ -187,7 +199,7 @@
}
BridgeInfo currentBridgeInfo = definition.getOptimizationInfo().getBridgeInfo();
- if (currentBridgeInfo == null) {
+ if (currentBridgeInfo == null || currentBridgeInfo.isDirectBridgeInfo()) {
// This is not a bridge, so the method needs to remain on the subclass.
continue;
}
@@ -196,14 +208,27 @@
VirtualBridgeInfo currentVirtualBridgeInfo = currentBridgeInfo.asVirtualBridgeInfo();
DexMethod invokedMethod = currentVirtualBridgeInfo.getInvokedMethod();
+
+ if (!clazz.getType().isSamePackage(subclass.getType())) {
+ DexEncodedMethod resolvedMethod =
+ appView.appInfo().resolveMethodOnClass(clazz, invokedMethod).getSingleTarget();
+ if (resolvedMethod == null || resolvedMethod.getAccessFlags().isPackagePrivate()) {
+ // After hoisting this bridge would now dispatch to another method, namely the package
+ // private method in the parent class.
+ continue;
+ }
+ }
+
Wrapper<DexMethod> wrapper = MethodSignatureEquivalence.get().wrap(invokedMethod);
eligibleVirtualInvokeBridges
.computeIfAbsent(wrapper, ignore -> new ArrayList<>())
.add(subclass);
}
- // There should be at least one method that is eligible for hoisting.
- assert !eligibleVirtualInvokeBridges.isEmpty();
+ // Check if any bridges may be eligible for hoisting.
+ if (eligibleVirtualInvokeBridges.isEmpty()) {
+ return;
+ }
Entry<Wrapper<DexMethod>, List<DexProgramClass>> mostFrequentBridge =
findMostFrequentBridge(eligibleVirtualInvokeBridges);
@@ -217,7 +242,7 @@
ProgramMethod representative = eligibleBridgeMethods.iterator().next();
// Guard against accessibility issues.
- if (mayBecomeInaccessibleAfterHoisting(clazz, representative)) {
+ if (mayBecomeInaccessibleAfterHoisting(clazz, eligibleBridgeMethods, representative)) {
return;
}
@@ -287,11 +312,21 @@
}
private boolean mayBecomeInaccessibleAfterHoisting(
- DexProgramClass clazz, ProgramMethod representative) {
- if (clazz.type.isSamePackage(representative.getHolder().type)) {
- return false;
+ DexProgramClass clazz,
+ List<ProgramMethod> eligibleBridgeMethods,
+ ProgramMethod representative) {
+ int representativeVisibility = representative.getAccessFlags().getVisibilityOrdinal();
+ for (ProgramMethod eligibleBridgeMethod : eligibleBridgeMethods) {
+ if (eligibleBridgeMethod.getAccessFlags().getVisibilityOrdinal()
+ != representativeVisibility) {
+ return true;
+ }
+ if (!clazz.getType().isSamePackage(eligibleBridgeMethod.getHolderType())
+ && !eligibleBridgeMethod.getDefinition().isPublic()) {
+ return true;
+ }
}
- return !representative.getDefinition().isPublic();
+ return false;
}
private Code createCodeForVirtualBridge(ProgramMethod representative, DexMethod methodToInvoke) {
diff --git a/src/test/java/com/android/tools/r8/bridgeremoval/hoisting/BridgeToPackagePrivateMethodTest.java b/src/test/java/com/android/tools/r8/bridgeremoval/hoisting/BridgeToPackagePrivateMethodTest.java
index 10fc6da..449975b 100644
--- a/src/test/java/com/android/tools/r8/bridgeremoval/hoisting/BridgeToPackagePrivateMethodTest.java
+++ b/src/test/java/com/android/tools/r8/bridgeremoval/hoisting/BridgeToPackagePrivateMethodTest.java
@@ -11,6 +11,7 @@
import com.android.tools.r8.TestParameters;
import com.android.tools.r8.TestParametersCollection;
import com.google.common.collect.ImmutableList;
+import java.io.IOException;
import java.util.List;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -35,18 +36,7 @@
@Test
public void testRuntime() throws Exception {
testForRuntime(parameters)
- .addProgramClassFileData(
- transformer(Main.class)
- .replaceClassDescriptorInMethodInstructions(
- descriptor(A.class), TRANSFORMED_A_DESCRIPTOR)
- .transform(),
- transformer(A.class).setClassDescriptor(TRANSFORMED_A_DESCRIPTOR).transform(),
- transformer(B.class)
- .setSuper(TRANSFORMED_A_DESCRIPTOR)
- .replaceClassDescriptorInMethodInstructions(
- descriptor(A.class), TRANSFORMED_A_DESCRIPTOR)
- .setBridge(B.class.getDeclaredMethod("bridge"))
- .transform())
+ .addProgramClassFileData(getProgramClassFileData())
.run(parameters.getRuntime(), Main.class)
.assertSuccessWithOutputLines(EXPECTED_OUTPUT);
}
@@ -54,18 +44,7 @@
@Test
public void testR8() throws Exception {
testForR8(parameters.getBackend())
- .addProgramClassFileData(
- transformer(Main.class)
- .replaceClassDescriptorInMethodInstructions(
- descriptor(A.class), TRANSFORMED_A_DESCRIPTOR)
- .transform(),
- transformer(A.class).setClassDescriptor(TRANSFORMED_A_DESCRIPTOR).transform(),
- transformer(B.class)
- .setSuper(TRANSFORMED_A_DESCRIPTOR)
- .replaceClassDescriptorInMethodInstructions(
- descriptor(A.class), TRANSFORMED_A_DESCRIPTOR)
- .setBridge(B.class.getDeclaredMethod("bridge"))
- .transform())
+ .addProgramClassFileData(getProgramClassFileData())
.addKeepMainRule(Main.class)
.enableInliningAnnotations()
.enableNeverClassInliningAnnotations()
@@ -73,11 +52,22 @@
.setMinApi(parameters.getApiLevel())
.compile()
.run(parameters.getRuntime(), Main.class)
- // TODO(b/233866639): Should succeed with "A", "B".
- .applyIf(
- parameters.isDexRuntime() && parameters.getDexRuntimeVersion().isDalvik(),
- runResult -> runResult.assertSuccessWithOutputLines(EXPECTED_OUTPUT),
- runResult -> runResult.assertSuccessWithOutputLines("A", "A"));
+ .assertSuccessWithOutputLines(EXPECTED_OUTPUT);
+ }
+
+ private List<byte[]> getProgramClassFileData() throws IOException, NoSuchMethodException {
+ return ImmutableList.of(
+ transformer(Main.class)
+ .replaceClassDescriptorInMethodInstructions(
+ descriptor(A.class), TRANSFORMED_A_DESCRIPTOR)
+ .transform(),
+ transformer(A.class).setClassDescriptor(TRANSFORMED_A_DESCRIPTOR).transform(),
+ transformer(B.class)
+ .setSuper(TRANSFORMED_A_DESCRIPTOR)
+ .replaceClassDescriptorInMethodInstructions(
+ descriptor(A.class), TRANSFORMED_A_DESCRIPTOR)
+ .setBridge(B.class.getDeclaredMethod("bridge"))
+ .transform());
}
static class Main {
diff --git a/src/test/java/com/android/tools/r8/bridgeremoval/hoisting/BridgeToPackagePrivateMethodThatOverridesPublicMethodTest.java b/src/test/java/com/android/tools/r8/bridgeremoval/hoisting/BridgeToPackagePrivateMethodThatOverridesPublicMethodTest.java
new file mode 100644
index 0000000..d099f9c
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/bridgeremoval/hoisting/BridgeToPackagePrivateMethodThatOverridesPublicMethodTest.java
@@ -0,0 +1,202 @@
+// Copyright (c) 2022, 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.bridgeremoval.hoisting;
+
+import static com.android.tools.r8.utils.codeinspector.Matchers.isAbsent;
+import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
+import static com.android.tools.r8.utils.codeinspector.Matchers.notIf;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+import com.android.tools.r8.NeverClassInline;
+import com.android.tools.r8.NeverInline;
+import com.android.tools.r8.NoVerticalClassMerging;
+import com.android.tools.r8.TestBase;
+import com.android.tools.r8.TestParameters;
+import com.android.tools.r8.utils.BooleanUtils;
+import com.android.tools.r8.utils.DescriptorUtils;
+import com.android.tools.r8.utils.codeinspector.ClassSubject;
+import com.android.tools.r8.utils.codeinspector.MethodSubject;
+import com.google.common.collect.ImmutableList;
+import java.io.IOException;
+import java.util.List;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameter;
+import org.junit.runners.Parameterized.Parameters;
+
+@RunWith(Parameterized.class)
+public class BridgeToPackagePrivateMethodThatOverridesPublicMethodTest extends TestBase {
+
+ private static final String TRANSFORMED_B_DESCRIPTOR = "LB;";
+
+ @Parameter(0)
+ public TestParameters parameters;
+
+ @Parameter(1)
+ public boolean removeBridgeMethodFromA;
+
+ @Parameters(name = "{0}, remove A.bridge(): {1}")
+ public static List<Object[]> data() {
+ return buildParameters(
+ getTestParameters().withAllRuntimesAndApiLevels().build(), BooleanUtils.values());
+ }
+
+ @Test
+ public void testRuntime() throws Exception {
+ testForRuntime(parameters)
+ .addProgramClasses(Base.class)
+ .addProgramClassFileData(getProgramClassFileData())
+ .run(parameters.getRuntime(), Main.class)
+ .assertSuccessWithOutputLines(getExpectedOutput());
+ }
+
+ @Test
+ public void testR8() throws Exception {
+ testForR8(parameters.getBackend())
+ .addProgramClasses(Base.class)
+ .addProgramClassFileData(getProgramClassFileData())
+ .addKeepMainRule(Main.class)
+ .enableInliningAnnotations()
+ .enableNeverClassInliningAnnotations()
+ .enableNoVerticalClassMergingAnnotations()
+ .setMinApi(parameters.getApiLevel())
+ .compile()
+ .inspect(
+ inspector -> {
+ // Inspect Base.
+ ClassSubject baseClassSubject = inspector.clazz(Base.class);
+ assertThat(baseClassSubject, isPresent());
+
+ MethodSubject bridgeOnBaseMethodSubject =
+ baseClassSubject.uniqueMethodWithName("bridge");
+ assertThat(bridgeOnBaseMethodSubject, isPresent());
+
+ // Inspect A. This should always have a bridge method. If A.bridge() is removed by
+ // the transformation, then the bridge method from C should be hoisted here.
+ // Otherwise, the original A.bridge method remains.
+ ClassSubject aClassSubject = inspector.clazz(A.class);
+ assertThat(aClassSubject, isPresent());
+
+ MethodSubject bridgeOnAMethodSubject = aClassSubject.uniqueMethodWithName("bridge");
+ assertThat(bridgeOnAMethodSubject, notIf(isPresent(), removeBridgeMethodFromA));
+
+ // Inspect B. This should have a bridge method if-and-only-if A.bridge() is not
+ // removed by the transformation.
+ ClassSubject bClassSubject =
+ inspector.clazz(DescriptorUtils.descriptorToJavaType(TRANSFORMED_B_DESCRIPTOR));
+ assertThat(bClassSubject, isPresent());
+
+ MethodSubject bridgeMethodSubject = bClassSubject.uniqueMethodWithName("bridge");
+ assertThat(bridgeMethodSubject, isAbsent());
+
+ MethodSubject testMethodSubject = bClassSubject.uniqueMethodWithName("test");
+ assertThat(testMethodSubject, isPresent());
+
+ // Inspect C. The method C.bridge() should always be removed by hoisting.
+ ClassSubject cClassSubject = inspector.clazz(C.class);
+ assertThat(cClassSubject, isPresent());
+
+ MethodSubject bridgeOnCMethodSubject = cClassSubject.uniqueMethodWithName("bridge");
+ assertThat(bridgeOnCMethodSubject, isPresent());
+ })
+ .run(parameters.getRuntime(), Main.class)
+ .assertSuccessWithOutputLines(getExpectedOutput());
+ }
+
+ private List<String> getExpectedOutput() {
+ if (removeBridgeMethodFromA) {
+ return ImmutableList.of("A.m()", "Base.bridge()", "C.m()", "C.m()");
+ }
+ return ImmutableList.of("A.m()", "A.bridge()", "C.m()", "C.m()");
+ }
+
+ private List<byte[]> getProgramClassFileData() throws IOException, NoSuchMethodException {
+ return ImmutableList.of(
+ transformer(Main.class)
+ .replaceClassDescriptorInMethodInstructions(
+ descriptor(B.class), TRANSFORMED_B_DESCRIPTOR)
+ .transform(),
+ transformer(A.class)
+ .applyIf(
+ removeBridgeMethodFromA, transformer -> transformer.removeMethodsWithName("bridge"))
+ .setPublic(A.class.getDeclaredMethod("m"))
+ .transform(),
+ transformer(B.class).setClassDescriptor(TRANSFORMED_B_DESCRIPTOR).transform(),
+ transformer(C.class)
+ .setSuper(TRANSFORMED_B_DESCRIPTOR)
+ .replaceClassDescriptorInMethodInstructions(
+ descriptor(B.class), TRANSFORMED_B_DESCRIPTOR)
+ .setBridge(C.class.getDeclaredMethod("bridge"))
+ .transform());
+ }
+
+ static class Main {
+
+ public static void main(String[] args) {
+ new A().callM();
+ B bAsB = System.currentTimeMillis() > 0 ? new B() : new C();
+ B cAsB = System.currentTimeMillis() > 0 ? new C() : new B();
+ Base cAsBase = System.currentTimeMillis() > 0 ? new C() : new Base();
+ bAsB.test(bAsB);
+ bAsB.test(cAsB);
+ cAsBase.bridge();
+ }
+ }
+
+ @NoVerticalClassMerging
+ public static class Base {
+
+ public void bridge() {
+ System.out.println("Base.bridge()");
+ }
+ }
+
+ @NeverClassInline
+ @NoVerticalClassMerging
+ public static class A extends Base {
+
+ @NeverInline
+ /*public*/ void m() {
+ System.out.println("A.m()");
+ }
+
+ @Override
+ public void bridge() {
+ System.out.println("A.bridge()");
+ }
+
+ @NeverInline
+ public void callM() {
+ m();
+ }
+ }
+
+ @NeverClassInline
+ @NoVerticalClassMerging
+ public static class /*otherpackage.*/ B extends A {
+
+ @NeverInline
+ public void test(B b) {
+ b.bridge();
+ }
+ }
+
+ @NeverClassInline
+ public static class C extends B {
+
+ @NeverInline
+ void m() {
+ System.out.println("C.m()");
+ }
+
+ // Not eligible for bridge hoisting, as the bridge will then dispatch to A.m instead of B.m.
+ @NeverInline
+ @Override
+ public /*bridge*/ void bridge() {
+ this.m();
+ }
+ }
+}
diff --git a/src/test/java/com/android/tools/r8/transformers/ClassFileTransformer.java b/src/test/java/com/android/tools/r8/transformers/ClassFileTransformer.java
index b4c6e12..322c1d4 100644
--- a/src/test/java/com/android/tools/r8/transformers/ClassFileTransformer.java
+++ b/src/test/java/com/android/tools/r8/transformers/ClassFileTransformer.java
@@ -1007,6 +1007,26 @@
}
@Override
+ public void visitFrame(
+ int type, int numLocal, Object[] local, int numStack, Object[] stack) {
+ for (int i = 0; i < numLocal; i++) {
+ Object object = local[i];
+ if (object instanceof String) {
+ local[i] = rewriteASMInternalTypeName((String) object);
+ }
+ i++;
+ }
+ for (int i = 0; i < numStack; i++) {
+ Object object = stack[i];
+ if (object instanceof String) {
+ stack[i] = rewriteASMInternalTypeName((String) object);
+ }
+ i++;
+ }
+ super.visitFrame(type, numLocal, local, numStack, stack);
+ }
+
+ @Override
public void visitLdcInsn(Object value) {
if (value instanceof Type) {
Type type = (Type) value;
