Relax dead condition of static-put.
by implementing instructionMayHaveSideEffects:
in addition to general side-effects of field instructions,
we need to check if the target field is read.
Bug: 129530569, 130561746
Change-Id: I606055d0b57a78f34d6deb84d947ea284f0a031a
diff --git a/src/main/java/com/android/tools/r8/ir/code/InstanceGet.java b/src/main/java/com/android/tools/r8/ir/code/InstanceGet.java
index 6bb754f..e633ab2 100644
--- a/src/main/java/com/android/tools/r8/ir/code/InstanceGet.java
+++ b/src/main/java/com/android/tools/r8/ir/code/InstanceGet.java
@@ -114,10 +114,10 @@
// * IncompatibleClassChangeError (instance-* instruction for static fields)
// * IllegalAccessError (not visible from the access context)
// * NullPointerException (null receiver)
- boolean canBeDeadCode = !instructionMayHaveSideEffects(appView, code.method.method.holder);
- assert appView.enableWholeProgramOptimizations() || !canBeDeadCode
+ boolean haveSideEffects = instructionMayHaveSideEffects(appView, code.method.method.holder);
+ assert appView.enableWholeProgramOptimizations() || haveSideEffects
: "Expected instance-get instruction to have side effects in D8";
- return canBeDeadCode;
+ return !haveSideEffects;
}
@Override
diff --git a/src/main/java/com/android/tools/r8/ir/code/StaticGet.java b/src/main/java/com/android/tools/r8/ir/code/StaticGet.java
index 6c48db2..ba106cb 100644
--- a/src/main/java/com/android/tools/r8/ir/code/StaticGet.java
+++ b/src/main/java/com/android/tools/r8/ir/code/StaticGet.java
@@ -107,10 +107,10 @@
// * IncompatibleClassChangeError (static-* instruction for instance fields)
// * IllegalAccessError (not visible from the access context)
// * side-effects in <clinit>
- boolean canBeDeadCode = !instructionMayHaveSideEffects(appView, code.method.method.holder);
- assert appView.enableWholeProgramOptimizations() || !canBeDeadCode
+ boolean haveSideEffects = instructionMayHaveSideEffects(appView, code.method.method.holder);
+ assert appView.enableWholeProgramOptimizations() || haveSideEffects
: "Expected static-get instruction to have side effects in D8";
- return canBeDeadCode;
+ return !haveSideEffects;
}
@Override
diff --git a/src/main/java/com/android/tools/r8/ir/code/StaticPut.java b/src/main/java/com/android/tools/r8/ir/code/StaticPut.java
index a87692d..ea103dd 100644
--- a/src/main/java/com/android/tools/r8/ir/code/StaticPut.java
+++ b/src/main/java/com/android/tools/r8/ir/code/StaticPut.java
@@ -26,6 +26,8 @@
import com.android.tools.r8.ir.optimize.Inliner.ConstraintWithTarget;
import com.android.tools.r8.ir.optimize.InliningConstraints;
import com.android.tools.r8.ir.regalloc.RegisterAllocator;
+import com.android.tools.r8.shaking.AppInfoWithLiveness;
+import com.android.tools.r8.shaking.ProguardMemberRule;
import org.objectweb.asm.Opcodes;
public class StaticPut extends FieldInstruction {
@@ -89,6 +91,51 @@
}
@Override
+ public boolean instructionMayHaveSideEffects(
+ AppView<? extends AppInfo> appView, DexType context) {
+ if (appView.appInfo().hasLiveness()) {
+ AppInfoWithLiveness appInfoWithLiveness = appView.appInfo().withLiveness();
+ // MemberValuePropagation will replace the field read only if the target field has bound
+ // -assumevalues rule whose return value is *single*.
+ //
+ // Note that, in principle, class initializer of the field's holder may have side effects.
+ // However, with -assumevalues, we assume that the developer wants to remove field accesses.
+ ProguardMemberRule rule = appInfoWithLiveness.assumedValues.get(getField());
+ if (rule != null && rule.getReturnValue().isSingleValue()) {
+ return false;
+ }
+
+ if (instructionInstanceCanThrow(appView, context).isThrowing()) {
+ return true;
+ }
+
+ if (appInfoWithLiveness.isFieldRead(getField())) {
+ return true;
+ }
+
+ return false;
+ }
+
+ // In D8, we always have to assume that the field can be read, and thus have side effects.
+ assert instructionInstanceCanThrow(appView, context).isThrowing();
+ return true;
+ }
+
+ @Override
+ public boolean canBeDeadCode(AppView<? extends AppInfo> appView, IRCode code) {
+ // static-put can be dead as long as it cannot have any of the following:
+ // * NoSuchFieldError (resolution failure)
+ // * IncompatibleClassChangeError (static-* instruction for instance fields)
+ // * IllegalAccessError (not visible from the access context)
+ // * side-effects in <clinit>
+ // * not read _globally_
+ boolean haveSideEffects = instructionMayHaveSideEffects(appView, code.method.method.holder);
+ assert appView.enableWholeProgramOptimizations() || haveSideEffects
+ : "Expected static-put instruction to have side effects in D8";
+ return !haveSideEffects;
+ }
+
+ @Override
public int maxInValueRegister() {
return Constants.U8BIT_MAX;
}
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 7d5e6bf..8c27bd7 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
@@ -375,7 +375,7 @@
if (current.isInvokeMethod()) {
rewriteInvokeMethodWithConstantValues(
code, callingContext, affectedValues, blocks, iterator, current.asInvokeMethod());
- } else if (current.isInstancePut() || current.isStaticPut()) {
+ } else if (current.isInstancePut()) {
rewritePutWithConstantValues(iterator, current.asFieldInstruction(), callingContext);
} else if (current.isStaticGet()) {
rewriteStaticGetWithConstantValues(
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/MemberValuePropagationTest.java b/src/test/java/com/android/tools/r8/ir/optimize/MemberValuePropagationTest.java
index 640a20d..c6402bd 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/MemberValuePropagationTest.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/MemberValuePropagationTest.java
@@ -6,32 +6,21 @@
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
-import com.android.tools.r8.CompilationFailedException;
-import com.android.tools.r8.R8Command;
import com.android.tools.r8.TestBase;
-import com.android.tools.r8.TestBase.Backend;
import com.android.tools.r8.ToolHelper;
import com.android.tools.r8.utils.FileUtils;
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 java.io.File;
-import java.io.IOException;
import java.nio.file.Path;
import java.nio.file.Paths;
-import java.util.Arrays;
-import java.util.Collections;
import java.util.Iterator;
-import java.util.List;
-import java.util.stream.Collectors;
-import org.junit.Rule;
import org.junit.Test;
-import org.junit.rules.TemporaryFolder;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
@RunWith(Parameterized.class)
-public class MemberValuePropagationTest {
+public class MemberValuePropagationTest extends TestBase {
private static final String PACKAGE_NAME = "write_only_field";
private static final String QUALIFIED_CLASS_NAME = PACKAGE_NAME + ".WriteOnlyCls";
private static final Path EXAMPLE_JAR =
@@ -54,13 +43,9 @@
this.backend = backend;
}
- @Rule
- public TemporaryFolder temp = ToolHelper.getTemporaryFolderForTest();
-
@Test
public void testWriteOnlyField_putObject_gone() throws Exception {
- List<Path> processedApp = runR8(EXAMPLE_KEEP);
- CodeInspector inspector = new CodeInspector(processedApp);
+ CodeInspector inspector = runR8(EXAMPLE_KEEP);
ClassSubject clazz = inspector.clazz(QUALIFIED_CLASS_NAME);
clazz.forAllMethods(
methodSubject -> {
@@ -74,10 +59,8 @@
@Test
public void testWriteOnlyField_dontoptimize() throws Exception {
- List<Path> processedApp = runR8(DONT_OPTIMIZE);
- CodeInspector inspector = new CodeInspector(processedApp);
+ CodeInspector inspector = runR8(DONT_OPTIMIZE);
ClassSubject clazz = inspector.clazz(QUALIFIED_CLASS_NAME);
- assert backend == Backend.DEX || backend == Backend.CF;
clazz.forAllMethods(
methodSubject -> {
Iterator<InstructionSubject> iterator = methodSubject.iterateInstructions();
@@ -88,7 +71,8 @@
++numPuts;
}
}
- assertEquals(1, numPuts);
+ // dead code removal is not part of -dontoptimize.
+ assertEquals(0, numPuts);
}
if (methodSubject.isInstanceInitializer()) {
int numPuts = 0;
@@ -102,29 +86,13 @@
});
}
- private List<Path> runR8(Path proguardConfig) throws IOException, CompilationFailedException {
- Path outputDir = temp.newFolder().toPath();
- assert backend == Backend.DEX || backend == Backend.CF;
- ToolHelper.runR8(
- R8Command.builder()
- .setOutput(outputDir, TestBase.outputMode(backend))
- .addProgramFiles(EXAMPLE_JAR)
- .addLibraryFiles(TestBase.runtimeJar(backend))
- .addProguardConfigurationFiles(proguardConfig)
- .setDisableMinification(true)
- .build(),
- o -> {
- o.enableClassInlining = false;
- });
-
- return backend == Backend.DEX
- ? Collections.singletonList(outputDir.resolve(Paths.get("classes.dex")))
- : Arrays.stream(
- outputDir
- .resolve(PACKAGE_NAME)
- .toFile()
- .listFiles(f -> f.toString().endsWith(".class")))
- .map(File::toPath)
- .collect(Collectors.toList());
+ private CodeInspector runR8(Path proguardConfig) throws Exception {
+ return testForR8(backend)
+ .addProgramFiles(EXAMPLE_JAR)
+ .addKeepRuleFiles(proguardConfig)
+ .noMinification()
+ .addOptionsModification(o -> o.enableClassInlining = false)
+ .compile()
+ .inspector();
}
}
diff --git a/src/test/java/com/android/tools/r8/naming/IdentifierNameStringMarkerTest.java b/src/test/java/com/android/tools/r8/naming/IdentifierNameStringMarkerTest.java
index dcea0e6..f02109d 100644
--- a/src/test/java/com/android/tools/r8/naming/IdentifierNameStringMarkerTest.java
+++ b/src/test/java/com/android/tools/r8/naming/IdentifierNameStringMarkerTest.java
@@ -162,6 +162,7 @@
List<String> pgConfigs = ImmutableList.of(
"-identifiernamestring class " + CLASS_NAME + " { static java.lang.String sClassName; }",
"-keep class " + CLASS_NAME,
+ "-keepclassmembers,allowobfuscation class " + CLASS_NAME + " { static <fields>; }",
"-dontoptimize");
CodeInspector inspector = getInspectorAfterRunR8(builder, pgConfigs);
@@ -193,6 +194,7 @@
List<String> pgConfigs = ImmutableList.of(
"-identifiernamestring class " + CLASS_NAME + " { static java.lang.String sClassName; }",
"-keep class " + CLASS_NAME,
+ "-keepclassmembers,allowobfuscation class " + CLASS_NAME + " { static <fields>; }",
"-dontoptimize");
CodeInspector inspector = getInspectorAfterRunR8(builder, pgConfigs);
@@ -230,6 +232,7 @@
List<String> pgConfigs = ImmutableList.of(
"-identifiernamestring class " + CLASS_NAME + " { static java.lang.String sClassName; }",
"-keep class " + CLASS_NAME,
+ "-keepclassmembers,allowobfuscation class " + CLASS_NAME + " { static <fields>; }",
"-keep,allowobfuscation class " + BOO,
"-dontoptimize");
CodeInspector inspector = getInspectorAfterRunR8(builder, pgConfigs);
diff --git a/src/test/java/com/android/tools/r8/shaking/FieldTypeTest.java b/src/test/java/com/android/tools/r8/shaking/FieldTypeTest.java
index 55f5370..105bf6b 100644
--- a/src/test/java/com/android/tools/r8/shaking/FieldTypeTest.java
+++ b/src/test/java/com/android/tools/r8/shaking/FieldTypeTest.java
@@ -19,6 +19,7 @@
import com.android.tools.r8.naming.MemberNaming.FieldSignature;
import com.android.tools.r8.naming.MemberNaming.MethodSignature;
import com.android.tools.r8.utils.AndroidApp;
+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.google.common.collect.ImmutableList;
@@ -76,7 +77,8 @@
"new " + impl1.name,
"dup",
"invokespecial " + impl1.name + "/<init>()V",
- // Unused, i.e., not read, field, yet still remained in the output.
+ // Unused, i.e., not read, field, hence removable.
+ // Needs an extra keep rule to keep this (to reproduce broken type hierarchy)
"putstatic " + client.name + "/" + obj1.name + " " + itf1.getDescriptor(),
"new " + impl2.name,
"dup",
@@ -105,11 +107,12 @@
);
final String mainClassName = mainClass.name;
- String proguardConfig =
- keepMainProguardConfiguration(mainClass.name, false, false)
- // AGP default is to not turn optimizations on, which disables MemberValuePropagation,
- // resulting in the problematic putstatic being remained.
- + "-dontoptimize\n";
+ String proguardConfig = StringUtils.lines(
+ keepMainProguardConfiguration(mainClass.name, false, false),
+ // AGP default is to not turn optimizations on
+ "-dontoptimize",
+ "-keepclassmembers class " + client.name + " { static ** " + obj1.name + "; }"
+ );
// Run input program on java.
Path outputDirectory = temp.newFolder().toPath();
diff --git a/src/test/java/com/android/tools/r8/shaking/assumevalues/DeadFieldAfterAssumevaluesTest.java b/src/test/java/com/android/tools/r8/shaking/assumevalues/DeadFieldAfterAssumevaluesTest.java
index 6dbd97e..b0211bc 100644
--- a/src/test/java/com/android/tools/r8/shaking/assumevalues/DeadFieldAfterAssumevaluesTest.java
+++ b/src/test/java/com/android/tools/r8/shaking/assumevalues/DeadFieldAfterAssumevaluesTest.java
@@ -4,6 +4,7 @@
package com.android.tools.r8.shaking.assumevalues;
import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
+import static org.hamcrest.CoreMatchers.not;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.Assert.assertTrue;
@@ -53,7 +54,7 @@
}
@Test
- public void b130561746() throws Exception {
+ public void testR8() throws Exception {
testForR8(parameters.getBackend())
.addProgramClasses(MAIN)
.addKeepMainRule(MAIN)
@@ -75,11 +76,10 @@
.noneMatch(i -> i.isIf() || i.isIfEqz() || i.isIfNez()));
FieldSubject hasR8 = main.uniqueFieldWithName("HAS_R8");
- // TODO(b/130561746): can be removed.
- assertThat(hasR8, isPresent());
+ assertThat(hasR8, not(isPresent()));
MethodSubject clinit = main.clinit();
- // TODO(b/130561746): can be removed if the above static field is gone.
+ // TODO(b/130561746): need to model that Boolean.parseBoolean doesn't have side-effects.
assertThat(clinit, isPresent());
}
}
