8073866: Fix for 8064703 is not sufficient

Side effects between allocation and arraycopy can be reexecuted, unreachable uninitialized array can be seen by GCs

Reviewed-by: kvn, vlivanov

hotspot/src/share/vm/opto/graphKit.cpp

@@ -2795,18 +2795,15 @@ Node* GraphKit::maybe_cast_profiled_receiver(Node* not_null_obj,
  */
 Node* GraphKit::maybe_cast_profiled_obj(Node* obj,
                                         ciKlass* type,
-                                        bool not_null,
+                                        bool not_null) {
-                                        SafePointNode* sfpt) {
   // type == NULL if profiling tells us this object is always null
   if (type != NULL) {
     Deoptimization::DeoptReason class_reason = Deoptimization::Reason_speculate_class_check;
     Deoptimization::DeoptReason null_reason = Deoptimization::Reason_speculate_null_check;
-    ciMethod* trap_method = (sfpt == NULL) ? method() : sfpt->jvms()->method();
-    int trap_bci = (sfpt == NULL) ? bci() : sfpt->jvms()->bci();
 
     if (!too_many_traps(null_reason) && !too_many_recompiles(null_reason) &&
-        !C->too_many_traps(trap_method, trap_bci, class_reason) &&
+        !too_many_traps(class_reason) &&
-        !C->too_many_recompiles(trap_method, trap_bci, class_reason)) {
+        !too_many_recompiles(class_reason)) {
       Node* not_null_obj = NULL;
       // not_null is true if we know the object is not null and
       // there's no need for a null check
@@ -2822,12 +2819,7 @@ Node* GraphKit::maybe_cast_profiled_obj(Node* obj,
       ciKlass* exact_kls = type;
       Node* slow_ctl  = type_check_receiver(exact_obj, exact_kls, 1.0,
                                             &exact_obj);
-      if (sfpt != NULL) {
+      {
-        GraphKit kit(sfpt->jvms());
-        PreserveJVMState pjvms(&kit);
-        kit.set_control(slow_ctl);
-        kit.uncommon_trap_exact(class_reason, Deoptimization::Action_maybe_recompile);
-      } else {
         PreserveJVMState pjvms(this);
         set_control(slow_ctl);
         uncommon_trap_exact(class_reason, Deoptimization::Action_maybe_recompile);

hotspot/src/share/vm/opto/graphKit.hpp

@@ -409,8 +409,7 @@ class GraphKit : public Phase {
   // Cast obj to type and emit guard unless we had too many traps here already
   Node* maybe_cast_profiled_obj(Node* obj,
                                 ciKlass* type,
-                                bool not_null = false,
+                                bool not_null = false);
-                                SafePointNode* sfpt = NULL);
 
   // Cast obj to not-null on this path
   Node* cast_not_null(Node* obj, bool do_replace_in_map = true);

hotspot/src/share/vm/opto/library_call.cpp

@@ -262,6 +262,9 @@ class LibraryCallKit : public GraphKit {
   bool inline_arraycopy();
   AllocateArrayNode* tightly_coupled_allocation(Node* ptr,
                                                 RegionNode* slow_region);
+  JVMState* arraycopy_restore_alloc_state(AllocateArrayNode* alloc, int& saved_reexecute_sp);
+  void arraycopy_move_allocation_here(AllocateArrayNode* alloc, Node* dest, JVMState* saved_jvms, int saved_reexecute_sp);
+
   typedef enum { LS_xadd, LS_xchg, LS_cmpxchg } LoadStoreKind;
   bool inline_unsafe_load_store(BasicType type,  LoadStoreKind kind);
   bool inline_unsafe_ordered_store(BasicType type);
@@ -4674,6 +4677,141 @@ bool LibraryCallKit::inline_native_clone(bool is_virtual) {
   return true;
 }
 
+// If we have a tighly coupled allocation, the arraycopy may take care
+// of the array initialization. If one of the guards we insert between
+// the allocation and the arraycopy causes a deoptimization, an
+// unitialized array will escape the compiled method. To prevent that
+// we set the JVM state for uncommon traps between the allocation and
+// the arraycopy to the state before the allocation so, in case of
+// deoptimization, we'll reexecute the allocation and the
+// initialization.
+JVMState* LibraryCallKit::arraycopy_restore_alloc_state(AllocateArrayNode* alloc, int& saved_reexecute_sp) {
+  if (alloc != NULL) {
+    ciMethod* trap_method = alloc->jvms()->method();
+    int trap_bci = alloc->jvms()->bci();
+
+    if (!C->too_many_traps(trap_method, trap_bci, Deoptimization::Reason_intrinsic) &
+          !C->too_many_traps(trap_method, trap_bci, Deoptimization::Reason_null_check)) {
+      // Make sure there's no store between the allocation and the
+      // arraycopy otherwise visible side effects could be rexecuted
+      // in case of deoptimization and cause incorrect execution.
+      bool no_interfering_store = true;
+      Node* mem = alloc->in(TypeFunc::Memory);
+      if (mem->is_MergeMem()) {
+        for (MergeMemStream mms(merged_memory(), mem->as_MergeMem()); mms.next_non_empty2(); ) {
+          Node* n = mms.memory();
+          if (n != mms.memory2() && !(n->is_Proj() && n->in(0) == alloc->initialization())) {
+            assert(n->is_Store(), "what else?");
+            no_interfering_store = false;
+            break;
+          }
+        }
+      } else {
+        for (MergeMemStream mms(merged_memory()); mms.next_non_empty(); ) {
+          Node* n = mms.memory();
+          if (n != mem && !(n->is_Proj() && n->in(0) == alloc->initialization())) {
+            assert(n->is_Store(), "what else?");
+            no_interfering_store = false;
+            break;
+          }
+        }
+      }
+
+      if (no_interfering_store) {
+        JVMState* old_jvms = alloc->jvms()->clone_shallow(C);
+        uint size = alloc->req();
+        SafePointNode* sfpt = new SafePointNode(size, old_jvms);
+        old_jvms->set_map(sfpt);
+        for (uint i = 0; i < size; i++) {
+          sfpt->init_req(i, alloc->in(i));
+        }
+        // re-push array length for deoptimization
+        sfpt->ins_req(old_jvms->stkoff() + old_jvms->sp(), alloc->in(AllocateNode::ALength));
+        old_jvms->set_sp(old_jvms->sp()+1);
+        old_jvms->set_monoff(old_jvms->monoff()+1);
+        old_jvms->set_scloff(old_jvms->scloff()+1);
+        old_jvms->set_endoff(old_jvms->endoff()+1);
+        old_jvms->set_should_reexecute(true);
+
+        sfpt->set_i_o(map()->i_o());
+        sfpt->set_memory(map()->memory());
+        sfpt->set_control(map()->control());
+
+        JVMState* saved_jvms = jvms();
+        saved_reexecute_sp = _reexecute_sp;
+
+        set_jvms(sfpt->jvms());
+        _reexecute_sp = jvms()->sp();
+
+        return saved_jvms;
+      }
+    }
+  }
+  return NULL;
+}
+
+// In case of a deoptimization, we restart execution at the
+// allocation, allocating a new array. We would leave an uninitialized
+// array in the heap that GCs wouldn't expect. Move the allocation
+// after the traps so we don't allocate the array if we
+// deoptimize. This is possible because tightly_coupled_allocation()
+// guarantees there's no observer of the allocated array at this point
+// and the control flow is simple enough.
+void LibraryCallKit::arraycopy_move_allocation_here(AllocateArrayNode* alloc, Node* dest, JVMState* saved_jvms, int saved_reexecute_sp) {
+  if (saved_jvms != NULL) {
+    assert(alloc != NULL, "only with a tightly coupled allocation");
+    // restore JVM state to the state at the arraycopy
+    saved_jvms->map()->set_control(map()->control());
+    assert(saved_jvms->map()->memory() == map()->memory(), "memory state changed?");
+    assert(saved_jvms->map()->i_o() == map()->i_o(), "IO state changed?");
+    // If we've improved the types of some nodes (null check) while
+    // emitting the guards, propagate them to the current state
+    map()->replaced_nodes().apply(saved_jvms->map());
+    set_jvms(saved_jvms);
+    _reexecute_sp = saved_reexecute_sp;
+
+    // Remove the allocation from above the guards
+    CallProjections callprojs;
+    alloc->extract_projections(&callprojs, true);
+    InitializeNode* init = alloc->initialization();
+    Node* alloc_mem = alloc->in(TypeFunc::Memory);
+    C->gvn_replace_by(callprojs.fallthrough_ioproj, alloc->in(TypeFunc::I_O));
+    C->gvn_replace_by(init->proj_out(TypeFunc::Memory), alloc_mem);
+    C->gvn_replace_by(init->proj_out(TypeFunc::Control), alloc->in(0));
+
+    // move the allocation here (after the guards)
+    _gvn.hash_delete(alloc);
+    alloc->set_req(TypeFunc::Control, control());
+    alloc->set_req(TypeFunc::I_O, i_o());
+    Node *mem = reset_memory();
+    set_all_memory(mem);
+    alloc->set_req(TypeFunc::Memory, mem);
+    set_control(init->proj_out(TypeFunc::Control));
+    set_i_o(callprojs.fallthrough_ioproj);
+
+    // Update memory as done in GraphKit::set_output_for_allocation()
+    const TypeInt* length_type = _gvn.find_int_type(alloc->in(AllocateNode::ALength));
+    const TypeOopPtr* ary_type = _gvn.type(alloc->in(AllocateNode::KlassNode))->is_klassptr()->as_instance_type();
+    if (ary_type->isa_aryptr() && length_type != NULL) {
+      ary_type = ary_type->is_aryptr()->cast_to_size(length_type);
+    }
+    const TypePtr* telemref = ary_type->add_offset(Type::OffsetBot);
+    int            elemidx  = C->get_alias_index(telemref);
+    set_memory(init->proj_out(TypeFunc::Memory), Compile::AliasIdxRaw);
+    set_memory(init->proj_out(TypeFunc::Memory), elemidx);
+
+    Node* allocx = _gvn.transform(alloc);
+    assert(allocx == alloc, "where has the allocation gone?");
+    assert(dest->is_CheckCastPP(), "not an allocation result?");
+
+    _gvn.hash_delete(dest);
+    dest->set_req(0, control());
+    Node* destx = _gvn.transform(dest);
+    assert(destx == dest, "where has the allocation result gone?");
+  }
+}
+
+
 //------------------------------inline_arraycopy-----------------------
 // public static native void java.lang.System.arraycopy(Object src,  int  srcPos,
 //                                                      Object dest, int destPos,
@@ -4686,6 +4824,19 @@ bool LibraryCallKit::inline_arraycopy() {
   Node* dest_offset = argument(3);  // type: int
   Node* length      = argument(4);  // type: int
 
+
+  // Check for allocation before we add nodes that would confuse
+  // tightly_coupled_allocation()
+  AllocateArrayNode* alloc = tightly_coupled_allocation(dest, NULL);
+
+  int saved_reexecute_sp = -1;
+  JVMState* saved_jvms = arraycopy_restore_alloc_state(alloc, saved_reexecute_sp);
+  // See arraycopy_restore_alloc_state() comment
+  // if alloc == NULL we don't have to worry about a tightly coupled allocation so we can emit all needed guards
+  // if saved_jvms != NULL (then alloc != NULL) then we can handle guards and a tightly coupled allocation
+  // if saved_jvms == NULL and alloc != NULL, we can’t emit any guards
+  bool can_emit_guards = (alloc == NULL || saved_jvms != NULL);
+
   // The following tests must be performed
   // (1) src and dest are arrays.
   // (2) src and dest arrays must have elements of the same BasicType
@@ -4699,42 +4850,20 @@ bool LibraryCallKit::inline_arraycopy() {
 
   // (3) src and dest must not be null.
   // always do this here because we need the JVM state for uncommon traps
-  src  = null_check(src,  T_ARRAY);
+  Node* null_ctl = top();
+  src  = saved_jvms != NULL ? null_check_oop(src, &null_ctl, true, true) : null_check(src,  T_ARRAY);
+  assert(null_ctl->is_top(), "no null control here");
   dest = null_check(dest, T_ARRAY);
 
-  // Check for allocation before we add nodes that would confuse
+  if (!can_emit_guards) {
-  // tightly_coupled_allocation()
+    // if saved_jvms == NULL and alloc != NULL, we don't emit any
-  AllocateArrayNode* alloc = tightly_coupled_allocation(dest, NULL);
+    // guards but the arraycopy node could still take advantage of a
-
+    // tightly allocated allocation. tightly_coupled_allocation() is
-  ciMethod* trap_method = method();
+    // called again to make sure it takes the null check above into
-  int trap_bci = bci();
+    // account: the null check is mandatory and if it caused an
-  SafePointNode* sfpt = NULL;
+    // uncommon trap to be emitted then the allocation can't be
-  if (alloc != NULL) {
+    // considered tightly coupled in this context.
-    // The JVM state for uncommon traps between the allocation and
+    alloc = tightly_coupled_allocation(dest, NULL);
-    // arraycopy is set to the state before the allocation: if the
-    // initialization is performed by the array copy, we don't want to
-    // go back to the interpreter with an unitialized array.
-    JVMState* old_jvms = alloc->jvms();
-    JVMState* jvms = old_jvms->clone_shallow(C);
-    uint size = alloc->req();
-    sfpt = new SafePointNode(size, jvms);
-    jvms->set_map(sfpt);
-    for (uint i = 0; i < size; i++) {
-      sfpt->init_req(i, alloc->in(i));
-    }
-    // re-push array length for deoptimization
-    sfpt->ins_req(jvms->stkoff() + jvms->sp(), alloc->in(AllocateNode::ALength));
-    jvms->set_sp(jvms->sp()+1);
-    jvms->set_monoff(jvms->monoff()+1);
-    jvms->set_scloff(jvms->scloff()+1);
-    jvms->set_endoff(jvms->endoff()+1);
-    jvms->set_should_reexecute(true);
-
-    sfpt->set_i_o(map()->i_o());
-    sfpt->set_memory(map()->memory());
-
-    trap_method = jvms->method();
-    trap_bci = jvms->bci();
   }
 
   bool validated = false;
@@ -4753,7 +4882,7 @@ bool LibraryCallKit::inline_arraycopy() {
   // Is the type for dest from speculation?
   bool dest_spec = false;
 
-  if (!has_src || !has_dest) {
+  if ((!has_src || !has_dest) && can_emit_guards) {
     // We don't have sufficient type information, let's see if
     // speculative types can help. We need to have types for both src
     // and dest so that it pays off.
@@ -4782,7 +4911,7 @@ bool LibraryCallKit::inline_arraycopy() {
     if (could_have_src && could_have_dest) {
       // This is going to pay off so emit the required guards
       if (!has_src) {
-        src = maybe_cast_profiled_obj(src, src_k, true, sfpt);
+        src = maybe_cast_profiled_obj(src, src_k, true);
         src_type  = _gvn.type(src);
         top_src  = src_type->isa_aryptr();
         has_src = (top_src != NULL && top_src->klass() != NULL);
@@ -4798,7 +4927,7 @@ bool LibraryCallKit::inline_arraycopy() {
     }
   }
 
-  if (has_src && has_dest) {
+  if (has_src && has_dest && can_emit_guards) {
     BasicType src_elem  = top_src->klass()->as_array_klass()->element_type()->basic_type();
     BasicType dest_elem = top_dest->klass()->as_array_klass()->element_type()->basic_type();
     if (src_elem  == T_ARRAY)  src_elem  = T_OBJECT;
@@ -4830,7 +4959,7 @@ bool LibraryCallKit::inline_arraycopy() {
       if (could_have_src && could_have_dest) {
         // If we can have both exact types, emit the missing guards
         if (could_have_src && !src_spec) {
-          src = maybe_cast_profiled_obj(src, src_k, true, sfpt);
+          src = maybe_cast_profiled_obj(src, src_k, true);
         }
         if (could_have_dest && !dest_spec) {
           dest = maybe_cast_profiled_obj(dest, dest_k, true);
@@ -4839,7 +4968,16 @@ bool LibraryCallKit::inline_arraycopy() {
     }
   }
 
-  if (!C->too_many_traps(trap_method, trap_bci, Deoptimization::Reason_intrinsic) && !src->is_top() && !dest->is_top()) {
+  ciMethod* trap_method = method();
+  int trap_bci = bci();
+  if (saved_jvms != NULL) {
+    trap_method = alloc->jvms()->method();
+    trap_bci = alloc->jvms()->bci();
+  }
+
+  if (!C->too_many_traps(trap_method, trap_bci, Deoptimization::Reason_intrinsic) &&
+      can_emit_guards &&
+      !src->is_top() && !dest->is_top()) {
     // validate arguments: enables transformation the ArrayCopyNode
     validated = true;
 
@@ -4875,28 +5013,13 @@ bool LibraryCallKit::inline_arraycopy() {
     Node* not_subtype_ctrl = gen_subtype_check(src_klass, dest_klass);
 
     if (not_subtype_ctrl != top()) {
-      if (sfpt != NULL) {
+      PreserveJVMState pjvms(this);
-        GraphKit kit(sfpt->jvms());
+      set_control(not_subtype_ctrl);
-        PreserveJVMState pjvms(&kit);
+      uncommon_trap(Deoptimization::Reason_intrinsic,
-        kit.set_control(not_subtype_ctrl);
+                    Deoptimization::Action_make_not_entrant);
-        kit.uncommon_trap(Deoptimization::Reason_intrinsic,
+      assert(stopped(), "Should be stopped");
-                          Deoptimization::Action_make_not_entrant);
-        assert(kit.stopped(), "Should be stopped");
-      } else {
-        PreserveJVMState pjvms(this);
-        set_control(not_subtype_ctrl);
-        uncommon_trap(Deoptimization::Reason_intrinsic,
-                      Deoptimization::Action_make_not_entrant);
-        assert(stopped(), "Should be stopped");
-      }
     }
-    if (sfpt != NULL) {
+    {
-      GraphKit kit(sfpt->jvms());
-      kit.set_control(_gvn.transform(slow_region));
-      kit.uncommon_trap(Deoptimization::Reason_intrinsic,
-                        Deoptimization::Action_make_not_entrant);
-      assert(kit.stopped(), "Should be stopped");
-    } else {
       PreserveJVMState pjvms(this);
       set_control(_gvn.transform(slow_region));
       uncommon_trap(Deoptimization::Reason_intrinsic,
@@ -4905,6 +5028,8 @@ bool LibraryCallKit::inline_arraycopy() {
     }
   }
 
+  arraycopy_move_allocation_here(alloc, dest, saved_jvms, saved_reexecute_sp);
+
   if (stopped()) {
     return true;
   }

hotspot/test/compiler/arraycopy/TestArrayCopyBadReexec.java

@@ -0,0 +1,67 @@
+/*
+ * Copyright (c) 2015, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/*
+ * @test
+ * @bug 8073866
+ * @summary Fix for 8064703 may also cause stores between the allocation and arraycopy to be rexecuted after a deoptimization
+ * @run main/othervm -XX:-BackgroundCompilation -XX:-UseOnStackReplacement TestArrayCopyBadReexec
+ *
+ */
+
+public class TestArrayCopyBadReexec {
+
+    static int val;
+
+    static int[] m1(int[] src, int l) {
+        if (src == null) {
+            return null;
+        }
+        int[] dest = new int[10];
+        val++;
+        try {
+            System.arraycopy(src, 0, dest, 0, l);
+        } catch (IndexOutOfBoundsException npe) {
+        }
+        return dest;
+    }
+
+    static public void main(String[] args) {
+        int[] src = new int[10];
+        int[] res = null;
+        boolean success = true;
+
+        for (int i = 0; i < 20000; i++) {
+            m1(src, 10);
+        }
+
+        int val_before = val;
+
+        m1(src, -1);
+
+        if (val - val_before != 1) {
+            System.out.println("Bad increment: " + (val - val_before));
+            throw new RuntimeException("Test failed");
+        }
+    }
+}

hotspot/test/compiler/arraycopy/TestArrayCopyNoInit.java

@@ -76,7 +76,7 @@ public class TestArrayCopyNoInit {
     static TestArrayCopyNoInit[] m5(Object[] src) {
         Object tmp = src[0];
         TestArrayCopyNoInit[] dest = new TestArrayCopyNoInit[10];
-        System.arraycopy(src, 0, dest, 0, 0);
+        System.arraycopy(src, 0, dest, 0, 10);
         return dest;
     }
 
@@ -110,7 +110,7 @@ public class TestArrayCopyNoInit {
     static H[] m6(Object[] src) {
         Object tmp = src[0];
         H[] dest = new H[10];
-        System.arraycopy(src, 0, dest, 0, 0);
+        System.arraycopy(src, 0, dest, 0, 10);
         return dest;
     }
 

hotspot/test/compiler/arraycopy/TestArrayCopyNoInitDeopt.java

@@ -29,7 +29,8 @@
  * @build TestArrayCopyNoInitDeopt
  * @run main ClassFileInstaller sun.hotspot.WhiteBox
  * @run main ClassFileInstaller com.oracle.java.testlibrary.Platform
- * @run main/othervm -Xmixed -Xbootclasspath/a:. -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI
+ * @run main/othervm -Xmixed -XX:Tier4InvocationThreshold=5000 -XX:Tier3InvokeNotifyFreqLog=10
+ *                    -Xbootclasspath/a:. -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI
  *                   -XX:-BackgroundCompilation -XX:-UseOnStackReplacement -XX:TypeProfileLevel=020
  *                   TestArrayCopyNoInitDeopt
  *