Revert "Relax the dead condition of field reads." This reverts commit 3fc9942045d6dc19d734b8ddc12703c1727df753. Reason for revert: test status failures on 6.0.1 Bug: 124039115, 70295684 Change-Id: I66403a0fd6958ef8dc729a440186636532d06d72
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 14d7ffb..40756ae 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
@@ -4,8 +4,6 @@ package com.android.tools.r8.ir.code; -import static com.android.tools.r8.optimize.MemberRebindingAnalysis.isVisibleFromOriginalContext; - import com.android.tools.r8.cf.LoadStoreHelper; import com.android.tools.r8.cf.TypeVerificationHelper; import com.android.tools.r8.cf.code.CfFieldInstruction; @@ -21,7 +19,6 @@ import com.android.tools.r8.graph.AppInfo; import com.android.tools.r8.graph.AppInfoWithSubtyping; import com.android.tools.r8.graph.AppView; -import com.android.tools.r8.graph.DexEncodedField; import com.android.tools.r8.graph.DexField; import com.android.tools.r8.graph.DexItemFactory; import com.android.tools.r8.graph.DexType; @@ -101,24 +98,6 @@ } @Override - public boolean canBeDeadCode(AppInfo appInfo, IRCode code) { - // instance-get can be dead code as long as it cannot have any of the following: - // * NoSuchFieldError (resolution failure) - // * IllegalAccessError (not visible from the access context) - // * NullPointerException (null receiver). - // TODO(b/123857022): Should be possible to use definitionFor(). - DexEncodedField resolvedField = appInfo.resolveFieldOn(getField().getHolder(), getField()); - if (resolvedField == null) { - return false; - } - if (code == null - || !isVisibleFromOriginalContext(appInfo, code.method.method.getHolder(), resolvedField)) { - return false; - } - return object().getTypeLattice().nullability().isDefinitelyNotNull(); - } - - @Override public int maxInValueRegister() { return Constants.U4BIT_MAX; }
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 80e74f3..b79d076 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
@@ -3,8 +3,6 @@ // BSD-style license that can be found in the LICENSE file. package com.android.tools.r8.ir.code; -import static com.android.tools.r8.optimize.MemberRebindingAnalysis.isVisibleFromOriginalContext; - import com.android.tools.r8.cf.LoadStoreHelper; import com.android.tools.r8.cf.TypeVerificationHelper; import com.android.tools.r8.cf.code.CfFieldInstruction; @@ -20,7 +18,6 @@ import com.android.tools.r8.graph.AppInfo; import com.android.tools.r8.graph.AppInfoWithSubtyping; import com.android.tools.r8.graph.AppView; -import com.android.tools.r8.graph.DexEncodedField; import com.android.tools.r8.graph.DexField; import com.android.tools.r8.graph.DexType; import com.android.tools.r8.ir.analysis.ClassInitializationAnalysis; @@ -94,25 +91,6 @@ } @Override - public boolean canBeDeadCode(AppInfo appInfo, IRCode code) { - // static-get can be dead as long as it cannot have any of the following: - // * NoSuchFieldError (resolution failure) - // * IllegalAccessError (not visible from the access context) - // * side-effects in <clinit> - // TODO(b/123857022): Should be possible to use definitionFor(). - DexEncodedField resolvedField = appInfo.resolveFieldOn(getField().getHolder(), getField()); - if (resolvedField == null) { - return false; - } - if (code == null - || !isVisibleFromOriginalContext(appInfo, code.method.method.getHolder(), resolvedField)) { - return false; - } - return appInfo.hasSubtyping() - && !getField().getHolder().classInitializationMayHaveSideEffects(appInfo); - } - - @Override public int maxInValueRegister() { return Constants.U8BIT_MAX; }
diff --git a/src/main/java/com/android/tools/r8/optimize/MemberRebindingAnalysis.java b/src/main/java/com/android/tools/r8/optimize/MemberRebindingAnalysis.java index 60aec09..608f915 100644 --- a/src/main/java/com/android/tools/r8/optimize/MemberRebindingAnalysis.java +++ b/src/main/java/com/android/tools/r8/optimize/MemberRebindingAnalysis.java
@@ -3,7 +3,6 @@ // BSD-style license that can be found in the LICENSE file. package com.android.tools.r8.optimize; -import com.android.tools.r8.graph.AppInfo; import com.android.tools.r8.graph.AppView; import com.android.tools.r8.graph.DexClass; import com.android.tools.r8.graph.DexEncodedField; @@ -292,15 +291,14 @@ Set<DexEncodedMethod> contexts = fieldsWithContexts.get(field); if (target != null && target.field != field && contexts.stream().allMatch(context -> - isVisibleFromOriginalContext(appInfo, context.method.getHolder(), target))) { + isVisibleFromOriginalContext(context.method.getHolder(), target))) { builder.map(field, lense.lookupField(validTargetFor(target.field, field, lookupTargetOnClass))); } } } - public static boolean isVisibleFromOriginalContext( - AppInfo appInfo, DexType context, DexEncodedField field) { + private boolean isVisibleFromOriginalContext(DexType context, DexEncodedField field) { DexType holderType = field.field.getHolder(); DexClass holder = appInfo.definitionFor(holderType); if (holder == null) {
diff --git a/src/test/java/com/android/tools/r8/jasmin/MemberResolutionTest.java b/src/test/java/com/android/tools/r8/jasmin/MemberResolutionTest.java index ea8ed0b..33f28fc 100644 --- a/src/test/java/com/android/tools/r8/jasmin/MemberResolutionTest.java +++ b/src/test/java/com/android/tools/r8/jasmin/MemberResolutionTest.java
@@ -462,7 +462,7 @@ main.addMainMethod( ".limit stack 2", ".limit locals 1", - " getstatic Empty/aField I", + " getstatic Empty/aMethod I", " return"); ensureRuntimeException(builder, NoSuchFieldError.class);
diff --git a/src/test/java/com/android/tools/r8/shaking/FieldReadsJasminTest.java b/src/test/java/com/android/tools/r8/shaking/FieldReadsJasminTest.java deleted file mode 100644 index ac1fd90..0000000 --- a/src/test/java/com/android/tools/r8/shaking/FieldReadsJasminTest.java +++ /dev/null
@@ -1,180 +0,0 @@ -// 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.shaking; - -import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertThat; -import static org.junit.Assert.assertTrue; -import static org.junit.Assert.fail; - -import com.android.tools.r8.jasmin.JasminBuilder; -import com.android.tools.r8.jasmin.JasminBuilder.ClassBuilder; -import com.android.tools.r8.jasmin.JasminTestBase; -import com.android.tools.r8.naming.MemberNaming.MethodSignature; -import com.android.tools.r8.utils.codeinspector.ClassSubject; -import com.android.tools.r8.utils.codeinspector.InstructionSubject; -import com.android.tools.r8.utils.codeinspector.MethodSubject; -import com.google.common.collect.ImmutableList; -import java.util.Iterator; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.junit.runners.Parameterized; - -@RunWith(Parameterized.class) -public class FieldReadsJasminTest extends JasminTestBase { - private static final String CLS = "Empty"; - private static final String MAIN = "Main"; - private final Backend backend; - - @Parameterized.Parameters(name = "Backend: {0}") - public static Object[] data() { - return Backend.values(); - } - - public FieldReadsJasminTest(Backend backend) { - this.backend = backend; - } - - @Test - public void testInstanceGet_nonNullReceiver() throws Exception { - JasminBuilder builder = new JasminBuilder(); - - ClassBuilder empty = builder.addClass(CLS); - empty.addField("protected", "aField", "I", null); - - ClassBuilder main = builder.addClass(MAIN); - main.addMainMethod( - ".limit stack 2", - ".limit locals 1", - " new Empty", - " dup", - " invokespecial Empty/<init>()V", - " getfield Empty/aField I", - " return"); - - ensureNoFields(builder, empty); - } - - @Test - public void testStaticGet_noSideEffect() throws Exception { - JasminBuilder builder = new JasminBuilder(); - - ClassBuilder empty = builder.addClass(CLS); - empty.addStaticField("sField", "I"); - - ClassBuilder main = builder.addClass(MAIN); - main.addMainMethod( - ".limit stack 2", - ".limit locals 1", - " getstatic Empty/sField I", - " return"); - - ensureNoFields(builder, empty); - } - - private void ensureNoFields(JasminBuilder app, ClassBuilder clazz) throws Exception { - testForR8(backend) - .addProgramClassFileData(app.buildClasses()) - .addKeepRules("-keep class * { <methods>; }") - .compile() - .inspect(inspector -> { - ClassSubject classSubject = inspector.clazz(clazz.name); - assertThat(classSubject, isPresent()); - classSubject.forAllFields(foundFieldSubject -> { - fail("Expect not to see any fields."); - }); - }); - } - - @Test - public void testInstanceGet_nullableReceiver() throws Exception { - JasminBuilder builder = new JasminBuilder(); - - ClassBuilder empty = builder.addClass(CLS); - empty.addDefaultConstructor(); - empty.addField("protected", "aField", "I", null); - MethodSignature foo = empty.addStaticMethod("foo", ImmutableList.of("L" + CLS + ";"), "V", - ".limit stack 2", - ".limit locals 1", - " aload 0", - " getfield Empty/aField I", - " return"); - - ClassBuilder main = builder.addClass(MAIN); - main.addMainMethod( - ".limit stack 2", - ".limit locals 1", - " aconst_null", - " invokestatic Empty/foo(L" + CLS + ";)V", - " return"); - - ensureFieldExists(builder, empty, foo, "aField"); - } - - @Test - public void testStaticGet_nonTrivialClinit() throws Exception { - JasminBuilder builder = new JasminBuilder(); - - ClassBuilder empty = builder.addClass(CLS); - empty.addDefaultConstructor(); - empty.addStaticField("sField", "I"); - empty.addClassInitializer( - ".limit stack 3", - ".limit locals 1", - " getstatic java/lang/System/out Ljava/io/PrintStream;", - " ldc \"hello\"", - " invokevirtual java/io/PrintStream/print(Ljava/lang/String;)V", - " return"); - - ClassBuilder main = builder.addClass(MAIN); - MethodSignature mainMethod = main.addMainMethod( - ".limit stack 2", - ".limit locals 1", - " getstatic Empty/sField I", - " return"); - - ensureFieldExists(builder, main, mainMethod, "sField"); - } - - @Test - public void testStaticGet_allocation() throws Exception { - JasminBuilder builder = new JasminBuilder(); - - ClassBuilder empty = builder.addClass(CLS); - empty.addDefaultConstructor(); - empty.addStaticField("sField", "Ljava/lang/String;", "\"8\""); - - ClassBuilder main = builder.addClass(MAIN); - MethodSignature mainMethod = main.addMainMethod( - ".limit stack 2", - ".limit locals 1", - " getstatic Empty/sField Ljava/lang/String;", - " return"); - - ensureFieldExists(builder, main, mainMethod, "sField"); - } - - private void ensureFieldExists( - JasminBuilder app, ClassBuilder clazz, MethodSignature method, String fieldName) - throws Exception { - testForR8(backend) - .addProgramClassFileData(app.buildClasses()) - .addKeepRules("-keep class * { <methods>; }") - .compile() - .inspect(inspector -> { - ClassSubject classSubject = inspector.clazz(clazz.name); - assertThat(classSubject, isPresent()); - MethodSubject methodSubject = classSubject.uniqueMethodWithName(method.name); - assertThat(methodSubject, isPresent()); - Iterator<InstructionSubject> it = - methodSubject.iterateInstructions(InstructionSubject::isFieldAccess); - assertTrue(it.hasNext()); - assertEquals( - inspector.clazz(CLS).uniqueFieldWithName(fieldName).getFinalName(), - it.next().getField().name.toString()); - }); - } - -}