Add tests and TODOs for rebinding of fields
Bug: 123857022
Change-Id: If0935cf3204c0dec77afa824eacefef99b1bcf41
diff --git a/src/main/java/com/android/tools/r8/graph/DexEncodedField.java b/src/main/java/com/android/tools/r8/graph/DexEncodedField.java
index 42cc0ce..0efc380 100644
--- a/src/main/java/com/android/tools/r8/graph/DexEncodedField.java
+++ b/src/main/java/com/android/tools/r8/graph/DexEncodedField.java
@@ -7,6 +7,7 @@
import com.android.tools.r8.dex.MixedSectionCollection;
import com.android.tools.r8.ir.code.Instruction;
import com.android.tools.r8.ir.code.Value;
+import com.android.tools.r8.shaking.Enqueuer.AppInfoWithLiveness;
public class DexEncodedField extends KeyedDexItem<DexField> {
@@ -109,22 +110,23 @@
}
// Returns a const instructions if this field is a compile time final const.
- public Instruction valueAsConstInstruction(AppInfo appInfo, Value dest) {
+ public Instruction valueAsConstInstruction(AppInfoWithLiveness appInfo, Value dest) {
// The only way to figure out whether the DexValue contains the final value
// is ensure the value is not the default or check <clinit> is not present.
- if (accessFlags.isStatic() && accessFlags.isPublic() && accessFlags.isFinal()) {
- DexClass clazz = appInfo.definitionFor(field.getHolder());
+ boolean isEffectivelyFinal =
+ (accessFlags.isFinal() || !appInfo.fieldsWritten.contains(field))
+ && !appInfo.isPinned(field);
+ if (!isEffectivelyFinal) {
+ return null;
+ }
+ if (accessFlags.isStatic()) {
+ DexClass clazz = appInfo.definitionFor(field.clazz);
assert clazz != null : "Class for the field must be present";
return getStaticValue().asConstInstruction(clazz.hasClassInitializer(), dest);
}
return null;
}
- public DexEncodedField toRenamedField(DexString name, DexItemFactory dexItemFactory) {
- return new DexEncodedField(dexItemFactory.createField(field.clazz, field.type, name),
- accessFlags, annotations, staticValue);
- }
-
public DexEncodedField toTypeSubstitutedField(DexField field) {
if (this.field == field) {
return this;
diff --git a/src/main/java/com/android/tools/r8/graph/DexType.java b/src/main/java/com/android/tools/r8/graph/DexType.java
index 0c48ea9..3e07a4f 100644
--- a/src/main/java/com/android/tools/r8/graph/DexType.java
+++ b/src/main/java/com/android/tools/r8/graph/DexType.java
@@ -13,6 +13,7 @@
import com.android.tools.r8.naming.NamingLens;
import com.android.tools.r8.utils.DescriptorUtils;
import com.android.tools.r8.utils.InternalOptions.OutlineOptions;
+import com.google.common.base.Predicates;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.common.collect.Sets;
@@ -120,6 +121,10 @@
return implementedInterfaces(appInfo).contains(appInfo.dexItemFactory.serializableType);
}
+ public boolean classInitializationMayHaveSideEffects(AppInfo appInfo) {
+ return classInitializationMayHaveSideEffects(appInfo, Predicates.alwaysFalse());
+ }
+
public boolean classInitializationMayHaveSideEffects(AppInfo appInfo, Predicate<DexType> ignore) {
DexClass clazz = appInfo.definitionFor(this);
return clazz == null || clazz.classInitializationMayHaveSideEffects(appInfo, ignore);
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/MemberValuePropagation.java b/src/main/java/com/android/tools/r8/ir/optimize/MemberValuePropagation.java
index 3deeb63..95d2275 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/MemberValuePropagation.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/MemberValuePropagation.java
@@ -221,6 +221,8 @@
InstructionListIterator iterator,
StaticGet current) {
DexField field = current.getField();
+
+ // TODO(b/123857022): Should be able to use definitionFor().
DexEncodedField target = appInfo.lookupStaticTarget(field.getHolder(), field);
if (target == null) {
return;
@@ -254,7 +256,7 @@
DexClass holderDefinition = appInfo.definitionFor(field.getHolder());
if (holderDefinition != null
&& holderDefinition.accessFlags.isFinal()
- && !appInfo.canTriggerStaticInitializer(field.getHolder(), true)) {
+ && !field.getHolder().classInitializationMayHaveSideEffects(appInfo)) {
Value outValue = current.dest();
DexEncodedMethod classInitializer = holderDefinition.getClassInitializer();
if (classInitializer != null && !isProcessedConcurrently.test(classInitializer)) {
@@ -277,12 +279,14 @@
private void rewritePutWithConstantValues(
InstructionIterator iterator, FieldInstruction current) {
- DexField field = current.asFieldInstruction().getField();
+ DexField field = current.getField();
+ // TODO(b/123857022): Should be possible to use definitionFor().
DexEncodedField target =
current.isInstancePut()
? appInfo.lookupInstanceTarget(field.getHolder(), field)
: appInfo.lookupStaticTarget(field.getHolder(), field);
- if (target != null && !isFieldRead(target)) {
+ // TODO(b/123857022): Should be possible to use `!isFieldRead(field)`.
+ if (target != null && !isFieldRead(target.field)) {
// Remove writes to dead (i.e. never read) fields.
iterator.removeOrReplaceByDebugLocalRead();
}
@@ -290,8 +294,8 @@
/**
* Replace invoke targets and field accesses with constant values where possible.
- * <p>
- * Also assigns value ranges to values where possible.
+ *
+ * <p>Also assigns value ranges to values where possible.
*/
public void rewriteWithConstantValues(
IRCode code, DexType callingContext, Predicate<DexEncodedMethod> isProcessedConcurrently) {
@@ -324,12 +328,16 @@
assert code.isConsistentSSA();
}
- private boolean isFieldRead(DexEncodedField field) {
- if (appInfo.fieldsRead.contains(field.field) || appInfo.isPinned(field.field)) {
- return true;
- }
- // For library classes we don't know whether a field is read.
- DexClass holder = appInfo.definitionFor(field.field.clazz);
+ private boolean isFieldRead(DexField field) {
+ return appInfo.fieldsRead.contains(field)
+ // TODO(b/121354886): Pinned fields should be in `fieldsRead`.
+ || appInfo.isPinned(field)
+ // For library classes we don't know whether a field is read.
+ || isLibraryField(field);
+ }
+
+ private boolean isLibraryField(DexField field) {
+ DexClass holder = appInfo.definitionFor(field.clazz);
return holder == null || holder.isLibraryClass();
}
}
diff --git a/src/test/java/com/android/tools/r8/memberrebinding/IllegalFieldRebindingTest.java b/src/test/java/com/android/tools/r8/memberrebinding/IllegalFieldRebindingTest.java
new file mode 100644
index 0000000..cc35cbd
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/memberrebinding/IllegalFieldRebindingTest.java
@@ -0,0 +1,125 @@
+// 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.memberrebinding;
+
+import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertThat;
+
+import com.android.tools.r8.TestBase;
+import com.android.tools.r8.memberrebinding.testclasses.IllegalFieldRebindingTestClasses;
+import com.android.tools.r8.memberrebinding.testclasses.IllegalFieldRebindingTestClasses.B;
+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.InstructionSubject;
+import com.android.tools.r8.utils.codeinspector.MethodSubject;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameters;
+
+@RunWith(Parameterized.class)
+public class IllegalFieldRebindingTest extends TestBase {
+
+ private final Backend backend;
+
+ @Parameters(name = "Backend: {0}")
+ public static Backend[] data() {
+ return Backend.values();
+ }
+
+ public IllegalFieldRebindingTest(Backend backend) {
+ this.backend = backend;
+ }
+
+ @Test
+ public void test() throws Exception {
+ String expectedOutput = StringUtils.lines("42");
+
+ if (backend == Backend.CF) {
+ testForJvm().addTestClasspath().run(TestClass.class).assertSuccessWithOutput(expectedOutput);
+ }
+
+ CodeInspector inspector =
+ testForR8(Backend.DEX)
+ .addInnerClasses(IllegalFieldRebindingTest.class)
+ .addInnerClasses(IllegalFieldRebindingTestClasses.class)
+ .addKeepMainRule(TestClass.class)
+ .enableMergeAnnotations()
+ .run(TestClass.class)
+ .assertSuccessWithOutput(expectedOutput)
+ .inspector();
+
+ ClassSubject classSubject = inspector.clazz(TestClass.class);
+ assertThat(classSubject, isPresent());
+
+ MethodSubject methodSubject = classSubject.mainMethod();
+ assertThat(methodSubject, isPresent());
+
+ // Verify that the static-put instruction has not been removed.
+ assertEquals(
+ 1, methodSubject.streamInstructions().filter(InstructionSubject::isStaticPut).count());
+ }
+
+ @Test
+ public void otherTest() throws Exception {
+ String expectedOutput = StringUtils.lines("0");
+
+ if (backend == Backend.CF) {
+ testForJvm()
+ .addTestClasspath()
+ .run(OtherTestClass.class)
+ .assertSuccessWithOutput(expectedOutput);
+ }
+
+ CodeInspector inspector =
+ testForR8(Backend.DEX)
+ .addInnerClasses(IllegalFieldRebindingTest.class)
+ .addInnerClasses(IllegalFieldRebindingTestClasses.class)
+ .addKeepMainRule(OtherTestClass.class)
+ .enableMergeAnnotations()
+ .run(OtherTestClass.class)
+ .assertSuccessWithOutput(expectedOutput)
+ .inspector();
+
+ // The static-get instruction in OtherTestClass.main() should have been replaced by the
+ // constant 0, which makes the remaining classes unreachable.
+ assertEquals(1, inspector.allClasses().size());
+
+ ClassSubject classSubject = inspector.clazz(OtherTestClass.class);
+ assertThat(classSubject, isPresent());
+
+ MethodSubject methodSubject = classSubject.mainMethod();
+ assertThat(methodSubject, isPresent());
+
+ // Verify that a constant 0 is being loaded in main().
+ assertEquals(
+ 1,
+ methodSubject
+ .streamInstructions()
+ .filter(instruction -> instruction.isConstNumber(0))
+ .count());
+ }
+
+ static class TestClass {
+
+ public static void main(String[] args) {
+ // Cannot be member rebound because A is inaccessible to this package. However, the
+ // instruction cannot be removed as the field `A.f` is actually read.
+ B.f = 42;
+ B.print();
+ }
+ }
+
+ static class OtherTestClass {
+
+ public static void main(String[] args) {
+ // Cannot be member rebound because A is inaccessible to this package.
+ // However, the instruction should still be replaced by the constant 0.
+ System.out.println(B.f);
+ }
+ }
+}
diff --git a/src/test/java/com/android/tools/r8/memberrebinding/testclasses/IllegalFieldRebindingTestClasses.java b/src/test/java/com/android/tools/r8/memberrebinding/testclasses/IllegalFieldRebindingTestClasses.java
new file mode 100644
index 0000000..64b4d1f
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/memberrebinding/testclasses/IllegalFieldRebindingTestClasses.java
@@ -0,0 +1,22 @@
+// 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.memberrebinding.testclasses;
+
+import com.android.tools.r8.NeverMerge;
+
+public class IllegalFieldRebindingTestClasses {
+
+ @NeverMerge
+ static class A {
+ public static int f;
+ }
+
+ public static class B extends A {
+
+ public static void print() {
+ System.out.println(A.f);
+ }
+ }
+}
