Check for resolved static targets when inserting forwarding methods
Bug: 171550305
Bug: 182189123
Change-Id: I96c34562f617c83e6c06535a038a42d7b59728e4
diff --git a/src/main/java/com/android/tools/r8/ir/desugar/ClassProcessor.java b/src/main/java/com/android/tools/r8/ir/desugar/ClassProcessor.java
index 630a67d..04b702a 100644
--- a/src/main/java/com/android/tools/r8/ir/desugar/ClassProcessor.java
+++ b/src/main/java/com/android/tools/r8/ir/desugar/ClassProcessor.java
@@ -27,6 +27,7 @@
import com.android.tools.r8.ir.synthetic.ExceptionThrowingSourceCode;
import com.android.tools.r8.ir.synthetic.SynthesizedCode;
import com.android.tools.r8.position.MethodPosition;
+import com.android.tools.r8.utils.BooleanBox;
import com.android.tools.r8.utils.IterableUtils;
import com.android.tools.r8.utils.MethodSignatureEquivalence;
import com.android.tools.r8.utils.WorkList;
@@ -533,27 +534,44 @@
// the 'addForward' call-back is called with the target of the forward.
private void resolveForwardForSignature(
DexClass clazz, DexMethod method, Consumer<DexClassAndMethod> addForward) {
- // Resolve the default method with base type as the symbolic holder as call sites are not known.
- // The dispatch target is then looked up from the possible "instance" class.
- // Doing so can cause an invalid invoke to become valid (at runtime resolution at a subtype
- // might have failed which is hidden by the insertion of the forward method). However, not doing
- // so could cause valid dispatches to become invalid by resolving to private overrides.
AppInfoWithClassHierarchy appInfo = appView.appInfoForDesugaring();
- DexClassAndMethod virtualDispatchTarget =
- appInfo
- .resolveMethodOnInterface(method.holder, method)
- .lookupVirtualDispatchTarget(clazz, appInfo);
- if (virtualDispatchTarget == null) {
- // If no target is found due to multiple default method targets, preserve ICCE behavior.
- ResolutionResult resolutionFromSubclass = appInfo.resolveMethodOn(clazz, method);
- if (resolutionFromSubclass.isIncompatibleClassChangeErrorResult()) {
- addICCEThrowingMethod(method, clazz);
+ ResolutionResult resolutionResult = appInfo.resolveMethodOn(clazz, method);
+ if (resolutionResult.isFailedResolution()
+ || resolutionResult.asSuccessfulMemberResolutionResult().getResolvedMember().isStatic()) {
+ // When doing resolution we may find a static or private targets and bubble up the failed
+ // resolution to preserve ICCE even though the resolution actually succeeded, ie. finding a
+ // method with the same name and descriptor. For invoke-virtual and invoke-interface, the
+ // selected method will only consider instance methods.
+ BooleanBox staticTarget = new BooleanBox(true);
+ if (resolutionResult.isFailedResolution()) {
+ resolutionResult
+ .asFailedResolution()
+ .forEachFailureDependency(target -> staticTarget.and(target.isStatic()));
+ } else if (resolutionResult.isSuccessfulMemberResolutionResult()) {
+ staticTarget.set(
+ resolutionResult.asSuccessfulMemberResolutionResult().getResolvedMember().isStatic());
+ }
+ if (staticTarget.isAssigned() && staticTarget.isTrue()) {
+ resolutionResult = appInfo.resolveMethodOnInterface(method.holder, method);
+ }
+ if (resolutionResult.isFailedResolution()) {
+ if (resolutionResult.isIncompatibleClassChangeErrorResult()) {
+ addICCEThrowingMethod(method, clazz);
+ return;
+ }
+ if (resolutionResult.isNoSuchMethodErrorResult(clazz, appInfo)) {
+ addNoSuchMethodErrorThrowingMethod(method, clazz);
+ return;
+ }
+ assert resolutionResult.isIllegalAccessErrorResult(clazz, appInfo);
+ addIllegalAccessErrorThrowingMethod(method, clazz);
return;
}
- assert resolutionFromSubclass.isFailedResolution()
- || resolutionFromSubclass.getSingleTarget().isPrivateMethod();
- return;
}
+ assert resolutionResult.isSuccessfulMemberResolutionResult();
+ DexClassAndMethod virtualDispatchTarget =
+ resolutionResult.lookupVirtualDispatchTarget(clazz, appInfo);
+ assert virtualDispatchTarget != null;
// Don't forward if the target is explicitly marked as 'dont-rewrite'
if (dontRewrite(virtualDispatchTarget)) {
@@ -612,6 +630,18 @@
}
private void addICCEThrowingMethod(DexMethod method, DexClass clazz) {
+ addThrowingMethod(method, clazz, dexItemFactory.icceType);
+ }
+
+ private void addIllegalAccessErrorThrowingMethod(DexMethod method, DexClass clazz) {
+ addThrowingMethod(method, clazz, dexItemFactory.illegalAccessErrorType);
+ }
+
+ private void addNoSuchMethodErrorThrowingMethod(DexMethod method, DexClass clazz) {
+ addThrowingMethod(method, clazz, dexItemFactory.noSuchMethodErrorType);
+ }
+
+ private void addThrowingMethod(DexMethod method, DexClass clazz, DexType errorType) {
if (!clazz.isProgramClass()) {
return;
}
@@ -625,8 +655,7 @@
ParameterAnnotationsList.empty(),
new SynthesizedCode(
callerPosition ->
- new ExceptionThrowingSourceCode(
- clazz.type, method, callerPosition, dexItemFactory.icceType)),
+ new ExceptionThrowingSourceCode(clazz.type, method, callerPosition, errorType)),
true);
addSyntheticMethod(clazz.asProgramClass(), newEncodedMethod);
}
diff --git a/src/main/java/com/android/tools/r8/utils/BooleanBox.java b/src/main/java/com/android/tools/r8/utils/BooleanBox.java
index eff5807..5ad9494 100644
--- a/src/main/java/com/android/tools/r8/utils/BooleanBox.java
+++ b/src/main/java/com/android/tools/r8/utils/BooleanBox.java
@@ -9,6 +9,7 @@
public class BooleanBox {
private boolean value;
+ private boolean assigned = false;
public BooleanBox() {}
@@ -39,6 +40,7 @@
}
public void set(boolean value) {
+ assigned = true;
this.value = value;
}
@@ -47,10 +49,14 @@
}
public void and(boolean value) {
- this.value = value && this.value;
+ set(value && this.value);
}
public void or(boolean value) {
- this.value = value || this.value;
+ set(value || this.value);
+ }
+
+ public boolean isAssigned() {
+ return assigned;
}
}
diff --git a/src/test/java/com/android/tools/r8/desugar/DefaultLambdaMethodWithPrivateSuperClassTest.java b/src/test/java/com/android/tools/r8/desugar/DefaultLambdaMethodWithPrivateSuperClassTest.java
index cbd1fb8..6ba2e12 100644
--- a/src/test/java/com/android/tools/r8/desugar/DefaultLambdaMethodWithPrivateSuperClassTest.java
+++ b/src/test/java/com/android/tools/r8/desugar/DefaultLambdaMethodWithPrivateSuperClassTest.java
@@ -48,11 +48,12 @@
.addInnerClasses(getClass())
.setMinApi(parameters.getApiLevel())
.run(parameters.getRuntime(), Main.class)
- // TODO(b/171550305): Should be illegal access for DEX.
.applyIf(
- parameters.isDexRuntime() || parameters.getApiLevel().isEqualTo(AndroidApiLevel.B),
- r -> r.assertSuccessWithOutputLines("interface"),
- r -> r.assertFailureWithErrorThatThrows(IllegalAccessError.class));
+ parameters.isCfRuntime()
+ || parameters.getApiLevel().isLessThanOrEqualTo(AndroidApiLevel.M),
+ r -> r.assertFailureWithErrorThatThrows(IllegalAccessError.class),
+ // TODO(b/152199517): Should be illegal access for DEX.
+ r -> r.assertSuccessWithOutputLines("interface"));
}
interface Named {
diff --git a/src/test/java/com/android/tools/r8/desugar/DefaultLambdaMethodWithPublicSuperClassTest.java b/src/test/java/com/android/tools/r8/desugar/DefaultLambdaMethodWithPublicSuperClassTest.java
index 11bc898..a439984 100644
--- a/src/test/java/com/android/tools/r8/desugar/DefaultLambdaMethodWithPublicSuperClassTest.java
+++ b/src/test/java/com/android/tools/r8/desugar/DefaultLambdaMethodWithPublicSuperClassTest.java
@@ -4,6 +4,8 @@
package com.android.tools.r8.desugar;
+import static org.junit.Assume.assumeTrue;
+
import com.android.tools.r8.TestBase;
import com.android.tools.r8.TestParameters;
import com.android.tools.r8.TestParametersCollection;
@@ -20,7 +22,7 @@
@Parameters(name = "{0}")
public static TestParametersCollection data() {
- return getTestParameters().withCfRuntimes().withAllApiLevelsAlsoForCf().build();
+ return getTestParameters().withAllRuntimes().withAllApiLevelsAlsoForCf().build();
}
public DefaultLambdaMethodWithPublicSuperClassTest(TestParameters parameters) {
@@ -29,6 +31,7 @@
@Test
public void testJvm() throws Exception {
+ assumeTrue(parameters.isCfRuntime());
testForJvm()
.addInnerClasses(getClass())
.run(parameters.getRuntime(), Main.class)
diff --git a/src/test/java/com/android/tools/r8/graph/invokevirtual/InvokeVirtualPrivateBaseWithDefaultDirectInvokeTest.java b/src/test/java/com/android/tools/r8/graph/invokevirtual/InvokeVirtualPrivateBaseWithDefaultDirectInvokeTest.java
index 86204ee..f989728 100644
--- a/src/test/java/com/android/tools/r8/graph/invokevirtual/InvokeVirtualPrivateBaseWithDefaultDirectInvokeTest.java
+++ b/src/test/java/com/android/tools/r8/graph/invokevirtual/InvokeVirtualPrivateBaseWithDefaultDirectInvokeTest.java
@@ -7,6 +7,7 @@
import static org.junit.Assume.assumeTrue;
import com.android.tools.r8.NoVerticalClassMerging;
+import com.android.tools.r8.SingleTestRunResult;
import com.android.tools.r8.TestBase;
import com.android.tools.r8.TestParameters;
import com.android.tools.r8.TestParametersCollection;
@@ -21,7 +22,6 @@
public class InvokeVirtualPrivateBaseWithDefaultDirectInvokeTest extends TestBase {
private final TestParameters parameters;
- private final String INVALID_EXPECTED = "I::foo";
@Parameters(name = "{0}")
public static TestParametersCollection data() {
@@ -40,9 +40,7 @@
.run(parameters.getRuntime(), Main.class)
.applyIf(
parameters.isCfRuntime(CfVm.JDK11),
- // TODO(B/182148338): Figure out why JVM 11 has changed resolution for interface
- // lookups.
- r -> r.assertSuccessWithOutputLines(INVALID_EXPECTED),
+ r -> r.assertSuccessWithOutputLines("I::foo"),
r -> r.assertFailureWithErrorThatThrows(IllegalAccessError.class));
}
@@ -52,14 +50,7 @@
.addInnerClasses(getClass())
.setMinApi(parameters.getApiLevel())
.run(parameters.getRuntime(), Main.class)
- // TODO(B/182148338): Figure out why JVM 11 has changed resolution for interface lookups.
- // TODO(b/171550305): Should be illegal access for DEX.
- .applyIf(
- parameters.isDexRuntime()
- || parameters.getApiLevel().isEqualTo(AndroidApiLevel.B)
- || parameters.isCfRuntime(CfVm.JDK11),
- r -> r.assertSuccessWithOutputLines(INVALID_EXPECTED),
- r -> r.assertFailureWithErrorThatThrows(IllegalAccessError.class));
+ .apply(this::assertResultIsCorrect);
}
@Test
@@ -70,10 +61,23 @@
.addKeepClassAndMembersRules(I.class)
.enableNoVerticalClassMergingAnnotations()
.setMinApi(parameters.getApiLevel())
- .compile()
.run(parameters.getRuntime(), Main.class)
- // TODO(b/171550305): Should be illegal access.
- .assertSuccessWithOutputLines(INVALID_EXPECTED);
+ // TODO(b/182189123): This should have the same behavior as D8.
+ .assertSuccessWithOutputLines("I::foo");
+ }
+
+ public void assertResultIsCorrect(SingleTestRunResult<?> result) {
+ if (parameters.isCfRuntime(CfVm.JDK11)
+ && parameters.getApiLevel().isGreaterThan(AndroidApiLevel.M)) {
+ result.assertSuccessWithOutputLines("I::foo");
+ return;
+ }
+ // TODO(b/152199517): Should be illegal access for DEX.
+ if (parameters.isDexRuntime() && parameters.getApiLevel().isGreaterThan(AndroidApiLevel.M)) {
+ result.assertSuccessWithOutputLines("I::foo");
+ return;
+ }
+ result.assertFailureWithErrorThatThrows(IllegalAccessError.class);
}
@NoVerticalClassMerging
diff --git a/src/test/java/com/android/tools/r8/graph/invokevirtual/InvokeVirtualPrivateBaseWithDefaultTest.java b/src/test/java/com/android/tools/r8/graph/invokevirtual/InvokeVirtualPrivateBaseWithDefaultTest.java
index 381eb18..9c9926d 100644
--- a/src/test/java/com/android/tools/r8/graph/invokevirtual/InvokeVirtualPrivateBaseWithDefaultTest.java
+++ b/src/test/java/com/android/tools/r8/graph/invokevirtual/InvokeVirtualPrivateBaseWithDefaultTest.java
@@ -20,7 +20,6 @@
public class InvokeVirtualPrivateBaseWithDefaultTest extends TestBase {
private final TestParameters parameters;
- private final String INVALID_EXPECTED = "I::foo";
@Parameters(name = "{0}")
public static TestParametersCollection data() {
@@ -46,11 +45,12 @@
.addInnerClasses(getClass())
.setMinApi(parameters.getApiLevel())
.run(parameters.getRuntime(), Main.class)
- // TODO(b/171550305): Should be illegal access for DEX.
.applyIf(
- parameters.isDexRuntime() || parameters.getApiLevel().isEqualTo(AndroidApiLevel.B),
- r -> r.assertSuccessWithOutputLines(INVALID_EXPECTED),
- r -> r.assertFailureWithErrorThatThrows(IllegalAccessError.class));
+ parameters.isCfRuntime()
+ || parameters.getApiLevel().isLessThanOrEqualTo(AndroidApiLevel.M),
+ r -> r.assertFailureWithErrorThatThrows(IllegalAccessError.class),
+ // TODO(b/152199517): Should be illegal access for DEX.
+ r -> r.assertSuccessWithOutputLines("I::foo"));
}
@Test
@@ -63,11 +63,12 @@
.setMinApi(parameters.getApiLevel())
.compile()
.run(parameters.getRuntime(), Main.class)
- // TODO(b/171550305): Should be illegal access.
.applyIf(
- parameters.isDexRuntime(),
- r -> r.assertSuccessWithOutputLines(INVALID_EXPECTED),
- r -> r.assertFailureWithErrorThatThrows(IllegalAccessError.class));
+ parameters.isCfRuntime()
+ || parameters.getApiLevel().isLessThanOrEqualTo(AndroidApiLevel.M),
+ r -> r.assertFailureWithErrorThatThrows(IllegalAccessError.class),
+ // TODO(b/152199517): Should be illegal access for DEX.
+ r -> r.assertSuccessWithOutputLines("I::foo"));
}
@NoVerticalClassMerging
