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
--- a/hotspot/src/share/vm/opto/arraycopynode.cpp Thu Jun 22 18:42:45 2017 +0000
+++ b/hotspot/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;
-}
--- a/hotspot/src/share/vm/opto/arraycopynode.hpp Thu Jun 22 18:42:45 2017 +0000
+++ b/hotspot/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;
--- a/hotspot/src/share/vm/opto/library_call.cpp Thu Jun 22 18:42:45 2017 +0000
+++ b/hotspot/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);
--- a/hotspot/src/share/vm/opto/memnode.cpp Thu Jun 22 18:42:45 2017 +0000
+++ b/hotspot/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;
--- a/hotspot/src/share/vm/opto/memnode.hpp Thu Jun 22 18:42:45 2017 +0000
+++ b/hotspot/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
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/hotspot/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);
+ }
+ }
+ }
+}