--- /dev/null 2015-02-18 19:04:22.401028410 +0100 +++ new/test/compiler/arraycopy/TestArrayCopyBadReexec.java 2015-03-12 16:59:43.329238582 +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"); + } + } +} --- old/test/compiler/arraycopy/TestArrayCopyNoInitDeopt.java 2015-03-12 16:59:43.526279020 +0100 +++ new/test/compiler/arraycopy/TestArrayCopyNoInitDeopt.java 2015-03-12 16:59:43.348990662 +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 * --- old/test/compiler/arraycopy/TestArrayCopyNoInit.java 2015-03-12 16:59:43.580166671 +0100 +++ new/test/compiler/arraycopy/TestArrayCopyNoInit.java 2015-03-12 16:59:43.366911197 +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; } --- old/src/share/vm/opto/graphKit.cpp 2015-03-12 16:59:43.660367671 +0100 +++ new/src/share/vm/opto/graphKit.cpp 2015-03-12 16:59:43.428332516 +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); --- old/src/share/vm/opto/graphKit.hpp 2015-03-12 16:59:43.685680009 +0100 +++ new/src/share/vm/opto/graphKit.hpp 2015-03-12 16:59:43.401064100 +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); --- old/src/share/vm/opto/library_call.cpp 2015-03-12 16:59:43.722227250 +0100 +++ new/src/share/vm/opto/library_call.cpp 2015-03-12 16:59:43.473684600 +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,140 @@ 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) { + // 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 +4823,14 @@ 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); + // 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 +4844,15 @@ // (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 (saved_jvms == NULL && alloc != NULL) { + // We're not emitting the guards, see if we have a tightly + // allocation now that we've done the null check + alloc = tightly_coupled_allocation(dest, NULL); } bool validated = false; @@ -4753,7 +4871,7 @@ // Is the type for dest from speculation? bool dest_spec = false; - if (!has_src || !has_dest) { + if ((!has_src || !has_dest) && (alloc == NULL || saved_jvms != NULL)) { // 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 +4900,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 +4916,7 @@ } } - if (has_src && has_dest) { + if (has_src && has_dest && (alloc == NULL || saved_jvms != NULL)) { 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 +4948,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 +4957,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) && + (alloc == NULL || saved_jvms != NULL) && + !src->is_top() && !dest->is_top()) { // validate arguments: enables transformation the ArrayCopyNode validated = true; @@ -4875,28 +5002,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 +5017,8 @@ } } + arraycopy_move_allocation_here(alloc, dest, saved_jvms, saved_reexecute_sp); + if (stopped()) { return true; }