8263361: Incorrect arraycopy stub selected by C2 for SATB collectors

Reviewed-by: eosterlund, vlivanov

Author: Nils Eliasson

src/hotspot/share/opto/library_call.cpp

@@ -4143,8 +4143,8 @@ bool LibraryCallKit::inline_native_clone(bool is_virtual) {
           PreserveJVMState pjvms2(this);
           set_control(is_obja);
           // Generate a direct call to the right arraycopy function(s).
-          Node* alloc = tightly_coupled_allocation(alloc_obj, NULL);
-          ArrayCopyNode* ac = ArrayCopyNode::make(this, true, obj, intcon(0), alloc_obj, intcon(0), obj_length, alloc != NULL, false);
+          // Clones are always tightly coupled.
+          ArrayCopyNode* ac = ArrayCopyNode::make(this, true, obj, intcon(0), alloc_obj, intcon(0), obj_length, true, false);
           ac->set_clone_oop_array();
           Node* n = _gvn.transform(ac);
           assert(n == ac, "cannot disappear");

src/hotspot/share/opto/macroArrayCopy.cpp

@@ -381,8 +381,9 @@ Node* PhaseMacroExpand::generate_arraycopy(ArrayCopyNode *ac, AllocateArrayNode*
     transform_later(slow_region);
   }
 
-  Node* original_dest      = dest;
-  bool  dest_uninitialized = false;
+  Node* original_dest = dest;
+  bool  dest_needs_zeroing   = false;
+  bool  acopy_to_uninitialized = false;
 
   // See if this is the initialization of a newly-allocated array.
   // If so, we will take responsibility here for initializing it to zero.
@@ -394,25 +395,34 @@ Node* PhaseMacroExpand::generate_arraycopy(ArrayCopyNode *ac, AllocateArrayNode*
       && basic_elem_type != T_CONFLICT // avoid corner case
       && !src->eqv_uncast(dest)
       && alloc != NULL
-      && _igvn.find_int_con(alloc->in(AllocateNode::ALength), 1) > 0
-      && alloc->maybe_set_complete(&_igvn)) {
-    // "You break it, you buy it."
-    InitializeNode* init = alloc->initialization();
-    assert(init->is_complete(), "we just did this");
-    init->set_complete_with_arraycopy();
-    assert(dest->is_CheckCastPP(), "sanity");
-    assert(dest->in(0)->in(0) == init, "dest pinned");
-    adr_type = TypeRawPtr::BOTTOM;  // all initializations are into raw memory
-    // From this point on, every exit path is responsible for
-    // initializing any non-copied parts of the object to zero.
-    // Also, if this flag is set we make sure that arraycopy interacts properly
-    // with G1, eliding pre-barriers. See CR 6627983.
-    dest_uninitialized = true;
+      && _igvn.find_int_con(alloc->in(AllocateNode::ALength), 1) > 0) {
+    assert(ac->is_alloc_tightly_coupled(), "sanity");
+    // acopy to uninitialized tightly coupled allocations
+    // needs zeroing outside the copy range
+    // and the acopy itself will be to uninitialized memory
+    acopy_to_uninitialized = true;
+    if (alloc->maybe_set_complete(&_igvn)) {
+      // "You break it, you buy it."
+      InitializeNode* init = alloc->initialization();
+      assert(init->is_complete(), "we just did this");
+      init->set_complete_with_arraycopy();
+      assert(dest->is_CheckCastPP(), "sanity");
+      assert(dest->in(0)->in(0) == init, "dest pinned");
+      adr_type = TypeRawPtr::BOTTOM;  // all initializations are into raw memory
+      // From this point on, every exit path is responsible for
+      // initializing any non-copied parts of the object to zero.
+      // Also, if this flag is set we make sure that arraycopy interacts properly
+      // with G1, eliding pre-barriers. See CR 6627983.
+      dest_needs_zeroing = true;
+    } else {
+      // dest_need_zeroing = false;
+    }
   } else {
-    // No zeroing elimination here.
-    alloc             = NULL;
-    //original_dest   = dest;
-    //dest_uninitialized = false;
+    // No zeroing elimination needed here.
+    alloc                  = NULL;
+    acopy_to_uninitialized = false;
+    //original_dest        = dest;
+    //dest_needs_zeroing   = false;
   }
 
   uint alias_idx = C->get_alias_index(adr_type);
@@ -446,11 +456,11 @@ Node* PhaseMacroExpand::generate_arraycopy(ArrayCopyNode *ac, AllocateArrayNode*
   Node* checked_value   = NULL;
 
   if (basic_elem_type == T_CONFLICT) {
-    assert(!dest_uninitialized, "");
+    assert(!dest_needs_zeroing, "");
     Node* cv = generate_generic_arraycopy(ctrl, &mem,
                                           adr_type,
                                           src, src_offset, dest, dest_offset,
-                                          copy_length, dest_uninitialized);
+                                          copy_length, acopy_to_uninitialized);
     if (cv == NULL)  cv = intcon(-1);  // failure (no stub available)
     checked_control = *ctrl;
     checked_i_o     = *io;
@@ -471,7 +481,7 @@ Node* PhaseMacroExpand::generate_arraycopy(ArrayCopyNode *ac, AllocateArrayNode*
     }
 
     // copy_length is 0.
-    if (dest_uninitialized) {
+    if (dest_needs_zeroing) {
       assert(!local_ctrl->is_top(), "no ctrl?");
       Node* dest_length = alloc->in(AllocateNode::ALength);
       if (copy_length->eqv_uncast(dest_length)
@@ -506,7 +516,7 @@ Node* PhaseMacroExpand::generate_arraycopy(ArrayCopyNode *ac, AllocateArrayNode*
     result_memory->init_req(zero_path, local_mem->memory_at(alias_idx));
   }
 
-  if (!(*ctrl)->is_top() && dest_uninitialized) {
+  if (!(*ctrl)->is_top() && dest_needs_zeroing) {
     // We have to initialize the *uncopied* part of the array to zero.
     // The copy destination is the slice dest[off..off+len].  The other slices
     // are dest_head = dest[0..off] and dest_tail = dest[off+len..dest.length].
@@ -547,7 +557,7 @@ Node* PhaseMacroExpand::generate_arraycopy(ArrayCopyNode *ac, AllocateArrayNode*
         didit = generate_block_arraycopy(&local_ctrl, &local_mem, local_io,
                                          adr_type, basic_elem_type, alloc,
                                          src, src_offset, dest, dest_offset,
-                                         dest_size, dest_uninitialized);
+                                         dest_size, acopy_to_uninitialized);
         if (didit) {
           // Present the results of the block-copying fast call.
           result_region->init_req(bcopy_path, local_ctrl);
@@ -635,7 +645,7 @@ Node* PhaseMacroExpand::generate_arraycopy(ArrayCopyNode *ac, AllocateArrayNode*
                                                 adr_type,
                                                 dest_elem_klass,
                                                 src, src_offset, dest, dest_offset,
-                                                ConvI2X(copy_length), dest_uninitialized);
+                                                ConvI2X(copy_length), acopy_to_uninitialized);
         if (cv == NULL)  cv = intcon(-1);  // failure (no stub available)
         checked_control = local_ctrl;
         checked_i_o     = *io;
@@ -660,11 +670,10 @@ Node* PhaseMacroExpand::generate_arraycopy(ArrayCopyNode *ac, AllocateArrayNode*
     Node* local_ctrl = *ctrl;
     MergeMemNode* local_mem = MergeMemNode::make(mem);
     transform_later(local_mem);
-
     is_partial_array_copy = generate_unchecked_arraycopy(&local_ctrl, &local_mem,
                                                          adr_type, copy_type, disjoint_bases,
                                                          src, src_offset, dest, dest_offset,
-                                                         ConvI2X(copy_length), dest_uninitialized);
+                                                         ConvI2X(copy_length), acopy_to_uninitialized);
 
     // Present the results of the fast call.
     result_region->init_req(fast_path, local_ctrl);
@@ -753,7 +762,7 @@ Node* PhaseMacroExpand::generate_arraycopy(ArrayCopyNode *ac, AllocateArrayNode*
     // Generate the slow path, if needed.
     local_mem->set_memory_at(alias_idx, slow_mem);
 
-    if (dest_uninitialized) {
+    if (dest_needs_zeroing) {
       generate_clear_array(local_ctrl, local_mem,
                            adr_type, dest, basic_elem_type,
                            intcon(0), NULL,

src/hotspot/share/runtime/stubRoutines.cpp

@@ -515,8 +515,8 @@ StubRoutines::select_arraycopy_function(BasicType t, bool aligned, bool disjoint
   name = #xxx_arraycopy; \
   return StubRoutines::xxx_arraycopy(); }
 
-#define RETURN_STUB_PARM(xxx_arraycopy, parm) {           \
-  name = #xxx_arraycopy; \
+#define RETURN_STUB_PARM(xxx_arraycopy, parm) { \
+  name = parm ? #xxx_arraycopy "_uninit": #xxx_arraycopy; \
   return StubRoutines::xxx_arraycopy(parm); }
 
   switch (t) {