8315082: [REDO] Generational ZGC: Tests crash with assert(index == 0 || is_power_of_2(index))

Co-authored-by: Stefan Karlsson <stefank@openjdk.org>
Co-authored-by: Erik Österlund <eosterlund@openjdk.org>
Reviewed-by: ayang, thartmann, kvn

Author: 3 people

src/hotspot/share/gc/shared/c2/barrierSetC2.cpp

@@ -675,8 +675,15 @@ void BarrierSetC2::clone(GraphKit* kit, Node* src_base, Node* dst_base, Node* si
   Node* payload_size = size;
   Node* offset = kit->MakeConX(base_off);
   payload_size = kit->gvn().transform(new SubXNode(payload_size, offset));
+  if (is_array) {
+    // Ensure the array payload size is rounded up to the next BytesPerLong
+    // multiple when converting to double-words. This is necessary because array
+    // size does not include object alignment padding, so it might not be a
+    // multiple of BytesPerLong for sub-long element types.
+    payload_size = kit->gvn().transform(new AddXNode(payload_size, kit->MakeConX(BytesPerLong - 1)));
+  }
   payload_size = kit->gvn().transform(new URShiftXNode(payload_size, kit->intcon(LogBytesPerLong)));
-  ArrayCopyNode* ac = ArrayCopyNode::make(kit, false, src_base, offset,  dst_base, offset, payload_size, true, false);
+  ArrayCopyNode* ac = ArrayCopyNode::make(kit, false, src_base, offset, dst_base, offset, payload_size, true, false);
   if (is_array) {
     ac->set_clone_array();
   } else {

src/hotspot/share/opto/arraycopynode.cpp

@@ -134,12 +134,13 @@ int ArrayCopyNode::get_count(PhaseGVN *phase) const {
       assert (ary_src != nullptr, "not an array or instance?");
       // clone passes a length as a rounded number of longs. If we're
       // cloning an array we'll do it element by element. If the
-      // length input to ArrayCopyNode is constant, length of input
-      // array must be too.
-
-      assert((get_length_if_constant(phase) == -1) != ary_src->size()->is_con() ||
+      // length of the input array is constant, ArrayCopyNode::Length
+      // must be too. Note that the opposite does not need to hold,
+      // because different input array lengths (e.g. int arrays with
+      // 3 or 4 elements) might lead to the same length input
+      // (e.g. 2 double-words).
+      assert(!ary_src->size()->is_con() || (get_length_if_constant(phase) >= 0) ||
              phase->is_IterGVN() || phase->C->inlining_incrementally() || StressReflectiveCode, "inconsistent");
-
       if (ary_src->size()->is_con()) {
         return ary_src->size()->get_con();
       }

src/hotspot/share/opto/graphKit.cpp

@@ -3771,7 +3771,10 @@ Node* GraphKit::new_instance(Node* klass_node,
 //-------------------------------new_array-------------------------------------
 // helper for both newarray and anewarray
 // The 'length' parameter is (obviously) the length of the array.
-// See comments on new_instance for the meaning of the other arguments.
+// The optional arguments are for specialized use by intrinsics:
+//  - If 'return_size_val', report the non-padded array size (sum of header size
+//    and array body) to the caller.
+//  - deoptimize_on_exception controls how Java exceptions are handled (rethrow vs deoptimize)
 Node* GraphKit::new_array(Node* klass_node,     // array klass (maybe variable)
                           Node* length,         // number of array elements
                           int   nargs,          // number of arguments to push back for uncommon trap
@@ -3822,25 +3825,21 @@ Node* GraphKit::new_array(Node* klass_node,     // array klass (maybe variable)
   // The rounding mask is strength-reduced, if possible.
   int round_mask = MinObjAlignmentInBytes - 1;
   Node* header_size = nullptr;
-  int   header_size_min  = arrayOopDesc::base_offset_in_bytes(T_BYTE);
   // (T_BYTE has the weakest alignment and size restrictions...)
   if (layout_is_con) {
     int       hsize  = Klass::layout_helper_header_size(layout_con);
     int       eshift = Klass::layout_helper_log2_element_size(layout_con);
-    BasicType etype  = Klass::layout_helper_element_type(layout_con);
     if ((round_mask & ~right_n_bits(eshift)) == 0)
       round_mask = 0;  // strength-reduce it if it goes away completely
     assert((hsize & right_n_bits(eshift)) == 0, "hsize is pre-rounded");
+    int header_size_min = arrayOopDesc::base_offset_in_bytes(T_BYTE);
     assert(header_size_min <= hsize, "generic minimum is smallest");
-    header_size_min = hsize;
-    header_size = intcon(hsize + round_mask);
+    header_size = intcon(hsize);
   } else {
     Node* hss   = intcon(Klass::_lh_header_size_shift);
     Node* hsm   = intcon(Klass::_lh_header_size_mask);
-    Node* hsize = _gvn.transform( new URShiftINode(layout_val, hss) );
-    hsize       = _gvn.transform( new AndINode(hsize, hsm) );
-    Node* mask  = intcon(round_mask);
-    header_size = _gvn.transform( new AddINode(hsize, mask) );
+    header_size = _gvn.transform(new URShiftINode(layout_val, hss));
+    header_size = _gvn.transform(new AndINode(header_size, hsm));
   }
 
   Node* elem_shift = nullptr;
@@ -3892,25 +3891,30 @@ Node* GraphKit::new_array(Node* klass_node,     // array klass (maybe variable)
   }
 #endif
 
-  // Combine header size (plus rounding) and body size.  Then round down.
-  // This computation cannot overflow, because it is used only in two
-  // places, one where the length is sharply limited, and the other
-  // after a successful allocation.
+  // Combine header size and body size for the array copy part, then align (if
+  // necessary) for the allocation part. This computation cannot overflow,
+  // because it is used only in two places, one where the length is sharply
+  // limited, and the other after a successful allocation.
   Node* abody = lengthx;
-  if (elem_shift != nullptr)
-    abody     = _gvn.transform( new LShiftXNode(lengthx, elem_shift) );
-  Node* size  = _gvn.transform( new AddXNode(headerx, abody) );
-  if (round_mask != 0) {
-    Node* mask = MakeConX(~round_mask);
-    size       = _gvn.transform( new AndXNode(size, mask) );
+  if (elem_shift != nullptr) {
+    abody = _gvn.transform(new LShiftXNode(lengthx, elem_shift));
   }
-  // else if round_mask == 0, the size computation is self-rounding
+  Node* non_rounded_size = _gvn.transform(new AddXNode(headerx, abody));
 
   if (return_size_val != nullptr) {
     // This is the size
-    (*return_size_val) = size;
+    (*return_size_val) = non_rounded_size;
   }
 
+  Node* size = non_rounded_size;
+  if (round_mask != 0) {
+    Node* mask1 = MakeConX(round_mask);
+    size = _gvn.transform(new AddXNode(size, mask1));
+    Node* mask2 = MakeConX(~round_mask);
+    size = _gvn.transform(new AndXNode(size, mask2));
+  }
+  // else if round_mask == 0, the size computation is self-rounding
+
   // Now generate allocation code
 
   // The entire memory state is needed for slow path of the allocation

src/hotspot/share/opto/library_call.cpp

@@ -5021,8 +5021,8 @@ bool LibraryCallKit::inline_native_clone(bool is_virtual) {
       PreserveJVMState pjvms(this);
       set_control(array_ctl);
       Node* obj_length = load_array_length(obj);
-      Node* obj_size  = nullptr;
-      Node* alloc_obj = new_array(obj_klass, obj_length, 0, &obj_size, /*deoptimize_on_exception=*/true);
+      Node* array_size = nullptr; // Size of the array without object alignment padding.
+      Node* alloc_obj = new_array(obj_klass, obj_length, 0, &array_size, /*deoptimize_on_exception=*/true);
 
       BarrierSetC2* bs = BarrierSet::barrier_set()->barrier_set_c2();
       if (bs->array_copy_requires_gc_barriers(true, T_OBJECT, true, false, BarrierSetC2::Parsing)) {
@@ -5055,7 +5055,7 @@ bool LibraryCallKit::inline_native_clone(bool is_virtual) {
       //  the object.)
 
       if (!stopped()) {
-        copy_to_clone(obj, alloc_obj, obj_size, true);
+        copy_to_clone(obj, alloc_obj, array_size, true);
 
         // Present the results of the copy.
         result_reg->init_req(_array_path, control());
@@ -5095,7 +5095,7 @@ bool LibraryCallKit::inline_native_clone(bool is_virtual) {
     if (!stopped()) {
       // It's an instance, and it passed the slow-path tests.
       PreserveJVMState pjvms(this);
-      Node* obj_size  = nullptr;
+      Node* obj_size = nullptr; // Total object size, including object alignment padding.
       // Need to deoptimize on exception from allocation since Object.clone intrinsic
       // is reexecuted if deoptimization occurs and there could be problems when merging
       // exception state between multiple Object.clone versions (reexecute=true vs reexecute=false).

test/hotspot/jtreg/compiler/arraycopy/TestCloneArrayWithDifferentLengthConstness.java

@@ -0,0 +1,66 @@
+/*
+ * Copyright (c) 2023, 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.
+ */
+
+package compiler.arraycopy;
+
+/**
+ * @test
+ * @bug 8315082
+ * @summary Test that idealization of clone-derived ArrayCopy nodes does not
+ *          trigger assertion failures for different combinations of
+ *          constant/variable array length (in number of elements) and array
+ *          copy length (in words).
+ * @run main/othervm -Xbatch -XX:-TieredCompilation
+ *                   -XX:CompileOnly=compiler.arraycopy.TestCloneArrayWithDifferentLengthConstness::test*
+ *                   compiler.arraycopy.TestCloneArrayWithDifferentLengthConstness
+ */
+
+public class TestCloneArrayWithDifferentLengthConstness {
+
+    public static void main(String[] args) {
+        for (int i = 0; i < 10_000; i++) {
+            testConstantArrayLengthAndConstantArrayCopyLength();
+        }
+        for (int i = 0; i < 10_000; i++) {
+            testVariableArrayLengthAndConstantArrayCopyLength(i % 2 == 0);
+        }
+        for (int i = 0; i < 10_000; i++) {
+            testVariableArrayLengthAndVariableArrayCopyLength(i % 2 == 0);
+        }
+    }
+
+    static int[] testConstantArrayLengthAndConstantArrayCopyLength() {
+        int[] src = new int[3];
+        return (int[])src.clone();
+    }
+
+    static int[] testVariableArrayLengthAndConstantArrayCopyLength(boolean p) {
+        int[] src = new int[p ? 3 : 4];
+        return (int[])src.clone();
+    }
+
+    static int[] testVariableArrayLengthAndVariableArrayCopyLength(boolean p) {
+        int[] src = new int[p ? 3 : 42];
+        return (int[])src.clone();
+    }
+}

test/hotspot/jtreg/compiler/gcbarriers/TestArrayCopyWithLargeObjectAlignment.java

@@ -0,0 +1,69 @@
+/*
+ * Copyright (c) 2023, 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.
+ */
+package compiler.gcbarriers;
+
+import java.util.Arrays;
+
+/**
+ * @test
+ * @bug 8312749
+ * @summary Test that, when using a larger object alignment, ZGC arraycopy
+ *          barriers are only applied to actual OOPs, and not to object
+ *          alignment padding words.
+ * @requires vm.gc.ZGenerational
+ * @run main/othervm -Xbatch -XX:-TieredCompilation
+ *                   -XX:CompileOnly=compiler.gcbarriers.TestArrayCopyWithLargeObjectAlignment::*
+ *                   -XX:ObjectAlignmentInBytes=16
+ *                   -XX:+UseZGC -XX:+ZGenerational
+ *                   compiler.gcbarriers.TestArrayCopyWithLargeObjectAlignment
+ */
+
+public class TestArrayCopyWithLargeObjectAlignment {
+
+    static Object[] doCopyOf(Object[] array) {
+        return Arrays.copyOf(array, array.length);
+    }
+
+    static Object[] doClone(Object[] array) {
+        return array.clone();
+    }
+
+    public static void main(String[] args) {
+        for (int i = 0; i < 10_000; i++) {
+            // This test allocates an array 'a', copies it into a new array 'b'
+            // using Arrays.copyOf, and clones 'b' into yet another array. For
+            // ObjectAlignmentInBytes=16, the intrinsic implementation of
+            // Arrays.copyOf leaves the object alignment padding word "b[1]"
+            // untouched, preserving the badHeapWordVal value '0xbaadbabe'. The
+            // test checks that this padding word is not processed as a valid
+            // OOP by the ZGC arraycopy stub underlying the intrinsic
+            // implementation of Object.clone. Allocating b using the intrinsic
+            // implementation of Arrays.copyOf is key to reproducing the issue
+            // because, unlike regular (fast or slow) array allocation,
+            // Arrays.copyOf does not zero-clear the padding word.
+            Object[] a = {new Object()};
+            Object[] b = doCopyOf(a);
+            doClone(b);
+        }
+    }
+}