Version 2.0.39
Cherry pick: Mark types with dependent instance constructor as instantiated
CL: https://r8-review.googlesource.com/c/r8/+/49085
Cherry pick: Also consider referenced fields in if rule evaluator
CL: https://r8-review.googlesource.com/c/r8/+/49089
Bug: 150189783, 149729626, 147972078
Change-Id: I52855f85275708631a765f2c806cda7db82fd9d7
diff --git a/src/main/java/com/android/tools/r8/Version.java b/src/main/java/com/android/tools/r8/Version.java
index 543adc6..53d04aa 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 = "2.0.38";
+ public static final String LABEL = "2.0.39";
private Version() {
}
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 6d418cd..9a33da2 100644
--- a/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
+++ b/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
@@ -1240,6 +1240,8 @@
annotations.forEach(annotation -> handleAnnotation(holder, annotation));
}
+ rootSet.forEachDependentInstanceConstructor(
+ holder, appView, this::enqueueHolderWithDependentInstanceConstructor);
rootSet.forEachDependentStaticMember(holder, appView, this::enqueueDependentItem);
compatEnqueueHolderIfDependentNonStaticMember(
holder, rootSet.getDependentKeepClassCompatRule(holder.getType()));
@@ -1294,6 +1296,13 @@
internalEnqueueRootItem(consequent, reasons, precondition);
}
+ private void enqueueHolderWithDependentInstanceConstructor(
+ DexProgramClass clazz,
+ DexEncodedMethod instanceInitializer,
+ Set<ProguardKeepRuleBase> reasons) {
+ enqueueRootItem(clazz, reasons);
+ }
+
private void processAnnotations(DexDefinition holder, DexAnnotation[] annotations) {
for (DexAnnotation annotation : annotations) {
processAnnotation(holder, annotation);
@@ -1896,6 +1905,11 @@
}
}
+ public boolean isFieldReferenced(DexEncodedField field) {
+ FieldAccessInfoImpl info = fieldAccessInfoCollection.get(field.field);
+ return info != null;
+ }
+
public boolean isFieldLive(DexEncodedField field) {
return liveFields.contains(field);
}
@@ -1927,6 +1941,18 @@
return directAndIndirectlyInstantiatedTypes.contains(clazz);
}
+ public boolean isMethodLive(DexEncodedMethod method) {
+ return liveMethods.contains(method);
+ }
+
+ public boolean isMethodTargeted(DexEncodedMethod method) {
+ return targetedMethods.contains(method);
+ }
+
+ public boolean isTypeLive(DexProgramClass clazz) {
+ return liveTypes.contains(clazz);
+ }
+
// Package protected due to entry point from worklist.
void markInstanceFieldAsReachable(DexEncodedField encodedField, KeepReason reason) {
DexField field = encodedField.field;
@@ -2372,15 +2398,7 @@
RootSetBuilder consequentSetBuilder = new RootSetBuilder(appView);
IfRuleEvaluator ifRuleEvaluator =
new IfRuleEvaluator(
- appView,
- executorService,
- activeIfRules,
- liveFields.getItems(),
- liveMethods.getItems(),
- liveTypes.getItems(),
- mode,
- consequentSetBuilder,
- targetedMethods.getItems());
+ appView, this, executorService, activeIfRules, consequentSetBuilder);
addConsequentRootSet(ifRuleEvaluator.run(), false);
if (!workList.isEmpty()) {
continue;
diff --git a/src/main/java/com/android/tools/r8/shaking/IfRuleEvaluator.java b/src/main/java/com/android/tools/r8/shaking/IfRuleEvaluator.java
index 9c6889e..c1dabc8 100644
--- a/src/main/java/com/android/tools/r8/shaking/IfRuleEvaluator.java
+++ b/src/main/java/com/android/tools/r8/shaking/IfRuleEvaluator.java
@@ -13,7 +13,6 @@
import com.android.tools.r8.graph.DexProgramClass;
import com.android.tools.r8.graph.DexReference;
import com.android.tools.r8.graph.DexType;
-import com.android.tools.r8.shaking.Enqueuer.Mode;
import com.android.tools.r8.shaking.RootSetBuilder.ConsequentRootSet;
import com.android.tools.r8.utils.ThreadUtils;
import com.google.common.base.Equivalence.Wrapper;
@@ -34,35 +33,23 @@
public class IfRuleEvaluator {
private final AppView<? extends AppInfoWithSubtyping> appView;
+ private final Enqueuer enqueuer;
private final ExecutorService executorService;
private final List<Future<?>> futures = new ArrayList<>();
private final Map<Wrapper<ProguardIfRule>, Set<ProguardIfRule>> ifRules;
- private final Set<DexEncodedField> liveFields;
- private final Set<DexEncodedMethod> liveMethods;
- private final Set<DexProgramClass> liveTypes;
- private final Mode mode;
private final RootSetBuilder rootSetBuilder;
- private final Set<DexEncodedMethod> targetedMethods;
IfRuleEvaluator(
AppView<? extends AppInfoWithSubtyping> appView,
+ Enqueuer enqueuer,
ExecutorService executorService,
Map<Wrapper<ProguardIfRule>, Set<ProguardIfRule>> ifRules,
- Set<DexEncodedField> liveFields,
- Set<DexEncodedMethod> liveMethods,
- Set<DexProgramClass> liveTypes,
- Mode mode,
- RootSetBuilder rootSetBuilder,
- Set<DexEncodedMethod> targetedMethods) {
+ RootSetBuilder rootSetBuilder) {
this.appView = appView;
+ this.enqueuer = enqueuer;
this.executorService = executorService;
this.ifRules = ifRules;
- this.liveFields = liveFields;
- this.liveMethods = liveMethods;
- this.liveTypes = liveTypes;
- this.mode = mode;
this.rootSetBuilder = rootSetBuilder;
- this.targetedMethods = targetedMethods;
}
public ConsequentRootSet run() throws ExecutionException {
@@ -188,7 +175,7 @@
// A type is effectively live if (1) it is truly live, (2) the value of one of its fields has
// been inlined by the member value propagation, or (3) the return value of one of its methods
// has been forwarded by the member value propagation.
- if (liveTypes.contains(clazz)) {
+ if (enqueuer.isTypeLive(clazz)) {
return true;
}
for (DexEncodedField field : clazz.fields()) {
@@ -243,15 +230,15 @@
filteredMembers,
targetClass.fields(
f ->
- (liveFields.contains(f) || f.getOptimizationInfo().valueHasBeenPropagated())
+ (enqueuer.isFieldReferenced(f) || f.getOptimizationInfo().valueHasBeenPropagated())
&& appView.graphLense().getOriginalFieldSignature(f.field).holder
== sourceClass.type));
Iterables.addAll(
filteredMembers,
targetClass.methods(
m ->
- (liveMethods.contains(m)
- || targetedMethods.contains(m)
+ (enqueuer.isMethodLive(m)
+ || enqueuer.isMethodTargeted(m)
|| m.getOptimizationInfo().returnValueHasBeenPropagated())
&& appView.graphLense().getOriginalMethodSignature(m.method).holder
== sourceClass.type));
@@ -294,7 +281,7 @@
DexItemFactory dexItemFactory = appView.dexItemFactory();
ProguardIfRule materializedRule = rule.materialize(dexItemFactory, preconditions);
- if (mode.isInitialTreeShaking() && !rule.isUsed()) {
+ if (enqueuer.getMode().isInitialTreeShaking() && !rule.isUsed()) {
// We need to abort class inlining of classes that could be matched by the condition of this
// -if rule.
ClassInlineRule neverClassInlineRuleForCondition =
diff --git a/src/main/java/com/android/tools/r8/shaking/RootSetBuilder.java b/src/main/java/com/android/tools/r8/shaking/RootSetBuilder.java
index 8555b93..f5fc58f 100644
--- a/src/main/java/com/android/tools/r8/shaking/RootSetBuilder.java
+++ b/src/main/java/com/android/tools/r8/shaking/RootSetBuilder.java
@@ -1352,6 +1352,22 @@
});
}
+ public void forEachDependentInstanceConstructor(
+ DexProgramClass clazz,
+ AppView<?> appView,
+ Consumer3<DexProgramClass, DexEncodedMethod, Set<ProguardKeepRuleBase>> fn) {
+ getDependentItems(clazz)
+ .forEach(
+ (reference, reasons) -> {
+ DexDefinition definition = appView.definitionFor(reference);
+ if (definition != null
+ && definition.isDexEncodedMethod()
+ && definition.asDexEncodedMethod().isInstanceInitializer()) {
+ fn.accept(clazz, definition.asDexEncodedMethod(), reasons);
+ }
+ });
+ }
+
public void copy(DexReference original, DexReference rewritten) {
if (noShrinking.containsKey(original)) {
noShrinking.put(rewritten, noShrinking.get(original));
diff --git a/src/test/java/com/android/tools/r8/TestShrinkerBuilder.java b/src/test/java/com/android/tools/r8/TestShrinkerBuilder.java
index 84fc2ee..dd3d56f 100644
--- a/src/test/java/com/android/tools/r8/TestShrinkerBuilder.java
+++ b/src/test/java/com/android/tools/r8/TestShrinkerBuilder.java
@@ -188,6 +188,10 @@
return addKeepRules("-keepattributes " + String.join(",", attributes));
}
+ public T addKeepRuntimeVisibleAnnotations() {
+ return addKeepAttributes("RuntimeVisibleAnnotations");
+ }
+
public T addKeepAllAttributes() {
return addKeepAttributes("*");
}
diff --git a/src/test/java/com/android/tools/r8/compatproguard/CompatKeepClassMemberNamesTestRunner.java b/src/test/java/com/android/tools/r8/compatproguard/CompatKeepClassMemberNamesTestRunner.java
index c7ffc01..412311e 100644
--- a/src/test/java/com/android/tools/r8/compatproguard/CompatKeepClassMemberNamesTestRunner.java
+++ b/src/test/java/com/android/tools/r8/compatproguard/CompatKeepClassMemberNamesTestRunner.java
@@ -3,6 +3,8 @@
// BSD-style license that can be found in the LICENSE file.
package com.android.tools.r8.compatproguard;
+import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
+import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.core.AllOf.allOf;
import static org.hamcrest.core.StringContains.containsString;
import static org.junit.Assert.assertFalse;
@@ -175,8 +177,8 @@
assertTrue(inspector.clazz(MAIN_CLASS).isPresent());
assertTrue(inspector.clazz(BAR_CLASS).isPresent());
assertBarGetInstanceIsNotInlined(inspector);
- assertFalse(inspector.clazz(BAR_CLASS).uniqueFieldWithName("i").isPresent());
- assertFalse(inspector.clazz(BAR_CLASS).uniqueMethodWithName("<init>").isPresent());
+ assertThat(inspector.clazz(BAR_CLASS).init(), isPresent());
+ assertThat(inspector.clazz(BAR_CLASS).uniqueFieldWithName("i"), isPresent());
});
}
@@ -286,8 +288,8 @@
assertTrue(inspector.clazz(MAIN_CLASS).isPresent());
assertTrue(inspector.clazz(BAR_CLASS).isPresent());
assertBarGetInstanceIsNotInlined(inspector);
- assertFalse(inspector.clazz(BAR_CLASS).uniqueFieldWithName("i").isPresent());
- assertFalse(inspector.clazz(BAR_CLASS).uniqueMethodWithName("<init>").isPresent());
+ assertThat(inspector.clazz(BAR_CLASS).uniqueMethodWithName("<init>"), isPresent());
+ assertThat(inspector.clazz(BAR_CLASS).uniqueFieldWithName("i"), isPresent());
});
}
diff --git a/src/test/java/com/android/tools/r8/shaking/annotations/B149729626.java b/src/test/java/com/android/tools/r8/shaking/annotations/B149729626.java
new file mode 100644
index 0000000..e0c497c
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/shaking/annotations/B149729626.java
@@ -0,0 +1,114 @@
+// Copyright (c) 2020, 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.shaking.annotations;
+
+import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+import com.android.tools.r8.TestBase;
+import com.android.tools.r8.TestParameters;
+import com.android.tools.r8.TestParametersCollection;
+import com.android.tools.r8.utils.codeinspector.ClassSubject;
+import com.android.tools.r8.utils.codeinspector.CodeInspector;
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameters;
+
+@RunWith(Parameterized.class)
+public class B149729626 extends TestBase {
+
+ private final TestParameters parameters;
+
+ @Parameters(name = "{0}")
+ public static TestParametersCollection params() {
+ return getTestParameters().withAllRuntimesAndApiLevels().build();
+ }
+
+ public B149729626(TestParameters parameters) {
+ this.parameters = parameters;
+ }
+
+ @Test
+ public void testR8WithKeepClassMembersRule() throws Exception {
+ testForR8(parameters.getBackend())
+ .addInnerClasses(B149729626.class)
+ .addKeepMainRule(TestClass.class)
+ .addKeepRules(
+ "-keepclassmembers @" + Marker.class.getTypeName() + " class * {",
+ " <init>(...);",
+ "}",
+ // TODO(b/149729626): Should not be required.
+ "-keep class " + TestClass.class.getTypeName() + " { void makeMarkerLive(); }")
+ // TODO(b/149729626): Should not be required.
+ .addKeepRuntimeVisibleAnnotations()
+ .setMinApi(parameters.getApiLevel())
+ .compile()
+ .inspect(this::inspect)
+ .run(parameters.getRuntime(), TestClass.class)
+ .assertSuccess();
+ }
+
+ @Test
+ public void testR8WithIfRule() throws Exception {
+ testForR8(parameters.getBackend())
+ .addInnerClasses(B149729626.class)
+ .addKeepMainRule(TestClass.class)
+ .addKeepRules(
+ "-if @" + Marker.class.getTypeName() + " class *",
+ "-keep class <1> {",
+ " <init>(...);",
+ "}",
+ // TODO(b/149729626): Should not be required.
+ "-keep class " + TestClass.class.getTypeName() + " { void makeMarkerLive(); }")
+ // TODO(b/149729626): Should not be required.
+ .addKeepRuntimeVisibleAnnotations()
+ .setMinApi(parameters.getApiLevel())
+ .compile()
+ .inspect(this::inspect)
+ .run(parameters.getRuntime(), TestClass.class)
+ .assertSuccess();
+ }
+
+ @Test
+ public void testCompat() throws Exception {
+ testForR8Compat(parameters.getBackend())
+ .addInnerClasses(B149729626.class)
+ .addKeepMainRule(TestClass.class)
+ .setMinApi(parameters.getApiLevel())
+ .compile()
+ .inspect(this::inspect)
+ .run(parameters.getRuntime(), TestClass.class)
+ .assertSuccess();
+ }
+
+ private void inspect(CodeInspector inspector) {
+ ClassSubject markedClassSubject = inspector.clazz(Marked.class);
+ assertThat(markedClassSubject, isPresent());
+ assertThat(markedClassSubject.init(), isPresent());
+ }
+
+ static class TestClass {
+
+ public static void main(String[] args) {
+ System.out.println(Marked.class);
+ }
+
+ static void makeMarkerLive() {
+ System.out.println(Marker.class);
+ }
+ }
+
+ @Target(ElementType.TYPE)
+ @Retention(RetentionPolicy.RUNTIME)
+ @interface Marker {}
+
+ @Marker
+ static class Marked {}
+}
diff --git a/src/test/java/com/android/tools/r8/shaking/ifrule/ConditionalKeepIfKeptTest.java b/src/test/java/com/android/tools/r8/shaking/ifrule/ConditionalKeepIfKeptTest.java
index 50443fc..790ea8d 100644
--- a/src/test/java/com/android/tools/r8/shaking/ifrule/ConditionalKeepIfKeptTest.java
+++ b/src/test/java/com/android/tools/r8/shaking/ifrule/ConditionalKeepIfKeptTest.java
@@ -8,6 +8,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.TestBase;
import com.android.tools.r8.TestParameters;
@@ -62,9 +63,9 @@
ClassSubject classSubject = inspector.clazz(StaticallyReferenced.class);
assertThat(classSubject, isPresent());
assertEquals(0, classSubject.allFields().size());
- // TODO(b/132318799): Full mode no-marker should not keep <init>() when not specified.
- assertEquals(useMarker ? 0 : 1, classSubject.allMethods().size());
- assertEquals(!useMarker, classSubject.init().isPresent());
+ // TODO(b/132318799): Should not keep <init>() when not specified.
+ assertEquals(1, classSubject.allMethods().size());
+ assertTrue(classSubject.init().isPresent());
});
}
diff --git a/src/test/java/com/android/tools/r8/shaking/ifrule/KeepClassesWithAnnotatedFieldsTest.java b/src/test/java/com/android/tools/r8/shaking/ifrule/KeepClassesWithAnnotatedFieldsTest.java
new file mode 100644
index 0000000..4b59d05
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/shaking/ifrule/KeepClassesWithAnnotatedFieldsTest.java
@@ -0,0 +1,83 @@
+// Copyright (c) 2020, 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.shaking.ifrule;
+
+import com.android.tools.r8.NeverInline;
+import com.android.tools.r8.NeverPropagateValue;
+import com.android.tools.r8.TestBase;
+import com.android.tools.r8.TestParameters;
+import com.android.tools.r8.TestParametersCollection;
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+@RunWith(Parameterized.class)
+public class KeepClassesWithAnnotatedFieldsTest extends TestBase {
+
+ private final TestParameters parameters;
+
+ @Parameterized.Parameters(name = "{0}")
+ public static TestParametersCollection data() {
+ return getTestParameters().withAllRuntimesAndApiLevels().build();
+ }
+
+ public KeepClassesWithAnnotatedFieldsTest(TestParameters parameters) {
+ this.parameters = parameters;
+ }
+
+ @Test
+ public void test() throws Exception {
+ testForR8(parameters.getBackend())
+ .addInnerClasses(KeepClassesWithAnnotatedFieldsTest.class)
+ .addKeepMainRule(TestClass.class)
+ .addKeepRules(
+ "-if class * { @" + typeName(MyAnnotation.class) + " <fields>; }",
+ "-keep class <1> { <init>(); }",
+ "-keepclassmembers class * {",
+ " @" + typeName(MyAnnotation.class) + " <fields>;",
+ "}")
+ .addKeepRuntimeVisibleAnnotations()
+ .enableInliningAnnotations()
+ .enableMemberValuePropagationAnnotations()
+ .setMinApi(parameters.getApiLevel())
+ .compile()
+ .run(parameters.getRuntime(), TestClass.class)
+ .assertSuccessWithOutputLines("Hello world!");
+ }
+
+ static class TestClass {
+
+ public static void main(String[] args) throws Exception {
+ DataClass obj = (DataClass) getDataClass().getDeclaredConstructor().newInstance();
+ setField(obj, "Hello world!");
+ System.out.println(obj.field);
+ }
+
+ @NeverInline
+ @NeverPropagateValue
+ static Class<?> getDataClass() {
+ return DataClass.class;
+ }
+
+ @NeverInline
+ static void setField(Object object, String value) throws Exception {
+ getDataClass().getDeclaredField("field").set(object, value);
+ }
+ }
+
+ static class DataClass {
+
+ @MyAnnotation
+ String field;
+ }
+
+ @Retention(RetentionPolicy.RUNTIME)
+ @Target(ElementType.FIELD)
+ @interface MyAnnotation {}
+}