Version 1.4.33
Cherry-pick:
Clarify test expectations of non-rebound field renaming.
https://r8-review.googlesource.com/c/r8/+/33740
Cherry-pick:
Rename non-rebound field references according to their definitions.
https://r8-review.googlesource.com/c/r8/+/33701
Cherry-pick:
Reproduce b/123068484: no field rebinding due to visibility restriction.
https://r8-review.googlesource.com/c/r8/+/33680
Bug: 123068484
Change-Id: Iecddaf96ad9a3145a8149ffa2ce39ca23e05d7d5
diff --git a/src/main/java/com/android/tools/r8/Version.java b/src/main/java/com/android/tools/r8/Version.java
index ec1ec47..20093ab 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 = "1.4.32";
+ public static final String LABEL = "1.4.33";
private Version() {
}
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 3b546a8..b589aa4 100644
--- a/src/main/java/com/android/tools/r8/naming/FieldNameMinifier.java
+++ b/src/main/java/com/android/tools/r8/naming/FieldNameMinifier.java
@@ -12,6 +12,7 @@
import com.android.tools.r8.shaking.RootSetBuilder.RootSet;
import com.android.tools.r8.utils.InternalOptions;
import com.android.tools.r8.utils.Timing;
+import com.google.common.collect.Sets;
import java.util.Map;
import java.util.function.Function;
@@ -45,11 +46,15 @@
DexType.forAllInterfaces(appInfo.dexItemFactory,
iface -> reserveNamesInSubtypes(iface, globalState));
timing.end();
- // Now rename the rest.
- timing.begin("rename");
+ // Rename the definitions.
+ timing.begin("rename-definitions");
renameFieldsInSubtypes(appInfo.dexItemFactory.objectType);
DexType.forAllInterfaces(appInfo.dexItemFactory, this::renameFieldsInSubtypes);
timing.end();
+ // Rename the references that are not rebound to definitions for some reasons.
+ timing.begin("rename-references");
+ renameNonReboundReferences();
+ timing.end();
return renaming;
}
@@ -92,4 +97,44 @@
state.assignNewNameFor(field.name, field.type, useUniqueMemberNames));
}
}
+
+ private void renameNonReboundReferences() {
+ // TODO(b/123068484): Collect non-rebound references instead of visiting all references.
+ Sets.union(
+ Sets.union(appInfo.staticFieldReads.keySet(), appInfo.staticFieldWrites.keySet()),
+ Sets.union(appInfo.instanceFieldReads.keySet(), appInfo.instanceFieldWrites.keySet()))
+ .forEach(this::renameNonReboundReference);
+ }
+
+ private void renameNonReboundReference(DexField field) {
+ // Already renamed
+ if (renaming.containsKey(field)) {
+ return;
+ }
+ DexEncodedField definition = appInfo.definitionFor(field);
+ if (definition != null) {
+ assert definition.field == field;
+ return;
+ }
+ // Now, `field` is reference. Find its definition and check if it's renamed.
+ DexType holderType = field.getHolder();
+ DexClass holder = appInfo.definitionFor(holderType);
+ // We don't care pruned types or library classes.
+ if (holder == null || holder.isLibraryClass()) {
+ return;
+ }
+ definition = appInfo.resolveFieldOn(holderType, field);
+ if (definition == null) {
+ // The program is already broken in the sense that it has an unresolvable field reference.
+ // Leave it as-is.
+ return;
+ }
+ assert definition.field != field;
+ assert definition.field.getHolder() != holderType;
+ // If the definition is renamed,
+ if (renaming.containsKey(definition.field)) {
+ // Assign the same, renamed name as the definition to the reference.
+ renaming.put(field, renaming.get(definition.field));
+ }
+ }
}
diff --git a/src/test/java/com/android/tools/r8/TestBase.java b/src/test/java/com/android/tools/r8/TestBase.java
index 1df6808..2b66df4 100644
--- a/src/test/java/com/android/tools/r8/TestBase.java
+++ b/src/test/java/com/android/tools/r8/TestBase.java
@@ -50,10 +50,13 @@
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Path;
+import java.nio.file.Paths;
import java.nio.file.StandardOpenOption;
import java.util.ArrayList;
import java.util.Arrays;
+import java.util.Collection;
import java.util.Collections;
+import java.util.LinkedList;
import java.util.List;
import java.util.concurrent.ExecutionException;
import java.util.function.Consumer;
@@ -903,18 +906,31 @@
return extractor.getClassInternalType();
}
- protected Path writeToJar(List<byte[]> classes) throws IOException {
- Path result = File.createTempFile("junit", ".jar", temp.getRoot()).toPath();
+ protected static void writeToJar(Path output, List<byte[]> classes) throws IOException {
try (ZipOutputStream out =
new ZipOutputStream(
Files.newOutputStream(
- result, StandardOpenOption.CREATE, StandardOpenOption.TRUNCATE_EXISTING))) {
+ output, StandardOpenOption.CREATE, StandardOpenOption.TRUNCATE_EXISTING))) {
for (byte[] clazz : classes) {
String name = extractClassName(clazz);
ZipUtils.writeToZipStream(
out, DescriptorUtils.getPathFromJavaType(name), clazz, ZipEntry.STORED);
}
}
+ }
+
+ protected static void writeToJar(Path output, Collection<Path> classes) throws IOException {
+ List<byte[]> bytes = new LinkedList<>();
+ for (Path classPath : classes) {
+ byte[] classBytes = Files.readAllBytes(Paths.get(classPath.toString()));
+ bytes.add(classBytes);
+ }
+ writeToJar(output, bytes);
+ }
+
+ protected Path writeToJar(List<byte[]> classes) throws IOException {
+ Path result = File.createTempFile("junit", ".jar", temp.getRoot()).toPath();
+ writeToJar(result, classes);
return result;
}
diff --git a/src/test/java/com/android/tools/r8/naming/b123068484/FieldRenamingTest.java b/src/test/java/com/android/tools/r8/naming/b123068484/FieldRenamingTest.java
new file mode 100644
index 0000000..4cedaea
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/naming/b123068484/FieldRenamingTest.java
@@ -0,0 +1,97 @@
+// 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.b123068484;
+
+import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotEquals;
+import static org.junit.Assert.assertThat;
+
+import com.android.tools.r8.TestBase;
+import com.android.tools.r8.ToolHelper;
+import com.android.tools.r8.naming.b123068484.data.Concrete1;
+import com.android.tools.r8.naming.b123068484.runner.Runner;
+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.FieldSubject;
+import com.android.tools.r8.utils.codeinspector.InstructionSubject;
+import com.android.tools.r8.utils.codeinspector.MethodSubject;
+import com.google.common.collect.ImmutableList;
+import java.nio.file.Path;
+import java.util.List;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+@RunWith(Parameterized.class)
+public class FieldRenamingTest extends TestBase {
+ private static final Class<?> MAIN = Runner.class;
+ private static final Class<?> CONCRETE1 = Concrete1.class;
+ private static List<Path> CLASSES;
+ private static final String EXPECTED_OUTPUT = StringUtils.lines("Runner");
+
+ private final Backend backend;
+
+ @Parameterized.Parameters(name = "Backend: {0}")
+ public static Object[] data() {
+ return Backend.values();
+ }
+
+ public FieldRenamingTest(Backend backend) {
+ this.backend = backend;
+ }
+
+ @BeforeClass
+ public static void setUpClass() throws Exception {
+ CLASSES = ImmutableList.<Path>builder()
+ .addAll(ToolHelper.getClassFilesForTestPackage(MAIN.getPackage()))
+ .addAll(ToolHelper.getClassFilesForTestPackage(CONCRETE1.getPackage()))
+ .build();
+ }
+
+ @Test
+ public void testProguard() throws Exception {
+ Path inJar = temp.newFile("input.jar").toPath().toAbsolutePath();
+ writeToJar(inJar, CLASSES);
+ testForProguard()
+ .addProgramFiles(inJar)
+ .addKeepMainRule(MAIN)
+ .run(MAIN)
+ .assertSuccessWithOutput(EXPECTED_OUTPUT)
+ .inspect(this::inspect);
+ }
+
+ @Test
+ public void testR8() throws Exception {
+ testForR8(backend)
+ .addProgramFiles(CLASSES)
+ .addKeepMainRule(MAIN)
+ .run(MAIN)
+ .assertSuccessWithOutput(EXPECTED_OUTPUT)
+ .inspect(this::inspect);
+ }
+
+ private void inspect(CodeInspector inspector) {
+ FieldSubject fld = inspector.clazz(CONCRETE1.getTypeName().replace("Concrete1", "Abs"))
+ .uniqueFieldWithName("strField");
+ assertThat(fld, isPresent());
+
+ ClassSubject main = inspector.clazz(MAIN);
+ assertThat(main, isPresent());
+ MethodSubject methodSubject = main.mainMethod();
+ assertThat(methodSubject, isPresent());
+
+ methodSubject
+ .iterateInstructions(InstructionSubject::isInstanceGet)
+ .forEachRemaining(instructionSubject -> {
+ String fieldName = instructionSubject.getField().name.toString();
+ // All of those field references will be renamed.
+ assertNotEquals("strField", fieldName);
+ assertEquals(fld.getFinalName(), fieldName);
+ });
+ }
+
+}
diff --git a/src/test/java/com/android/tools/r8/naming/b123068484/data/Abs.java b/src/test/java/com/android/tools/r8/naming/b123068484/data/Abs.java
new file mode 100644
index 0000000..d40899a
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/naming/b123068484/data/Abs.java
@@ -0,0 +1,13 @@
+// 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.b123068484.data;
+
+// Hide the strField via class-level access modifier.
+abstract class Abs extends PublicAbs {
+ public final String strField;
+
+ Abs(String x) {
+ strField = x;
+ }
+}
diff --git a/src/test/java/com/android/tools/r8/naming/b123068484/data/Concrete1.java b/src/test/java/com/android/tools/r8/naming/b123068484/data/Concrete1.java
new file mode 100644
index 0000000..02ce3b6
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/naming/b123068484/data/Concrete1.java
@@ -0,0 +1,12 @@
+// 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.b123068484.data;
+
+public class Concrete1 extends Abs {
+ Object otherField;
+ public Concrete1(String x) {
+ super(x);
+ otherField = new Throwable("Concrete1::<init>");
+ }
+}
diff --git a/src/test/java/com/android/tools/r8/naming/b123068484/data/Concrete2.java b/src/test/java/com/android/tools/r8/naming/b123068484/data/Concrete2.java
new file mode 100644
index 0000000..55e8dce
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/naming/b123068484/data/Concrete2.java
@@ -0,0 +1,12 @@
+// 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.b123068484.data;
+
+public class Concrete2 extends Abs {
+ Object otherField;
+ public Concrete2(String x) {
+ super(x);
+ otherField = new Exception("Concrete2::<init>");
+ }
+}
diff --git a/src/test/java/com/android/tools/r8/naming/b123068484/data/PublicAbs.java b/src/test/java/com/android/tools/r8/naming/b123068484/data/PublicAbs.java
new file mode 100644
index 0000000..d7d0e45
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/naming/b123068484/data/PublicAbs.java
@@ -0,0 +1,9 @@
+// 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.b123068484.data;
+
+// Just to abstract Concrete(1|2) and make them visible from different packages.
+public abstract class PublicAbs {
+ static Object foo;
+}
diff --git a/src/test/java/com/android/tools/r8/naming/b123068484/runner/Runner.java b/src/test/java/com/android/tools/r8/naming/b123068484/runner/Runner.java
new file mode 100644
index 0000000..84062c9
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/naming/b123068484/runner/Runner.java
@@ -0,0 +1,28 @@
+// 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.b123068484.runner;
+
+import com.android.tools.r8.naming.b123068484.data.PublicAbs;
+import com.android.tools.r8.naming.b123068484.data.Concrete1;
+import com.android.tools.r8.naming.b123068484.data.Concrete2;
+
+public class Runner {
+ private static int counter = 0;
+ static PublicAbs create(String x) {
+ if (counter++ % 2 == 0) {
+ return new Concrete1(x);
+ } else {
+ return new Concrete2(x);
+ }
+ }
+
+ public static void main(String[] args) {
+ PublicAbs instance = create("Runner");
+ if (instance instanceof Concrete1) {
+ System.out.println(((Concrete1) instance).strField);
+ } else {
+ System.out.println(((Concrete2) instance).strField);
+ }
+ }
+}