stan/jdk-24
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

8181742: Load that bypasses arraycopy has wrong memory state

Browse Source 
Set load memory edge to the memory state right before the arraycopy.

Reviewed-by: kvn, thartmann
This commit is contained in:
Roland Westrelin 2017-06-23 09:33:21 +02:00
parent f3ee3de2a3
commit 10d189d284
6 changed files with 116 additions and 65 deletions

38
hotspot/src/share/vm/opto/arraycopynode.cpp

@ -748,41 +748,3 @@ bool ArrayCopyNode::modifies(intptr_t offset_lo, intptr_t offset_hi, PhaseTransf
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;
}

1
hotspot/src/share/vm/opto/arraycopynode.hpp

@ -168,7 +168,6 @@ public:
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;

4
hotspot/src/share/vm/opto/library_call.cpp

@ -5171,6 +5171,10 @@ bool LibraryCallKit::inline_arraycopy() {
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);

50
hotspot/src/share/vm/opto/memnode.cpp

@ -885,7 +885,7 @@ static bool skip_through_membars(Compile::AliasType* atp, const TypeInstPtr* tp,
// 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 @@ Node* LoadNode::can_see_arraycopy_value(Node* st, PhaseTransform* phase) const {
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* LoadNode::can_see_arraycopy_value(Node* st, PhaseTransform* phase) const {
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;

2
hotspot/src/share/vm/opto/memnode.hpp

@ -270,7 +270,7 @@ protected:
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

86
hotspot/test/compiler/arraycopy/TestLoadBypassACWithWrongMem.java Normal file

@ -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);
}
}
}
}