# HG changeset patch # User roland # Date 1498203201 -7200 # Node ID 2ab74e5dbdc2b6a962c865500cafd23cf387dc60 # Parent 76a4975620145493edba3361190e842abbb88157 8181742: Load that bypasses arraycopy has wrong memory state Summary: Set load memory edge to the memory state right before the arraycopy. Reviewed-by: kvn, thartmann diff -r 76a497562014 -r 2ab74e5dbdc2 src/share/vm/opto/arraycopynode.cpp --- a/src/share/vm/opto/arraycopynode.cpp Thu Jun 22 18:42:45 2017 +0000 +++ b/src/share/vm/opto/arraycopynode.cpp Fri Jun 23 09:33:21 2017 +0200 @@ -748,41 +748,3 @@ return false; } -// We try to replace a load from the destination of an arraycopy with -// a load from the source so the arraycopy has a chance to be -// eliminated. It's only valid if the arraycopy doesn't change the -// element that would be loaded from the source array. -bool ArrayCopyNode::can_replace_dest_load_with_src_load(intptr_t offset_lo, intptr_t offset_hi, PhaseTransform* phase) const { - assert(_kind == ArrayCopy || _kind == CopyOf || _kind == CopyOfRange, "only for real array copies"); - - Node* src = in(Src); - Node* dest = in(Dest); - - // Check whether, assuming source and destination are the same - // array, the arraycopy modifies the element from the source we - // would load. - if ((src != dest && in(SrcPos) == in(DestPos)) || !modifies(offset_lo, offset_hi, phase, false)) { - // if not the transformation is legal - return true; - } - - AllocateNode* src_alloc = AllocateNode::Ideal_allocation(src, phase); - AllocateNode* dest_alloc = AllocateNode::Ideal_allocation(dest, phase); - - // Check whether source and destination can be proved to be - // different arrays - const TypeOopPtr* t_src = phase->type(src)->isa_oopptr(); - const TypeOopPtr* t_dest = phase->type(dest)->isa_oopptr(); - - if (t_src != NULL && t_dest != NULL && - (t_src->is_known_instance() || t_dest->is_known_instance()) && - t_src->instance_id() != t_dest->instance_id()) { - return true; - } - - if (MemNode::detect_ptr_independence(src->uncast(), src_alloc, dest->uncast(), dest_alloc, phase)) { - return true; - } - - return false; -} diff -r 76a497562014 -r 2ab74e5dbdc2 src/share/vm/opto/arraycopynode.hpp --- a/src/share/vm/opto/arraycopynode.hpp Thu Jun 22 18:42:45 2017 +0000 +++ b/src/share/vm/opto/arraycopynode.hpp Fri Jun 23 09:33:21 2017 +0200 @@ -168,7 +168,6 @@ static bool may_modify(const TypeOopPtr *t_oop, MemBarNode* mb, PhaseTransform *phase, ArrayCopyNode*& ac); bool modifies(intptr_t offset_lo, intptr_t offset_hi, PhaseTransform* phase, bool must_modify) const; - bool can_replace_dest_load_with_src_load(intptr_t offset_lo, intptr_t offset_hi, PhaseTransform* phase) const; #ifndef PRODUCT virtual void dump_spec(outputStream *st) const; diff -r 76a497562014 -r 2ab74e5dbdc2 src/share/vm/opto/library_call.cpp --- a/src/share/vm/opto/library_call.cpp Thu Jun 22 18:42:45 2017 +0000 +++ b/src/share/vm/opto/library_call.cpp Fri Jun 23 09:33:21 2017 +0200 @@ -5171,6 +5171,10 @@ Deoptimization::Action_make_not_entrant); assert(stopped(), "Should be stopped"); } + + const TypeKlassPtr* dest_klass_t = _gvn.type(dest_klass)->is_klassptr(); + const Type *toop = TypeOopPtr::make_from_klass(dest_klass_t->klass()); + src = _gvn.transform(new CheckCastPPNode(control(), src, toop)); } arraycopy_move_allocation_here(alloc, dest, saved_jvms, saved_reexecute_sp, new_idx); diff -r 76a497562014 -r 2ab74e5dbdc2 src/share/vm/opto/memnode.cpp --- a/src/share/vm/opto/memnode.cpp Thu Jun 22 18:42:45 2017 +0000 +++ b/src/share/vm/opto/memnode.cpp Fri Jun 23 09:33:21 2017 +0200 @@ -885,7 +885,7 @@ // Is the value loaded previously stored by an arraycopy? If so return // a load node that reads from the source array so we may be able to // optimize out the ArrayCopy node later. -Node* LoadNode::can_see_arraycopy_value(Node* st, PhaseTransform* phase) const { +Node* LoadNode::can_see_arraycopy_value(Node* st, PhaseGVN* phase) const { Node* ld_adr = in(MemNode::Address); intptr_t ld_off = 0; AllocateNode* ld_alloc = AllocateNode::Ideal_allocation(ld_adr, phase, ld_off); @@ -893,23 +893,27 @@ if (ac != NULL) { assert(ac->is_ArrayCopy(), "what kind of node can this be?"); - Node* ld = clone(); + Node* mem = ac->in(TypeFunc::Memory); + Node* ctl = ac->in(0); + Node* src = ac->in(ArrayCopyNode::Src); + + if (!ac->as_ArrayCopy()->is_clonebasic() && !phase->type(src)->isa_aryptr()) { + return NULL; + } + + LoadNode* ld = clone()->as_Load(); + Node* addp = in(MemNode::Address)->clone(); if (ac->as_ArrayCopy()->is_clonebasic()) { assert(ld_alloc != NULL, "need an alloc"); - Node* addp = in(MemNode::Address)->clone(); assert(addp->is_AddP(), "address must be addp"); assert(addp->in(AddPNode::Base) == ac->in(ArrayCopyNode::Dest)->in(AddPNode::Base), "strange pattern"); assert(addp->in(AddPNode::Address) == ac->in(ArrayCopyNode::Dest)->in(AddPNode::Address), "strange pattern"); - addp->set_req(AddPNode::Base, ac->in(ArrayCopyNode::Src)->in(AddPNode::Base)); - addp->set_req(AddPNode::Address, ac->in(ArrayCopyNode::Src)->in(AddPNode::Address)); - ld->set_req(MemNode::Address, phase->transform(addp)); - if (in(0) != NULL) { - assert(ld_alloc->in(0) != NULL, "alloc must have control"); - ld->set_req(0, ld_alloc->in(0)); - } + addp->set_req(AddPNode::Base, src->in(AddPNode::Base)); + addp->set_req(AddPNode::Address, src->in(AddPNode::Address)); } else { - Node* src = ac->in(ArrayCopyNode::Src); - Node* addp = in(MemNode::Address)->clone(); + assert(ac->as_ArrayCopy()->is_arraycopy_validated() || + ac->as_ArrayCopy()->is_copyof_validated() || + ac->as_ArrayCopy()->is_copyofrange_validated(), "only supported cases"); assert(addp->in(AddPNode::Base) == addp->in(AddPNode::Address), "should be"); addp->set_req(AddPNode::Base, src); addp->set_req(AddPNode::Address, src); @@ -927,21 +931,17 @@ Node* offset = phase->transform(new AddXNode(addp->in(AddPNode::Offset), diff)); addp->set_req(AddPNode::Offset, offset); - ld->set_req(MemNode::Address, phase->transform(addp)); - - const TypeX *ld_offs_t = phase->type(offset)->isa_intptr_t(); - - if (!ac->as_ArrayCopy()->can_replace_dest_load_with_src_load(ld_offs_t->_lo, ld_offs_t->_hi, phase)) { - return NULL; - } - - if (in(0) != NULL) { - assert(ac->in(0) != NULL, "alloc must have control"); - ld->set_req(0, ac->in(0)); - } } + addp = phase->transform(addp); +#ifdef ASSERT + const TypePtr* adr_type = phase->type(addp)->is_ptr(); + ld->_adr_type = adr_type; +#endif + ld->set_req(MemNode::Address, addp); + ld->set_req(0, ctl); + ld->set_req(MemNode::Memory, mem); // load depends on the tests that validate the arraycopy - ld->as_Load()->_control_dependency = Pinned; + ld->_control_dependency = Pinned; return ld; } return NULL; diff -r 76a497562014 -r 2ab74e5dbdc2 src/share/vm/opto/memnode.hpp --- a/src/share/vm/opto/memnode.hpp Thu Jun 22 18:42:45 2017 +0000 +++ b/src/share/vm/opto/memnode.hpp Fri Jun 23 09:33:21 2017 +0200 @@ -270,7 +270,7 @@ const Type* load_array_final_field(const TypeKlassPtr *tkls, ciKlass* klass) const; - Node* can_see_arraycopy_value(Node* st, PhaseTransform* phase) const; + Node* can_see_arraycopy_value(Node* st, PhaseGVN* phase) const; // depends_only_on_test is almost always true, and needs to be almost always // true to enable key hoisting & commoning optimizations. However, for the diff -r 76a497562014 -r 2ab74e5dbdc2 test/compiler/arraycopy/TestLoadBypassACWithWrongMem.java --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/test/compiler/arraycopy/TestLoadBypassACWithWrongMem.java Fri Jun 23 09:33:21 2017 +0200 @@ -0,0 +1,86 @@ +/* + * Copyright (c) 2017, Red Hat, Inc. 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 8181742 + * @summary Loads that bypass arraycopy ends up with wrong memory state + * + * @run main/othervm -XX:-BackgroundCompilation -XX:-UseOnStackReplacement -XX:+UnlockDiagnosticVMOptions -XX:+IgnoreUnrecognizedVMOptions -XX:+StressGCM -XX:+StressLCM TestLoadBypassACWithWrongMem + * + */ + +import java.util.Arrays; + +public class TestLoadBypassACWithWrongMem { + + static int test1(int[] src) { + int[] dst = new int[10]; + System.arraycopy(src, 0, dst, 0, 10); + src[1] = 0x42; + // dst[1] is transformed to src[1], src[1] must use the + // correct memory state (not the store above). + return dst[1]; + } + + static int test2(int[] src) { + int[] dst = (int[])src.clone(); + src[1] = 0x42; + // Same as above for clone + return dst[1]; + } + + static Object test5_src = null; + static int test3() { + int[] dst = new int[10]; + System.arraycopy(test5_src, 0, dst, 0, 10); + ((int[])test5_src)[1] = 0x42; + System.arraycopy(test5_src, 0, dst, 0, 10); + // dst[1] is transformed to test5_src[1]. test5_src is Object + // but test5_src[1] must be on the slice for int[] not + // Object+some offset. + return dst[1]; + } + + static public void main(String[] args) { + int[] src = new int[10]; + for (int i = 0; i < 20000; i++) { + Arrays.fill(src, 0); + int res = test1(src); + if (res != 0) { + throw new RuntimeException("bad result: " + res + " != " + 0); + } + Arrays.fill(src, 0); + res = test2(src); + if (res != 0) { + throw new RuntimeException("bad result: " + res + " != " + 0); + } + Arrays.fill(src, 0); + test5_src = src; + res = test3(); + if (res != 0x42) { + throw new RuntimeException("bad result: " + res + " != " + 0x42); + } + } + } +}