Merge "CF backend: Don't read stack frames in with ASM"
diff --git a/src/main/java/com/android/tools/r8/graph/DexMethod.java b/src/main/java/com/android/tools/r8/graph/DexMethod.java
index c0c2e27..9c3e9a5 100644
--- a/src/main/java/com/android/tools/r8/graph/DexMethod.java
+++ b/src/main/java/com/android/tools/r8/graph/DexMethod.java
@@ -71,6 +71,14 @@
return false;
}
+ /**
+ * Returns true if the other method has the same name and prototype (including signature and
+ * return type), false otherwise.
+ */
+ public boolean hasSameProtoAndName(DexMethod other) {
+ return name == other.name && proto == other.proto;
+ }
+
@Override
public int compareTo(DexMethod other) {
return sortedCompareTo(other.getSortedIndex());
diff --git a/src/main/java/com/android/tools/r8/ir/analysis/constant/SparseConditionalConstantPropagation.java b/src/main/java/com/android/tools/r8/ir/analysis/constant/SparseConditionalConstantPropagation.java
index 3379864..6c9f075 100644
--- a/src/main/java/com/android/tools/r8/ir/analysis/constant/SparseConditionalConstantPropagation.java
+++ b/src/main/java/com/android/tools/r8/ir/analysis/constant/SparseConditionalConstantPropagation.java
@@ -129,16 +129,24 @@
BasicBlock phiBlock = phi.getBlock();
int phiBlockNumber = phiBlock.getNumber();
LatticeElement element = Top.getInstance();
- for (int i = 0; i < phiBlock.getPredecessors().size(); i++) {
- BasicBlock predecessor = phiBlock.getPredecessors().get(i);
+ List<BasicBlock> predecessors = phiBlock.getPredecessors();
+ int size = predecessors.size();
+ for (int i = 0; i < size; i++) {
+ BasicBlock predecessor = predecessors.get(i);
if (isExecutableEdge(predecessor.getNumber(), phiBlockNumber)) {
element = element.meet(getLatticeElement(phi.getOperand(i)));
+ // bottom lattice can no longer be changed, thus no need to continue
+ if (element.isBottom()) {
+ break;
+ }
}
}
- LatticeElement currentPhiElement = getLatticeElement(phi);
- if (!element.isTop() && currentPhiElement.meet(element) != currentPhiElement) {
- ssaEdges.add(phi);
- setLatticeElement(phi, element);
+ if (!element.isTop()) {
+ LatticeElement currentPhiElement = getLatticeElement(phi);
+ if (currentPhiElement.meet(element) != currentPhiElement) {
+ ssaEdges.add(phi);
+ setLatticeElement(phi, element);
+ }
}
}
diff --git a/src/main/java/com/android/tools/r8/ir/conversion/DexSourceCode.java b/src/main/java/com/android/tools/r8/ir/conversion/DexSourceCode.java
index 228f594..9c6bb86 100644
--- a/src/main/java/com/android/tools/r8/ir/conversion/DexSourceCode.java
+++ b/src/main/java/com/android/tools/r8/ir/conversion/DexSourceCode.java
@@ -172,7 +172,9 @@
}
@Override
- public void buildInstruction(IRBuilder builder, int instructionIndex) throws ApiLevelException {
+ public void buildInstruction(
+ IRBuilder builder, int instructionIndex, boolean firstBlockInstruction)
+ throws ApiLevelException {
updateCurrentCatchHandlers(instructionIndex);
updateDebugPosition(instructionIndex, builder);
currentDexInstruction = code.instructions[instructionIndex];
diff --git a/src/main/java/com/android/tools/r8/ir/conversion/IRBuilder.java b/src/main/java/com/android/tools/r8/ir/conversion/IRBuilder.java
index 5bea64e..13d66c2 100644
--- a/src/main/java/com/android/tools/r8/ir/conversion/IRBuilder.java
+++ b/src/main/java/com/android/tools/r8/ir/conversion/IRBuilder.java
@@ -504,7 +504,7 @@
addToWorklist(info.block, i);
break;
}
- source.buildInstruction(this, i);
+ source.buildInstruction(this, i, i == item.firstInstructionIndex);
}
}
}
diff --git a/src/main/java/com/android/tools/r8/ir/conversion/JarSourceCode.java b/src/main/java/com/android/tools/r8/ir/conversion/JarSourceCode.java
index 4dd5656..b5dfdbb 100644
--- a/src/main/java/com/android/tools/r8/ir/conversion/JarSourceCode.java
+++ b/src/main/java/com/android/tools/r8/ir/conversion/JarSourceCode.java
@@ -488,7 +488,9 @@
}
@Override
- public void buildInstruction(IRBuilder builder, int instructionIndex) throws ApiLevelException {
+ public void buildInstruction(
+ IRBuilder builder, int instructionIndex, boolean firstBlockInstruction)
+ throws ApiLevelException {
if (instructionIndex == EXCEPTIONAL_SYNC_EXIT_OFFSET) {
buildExceptionalPostlude(builder);
return;
@@ -498,8 +500,17 @@
assert verifyExceptionEdgesAreRecorded(insn);
// If a new block is starting here, we restore the computed JarState.
- if (builder.getCFG().containsKey(instructionIndex) || instructionIndex == 0) {
+ // current position needs to be compute only for the first instruction of a block, thereafter
+ // current position will be updated by LineNumberNode into this block.
+ if (firstBlockInstruction || instructionIndex == 0) {
state.restoreState(instructionIndex);
+ // Don't include line changes when processing a label. Doing so will end up emitting local
+ // writes after the line has changed and thus causing locals to become visible too late.
+ currentPosition =
+ getDebugPositionAtOffset(
+ ((instructionIndex > 0) && (insn instanceof LabelNode))
+ ? instructionIndex - 1
+ : instructionIndex);
}
String preInstructionState;
@@ -507,14 +518,6 @@
preInstructionState = state.toString();
}
- // Don't include line changes when processing a label. Doing so will end up emitting local
- // writes after the line has changed and thus causing locals to become visible too late.
- currentPosition =
- getDebugPositionAtOffset(
- ((instructionIndex > 0) && (insn instanceof LabelNode))
- ? instructionIndex - 1
- : instructionIndex);
-
build(insn, builder);
if (Log.ENABLED && !(insn instanceof LineNumberNode)) {
diff --git a/src/main/java/com/android/tools/r8/ir/conversion/SourceCode.java b/src/main/java/com/android/tools/r8/ir/conversion/SourceCode.java
index 13d1551..be1c670 100644
--- a/src/main/java/com/android/tools/r8/ir/conversion/SourceCode.java
+++ b/src/main/java/com/android/tools/r8/ir/conversion/SourceCode.java
@@ -47,7 +47,9 @@
// Delegates for IR building.
void buildPrelude(IRBuilder builder);
- void buildInstruction(IRBuilder builder, int instructionIndex) throws ApiLevelException;
+
+ void buildInstruction(IRBuilder builder, int instructionIndex, boolean firstBlockInstruction)
+ throws ApiLevelException;
void buildPostlude(IRBuilder builder);
// Helper to resolve switch payloads and build switch instructions (dex code only).
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/Outliner.java b/src/main/java/com/android/tools/r8/ir/optimize/Outliner.java
index dba6183..126b127 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/Outliner.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/Outliner.java
@@ -859,7 +859,8 @@
}
@Override
- public void buildInstruction(IRBuilder builder, int instructionIndex) {
+ public void buildInstruction(
+ IRBuilder builder, int instructionIndex, boolean firstBlockInstruction) {
if (instructionIndex == outline.templateInstructions.size()) {
if (outline.returnType == dexItemFactory.voidType) {
builder.addReturn();
diff --git a/src/main/java/com/android/tools/r8/ir/synthetic/SyntheticSourceCode.java b/src/main/java/com/android/tools/r8/ir/synthetic/SyntheticSourceCode.java
index 5d03dfd..5a6cc2d 100644
--- a/src/main/java/com/android/tools/r8/ir/synthetic/SyntheticSourceCode.java
+++ b/src/main/java/com/android/tools/r8/ir/synthetic/SyntheticSourceCode.java
@@ -188,7 +188,8 @@
}
@Override
- public final void buildInstruction(IRBuilder builder, int instructionIndex)
+ public final void buildInstruction(
+ IRBuilder builder, int instructionIndex, boolean firstBlockInstruction)
throws ApiLevelException {
constructors.get(instructionIndex).accept(builder);
}
diff --git a/src/main/java/com/android/tools/r8/optimize/VisibilityBridgeRemover.java b/src/main/java/com/android/tools/r8/optimize/VisibilityBridgeRemover.java
index c815cf8..5847a80 100644
--- a/src/main/java/com/android/tools/r8/optimize/VisibilityBridgeRemover.java
+++ b/src/main/java/com/android/tools/r8/optimize/VisibilityBridgeRemover.java
@@ -33,8 +33,9 @@
method.getCode().registerCodeReferences(targetExtractor);
DexMethod target = targetExtractor.getTarget();
InvokeKind kind = targetExtractor.getKind();
- if (target != null &&
- target.proto == method.method.proto) {
+ // javac-generated visibility forward bridge method has same descriptor (name, signature and
+ // return type).
+ if (target != null && target.hasSameProtoAndName(method.method)) {
assert !method.accessFlags.isPrivate() && !method.accessFlags.isConstructor();
if (kind == InvokeKind.SUPER) {
// This is a visibility forward, so check for the direct target.
diff --git a/src/main/java/com/android/tools/r8/utils/InternalOptions.java b/src/main/java/com/android/tools/r8/utils/InternalOptions.java
index dc0cd0f..42775d2 100644
--- a/src/main/java/com/android/tools/r8/utils/InternalOptions.java
+++ b/src/main/java/com/android/tools/r8/utils/InternalOptions.java
@@ -9,6 +9,7 @@
import com.android.tools.r8.ProgramConsumer;
import com.android.tools.r8.StringConsumer;
import com.android.tools.r8.dex.Marker;
+import com.android.tools.r8.errors.CompilationError;
import com.android.tools.r8.errors.InvalidDebugInfoException;
import com.android.tools.r8.graph.DexEncodedMethod;
import com.android.tools.r8.graph.DexItemFactory;
@@ -100,6 +101,8 @@
public boolean verbose = false;
// Silencing output.
public boolean quiet = false;
+ // Throw exception if there is a warning about invalid debug info.
+ public boolean invalidDebugInfoFatal = false;
// Hidden marker for classes.dex
private boolean hasMarker = false;
@@ -268,6 +271,9 @@
public void warningInvalidDebugInfo(
DexEncodedMethod method, Origin origin, InvalidDebugInfoException e) {
+ if (invalidDebugInfoFatal) {
+ throw new CompilationError("Fatal warning: Invalid debug info", e);
+ }
synchronized (warningInvalidDebugInfo) {
warningInvalidDebugInfo.computeIfAbsent(
origin, k -> new ArrayList<>()).add(new Pair<>(method, e.getMessage()));
diff --git a/src/test/java/com/android/tools/r8/TestBase.java b/src/test/java/com/android/tools/r8/TestBase.java
index d7d8b99..a29d673 100644
--- a/src/test/java/com/android/tools/r8/TestBase.java
+++ b/src/test/java/com/android/tools/r8/TestBase.java
@@ -398,6 +398,13 @@
+ (obfuscate ? "-printmapping\n" : "-dontobfuscate\n");
}
+ public static String keepMainProguardConfiguration(
+ String clazz, boolean allowaccessmodification, boolean obfuscate) {
+ return keepMainProguardConfiguration(clazz)
+ + (allowaccessmodification ? "-allowaccessmodification\n" : "")
+ + (obfuscate ? "-printmapping\n" : "-dontobfuscate\n");
+ }
+
/**
* Run application on the specified version of Art with the specified main class.
*/
diff --git a/src/test/java/com/android/tools/r8/bridgeremoval/RemoveVisibilityBridgeMethodsTest.java b/src/test/java/com/android/tools/r8/bridgeremoval/RemoveVisibilityBridgeMethodsTest.java
index 6e3c114..35a7613 100644
--- a/src/test/java/com/android/tools/r8/bridgeremoval/RemoveVisibilityBridgeMethodsTest.java
+++ b/src/test/java/com/android/tools/r8/bridgeremoval/RemoveVisibilityBridgeMethodsTest.java
@@ -4,14 +4,26 @@
package com.android.tools.r8.bridgeremoval;
+import static com.android.tools.r8.utils.DexInspectorMatchers.isPresent;
+import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertThat;
import com.android.tools.r8.TestBase;
+import com.android.tools.r8.ToolHelper;
+import com.android.tools.r8.ToolHelper.ProcessResult;
import com.android.tools.r8.bridgeremoval.bridgestoremove.Main;
import com.android.tools.r8.bridgeremoval.bridgestoremove.Outer;
+import com.android.tools.r8.jasmin.JasminBuilder;
+import com.android.tools.r8.jasmin.JasminBuilder.ClassBuilder;
+import com.android.tools.r8.utils.AndroidApp;
import com.android.tools.r8.utils.DexInspector;
+import com.android.tools.r8.utils.DexInspector.ClassSubject;
+import com.android.tools.r8.utils.DexInspector.MethodSubject;
import com.google.common.collect.ImmutableList;
import java.lang.reflect.Method;
+import java.nio.file.Path;
+import java.util.Collections;
import java.util.List;
import org.junit.Test;
@@ -40,4 +52,81 @@
public void testWithoutObfuscation() throws Exception {
run(false);
}
+
+ @Test
+ public void regressionTest_b76383728_WithObfuscation() throws Exception {
+ runRegressionTest_b76383728(true);
+ }
+
+ @Test
+ public void regressionTest_b76383728_WithoutObfuscation() throws Exception {
+ runRegressionTest_b76383728(false);
+ }
+
+ /**
+ * Regression test for b76383728 to make sure we correctly identify and remove real visibility
+ * forward bridge methods synthesized by javac.
+ */
+ private void runRegressionTest_b76383728(boolean obfuscate) throws Exception {
+ JasminBuilder jasminBuilder = new JasminBuilder();
+
+ ClassBuilder superClass = jasminBuilder.addClass("SuperClass");
+ superClass.addDefaultConstructor();
+ superClass.addVirtualMethod("method", Collections.emptyList(), "Ljava/lang/String;",
+ ".limit stack 1",
+ "ldc \"Hello World\"",
+ "areturn");
+
+ // Generate a subclass with a method with same
+ ClassBuilder subclass = jasminBuilder.addClass("SubClass", superClass.name);
+ subclass.addBridgeMethod("getMethod", Collections.emptyList(), "Ljava/lang/String;",
+ ".limit stack 1",
+ "aload_0",
+ "invokespecial " + superClass.name + "/method()Ljava/lang/String;",
+ "areturn");
+
+ ClassBuilder mainClass = jasminBuilder.addClass("Main");
+ mainClass.addMainMethod(
+ ".limit stack 3",
+ ".limit locals 2",
+ "getstatic java/lang/System/out Ljava/io/PrintStream;",
+ "new " + subclass.name,
+ "dup",
+ "invokespecial " + subclass.name + "/<init>()V",
+ "invokevirtual " + subclass.name + "/getMethod()Ljava/lang/String;",
+ "invokevirtual java/io/PrintStream/print(Ljava/lang/String;)V",
+ "return"
+ );
+
+ final String mainClassName = mainClass.name;
+
+ String proguardConfig = keepMainProguardConfiguration(mainClass.name, true, obfuscate);
+
+ // Run input program on java.
+ Path outputDirectory = temp.newFolder().toPath();
+ jasminBuilder.writeClassFiles(outputDirectory);
+ ProcessResult javaResult = ToolHelper.runJava(outputDirectory, mainClassName);
+ assertEquals(0, javaResult.exitCode);
+
+ AndroidApp optimizedApp = compileWithR8(jasminBuilder.build(), proguardConfig,
+ // Disable inlining to avoid the (short) tested method from being inlined then removed.
+ internalOptions -> internalOptions.enableInlining = false);
+
+ // Run optimized (output) program on ART
+ String artResult = runOnArt(optimizedApp, mainClassName);
+ assertEquals(javaResult.stdout, artResult);
+
+ DexInspector inspector = new DexInspector(optimizedApp);
+
+ ClassSubject classSubject = inspector.clazz(superClass.name);
+ assertThat(classSubject, isPresent());
+ MethodSubject methodSubject = classSubject
+ .method("java.lang.String", "method", Collections.emptyList());
+ assertThat(methodSubject, isPresent());
+
+ classSubject = inspector.clazz(subclass.name);
+ assertThat(classSubject, isPresent());
+ methodSubject = classSubject.method("java.lang.String", "getMethod", Collections.emptyList());
+ assertThat(methodSubject, isPresent());
+ }
}
diff --git a/src/test/java/com/android/tools/r8/cf/DebugInfoTest.java b/src/test/java/com/android/tools/r8/cf/DebugInfoTest.java
new file mode 100644
index 0000000..6e009e8
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/cf/DebugInfoTest.java
@@ -0,0 +1,27 @@
+// Copyright (c) 2018, 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.cf;
+
+public class DebugInfoTest {
+
+ public static void main(String[] args) {
+ if (args.length > 0) {
+ arg = args.length % 2 == 0;
+ DebugInfoTest.method();
+ }
+ }
+
+ private static boolean arg;
+
+ private static void method() {
+ int intVar;
+ if (arg) {
+ float floatVar1 = 0f;
+ intVar = (int) floatVar1;
+ } else {
+ float floatVar2 = 0f;
+ intVar = (int) floatVar2;
+ }
+ }
+}
diff --git a/src/test/java/com/android/tools/r8/cf/DebugInfoTestRunner.java b/src/test/java/com/android/tools/r8/cf/DebugInfoTestRunner.java
new file mode 100644
index 0000000..fed7637
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/cf/DebugInfoTestRunner.java
@@ -0,0 +1,68 @@
+// Copyright (c) 2018, 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.cf;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+
+import com.android.tools.r8.ClassFileConsumer;
+import com.android.tools.r8.CompilationMode;
+import com.android.tools.r8.ProgramConsumer;
+import com.android.tools.r8.R8Command;
+import com.android.tools.r8.R8Command.Builder;
+import com.android.tools.r8.TestBase;
+import com.android.tools.r8.ToolHelper;
+import com.android.tools.r8.ToolHelper.ProcessResult;
+import com.android.tools.r8.errors.CompilationError;
+import com.android.tools.r8.errors.InvalidDebugInfoException;
+import com.android.tools.r8.origin.Origin;
+import com.android.tools.r8.utils.ThrowingConsumer;
+import java.nio.file.Path;
+import org.junit.Test;
+
+public class DebugInfoTestRunner extends TestBase {
+ static final Class CLASS = DebugInfoTest.class;
+
+ @Test
+ public void test() throws Exception {
+ ProcessResult runInput =
+ ToolHelper.runJava(ToolHelper.getClassPathForTests(), CLASS.getCanonicalName());
+ assertEquals(0, runInput.exitCode);
+ Path out1 = temp.getRoot().toPath().resolve("out1.zip");
+ build(
+ builder -> builder.addClassProgramData(ToolHelper.getClassAsBytes(CLASS), Origin.unknown()),
+ new ClassFileConsumer.ArchiveConsumer(out1));
+ ProcessResult run1 = ToolHelper.runJava(out1, CLASS.getCanonicalName());
+ assertEquals(runInput.toString(), run1.toString());
+ Path out2 = temp.getRoot().toPath().resolve("out2.zip");
+ boolean invalidDebugInfo = false;
+ try {
+ build(builder -> builder.addProgramFiles(out1), new ClassFileConsumer.ArchiveConsumer(out2));
+ } catch (CompilationError e) {
+ invalidDebugInfo = e.getCause() instanceof InvalidDebugInfoException;
+ }
+ // TODO(b/77522100): Change to assertFalse when fixed.
+ assertTrue(invalidDebugInfo);
+ if (!invalidDebugInfo) {
+ ProcessResult run2 = ToolHelper.runJava(out2, CLASS.getCanonicalName());
+ assertEquals(runInput.toString(), run2.toString());
+ }
+ }
+
+ private void build(ThrowingConsumer<Builder, Exception> input, ProgramConsumer consumer)
+ throws Exception {
+ Builder builder =
+ R8Command.builder()
+ .setMode(CompilationMode.DEBUG)
+ .addLibraryFiles(ToolHelper.getAndroidJar(ToolHelper.getMinApiLevelForDexVm()))
+ .setProgramConsumer(consumer);
+ input.accept(builder);
+ ToolHelper.runR8(
+ builder.build(),
+ o -> {
+ o.invalidDebugInfoFatal = true;
+ o.enableInlining = false;
+ });
+ }
+}
diff --git a/src/test/java/com/android/tools/r8/jasmin/JasminBuilder.java b/src/test/java/com/android/tools/r8/jasmin/JasminBuilder.java
index 64d092b..ff3f81a 100644
--- a/src/test/java/com/android/tools/r8/jasmin/JasminBuilder.java
+++ b/src/test/java/com/android/tools/r8/jasmin/JasminBuilder.java
@@ -120,6 +120,15 @@
return addMethod("public", name, argumentTypes, returnType, lines);
}
+ public MethodSignature addBridgeMethod(
+ String name,
+ List<String> argumentTypes,
+ String returnType,
+ String... lines) {
+ makeInit = true;
+ return addMethod("public bridge", name, argumentTypes, returnType, lines);
+ }
+
public MethodSignature addPrivateVirtualMethod(
String name,
List<String> argumentTypes,
diff --git a/src/test/java/com/android/tools/r8/maindexlist/MainDexListTests.java b/src/test/java/com/android/tools/r8/maindexlist/MainDexListTests.java
index 35df33e..a6d0177 100644
--- a/src/test/java/com/android/tools/r8/maindexlist/MainDexListTests.java
+++ b/src/test/java/com/android/tools/r8/maindexlist/MainDexListTests.java
@@ -707,7 +707,8 @@
}
@Override
- public void buildInstruction(IRBuilder builder, int instructionIndex) {
+ public void buildInstruction(
+ IRBuilder builder, int instructionIndex, boolean firstBlockInstruction) {
assert instructionIndex == 0;
builder.addReturn();
}
