Don't insert visibility bridges in contexts without access.
Fix: b/427887773
Change-Id: I62cb8e15abc99933653cb2ec585caf2bb0915e14
diff --git a/src/main/java/com/android/tools/r8/graph/MethodResolution.java b/src/main/java/com/android/tools/r8/graph/MethodResolution.java
index 9a75cfe..fad11a2 100644
--- a/src/main/java/com/android/tools/r8/graph/MethodResolution.java
+++ b/src/main/java/com/android/tools/r8/graph/MethodResolution.java
@@ -509,7 +509,7 @@
* Section 5.4.3.4 of the JVM Spec</a>.
*
* <p>The resolved method is not the method that will actually be invoked. Which methods gets
- * invoked depends on the invoke instruction used. However, it is always save to rewrite any
+ * invoked depends on the invoke instruction used. However, it is always safe to rewrite any
* invoke on the given descriptor to a corresponding invoke on the resolved descriptor, as the
* resolved method is used as basis for dispatch.
*/
diff --git a/src/main/java/com/android/tools/r8/optimize/MemberRebindingAnalysis.java b/src/main/java/com/android/tools/r8/optimize/MemberRebindingAnalysis.java
index e18fa27..7fcedfc 100644
--- a/src/main/java/com/android/tools/r8/optimize/MemberRebindingAnalysis.java
+++ b/src/main/java/com/android/tools/r8/optimize/MemberRebindingAnalysis.java
@@ -321,40 +321,58 @@
originalClass.accessFlags.isPublic() ? null : method.holder.getPackageDescriptor();
if (packageDescriptor == null
|| !packageDescriptor.equals(target.getHolderType().getPackageDescriptor())) {
- DexProgramClass bridgeHolder =
+ DexClass bridgeHolder =
findHolderForVisibilityBridge(originalClass, target.getHolder(), packageDescriptor);
assert bridgeHolder != null;
- bridges.accept(bridgeHolder, method, target);
- return target.getReference().withHolder(bridgeHolder.getType(), appView.dexItemFactory());
+ if (bridgeHolder.isClasspathClass()) {
+ // Intentionally empty. We do not need to insert a bridge on a classpath class.
+ } else if (bridgeHolder.isLibraryClass()) {
+ // Caution is needed when member rebinding into the library.
+ // This is handled by validMemberRebindingTargetForNonProgramMethod.
+ return null;
+ } else {
+ // Insert a bridge on this program class.
+ bridges.accept(bridgeHolder.asProgramClass(), method, target);
+ }
+ return target.getReference().withHolder(bridgeHolder, appView.dexItemFactory());
}
return target.getReference();
}
- private DexProgramClass findHolderForVisibilityBridge(
- DexClass originalClass, DexClass targetClass, String packageDescriptor) {
- if (originalClass == targetClass || originalClass.isNotProgramClass()) {
+ private DexClass findHolderForVisibilityBridge(
+ DexClass currentClass, DexClass resolvedHolder, String initialResolutionPackage) {
+ if (currentClass == resolvedHolder) {
return null;
}
- DexProgramClass newHolder = null;
+ DexClass newHolder = null;
// Recurse through supertype chain.
- if (appView.appInfo().isSubtype(originalClass.superType, targetClass.type)) {
- DexClass superClass = appView.definitionFor(originalClass.superType);
- newHolder = findHolderForVisibilityBridge(superClass, targetClass, packageDescriptor);
+ if (appView.appInfo().isSubtype(currentClass.superType, resolvedHolder.type)) {
+ DexClass superClass = appView.definitionFor(currentClass.superType);
+ newHolder =
+ findHolderForVisibilityBridge(superClass, resolvedHolder, initialResolutionPackage);
} else {
- for (DexType iface : originalClass.interfaces.values) {
- if (appView.appInfo().isSubtype(iface, targetClass.type)) {
+ for (DexType iface : currentClass.interfaces.values) {
+ if (appView.appInfo().isSubtype(iface, resolvedHolder.type)) {
DexClass interfaceClass = appView.definitionFor(iface);
- newHolder = findHolderForVisibilityBridge(interfaceClass, targetClass, packageDescriptor);
+ newHolder =
+ findHolderForVisibilityBridge(
+ interfaceClass, resolvedHolder, initialResolutionPackage);
}
}
}
if (newHolder != null) {
// A supertype fulfills the visibility requirements.
return newHolder;
- } else if (originalClass.accessFlags.isPublic()
- || originalClass.type.getPackageDescriptor().equals(packageDescriptor)) {
- // This class is visible. Return it if it is a program class, otherwise null.
- return originalClass.asProgramClass();
+ } else {
+ boolean hasAccessToTarget =
+ AccessControl.isClassAccessible(resolvedHolder, currentClass, appView).isTrue();
+ boolean isVisibleToCallers =
+ currentClass.isPublic()
+ || currentClass.type.getPackageDescriptor().equals(initialResolutionPackage);
+ if (hasAccessToTarget && isVisibleToCallers) {
+ // This class is visible and can access the target class.
+ return currentClass;
+ }
}
return null;
}
diff --git a/src/test/java/com/android/tools/r8/memberrebinding/MemberRebindingClasspathSplitTest.java b/src/test/java/com/android/tools/r8/memberrebinding/MemberRebindingClasspathSplitTest.java
new file mode 100644
index 0000000..e87a04d
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/memberrebinding/MemberRebindingClasspathSplitTest.java
@@ -0,0 +1,143 @@
+// Copyright (c) 2025, 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.memberrebinding;
+
+import com.android.tools.r8.TestBase;
+import com.android.tools.r8.TestBuilder;
+import com.android.tools.r8.TestCompileResult;
+import com.android.tools.r8.TestParameters;
+import com.android.tools.r8.ThrowableConsumer;
+import com.android.tools.r8.memberrebinding.testclasses.MemberRebindingClasspathSplitTestClasses;
+import com.google.common.collect.ImmutableList;
+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 MemberRebindingClasspathSplitTest extends TestBase {
+ @Parameter(0)
+ public TestParameters parameters;
+
+ private static class TestConfig {
+ private final String desc;
+
+ private final ThrowableConsumer<? super TestBuilder<?, ?>> addToClasspath;
+ private final ThrowableConsumer<? super TestBuilder<?, ?>> addToProgrampath;
+ private final ThrowableConsumer<? super TestCompileResult<?, ?>> addToRunClasspath;
+
+ private TestConfig(
+ String desc,
+ ThrowableConsumer<? super TestBuilder<?, ?>> addToClasspath,
+ ThrowableConsumer<? super TestBuilder<?, ?>> addToProgrampath,
+ ThrowableConsumer<? super TestCompileResult<?, ?>> addToRunClasspath) {
+ this.desc = desc;
+ this.addToClasspath = addToClasspath;
+ this.addToProgrampath = addToProgrampath;
+ this.addToRunClasspath = addToRunClasspath;
+ }
+
+ @Override
+ public String toString() {
+ return desc;
+ }
+ }
+
+ @Parameter(1)
+ public TestConfig split;
+
+ @Parameters(name = "{0}, classpathsplit: {1}")
+ public static List<Object[]> data() {
+ return buildParameters(
+ getTestParameters().withAllRuntimesAndApiLevels().build(),
+ ImmutableList.of(
+ new TestConfig(
+ "Both A and B on classpath",
+ b -> {
+ b.addClasspathClasses(
+ MemberRebindingClasspathSplitTestClasses.getA(),
+ MemberRebindingClasspathSplitTestClasses.getB());
+ },
+ b -> {},
+ b -> {
+ b.addRunClasspathClasses(
+ MemberRebindingClasspathSplitTestClasses.getA(),
+ MemberRebindingClasspathSplitTestClasses.getB());
+ }),
+ new TestConfig(
+ "Both A and B on classpath, m() bridge removed from B",
+ b -> {
+ b.addClasspathClasses(MemberRebindingClasspathSplitTestClasses.getA());
+ b.addClasspathClassFileData(
+ transformer(MemberRebindingClasspathSplitTestClasses.getB())
+ .removeMethodsWithName("m")
+ .transform());
+ },
+ b -> {},
+ b -> {
+ b.addRunClasspathClasses(MemberRebindingClasspathSplitTestClasses.getA());
+ b.addRunClasspathClassFileData(
+ transformer(MemberRebindingClasspathSplitTestClasses.getB())
+ .removeMethodsWithName("m")
+ .transform());
+ }),
+ new TestConfig(
+ "A on classpath and B on programpath",
+ b -> {
+ b.addClasspathClasses(MemberRebindingClasspathSplitTestClasses.getA());
+ },
+ b -> {
+ b.addProgramClasses(MemberRebindingClasspathSplitTestClasses.getB());
+ },
+ b -> {
+ b.addRunClasspathClasses(MemberRebindingClasspathSplitTestClasses.getA());
+ }),
+ new TestConfig(
+ "A on classpath and B on programpath, m() bridge removed from B",
+ b -> {
+ b.addClasspathClasses(MemberRebindingClasspathSplitTestClasses.getA());
+ },
+ b -> {
+ b.addProgramClassFileData(
+ transformer(MemberRebindingClasspathSplitTestClasses.getB())
+ .removeMethodsWithName("m")
+ .transform());
+ },
+ b -> {
+ b.addRunClasspathClasses(MemberRebindingClasspathSplitTestClasses.getA());
+ })));
+ }
+
+ @Test
+ public void testR8() throws Exception {
+ testForR8(parameters)
+ .addInnerClasses(getClass())
+ .apply(split.addToClasspath)
+ .apply(split.addToProgrampath)
+ .allowStdoutMessages()
+ .addDontObfuscate()
+ .addDontOptimize()
+ .addKeepRules("-keep class " + Main.class.getTypeName() + " { void main(...); }")
+ .compile()
+ .apply(split.addToRunClasspath)
+ .run(parameters.getRuntime(), Main.class)
+ .assertSuccessWithOutputLines("A", "A");
+ }
+
+ public static class C extends MemberRebindingClasspathSplitTestClasses.B {
+ public String superm() {
+ return super.m();
+ }
+ }
+
+ public static class Main {
+ public static void main(String[] strArr) {
+ C c = new C();
+ System.out.println(c.m());
+ System.out.println(c.superm());
+ }
+ }
+}
diff --git a/src/test/java/com/android/tools/r8/memberrebinding/testclasses/MemberRebindingClasspathSplitTestClasses.java b/src/test/java/com/android/tools/r8/memberrebinding/testclasses/MemberRebindingClasspathSplitTestClasses.java
new file mode 100644
index 0000000..5c0d27c
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/memberrebinding/testclasses/MemberRebindingClasspathSplitTestClasses.java
@@ -0,0 +1,22 @@
+// Copyright (c) 2025, 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.memberrebinding.testclasses;
+
+public class MemberRebindingClasspathSplitTestClasses {
+ public static Class<?> getA() {
+ return A.class;
+ }
+
+ public static Class<?> getB() {
+ return B.class;
+ }
+
+ static class A {
+ public String m() {
+ return "A";
+ }
+ }
+
+ public static class B extends A {}
+}
diff --git a/src/test/java/com/android/tools/r8/regress/b426351560/Regress426351560Test.java b/src/test/java/com/android/tools/r8/regress/b426351560/Regress426351560Test.java
index 9da5d33..c11e2a0 100644
--- a/src/test/java/com/android/tools/r8/regress/b426351560/Regress426351560Test.java
+++ b/src/test/java/com/android/tools/r8/regress/b426351560/Regress426351560Test.java
@@ -31,8 +31,6 @@
@Test
public void testR8() throws Exception {
- // TODO(b/427887773)
- assumeFalse(parameters.isRandomPartialCompilation());
testForR8(parameters)
.addInnerClasses(Regress426351560TestClasses.class, getClass())
.addKeepRules("-keep class " + Main.class.getTypeName() + " { *; }")
diff --git a/src/test/java/com/android/tools/r8/regress/b426351560/Regress427887773Test.java b/src/test/java/com/android/tools/r8/regress/b426351560/Regress427887773Test.java
index 67019be..76b7c51 100644
--- a/src/test/java/com/android/tools/r8/regress/b426351560/Regress427887773Test.java
+++ b/src/test/java/com/android/tools/r8/regress/b426351560/Regress427887773Test.java
@@ -40,13 +40,7 @@
.compile()
.addRunClasspathClasses(Regress426351560TestClasses.getClasses())
.run(parameters.getRuntime(), Main.class)
- .applyIf(
- parameters.isDexRuntime()
- && parameters.getApiLevel().isGreaterThanOrEqualTo(AndroidApiLevel.N),
- rr -> rr.assertFailureWithErrorThatThrows(IllegalAccessError.class),
- // for jdk based tests: java.lang.VerifyError: Bad invokespecial instruction:
- // interface method reference is in an indirect superinterface.
- rr -> rr.assertFailureWithErrorThatThrows(VerifyError.class));
+ .assertSuccessWithOutputLines("Hello, world!");
} catch (CompilationFailedException e) {
if (parameters.isDexRuntime() && parameters.getApiLevel().isLessThan(AndroidApiLevel.N)) {
assertTrue(e.getCause() instanceof AssertionError);
