Remove invalid nest check from isValidVirtualTarget predicate.
A virtual dispatch to a private method on self is valid regardless of nest
access. This would be correct once the proper access check routines are used,
but since the current checks are not context aware we can only approximate this
by allowing all dispatch to private methods.
Bug: 146620831
Bug: 145187573
Change-Id: I11483f906d5774becea31533e2f3428ca74ec9c2
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
