Reland "Add UninitializedThisLocalRead to instance initializer exits"
This reverts commit 376b4a5dcb300fe3855c91c00f1e6be689c05f41.
Bug: 183285081
Bug: 166738818
Change-Id: Id74ace648d61709ad6e18ea97172f96551e0b601
diff --git a/src/main/java/com/android/tools/r8/ir/code/Opcodes.java b/src/main/java/com/android/tools/r8/ir/code/Opcodes.java
index a3dc92b..51b19d8e 100644
--- a/src/main/java/com/android/tools/r8/ir/code/Opcodes.java
+++ b/src/main/java/com/android/tools/r8/ir/code/Opcodes.java
@@ -73,4 +73,5 @@
int THROW = 64;
int USHR = 65;
int XOR = 66;
+ int UNINITIALIZED_THIS_LOCAL_READ = 67;
}
diff --git a/src/main/java/com/android/tools/r8/ir/code/UninitializedThisLocalRead.java b/src/main/java/com/android/tools/r8/ir/code/UninitializedThisLocalRead.java
new file mode 100644
index 0000000..3d8419d
--- /dev/null
+++ b/src/main/java/com/android/tools/r8/ir/code/UninitializedThisLocalRead.java
@@ -0,0 +1,92 @@
+// Copyright (c) 2021, 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.dex.Constants;
+import com.android.tools.r8.errors.Unreachable;
+import com.android.tools.r8.graph.AppView;
+import com.android.tools.r8.graph.ProgramMethod;
+import com.android.tools.r8.ir.conversion.CfBuilder;
+import com.android.tools.r8.ir.conversion.DexBuilder;
+import com.android.tools.r8.ir.optimize.DeadCodeRemover.DeadInstructionResult;
+import com.android.tools.r8.ir.optimize.Inliner.ConstraintWithTarget;
+import com.android.tools.r8.ir.optimize.InliningConstraints;
+
+/**
+ * To preserve stack-map table information regarding uninitializedThis flags the
+ * UninitializedThisLocalRead is added to every instance-initializer's exits to fake a "read" of
+ * this. For more information, see b/183285081
+ */
+public class UninitializedThisLocalRead extends Instruction {
+
+ private static final String ERROR_MESSAGE =
+ "Unexpected attempt to emit/use UninitializedThisLocalRead.";
+
+ public UninitializedThisLocalRead(Value inValue) {
+ super(null, inValue);
+ }
+
+ @Override
+ public int opcode() {
+ return Opcodes.UNINITIALIZED_THIS_LOCAL_READ;
+ }
+
+ @Override
+ public <T> T accept(InstructionVisitor<T> visitor) {
+ throw new Unreachable(ERROR_MESSAGE);
+ }
+
+ @Override
+ public void buildDex(DexBuilder builder) {
+ throw new Unreachable(ERROR_MESSAGE);
+ }
+
+ @Override
+ public void buildCf(CfBuilder builder) {
+ throw new Unreachable(ERROR_MESSAGE);
+ }
+
+ @Override
+ public boolean identicalNonValueNonPositionParts(Instruction other) {
+ throw new Unreachable(ERROR_MESSAGE);
+ }
+
+ @Override
+ public boolean instructionMayTriggerMethodInvocation(AppView<?> appView, ProgramMethod context) {
+ return false;
+ }
+
+ @Override
+ public int maxInValueRegister() {
+ return Constants.U16BIT_MAX;
+ }
+
+ @Override
+ public int maxOutValueRegister() {
+ return Constants.U16BIT_MAX;
+ }
+
+ @Override
+ public ConstraintWithTarget inliningConstraint(
+ InliningConstraints inliningConstraints, ProgramMethod context) {
+ throw new Unreachable(ERROR_MESSAGE);
+ }
+
+ @Override
+ public void insertLoadAndStores(InstructionListIterator it, LoadStoreHelper helper) {
+ // Non-materializing so no stack values are needed.
+ }
+
+ @Override
+ public DeadInstructionResult canBeDeadCode(AppView<?> appView, IRCode code) {
+ throw new Unreachable(ERROR_MESSAGE);
+ }
+
+ @Override
+ public boolean hasInvariantOutType() {
+ return true;
+ }
+}
diff --git a/src/main/java/com/android/tools/r8/ir/conversion/CfBuilder.java b/src/main/java/com/android/tools/r8/ir/conversion/CfBuilder.java
index 6595192..0d7ad7f 100644
--- a/src/main/java/com/android/tools/r8/ir/conversion/CfBuilder.java
+++ b/src/main/java/com/android/tools/r8/ir/conversion/CfBuilder.java
@@ -45,6 +45,7 @@
import com.android.tools.r8.ir.code.Position;
import com.android.tools.r8.ir.code.StackValue;
import com.android.tools.r8.ir.code.StackValues;
+import com.android.tools.r8.ir.code.UninitializedThisLocalRead;
import com.android.tools.r8.ir.code.Value;
import com.android.tools.r8.ir.code.Xor;
import com.android.tools.r8.ir.optimize.CodeRewriter;
@@ -52,6 +53,8 @@
import com.android.tools.r8.ir.optimize.PeepholeOptimizer;
import com.android.tools.r8.ir.optimize.PhiOptimizations;
import com.android.tools.r8.ir.optimize.peepholes.BasicBlockMuncher;
+import com.android.tools.r8.utils.WorkList;
+import com.google.common.collect.Sets;
import it.unimi.dsi.fastutil.ints.Int2ReferenceAVLTreeMap;
import it.unimi.dsi.fastutil.ints.Int2ReferenceMap;
import it.unimi.dsi.fastutil.ints.Int2ReferenceMap.Entry;
@@ -59,9 +62,11 @@
import it.unimi.dsi.fastutil.ints.Int2ReferenceSortedMap;
import java.util.ArrayDeque;
import java.util.ArrayList;
+import java.util.Collections;
import java.util.Deque;
import java.util.HashMap;
import java.util.HashSet;
+import java.util.LinkedList;
import java.util.List;
import java.util.ListIterator;
import java.util.Map;
@@ -148,8 +153,19 @@
}
}
assert code.isConsistentSSA();
+ // Insert reads for uninitialized read blocks to ensure correct stack maps.
+ Set<UninitializedThisLocalRead> uninitializedThisLocalReads =
+ insertUninitializedThisLocalReads();
registerAllocator = new CfRegisterAllocator(appView, code, typeVerificationHelper);
registerAllocator.allocateRegisters();
+ // Remove any uninitializedThisLocalRead now that the register allocation will preserve this
+ // for instance initializers.
+ if (!uninitializedThisLocalReads.isEmpty()) {
+ for (UninitializedThisLocalRead uninitializedThisLocalRead : uninitializedThisLocalReads) {
+ uninitializedThisLocalRead.getBlock().removeInstruction(uninitializedThisLocalRead);
+ }
+ }
+
loadStoreHelper.insertPhiMoves(registerAllocator);
for (int i = 0; i < PEEPHOLE_OPTIMIZATION_PASSES; i++) {
@@ -168,6 +184,27 @@
return code;
}
+ private Set<UninitializedThisLocalRead> insertUninitializedThisLocalReads() {
+ if (!method.isInstanceInitializer()) {
+ return Collections.emptySet();
+ }
+ // Find all non-normal exit blocks.
+ Set<UninitializedThisLocalRead> uninitializedThisLocalReads = Sets.newIdentityHashSet();
+ for (BasicBlock exitBlock : code.blocks) {
+ if (exitBlock.exit().isThrow() && !exitBlock.hasCatchHandlers()) {
+ LinkedList<Instruction> instructions = exitBlock.getInstructions();
+ Instruction throwing = instructions.removeLast();
+ assert throwing.isThrow();
+ UninitializedThisLocalRead read = new UninitializedThisLocalRead(code.getThis());
+ uninitializedThisLocalReads.add(read);
+ read.setBlock(exitBlock);
+ instructions.addLast(read);
+ instructions.addLast(throwing);
+ }
+ }
+ return uninitializedThisLocalReads;
+ }
+
private static boolean verifyInvokeInterface(CfCode code, AppView<?> appView) {
if (appView.options().testing.allowInvokeErrors) {
return true;
@@ -590,31 +627,32 @@
throw new Unreachable("Unexpected type info: " + typeInfo);
}
BasicBlock definitionBlock = definition.getBlock();
- Set<BasicBlock> visited = new HashSet<>();
- Deque<BasicBlock> toVisit = new ArrayDeque<>();
+ assert definitionBlock != liveBlock;
+ Set<BasicBlock> initializerBlocks = new HashSet<>();
List<InvokeDirect> valueInitializers =
definition.isArgument() ? thisInitializers : initializers.get(definition.asNewInstance());
for (InvokeDirect initializer : valueInitializers) {
BasicBlock initializerBlock = initializer.getBlock();
if (initializerBlock == liveBlock) {
+ // We assume that we are not initializing an object twice.
return res;
}
- if (initializerBlock != definitionBlock && visited.add(initializerBlock)) {
- toVisit.addLast(initializerBlock);
+ initializerBlocks.add(initializerBlock);
+ }
+ // If the live-block has a predecessor that is an initializer-block the value is initialized,
+ // otherwise it is not.
+ WorkList<BasicBlock> findInitWorkList =
+ WorkList.newEqualityWorkList(liveBlock.getPredecessors());
+ while (findInitWorkList.hasNext()) {
+ BasicBlock predecessor = findInitWorkList.next();
+ if (initializerBlocks.contains(predecessor)) {
+ return null;
+ }
+ if (predecessor != definitionBlock) {
+ findInitWorkList.addIfNotSeen(predecessor.getPredecessors());
}
}
- while (!toVisit.isEmpty()) {
- BasicBlock block = toVisit.removeLast();
- for (BasicBlock predecessor : block.getPredecessors()) {
- if (predecessor == liveBlock) {
- return res;
- }
- if (predecessor != definitionBlock && visited.add(predecessor)) {
- toVisit.addLast(predecessor);
- }
- }
- }
- return null;
+ return res;
}
private void emitLabel(CfLabel label) {
diff --git a/src/test/java/com/android/tools/r8/cf/StackMapFlagThisUninitTest.java b/src/test/java/com/android/tools/r8/cf/StackMapFlagThisUninitTest.java
index b3f7fe4..977d7c6 100644
--- a/src/test/java/com/android/tools/r8/cf/StackMapFlagThisUninitTest.java
+++ b/src/test/java/com/android/tools/r8/cf/StackMapFlagThisUninitTest.java
@@ -51,11 +51,7 @@
.addProgramClassFileData(ADump.dump())
.addKeepAllClassesRule()
.run(parameters.getRuntime(), Main.class)
- // TODO(b/166738818): We should generate the correct stack map entry.
- .assertFailureWithErrorThatMatches(
- containsString(
- "Exception in thread \"main\" java.lang.VerifyError: Inconsistent stackmap frames"
- + " at branch target 22"));
+ .assertSuccessWithOutputLines("foo");
}
public static class Main {
diff --git a/src/test/java/com/android/tools/r8/cf/StackMapForThrowingInitializerTest.java b/src/test/java/com/android/tools/r8/cf/StackMapForThrowingInitializerTest.java
index bea206e..e7a8f4f 100644
--- a/src/test/java/com/android/tools/r8/cf/StackMapForThrowingInitializerTest.java
+++ b/src/test/java/com/android/tools/r8/cf/StackMapForThrowingInitializerTest.java
@@ -4,8 +4,6 @@
package com.android.tools.r8.cf;
-import static org.hamcrest.CoreMatchers.containsString;
-
import com.android.tools.r8.NeverClassInline;
import com.android.tools.r8.NeverInline;
import com.android.tools.r8.TestBase;
@@ -65,9 +63,7 @@
.enableNeverClassInliningAnnotations()
.setMinApi(parameters.getApiLevel())
.run(parameters.getRuntime(), Main.class, EXPECTED)
- // TODO(b/183285081): Should not have verification errors.
- .assertFailureWithErrorThatMatches(
- containsString("java.lang.VerifyError: Inconsistent stackmap frames at branch target"));
+ .assertSuccessWithOutputLines(EXPECTED);
}
@NeverClassInline
