Version 1.2.47
Merge: Insert a nop on any exceptional edges directly targeting a loop header.
CL: https://r8-review.googlesource.com/c/r8/+/26920
Bug: 111337896
Change-Id: Ib9f5cf5e75335b2c394a8f9a251f665cb1b26f6d
diff --git a/src/main/java/com/android/tools/r8/Version.java b/src/main/java/com/android/tools/r8/Version.java
index cc8472a..ddbe2a1 100644
--- a/src/main/java/com/android/tools/r8/Version.java
+++ b/src/main/java/com/android/tools/r8/Version.java
@@ -11,7 +11,7 @@
// This field is accessed from release scripts using simple pattern matching.
// Therefore, changing this field could break our release scripts.
- public static final String LABEL = "1.2.46";
+ public static final String LABEL = "1.2.47";
private Version() {
}
diff --git a/src/main/java/com/android/tools/r8/ir/code/AlwaysMaterializingNop.java b/src/main/java/com/android/tools/r8/ir/code/AlwaysMaterializingNop.java
new file mode 100644
index 0000000..e5ba1de
--- /dev/null
+++ b/src/main/java/com/android/tools/r8/ir/code/AlwaysMaterializingNop.java
@@ -0,0 +1,66 @@
+// 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.ir.code;
+
+import com.android.tools.r8.cf.LoadStoreHelper;
+import com.android.tools.r8.cf.code.CfNop;
+import com.android.tools.r8.errors.Unreachable;
+import com.android.tools.r8.graph.DexType;
+import com.android.tools.r8.ir.conversion.CfBuilder;
+import com.android.tools.r8.ir.conversion.DexBuilder;
+import com.android.tools.r8.ir.optimize.Inliner.Constraint;
+import com.android.tools.r8.shaking.Enqueuer.AppInfoWithLiveness;
+import com.android.tools.r8.utils.InternalOptions;
+
+public class AlwaysMaterializingNop extends Instruction {
+
+ public AlwaysMaterializingNop() {
+ super(null);
+ }
+
+ @Override
+ public boolean canBeDeadCode(IRCode code, InternalOptions options) {
+ return false;
+ }
+
+ @Override
+ public void buildDex(DexBuilder builder) {
+ builder.addNop(this);
+ }
+
+ @Override
+ public void buildCf(CfBuilder builder) {
+ builder.add(new CfNop());
+ }
+
+ @Override
+ public boolean identicalNonValueNonPositionParts(Instruction other) {
+ return other instanceof AlwaysMaterializingNop;
+ }
+
+ @Override
+ public int compareNonValueParts(Instruction other) {
+ return 0;
+ }
+
+ @Override
+ public int maxInValueRegister() {
+ throw new Unreachable();
+ }
+
+ @Override
+ public int maxOutValueRegister() {
+ throw new Unreachable();
+ }
+
+ @Override
+ public Constraint inliningConstraint(AppInfoWithLiveness info, DexType invocationContext) {
+ return Constraint.ALWAYS;
+ }
+
+ @Override
+ public void insertLoadAndStores(InstructionListIterator it, LoadStoreHelper helper) {
+ // Nothing to do.
+ }
+}
diff --git a/src/main/java/com/android/tools/r8/ir/code/AlwaysMaterializingUser.java b/src/main/java/com/android/tools/r8/ir/code/AlwaysMaterializingUser.java
index 4530040..4117cec 100644
--- a/src/main/java/com/android/tools/r8/ir/code/AlwaysMaterializingUser.java
+++ b/src/main/java/com/android/tools/r8/ir/code/AlwaysMaterializingUser.java
@@ -28,7 +28,7 @@
@Override
public void buildDex(DexBuilder builder) {
- builder.addNop(this);
+ builder.addNothing(this);
}
@Override
diff --git a/src/main/java/com/android/tools/r8/ir/code/BasicBlock.java b/src/main/java/com/android/tools/r8/ir/code/BasicBlock.java
index e10f789..df0c9f3 100644
--- a/src/main/java/com/android/tools/r8/ir/code/BasicBlock.java
+++ b/src/main/java/com/android/tools/r8/ir/code/BasicBlock.java
@@ -840,7 +840,7 @@
if (!hasCatchHandlers()) {
return false;
}
- return catchHandlers.getAllTargets().contains(successors.indexOf(block));
+ return catchHandlers.getUniqueTargets().contains(successors.indexOf(block));
}
public int guardsForCatchSuccessor(BasicBlock block) {
diff --git a/src/main/java/com/android/tools/r8/ir/code/ConstNumber.java b/src/main/java/com/android/tools/r8/ir/code/ConstNumber.java
index 71f1bec..3a03afd 100644
--- a/src/main/java/com/android/tools/r8/ir/code/ConstNumber.java
+++ b/src/main/java/com/android/tools/r8/ir/code/ConstNumber.java
@@ -114,7 +114,7 @@
public void buildDex(DexBuilder builder) {
if (!dest().needsRegister()) {
forceSetPosition(Position.none());
- builder.addNop(this);
+ builder.addNothing(this);
return;
}
diff --git a/src/main/java/com/android/tools/r8/ir/code/DebugLocalsChange.java b/src/main/java/com/android/tools/r8/ir/code/DebugLocalsChange.java
index edf68f3..8ff3c64 100644
--- a/src/main/java/com/android/tools/r8/ir/code/DebugLocalsChange.java
+++ b/src/main/java/com/android/tools/r8/ir/code/DebugLocalsChange.java
@@ -55,7 +55,7 @@
@Override
public void buildDex(DexBuilder builder) {
- builder.addNop(this);
+ builder.addNothing(this);
}
@Override
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 8201562..07107a5 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
@@ -424,7 +424,7 @@
if (jump.getTarget() != nextBlock) {
add(jump, new GotoInfo(jump));
} else {
- addNop(jump);
+ addNothing(jump);
}
}
@@ -437,7 +437,7 @@
add(move, new MoveInfo(move));
}
- public void addNop(com.android.tools.r8.ir.code.Instruction instruction) {
+ public void addNothing(com.android.tools.r8.ir.code.Instruction instruction) {
assert instruction.getPosition().isNone();
add(instruction, new FallThroughInfo(instruction));
}
@@ -457,10 +457,14 @@
&& !(instruction.outValue().needsRegister());
}
+ public void addNop(com.android.tools.r8.ir.code.Instruction ir) {
+ add(ir, new FixedSizeInfo(ir, new Nop()));
+ }
+
public void addDebugPosition(DebugPosition position) {
// Remaining debug positions always require we emit an actual nop instruction.
// See removeRedundantDebugPositions.
- add(position, new FixedSizeInfo(position, new Nop()));
+ addNop(position);
}
public void add(com.android.tools.r8.ir.code.Instruction ir, Instruction dex) {
@@ -493,7 +497,7 @@
if (nextBlock != null
&& ret.identicalAfterRegisterAllocation(nextBlock.entry(), registerAllocator)) {
ret.forceSetPosition(Position.none());
- addNop(ret);
+ addNothing(ret);
} else {
add(ret, dex);
}
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 cc0a5f9..d2f1d60 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
@@ -841,6 +841,9 @@
workaroundForwardingInitializerBug(code);
LinearScanRegisterAllocator registerAllocator = new LinearScanRegisterAllocator(code, options);
registerAllocator.allocateRegisters(options.debug);
+ if (options.canHaveExceptionTargetingLoopHeaderBug()) {
+ codeRewriter.workaroundExceptionTargetingLoopHeaderBug(code);
+ }
printMethod(code, "After register allocation (non-SSA)");
for (int i = 0; i < PEEPHOLE_OPTIMIZATION_PASSES; i++) {
CodeRewriter.collapsTrivialGotos(method, code);
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/CodeRewriter.java b/src/main/java/com/android/tools/r8/ir/optimize/CodeRewriter.java
index 67e779a..748c078 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/CodeRewriter.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/CodeRewriter.java
@@ -32,6 +32,7 @@
import com.android.tools.r8.graph.DexValue.DexValueString;
import com.android.tools.r8.ir.analysis.type.TypeEnvironment;
import com.android.tools.r8.ir.analysis.type.TypeLatticeElement;
+import com.android.tools.r8.ir.code.AlwaysMaterializingNop;
import com.android.tools.r8.ir.code.ArrayPut;
import com.android.tools.r8.ir.code.BasicBlock;
import com.android.tools.r8.ir.code.Binop;
@@ -2985,4 +2986,28 @@
}
}
}
+
+ // If an exceptional edge could target a conditional-loop header ensure that we have a
+ // materializing instruction on that path to work around a bug in some L x86_64 non-emulator VMs.
+ // See b/111337896.
+ public void workaroundExceptionTargetingLoopHeaderBug(IRCode code) {
+ for (BasicBlock block : code.blocks) {
+ if (block.hasCatchHandlers()) {
+ for (BasicBlock handler : block.getCatchHandlers().getUniqueTargets()) {
+ // We conservatively assume that a block with at least two normal predecessors is a loop
+ // header. If we ever end up computing exact loop headers, use that here instead.
+ // The loop is conditional if it has at least two normal successors.
+ BasicBlock target = handler.endOfGotoChain();
+ if (target.getPredecessors().size() > 2
+ && target.getNormalPredecessors().size() > 1
+ && target.getNormalSuccessors().size() > 1) {
+ Instruction fixit = new AlwaysMaterializingNop();
+ fixit.setBlock(handler);
+ fixit.setPosition(handler.getPosition());
+ handler.getInstructions().addFirst(fixit);
+ }
+ }
+ }
+ }
+ }
}
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 470f64a..97b870c 100644
--- a/src/main/java/com/android/tools/r8/utils/InternalOptions.java
+++ b/src/main/java/com/android/tools/r8/utils/InternalOptions.java
@@ -658,4 +658,16 @@
public boolean canHaveForwardingInitInliningBug() {
return minApiLevel < AndroidApiLevel.M.getLevel();
}
+
+ // Some Lollipop x86_64 VMs have a bug causing a segfault if an exception handler directly targets
+ // a conditional-loop header. This cannot happen for debug builds as the existence of a
+ // move-exception instruction will ensure a non-direct target.
+ //
+ // To workaround this in release builds, we insert a materializing nop instruction in the
+ // exception handler forcing it not directly target any loop header.
+ //
+ // See b/111337896.
+ public boolean canHaveExceptionTargetingLoopHeaderBug() {
+ return isGeneratingDex() && !debug && minApiLevel < AndroidApiLevel.M.getLevel();
+ }
}
diff --git a/src/test/java/com/android/tools/r8/debuginfo/DebugInfoTestBase.java b/src/test/java/com/android/tools/r8/debuginfo/DebugInfoTestBase.java
index 6f85b18..da83ed9 100644
--- a/src/test/java/com/android/tools/r8/debuginfo/DebugInfoTestBase.java
+++ b/src/test/java/com/android/tools/r8/debuginfo/DebugInfoTestBase.java
@@ -35,7 +35,7 @@
@Rule
public TestDescriptionWatcher watcher = new TestDescriptionWatcher();
- static AndroidApp compileWithD8(Class... classes) throws IOException, CompilationFailedException {
+ static AndroidApp compileWithD8(Class... classes) throws CompilationFailedException {
D8Command.Builder builder = D8Command.builder();
for (Class clazz : classes) {
builder.addProgramFiles(ToolHelper.getClassFileForTestClass(clazz));
diff --git a/src/test/java/com/android/tools/r8/debuginfo/Regress111337896Test.java b/src/test/java/com/android/tools/r8/debuginfo/Regress111337896Test.java
new file mode 100644
index 0000000..bcb8215
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/debuginfo/Regress111337896Test.java
@@ -0,0 +1,40 @@
+// 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.Arrays;
+import java.util.Iterator;
+
+public class Regress111337896Test {
+
+ public static void regress111337896() {
+ Iterator it = Arrays.asList(new Object()).iterator();
+ while (it.hasNext()) { // Loop must be a conditional to hit the issue.
+ it.next();
+ try {
+ noThrow();
+ doThrow();
+ noThrow();
+ } catch (Exception e) {
+ // Handler targeting the loop header causes segfault on some ART 5.0 x86 devices.
+ continue;
+ }
+ // The normal successor may differ from the exceptional one and still cause the issue.
+ it.hasNext();
+ }
+ }
+
+ public static void doThrow() throws Exception {
+ throw new Exception();
+ }
+
+ public static void noThrow() throws Exception {
+ // Intentionally empty.
+ }
+
+ public static void main(String[] args) {
+ regress111337896();
+ System.out.print("aok");
+ }
+}
diff --git a/src/test/java/com/android/tools/r8/debuginfo/Regress111337896TestRunner.java b/src/test/java/com/android/tools/r8/debuginfo/Regress111337896TestRunner.java
new file mode 100644
index 0000000..1404f35
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/debuginfo/Regress111337896TestRunner.java
@@ -0,0 +1,71 @@
+// 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.assertEquals;
+
+import com.android.tools.r8.CompilationMode;
+import com.android.tools.r8.D8;
+import com.android.tools.r8.D8Command;
+import com.android.tools.r8.ToolHelper;
+import com.android.tools.r8.code.Instruction;
+import com.android.tools.r8.code.Nop;
+import com.android.tools.r8.utils.AndroidApiLevel;
+import com.android.tools.r8.utils.AndroidApp;
+import com.android.tools.r8.utils.AndroidAppConsumers;
+import java.util.Arrays;
+import org.junit.Test;
+
+public class Regress111337896TestRunner extends DebugInfoTestBase {
+
+ @Test
+ public void test() throws Exception {
+ Class clazz = Regress111337896Test.class;
+
+ // Check the regression test is valid code.
+ String expected = "aok";
+ assertEquals(expected, runOnJava(clazz));
+
+ for (AndroidApiLevel minApi : Arrays.asList(AndroidApiLevel.L, AndroidApiLevel.M)) {
+ for (CompilationMode mode : CompilationMode.values()) {
+ AndroidAppConsumers appSink = new AndroidAppConsumers();
+ D8.run(
+ D8Command.builder()
+ .addProgramFiles(ToolHelper.getClassFileForTestClass(clazz))
+ .setProgramConsumer(appSink.wrapDexIndexedConsumer(null))
+ .setMode(mode)
+ .setMinApiLevel(minApi.getLevel())
+ .build());
+ AndroidApp app = appSink.build();
+ assertEquals(expected, runOnArt(app, clazz.getCanonicalName()));
+
+ // Check that the compiled output contains a nop to workaround the issue.
+ // We can't really check much else as this only reproduces on some physical x86_64 devices.
+ check(inspectMethod(app, clazz, "void", "regress111337896"), mode, minApi);
+ }
+ }
+ }
+
+ private void check(DebugInfoInspector info, CompilationMode mode, AndroidApiLevel minApi) {
+ info.checkStartLine(12);
+ assertEquals(1, info.checkLineExists(18));
+ int nopsFound = 0;
+ for (Instruction instruction : info.getMethod().getCode().asDexCode().instructions) {
+ if (instruction instanceof Nop) {
+ nopsFound++;
+ }
+ }
+ if (mode == CompilationMode.DEBUG) {
+ // In debug mode a nop is used to preserve line 21.
+ info.checkLineExists(21);
+ assertEquals(1, nopsFound);
+ } else {
+ // Release mode will have removed the line.
+ info.checkNoLine(21);
+ // In release mode the workaround will have inserted a nop if below M.
+ int expectedNops = minApi.getLevel() < AndroidApiLevel.M.getLevel() ? 1 : 0;
+ assertEquals(expectedNops, nopsFound);
+ }
+ }
+}
