Fix a bug when creating a HDeoptimization instruction.

We need to copy the environment, instead of just pointing
to an existing one. Otherwise, if the instruction that initially
holds the environemnt gets removed from the graph, any update
to an instruction in that environment will not be reflected in it.

bug:20058506

Change-Id: I2a62476d0851ecbc3707c0da395d8502ee437422

7 files changed, 64 insertions, 4 deletions

diff --git a/compiler/optimizing/bounds_check_elimination.cc b/compiler/optimizing/bounds_check_elimination.cc
index 01f7e91..dce02f7 100644
--- a/compiler/optimizing/bounds_check_elimination.cc
+++ b/compiler/optimizing/bounds_check_elimination.cc
@@ -1011,7 +1011,7 @@ class BCEVisitor : public HGraphVisitor {
         HDeoptimize(cond, bounds_check->GetDexPc());
     block->InsertInstructionBefore(cond, bounds_check);
     block->InsertInstructionBefore(deoptimize, bounds_check);
-    deoptimize->SetEnvironment(bounds_check->GetEnvironment());
+    deoptimize->CopyEnvironmentFrom(bounds_check->GetEnvironment());
   }
 
   void AddComparesWithDeoptimization(HBasicBlock* block) {
diff --git a/compiler/optimizing/nodes.h b/compiler/optimizing/nodes.h
index 6827cd0..f764eb4 100644
--- a/compiler/optimizing/nodes.h
+++ b/compiler/optimizing/nodes.h
@@ -1192,7 +1192,17 @@ class HInstruction : public ArenaObject<kArenaAllocMisc> {
 
   bool HasEnvironment() const { return environment_ != nullptr; }
   HEnvironment* GetEnvironment() const { return environment_; }
-  void SetEnvironment(HEnvironment* environment) { environment_ = environment; }
+  // Set the `environment_` field. Raw because this method does not
+  // update the uses lists.
+  void SetRawEnvironment(HEnvironment* environment) { environment_ = environment; }
+
+  // Set the environment of this instruction, copying it from `environment`. While
+  // copying, the uses lists are being updated.
+  void CopyEnvironmentFrom(HEnvironment* environment) {
+    ArenaAllocator* allocator = GetBlock()->GetGraph()->GetArena();
+    environment_ = new (allocator) HEnvironment(allocator, environment->Size());
+    environment_->CopyFrom(environment);
+  }
 
   // Returns the number of entries in the environment. Typically, that is the
   // number of dex registers in a method. It could be more in case of inlining.
diff --git a/compiler/optimizing/nodes_test.cc b/compiler/optimizing/nodes_test.cc
index 4cf22d3..4e83ce5 100644
--- a/compiler/optimizing/nodes_test.cc
+++ b/compiler/optimizing/nodes_test.cc
@@ -50,7 +50,7 @@ TEST(Node, RemoveInstruction) {
   exit_block->AddInstruction(new (&allocator) HExit());
 
   HEnvironment* environment = new (&allocator) HEnvironment(&allocator, 1);
-  null_check->SetEnvironment(environment);
+  null_check->SetRawEnvironment(environment);
   environment->SetRawEnvAt(0, parameter);
   parameter->AddEnvUseAt(null_check->GetEnvironment(), 0);
 
diff --git a/compiler/optimizing/ssa_builder.cc b/compiler/optimizing/ssa_builder.cc
index fcc4e69..e154ea4 100644
--- a/compiler/optimizing/ssa_builder.cc
+++ b/compiler/optimizing/ssa_builder.cc
@@ -487,7 +487,7 @@ void SsaBuilder::VisitInstruction(HInstruction* instruction) {
   HEnvironment* environment = new (GetGraph()->GetArena()) HEnvironment(
       GetGraph()->GetArena(), current_locals_->Size());
   environment->CopyFrom(current_locals_);
-  instruction->SetEnvironment(environment);
+  instruction->SetRawEnvironment(environment);
 }
 
 void SsaBuilder::VisitTemporary(HTemporary* temp) {
diff --git a/test/471-deopt-environment/expected.txt b/test/471-deopt-environment/expected.txt
new file mode 100644
index 0000000..e69de29
--- /dev/null
+++ b/test/471-deopt-environment/expected.txt
diff --git a/test/471-deopt-environment/info.txt b/test/471-deopt-environment/info.txt
new file mode 100644
index 0000000..bcb9575
--- /dev/null
+++ b/test/471-deopt-environment/info.txt
@@ -0,0 +1,3 @@
+Regression test for the bounds check elimination pass, which
+uses to generate a HDeoptimization instruction with an
+HEnvironment that does not have the uses lists updated.
diff --git a/test/471-deopt-environment/src/Main.java b/test/471-deopt-environment/src/Main.java
new file mode 100644
index 0000000..5c5080b
--- /dev/null
+++ b/test/471-deopt-environment/src/Main.java
@@ -0,0 +1,47 @@
+/*
+ * Copyright (C) 2015 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+public class Main {
+
+  private static int willInline(int a, int b) {
+    return a & b;
+  }
+
+  static int[] a = new int[4];
+  static int field = 42;
+
+  public static void main(String[] args) throws Exception {
+    // The order of optimizations that would lead to the problem was:
+    // 1) Inlining of `willInline`.
+    // 2) Bounds check elimination inserting a deopt at a[0] and removing the HBoundsCheck.
+    // 3) Instruction simplifier simpilifying the inlined willInline to just `field`.
+    //
+    // At this point, if the environment of the HDeoptimization instruction was
+    // just a pointer to the one in a[0], the uses lists would have not been updated
+    // and the HBoundsCheck being dead code after the HDeoptimization, the simplifcation
+    // at step 3) would not updated that environment.
+    int inEnv = willInline(field, field);
+    int doAdds = a[0] + a[1] + a[2] + a[3];
+
+    if (inEnv != 42) {
+      throw new Error("Expected 42");
+    }
+
+    if (doAdds != 0) {
+      throw new Error("Expected 0");
+    }
+  }
+}