Restrict lambda unpinning to moved methods.
Bug: 157700141
Change-Id: I4d74729a70fd5864a26597714af0fb38df52166d
diff --git a/src/main/java/com/android/tools/r8/ir/desugar/LambdaClass.java b/src/main/java/com/android/tools/r8/ir/desugar/LambdaClass.java
index 0f050c5..c9e47c2 100644
--- a/src/main/java/com/android/tools/r8/ir/desugar/LambdaClass.java
+++ b/src/main/java/com/android/tools/r8/ir/desugar/LambdaClass.java
@@ -632,7 +632,7 @@
encodedMethod.getCode(),
true);
newMethod.copyMetadata(encodedMethod);
- rewriter.lensBuilder.move(encodedMethod.method, callTarget);
+ rewriter.forcefullyMoveMethod(encodedMethod.method, callTarget);
DexEncodedMethod.setDebugInfoWithFakeThisParameter(
newMethod.getCode(), callTarget.getArity(), appView);
@@ -705,7 +705,7 @@
encodedMethod.getCode(),
true);
newMethod.copyMetadata(encodedMethod);
- rewriter.lensBuilder.move(encodedMethod.method, callTarget);
+ rewriter.forcefullyMoveMethod(encodedMethod.method, callTarget);
return newMethod;
});
return new ProgramMethod(implMethodHolder, replacement);
diff --git a/src/main/java/com/android/tools/r8/ir/desugar/LambdaRewriter.java b/src/main/java/com/android/tools/r8/ir/desugar/LambdaRewriter.java
index 7bb8d44..11cff1a 100644
--- a/src/main/java/com/android/tools/r8/ir/desugar/LambdaRewriter.java
+++ b/src/main/java/com/android/tools/r8/ir/desugar/LambdaRewriter.java
@@ -46,6 +46,7 @@
import com.google.common.base.Suppliers;
import com.google.common.collect.BiMap;
import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
import java.util.ArrayList;
import java.util.Collection;
import java.util.IdentityHashMap;
@@ -77,7 +78,8 @@
final DexString instanceFieldName;
- final LambdaRewriterLens.Builder lensBuilder = LambdaRewriterLens.builder();
+ private final LambdaRewriterLens.Builder lensBuilder = LambdaRewriterLens.builder();
+ private final Set<DexMethod> forcefullyMovedMethods = Sets.newIdentityHashSet();
// Maps call sites seen so far to inferred lambda descriptor. It is intended
// to help avoid re-matching call sites we already seen. Note that same call
@@ -95,6 +97,15 @@
this.instanceFieldName = appView.dexItemFactory().createString(LAMBDA_INSTANCE_FIELD_NAME);
}
+ void forcefullyMoveMethod(DexMethod from, DexMethod to) {
+ lensBuilder.move(from, to);
+ forcefullyMovedMethods.add(from);
+ }
+
+ public Set<DexMethod> getForcefullyMovedMethods() {
+ return forcefullyMovedMethods;
+ }
+
private void synthesizeAccessibilityBridgesForLambdaClassesD8(
Collection<LambdaClass> lambdaClasses, IRConverter converter, ExecutorService executorService)
throws ExecutionException {
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 d1200b4..21bf839 100644
--- a/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
+++ b/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
@@ -263,8 +263,6 @@
* Set of direct methods that are the immediate target of an invoke-dynamic.
*/
private final Set<DexMethod> methodsTargetedByInvokeDynamic = Sets.newIdentityHashSet();
- /** Set of direct lambda implementation methods that have been desugared, thus they may move. */
- private final Set<DexMethod> desugaredLambdaImplementationMethods = Sets.newIdentityHashSet();
/**
* Set of virtual methods that are the immediate target of an invoke-direct.
*/
@@ -865,9 +863,6 @@
classesWithSerializableLambdas.add(context.getHolder());
}
}
- if (descriptor.delegatesToLambdaImplMethod()) {
- desugaredLambdaImplementationMethods.add(descriptor.implHandle.asMethod());
- }
} else {
markLambdaAsInstantiated(descriptor, context);
transitionMethodsForInstantiatedLambda(descriptor);
@@ -3088,6 +3083,16 @@
liveMethods.add(accessor, graphReporter.fakeReportShouldNotBeUsed());
}
});
+ unpinLambdaMethods();
+ }
+
+ // TODO(b/157700141): Determine if this is the right way to allow modification of pinned lambdas.
+ private void unpinLambdaMethods() {
+ assert lambdaRewriter != null;
+ for (DexMethod method : lambdaRewriter.getForcefullyMovedMethods()) {
+ keepInfo.unsafeUnpinMethod(method);
+ rootSet.prune(method);
+ }
}
private boolean verifyMissingTypes() {
@@ -3296,7 +3301,6 @@
} finally {
timing.end();
}
- unpinLambdaMethods();
}
private long getNumberOfLiveItems() {
@@ -3392,17 +3396,6 @@
action.getAction().accept(builder);
}
- // TODO(b/157700141): Determine if this is the right way to avoid modification of pinned lambdas.
- private void unpinLambdaMethods() {
- assert desugaredLambdaImplementationMethods.isEmpty()
- || options.desugarState == DesugarState.ON;
- for (DexMethod method : desugaredLambdaImplementationMethods) {
- keepInfo.unsafeUnpinMethod(method);
- rootSet.prune(method);
- }
- desugaredLambdaImplementationMethods.clear();
- }
-
void retainAnnotationForFinalTreeShaking(List<DexAnnotation> annotations) {
assert mode.isInitialTreeShaking();
if (annotationRemoverBuilder != null) {
diff --git a/src/test/java/com/android/tools/r8/desugar/DesugarLambdaWithAnonymousClass.java b/src/test/java/com/android/tools/r8/desugar/DesugarLambdaWithAnonymousClass.java
index b2d5b54..5d84439 100644
--- a/src/test/java/com/android/tools/r8/desugar/DesugarLambdaWithAnonymousClass.java
+++ b/src/test/java/com/android/tools/r8/desugar/DesugarLambdaWithAnonymousClass.java
@@ -113,18 +113,17 @@
public void testR8() throws Exception {
try {
testForR8(parameters.getBackend())
- // TODO(b/158752316, b/157700141): Disable inlining to keep the synthetic lambda methods.
- .addOptionsModification(options -> options.enableInlining = false)
.addInnerClasses(DesugarLambdaWithAnonymousClass.class)
.setMinApi(parameters.getApiLevel())
- .addKeepRules("-keep class * { *; }")
- // Keeping the synthetic lambda methods is currently not supported - they are
- // forcefully unpinned. The following rule has no effect. See b/b/157700141.
- .addKeepRules("-keep class **.*$TestClass { synthetic *; }")
+ // Keep the synthesized inner classes.
+ .addKeepRules("-keep class **.*$TestClass$1")
+ .addKeepRules("-keep class **.*$TestClass$2")
+ // Keep the outer context: TestClass *and* the synthetic lambda methods.
+ .addKeepRules("-keep class **.*$TestClass { private synthetic void lambda$*(*); }")
.addKeepAttributes("InnerClasses", "EnclosingMethod")
- .compile()
- .inspect(this::checkEnclosingMethod)
+ .addKeepMainRule(TestClass.class)
.run(parameters.getRuntime(), TestClass.class)
+ .inspect(this::checkEnclosingMethod)
.assertSuccessWithOutputLines(
parameters.isCfRuntime() ? EXPECTED_JAVAC_RESULT : EXPECTED_DESUGARED_RESULT);
assertFalse(parameters.isDexRuntime());
diff --git a/src/test/java/com/android/tools/r8/desugar/DesugarLambdaWithLocalClass.java b/src/test/java/com/android/tools/r8/desugar/DesugarLambdaWithLocalClass.java
index 8c539d2..f272f26 100644
--- a/src/test/java/com/android/tools/r8/desugar/DesugarLambdaWithLocalClass.java
+++ b/src/test/java/com/android/tools/r8/desugar/DesugarLambdaWithLocalClass.java
@@ -36,9 +36,6 @@
"Hello from inside lambda$test$0$DesugarLambdaWithLocalClass$TestClass",
"Hello from inside lambda$testStatic$1");
- private List<String> EXPECTED_DESUGARED_RESULT_R8_WITHOUT_INLINING =
- ImmutableList.of("Hello from inside lambda$test$0", "Hello from inside lambda$testStatic$1");
-
@Parameterized.Parameters(name = "{0}")
public static TestParametersCollection data() {
return getTestParameters().withAllRuntimes().withAllApiLevelsAlsoForCf().build();
@@ -113,22 +110,18 @@
@Test
public void testR8() throws Exception {
testForR8(parameters.getBackend())
- // TODO(b/158752316, b/157700141): Disable inlining to keep the synthetic lambda methods.
- .addOptionsModification(options -> options.enableInlining = false)
.addInnerClasses(DesugarLambdaWithLocalClass.class)
.setMinApi(parameters.getApiLevel())
- .addKeepRules("-keep class * { *; }")
- // Keeping the synthetic lambda methods is currently not supported - they are
- // forcefully unpinned. The following rule has no effect. See b/b/157700141.
- .addKeepRules("-keep class **.*$TestClass { synthetic *; }")
+ // Keep the synthesized inner classes.
+ .addKeepRules("-keep class **.*$TestClass$*MyConsumerImpl")
+ // Keep the outer context: TestClass *and* the synthetic lambda methods.
+ .addKeepRules("-keep class **.*$TestClass { private synthetic void lambda$*(*); }")
.addKeepAttributes("InnerClasses", "EnclosingMethod")
- .compile()
- .inspect(this::checkEnclosingMethod)
+ .addKeepMainRule(TestClass.class)
.run(parameters.getRuntime(), TestClass.class)
+ .inspect(this::checkEnclosingMethod)
.assertSuccessWithOutputLines(
- parameters.isCfRuntime()
- ? EXPECTED_JAVAC_RESULT
- : EXPECTED_DESUGARED_RESULT_R8_WITHOUT_INLINING);
+ parameters.isCfRuntime() ? EXPECTED_JAVAC_RESULT : EXPECTED_JAVAC_RESULT);
}
public interface MyConsumer<T> {
