Prioritize noSideEffects in side-effect analysis of invocations.
Bug: 130804193, 130561746
Change-Id: I2469b4f0988d86f54a2e11042ee6af57f67a023d
diff --git a/src/main/java/com/android/tools/r8/ir/code/InvokeDirect.java b/src/main/java/com/android/tools/r8/ir/code/InvokeDirect.java
index c1f12a1..bddfc6a 100644
--- a/src/main/java/com/android/tools/r8/ir/code/InvokeDirect.java
+++ b/src/main/java/com/android/tools/r8/ir/code/InvokeDirect.java
@@ -183,23 +183,18 @@
assert false : "Expected to be able to find the enclosing class of a method definition";
return true;
}
-
boolean targetMayHaveSideEffects;
- if (clazz.isProgramClass()) {
- targetMayHaveSideEffects =
- target.getOptimizationInfo().mayHaveSideEffects()
- && !appViewWithLiveness.appInfo().noSideEffects.containsKey(target.method);
+ if (appViewWithLiveness.appInfo().noSideEffects.containsKey(target.method)) {
+ targetMayHaveSideEffects = false;
+ } else if (clazz.isProgramClass()) {
+ targetMayHaveSideEffects = target.getOptimizationInfo().mayHaveSideEffects();
} else {
+ assert clazz.isLibraryClass();
targetMayHaveSideEffects =
!appView.dexItemFactory().libraryMethodsWithoutSideEffects.contains(target.method);
}
- if (targetMayHaveSideEffects) {
- return true;
- }
-
- // Success, the instruction does not have any side effects.
- return false;
+ return targetMayHaveSideEffects;
}
return true;
diff --git a/src/main/java/com/android/tools/r8/ir/code/InvokeStatic.java b/src/main/java/com/android/tools/r8/ir/code/InvokeStatic.java
index ed1ba571..e659c39 100644
--- a/src/main/java/com/android/tools/r8/ir/code/InvokeStatic.java
+++ b/src/main/java/com/android/tools/r8/ir/code/InvokeStatic.java
@@ -164,18 +164,21 @@
}
// Verify that the target method does not have side-effects.
- boolean targetMayHaveSideEffects =
- target.getOptimizationInfo().mayHaveSideEffects()
- && !appViewWithLiveness.appInfo().noSideEffects.containsKey(target.method);
- if (targetMayHaveSideEffects) {
- return true;
+ boolean targetMayHaveSideEffects;
+ if (appViewWithLiveness.appInfo().noSideEffects.containsKey(target.method)) {
+ targetMayHaveSideEffects = false;
+ } else {
+ targetMayHaveSideEffects =
+ target.getOptimizationInfo().mayHaveSideEffects()
+ // Verify that calling the target method won't lead to class initialization.
+ || target.method.holder.classInitializationMayHaveSideEffects(
+ appView.appInfo(),
+ // Types that are a super type of `context` are guaranteed to be initialized
+ // already.
+ type -> appView.isSubtype(context, type).isTrue());
}
- // Verify that calling the target method won't lead to class initialization.
- return target.method.holder.classInitializationMayHaveSideEffects(
- appView.appInfo(),
- // Types that are a super type of `context` are guaranteed to be initialized already.
- type -> appView.isSubtype(context, type).isTrue());
+ return targetMayHaveSideEffects;
}
return true;
diff --git a/src/main/java/com/android/tools/r8/ir/code/InvokeVirtual.java b/src/main/java/com/android/tools/r8/ir/code/InvokeVirtual.java
index 04453b9..acfb6da 100644
--- a/src/main/java/com/android/tools/r8/ir/code/InvokeVirtual.java
+++ b/src/main/java/com/android/tools/r8/ir/code/InvokeVirtual.java
@@ -121,6 +121,10 @@
@Override
public boolean instructionMayHaveSideEffects(AppView<?> appView, DexType context) {
+ if (!appView.enableWholeProgramOptimizations()) {
+ return true;
+ }
+
if (appView.options().debug) {
return true;
}
@@ -136,6 +140,25 @@
return false;
}
+ // Find the target and check if the invoke may have side effects.
+ if (appView.appInfo().hasLiveness()) {
+ AppView<AppInfoWithLiveness> appViewWithLiveness = appView.withLiveness();
+ DexEncodedMethod target = lookupSingleTarget(appViewWithLiveness, context);
+ if (target == null) {
+ return true;
+ }
+
+ // Verify that the target method does not have side-effects.
+ boolean targetMayHaveSideEffects;
+ if (appViewWithLiveness.appInfo().noSideEffects.containsKey(target.method)) {
+ targetMayHaveSideEffects = false;
+ } else {
+ targetMayHaveSideEffects = target.getOptimizationInfo().mayHaveSideEffects();
+ }
+
+ return targetMayHaveSideEffects;
+ }
+
return true;
}
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 b1e36a0..f5d6d47 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
@@ -206,7 +206,9 @@
}
// TODO(b/130804193): search for all call targets and apply -assumenosideeffects if one of
// call targets has a matching rule?
- DexEncodedMethod definition = current.lookupSingleTarget(appView, callingContext);
+ // TODO(b/130804193): using refined receiver type for InvokeMethodWithReceiver?
+ DexEncodedMethod definition =
+ appView.appInfo().lookupSingleTarget(current.getType(), invokedMethod, callingContext);
ProguardMemberRuleLookup lookup = lookupMemberRule(definition);
boolean invokeReplaced = false;
if (lookup != null) {
diff --git a/src/test/examples/annotationremoval/keep-rules-keep-innerannotation.txt b/src/test/examples/annotationremoval/keep-rules-keep-innerannotation.txt
index ce89fb2..e2f4953 100644
--- a/src/test/examples/annotationremoval/keep-rules-keep-innerannotation.txt
+++ b/src/test/examples/annotationremoval/keep-rules-keep-innerannotation.txt
@@ -14,5 +14,5 @@
-keepattributes InnerClasses,EnclosingMethod
-# allow access modification to enable minification
--allowaccessmodification
+# Examples are too simple and side-effects free, and thus can be easily inlined/pruned.
+-dontoptimize
diff --git a/src/test/examples/annotationremoval/keep-rules.txt b/src/test/examples/annotationremoval/keep-rules.txt
index b5aa38d..c450322 100644
--- a/src/test/examples/annotationremoval/keep-rules.txt
+++ b/src/test/examples/annotationremoval/keep-rules.txt
@@ -8,5 +8,5 @@
public static void main(...);
}
-# allow access modification to enable minification
--allowaccessmodification
+# Examples are too simple and side-effects free, and thus can be easily inlined/pruned.
+-dontoptimize
diff --git a/src/test/examples/naming001/keep-rules-105.txt b/src/test/examples/naming001/keep-rules-105.txt
index 634edb6..011596b 100644
--- a/src/test/examples/naming001/keep-rules-105.txt
+++ b/src/test/examples/naming001/keep-rules-105.txt
@@ -9,4 +9,8 @@
public void keep();
}
+-keep,allowobfuscation class naming001.E {
+ <methods>;
+}
+
-applymapping mapping-105.txt
diff --git a/src/test/java/com/android/tools/r8/accessrelaxation/InvokeTypeConversionTest.java b/src/test/java/com/android/tools/r8/accessrelaxation/InvokeTypeConversionTest.java
index 787568b..5c04f51 100644
--- a/src/test/java/com/android/tools/r8/accessrelaxation/InvokeTypeConversionTest.java
+++ b/src/test/java/com/android/tools/r8/accessrelaxation/InvokeTypeConversionTest.java
@@ -5,9 +5,9 @@
import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
import static org.hamcrest.CoreMatchers.containsString;
+import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
-import static org.junit.Assert.assertThat;
import static org.junit.Assert.assertTrue;
import com.android.tools.r8.R8Command;
@@ -66,7 +66,8 @@
AndroidApp app = buildApplication(builder);
List<String> pgConfigs = ImmutableList.of(
keepMainProguardConfiguration(CLASS_NAME),
- "-printmapping",
+ // We're testing lense-based invocation type conversions.
+ "-dontoptimize",
"-dontobfuscate",
"-allowaccessmodification");
R8Command.Builder command = ToolHelper.prepareR8CommandBuilder(app);
diff --git a/src/test/java/com/android/tools/r8/checkdiscarded/CheckDiscardedTest.java b/src/test/java/com/android/tools/r8/checkdiscarded/CheckDiscardedTest.java
index 7c09fa2..6593110 100644
--- a/src/test/java/com/android/tools/r8/checkdiscarded/CheckDiscardedTest.java
+++ b/src/test/java/com/android/tools/r8/checkdiscarded/CheckDiscardedTest.java
@@ -38,9 +38,9 @@
private String checkDiscardRule(boolean member, Class annotation) {
if (member) {
- return "-checkdiscard class * { @" + annotation.getCanonicalName() + " *; }";
+ return "-checkdiscard class * { @" + annotation.getName() + " *; }";
} else {
- return "-checkdiscard @" + annotation.getCanonicalName() + " class *";
+ return "-checkdiscard @" + annotation.getName() + " class *";
}
}
diff --git a/src/test/java/com/android/tools/r8/checkdiscarded/testclasses/UsedClass.java b/src/test/java/com/android/tools/r8/checkdiscarded/testclasses/UsedClass.java
index 253c114..ee1cbca 100644
--- a/src/test/java/com/android/tools/r8/checkdiscarded/testclasses/UsedClass.java
+++ b/src/test/java/com/android/tools/r8/checkdiscarded/testclasses/UsedClass.java
@@ -8,6 +8,7 @@
@WillStay
public String hello() {
+ System.out.println("side-effect!");
return "hello";
}
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/string/NameThenLengthTest.java b/src/test/java/com/android/tools/r8/ir/optimize/string/NameThenLengthTest.java
index 8f227e3..b7ccc01 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/string/NameThenLengthTest.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/string/NameThenLengthTest.java
@@ -154,7 +154,7 @@
.run(parameters.getRuntime(), MAIN)
.assertSuccessWithOutput(JAVA_OUTPUT);
// TODO(b/125303292): NAME_LENGTH is still not computed at compile time.
- test(result, 1, 0, 0, 1);
+ test(result, 1, 1, 0, 1);
}
@Test
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/unusedarguments/CollisionWithLibraryMethodsTest.java b/src/test/java/com/android/tools/r8/ir/optimize/unusedarguments/CollisionWithLibraryMethodsTest.java
index 031a8b3..5c89be9 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/unusedarguments/CollisionWithLibraryMethodsTest.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/unusedarguments/CollisionWithLibraryMethodsTest.java
@@ -84,7 +84,8 @@
@NeverInline
public String toString(Object unused) {
- return "Hello world!";
+ System.out.print("Hello ");
+ return "world!";
}
}
}
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/unusedarguments/UnusedArgumentRemovalWithOverridingTest.java b/src/test/java/com/android/tools/r8/ir/optimize/unusedarguments/UnusedArgumentRemovalWithOverridingTest.java
index 5b87e3a..a3f1b9b 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/unusedarguments/UnusedArgumentRemovalWithOverridingTest.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/unusedarguments/UnusedArgumentRemovalWithOverridingTest.java
@@ -90,7 +90,8 @@
@NeverInline
@Override
public String greeting(String unused) {
- return "Hello world!";
+ System.out.print("Hello ");
+ return "world!";
}
}
}
diff --git a/src/test/java/com/android/tools/r8/naming/applymapping/sourcelibrary/ApplyMappingTest.java b/src/test/java/com/android/tools/r8/naming/applymapping/sourcelibrary/ApplyMappingTest.java
index b5e57b5..f64424e 100644
--- a/src/test/java/com/android/tools/r8/naming/applymapping/sourcelibrary/ApplyMappingTest.java
+++ b/src/test/java/com/android/tools/r8/naming/applymapping/sourcelibrary/ApplyMappingTest.java
@@ -5,9 +5,9 @@
package com.android.tools.r8.naming.applymapping.sourcelibrary;
import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
+import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
-import static org.junit.Assert.assertThat;
import com.android.tools.r8.CompilationFailedException;
import com.android.tools.r8.R8Command;
diff --git a/src/test/java/com/android/tools/r8/regress/b69825683/Regress69825683Test.java b/src/test/java/com/android/tools/r8/regress/b69825683/Regress69825683Test.java
index c02e401..624b2e4 100644
--- a/src/test/java/com/android/tools/r8/regress/b69825683/Regress69825683Test.java
+++ b/src/test/java/com/android/tools/r8/regress/b69825683/Regress69825683Test.java
@@ -8,6 +8,8 @@
import static org.junit.Assert.assertEquals;
import com.android.tools.r8.TestBase;
+import com.android.tools.r8.TestParameters;
+import com.android.tools.r8.TestParametersCollection;
import com.android.tools.r8.ToolHelper;
import com.android.tools.r8.utils.codeinspector.CodeInspector;
import com.android.tools.r8.utils.codeinspector.FoundClassSubject;
@@ -18,15 +20,15 @@
@RunWith(Parameterized.class)
public class Regress69825683Test extends TestBase {
- private Backend backend;
+ private final TestParameters parameters;
- @Parameterized.Parameters(name = "Backend: {0}")
- public static Backend[] data() {
- return ToolHelper.getBackends();
+ @Parameterized.Parameters(name = "{0}")
+ public static TestParametersCollection data() {
+ return getTestParameters().withAllRuntimes().build();
}
- public Regress69825683Test(Backend backend) {
- this.backend = backend;
+ public Regress69825683Test(TestParameters parameters) {
+ this.parameters = parameters;
}
@Test
@@ -39,17 +41,18 @@
innerName = innerName.substring(0, index) + "$" + innerName.substring(index + 1);
CodeInspector inspector =
- testForR8(backend)
+ testForR8(parameters.getBackend())
.addProgramFiles(ToolHelper.getClassFilesForTestPackage(outer.getPackage()))
.addKeepMainRule(outer)
.enableSideEffectAnnotations()
.addKeepRules(
- "-assumemayhavesideeffects class " + inner.getTypeName() + " {",
+ "-assumemayhavesideeffects class " + inner.getName() + " {",
" synthetic void <init>(...);",
"}")
.addOptionsModification(options -> options.enableClassInlining = false)
.noMinification()
- .run(outer)
+ .setMinApi(parameters.getRuntime())
+ .run(parameters.getRuntime(), outer)
// Run code to check that the constructor with synthetic class as argument is present.
.assertSuccessWithOutputThatMatches(startsWith(innerName))
.inspector();
@@ -69,15 +72,21 @@
public void innerConstructsOuter() throws Exception {
Class<?> clazz = com.android.tools.r8.regress.b69825683.innerconstructsouter.Outer.class;
CodeInspector inspector =
- testForR8(backend)
+ testForR8(parameters.getBackend())
.addProgramFiles(ToolHelper.getClassFilesForTestPackage(clazz.getPackage()))
.addKeepMainRule(clazz)
.enableInliningAnnotations()
+ .enableSideEffectAnnotations()
+ .addKeepRules(
+ "-assumemayhavesideeffects class " + clazz.getName() + " {",
+ " void <init>(...);",
+ "}")
.noMinification()
.addOptionsModification(o -> o.enableClassInlining = false)
+ .setMinApi(parameters.getRuntime())
// Run code to check that the constructor with synthetic class as argument is present.
- .run(clazz)
- .assertSuccessWithOutputThatMatches(startsWith(clazz.getTypeName()))
+ .run(parameters.getRuntime(), clazz)
+ .assertSuccessWithOutputThatMatches(startsWith(clazz.getName()))
.inspector();
List<FoundClassSubject> classes = inspector.allClasses();
diff --git a/src/test/java/com/android/tools/r8/rewrite/staticvalues/StaticValuesTest.java b/src/test/java/com/android/tools/r8/rewrite/staticvalues/StaticValuesTest.java
index 7937f4e..f0270f1 100644
--- a/src/test/java/com/android/tools/r8/rewrite/staticvalues/StaticValuesTest.java
+++ b/src/test/java/com/android/tools/r8/rewrite/staticvalues/StaticValuesTest.java
@@ -3,16 +3,14 @@
// BSD-style license that can be found in the LICENSE file.
package com.android.tools.r8.rewrite.staticvalues;
+import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
+import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
import com.android.tools.r8.ToolHelper.ProcessResult;
import com.android.tools.r8.code.IfEqz;
-import com.android.tools.r8.code.Instruction;
import com.android.tools.r8.code.SgetBoolean;
-import com.android.tools.r8.code.Sput;
-import com.android.tools.r8.code.SputObject;
import com.android.tools.r8.graph.DexCode;
import com.android.tools.r8.graph.DexValue;
import com.android.tools.r8.graph.DexValue.DexValueBoolean;
@@ -105,7 +103,7 @@
assertTrue(inspector.clazz("Test").field("boolean", "booleanField").hasExplicitStaticValue());
value = inspector.clazz("Test").field("boolean", "booleanField").getStaticValue();
assertTrue(value instanceof DexValueBoolean);
- assertEquals(true, ((DexValueBoolean) value).getValue());
+ assertTrue(((DexValueBoolean) value).getValue());
assertTrue(inspector.clazz("Test").field("byte", "byteField").hasExplicitStaticValue());
value = inspector.clazz("Test").field("byte", "byteField").getStaticValue();
@@ -387,7 +385,7 @@
AndroidApp processedApplication = processApplication(originalApplication);
CodeInspector inspector = new CodeInspector(processedApplication);
- assertTrue(inspector.clazz("Test").clinit().isPresent());
+ assertThat(inspector.clazz("Test").clinit(), isPresent());
assertTrue(inspector.clazz("Test").field("int", "intField").hasExplicitStaticValue());
DexValue value = inspector.clazz("Test").field("int", "intField").getStaticValue();
@@ -466,7 +464,7 @@
AndroidApp processedApplication = processApplication(originalApplication);
CodeInspector inspector = new CodeInspector(processedApplication);
- assertTrue(inspector.clazz(className).isPresent());
+ assertThat(inspector.clazz(className), isPresent());
// Test is running without tree-shaking, so the empty <clinit> is not removed.
assertTrue(
inspector.clazz(className).clinit().getMethod().getCode().asDexCode().isEmptyVoidMethod());
@@ -515,8 +513,8 @@
AndroidApp processedApplication = processApplication(originalApplication);
CodeInspector inspector = new CodeInspector(processedApplication);
- assertTrue(inspector.clazz(className).isPresent());
- assertTrue(inspector.clazz(className).clinit().isPresent());
+ assertThat(inspector.clazz(className), isPresent());
+ assertThat(inspector.clazz(className).clinit(), isPresent());
String result = runArt(processedApplication, className);
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 b0211bc..81d33fc 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
@@ -39,6 +39,9 @@
private static final String RULES = StringUtils.lines(
"-assumevalues class **.TestClass {",
" static boolean HAS_R8 return true;",
+ "}",
+ "-assumenosideeffects class java.lang.Boolean {",
+ " static boolean parseBoolean(java.lang.String);",
"}"
);
@@ -79,7 +82,6 @@
assertThat(hasR8, not(isPresent()));
MethodSubject clinit = main.clinit();
- // TODO(b/130561746): need to model that Boolean.parseBoolean doesn't have side-effects.
- assertThat(clinit, isPresent());
+ assertThat(clinit, not(isPresent()));
}
}
