Move desugaring of backport methods to the enqueuer.
Bug: 150693139
Change-Id: If68b026b6498ac5a76fab76412ec45920ab60e0c
diff --git a/src/main/java/com/android/tools/r8/graph/DexEncodedMethod.java b/src/main/java/com/android/tools/r8/graph/DexEncodedMethod.java
index a972aed..aee51a6 100644
--- a/src/main/java/com/android/tools/r8/graph/DexEncodedMethod.java
+++ b/src/main/java/com/android/tools/r8/graph/DexEncodedMethod.java
@@ -67,6 +67,7 @@
import com.android.tools.r8.naming.MemberNaming.MethodSignature;
import com.android.tools.r8.naming.MemberNaming.Signature;
import com.android.tools.r8.naming.NamingLens;
+import com.android.tools.r8.position.MethodPosition;
import com.android.tools.r8.shaking.AnnotationRemover;
import com.android.tools.r8.shaking.AppInfoWithLiveness;
import com.android.tools.r8.utils.BooleanUtils;
@@ -1366,6 +1367,10 @@
return method;
}
+ public MethodPosition getPosition() {
+ return new MethodPosition(method.asMethodReference());
+ }
+
@Override
public boolean isDexEncodedMethod() {
checkIfObsolete();
diff --git a/src/main/java/com/android/tools/r8/graph/ProgramMethod.java b/src/main/java/com/android/tools/r8/graph/ProgramMethod.java
index a1fe5c7..0765824 100644
--- a/src/main/java/com/android/tools/r8/graph/ProgramMethod.java
+++ b/src/main/java/com/android/tools/r8/graph/ProgramMethod.java
@@ -11,6 +11,7 @@
import com.android.tools.r8.ir.conversion.MethodProcessor;
import com.android.tools.r8.logging.Log;
import com.android.tools.r8.origin.Origin;
+import com.android.tools.r8.position.MethodPosition;
/** Type representing a method definition in the programs compilation unit and its holder. */
public final class ProgramMethod extends DexClassAndMethod
@@ -81,4 +82,8 @@
assert holder.isProgramClass();
return holder.asProgramClass();
}
+
+ public MethodPosition getPosition() {
+ return getDefinition().getPosition();
+ }
}
diff --git a/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java b/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java
index b7964c8..ba89d0c 100644
--- a/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java
+++ b/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java
@@ -269,10 +269,13 @@
? new InterfaceMethodRewriter(appView, this)
: null;
this.twrCloseResourceRewriter =
- ((options.desugarState == DesugarState.ON) && enableTwrCloseResourceDesugaring())
+ (options.desugarState == DesugarState.ON && enableTwrCloseResourceDesugaring())
? new TwrCloseResourceRewriter(appView, this)
: null;
- this.backportedMethodRewriter = new BackportedMethodRewriter(appView);
+ this.backportedMethodRewriter =
+ (options.desugarState == DesugarState.ON && !appView.enableWholeProgramOptimizations())
+ ? new BackportedMethodRewriter(appView)
+ : null;
this.desugaredLibraryRetargeter =
options.desugaredLibraryConfiguration.getRetargetCoreLibMember().isEmpty()
? null
diff --git a/src/main/java/com/android/tools/r8/ir/desugar/BackportedMethodRewriter.java b/src/main/java/com/android/tools/r8/ir/desugar/BackportedMethodRewriter.java
index b45fe42..04fb914 100644
--- a/src/main/java/com/android/tools/r8/ir/desugar/BackportedMethodRewriter.java
+++ b/src/main/java/com/android/tools/r8/ir/desugar/BackportedMethodRewriter.java
@@ -37,6 +37,7 @@
import com.android.tools.r8.utils.AndroidApp;
import com.android.tools.r8.utils.InternalOptions;
import com.android.tools.r8.utils.InternalOptions.DesugarState;
+import com.android.tools.r8.utils.StringDiagnostic;
import com.android.tools.r8.utils.Timing;
import java.io.IOException;
import java.util.ArrayList;
@@ -106,9 +107,25 @@
}
public boolean desugar(ProgramMethod method, AppInfoWithClassHierarchy appInfo) {
+ return desugar(method, appInfo, synthesizedMethods::add);
+ }
+
+ public boolean desugar(
+ ProgramMethod method, AppInfoWithClassHierarchy appInfo, Consumer<ProgramMethod> consumer) {
if (!enabled) {
return false;
}
+ if (method.getDefinition().getCode().isDexCode()) {
+ appView
+ .options()
+ .reporter
+ .error(
+ new StringDiagnostic(
+ "Unsupported attempt to desugar DEX code",
+ method.getOrigin(),
+ method.getPosition()));
+ return false;
+ }
CfCode code = method.getDefinition().getCode().asCfCode();
ListIterator<CfInstruction> iterator = code.getInstructions().listIterator();
boolean replaced = false;
@@ -127,8 +144,7 @@
iterator = mutableInstructions.listIterator(iterator.previousIndex());
iterator.next();
}
- provider.rewriteInvoke(
- invoke, iterator, method.getHolder(), appInfo, synthesizedMethods::add);
+ provider.rewriteInvoke(invoke, iterator, method.getHolder(), appInfo, consumer);
replaced = true;
}
}
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 ec4e675..629506c 100644
--- a/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
+++ b/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
@@ -78,6 +78,7 @@
import com.android.tools.r8.ir.code.InvokeMethod;
import com.android.tools.r8.ir.code.InvokeVirtual;
import com.android.tools.r8.ir.code.Value;
+import com.android.tools.r8.ir.desugar.BackportedMethodRewriter;
import com.android.tools.r8.ir.desugar.DesugaredLibraryAPIConverter;
import com.android.tools.r8.ir.desugar.LambdaClass;
import com.android.tools.r8.ir.desugar.LambdaDescriptor;
@@ -340,11 +341,13 @@
private final GraphReporter graphReporter;
private final LambdaRewriter lambdaRewriter;
+ private final BackportedMethodRewriter backportRewriter;
private final DesugaredLibraryConversionWrapperAnalysis desugaredLibraryWrapperAnalysis;
private final Map<DexType, Pair<LambdaClass, ProgramMethod>> lambdaClasses =
new IdentityHashMap<>();
private final Map<DexEncodedMethod, Map<DexCallSite, LambdaClass>> lambdaCallSites =
new IdentityHashMap<>();
+ private final Map<DexMethod, ProgramMethod> methodsWithBackports = new IdentityHashMap<>();
private final Set<DexProgramClass> classesWithSerializableLambdas = Sets.newIdentityHashSet();
Enqueuer(
@@ -383,6 +386,8 @@
liveMethods = new LiveMethodsSet(graphReporter::registerMethod);
liveFields = new LiveFieldsSet(graphReporter::registerField);
lambdaRewriter = options.desugarState == DesugarState.ON ? new LambdaRewriter(appView) : null;
+ backportRewriter =
+ options.desugarState == DesugarState.ON ? new BackportedMethodRewriter(appView) : null;
objectAllocationInfoCollection =
ObjectAllocationInfoCollectionImpl.builder(mode.isInitialTreeShaking(), graphReporter);
@@ -1077,6 +1082,10 @@
private void traceInvokeDirect(
DexMethod invokedMethod, ProgramMethod context, KeepReason reason) {
+ if (registerBackportInvoke(invokedMethod, context)) {
+ return;
+ }
+
if (!registerMethodWithTargetAndContext(
methodAccessInfoCollection::registerInvokeDirectInContext, invokedMethod, context)) {
return;
@@ -1098,6 +1107,10 @@
private void traceInvokeInterface(
DexMethod method, ProgramMethod context, KeepReason keepReason) {
+ if (registerBackportInvoke(method, context)) {
+ return;
+ }
+
if (!registerMethodWithTargetAndContext(
methodAccessInfoCollection::registerInvokeInterfaceInContext, method, context)) {
return;
@@ -1117,8 +1130,20 @@
traceInvokeStatic(invokedMethod, context, KeepReason.invokedFromLambdaCreatedIn(context));
}
+ private boolean registerBackportInvoke(DexMethod invokedMethod, ProgramMethod context) {
+ if (backportRewriter != null && backportRewriter.needsDesugaring(invokedMethod)) {
+ methodsWithBackports.putIfAbsent(context.getReference(), context);
+ return true;
+ }
+ return false;
+ }
+
private void traceInvokeStatic(
DexMethod invokedMethod, ProgramMethod context, KeepReason reason) {
+ if (registerBackportInvoke(invokedMethod, context)) {
+ return;
+ }
+
DexItemFactory dexItemFactory = appView.dexItemFactory();
if (dexItemFactory.classMethods.isReflectiveClassLookup(invokedMethod)
|| dexItemFactory.atomicFieldUpdaterMethods.isFieldUpdater(invokedMethod)) {
@@ -1150,6 +1175,9 @@
}
void traceInvokeSuper(DexMethod invokedMethod, ProgramMethod context) {
+ if (registerBackportInvoke(invokedMethod, context)) {
+ return;
+ }
// We have to revisit super invokes based on the context they are found in. The same
// method descriptor will hit different targets, depending on the context it is used in.
DexMethod actualTarget = getInvokeSuperTarget(invokedMethod, context);
@@ -1174,6 +1202,10 @@
private void traceInvokeVirtual(
DexMethod invokedMethod, ProgramMethod context, KeepReason reason) {
+ if (registerBackportInvoke(invokedMethod, context)) {
+ return;
+ }
+
if (invokedMethod == appView.dexItemFactory().classMethods.newInstance
|| invokedMethod == appView.dexItemFactory().constructorMethods.newInstance) {
pendingReflectiveUses.add(context);
@@ -2892,6 +2924,7 @@
synthesizeInterfaceMethodBridges(additions);
synthesizeLambdas(additions);
synthesizeLibraryConversionWrappers(additions);
+ synthesizeBackports(additions);
if (additions.isEmpty()) {
return;
}
@@ -2923,6 +2956,12 @@
syntheticInterfaceMethodBridges.clear();
}
+ private void synthesizeBackports(SyntheticAdditions additions) {
+ for (ProgramMethod method : methodsWithBackports.values()) {
+ backportRewriter.desugar(method, appInfo, additions::addLiveMethod);
+ }
+ }
+
private void synthesizeLambdas(SyntheticAdditions additions) {
if (lambdaRewriter == null || lambdaClasses.isEmpty()) {
assert lambdaCallSites.isEmpty();
diff --git a/src/main/java/com/android/tools/r8/shaking/StaticClassMerger.java b/src/main/java/com/android/tools/r8/shaking/StaticClassMerger.java
index abd2a99..974fdb1 100644
--- a/src/main/java/com/android/tools/r8/shaking/StaticClassMerger.java
+++ b/src/main/java/com/android/tools/r8/shaking/StaticClassMerger.java
@@ -248,6 +248,12 @@
}
private MergeGroup getMergeGroup(DexProgramClass clazz) {
+ if (appView.getSyntheticItems().isSyntheticClass(clazz)) {
+ // In principle it would be valid to merge synthetic classes into program classes.
+ // Doing so may however increase size as static methods will not be de-duplicated
+ // at synthetic finalization.
+ return MergeGroup.DONT_MERGE;
+ }
if (appView.appInfo().getNoStaticClassMergingSet().contains(clazz.type)) {
return MergeGroup.DONT_MERGE;
}
diff --git a/src/main/java/com/android/tools/r8/synthesis/SyntheticFinalization.java b/src/main/java/com/android/tools/r8/synthesis/SyntheticFinalization.java
index 9dd8bee..f8c58b7 100644
--- a/src/main/java/com/android/tools/r8/synthesis/SyntheticFinalization.java
+++ b/src/main/java/com/android/tools/r8/synthesis/SyntheticFinalization.java
@@ -418,6 +418,10 @@
SyntheticMethodDefinition method = (SyntheticMethodDefinition) definition;
if (SyntheticMethodBuilder.isValidSyntheticMethod(method.getMethod().getDefinition())) {
methods.add(method);
+ } else {
+ // Failing this check indicates that an optimization has modified the synthetic in a
+ // disruptive way.
+ assert false;
}
}
return methods;
diff --git a/src/main/java/com/android/tools/r8/synthesis/SyntheticItems.java b/src/main/java/com/android/tools/r8/synthesis/SyntheticItems.java
index 6f5c49d..6a5e9a3 100644
--- a/src/main/java/com/android/tools/r8/synthesis/SyntheticItems.java
+++ b/src/main/java/com/android/tools/r8/synthesis/SyntheticItems.java
@@ -308,7 +308,15 @@
ImmutableMap.Builder<DexType, SyntheticReference> rewrittenItems = ImmutableMap.builder();
for (SyntheticReference reference : nonLecacySyntheticItems.values()) {
SyntheticReference rewritten = reference.rewrite(lens);
- rewrittenItems.put(rewritten.getHolder(), rewritten);
+ // If the reference has been rewritten the compiler has changed it and we drop it from the
+ // set of synthetics.
+ if (reference == rewritten) {
+ rewrittenItems.put(rewritten.getHolder(), rewritten);
+ } else {
+ // If the item is rewritten, it should be moved to another holder as the synthetic holder
+ // is no longer part of the synthetic collection.
+ assert reference.getHolder() != rewritten.getHolder();
+ }
}
// No pending item should need rewriting.
assert legacyPendingClasses.keySet().equals(lens.rewriteTypes(legacyPendingClasses.keySet()));
diff --git a/src/main/java/com/android/tools/r8/synthesis/SyntheticMethodReference.java b/src/main/java/com/android/tools/r8/synthesis/SyntheticMethodReference.java
index 86704ea..571331b 100644
--- a/src/main/java/com/android/tools/r8/synthesis/SyntheticMethodReference.java
+++ b/src/main/java/com/android/tools/r8/synthesis/SyntheticMethodReference.java
@@ -35,8 +35,8 @@
return null;
}
assert clazz.isProgramClass();
- ProgramMethod definition = clazz.asProgramClass().lookupProgramMethod(this.method);
- return new SyntheticMethodDefinition(getContext(), definition);
+ ProgramMethod definition = clazz.asProgramClass().lookupProgramMethod(method);
+ return definition != null ? new SyntheticMethodDefinition(getContext(), definition) : null;
}
@Override
diff --git a/src/test/java/com/android/tools/r8/R8RunArtTestsTest.java b/src/test/java/com/android/tools/r8/R8RunArtTestsTest.java
index 67721e4..d14254e 100644
--- a/src/test/java/com/android/tools/r8/R8RunArtTestsTest.java
+++ b/src/test/java/com/android/tools/r8/R8RunArtTestsTest.java
@@ -1032,6 +1032,12 @@
.put("552-checker-primitive-typeprop", TestCondition.match(TestCondition.R8DEX_COMPILER))
// Test with invalid register usage: invoke-static {v0,v0}, foo(IL)V
.put("557-checker-ref-equivalent", TestCondition.match(TestCondition.R8DEX_COMPILER))
+ // Test with smali code that calls a method that needs to be desugared.
+ // The smali code is only present in the non-legacy test distrubution, so this only fails
+ // when running the "default" runtime.
+ .put(
+ "567-checker-compare",
+ TestCondition.match(TestCondition.runtimes(Runtime.ART_DEFAULT)))
// This test is starting from invalid dex code. It splits up a double value and uses
// the first register of a double with the second register of another double.
.put("800-smali", TestCondition.match(TestCondition.R8DEX_COMPILER))
@@ -2398,7 +2404,7 @@
CompilationMode compilationMode)
throws Throwable {
if (specification.expectedToFailWithX8) {
- expectException(CompilationError.class);
+ expectedException = true;
try {
executeCompilerUnderTest(
compilerUnderTest,
@@ -2406,7 +2412,8 @@
resultDir.getCanonicalPath(),
compilationMode,
new CompilationOptions(specification));
- } catch (CompilationFailedException e) {
+ } catch (CompilationFailedException | CompilationError e) {
+ expectException(CompilationError.class);
throw new CompilationError(e.getMessage(), e);
}
System.err.println("Should have failed R8/D8 compilation with a CompilationError.");
diff --git a/src/test/java/com/android/tools/r8/R8RunExamplesCommon.java b/src/test/java/com/android/tools/r8/R8RunExamplesCommon.java
index 5a90d52..d288099 100644
--- a/src/test/java/com/android/tools/r8/R8RunExamplesCommon.java
+++ b/src/test/java/com/android/tools/r8/R8RunExamplesCommon.java
@@ -17,6 +17,7 @@
import com.android.tools.r8.utils.InternalOptions.LineNumberOptimization;
import com.android.tools.r8.utils.TestDescriptionWatcher;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
import java.io.IOException;
import java.nio.file.NoSuchFileException;
import java.nio.file.Path;
@@ -172,6 +173,10 @@
}
private boolean shouldCompileFail() {
+ if (input == Input.DX && getFailingCompileDxToDex().contains(mainClass)) {
+ assert output == Output.DEX;
+ return true;
+ }
if (output == Output.CF && getFailingCompileCf().contains(mainClass)) {
return true;
}
@@ -261,6 +266,10 @@
protected abstract Set<String> getFailingCompileCfToDex();
+ protected Set<String> getFailingCompileDxToDex() {
+ return ImmutableSet.of();
+ }
+
// TODO(mathiasr): Add CompilerSet for CfToDex so we can fold this into getFailingRun().
protected abstract Set<String> getFailingRunCfToDex();
diff --git a/src/test/java/com/android/tools/r8/R8RunExamplesTest.java b/src/test/java/com/android/tools/r8/R8RunExamplesTest.java
index 5eaef9e..04ff03f 100644
--- a/src/test/java/com/android/tools/r8/R8RunExamplesTest.java
+++ b/src/test/java/com/android/tools/r8/R8RunExamplesTest.java
@@ -158,6 +158,15 @@
}
@Override
+ protected Set<String> getFailingCompileDxToDex() {
+ return new ImmutableSet.Builder<String>()
+ .add("regress_72361252.Test") // requires desugar
+ .add("regress_70703087.Test") // requires desugar
+ .add("regress_70737019.Test") // requires desugar
+ .build();
+ }
+
+ @Override
protected Set<String> getFailingCompileCf() {
return new ImmutableSet.Builder<String>()
.build();
diff --git a/src/test/java/com/android/tools/r8/desugar/backports/BackportDuplicationTest.java b/src/test/java/com/android/tools/r8/desugar/backports/BackportDuplicationTest.java
index d4346cc..b4a4c48 100644
--- a/src/test/java/com/android/tools/r8/desugar/backports/BackportDuplicationTest.java
+++ b/src/test/java/com/android/tools/r8/desugar/backports/BackportDuplicationTest.java
@@ -7,6 +7,7 @@
import static org.hamcrest.CoreMatchers.not;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
import com.android.tools.r8.OutputMode;
import com.android.tools.r8.TestBase;
@@ -19,6 +20,7 @@
import com.android.tools.r8.utils.StringUtils;
import com.android.tools.r8.utils.SyntheticItemsTestUtils;
import com.android.tools.r8.utils.codeinspector.CodeInspector;
+import com.android.tools.r8.utils.codeinspector.InstructionSubject;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Sets;
@@ -69,7 +71,7 @@
.minification(minify)
.run(parameters.getRuntime(), TestClass.class)
.assertSuccessWithOutput(EXPECTED)
- .inspect(this::checkNoInternalSyntheticNames);
+ .inspect(this::checkNoOriginalsAndNoInternalSynthetics);
}
@Test
@@ -79,7 +81,7 @@
.setMinApi(parameters.getApiLevel())
.run(parameters.getRuntime(), TestClass.class)
.assertSuccessWithOutput(EXPECTED)
- .inspect(this::checkNoInternalSyntheticNames)
+ .inspect(this::checkNoOriginalsAndNoInternalSynthetics)
.inspect(this::checkExpectedSynthetics);
}
@@ -133,7 +135,7 @@
.writeToZip(out3)
.run(parameters.getRuntime(), TestClass.class)
.assertSuccessWithOutput(EXPECTED)
- .inspect(this::checkNoInternalSyntheticNames)
+ .inspect(this::checkNoOriginalsAndNoInternalSynthetics)
.inspect(inspector -> assertEquals(syntheticsInParts, getSyntheticMethods(inspector)));
// Finally do a non-intermediate merge.
@@ -142,7 +144,7 @@
.setMinApi(parameters.getApiLevel())
.run(parameters.getRuntime(), TestClass.class)
.assertSuccessWithOutput(EXPECTED)
- .inspect(this::checkNoInternalSyntheticNames)
+ .inspect(this::checkNoOriginalsAndNoInternalSynthetics)
.inspect(
inspector -> {
if (intermediate) {
@@ -179,16 +181,28 @@
.setMinApi(parameters.getApiLevel())
.run(parameters.getRuntime(), TestClass.class)
.assertSuccessWithOutput(EXPECTED)
- .inspect(this::checkNoInternalSyntheticNames)
+ .inspect(this::checkNoOriginalsAndNoInternalSynthetics)
.inspect(this::checkExpectedSynthetics);
}
- private void checkNoInternalSyntheticNames(CodeInspector inspector) {
+ private void checkNoOriginalsAndNoInternalSynthetics(CodeInspector inspector) {
inspector.forAllClasses(
clazz -> {
assertThat(
clazz.getFinalName(),
not(containsString(SyntheticItems.INTERNAL_SYNTHETIC_CLASS_SEPARATOR)));
+ if (!clazz.getOriginalName().equals(MiniAssert.class.getTypeName())) {
+ clazz.forAllMethods(
+ method ->
+ assertTrue(
+ "Unexpected static invoke to java.lang method:\n"
+ + method.getMethod().codeToString(),
+ method
+ .streamInstructions()
+ .filter(InstructionSubject::isInvokeStatic)
+ .noneMatch(
+ i -> i.getMethod().qualifiedName().startsWith("java.lang"))));
+ }
});
}
diff --git a/src/test/java/com/android/tools/r8/enumunboxing/NullCheckEnumUnboxingTest.java b/src/test/java/com/android/tools/r8/enumunboxing/NullCheckEnumUnboxingTest.java
index 25a3434..0f313cf 100644
--- a/src/test/java/com/android/tools/r8/enumunboxing/NullCheckEnumUnboxingTest.java
+++ b/src/test/java/com/android/tools/r8/enumunboxing/NullCheckEnumUnboxingTest.java
@@ -8,7 +8,6 @@
import com.android.tools.r8.NeverInline;
import com.android.tools.r8.R8TestRunResult;
import com.android.tools.r8.TestParameters;
-import com.android.tools.r8.utils.AndroidApiLevel;
import java.util.List;
import java.util.Objects;
import org.junit.Test;
@@ -51,13 +50,10 @@
.inspectDiagnosticMessages(
m -> {
assertEnumIsUnboxed(MyEnum.class, MyEnum.class.getSimpleName(), m);
- // MyEnum19 is unboxed only if minAPI > 19 because Objects#requiredNonNull is then
- // present.
- if (parameters.getApiLevel().isGreaterThanOrEqualTo(AndroidApiLevel.K)) {
- assertEnumIsUnboxed(MyEnum19.class, MyEnum19.class.getSimpleName(), m);
- } else {
- assertEnumIsBoxed(MyEnum19.class, MyEnum19.class.getSimpleName(), m);
- }
+ // MyEnum19 is always unboxed. If minAPI > 19 the unboxer will identify
+ // Objects#requiredNonNull usage. For 19 and prior, the backport code should not
+ // prohibit the unboxing either.
+ assertEnumIsUnboxed(MyEnum19.class, MyEnum19.class.getSimpleName(), m);
})
.run(parameters.getRuntime(), MainNullTest.class)
.assertSuccess();
