Resolution must fail if private resolutions don't match the symbolic reference.
This CL also shares the common parts of invoke-special and invoke-super. The
prevous implementation of invoke-super is invalid after the change to resolution
as it internally used resolution to lookup the target method.
Bug: 145775365
Bug: 145187969
Change-Id: I09b7dccced1c0e5e61b88d85f9bb428326034a9c
diff --git a/src/main/java/com/android/tools/r8/PrintUses.java b/src/main/java/com/android/tools/r8/PrintUses.java
index b906cb2..4a41c67 100644
--- a/src/main/java/com/android/tools/r8/PrintUses.java
+++ b/src/main/java/com/android/tools/r8/PrintUses.java
@@ -79,10 +79,16 @@
class UseCollector extends UseRegistry {
+ private DexProgramClass context;
+
UseCollector(DexItemFactory factory) {
super(factory);
}
+ public void setContext(DexProgramClass context) {
+ this.context = context;
+ }
+
@Override
public boolean registerInvokeVirtual(DexMethod method) {
DexEncodedMethod target = appInfo.lookupVirtualTarget(method.holder, method);
@@ -231,7 +237,10 @@
}
private void registerMethod(DexEncodedMethod method) {
- DexEncodedMethod superTarget = appInfo.lookupSuperTarget(method.method, method.method.holder);
+ DexEncodedMethod superTarget =
+ appInfo
+ .resolveMethod(method.method.holder, method.method)
+ .lookupInvokeSpecialTarget(context, appInfo);
if (superTarget != null) {
addMethod(superTarget.method);
}
@@ -344,6 +353,7 @@
private void analyze() {
UseCollector useCollector = new UseCollector(appInfo.dexItemFactory());
for (DexProgramClass dexProgramClass : application.classes()) {
+ useCollector.setContext(dexProgramClass);
useCollector.registerSuperType(dexProgramClass, dexProgramClass.superType);
for (DexType implementsType : dexProgramClass.interfaces.values) {
useCollector.registerSuperType(dexProgramClass, implementsType);
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 cc8af52..b9542eb 100644
--- a/src/main/java/com/android/tools/r8/graph/AppInfo.java
+++ b/src/main/java/com/android/tools/r8/graph/AppInfo.java
@@ -5,6 +5,7 @@
import com.android.tools.r8.graph.ResolutionResult.ArrayCloneMethodResult;
import com.android.tools.r8.graph.ResolutionResult.ClassNotFoundResult;
+import com.android.tools.r8.graph.ResolutionResult.IllegalAccessOrNoSuchMethodResult;
import com.android.tools.r8.graph.ResolutionResult.IncompatibleClassResult;
import com.android.tools.r8.graph.ResolutionResult.NoSuchMethodResult;
import com.android.tools.r8.graph.ResolutionResult.SingleResolutionResult;
@@ -348,7 +349,7 @@
assert checkIfObsolete();
assert !clazz.isInterface();
// Step 2:
- SingleResolutionResult result = resolveMethodOnClassStep2(clazz, method, clazz);
+ ResolutionResult result = resolveMethodOnClassStep2(clazz, method, clazz);
if (result != null) {
return result;
}
@@ -361,7 +362,7 @@
* href="https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-5.html#jvms-5.4.3.3">Section
* 5.4.3.3 of the JVM Spec</a>.
*/
- private SingleResolutionResult resolveMethodOnClassStep2(
+ private ResolutionResult resolveMethodOnClassStep2(
DexClass clazz, DexMethod method, DexClass initialResolutionHolder) {
// Pt. 1: Signature polymorphic method check.
// See also <a href="https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-2.html#jvms-2.9">
@@ -373,6 +374,14 @@
// Pt 2: Find a method that matches the descriptor.
result = clazz.lookupMethod(method);
if (result != null) {
+ // If the resolved method is private, then it can only be accessed if the symbolic reference
+ // that initiated the resolution was the type at which the method resolved on. If that is not
+ // the case, then the error is either an IllegalAccessError, or in the case where access is
+ // allowed because of nests, a NoSuchMethodError. Which error cannot be determined without
+ // knowing the calling context.
+ if (result.isPrivateMethod() && clazz != initialResolutionHolder) {
+ return new IllegalAccessOrNoSuchMethodResult(result);
+ }
return new SingleResolutionResult(initialResolutionHolder, clazz, result);
}
// Pt 3: Apply step two to direct superclass of holder.
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 65a2c7c..3080960 100644
--- a/src/main/java/com/android/tools/r8/graph/ResolutionResult.java
+++ b/src/main/java/com/android/tools/r8/graph/ResolutionResult.java
@@ -12,6 +12,7 @@
import java.util.Collection;
import java.util.Collections;
import java.util.Set;
+import java.util.function.BiPredicate;
import java.util.function.Consumer;
public abstract class ResolutionResult {
@@ -162,8 +163,70 @@
if (!isAccessibleFrom(context, appInfo)) {
return null;
}
+ DexEncodedMethod target =
+ internalInvokeSpecialOrSuper(
+ context, appInfo, (sup, sub) -> isSuperclass(sup, sub, appInfo));
+ if (target == null) {
+ return null;
+ }
+ // Should we check access control again?
+ DexClass holder = appInfo.definitionFor(target.method.holder);
+ if (!AccessControl.isMethodAccessible(target, holder, context, appInfo)) {
+ return null;
+ }
+ return target;
+ }
- // Statics cannot be targeted by invoke-special.
+ /**
+ * Lookup the target of an invoke-super.
+ *
+ * <p>This will return the target iff the resolution succeeded and the target is valid (i.e.,
+ * non-static and non-initializer) and accessible from {@code context}.
+ *
+ * <p>Additionally, this will also verify that the invoke-super is valid, i.e., it is on the a
+ * super type of the current context. Any invoke-special targeting the same type should have
+ * been mapped to an invoke-direct, but could change due to merging so we need to still allow
+ * the context to be equal to the targeted (symbolically referenced) type.
+ *
+ * @param context Class the invoke is contained in, i.e., the holder of the caller.
+ * @param appInfo Application info.
+ * @return The actual target for the invoke-super or {@code null} if no valid target is found.
+ */
+ @Override
+ public DexEncodedMethod lookupInvokeSuperTarget(
+ DexProgramClass context, AppInfoWithSubtyping appInfo) {
+ if (!isAccessibleFrom(context, appInfo)) {
+ return null;
+ }
+ DexEncodedMethod target = lookupInvokeSuperTarget(context.asDexClass(), appInfo);
+ if (target == null) {
+ return null;
+ }
+ // Should we check access control again?
+ DexClass holder = appInfo.definitionFor(target.method.holder);
+ if (!AccessControl.isMethodAccessible(target, holder, context, appInfo)) {
+ return null;
+ }
+ return target;
+ }
+
+ @Override
+ public DexEncodedMethod lookupInvokeSuperTarget(DexClass context, AppInfo appInfo) {
+ assert context != null;
+ if (resolvedMethod.isInstanceInitializer()
+ || (appInfo.hasSubtyping()
+ && initialResolutionHolder != context
+ && !isSuperclass(initialResolutionHolder, context, appInfo.withSubtyping()))) {
+ throw new CompilationError(
+ "Illegal invoke-super to " + resolvedMethod.toSourceString(), context.getOrigin());
+ }
+ return internalInvokeSpecialOrSuper(context, appInfo, (sup, sub) -> true);
+ }
+
+ private DexEncodedMethod internalInvokeSpecialOrSuper(
+ DexClass context, AppInfo appInfo, BiPredicate<DexClass, DexClass> isSuperclass) {
+
+ // Statics cannot be targeted by invoke-special/super.
if (getResolvedMethod().isStatic()) {
return null;
}
@@ -182,9 +245,9 @@
final DexClass initialType;
if (!resolvedMethod.isInstanceInitializer()
&& !symbolicReference.isInterface()
- && isSuperclass(symbolicReference, context, appInfo)) {
+ && isSuperclass.test(symbolicReference, context)) {
// If reference is a super type of the context then search starts at the immediate super.
- initialType = appInfo.definitionFor(context.superType);
+ initialType = context.superType == null ? null : appInfo.definitionFor(context.superType);
} else {
// Otherwise it starts at the reference itself.
initialType = symbolicReference;
@@ -228,10 +291,6 @@
if (target.isAbstract()) {
return null;
}
- // Should we check access control again?
- if (!AccessControl.isMethodAccessible(target, initialType, context, appInfo)) {
- return null;
- }
return target;
}
@@ -239,72 +298,6 @@
return sup != sub && appInfo.isSubtype(sub.type, sup.type);
}
- /**
- * Lookup super method following the super chain from the holder of {@code method}.
- *
- * <p>This method will resolve the method on the holder of {@code method} and only return a
- * non-null value if the result of resolution was an instance (i.e. non-static) method.
- *
- * <p>Additionally, this will also verify that the invoke super is valid, i.e., it is on the
- * same type or a super type of the current context. The spec says that it has invoke super
- * semantics, if the type is a supertype of the current class. If it is the same or a subtype,
- * it has invoke direct semantics. The latter case is illegal, so we map it to a super call
- * here. In R8, we abort at a later stage (see. See also <a href=
- * "https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-6.html#jvms-6.5.invokespecial" </a>
- * for invokespecial dispatch and <a href="https://docs.oracle.com/javase/specs/jvms/"
- * "se7/html/jvms-4.html#jvms-4.10.1.9.invokespecial"</a> for verification requirements. In
- * particular, the requirement isAssignable(class(CurrentClassName, L), class(MethodClassName,
- * L)). com.android.tools.r8.cf.code.CfInvoke#isInvokeSuper(DexType)}.
- *
- * @param context the class the invoke is contained in, i.e., the holder of the caller.
- * @param appInfo Application info.
- * @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;
- // TODO(b/145775365): Check the requirements for an invoke-special to a protected method.
- // See https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-6.html#jvms-6.5.invokespecial
-
- if (appInfo.hasSubtyping()
- && !appInfo.withSubtyping().isSubtype(context.type, initialResolutionHolder.type)) {
- throw new CompilationError(
- "Illegal invoke-super to " + method.toSourceString() + " from class " + context,
- context.getOrigin());
- }
-
- // According to
- // https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-6.html#jvms-6.5.invokespecial, use
- // the "symbolic reference" if the "symbolic reference" does not name a class.
- // TODO(b/145775365): This looks like the exact opposite of what the spec says, second item is
- // - is-class(sym-ref) => is-super(sym-ref, current-class)
- // this implication trivially holds for !is-class(sym-ref) == is-inteface(sym-ref), thus
- // the resolution should specifically *not* use the "symbolic reference".
- if (initialResolutionHolder.isInterface()) {
- // TODO(b/145775365): This does not consider a static method!
- return appInfo.resolveMethodOnInterface(initialResolutionHolder, method).getSingleTarget();
- }
- // Then, resume on the search, but this time, starting from the holder of the caller.
- if (context.superType == null) {
- return null;
- }
- SingleResolutionResult resolution =
- appInfo.resolveMethodOnClass(context.superType, method).asSingleResolution();
- return resolution != null && !resolution.resolvedMethod.isStatic()
- ? resolution.resolvedMethod
- : null;
- }
-
@Override
// TODO(b/140204899): Leverage refined receiver type if available.
public Set<DexEncodedMethod> lookupVirtualTargets(AppInfoWithSubtyping appInfo) {
@@ -515,33 +508,45 @@
}
}
- public static class IncompatibleClassResult extends FailedResolutionResult {
- static final IncompatibleClassResult INSTANCE =
- new IncompatibleClassResult(Collections.emptyList());
+ abstract static class FailedResolutionWithCausingMethods extends FailedResolutionResult {
private final Collection<DexEncodedMethod> methodsCausingError;
- private IncompatibleClassResult(Collection<DexEncodedMethod> methodsCausingError) {
+ private FailedResolutionWithCausingMethods(Collection<DexEncodedMethod> methodsCausingError) {
this.methodsCausingError = methodsCausingError;
}
- static IncompatibleClassResult create(Collection<DexEncodedMethod> methodsCausingError) {
- return methodsCausingError.isEmpty()
- ? INSTANCE
- : new IncompatibleClassResult(methodsCausingError);
- }
-
@Override
public void forEachFailureDependency(Consumer<DexEncodedMethod> methodCausingFailureConsumer) {
this.methodsCausingError.forEach(methodCausingFailureConsumer);
}
}
- public static class NoSuchMethodResult extends FailedResolutionResult {
- static final NoSuchMethodResult INSTANCE = new NoSuchMethodResult();
+ public static class IncompatibleClassResult extends FailedResolutionWithCausingMethods {
+ static final IncompatibleClassResult INSTANCE =
+ new IncompatibleClassResult(Collections.emptyList());
- private NoSuchMethodResult() {
- // Intentionally left empty.
+ private IncompatibleClassResult(Collection<DexEncodedMethod> methodsCausingError) {
+ super(methodsCausingError);
+ }
+
+ static IncompatibleClassResult create(Collection<DexEncodedMethod> methodsCausingError) {
+ return methodsCausingError.isEmpty()
+ ? INSTANCE
+ : new IncompatibleClassResult(methodsCausingError);
+ }
+ }
+
+ public static class NoSuchMethodResult extends FailedResolutionResult {
+
+ static final NoSuchMethodResult INSTANCE = new NoSuchMethodResult();
+ }
+
+ public static class IllegalAccessOrNoSuchMethodResult extends FailedResolutionWithCausingMethods {
+
+ public IllegalAccessOrNoSuchMethodResult(DexEncodedMethod methodCausingError) {
+ super(Collections.singletonList(methodCausingError));
+ assert methodCausingError != null;
}
}
}
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 f4fbdde..dd95b28 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
@@ -290,8 +290,9 @@
ResolutionResult resolution = appView.appInfo().resolveMethod(clazz, method);
// If resolution fails, install a method throwing IncompatibleClassChangeError.
if (resolution.isFailedResolution()) {
- assert resolution instanceof IncompatibleClassResult;
- addICCEThrowingMethod(method, clazz);
+ if (resolution instanceof IncompatibleClassResult) {
+ addICCEThrowingMethod(method, clazz);
+ }
return;
}
DexEncodedMethod target = resolution.getSingleTarget();
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/ArgumentRemovalUtils.java b/src/main/java/com/android/tools/r8/ir/optimize/ArgumentRemovalUtils.java
index f09f73f..5892466 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/ArgumentRemovalUtils.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/ArgumentRemovalUtils.java
@@ -15,7 +15,7 @@
public static boolean isPinned(DexEncodedMethod method, AppView<AppInfoWithLiveness> appView) {
return appView.appInfo().isPinned(method.method)
|| appView.appInfo().bootstrapMethods.contains(method.method)
- || appView.appInfo().brokenSuperInvokes.contains(method.method)
+ || appView.appInfo().failedResolutionTargets.contains(method.method)
|| appView.appInfo().methodsTargetedByInvokeDynamic.contains(method.method)
|| method.accessFlags.isNative();
}
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/UnusedArgumentsCollector.java b/src/main/java/com/android/tools/r8/ir/optimize/UnusedArgumentsCollector.java
index a938ff2..194b84a 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/UnusedArgumentsCollector.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/UnusedArgumentsCollector.java
@@ -227,9 +227,8 @@
for (int i = 0; i < directMethods.size(); i++) {
DexEncodedMethod method = directMethods.get(i);
- // If this is a private or static method that is targeted by an invoke-super instruction, then
- // don't remove any unused arguments.
- if (appView.appInfo().brokenSuperInvokes.contains(method.method)) {
+ // If this is a method with known resolution issues, then don't remove any unused arguments.
+ if (appView.appInfo().failedResolutionTargets.contains(method.method)) {
continue;
}
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 0aea415..25a7b15 100644
--- a/src/main/java/com/android/tools/r8/shaking/AppInfoWithLiveness.java
+++ b/src/main/java/com/android/tools/r8/shaking/AppInfoWithLiveness.java
@@ -82,7 +82,9 @@
*/
final SortedSet<DexMethod> targetedMethods;
- final Set<DexMethod> targetedMethodsThatMustRemainNonAbstract;
+ /** Set of targets that lead to resolution errors, such as non-existing or invalid targets. */
+ public final Set<DexMethod> failedResolutionTargets;
+
/**
* Set of program methods that are used as the bootstrap method for an invoke-dynamic instruction.
*/
@@ -117,11 +119,6 @@
* will have been removed from the code.
*/
public final Set<DexCallSite> callSites;
- /**
- * Set of method signatures used in invoke-super instructions that either cannot be resolved or
- * resolve to a private method (leading to an IllegalAccessError).
- */
- public final SortedSet<DexMethod> brokenSuperInvokes;
/** Set of all items that have to be kept independent of whether they are used. */
final Set<DexReference> pinnedItems;
/** All items with assumemayhavesideeffects rule. */
@@ -189,7 +186,7 @@
Set<DexType> instantiatedAppServices,
Set<DexType> instantiatedTypes,
SortedSet<DexMethod> targetedMethods,
- Set<DexMethod> targetedMethodsThatMustRemainNonAbstract,
+ Set<DexMethod> failedResolutionTargets,
SortedSet<DexMethod> bootstrapMethods,
SortedSet<DexMethod> methodsTargetedByInvokeDynamic,
SortedSet<DexMethod> virtualMethodsTargetedByInvokeDirect,
@@ -201,7 +198,6 @@
SortedMap<DexMethod, Set<DexEncodedMethod>> directInvokes,
SortedMap<DexMethod, Set<DexEncodedMethod>> staticInvokes,
Set<DexCallSite> callSites,
- SortedSet<DexMethod> brokenSuperInvokes,
Set<DexReference> pinnedItems,
Map<DexReference, ProguardMemberRule> mayHaveSideEffects,
Map<DexReference, ProguardMemberRule> noSideEffects,
@@ -228,7 +224,7 @@
this.instantiatedAppServices = instantiatedAppServices;
this.instantiatedTypes = instantiatedTypes;
this.targetedMethods = targetedMethods;
- this.targetedMethodsThatMustRemainNonAbstract = targetedMethodsThatMustRemainNonAbstract;
+ this.failedResolutionTargets = failedResolutionTargets;
this.bootstrapMethods = bootstrapMethods;
this.methodsTargetedByInvokeDynamic = methodsTargetedByInvokeDynamic;
this.virtualMethodsTargetedByInvokeDirect = virtualMethodsTargetedByInvokeDirect;
@@ -244,7 +240,6 @@
this.directInvokes = directInvokes;
this.staticInvokes = staticInvokes;
this.callSites = callSites;
- this.brokenSuperInvokes = brokenSuperInvokes;
this.alwaysInline = alwaysInline;
this.forceInline = forceInline;
this.neverInline = neverInline;
@@ -270,7 +265,7 @@
Set<DexType> instantiatedAppServices,
Set<DexType> instantiatedTypes,
SortedSet<DexMethod> targetedMethods,
- Set<DexMethod> targetedMethodsThatMustRemainNonAbstract,
+ Set<DexMethod> failedResolutionTargets,
SortedSet<DexMethod> bootstrapMethods,
SortedSet<DexMethod> methodsTargetedByInvokeDynamic,
SortedSet<DexMethod> virtualMethodsTargetedByInvokeDirect,
@@ -282,7 +277,6 @@
SortedMap<DexMethod, Set<DexEncodedMethod>> directInvokes,
SortedMap<DexMethod, Set<DexEncodedMethod>> staticInvokes,
Set<DexCallSite> callSites,
- SortedSet<DexMethod> brokenSuperInvokes,
Set<DexReference> pinnedItems,
Map<DexReference, ProguardMemberRule> mayHaveSideEffects,
Map<DexReference, ProguardMemberRule> noSideEffects,
@@ -309,7 +303,7 @@
this.instantiatedAppServices = instantiatedAppServices;
this.instantiatedTypes = instantiatedTypes;
this.targetedMethods = targetedMethods;
- this.targetedMethodsThatMustRemainNonAbstract = targetedMethodsThatMustRemainNonAbstract;
+ this.failedResolutionTargets = failedResolutionTargets;
this.bootstrapMethods = bootstrapMethods;
this.methodsTargetedByInvokeDynamic = methodsTargetedByInvokeDynamic;
this.virtualMethodsTargetedByInvokeDirect = virtualMethodsTargetedByInvokeDirect;
@@ -325,7 +319,6 @@
this.directInvokes = directInvokes;
this.staticInvokes = staticInvokes;
this.callSites = callSites;
- this.brokenSuperInvokes = brokenSuperInvokes;
this.alwaysInline = alwaysInline;
this.forceInline = forceInline;
this.neverInline = neverInline;
@@ -352,7 +345,7 @@
previous.instantiatedAppServices,
previous.instantiatedTypes,
previous.targetedMethods,
- previous.targetedMethodsThatMustRemainNonAbstract,
+ previous.failedResolutionTargets,
previous.bootstrapMethods,
previous.methodsTargetedByInvokeDynamic,
previous.virtualMethodsTargetedByInvokeDirect,
@@ -364,7 +357,6 @@
previous.directInvokes,
previous.staticInvokes,
previous.callSites,
- previous.brokenSuperInvokes,
previous.pinnedItems,
previous.mayHaveSideEffects,
previous.noSideEffects,
@@ -400,7 +392,7 @@
previous.instantiatedAppServices,
previous.instantiatedTypes,
previous.targetedMethods,
- previous.targetedMethodsThatMustRemainNonAbstract,
+ previous.failedResolutionTargets,
previous.bootstrapMethods,
previous.methodsTargetedByInvokeDynamic,
previous.virtualMethodsTargetedByInvokeDirect,
@@ -412,7 +404,6 @@
previous.directInvokes,
previous.staticInvokes,
previous.callSites,
- previous.brokenSuperInvokes,
additionalPinnedItems == null
? previous.pinnedItems
: CollectionUtils.mergeSets(previous.pinnedItems, additionalPinnedItems),
@@ -452,8 +443,8 @@
this.instantiatedTypes = rewriteItems(previous.instantiatedTypes, lense::lookupType);
this.instantiatedLambdas = rewriteItems(previous.instantiatedLambdas, lense::lookupType);
this.targetedMethods = lense.rewriteMethodsConservatively(previous.targetedMethods);
- this.targetedMethodsThatMustRemainNonAbstract =
- lense.rewriteMethodsConservatively(previous.targetedMethodsThatMustRemainNonAbstract);
+ this.failedResolutionTargets =
+ lense.rewriteMethodsConservatively(previous.failedResolutionTargets);
this.bootstrapMethods = lense.rewriteMethodsConservatively(previous.bootstrapMethods);
this.methodsTargetedByInvokeDynamic =
lense.rewriteMethodsConservatively(previous.methodsTargetedByInvokeDynamic);
@@ -481,7 +472,6 @@
// TODO(sgjesse): Rewrite call sites as well? Right now they are only used by minification
// after second tree shaking.
this.callSites = previous.callSites;
- this.brokenSuperInvokes = lense.rewriteMethodsConservatively(previous.brokenSuperInvokes);
// Don't rewrite pruned types - the removed types are identified by their original name.
this.prunedTypes = previous.prunedTypes;
this.mayHaveSideEffects =
@@ -535,8 +525,7 @@
this.instantiatedTypes = previous.instantiatedTypes;
this.instantiatedLambdas = previous.instantiatedLambdas;
this.targetedMethods = previous.targetedMethods;
- this.targetedMethodsThatMustRemainNonAbstract =
- previous.targetedMethodsThatMustRemainNonAbstract;
+ this.failedResolutionTargets = previous.failedResolutionTargets;
this.bootstrapMethods = previous.bootstrapMethods;
this.methodsTargetedByInvokeDynamic = previous.methodsTargetedByInvokeDynamic;
this.virtualMethodsTargetedByInvokeDirect = previous.virtualMethodsTargetedByInvokeDirect;
@@ -552,7 +541,6 @@
this.directInvokes = previous.directInvokes;
this.staticInvokes = previous.staticInvokes;
this.callSites = previous.callSites;
- this.brokenSuperInvokes = previous.brokenSuperInvokes;
this.alwaysInline = previous.alwaysInline;
this.forceInline = previous.forceInline;
this.neverInline = previous.neverInline;
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 8329fcf..335f662 100644
--- a/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
+++ b/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
@@ -156,11 +156,6 @@
private final Set<DexReference> identifierNameStrings = Sets.newIdentityHashSet();
/**
- * Set of method signatures used in invoke-super instructions that either cannot be resolved or
- * resolve to a private method (leading to an IllegalAccessError).
- */
- private final Set<DexMethod> brokenSuperInvokes = Sets.newIdentityHashSet();
- /**
* This map keeps a view of all virtual methods that are reachable from virtual invokes. A method
* is reachable even if no live subtypes exist, so this is not sufficient for inclusion in the
* live set.
@@ -211,8 +206,8 @@
*/
private final SetWithReason<DexEncodedMethod> targetedMethods;
- /** Subset of 'targetedMethods' for which the method must not be marked abstract. */
- private final Set<DexEncodedMethod> targetedMethodsThatMustRemainNonAbstract;
+ /** Set of methods that have invalid resolutions or lookups. */
+ private final Set<DexMethod> failedResolutionTargets;
/**
* Set of program methods that are used as the bootstrap method for an invoke-dynamic instruction.
@@ -329,7 +324,7 @@
// This set is only populated in edge cases due to multiple default interface methods.
// The set is generally expected to be empty and in the unlikely chance it is not, it will
// likely contain two methods. Thus the default capacity of 2.
- targetedMethodsThatMustRemainNonAbstract = SetUtils.newIdentityHashSet(2);
+ failedResolutionTargets = SetUtils.newIdentityHashSet(2);
liveMethods = new LiveMethodsSet(graphReporter::registerMethod);
liveFields = new SetWithReason<>(graphReporter::registerField);
instantiatedInterfaceTypes = new SetWithReason<>(graphReporter::registerInterface);
@@ -1327,21 +1322,22 @@
annotation.annotation.collectIndexedItems(referenceMarker);
}
- private void handleInvokeOfStaticTarget(DexMethod method, KeepReason reason) {
+ private ResolutionResult resolveMethod(DexMethod method, KeepReason reason) {
ResolutionResult resolutionResult = appInfo.resolveMethod(method.holder, method);
- if (resolutionResult == null) {
+ if (resolutionResult.isFailedResolution()) {
reportMissingMethod(method);
+ markFailedResolutionTargets(method, resolutionResult.asFailedResolution(), reason);
+ }
+ return resolutionResult;
+ }
+
+ private void handleInvokeOfStaticTarget(DexMethod method, KeepReason reason) {
+ SingleResolutionResult resolution = resolveMethod(method, reason).asSingleResolution();
+ if (resolution == null || resolution.getResolvedHolder().isNotProgramClass()) {
return;
}
- DexEncodedMethod encodedMethod = resolutionResult.getSingleTarget();
- if (encodedMethod == null) {
- // Note: should this be reported too? Or is this unreachable?
- return;
- }
- DexProgramClass clazz = getProgramClassOrNull(encodedMethod.method.holder);
- if (clazz == null) {
- return;
- }
+ DexProgramClass clazz = resolution.getResolvedHolder().asProgramClass();
+ DexEncodedMethod encodedMethod = resolution.getResolvedMethod();
// We have to mark the resolved method as targeted even if it cannot actually be invoked
// to make sure the invocation will keep failing in the appropriate way.
@@ -1636,7 +1632,7 @@
assert libraryClass.isNotProgramClass();
assert !instantiatedClass.isInterface() || instantiatedClass.accessFlags.isAnnotation();
for (DexEncodedMethod method : libraryClass.virtualMethods()) {
- // Note: it may be worthwhile to add a resolution cache here. If so, it must till ensure
+ // Note: it may be worthwhile to add a resolution cache here. If so, it must still ensure
// that all library override edges are reported to the kept-graph consumer.
ResolutionResult firstResolution =
appView.appInfo().resolveMethod(instantiatedClass, method.method);
@@ -2100,7 +2096,7 @@
appInfo.resolveMethod(method.holder, method, interfaceInvoke);
if (resolutionResult.isFailedResolution()) {
// If the resolution fails, mark each dependency causing a failure.
- markFailedResolutionTargets(resolutionResult.asFailedResolution(), reason);
+ markFailedResolutionTargets(method, resolutionResult.asFailedResolution(), reason);
return MarkedResolutionTarget.unresolved();
}
@@ -2132,12 +2128,13 @@
}
private void markFailedResolutionTargets(
- FailedResolutionResult failedResolution, KeepReason reason) {
+ DexMethod symbolicMethod, FailedResolutionResult failedResolution, KeepReason reason) {
+ failedResolutionTargets.add(symbolicMethod);
failedResolution.forEachFailureDependency(
method -> {
DexProgramClass clazz = getProgramClassOrNull(method.method.holder);
if (clazz != null) {
- targetedMethodsThatMustRemainNonAbstract.add(method);
+ failedResolutionTargets.add(method.method);
markMethodAsTargeted(clazz, method, reason);
}
});
@@ -2168,27 +2165,22 @@
// Package protected due to entry point from worklist.
void markSuperMethodAsReachable(DexMethod method, DexEncodedMethod from) {
- // If the method does not resolve, mark it broken to avoid hiding errors in other optimizations.
- SingleResolutionResult resolution =
- appInfo.resolveMethod(method.holder, method).asSingleResolution();
+ KeepReason reason = KeepReason.targetedBySuperFrom(from);
+ SingleResolutionResult resolution = resolveMethod(method, reason).asSingleResolution();
if (resolution == null) {
- brokenSuperInvokes.add(method);
- reportMissingMethod(method);
return;
}
// If the resolution is in the program, mark it targeted.
if (resolution.getResolvedHolder().isProgramClass()) {
markMethodAsTargeted(
- resolution.getResolvedHolder().asProgramClass(),
- resolution.getResolvedMethod(),
- KeepReason.targetedBySuperFrom(from));
+ resolution.getResolvedHolder().asProgramClass(), resolution.getResolvedMethod(), reason);
}
// 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) {
- brokenSuperInvokes.add(resolution.getResolvedMethod().method);
+ failedResolutionTargets.add(resolution.getResolvedMethod().method);
return;
}
@@ -2281,8 +2273,7 @@
Collections.unmodifiableSet(instantiatedAppServices),
SetUtils.mapIdentityHashSet(instantiatedTypes.getItems(), DexProgramClass::getType),
Enqueuer.toSortedDescriptorSet(targetedMethods.getItems()),
- SetUtils.mapIdentityHashSet(
- targetedMethodsThatMustRemainNonAbstract, DexEncodedMethod::getKey),
+ Collections.unmodifiableSet(failedResolutionTargets),
ImmutableSortedSet.copyOf(DexMethod::slowCompareTo, bootstrapMethods),
ImmutableSortedSet.copyOf(DexMethod::slowCompareTo, methodsTargetedByInvokeDynamic),
ImmutableSortedSet.copyOf(
@@ -2297,7 +2288,6 @@
toImmutableSortedMap(directInvokes, PresortedComparable::slowCompare),
toImmutableSortedMap(staticInvokes, PresortedComparable::slowCompare),
callSites,
- ImmutableSortedSet.copyOf(DexMethod::slowCompareTo, brokenSuperInvokes),
pinnedItems,
rootSet.mayHaveSideEffects,
rootSet.noSideEffects,
@@ -2642,6 +2632,7 @@
markParameterAndReturnTypesAsLive(method);
}
+
private void markParameterAndReturnTypesAsLive(DexEncodedMethod method) {
for (DexType parameterType : method.method.proto.parameters.values) {
markTypeAsLive(
diff --git a/src/main/java/com/android/tools/r8/shaking/TreePruner.java b/src/main/java/com/android/tools/r8/shaking/TreePruner.java
index e7a7fe5..116e315 100644
--- a/src/main/java/com/android/tools/r8/shaking/TreePruner.java
+++ b/src/main/java/com/android/tools/r8/shaking/TreePruner.java
@@ -297,7 +297,7 @@
&& !method.accessFlags.isSynchronized()
&& !method.accessFlags.isPrivate()
&& !method.accessFlags.isStatic()
- && !appInfo.targetedMethodsThatMustRemainNonAbstract.contains(method.method);
+ && !appInfo.failedResolutionTargets.contains(method.method);
// Private methods and static methods can only be targeted yet non-live as the result of
// an invalid invoke. They will not actually be called at runtime but we have to keep them
// as non-abstract (see above) to produce the same failure mode.
diff --git a/src/main/java/com/android/tools/r8/shaking/VerticalClassMerger.java b/src/main/java/com/android/tools/r8/shaking/VerticalClassMerger.java
index 9a477cc..c518937 100644
--- a/src/main/java/com/android/tools/r8/shaking/VerticalClassMerger.java
+++ b/src/main/java/com/android/tools/r8/shaking/VerticalClassMerger.java
@@ -263,24 +263,6 @@
}
}
- // Avoid merging two types if this could remove a NoSuchMethodError, as illustrated by the
- // following example. (Alternatively, it would be possible to merge A and B and rewrite the
- // "invoke-super A.m" instruction into "invoke-super Object.m" to preserve the error. This
- // situation should generally not occur in practice, though.)
- //
- // class A {}
- // class B extends A {
- // public void m() {}
- // }
- // class C extends A {
- // public void m() {
- // invoke-super "A.m" <- should yield NoSuchMethodError, cannot merge A and B
- // }
- // }
- for (DexMethod signature : appInfo.brokenSuperInvokes) {
- markTypeAsPinned(signature.holder, AbortReason.UNHANDLED_INVOKE_SUPER);
- }
-
// It is valid to have an invoke-direct instruction in a default interface method that targets
// another default method in the same interface (see InterfaceMethodDesugaringTests.testInvoke-
// SpecialToDefaultMethod). However, in a class, that would lead to a verification error.
@@ -290,7 +272,7 @@
}
// The set of targets that must remain for proper resolution error cases should not be merged.
- for (DexMethod method : appInfo.targetedMethodsThatMustRemainNonAbstract) {
+ for (DexMethod method : appInfo.failedResolutionTargets) {
markTypeAsPinned(method.holder, AbortReason.RESOLUTION_FOR_METHODS_MAY_CHANGE);
}
}
diff --git a/src/test/java/com/android/tools/r8/TestCompilerBuilder.java b/src/test/java/com/android/tools/r8/TestCompilerBuilder.java
index 0f613d5..cc8d46a 100644
--- a/src/test/java/com/android/tools/r8/TestCompilerBuilder.java
+++ b/src/test/java/com/android/tools/r8/TestCompilerBuilder.java
@@ -118,6 +118,7 @@
}
@Override
+ @Deprecated
public RR run(String mainClass)
throws CompilationFailedException, ExecutionException, IOException {
return compile().run(mainClass);
diff --git a/src/test/java/com/android/tools/r8/classFiltering/TestDesugar.java b/src/test/java/com/android/tools/r8/classFiltering/TestDesugar.java
index a346177..d7cdcd0 100644
--- a/src/test/java/com/android/tools/r8/classFiltering/TestDesugar.java
+++ b/src/test/java/com/android/tools/r8/classFiltering/TestDesugar.java
@@ -9,8 +9,7 @@
lambda.consume("TestDesugar.consume");
}
-
- public interface Consumer {
+ interface Consumer {
public void consume(String s);
}
}
diff --git a/src/test/java/com/android/tools/r8/classmerging/VerticalClassMergerTest.java b/src/test/java/com/android/tools/r8/classmerging/VerticalClassMergerTest.java
index 7fab7a1..cf013d1 100644
--- a/src/test/java/com/android/tools/r8/classmerging/VerticalClassMergerTest.java
+++ b/src/test/java/com/android/tools/r8/classmerging/VerticalClassMergerTest.java
@@ -733,11 +733,6 @@
CF_DIR.resolve("SuperClassWithReferencedMethod.class"),
CF_DIR.resolve("SuperCallRewritingTest.class")
};
- Set<String> preservedClassNames =
- ImmutableSet.of(
- "classmerging.SubClassThatReferencesSuperMethod",
- "classmerging.SuperClassWithReferencedMethod",
- "classmerging.SuperCallRewritingTest");
// Build SubClassThatReferencesMethod.
SmaliBuilder smaliBuilder =
@@ -761,19 +756,28 @@
"move-result-object v1",
"return-object v1");
- // Build app.
- AndroidApp.Builder builder = AndroidApp.builder();
- builder.addProgramFiles(programFiles);
- builder.addDexProgramData(smaliBuilder.compile(), Origin.unknown());
+ String expectedOutput =
+ StringUtils.lines(
+ "Calling referencedMethod on SubClassThatReferencesSuperMethod",
+ "In referencedMethod on SubClassThatReferencesSuperMethod",
+ "In referencedMethod on SuperClassWithReferencedMethod",
+ "SuperClassWithReferencedMethod.referencedMethod()");
- // Run test.
- runTestOnInput(
- main,
- builder.build(),
- preservedClassNames::contains,
- // Prevent class merging, such that the generated code would be invalid if we rewrite the
- // invoke-super instruction into an invoke-direct instruction.
- getProguardConfig(EXAMPLE_KEEP, "-keep class *"));
+ testForD8()
+ .addProgramFiles(programFiles)
+ .addProgramDexFileData(smaliBuilder.compile())
+ .run(main)
+ .assertSuccessWithOutput(expectedOutput);
+
+ testForR8(Backend.DEX)
+ .addOptionsModification(this::configure)
+ .addKeepMainRule(main)
+ // Keep the classes to avoid merge, but don't keep methods which allows inlining.
+ .addKeepRules("-keep class *")
+ .addProgramFiles(programFiles)
+ .addProgramDexFileData(smaliBuilder.compile())
+ .run(main)
+ .assertSuccessWithOutput(expectedOutput);
}
// The following test checks that our rewriting of invoke-super instructions in a class F works
diff --git a/src/test/java/com/android/tools/r8/graph/TargetLookupTest.java b/src/test/java/com/android/tools/r8/graph/TargetLookupTest.java
index db68c25..289a0ba 100644
--- a/src/test/java/com/android/tools/r8/graph/TargetLookupTest.java
+++ b/src/test/java/com/android/tools/r8/graph/TargetLookupTest.java
@@ -170,7 +170,8 @@
assertNull(appInfo.lookupDirectTarget(methodXOnTest));
assertNotNull(appInfo.lookupStaticTarget(methodXOnTestSuper));
- assertNotNull(appInfo.lookupStaticTarget(methodXOnTest));
+ // Accessing a private target on a different type will fail resolution outright.
+ assertNull(appInfo.lookupStaticTarget(methodXOnTest));
assertEquals("OK", runArt(application));
}
diff --git a/src/test/java/com/android/tools/r8/resolution/VirtualOverrideOfPrivateStaticMethodTest.java b/src/test/java/com/android/tools/r8/resolution/VirtualOverrideOfPrivateStaticMethodTest.java
index 5fe17d5..eb4cd97 100644
--- a/src/test/java/com/android/tools/r8/resolution/VirtualOverrideOfPrivateStaticMethodTest.java
+++ b/src/test/java/com/android/tools/r8/resolution/VirtualOverrideOfPrivateStaticMethodTest.java
@@ -4,23 +4,21 @@
package com.android.tools.r8.resolution;
import static org.hamcrest.CoreMatchers.containsString;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
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 com.android.tools.r8.ToolHelper.DexVm;
-import com.android.tools.r8.graph.DexEncodedMethod;
import com.android.tools.r8.graph.DexMethod;
import com.android.tools.r8.graph.ResolutionResult;
+import com.android.tools.r8.graph.ResolutionResult.IllegalAccessOrNoSuchMethodResult;
import com.android.tools.r8.shaking.AppInfoWithLiveness;
import com.google.common.collect.ImmutableList;
import java.io.IOException;
import java.util.List;
import java.util.concurrent.ExecutionException;
-import org.junit.Assert;
import org.junit.BeforeClass;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -81,22 +79,9 @@
}
@Test
- public void lookupSingleTarget() {
+ public void resolveTarget() {
ResolutionResult resolutionResult = appInfo.resolveMethodOnClass(methodOnB.holder, methodOnB);
- assertFalse(resolutionResult.isValidVirtualTarget(appInfo.app().options));
- DexEncodedMethod resolved = resolutionResult.getSingleTarget();
- assertEquals(methodOnA, resolved.method);
- DexEncodedMethod singleVirtualTarget =
- appInfo.lookupSingleVirtualTarget(methodOnB, methodOnB.holder);
- Assert.assertNull(singleVirtualTarget);
- }
-
- @Test
- public void lookupVirtualTargets() {
- ResolutionResult resolutionResult = appInfo.resolveMethodOnClass(methodOnB.holder, methodOnB);
- DexEncodedMethod resolved = resolutionResult.getSingleTarget();
- assertEquals(methodOnA, resolved.method);
- assertFalse(resolutionResult.isValidVirtualTarget(appInfo.app().options));
+ assertTrue(resolutionResult instanceof IllegalAccessOrNoSuchMethodResult);
}
@Test
diff --git a/src/test/java/com/android/tools/r8/resolution/VirtualOverrideOfPrivateStaticMethodWithVirtualParentTest.java b/src/test/java/com/android/tools/r8/resolution/VirtualOverrideOfPrivateStaticMethodWithVirtualParentTest.java
index 22445e6..85ace0b 100644
--- a/src/test/java/com/android/tools/r8/resolution/VirtualOverrideOfPrivateStaticMethodWithVirtualParentTest.java
+++ b/src/test/java/com/android/tools/r8/resolution/VirtualOverrideOfPrivateStaticMethodWithVirtualParentTest.java
@@ -4,34 +4,29 @@
package com.android.tools.r8.resolution;
import static org.hamcrest.CoreMatchers.containsString;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
-import com.android.tools.r8.AsmTestBase;
import com.android.tools.r8.R8TestRunResult;
+import com.android.tools.r8.TestBase;
import com.android.tools.r8.TestParameters;
import com.android.tools.r8.TestParametersCollection;
import com.android.tools.r8.TestRunResult;
import com.android.tools.r8.TestRuntime;
import com.android.tools.r8.ToolHelper.DexVm;
-import com.android.tools.r8.graph.DexEncodedMethod;
import com.android.tools.r8.graph.DexMethod;
import com.android.tools.r8.graph.ResolutionResult;
import com.android.tools.r8.shaking.AppInfoWithLiveness;
import com.google.common.collect.ImmutableList;
import java.util.List;
-import org.junit.Assert;
import org.junit.BeforeClass;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
import org.junit.runners.Parameterized.Parameters;
-import org.objectweb.asm.ClassWriter;
-import org.objectweb.asm.MethodVisitor;
import org.objectweb.asm.Opcodes;
@RunWith(Parameterized.class)
-public class VirtualOverrideOfPrivateStaticMethodWithVirtualParentTest extends AsmTestBase {
+public class VirtualOverrideOfPrivateStaticMethodWithVirtualParentTest extends TestBase {
public interface I {
default void f() {}
@@ -64,51 +59,26 @@
public static class BaseDump implements Opcodes {
- static String prefix(String suffix) {
- return VirtualOverrideOfPrivateStaticMethodWithVirtualParentTest.class
- .getTypeName()
- .replace('.', '/')
- + suffix;
- }
-
- public static byte[] dump() {
- ClassWriter cw = new ClassWriter(0);
- MethodVisitor mv;
- cw.visit(V1_8, ACC_PUBLIC | ACC_SUPER, prefix("$Base"), null, "java/lang/Object", null);
- cw.visitSource("VirtualOverrideOfPrivateStaticMethodWithVirtualParentTest.java", null);
- {
- mv = cw.visitMethod(ACC_PUBLIC, "<init>", "()V", null, null);
- mv.visitCode();
- mv.visitVarInsn(ALOAD, 0);
- mv.visitMethodInsn(INVOKESPECIAL, "java/lang/Object", "<init>", "()V", false);
- mv.visitInsn(RETURN);
- mv.visitMaxs(1, 1);
- mv.visitEnd();
- }
- {
- // Changed ACC_PRIVATE to ACC_PUBLIC
- mv = cw.visitMethod(ACC_PUBLIC, "f", "()V", null, null);
- mv.visitCode();
- mv.visitInsn(RETURN);
- mv.visitMaxs(0, 1);
- mv.visitEnd();
- }
- cw.visitEnd();
- return cw.toByteArray();
+ public static byte[] dump() throws Exception {
+ return transformer(Base.class).setPublic(Base.class.getDeclaredMethod("f")).transform();
}
}
public static List<Class<?>> CLASSES =
ImmutableList.of(A.class, B.class, C.class, I.class, Main.class);
- public static List<byte[]> DUMPS = ImmutableList.of(BaseDump.dump());
+ public static List<byte[]> getDumps() throws Exception {
+ return ImmutableList.of(BaseDump.dump());
+ }
private static AppInfoWithLiveness appInfo;
@BeforeClass
public static void computeAppInfo() throws Exception {
appInfo =
- computeAppViewWithLiveness(readClassesAndAsmDump(CLASSES, DUMPS), Main.class).appInfo();
+ computeAppViewWithLiveness(
+ buildClasses(CLASSES).addClassProgramData(getDumps()).build(), Main.class)
+ .appInfo();
}
private static DexMethod buildMethod(Class clazz, String name) {
@@ -130,21 +100,9 @@
}
@Test
- public void lookupSingleTarget() {
- DexEncodedMethod resolved =
- appInfo.resolveMethodOnClass(methodOnB.holder, methodOnB).getSingleTarget();
- assertEquals(methodOnA, resolved.method);
- DexEncodedMethod singleVirtualTarget =
- appInfo.lookupSingleVirtualTarget(methodOnB, methodOnB.holder);
- Assert.assertNull(singleVirtualTarget);
- }
-
- @Test
- public void lookupVirtualTargets() {
+ public void testResolution() {
ResolutionResult resolutionResult = appInfo.resolveMethodOnClass(methodOnB.holder, methodOnB);
- DexEncodedMethod resolved = resolutionResult.getSingleTarget();
- assertEquals(methodOnA, resolved.method);
- assertFalse(resolutionResult.isValidVirtualTarget(appInfo.app().options));
+ assertTrue(resolutionResult.isFailedResolution());
}
@Test
@@ -154,13 +112,13 @@
runResult =
testForJvm()
.addProgramClasses(CLASSES)
- .addProgramClassFileData(DUMPS)
+ .addProgramClassFileData(getDumps())
.run(parameters.getRuntime(), Main.class);
} else {
runResult =
testForD8()
.addProgramClasses(CLASSES)
- .addProgramClassFileData(DUMPS)
+ .addProgramClassFileData(getDumps())
.setMinApi(parameters.getApiLevel())
.run(parameters.getRuntime(), Main.class);
}
@@ -172,7 +130,7 @@
R8TestRunResult runResult =
testForR8(parameters.getBackend())
.addProgramClasses(CLASSES)
- .addProgramClassFileData(DUMPS)
+ .addProgramClassFileData(getDumps())
.addKeepMainRule(Main.class)
.setMinApi(parameters.getApiLevel())
.run(parameters.getRuntime(), Main.class);
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 52dbd4b..8e21be0 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
@@ -18,6 +18,7 @@
import com.android.tools.r8.graph.DexMethod;
import com.android.tools.r8.graph.DexProgramClass;
import com.android.tools.r8.graph.ResolutionResult;
+import com.android.tools.r8.graph.ResolutionResult.IllegalAccessOrNoSuchMethodResult;
import com.android.tools.r8.references.Reference;
import com.android.tools.r8.shaking.AppInfoWithLiveness;
import com.android.tools.r8.transformers.ClassFileTransformer;
@@ -121,6 +122,12 @@
assertEquals(method.holder, declaredClassDefinition.type);
ResolutionResult resolutionResult = appInfo.resolveMethod(declaredClassDefinition, method);
+ // Resolution fails when there is a mismatch between the symbolic reference and the definition.
+ if (!symbolicReferenceIsDefiningType) {
+ assertTrue(resolutionResult instanceof IllegalAccessOrNoSuchMethodResult);
+ return;
+ }
+
// Verify that the resolved method is on the defining class.
assertEquals(
definingClassDefinition, resolutionResult.asSingleResolution().getResolvedHolder());
@@ -178,7 +185,7 @@
.addProgramClasses(getClasses())
.addProgramClassFileData(getTransformedClasses())
.run(parameters.getRuntime(), Main.class)
- .apply(result -> checkExpectedResult(result, false));
+ .apply(this::checkExpectedResult);
}
@Test
@@ -189,10 +196,10 @@
.setMinApi(parameters.getApiLevel())
.addKeepMainRule(Main.class)
.run(parameters.getRuntime(), Main.class)
- .apply(result -> checkExpectedResult(result, true));
+ .apply(this::checkExpectedResult);
}
- private void checkExpectedResult(TestRunResult<?> result, boolean isR8) {
+ private void checkExpectedResult(TestRunResult<?> result) {
// If not in the same nest, the error is always illegal access.
if (!inSameNest) {
result.assertFailureWithErrorThatThrows(IllegalAccessError.class);
@@ -201,11 +208,6 @@
// If in the same nest but the reference is not exact, the error is always no such method.
if (!symbolicReferenceIsDefiningType) {
- // TODO(b/145775365): R8 incorrectly compiles the input to a working program.
- if (isR8) {
- result.assertSuccessWithOutput(EXPECTED);
- return;
- }
// TODO(b/145775365): D8/R8 does not preserve the thrown error.
if (parameters.isDexRuntime()) {
result.assertFailureWithErrorThatThrows(IllegalAccessError.class);
diff --git a/src/test/java/com/android/tools/r8/resolution/access/NestStaticMethodAccessWithIntermediateClassTest.java b/src/test/java/com/android/tools/r8/resolution/access/NestStaticMethodAccessWithIntermediateClassTest.java
index bd6dbed..e3ca436 100644
--- a/src/test/java/com/android/tools/r8/resolution/access/NestStaticMethodAccessWithIntermediateClassTest.java
+++ b/src/test/java/com/android/tools/r8/resolution/access/NestStaticMethodAccessWithIntermediateClassTest.java
@@ -92,20 +92,11 @@
.setMinApi(parameters.getApiLevel())
.addKeepMainRule(Main.class)
.run(parameters.getRuntime(), Main.class)
- .apply(
- result -> {
- if (inSameNest) {
- // TODO(b/145187969): R8 incorrectly compiles out the incorrect access.
- result.assertSuccessWithOutput(EXPECTED);
- } else {
- checkExpectedResult(result);
- }
- });
+ .apply(this::checkExpectedResult);
}
private void checkExpectedResult(TestRunResult<?> result) {
if (inSameNest && parameters.isCfRuntime()) {
- // TODO(b/145187969): Investigate if the change to NoSuchMethodError is according to spec?
result.assertFailureWithErrorThatMatches(containsString(NoSuchMethodError.class.getName()));
} else {
result.assertFailureWithErrorThatMatches(containsString(IllegalAccessError.class.getName()));
diff --git a/src/test/java/com/android/tools/r8/resolution/access/NestVirtualMethodAccessWithIntermediateClassTest.java b/src/test/java/com/android/tools/r8/resolution/access/NestVirtualMethodAccessWithIntermediateClassTest.java
index 3e85f1a..cdf2b22 100644
--- a/src/test/java/com/android/tools/r8/resolution/access/NestVirtualMethodAccessWithIntermediateClassTest.java
+++ b/src/test/java/com/android/tools/r8/resolution/access/NestVirtualMethodAccessWithIntermediateClassTest.java
@@ -91,23 +91,11 @@
.setMinApi(parameters.getApiLevel())
.addKeepMainRule(Main.class)
.run(parameters.getRuntime(), Main.class)
- .apply(
- result -> {
- if (inSameNest && parameters.isCfRuntime()) {
- // TODO(b/145187969): R8/CF compiles to a "working" program.
- result.assertSuccessWithOutput(EXPECTED);
- } else if (inSameNest && parameters.isDexRuntime()) {
- // TODO(b/145187969): R8/DEX compiles to a throw null program.
- result.assertFailureWithErrorThatThrows(NullPointerException.class);
- } else {
- checkExpectedResult(result);
- }
- });
+ .apply(this::checkExpectedResult);
}
private void checkExpectedResult(TestRunResult<?> result) {
if (inSameNest && parameters.isCfRuntime()) {
- // TODO(b/145187969): Investigate if the change to NoSuchMethodError is according to spec?
result.assertFailureWithErrorThatThrows(NoSuchMethodError.class);
} else {
result.assertFailureWithErrorThatThrows(IllegalAccessError.class);
diff --git a/src/test/java/com/android/tools/r8/transformers/ClassFileTransformer.java b/src/test/java/com/android/tools/r8/transformers/ClassFileTransformer.java
index 461323d..3d653d0 100644
--- a/src/test/java/com/android/tools/r8/transformers/ClassFileTransformer.java
+++ b/src/test/java/com/android/tools/r8/transformers/ClassFileTransformer.java
@@ -20,6 +20,7 @@
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
+import java.util.function.Consumer;
import java.util.stream.Collectors;
import org.objectweb.asm.ClassReader;
import org.objectweb.asm.ClassVisitor;
@@ -238,7 +239,27 @@
});
}
+ public ClassFileTransformer setPublic(Method method) {
+ return setAccessFlags(
+ method,
+ accessFlags -> {
+ accessFlags.unsetPrivate();
+ accessFlags.unsetProtected();
+ accessFlags.setPublic();
+ });
+ }
+
public ClassFileTransformer setPrivate(Method method) {
+ return setAccessFlags(
+ method,
+ accessFlags -> {
+ accessFlags.unsetPublic();
+ accessFlags.unsetProtected();
+ accessFlags.setPrivate();
+ });
+ }
+
+ public ClassFileTransformer setAccessFlags(Method method, Consumer<MethodAccessFlags> setter) {
return addClassTransformer(
new ClassTransformer() {
final MethodReference methodReference = Reference.methodFromMethod(method);
@@ -253,9 +274,7 @@
MethodAccessFlags.fromCfAccessFlags(access, isConstructor);
if (name.equals(methodReference.getMethodName())
&& descriptor.equals(methodReference.getMethodDescriptor())) {
- accessFlags.unsetPublic();
- accessFlags.unsetProtected();
- accessFlags.setPrivate();
+ setter.accept(accessFlags);
}
return super.visitMethod(
accessFlags.getAsCfAccessFlags(), name, descriptor, signature, exceptions);
