Check accessibility of invoke-super in the enqueuer.
Bug: 145187573
Change-Id: Ie35707d45086e6d1d7d99895a4bec033e3790a80
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 32f691c..65a2c7c 100644
--- a/src/main/java/com/android/tools/r8/graph/ResolutionResult.java
+++ b/src/main/java/com/android/tools/r8/graph/ResolutionResult.java
@@ -65,6 +65,10 @@
DexProgramClass context, AppInfoWithSubtyping appInfo);
/** Lookup the single target of an invoke-super on this resolution result if possible. */
+ public abstract DexEncodedMethod lookupInvokeSuperTarget(
+ DexProgramClass context, AppInfoWithSubtyping appInfo);
+
+ @Deprecated
public abstract DexEncodedMethod lookupInvokeSuperTarget(DexClass context, AppInfo appInfo);
public final Set<DexEncodedMethod> lookupVirtualDispatchTargets(
@@ -154,7 +158,7 @@
@Override
public DexEncodedMethod lookupInvokeSpecialTarget(
DexProgramClass context, AppInfoWithSubtyping appInfo) {
- // If the resolution is non-accessible the no target exists.
+ // If the resolution is non-accessible then no target exists.
if (!isAccessibleFrom(context, appInfo)) {
return null;
}
@@ -257,6 +261,15 @@
* @return The actual target for the invoke-super or {@code null} if none found.
*/
@Override
+ public DexEncodedMethod lookupInvokeSuperTarget(
+ DexProgramClass context, AppInfoWithSubtyping appInfo) {
+ if (!isAccessibleFrom(context, appInfo)) {
+ return null;
+ }
+ return lookupInvokeSuperTarget(context.asDexClass(), appInfo);
+ }
+
+ @Override
public DexEncodedMethod lookupInvokeSuperTarget(DexClass context, AppInfo appInfo) {
assert context != null;
DexMethod method = resolvedMethod.method;
@@ -403,6 +416,12 @@
}
@Override
+ public DexEncodedMethod lookupInvokeSuperTarget(
+ DexProgramClass context, AppInfoWithSubtyping appInfo) {
+ return null;
+ }
+
+ @Override
public final DexEncodedMethod lookupInvokeSuperTarget(DexClass context, AppInfo appInfo) {
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 f4f02d3..8f41082 100644
--- a/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
+++ b/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
@@ -2168,11 +2168,7 @@
// Package protected due to entry point from worklist.
void markSuperMethodAsReachable(DexMethod method, DexEncodedMethod from) {
- // We have to mark the immediate target of the descriptor as targeted, as otherwise
- // the invoke super will fail in the resolution step with a NSM error.
- // See <a
- // href="https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-6.html#jvms-6.5.invokespecial">
- // the JVM spec for invoke-special.
+ // If the method does not resolve, mark it broken to avoid hiding errors in other optimizations.
SingleResolutionResult resolution =
appInfo.resolveMethod(method.holder, method).asSingleResolution();
if (resolution == null) {
@@ -2180,34 +2176,26 @@
reportMissingMethod(method);
return;
}
-
- // We should be passing the full program context and not looking it up again here.
- DexProgramClass fromHolder = appInfo.definitionFor(from.method.holder).asProgramClass();
-
- DexEncodedMethod resolutionTarget = resolution.getResolvedMethod();
- // TODO(b/145187573): Check access.
- if (resolutionTarget.accessFlags.isPrivate() || resolutionTarget.accessFlags.isStatic()) {
- brokenSuperInvokes.add(resolutionTarget.method);
- }
- DexProgramClass resolutionTargetClass = resolution.getResolvedHolder().asProgramClass();
- if (resolutionTargetClass != null) {
+ // If the resolution is in the program, mark it targeted.
+ if (resolution.getResolvedHolder().isProgramClass()) {
markMethodAsTargeted(
- resolutionTargetClass, resolutionTarget, KeepReason.targetedBySuperFrom(from));
+ resolution.getResolvedHolder().asProgramClass(),
+ resolution.getResolvedMethod(),
+ KeepReason.targetedBySuperFrom(from));
}
- // Now we need to compute the actual target in the context.
+ // If invoke target is invalid (inaccessible or not an instance-method) record it and stop.
+ // TODO(b/146016987): We should be passing the full program context and not looking it up again.
+ DexProgramClass fromHolder = appInfo.definitionFor(from.method.holder).asProgramClass();
DexEncodedMethod target = resolution.lookupInvokeSuperTarget(fromHolder, appInfo);
if (target == null) {
- // The actual implementation in the super class is missing.
- reportMissingMethod(method);
+ brokenSuperInvokes.add(resolution.getResolvedMethod().method);
return;
}
+
DexProgramClass clazz = getProgramClassOrNull(target.method.holder);
if (clazz == null) {
return;
}
- if (target.accessFlags.isPrivate()) {
- brokenSuperInvokes.add(resolutionTarget.method);
- }
if (Log.ENABLED) {
Log.verbose(getClass(), "Adding super constraint from `%s` to `%s`", from.method,
target.method);
diff --git a/src/test/java/com/android/tools/r8/resolution/access/NestInvokeSpecialInterfaceMethodAccessTest.java b/src/test/java/com/android/tools/r8/resolution/access/NestInvokeSpecialInterfaceMethodAccessTest.java
index bc15e9e..d0d4398 100644
--- a/src/test/java/com/android/tools/r8/resolution/access/NestInvokeSpecialInterfaceMethodAccessTest.java
+++ b/src/test/java/com/android/tools/r8/resolution/access/NestInvokeSpecialInterfaceMethodAccessTest.java
@@ -6,7 +6,6 @@
import static com.android.tools.r8.TestRuntime.CfVm.JDK11;
import static org.hamcrest.core.StringContains.containsString;
import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;
@@ -139,8 +138,7 @@
assertEquals(targetSpecial, targetSuper);
} else {
assertNull(targetSpecial);
- // TODO(b/145775365): Invoke super currently returns the resolution target.
- assertNotNull(targetSuper);
+ assertNull(targetSuper);
}
}
@@ -192,19 +190,35 @@
}
private void checkExpectedResult(TestRunResult<?> result, boolean isR8) {
+ if (isR8 && parameters.isDexRuntime() && inSameNest && symbolicReferenceIsDefiningType) {
+ // TODO(b/145187969): Incorrect nest desugaring.
+ result.assertFailureWithErrorThatMatches(containsString(VerifyError.class.getName()));
+ return;
+ }
+
if (!symbolicReferenceIsDefiningType) {
result.assertFailureWithErrorThatMatches(containsString(NoSuchMethodError.class.getName()));
- } else if (isDesugaring()) {
+ return;
+ }
+
+ if (isDesugaring()) {
// TODO(b/145775365): Desugaring results in an incorrect program.
result.assertFailureWithErrorThatMatches(containsString(NoSuchMethodError.class.getName()));
- } else if (!inSameNest) {
+ return;
+ }
+
+ if (!inSameNest) {
result.assertFailureWithErrorThatMatches(containsString(IllegalAccessError.class.getName()));
- } else if (parameters.isDexRuntime()) {
+ return;
+ }
+
+ if (parameters.isDexRuntime()) {
// TODO(b/145187969): Incorrect nest desugaring.
result.assertFailureWithErrorThatMatches(containsString(IllegalAccessError.class.getName()));
- } else {
- result.assertSuccessWithOutput(EXPECTED);
+ return;
}
+
+ result.assertSuccessWithOutput(EXPECTED);
}
private boolean isDesugaring() {
diff --git a/src/test/java/com/android/tools/r8/resolution/access/NestInvokeSpecialMethodAccessTest.java b/src/test/java/com/android/tools/r8/resolution/access/NestInvokeSpecialMethodAccessTest.java
index 67d6b4e..8d54c3a 100644
--- a/src/test/java/com/android/tools/r8/resolution/access/NestInvokeSpecialMethodAccessTest.java
+++ b/src/test/java/com/android/tools/r8/resolution/access/NestInvokeSpecialMethodAccessTest.java
@@ -6,7 +6,6 @@
import static com.android.tools.r8.TestRuntime.CfVm.JDK11;
import static org.hamcrest.core.StringContains.containsString;
import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;
@@ -115,8 +114,7 @@
assertEquals(targetSpecial, targetSuper);
} else {
assertNull(targetSpecial);
- // TODO(b/145775365): Invoke super currently returns the resolution target.
- assertNotNull(targetSuper);
+ assertNull(targetSuper);
}
}
@@ -152,7 +150,7 @@
.addProgramClasses(getClasses())
.addProgramClassFileData(getTransformedClasses())
.run(parameters.getRuntime(), Main.class)
- .apply(this::checkExpectedResult);
+ .apply(result -> checkExpectedResult(result, false));
}
@Test
@@ -163,15 +161,19 @@
.setMinApi(parameters.getApiLevel())
.addKeepMainRule(Main.class)
.run(parameters.getRuntime(), Main.class)
- .apply(this::checkExpectedResult);
+ .apply(result -> checkExpectedResult(result, true));
}
- private void checkExpectedResult(TestRunResult<?> result) {
+ private void checkExpectedResult(TestRunResult<?> result, boolean isR8) {
if (inSameNest) {
if (parameters.isDexRuntime()) {
// TODO(b/145187969): D8/R8 incorrectly compiles the nest based access away.
- result.assertFailureWithErrorThatMatches(
- containsString(IllegalAccessError.class.getName()));
+ if (isR8) {
+ result.assertFailureWithErrorThatMatches(containsString(VerifyError.class.getName()));
+ } else {
+ result.assertFailureWithErrorThatMatches(
+ containsString(IllegalAccessError.class.getName()));
+ }
} else {
result.assertSuccessWithOutput(EXPECTED);
}
diff --git a/src/test/java/com/android/tools/r8/resolution/access/NestInvokeSpecialMethodAccessWithIntermediateTest.java b/src/test/java/com/android/tools/r8/resolution/access/NestInvokeSpecialMethodAccessWithIntermediateTest.java
index 20e58a8..3f5efb9 100644
--- a/src/test/java/com/android/tools/r8/resolution/access/NestInvokeSpecialMethodAccessWithIntermediateTest.java
+++ b/src/test/java/com/android/tools/r8/resolution/access/NestInvokeSpecialMethodAccessWithIntermediateTest.java
@@ -140,8 +140,12 @@
assertEquals(targetSpecial, targetSuper);
} else {
assertNull(targetSpecial);
- // TODO(b/145775365): The current invoke-super will return the resolution target.
- assertNotNull(targetSuper);
+ if (!inSameNest) {
+ assertNull(targetSuper);
+ } else {
+ // TODO(b/145775365): The current invoke-super will return the resolution target.
+ assertNotNull(targetSuper);
+ }
}
}
