Fix incorrect visibility check in inliner
Bug: 142239408
Change-Id: I33f7f73b13810e452484e5a40d549621f55166af
diff --git a/src/main/java/com/android/tools/r8/graph/AppInfo.java b/src/main/java/com/android/tools/r8/graph/AppInfo.java
index e2ad67e..773f4ad 100644
--- a/src/main/java/com/android/tools/r8/graph/AppInfo.java
+++ b/src/main/java/com/android/tools/r8/graph/AppInfo.java
@@ -10,6 +10,7 @@
import com.android.tools.r8.graph.ResolutionResult.NoSuchMethodResult;
import com.android.tools.r8.origin.Origin;
import com.android.tools.r8.shaking.AppInfoWithLiveness;
+import com.android.tools.r8.utils.InternalOptions;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableMap.Builder;
import com.google.common.collect.ImmutableSet;
@@ -45,6 +46,10 @@
copyMetadataFromPrevious(previous);
}
+ protected InternalOptions options() {
+ return app.options;
+ }
+
public void copyMetadataFromPrevious(AppInfo previous) {
this.synthesizedClasses.putAll(previous.synthesizedClasses);
}
diff --git a/src/main/java/com/android/tools/r8/graph/AppInfoWithSubtyping.java b/src/main/java/com/android/tools/r8/graph/AppInfoWithSubtyping.java
index ec90464..460885c 100644
--- a/src/main/java/com/android/tools/r8/graph/AppInfoWithSubtyping.java
+++ b/src/main/java/com/android/tools/r8/graph/AppInfoWithSubtyping.java
@@ -325,7 +325,6 @@
assert checkIfObsolete();
if (!isSubtype(invocationContext, method.holder)) {
DexClass contextClass = definitionFor(invocationContext);
- isSubtype(invocationContext, method.holder);
throw new CompilationError(
"Illegal invoke-super to " + method.toSourceString() + " from class " + invocationContext,
contextClass != null ? contextClass.getOrigin() : Origin.unknown());
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/DefaultInliningOracle.java b/src/main/java/com/android/tools/r8/ir/optimize/DefaultInliningOracle.java
index 87350e7..263da6c 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/DefaultInliningOracle.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/DefaultInliningOracle.java
@@ -264,14 +264,6 @@
return false;
}
- DexClass holder = appView.definitionFor(singleTarget.method.holder);
- if (holder.isInterface()) {
- // Art978_virtual_interfaceTest correctly expects an IncompatibleClassChangeError exception at
- // runtime.
- whyAreYouNotInliningReporter.reportIncompatibleClassChangeError();
- return false;
- }
-
// Don't inline if target is synchronized.
if (singleTarget.accessFlags.isSynchronized()) {
whyAreYouNotInliningReporter.reportSynchronizedMethod();
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/inliner/NopWhyAreYouNotInliningReporter.java b/src/main/java/com/android/tools/r8/ir/optimize/inliner/NopWhyAreYouNotInliningReporter.java
index 818b8c2..b5c96d4 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/inliner/NopWhyAreYouNotInliningReporter.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/inliner/NopWhyAreYouNotInliningReporter.java
@@ -43,9 +43,6 @@
public void reportInaccessible() {}
@Override
- public void reportIncompatibleClassChangeError() {}
-
- @Override
public void reportIncorrectArity(int numberOfArguments, int arity) {}
@Override
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/inliner/WhyAreYouNotInliningReporter.java b/src/main/java/com/android/tools/r8/ir/optimize/inliner/WhyAreYouNotInliningReporter.java
index c560944..1e869eb 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/inliner/WhyAreYouNotInliningReporter.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/inliner/WhyAreYouNotInliningReporter.java
@@ -60,8 +60,6 @@
public abstract void reportInaccessible();
- public abstract void reportIncompatibleClassChangeError();
-
public abstract void reportIncorrectArity(int numberOfArguments, int arity);
public abstract void reportInlineeDoesNotHaveCode();
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/inliner/WhyAreYouNotInliningReporterImpl.java b/src/main/java/com/android/tools/r8/ir/optimize/inliner/WhyAreYouNotInliningReporterImpl.java
index 6bc09fd..dcc122b 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/inliner/WhyAreYouNotInliningReporterImpl.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/inliner/WhyAreYouNotInliningReporterImpl.java
@@ -88,11 +88,6 @@
}
@Override
- public void reportIncompatibleClassChangeError() {
- print("invoke may fail with an IncompatibleClassChangeError.");
- }
-
- @Override
public void reportIncorrectArity(int numberOfArguments, int arity) {
print(
"number of arguments ("
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 4762db0..e3cce51 100644
--- a/src/main/java/com/android/tools/r8/shaking/AppInfoWithLiveness.java
+++ b/src/main/java/com/android/tools/r8/shaking/AppInfoWithLiveness.java
@@ -880,6 +880,31 @@
}
}
+ private DexEncodedMethod validateSingleVirtualTarget(
+ DexEncodedMethod singleTarget, DexEncodedMethod resolutionResult) {
+ assert resolutionResult.isValidVirtualTarget(options());
+
+ if (singleTarget == null) {
+ return null;
+ }
+
+ // Art978_virtual_interfaceTest correctly expects an IncompatibleClassChangeError exception
+ // at runtime.
+ if (isInvalidSingleVirtualTarget(singleTarget, resolutionResult)) {
+ return null;
+ }
+
+ return singleTarget;
+ }
+
+ private boolean isInvalidSingleVirtualTarget(
+ DexEncodedMethod singleTarget, DexEncodedMethod resolutionResult) {
+ assert resolutionResult.isValidVirtualTarget(options());
+ // Art978_virtual_interfaceTest correctly expects an IncompatibleClassChangeError exception
+ // at runtime.
+ return !singleTarget.accessFlags.isAtLeastAsVisibleAs(resolutionResult.accessFlags);
+ }
+
/** For mapping invoke virtual instruction to single target method. */
public DexEncodedMethod lookupSingleVirtualTarget(DexMethod method, DexType invocationContext) {
assert checkIfObsolete();
@@ -903,11 +928,13 @@
if (receiverLowerBoundType != null) {
if (receiverLowerBoundType.getClassType() == refinedReceiverType) {
ResolutionResult resolutionResult = resolveMethod(method.holder, method, false);
- if (resolutionResult.isValidVirtualTargetForDynamicDispatch()) {
+ if (resolutionResult.hasSingleTarget()
+ && resolutionResult.isValidVirtualTargetForDynamicDispatch()) {
ResolutionResult refinedResolutionResult = resolveMethod(refinedReceiverType, method);
if (refinedResolutionResult.hasSingleTarget()
&& refinedResolutionResult.isValidVirtualTargetForDynamicDispatch()) {
- return refinedResolutionResult.asSingleTarget();
+ return validateSingleVirtualTarget(
+ refinedResolutionResult.asSingleTarget(), resolutionResult.asSingleTarget());
}
}
return null;
@@ -942,27 +969,28 @@
// 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.hasSingleTarget() || !topMethod.isValidVirtualTarget(app().options)) {
+ if (!topMethod.hasSingleTarget() || !topMethod.isValidVirtualTarget(options())) {
method.setSingleVirtualMethodCache(refinedReceiverType, null);
return null;
}
// Now, resolve the target with the refined receiver type.
- if (refinedReceiverIsStrictSubType) {
- topMethod = resolveMethodOnClass(refinedHolder, method);
- }
- DexEncodedMethod topSingleTarget = topMethod.asSingleTarget();
+ ResolutionResult refinedResolutionResult =
+ refinedReceiverIsStrictSubType ? resolveMethodOnClass(refinedHolder, method) : topMethod;
+ DexEncodedMethod topSingleTarget = refinedResolutionResult.asSingleTarget();
DexClass topHolder = definitionFor(topSingleTarget.method.holder);
// We need to know whether the top method is from an interface, as that would allow it to be
// shadowed by a default method from an interface further down.
boolean topIsFromInterface = topHolder.isInterface();
// Now look at all subtypes and search for overrides.
DexEncodedMethod result =
- findSingleTargetFromSubtypes(
- refinedReceiverType,
- method,
- topSingleTarget,
- !refinedHolder.accessFlags.isAbstract(),
- topIsFromInterface);
+ validateSingleVirtualTarget(
+ findSingleTargetFromSubtypes(
+ refinedReceiverType,
+ method,
+ topSingleTarget,
+ !refinedHolder.accessFlags.isAbstract(),
+ topIsFromInterface),
+ topMethod.asSingleTarget());
// Map the failure case of SENTINEL to null.
result = result == DexEncodedMethod.SENTINEL ? null : result;
method.setSingleVirtualMethodCache(refinedReceiverType, result);
@@ -1099,11 +1127,13 @@
if (receiverLowerBoundType != null) {
if (receiverLowerBoundType.getClassType() == refinedReceiverType) {
ResolutionResult resolutionResult = resolveMethod(method.holder, method, true);
- if (resolutionResult.isValidVirtualTargetForDynamicDispatch()) {
+ if (resolutionResult.hasSingleTarget()
+ && resolutionResult.isValidVirtualTargetForDynamicDispatch()) {
ResolutionResult refinedResolutionResult = resolveMethod(refinedReceiverType, method);
if (refinedResolutionResult.hasSingleTarget()
&& refinedResolutionResult.isValidVirtualTargetForDynamicDispatch()) {
- return refinedResolutionResult.asSingleTarget();
+ return validateSingleVirtualTarget(
+ refinedResolutionResult.asSingleTarget(), resolutionResult.asSingleTarget());
}
}
return null;
@@ -1119,11 +1149,8 @@
}
// First check that there is a target for this invoke-interface to hit. If there is none,
// this will fail at runtime.
- ResolutionResult topTarget = resolveMethodOnInterface(holder, method);
- if (!topTarget.isValidVirtualTarget(app().options)) {
- return null;
- }
- if (topTarget.asResultOfResolve() == null) {
+ DexEncodedMethod topTarget = resolveMethodOnInterface(holder, method).asSingleTarget();
+ if (topTarget == null || !topTarget.isValidVirtualTarget(options())) {
return null;
}
// For kept types we cannot ensure a single target.
@@ -1152,19 +1179,18 @@
// override them, so we ignore interface methods here. Otherwise, we would look up
// default methods that are factually never used.
} else if (!clazz.accessFlags.isAbstract()) {
- ResolutionResult resolutionResult = resolveMethodOnClass(clazz, method);
- if (resolutionResult.hasSingleTarget()) {
- if ((result != null) && (result != resolutionResult.asSingleTarget())) {
- return null;
- } else {
- result = resolutionResult.asSingleTarget();
- }
- } else {
+ DexEncodedMethod resolutionResult = resolveMethodOnClass(clazz, method).asSingleTarget();
+ if (resolutionResult == null || isInvalidSingleVirtualTarget(resolutionResult, topTarget)) {
// This will fail at runtime.
return null;
}
+ if (result != null && result != resolutionResult) {
+ return null;
+ }
+ result = resolutionResult;
}
}
+ assert result == null || !isInvalidSingleVirtualTarget(result, topTarget);
return result == null || !result.isVirtualMethod() ? null : result;
}
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/inliner/interfacemethods/InlineDefaultInterfaceMethodTest.java b/src/test/java/com/android/tools/r8/ir/optimize/inliner/interfacemethods/InlineDefaultInterfaceMethodTest.java
index 26a4f2f..df9ab8d 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/inliner/interfacemethods/InlineDefaultInterfaceMethodTest.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/inliner/interfacemethods/InlineDefaultInterfaceMethodTest.java
@@ -4,53 +4,58 @@
package com.android.tools.r8.ir.optimize.inliner.interfacemethods;
-import static com.android.tools.r8.ir.desugar.InterfaceMethodRewriter.COMPANION_CLASS_NAME_SUFFIX;
-import static com.android.tools.r8.ir.desugar.InterfaceMethodRewriter.DEFAULT_METHOD_PREFIX;
-import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
-import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.Assert.assertEquals;
import com.android.tools.r8.NeverClassInline;
-import com.android.tools.r8.NeverInline;
import com.android.tools.r8.NeverMerge;
import com.android.tools.r8.TestBase;
-import com.android.tools.r8.utils.AndroidApiLevel;
+import com.android.tools.r8.TestParameters;
+import com.android.tools.r8.TestParametersCollection;
import com.android.tools.r8.utils.StringUtils;
-import com.android.tools.r8.utils.codeinspector.ClassSubject;
import com.android.tools.r8.utils.codeinspector.CodeInspector;
import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameters;
+@RunWith(Parameterized.class)
public class InlineDefaultInterfaceMethodTest extends TestBase {
+ private final TestParameters parameters;
+
+ @Parameters(name = "{0}")
+ public static TestParametersCollection data() {
+ return getTestParameters().withAllRuntimesAndApiLevels().build();
+ }
+
+ public InlineDefaultInterfaceMethodTest(TestParameters parameters) {
+ this.parameters = parameters;
+ }
+
@Test
public void test() throws Exception {
String expectedOutput = StringUtils.lines("Hello world!");
CodeInspector inspector =
- testForR8(Backend.DEX)
+ testForR8(parameters.getBackend())
.addInnerClasses(InlineDefaultInterfaceMethodTest.class)
.addKeepMainRule(TestClass.class)
- .setMinApi(AndroidApiLevel.M)
+ .setMinApi(parameters.getApiLevel())
.enableClassInliningAnnotations()
.enableMergeAnnotations()
.noMinification()
- .run(TestClass.class)
+ .run(parameters.getRuntime(), TestClass.class)
.assertSuccessWithOutput(expectedOutput)
.inspector();
- // TODO(b/124017330): interface methods should have been inlined into C.method().
- ClassSubject classSubject =
- inspector.clazz(I.class.getTypeName() + COMPANION_CLASS_NAME_SUFFIX);
- assertThat(classSubject, isPresent());
- assertThat(classSubject.uniqueMethodWithName(DEFAULT_METHOD_PREFIX + "hello"), isPresent());
- assertThat(classSubject.uniqueMethodWithName(DEFAULT_METHOD_PREFIX + "space"), isPresent());
- assertThat(classSubject.uniqueMethodWithName(DEFAULT_METHOD_PREFIX + "world"), isPresent());
+ // After inlining, only one class remains, namely TestClass.
+ assertEquals(1, inspector.allClasses().size());
}
static class TestClass {
public static void main(String[] args) {
- C obj = new C();
- obj.method();
+ new C().method();
}
}
@@ -73,7 +78,6 @@
@NeverClassInline
static class C implements I {
- @NeverInline
public void method() {
// invoke-virtual
hello();
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/inliner/interfacemethods/InlineStaticInterfaceMethodTest.java b/src/test/java/com/android/tools/r8/ir/optimize/inliner/interfacemethods/InlineStaticInterfaceMethodTest.java
index 9eb57fb..49dee4b 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/inliner/interfacemethods/InlineStaticInterfaceMethodTest.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/inliner/interfacemethods/InlineStaticInterfaceMethodTest.java
@@ -4,37 +4,47 @@
package com.android.tools.r8.ir.optimize.inliner.interfacemethods;
-import static com.android.tools.r8.ir.desugar.InterfaceMethodRewriter.COMPANION_CLASS_NAME_SUFFIX;
-import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
-import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.Assert.assertEquals;
import com.android.tools.r8.TestBase;
-import com.android.tools.r8.utils.AndroidApiLevel;
+import com.android.tools.r8.TestParameters;
+import com.android.tools.r8.TestParametersCollection;
import com.android.tools.r8.utils.StringUtils;
-import com.android.tools.r8.utils.codeinspector.ClassSubject;
import com.android.tools.r8.utils.codeinspector.CodeInspector;
import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameters;
+@RunWith(Parameterized.class)
public class InlineStaticInterfaceMethodTest extends TestBase {
+ private final TestParameters parameters;
+
+ @Parameters(name = "{0}")
+ public static TestParametersCollection data() {
+ return getTestParameters().withAllRuntimesAndApiLevels().build();
+ }
+
+ public InlineStaticInterfaceMethodTest(TestParameters parameters) {
+ this.parameters = parameters;
+ }
+
@Test
public void test() throws Exception {
String expectedOutput = StringUtils.lines("Hello world!");
CodeInspector inspector =
- testForR8(Backend.DEX)
+ testForR8(parameters.getBackend())
.addInnerClasses(InlineStaticInterfaceMethodTest.class)
.addKeepMainRule(TestClass.class)
- .setMinApi(AndroidApiLevel.M)
- .run(TestClass.class)
+ .setMinApi(parameters.getApiLevel())
+ .run(parameters.getRuntime(), TestClass.class)
.assertSuccessWithOutput(expectedOutput)
.inspector();
- // TODO(b/124017330): greet() should have been inlined into main().
- ClassSubject classSubject =
- inspector.clazz(I.class.getTypeName() + COMPANION_CLASS_NAME_SUFFIX);
- assertThat(classSubject, isPresent());
- assertThat(classSubject.uniqueMethodWithName("greet"), isPresent());
+ // After inlining of I.greet(), only one class remains, namely TestClass.
+ assertEquals(1, inspector.allClasses().size());
}
static class TestClass {
