Skip to content

Commit

Permalink
8315082: [REDO] Generational ZGC: Tests crash with assert(index == 0 …
Browse files Browse the repository at this point in the history
…|| is_power_of_2(index))

Backport-of: ceff47b462ccbaff5cc16111dc65463a6d8d3d8d
  • Loading branch information
shipilev committed Nov 27, 2023
1 parent afc506e commit f01df29
Show file tree
Hide file tree
Showing 6 changed files with 178 additions and 31 deletions.
9 changes: 8 additions & 1 deletion src/hotspot/share/gc/shared/c2/barrierSetC2.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
11 changes: 6 additions & 5 deletions src/hotspot/share/opto/arraycopynode.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand Down
46 changes: 25 additions & 21 deletions src/hotspot/share/opto/graphKit.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down
8 changes: 4 additions & 4 deletions src/hotspot/share/opto/library_call.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4992,8 +4992,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)) {
Expand Down Expand Up @@ -5026,7 +5026,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());
Expand Down Expand Up @@ -5066,7 +5066,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).
Expand Down
Original file line number Diff line number Diff line change
@@ -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();
}
}
Original file line number Diff line number Diff line change
@@ -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);
}
}
}

1 comment on commit f01df29

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign in to comment.