Merge "Revert "Implicitly add necessary rules for Java reflections in any R8 mode.""
diff --git a/src/main/java/com/android/tools/r8/naming/IdentifierMinifier.java b/src/main/java/com/android/tools/r8/naming/IdentifierMinifier.java
index 8714b28..d92de6b 100644
--- a/src/main/java/com/android/tools/r8/naming/IdentifierMinifier.java
+++ b/src/main/java/com/android/tools/r8/naming/IdentifierMinifier.java
@@ -26,15 +26,15 @@
import com.android.tools.r8.graph.DexValue.DexValueString;
import com.android.tools.r8.shaking.Enqueuer.AppInfoWithLiveness;
import com.android.tools.r8.shaking.ProguardClassFilter;
-import it.unimi.dsi.fastutil.objects.Object2BooleanMap;
import java.util.Map;
+import java.util.Set;
class IdentifierMinifier {
private final AppInfoWithLiveness appInfo;
private final ProguardClassFilter adaptClassStrings;
private final NamingLens lens;
- private final Object2BooleanMap<DexItem> identifierNameStrings;
+ private final Set<DexItem> identifierNameStrings;
IdentifierMinifier(
AppInfoWithLiveness appInfo, ProguardClassFilter adaptClassStrings, NamingLens lens) {
diff --git a/src/main/java/com/android/tools/r8/naming/IdentifierNameStringMarker.java b/src/main/java/com/android/tools/r8/naming/IdentifierNameStringMarker.java
index 830391a..b0f4dc0 100644
--- a/src/main/java/com/android/tools/r8/naming/IdentifierNameStringMarker.java
+++ b/src/main/java/com/android/tools/r8/naming/IdentifierNameStringMarker.java
@@ -35,17 +35,17 @@
import com.android.tools.r8.shaking.Enqueuer.AppInfoWithLiveness;
import com.android.tools.r8.utils.InternalOptions;
import com.google.common.collect.Streams;
-import it.unimi.dsi.fastutil.objects.Object2BooleanMap;
import java.util.Arrays;
import java.util.List;
import java.util.ListIterator;
import java.util.Objects;
+import java.util.Set;
import java.util.stream.Collectors;
public class IdentifierNameStringMarker {
private final AppInfo appInfo;
private final DexItemFactory dexItemFactory;
- private final Object2BooleanMap<DexItem> identifierNameStrings;
+ private final Set<DexItem> identifierNameStrings;
private final InternalOptions options;
public IdentifierNameStringMarker(AppInfoWithLiveness appInfo, InternalOptions options) {
@@ -63,7 +63,7 @@
}
private void decoupleIdentifierNameStringInField(DexEncodedField encodedField) {
- if (!identifierNameStrings.containsKey(encodedField.field)) {
+ if (!identifierNameStrings.contains(encodedField.field)) {
return;
}
if (!encodedField.accessFlags.isStatic()) {
@@ -102,27 +102,22 @@
if (instruction.isStaticPut() || instruction.isInstancePut()) {
FieldInstruction fieldPut = instruction.asFieldInstruction();
DexField field = fieldPut.getField();
- if (!identifierNameStrings.containsKey(field)) {
+ if (!identifierNameStrings.contains(field)) {
continue;
}
- boolean isExplicitRule = identifierNameStrings.getBoolean(field);
Value in = instruction.isStaticPut()
? instruction.asStaticPut().inValue()
: instruction.asInstancePut().value();
if (!in.isConstString()) {
- if (isExplicitRule) {
- warnUndeterminedIdentifierIfNecessary(
- appInfo, options, field, originHolder, instruction, null);
- }
+ warnUndeterminedIdentifierIfNecessary(
+ appInfo, options, field, originHolder, instruction, null);
continue;
}
DexString original = in.getConstInstruction().asConstString().getValue();
DexItemBasedString itemBasedString = inferMemberOrTypeFromNameString(appInfo, original);
if (itemBasedString == null) {
- if (isExplicitRule) {
- warnUndeterminedIdentifierIfNecessary(
- appInfo, options, field, originHolder, instruction, original);
- }
+ warnUndeterminedIdentifierIfNecessary(
+ appInfo, options, field, originHolder, instruction, original);
continue;
}
// Move the cursor back to $fieldPut
@@ -168,19 +163,16 @@
} else if (instruction.isInvokeMethod()) {
InvokeMethod invoke = instruction.asInvokeMethod();
DexMethod invokedMethod = invoke.getInvokedMethod();
- if (!identifierNameStrings.containsKey(invokedMethod)) {
+ if (!identifierNameStrings.contains(invokedMethod)) {
continue;
}
- boolean isExplicitRule = identifierNameStrings.getBoolean(invokedMethod);
List<Value> ins = invoke.arguments();
Value[] changes = new Value [ins.size()];
if (isReflectionMethod(dexItemFactory, invokedMethod)) {
DexItemBasedString itemBasedString = identifyIdentiferNameString(appInfo, invoke);
if (itemBasedString == null) {
- if (isExplicitRule) {
- warnUndeterminedIdentifierIfNecessary(
- appInfo, options, invokedMethod, originHolder, instruction, null);
- }
+ warnUndeterminedIdentifierIfNecessary(
+ appInfo, options, invokedMethod, originHolder, instruction, null);
continue;
}
DexType returnType = invoke.getReturnType();
@@ -225,20 +217,16 @@
for (int i = 0; i < ins.size(); i++) {
Value in = ins.get(i);
if (!in.isConstString()) {
- if (isExplicitRule) {
- warnUndeterminedIdentifierIfNecessary(
- appInfo, options, invokedMethod, originHolder, instruction, null);
- }
+ warnUndeterminedIdentifierIfNecessary(
+ appInfo, options, invokedMethod, originHolder, instruction, null);
continue;
}
DexString original = in.getConstInstruction().asConstString().getValue();
DexItemBasedString itemBasedString =
inferMemberOrTypeFromNameString(appInfo, original);
if (itemBasedString == null) {
- if (isExplicitRule) {
- warnUndeterminedIdentifierIfNecessary(
- appInfo, options, invokedMethod, originHolder, instruction, original);
- }
+ warnUndeterminedIdentifierIfNecessary(
+ appInfo, options, invokedMethod, originHolder, instruction, original);
continue;
}
// Move the cursor back to $invoke
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 5936e20..6a12b60 100644
--- a/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
+++ b/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
@@ -5,6 +5,7 @@
import static com.android.tools.r8.naming.IdentifierNameStringUtils.identifyIdentiferNameString;
import static com.android.tools.r8.naming.IdentifierNameStringUtils.isReflectionMethod;
+import static com.android.tools.r8.naming.IdentifierNameStringUtils.warnUndeterminedIdentifierIfNecessary;
import static com.android.tools.r8.shaking.ProguardConfigurationUtils.buildIdentifierNameStringRule;
import com.android.tools.r8.Diagnostic;
@@ -57,8 +58,6 @@
import com.google.common.collect.Sets;
import com.google.common.collect.Sets.SetView;
import it.unimi.dsi.fastutil.ints.Int2ReferenceMap;
-import it.unimi.dsi.fastutil.objects.Object2BooleanArrayMap;
-import it.unimi.dsi.fastutil.objects.Object2BooleanMap;
import it.unimi.dsi.fastutil.objects.Reference2IntMap;
import java.util.ArrayDeque;
import java.util.Collection;
@@ -187,9 +186,10 @@
private final Queue<Action> proguardCompatibilityWorkList = Queues.newArrayDeque();
/**
- * A set of methods that need code inspection for Java reflection in use.
+ * A set of methods that need code inspection for Proguard compatibility rules.
*/
- private final Set<DexEncodedMethod> pendingReflectiveUses = Sets.newLinkedHashSet();
+ private final Set<DexEncodedMethod> pendingProguardReflectiveCompatibility =
+ Sets.newLinkedHashSet();
/**
* A cache for DexMethod that have been marked reachable.
@@ -305,15 +305,13 @@
boolean registerInvokeVirtual(DexMethod method, KeepReason keepReason) {
if (appInfo.dexItemFactory.classMethods.isReflectiveMemberLookup(method)) {
- // Implicitly add -identifiernamestring rule for the Java reflection in use.
- if (identifierNameStrings.add(method)) {
- // Add -identifiernamestring rule to Compatibility configuration builder for testing.
- if (forceProguardCompatibility && compatibility != null) {
+ if (forceProguardCompatibility) {
+ // TODO(b/76181966): whether or not add this rule in normal mode.
+ if (identifierNameStrings.add(method) && compatibility != null) {
compatibility.addRule(buildIdentifierNameStringRule(method));
}
+ pendingProguardReflectiveCompatibility.add(currentMethod);
}
- // Revisit the current method to implicitly add -keep rule for items with reflective access.
- pendingReflectiveUses.add(currentMethod);
}
if (!registerItemWithTarget(virtualInvokes, method)) {
return false;
@@ -349,15 +347,13 @@
boolean registerInvokeStatic(DexMethod method, KeepReason keepReason) {
if (method == appInfo.dexItemFactory.classMethods.forName
|| appInfo.dexItemFactory.atomicFieldUpdaterMethods.isFieldUpdater(method)) {
- // Implicitly add -identifiernamestring rule for the Java reflection in use.
- if (identifierNameStrings.add(method)) {
- // Add -identifiernamestring rule to Compatibility configuration builder for testing.
- if(forceProguardCompatibility && compatibility != null) {
+ if (forceProguardCompatibility) {
+ // TODO(b/76181966): whether or not add this rule in normal mode.
+ if (identifierNameStrings.add(method) && compatibility != null) {
compatibility.addRule(buildIdentifierNameStringRule(method));
}
+ pendingProguardReflectiveCompatibility.add(currentMethod);
}
- // Revisit the current method to implicitly add -keep rule for items with reflective access.
- pendingReflectiveUses.add(currentMethod);
}
if (!registerItemWithTarget(staticInvokes, method)) {
return false;
@@ -1274,15 +1270,15 @@
}
// Continue fix-point processing while there are additional work items to ensure
- // items that are passed to Java reflections are traced.
+ // Proguard compatibility.
if (proguardCompatibilityWorkList.isEmpty()
- && pendingReflectiveUses.isEmpty()) {
+ && pendingProguardReflectiveCompatibility.isEmpty()) {
break;
}
- pendingReflectiveUses.forEach(this::handleReflectiveBehavior);
+ pendingProguardReflectiveCompatibility.forEach(this::handleProguardReflectiveBehavior);
workList.addAll(proguardCompatibilityWorkList);
proguardCompatibilityWorkList.clear();
- pendingReflectiveUses.clear();
+ pendingProguardReflectiveCompatibility.clear();
}
if (Log.ENABLED) {
Set<DexEncodedMethod> allLive = Sets.newIdentityHashSet();
@@ -1479,15 +1475,15 @@
Action.markMethodLive(method, KeepReason.dueToProguardCompatibilityKeepRule(rule)));
}
- private void handleReflectiveBehavior(DexEncodedMethod method) {
+ private void handleProguardReflectiveBehavior(DexEncodedMethod method) {
DexType originHolder = method.method.holder;
Origin origin = appInfo.originFor(originHolder);
IRCode code = method.buildIR(appInfo, options, origin);
code.instructionIterator().forEachRemaining(instr ->
- handleReflectiveBehavior(instr, originHolder));
+ handleProguardReflectiveBehavior(instr, originHolder));
}
- private void handleReflectiveBehavior(Instruction instruction, DexType originHolder) {
+ private void handleProguardReflectiveBehavior(Instruction instruction, DexType originHolder) {
if (!instruction.isInvokeMethod()) {
return;
}
@@ -1498,6 +1494,8 @@
}
DexItemBasedString itemBasedString = identifyIdentiferNameString(appInfo, invoke);
if (itemBasedString == null) {
+ warnUndeterminedIdentifierIfNecessary(
+ appInfo, options, invokedMethod, originHolder, instruction, null);
return;
}
if (itemBasedString.basedOn instanceof DexType) {
@@ -1685,10 +1683,8 @@
public final Set<DexItem> neverInline;
/**
* All items with -identifiernamestring rule.
- * Bound boolean value indicates the rule is explicitly specified by users (<code>true</code>)
- * or not, i.e., implicitly added by R8 (<code>false</code>).
*/
- public final Object2BooleanMap<DexItem> identifierNameStrings;
+ public final Set<DexItem> identifierNameStrings;
/**
* Set of fields that have been identified as proto-lite fields by the
* {@link ProtoLiteExtension}.
@@ -1745,8 +1741,8 @@
this.alwaysInline = enqueuer.rootSet.alwaysInline;
this.forceInline = enqueuer.rootSet.forceInline;
this.neverInline = enqueuer.rootSet.neverInline;
- this.identifierNameStrings = joinIdentifierNameStrings(
- enqueuer.rootSet.identifierNameStrings, enqueuer.identifierNameStrings);
+ this.identifierNameStrings =
+ Sets.union(enqueuer.rootSet.identifierNameStrings, enqueuer.identifierNameStrings);
this.protoLiteFields = enqueuer.protoLiteFields;
this.prunedTypes = Collections.emptySet();
this.switchMaps = Collections.emptyMap();
@@ -1917,18 +1913,6 @@
.collect(ImmutableSortedSet.toImmutableSortedSet(PresortedComparable::slowCompare));
}
- private Object2BooleanMap<DexItem> joinIdentifierNameStrings(
- Set<DexItem> explicit, Set<DexItem> implicit) {
- Object2BooleanMap<DexItem> result = new Object2BooleanArrayMap<>();
- for (DexItem e : explicit) {
- result.putIfAbsent(e, true);
- }
- for (DexItem i : implicit) {
- result.putIfAbsent(i, false);
- }
- return result;
- }
-
private <T extends PresortedComparable<T>> SortedSet<T> toSortedDescriptorSet(
Set<? extends KeyedDexItem<T>> set) {
ImmutableSortedSet.Builder<T> builder =
@@ -2038,32 +2022,6 @@
return builder.build();
}
- private static Object2BooleanMap<DexItem> rewriteMixedItemsConservatively(
- Object2BooleanMap<DexItem> original, GraphLense lense) {
- Object2BooleanMap<DexItem> result = new Object2BooleanArrayMap<>();
- for (Object2BooleanMap.Entry<DexItem> entry : original.object2BooleanEntrySet()) {
- DexItem item = entry.getKey();
- // TODO(b/67934123) There should be a common interface to perform the dispatch.
- if (item instanceof DexType) {
- result.put(lense.lookupType((DexType) item), entry.getBooleanValue());
- } else if (item instanceof DexMethod) {
- DexMethod method = (DexMethod) item;
- if (lense.isContextFreeForMethod(method)) {
- result.put(lense.lookupMethod(method), entry.getBooleanValue());
- } else {
- for (DexMethod candidate: lense.lookupMethodInAllContexts(method)) {
- result.put(candidate, entry.getBooleanValue());
- }
- }
- } else if (item instanceof DexField) {
- result.put(lense.lookupField((DexField) item), entry.getBooleanValue());
- } else {
- throw new Unreachable();
- }
- }
- return result;
- }
-
private static <T> Set<T> mergeSets(Collection<T> first, Collection<T> second) {
ImmutableSet.Builder<T> builder = ImmutableSet.builder();
builder.addAll(first);
diff --git a/src/test/examples/identifiernamestring/keep-rules-3.txt b/src/test/examples/identifiernamestring/keep-rules-3.txt
index 1c69314..aa23772 100644
--- a/src/test/examples/identifiernamestring/keep-rules-3.txt
+++ b/src/test/examples/identifiernamestring/keep-rules-3.txt
@@ -11,7 +11,7 @@
-dontshrink
--identifiernamestring class **.R {
+-identifiernamestring class * {
static java.lang.reflect.Field *(java.lang.Class, java.lang.String);
static java.lang.reflect.Method *(java.lang.Class, java.lang.String, java.lang.Class[]);
}
diff --git a/src/test/java/com/android/tools/r8/naming/IdentifierMinifierTest.java b/src/test/java/com/android/tools/r8/naming/IdentifierMinifierTest.java
index 73cdaaf..a9ee851 100644
--- a/src/test/java/com/android/tools/r8/naming/IdentifierMinifierTest.java
+++ b/src/test/java/com/android/tools/r8/naming/IdentifierMinifierTest.java
@@ -142,7 +142,7 @@
FoundMethodSubject foundMain = (FoundMethodSubject) main;
verifyPresenceOfConstString(foundMain);
int renamedYetFoundIdentifierCount = countRenamedClassIdentifier(inspector, foundMain);
- assertEquals(4, renamedYetFoundIdentifierCount);
+ assertEquals(0, renamedYetFoundIdentifierCount);
ClassSubject aClass = inspector.clazz("adaptclassstrings.A");
MethodSubject bar = aClass.method("void", "bar", ImmutableList.of());
@@ -165,7 +165,7 @@
FoundMethodSubject foundMain = (FoundMethodSubject) main;
verifyPresenceOfConstString(foundMain);
int renamedYetFoundIdentifierCount = countRenamedClassIdentifier(inspector, foundMain);
- assertEquals(4, renamedYetFoundIdentifierCount);
+ assertEquals(0, renamedYetFoundIdentifierCount);
ClassSubject aClass = inspector.clazz("adaptclassstrings.A");
MethodSubject bar = aClass.method("void", "bar", ImmutableList.of());
@@ -231,7 +231,7 @@
FoundMethodSubject foundMain = (FoundMethodSubject) main;
verifyPresenceOfConstString(foundMain);
int renamedYetFoundIdentifierCount = countRenamedClassIdentifier(inspector, foundMain);
- assertEquals(1, renamedYetFoundIdentifierCount);
+ assertEquals(0, renamedYetFoundIdentifierCount);
ClassSubject aClass = inspector.clazz("identifiernamestring.A");
MethodSubject aInit =
@@ -255,7 +255,7 @@
FoundMethodSubject foundMain = (FoundMethodSubject) main;
verifyPresenceOfConstString(foundMain);
int renamedYetFoundIdentifierCount = countRenamedClassIdentifier(inspector, foundMain);
- assertEquals(2, renamedYetFoundIdentifierCount);
+ assertEquals(1, renamedYetFoundIdentifierCount);
ClassSubject aClass = inspector.clazz("identifiernamestring.A");
MethodSubject aInit =
@@ -271,7 +271,7 @@
assertEquals(2, renamedYetFoundIdentifierCount);
}
- // With -identifiernamestring for reflective methods in testing class R.
+ // With -identifiernamestring for reflective methods
private static void test2_rule3(CodeInspector inspector) {
ClassSubject mainClass = inspector.clazz("identifiernamestring.Main");
MethodSubject main = mainClass.method(CodeInspector.MAIN);
diff --git a/src/test/java/com/android/tools/r8/naming/WarnReflectiveAccessTest.java b/src/test/java/com/android/tools/r8/naming/WarnReflectiveAccessTest.java
index 49ba186..829cdd0 100644
--- a/src/test/java/com/android/tools/r8/naming/WarnReflectiveAccessTest.java
+++ b/src/test/java/com/android/tools/r8/naming/WarnReflectiveAccessTest.java
@@ -9,7 +9,6 @@
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotEquals;
import static org.junit.Assert.assertThat;
-import static org.junit.Assert.assertTrue;
import com.android.tools.r8.DiagnosticsChecker;
import com.android.tools.r8.OutputMode;
@@ -73,17 +72,15 @@
byte[][] classes,
Class main,
Path out,
- boolean explicitRule,
boolean enableMinification,
boolean forceProguardCompatibility)
throws Exception {
String minificationModifier = enableMinification ? ",allowobfuscation " : " ";
- String reflectionRules = explicitRule
- ? "-identifiernamestring class java.lang.Class {\n"
+ String reflectionRules = forceProguardCompatibility ? ""
+ : "-identifiernamestring class java.lang.Class {\n"
+ "static java.lang.Class forName(java.lang.String);\n"
+ "java.lang.reflect.Method getDeclaredMethod(java.lang.String, java.lang.Class[]);\n"
- + "}\n"
- : "";
+ + "}\n";
R8Command.Builder commandBuilder =
R8Command.builder(reporter)
.addProguardConfiguration(
@@ -105,8 +102,7 @@
});
}
- private void reflectionWithBuilder(
- boolean explicitRule,
+ private void reflectionWithBuildter(
boolean enableMinification,
boolean forceProguardCompatibility) throws Exception {
byte[][] classes = {
@@ -114,7 +110,7 @@
};
Path out = temp.getRoot().toPath();
AndroidApp processedApp = runR8(classes, WarnReflectiveAccessTestMain.class, out,
- explicitRule, enableMinification, forceProguardCompatibility);
+ enableMinification, forceProguardCompatibility);
String main = WarnReflectiveAccessTestMain.class.getCanonicalName();
CodeInspector codeInspector = new CodeInspector(processedApp);
@@ -137,59 +133,35 @@
}
@Test
- public void test_explicit_minification_forceProguardCompatibility() throws Exception {
- reflectionWithBuilder(true, true, true);
+ public void test_minification_forceProguardCompatibility() throws Exception {
+ reflectionWithBuildter(true, true);
assertFalse(handler.warnings.isEmpty());
- DiagnosticsChecker.checkDiagnostic(handler.warnings.get(0), (Path) null, 55, 1,
+ DiagnosticsChecker.checkDiagnostic(handler.warnings.get(0), (Path) null, 54, 1,
+ "Cannot determine", "getDeclaredMethod", "resolution failure");
+ }
+
+ @Test
+ public void test_noMinification_forceProguardCompatibility() throws Exception {
+ reflectionWithBuildter(false, true);
+ assertFalse(handler.warnings.isEmpty());
+ DiagnosticsChecker.checkDiagnostic(handler.warnings.get(0), (Path) null, 54, 1,
+ "Cannot determine", "getDeclaredMethod", "resolution failure");
+ }
+
+ @Test
+ public void test_minification_R8() throws Exception {
+ reflectionWithBuildter(true, false);
+ assertFalse(handler.warnings.isEmpty());
+ DiagnosticsChecker.checkDiagnostic(handler.warnings.get(0), (Path) null, 54, 1,
"Cannot determine", "getDeclaredMethod", "-identifiernamestring", "resolution failure");
}
@Test
- public void test_explicit_noMinification_forceProguardCompatibility() throws Exception {
- reflectionWithBuilder(true, false, true);
+ public void test_noMinification_R8() throws Exception {
+ reflectionWithBuildter(false, false);
assertFalse(handler.warnings.isEmpty());
- DiagnosticsChecker.checkDiagnostic(handler.warnings.get(0), (Path) null, 55, 1,
+ DiagnosticsChecker.checkDiagnostic(handler.warnings.get(0), (Path) null, 54, 1,
"Cannot determine", "getDeclaredMethod", "-identifiernamestring", "resolution failure");
}
- @Test
- public void test_explicit_minification_R8() throws Exception {
- reflectionWithBuilder(true, true, false);
- assertFalse(handler.warnings.isEmpty());
- DiagnosticsChecker.checkDiagnostic(handler.warnings.get(0), (Path) null, 55, 1,
- "Cannot determine", "getDeclaredMethod", "-identifiernamestring", "resolution failure");
- }
-
- @Test
- public void test_explicit_noMinification_R8() throws Exception {
- reflectionWithBuilder(true, false, false);
- assertFalse(handler.warnings.isEmpty());
- DiagnosticsChecker.checkDiagnostic(handler.warnings.get(0), (Path) null, 55, 1,
- "Cannot determine", "getDeclaredMethod", "-identifiernamestring", "resolution failure");
- }
-
- @Test
- public void test_implicit_minification_forceProguardCompatibility() throws Exception {
- reflectionWithBuilder(false, true, true);
- assertTrue(handler.warnings.isEmpty());
- }
-
- @Test
- public void test_implicit_noMinification_forceProguardCompatibility() throws Exception {
- reflectionWithBuilder(false, false, true);
- assertTrue(handler.warnings.isEmpty());
- }
-
- @Test
- public void test_implicit_minification_R8() throws Exception {
- reflectionWithBuilder(false, true, false);
- assertTrue(handler.warnings.isEmpty());
- }
-
- @Test
- public void test_implicit_noMinification_R8() throws Exception {
- reflectionWithBuilder(false, false, false);
- assertTrue(handler.warnings.isEmpty());
- }
-
}
