Version 1.3.44
Merge: Sanitize quasi-trivial phis in re-processing IR code.
CL: https://r8-review.googlesource.com/c/r8/+/31240/
Merge: Remove the associated Kotlin Metadata if a class is renamed.
CL: https://r8-review.googlesource.com/c/r8/+/30840/
Bug: 119414448, 119626580
Change-Id: Ief02aff8bd8f1a046536da75cd88c680b7321ddc
diff --git a/src/main/java/com/android/tools/r8/Version.java b/src/main/java/com/android/tools/r8/Version.java
index 57a282f..0c63c7b 100644
--- a/src/main/java/com/android/tools/r8/Version.java
+++ b/src/main/java/com/android/tools/r8/Version.java
@@ -11,7 +11,7 @@
// This field is accessed from release scripts using simple pattern matching.
// Therefore, changing this field could break our release scripts.
- public static final String LABEL = "1.3.43";
+ public static final String LABEL = "1.3.44";
private Version() {
}
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/lambda/LambdaMerger.java b/src/main/java/com/android/tools/r8/ir/optimize/lambda/LambdaMerger.java
index d1b56f2..4c9b82a 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/lambda/LambdaMerger.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/lambda/LambdaMerger.java
@@ -304,9 +304,11 @@
.sorted(DexEncodedMethod::slowCompare)
.collect(Collectors.toList());
for (DexEncodedMethod method : methods) {
- converter.processMethod(method, feedback,
+ DexEncodedMethod mappedMethod =
+ converter.getGraphLense().mapDexEncodedMethod(converter.appInfo, method);
+ converter.processMethod(mappedMethod, feedback,
x -> false, CallSiteInformation.empty(), Outliner::noProcessing);
- assert method.isProcessed();
+ assert mappedMethod.isProcessed();
}
}
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/staticizer/StaticizingProcessor.java b/src/main/java/com/android/tools/r8/ir/optimize/staticizer/StaticizingProcessor.java
index 18a9920..79f18ed 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/staticizer/StaticizingProcessor.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/staticizer/StaticizingProcessor.java
@@ -20,6 +20,7 @@
import com.android.tools.r8.ir.code.InvokeMethodWithReceiver;
import com.android.tools.r8.ir.code.InvokeStatic;
import com.android.tools.r8.ir.code.MemberType;
+import com.android.tools.r8.ir.code.Phi;
import com.android.tools.r8.ir.code.StaticGet;
import com.android.tools.r8.ir.code.StaticPut;
import com.android.tools.r8.ir.code.Value;
@@ -35,6 +36,7 @@
import com.google.common.collect.Streams;
import java.util.ArrayList;
import java.util.HashMap;
+import java.util.HashSet;
import java.util.IdentityHashMap;
import java.util.Iterator;
import java.util.List;
@@ -238,7 +240,7 @@
}
private void removeReferencesToThis(DexEncodedMethod method, IRCode code) {
- fixupStaticizedValueUsers(code, code.getThis());
+ fixupStaticizedThisUsers(code, code.getThis());
}
private void rewriteReferences(DexEncodedMethod method, IRCode code) {
@@ -251,11 +253,12 @@
.collect(Collectors.toList());
singletonFieldReads.forEach(read -> {
- CandidateInfo candidateInfo = singletonFields.get(read.getField());
+ DexField field = read.getField();
+ CandidateInfo candidateInfo = singletonFields.get(field);
assert candidateInfo != null;
Value value = read.dest();
if (value != null) {
- fixupStaticizedValueUsers(code, value);
+ fixupStaticizedFieldReadUsers(code, value, field);
}
if (!candidateInfo.preserveRead.get()) {
read.removeOrReplaceByDebugLocalRead();
@@ -267,12 +270,116 @@
}
}
- // Fixup value usages: rewrites all method calls so that they point to static methods.
- private void fixupStaticizedValueUsers(IRCode code, Value thisValue) {
+ // Fixup `this` usages: rewrites all method calls so that they point to static methods.
+ private void fixupStaticizedThisUsers(IRCode code, Value thisValue) {
assert thisValue != null;
assert thisValue.numberOfPhiUsers() == 0;
- for (Instruction user : thisValue.uniqueUsers()) {
+ fixupStaticizedValueUsers(code, thisValue.uniqueUsers());
+
+ assert thisValue.numberOfUsers() == 0;
+ }
+
+ // Re-processing finalized code may create slightly different IR code than what the examining
+ // phase has seen. For example,
+ //
+ // b1:
+ // s1 <- static-get singleton
+ // ...
+ // invoke-virtual { s1, ... } mtd1
+ // goto Exit
+ // b2:
+ // s2 <- static-get singleoton
+ // ...
+ // invoke-virtual { s2, ... } mtd1
+ // goto Exit
+ // ...
+ // Exit: ...
+ //
+ // ~>
+ //
+ // b1:
+ // s1 <- static-get singleton
+ // ...
+ // goto Exit
+ // b2:
+ // s2 <- static-get singleton
+ // ...
+ // goto Exit
+ // Exit:
+ // sp <- phi(s1, s2)
+ // invoke-virtual { sp, ... } mtd1
+ // ...
+ //
+ // From staticizer's viewpoint, `sp` is trivial in the sense that it is composed of values that
+ // refer to the same singleton field. If so, we can safely relax the assertion; remove uses of
+ // field reads; remove quasi-trivial phis; and then remove original field reads.
+ private boolean testAndcollectPhisComposedOfSameFieldRead(
+ Set<Phi> phisToCheck, DexField field, Set<Phi> trivialPhis) {
+ for (Phi phi : phisToCheck) {
+ Set<Phi> chainedPhis = Sets.newIdentityHashSet();
+ for (Value operand : phi.getOperands()) {
+ if (operand.isPhi()) {
+ chainedPhis.add(operand.asPhi());
+ } else {
+ if (!operand.definition.isStaticGet()) {
+ return false;
+ }
+ if (operand.definition.asStaticGet().getField() != field) {
+ return false;
+ }
+ }
+ }
+ if (!chainedPhis.isEmpty()) {
+ if (!testAndcollectPhisComposedOfSameFieldRead(chainedPhis, field, trivialPhis)) {
+ return false;
+ }
+ }
+ trivialPhis.add(phi);
+ }
+ return true;
+ }
+
+ // Fixup field read usages. Same as {@link #fixupStaticizedThisUsers} except this one is handling
+ // quasi-trivial phis that might be introduced while re-processing finalized code.
+ private void fixupStaticizedFieldReadUsers(IRCode code, Value dest, DexField field) {
+ assert dest != null;
+ // During the examine phase, field reads with any phi users have been invalidated, hence zero.
+ // However, it may be not true if re-processing introduces phis after optimizing common suffix.
+ Set<Phi> trivialPhis = Sets.newIdentityHashSet();
+ boolean hasTrivialPhis =
+ testAndcollectPhisComposedOfSameFieldRead(dest.uniquePhiUsers(), field, trivialPhis);
+ assert dest.numberOfPhiUsers() == 0 || hasTrivialPhis;
+ Set<Instruction> users = new HashSet<>(dest.uniqueUsers());
+ // If that is the case, method calls we want to fix up include users of those phis.
+ if (hasTrivialPhis) {
+ for (Phi phi : trivialPhis) {
+ users.addAll(phi.uniqueUsers());
+ }
+ }
+
+ fixupStaticizedValueUsers(code, users);
+
+ if (hasTrivialPhis) {
+ // We can't directly use Phi#removeTrivialPhi because they still refer to different operands.
+ for (Phi phi : trivialPhis) {
+ // First, make sure phi users are indeed uses of field reads and removed via fixup.
+ assert phi.numberOfUsers() == 0;
+ // Then, manually clean up this from all of the operands.
+ for (Value operand : phi.getOperands()) {
+ operand.removePhiUser(phi);
+ }
+ // And remove it from the containing block.
+ phi.getBlock().removePhi(phi);
+ }
+ }
+
+ // No matter what, number of phi users should be zero too.
+ assert dest.numberOfUsers() == 0 && dest.numberOfPhiUsers() == 0;
+ }
+
+ private void fixupStaticizedValueUsers(IRCode code, Set<Instruction> users) {
+ for (Instruction user : users) {
assert user.isInvokeVirtual() || user.isInvokeDirect();
InvokeMethodWithReceiver invoke = user.asInvokeMethodWithReceiver();
Value newValue = null;
@@ -288,8 +395,6 @@
invoke.replace(new InvokeStatic(
invoke.getInvokedMethod(), newValue, args.subList(1, args.size())));
}
-
- assert thisValue.numberOfUsers() == 0;
}
private void remapMovedCandidates(IRCode code) {
diff --git a/src/main/java/com/android/tools/r8/naming/ClassNameMinifier.java b/src/main/java/com/android/tools/r8/naming/ClassNameMinifier.java
index 37e6de9..ecf7158 100644
--- a/src/main/java/com/android/tools/r8/naming/ClassNameMinifier.java
+++ b/src/main/java/com/android/tools/r8/naming/ClassNameMinifier.java
@@ -124,6 +124,7 @@
timing.begin("rename-classes");
for (DexClass clazz : classes) {
if (!renaming.containsKey(clazz.type)) {
+ clazz.annotations = clazz.annotations.keepIf(this::isNotKotlinMetadata);
DexString renamed = computeName(clazz.type);
renaming.put(clazz.type, renamed);
}
@@ -581,4 +582,8 @@
}
return packagePrefix.substring(0, i);
}
+
+ private boolean isNotKotlinMetadata(DexAnnotation annotation) {
+ return annotation.annotation.type != appInfo.dexItemFactory.kotlin.metadata.kotlinMetadataType;
+ }
}
diff --git a/src/test/java/com/android/tools/r8/ToolHelper.java b/src/test/java/com/android/tools/r8/ToolHelper.java
index e185b33..c1ba2c5 100644
--- a/src/test/java/com/android/tools/r8/ToolHelper.java
+++ b/src/test/java/com/android/tools/r8/ToolHelper.java
@@ -99,6 +99,7 @@
public static final String JAVA_8_RUNTIME = "third_party/openjdk/openjdk-rt-1.8/rt.jar";
public static final String KT_RUNTIME = "third_party/kotlin/kotlinc/lib/kotlin-runtime.jar";
+ public static final String KT_REFLECT = "third_party/kotlin/kotlinc/lib/kotlin-reflect.jar";
private static final String ANDROID_JAR_PATTERN = "third_party/android_jar/lib-v%d/android.jar";
private static final AndroidApiLevel DEFAULT_MIN_SDK = AndroidApiLevel.I;
@@ -573,6 +574,12 @@
return path;
}
+ public static Path getKotlinReflectJar() {
+ Path path = Paths.get(KT_REFLECT);
+ assert Files.exists(path) : "Expected kotlin reflect jar";
+ return path;
+ }
+
public static Path getJdwpTestsCfJarPath(AndroidApiLevel minSdk) {
if (minSdk.getLevel() >= AndroidApiLevel.N.getLevel()) {
return Paths.get("third_party", "jdwp-tests", "apache-harmony-jdwp-tests-host.jar");
diff --git a/src/test/java/com/android/tools/r8/kotlin/MetadataStripTest.java b/src/test/java/com/android/tools/r8/kotlin/MetadataStripTest.java
new file mode 100644
index 0000000..c762aa8
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/kotlin/MetadataStripTest.java
@@ -0,0 +1,111 @@
+// Copyright (c) 2018, the R8 project authors. Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+package com.android.tools.r8.kotlin;
+
+import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
+import static com.android.tools.r8.utils.codeinspector.Matchers.isRenamed;
+import static org.hamcrest.CoreMatchers.not;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertThat;
+import static org.junit.Assume.assumeTrue;
+
+import com.android.tools.r8.TestBase;
+import com.android.tools.r8.TestRunResult;
+import com.android.tools.r8.ToolHelper;
+import com.android.tools.r8.ToolHelper.KotlinTargetVersion;
+import com.android.tools.r8.graph.DexAnnotation;
+import com.android.tools.r8.graph.DexClass;
+import com.android.tools.r8.utils.FileUtils;
+import com.android.tools.r8.utils.InternalOptions;
+import com.android.tools.r8.utils.codeinspector.ClassSubject;
+import com.android.tools.r8.utils.codeinspector.CodeInspector;
+import com.google.common.collect.ImmutableList;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.Collection;
+import java.util.function.Consumer;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+@RunWith(Parameterized.class)
+public class MetadataStripTest extends TestBase {
+ private static final String METADATA_DESCRIPTOR = "Lkotlin/Metadata;";
+ private static final String KEEP_ANNOTATIONS = "-keepattributes *Annotation*";
+
+ private final Backend backend;
+ private final KotlinTargetVersion targetVersion;
+
+ private Consumer<InternalOptions> optionsModifier =
+ o -> {
+ o.enableTreeShaking = true;
+ o.enableMinification = true;
+ };
+
+ @Parameterized.Parameters(name = "Backend: {0} target: {1}")
+ public static Collection<Object[]> data() {
+ ImmutableList.Builder<Object[]> builder = new ImmutableList.Builder<>();
+ for (Backend backend : Backend.values()) {
+ for (KotlinTargetVersion targetVersion : KotlinTargetVersion.values()) {
+ builder.add(new Object[]{backend, targetVersion});
+ }
+ }
+ return builder.build();
+ }
+
+ public MetadataStripTest(Backend backend, KotlinTargetVersion targetVersion) {
+ this.backend = backend;
+ this.targetVersion = targetVersion;
+ }
+
+ @Test
+ public void testJstyleRunnable() throws Exception {
+ assumeTrue("Non-trivial changes between 1.3 and master.", backend != Backend.CF);
+ final String folder = "lambdas_jstyle_runnable";
+ final String mainClassName = "lambdas_jstyle_runnable.MainKt";
+ final String implementer1ClassName = "lambdas_jstyle_runnable.Implementer1Kt";
+ TestRunResult result = testForR8(backend)
+ .addProgramFiles(getKotlinJarFile(folder))
+ .addProgramFiles(getJavaJarFile(folder))
+ .addProgramFiles(ToolHelper.getKotlinReflectJar())
+ .enableProguardTestOptions()
+ .enableInliningAnnotations()
+ .addKeepMainRule(mainClassName)
+ .addKeepRules(KEEP_ANNOTATIONS)
+ .addKeepRules("-keep class kotlin.Metadata")
+ .addOptionsModification(optionsModifier)
+ .run(mainClassName);
+ CodeInspector inspector = result.inspector();
+ ClassSubject clazz = inspector.clazz(mainClassName);
+ assertThat(clazz, isPresent());
+ assertThat(clazz, not(isRenamed()));
+ // Main class is kept, hence the presence of Metadata.
+ assertNotNull(retrieveMetadata(clazz.getDexClass()));
+ ClassSubject impl1 = inspector.clazz(implementer1ClassName);
+ assertThat(impl1, isPresent());
+ assertThat(impl1, isRenamed());
+ // All other classes can be renamed, hence the absence of Metadata;
+ assertNull(retrieveMetadata(impl1.getDexClass()));
+ }
+
+ private DexAnnotation retrieveMetadata(DexClass dexClass) {
+ for (DexAnnotation annotation : dexClass.annotations.annotations) {
+ if (annotation.annotation.type.toDescriptorString().equals(METADATA_DESCRIPTOR)) {
+ return annotation;
+ }
+ }
+ return null;
+ }
+
+ private Path getKotlinJarFile(String folder) {
+ return Paths.get(ToolHelper.TESTS_BUILD_DIR, "kotlinR8TestResources",
+ targetVersion.getFolderName(), folder + FileUtils.JAR_EXTENSION);
+ }
+
+ private Path getJavaJarFile(String folder) {
+ return Paths.get(ToolHelper.TESTS_BUILD_DIR, "kotlinR8TestResources",
+ targetVersion.getFolderName(), folder + ".java" + FileUtils.JAR_EXTENSION);
+ }
+}