From 002b59487094f98d9805997b5d1122c1a411b391 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roberto=20Casta=C3=B1eda=20Lozano?= Date: Fri, 25 Aug 2023 07:18:34 +0000 Subject: [PATCH] 8312749: Generational ZGC: Tests crash with assert(index == 0 || is_power_of_2(index)) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Stefan Karlsson Co-authored-by: Erik Österlund Reviewed-by: thartmann, ayang, kvn --- .../share/gc/shared/c2/barrierSetC2.cpp | 9 ++- src/hotspot/share/opto/graphKit.cpp | 46 +++++++------ src/hotspot/share/opto/library_call.cpp | 8 +-- ...TestArrayCopyWithLargeObjectAlignment.java | 69 +++++++++++++++++++ 4 files changed, 106 insertions(+), 26 deletions(-) create mode 100644 test/hotspot/jtreg/compiler/gcbarriers/TestArrayCopyWithLargeObjectAlignment.java diff --git a/src/hotspot/share/gc/shared/c2/barrierSetC2.cpp b/src/hotspot/share/gc/shared/c2/barrierSetC2.cpp index 90fae8c3588..41bd15ab000 100644 --- a/src/hotspot/share/gc/shared/c2/barrierSetC2.cpp +++ b/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 { diff --git a/src/hotspot/share/opto/graphKit.cpp b/src/hotspot/share/opto/graphKit.cpp index d9ea82c03c1..e3fe25c925b 100644 --- a/src/hotspot/share/opto/graphKit.cpp +++ b/src/hotspot/share/opto/graphKit.cpp @@ -3728,7 +3728,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 @@ -3779,25 +3782,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; @@ -3849,25 +3848,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 diff --git a/src/hotspot/share/opto/library_call.cpp b/src/hotspot/share/opto/library_call.cpp index dec7f30d070..587c3d84f79 100644 --- a/src/hotspot/share/opto/library_call.cpp +++ b/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). diff --git a/test/hotspot/jtreg/compiler/gcbarriers/TestArrayCopyWithLargeObjectAlignment.java b/test/hotspot/jtreg/compiler/gcbarriers/TestArrayCopyWithLargeObjectAlignment.java new file mode 100644 index 00000000000..dd2d485fb76 --- /dev/null +++ b/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); + } + } +}