Revert "Add UninitializedThisLocalRead to instance initializer exits"
This reverts commit 22d90164fc238c5340917961b94af9866e265f62.
Reason for revert: Breaking the bots
Change-Id: I226327b78b1265a55b9d9f902074096c6876825c
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 51b19d8e..a3dc92b 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,5 +73,4 @@
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
deleted file mode 100644
index e3c3d3d..0000000
--- a/src/main/java/com/android/tools/r8/ir/code/UninitializedThisLocalRead.java
+++ /dev/null
@@ -1,102 +0,0 @@
-// 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) {
- return visitor.visit(this);
- }
-
- @Override
- public boolean isUninitializedThisLocalRead() {
- return true;
- }
-
- @Override
- public UninitializedThisLocalRead asUninitializedThisLocalRead() {
- return this;
- }
-
- @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) {
- return other.isUninitializedThisLocalRead();
- }
-
- @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 7d332f0..682ca02 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,7 +45,6 @@
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;
@@ -53,8 +52,6 @@
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;
@@ -62,11 +59,9 @@
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;
@@ -153,19 +148,8 @@
}
}
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++) {
@@ -184,27 +168,6 @@
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;
@@ -627,32 +590,31 @@
throw new Unreachable("Unexpected type info: " + typeInfo);
}
BasicBlock definitionBlock = definition.getBlock();
- assert definitionBlock != liveBlock;
- Set<BasicBlock> initializerBlocks = new HashSet<>();
+ Set<BasicBlock> visited = new HashSet<>();
+ Deque<BasicBlock> toVisit = new ArrayDeque<>();
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;
}
- 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());
+ if (initializerBlock != definitionBlock && visited.add(initializerBlock)) {
+ toVisit.addLast(initializerBlock);
}
}
- return res;
+ 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;
}
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 977d7c6..b3f7fe4 100644
--- a/src/test/java/com/android/tools/r8/cf/StackMapFlagThisUninitTest.java
+++ b/src/test/java/com/android/tools/r8/cf/StackMapFlagThisUninitTest.java
@@ -51,7 +51,11 @@
.addProgramClassFileData(ADump.dump())
.addKeepAllClassesRule()
.run(parameters.getRuntime(), Main.class)
- .assertSuccessWithOutputLines("foo");
+ // 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"));
}
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 e7a8f4f..bea206e 100644
--- a/src/test/java/com/android/tools/r8/cf/StackMapForThrowingInitializerTest.java
+++ b/src/test/java/com/android/tools/r8/cf/StackMapForThrowingInitializerTest.java
@@ -4,6 +4,8 @@
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;
@@ -63,7 +65,9 @@
.enableNeverClassInliningAnnotations()
.setMinApi(parameters.getApiLevel())
.run(parameters.getRuntime(), Main.class, EXPECTED)
- .assertSuccessWithOutputLines(EXPECTED);
+ // TODO(b/183285081): Should not have verification errors.
+ .assertFailureWithErrorThatMatches(
+ containsString("java.lang.VerifyError: Inconsistent stackmap frames at branch target"));
}
@NeverClassInline
