Use original type/signature when checking noObfuscation.
Otherwise, after certain optimizations that may change signatures,
items that are expected not to be renamed could be renamed.
Bug: 130791310
Change-Id: Ia23b0769d6fae8e159edb6a24b0d69adea95cf4d
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 725f500..f37e60f 100644
--- a/src/main/java/com/android/tools/r8/naming/ClassNameMinifier.java
+++ b/src/main/java/com/android/tools/r8/naming/ClassNameMinifier.java
@@ -15,7 +15,6 @@
import com.android.tools.r8.graph.DexEncodedField;
import com.android.tools.r8.graph.DexEncodedMethod;
import com.android.tools.r8.graph.DexProto;
-import com.android.tools.r8.graph.DexReference;
import com.android.tools.r8.graph.DexString;
import com.android.tools.r8.graph.DexType;
import com.android.tools.r8.graph.InnerClassAttribute;
@@ -105,7 +104,7 @@
// Collect names we have to keep.
timing.begin("reserve");
for (DexClass clazz : classes) {
- if (classNamingStrategy.noObfuscation().contains(clazz.type)) {
+ if (classNamingStrategy.noObfuscation(clazz.type)) {
assert !renaming.containsKey(clazz.type);
registerClassAsUsed(clazz.type);
}
@@ -183,7 +182,7 @@
private void renameDanglingType(DexType type) {
if (appView.appInfo().wasPruned(type)
&& !renaming.containsKey(type)
- && !classNamingStrategy.noObfuscation().contains(type)) {
+ && !classNamingStrategy.noObfuscation(type)) {
// We have a type that is defined in the program source but is only used in a proto or
// return type. As we don't need the class, we can rename it to anything as long as it is
// unique.
@@ -201,7 +200,7 @@
DexType outerClass = getOutClassForType(type);
if (outerClass != null) {
if (!renaming.containsKey(outerClass)
- && !classNamingStrategy.noObfuscation().contains(outerClass)) {
+ && !classNamingStrategy.noObfuscation(outerClass)) {
// The outer class was not previously kept and will not be kept.
// We have to force keep the outer class now.
registerClassAsUsed(outerClass);
@@ -451,7 +450,7 @@
boolean bypassDictionary();
- Set<DexReference> noObfuscation();
+ boolean noObfuscation(DexType type);
}
protected interface PackageNamingStrategy {
diff --git a/src/main/java/com/android/tools/r8/naming/FieldNameMinifier.java b/src/main/java/com/android/tools/r8/naming/FieldNameMinifier.java
index 06c2909..991805a 100644
--- a/src/main/java/com/android/tools/r8/naming/FieldNameMinifier.java
+++ b/src/main/java/com/android/tools/r8/naming/FieldNameMinifier.java
@@ -114,7 +114,8 @@
return true;
}
if (!appView.options().getProguardConfiguration().hasApplyMappingFile()
- && appView.rootSet().noObfuscation.contains(field.field)) {
+ && appView.rootSet().noObfuscation.contains(
+ appView.graphLense().getOriginalFieldSignature(field.field))) {
return true;
}
return false;
diff --git a/src/main/java/com/android/tools/r8/naming/MemberNamingStrategy.java b/src/main/java/com/android/tools/r8/naming/MemberNamingStrategy.java
index 5c37c46..8a58da2 100644
--- a/src/main/java/com/android/tools/r8/naming/MemberNamingStrategy.java
+++ b/src/main/java/com/android/tools/r8/naming/MemberNamingStrategy.java
@@ -8,7 +8,6 @@
import com.android.tools.r8.graph.DexMethod;
import com.android.tools.r8.graph.DexReference;
import com.android.tools.r8.graph.DexString;
-import java.util.Set;
public interface MemberNamingStrategy {
@@ -20,5 +19,5 @@
boolean breakOnNotAvailable(DexReference source, DexString name);
- Set<DexReference> noObfuscation();
+ boolean noObfuscation(DexReference reference);
}
diff --git a/src/main/java/com/android/tools/r8/naming/MethodNameMinifier.java b/src/main/java/com/android/tools/r8/naming/MethodNameMinifier.java
index a8892d8..ee1b64d 100644
--- a/src/main/java/com/android/tools/r8/naming/MethodNameMinifier.java
+++ b/src/main/java/com/android/tools/r8/naming/MethodNameMinifier.java
@@ -293,7 +293,7 @@
// methods?
if (keepAll
|| method.accessFlags.isConstructor()
- || strategy.noObfuscation().contains(method.method)) {
+ || strategy.noObfuscation(method.method)) {
reserveNamesForMethod(method.method, state);
}
}
diff --git a/src/main/java/com/android/tools/r8/naming/Minifier.java b/src/main/java/com/android/tools/r8/naming/Minifier.java
index e9dc45b..264167e 100644
--- a/src/main/java/com/android/tools/r8/naming/Minifier.java
+++ b/src/main/java/com/android/tools/r8/naming/Minifier.java
@@ -7,7 +7,6 @@
import com.android.tools.r8.graph.DexCallSite;
import com.android.tools.r8.graph.DexClass;
import com.android.tools.r8.graph.DexField;
-import com.android.tools.r8.graph.DexItemFactory;
import com.android.tools.r8.graph.DexMethod;
import com.android.tools.r8.graph.DexReference;
import com.android.tools.r8.graph.DexString;
@@ -46,8 +45,7 @@
ClassNameMinifier classNameMinifier =
new ClassNameMinifier(
appView,
- new MinificationClassNamingStrategy(
- appView.dexItemFactory(), appView.rootSet().noObfuscation),
+ new MinificationClassNamingStrategy(appView),
new MinificationPackageNamingStrategy(),
// Use deterministic class order to make sure renaming is deterministic.
appView.appInfo().classesWithDeterministicOrder());
@@ -58,8 +56,7 @@
appView, classRenaming, MethodRenaming.empty(), FieldRenaming.empty())
.verifyNoCollisions(appView.appInfo().classes(), appView.dexItemFactory());
- MemberNamingStrategy minifyMembers =
- new MinifierMemberNamingStrategy(appView.dexItemFactory(), appView.rootSet().noObfuscation);
+ MemberNamingStrategy minifyMembers = new MinifierMemberNamingStrategy(appView);
timing.begin("MinifyMethods");
MethodRenaming methodRenaming =
new MethodNameMinifier(appView, minifyMembers)
@@ -85,13 +82,11 @@
static class MinificationClassNamingStrategy implements ClassNamingStrategy {
- private final DexItemFactory factory;
+ private final AppView<?> appView;
private final Object2IntMap<Namespace> namespaceCounters = new Object2IntLinkedOpenHashMap<>();
- private final Set<DexReference> noObfuscation;
- MinificationClassNamingStrategy(DexItemFactory factory, Set<DexReference> noObfuscation) {
- this.factory = factory;
- this.noObfuscation = noObfuscation;
+ MinificationClassNamingStrategy(AppView<?> appView) {
+ this.appView = appView;
namespaceCounters.defaultReturnValue(1);
}
@@ -99,7 +94,8 @@
public DexString next(Namespace namespace, DexType type, char[] packagePrefix) {
int counter = namespaceCounters.put(namespace, namespaceCounters.getInt(namespace) + 1);
DexString string =
- factory.createString(StringUtils.numberToIdentifier(packagePrefix, counter, true));
+ appView.dexItemFactory()
+ .createString(StringUtils.numberToIdentifier(packagePrefix, counter, true));
return string;
}
@@ -109,8 +105,8 @@
}
@Override
- public Set<DexReference> noObfuscation() {
- return noObfuscation;
+ public boolean noObfuscation(DexType type) {
+ return appView.rootSet().noObfuscation.contains(appView.graphLense().getOriginalType(type));
}
}
@@ -142,18 +138,17 @@
public static char[] EMPTY_CHAR_ARRAY = new char[0];
- private final DexItemFactory factory;
- private final Set<DexReference> noObfuscation;
+ private final AppView<?> appView;
- public MinifierMemberNamingStrategy(DexItemFactory factory, Set<DexReference> noObfuscation) {
- this.factory = factory;
- this.noObfuscation = noObfuscation;
+ public MinifierMemberNamingStrategy(AppView<?> appView) {
+ this.appView = appView;
}
@Override
public DexString next(DexMethod method, MethodNamingState.InternalState internalState) {
int counter = internalState.incrementAndGet();
- return factory.createString(StringUtils.numberToIdentifier(EMPTY_CHAR_ARRAY, counter, false));
+ return appView.dexItemFactory()
+ .createString(StringUtils.numberToIdentifier(EMPTY_CHAR_ARRAY, counter, false));
}
@Override
@@ -172,8 +167,15 @@
}
@Override
- public Set<DexReference> noObfuscation() {
- return noObfuscation;
+ public boolean noObfuscation(DexReference reference) {
+ if (reference.isDexField()) {
+ return appView.rootSet().noObfuscation.contains(
+ appView.graphLense().getOriginalFieldSignature(reference.asDexField()));
+ } else {
+ assert reference.isDexMethod();
+ return appView.rootSet().noObfuscation.contains(
+ appView.graphLense().getOriginalMethodSignature(reference.asDexMethod()));
+ }
}
}
}
diff --git a/src/main/java/com/android/tools/r8/naming/ProguardMapMinifier.java b/src/main/java/com/android/tools/r8/naming/ProguardMapMinifier.java
index afdc1e5..a3f5ca5 100644
--- a/src/main/java/com/android/tools/r8/naming/ProguardMapMinifier.java
+++ b/src/main/java/com/android/tools/r8/naming/ProguardMapMinifier.java
@@ -28,7 +28,6 @@
import com.android.tools.r8.utils.Reporter;
import com.android.tools.r8.utils.Timing;
import java.util.ArrayList;
-import java.util.HashSet;
import java.util.IdentityHashMap;
import java.util.List;
import java.util.Map;
@@ -200,8 +199,6 @@
static class ApplyMappingClassNamingStrategy implements ClassNamingStrategy {
private final Map<DexType, DexString> mappings;
- // We have an explicit mapping from the proguard map thus everything might have to be renamed.
- private final Set<DexReference> noObfuscation = new HashSet<>();
ApplyMappingClassNamingStrategy(Map<DexType, DexString> mappings) {
this.mappings = mappings;
@@ -218,8 +215,9 @@
}
@Override
- public Set<DexReference> noObfuscation() {
- return noObfuscation;
+ public boolean noObfuscation(DexType type) {
+ // We have an explicit mapping from the proguard map thus everything might have to be renamed.
+ return false;
}
}
@@ -228,8 +226,6 @@
private final Map<DexReference, MemberNaming> mappedNames;
private final DexItemFactory factory;
private final Reporter reporter;
- // We have an explicit mapping from the proguard map thus everything might have to be renamed.
- private final Set<DexReference> noObfuscation = new HashSet<>();
public ApplyMappingMemberNamingStrategy(
Map<DexReference, MemberNaming> mappedNames, DexItemFactory factory, Reporter reporter) {
@@ -284,8 +280,9 @@
}
@Override
- public Set<DexReference> noObfuscation() {
- return noObfuscation;
+ public boolean noObfuscation(DexReference reference) {
+ // We have an explicit mapping from the proguard map thus everything might have to be renamed.
+ return false;
}
}
}
diff --git a/src/test/java/com/android/tools/r8/naming/b130791310/B130791310.java b/src/test/java/com/android/tools/r8/naming/b130791310/B130791310.java
new file mode 100644
index 0000000..40482d3
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/naming/b130791310/B130791310.java
@@ -0,0 +1,145 @@
+// Copyright (c) 2019, 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.b130791310;
+
+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.hamcrest.MatcherAssert.assertThat;
+
+import com.android.tools.r8.ProguardTestBuilder;
+import com.android.tools.r8.R8FullTestBuilder;
+import com.android.tools.r8.TestBase;
+import com.android.tools.r8.ToolHelper;
+import com.android.tools.r8.utils.BooleanUtils;
+import com.android.tools.r8.utils.StringUtils;
+import com.android.tools.r8.utils.codeinspector.ClassSubject;
+import com.android.tools.r8.utils.codeinspector.CodeInspector;
+import com.android.tools.r8.utils.codeinspector.MethodSubject;
+import com.google.common.collect.ImmutableList;
+import java.util.List;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+class TestClass {
+ SomeLogic instance;
+
+ public void create() {
+ instance = new SomeLogic(new Object());
+ }
+
+ void caller() {
+ if (instance.someMethod(new SomeClass()) == null) {
+ System.out.println("TestMain#caller");
+ }
+ }
+
+ public static void main(String[] args) {
+ TestClass m = new TestClass();
+ m.create();
+ m.caller();
+ }
+}
+
+class SomeLogic {
+ Object otherField;
+
+ public SomeLogic(Object o) {
+ otherField = o;
+ }
+
+ public SomeInterface someMethod(SomeClass list) {
+ System.out.println("list is" + ((list == null) ? "" : " not") + " null");
+ return null;
+ }
+}
+
+interface SomeInterface {
+ byte foo();
+}
+
+class SomeClass implements SomeInterface {
+ @Override
+ public byte foo() {
+ return 0x8;
+ }
+}
+
+// SomeInterface has a single implementer SomeClass: vertical merging candidate.
+// If -keepnames specifies a member whose signature can be changed due to vertical merging, their
+// names are not preserved because that rule allows shrinking (and optimization) implicitly.
+// According to b/130791310, Proguard (via AGP) still keeps that name, but this reproduction shows
+// that both R8 and Proguard ignores -keepnames. It turns out that, as per
+// https://android.googlesource.com/platform/sdk/+/master/files/proguard-android-optimize.txt#13
+// class merging is disabled for Proguard.
+@RunWith(Parameterized.class)
+public class B130791310 extends TestBase {
+ private static final Class<?> MAIN = TestClass.class;
+ private static final List<Class<?>> CLASSES =
+ ImmutableList.of(MAIN, SomeLogic.class, SomeInterface.class, SomeClass.class);
+ private static final String RULES = StringUtils.lines(
+ "-keepnames class **.SomeLogic {",
+ " **.SomeInterface someMethod(**.SomeClass);",
+ "}"
+ );
+
+ private final boolean enableClassMerging;
+
+ @Parameterized.Parameters(name = "enable class merging: {0}")
+ public static Boolean[] data() {
+ return BooleanUtils.values();
+ }
+
+ public B130791310(boolean enableClassMerging) {
+ this.enableClassMerging = enableClassMerging;
+ }
+
+ private void inspect(CodeInspector inspector, boolean isR8) {
+ ClassSubject holder = inspector.clazz(SomeLogic.class);
+ assertThat(holder, isPresent());
+ assertThat(holder, not(isRenamed()));
+ MethodSubject someMethod = holder.uniqueMethodWithName("someMethod");
+ if (enableClassMerging && !isR8) {
+ // Note that the method is not entirely gone, but merged to the implementer, along with some
+ // method signature modification.
+ assertThat(someMethod, not(isPresent()));
+ } else {
+ assertThat(someMethod, isPresent());
+ assertThat(someMethod, not(isRenamed()));
+ }
+ }
+
+ @Test
+ public void testProguard() throws Exception {
+ ProguardTestBuilder builder =
+ testForProguard()
+ .addProgramClasses(CLASSES)
+ .addLibraryFiles(ToolHelper.getDefaultAndroidJar())
+ .addKeepClassAndMembersRules(MAIN)
+ .addKeepRules(RULES);
+ if (!enableClassMerging) {
+ builder.addKeepRules("-optimizations !class/merging/*");
+ }
+ builder
+ .compile()
+ .inspect(inspector -> inspect(inspector, false));
+ }
+
+ @Test
+ public void testR8() throws Exception {
+ R8FullTestBuilder builder =
+ testForR8(Backend.DEX)
+ .addProgramClasses(CLASSES)
+ .addLibraryFiles(ToolHelper.getDefaultAndroidJar())
+ .addKeepClassAndMembersRules(MAIN)
+ .addKeepRules(RULES);
+ if (!enableClassMerging) {
+ builder.addOptionsModification(o -> o.enableVerticalClassMerging = false);
+ }
+ builder
+ .compile()
+ .inspect(inspector -> inspect(inspector, true));
+ }
+}
