Refactor construction of compatibility rule for keepclassmembers.
The compatibility rule is *not* added to keepclassmembernames since the
use of allowshrinking renders any such rule unreliable and thus R8 should
not introduce compatibility support for it. See b/132597326.
Bug: 132597326
Bug: 119076934
Bug: 132828740
Change-Id: I19c2d8831ab7b5ccfe6f1e48b96a64a10e796212
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 492e4cd..c117087 100644
--- a/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
+++ b/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
@@ -398,27 +398,12 @@
}
}
- private void enqueueHolderIfDependentNonStaticMember(
- DexClass holder, Map<DexReference, Set<ProguardKeepRule>> dependentItems) {
- // Check if any dependent members are not static, and in that case enqueue the class as well.
- // Having a dependent rule like -keepclassmembers with non static items indicates that class
- // instances will be present even if tracing do not find any instantiation. See b/115867670.
- for (Entry<DexReference, Set<ProguardKeepRule>> entry : dependentItems.entrySet()) {
- DexReference dependentItem = entry.getKey();
- if (dependentItem.isDexType()) {
- continue;
- }
- DexDefinition dependentDefinition = appView.definitionFor(dependentItem);
- if (dependentDefinition == null) {
- assert false;
- continue;
- }
- if (!dependentDefinition.isStaticMember()) {
- enqueueRootItem(holder, entry.getValue());
- // Enough to enqueue the known holder once.
- break;
- }
+ private void compatEnqueueHolderIfDependentNonStaticMember(
+ DexClass holder, Set<ProguardKeepRule> compatRules) {
+ if (!forceProguardCompatibility || compatRules == null) {
+ return;
}
+ enqueueRootItem(holder, compatRules);
}
//
@@ -890,9 +875,8 @@
assert !deferredAnnotations.containsKey(holder.type);
}
rootSet.forEachDependentStaticMember(holder, appView, this::enqueueRootItem);
- if (forceProguardCompatibility) {
- enqueueHolderIfDependentNonStaticMember(holder, rootSet.getDependentItems(holder));
- }
+ compatEnqueueHolderIfDependentNonStaticMember(
+ holder, rootSet.getDependentKeepClassCompatRule(holder.getType()));
}
}
@@ -1691,19 +1675,21 @@
targetedMethods.getItems(),
executorService);
ConsequentRootSet consequentRootSet = ifRuleEvaluator.run(liveTypes);
+ // TODO(b/132600955): This modifies the root set. Should the consequent be persistent?
rootSet.addConsequentRootSet(consequentRootSet);
enqueueRootItems(consequentRootSet.noShrinking);
- // Check if any newly dependent members are not static, and in that case find the holder
- // and enqueue it as well. This is -if version of workaround for b/115867670.
+ // TODO(b/132828740): Seems incorrect that the precondition is not always met here.
consequentRootSet.dependentNoShrinking.forEach(
- (precondition, dependentItems) -> {
- if (precondition.isDexType() && forceProguardCompatibility) {
+ (precondition, dependentItems) -> enqueueRootItems(dependentItems));
+ // Check for compatibility rules indicating that the holder must be implicitly kept.
+ if (forceProguardCompatibility) {
+ consequentRootSet.dependentKeepClassCompatRule.forEach(
+ (precondition, compatRules) -> {
+ assert precondition.isDexType();
DexClass preconditionHolder = appView.definitionFor(precondition.asDexType());
- enqueueHolderIfDependentNonStaticMember(preconditionHolder, dependentItems);
- }
- // Add all dependent members to the workqueue.
- enqueueRootItems(dependentItems);
- });
+ compatEnqueueHolderIfDependentNonStaticMember(preconditionHolder, compatRules);
+ });
+ }
if (!workList.isEmpty()) {
continue;
}
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 fa8b7ed..9664821 100644
--- a/src/main/java/com/android/tools/r8/shaking/RootSetBuilder.java
+++ b/src/main/java/com/android/tools/r8/shaking/RootSetBuilder.java
@@ -84,6 +84,8 @@
private final Set<DexReference> neverPropagateValue = Sets.newIdentityHashSet();
private final Map<DexReference, Map<DexReference, Set<ProguardKeepRule>>> dependentNoShrinking =
new IdentityHashMap<>();
+ private final Map<DexType, Set<ProguardKeepRule>> dependentKeepClassCompatRule =
+ new IdentityHashMap<>();
private final Map<DexReference, ProguardMemberRule> mayHaveSideEffects = new IdentityHashMap<>();
private final Map<DexReference, ProguardMemberRule> noSideEffects = new IdentityHashMap<>();
private final Map<DexReference, ProguardMemberRule> assumedValues = new IdentityHashMap<>();
@@ -285,6 +287,7 @@
noSideEffects,
assumedValues,
dependentNoShrinking,
+ dependentKeepClassCompatRule,
identifierNameStrings,
ifRules);
}
@@ -362,7 +365,8 @@
noShrinking,
noOptimization,
noObfuscation,
- dependentNoShrinking);
+ dependentNoShrinking,
+ dependentKeepClassCompatRule);
}
/**
@@ -968,6 +972,17 @@
ProguardKeepRule keepRule = (ProguardKeepRule) context;
ProguardKeepRuleModifiers modifiers = keepRule.getModifiers();
+ // In compatibility mode, for a match on instance members a referenced class becomes live.
+ if (options.forceProguardCompatibility
+ && !modifiers.allowsShrinking
+ && precondition != null
+ && precondition.isDexClass()) {
+ if (!item.isDexClass() && !item.isStaticMember()) {
+ dependentKeepClassCompatRule
+ .computeIfAbsent(precondition.asDexClass().getType(), i -> new HashSet<>())
+ .add(keepRule);
+ }
+ }
if (!modifiers.allowsShrinking) {
if (precondition != null) {
dependentNoShrinking
@@ -1090,6 +1105,7 @@
public final Map<DexReference, ProguardMemberRule> assumedValues;
private final Map<DexReference, Map<DexReference, Set<ProguardKeepRule>>>
dependentNoShrinking;
+ private final Map<DexType, Set<ProguardKeepRule>> dependentKeepClassCompatRule;
public final Set<DexReference> identifierNameStrings;
public final Set<ProguardIfRule> ifRules;
@@ -1111,6 +1127,7 @@
Map<DexReference, ProguardMemberRule> noSideEffects,
Map<DexReference, ProguardMemberRule> assumedValues,
Map<DexReference, Map<DexReference, Set<ProguardKeepRule>>> dependentNoShrinking,
+ Map<DexType, Set<ProguardKeepRule>> dependentKeepClassCompatRule,
Set<DexReference> identifierNameStrings,
Set<ProguardIfRule> ifRules) {
this.noShrinking = noShrinking;
@@ -1130,6 +1147,7 @@
this.noSideEffects = noSideEffects;
this.assumedValues = assumedValues;
this.dependentNoShrinking = dependentNoShrinking;
+ this.dependentKeepClassCompatRule = dependentKeepClassCompatRule;
this.identifierNameStrings = Collections.unmodifiableSet(identifierNameStrings);
this.ifRules = Collections.unmodifiableSet(ifRules);
}
@@ -1159,6 +1177,8 @@
rewriteMutableReferenceKeys(previous.assumedValues, lense::lookupReference);
this.dependentNoShrinking =
rewriteDependentReferenceKeys(previous.dependentNoShrinking, lense::lookupReference);
+ this.dependentKeepClassCompatRule =
+ rewriteReferenceKeys(previous.dependentKeepClassCompatRule, lense::lookupType);
this.identifierNameStrings =
lense.rewriteReferencesConservatively(previous.identifierNameStrings);
this.ifRules = Collections.unmodifiableSet(previous.ifRules);
@@ -1183,6 +1203,10 @@
noOptimization.addAll(consequentRootSet.noOptimization);
noObfuscation.addAll(consequentRootSet.noObfuscation);
addDependentItems(consequentRootSet.dependentNoShrinking);
+ consequentRootSet.dependentKeepClassCompatRule.forEach(
+ (type, rules) ->
+ dependentKeepClassCompatRule.computeIfAbsent(
+ type, k -> new HashSet<>()).addAll(rules));
}
// Add dependent items that depend on -if rules.
@@ -1195,6 +1219,10 @@
.putAll(dependence));
}
+ Set<ProguardKeepRule> getDependentKeepClassCompatRule(DexType type) {
+ return dependentKeepClassCompatRule.get(type);
+ }
+
Map<DexReference, Set<ProguardKeepRule>> getDependentItems(DexDefinition item) {
return Collections.unmodifiableMap(
dependentNoShrinking.getOrDefault(item.toReference(), Collections.emptyMap()));
@@ -1452,6 +1480,7 @@
final Set<DexReference> noOptimization;
final Set<DexReference> noObfuscation;
final Map<DexReference, Map<DexReference, Set<ProguardKeepRule>>> dependentNoShrinking;
+ final Map<DexType, Set<ProguardKeepRule>> dependentKeepClassCompatRule;
private ConsequentRootSet(
Set<DexMethod> neverInline,
@@ -1459,13 +1488,15 @@
Map<DexReference, Set<ProguardKeepRule>> noShrinking,
Set<DexReference> noOptimization,
Set<DexReference> noObfuscation,
- Map<DexReference, Map<DexReference, Set<ProguardKeepRule>>> dependentNoShrinking) {
+ Map<DexReference, Map<DexReference, Set<ProguardKeepRule>>> dependentNoShrinking,
+ Map<DexType, Set<ProguardKeepRule>> dependentKeepClassCompatRule) {
this.neverInline = Collections.unmodifiableSet(neverInline);
this.neverClassInline = Collections.unmodifiableSet(neverClassInline);
this.noShrinking = Collections.unmodifiableMap(noShrinking);
this.noOptimization = Collections.unmodifiableSet(noOptimization);
this.noObfuscation = Collections.unmodifiableSet(noObfuscation);
this.dependentNoShrinking = Collections.unmodifiableMap(dependentNoShrinking);
+ this.dependentKeepClassCompatRule = Collections.unmodifiableMap(dependentKeepClassCompatRule);
}
}
}
diff --git a/src/test/java/com/android/tools/r8/compatproguard/CompatKeepClassMemberNamesTest.java b/src/test/java/com/android/tools/r8/compatproguard/CompatKeepClassMemberNamesTest.java
index 27297bd..ff94a62 100644
--- a/src/test/java/com/android/tools/r8/compatproguard/CompatKeepClassMemberNamesTest.java
+++ b/src/test/java/com/android/tools/r8/compatproguard/CompatKeepClassMemberNamesTest.java
@@ -3,12 +3,15 @@
// BSD-style license that can be found in the LICENSE file.
package com.android.tools.r8.compatproguard;
+import com.android.tools.r8.NeverInline;
+
public class CompatKeepClassMemberNamesTest {
public static class Bar {
public int i = 42;
+ @NeverInline
public static Bar instance() {
throw new RuntimeException();
}
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 44e3756..c7ffc01 100644
--- a/src/test/java/com/android/tools/r8/compatproguard/CompatKeepClassMemberNamesTestRunner.java
+++ b/src/test/java/com/android/tools/r8/compatproguard/CompatKeepClassMemberNamesTestRunner.java
@@ -9,6 +9,7 @@
import static org.junit.Assert.assertTrue;
import com.android.tools.r8.BaseCompilerCommand;
+import com.android.tools.r8.NeverInline;
import com.android.tools.r8.TestBase;
import com.android.tools.r8.TestCompileResult;
import com.android.tools.r8.TestParameters;
@@ -23,7 +24,6 @@
import com.android.tools.r8.utils.codeinspector.MethodSubject;
import com.google.common.collect.ImmutableList;
import java.util.Collection;
-import org.junit.Ignore;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
@@ -35,7 +35,8 @@
private static Class<?> MAIN_CLASS = CompatKeepClassMemberNamesTest.class;
private static Class<?> BAR_CLASS = CompatKeepClassMemberNamesTest.Bar.class;
- private static Collection<Class<?>> CLASSES = ImmutableList.of(MAIN_CLASS, BAR_CLASS);
+ private static Collection<Class<?>> CLASSES =
+ ImmutableList.of(MAIN_CLASS, BAR_CLASS, NeverInline.class);
private static String KEEP_RULE =
"class "
@@ -363,11 +364,11 @@
}
@Test
- @Ignore("b/119076934")
- // TODO(b/119076934): This fails the Bar.instance() is not inlined check.
public void testWithMemberNamesRuleCompatR8() throws Exception {
assertMemberNamesRuleCompatResult(
- buildWithMemberNamesRule(testForR8Compat(parameters.getBackend())).compile());
+ buildWithMemberNamesRule(testForR8Compat(parameters.getBackend()))
+ .enableInliningAnnotations()
+ .compile());
}
@Test
@@ -420,11 +421,10 @@
}
@Test
- @Ignore("b/119076934")
- // TODO(b/119076934): This fails the Bar.instance() is not inlined check.
public void testWithMemberNamesRuleEnableMinificationCompatR8() throws Exception {
assertMemberNamesRuleEnableMinificationCompatResult(
buildWithMemberNamesRuleEnableMinification(testForR8Compat(parameters.getBackend()))
+ .enableInliningAnnotations()
.compile());
}
