Reland "Remove invalid nest check from isValidVirtualTarget predicate."
Bug: 146620831
Bug: 145187573
Bug: 145187969
This reverts commit ef8e73e75d2be92fb87f171d68dbf36a2cea333d.
Change-Id: Ic3c5c9423fa66026f5a88cc204f6e82cc010dcb5
diff --git a/src/main/java/com/android/tools/r8/graph/ResolutionResult.java b/src/main/java/com/android/tools/r8/graph/ResolutionResult.java
index 3080960..2c60a43 100644
--- a/src/main/java/com/android/tools/r8/graph/ResolutionResult.java
+++ b/src/main/java/com/android/tools/r8/graph/ResolutionResult.java
@@ -6,7 +6,6 @@
import static com.android.tools.r8.graph.DexProgramClass.asProgramClassOrNull;
import com.android.tools.r8.errors.CompilationError;
-import com.android.tools.r8.utils.InternalOptions;
import com.android.tools.r8.utils.SetUtils;
import com.google.common.collect.Sets;
import java.util.Collection;
@@ -57,8 +56,12 @@
public abstract boolean isAccessibleForVirtualDispatchFrom(
DexProgramClass context, AppInfoWithSubtyping appInfo);
- public abstract boolean isValidVirtualTarget(InternalOptions options);
+ // TODO(b/145187573): Remove this and use proper access checks.
+ @Deprecated
+ public abstract boolean isValidVirtualTarget();
+ // TODO(b/145187573): Remove this and use proper access checks.
+ @Deprecated
public abstract boolean isValidVirtualTargetForDynamicDispatch();
/** Lookup the single target of an invoke-special on this resolution result if possible. */
@@ -87,10 +90,10 @@
private final DexClass resolvedHolder;
private final DexEncodedMethod resolvedMethod;
- public static boolean isValidVirtualTarget(InternalOptions options, DexEncodedMethod target) {
- return options.canUseNestBasedAccess()
- ? (!target.accessFlags.isStatic() && !target.accessFlags.isConstructor())
- : target.isVirtualMethod();
+ // TODO(b/145187573): Remove this and use proper access checks.
+ @Deprecated
+ public static boolean isValidVirtualTarget(DexEncodedMethod target) {
+ return !target.accessFlags.isStatic() && !target.accessFlags.isConstructor();
}
public SingleResolutionResult(
@@ -141,8 +144,8 @@
}
@Override
- public boolean isValidVirtualTarget(InternalOptions options) {
- return isValidVirtualTarget(options, resolvedMethod);
+ public boolean isValidVirtualTarget() {
+ return isValidVirtualTarget(resolvedMethod);
}
@Override
@@ -301,7 +304,7 @@
@Override
// TODO(b/140204899): Leverage refined receiver type if available.
public Set<DexEncodedMethod> lookupVirtualTargets(AppInfoWithSubtyping appInfo) {
- assert isValidVirtualTarget(appInfo.app().options);
+ assert isValidVirtualTarget();
// First add the target for receiver type method.type.
DexEncodedMethod encodedMethod = getSingleTarget();
Set<DexEncodedMethod> result = SetUtils.newIdentityHashSet(encodedMethod);
@@ -325,7 +328,7 @@
@Override
// TODO(b/140204899): Leverage refined receiver type if available.
public Set<DexEncodedMethod> lookupInterfaceTargets(AppInfoWithSubtyping appInfo) {
- assert isValidVirtualTarget(appInfo.app().options);
+ assert isValidVirtualTarget();
Set<DexEncodedMethod> result = Sets.newIdentityHashSet();
if (isSingleResolution()) {
// Add default interface methods to the list of targets.
@@ -451,7 +454,7 @@
}
@Override
- public boolean isValidVirtualTarget(InternalOptions options) {
+ public boolean isValidVirtualTarget() {
return true;
}
@@ -490,7 +493,7 @@
}
@Override
- public boolean isValidVirtualTarget(InternalOptions options) {
+ public boolean isValidVirtualTarget() {
return false;
}
diff --git a/src/main/java/com/android/tools/r8/ir/conversion/CallGraphBuilderBase.java b/src/main/java/com/android/tools/r8/ir/conversion/CallGraphBuilderBase.java
index c7939b5..0e6d2a3 100644
--- a/src/main/java/com/android/tools/r8/ir/conversion/CallGraphBuilderBase.java
+++ b/src/main/java/com/android/tools/r8/ir/conversion/CallGraphBuilderBase.java
@@ -180,7 +180,7 @@
method -> {
ResolutionResult resolution =
appView.appInfo().resolveMethod(method.holder, method, isInterface);
- if (resolution.isValidVirtualTarget(appView.options())) {
+ if (resolution.isValidVirtualTarget()) {
return resolution.lookupVirtualDispatchTargets(isInterface, appView.appInfo());
}
return null;
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/CallSiteOptimizationInfoPropagator.java b/src/main/java/com/android/tools/r8/ir/optimize/CallSiteOptimizationInfoPropagator.java
index e83a17a..133eff3 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/CallSiteOptimizationInfoPropagator.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/CallSiteOptimizationInfoPropagator.java
@@ -88,7 +88,7 @@
ResolutionResult resolutionResult =
appView.appInfo().resolveMethod(invokedMethod.holder, invokedMethod);
// For virtual and interface calls, proceed on valid results only (since it's enforced).
- if (!resolutionResult.isValidVirtualTarget(appView.options())) {
+ if (!resolutionResult.isValidVirtualTarget()) {
continue;
}
// If the resolution ended up with a single target, check if it is a library override.
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/InliningConstraints.java b/src/main/java/com/android/tools/r8/ir/optimize/InliningConstraints.java
index 4977d4c..e0219c5 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/InliningConstraints.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/InliningConstraints.java
@@ -352,7 +352,7 @@
// resolution result.
ResolutionResult resolutionResult =
appView.appInfo().resolveMethod(method.holder, method, isInterface);
- if (!resolutionResult.isValidVirtualTarget(appView.options())) {
+ if (!resolutionResult.isValidVirtualTarget()) {
return ConstraintWithTarget.NEVER;
}
diff --git a/src/main/java/com/android/tools/r8/shaking/AppInfoWithLiveness.java b/src/main/java/com/android/tools/r8/shaking/AppInfoWithLiveness.java
index 25a7b15..4e248d9 100644
--- a/src/main/java/com/android/tools/r8/shaking/AppInfoWithLiveness.java
+++ b/src/main/java/com/android/tools/r8/shaking/AppInfoWithLiveness.java
@@ -979,7 +979,7 @@
private DexEncodedMethod validateSingleVirtualTarget(
DexEncodedMethod singleTarget, DexEncodedMethod resolutionResult) {
- assert SingleResolutionResult.isValidVirtualTarget(options(), resolutionResult);
+ assert SingleResolutionResult.isValidVirtualTarget(resolutionResult);
if (singleTarget == null || singleTarget == DexEncodedMethod.SENTINEL) {
return null;
@@ -996,7 +996,7 @@
private boolean isInvalidSingleVirtualTarget(
DexEncodedMethod singleTarget, DexEncodedMethod resolutionResult) {
- assert SingleResolutionResult.isValidVirtualTarget(options(), resolutionResult);
+ assert SingleResolutionResult.isValidVirtualTarget(resolutionResult);
// Art978_virtual_interfaceTest correctly expects an IncompatibleClassChangeError exception
// at runtime.
return !singleTarget.accessFlags.isAtLeastAsVisibleAs(resolutionResult.accessFlags);
@@ -1081,7 +1081,7 @@
// First get the target for the holder type.
ResolutionResult topMethod = resolveMethodOnClass(holder, method);
// We might hit none or multiple targets. Both make this fail at runtime.
- if (!topMethod.isSingleResolution() || !topMethod.isValidVirtualTarget(options())) {
+ if (!topMethod.isSingleResolution() || !topMethod.isValidVirtualTarget()) {
method.setSingleVirtualMethodCache(refinedReceiverType, null);
return 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 4b412cf..1fa2300 100644
--- a/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
+++ b/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
@@ -1658,7 +1658,7 @@
}
private void markResolutionAsLive(DexClass libraryClass, ResolutionResult resolution) {
- if (resolution.isValidVirtualTarget(options)) {
+ if (resolution.isValidVirtualTarget()) {
DexEncodedMethod target = resolution.getSingleTarget();
DexProgramClass targetHolder = getProgramClassOrNull(target.method.holder);
if (targetHolder != null
@@ -2004,7 +2004,7 @@
resolution = findAndMarkResolutionTarget(method, interfaceInvoke, reason);
virtualTargetsMarkedAsReachable.put(method, resolution);
if (resolution.isUnresolved()
- || !SingleResolutionResult.isValidVirtualTarget(options, resolution.method)) {
+ || !SingleResolutionResult.isValidVirtualTarget(resolution.method)) {
// There is no valid resolution, so any call will lead to a runtime exception.
return;
}
@@ -2035,7 +2035,7 @@
MarkedResolutionTarget reason,
BiPredicate<DexProgramClass, DexEncodedMethod> possibleTargetsFilter,
DexEncodedMethod encodedPossibleTarget) {
- assert encodedPossibleTarget.isVirtualMethod() || options.canUseNestBasedAccess();
+ assert encodedPossibleTarget.isVirtualMethod() || encodedPossibleTarget.isPrivateMethod();
assert !encodedPossibleTarget.isAbstract();
DexMethod possibleTarget = encodedPossibleTarget.method;
DexProgramClass clazz = getProgramClassOrNull(possibleTarget.holder);
diff --git a/src/main/java/com/android/tools/r8/shaking/RootSetBuilder.java b/src/main/java/com/android/tools/r8/shaking/RootSetBuilder.java
index b86857a..bd05dc3 100644
--- a/src/main/java/com/android/tools/r8/shaking/RootSetBuilder.java
+++ b/src/main/java/com/android/tools/r8/shaking/RootSetBuilder.java
@@ -533,8 +533,7 @@
}
ResolutionResult resolutionResult =
appView.appInfo().resolveMethod(originalClazz, method.method);
- if (!resolutionResult.isValidVirtualTarget(appView.options())
- || !resolutionResult.isSingleResolution()) {
+ if (!resolutionResult.isValidVirtualTarget() || !resolutionResult.isSingleResolution()) {
return;
}
DexEncodedMethod methodToKeep = resolutionResult.getSingleTarget();
diff --git a/src/test/java/com/android/tools/r8/resolution/PrivateInvokeVirtualTest.java b/src/test/java/com/android/tools/r8/resolution/PrivateInvokeVirtualTest.java
new file mode 100644
index 0000000..543ecad
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/resolution/PrivateInvokeVirtualTest.java
@@ -0,0 +1,95 @@
+// Copyright (c) 2019, 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.resolution;
+
+import static org.junit.Assert.assertEquals;
+
+import com.android.tools.r8.CompilationFailedException;
+import com.android.tools.r8.TestBase;
+import com.android.tools.r8.TestParameters;
+import com.android.tools.r8.TestParametersCollection;
+import java.io.IOException;
+import java.util.concurrent.ExecutionException;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameters;
+import org.objectweb.asm.Opcodes;
+
+@RunWith(Parameterized.class)
+public class PrivateInvokeVirtualTest extends TestBase {
+
+ private final TestParameters parameters;
+
+ @Parameters(name = "{0}")
+ public static TestParametersCollection data() {
+ return getTestParameters().withAllRuntimesAndApiLevels().build();
+ }
+
+ public PrivateInvokeVirtualTest(TestParameters parameters) {
+ this.parameters = parameters;
+ }
+
+ public static class UInt implements Comparable<UInt> {
+
+ private final int val;
+
+ public UInt(int val) {
+ this.val = val;
+ }
+
+ private int compareToHelper(int i) {
+ return Integer.compare(val, i);
+ }
+
+ @Override
+ public int compareTo(UInt o) {
+ return compareToHelper(o.val);
+ }
+ }
+
+ public static class Main {
+
+ public static void main(String[] args) {
+ System.out.println(new UInt(3).compareTo(new UInt(args.length)));
+ }
+ }
+
+ @Test
+ public void testRuntime() throws IOException, CompilationFailedException, ExecutionException {
+ testForRuntime(parameters)
+ .addProgramClassFileData(getUIntWithTransformedInvoke())
+ .addProgramClasses(Main.class)
+ .run(parameters.getRuntime(), Main.class)
+ .assertSuccessWithOutputLines("1");
+ }
+
+ @Test
+ public void testPrivateInvokeVirtual()
+ throws IOException, CompilationFailedException, ExecutionException {
+ testForR8(parameters.getBackend())
+ .addProgramClasses(Main.class)
+ .addProgramClassFileData(getUIntWithTransformedInvoke())
+ .addKeepMainRule(Main.class)
+ .setMinApi(parameters.getApiLevel())
+ .run(parameters.getRuntime(), Main.class)
+ .assertSuccessWithOutputLines("1");
+ }
+
+ private byte[] getUIntWithTransformedInvoke() throws IOException {
+ return transformer(UInt.class)
+ .transformMethodInsnInMethod(
+ "compareTo",
+ (opcode, owner, name, descriptor, isInterface, continuation) -> {
+ if (name.contains("compareToHelper")) {
+ assertEquals(Opcodes.INVOKESPECIAL, opcode);
+ continuation.apply(Opcodes.INVOKEVIRTUAL, owner, name, descriptor, isInterface);
+ } else {
+ continuation.apply(opcode, owner, name, descriptor, isInterface);
+ }
+ })
+ .transform();
+ }
+}
diff --git a/src/test/java/com/android/tools/r8/resolution/SingleTargetLookupTest.java b/src/test/java/com/android/tools/r8/resolution/SingleTargetLookupTest.java
index b1bcb52..27c7724 100644
--- a/src/test/java/com/android/tools/r8/resolution/SingleTargetLookupTest.java
+++ b/src/test/java/com/android/tools/r8/resolution/SingleTargetLookupTest.java
@@ -228,7 +228,7 @@
Assert.assertNotNull(
appInfo.resolveMethod(toType(invokeReceiver, appInfo), method).getSingleTarget());
ResolutionResult resolutionResult = appInfo.resolveMethod(method.holder, method);
- if (resolutionResult.isValidVirtualTarget(appInfo.app().options)) {
+ if (resolutionResult.isValidVirtualTarget()) {
Set<DexEncodedMethod> targets = resolutionResult.lookupVirtualTargets(appInfo);
Set<DexType> targetHolders =
targets.stream().map(m -> m.method.holder).collect(Collectors.toSet());
diff --git a/src/test/java/com/android/tools/r8/resolution/VirtualOverrideOfStaticMethodWithVirtualParentInterfaceTest.java b/src/test/java/com/android/tools/r8/resolution/VirtualOverrideOfStaticMethodWithVirtualParentInterfaceTest.java
index 63aa76f..82452a8 100644
--- a/src/test/java/com/android/tools/r8/resolution/VirtualOverrideOfStaticMethodWithVirtualParentInterfaceTest.java
+++ b/src/test/java/com/android/tools/r8/resolution/VirtualOverrideOfStaticMethodWithVirtualParentInterfaceTest.java
@@ -122,7 +122,7 @@
appInfo.resolveMethodOnInterface(methodOnB.holder, methodOnB);
DexEncodedMethod resolved = resolutionResult.getSingleTarget();
assertEquals(methodOnB, resolved.method);
- assertFalse(resolutionResult.isValidVirtualTarget(appInfo.app().options));
+ assertFalse(resolutionResult.isValidVirtualTarget());
DexEncodedMethod singleVirtualTarget =
appInfo.lookupSingleInterfaceTarget(methodOnB, methodOnB.holder);
Assert.assertNull(singleVirtualTarget);
@@ -133,7 +133,7 @@
ResolutionResult resolutionResult = appInfo.resolveMethodOnInterface(methodOnB.holder, methodOnB);
DexEncodedMethod resolved = resolutionResult.getSingleTarget();
assertEquals(methodOnB, resolved.method);
- assertFalse(resolutionResult.isValidVirtualTarget(appInfo.app().options));
+ assertFalse(resolutionResult.isValidVirtualTarget());
}
@Test
diff --git a/src/test/java/com/android/tools/r8/resolution/VirtualOverrideOfStaticMethodWithVirtualParentTest.java b/src/test/java/com/android/tools/r8/resolution/VirtualOverrideOfStaticMethodWithVirtualParentTest.java
index 22a12f2..702eca9 100644
--- a/src/test/java/com/android/tools/r8/resolution/VirtualOverrideOfStaticMethodWithVirtualParentTest.java
+++ b/src/test/java/com/android/tools/r8/resolution/VirtualOverrideOfStaticMethodWithVirtualParentTest.java
@@ -168,7 +168,7 @@
ResolutionResult resolutionResult = appInfo.resolveMethodOnClass(methodOnB.holder, methodOnB);
DexEncodedMethod resolved = resolutionResult.getSingleTarget();
assertEquals(methodOnA, resolved.method);
- assertFalse(resolutionResult.isValidVirtualTarget(appInfo.app().options));
+ assertFalse(resolutionResult.isValidVirtualTarget());
DexEncodedMethod singleVirtualTarget =
appInfo.lookupSingleVirtualTarget(methodOnB, methodOnB.holder);
Assert.assertNull(singleVirtualTarget);
@@ -179,7 +179,7 @@
ResolutionResult resolutionResult = appInfo.resolveMethodOnClass(methodOnB.holder, methodOnB);
DexEncodedMethod resolved = resolutionResult.getSingleTarget();
assertEquals(methodOnA, resolved.method);
- assertFalse(resolutionResult.isValidVirtualTarget(appInfo.app().options));
+ assertFalse(resolutionResult.isValidVirtualTarget());
}
@Test
diff --git a/src/test/java/com/android/tools/r8/resolution/access/NestVirtualMethodAccessTest.java b/src/test/java/com/android/tools/r8/resolution/access/NestVirtualMethodAccessTest.java
index 69cdad5..f1c5fa1 100644
--- a/src/test/java/com/android/tools/r8/resolution/access/NestVirtualMethodAccessTest.java
+++ b/src/test/java/com/android/tools/r8/resolution/access/NestVirtualMethodAccessTest.java
@@ -92,16 +92,7 @@
.setMinApi(parameters.getApiLevel())
.addKeepMainRule(Main.class)
.run(parameters.getRuntime(), Main.class)
- .apply(
- result -> {
- if (parameters.isDexRuntime() && inSameNest) {
- // TODO(b/145187969): R8 incorrectly compiles the nest based access away.
- result.assertFailureWithErrorThatMatches(
- containsString(NullPointerException.class.getName()));
- } else {
- checkExpectedResult(result);
- }
- });
+ .apply(this::checkExpectedResult);
}
private void checkExpectedResult(TestRunResult<?> result) {
diff --git a/src/test/java/com/android/tools/r8/resolution/access/SelfVirtualMethodAccessTest.java b/src/test/java/com/android/tools/r8/resolution/access/SelfVirtualMethodAccessTest.java
index a997622..f2f3309 100644
--- a/src/test/java/com/android/tools/r8/resolution/access/SelfVirtualMethodAccessTest.java
+++ b/src/test/java/com/android/tools/r8/resolution/access/SelfVirtualMethodAccessTest.java
@@ -3,7 +3,6 @@
// BSD-style license that can be found in the LICENSE file.
package com.android.tools.r8.resolution.access;
-import static org.hamcrest.CoreMatchers.containsString;
import static org.junit.Assert.assertTrue;
import com.android.tools.r8.TestBase;
@@ -77,16 +76,7 @@
.setMinApi(parameters.getApiLevel())
.addKeepMainRule(Main.class)
.run(parameters.getRuntime(), Main.class)
- .apply(
- result -> {
- if (parameters.isCfRuntime()) {
- result.assertSuccessWithOutput(EXPECTED);
- } else {
- // TODO(b/145187969): R8 compiles an incorrect program.
- result.assertFailureWithErrorThatMatches(
- containsString(NullPointerException.class.getName()));
- }
- });
+ .assertSuccessWithOutput(EXPECTED);
}
static class A {
