Prevent reordered instructions w/o positions getting random positions.
Moving instructions without positions (e.g. preamble) past
instructions with actual position would make them inherit those
positions. In this change we assign line number = 0 to those
instructions to prevent this.
Bug: 73150368
Change-Id: I564190417a8e11eb04d5bb354de7430fcc035237
diff --git a/src/main/java/com/android/tools/r8/graph/DexDebugEventBuilder.java b/src/main/java/com/android/tools/r8/graph/DexDebugEventBuilder.java
index 69fb348..bfc0a9f 100644
--- a/src/main/java/com/android/tools/r8/graph/DexDebugEventBuilder.java
+++ b/src/main/java/com/android/tools/r8/graph/DexDebugEventBuilder.java
@@ -195,11 +195,13 @@
private void emitDebugPosition(int pc, Position position) {
assert !position.equals(emittedPosition);
if (startLine == NO_LINE_INFO) {
- if (position.synthetic) {
+ assert emittedPosition.isNone();
+ if (position.synthetic && position.callerPosition == null) {
// Ignore synthetic positions prior to any actual position.
+ // We do need to preserve synthetic position establishing the stack frame for inlined
+ // methods.
return;
}
- assert emittedPosition.isNone();
startLine = position.line;
emittedPosition = new Position(position.line, null, method.method, null);
}
diff --git a/src/main/java/com/android/tools/r8/ir/code/DebugPosition.java b/src/main/java/com/android/tools/r8/ir/code/DebugPosition.java
index cde22b0..8d522de 100644
--- a/src/main/java/com/android/tools/r8/ir/code/DebugPosition.java
+++ b/src/main/java/com/android/tools/r8/ir/code/DebugPosition.java
@@ -31,6 +31,7 @@
@Override
public void buildDex(DexBuilder builder) {
+ assert getPosition().isSome() && !getPosition().synthetic;
builder.addDebugPosition(this);
}
@@ -73,6 +74,7 @@
@Override
public void buildCf(CfBuilder builder) {
+ assert getPosition().isSome() && !getPosition().synthetic;
// All redundant debug positions are removed. Remaining ones must force a pc advance.
builder.add(new CfNop());
}
diff --git a/src/main/java/com/android/tools/r8/ir/conversion/DexBuilder.java b/src/main/java/com/android/tools/r8/ir/conversion/DexBuilder.java
index 96b2122..3ad0168 100644
--- a/src/main/java/com/android/tools/r8/ir/conversion/DexBuilder.java
+++ b/src/main/java/com/android/tools/r8/ir/conversion/DexBuilder.java
@@ -984,14 +984,7 @@
int source = builder.getInfo(jump).getOffset();
Info targetInfo = builder.getTargetInfo(jump.getTarget());
int relativeOffset = targetInfo.getOffset() - source;
- // Emit a return if the target is a return and the size of the return is the computed
- // size of this instruction.
- Return ret = targetInfo.getIR().asReturn();
- if (ret != null && size == targetInfo.getSize() && ret.getPosition().isNone()) {
- Instruction dex = ret.createDexInstruction(builder);
- dex.setOffset(getOffset()); // for better printing of the dex code.
- instructions.add(dex);
- } else if (size == relativeOffset) {
+ if (size == relativeOffset) {
// We should never generate a goto targeting the next instruction. However, if we do
// we replace it with nops. This works around a dalvik bug where the dalvik tracing
// jit crashes on 'goto next instruction' on Android 4.1.1.
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 55e4b83..b9bbe5a 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
@@ -136,7 +136,7 @@
@Override
public void buildPrelude(IRBuilder builder) {
- currentPosition = Position.none();
+ currentPosition = Position.synthetic(0, method, null);
if (code.incomingRegisterSize == 0) {
return;
}
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 98c1015..6364c82 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
@@ -396,10 +396,6 @@
// necessary.
ir.splitCriticalEdges();
- if (options.testing.invertConditionals) {
- invertConditionalsForTesting(ir);
- }
-
// Create block order and make sure that all blocks are immediately followed by their
// fallthrough block if any.
ir.traceBlocks();
@@ -440,7 +436,7 @@
} else {
current = position;
}
- } else if (position.isSome() && !position.equals(current)) {
+ } else if (position.isSome() && !position.synthetic && !position.equals(current)) {
DebugPosition positionChange = new DebugPosition();
positionChange.setPosition(position);
it.previous();
@@ -2006,14 +2002,6 @@
return type != NumericType.FLOAT && type != NumericType.DOUBLE && type != NumericType.LONG;
}
- private static void invertConditionalsForTesting(IRCode code) {
- for (BasicBlock block : code.blocks) {
- if (block.exit().isIf()) {
- block.exit().asIf().invert();
- }
- }
- }
-
@Override
public String toString() {
StringBuilder builder = new StringBuilder();
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 39cea77..a23505f 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
@@ -24,6 +24,7 @@
import com.android.tools.r8.ir.analysis.constant.SparseConditionalConstantPropagation;
import com.android.tools.r8.ir.analysis.type.TypeAnalysis;
import com.android.tools.r8.ir.analysis.type.TypeEnvironment;
+import com.android.tools.r8.ir.code.BasicBlock;
import com.android.tools.r8.ir.code.IRCode;
import com.android.tools.r8.ir.desugar.InterfaceMethodRewriter;
import com.android.tools.r8.ir.desugar.LambdaRewriter;
@@ -511,6 +512,14 @@
}
}
+ private static void invertConditionalsForTesting(IRCode code) {
+ for (BasicBlock block : code.blocks) {
+ if (block.exit().isIf()) {
+ block.exit().asIf().invert();
+ }
+ }
+ }
+
private void rewriteCode(DexEncodedMethod method,
OptimizationFeedback feedback,
Predicate<DexEncodedMethod> isProcessedConcurrently,
@@ -589,6 +598,12 @@
codeRewriter.rewriteSwitch(code);
codeRewriter.processMethodsNeverReturningNormally(code);
codeRewriter.simplifyIf(code, typeEnvironment);
+
+ if (options.testing.invertConditionals) {
+ invertConditionalsForTesting(code);
+ code.traceBlocks();
+ }
+
if (options.enableNonNullTracking && nonNullTracker != null) {
nonNullTracker.cleanupNonNull(code);
assert code.isConsistentSSA();
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 69ecb83..12429e9 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
@@ -186,6 +186,9 @@
// Canonicalized positions to lower memory usage.
private final Int2ReferenceMap<Position> canonicalPositions = new Int2ReferenceOpenHashMap<>();
+ // Synthetic position with line = 0.
+ private Position preamblePosition = null;
+
// Cooked position to indicate positions in synthesized code (ie, for synchronization).
private Position syntheticPosition = null;
@@ -262,6 +265,8 @@
@Override
public void buildPrelude(IRBuilder builder) {
+ currentPosition = getPreamblePosition();
+
// Record types for arguments.
Int2ReferenceMap<ValueType> argumentLocals = recordArgumentTypes();
Int2ReferenceMap<ValueType> initializedLocals = new Int2ReferenceOpenHashMap<>(argumentLocals);
@@ -327,8 +332,6 @@
locals = state.openLocals(getOffset(initialLabel));
}
- currentPosition = Position.none();
-
// Build the actual argument instructions now that type and debug information is known
// for arguments.
buildArgumentInstructions(builder);
@@ -457,7 +460,7 @@
private void buildExceptionalPostlude(IRBuilder builder) {
assert isSynchronized();
generatingMethodSynchronization = true;
- currentPosition = getSyntheticPosition();
+ currentPosition = getExceptionalExitPosition();
buildMonitorExit(builder);
builder.addThrow(getMoveExceptionRegister());
generatingMethodSynchronization = false;
@@ -503,7 +506,9 @@
// writes after the line has changed and thus causing locals to become visible too late.
currentPosition =
getDebugPositionAtOffset(
- insn instanceof LabelNode ? instructionIndex - 1 : instructionIndex);
+ ((instructionIndex > 0) && (insn instanceof LabelNode))
+ ? instructionIndex - 1
+ : instructionIndex);
build(insn, builder);
@@ -2906,10 +2911,11 @@
@Override
public Position getDebugPositionAtOffset(int offset) {
if (offset == EXCEPTIONAL_SYNC_EXIT_OFFSET) {
- return getSyntheticPosition();
+ return getExceptionalExitPosition();
}
int index = instructionIndex(offset);
if (index < 0 || instructionCount() <= index) {
+ assert false;
return Position.none();
}
AbstractInsnNode insn = node.instructions.get(index);
@@ -2923,7 +2929,7 @@
LineNumberNode line = (LineNumberNode) insn;
return getCanonicalPosition(line.line);
}
- return Position.none();
+ return getPreamblePosition();
}
@Override
@@ -2936,12 +2942,19 @@
line, l -> new Position(l, null, method, callerPosition));
}
+ private Position getPreamblePosition() {
+ if (preamblePosition == null) {
+ preamblePosition = Position.synthetic(0, method, null);
+ }
+ return preamblePosition;
+ }
+
// If we need to emit a synthetic position for exceptional monitor exits, we try to cook up a
// position that is not actually a valid program position, so as not to incorrectly position the
// user on an exit that is not the actual exit being taken. Our heuristic for this is that if the
// method has at least two positions we use the first position minus one as the synthetic exit.
// If the method only has one position it is safe to just use that position.
- private Position getSyntheticPosition() {
+ private Position getExceptionalExitPosition() {
if (syntheticPosition == null) {
int min = Integer.MAX_VALUE;
int max = Integer.MIN_VALUE;
diff --git a/src/test/java/com/android/tools/r8/ToolHelper.java b/src/test/java/com/android/tools/r8/ToolHelper.java
index af7f054..0c0cd84 100644
--- a/src/test/java/com/android/tools/r8/ToolHelper.java
+++ b/src/test/java/com/android/tools/r8/ToolHelper.java
@@ -949,6 +949,10 @@
return Paths.get(System.getProperty("java.home"), "bin", "java").toString();
}
+ public static ProcessResult runArtRaw(ArtCommandBuilder builder) throws IOException {
+ return runArtProcessRaw(builder);
+ }
+
public static ProcessResult runArtRaw(String file, String mainClass)
throws IOException {
return runArtRaw(Collections.singletonList(file), mainClass, null);
diff --git a/src/test/java/com/android/tools/r8/debuginfo/PreamblePositionTestRunner.java b/src/test/java/com/android/tools/r8/debuginfo/PreamblePositionTestRunner.java
new file mode 100644
index 0000000..14f9f98
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/debuginfo/PreamblePositionTestRunner.java
@@ -0,0 +1,83 @@
+// 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.debuginfo;
+
+import static org.junit.Assert.assertNotEquals;
+import static org.junit.Assert.assertTrue;
+
+import com.android.tools.r8.CompilationMode;
+import com.android.tools.r8.D8Command;
+import com.android.tools.r8.OutputMode;
+import com.android.tools.r8.ToolHelper;
+import com.android.tools.r8.ToolHelper.ArtCommandBuilder;
+import com.android.tools.r8.ToolHelper.ProcessResult;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+
+public class PreamblePositionTestRunner {
+
+ private static final String TEST_CLASS = "PreamblePositionTestSource";
+ private static final String TEST_PACKAGE = "com.android.tools.r8.debuginfo";
+
+ @ClassRule public static TemporaryFolder temp = ToolHelper.getTemporaryFolderForTest();
+
+ @Test
+ public void testBothBranchesDefaultConditionals() throws Exception {
+ testBothBranches(false);
+ }
+
+ @Test
+ public void testBothBranchesInvertConditionals() throws Exception {
+ testBothBranches(true);
+ }
+
+ private void testBothBranches(boolean invertConditionals) throws Exception {
+ Path testClassDir = temp.newFolder(TEST_PACKAGE.split(".")).toPath();
+ Path testClassPath = testClassDir.resolve(TEST_CLASS + ".class");
+ Path outputDexPath = temp.newFolder().toPath();
+
+ Files.write(testClassPath, PreamblePositionTestSourceDump.dump());
+
+ ToolHelper.runD8(
+ D8Command.builder()
+ .addProgramFiles(testClassPath)
+ .setOutput(outputDexPath, OutputMode.DexIndexed)
+ .setMode(CompilationMode.RELEASE),
+ options -> {
+ options.testing.invertConditionals = invertConditionals;
+ });
+
+ String fileName = TEST_CLASS + ".java";
+
+ for (boolean testTrueBranch : new boolean[] {false, true}) {
+ ArtCommandBuilder artCommandBuilder = new ArtCommandBuilder();
+ artCommandBuilder.appendClasspath(outputDexPath.resolve("classes.dex").toString());
+ artCommandBuilder.setMainClass(TEST_PACKAGE + "." + TEST_CLASS);
+ if (!testTrueBranch) {
+ artCommandBuilder.appendProgramArgument("1");
+ }
+
+ ProcessResult result = ToolHelper.runArtRaw(artCommandBuilder);
+
+ assertNotEquals(result.exitCode, 0);
+ if (testTrueBranch) {
+ assertTrue(result.stderr.contains("<true-branch-exception>"));
+ // Must have either explicit line = 0 or no line info at all.
+ assertTrue(
+ result.stderr.contains(fileName + ":0")
+ || (result.stderr.contains("at " + TEST_PACKAGE + "." + TEST_CLASS + ".main")
+ && !result.stderr.contains(fileName + ":")));
+ } else {
+ assertTrue(result.stderr.contains("<false-branch-exception>"));
+ assertTrue(
+ result.stderr.contains(
+ fileName + ":" + PreamblePositionTestSourceDump.FALSE_BRANCH_LINE_NUMBER));
+ }
+ }
+ }
+}
diff --git a/src/test/java/com/android/tools/r8/debuginfo/PreamblePositionTestSourceDump.java b/src/test/java/com/android/tools/r8/debuginfo/PreamblePositionTestSourceDump.java
new file mode 100644
index 0000000..5ed2a4e
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/debuginfo/PreamblePositionTestSourceDump.java
@@ -0,0 +1,106 @@
+// 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.debuginfo;
+
+import java.util.*;
+import org.objectweb.asm.*;
+
+/*
+Generated from the source code below, line numbers removed, except for the false branch,
+which is set to FALSE_BRANCH_LINE_NUMBER.
+
+ package com.android.tools.r8.debuginfo;
+
+ public class PreamblePositionTestSource {
+ public static void main(String[] args) {
+ System.err.println("<first-line>");
+ if (args.length == 0) {
+ throw new RuntimeException("<true-branch-exception>");
+ } else {
+ throw new RuntimeException("<false-branch-exception>");
+ }
+ }
+ }
+*/
+
+public class PreamblePositionTestSourceDump implements Opcodes {
+
+ static final int FALSE_BRANCH_LINE_NUMBER = 123;
+
+ public static byte[] dump() throws Exception {
+
+ ClassWriter cw = new ClassWriter(0);
+ FieldVisitor fv;
+ MethodVisitor mv;
+ AnnotationVisitor av0;
+
+ cw.visit(
+ V1_8,
+ ACC_PUBLIC + ACC_SUPER,
+ "com/android/tools/r8/debuginfo/PreamblePositionTestSource",
+ null,
+ "java/lang/Object",
+ null);
+
+ cw.visitSource("PreamblePositionTestSource.java", null);
+
+ {
+ mv = cw.visitMethod(ACC_PUBLIC, "<init>", "()V", null, null);
+ mv.visitCode();
+ Label l0 = new Label();
+ mv.visitLabel(l0);
+ mv.visitVarInsn(ALOAD, 0);
+ mv.visitMethodInsn(INVOKESPECIAL, "java/lang/Object", "<init>", "()V", false);
+ mv.visitInsn(RETURN);
+ Label l1 = new Label();
+ mv.visitLabel(l1);
+ mv.visitLocalVariable(
+ "this", "Lcom/android/tools/r8/debuginfo/PreamblePositionTestSource;", null, l0, l1, 0);
+ mv.visitMaxs(1, 1);
+ mv.visitEnd();
+ }
+ {
+ mv = cw.visitMethod(ACC_PUBLIC + ACC_STATIC, "main", "([Ljava/lang/String;)V", null, null);
+ mv.visitCode();
+ Label l0 = new Label();
+ mv.visitLabel(l0);
+ mv.visitFieldInsn(GETSTATIC, "java/lang/System", "err", "Ljava/io/PrintStream;");
+ mv.visitLdcInsn("<first-line>");
+ mv.visitMethodInsn(
+ INVOKEVIRTUAL, "java/io/PrintStream", "println", "(Ljava/lang/String;)V", false);
+ Label l1 = new Label();
+ mv.visitLabel(l1);
+ mv.visitVarInsn(ALOAD, 0);
+ mv.visitInsn(ARRAYLENGTH);
+ Label l2 = new Label();
+ mv.visitJumpInsn(IFNE, l2);
+ Label l3 = new Label();
+ mv.visitLabel(l3);
+ mv.visitTypeInsn(NEW, "java/lang/RuntimeException");
+ mv.visitInsn(DUP);
+ mv.visitLdcInsn("<true-branch-exception>");
+ mv.visitMethodInsn(
+ INVOKESPECIAL, "java/lang/RuntimeException", "<init>", "(Ljava/lang/String;)V", false);
+ mv.visitInsn(ATHROW);
+ mv.visitLabel(l2);
+ mv.visitLineNumber(FALSE_BRANCH_LINE_NUMBER, l2);
+ mv.visitFrame(Opcodes.F_SAME, 0, null, 0, null);
+ mv.visitTypeInsn(NEW, "java/lang/RuntimeException");
+ mv.visitInsn(DUP);
+ mv.visitLdcInsn("<false-branch-exception>");
+ mv.visitMethodInsn(
+ INVOKESPECIAL, "java/lang/RuntimeException", "<init>", "(Ljava/lang/String;)V", false);
+ mv.visitInsn(ATHROW);
+ Label l4 = new Label();
+ mv.visitLabel(l4);
+ mv.visitLocalVariable("args", "[Ljava/lang/String;", null, l0, l4, 0);
+ mv.visitMaxs(3, 1);
+ mv.visitEnd();
+ }
+ cw.visitEnd();
+
+ return cw.toByteArray();
+ }
+}
diff --git a/src/test/java/com/android/tools/r8/smali/OutlineTest.java b/src/test/java/com/android/tools/r8/smali/OutlineTest.java
index 64ea9b4..5088aeb 100644
--- a/src/test/java/com/android/tools/r8/smali/OutlineTest.java
+++ b/src/test/java/com/android/tools/r8/smali/OutlineTest.java
@@ -13,6 +13,7 @@
import com.android.tools.r8.code.ConstWideHigh16;
import com.android.tools.r8.code.DivInt;
import com.android.tools.r8.code.DivInt2Addr;
+import com.android.tools.r8.code.Goto;
import com.android.tools.r8.code.InvokeStatic;
import com.android.tools.r8.code.InvokeVirtual;
import com.android.tools.r8.code.MoveResult;
@@ -1244,7 +1245,7 @@
assertTrue(code.instructions[1] instanceof InvokeStatic);
assertTrue(code.instructions[2] instanceof MoveResult);
assertTrue(code.instructions[3] instanceof DivInt2Addr);
- assertTrue(code.instructions[4] instanceof Return);
+ assertTrue(code.instructions[4] instanceof Goto);
assertTrue(code.instructions[5] instanceof Const4);
assertTrue(code.instructions[6] instanceof Return);
InvokeStatic invoke = (InvokeStatic) code.instructions[1];
