Warn users of unidentified reflection while minification is allowed.
Bug: 72858955
Change-Id: I6bc8ce2d78659e6a707e9189f7539ea00832847d
diff --git a/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java b/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java
index 3288aa9..fffbc44 100644
--- a/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java
+++ b/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java
@@ -149,7 +149,8 @@
this.protoLiteRewriter =
options.forceProguardCompatibility ? null : new ProtoLitePruner(appInfo.withLiveness());
if (!appInfo.withLiveness().identifierNameStrings.isEmpty() && options.enableMinification) {
- this.identifierNameStringMarker = new IdentifierNameStringMarker(appInfo.withLiveness());
+ this.identifierNameStringMarker =
+ new IdentifierNameStringMarker(appInfo.withLiveness(), options);
} else {
this.identifierNameStringMarker = null;
}
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 57e8514..581ac8a 100644
--- a/src/main/java/com/android/tools/r8/naming/IdentifierNameStringMarker.java
+++ b/src/main/java/com/android/tools/r8/naming/IdentifierNameStringMarker.java
@@ -6,6 +6,7 @@
import static com.android.tools.r8.naming.IdentifierNameStringUtils.identifyIdentiferNameString;
import static com.android.tools.r8.naming.IdentifierNameStringUtils.inferMemberOrTypeFromNameString;
import static com.android.tools.r8.naming.IdentifierNameStringUtils.isReflectionMethod;
+import static com.android.tools.r8.naming.IdentifierNameStringUtils.warnUndeterminedIdentifier;
import com.android.tools.r8.graph.AppInfo;
import com.android.tools.r8.graph.DexEncodedField;
@@ -31,7 +32,9 @@
import com.android.tools.r8.ir.code.InvokeMethod;
import com.android.tools.r8.ir.code.StaticPut;
import com.android.tools.r8.ir.code.Value;
+import com.android.tools.r8.origin.Origin;
import com.android.tools.r8.shaking.Enqueuer.AppInfoWithLiveness;
+import com.android.tools.r8.utils.InternalOptions;
import com.google.common.collect.Streams;
import java.util.Arrays;
import java.util.List;
@@ -44,12 +47,14 @@
private final AppInfo appInfo;
private final DexItemFactory dexItemFactory;
private final Set<DexItem> identifierNameStrings;
+ private final InternalOptions options;
- public IdentifierNameStringMarker(AppInfoWithLiveness appInfo) {
+ public IdentifierNameStringMarker(AppInfoWithLiveness appInfo, InternalOptions options) {
this.appInfo = appInfo;
this.dexItemFactory = appInfo.dexItemFactory;
// Note that this info is only available at AppInfoWithLiveness.
this.identifierNameStrings = appInfo.identifierNameStrings;
+ this.options = options;
}
public void decoupleIdentifierNameStringsInFields() {
@@ -77,6 +82,7 @@
}
public void decoupleIdentifierNameStringsInMethod(DexEncodedMethod encodedMethod, IRCode code) {
+ Origin origin = appInfo.originFor(code.method.method.getHolder());
ListIterator<BasicBlock> blocks = code.listIterator();
while (blocks.hasNext()) {
BasicBlock block = blocks.next();
@@ -104,11 +110,17 @@
? instruction.asStaticPut().inValue()
: instruction.asInstancePut().value();
if (!in.isConstString()) {
+ if (options.proguardConfiguration.isObfuscating()) {
+ warnUndeterminedIdentifier(options.reporter, field, origin, instruction, null);
+ }
continue;
}
DexString original = in.getConstInstruction().asConstString().getValue();
DexItemBasedString itemBasedString = inferMemberOrTypeFromNameString(appInfo, original);
if (itemBasedString == null) {
+ if (options.proguardConfiguration.isObfuscating()) {
+ warnUndeterminedIdentifier(options.reporter, field, origin, instruction, original);
+ }
continue;
}
// Move the cursor back to $fieldPut
@@ -162,6 +174,10 @@
if (isReflectionMethod(dexItemFactory, invokedMethod)) {
DexItemBasedString itemBasedString = identifyIdentiferNameString(appInfo, invoke);
if (itemBasedString == null) {
+ if (options.proguardConfiguration.isObfuscating()) {
+ warnUndeterminedIdentifier(
+ options.reporter, invokedMethod, origin, instruction, null);
+ }
continue;
}
DexType returnType = invoke.getReturnType();
@@ -206,12 +222,20 @@
for (int i = 0; i < ins.size(); i++) {
Value in = ins.get(i);
if (!in.isConstString()) {
+ if (options.proguardConfiguration.isObfuscating()) {
+ warnUndeterminedIdentifier(
+ options.reporter, invokedMethod, origin, instruction, null);
+ }
continue;
}
DexString original = in.getConstInstruction().asConstString().getValue();
DexItemBasedString itemBasedString =
inferMemberOrTypeFromNameString(appInfo, original);
if (itemBasedString == null) {
+ if (options.proguardConfiguration.isObfuscating()) {
+ warnUndeterminedIdentifier(
+ options.reporter, invokedMethod, origin, instruction, original);
+ }
continue;
}
// Move the cursor back to $invoke
diff --git a/src/main/java/com/android/tools/r8/naming/IdentifierNameStringUtils.java b/src/main/java/com/android/tools/r8/naming/IdentifierNameStringUtils.java
index 4c64cf6..4f66be2 100644
--- a/src/main/java/com/android/tools/r8/naming/IdentifierNameStringUtils.java
+++ b/src/main/java/com/android/tools/r8/naming/IdentifierNameStringUtils.java
@@ -9,6 +9,7 @@
import com.android.tools.r8.graph.DexClass;
import com.android.tools.r8.graph.DexEncodedField;
import com.android.tools.r8.graph.DexEncodedMethod;
+import com.android.tools.r8.graph.DexField;
import com.android.tools.r8.graph.DexItem;
import com.android.tools.r8.graph.DexItemBasedString;
import com.android.tools.r8.graph.DexItemFactory;
@@ -23,6 +24,10 @@
import com.android.tools.r8.ir.code.InstructionIterator;
import com.android.tools.r8.ir.code.InvokeMethod;
import com.android.tools.r8.ir.code.Value;
+import com.android.tools.r8.origin.Origin;
+import com.android.tools.r8.position.TextPosition;
+import com.android.tools.r8.utils.Reporter;
+import com.android.tools.r8.utils.StringDiagnostic;
import it.unimi.dsi.fastutil.ints.Int2ObjectArrayMap;
import java.util.List;
import java.util.Map;
@@ -366,4 +371,23 @@
}
return new DexTypeList(types);
}
+
+ public static void warnUndeterminedIdentifier(
+ Reporter reporter,
+ DexItem member,
+ Origin origin,
+ Instruction instruction,
+ DexString original) {
+ assert member instanceof DexField || member instanceof DexMethod;
+ String kind = member instanceof DexField ? "field" : "method";
+ String originalMessage = original == null ? "what identifier string flows to "
+ : "what '" + original.toString() + "' refers to, which flows to ";
+ reporter.warning(new StringDiagnostic(
+ "Cannot determine " + originalMessage + member.toSourceString()
+ + " that is specified in -identifiernamestring rules."
+ + " Thus, not all identifier strings flowing to that " + kind
+ + " are renamed, which can cause resolution failures at runtime.",
+ origin,
+ new TextPosition(0L, instruction.getPosition().line, 1)));
+ }
}
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 baacb1e..69e0221 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.warnUndeterminedIdentifier;
import static com.android.tools.r8.shaking.ProguardConfigurationUtils.buildIdentifierNameStringRule;
import com.android.tools.r8.Diagnostic;
@@ -39,6 +40,7 @@
import com.android.tools.r8.ir.code.Invoke.Type;
import com.android.tools.r8.ir.code.InvokeMethod;
import com.android.tools.r8.logging.Log;
+import com.android.tools.r8.origin.Origin;
import com.android.tools.r8.shaking.RootSetBuilder.ConsequentRootSet;
import com.android.tools.r8.shaking.RootSetBuilder.RootSet;
import com.android.tools.r8.shaking.protolite.ProtoLiteExtension;
@@ -1357,11 +1359,13 @@
}
private void handleProguardReflectiveBehavior(DexEncodedMethod method) {
- IRCode code = method.buildIR(appInfo, options, appInfo.originFor(method.method.holder));
- code.instructionIterator().forEachRemaining(this::handleProguardReflectiveBehavior);
+ Origin origin = appInfo.originFor(method.method.holder);
+ IRCode code = method.buildIR(appInfo, options, origin);
+ code.instructionIterator().forEachRemaining(instr ->
+ handleProguardReflectiveBehavior(instr, origin));
}
- private void handleProguardReflectiveBehavior(Instruction instruction) {
+ private void handleProguardReflectiveBehavior(Instruction instruction, Origin origin) {
if (!instruction.isInvokeMethod()) {
return;
}
@@ -1372,6 +1376,9 @@
}
DexItemBasedString itemBasedString = identifyIdentiferNameString(appInfo, invoke);
if (itemBasedString == null) {
+ if (options.proguardConfiguration.isObfuscating()) {
+ warnUndeterminedIdentifier(options.reporter, invokedMethod, origin, instruction, null);
+ }
return;
}
if (itemBasedString.basedOn instanceof DexType) {
diff --git a/src/test/java/com/android/tools/r8/naming/WarnReflectiveAccessTest.java b/src/test/java/com/android/tools/r8/naming/WarnReflectiveAccessTest.java
new file mode 100644
index 0000000..ddd0aa2
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/naming/WarnReflectiveAccessTest.java
@@ -0,0 +1,167 @@
+// 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.naming;
+
+import static com.android.tools.r8.utils.DexInspectorMatchers.isPresent;
+import static org.hamcrest.CoreMatchers.containsString;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotEquals;
+import static org.junit.Assert.assertThat;
+
+import com.android.tools.r8.DiagnosticsChecker;
+import com.android.tools.r8.OutputMode;
+import com.android.tools.r8.R8Command;
+import com.android.tools.r8.TestBase;
+import com.android.tools.r8.ToolHelper;
+import com.android.tools.r8.ToolHelper.ProcessResult;
+import com.android.tools.r8.VmTestRunner;
+import com.android.tools.r8.origin.Origin;
+import com.android.tools.r8.utils.AndroidApp;
+import com.android.tools.r8.utils.DexInspector;
+import com.android.tools.r8.utils.DexInspector.ClassSubject;
+import com.android.tools.r8.utils.KeepingDiagnosticHandler;
+import com.android.tools.r8.utils.Reporter;
+import com.google.common.collect.ImmutableList;
+import java.lang.reflect.Method;
+import java.nio.file.Path;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+class WarnReflectiveAccessTestMain {
+ int counter = 0;
+
+ WarnReflectiveAccessTestMain() {
+ }
+
+ public void foo() {
+ System.out.println("TestMain::foo(" + counter++ + ")");
+ }
+
+ public int boo() {
+ System.out.println("TestMain::boo(" + counter + ")");
+ return counter;
+ }
+
+ public static void main(String[] args) throws Exception {
+ WarnReflectiveAccessTestMain instance = new WarnReflectiveAccessTestMain();
+
+ StringBuilder builder = new StringBuilder();
+ builder.append("f");
+ builder.append("o").append("o");
+ Method foo = instance.getClass().getDeclaredMethod(builder.toString());
+ foo.invoke(instance);
+ }
+}
+
+@RunWith(VmTestRunner.class)
+public class WarnReflectiveAccessTest extends TestBase {
+
+ private KeepingDiagnosticHandler handler;
+ private Reporter reporter;
+
+ @Before
+ public void reset() {
+ handler = new KeepingDiagnosticHandler();
+ reporter = new Reporter(handler);
+ }
+
+ private AndroidApp runR8(
+ byte[][] classes,
+ Class main,
+ Path out,
+ boolean enableMinification,
+ boolean forceProguardCompatibility)
+ throws Exception {
+ String minificationModifier = enableMinification ? ",allowobfuscation " : " ";
+ 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";
+ R8Command.Builder commandBuilder =
+ R8Command.builder(reporter)
+ .addProguardConfiguration(
+ ImmutableList.of(
+ "-keep" + minificationModifier + "class " + main.getCanonicalName() + " {"
+ + " <methods>;"
+ + "}",
+ "-printmapping",
+ reflectionRules),
+ Origin.unknown())
+ .setOutput(out, OutputMode.DexIndexed);
+ for (byte[] clazz : classes) {
+ commandBuilder.addClassProgramData(clazz, Origin.unknown());
+ }
+ commandBuilder.addLibraryFiles(ToolHelper.getDefaultAndroidJar());
+ return ToolHelper.runR8(commandBuilder.build(), o -> {
+ o.enableInlining = false;
+ o.forceProguardCompatibility = forceProguardCompatibility;
+ });
+ }
+
+ private void reflectionWithBuildter(
+ boolean enableMinification,
+ boolean forceProguardCompatibility) throws Exception {
+ byte[][] classes = {
+ ToolHelper.getClassAsBytes(WarnReflectiveAccessTestMain.class)
+ };
+ Path out = temp.getRoot().toPath();
+ AndroidApp processedApp = runR8(classes, WarnReflectiveAccessTestMain.class, out,
+ enableMinification, forceProguardCompatibility);
+
+ String main = WarnReflectiveAccessTestMain.class.getCanonicalName();
+ DexInspector dexInspector = new DexInspector(processedApp);
+ ClassSubject mainSubject = dexInspector.clazz(main);
+ assertThat(mainSubject, isPresent());
+
+ ProcessResult javaOutput = runOnJavaRaw(main, classes);
+ assertEquals(0, javaOutput.exitCode);
+ assertThat(javaOutput.stdout, containsString("TestMain::foo"));
+
+ ProcessResult artOutput = runOnArtRaw(processedApp,
+ enableMinification ? mainSubject.getFinalName() : main);
+ if (enableMinification) {
+ assertNotEquals(0, artOutput.exitCode);
+ assertThat(artOutput.stderr, containsString("NoSuchMethodError"));
+ } else {
+ assertEquals(0, artOutput.exitCode);
+ assertThat(artOutput.stdout, containsString("TestMain::foo"));
+ }
+ }
+
+ @Test
+ public void test_minification_forceProguardCompatibility() throws Exception {
+ reflectionWithBuildter(true, true);
+ assertFalse(handler.warnings.isEmpty());
+ 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_noMinification_R8() throws Exception {
+ reflectionWithBuildter(false, false);
+ assertFalse(handler.warnings.isEmpty());
+ DiagnosticsChecker.checkDiagnostic(handler.warnings.get(0), (Path) null, 54, 1,
+ "Cannot determine", "getDeclaredMethod", "-identifiernamestring", "resolution failure");
+ }
+
+}
diff --git a/src/test/java/com/android/tools/r8/utils/DexInspector.java b/src/test/java/com/android/tools/r8/utils/DexInspector.java
index 0b8de12..eba3be8 100644
--- a/src/test/java/com/android/tools/r8/utils/DexInspector.java
+++ b/src/test/java/com/android/tools/r8/utils/DexInspector.java
@@ -413,6 +413,8 @@
public abstract String getOriginalDescriptor();
+ public abstract String getFinalName();
+
public abstract String getFinalDescriptor();
public abstract boolean isMemberClass();
@@ -482,6 +484,11 @@
}
@Override
+ public String getFinalName() {
+ return null;
+ }
+
+ @Override
public String getFinalDescriptor() {
return null;
}
@@ -639,7 +646,7 @@
if (naming != null) {
return naming.originalName;
} else {
- return DescriptorUtils.descriptorToJavaType(getFinalDescriptor());
+ return getFinalName();
}
}
@@ -653,6 +660,11 @@
}
@Override
+ public String getFinalName() {
+ return DescriptorUtils.descriptorToJavaType(getFinalDescriptor());
+ }
+
+ @Override
public String getFinalDescriptor() {
return dexClass.type.descriptor.toString();
}
