Revert "Transform ConstString into DexItemBasedConstString when compared to a class name"
This reverts commit 8b928f1f59deb9e6cb2cf7e1d87fcacabd9077c6.
Reason for revert: Results in NPE for com.android.tools.r8.kotlin.MetadataStripTest > testJstyleRunnable[jdk8 target: JAVA_8]
Change-Id: If1c8f750334e4fa16e41c3963b0eb1f33a1d25bf
diff --git a/src/main/java/com/android/tools/r8/graph/DexItemFactory.java b/src/main/java/com/android/tools/r8/graph/DexItemFactory.java
index 2bd69e5..ab5a2dd 100644
--- a/src/main/java/com/android/tools/r8/graph/DexItemFactory.java
+++ b/src/main/java/com/android/tools/r8/graph/DexItemFactory.java
@@ -270,7 +270,6 @@
public final DexType enumType = createType(enumDescriptor);
public final DexType annotationType = createType(annotationDescriptor);
public final DexType iterableType = createType(iterableDescriptor);
- public final DexType referenceFieldUpdaterType = createType(referenceFieldUpdaterDescriptor);
public final DexType classType = createType(classDescriptor);
public final DexType classLoaderType = createType(classLoaderDescriptor);
diff --git a/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java b/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java
index 9964c7f..6f55b72 100644
--- a/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java
+++ b/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java
@@ -228,7 +228,7 @@
this.outliner = new Outliner(appViewWithLiveness, this);
this.memberValuePropagation =
options.enableValuePropagation ? new MemberValuePropagation(appViewWithLiveness) : null;
- if (options.isMinifying()) {
+ if (!appInfoWithLiveness.identifierNameStrings.isEmpty() && options.isMinifying()) {
this.identifierNameStringMarker = new IdentifierNameStringMarker(appViewWithLiveness);
} else {
this.identifierNameStringMarker = null;
diff --git a/src/main/java/com/android/tools/r8/naming/IdentifierNameStringMarker.java b/src/main/java/com/android/tools/r8/naming/IdentifierNameStringMarker.java
index 4e54043..b2c5675 100644
--- a/src/main/java/com/android/tools/r8/naming/IdentifierNameStringMarker.java
+++ b/src/main/java/com/android/tools/r8/naming/IdentifierNameStringMarker.java
@@ -3,10 +3,8 @@
// BSD-style license that can be found in the LICENSE file.
package com.android.tools.r8.naming;
-import static com.android.tools.r8.naming.IdentifierNameStringUtils.getPositionOfFirstConstString;
import static com.android.tools.r8.naming.IdentifierNameStringUtils.identifyIdentifier;
import static com.android.tools.r8.naming.IdentifierNameStringUtils.inferMemberOrTypeFromNameString;
-import static com.android.tools.r8.naming.IdentifierNameStringUtils.isClassNameComparison;
import static com.android.tools.r8.naming.IdentifierNameStringUtils.isReflectionMethod;
import com.android.tools.r8.graph.AppView;
@@ -24,7 +22,6 @@
import com.android.tools.r8.graph.DexValue.DexValueString;
import com.android.tools.r8.ir.code.BasicBlock;
import com.android.tools.r8.ir.code.BasicBlock.ThrowingInfo;
-import com.android.tools.r8.ir.code.ConstString;
import com.android.tools.r8.ir.code.DexItemBasedConstString;
import com.android.tools.r8.ir.code.FieldInstruction;
import com.android.tools.r8.ir.code.IRCode;
@@ -190,68 +187,53 @@
InstructionListIterator iterator,
InvokeMethod invoke) {
DexMethod invokedMethod = invoke.getInvokedMethod();
- boolean isClassNameComparison = isClassNameComparison(invoke, appView.dexItemFactory());
- if (!identifierNameStrings.containsKey(invokedMethod) && !isClassNameComparison) {
+ if (!identifierNameStrings.containsKey(invokedMethod)) {
return iterator;
}
List<Value> ins = invoke.arguments();
Value[] changes = new Value[ins.size()];
- if (isReflectionMethod(appView.dexItemFactory(), invokedMethod) || isClassNameComparison) {
+ if (isReflectionMethod(appView.dexItemFactory(), invokedMethod)) {
DexReference itemBasedString = identifyIdentifier(invoke, appView);
if (itemBasedString == null) {
DexType context = method.method.holder;
warnUndeterminedIdentifierIfNecessary(invokedMethod, context, invoke, null);
return iterator;
}
- int identifierPosition = getIdentifierPositionInArguments(invoke);
- Value in = invoke.arguments().get(identifierPosition);
+ DexType returnType = invoke.getReturnType();
+ boolean isClassForName = returnType.descriptor == appView.dexItemFactory().classDescriptor;
+ boolean isReferenceFieldUpdater =
+ returnType.descriptor == appView.dexItemFactory().referenceFieldUpdaterDescriptor;
+ int positionOfIdentifier = isClassForName ? 0 : (isReferenceFieldUpdater ? 2 : 1);
+ Value in = invoke.arguments().get(positionOfIdentifier);
+ // Move the cursor back to $invoke
+ assert iterator.peekPrevious() == invoke;
+ iterator.previous();
// Prepare $decoupled just before $invoke
Value newIn = code.createValue(in.getTypeLattice(), in.getLocalInfo());
DexItemBasedConstString decoupled =
new DexItemBasedConstString(newIn, itemBasedString, throwingInfo);
- changes[identifierPosition] = newIn;
-
- if (in.numberOfAllUsers() == 1) {
- // Simply replace the existing ConstString by a DexItemBasedConstString. No need to check
- // for catch handlers, as this is replacing one throwing instruction with another.
- ConstString constString = in.definition.asConstString();
- if (constString.getBlock() == invoke.getBlock()) {
- iterator.previousUntil(instruction -> instruction == constString);
- Instruction current = iterator.next();
- assert current == constString;
- iterator.replaceCurrentInstruction(decoupled);
- iterator.nextUntil(instruction -> instruction == invoke);
- } else {
- in.definition.replace(decoupled);
- }
- } else {
- decoupled.setPosition(invoke.getPosition());
-
- // Move the cursor back to $invoke
- assert iterator.peekPrevious() == invoke;
+ decoupled.setPosition(invoke.getPosition());
+ changes[positionOfIdentifier] = newIn;
+ // If the current block has catch handler, split into two blocks.
+ // Because const-string we're about to add is also a throwing instr, we need to split
+ // before adding it.
+ BasicBlock block = invoke.getBlock();
+ BasicBlock blockWithInvoke = block.hasCatchHandlers() ? iterator.split(code, blocks) : block;
+ if (blockWithInvoke != block) {
+ // If we split, add const-string at the end of the currently visiting block.
+ iterator = block.listIterator(block.getInstructions().size());
iterator.previous();
- // If the current block has catch handler, split into two blocks.
- // Because const-string we're about to add is also a throwing instr, we need to split
- // before adding it.
- BasicBlock block = invoke.getBlock();
- BasicBlock blockWithInvoke =
- block.hasCatchHandlers() ? iterator.split(code, blocks) : block;
- if (blockWithInvoke != block) {
- // If we split, add const-string at the end of the currently visiting block.
- iterator = block.listIterator(block.getInstructions().size());
- iterator.previous();
- iterator.add(decoupled);
- // Restore the cursor and block.
- iterator = blockWithInvoke.listIterator();
- assert iterator.peekNext() == invoke;
- iterator.next();
- } else {
- // Otherwise, just add it to the current block at the position of the iterator.
- iterator.add(decoupled);
- // Restore the cursor.
- assert iterator.peekNext() == invoke;
- iterator.next();
- }
+ iterator.add(decoupled);
+ // Restore the cursor and block.
+ iterator = blockWithInvoke.listIterator();
+ assert iterator.peekNext() == invoke;
+ iterator.next();
+ } else {
+ // Otherwise, just add it to the current block at the position of the iterator.
+ iterator.add(decoupled);
+ // Restore the cursor.
+ assert iterator.peekNext() == invoke;
+ iterator.next();
}
} else {
// For general invoke. Multiple arguments can be string literals to be renamed.
@@ -315,29 +297,6 @@
return iterator;
}
- private int getIdentifierPositionInArguments(InvokeMethod invoke) {
- DexType returnType = invoke.getReturnType();
- if (isClassNameComparison(invoke, appView.dexItemFactory())) {
- return getPositionOfFirstConstString(invoke);
- }
-
- boolean isClassForName = returnType == appView.dexItemFactory().classType;
- if (isClassForName) {
- assert invoke.getInvokedMethod() == appView.dexItemFactory().classMethods.forName;
- return 0;
- }
-
- boolean isReferenceFieldUpdater =
- returnType == appView.dexItemFactory().referenceFieldUpdaterType;
- if (isReferenceFieldUpdater) {
- assert invoke.getInvokedMethod()
- == appView.dexItemFactory().atomicFieldUpdaterMethods.referenceUpdater;
- return 2;
- }
-
- return 1;
- }
-
private void warnUndeterminedIdentifierIfNecessary(
DexReference member, DexType originHolder, Instruction instruction, DexString original) {
assert member.isDexField() || member.isDexMethod();
diff --git a/src/main/java/com/android/tools/r8/naming/IdentifierNameStringUtils.java b/src/main/java/com/android/tools/r8/naming/IdentifierNameStringUtils.java
index 37ac36b..3a759c4 100644
--- a/src/main/java/com/android/tools/r8/naming/IdentifierNameStringUtils.java
+++ b/src/main/java/com/android/tools/r8/naming/IdentifierNameStringUtils.java
@@ -24,7 +24,6 @@
import com.android.tools.r8.ir.code.InstructionListIterator;
import com.android.tools.r8.ir.code.InvokeMethod;
import com.android.tools.r8.ir.code.InvokeStatic;
-import com.android.tools.r8.ir.code.InvokeVirtual;
import com.android.tools.r8.ir.code.NewArrayEmpty;
import com.android.tools.r8.ir.code.Value;
import com.google.common.collect.Sets;
@@ -139,30 +138,6 @@
}
/**
- * Returns true if the given invoke instruction is calling `boolean java.lang.String.equals(
- * java.lang.String)`, and one of the arguments is defined by an invoke-instruction that calls
- * `java.lang.String java.lang.Class.getName()`.
- */
- public static boolean isClassNameComparison(InvokeMethod invoke, DexItemFactory dexItemFactory) {
- return invoke.isInvokeVirtual()
- && isClassNameComparison(invoke.asInvokeVirtual(), dexItemFactory);
- }
-
- public static boolean isClassNameComparison(InvokeVirtual invoke, DexItemFactory dexItemFactory) {
- return invoke.getInvokedMethod() == dexItemFactory.stringMethods.equals
- && (isClassNameValue(invoke.getReceiver(), dexItemFactory)
- || isClassNameValue(invoke.inValues().get(1), dexItemFactory));
- }
-
- private static boolean isClassNameValue(Value value, DexItemFactory dexItemFactory) {
- Value root = value.getAliasedValue();
- return !root.isPhi()
- && root.definition.isInvokeVirtual()
- && root.definition.asInvokeVirtual().getInvokedMethod()
- == dexItemFactory.classMethods.getName;
- }
-
- /**
* Returns a {@link DexReference} if one of the arguments to the invoke instruction is a constant
* string that corresponds to either a class or member name (i.e., an identifier).
*
@@ -182,17 +157,6 @@
}
}
- if (invoke.isInvokeVirtual()) {
- InvokeVirtual invokeVirtual = invoke.asInvokeVirtual();
- if (isClassNameComparison(invokeVirtual, definitions.dexItemFactory())) {
- int argumentIndex = getPositionOfFirstConstString(invokeVirtual);
- if (argumentIndex >= 0) {
- return inferTypeFromConstStringValue(
- definitions, invokeVirtual.inValues().get(argumentIndex));
- }
- }
- }
-
// All the other cases receive either (Class, String) or (Class, String, Class[]) as ins.
if (ins.size() == 1) {
return null;
@@ -246,17 +210,6 @@
return null;
}
- static int getPositionOfFirstConstString(Instruction instruction) {
- List<Value> inValues = instruction.inValues();
- for (int i = 0; i < inValues.size(); i++) {
- Value value = inValues.get(i);
- if (value.getAliasedValue().isConstString()) {
- return i;
- }
- }
- return -1;
- }
-
static DexReference inferMemberOrTypeFromNameString(
DexDefinitionSupplier definitions, DexString dexString) {
// "fully.qualified.ClassName.fieldOrMethodName"
@@ -278,14 +231,6 @@
return null;
}
- public static DexType inferTypeFromConstStringValue(
- DexDefinitionSupplier definitions, Value value) {
- assert value.definition != null;
- assert value.getAliasedValue().isConstString();
- return inferTypeFromNameString(
- definitions, value.getAliasedValue().definition.asConstString().getValue());
- }
-
private static DexReference inferMemberFromNameString(
DexDefinitionSupplier definitions, DexString dexString) {
String identifier = dexString.toString();
diff --git a/src/test/java/com/android/tools/r8/internal/proto/Proto2ShrinkingTest.java b/src/test/java/com/android/tools/r8/internal/proto/Proto2ShrinkingTest.java
index 1f27829..485d90a 100644
--- a/src/test/java/com/android/tools/r8/internal/proto/Proto2ShrinkingTest.java
+++ b/src/test/java/com/android/tools/r8/internal/proto/Proto2ShrinkingTest.java
@@ -33,6 +33,8 @@
.addProgramFiles(PROTO2_EXAMPLES_JAR, PROTO2_PROTO_JAR, PROTOBUF_LITE_JAR)
.addKeepMainRule("proto2.TestClass")
.addKeepRules(
+ // TODO(b/112437944): Fix -identifiernamestring support.
+ "-keepnames class * extends com.google.protobuf.GeneratedMessageLite",
// TODO(b/112437944): Use dex item based const strings for proto schema definitions.
"-keepclassmembernames class * extends com.google.protobuf.GeneratedMessageLite {",
" <fields>;",
@@ -44,7 +46,6 @@
"}",
allowAccessModification ? "-allowaccessmodification" : "")
.addKeepRuleFiles(PROTOBUF_LITE_PROGUARD_RULES)
- .addOptionsModification(options -> options.enableStringSwitchConversion = true)
.setMinApi(parameters.getRuntime())
.compile()
.run(parameters.getRuntime(), "proto2.TestClass")
diff --git a/src/test/java/com/android/tools/r8/naming/identifiernamestring/ClassNameComparisonTest.java b/src/test/java/com/android/tools/r8/naming/identifiernamestring/ClassNameComparisonTest.java
deleted file mode 100644
index 5aaf063..0000000
--- a/src/test/java/com/android/tools/r8/naming/identifiernamestring/ClassNameComparisonTest.java
+++ /dev/null
@@ -1,70 +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.naming.identifiernamestring;
-
-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 org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
-import org.junit.runners.Parameterized.Parameters;
-
-@RunWith(Parameterized.class)
-public class ClassNameComparisonTest extends TestBase {
-
- private final TestParameters parameters;
-
- @Parameters(name = "{0}")
- public static TestParametersCollection data() {
- return getTestParameters().withAllRuntimes().build();
- }
-
- public ClassNameComparisonTest(TestParameters parameters) {
- this.parameters = parameters;
- }
-
- @Test
- public void test() throws Exception {
- testForR8(parameters.getBackend())
- .addInnerClasses(ClassNameComparisonTest.class)
- .addKeepMainRule(TestClass.class)
- .setMinApi(parameters.getRuntime())
- .compile()
- .run(parameters.getRuntime(), TestClass.class)
- .assertSuccessWithOutputLines("Hello!", "Hello " + B.class.getName() + "!");
- }
-
- @Test
- public void testCorrectnessOfNames() {
- assertEquals(A.class.getName(), TestClass.NAME_A);
- assertEquals(B.class.getName(), TestClass.NAME_B);
- }
-
- static class TestClass {
-
- private static final String NAME_A =
- "com.android.tools.r8.naming.identifiernamestring.ClassNameComparisonTest$A";
-
- private static final String NAME_B =
- "com.android.tools.r8.naming.identifiernamestring.ClassNameComparisonTest$B";
-
- public static void main(String[] args) {
- if (A.class.getName().equals(NAME_A)) {
- System.out.println("Hello!");
- }
- String name = NAME_B;
- if (B.class.getName().equals(name)) {
- System.out.println("Hello " + name + "!");
- }
- }
- }
-
- static class A {}
-
- static class B {}
-}
