8159611: C2: ArrayCopy elimination skips required parameter checks
Reviewed-by: kvn, zmajo, thartmann
--- a/hotspot/src/cpu/x86/vm/c1_LIRAssembler_x86.cpp Tue Nov 01 09:19:14 2016 +0100
+++ b/hotspot/src/cpu/x86/vm/c1_LIRAssembler_x86.cpp Thu Oct 06 18:51:24 2016 +0200
@@ -3187,7 +3187,6 @@
if (flags & LIR_OpArrayCopy::length_positive_check) {
__ testl(length, length);
__ jcc(Assembler::less, *stub->entry());
- __ jcc(Assembler::zero, *stub->continuation());
}
#ifdef _LP64
--- a/hotspot/src/share/vm/opto/arraycopynode.cpp Tue Nov 01 09:19:14 2016 +0100
+++ b/hotspot/src/share/vm/opto/arraycopynode.cpp Thu Oct 06 18:51:24 2016 +0200
@@ -26,9 +26,10 @@
#include "opto/arraycopynode.hpp"
#include "opto/graphKit.hpp"
-ArrayCopyNode::ArrayCopyNode(Compile* C, bool alloc_tightly_coupled)
+ArrayCopyNode::ArrayCopyNode(Compile* C, bool alloc_tightly_coupled, bool has_negative_length_guard)
: CallNode(arraycopy_type(), NULL, TypeRawPtr::BOTTOM),
_alloc_tightly_coupled(alloc_tightly_coupled),
+ _has_negative_length_guard(has_negative_length_guard),
_kind(None),
_arguments_validated(false),
_src_type(TypeOopPtr::BOTTOM),
@@ -45,10 +46,11 @@
Node* dest, Node* dest_offset,
Node* length,
bool alloc_tightly_coupled,
+ bool has_negative_length_guard,
Node* src_klass, Node* dest_klass,
Node* src_length, Node* dest_length) {
- ArrayCopyNode* ac = new ArrayCopyNode(kit->C, alloc_tightly_coupled);
+ ArrayCopyNode* ac = new ArrayCopyNode(kit->C, alloc_tightly_coupled, has_negative_length_guard);
Node* prev_mem = kit->set_predefined_input_for_runtime_call(ac);
ac->init_req(ArrayCopyNode::Src, src);
--- a/hotspot/src/share/vm/opto/arraycopynode.hpp Tue Nov 01 09:19:14 2016 +0100
+++ b/hotspot/src/share/vm/opto/arraycopynode.hpp Thu Oct 06 18:51:24 2016 +0200
@@ -58,6 +58,7 @@
// the arraycopy is not parsed yet so doesn't exist when
// LibraryCallKit::tightly_coupled_allocation() is called.
bool _alloc_tightly_coupled;
+ bool _has_negative_length_guard;
bool _arguments_validated;
@@ -82,7 +83,7 @@
return TypeFunc::make(domain, range);
}
- ArrayCopyNode(Compile* C, bool alloc_tightly_coupled);
+ ArrayCopyNode(Compile* C, bool alloc_tightly_coupled, bool has_negative_length_guard);
intptr_t get_length_if_constant(PhaseGVN *phase) const;
int get_count(PhaseGVN *phase) const;
@@ -133,6 +134,7 @@
Node* dest, Node* dest_offset,
Node* length,
bool alloc_tightly_coupled,
+ bool has_negative_length_guard,
Node* src_klass = NULL, Node* dest_klass = NULL,
Node* src_length = NULL, Node* dest_length = NULL);
@@ -162,6 +164,8 @@
bool is_alloc_tightly_coupled() const { return _alloc_tightly_coupled; }
+ bool has_negative_length_guard() const { return _has_negative_length_guard; }
+
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);
--- a/hotspot/src/share/vm/opto/library_call.cpp Tue Nov 01 09:19:14 2016 +0100
+++ b/hotspot/src/share/vm/opto/library_call.cpp Thu Oct 06 18:51:24 2016 +0200
@@ -4046,7 +4046,7 @@
if (!stopped()) {
newcopy = new_array(klass_node, length, 0); // no arguments to push
- ArrayCopyNode* ac = ArrayCopyNode::make(this, true, original, start, newcopy, intcon(0), moved, true,
+ ArrayCopyNode* ac = ArrayCopyNode::make(this, true, original, start, newcopy, intcon(0), moved, true, false,
load_object_klass(original), klass_node);
if (!is_copyOfRange) {
ac->set_copyof(validated);
@@ -4566,7 +4566,7 @@
const TypePtr* raw_adr_type = TypeRawPtr::BOTTOM;
- ArrayCopyNode* ac = ArrayCopyNode::make(this, false, src, NULL, dest, NULL, countx, false);
+ ArrayCopyNode* ac = ArrayCopyNode::make(this, false, src, NULL, dest, NULL, countx, false, false);
ac->set_clonebasic();
Node* n = _gvn.transform(ac);
if (n == ac) {
@@ -4703,7 +4703,7 @@
set_control(is_obja);
// Generate a direct call to the right arraycopy function(s).
Node* alloc = tightly_coupled_allocation(alloc_obj, NULL);
- ArrayCopyNode* ac = ArrayCopyNode::make(this, true, obj, intcon(0), alloc_obj, intcon(0), obj_length, alloc != NULL);
+ ArrayCopyNode* ac = ArrayCopyNode::make(this, true, obj, intcon(0), alloc_obj, intcon(0), obj_length, alloc != NULL, false);
ac->set_cloneoop();
Node* n = _gvn.transform(ac);
assert(n == ac, "cannot disappear");
@@ -5100,6 +5100,8 @@
trap_bci = alloc->jvms()->bci();
}
+ bool negative_length_guard_generated = false;
+
if (!C->too_many_traps(trap_method, trap_bci, Deoptimization::Reason_intrinsic) &&
can_emit_guards &&
!src->is_top() && !dest->is_top()) {
@@ -5132,6 +5134,15 @@
load_array_length(dest),
slow_region);
+ // (6) length must not be negative.
+ // This is also checked in generate_arraycopy() during macro expansion, but
+ // we also have to check it here for the case where the ArrayCopyNode will
+ // be eliminated by Escape Analysis.
+ if (EliminateAllocations) {
+ generate_negative_guard(length, slow_region);
+ negative_length_guard_generated = true;
+ }
+
// (9) each element of an oop array must be assignable
Node* src_klass = load_object_klass(src);
Node* dest_klass = load_object_klass(dest);
@@ -5159,7 +5170,7 @@
return true;
}
- ArrayCopyNode* ac = ArrayCopyNode::make(this, true, src, src_offset, dest, dest_offset, length, alloc != NULL,
+ ArrayCopyNode* ac = ArrayCopyNode::make(this, true, src, src_offset, dest, dest_offset, length, alloc != NULL, negative_length_guard_generated,
// Create LoadRange and LoadKlass nodes for use during macro expansion here
// so the compiler has a chance to eliminate them: during macro expansion,
// we have to set their control (CastPP nodes are eliminated).
--- a/hotspot/src/share/vm/opto/macroArrayCopy.cpp Tue Nov 01 09:19:14 2016 +0100
+++ b/hotspot/src/share/vm/opto/macroArrayCopy.cpp Thu Oct 06 18:51:24 2016 +0200
@@ -1154,7 +1154,10 @@
// Call StubRoutines::generic_arraycopy stub.
Node* mem = generate_arraycopy(ac, NULL, &ctrl, merge_mem, &io,
TypeRawPtr::BOTTOM, T_CONFLICT,
- src, src_offset, dest, dest_offset, length);
+ src, src_offset, dest, dest_offset, length,
+ // If a negative length guard was generated for the ArrayCopyNode,
+ // the length of the array can never be negative.
+ false, ac->has_negative_length_guard());
// Do not let reads from the destination float above the arraycopy.
// Since we cannot type the arrays, we don't know which slices
@@ -1258,5 +1261,7 @@
generate_arraycopy(ac, alloc, &ctrl, merge_mem, &io,
adr_type, dest_elem,
src, src_offset, dest, dest_offset, length,
- false, false, slow_region);
+ // If a negative length guard was generated for the ArrayCopyNode,
+ // the length of the array can never be negative.
+ false, ac->has_negative_length_guard(), slow_region);
}
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/hotspot/test/compiler/escapeAnalysis/TestArrayCopy.java Thu Oct 06 18:51:24 2016 +0200
@@ -0,0 +1,115 @@
+/*
+ * Copyright (c) 2016, SAP SE 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 8159611
+ * @summary The elimination of System.arraycopy by EscapeAnalysis prevents
+ * an IndexOutOfBoundsException from being thrown if the arraycopy
+ * is called with a negative length argument.
+ * @modules java.base/jdk.internal.misc
+ * @library /testlibrary /test/lib
+ * @build sun.hotspot.WhiteBox
+ * @run driver ClassFileInstaller sun.hotspot.WhiteBox
+ * sun.hotspot.WhiteBox$WhiteBoxPermission
+ *
+ * @run main/othervm
+ * -Xbootclasspath/a:.
+ * -XX:+UnlockDiagnosticVMOptions
+ * -XX:+WhiteBoxAPI
+ * -XX:-UseOnStackReplacement
+ * compiler.escapeAnalysis.TestArrayCopy
+ *
+ * @author Volker Simonis
+ */
+
+package compiler.escapeAnalysis;
+
+import sun.hotspot.WhiteBox;
+import java.lang.reflect.Method;
+
+public class TestArrayCopy {
+
+ private static final WhiteBox WB = WhiteBox.getWhiteBox();
+ // DST_LEN Must be const, otherwise EliminateAllocations won't work
+ static final int DST_LEN = 4;
+ static final int SRC_LEN = 8;
+
+ public static boolean do_test1(Object src, int src_pos, int dst_pos, int cpy_len) {
+ try {
+ System.arraycopy(src, src_pos, new Object[DST_LEN], dst_pos, cpy_len);
+ return false;
+ } catch (IndexOutOfBoundsException e) {
+ return true;
+ }
+ }
+
+ public static int do_test2(Object src, int src_pos, int dst_pos, int cpy_len) {
+ try {
+ System.arraycopy(src, src_pos, new Object[DST_LEN], dst_pos, cpy_len);
+ return 0;
+ } catch (IndexOutOfBoundsException e) {
+ return 1;
+ } catch (ArrayStoreException e) {
+ return 2;
+ }
+ }
+
+ static final int COUNT = 100_000;
+ static final int[] src_pos = { 0, -1, -1, 0, 0, 0, 1, 1, 1, 1, 1 };
+ static final int[] dst_pos = { 0, -1, 0, -1, 0, 1, 0, 1, 1, 1, 1 };
+ static final int[] cpy_len = { 0, 0, 0, 0, -1, -1, -1, -1, 8, 4, 2 };
+
+ public static void main(String args[]) throws Exception {
+ int length = args.length > 0 ? Integer.parseInt(args[0]) : -1;
+ int[] int_arr = new int[SRC_LEN];
+ Object[] obj_arr = new Object[SRC_LEN];
+
+ Method test1 = TestArrayCopy.class.getMethod("do_test1", Object.class, int.class, int.class, int.class);
+ Method test2 = TestArrayCopy.class.getMethod("do_test2", Object.class, int.class, int.class, int.class);
+
+ for (int i = 0; i < src_pos.length; i++) {
+ int sp = src_pos[i];
+ int dp = dst_pos[i];
+ int cl = cpy_len[i];
+ String version1 = String.format("System.arraycopy(Object[8], %d, new Object[%d], %d, %d)", sp, DST_LEN, dp, cl);
+ String version2 = String.format("System.arraycopy(int[8], %d, new Object[%d], %d, %d)", sp, DST_LEN, dp, cl);
+ System.out.format("Testing " + version1 + "\nand " + version2).flush();
+ for (int x = 0; x < COUNT; x++) {
+ if (!do_test1(obj_arr, sp, dp, cl) &&
+ (sp < 0 || dp < 0 || cl < 0 || (sp + cl >= SRC_LEN) || (dp + cl >= DST_LEN))) {
+ throw new RuntimeException("Expected IndexOutOfBoundsException for " + version1);
+ }
+ int res = do_test2(int_arr, sp, dp, cl);
+ if (res == 0 || res == 1) {
+ throw new RuntimeException("Expected ArrayStoreException for " + version2);
+ }
+ }
+ WB.deoptimizeMethod(test1);
+ WB.clearMethodState(test1);
+ WB.deoptimizeMethod(test2);
+ WB.clearMethodState(test2);
+ }
+
+ }
+}