8073866: Fix for 8064703 is not sufficient
Summary: side effects between allocation and arraycopy can be reexecuted, unreachable uninitialized array can be seen by GCs
Reviewed-by: kvn, vlivanov
--- a/hotspot/src/share/vm/opto/graphKit.cpp Fri Mar 20 11:53:01 2015 +0100
+++ b/hotspot/src/share/vm/opto/graphKit.cpp Mon Mar 16 12:24:06 2015 +0100
@@ -2795,18 +2795,15 @@
*/
Node* GraphKit::maybe_cast_profiled_obj(Node* obj,
ciKlass* type,
- bool not_null,
- SafePointNode* sfpt) {
+ bool not_null) {
// 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) &&
- !C->too_many_recompiles(trap_method, trap_bci, class_reason)) {
+ !too_many_traps(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 @@
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);
--- a/hotspot/src/share/vm/opto/graphKit.hpp Fri Mar 20 11:53:01 2015 +0100
+++ b/hotspot/src/share/vm/opto/graphKit.hpp Mon Mar 16 12:24:06 2015 +0100
@@ -409,8 +409,7 @@
// 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,
- SafePointNode* sfpt = NULL);
+ bool not_null = false);
// Cast obj to not-null on this path
Node* cast_not_null(Node* obj, bool do_replace_in_map = true);
--- a/hotspot/src/share/vm/opto/library_call.cpp Fri Mar 20 11:53:01 2015 +0100
+++ b/hotspot/src/share/vm/opto/library_call.cpp Mon Mar 16 12:24:06 2015 +0100
@@ -262,6 +262,9 @@
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 @@
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 @@
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 @@
// (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
- // tightly_coupled_allocation()
- AllocateArrayNode* alloc = tightly_coupled_allocation(dest, NULL);
-
- ciMethod* trap_method = method();
- int trap_bci = bci();
- SafePointNode* sfpt = NULL;
- if (alloc != NULL) {
- // The JVM state for uncommon traps between the allocation and
- // 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();
+ if (!can_emit_guards) {
+ // if saved_jvms == NULL and alloc != NULL, we don't emit any
+ // guards but the arraycopy node could still take advantage of a
+ // tightly allocated allocation. tightly_coupled_allocation() is
+ // called again to make sure it takes the null check above into
+ // account: the null check is mandatory and if it caused an
+ // uncommon trap to be emitted then the allocation can't be
+ // considered tightly coupled in this context.
+ alloc = tightly_coupled_allocation(dest, NULL);
}
bool validated = false;
@@ -4753,7 +4882,7 @@
// 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 @@
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 @@
}
}
- 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 @@
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 @@
}
}
- 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 @@
Node* not_subtype_ctrl = gen_subtype_check(src_klass, dest_klass);
if (not_subtype_ctrl != top()) {
- if (sfpt != NULL) {
- GraphKit kit(sfpt->jvms());
- PreserveJVMState pjvms(&kit);
- kit.set_control(not_subtype_ctrl);
- kit.uncommon_trap(Deoptimization::Reason_intrinsic,
- 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");
- }
+ 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 @@
}
}
+ arraycopy_move_allocation_here(alloc, dest, saved_jvms, saved_reexecute_sp);
+
if (stopped()) {
return true;
}
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/hotspot/test/compiler/arraycopy/TestArrayCopyBadReexec.java Mon Mar 16 12:24:06 2015 +0100
@@ -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");
+ }
+ }
+}
--- a/hotspot/test/compiler/arraycopy/TestArrayCopyNoInit.java Fri Mar 20 11:53:01 2015 +0100
+++ b/hotspot/test/compiler/arraycopy/TestArrayCopyNoInit.java Mon Mar 16 12:24:06 2015 +0100
@@ -76,7 +76,7 @@
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 @@
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;
}
--- a/hotspot/test/compiler/arraycopy/TestArrayCopyNoInitDeopt.java Fri Mar 20 11:53:01 2015 +0100
+++ b/hotspot/test/compiler/arraycopy/TestArrayCopyNoInitDeopt.java Mon Mar 16 12:24:06 2015 +0100
@@ -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
*